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

🐛 Fix edge case with repeated aliases names not shown in OpenAPI #2351

Merged
merged 3 commits into from Aug 18, 2022

Conversation

klaa97
Copy link
Contributor

@klaa97 klaa97 commented Nov 14, 2020

Currently, routes' parameters defined in multiple dependencies are not duplicated since only one parameter per position (path, query, headers, cookie) is retrieved. The current solution has the caveat of not identifying parameters with the same alias, but different position.

Problem example
Example of parameters shown in the current FastAPI Schema for the given route:

@app.get("/{repeated_alias}")
def get_parameters_with_repeated_aliases(
    path: str = Path(..., alias="repeated_alias"),
    query: str = Query(..., alias="repeated_alias"),
):
    return {"path": path, "query": query}
'parameters': [{'in': 'query',
                                               'name': 'repeated_alias',
                                               'required': True,
                                               'schema': {'title': 'Repeated '
                                                                   'Alias',
                                                          'type': 'string'}}]

Solution
To fix the name clashing, it is sufficient to consider parameters duplicated only if they have the same alias and the same type.

The OpenAPI generated by the PR solution is:

"parameters": [
                    {
                        "in": "path",
                        "name": "repeated_alias",
                        "required": True,
                        "schema": {"title": "Repeated " "Alias", "type": "string"},
                    },
                    {
                        "in": "query",
                        "name": "repeated_alias",
                        "required": True,
                        "schema": {"title": "Repeated " "Alias", "type": "string"},
                    },
                ]

@codecov
Copy link

codecov bot commented Nov 14, 2020

Codecov Report

Merging #2351 (2f2a1ae) into master (3a3b61d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2351   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          532       533    +1     
  Lines        13684     13701   +17     
=========================================
+ Hits         13684     13701   +17     
Impacted Files Coverage Δ
fastapi/openapi/utils.py 100.00% <ø> (ø)
tests/test_repeated_parameter_alias.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 2078360926f56483db28a5a66c9fc9d1e610c8fc at: https://5fafc65b048ac31efbfc9ef8--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 8d5fc97 at: https://5fafc9c1048ac31bf9fc9f69--fastapi.netlify.app

@falkben
Copy link
Sponsor Contributor

falkben commented Nov 17, 2020

maybe i'm a bit dense, but why do you need the same alias to a query and a path param?

@klaa97
Copy link
Contributor Author

klaa97 commented Nov 17, 2020

maybe i'm a bit dense, but why do you need the same alias to a query and a path param?

Thank you, the path param and query is a corner case indeed and easily solvable by just changing the name of the query param (which is an implementation detail). On the other hand, consider a combination of query/cookie/header parameter. I agree it can be a corner case, but it is not an implementation detail and may be a particular requirement for an API to expose

@github-actions
Copy link
Contributor

📝 Docs preview for commit 329cc33 at: https://62fe9f757e6f0d007c146366--fastapi.netlify.app

@tiangolo tiangolo changed the title Fix repeated aliases name not shown in OpenAPI 🐛 Fix edge case with repeated aliases names not shown in OpenAPI Aug 18, 2022
@tiangolo
Copy link
Owner

Thanks @klaa97! 🍰

I don't see an obvious use case that needs this, but it also doesn't hurt, so I'm accepting it. 😄

I updated the branch and the test with some recent updates.

Thanks for the contribution! ☕

And thanks for the input @falkben! 🤓

@tiangolo tiangolo enabled auto-merge (squash) August 18, 2022 20:30
@tiangolo tiangolo merged commit e88089e into tiangolo:master Aug 18, 2022
JeanArhancet pushed a commit to JeanArhancet/fastapi that referenced this pull request Aug 20, 2022
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

3 participants