784599 - Allow POSTing BSOs without payloads, to update ttl

Status: RESOLVED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Ryan Kelly [:rfkelly]]

Currently the Sync2.0 spec forbids you from POSTing bsos without a payload field, or from PUTing a bso without payload field unless it already exists in the database.  Since this functionality is useful for bulk-updating ttls, I think we should allow it.

Instead of being an error, payload-less updates to a BSO that does not exist should just default it to an empty payload.

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: docs patch to permit bulk updates of ttls

Docs patch to permit bulk updating of ttls without re-submitting the payloads.  This makes it explicit that any BSO created without a payload will get the empty string by default, and it explicitly calls out that submitting partial BSO data is permitted in both PUT and POST requests.

To be clear, these definitions mean that the following request:

    POST /storage/collection
    [{"id": "some_nonexistent_bso", "ttl": 42}]

will result in the BSO being created and having an empty payload.

It may be better to return an error in this case.  But it would be tricky to do that efficiently when you post a whole bunch of bsos, some of which exist and some of which should error out.  Defaulting to an empty payload makes my job much easier.

Comment 2

[Ryan Kelly [:rfkelly]]

Attachment: code patch to permit bulk updates of ttls

Code patch to implement this functionality.  Actually it already works after the database backend refactor, all I had to do was set a default for the "payload" column and add some tests.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 657985 [details] [diff] [review]
docs patch to permit bulk updates of ttls

Review of attachment 657985 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on POST. r- on PUT. I can be convinced otherwise.

::: source/storage/apis-2.0.rst
@@ +263,5 @@
> +
> +    If the target BSO already exists then it will be updated with the data
> +    from the request body.  Fields that are not provided in the request body
> +    will not be overwritten, so it is possible to e.g. update the `ttl` field
> +    of a BSO without re-submitting its `payload`.

This is a violation of HTTP PUT semantics. See https://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-20#section-2.3.5

Essentially, a PUT is supposed to replace the server resource with what the client provided. Granted, we are already violating PUT semantics today by having the server create the created property (something not provided by the client). But, I think this violation is much more severe.

@@ +299,5 @@
> +    body will not be overwritten, so it is possible to e.g. update the `ttl`
> +    field on a batch of BSOs without re-submitting their payloads.
> +
> +    This request returns an object with details of success or failure for each
> +    each BSO.  It will have the following keys:

Using POST to update the TTLs is acceptable within the defined semantics of HTTP.

Comment 4

[Ryan Kelly [:rfkelly]]

(In reply to Gregory Szorc [:gps] from comment #3)
> ::: source/storage/apis-2.0.rst
> @@ +263,5 @@
> > +
> > +    If the target BSO already exists then it will be updated with the data
> > +    from the request body.  Fields that are not provided in the request body
> > +    will not be overwritten, so it is possible to e.g. update the `ttl` field
> > +    of a BSO without re-submitting its `payload`.
> 
> This is a violation of HTTP PUT semantics. See
> https://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-20#section-2.3.5
> 
> Essentially, a PUT is supposed to replace the server resource with what the
> client provided.

Fair, but for completeness I'll note that this behaviour is already present in the sync1.1 API.

Since we need to keep at least some server-defined fields here, we could consider just changing this to a POST instead of PUT.

Comment 5

[Ryan Kelly [:rfkelly]]

Attachment: docs patch to use POST for all bso updates

Here's what it would look like with the PUT replaced by a POST.

I'm open to having POST /storage/collection as the only way to do ttl updates if you feel strongly about it.  To me it feels like a strange asymmetry to offer this in one place and not the other.

Comment 6

[Ryan Kelly [:rfkelly]]

Comment on attachment 662799 [details] [diff] [review]
docs patch to use POST for all bso updates

Sounds like :gps is pretty busy at the moment, looping in :rnewman for another opinion.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 662799 [details] [diff] [review]
docs patch to use POST for all bso updates

Review of attachment 662799 [details] [diff] [review]:
-----------------------------------------------------------------

I'm only passionate about no PUT for the "update timestamp" operation. If we want to upload individual BSOs with PUT, I'm still OK with that (despite the "created" field violating PUT semantics - I think).

Comment 8

[Richard Newman [:rnewman]]

My opinion aligns with Greg's: I think it's acceptable for PUT to have server-specified side-effects, and it's a meaningful verb, so I'd like to keep it for uploading individual records.

I don't think the PUT handler should allow TTL bumping.


But then, the TTL bumping is kinda icky anyway. "Hey, if you don't give us enough details, we'll just merge the two".

(And there's a hole in the semantics. How do I modify a record to have the server's default TTL? I can't, because 'blank' now means 'existing value'. PUT has a place.)


POST is certainly the right way to do merging, but perhaps a cleaner way to do this kind of modification is to either:

* Allow a POST to a collection-related URI with IDs to bump, making the operation explicit:
    POST /storage/bookmarks?refresh=abcde,ghijk&ttl=12345

* Reify the fields of a record.
    POST /storage/bookmarks/abcde/ttl?value=12345        or
    POST /storage/bookmarks/abcde?field=ttl&value=12345

That is, treat the times as server resources, and make explicit the purpose of the operation.


Detailed review:

>     The request URL does not support the specific request method.  For example,
>-    attempting a PUT request to /info/quota would produce a 405 response.
>+    attempting a POST request to /info/quota would produce a 405 response.

This still makes sense as PUT… over-zealous find-and-replace?

Comment 9

[Ryan Kelly [:rfkelly]]

(In reply to Richard Newman [:rnewman] from comment #8)
> My opinion aligns with Greg's: I think it's acceptable for PUT to have
> server-specified side-effects, and it's a meaningful verb, so I'd like to
> keep it for uploading individual records.

OK, makes sense.  PUT stays.
 
> But then, the TTL bumping is kinda icky anyway. "Hey, if you don't give us
> enough details, we'll just merge the two".
> 
> (And there's a hole in the semantics. How do I modify a record to have the
> server's default TTL? I can't, because 'blank' now means 'existing value'.
> PUT has a place.)

Hmm, this is a bit of a hole in the protocol as a whole, as it doesn't say anything about a "default TTL".  I think it's implied that items without an explicit ttl will never expire, but we should be explicit about that.

(In code, the server *does* set a default ttl of some far-future timestamp in order to simulate "never expires")

I think that POSTing {"ttl": null} makes sense for resetting fields to the server's default value, but this could be criticised as rather ad-hoc. 

I'll have to think about the alternative proposals a little more before responding.

> >     The request URL does not support the specific request method.  For example,
> >-    attempting a PUT request to /info/quota would produce a 405 response.
> >+    attempting a POST request to /info/quota would produce a 405 response.
> 
> This still makes sense as PUT… over-zealous find-and-replace?

Intentional, since it would have been the only occurrence of PUT left in the document.  But yeah, now that you point it out, it is still semantically correct.  

Moot now anyway since PUT stays.

Comment 10

[Ryan Kelly [:rfkelly]]

Attachment: updated docs patch defining partial BSO updates

OK, here's another try at this.  There are now three relevant API calls:

   PUT /storage/collection/id:   create or replace, needs full json body
   POST /storage/collection/id:  create or update, body can be partial
   POST /storage/collection:     same as a POST for each individual item

It also clarifies what should happen if there is no TTL specified for an item.
In my subjective opinion, updating by POSTing a JSON object with just the fields you want to write seems like the cleanest option here.

One thing you can't express in this setup is "update ttls for the following BSOs iff they exist".  If you try to update an item that does not exist, it will be created automatically.  If this is a problem, we'll have to think harder about this functionality and how to implement it effectively on the server.

Comment 11

[Ryan Kelly [:rfkelly]]

Docs patch committed in https://github.com/mozilla-services/docs/commit/293136f32dc50519c1255db957c15a2327154a0a

Will need another code patch to update for latest changes.

Comment 12

[Ryan Kelly [:rfkelly]]

Attachment: code patch to allow ttl updates via POST

Re-doing the code based on above discussion.  As before, it's mostly tests with some minimal code changes:

  * allow POST as well as PUT on a single item
  * provide default payload if not specified in request body
  * forbid sending partial data via PUT

Comment 13

[Ryan Kelly [:rfkelly]]

Attachment: code patch to allow ttl updates via POST

Tweaked code for the refactoring in Bug 812432 (which already contains @item.put and @item.post as view functions, so it just needs the database tweaks and tests)

Comment 14

[Toby Elliott [:telliott]]

Comment on attachment 685857 [details] [diff] [review]
code patch to allow ttl updates via POST

Yeah, I just checked, and most of the interesting work in this bug was already folded into the refactor. Tests are good.

Comment 15

[Ryan Kelly [:rfkelly]]

Attachment: patch to avoid resetting ttls when they're not specified in the POST body

Committed in https://github.com/mozilla-services/server-syncstorage/commit/e93085a860b63b1cc549cdb00e7f0e7ab477cc44

However, Tarek's review of Bug 812432 pointed out some weirdness that translates into a bug here.  If you do not specify a ttl when doing a POST, it currently gets reset to the default MAX_TTL rather being left unchanged.

Additional patch adds an explicit test for this scenario, and changes the code to fix it.

Comment 16

[Ryan Kelly [:rfkelly]]

https://github.com/mozilla-services/server-syncstorage/commit/aa77ded505d6eec1568004cc25073bf932bc28fc