786523 - Add "error_code" field to cornice-format error messages

Status: VERIFIED WONTFIX

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

Description

[Ryan Kelly [:rfkelly]]

Spinning this off from Bug 783598.

It would be good if our cornice-format error responses included a machine-readable "error_code" field which could be used to differentiate between different kinds of error.  For example, it could be used to differentiate between a missing header and a malformed header.

I propose experimenting with the precise semantics of such a field as part of Bug 784592, in which all sync2.0 error messages will be converted to cornice format.

Comment 1

[Alexis Metaireau (:alexis)]

This means that we would need to list all the possible errors.

We would have, for each location, "malformed" and "missing". I propose that we use two digits error codes. The first digit could be used for the location (header, body, querystring) while the second one could be used for the error relative to this location.

If we use 0 (body), 1 (header) and 2 (querystring) for the first digit and 0 (missing), 1 (malformed) for the second one, would it be enough? 

Do we want to have more precise handling of errors? Do we want the different error codes if the error is related to different variables ? (with what I'm proposing, a missing header will always have the same error code than any other missing header, even if the header key is not the same.

Thoughts.

Comment 2

[Ryan Kelly [:rfkelly]]

(In reply to Alexis Metaireau (:alexis) from comment #1)
> This means that we would need to list all the possible errors.
> [...snip...] 
> Do we want to have more precise handling of errors? Do we want the different
> error codes if the error is related to different variables?

For reasons such as this, I'm having second thoughts about numeric error codes here.  IMHO it would be good to avoid a proliferation of extremely-specific error codes.

Strawman proposal: what if we add another field to the error named "reason", and put in a short human-readable adjective rather than an inscrutable numeric code.  Here's a hypothetical example from sync2.0, related to Bug 735039.

    { 'status': 'error',
      'errors': [{'location': 'header',
                  'name': 'X-If-Modified-Since',
                  'reason': 'malformed',
                  'description': 'value is not an integer'}]}

The client code would switch on the (location, name, reason) tuple to decide what to do about the error, and the extra description is included for human debugging purposes.

We would have to maintain a standard list of adjectives ("malformed", "missing", "unexpected", etc...) but somehow that seems more manageable to me than a list of numeric codes.

:gps or :rnewman, what are your thoughts as potential consumers of these error responses?

Comment 3

[Richard Newman [:rnewman]]

My 2¢!

(In reply to Alexis Metaireau (:alexis) from comment #1)
> This means that we would need to list all the possible errors.
> 
> We would have, for each location, "malformed" and "missing". I propose that
> we use two digits error codes. The first digit could be used for the
> location (header, body, querystring) while the second one could be used for
> the error relative to this location.
> 
> If we use 0 (body), 1 (header) and 2 (querystring) for the first digit and 0
> (missing), 1 (malformed) for the second one, would it be enough? 

Realistically, I think clients are unlikely to spend the time to decode a numeric format like this and figure out how to respond. (This is an error path, so I'm assuming you're doing numeric codes for the benefit of a computer, because there's no point saving bytes.)

The stuff that will be handled specially by a client is typically afforded its own HTTP code -- I'm mainly thinking about auth errors here.

> Do we want to have more precise handling of errors? Do we want the different
> error codes if the error is related to different variables ?

Clients won't make requests that they know to be bad, and they probably won't retry after parsing a response -- if they knew how to fix the problem, they wouldn't be in that state to start with! They'll just give up or try again later. (Useful distinctions, then, are a bit HTTPey -- server vs client error, retryable or not.)

The consumer of a detailed error report is a developer, so use strings.


(In reply to Ryan Kelly [:rfkelly] from comment #2)

> The client code would switch on the (location, name, reason) tuple to decide
> what to do about the error, and the extra description is included for human
> debugging purposes.

See above for my opinion on sophisticated error handling! But this output is the kind of thing that's very helpful to a developer, and if some client dev is feeling really masochistic, they can apply automated processing, too.

> We would have to maintain a standard list of adjectives ("malformed",
> "missing", "unexpected", etc...) but somehow that seems more manageable to
> me than a list of numeric codes.

I concur.

Comment 4

[Ryan Kelly [:rfkelly]]

(In reply to Richard Newman [:rnewman] from comment #3)
> > The client code would switch on the (location, name, reason) tuple to decide
> > what to do about the error, and the extra description is included for human
> > debugging purposes.
> 
> See above for my opinion on sophisticated error handling! But this output is
> the kind of thing that's very helpful to a developer, and if some client dev
> is feeling really masochistic, they can apply automated processing, too.

I agree for the most part, but sometimes these responses could show up in the expected flow of the protocol.  For example, correct handling of the "conditions not accepted" error response in tokenserver requires inspecting the response to determine "this is a 403 error due to missing X-Conditions-Accepted header".  So I like the notion that machines can determine the cause of the error in principle, even if they rarely do so in practice.

Comment 5

[Alexis Metaireau (:alexis)]

+1 for a non-numeric way of handling error messages.

The other advantage I see to using adjectives rather than error-codes is that we can add new ones without breaking anything.

I've been experiencing with error messages a bit more, and returning a list of possibility in the case the input is invalid can be an useful thing for the end-user for instance (say we want a value to be either "blue" or "green", if the user sends "red", we can tell him why this is wrong.

So I'm +1 for adding this "reason" key everywhere. It means changing a bit more cornice and the APIs that are using it to match this, though.

Comment 6

[Ryan Kelly [:rfkelly]]

(In reply to Alexis Metaireau (:alexis) from comment #5)
> So I'm +1 for adding this "reason" key everywhere. It means changing a bit
> more cornice and the APIs that are using it to match this, though.

Do we need to change cornice in order to push ahead with this for Sync2.0?  IIRC the latest cornice allows adding arbitrary extra fields to the error description object, so we should document "reason" as an additional field in the Sync2.0 protocol independently of it being added into cornice itself.

Comment 7

[Alexis Metaireau (:alexis)]

I think cornice is ready for this, so we have two options:

1. put this in cornice, with an option to activate / deactivate it (force the errors to have a reason, for instance)
2. put this in mozsvc, and define the error reasons there.

I think I prefer 1) but at the same time, it's not clear to me if we want to expose this way of thinking our web services to everyone using cornice, thus the disabled by default thing. Maybe Mozsvc could just set the option to True, so we'll be using this by default for all our mozilla services?

Comment 8

[Ryan Kelly [:rfkelly]]

The cloudservices API landscape is different enough now that I don't think this bug is of any concrete use.  Some excellent discussion but no concrete actions to come out of it.

Comment 9

[James Bonacci [:jbonacci]]

ok. that's fine