danielsaidi / ApiKit

ApiKit is a Swift SDK that helps you integrate with any REST API.
MIT License
51 stars 3 forks source link

Status code handling is misleading #4

Closed jmg-duarte closed 1 week ago

jmg-duarte commented 5 months ago

Hello!

I'm currently trying out ApiKit in my app, the overall API is very ergonomic! However, the way responses are handled are poorly documented and downright misleading.

Starting with poorly documented because in the "Getting started" there's no mention of error handling, let alone how the status codes are handled. So what I did was try to use what seemed to be the "lowest level" API — fetch — since it returns the URLResponse and tried to handle the cases myself.

To my surprise, errors were being thrown when the authentication failed. Which leads me to the second part:

An "invalid status code" is not a status code outside of the 200-299 range, but rather outside the 100-599 range.

All codes between 100-599 are have categories, even if they're not defined by the standard:

As such, any code outside of the 100-599 range should be invalid, but the other ones are not.

I'm open to help rewrite the "valid" into "successful" and add other methods for the categories. Let me know what you think!

danielsaidi commented 5 months ago

Hi @jmg-duarte

Although the response code handling may need some tweaking, these are the HTTP response status code ranges:

So while 100-199 & 300-399 maybe shouldn't be handled as errors by default, and instead just let any mapping errors be thrown, the 400-599 statuses are errors, provided that the API actually honors the standard.

But perhaps the API should just throw the errors thrown by the system, and not add any custom error handling to the mix.

jmg-duarte commented 5 months ago

Hello! Thanks for the reply!

I don't disagree with throwing an error! My complain resides in the fact that the behavior is not documented and the language used implies the code is not valid, when in fact it is valid, just not successful.

In Python's requests/httpx you have raise_for_status which (it seems to me) is the validation behavior your API offers by default. It throws an exception on anything that is not successful, redirects included, so throwing on non-successful responses doesn't seem that off to me.

I think improvements would be:

danielsaidi commented 5 months ago

Hi @jmg-duarte

Oh, yeah that's a pretty poor choice of wording haha.

Thank you for bringing this up, I will adjust the SDK accordingly.

jmg-duarte commented 5 months ago

If you're interested in any of the other changes, I'm very keen in helping.

I need to improve my Swift-fu 😅

danielsaidi commented 4 months ago

I can't wait to get back to this project and adjust it according to your comments 😅

danielsaidi commented 1 week ago

Hi @jmg-duarte

It took a while, but I will release a 0.9.1 patch that renames these errors and instead talk about codes outside the 100-599 range as invalid, and outside 200-299 as unsuccessful.