Code review comment for lp:~salvatore-orlando/neutron/quantum-api-alignment

On Thu, Aug 25, 2011 at 2:38 AM, Salvatore Orlando <
<email address hidden>> wrote:

> > Hi salvatore, thanks for checking with me about this branch. My main
> concern
> > before was just regarding the issue of state/status, which we've cleared
> up.
> > Below are some minor comments, but overall things look great and I am
> > switching my review to Approve.
> >
> > quantum/api/api_common.py
> >
> > I have a general question about the API validation performed here, based
> on a
> > problem I saw a while back. The code uses appears to use syntax like "if
> not
> > param_value and param['required']:" to determine if a parameter exists.
> This
> > can be dangerous, as the above expression will evaluate to false if the
> > parameter value is zero, or an empty dictionary, a boolean false, etc.
> which
> > may in fact be valid values. It seems like really the code should either
> be
> > checking to see if the key exists in the params dictionary, or use an
> explicit
> > check for None (e.g., if param_value is None).
>
> This is a widely used 'pattern' in python programming.

Hmm... the python experts I know (I am certainly not one myself) all seem to
recommend against using this approach to check for None, exactly due to the
ambiguity we're talking about. PEP-8 states:

'Also, beware of writing "if x" when you really mean "if x is not None" --
e.g. when testing whether a variable or argument that defaults to None was
set to some other value. The other value might have a type (such as a
container) that could be false in a boolean context!'

It makes me feel a Pythonista although the C++/C#/Java guy which is in me
> would probably write:
>
> param_value is None and param['required']
>
> possibly adding a bunch of parenthesis as well :)
> On this specific issue, given the way our deserializer currently works,
> param_value can only be a string.

Is that true? I was under the impression that a value could also be a
nested array or dictionary, in which case an empty array or dictionary would
be interpreted as an invalid value, even if it was valid. If that is the
case, this could impact anyone writing an extension with nested
arrays/dictionary.

> As the deserializer can change in the future, or there might be cases in
> which an empty string is a valid parameter value, it makes sense to file a
> bug.
>
> >
> > quantum/api/faults.py
> >
> > Is this still left in as a TODO?
> >
> > + #MUST RETURN 202???
>
> I think we discussed in an email thread or earlier meeting this issue. From
> the spec the API should return 202, but the implementation returns 200. It
> turns out the Openstack API has this problem as well. We 'inherited' it by
> grabbing their wsgi framework. I was planning to file a bug and have this
> problem resolved before integration freeze. It is not trivial and I did not
> want to halt progress on the API work for this issue.
>
> >
> > quantum/db/api.py,
> >
> > definitely feel free to remove the check_duplicate_net_name method, as
> your
> > comment suggests.
>
> I'll file a bug for this; low priority should be enough for this work item.
>
> >
> > quantum/tools/batch_config.py
> >
> > Great that you updated the cli.py and tests. Could you also update
> > batch_config.py? Looks like the name change from 'net-name' at least is
> an
> > issue, though perhaps there are others.
> >
>
> I've updated them, but the tests are not yet there. I have a rather large
> merge coming through today with tests and some CLI changes (templated output
> and removal of the direct plugin mode).
>
> I hope to find some time to fix batch_config as well, but I also need to
> fix XML deserialization on the client side, so I might run short on time!
>
> >
> > danwent@ubuntu:~/quantum-api-alignment$ PYTHONPATH=. python
> > ./tools/batch_config.py foo net1=foo:net2=bar
> > nets: {'net2': ['bar'], 'net1': ['foo']}
> > Traceback (most recent call last):
> > File "./tools/batch_config.py", line 111, in <module>
> > create_net_with_attachments(client, net_name, iface_ids)
> > File "./tools/batch_config.py", line 46, in create_net_with_attachments
> > res = client.create_network(data)
> > File "/home/danwent/quantum-api-alignment/quantum/client.py", line 53,
> in
> > with_params
> > ret = self.function(instance, *args)
> > File "/home/danwent/quantum-api-alignment/quantum/client.py", line 240,
> in
> > create_network
> > return self.do_request("POST", self.networks_path, body=body)
> > File "/home/danwent/quantum-api-alignment/quantum/client.py", line 174,
> in
> > do_request
> > raise EXCEPTIONS[res.status]()
> > quantum.common.exceptions.BadInputError
> --
>
> https://code.launchpad.net/~salvatore-orlando/quantum/quantum-api-alignment/+merge/70788
> You are reviewing the proposed merge of
> lp:~salvatore-orlando/quantum/quantum-api-alignment into lp:quantum.
>

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira Networks, Inc.
www.nicira.com | www.openvswitch.org
Sr. Product Manager
cell: 650-906-2650
~~~~~~~~~~~~~~~~~~~~~~~~~~~