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

Revisit OpenAPI and the generated python API for mutation #655

Closed
illusional opened this issue Jan 18, 2024 · 4 comments
Closed

Revisit OpenAPI and the generated python API for mutation #655

illusional opened this issue Jan 18, 2024 · 4 comments

Comments

@illusional
Copy link
Contributor

The OpenAPI API spec and generated python module has been a bit of a painpoint for a while - and it looks like we're going to run into issues with our generator being pinned back, but the newer versions of FastAPI will generate a new version of the OpenAPI spec (3.1): #643 (comment)

We should consider whether the REST / OpenAPI / generated python module is the best approach here. I have a couple of suggestions, but not restricted to that:

  • We add mutation to graphql: I'm a little sceptical on the user experience here, less discoverable?
  • We use contract-driven GRPC to generate another Python client, but it should be more stable (I hope)
@nevoodoo
Copy link
Collaborator

nevoodoo commented Mar 8, 2024

I've done a LOT of research over the last week or so and this looks tough for various reasons:

  • OSS options seem to be limited in the support they can offer to this issue. OpenAPI Gen has had the issue Open since 2021 with no clear communication regarding the roadmap for this :(
  • Alternatives like AutoRest are great but do not support anyOf at the moment, which come from the Optional typing we have on a lot of our endpoints.
  • Pinning openapi versions on FastApi instantiation etc doesn't seem to work that well and I fear will make the package quite unstable.
  • Unless we figure out the OAI issues, we will not be able to support the existing client, AND implement a gql client alongside since the Ariadne codegen requires pydanticv2.

I'm considering options that are endorsed by fastapi such as Fern and Speakeasy. Their pricing looks very expensive for our needs but hopefully we might be able to get a special pricing due to the special nature of the project perhaps? Trials with Speakeasy looked promising, and I have to say the CLI etc goes above and beyond for debugging but also codegen itself. Also has extremely strong support for OAI3 and OAI3.1.

Best case scenario with this solution would look like:

  • Current client generated by Fern/Speakeasy via OAI3.x spec (drop-in replacement)
  • gql client generated by Ariadne Codegen

and they would be able to share the same environment.

We could also consider the github pinned package approach where we only generate the client as needed so it does not break but I am personally less inclined to go with this approach (mainly due to a lack of knowledge on how to implement this, maintain this)

Would love to get your thoughts :))

@illusional
Copy link
Contributor Author

Thanks @nevoodoo, this is comprehensive!

After some thought, I agree that supporting the generator indefinitely is a bad approach - I think generating and "freezing" the old API is the approach we should take once we know what we want in the future - so we can move forward as we help analysts upgrade their scripts.

Whether that be move completely to GraphQL for mutations, or an gRPC approach, I don't mind, as long as we can get the client side stable.

To be clear, I'm not tied to api/routes at all, we can totally ditch it + fastapi, or write completely new entrypoints.

@nevoodoo
Copy link
Collaborator

Yeah totally agree. I think that sounds like a good, actionable approach to me :)

Correct me if I'm wrong but ditching the endpoints + fastapi wouldn't change much if we need to maintain an SDK for our users and we're generating it from OpenAPI, right?

Maybe we just freeze the SDK for now and then look at working on the graphql version of the SDK in the meantime.

@nevoodoo
Copy link
Collaborator

nevoodoo commented Mar 13, 2024

Update: I attempted to upgrade pydantic without upgrading fastapi but that was a bust :( There is a breaking change in some Field definitions that causes this:

from pydantic.fields import FieldInfo, Undefined
ImportError: cannot import name 'Undefined' from 'pydantic.fields'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants