search

chridou / http-api-problem

A problem type to be returned by HTTP APIs
Apache License 2.0
53 stars 12 forks source link

Handle invalid status codes deserialization #31

Closed buinauskas closed 3 years ago

buinauskas commented 3 years ago

Instead of returning None, this should return an error when it encounters an invalid status code during deserialization.

Submitting because of the following comment remaining in code:

// If the status code numeral is invalid we simply have none...

chridou commented 3 years ago

Thanks for your attention!

Actually there is a reason for this which I should better had documented: This would now make the whole serialization fail om an invalid status code but a client might still want to have access to the remaining information which might still be useful. status_code is also a field which is not required by the RFC. Furthermore the client is already on an "error path" and I was under the impression that the client code should not have to deal with that in addition (how often would this even happen?). The the original status is in the response anyways.

Would it be a solution to maybe keep it as None and add a field like __invalid_status_code to the parsed problem itself so that the information does not get lost? I guess in most cases the problem simply lands in a log file...

buinauskas commented 3 years ago

Ah, I was under the impression that change was supposed to happen but never did, so I submitted a PR.

Frankly, I'm mostly fine with deserializing an incorrect status code as None for the sake of DX.

On the contrary, failing deserialization in such a case feels to be more correct.

chridou commented 3 years ago

Yes. From a point of correctness I totally agree. It is a trade off.

buinauskas commented 3 years ago

No worries. I'll close the PR. Thanks for explain the reasoning behind the implementation!