Merge lp:~radix/txaws/ordered-parameters into lp:txaws

[Christopher Armstrong]

This branch adds the following methods:

- Schema.get_parameters
- MethodRegistry.get_action
- MethodRegistry.get_versions

It also goes to great lengths (gulp) to maintain the original order of parameters as passed to Schema() (both as positional arguments and when passed in the parameters= keyword argument).

Again, I'll be assigning the review to landscape for convenience, but if anyone else wants to review this branch, please feel free.

[Thomas Herve (therve)]

Looks good to me, +1!

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Nice work, I have only minor comments, +1!

[1]

+ @param parameters: (keyword) The parameters of the API, as a either a

s/as a either/as either/

[2]

+ list of named L{Parameter} instances, or a mapping of parameter
+ names to L{Parameter} instances.

Are we retaining the old behavior for backward-compatibility? Eventually I believe it'd be desirable to stick to one form only. It's not really a problem, just something could be worth doing mid-term.

[3]

- def _inner_convert_old_schema(self, mapping, depth):
+ def _inner_convert_old_schema(self, mapping, depth, name):
         """
         Internal recursion helper for L{_convert_old_schema}.
         """
- if not isinstance(mapping, dict):
+ if not isinstance(mapping, list):

It took me a while to understand what the "mapping" parameter is. Firstly because "mapping" makes one thing to a dict, secondly because the term "mapping" is very generic and doesn't give any hint about what it actually maps.

Afaics "mapping" is a tree structure represented by an associative array, that is been traversed recursively, so maybe the term "node" would be more appropriate, e.g.:

def _inner_convert_old_schema(self, node_name, node_value, node_depth):
    if not isinstance(node_value, list):
        return node
    ....

[4]

+def _merge_alist(alist, path, value):

Calling it _merge_associative_list would be more verbose, but hopefully more clear for a casual reader of the code ("alist" could be mistaken for "a list", as some list).

review: Approve

[Free Ekanayaka (free.ekanayaka)]

About [3], there's a typo: s/node/node_value/, and maybe a comment would be nice too:

def _inner_convert_old_schema(self, node_name, node_value, node_depth):
    if not isinstance(node_value, list):
        # This is a leaf node, we have a plain parameter that doesn't
        # need any conversion
        return node_value

[lp:~radix/txaws/ordered-parameters]

153. By Christopher Armstrong on 2012-05-24

Address all of free's comments [1-4].
- remove support for parameters={...}
- document schema conversion a lot more

=== modified file 'txaws/server/registry.py'
--- txaws/server/registry.py	2012-03-18 14:15:01 +0000
+++ txaws/server/registry.py	2012-05-24 16:02:20 +0000
@@ -52,3 +52,15 @@
             # Only pass it if specified, for backward compatibility
             kwargs["ignore"] = ignore
         scanner.scan(module, **kwargs)
+
+    def get_actions(self):
+        """
+        Get a list of all action names.
+        """
+        return self._by_action.keys()
+
+    def get_versions(self, action_name):
+        """
+        Get a list of versions supported by the given action name.
+        """
+        return self._by_action[action_name].keys()

=== modified file 'txaws/server/schema.py'
--- txaws/server/schema.py	2012-05-04 17:46:22 +0000
+++ txaws/server/schema.py	2012-05-24 16:02:20 +0000
@@ -384,6 +384,7 @@
             raise TypeError("Must provide fields")
         super(Structure, self).__init__(name, optional=optional,
                                         default=default, doc=doc)
+        _namify_arguments(fields)
         self.fields = fields
 
     def parse(self, value):
@@ -471,6 +472,18 @@
             return value
 
 
+def _namify_arguments(mapping):
+    """
+    Ensure that a mapping of names to parameters has the parameters set to the
+    correct name.
+    """
+    result = []
+    for name, parameter in mapping.iteritems():
+        parameter.name = name
+        result.append(parameter)
+    return result
+
+
 class Schema(object):
     """
     The schema that the arguments of an HTTP request must be compliant with.
@@ -482,7 +495,7 @@
         Any number of L{Parameter} instances can be passed. The parameter names
         are used in L{Schema.extract} and L{Schema.bundle}. For example::
 
-          schema = Schema(name="SetName", parameters={"Name": Unicode()})
+          schema = Schema(name="SetName", parameters=[Unicode("Name")])
 
         means that the result of L{Schema.extract} would have a C{Name}
         attribute. Similarly, L{Schema.bundle} would look for a C{Name}
@@ -492,7 +505,7 @@
 
           schema = Schema(
               name="SetNames",
-              parameters={"Names": List(item=Unicode())})
+              parameters=[List("Names", Unicode())])
 
         means that the result of L{Schema.extract} would have a C{Names}
         attribute, which would itself contain a list of names. Similarly,
@@ -504,13 +517,12 @@
 
         @param name: (keyword) The name of the API call that this schema
             represents. Accessible via the C{name} attribute.
-        @param parameters: (keyword) The parameters of the API, as a mapping
-            of parameter names to L{Parameter} instances.
+        @param parameters: (keyword) The parameters of the API, as a list of
+            named L{Parameter} instances.
         @param doc: (keyword) The documentation of this API Call. Accessible
             via the C{doc} attribute.
-        @param result: (keyword) A description of the result of this API call,
-            in the same format as C{parameters}. Accessible via the C{result}
-            attribute.
+        @param result: (keyword) A description of the result of this API
+            call. Accessible via the C{result} attribute.
         @param errors: (keyword) A sequence of exception classes that the API
             can potentially raise. Accessible as a L{set} via the C{errors}
             attribute.
@@ -527,6 +539,12 @@
         else:
             self._parameters = self._convert_old_schema(_parameters)
 
+    def get_parameters(self):
+        """
+        Get the list of parameters this schema supports.
+        """
+        return self._parameters[:]
+
     def extract(self, params):
         """Extract parameters from a raw C{dict} according to this schema.
 
@@ -534,7 +552,8 @@
         @return: A tuple of an L{Arguments} object holding the extracted
             arguments and any unparsed arguments.
         """
-        structure = Structure(fields=self._parameters)
+        structure = Structure(fields=dict([(p.name, p)
+                                           for p in self._parameters]))
         try:
             tree = structure.coerce(self._convert_flat_to_nest(params))
             rest = {}
@@ -563,7 +582,7 @@
                 continue
             segments = name.split('.')
             first = segments[0]
-            parameter = self._parameters.get(first)
+            parameter = self.get_parameter(first)
             if parameter is None:
                 raise RuntimeError("Parameter '%s' not in schema" % name)
             else:
@@ -574,6 +593,14 @@
 
         return self._convert_nest_to_flat(result)
 
+    def get_parameter(self, name):
+        """
+        Get the parameter on this schema with the given C{name}.
+        """
+        for parameter in self._parameters:
+            if parameter.name == name:
+                return parameter
+
     def _convert_flat_to_nest(self, params):
         """
         Convert a structure in the form of::
@@ -647,17 +674,19 @@
         new_kwargs = {
             'name': self.name,
             'doc': self.doc,
-            'parameters': self._parameters.copy(),
+            'parameters': self._parameters[:],
             'result': self.result.copy() if self.result else {},
             'errors': self.errors.copy() if self.errors else set()}
-        new_kwargs['parameters'].update(kwargs.pop('parameters', {}))
+        if 'parameters' in kwargs:
+            new_params = kwargs.pop('parameters')
+            new_kwargs['parameters'].extend(new_params)
         new_kwargs['result'].update(kwargs.pop('result', {}))
         new_kwargs['errors'].update(kwargs.pop('errors', set()))
         new_kwargs.update(kwargs)
 
         if schema_items:
             parameters = self._convert_old_schema(schema_items)
-            new_kwargs['parameters'].update(parameters)
+            new_kwargs['parameters'].extend(parameters)
         return Schema(**new_kwargs)
 
     def _convert_old_schema(self, parameters):
@@ -672,37 +701,91 @@
 
         becomes::
 
-            {"foo": List(
+            [List(
+                "foo",
                 item=Structure(
                     fields={"baz": List(item=Integer()),
-                            "shimmy": Integer()}))}
+                            "shimmy": Integer()}))]
 
         By design, the old schema syntax ignored the names "bar" and "quux".
         """
-        crap = {}
+        # 'merged' here is an associative list that maps parameter names to
+        # Parameter instances, OR sub-associative lists which represent nested
+        # lists and structures.
+        # e.g.,
+        #    [Integer("foo")]
+        # becomes
+        #    [("foo", Integer("foo"))]
+        # and
+        #    [Integer("foo.bar")]
+        # (which represents a list of integers called "foo" with a meaningless
+        # index name of "bar") becomes
+        #     [("foo", [("bar", Integer("foo.bar"))])].
+        merged = []
         for parameter in parameters:
-            crap[parameter.name] = parameter
-        nest = self._convert_flat_to_nest(crap)
-        return self._inner_convert_old_schema(nest, 0).fields
+            segments = parameter.name.split('.')
+            _merge_associative_list(merged, segments, parameter)
+        result = [self._inner_convert_old_schema(node, 1) for node in merged]
+        return result
 
-    def _inner_convert_old_schema(self, mapping, depth):
+    def _inner_convert_old_schema(self, node, depth):
         """
         Internal recursion helper for L{_convert_old_schema}.
+
+        @param node: A node in the associative list tree as described in
+            _convert_old_schema. A two tuple of (name, parameter).
+        @param depth: The depth that the node is at. This is important to know
+            if we're currently processing a list or a structure. ("foo.N" is a
+            list called "foo", "foo.N.fieldname" describes a field in a list of
+            structs).
         """
-        if not isinstance(mapping, dict):
-            return mapping
+        name, parameter_description = node
+        if not isinstance(parameter_description, list):
+            # This is a leaf, i.e., an actual L{Parameter} instance.
+            return parameter_description
         if depth % 2 == 0:
+            # we're processing a structure.
             fields = {}
-            for k, v in mapping.iteritems():
-                fields[k] = self._inner_convert_old_schema(v, depth + 1)
-            return Structure("anonymous_structure", fields=fields)
+            for node in parameter_description:
+                fields[node[0]] = self._inner_convert_old_schema(node, depth + 1)
+            return Structure(name, fields=fields)
         else:
-            if not isinstance(mapping, dict):
-                raise TypeError("mapping %r must be a dict" % (mapping,))
-            if not len(mapping) == 1:
-                raise ValueError("Multiple different index names specified: %r"
-                                 % (mapping.keys(),))
-            item = mapping.values()[0]
-            item = self._inner_convert_old_schema(item, depth + 1)
-            name = item.name.split('.', 1)[0]
+            # we're processing a list.
+            if not isinstance(parameter_description, list):
+                raise TypeError("node %r must be an associative list"
+                                % (parameter_description,))
+            if not len(parameter_description) == 1:
+                raise ValueError(
+                    "Multiple different index names specified: %r"
+                    % ([item[0] for item in parameter_description],))
+            subnode = parameter_description[0]
+            item = self._inner_convert_old_schema(subnode, depth + 1)
             return List(name=name, item=item, optional=item.optional)
+
+
+def _merge_associative_list(alist, path, value):
+    """
+    Merge a value into an associative list at the given path, maintaining
+    insertion order. Examples will explain it::
+
+        >>> alist = []
+        >>> _merge_associative_list(alist, ["foo", "bar"], "barvalue")
+        >>> _merge_associative_list(alist, ["foo", "baz"], "bazvalue")
+        >>> alist == [("foo", [("bar", "barvalue"), ("baz", "bazvalue")])]
+
+    @param alist: An associative list of names to values.
+    @param path: A path through sub-alists which we ultimately want to point to
+    C{value}.
+    @param value: The value to set.
+    @return: None. This operation mutates the associative list in place.
+    """
+    for key in path[:-1]:
+        for item in alist:
+            if item[0] == key:
+                alist = item[1]
+                break
+        else:
+            subalist = []
+            alist.append((key, subalist))
+            alist = subalist
+    alist.append((path[-1], value))

=== modified file 'txaws/server/tests/test_registry.py'
--- txaws/server/tests/test_registry.py	2012-03-18 14:15:01 +0000
+++ txaws/server/tests/test_registry.py	2012-05-24 16:02:20 +0000
@@ -51,6 +51,29 @@
         self.assertRaises(RuntimeError, self.registry.add, TestMethod2,
                           "test", "1.0")
 
+    def test_get_actions(self):
+        """
+        L{MethodRegistry.get_actions} returns a list of action names.
+        """
+        self.registry.add(TestMethod, "test", "1.0")
+        self.registry.add(TestMethod, "test", "2.0")
+        self.registry.add(TestMethod, "test2", "1.0")
+        self.assertEqual(["test", "test2"],
+                         sorted(self.registry.get_actions()))
+
+    def test_get_versions(self):
+        """
+        L{MethodRegistry.get_versions} returns a list of versions supported by
+        the action.
+        """
+        self.registry.add(TestMethod, "test", "1.0")
+        self.registry.add(TestMethod, "test", "2.0")
+        self.registry.add(TestMethod, "test2", "1.0")
+        self.assertEqual(["1.0", "2.0"],
+                         sorted(self.registry.get_versions("test")))
+        self.assertEqual(["1.0"],
+                         sorted(self.registry.get_versions("test2")))
+
     def test_get(self):
         """
         L{MethodRegistry.get} returns the method class registered for the

