ovh / python-ovh

Thin wrapper around OVH's APIs. Handles all the hard work including credential creation and requests signing.
https://pypi.org/project/ovh/
Other
278 stars 76 forks source link

Exclude args with None value from query string #46

Closed antoineco closed 7 years ago

antoineco commented 7 years ago

The following example will fail if region is None, because eventually { 'foo': None } will be returned as 'foo=None' by urlencode() and 'None' is not a valid value:

params = { 'region': region }                                                                                                            
flavors = client.get('/cloud/project/{}/flavor'.format(project), **params)

throws: ovh.exceptions.BadParametersError: Invalid region parameter

It sounds safe to me to exclude None values in the client._prepare_query_string() method because I doubt it's ever intentional to have a NoneType converted to the 'None' string in this context.


Besides we might end up with an empty query_string if the original kwargs contained only None values, so I included a condition for that in a second commit to avoid the following exception:

Traceback (most recent call last):
(...)
  File "/lib/python3.6/site-packages/ovh/client.py", line 347, in get
    return self.call('GET', _target, None, _need_auth)
  File "/lib/python3.6/site-packages/ovh/client.py", line 446, in call
    response=result)
ovh.exceptions.BadParametersError: Invalid signature

_target was /cloud/project/<projectid>/flavor?

rbeuque74 commented 7 years ago

Converting to ?region=null would make more sense to you ? For me, if you have region: None in your parameters means you want explicitely to send it to the API. As we can't assume that the default value for a parameter is equal to null (if you don't send it), it could lead to some misunderstanding. Don't you think ?

antoineco commented 7 years ago

@Rbeuque74 I think I'd rather send nothing than the 'null' string, but it depends how the API handles it. Do you know how key=null differs from not sending anything?

My understanding is that if you explicitly want to send the 'null' string you should do it on your side, but if your parameter's value is None (I'm talking about an actual NoneType) you'd better ignore it. You said it yourself, you don't know if the default for the parameter is ' null', so if you send nothing you actually respect the default.

yadutaf commented 7 years ago

The correct way of fixing this issue is to send null rather than None. As @Rbeuque74 said, the API engine has no way to differentiate a "is null" filter from a "ignore" filter if not explicitly told. This is how the reference implementation works https://api.ovh.com/console/#/cloud/project#GET

Hence, there are 2 fixes here:

antoineco commented 7 years ago

👍 encoding the param with a null value sounds good to me, thanks for the commit. I'm still unsure how /endpoint?region=null differs from /endpoint but at least it's explicit.

btw, the coveralls step fails in Travis.