Add x reason header to error responses #29
Conversation
Replaces json error messages with the use of the X-Reason header as specified by BUD-01. Updates error scenario tests to validate the header.
Adds a bunch of tests for the HEAD /upload endpoint. Fixes a tiny bug in content length calculations.
|
@laanwj would you mind taking a look at this PR? I usually just merge my own PRs but wouldn't hurt with some feedback if you have any. If you don't have time, I'll just merge it. |
|
sure, will take a look |
|
Code changes LGTM. i don't know enough about implementations of blossom whether getting rid of JSON responses at the same time causes compatibility issues. But if so, they need to be updated to be compatible with BUD-01 anyway. |
BUD-01 just says that error responses MAY include the X-Reason header but nowhere does it mention returning a JSON response on error responses. Therefore I figured we'd just replace it entirely with the X-Reason header. The only difference is that you'd have to consume an error response from the header instead of the "message" field in the json response really. |
Right. All it can affect is what error is displayed to the user, when something goes SNAFU anyway. It's fine imo. |
| if let Ok(content_length) = content_length.parse::<u64>() { | ||
| let blob_size_in_mb = bytes_to_mb(content_length as f64); | ||
| if blob_size_in_mb > app_state.config.upload.max_size { | ||
| if let Ok(content_length) = content_length.parse::<f64>() { |
There was a problem hiding this comment.
Oops i only notice this now. This isn't strictly correct. The Content-Length header should be parsed as an unsigned integer, not a floating point number. Better to keep it as <u64> then cast it to f64 later.
(This would accept things like Content-Length: -1e8 or Content-Length: 1.5 which make no sense)
There was a problem hiding this comment.
Oh right, I'll make a new PR for this
Refactors handlers to stop returning JSON body in error responses and instead set the X-Reason header as per BUD-01.
Adds a bunch of tests for the HEAD /upload endpoint to ensure that's also tested properly.
Fixes #19.