736132 - HTTP compliance: Ensure all PUT methods return 201 when a new record is created in the database

Status: VERIFIED FIXED

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

Description

[:Atoll]

We currently return 200 when a new row is created in the database, which is illegal.

9.6 PUT

If a new resource is created, the origin server MUST inform the user agent via the 201 (Created) response. If an existing resource is modified, either the 200 (OK) or 204 (No Content) response codes SHOULD be sent to indicate successful completion of the request.

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: patch to return 201 when a new item is created

Here's the necessary code.  Also needs a doc update before this bug can be closed.

Comment 2

[Toby Elliott [:telliott]]

Comment on attachment 607845 [details] [diff] [review]
patch to return 201 when a new item is created

Having to do two calls here is frowny, but at least they're very fast (and live in production right now, so clearly OK). Otherwise, looks good.

Comment 3

[Ryan Kelly [:rfkelly]]

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

Leaving bug open as a reminder to do the docs patch.

Comment 4

[Ryan Kelly [:rfkelly]]

Attachment: patch to docs describing 201-created response

Here's the corresponding docs patch for posterity

Comment 5

[Ryan Kelly [:rfkelly]]

Closing this bug since it now works as required by the spec.  The ability to differentiate "created" vs "updated" with a single database call will come about when we implement "on duplicate key" handling as described in Bug 691315.

Comment 6

[James Bonacci [:jbonacci]]

:atoll or :rfkelly - what QA work is needed here to verify this fix (or is any needed)?

Comment 7

[:Atoll]

(In reply to James Bonacci [:jbonacci] from comment #6)
> :atoll or :rfkelly - what QA work is needed here to verify this fix (or is
> any needed)?

I think you will need to test the PUT api calls and ensure they return 201, rather than 200, in various scenarios.

:rfkelly would be able to indicate which scenarios.

Comment 8

[Ryan Kelly [:rfkelly]]

(In reply to James Bonacci [:jbonacci] from comment #6)
> :atoll or :rfkelly - what QA work is needed here to verify this fix (or is
> any needed)?

The API only has a single PUT request, for uploading a BSO.  The patch for this bug includes an API testcase that exercises it.  So, if you're happy that the test looks right, and the functional testsuite continues to run OK, then we should be all good.

Comment 9

[James Bonacci [:jbonacci]]

Indeed, a new pull of the Sync 2.0 and AITC 1.0 code yields clean functional test runs.