airbytehq / airbyte

The leading data integration platform for ETL / ELT data pipelines from APIs, databases & files to data warehouses, data lakes & data lakehouses. Both self-hosted and Cloud-hosted.
https://airbyte.com
Other
16.28k stars 4.15k forks source link

Lowcode server: generate proper exception types #19324

Open sherifnada opened 2 years ago

sherifnada commented 2 years ago

Tell us about the problem you're trying to solve

The FastAPI framework typically handles errors by expecting the developer to raise them as Exceptions sub-classing HTTPException. The Framework then transforms it to the correct HTTP response, and returns it to the API consumer.

In our OpenAPI spec we define some error responses e.g: it defines that when returning 400s, the KnownException type should be used.

But the generated code is just a "plain old object" -- it does not subclass HTTPException. This causes a problem: if an endpoint raises a KnownException we get the following error:

  File "/Users/lakemossman/code/airbyte/airbyte-connector-builder-server/./connector_builder/impl/default_api.py", line 63, in list_streams
    raise KnownExceptionInfo(message="failure")
TypeError: exceptions must derive from BaseException

Describe the solution you’d like

I would like the autogenerated exception classes to extend the HTTPException class defined by FastAPI.

juweins commented 2 years ago

@sherifnada You can assign it to me, I would like to make this my first contribution!

sherifnada commented 2 years ago

@juweins I may have been too optimistic in labeling this as a good first issue -- my guess is that it might be involved as it requires an understanding of the OpenAPI generator. You're welcome to give it a shot, but just wanted to flag that it may be more complicated than I initially anticipated. I've assigned to you, let me know if you want me to unassign

juweins commented 2 years ago

@sherifnada You can unassign me, my upcoming proposal has already been implemented with the latest pull today....

sherifnada commented 2 years ago

@juweins which pull are you referencing?

juweins commented 2 years ago

@sherifnada I was pretty confused by the latest merge to that specific class default_api.py. However, I found some time last weekend to get near a possible solution. Hopefully I am able to propose my solution around 10 AM GMT-8.

juweins commented 2 years ago

@sherifnada I committed my proposal to my repo. Can you maybe have a look on my implementation & make suggestions? Am I on the right track to solve this issue? Thanks!

sherifnada commented 2 years ago

@juweins the code is on the right track, but the ticket is saying that the OpenAPI generator we use to generate all the code under the generated/ directory should be the one generating this code, rather than us writing it by hand if that makes sense. See the README for a description of how to invoke the code generator. An example of a type defined in OpenAPI which should generate a class which extends HTTPException is the KnownException class. We want to define this in the OpenAPI spec yaml in such a way that the generated class extends HTTPException.

Does that help?

sherifnada commented 2 years ago

Looking at this issue in the openAPi generator, it's actually not clear to me if openAPI even natively supports it. My guess is that there might be a code generator flag which tells the generator to use a specific super class when generating python code. But short of something like that, I'm not sure the spec itself can support this.

dsinghvi commented 1 year ago

@sherifnada Fern supports this! Here's a link to a generated exception and the corresponding definition.