launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12774
[Merge] lp:~allenap/maas/query-strings-and-request-bodies into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/query-strings-and-request-bodies into lp:maas.
Commit message:
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.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1060079 in MAAS: "CLI sends parameters in the query string for idempotent operations"
https://bugs.launchpad.net/maas/+bug/1060079
For more details, see:
https://code.launchpad.net/~allenap/maas/query-strings-and-request-bodies/+merge/127479
--
https://code.launchpad.net/~allenap/maas/query-strings-and-request-bodies/+merge/127479
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/query-strings-and-request-bodies into lp:maas.
=== 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)
