Repository: proxmoxer/proxmoxer

Issue:
Implicitly remove None values from params
When calling proxmoxer API methods with `None` values, it will behave differently when using https backend vs others.

The difference comes from requests library, which will [implicitly remove `None` values from parameters](https://github.com/psf/requests/blob/1e62a3ec18e19f85ddae03b4cbbdf0b4c62834c0/requests/sessions.py#L82-L86).
On other backends the `None` values are implicitly converted to string, and providing them to the pvesh command.

A stupid, but simple example of this would be:
```python
proxmox.storage.get(type=None) # Will fail, unless https backend is used
```

Would you guys accept a PR removing `None` values just as the requests module does?

This issue surfaced from a [ansible.community issue](https://github.com/ansible-collections/community.general/issues/5449#issuecomment-1304649767) about implementing local backend.

Discussion:
jhollowe: Yeah, separating the data integrity checks into a separate PR would be good.
kristianheljas: Alright, this PR is formatted now :)

I prematurely made second PR as well before noticing the pre-commit comment, gonna fix that in a jiffy.

Patch:
diff --git a/proxmoxer/core.py b/proxmoxer/core.py
index bb91883..fabb6a7 100644
--- a/proxmoxer/core.py
+++ b/proxmoxer/core.py
@@ -93,7 +93,7 @@ class ProxmoxResource(object):
         return urlparse.urlunsplit([scheme, netloc, path, query, fragment])
 
     def __call__(self, resource_id=None):
-        if resource_id in (None, ''):
+        if resource_id in (None, ""):
             return self
 
         if isinstance(resource_id, basestring):
@@ -113,6 +113,21 @@ class ProxmoxResource(object):
             logger.info("%s %s %r", method, url, data)
         else:
             logger.info("%s %s", method, url)
+
+        # passing None values to pvesh command breaks it, let's remove them just as requests library does
+        # helpful when dealing with function default values higher in the chain, no need to clean up in multiple places
+        if params:
+            # remove keys that are set to None
+            params_none_keys = [k for (k, v) in params.items() if v is None]
+            for key in params_none_keys:
+                del params[key]
+
+        if data:
+            # remove keys that are set to None
+            data_none_keys = [k for (k, v) in data.items() if v is None]
+            for key in data_none_keys:
+                del data[key]
+
         resp = self._store["session"].request(method, url, data=data, params=params)
         logger.debug("Status code: %s, output: %s", resp.status_code, resp.content)
 


Test patch:
diff --git a/tests/test_core.py b/tests/test_core.py
index 109a2cc..b864643 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -196,6 +196,16 @@ class TestProxmoxResource:
         assert exc_info.value.content == "this is the reason"
         assert exc_info.value.errors == {"errors": b"this is the error"}
 
+    def test_request_params_cleanup(self, mock_resource):
+        mock_resource._request("GET", params={"key": "value", "remove_me": None})
+
+        assert mock_resource._store["session"].params == {"key": "value"}
+
+    def test_request_data_cleanup(self, mock_resource):
+        mock_resource._request("POST", data={"key": "value", "remove_me": None})
+
+        assert mock_resource._store["session"].data == {"key": "value"}
+
 
 class TestProxmoxResourceMethods:
     _resource = core.ProxmoxResource(base_url="https://example.com")