-
Notifications
You must be signed in to change notification settings - Fork 6
Implement batch api #93
Changes from all commits
92f984e
3c93d4e
b88cc2d
49e0ece
27ce9bc
4daed26
406b661
badd5d2
a9bed91
6c11de7
52e7dce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
################ | ||
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 | ||
|
||
:: | ||
|
||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we provide examples, we should do it with HTTPie so that it's easier for clients to reproduce. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will be adressed in #101 |
||
"defaults": { | ||
"method" : "POST", | ||
"path" : "/articles", | ||
"headers" : { | ||
... | ||
} | ||
}, | ||
"requests": [ | ||
{ | ||
"body" : { | ||
"title": "MoFo", | ||
"url" : "http://mozilla.org" | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: update this to put body as string There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a requirements? It seems strange to me to have JSON encoded string inside a JSON. I rather prefer to have a nested object instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you're right. But we should support both then (suppose we want to support xml or form encoded payloads) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well we don't :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I concur here: we currently don't and in case we want to, we can re-assess this formalism to include arbitrary strings. |
||
}, | ||
{ | ||
"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: | ||
|
||
:: | ||
|
||
{ | ||
"responses": [ | ||
{ | ||
"path" : "/articles/409", | ||
"status": 200, | ||
"body" : { | ||
"id": 409, | ||
"url": "...", | ||
... | ||
"read_position" : 3477 | ||
}, | ||
"headers": { | ||
... | ||
} | ||
}, | ||
{ | ||
"status": 201, | ||
"path" : "/articles", | ||
"body" : { | ||
"id": 411, | ||
"title": "MoFo", | ||
"url" : "http://mozilla.org", | ||
... | ||
}, | ||
}, | ||
{ | ||
"status": 201, | ||
"path" : "/articles", | ||
"body" : { | ||
"id": 412, | ||
"title": "MoCo", | ||
"url" : "http://mozilla.com", | ||
... | ||
}, | ||
}, | ||
] | ||
] | ||
|
||
|
||
:warning: | ||
|
||
Since the requests bodies are necessarily mappings, posting arbitrary data | ||
(*like raw text or binary*)is not supported. | ||
|
||
:note: | ||
|
||
The responses are in the same order of the requests. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe this is english enough :)
|
||
|
||
|
||
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 <http://neo4j.com/docs/milestone/rest-api-batch-ops.html>`_, `Facebook <https://developers.facebook.com/docs/graph-api/making-multiple-requests>`_, `Parse <ttps://parse.com/docs/rest#objects-batch>`_) | ||
* 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ Contents: | |
model | ||
authentication | ||
api | ||
batch | ||
utilities | ||
timestamps | ||
versionning | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Miam 👍 |
||
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While reading the code (without more information), I found it hard to unsderstand what this was for. We probably should rename this argument to
default_values
ordefault_request_values
, that would make it clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or
request_defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan to be honest :(
defaults
is fine IMO :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the docs it's fine, but the name of the argument doesn't convey much more meaning, and I had to read the docs to understand what the purpose was.