Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lowcode server: generate proper exception types #19324

Open
sherifnada opened this issue Nov 10, 2022 · 9 comments
Open

Lowcode server: generate proper exception types #19324

sherifnada opened this issue Nov 10, 2022 · 9 comments

Comments

@sherifnada
Copy link
Contributor

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
Copy link
Contributor

juweins commented Nov 14, 2022

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

@sherifnada
Copy link
Contributor Author

@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
Copy link
Contributor

juweins commented Nov 17, 2022

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

@sherifnada
Copy link
Contributor Author

@juweins which pull are you referencing?

@juweins
Copy link
Contributor

juweins commented Nov 21, 2022

@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
Copy link
Contributor

juweins commented Nov 21, 2022

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link

dsinghvi commented Dec 2, 2022

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

@juweins juweins removed their assignment Dec 5, 2022
@girarda girarda added the technical-debt issues to fix code smell label Dec 22, 2022
@bleonard bleonard added the frozen Not being actively worked on label Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants