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

✨ Add support for extensions and updates to the OpenAPI schema in path operations #1922

Merged
merged 11 commits into from
Jul 29, 2021

Conversation

edouardlp
Copy link
Contributor

@edouardlp edouardlp commented Aug 19, 2020

Description

This PR adds support for specifying extra info for the JSON schema of path operations. This is so that OpenAPI extensions can be used when declaring routes.

Use case

We need this because we rely on open api extensions to configure our service mesh (linkerd) for retries and timeouts, but there are probably many use cases for this.

Implementation

  • There is a lot of boilerplate to add support for extra arguments, but it does not look like there is a way around it. Suggestions welcome.
  • The pydantic model of Operation accepts any extra info, which matches the behavior of Path, Field, Query, etc. There might be an argument to restrict it to x- prefixed fields, but this will be more complex. I'm not sure it is worth it.

@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #1922 (0e3d84b) into master (7db3591) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master     #1922    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          401       408     +7     
  Lines        10095     10196   +101     
==========================================
+ Hits         10095     10196   +101     
Impacted Files Coverage Δ
fastapi/applications.py 100.00% <ø> (ø)
...th_operation_advanced_configuration/tutorial005.py 100.00% <100.00%> (ø)
...th_operation_advanced_configuration/tutorial006.py 100.00% <100.00%> (ø)
...th_operation_advanced_configuration/tutorial007.py 100.00% <100.00%> (ø)
fastapi/openapi/models.py 100.00% <100.00%> (ø)
fastapi/openapi/utils.py 100.00% <100.00%> (ø)
fastapi/routing.py 100.00% <100.00%> (ø)
tests/test_openapi_route_extensions.py 100.00% <100.00%> (ø)
...ration_advanced_configurations/test_tutorial005.py 100.00% <100.00%> (ø)
...ration_advanced_configurations/test_tutorial006.py 100.00% <100.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7db3591...0e3d84b. Read the comment docs.

@github-actions
Copy link
Contributor

📝 Docs preview for commit ef11de8 at: https://5f3d3325b71f19b554d54b99--fastapi.netlify.app

@edouardlp edouardlp changed the title fix(openapi): add support for extensions in path operations Add support for extensions in path operations Aug 19, 2020
@edouardlp edouardlp changed the title Add support for extensions in path operations Add support for OpenAPI extensions in path operations Aug 19, 2020
@edouardlp edouardlp changed the title Add support for OpenAPI extensions in path operations ✨ Add support for OpenAPI extensions in path operations Aug 19, 2020
@edouardlp edouardlp marked this pull request as ready for review August 19, 2020 14:31
@edouardlp
Copy link
Contributor Author

Note that this is similar to #1917 in that APIRoute accepts extra arguments, but I believe the similarities end there. Both PRs appear compatible to me.

@github-actions
Copy link
Contributor

📝 Docs preview for commit b73f7af at: https://5f3d4104691656ae542c442a--fastapi.netlify.app

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Aug 19, 2020

Note that this is similar to #1917 in that APIRoute accepts extra arguments, but I believe the similarities end there. Both PRs appear compatible to me.

Right now, the PR's are not compatible... To make them compatible extra should be a dict, not a keyworded .
But as I said on that PR, I'm not sure if it will be accepted so I don't think you should addapt this PR (or be worried about compatibility) for the sake of that one. We should wait to see what @tiangolo has to say.

app = FastAPI()


@app.get("/items/", **{"x-my-open-api-extension": "value"})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather write something like:

Suggested change
@app.get("/items/", **{"x-my-open-api-extension": "value"})
@app.get("/items/", openapi_extensions={"x-my-open-api-extension": "value"})

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicit is better than implicit.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realized @Kludex commented the exact same thing 😬

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't said anything, have I? 😮

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make them compatible extra should be a dict, not a keyworded.

I mis-read.

Copy link
Contributor Author

@edouardlp edouardlp Aug 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do prefer it explicit as well, but I was mirroring Path, Field and Query which will add any other arguments to the open api spec. I'll make the change, thanks for the suggestion!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hadrien @edouardlp I think it should be openapi_extensions={..}, or if we'd use ** we should update the operation late in get_openapi_path instead of in get_openapi_operation_metadata as that would give us a "openapi override" so we could set requestBody or whatever and not having it overwritten somewhere in get_openapi_path.
What do you guys think?

@hadrien
Copy link

hadrien commented Aug 21, 2020

Would there be a way to add extension to the router as well?

@mbelang
Copy link

mbelang commented Sep 21, 2020

@tiangolo Any chance this could be looked at please? We think it is a nice enhancement to the openapi support and that would solve a big part of automation for us. Thx

@triptec
Copy link

triptec commented Oct 9, 2020

I tried this branch out and found a missing **route.extra in the call to add_api_route in include_router and also moved

            if route.extra:
                operation.update(route.extra)

to get_openapi_path.
I've tried to make PR to edouardlp:fix/support_openapi_extensions, but couldn't for some reason. The changes can be found at Future-Position-X@4d3f781

Is it perhaps something of interest?

@tiangolo tiangolo changed the title ✨ Add support for OpenAPI extensions in path operations ✨ Add support for extensions and updates to the OpenAPI schema in path operations Jul 29, 2021
@github-actions
Copy link
Contributor

📝 Docs preview for commit 0e3d84b at: https://61030891eb9ff5337060b81d--fastapi.netlify.app

@tiangolo
Copy link
Owner

Thanks for the work @edouardlp! 🚀 🍰

And thanks for the discussion everyone.

I updated this a bit, renamed the parameter to openapi_extra, to keep in line with Pydantic's schema_extra while making it explicit that this is about OpenAPI.

I also refactored the implementation to make sure that it does a deep merge of the schema and moved it to do it after the other stuff is added. This way this can be used not only for extensions but also to modify the automatically generated schema.

I also added some other examples, docs, and tests.

This will be available in FastAPI version 0.68.0 in some minutes. 🔖🎉

@tiangolo tiangolo merged commit 836bb97 into tiangolo:master Jul 29, 2021
solomein-sv pushed a commit to solomein-sv/fastapi that referenced this pull request Jul 30, 2021
…h operations (tiangolo#1922)

Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
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

Successfully merging this pull request may close these issues.

None yet

6 participants