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 add_middleware interface #1683

Merged
merged 3 commits into from
Apr 22, 2023
Merged

Add add_middleware interface #1683

merged 3 commits into from
Apr 22, 2023

Conversation

RobbeSneyders
Copy link
Member

This PR adds an add_middleware method to the apps and ConnexionMiddleware to easily add middleware to the stack. Before, the only way to do this was to pass in a complete middleware stack.

The default position to add the new middleware is right before the ContextMiddleware, which is the final middleware in the stack. Another position can be selected by passing in a MiddlewarePosition enum, which defines some positions which make sense.

Since we can no longer assume that the whole middleware stack is defined when initializing the ConnexionMiddleware, we need to delay building the middleware stack until the ConnexionMiddleware is actually called. This also means we need to delay registering the APIs and error handlers. This is now all done in the _build_middleware_stack method.

tests/test_cli.py Show resolved Hide resolved
tests/test_cli.py Show resolved Hide resolved
@@ -27,19 +26,6 @@ def test_app_with_relative_path(simple_api_spec_dir, spec):
assert get_bye.text == "Goodbye jsantos"


def test_app_with_resolver(simple_api_spec_dir, spec):
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests only test if the resolver becomes available as api.resolver, which doesn't really seem that useful.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it's good to be sure that it's actually the resolver that was passed in 😅 But seems indeed too trivial to have a test for it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes and add_api no longer returns the same API object. So it's not as easy to test this anymore. I see we don't have an integration test for this either though, I'll add one.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do already have integration tests for this in test_resolver_methodview.py, so will merge as is.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4686618629

  • 36 of 43 (83.72%) changed or added relevant lines in 3 files are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.3%) to 93.583%

Changes Missing Coverage Covered Lines Changed/Added Lines %
connexion/middleware/main.py 32 39 82.05%
Files with Coverage Reduction New Missed Lines %
connexion/apps/abstract.py 1 90.91%
connexion/middleware/main.py 7 80.15%
Totals Coverage Status
Change from base Build 4646044651: -0.3%
Covered Lines: 3354
Relevant Lines: 3584

💛 - Coveralls

@RobbeSneyders RobbeSneyders added this to the Connexion 3.0 milestone Apr 13, 2023
Copy link
Member

@Ruwann Ruwann left a comment

Choose a reason for hiding this comment

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

LGTM! Makes me think of the Starlette implementation a bit - I wonder whether the same middleware class makes sense here as it does seem elegant.

@@ -27,19 +26,6 @@ def test_app_with_relative_path(simple_api_spec_dir, spec):
assert get_bye.text == "Goodbye jsantos"


def test_app_with_resolver(simple_api_spec_dir, spec):
Copy link
Member

Choose a reason for hiding this comment

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

Well, it's good to be sure that it's actually the resolver that was passed in 😅 But seems indeed too trivial to have a test for it

@RobbeSneyders
Copy link
Member Author

Yes, I went for this approach instead of leveraging the starlette Middleware class since I think:

  • Just passing the middleware and options to add_middleware is a more user-friendly interface than having to wrap it into a separate class first
  • partials are Python builtin functionality that cover our need for providing options as part of the middleware stack.

Happy to hear if you don't agree.

@RobbeSneyders RobbeSneyders merged commit 7f11150 into main Apr 22, 2023
@RobbeSneyders RobbeSneyders deleted the feature/add-middleware branch April 22, 2023 19:37
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.

3 participants