=== modified file 'txaws/server/tests/test_schema.py'
--- txaws/server/tests/test_schema.py	2012-05-04 17:48:57 +0000
+++ txaws/server/tests/test_schema.py	2012-05-24 16:02:20 +0000
@@ -384,6 +384,29 @@
 
 class SchemaTestCase(TestCase):
 
+    def test_get_parameters(self):
+        """
+        L{Schema.get_parameters} returns the original list of parameters.
+        """
+        schema = Schema(parameters=[
+                Unicode("name"),
+                List("scores", Integer())])
+        parameters = schema.get_parameters()
+        self.assertEqual("name", parameters[0].name)
+        self.assertEqual("scores", parameters[1].name)
+
+    def test_get_parameters_order_on_parameter_only_construction(self):
+        """
+        L{Schema.get_parameters} returns the original list of L{Parameter}s
+        even when they are passed as positional arguments to L{Schema}.
+        """
+        schema = Schema(
+            Unicode("name"),
+            List("scores", Integer()),
+            Integer("index", Integer()))
+        self.assertEqual(["name", "scores", "index"],
+                         [p.name for p in schema.get_parameters()])
+
     def test_extract(self):
         """
         L{Schema.extract} returns an L{Argument} object whose attributes are
@@ -516,7 +539,7 @@
         """
         schema = Schema(Unicode("name.n"))
         self.assertRaises(InconsistentParameterError,
-                          schema.extract, {"name": "foo", "name.1": "bar"})
+                          schema.extract, {"nameFOOO": "foo", "nameFOOO.1": "bar"})
 
     def test_extract_with_non_numbered_template(self):
         """
@@ -614,16 +637,16 @@
     def test_bundle_with_structure(self):
         """L{Schema.bundle} can bundle L{Structure}s."""
         schema = Schema(
-            parameters={
-                "struct": Structure(fields={"field1": Unicode(),
-                                            "field2": Integer()})})
+            parameters=[
+                Structure("struct", fields={"field1": Unicode(),
+                                            "field2": Integer()})])
         params = schema.bundle(struct={"field1": "hi", "field2": 59})
         self.assertEqual({"struct.field1": "hi", "struct.field2": "59"},
                          params)
 
     def test_bundle_with_list(self):
         """L{Schema.bundle} can bundle L{List}s."""
-        schema = Schema(parameters={"things": List(item=Unicode())})
+        schema = Schema(parameters=[List("things", item=Unicode())])
         params = schema.bundle(things=["foo", "bar"])
         self.assertEqual({"things.1": "foo", "things.2": "bar"}, params)
 
@@ -632,9 +655,9 @@
         L{Schema.bundle} can bundle L{Structure}s (specified as L{Arguments}).
         """
         schema = Schema(
-            parameters={
-                "struct": Structure(fields={"field1": Unicode(),
-                                            "field2": Integer()})})
+            parameters=[
+                Structure("struct", fields={"field1": Unicode(),
+                                            "field2": Integer()})])
         params = schema.bundle(struct=Arguments({"field1": "hi",
                                                  "field2": 59}))
         self.assertEqual({"struct.field1": "hi", "struct.field2": "59"},
@@ -642,7 +665,7 @@
 
     def test_bundle_with_list_with_arguments(self):
         """L{Schema.bundle} can bundle L{List}s (specified as L{Arguments})."""
-        schema = Schema(parameters={"things": List(item=Unicode())})
+        schema = Schema(parameters=[List("things", item=Unicode())])
         params = schema.bundle(things=Arguments({1: "foo", 2: "bar"}))
         self.assertEqual({"things.1": "foo", "things.2": "bar"}, params)
 
@@ -770,17 +793,20 @@
         {name: field} format.
         """
         schema = Schema(
-            parameters={"foo": Structure(
-                    fields={"l": List(item=Integer())})})
+            parameters=[Structure("foo",
+                    fields={"l": List(item=Integer())})])
         arguments, _ = schema.extract({"foo.l.1": "1", "foo.l.2": "2"})
         self.assertEqual([1, 2], arguments.foo.l)
 
-    def test_schema_conversion_list_name(self):
+    def test_schema_conversion_list(self):
         """
         Backwards-compatibility conversion maintains the name of lists.
         """
         schema = Schema(Unicode("foos.N"))
-        self.assertEqual("foos", schema._parameters["foos"].name)
+        parameters = schema.get_parameters()
+        self.assertEqual(1, len(parameters))
+        self.assertTrue(isinstance(parameters[0], List))
+        self.assertEqual("foos", parameters[0].name)
 
     def test_schema_conversion_structure_name(self):
         """
@@ -789,12 +815,16 @@
         """
         schema = Schema(Unicode("foos.N.field"),
                         Unicode("foos.N.field2"))
-        self.assertEqual("anonymous_structure",
-                         schema._parameters["foos"].item.name)
-        self.assertEqual("foos.N.field",
-                         schema._parameters["foos"].item.fields["field"].name)
-        self.assertEqual("foos.N.field2",
-                         schema._parameters["foos"].item.fields["field2"].name)
+        parameters = schema.get_parameters()
+        self.assertEqual(1, len(parameters))
+        self.assertTrue(isinstance(parameters[0], List))
+        self.assertEqual("foos", parameters[0].name)
+        self.assertEqual("N",
+                         parameters[0].item.name)
+        self.assertEqual("field",
+                         parameters[0].item.fields["field"].name)
+        self.assertEqual("field2",
+                         parameters[0].item.fields["field2"].name)
 
     def test_schema_conversion_optional_list(self):
         """
@@ -829,9 +859,9 @@
         schema = Schema(
             name="GetStuff",
             doc="""Get the stuff.""",
-            parameters={
-                'id': Integer(),
-                'scope': Unicode()},
+            parameters=[
+                Integer("id"),
+                Unicode("scope")],
             result=result,
             errors=errors)
 
@@ -852,12 +882,12 @@
 
         schema = Schema(
             name="GetStuff",
-            parameters={"id": Integer()})
+            parameters=[Integer("id")])
 
         schema2 = schema.extend(
             name="GetStuff2",
             doc="Get stuff 2",
-            parameters={'scope': Unicode()},
+            parameters=[Unicode("scope")],
             result=result,
             errors=errors)
 
