public.json should 404 when participant is unknown

[chadwhitacre]

Right now it gives {"receiving": "0.00"}.

[clone1018]

I'd like to rush ahead with this one so I can implement the following functionality into Gittip Everywhere:

if(user 404's on gittip) don't show Gittip button (unless on GitHub/Twitter)
else show gittip button and hope they're the right user :p

[chadwhitacre]

@clone1018 Go ahead and assign it to yourself so we know you're working on it.

Any questions I can answer?

[clone1018]

Oh, I didn't mean to say I'd work on it, although I can if you're up for fixing what I break :P

[chadwhitacre]

@clone1018 I am definitely up for fixing what you break. :-)

[joonas]

This got me thinking, what's the appropriate thing to return in the case of 404 for json?

Currently, if you raise Response(404), you'll get back HTML, but in this case since we're requesting json, it would be more appropriate to return a json-formatted response, say:

{
  "error": {
    "code": 404,
    "description": "Not Found"
  }
}

For other resources, it may also make sense to include other bits of information (such as validation?) in the error.

[clone1018]

Just so you guys know, I'll be super busy over the next couple of days and won't have time for this, sorry!

[lyndsysimon]

Here's my basic return format for an API error:

{
    "error": {
        "type": "NotFound",
        "message": "User 'notarealuser' not found."
     }
}

The type would be an error type defined by the API spec, and not all that useful here since HTTP status 404 is pretty explicit. A better, real-world example (sent with an HTTP 400):

{
    "error": {
        "type": "BadParamValue",
        "message": "The value 'asdf' is not valid of 'org_id' parameter."
     }
}

[sigmavirus24]

I've never seen the type attribute in an error response from an API. Usually they use the HTTP status codes to convey the type and leave the message to be more descriptive. And honestly, I think 422 is a validation error (I can't check at the moment) so I would think even "BadParamValue" would be a bit redundant. Essentially, instead of trying to come up with descriptive types, we should just use descriptive status codes. They do exist after all.

[chadwhitacre]

@joonas Yeah, there's a ticket in Aspen to make it easier to return errors with different mimetypes: https://github.com/zetaweb/aspen/issues/19. If you'd like to work on it let me know and I'll add you to the Aspen team as well so you can assign it to yourself.

[joonas]

@whit537 I can take a look at the code and decide then whether I should work on it or leave it to someone more knowledgeable ;) So don't add me yet, I'll let you know when I've had a chance to take a look at the relevant code

[chadwhitacre]

This would actually be a good first ticket to work on in Aspen. There's a function that resolves those error pages, and we just need to compute the extension to use instead of hard-wiring '.html'.

[lyndsysimon]

I assigned this to myself, but I haven't began work on it other than to verify that the issue is upstream in Aspen. @joonas, if you want it, go for it.

I'll unassign myself for the moment, and check back later.

[lyndsysimon]

@sigmavirus24 I originally got the idea from Namecheap's API. They use an encoding system though, whereas I prefer to have the name of the error itself to be descriptive. I think that's just my desire to be more idiomatic in my code - I wouldn't have API_ERROR_123 in my code, I'd have InvalidParameterValue.

You're correct on using 422 vs. 400, upon my reviewing the spec.

I do see utility in providing errors in the return, beyond the HTTP code. For example, if two parameters are mutually exclusive, then specifying both is syntactically correct and can be understood by the server (hence, 400 is not correct), but make no semantic sense (so return a 422). Providing an API Error in the response could specify specifically what fields are causing the conflict, potentially greatly helping a developer trying to figure out why the 422 was returned.

[sigmavirus24]

I don't disagree that it is a good idea, I would just love a clever way to be as explicit about it as possible. I dealt unfortunately directly with AWS (because using boto was overkill) and their error messages had types like that but were not even close to being helpful in my effort to debug the issue. So I guess having dealt with GitHub's API (mostly) and having not seen them there (and having had a good experience with it) contrasted with AWS (where I had a poor experience and limited exposure). But yeah, if some explicit errors can be given with descriptive messages, I'm all for it. And I didn't mean to sound pretentious in bringing up 422 (as I realize now I might have sounded). Sorry if it came across that way.

Edit Also, me no speak good english while doing homework. (Sorry if the above seems jumbled)

[joonas]

I stumbled upon this again as I was going through the errors on Sentry, and thus I thought it would be appropriate to address this.

So picking up from the previous discussion, what would be the most appropriate course of action here?

Return a 404 response with the body containing some JSON with key value pairs explaining the error state?

Currently this is generating 500 errors, which is obviously less than desirable.

[chadwhitacre]

This should be a 404. We have a ticket in Aspen to make media types of error pages match the file extension. Or if we don't, we should.

https://github.com/gittip/aspen/issues/19

[wyze]

I don't know if this can be closed, but it is returning a 404 response code, along with a HTML for the 404 error.