From 92f984e22416692927071cdf6f8543e98a064fe3 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Wed, 4 Feb 2015 23:40:14 +0100 Subject: [PATCH 01/11] Update docs (ref #2) --- docs/batch.rst | 118 +++++++++++++++++++++++++++++++++++++++++++++++++ docs/index.rst | 1 + 2 files changed, 119 insertions(+) create mode 100644 docs/batch.rst diff --git a/docs/batch.rst b/docs/batch.rst new file mode 100644 index 0000000..24111a3 --- /dev/null +++ b/docs/batch.rst @@ -0,0 +1,118 @@ +################ +Batch operations +################ + +.. _batch: + +POST /batch +=========== + +**Requires an FxA OAuth authentication** + +The POST body is a mapping, with the following attributes: + +- ``requests``: the list of requests +- ``defaults``: (*optional*) values in common for all requests + + Each request is a JSON mapping, with the following attribute: + +- ``method``: HTTP verb +- ``path``: URI +- ``body``: a mapping +- ``headers``: (*optional*), otherwise take those of batch request + +:: + + { + "defaults": { + "method" : "POST", + "path" : "/articles", + "headers" : { + ... + } + }, + "requests": [ + { + "body" : { + "title": "MoFo", + "url" : "http://mozilla.org" + } + }, + { + "body" : { + "title": "MoCo", + "url" : "http://mozilla.com" + } + }, + { + "method" : "PATCH", + "path" : "/articles/409", + "body" : { + "read_position" : 3477 + } + } + ] + ] + + +The response body is a list of all responses: + +:: + + { + "defaults": { + "path" : "/articles", + }, + "responses": [ + { + "path" : "/articles/409", + "status": 200, + "body" : { + "id": 409, + "url": "...", + ... + "read_position" : 3477 + }, + "headers": { + ... + } + }, + { + "status": 201, + "body" : { + "id": 411, + "title": "MoFo", + "url" : "http://mozilla.org", + ... + }, + }, + { + "status": 201, + "body" : { + "id": 412, + "title": "MoCo", + "url" : "http://mozilla.com", + ... + }, + }, + ] + ] + + +:note: + + The responses are not necessarily in the same order of the requests. + + +Pros & Cons +::::::::::: + +* This respects REST principles +* This is easy for the client to handle, since it just has to pile up HTTP requests while offline +* It looks to be a convention for several REST APIs (`Neo4J `_, `Facebook `_, `Parse `_) +* Payload of response can be heavy, especially while importing huge collections +* Payload of response must all be iterated to look-up errors + +:note: + + A form of payload optimization for massive operations is planned. diff --git a/docs/index.rst b/docs/index.rst index a496db8..47bd8a0 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -19,6 +19,7 @@ Contents: model authentication api + batch utilities timestamps versionning From 3c93d4e02010c4b839c35e29cc11ccf3d6eeb79b Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Wed, 4 Feb 2015 23:41:15 +0100 Subject: [PATCH 02/11] Initialize batch endpoint (ref #2) --- readinglist/tests/test_batch.py | 8 ++++++++ readinglist/views/batch.py | 9 +++++++++ 2 files changed, 17 insertions(+) create mode 100644 readinglist/tests/test_batch.py create mode 100644 readinglist/views/batch.py diff --git a/readinglist/tests/test_batch.py b/readinglist/tests/test_batch.py new file mode 100644 index 0000000..6dbeecc --- /dev/null +++ b/readinglist/tests/test_batch.py @@ -0,0 +1,8 @@ +from .support import BaseWebTest, unittest + + +class BatchViewTest(BaseWebTest, unittest.TestCase): + + def test_returns_empty_response_if_empty_requests(self): + resp = self.app.post('/batch', {}, headers=self.headers) + self.assertEqual(resp.json, {}) diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py new file mode 100644 index 0000000..a627828 --- /dev/null +++ b/readinglist/views/batch.py @@ -0,0 +1,9 @@ +from cornice import Service + +batch = Service(name="batch", path='/batch', + description="Batch operations") + + +@batch.post() +def post_batch(request): + return {} From b88cc2d78f73f51bfeee673a9eacba501af5d93c Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Thu, 5 Feb 2015 14:33:08 +0100 Subject: [PATCH 03/11] Start testing batch endpoint (ref #2) --- readinglist/authentication.py | 1 + readinglist/tests/resource/__init__.py | 13 +----- readinglist/tests/support.py | 17 ++++++- readinglist/tests/test_batch.py | 8 ---- readinglist/tests/test_views_batch.py | 64 ++++++++++++++++++++++++++ readinglist/views/batch.py | 19 +++++++- 6 files changed, 100 insertions(+), 22 deletions(-) delete mode 100644 readinglist/tests/test_batch.py create mode 100644 readinglist/tests/test_views_batch.py diff --git a/readinglist/authentication.py b/readinglist/authentication.py index 90d694b..31b3cfd 100644 --- a/readinglist/authentication.py +++ b/readinglist/authentication.py @@ -87,6 +87,7 @@ def permits(self, context, principals, permission): Authenticated users only are allowed. """ PERMISSIONS = { + 'batch': Authenticated, 'readonly': Authenticated, 'readwrite': Authenticated, } diff --git a/readinglist/tests/resource/__init__.py b/readinglist/tests/resource/__init__.py index 5698289..2cd441a 100644 --- a/readinglist/tests/resource/__init__.py +++ b/readinglist/tests/resource/__init__.py @@ -1,9 +1,5 @@ -import mock - -from cornice import errors as cornice_errors - from readinglist.backend.memory import Memory -from readinglist.tests.support import unittest +from readinglist.tests.support import unittest, DummyRequest from readinglist.resource import BaseResource @@ -16,13 +12,8 @@ def setUp(self): self.resource = BaseResource(self.get_request()) def get_request(self): - request = mock.MagicMock(headers={}) + request = DummyRequest() request.db = self.db - request.errors = cornice_errors.Errors() - request.authenticated_userid = 'bob' - request.validated = {} - request.matchdict = {} - request.response = mock.MagicMock(headers={}) return request @property diff --git a/readinglist/tests/support.py b/readinglist/tests/support.py index f32485d..6441543 100644 --- a/readinglist/tests/support.py +++ b/readinglist/tests/support.py @@ -1,16 +1,31 @@ import mock import threading -import webtest try: import unittest2 as unittest except ImportError: import unittest # NOQA +from cornice import errors as cornice_errors +import webtest + from readinglist import API_VERSION from readinglist.utils import random_bytes_hex + +class DummyRequest(mock.MagicMock): + def __init__(self, *args, **kwargs): + super(DummyRequest, self).__init__(*args, **kwargs) + self.GET = {} + self.headers = {} + self.errors = cornice_errors.Errors() + self.authenticated_userid = 'bob' + self.validated = {} + self.matchdict = {} + self.response = mock.MagicMock(headers={}) + + class PrefixedRequestClass(webtest.app.TestRequest): @classmethod diff --git a/readinglist/tests/test_batch.py b/readinglist/tests/test_batch.py deleted file mode 100644 index 6dbeecc..0000000 --- a/readinglist/tests/test_batch.py +++ /dev/null @@ -1,8 +0,0 @@ -from .support import BaseWebTest, unittest - - -class BatchViewTest(BaseWebTest, unittest.TestCase): - - def test_returns_empty_response_if_empty_requests(self): - resp = self.app.post('/batch', {}, headers=self.headers) - self.assertEqual(resp.json, {}) diff --git a/readinglist/tests/test_views_batch.py b/readinglist/tests/test_views_batch.py new file mode 100644 index 0000000..01c41d7 --- /dev/null +++ b/readinglist/tests/test_views_batch.py @@ -0,0 +1,64 @@ +import colander + +from readinglist.views.batch import BatchRequestSchema, batch as batch_service +from readinglist.tests.support import BaseWebTest, unittest, DummyRequest + + +class BatchViewTest(BaseWebTest, unittest.TestCase): + + def test_requires_authentication(self): + self.app.post('/batch', {}, status=401) + + def test_returns_400_if_body_has_missing_requests(self): + self.app.post('/batch', {}, headers=self.headers, status=400) + + def test_returns_responses_if_schema_is_valid(self): + body = {'requests': []} + resp = self.app.post_json('/batch', body, headers=self.headers) + self.assertIn('responses', resp.json) + + def test_only_post_is_allowed(self): + self.app.get('/batch', headers=self.headers, status=405) + self.app.put('/batch', headers=self.headers, status=405) + self.app.patch('/batch', headers=self.headers, status=405) + self.app.delete('/batch', headers=self.headers, status=405) + + +class BatchSchemaTest(unittest.TestCase): + def setUp(self): + self.schema = BatchRequestSchema() + + def assertInvalid(self, payload): + self.assertRaises(colander.Invalid, self.schema.deserialize, payload) + + def test_requests_is_mandatory(self): + self.assertRaises({}) + + def test_unknown_attributes_are_dropped(self): + deserialized = self.schema.deserialize({'requests': [], 'unknown': 42}) + self.assertNotIn('unknown', deserialized) + + def test_list_of_requests_can_be_empty(self): + self.schema.deserialize({'requests': []}) + + def test_list_of_requests_must_be_a_list(self): + self.assertRaises({'requests': {}}) + + +class BatchServiceTest(unittest.TestCase): + def setUp(self): + self.method, self.view, self.options = batch_service.definitions[0] + + def post(self, validated): + request = DummyRequest() + request.validated = validated + return self.view(request) + + def test_returns_empty_list_of_responses_if_requests_empty(self): + result = self.post({'requests': []}) + self.assertEqual(result['responses'], []) + + def test_returns_one_response_per_request(self): + requests = [{}] + result = self.post({'requests': requests}) + self.assertEqual(len(result['responses']), len(requests)) diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py index a627828..46c32f7 100644 --- a/readinglist/views/batch.py +++ b/readinglist/views/batch.py @@ -1,9 +1,24 @@ +import colander from cornice import Service + batch = Service(name="batch", path='/batch', description="Batch operations") -@batch.post() +class BatchRequestSchema(colander.MappingSchema): + pass + + +class BatchPayloadSchema(colander.MappingSchema): + requests = colander.SchemaNode(colander.Sequence(), + BatchRequestSchema()) + + +@batch.post(permission='readonly', schema=BatchPayloadSchema) def post_batch(request): - return {} + responses = [] + + return { + 'responses': responses + } From 49e0ece5c147c098cec436333c0fc48ab726ba70 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Thu, 5 Feb 2015 18:49:06 +0100 Subject: [PATCH 04/11] Move HTTPInternalError to errors --- readinglist/errors.py | 21 +++++++++++++++++++-- readinglist/views/errors.py | 14 +++----------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/readinglist/errors.py b/readinglist/errors.py index ab59366..5888b20 100644 --- a/readinglist/errors.py +++ b/readinglist/errors.py @@ -1,7 +1,8 @@ import six from pyramid.config import global_registries from pyramid.httpexceptions import ( - HTTPServiceUnavailable as PyramidHTTPServiceUnavailable, HTTPBadRequest + HTTPServiceUnavailable as PyramidHTTPServiceUnavailable, HTTPBadRequest, + HTTPInternalServerError as PyramidHTTPInternalServerError ) from readinglist.utils import Enum, json @@ -41,7 +42,7 @@ def format_error(code, errno, error, message=None, info=None): class HTTPServiceUnavailable(PyramidHTTPServiceUnavailable): - """Return an HTTPServiceUnavailable formatted error.""" + """A HTTPServiceUnavailable formatted in JSON.""" def __init__(self, **kwargs): if 'body' not in kwargs: @@ -65,6 +66,22 @@ def __init__(self, **kwargs): super(HTTPServiceUnavailable, self).__init__(**kwargs) +class HTTPInternalServerError(PyramidHTTPInternalServerError): + """A HTTPInternalServerError formatted in JSON.""" + + def __init__(self, **kwargs): + kwargs.setdefault('body', format_error( + 500, + ERRORS.UNDEFINED, + "Internal Server Error", + "A programmatic error occured, developers have been informed.", + "https://github.com/mozilla-services/readinglist/issues/")) + + kwargs.setdefault('content_type', 'application/json') + + super(HTTPInternalServerError, self).__init__(**kwargs) + + def json_error(errors): """Return an HTTPError with the given status and message. diff --git a/readinglist/views/errors.py b/readinglist/views/errors.py index b07b783..f566bdd 100644 --- a/readinglist/views/errors.py +++ b/readinglist/views/errors.py @@ -3,8 +3,7 @@ from cornice.cors import ensure_origin from pyramid.httpexceptions import ( - HTTPForbidden, HTTPUnauthorized, HTTPNotFound, HTTPInternalServerError, - Response + HTTPForbidden, HTTPUnauthorized, HTTPNotFound, Response ) from pyramid.security import forget from pyramid.view import ( @@ -12,7 +11,7 @@ ) from readinglist import logger -from readinglist.errors import format_error, ERRORS +from readinglist.errors import format_error, ERRORS, HTTPInternalServerError def reapply_cors(request, response): @@ -89,13 +88,6 @@ def error(context, request): logger.exception(context) - response = HTTPInternalServerError( - body=format_error( - 500, - ERRORS.UNDEFINED, - "Internal Server Error", - "A programmatic error occured, developers have been informed.", - "https://github.com/mozilla-services/readinglist/issues/"), - content_type='application/json') + response = HTTPInternalServerError() return reapply_cors(request, response) From 27ce9bc1e4ca69b11a3c2f3e3956a448d61768ab Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Thu, 5 Feb 2015 18:49:22 +0100 Subject: [PATCH 05/11] Basic working batch endpoint (ref #2) --- readinglist/tests/support.py | 1 - readinglist/tests/test_views_batch.py | 111 ++++++++++++++++++++++++-- readinglist/views/batch.py | 83 ++++++++++++++++++- readinglist/views/hello.py | 6 +- 4 files changed, 191 insertions(+), 10 deletions(-) diff --git a/readinglist/tests/support.py b/readinglist/tests/support.py index 6441543..580fe1a 100644 --- a/readinglist/tests/support.py +++ b/readinglist/tests/support.py @@ -13,7 +13,6 @@ from readinglist.utils import random_bytes_hex - class DummyRequest(mock.MagicMock): def __init__(self, *args, **kwargs): super(DummyRequest, self).__init__(*args, **kwargs) diff --git a/readinglist/tests/test_views_batch.py b/readinglist/tests/test_views_batch.py index 01c41d7..8ffab13 100644 --- a/readinglist/tests/test_views_batch.py +++ b/readinglist/tests/test_views_batch.py @@ -1,6 +1,9 @@ +import json + import colander +import mock -from readinglist.views.batch import BatchRequestSchema, batch as batch_service +from readinglist.views.batch import BatchPayloadSchema, batch as batch_service from readinglist.tests.support import BaseWebTest, unittest, DummyRequest @@ -23,16 +26,69 @@ def test_only_post_is_allowed(self): self.app.patch('/batch', headers=self.headers, status=405) self.app.delete('/batch', headers=self.headers, status=405) + def test_responses_are_redirects_if_no_prefix(self): + request = {'path': '/'} + body = {'requests': [request]} + resp = self.app.post_json('/batch', body, headers=self.headers) + redirect = resp.json['responses'][0] + self.assertEqual(redirect['path'], '/') + self.assertEqual(redirect['status'], 307) + self.assertEqual(redirect['body'], '') + self.assertEqual(redirect['headers']['Location'], '/v0/') + + def test_responses_are_resolved_on_api_with_prefix(self): + request = {'path': '/v0/'} + body = {'requests': [request]} + resp = self.app.post_json('/batch', body, headers=self.headers) + hello = resp.json['responses'][0] + self.assertEqual(hello['path'], '/v0/') + self.assertEqual(hello['status'], 200) + self.assertEqual(hello['body']['hello'], 'readinglist') + self.assertIn('application/json', hello['headers']['Content-Type']) + + def test_empty_response_body_with_head(self): + request = {'path': '/v0/', 'method': 'HEAD'} + body = {'requests': [request]} + resp = self.app.post_json('/batch', body, headers=self.headers) + head = resp.json['responses'][0] + self.assertEqual(head['body'], '') + self.assertNotEqual(len(head['headers']), 0) + + def test_internal_errors_are_returned_within_responses(self): + request = {'path': '/v0/'} + body = {'requests': [request]} + + with mock.patch('readinglist.views.hello.get_eos') as mocked: + mocked.side_effect = AttributeError + resp = self.app.post_json('/batch', body, headers=self.headers) + + error = resp.json['responses'][0] + self.assertEqual(error['status'], 500) + self.assertEqual(error['body']['errno'], 999) + + def test_can_be_recursive(self): + requests = json.dumps({'requests': [{'path': '/v0/'}]}) + + request = {'method': 'POST', 'path': '/v0/batch', 'body': requests} + body = {'requests': [request]} + resp = self.app.post_json('/batch', body, headers=self.headers) + + hello = resp.json['responses'][0]['body']['responses'][0] + self.assertEqual(hello['body']['hello'], 'readinglist') + + def test_path_not_url_encoded(self): + pass + class BatchSchemaTest(unittest.TestCase): def setUp(self): - self.schema = BatchRequestSchema() + self.schema = BatchPayloadSchema() def assertInvalid(self, payload): self.assertRaises(colander.Invalid, self.schema.deserialize, payload) def test_requests_is_mandatory(self): - self.assertRaises({}) + self.assertInvalid({}) def test_unknown_attributes_are_dropped(self): deserialized = self.schema.deserialize({'requests': [], 'unknown': 42}) @@ -42,7 +98,52 @@ def test_list_of_requests_can_be_empty(self): self.schema.deserialize({'requests': []}) def test_list_of_requests_must_be_a_list(self): - self.assertRaises({'requests': {}}) + self.assertInvalid({'requests': {}}) + + def test_request_path_must_start_with_slash(self): + request = {'path': 'http://localhost'} + self.assertInvalid({'requests': [request]}) + + def test_request_method_must_be_known_uppercase_word(self): + request = {'path': '/', 'method': 'get'} + self.assertInvalid({'requests': [request]}) + + # + # headers + # + + def test_request_headers_should_be_strings(self): + headers = {'Accept': 3.14} + request = {'path': '/', 'headers': headers} + self.assertInvalid({'requests': [request]}) + + def test_request_headers_cannot_be_recursive(self): + headers = {'Accept': {'sub': 'dict'}} + request = {'path': '/', 'headers': headers} + self.assertInvalid({'requests': [request]}) + + def test_request_headers_are_preserved(self): + headers = {'Accept': 'audio/*'} + request = {'path': '/', 'headers': headers} + deserialized = self.schema.deserialize({'requests': [request]}) + self.assertEqual(deserialized['requests'][0]['headers']['Accept'], + 'audio/*') + + # + # body + # + + def test_body_is_an_arbitrary_string(self): + payload = '{"json": "payload"}' + request = {'path': '/', 'body': payload} + deserialized = self.schema.deserialize({'requests': [request]}) + self.assertEqual(deserialized['requests'][0]['body'], payload) + + def test_body_is_dropped_if_empty_string(self): + payload = '' + request = {'path': '/', 'body': payload} + deserialized = self.schema.deserialize({'requests': [request]}) + self.assertNotIn('body', deserialized['requests'][0]) class BatchServiceTest(unittest.TestCase): @@ -59,6 +160,6 @@ def test_returns_empty_list_of_responses_if_requests_empty(self): self.assertEqual(result['responses'], []) def test_returns_one_response_per_request(self): - requests = [{}] + requests = [{'path': '/'}] result = self.post({'requests': requests}) self.assertEqual(len(result['responses']), len(requests)) diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py index 46c32f7..22d447f 100644 --- a/readinglist/views/batch.py +++ b/readinglist/views/batch.py @@ -1,13 +1,41 @@ import colander from cornice import Service +from pyramid.request import Request +from pyramid import httpexceptions +import six +from readinglist import logger +from readinglist.errors import HTTPInternalServerError -batch = Service(name="batch", path='/batch', - description="Batch operations") + +valid_http_method = colander.OneOf(('GET', 'HEAD', 'DELETE', 'TRACE', + 'POST', 'PUT', 'PATCH')) + + +def string_values(node, cstruct): + """Validate that a ``colander.Mapping`` only has strings in its values. + + :warning: + + Should be associated to a ``colander.Mapping`` schema node. + """ + are_strings = [isinstance(v, six.string_types) for v in cstruct.values()] + if not all(are_strings): + error_msg = '%s contains non string value' % cstruct + raise colander.Invalid(node, error_msg) class BatchRequestSchema(colander.MappingSchema): - pass + method = colander.SchemaNode(colander.String(), + validator=valid_http_method, + missing=colander.drop) + path = colander.SchemaNode(colander.String(), + validator=colander.Regex('^/')) + headers = colander.SchemaNode(colander.Mapping(unknown='preserve'), + validator=string_values, + missing=colander.drop) + body = colander.SchemaNode(colander.String(), + missing=colander.drop) class BatchPayloadSchema(colander.MappingSchema): @@ -15,10 +43,59 @@ class BatchPayloadSchema(colander.MappingSchema): BatchRequestSchema()) +batch = Service(name="batch", path='/batch', + description="Batch operations") + + @batch.post(permission='readonly', schema=BatchPayloadSchema) def post_batch(request): responses = [] + for subrequest_spec in request.validated['requests']: + subrequest = build_request(request, subrequest_spec) + try: + subresponse = request.invoke_subrequest(subrequest) + except httpexceptions.HTTPException as e: + subresponse = e + except Exception as e: + logger.exception(e) + subresponse = HTTPInternalServerError() + + subresponse = build_response(subresponse, subrequest) + responses.append(subresponse) + return { 'responses': responses } + + +def build_request(original, dict_obj): + method = dict_obj.get('method') or 'GET' + path = dict_obj['path'] + headers = dict(original.headers) + headers.update(**dict_obj.get('headers') or {}) + + payload = dict_obj.get('body') or None + request = Request.blank(path=path, + headers=headers, + POST=payload, + method=method) + return request + + +def build_response(response, request): + dict_obj = {} + dict_obj['path'] = request.path + dict_obj['status'] = response.status_code + dict_obj['headers'] = dict(response.headers) + + body = '' + if request.method != 'HEAD': + # XXX : Pyramid should not have built response body! + try: + body = response.json + except ValueError: + pass + + dict_obj['body'] = body + return dict_obj diff --git a/readinglist/views/hello.py b/readinglist/views/hello.py index aee1598..8b26d1f 100644 --- a/readinglist/views/hello.py +++ b/readinglist/views/hello.py @@ -15,8 +15,12 @@ def get_hello(request): documentation="https://readinglist.rtfd.org/" ) - eos = request.registry.settings.get('readinglist.eos', '').strip() or None + eos = get_eos(request) if eos: data['eos'] = eos return data + + +def get_eos(request): + return request.registry.settings.get('readinglist.eos', '').strip() or None From 4daed266b0b369db9becd9a15eb0812f273ce73d Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Thu, 5 Feb 2015 19:02:27 +0100 Subject: [PATCH 06/11] @leplatrem review --- readinglist/views/batch.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py index 22d447f..0dc892d 100644 --- a/readinglist/views/batch.py +++ b/readinglist/views/batch.py @@ -53,6 +53,7 @@ def post_batch(request): for subrequest_spec in request.validated['requests']: subrequest = build_request(request, subrequest_spec) + try: subresponse = request.invoke_subrequest(subrequest) except httpexceptions.HTTPException as e: @@ -70,20 +71,33 @@ def post_batch(request): def build_request(original, dict_obj): + """ + Transform a dict object into a ``pyramid.request.Request`` object. + + :param original: the original batch request. + :param dict_obj: a dict object with the sub-request specifications. + """ method = dict_obj.get('method') or 'GET' path = dict_obj['path'] headers = dict(original.headers) headers.update(**dict_obj.get('headers') or {}) - payload = dict_obj.get('body') or None + request = Request.blank(path=path, headers=headers, POST=payload, method=method) + return request def build_response(response, request): + """ + Transform a ``pyramid.response.Response`` object into a serializable dict. + + :param response: a response object, returned by Pyramid. + :param request: the request that was used to get the response. + """ dict_obj = {} dict_obj['path'] = request.path dict_obj['status'] = response.status_code @@ -95,7 +109,7 @@ def build_response(response, request): try: body = response.json except ValueError: - pass - + body = response.body dict_obj['body'] = body + return dict_obj From 406b66159bcf60ff2bb7c63b650a97d14a179757 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 6 Feb 2015 15:01:33 +0100 Subject: [PATCH 07/11] @Natim review --- docs/batch.rst | 12 ++++++++---- readinglist/authentication.py | 1 - readinglist/tests/test_views_batch.py | 10 +++++----- readinglist/views/batch.py | 10 ++++++++-- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/docs/batch.rst b/docs/batch.rst index 24111a3..e3cf148 100644 --- a/docs/batch.rst +++ b/docs/batch.rst @@ -60,9 +60,6 @@ The response body is a list of all responses: :: { - "defaults": { - "path" : "/articles", - }, "responses": [ { "path" : "/articles/409", @@ -79,6 +76,7 @@ The response body is a list of all responses: }, { "status": 201, + "path" : "/articles", "body" : { "id": 411, "title": "MoFo", @@ -88,6 +86,7 @@ The response body is a list of all responses: }, { "status": 201, + "path" : "/articles", "body" : { "id": 412, "title": "MoCo", @@ -99,9 +98,14 @@ The response body is a list of all responses: ] +:warning: + + Since the requests bodies are necessarily mappings, posting arbitrary data + (*like raw text or binary*)is not supported. + :note: - The responses are not necessarily in the same order of the requests. + The responses are in the same order of the requests. Pros & Cons diff --git a/readinglist/authentication.py b/readinglist/authentication.py index 31b3cfd..90d694b 100644 --- a/readinglist/authentication.py +++ b/readinglist/authentication.py @@ -87,7 +87,6 @@ def permits(self, context, principals, permission): Authenticated users only are allowed. """ PERMISSIONS = { - 'batch': Authenticated, 'readonly': Authenticated, 'readwrite': Authenticated, } diff --git a/readinglist/tests/test_views_batch.py b/readinglist/tests/test_views_batch.py index 8ffab13..7a544de 100644 --- a/readinglist/tests/test_views_batch.py +++ b/readinglist/tests/test_views_batch.py @@ -1,5 +1,4 @@ -import json - +# -*- coding: utf-8 -*- import colander import mock @@ -9,8 +8,9 @@ class BatchViewTest(BaseWebTest, unittest.TestCase): - def test_requires_authentication(self): - self.app.post('/batch', {}, status=401) + def test_does_not_require_authentication(self): + body = {'requests': []} + self.app.post_json('/batch', body) def test_returns_400_if_body_has_missing_requests(self): self.app.post('/batch', {}, headers=self.headers, status=400) @@ -67,7 +67,7 @@ def test_internal_errors_are_returned_within_responses(self): self.assertEqual(error['body']['errno'], 999) def test_can_be_recursive(self): - requests = json.dumps({'requests': [{'path': '/v0/'}]}) + requests = {'requests': [{'path': '/v0/'}]} request = {'method': 'POST', 'path': '/v0/batch', 'body': requests} body = {'requests': [request]} diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py index 0dc892d..2b72fb5 100644 --- a/readinglist/views/batch.py +++ b/readinglist/views/batch.py @@ -34,7 +34,7 @@ class BatchRequestSchema(colander.MappingSchema): headers = colander.SchemaNode(colander.Mapping(unknown='preserve'), validator=string_values, missing=colander.drop) - body = colander.SchemaNode(colander.String(), + body = colander.SchemaNode(colander.Mapping(unknown='preserve'), missing=colander.drop) @@ -47,7 +47,7 @@ class BatchPayloadSchema(colander.MappingSchema): description="Batch operations") -@batch.post(permission='readonly', schema=BatchPayloadSchema) +@batch.post(schema=BatchPayloadSchema) def post_batch(request): responses = [] @@ -83,6 +83,12 @@ def build_request(original, dict_obj): headers.update(**dict_obj.get('headers') or {}) payload = dict_obj.get('body') or None + # Payload is always a dict (from ``BatchRequestSchema.body``). + # Send it as JSON for subrequests. + if isinstance(payload, dict): + headers['Content-Type'] = 'application/json; charset=utf-8' + payload = json.dumps(payload) + request = Request.blank(path=path, headers=headers, POST=payload, From badd5d2d857ca2dca432a428052db8008d9ccaff Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 6 Feb 2015 15:06:56 +0100 Subject: [PATCH 08/11] URL quote and encoding tests (ref #2) --- readinglist/tests/test_views_batch.py | 67 +++++++++++++++++++++------ readinglist/views/batch.py | 7 ++- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/readinglist/tests/test_views_batch.py b/readinglist/tests/test_views_batch.py index 7a544de..8b4afbb 100644 --- a/readinglist/tests/test_views_batch.py +++ b/readinglist/tests/test_views_batch.py @@ -76,9 +76,6 @@ def test_can_be_recursive(self): hello = resp.json['responses'][0]['body']['responses'][0] self.assertEqual(hello['body']['hello'], 'readinglist') - def test_path_not_url_encoded(self): - pass - class BatchSchemaTest(unittest.TestCase): def setUp(self): @@ -133,27 +130,21 @@ def test_request_headers_are_preserved(self): # body # - def test_body_is_an_arbitrary_string(self): - payload = '{"json": "payload"}' + def test_body_is_an_arbitrary_mapping(self): + payload = {"json": "payload"} request = {'path': '/', 'body': payload} deserialized = self.schema.deserialize({'requests': [request]}) self.assertEqual(deserialized['requests'][0]['body'], payload) - def test_body_is_dropped_if_empty_string(self): - payload = '' - request = {'path': '/', 'body': payload} - deserialized = self.schema.deserialize({'requests': [request]}) - self.assertNotIn('body', deserialized['requests'][0]) - class BatchServiceTest(unittest.TestCase): def setUp(self): self.method, self.view, self.options = batch_service.definitions[0] + self.request = DummyRequest() def post(self, validated): - request = DummyRequest() - request.validated = validated - return self.view(request) + self.request.validated = validated + return self.view(self.request) def test_returns_empty_list_of_responses_if_requests_empty(self): result = self.post({'requests': []}) @@ -163,3 +154,51 @@ def test_returns_one_response_per_request(self): requests = [{'path': '/'}] result = self.post({'requests': requests}) self.assertEqual(len(result['responses']), len(requests)) + + def test_relies_on_pyramid_invoke_subrequest(self): + self.post({'requests': [{'path': '/'}]}) + self.assertTrue(self.request.invoke_subrequest.called) + + def test_returns_requests_path_in_responses(self): + result = self.post({'requests': [{'path': '/'}]}) + self.assertEqual(result['responses'][0]['path'], '/') + + def test_subrequests_are_GET_by_default(self): + self.post({'requests': [{'path': '/'}]}) + subrequest, = self.request.invoke_subrequest.call_args[0] + self.assertEqual(subrequest.method, 'GET') + + def test_original_request_headers_are_passed_to_subrequests(self): + self.request.headers['Authorization'] = 'Basic ertyfghjkl' + self.post({'requests': [{'path': '/'}]}) + subrequest, = self.request.invoke_subrequest.call_args[0] + self.assertIn('Basic', subrequest.headers['Authorization']) + + def test_subrequests_body_are_json_serialized(self): + request = {'path': '/', 'body': {'json': 'payload'}} + self.post({'requests': [request]}) + subrequest, = self.request.invoke_subrequest.call_args[0] + self.assertEqual(subrequest.body.decode('utf8'), + '{"json": "payload"}') + + def test_subrequests_body_have_json_content_type(self): + self.request.headers['Content-Type'] = 'text/xml' + request = {'path': '/', 'body': {'json': 'payload'}} + self.post({'requests': [request]}) + subrequest, = self.request.invoke_subrequest.call_args[0] + self.assertIn('application/json', + subrequest.headers['Content-Type']) + + def test_subrequests_body_have_utf8_charset(self): + request = {'path': '/', 'body': {'json': u"😂"}} + self.post({'requests': [request]}) + subrequest, = self.request.invoke_subrequest.call_args[0] + self.assertIn('charset=utf-8', subrequest.headers['Content-Type']) + self.assertEqual(subrequest.body.decode('utf8'), + '{"json": "\\ud83d\\ude02"}') + + def test_subrequests_paths_are_url_encoded(self): + request = {'path': u'/ð ® ©'} + self.post({'requests': [request]}) + subrequest, = self.request.invoke_subrequest.call_args[0] + self.assertEqual(subrequest.path, '/%C3%B0%20%C2%AE%20%C2%A9') diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py index 2b72fb5..04b2a01 100644 --- a/readinglist/views/batch.py +++ b/readinglist/views/batch.py @@ -1,8 +1,11 @@ +import json + import colander from cornice import Service from pyramid.request import Request from pyramid import httpexceptions import six +from six.moves.urllib import parse as urlparse from readinglist import logger from readinglist.errors import HTTPInternalServerError @@ -77,8 +80,10 @@ def build_request(original, dict_obj): :param original: the original batch request. :param dict_obj: a dict object with the sub-request specifications. """ - method = dict_obj.get('method') or 'GET' path = dict_obj['path'] + path = urlparse.quote(path.encode('utf8')) + + method = dict_obj.get('method') or 'GET' headers = dict(original.headers) headers.update(**dict_obj.get('headers') or {}) payload = dict_obj.get('body') or None From a9bed9122b1e72d98cc05915cdc5c2dd538b673b Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 6 Feb 2015 15:21:21 +0100 Subject: [PATCH 09/11] Limit number of requests supported (ref #2) --- config/readinglist.ini | 1 + readinglist/tests/test_views_batch.py | 19 +++++++++++++++++++ readinglist/views/batch.py | 10 +++++++++- 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/config/readinglist.ini b/config/readinglist.ini index 16133c0..151a152 100644 --- a/config/readinglist.ini +++ b/config/readinglist.ini @@ -8,6 +8,7 @@ readinglist.eos = # readinglist.basic_auth_backdoor = true # readinglist.backoff = 10 readinglist.userid_hmac_secret = b4c96a8692291d88fe5a97dd91846eb4 +# readinglist.batch_max_requests = 25 fxa-oauth.client_id = fxa-oauth.client_secret = diff --git a/readinglist/tests/test_views_batch.py b/readinglist/tests/test_views_batch.py index 8b4afbb..09c89bc 100644 --- a/readinglist/tests/test_views_batch.py +++ b/readinglist/tests/test_views_batch.py @@ -141,6 +141,7 @@ class BatchServiceTest(unittest.TestCase): def setUp(self): self.method, self.view, self.options = batch_service.definitions[0] self.request = DummyRequest() + self.request.registry = mock.Mock(settings={}) def post(self, validated): self.request.validated = validated @@ -202,3 +203,21 @@ def test_subrequests_paths_are_url_encoded(self): self.post({'requests': [request]}) subrequest, = self.request.invoke_subrequest.call_args[0] self.assertEqual(subrequest.path, '/%C3%B0%20%C2%AE%20%C2%A9') + + def test_number_of_requests_is_not_limited_by_default(self): + requests = {} + for i in range(500): + requests.setdefault('requests', []).append({'path': '/'}) + self.post(requests) + + def test_return_400_if_number_of_requests_is_greater_than_settings(self): + self.request.registry.settings['readinglist.batch_max_requests'] = 25 + + requests = {} + for i in range(26): + requests.setdefault('requests', []).append({'path': '/'}) + result = self.post(requests) + + self.assertEqual(self.request.errors[0]['description'], + 'Number of requests is limited to 25') + self.assertIsNone(result) # rest of view not executed diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py index 04b2a01..29dfc70 100644 --- a/readinglist/views/batch.py +++ b/readinglist/views/batch.py @@ -52,9 +52,17 @@ class BatchPayloadSchema(colander.MappingSchema): @batch.post(schema=BatchPayloadSchema) def post_batch(request): + requests = request.validated['requests'] + + limit = request.registry.settings.get('readinglist.batch_max_requests') + if limit and len(requests) > limit: + error_msg = 'Number of requests is limited to %s' % limit + request.errors.add('body', 'requests', error_msg) + return + responses = [] - for subrequest_spec in request.validated['requests']: + for subrequest_spec in requests: subrequest = build_request(request, subrequest_spec) try: From 6c11de78540aeaba678157d43dec6604a10d10a0 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 6 Feb 2015 15:41:04 +0100 Subject: [PATCH 10/11] Do not allow recursive calls (ref #2) --- readinglist/tests/test_views_batch.py | 9 +++------ readinglist/views/batch.py | 13 ++++++++++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/readinglist/tests/test_views_batch.py b/readinglist/tests/test_views_batch.py index 09c89bc..1f5d5db 100644 --- a/readinglist/tests/test_views_batch.py +++ b/readinglist/tests/test_views_batch.py @@ -66,15 +66,12 @@ def test_internal_errors_are_returned_within_responses(self): self.assertEqual(error['status'], 500) self.assertEqual(error['body']['errno'], 999) - def test_can_be_recursive(self): + def test_batch_cannot_be_recursive(self): requests = {'requests': [{'path': '/v0/'}]} - request = {'method': 'POST', 'path': '/v0/batch', 'body': requests} body = {'requests': [request]} - resp = self.app.post_json('/batch', body, headers=self.headers) - - hello = resp.json['responses'][0]['body']['responses'][0] - self.assertEqual(hello['body']['hello'], 'readinglist') + resp = self.app.post_json('/batch', body, status=400) + self.assertIn('Recursive', resp.json['message']) class BatchSchemaTest(unittest.TestCase): diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py index 29dfc70..ab9721c 100644 --- a/readinglist/views/batch.py +++ b/readinglist/views/batch.py @@ -8,7 +8,7 @@ from six.moves.urllib import parse as urlparse from readinglist import logger -from readinglist.errors import HTTPInternalServerError +from readinglist import errors valid_http_method = colander.OneOf(('GET', 'HEAD', 'DELETE', 'TRACE', @@ -47,7 +47,8 @@ class BatchPayloadSchema(colander.MappingSchema): batch = Service(name="batch", path='/batch', - description="Batch operations") + description="Batch operations", + error_handler=errors.json_error) @batch.post(schema=BatchPayloadSchema) @@ -60,6 +61,12 @@ def post_batch(request): request.errors.add('body', 'requests', error_msg) return + is_recursive = lambda req: batch.path in req['path'] + if any([is_recursive(req) for req in requests]): + error_msg = 'Recursive call on %s endpoint is forbidden.' % batch.path + request.errors.add('body', 'requests', error_msg) + return + responses = [] for subrequest_spec in requests: @@ -71,7 +78,7 @@ def post_batch(request): subresponse = e except Exception as e: logger.exception(e) - subresponse = HTTPInternalServerError() + subresponse = errors.HTTPInternalServerError() subresponse = build_response(subresponse, subrequest) responses.append(subresponse) From 52e7dce859a1f0cfef01c4e6042e4c4cd3d053b7 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Fri, 6 Feb 2015 17:34:53 +0100 Subject: [PATCH 11/11] Support default values for batch requests (fixes #2) --- readinglist/tests/test_views_batch.py | 56 +++++++++++++++++++++++++++ readinglist/views/batch.py | 24 ++++++++++++ 2 files changed, 80 insertions(+) diff --git a/readinglist/tests/test_views_batch.py b/readinglist/tests/test_views_batch.py index 1f5d5db..faa968d 100644 --- a/readinglist/tests/test_views_batch.py +++ b/readinglist/tests/test_views_batch.py @@ -133,6 +133,62 @@ def test_body_is_an_arbitrary_mapping(self): deserialized = self.schema.deserialize({'requests': [request]}) self.assertEqual(deserialized['requests'][0]['body'], payload) + # + # defaults + # + + def test_defaults_must_be_a_mapping_if_specified(self): + request = {'path': '/'} + batch_payload = {'requests': [request], 'defaults': 42} + self.assertInvalid(batch_payload) + + def test_defaults_must_be_a_request_schema_if_specified(self): + request = {'path': '/'} + defaults = {'body': 3} + batch_payload = {'requests': [request], 'defaults': defaults} + self.assertInvalid(batch_payload) + + def test_unknown_defaults_are_ignored_silently(self): + request = {'path': '/'} + defaults = {'foo': 'bar'} + batch_payload = {'requests': [request], 'defaults': defaults} + result = self.schema.deserialize(batch_payload) + self.assertNotIn('foo', result['requests'][0]) + + def test_defaults_can_be_specified_empty(self): + request = {'path': '/'} + defaults = {} + batch_payload = {'requests': [request], 'defaults': defaults} + self.schema.deserialize(batch_payload) + + def test_defaults_values_are_applied_to_requests(self): + request = {'path': '/'} + defaults = {'body': {'json': 'payload'}} + batch_payload = {'requests': [request], 'defaults': defaults} + result = self.schema.deserialize(batch_payload) + self.assertEqual(result['requests'][0]['body'], {'json': 'payload'}) + + def test_defaults_values_do_not_overwrite_requests_values(self): + request = {'path': '/', 'headers': {'Authorization': 'me'}} + defaults = {'headers': {'Authorization': 'you', 'Accept': '*/*'}} + batch_payload = {'requests': [request], 'defaults': defaults} + result = self.schema.deserialize(batch_payload) + self.assertEqual(result['requests'][0]['headers'], + {'Authorization': 'me', 'Accept': '*/*'}) + + def test_defaults_values_can_be_path(self): + request = {} + defaults = {'path': '/'} + batch_payload = {'requests': [request], 'defaults': defaults} + result = self.schema.deserialize(batch_payload) + self.assertEqual(result['requests'][0]['path'], '/') + + def test_defaults_values_for_path_must_start_with_slash(self): + request = {} + defaults = {'path': 'http://localhost'} + batch_payload = {'requests': [request], 'defaults': defaults} + self.assertInvalid(batch_payload) + class BatchServiceTest(unittest.TestCase): def setUp(self): diff --git a/readinglist/views/batch.py b/readinglist/views/batch.py index ab9721c..2b6c97e 100644 --- a/readinglist/views/batch.py +++ b/readinglist/views/batch.py @@ -45,6 +45,30 @@ class BatchPayloadSchema(colander.MappingSchema): requests = colander.SchemaNode(colander.Sequence(), BatchRequestSchema()) + def deserialize(self, cstruct=colander.null): + """Preprocess received data + """ + if cstruct is colander.null: + return colander.null + + # See if defaults was specified in payload + defaults = cstruct.get('defaults', {}) + if isinstance(defaults, dict): + defaults.setdefault('path', '/') + defaults = BatchRequestSchema().deserialize(defaults) + + # Fill requests values with defaults + requests = cstruct.get('requests', []) + for request in requests: + for k, v in defaults.items(): + if k == 'headers': + for i, j in v.items(): + request.get(k, {}).setdefault(i, j) + else: + request.setdefault(k, v) + + return super(BatchPayloadSchema, self).deserialize(cstruct) + batch = Service(name="batch", path='/batch', description="Batch operations",