-
-
Notifications
You must be signed in to change notification settings - Fork 769
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
Implement user facing interface for ConnexionMiddleware #1621
Conversation
Pull Request Test Coverage Report for Build 4015552946
💛 - Coveralls |
from .apps import AbstractApp # NOQA | ||
from .apps.async_app import AsyncApp | ||
from .apps.asynchronous import AsyncApp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, you can't name a file async.py
😢
return AsyncOperation.from_operation(operation, self.pythonic_params) | ||
|
||
|
||
class AsyncMiddlewareApp(RoutedMiddleware[AsyncApi]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open for better names :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well...
There are only two hard things in Computer Science: cache invalidation and naming things.
This class is also an app, but a different app than the AsyncApp
?
|
||
|
||
def get_root_path(import_name: str) -> str: | ||
"""Copied from Flask: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied, since I don't want to have Flask as a fixed dependency anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a lawyer but there might be some implications for the license?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, BSD3 (Flask's license) is compatible with Apache 2 as long as you copy the notice, which can be found with the provided link.
@@ -22,9 +22,6 @@ def test_security_over_nonexistent_endpoints(oauth_requests, secure_api_app): | |||
) # type: flask.Response | |||
assert get_inexistent_endpoint.status_code == 401 | |||
|
|||
swagger_ui = app_client.get("/v1.0/ui/") # type: flask.Response | |||
assert swagger_ui.status_code == 401 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SwaggerUIMiddleware is positioned before the SecurityMiddleware, so its routes are not secured. It's probably possible to add separate security by subclassing the SwaggerUIMiddleware. We might want to provide this as a built-in option in the future.
flask_app = app.app | ||
proxied = ReverseProxied(flask_app.wsgi_app, script_name="/reverse_proxied/") | ||
flask_app.wsgi_app = proxied | ||
app.middleware = ReverseProxied(app.middleware, root_path="/reverse_proxied/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now wrap the ConnexionMiddleware without a AsgiToWsgi wrapper in between, so I had to update the middleware to work on ASGI instead of WSGI. I copied this from our reverse proxy example where I already implemented this earlier.
8fefd94
to
0bed769
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a first look, haven't fully grokked it yet, so while I left some smaller comments, didn't really have substantive comments
return AsyncOperation.from_operation(operation, self.pythonic_params) | ||
|
||
|
||
class AsyncMiddlewareApp(RoutedMiddleware[AsyncApi]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well...
There are only two hard things in Computer Science: cache invalidation and naming things.
This class is also an app, but a different app than the AsyncApp
?
return resolver_error_handler | ||
return None | ||
|
||
def replace(self, **changes) -> "_Options": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not ideal that this is a separate function that needs to be called (if I understand correctly) - is there a way to make it part of the initialization of the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is called after the initialization to update the instance with new options.
The instance is created in the Middleware __init__
method and updated in the add_api
method. This way, we can set default options for the middleware and override them on an API level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it now, the split is intentional :) I thought it was always something like
options = _Options(...)
options.replace(...)
|
||
|
||
def get_root_path(import_name: str) -> str: | ||
"""Copied from Flask: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a lawyer but there might be some implications for the license?
connexion/apps/asynchronous.py
Outdated
from connexion.resolver import Resolver | ||
from connexion.uri_parsing import AbstractURIParser | ||
|
||
logger = logging.getLogger("Connexion.app") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--> __main__
return resolver_error_handler | ||
return None | ||
|
||
def replace(self, **changes) -> "_Options": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it now, the split is intentional :) I thought it was always something like
options = _Options(...)
options.replace(...)
2199c02
to
a738fcf
Compare
This PR adds an interface for the ConnexionMiddleware, similar to the interface of the Connexion Apps.
The Connexion Apps are now a simple wrapper around the ConnexionMiddleware and framework app, delegating the work to the middleware. This enables a similar interface and behavior for users when using either the middleware or apps.
The arguments are repeated everywhere there is a user interface, but are parsed in a central place. Repeating the arguments is not DRY, but needed to provide users with IDE autocomplete, typing, etc. They are parsed in a single
_Options
class, which also provides a mechanism to set default options on an App level, and override them on the more granular API level.This makes the long list of provided parameters a lot more manageable, so I would like to use it for the
Jsonifier
as well, and re-add thedebug
andextra_files
arguments which I have dropped in previous PRs. I'll submit a separate PR for this.I renamed the
options
parameter toswagger_ui_options
since it only contains swagger UI options. This is a breaking change though, and we'll need to highlight this upon release.We still have quite a lot of
App
,MiddlewareApp
, and abstract classes. It would be great if we could find a way to reduce those further, or at least find better naming to make it more clear what each one does 🙂 .Finally, I added examples on how the middleware can be used with third party frameworks under
examples/frameworks
. Currently there's an example for Starlette and Quart, but this should be easy to extend. They also show how theASGIDecorator
andStarletteDecorator
from my previous PR can be used.