Merge lp:~allenap/maas/query-strings-and-request-bodies into lp:maas/trunk

[Gavin Panella]

For the CLI, only encode request data in the query string for GET requests.

Previously, request data was being encoded in the query string for PUT and DELETE requests too.

[Raphaël Badin (rvb)]

Looks good. ("from itertools import chain", you like that method don't you ;) )

review: Approve

[Gavin Panella (allenap)]

> Looks good. ("from itertools import chain", you like that method don't you ;)

Yeah, it's teh awesome :)

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-09-27 01:39:52 +0000
+++ src/maascli/api.py	2012-10-02 13:04:33 +0000
@@ -16,6 +16,7 @@
 
 from getpass import getpass
 import httplib
+from itertools import chain
 import json
 import sys
 from urlparse import (
@@ -193,7 +194,6 @@
 
     uri = property(lambda self: self.handler["uri"])
     method = property(lambda self: self.action["method"])
-    is_restful = property(lambda self: self.action["restful"])
     credentials = property(lambda self: self.profile["credentials"])
     op = property(lambda self: self.action["op"])
 
@@ -210,13 +210,9 @@
         # <https://github.com/uri-templates/uritemplate-py> here?
         uri = self.uri.format(**vars(options))
 
-        # Add the operation to the data set.
-        if self.op is not None:
-            options.data.append(("op", self.op))
-
         # Bundle things up ready to throw over the wire.
         uri, body, headers = self.prepare_payload(
-            self.method, self.is_restful, uri, options.data)
+            self.op, self.method, uri, options.data)
 
         # Sign request if credentials have been provided.
         if self.credentials is not None:
@@ -247,31 +243,31 @@
                 "%r is not a name=value pair" % string)
 
     @staticmethod
-    def prepare_payload(method, is_restful, uri, data):
+    def prepare_payload(op, method, uri, data):
         """Return the URI (modified perhaps) and body and headers.
 
+        - For GET requests, encode parameters in the query string.
+
+        - Otherwise always encode parameters in the request body.
+
+        - Except op; this can always go in the query string.
+
         :param method: The HTTP method.
-        :param is_restful: Is this a ReSTful operation?
         :param uri: The URI of the action.
         :param data: A dict or iterable of name=value pairs to pack into the
             body or headers, depending on the type of request.
         """
-        if method == "POST" and not is_restful:
-            # Encode the data as multipart for non-ReSTful POST requests; all
-            # others should use query parameters. TODO: encode_multipart_data
-            # insists on a dict for the data, which prevents specifying
-            # multiple values for a field, like mac_addresses.  This needs to
-            # be fixed.
-            body, headers = encode_multipart_data(data, {})
-            # TODO: make encode_multipart_data work with arbitrarily encoded
-            # strings; atm, it blows up when encountering a non-ASCII string.
-        else:
-            # TODO: deal with state information, i.e. where to stuff CRUD
-            # data, content types, etc.
+        if method == "GET":
+            query = data if op is None else chain([("op", op)], data)
             body, headers = None, {}
-            # TODO: smarter merging of data with query.
-            uri = urlparse(uri)._replace(query=urlencode(data)).geturl()
+        else:
+            query = [] if op is None else [("op", op)]
+            if data:
+                body, headers = encode_multipart_data(data)
+            else:
+                body, headers = None, {}
 
+        uri = urlparse(uri)._replace(query=urlencode(query)).geturl()
         return uri, body, headers
 
     @staticmethod

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2012-09-27 03:02:58 +0000
+++ src/maascli/tests/test_api.py	2012-10-02 13:04:33 +0000
@@ -27,7 +27,6 @@
 from mock import sentinel
 from testtools.matchers import (
     Equals,
-    Is,
     IsInstance,
     MatchesAll,
     MatchesListwise,
@@ -154,57 +153,137 @@
             api.Action.name_value_pair(" foo = bar "))
 
 
-class TestActionReSTful(TestCase):
-    """Tests for ReSTful operations in `maascli.api.Action`."""
-
-    scenarios = (
-        ("create", dict(method="POST")),
-        ("read", dict(method="GET")),
-        ("update", dict(method="PUT")),
-        ("delete", dict(method="DELETE")),
-        )
-
-    def test_prepare_payload_without_data(self):
-        # prepare_payload() is almost a no-op for ReSTful methods that don't
-        # specify any extra data.
-        uri_base = "http://example.com/MAAS/api/1.0/"
-        payload = api.Action.prepare_payload(
-            method=self.method, is_restful=True, uri=uri_base, data=[])
-        expected = (
-            Equals(uri_base),  # uri
-            Is(None),  # body
-            Equals({}),  # headers
-            )
-        self.assertThat(payload, MatchesListwise(expected))
-
-    def test_prepare_payload_with_data(self):
-        # Given data is always encoded as query parameters.
-        uri_base = "http://example.com/MAAS/api/1.0/"
-        payload = api.Action.prepare_payload(
-            method=self.method, is_restful=True, uri=uri_base,
-            data=[("foo", "bar"), ("foo", "baz")])
-        expected = (
-            Equals(uri_base + "?foo=bar&foo=baz"),  # uri
-            Is(None),  # body
-            Equals({}),  # headers
-            )
-        self.assertThat(payload, MatchesListwise(expected))
-
-
-class TestActionOperations(TestCase):
-    """Tests for non-ReSTful operations in `maascli.api.Action`."""
-
-    def test_prepare_payload_POST_non_restful(self):
-        # Non-ReSTful POSTs encode the given data using encode_multipart_data.
+class TestPayloadPreparation(TestCase):
+    """Tests for `maascli.api.Action.prepare_payload`."""
+
+    uri_base = "http://example.com/MAAS/api/1.0/"
+
+    # Scenarios for ReSTful operations; i.e. without an "op" parameter.
+    scenarios_without_op = (
+        # Without data, all requests have an empty request body and no extra
+        # headers.
+        ("create",
+         {"method": "POST", "data": [],
+          "expected_uri": uri_base,
+          "expected_body": None,
+          "expected_headers": {}}),
+        ("read",
+         {"method": "GET", "data": [],
+          "expected_uri": uri_base,
+          "expected_body": None,
+          "expected_headers": {}}),
+        ("update",
+         {"method": "PUT", "data": [],
+          "expected_uri": uri_base,
+          "expected_body": None,
+          "expected_headers": {}}),
+        ("delete",
+         {"method": "DELETE", "data": [],
+          "expected_uri": uri_base,
+          "expected_body": None,
+          "expected_headers": {}}),
+        # With data, PUT, POST, and DELETE requests have their body and extra
+        # headers prepared by encode_multipart_data. For GET requests, the
+        # data is encoded into the query string, and both the request body and
+        # extra headers are empty.
+        ("create-with-data",
+         {"method": "POST", "data": [("foo", "bar"), ("foo", "baz")],
+          "expected_uri": uri_base,
+          "expected_body": sentinel.body,
+          "expected_headers": sentinel.headers}),
+        ("read-with-data",
+         {"method": "GET", "data": [("foo", "bar"), ("foo", "baz")],
+          "expected_uri": uri_base + "?foo=bar&foo=baz",
+          "expected_body": None,
+          "expected_headers": {}}),
+        ("update-with-data",
+         {"method": "PUT", "data": [("foo", "bar"), ("foo", "baz")],
+          "expected_uri": uri_base,
+          "expected_body": sentinel.body,
+          "expected_headers": sentinel.headers}),
+        ("delete-with-data",
+         {"method": "DELETE", "data": [("foo", "bar"), ("foo", "baz")],
+          "expected_uri": uri_base,
+          "expected_body": sentinel.body,
+          "expected_headers": sentinel.headers}),
+        )
+
+    # Scenarios for non-ReSTful operations; i.e. with an "op" parameter.
+    scenarios_with_op = (
+        # Without data, all requests have an empty request body and no extra
+        # headers. The operation is encoded into the query string.
+        ("create",
+         {"method": "POST", "data": [],
+          "expected_uri": uri_base + "?op=something",
+          "expected_body": None,
+          "expected_headers": {}}),
+        ("read",
+         {"method": "GET", "data": [],
+          "expected_uri": uri_base + "?op=something",
+          "expected_body": None,
+          "expected_headers": {}}),
+        ("update",
+         {"method": "PUT", "data": [],
+          "expected_uri": uri_base + "?op=something",
+          "expected_body": None,
+          "expected_headers": {}}),
+        ("delete",
+         {"method": "DELETE", "data": [],
+          "expected_uri": uri_base + "?op=something",
+          "expected_body": None,
+          "expected_headers": {}}),
+        # With data, PUT, POST, and DELETE requests have their body and extra
+        # headers prepared by encode_multipart_data. For GET requests, the
+        # data is encoded into the query string, and both the request body and
+        # extra headers are empty. The operation is encoded into the query
+        # string.
+        ("create-with-data",
+         {"method": "POST", "data": [("foo", "bar"), ("foo", "baz")],
+          "expected_uri": uri_base + "?op=something",
+          "expected_body": sentinel.body,
+          "expected_headers": sentinel.headers}),
+        ("read-with-data",
+         {"method": "GET", "data": [("foo", "bar"), ("foo", "baz")],
+          "expected_uri": uri_base + "?op=something&foo=bar&foo=baz",
+          "expected_body": None,
+          "expected_headers": {}}),
+        ("update-with-data",
+         {"method": "PUT", "data": [("foo", "bar"), ("foo", "baz")],
+          "expected_uri": uri_base + "?op=something",
+          "expected_body": sentinel.body,
+          "expected_headers": sentinel.headers}),
+        ("delete-with-data",
+         {"method": "DELETE", "data": [("foo", "bar"), ("foo", "baz")],
+          "expected_uri": uri_base + "?op=something",
+          "expected_body": sentinel.body,
+          "expected_headers": sentinel.headers}),
+        )
+
+    scenarios_without_op = tuple(
+        ("%s-without-op" % name, dict(scenario, op=None))
+        for name, scenario in scenarios_without_op)
+
+    scenarios_with_op = tuple(
+        ("%s-with-op" % name, dict(scenario, op="something"))
+        for name, scenario in scenarios_with_op)
+
+    scenarios = scenarios_without_op + scenarios_with_op
+
+    def test_prepare_payload(self):
+        # Patch encode_multipart_data to match the scenarios.
         encode_multipart_data = self.patch(api, "encode_multipart_data")
         encode_multipart_data.return_value = sentinel.body, sentinel.headers
-        uri_base = "http://example.com/MAAS/api/1.0/"
+        # The payload returned is a 3-tuple of (uri, body, headers).
         payload = api.Action.prepare_payload(
-            method="POST", is_restful=False, uri=uri_base,
-            data=[("foo", "bar"), ("foo", "baz")])
+            op=self.op, method=self.method,
+            uri=self.uri_base, data=self.data)
         expected = (
-            Equals(uri_base),  # uri
-            Is(sentinel.body),  # body
-            Is(sentinel.headers),  # headers
+            Equals(self.expected_uri),
+            Equals(self.expected_body),
+            Equals(self.expected_headers),
             )
         self.assertThat(payload, MatchesListwise(expected))
+        # encode_multipart_data, when called, is passed the data
+        # unadulterated.
+        if self.expected_body is sentinel.body:
+            api.encode_multipart_data.assert_called_once_with(self.data)