openapi-python-client icon indicating copy to clipboard operation
openapi-python-client copied to clipboard

Option to raise exception on error codes

Open dbanty opened this issue 4 years ago • 5 comments

Discussed in https://github.com/openapi-generators/openapi-python-client/discussions/254

Originally posted by theFong November 28, 2020 Is your feature request related to a problem? Please describe. In some cases, if an unexpected error code is returned (or even an expected error code), I would rather raise an exception than return None. This is because depending on the linter and adoption of type annotation in a code base I may not handle a None variable. In this case an exception raising status code, message or other meta data would be more useful to debug especially when using exception logging software like Sentry.

Describe the solution you'd like I see a couple of interface possibilities if this feature would make sense to implement.

Add new functions with a must prefix:

  • must_sync
  • must_sync_detailed
  • must_async
  • must_async_detailed This is inspired by a Golang's pattern of prefixing "must" to functions which may panic. The main downside is the addition of 4 new methods.

Another solution would be to provide a raise_for_status method similar to python requests library on the Response object. This interface would allow the passing of args to specify raising in case of any error code or just unexpected ones. The side effect here is that like remembering to handle None you would have to remember to raise_for_status. This side effect exists when using the requests library so is not a big deal.

Describe alternatives you've considered Currently in cases where I would like to fail hard, I have a wrapper function which takes a ..._detailed function and builds an exception with meta data if parsed is None. This works fine.

Interested in thoughts/discussion!

dbanty avatar Sep 05 '21 21:09 dbanty

Something related to this that I think would be really nice is a way for the user of the generated code to more easily inspect the HTTP status code and body on error. Currently, when the API returns, if it is None, the user knows it fails but has no context as to why it failed, and thus can't display this to the end user or do much to act open it.

Instead of returning Optional[T], perhaps we could devise a return type like:

@dataclass
class TResponse:
    data: Optional[T]
    status_code: int
    raw_payload: Dict[str, Any]

The data member would be exactly what is returned today. status_code would allow the consumer of the API to check for different status codes (403 vs 500, etc) and the raw_payload would allow for, on error, displaying the payload returned by the server, e.g. "field": "field1 is a required field.". I'm most fuzzy on the raw_payload part. Perhaps just passing back up the full httpx.Response would be better? Not sure if that breaks some encapsulation though.

I came across this while working on writing a small consumer for my API. The requests were failing, but I couldn't tell why without breaking out Wireshark and looking at the raw server HTTP response messages.

johnthagen avatar Nov 10 '21 11:11 johnthagen

That's what the *_detailed functions are for, they include additional info (like status code) and should be used when there's something you can do about the failure case (other than just "I didn't get the data").

I avoided returning the actual httpx response class just because I wasn't sure for a while whether I wanted to couple to it, but I don't see us switching to anything else so if adding that to _detailed functions would be useful we can do so.

dbanty avatar Nov 10 '21 15:11 dbanty

That's what the *_detailed functions are for,

🤦 Totally missed those. Thanks for the heads up, you already have exactly what I was looking for.

johnthagen avatar Nov 10 '21 15:11 johnthagen

They're mentioned in the generated READMEs but it's not super obvious. If there's a better way we could call those out so they're more discoverable, please let me know or make a PR with an improvement. Clear documentation is supposed to be a feature of this project 😅

dbanty avatar Nov 10 '21 15:11 dbanty

Another alternative is creating the client with some extra configuration, in this case a raise_on_error_code flag

client = Client(
    base_url="https://api.example.com",
    raise_on_error_code=True,
)

This would be kind of similar to what OpenApi client generator does for other languages, for example Typescript:

export interface ConfigurationParameters {
    basePath?: string; // override base path
    fetchApi?: FetchAPI; // override for fetch implementation
    middleware?: Middleware[]; // middleware to apply before/after fetch requests
    queryParamsStringify?: (params: HTTPQuery) => string; // stringify function for query strings
    username?: string; // parameter for basic security
    password?: string; // parameter for basic security
    apiKey?: string | ((name: string) => string); // parameter for apiKey security
    accessToken?: string | ((name?: string, scopes?: string[]) => string); // parameter for oauth2 security
    headers?: HTTPHeaders; //header params we want to use on every request
    credentials?: RequestCredentials; //value for the credentials param we want to use on each request
}

This will allow also the creation of middlewares which can be useful too.

I know this solution is not ideal either as having a clean client object is pretty nice, but if you are ok with it I can contribute with a PR adding this feature.

JorgeGarciaIrazabal avatar Nov 29 '21 03:11 JorgeGarciaIrazabal

After #775 raising exceptiosn for error codes can be done via event hooks—so I'm considering that as closing this.

dbanty avatar Jul 08 '23 17:07 dbanty

@dbanty One downside of that approach is that error responses often don't have response types in an openapi schema, so any endpoint that enumerates its error codes ends up with a return type of Union[ActualResponseType, Any].

fdintino avatar Jul 17 '23 17:07 fdintino

Closing this, will track built-in exceptions in #254

dbanty avatar Aug 13 '23 01:08 dbanty