784592 - Use cornice-format error responses in Sync2.0

Status: RESOLVED FIXED

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

Description

[Ryan Kelly [:rfkelly]]

Sync2.0 has inherited the numeric-error-code JSON responses from Sync1.1.  Unfortunately most of them are now either irrelevant (e.g. ERROR_INVALID_CAPTCHA, ERROR_WEAK_PASSWORD) or are subsumed by HTTP Response Codes (e.g. ERROR_ILLEGAL_METHOD).

There are also new error cases for which none of the existing codes make sense, such a reporting malformed headers per Bug 735039.

What do you think about standardizing on cornice-format errors with terse human-readable descriptions for all non-2XX response bodies?  I recall talking about this on the mailing list ages ago but not reaching an actionable conclusion.

Comment 1

[Gregory Szorc [:gps]]

Yes, please! Just as long as there is a way for machines to "switch" on the error codes.

Comment 2

[Ryan Kelly [:rfkelly]]

(In reply to Gregory Szorc [:gps] from comment #1)
> Just as long as there is a way for machines to "switch" on the error codes.

So you want a numeric error code that's more specific than the HTTP response code?  Makes sense.  If you have a couple of specific examples in mind please post them to ensure that we're on the same page here.

Comment 3

[Ryan Kelly [:rfkelly]]

Ah right, the conditions-required bug you filed previously is a good example here.  Adding Bug 783598 as a blocker.

Comment 4

[Ryan Kelly [:rfkelly]]

Attachment: docs patch specifying cornice-format error responses

Attaching initial docs patch to define this.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 660307 [details] [diff] [review]
docs patch specifying cornice-format error responses

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

This looks good.

I'm still a bit confused on where we see Cornice errors vs other errors. Specifically, I want to know in what circumstances a perfectly implemented client will encounter a Cornice error. I /think/ that may be never: Cornice errors will only be seen if the client is not following the protocol or if some kind of transport or server failure corrupted the request somehow. If this is the case, great. Clients will just log Cornice errors and take that as a sign they are buggy.

If this is not the case and we could see Cornice errors as part of normal operations, then we need to talk some more. Basically, if a Cornice error could represent a recoverable error scenario, I would like a clearly enumerated/differentiated machine-readable error code/enumeration/type so clients don't treat the error as unrecoverable.

::: source/storage/apis-2.0.rst
@@ +643,5 @@
> +The top-level JSON object in the response will contain the following key-value
> +pairs:
> +
> +  * **status**:  the constant string "error".
> +  * **errors**:  a list of JSON objects, each describing on particular

"on particular"

@@ +654,5 @@
> +    of the request; one of "querystring", "header" or "body".
> +  * **name**:  a string giving the name of the offending component of the
> +    request; for example the name of a specific header.
> +  * **reason**:  a string identifying the cause of the error; one of
> +    missing", "invalid" or "unexpected".

missing quote before "missing"

Comment 6

[Ryan Kelly [:rfkelly]]

(In reply to Gregory Szorc [:gps] from comment #5)
> If this is not the case and we could see Cornice errors as part of normal
> operations, then we need to talk some more. Basically, if a Cornice error
> could represent a recoverable error scenario, I would like a clearly
> enumerated/differentiated machine-readable error code/enumeration/type so
> clients don't treat the error as unrecoverable.

Bug 785045 is relevant here, as it introduces a response that perfectly-implemented clients might still encounter.

Comment 7

[Ryan Kelly [:rfkelly]]

Attachment: updated docs patch specifying cornice-format error responses

I am attaching this patch for re-review, based on discussion in Bug 785045 and Bug 792674.  It now allows for the "status" field of the error response to be one of a defined list of error conditions rather than just the constant string "error".  This should help with machine-handle-ability of the responses.

Comment 8

[Ryan Kelly [:rfkelly]]

Comment on attachment 667813 [details] [diff] [review]
updated docs patch specifying cornice-format error responses

Switching review to :rnewman since he appears to be picking up sync2.0 stuff again

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 667813 [details] [diff] [review]
updated docs patch specifying cornice-format error responses

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

::: source/storage/apis-2.0.rst
@@ +136,5 @@
>  Request and response bodies are all JSON-encoded unless otherwise specified.
>  
> +Error responses generated by the SyncStorage server will, wherever possible,
> +follow the JSON error format described in :ref:`syncstorage_error_format`.
> +The format of a successful response  is defined in the appropriate section

s/  / /g

@@ +728,5 @@
> +*X-If-Modified-Since* header would result in the following error response::
> +
> +    HTTP/1.1 400 Bad Request
> +    Content-Type: application/json
> +        

Is this whitespace required for proper RST output?

Comment 10

[Ryan Kelly [:rfkelly]]

(In reply to Richard Newman [:rnewman] from comment #9)
> @@ +728,5 @@
> > +*X-If-Modified-Since* header would result in the following error response::
> > +
> > +    HTTP/1.1 400 Bad Request
> > +    Content-Type: application/json
> > +        
> 
> Is this whitespace required for proper RST output?

I had thought it was, but removing it doesn't seem to affect the rendering.  Committed with these whitespace issues fixed in https://github.com/mozilla-services/docs/commit/3689a1d63181b52c999e79e4eb224d60f3740889

Bug stays open to track implementation progress.

Comment 11

[Ryan Kelly [:rfkelly]]

Implemented in Bug 812432, closing this out.