Merge lp:~wallyworld/lazr.restful/dict-parameters into lp:lazr.restful

[Ian Booth]

Add support for marshalling exported method parameters of type dict.

== Implementation ==

Add the zcml registration and implementation class for a dictionary marshaller: DictFieldMarshaller.

Also fixed some drive-by lint issues.

== Tests ==

Add new tests to the webservice-marshallers.txt doc test.

== Lint ==

Linting changed files:
  src/lazr/restful/NEWS.txt
  src/lazr/restful/configure.zcml
  src/lazr/restful/marshallers.py
  src/lazr/restful/version.txt
  src/lazr/restful/docs/webservice-marshallers.txt

./src/lazr/restful/marshallers.py
     157: E261 at least two spaces before inline comment

[lp:~wallyworld/lazr.restful/dict-parameters]

201. By Ian Booth on 2012-03-13

Fix unmarshalling implementation

202. By Ian Booth on 2012-03-13

Remove debugging code

203. By Ian Booth on 2012-03-13

Add support for marshalling single element tuples as a string

[Francis J. Lacoste (flacoste)]

Hi Ian,

Thank you very much for this addition to lazr.restful!

I'm approving this on the premise that you'll address my main comment.

[1]

255 + def __init__(self, field, request):
256 + """See `SimpleFieldMarshaller`.
257 +
258 + This also looks for the appropriate marshaller for key_type and
259 + value_type.
260 + """
261 + super(DictFieldMarshaller, self).__init__(
262 + field, request)
263 + self.key_marshaller = getMultiAdapter(
264 + (field.key_type, request), IFieldMarshaller)
265 + self.value_marshaller = getMultiAdapter(
266j+ (field.value_type, request), IFieldMarshaller)

I think we should probably raise an error if key_type or value_type aren't defined?

[2]

307 + @property
308 + def _python_collection_factory(self):
309 + """Create the appropriate python collection from a dict."""
310 + # For the DictMarshaller, _type contains the type object,
311 + # which can be used as a factory.
312 + return self.field._type

Why this indirection? Do you really expect users to subclass the marshaller to
override the default? YAGNI

[3]

300 + value = super(
301 + DictFieldMarshaller, self)._marshall_from_json_data(value)

Why is this needed?

value is already a dict, what do you expect the superclass to handle? Ah,
I think it's error handling? Maybe add a comment about that? Which actually
points to the fact that you are missing a test for this. You test that ValueError
are raised if the key or value are of the improper type, but not if the request/json is
not actually a dict (or of the proper representation).

Thanks for the drive-by formatting fixes.

Cheers

review: Approve

[Ian Booth (wallyworld)]

Hi

Thanks for the review

>
> [1]
>
> 255 + def __init__(self, field, request):
> 256 + """See `SimpleFieldMarshaller`.
> 257 +
> 258 + This also looks for the appropriate marshaller for key_type and
> 259 + value_type.
> 260 + """
> 261 + super(DictFieldMarshaller, self).__init__(
> 262 + field, request)
> 263 + self.key_marshaller = getMultiAdapter(
> 264 + (field.key_type, request), IFieldMarshaller)
> 265 + self.value_marshaller = getMultiAdapter(
> 266j+ (field.value_type, request), IFieldMarshaller)
>
> I think we should probably raise an error if key_type or value_type aren't defined?
>

There's no need - the default adaptor lookup will simply return a
SimpleFieldMarshaller. The AbstractCollectionFieldMarshaller which is
used for List and Set also works the same way.

>
> [2]
>
> 307 + @property
> 308 + def _python_collection_factory(self):
> 309 + """Create the appropriate python collection from a dict."""
> 310 + # For the DictMarshaller, _type contains the type object,
> 311 + # which can be used as a factory.
> 312 + return self.field._type
>
> Why this indirection? Do you really expect users to subclass the marshaller to
> override the default? YAGNI
>

I based the code on what AbstractCollectionFieldMarshaller does, but in
this case we know it's a dict so I agree YAGNI, will fix.

> [3]
>
> 300 + value = super(
> 301 + DictFieldMarshaller, self)._marshall_from_json_data(value)
>
> Why is this needed?
>
> value is already a dict, what do you expect the superclass to handle? Ah,
> I think it's error handling? Maybe add a comment about that? Which actually
> points to the fact that you are missing a test for this. You test that ValueError
> are raised if the key or value are of the improper type, but not if the request/json is
> not actually a dict (or of the proper representation).
>

Yes, it's for error handling. I'll add a comment and yes, I did miss out
adding a test for this (I meant to). Thanks for picking that up.

[Ian Booth (wallyworld)]

Argh, poor wording.

>
> Yes, it's for error handling. I'll add a comment and yes, I did miss out
> adding a test for this (I meant to). Thanks for picking that up.
>

I mean to say that I had intended to add a test but somehow didn't.

[lp:~wallyworld/lazr.restful/dict-parameters]

204. By Ian Booth on 2012-03-13

Add better error reporting and update test

=== modified file '.bzrignore'
--- .bzrignore	2011-11-18 22:23:44 +0000
+++ .bzrignore	2012-03-13 23:50:24 +0000
@@ -12,3 +12,4 @@
 *.egg
 dist
 .DS_Store
+MANIFEST
 
=== modified file 'src/lazr/restful/NEWS.txt'
--- src/lazr/restful/NEWS.txt	2011-10-11 01:45:29 +0000
+++ src/lazr/restful/NEWS.txt	2012-03-13 23:50:24 +0000
@@ -2,6 +2,12 @@
 NEWS for lazr.restful
 =====================
 
+0.19.5 (2012-03-13)
+===================
+
+Fixed bug 953587: add a dict marshaller so that exported method parameters
+can be of type dict.
+
 0.19.4 (2011-10-11)
 ===================
 
 
=== modified file 'src/lazr/restful/configure.zcml'
--- src/lazr/restful/configure.zcml	2011-03-22 14:42:59 +0000
+++ src/lazr/restful/configure.zcml	2012-03-13 23:50:24 +0000
@@ -272,6 +272,13 @@
         />
 
     <adapter
+        for="zope.schema.interfaces.IDict
+             zope.publisher.interfaces.http.IHTTPRequest"
+        provides="lazr.restful.interfaces.IFieldMarshaller"
+        factory="lazr.restful.marshallers.DictFieldMarshaller"
+        />
+
+    <adapter
         for="lazr.restful.interfaces.ICollectionField
              zope.publisher.interfaces.http.IHTTPRequest"
         provides="lazr.restful.interfaces.IFieldMarshaller"
 
=== modified file 'src/lazr/restful/docs/webservice-marshallers.txt'
--- src/lazr/restful/docs/webservice-marshallers.txt	2010-10-05 15:49:55 +0000
+++ src/lazr/restful/docs/webservice-marshallers.txt	2012-03-13 23:50:24 +0000
@@ -700,9 +700,10 @@
 collection of objects associated with some other object. The generic
 collection marshaller will take care of marshalling to the proper
 collection type, and of marshalling the individual items using the
-marshaller for its value_type.
+marshaller for its value_type. Dictionaries may specify separate
+marshallers for their keys and values.
 
-    >>> from zope.schema import List, Tuple, Set
+    >>> from zope.schema import Dict, List, Tuple, Set
     >>> list_of_strings_field = List(value_type=Text())
     >>> from lazr.restful.example.base.interfaces import Cuisine
     >>> tuple_of_ints_field = Tuple(value_type=Int())
@@ -710,6 +711,9 @@
     ...     value_type=Choice(vocabulary=Cuisine))
     >>> set_of_choices_field = Set(
     ...  value_type=Choice(vocabulary=Cuisine)).bind(None)
+    >>> dict_of_choices_field = Dict(
+    ...     key_type=Text(),
+    ...     value_type=Choice(vocabulary=Cuisine))
 
     >>> list_marshaller = getMultiAdapter(
     ...     (list_of_strings_field, request), IFieldMarshaller)
@@ -731,9 +735,22 @@
     >>> verifyObject(IFieldMarshaller, set_marshaller)
     True
 
-The only JSON representation for the collection itself is a list, since
-that's the only sequence type available in JSON. Anything else will
-raise a ValueError.
+    >>> dict_marshaller = getMultiAdapter(
+    ...     (dict_of_choices_field, request), IFieldMarshaller)
+    >>> verifyObject(IFieldMarshaller, dict_marshaller)
+    True
+    >>> dict_key_marshaller = getMultiAdapter(
+    ...     (dict_of_choices_field.key_type, request), IFieldMarshaller)
+    >>> verifyObject(IFieldMarshaller, dict_key_marshaller)
+    True
+    >>> dict_value_marshaller = getMultiAdapter(
+    ...     (dict_of_choices_field.value_type, request), IFieldMarshaller)
+    >>> verifyObject(IFieldMarshaller, dict_value_marshaller)
+    True
+
+For sequences, the only JSON representation for the collection itself is a
+list, since that's the only sequence type available in JSON. Anything else
+will raise a ValueError.
 
     >>> list_marshaller.marshall_from_json_data([u"Test"])
     [u'Test']
@@ -743,11 +760,40 @@
       ...
     ValueError: got 'unicode', expected list: u'Test'
 
+For dicts, we support marshalling from sequences of (name, value) pairs as
+well as from dicts or even strings which are interpreted as single element
+lists.
+
+    >>> dict_marshaller.marshall_from_json_data({u"foo": u"Vegetarian"})
+    {u'foo': <Item Cuisine.VEGETARIAN, ...>}
+
+    >>> dict_marshaller.marshall_from_json_data([(u"foo", u"Vegetarian")])
+    {u'foo': <Item Cuisine.VEGETARIAN, ...>}
+
+    >>> dict_marshaller.marshall_from_request(u"foo,Vegetarian")
+    {u'foo': <Item Cuisine.VEGETARIAN, ...>}
+
+If we attempt to marshall something other than one of the above data formats,
+a ValueError will be raised.
+
+    >>> dict_marshaller.marshall_from_json_data(u"Test")
+    Traceback (most recent call last):
+      ...
+    ValueError: got 'unicode', expected dict: u'Test'
+
+    >>> dict_marshaller.marshall_from_request(u"Test")
+    Traceback (most recent call last):
+      ...
+    ValueError: got '[u'Test']', list of name,value pairs
+
 None is passed through though.
 
     >>> print list_marshaller.marshall_from_json_data(None)
     None
 
+    >>> print dict_marshaller.marshall_from_json_data(None)
+    None
+
 ValueError is also raised if one of the value in the list doesn't
 validate against the more specific marshaller.
 
@@ -760,6 +806,18 @@
     ...     [u'Vegetarian', u'NoSuchChoice'])
     ValueError: Invalid value "NoSuchChoice"...
 
+ValueError is also raised if one of the keys or values in the dict doesn't
+validate against the more specific marshaller.
+
+    >>> dict_marshaller.marshall_from_json_data({1: u"Vegetarian"})
+    Traceback (most recent call last):
+      ...
+    ValueError: got 'int', expected unicode: 1
+
+    >>> show_ValueError(dict_marshaller.marshall_from_request,
+    ...     {u'foo': u'NoSuchChoice'})
+    ValueError: Invalid value "NoSuchChoice"...
+
 The return type is correctly typed to the concrete collection.
 
     >>> tuple_marshaller.marshall_from_json_data([1, 2, 3])
@@ -776,16 +834,28 @@
     >>> [item.title for item in result]
     ['Vegetarian', 'General']
 
+    >>> marshalled_dict = dict_marshaller.marshall_from_json_data(
+    ...     {u'foo': u'Vegetarian', u'bar': u'General'})
+    >>> type(marshalled_dict)
+    <type 'dict'>
+    >>> sorted(
+    ...     [(key, value.title) for key, value in marshalled_dict.items()])
+    [(u'bar', 'General'), (u'foo', 'Vegetarian')]
 
 When coming from the request, either a list or a JSON-encoded
 representation is accepted. The normal request rules for the
-underlying type are then followed.
+underlying type are then followed. When marshalling dicts, the
+list elements are name,value strings which are pulled apart and
+used to populate the dict.
 
     >>> list_marshaller.marshall_from_request([u'1', u'2'])
     [u'1', u'2']
     >>> list_marshaller.marshall_from_request('["1", "2"]')
     [u'1', u'2']
 
+    >>> dict_marshaller.marshall_from_request('["foo,Vegetarian"]')
+    {u'foo': <Item Cuisine.VEGETARIAN, ...>}
+
     >>> tuple_marshaller.marshall_from_request([u'1', u'2'])
     (1, 2)
 
@@ -794,6 +864,9 @@
     >>> print list_marshaller.marshall_from_request('null')
     None
 
+    >>> print dict_marshaller.marshall_from_request('null')
+    None
+
 Also, as a convenience for web client, so that they don't have to JSON
 encode single-element list, non-list value are promoted into a
 single-element list.
@@ -810,6 +883,10 @@
     >>> sorted(set_marshaller.unmarshall(None, marshalled_set))
     ['Dessert', 'Vegetarian']
 
+    >>> unmarshalled = dict_marshaller.unmarshall(None, marshalled_dict)
+    >>> sorted(unmarshalled.iteritems())
+    [(u'bar', 'General'), (u'foo', 'Vegetarian')]
+
 CollectionField
 ---------------
 
 
=== modified file 'src/lazr/restful/marshallers.py'
--- src/lazr/restful/marshallers.py	2011-01-21 21:12:17 +0000
+++ src/lazr/restful/marshallers.py	2012-03-13 23:50:24 +0000
@@ -9,6 +9,7 @@
     'BytesFieldMarshaller',
     'CollectionFieldMarshaller',
     'DateTimeFieldMarshaller',
+    'DictFieldMarshaller',
     'FloatFieldMarshaller',
     'IntFieldMarshaller',
     'ObjectLookupFieldMarshaller',
@@ -112,7 +113,7 @@
         path_parts = [urllib.unquote(part) for part in path.split('/')]
         path_parts.pop(0)
         path_parts.reverse()
-        request = config.createRequest(StringIO(), {'PATH_INFO' : path})
+        request = config.createRequest(StringIO(), {'PATH_INFO': path})
         request.setTraversalStack(path_parts)
         root = request.publication.getApplication(self.request)
         return request.traverse(root)
@@ -125,7 +126,6 @@
     """
     implements(IFieldMarshaller)
 
-
     # Set this to type or tuple of types that the JSON value must be of.
     _type = None
 
@@ -153,7 +153,7 @@
         if value != '':
             try:
                 v = value
-                if isinstance (v, str):
+                if isinstance(v, str):
                     v = v.decode('utf8') # assume utf8
                 # XXX gary 2009-03-28
                 # The use of the enclosing brackets is a hack to work around
@@ -327,7 +327,7 @@
         """Describe all values, not just the selected value."""
         unmarshalled = []
         for item in self.field.vocabulary:
-            item_dict = {'token' : item.token, 'title' : item.title}
+            item_dict = {'token': item.token, 'title': item.title}
             if value.title == item.title:
                 item_dict['selected'] = True
             unmarshalled.append(item_dict)
@@ -462,6 +462,84 @@
         return set
 
 
+class DictFieldMarshaller(SimpleFieldMarshaller):
+    """A marshaller for Dicts.
+
+    It looks up the marshallers for its (key-type, value-type), to handle its
+    contained name/value pairs.
+    """
+    _type = dict
+    _type_error_message = 'not a dict: %r'
+
+    def __init__(self, field, request):
+        """See `SimpleFieldMarshaller`.
+
+        This also looks for the appropriate marshaller for key_type and
+        value_type.
+        """
+        super(DictFieldMarshaller, self).__init__(
+            field, request)
+        self.key_marshaller = getMultiAdapter(
+            (field.key_type, request), IFieldMarshaller)
+        self.value_marshaller = getMultiAdapter(
+            (field.value_type, request), IFieldMarshaller)
+
+    def _marshall_from_request(self, value):
+        """See `IFieldMarshaller`.
+
+        All items in the dict are marshalled using the appropriate
+        `IFieldMarshaller` for the key_type, value_type.
+
+        When coming in via HTML request parameters, the dict is sent as a list
+        of comma separated "key,value" strings.
+        If the value isn't a list or dict, transform it into a one-element list.
+        That allows web client to submit one-element list of strings
+        without having to JSON-encode it.
+        """
+        if not isinstance(value, list) and not isinstance(value, dict):
+            value = [value]
+        if isinstance(value, list):
+            try:
+                value = dict(nv.split(',', 2) for nv in value)
+            except ValueError:
+                raise ValueError(
+                    u"got '%s', list of name,value pairs" % value)
+        return dict(
+            (self.key_marshaller.marshall_from_json_data(key),
+            self.value_marshaller.marshall_from_json_data(val))
+            for key, val in value.items())
+
+    def _marshall_from_json_data(self, value):
+        """See `SimpleFieldMarshaller`.
+
+        Marshall the items in the dict using the appropriate
+        (key,value) marshallers.
+
+        The incoming value may be a list of name,value pairs in which case
+        we convert to a dict first.
+        """
+        if isinstance(value, list):
+            value = dict(value)
+        # Call super to check for errors and raise a ValueError if necessary.
+        value = super(
+            DictFieldMarshaller, self)._marshall_from_json_data(value)
+        return dict(
+            (self.key_marshaller.marshall_from_json_data(key),
+            self.value_marshaller.marshall_from_json_data(val))
+            for key, val in value.items())
+
+    def unmarshall(self, entry, value):
+        """See `SimpleFieldMarshaller`.
+
+        The value is unmarshalled into a dict and all its items are
+        unmarshalled using the appropriate FieldMarshaller.
+        """
+        return dict(
+            (self.key_marshaller.unmarshall(entry, key),
+            self.value_marshaller.unmarshall(entry, val))
+               for key, val in value.items())
+
+
 class CollectionFieldMarshaller(SimpleFieldMarshaller):
     """A marshaller for collection fields."""
     implements(IUnmarshallingDoesntNeedValue)
@@ -569,4 +647,3 @@
         # to the object underlying a resource, we need to strip its
         # security proxy.
         return removeSecurityProxy(resource).context
-
 
=== modified file 'src/lazr/restful/version.txt'
--- src/lazr/restful/version.txt	2011-10-11 01:45:29 +0000
+++ src/lazr/restful/version.txt	2012-03-13 23:50:24 +0000
@@ -1,1 +1,1 @@
-0.19.4
+0.19.5