feat: Add `raise_on_unexpected_status` flag to `Client`

[JamesHinshelwood]

Close #491

Not sure this is the right approach. Just wanted to raise a PR to gather feedback :)

[codecov[bot]]

Codecov Report

Merging #593 (a3bd7c8) into main (99638b1) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #593   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           49        49           
  Lines         1966      1969    +3     
=========================================
+ Hits          1966      1969    +3     

Impacted Files	Coverage Δ
openapi_python_client/__init__.py	100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[dbanty]

Hey @JamesHinshelwood, thanks for the contribution! I am wondering if maybe we should just expose the underlying httpx.Response object within our own Response class that comes back from the _detailed functions. Then folks could more easily do whatever they want with it after the fact (e.g., calling the raise_for_status method). It also would let them choose per-request which responses should generate exceptions instead of doing it at the client level which is more explicit and therefore (I think) better?

This could come in the form of subclassing (if it doesn't get too messy to mix in with generics) or as an inner attribute to which we delegate some methods for easier access.

What do you think?

[JamesHinshelwood]

Hey @JamesHinshelwood, thanks for the contribution! I am wondering if maybe we should just expose the underlying httpx.Response object within our own Response class that comes back from the _detailed functions. Then folks could more easily do whatever they want with it after the fact (e.g., calling the raise_for_status method). It also would let them choose per-request which responses should generate exceptions instead of doing it at the client level which is more explicit and therefore (I think) better?

This could come in the form of subclassing (if it doesn't get too messy to mix in with generics) or as an inner attribute to which we delegate some methods for easier access.

What do you think?

For my use-case, what I really want is an API where I can just call an endpoint and handle the 'expected' responses, then I would get an exception raised for anything unexpected. Even exposing the response directly wouldn't be ideal for me, because then I still need to convert the 'unexpected' response to exceptions myself.

I've realised though that 'expected' doesn't necessarily mean 200 <= x < 300. I think the better behaviour would be to throw if the returned status code was something not listed in the openapi specification. Looking at the generated code, I think this would effectively mean changing the else branch in _parse_response:

def _parse_response(*, response: httpx.Response) -> Optional[GoodResponse]:
    if response.status_code == 200:
        response_200 = GoodResponse.from_dict(response.json())

        return response_200
    # return None
    raise Exception(f"Unexpected status code: {response.status_code})

Also tagging @theFong @ramnes @johnthagen @JorgeGarciaIrazabal who expressed an interest in the discussion, but might have missed this PR.

[JamesHinshelwood]

I've updated this PR with what I (hopefully) described above. Let me know what you think.

[ramnes]

Raising on 2xx responses doesn't feel right to me, why do you feel it's needed? A lot of APIs use 201 and 204, for example.

[JamesHinshelwood]

Raising on 2xx responses doesn't feel right to me, why do you feel it's needed? A lot of APIs use 201 and 204, for example.

If the openapi.yaml specification doesn't list a status code in the responses list, then I consider it 'unexpected' and thus raise an exception. Therefore, if the specification only lists 200 as the expected response code, then yes this PR will raise an exception for other 2xx statuses.

[ramnes]

Ah, right, sorry! I thought you were always raising on non-200 status codes. LGTM. :)

[dbanty]

This is definitely an interesting concept—it basically makes the client stricter by enforcing that the server only returns the types that the spec says it will. I cannot count the amount of times I have seen undocumented responses from servers 😓. I think this is a good behavior to have, I would just like a couple of tweaks to make it a bit more user friendly:

1. Add a note in the Client docstring about what that parameter does. We should probably document all the parameters at some point but this is a good place to start! I've been using Google style docstrings in this project I think, so it would be under an Attributes: section.
2. Declare a new exception type and raise that rather than a base exception so it's easier to users to catch it specifically if they want to. This should probably be defined in the shared types module and imported to everywhere it's needed.
3. Document the potential of raising that exception (and mention the conditions under which it happens) using a Raises section in the docstrings of all the top-level functions which could raise this. I wish Python just had a way to represent exceptions with the type system but... as long as it doesn't, we should make sure to document in docstrings.

[gwenshap]

This seems really important, otherwise you need the full _detailed in order to check whether None means "expected 204" or "unexpected error".

From @dbanty response, it looks like we are only 3 small steps away from getting this in? @JamesHinshelwood if you are too busy, I'll be happy to help and nudge it along.

[JamesHinshelwood]

@gwenshap Thanks for the offer. It will be a while before I can look at this again. If you're willing to rebase and address @dbanty 's comments I'd be very grateful! I've given you access to my fork, but feel free to just take my commits and raise a new PR if preferred.