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

Feature/multiple yaml same base path #1598

Conversation

leonardofesta
Copy link
Contributor

Fixes #1542 .

Changes proposed in this pull request:

It is possible to pass a name parameter to the connexion.app_api(..) method
the name parameter will be assigned to the Flask blueprint
in case you don't pass the name, it will rely on previous/default behaviour (base path as name in Flask case)
Having multiple specification in the same path, opens to new conflicting case.

In case the same endpoint is defined from 2 specs, the first one added and covering the case will be accepted, the second one ignored.
A more niche case, in case you define 2 operation on the same path but different method, the second one won't work, even if they are complementary.

Eg:
spec1 get foo/
spec2 post foo/
if you call post foo/ --> response will be 405, method not allowed (it ignores 2nd definition of the same path)

I think these cases can be fine as long they are documented (let me know where do you think I should amend the documentation).

Let me know if you want more unit tests

Ps: sorry for the mess on the other pr, somehow a work account. ended up as commentor

@leonardofesta leonardofesta force-pushed the feature/multiple-yaml-same-base-path branch from 699d860 to 25172a9 Compare September 27, 2022 22:03
@coveralls
Copy link

coveralls commented Sep 27, 2022

Pull Request Test Coverage Report for Build 3144853041

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 95.246%

Totals Coverage Status
Change from base Build 3130164113: 0.003%
Covered Lines: 2845
Relevant Lines: 2987

💛 - Coveralls

@RobbeSneyders
Copy link
Member

RobbeSneyders commented Sep 29, 2022

Thanks @leonardofesta!

In case the same endpoint is defined from 2 specs, the first one added and covering the case will be accepted, the second one ignored.
A more niche case, in case you define 2 operation on the same path but different method, the second one won't work, even if they are complementary.

I don't really like this behavior as it is not the behavior I would expect. I think ideally we would:

  • Raise an error if multiple operations are registered for the same path and method
  • Allow different operations on the same path as long as they are complementary

I think this behavior is generated by the combined behavior of Flask Blueprints and how apis are registered in our RoutingMiddleware (see #1489 for background):

def add_api(
self,
specification: t.Union[pathlib.Path, str, dict],
base_path: t.Optional[str] = None,
arguments: t.Optional[dict] = None,
**kwargs
) -> None:
"""Add an API to the router based on a OpenAPI spec.
:param specification: OpenAPI spec as dict or path to file.
:param base_path: Base path where to add this API.
:param arguments: Jinja arguments to replace in the spec.
"""
api = RoutingAPI(
specification,
base_path=base_path,
arguments=arguments,
next_app=self.app,
**kwargs
)
self.router.mount(api.base_path, app=api.router)

I don't immediately see a way to get the desired behavior without big changes in how routing works.

@RobbeSneyders
Copy link
Member

Updated my comment above after having a second look.

@leonardofesta
Copy link
Contributor Author

leonardofesta commented Oct 7, 2022

Yes, it doesn't seem straightforward.

As far as I understood for each add_api() we create a blueprint and a starlette router.
This make difficult to check issues between the 2 yaml configurations.

Also from the starlette router, in their documentation on routing they specify they use a policy of priority on routes, in case of conflicts first win.

The second issue where you get 405 for same path and different method, I think is caused also from starlette implementation for this piece of code here

They have this concept of partial match. When the path is registered but the method is not present, it causes an exception and returns 405 meethod not allowed.

@RobbeSneyders
Copy link
Member

Closing in favor of #1736. I cherry-picked your commits there.

RobbeSneyders added a commit that referenced this pull request Oct 17, 2023
Fixes #1542 
Fixes #1724 

Cherry-picked some commits from #1598.

---------

Co-authored-by: Leonardo Festa <4375330+leonardofesta@users.noreply.github.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.

Multiple Swagger file on same connexion app
3 participants