Return errors rather than panicking for callable APIs #809
Conversation
davepacheco
left a comment
davepacheco
left a comment
There was a problem hiding this comment.
Thanks for doing this! I was actually going to do this as well so that automated tests (e.g., for authn) could try calling all endpoints without crashing Nexus.
common/src/api/external/error.rs
Outdated
|
|
||
| Error::NotImplemented => HttpError::for_status( | ||
| Some(String::from("Not Implemented")), | ||
| http::StatusCode::NOT_IMPLEMENTED, |
There was a problem hiding this comment.
I only wonder whether this is ever what we would want to return to clients, or if we'd want this to be a 500?
There was a problem hiding this comment.
I don't have a strong opinion - if you want me to fold it into 500, that's an easy change to make - but if this isn't the right use-case for 501, what is?
There was a problem hiding this comment.
It may well be! I didn't know it existed. The only reason I ask is that it feels like TMI: if a customer sees it it's like we're telling them "not only is there a problem on our side here, but it's that we didn't notice we forgot to implement this". Doesn't really matter.
There was a problem hiding this comment.
Actually, reading https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501 a little closer:
If the server does recognize the method, but intentionally does not support it, the appropriate response is 405 Method Not Allowed.
I'll send back 405 instead.
There was a problem hiding this comment.
I get why that technically sounds right, but 405 seems much worse to me. If a customer sees this, it's because we literally forgot to implement it, right? This isn't a client error.
There was a problem hiding this comment.
Ack, switched to 500 then. I'm running out of error codes to try at this point 😁
There was a problem hiding this comment.
If this is intended to represent todo! externally, I think it should be a 501 because that "implies future availability". 405 is "not supported" and unlikely to change (aka this endpoint does not accept PATCH).
We should keep 500 for actual server errors.
|
@smklein is this PR still valid? |
No description provided.