Generalized Cornice-like error response format

RESOLVED WORKSFORME

Status

Cloud Services
Server: Sync
RESOLVED WORKSFORME
5 years ago
2 years ago

People

(Reporter: rfkelly, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa?])

(Reporter)

Description

5 years ago
I'm spinning this off as a separate bug to try to take stock of several different threads of discussion on error-reporting (Bug 783598, Bug 786523, Bug 784592, Bug 785045, Bug 735039).  We have two related-but-different situations in which we need to return a non-200 response code with some additional information in the response body.

One is for control-flow responses that are part of the application protocol, like the conditions-required response from Bug 783598 or the upgrade-required response from Bug 785045.  In these cases the client is supposed to detect the error and take some action to resolve it, so we need a simple and unambiguous way for the client to identify a specific error condition.

The other is for unexpected error conditions such as invalid header values, malformed JSON and the like.  In this case, there's really nothing the client can do to fix the situation.  It needs to be able to unambiguously detect a fatal error, and then produce some sort of log with as much detail as possible.

I propose that we generalize the cornice error JSON format to allow arbitrary top-level status codes.  The general form of the JSON response body would be:

    {
      'status': <a short string identifying what kind of error this is>,
      'errors': [<zero or more detailed error description objects>],
      <status-specific additional keys>: <and matching values>
    }

The status string "error" always identifies an unexpected error, and works the same as existing cornice error messages:

    400 Bad Request
    {
      'status': 'error',
      'errors': [{'location': 'header',
                  'name': 'X-If-Modified-Since',
                  'reason': 'invalid'
                  'description': 'that aint no integer'}]
    }

Applications may define other status codes that are meaningful within their protocol, and document how clients should behave when they see these statuses.  For example, the tokenserver API could use a 'conditions-required' status:

    403 Forbidden
    {
      'status': 'conditions-required',
      'errors': [{'location': 'header',
                  'name': 'X-Conditions-Accepted',
                  'reason': 'missing'
                  'description': 'Need to Accept conditions'}],
      'condition_urls': {'tos': 'http://url-to-tos'}
    }

Here the "errors" list is informational only.  The client code doesn't have to process it in detail in order to decide what to do, but it gives the developer and/or the logfile some insight as to the details of what's going on.  We could omit it to save bytes.

I'd even go so far as to allow the 'errors' field to be omitted if empty.  Then syncserver could define an 'upgrade-required' status response like so:


    403 Forbidden
    {
      'status': 'upgrade-required'
    }


The result is a little bulkier than the numeric error codes used in sync1.1, but I think it's worth it.  The list of error descriptions means we don't have to enumerate unique codes for every corner-case of a badly implemented client, but the top-level "status" field means we can unambiguously identify specific error responses that are expected as part of the application protocol.

I'll take Bug 785045 Comment 16 and Bug 785045 Comment 17 as tentative approval from the client team.  Toby and Alexis, your thoughts?
(Reporter)

Updated

5 years ago
Blocks: 784592
(Reporter)

Updated

5 years ago
Blocks: 783598
(In reply to Ryan Kelly [:rfkelly] from comment #0)

> One is for control-flow responses that are part of the application protocol,
> …
> The other is for unexpected error conditions such as invalid header values,
> …

This paragraph belongs in a doc somewhere :D

> Here the "errors" list is informational only.  The client code doesn't have
> to process it in detail in order to decide what to do, but it gives the
> developer and/or the logfile some insight as to the details of what's going
> on.  We could omit it to save bytes.

This is the key point, I think. status is machine-switchable, errors is for debugging. Thumbs up.

> I'd even go so far as to allow the 'errors' field to be omitted if empty. 

Sounds good to me. Would be helpful for when these responses are actually being produced by Zeus or similar.

> The result is a little bulkier than the numeric error codes used in sync1.1,
> but I think it's worth it.

Agreed.
OS: Linux → All
Hardware: x86_64 → All
Summary: Generalized cornice-like error response format → Generalized Cornice-like error response format
(Reporter)

Comment 2

5 years ago
cc'ing Tarek on this bug, since it's relevant to some stuff he's reviewing in Bug 812432
(Reporter)

Comment 3

5 years ago
Tarek, thoughts on this approach, and whether to grow cornice API to support it easily?
Flags: needinfo?(tarek)
(Reporter)

Updated

5 years ago
No longer blocks: 784592
I am all for it
Flags: needinfo?(tarek)
Whiteboard: [qa?]
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.