PrefectHQ / prefect

Prefect is a workflow orchestration framework for building resilient data pipelines in Python.
https://prefect.io
Apache License 2.0
15.92k stars 1.56k forks source link

override default `operationId`s generated by `fastapi` #6754

Open darin-reify opened 2 years ago

darin-reify commented 2 years ago

First check

Prefect Version

2.x

Describe the current behavior

Currently, fastapi auto-generates the operationIds for each route with a combination of route name, parameters, request method, etc. While descriptive, this leads to some really convoluted and unwieldy operationIds; especially when libraries use these operationIds to generalize calling into the API.

Describe the proposed behavior

Instead of the generic handling, fastapi provides a way to customize the operationIds it exposes. I would like to propose orion use this functionality to provide more useful operationIds and, thus, help support 3rd-party integration.

Example Use

No response

Additional context

No response

zanieb commented 2 years ago

Hi. Thanks for this request! We'll need to think about this a bit. Can you provide of some example routes with existing and preferred operation identifiers? It'd also be great to give a high level description of what functionality would be enabled in 3rd-party integrations for our product team to understand how this helps Prefect fit into the ecosystem.

darin-reify commented 2 years ago

My biggest problem with the openapi document as it stands is its funky formatting of operationIds. I'm fine with doing some post-processing as long as I can easily (and reliably) go from a super descriptive operationId to one that's more human-friendly and palatable.

Roughly, all operationIds follow the following pattern: <route-name>_<group_name>__[<parmeters>]__<method> however there are a couple that don't seem to match this pattern which makes said-post-processing kludgy. (I honestly just decided to update the operationIds to a kebab'd and lower-cased version of the route's Summary attribute).


(I wrote this section before I realized I could narrow down my request/description. I've left it here for as a supplementary reading/background)

Sure! Here's some more background.

One of the benefits of a data-described API is being able to rely on the data to do a lot of things for you: coercion, validation, etc. One of the biggest benefits, IMO, is the flexibility of being able to define handlers for each endpoint dynamically or, the inverse of that, having a single generic entrypoint that can fan out to whatever endpoint it was told to.

In my clojure wrapper I decided, instead of writing API namespaces wrapping the orion endpoints, I would use the martian library to read orion's openapi document and do the needful. This meant all my code needed to do was call a single fn whenever I needed to talk to orion.

The key martian uses to key handlers is the endpoint's operationId (which is the right choice). However, because of how fastapi generates operationIds, these keys are pretty unwieldy:

(martian/response-for :create_flow_run_flow_runs__post {:some "data"})

As it stands, if I ever need to create a flow run in multiple places I have to either:

The first option is super error-prone while the the second option which essentially dilutes the biggest benefit a data-driven API provides. This is what I'd love to do:

(martian/response-for :create_flow_run {:some "data"})
zanieb commented 2 years ago

Thanks! We'll need to gather some feedback from some relevant stakeholders before we can add this to our backlog. Are you interested in contributing this if we're willing to accept it?

darin-reify commented 2 years ago

Sure, I can take a swing at it if it gets approved.

zanieb commented 1 year ago

@abrookins let's try to get this issue out of triage.

@darin-reify I don't see any reason for us not to do this, we'll just need to decide on a consistent naming pattern for the operations. Do you know of an existing library doing this that we could use as a best-practice example?

abrookins commented 1 year ago

You had me at “Clojure wrapper.”

abrookins commented 1 year ago

I think we should do this and use our route names as the operation IDs. E.g. with the FastAPI recommendation:

    # Use route names as OpenAPI operation IDs.
    for route in api_app.routes:
        if isinstance(route, APIRoute):
            route.operation_id = route.name

These look way better, and we already informally name routes with an appropriate pattern. We could do a sweep to find any that are "weird" and fix them with an override, maybe.

I'll run it by product but I doubt they'll have any issues with this.

abrookins commented 1 year ago

This is ready for us to move forward. I'd be happy if we used the current method names and changed them later if we find some that are sub-optimal -- with the understanding that we don't guarantee that these names will remain static at this time.