@@ -884,11 +914,11 @@
         schema = Schema(
             name="GetStuff",
             doc="""Get the stuff.""",
-            parameters={'id': Integer()},
+            parameters=[Integer("id")],
             result=result,
             errors=errors)
 
-        schema2 = schema.extend(parameters={'scope': Unicode()})
+        schema2 = schema.extend(parameters=[Unicode("scope")])
 
         self.assertEqual("GetStuff", schema2.name)
         self.assertEqual("Get the stuff.", schema2.doc)
@@ -917,6 +947,20 @@
         """
         Errors can be extended with L{Schema.extend}.
         """
-        schema = Schema(parameters={}, errors=[APIError])
+        schema = Schema(parameters=[], errors=[APIError])
         schema2 = schema.extend(errors=[ZeroDivisionError])
         self.assertEqual(set([APIError, ZeroDivisionError]), schema2.errors)
+
+    def test_extend_maintains_parameter_order(self):
+        """
+        Extending a schema with additional parameters puts the new parameters
+        at the end.
+        """
+        schema = Schema(parameters=[Unicode("name"), Unicode("value")])
+        schema2 = schema.extend(parameters=[Integer("foo"), Unicode("index")])
+        self.assertEqual(["name", "value", "foo", "index"],
+                         [p.name for p in schema2.get_parameters()])
+
+    def test_schema_field_names(self):
+        structure = Structure(fields={"foo": Integer()})
+        self.assertEqual("foo", structure.fields["foo"].name)