You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2024/04/16 08:44:09 UTC
[PR] Switch to Connexion 3 framework [airflow]
potiuk opened a new pull request, #39055:
URL: https://github.com/apache/airflow/pull/39055
This is a huge PR being result of over a 100 commits made by a number of people in ##36052 and #37638. It switches to Connexion 3 as the driving backend
implementation for both - Airflow REST APIs and Flask app that powers Airflow UI. It should be largely
backwards compatible when it comes to behaviour of both APIs and Airflow Webserver views, however due to decisions made by Connexion 3 maintainers, it changes heavily the technology stack used under-the-hood:
1) Connexion App is an ASGI-compatible Open-API spec-first
framework using ASGI as an interface between webserver
and Python web application. ASGI is an asynchronous
successor of WSGI.
2) Connexion itself is using Starlette to run asynchronous
web services in Python.
3) We continue using gunicorn appliation server that still
uses WSGI standard, which means that we can continue using
Flask and we are usig standard Uvicorn ASGI webserver that
converts the ASGI interface to WSGI interface of Gunicorn
Some of the problems handled in this PR
There were two problem was with session handling:
* the get_session_cookie - did not get the right cookie - it returned "session" string. The right fix was to change cookie_jar into cookie.jar because this is where apparently TestClient of starlette is holding the cookies (visible when you debug)
* The client does not accept "set_cookie" method - it accepts passing cookies via "cookies" dictionary - this is the usual httpx client
- see https://www.starlette.io/testclient/ - so we have to set cookie directly in the get method to try it out
Add "flask_client_with_login" for tests that neeed flask client
Some tests require functionality not available to Starlette test client as they use Flask test client specific features - for those we have an option to get flask test client instead of starlette one.
Fix error handling for new connection 3 approach
Error handling for Connexion 3 integration needed to be reworked.
The way it behaves is much the same as it works in main:
* for API errors - we get application/problem+json responses
* for UI erros - we have rendered views
* for redirection - we have correct location header (it's been missing)
* the api error handled was not added as available middleware in the www tests
It should fix all test_views_base.py tests which were failing on lack of location header for redirection.
Fix wrong response is tests_view_cluster_activity
The problem in the test was that Starlette Test Client opens a new connection and start new session, while flask test client uses the same database session. The test did not show data because the data was not committed and session was not closed - which also failed sqlite local tests with "database is locked" error.
Fix test_extra_links
The tests were failing again because the dagrun created was not committed and session not closed. This worked with flask client that used the same session accidentally but did not work with test client from Starlette. Also it caused "database locked" in sqlite / local tests.
Switch to non-deprecated auth manager
Fix to test_views_log.py
This PR partially fixes sessions and request parameter for test_views_log. Some tests are still failing but for different reasons - to be investigated.
Fix views_custom_user_views tests
The problem in those tests was that the check in security manager was based on the assumption that the security manager was shared between the client and test flask application - because they were coming from the same flask app. But when we use starlette, the call goes to a new process started and the user is deleted in the database - so the shortcut of checking the security manager did not work.
The change is that we are now checking if the user is deleted by calling /users/show (we need a new users READ permission for that)
- this way we go to the database and check if the user was indeed deleted.
Fix test_task_instance_endpoint tests
There were two reasons for the test failed:
* when the Job was added to task instance, the task instance was not merged in session, which means that commit did not store the added Job
* some of the tests were expecting a call with specific session and they failed because session was different. Replacing the session with mock.ANY tells pytest that this parameter can be anything - we will have different session when when the call will be made with ASGI/Starlette
Fix parameter validation
* added default value for limit parameter across the board. Connexion 3 does not like if the parameter had no default and we had not provided one - even if our custom decorated was adding it. Adding default value and updating our decorator to treat None as `default` fixed a number of problems where limits were not passed
* swapped openapi specification for /datasets/{uri} and /dataset/events. Since `{uri}` was defined first, connection matched `events` with `{uri}` and chose parameter definitions from `{uri}` not events
Fix test_log_enpoint tests
The problem here was that some sessions should be committed/closed but also in order to run it standalone we wanted to create log templates in the database - as it relied implcitly on log templates created by other tests.
Fix test_views_dagrun, test_views_tasks and test_views_log
Fixed by switching to use flask client for testing rather than starlette. Starlette client in this case has some side effects that are also impacting Sqlite's session being created in a different thread and deleted with close_all_sessions fixture.
Fix test_views_dagrun
Fixed by switching to use flask client for testing rather than starlette. Starlette client in this case has some side effects that are also impacting Sqlite's session being created in a different thread and deleted with close_all_sessions fixture.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570542326
##########
airflow/api_connexion/endpoints/connection_endpoint.py:
##########
@@ -91,7 +91,7 @@ def get_connection(*, connection_id: str, session: Session = NEW_SESSION) -> API
@provide_session
def get_connections(
*,
- limit: int,
+ limit: int | None = None,
Review Comment:
@RobbeSneyders: yes:
Example stack trace here (from https://github.com/apache/airflow/actions/runs/8581603105/job/23518857077 for example).
```
ERROR connexion.middleware.exceptions:exceptions.py:97 TypeError("get_dataset_events() missing 1 required keyword-only argument: 'limit'")
Traceback (most recent call last):
File "/usr/local/lib/python3.8/site-packages/starlette/_exception_handler.py", line 53, in wrapped_app
await app(scope, receive, sender)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/swagger_ui.py", line 222, in __call__
await self.router(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 756, in __call__
await self.middleware_stack(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 776, in app
await route.handle(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 485, in handle
await self.app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 756, in __call__
await self.middleware_stack(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 806, in app
await self.default(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/swagger_ui.py", line 235, in default_fn
await self.app(original_scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/middleware/cors.py", line 85, in __call__
await self.app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/middleware/cors.py", line 85, in __call__
await self.app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/routing.py", line 154, in __call__
await self.router(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 756, in __call__
await self.middleware_stack(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 776, in app
await route.handle(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 485, in handle
await self.app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 756, in __call__
await self.middleware_stack(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 776, in app
await route.handle(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 297, in handle
await self.app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/routing.py", line 48, in __call__
await self.next_app(original_scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/abstract.py", line 264, in __call__
return await operation(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/security.py", line 106, in __call__
await self.next_app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/abstract.py", line 264, in __call__
return await operation(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/request_validation.py", line 142, in __call__
await self.next_app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/abstract.py", line 264, in __call__
return await operation(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/lifespan.py", line 26, in __call__
await self.next_app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/abstract.py", line 264, in __call__
return await operation(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/middleware/context.py", line 25, in __call__
await self.next_app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/connexion/apps/flask.py", line 151, in __call__
return await self.asgi_app(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/a2wsgi/wsgi.py", line 165, in __call__
return await responder(scope, receive, send)
File "/usr/local/lib/python3.8/site-packages/a2wsgi/wsgi.py", line 200, in __call__
await self.loop.run_in_executor(
File "/usr/local/lib/python3.8/concurrent/futures/thread.py", line 57, in run
result = self.fn(*self.args, **self.kwargs)
File "/usr/local/lib/python3.8/site-packages/a2wsgi/wsgi.py", line 256, in wsgi
iterable = self.app(environ, start_response)
File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 2532, in wsgi_app
response = self.handle_exception(e)
File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 2529, in wsgi_app
response = self.full_dispatch_request()
File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1825, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1823, in full_dispatch_request
rv = self.dispatch_request()
File "/usr/local/lib/python3.8/site-packages/flask/app.py", line 1799, in dispatch_request
return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
File "/usr/local/lib/python3.8/site-packages/connexion/apps/flask.py", line 68, in __call__
return self.fn(*args, **kwargs)
File "/usr/local/lib/python3.8/site-packages/connexion/decorators/main.py", line 134, in wrapper
return decorated_function(request)
File "/usr/local/lib/python3.8/site-packages/connexion/decorators/response.py", line 171, in wrapper
handler_response = function(*args, **kwargs)
File "/usr/local/lib/python3.8/site-packages/connexion/decorators/parameter.py", line 87, in wrapper
return function(**kwargs)
File "/usr/local/lib/python3.8/site-packages/connexion/decorators/main.py", line 123, in wrapper
return function(*args, **kwargs)
File "/opt/airflow/airflow/api_connexion/security.py", line 182, in decorated
return _requires_access(
File "/opt/airflow/airflow/api_connexion/security.py", line 92, in _requires_access
return func(*args, **kwargs)
File "/opt/airflow/airflow/utils/session.py", line 79, in wrapper
return func(*args, session=session, **kwargs)
File "/opt/airflow/airflow/api_connexion/parameters.py", line 104, in wrapped_function
return func(*args, **kwargs)
TypeError: get_dataset_events() missing 1 required keyword-only argument: 'limit'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567011655
##########
airflow/api_connexion/openapi/v1.yaml:
##########
@@ -2186,6 +2171,30 @@ paths:
'404':
$ref: '#/components/responses/NotFound'
+ /datasets/{uri}:
Review Comment:
@RobbeSneyders -> this is one you might find interesting - it might be something that you might want to correct in parameter validation implemented by Connexion.
We have (and maybe that is not best) two endpoints here:
* /datasets/{uri}:
* /datasets/events
Where `/datasets/events` returns evnets specific for a dataset and `/dataset/{uri}` allows to query the datasets by their URI. They obviously overlap in a bad way, but I guess due to back-compatibility it won't be easy to avoid that. This works fine with OpenAPI in general (correct endpoints are executed when we run either) but there was a problem with Connexion 3 that when the `/dataset/{uri}` was specified before `/dataset/events` - parameter validation thought that `/dataset/events' parameters were wrong (because it toook /dataset/events` parameters as the source of truth) - we fixed it by moving the `/dataset/{uri}` after the `/dataset/events` and it works fine now - but you might want to handle it better: either by handling the case automatically or flagging when more specific datapoint is specified after the more generic one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567031173
##########
airflow/www/extensions/init_appbuilder_links.py:
##########
@@ -53,7 +53,7 @@ def init_appbuilder_links(app):
appbuilder.add_link(
name=RESOURCE_DOCS,
label="REST API Reference (Swagger UI)",
- href="/api/v1./api/v1_swagger_ui_index",
+ href="/api/v1/ui",
Review Comment:
We use embedded swagger UI now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570511980
##########
airflow/api_connexion/openapi/v1.yaml:
##########
@@ -2186,6 +2171,30 @@ paths:
'404':
$ref: '#/components/responses/NotFound'
+ /datasets/{uri}:
Review Comment:
Cool. Good it's addressed :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #39055:
URL: https://github.com/apache/airflow/pull/39055#issuecomment-2063673873
@RobbeSneyders
> I have to admit that the usage of Connexion by Airflow was a bit of a blind spot for us. Connexion was used differently here than we anticipated, leading to a much larger migration effort than for most of our users.
Indeed. It's a huge one.
> Aligning the implementation more with our intended interfaces should improve the robustness towards the future though. And the redesign of Connexion 3 makes Connexion itself more future-proof and easier to maintain as well. So there is definitely long-term value in this migration.
Yes. I truly hope we can make the decision eventually that we want to go this direction - seeing a number of changes and potential disruptions it may cause makes - of course - maintainers concerned. After trying it out and seeing what it takes, it's not relly a simple "library version" upgrade. So it's not likely the PR will make it into the codebase in this form. But I hope we can build a consensus and a task force to make it happen - so any comments and insights on what else we can do to make it more "standard" is really appreciated. I started a discussion - where I hope we can make at least decision on moving things in the right direction: https://lists.apache.org/thread/yrvp2mg25xcwznt8yr9dmdmxrmomwjs2
> Now that we understand better how Airflow uses Connexion, we will keep it in mind when taking future decisions and we hope that you will keep actively providing feedback. We would rather collaborate on improvements upstream than having you depend on workarounds.
Yes. If you can see something that could be improved and make our lives better - it will still take quite some time to make this one into "merged" status, so if there is anything Connexion maintainers could do to address the use case we have and make it esier and more future-proof to migrate it would be great.
> I already addressed a few of your comments, but need some more time to look into the remaining ones.
Please. I think a number of our decisions were mostly guesses, and even the way we have been integrating in the past were somewhat not the way things were intended originally, so any comments and insights from your side would be great.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567870137
##########
airflow/www/package.json:
##########
@@ -141,7 +141,8 @@
"reactflow": "^11.7.4",
"redoc": "^2.0.0-rc.72",
"remark-gfm": "^3.0.1",
- "swagger-ui-dist": "4.1.3",
+ "sanitize-html": "^2.12.1",
Review Comment:
Are you sure this dependency is needed? It was just removed when adding color logs. Or is this a merge-mistake? Can not see it used in JS code... (or I have tomatoes on my eyes?)
##########
airflow/api_connexion/endpoints/connection_endpoint.py:
##########
@@ -91,7 +91,7 @@ def get_connection(*, connection_id: str, session: Session = NEW_SESSION) -> API
@provide_session
def get_connections(
*,
- limit: int,
+ limit: int | None = None,
Review Comment:
If you want to reduce the complexity of this PR, could such simple bulk changes separated-out to s different PR and merged beforehand? Or would this influence/break the old Connexion setup?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567021838
##########
airflow/cli/commands/webserver_command.py:
##########
@@ -356,11 +356,11 @@ def webserver(args):
print(f"Starting the web server on port {args.port} and host {args.hostname}.")
app = create_app(testing=conf.getboolean("core", "unit_test_mode"))
app.run(
- debug=True,
- use_reloader=not app.config["TESTING"],
+ log_level="debug",
Review Comment:
TODO: We need to figure ouy our reloading here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570510659
##########
airflow/www/package.json:
##########
@@ -141,7 +141,8 @@
"reactflow": "^11.7.4",
"redoc": "^2.0.0-rc.72",
"remark-gfm": "^3.0.1",
- "swagger-ui-dist": "4.1.3",
+ "sanitize-html": "^2.12.1",
Review Comment:
Thanks. Yes, that's likely a bad merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570543905
##########
tests/conftest.py:
##########
@@ -1264,6 +1265,16 @@ def initialize_providers_manager():
ProvidersManager().initialize_providers_configuration()
+@pytest.fixture(autouse=True)
+def create_swagger_ui_dir_if_missing():
+ """
+ The directory needs to exist to satisfy starlette attempting to register it as middleware
+ :return:
+ """
+ swagger_ui_dir = AIRFLOW_SOURCES_ROOT_DIR / "airflow" / "www" / "static" / "dist" / "swagger-ui"
+ swagger_ui_dir.mkdir(exist_ok=True, parents=True)
Review Comment:
Good point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1573342812
##########
airflow/www/views.py:
##########
@@ -3574,7 +3573,7 @@ class RedocView(AirflowBaseView):
@expose("/redoc")
def redoc(self):
"""Redoc API documentation."""
- openapi_spec_url = url_for("/api/v1./api/v1_openapi_yaml")
+ openapi_spec_url = "/api/v1/openapi.yaml"
Review Comment:
I replaced it with relative references - which seem to work. but base_url/ wsgl_middleware/proxy does not work in this version so we need to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "mik-laj (via GitHub)" <gi...@apache.org>.
mik-laj commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1568609055
##########
airflow/www/app.py:
##########
@@ -49,19 +50,20 @@
)
from airflow.www.extensions.init_session import init_airflow_session_interface
from airflow.www.extensions.init_views import (
- init_api_auth_provider,
+ init_api_auth_manager,
init_api_connexion,
init_api_error_handlers,
init_api_experimental,
init_api_internal,
init_appbuilder_views,
+ init_cors_middleware,
init_error_handlers,
init_flash_views,
init_plugins,
)
from airflow.www.extensions.init_wsgi_middlewares import init_wsgi_middleware
-app: Flask | None = None
+app: connexion.FlaskApp | None = None
Review Comment:
I'm afraid this might break some plugins that might rely on this being an object from the Flask library here. Can we avoid this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567036645
##########
airflow/www/extensions/init_views.py:
##########
@@ -167,26 +170,6 @@ def init_error_handlers(app: Flask):
from airflow.www import views
app.register_error_handler(500, views.show_traceback)
- app.register_error_handler(404, views.not_found)
-
-
-def set_cors_headers_on_response(response):
Review Comment:
Replaced by CORS middleware.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567015376
##########
airflow/cli/commands/internal_api_command.py:
##########
@@ -102,7 +102,7 @@ def internal_api(args):
"--workers",
str(num_workers),
"--worker-class",
- str(args.workerclass),
+ "uvicorn.workers.UvicornWorker",
Review Comment:
TODO: We need to remove option of using different workers and only leave the uvicorn worker.
##########
airflow/cli/commands/internal_api_command.py:
##########
@@ -74,8 +74,8 @@ def internal_api(args):
log.info("Starting the Internal API server on port %s and host %s.", args.port, args.hostname)
app = create_app(testing=conf.getboolean("core", "unit_test_mode"))
app.run(
- debug=True, # nosec
- use_reloader=not app.config["TESTING"],
+ log_level="debug",
+ # reload=not app.app.config["TESTING"],
Review Comment:
TODO: We still need to sort out this reload option here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567001729
##########
airflow/api_connexion/openapi/v1.yaml:
##########
@@ -2020,7 +2028,7 @@ paths:
properties:
content:
type: string
- plain/text:
+ text/plain:
Review Comment:
That was a mistake in the specification
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1566990719
##########
airflow/api_connexion/endpoints/connection_endpoint.py:
##########
@@ -91,7 +91,7 @@ def get_connection(*, connection_id: str, session: Session = NEW_SESSION) -> API
@provide_session
def get_connections(
*,
- limit: int,
+ limit: int | None = None,
Review Comment:
@RobbeSneyders -> we found that the new Connexion 3 implements more validation and responds with error where no default values are provided for parameters that are no defined as optional (but they are provided by decorators - see above `@format_parameters` decorator - it will set the default value of the parameter if not set - but Connexion 3 will respond with "bad request" if the parameter has no default value. We fixed it by adding default None and reworking the decorator to check it and replace the default value (and apply range limits) if None is set, but I just wanted you to know and ask if that is intended behaviour/right way of fixing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567033750
##########
airflow/cli/commands/webserver_command.py:
##########
@@ -356,11 +356,11 @@ def webserver(args):
print(f"Starting the web server on port {args.port} and host {args.hostname}.")
app = create_app(testing=conf.getboolean("core", "unit_test_mode"))
app.run(
- debug=True,
- use_reloader=not app.config["TESTING"],
Review Comment:
https://github.com/apache/airflow/pull/39055#discussion_r1567021838
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567033471
##########
airflow/www/extensions/init_views.py:
##########
@@ -323,11 +316,20 @@ def init_api_experimental(app):
app.extensions["csrf"].exempt(endpoints.api_experimental)
-def init_api_auth_provider(app):
+def init_api_auth_manager(connexion_app: connexion.FlaskApp):
"""Initialize the API offered by the auth manager."""
auth_mgr = get_auth_manager()
- blueprint = auth_mgr.get_api_endpoints()
- if blueprint:
- base_paths.append(blueprint.url_prefix)
- app.register_blueprint(blueprint)
- app.extensions["csrf"].exempt(blueprint)
+ auth_mgr.set_api_endpoints(connexion_app)
+
+
+def init_cors_middleware(connexion_app: connexion.FlaskApp):
Review Comment:
@RobbeSneyders - followint your advice we replaced our custom CORS implementation with the middleware - does it look right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567020644
##########
airflow/cli/commands/internal_api_command.py:
##########
@@ -198,7 +198,8 @@ def start_and_monitor_gunicorn(args):
def create_app(config=None, testing=False):
"""Create a new instance of Airflow Internal API app."""
- flask_app = Flask(__name__)
+ connexion_app = connexion.FlaskApp(__name__)
Review Comment:
@RobbeSneyders - where we are doing what you recommended originally - so we are using connexion's FlaskApp to wrap and create Flask Application that Connexion will use - and then we are using that Flask application down the line to register our specific application endpoints, error handling and some "Flask" middleware (we still have not connected all our extensions to ASGI middleware because we wanted to minimise the changes needed. Does it sound about right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567012952
##########
airflow/api_connexion/parameters.py:
##########
@@ -41,7 +41,7 @@ def validate_istimezone(value: datetime) -> None:
raise BadRequest("Invalid datetime format", detail="Naive datetime is disallowed")
-def format_datetime(value: str) -> datetime:
+def format_datetime(value: str | None) -> datetime | None:
Review Comment:
We needdd to convert the decorators to handle None as default value - see https://github.com/apache/airflow/pull/39055/files#r1566990719
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567030490
##########
airflow/www/app.py:
##########
@@ -192,3 +209,8 @@ def purge_cached_app():
"""Remove the cached version of the app in global state."""
global app
app = None
+
+
+def cached_flask_app(config=None, testing=False):
Review Comment:
TODO: Needed for flask cli ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567067801
##########
airflow/www/extensions/init_views.py:
##########
@@ -220,90 +203,100 @@ def resolve(self, operation):
return _LazyResolution(self.resolve_function_from_operation_id, operation_id)
-class _CustomErrorRequestBodyValidator(RequestBodyValidator):
- """Custom request body validator that overrides error messages.
-
- By default, Connextion emits a very generic *None is not of type 'object'*
- error when receiving an empty request body (with the view specifying the
- body as non-nullable). We overrides it to provide a more useful message.
- """
-
- def validate_schema(self, data, url):
- if not self.is_null_value_valid and data is None:
- raise BadRequestProblem(detail="Request body must not be empty")
- return super().validate_schema(data, url)
+base_paths: list[str] = ["/auth/fab/v1"] # contains the list of base paths that have api endpoints
-base_paths: list[str] = [] # contains the list of base paths that have api endpoints
-
-
-def init_api_error_handlers(app: Flask) -> None:
+def init_api_error_handlers(connexion_app: connexion.FlaskApp) -> None:
"""Add error handlers for 404 and 405 errors for existing API paths."""
from airflow.www import views
- @app.errorhandler(404)
- def _handle_api_not_found(ex):
+ def _handle_api_not_found(error) -> ConnexionResponse | str:
+ from flask.globals import request
+
if any([request.path.startswith(p) for p in base_paths]):
# 404 errors are never handled on the blueprint level
# unless raised from a view func so actual 404 errors,
# i.e. "no route for it" defined, need to be handled
# here on the application level
- return common_error_handler(ex)
- else:
- return views.not_found(ex)
-
- @app.errorhandler(405)
- def _handle_method_not_allowed(ex):
- if any([request.path.startswith(p) for p in base_paths]):
- return common_error_handler(ex)
- else:
- return views.method_not_allowed(ex)
-
- app.register_error_handler(ProblemException, common_error_handler)
+ return connexion_app._http_exception(error)
+ return views.not_found(error)
+ def _handle_api_method_not_allowed(error) -> ConnexionResponse | str:
+ from flask.globals import request
-def init_api_connexion(app: Flask) -> None:
+ if any([request.path.startswith(p) for p in base_paths]):
+ return connexion_app._http_exception(error)
+ return views.method_not_allowed(error)
+
+ def _handle_redirect(
Review Comment:
@RobbeSneyders - here is a strange one that I need your comments on. When we run our tests with TestClient from Strlette, we got redirect errors returned by Connection - but they contained no `Location` header. Which mens that even if we set `follow_redirect=True` in the client, the redirects have not been followed, because redirects in httpx client only happen when "Location" header is present. It could be an error somewhere in the stack - I am not sure. We workarounded it with those custom redirect handlers - but I am not sure if this is correct way of doing it (or maybe that is a bug that should be fixed somewhere in connexion/starlette?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567544565
##########
airflow/www/app.py:
##########
@@ -70,7 +72,20 @@
def create_app(config=None, testing=False):
"""Create a new instance of Airflow WWW app."""
- flask_app = Flask(__name__)
+ connexion_app = connexion.FlaskApp(__name__)
+
+ @connexion_app.app.before_request
Review Comment:
Correct. The path is `/auth/fab/v1`. Ideally we should have a set containing all the paths that need to ignore CSRF. SO far I can see only 2: Airflow API and Fab API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567159018
##########
.github/workflows/basic-tests.yml:
##########
@@ -148,6 +148,8 @@ jobs:
env:
HATCH_ENV: "test"
working-directory: ./clients/python
+ - name: Compile www assets
+ run: breeze compile-www-assets
Review Comment:
TODO: Generic comment: the tests seem to pass all thests on Python 3.9 + but the Pytest tests do not exit - likely some cleanup needs to happen there. For now switching to DEfault Python only but needs to be fixed before merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "RobbeSneyders (via GitHub)" <gi...@apache.org>.
RobbeSneyders commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567861339
##########
airflow/cli/commands/internal_api_command.py:
##########
@@ -198,7 +198,8 @@ def start_and_monitor_gunicorn(args):
def create_app(config=None, testing=False):
"""Create a new instance of Airflow Internal API app."""
- flask_app = Flask(__name__)
+ connexion_app = connexion.FlaskApp(__name__)
Review Comment:
Yes that sounds fine.
##########
airflow/api_connexion/endpoints/connection_endpoint.py:
##########
@@ -91,7 +91,7 @@ def get_connection(*, connection_id: str, session: Session = NEW_SESSION) -> API
@provide_session
def get_connections(
*,
- limit: int,
+ limit: int | None = None,
Review Comment:
I can't immediately think of a change that could trigger this. I tried to reproduce it with a minimal example, but it works as expected:
```python
def insert(**kwargs):
def decorator(f):
async def wrapped_function():
return await f(**kwargs)
return wrapped_function
return decorator
@insert(name="Igor")
async def post_greeting(name: str):
return f"Hello {name}", 200
```
```yaml
paths:
/greeting:
post:
operationId: hello.post_greeting
parameters:
- name: name
in: query
schema:
type: string
```
Do you have a stack trace by any chance?
##########
airflow/www/extensions/init_views.py:
##########
@@ -220,90 +203,100 @@ def resolve(self, operation):
return _LazyResolution(self.resolve_function_from_operation_id, operation_id)
-class _CustomErrorRequestBodyValidator(RequestBodyValidator):
Review Comment:
Yes, this was merged upstream in https://github.com/spec-first/connexion/pull/1761
##########
airflow/providers/fab/auth_manager/fab_auth_manager.py:
##########
@@ -147,19 +149,24 @@ def get_cli_commands() -> list[CLICommand]:
SYNC_PERM_COMMAND, # not in a command group
]
- def get_api_endpoints(self) -> None | Blueprint:
+ def set_api_endpoints(self, connexion_app: connexion.FlaskApp) -> None:
folder = Path(__file__).parents[0].resolve() # this is airflow/auth/managers/fab/
with folder.joinpath("openapi", "v1.yaml").open() as f:
specification = safe_load(f)
- return FlaskApi(
+
+ swagger_ui_options = SwaggerUIOptions(
Review Comment:
Yes, the ownership of the bundled Swagger UI has been donated to our spec-first organization, so we no longer have a dependency on third-party maintainers to update this.
##########
airflow/api_connexion/openapi/v1.yaml:
##########
@@ -2186,6 +2171,30 @@ paths:
'404':
$ref: '#/components/responses/NotFound'
+ /datasets/{uri}:
Review Comment:
Thanks, this is a [known change](https://connexion.readthedocs.io/en/latest/v3.html#:~:text=Route%20priority%20changed.%20The%20most%20specific%20route%20should%20now%20be%20defined%20first%20in%20the%20specification.) compared to Connexion 2 due to different route priority in Starlette compared to Flask. There is an [open PR](https://github.com/spec-first/connexion/pull/1898) to handle this automatically by sorting the paths before registering them. So this should be fixed soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567061989
##########
airflow/www/extensions/init_views.py:
##########
@@ -220,90 +203,100 @@ def resolve(self, operation):
return _LazyResolution(self.resolve_function_from_operation_id, operation_id)
-class _CustomErrorRequestBodyValidator(RequestBodyValidator):
- """Custom request body validator that overrides error messages.
-
- By default, Connextion emits a very generic *None is not of type 'object'*
- error when receiving an empty request body (with the view specifying the
- body as non-nullable). We overrides it to provide a more useful message.
- """
-
- def validate_schema(self, data, url):
- if not self.is_null_value_valid and data is None:
- raise BadRequestProblem(detail="Request body must not be empty")
- return super().validate_schema(data, url)
+base_paths: list[str] = ["/auth/fab/v1"] # contains the list of base paths that have api endpoints
-base_paths: list[str] = [] # contains the list of base paths that have api endpoints
-
-
-def init_api_error_handlers(app: Flask) -> None:
+def init_api_error_handlers(connexion_app: connexion.FlaskApp) -> None:
Review Comment:
@RobbeSneyders - here, I think we need your expert review on error handling here:
We had to do custom eror handlng here, because we mix API / webapp in the same app and we wanted to handle them differently:
For API "/api/v1" and "/auth/fab" and "/internal/api" - we want to return `application/error+json` , for regular web app errors we want to provide custom flask-rendered view with Airflow logo and "go back to main" link.
That's why we had t handle errors a bit differently - and handle them on both `Flask` and `Connexion` level - and I am not sure if it's the right approach - maybe it can be done better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570509947
##########
airflow/providers/fab/auth_manager/fab_auth_manager.py:
##########
@@ -147,19 +149,24 @@ def get_cli_commands() -> list[CLICommand]:
SYNC_PERM_COMMAND, # not in a command group
]
- def get_api_endpoints(self) -> None | Blueprint:
+ def set_api_endpoints(self, connexion_app: connexion.FlaskApp) -> None:
folder = Path(__file__).parents[0].resolve() # this is airflow/auth/managers/fab/
with folder.joinpath("openapi", "v1.yaml").open() as f:
specification = safe_load(f)
- return FlaskApi(
+
+ swagger_ui_options = SwaggerUIOptions(
Review Comment:
Thanks! Resolving then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570547457
##########
airflow/www/views.py:
##########
@@ -3574,7 +3573,7 @@ class RedocView(AirflowBaseView):
@expose("/redoc")
def redoc(self):
"""Redoc API documentation."""
- openapi_spec_url = url_for("/api/v1./api/v1_openapi_yaml")
+ openapi_spec_url = "/api/v1/openapi.yaml"
Review Comment:
Good point. we'll keep it in mind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567025218
##########
airflow/providers/fab/auth_manager/fab_auth_manager.py:
##########
@@ -147,19 +149,24 @@ def get_cli_commands() -> list[CLICommand]:
SYNC_PERM_COMMAND, # not in a command group
]
- def get_api_endpoints(self) -> None | Blueprint:
+ def set_api_endpoints(self, connexion_app: connexion.FlaskApp) -> None:
folder = Path(__file__).parents[0].resolve() # this is airflow/auth/managers/fab/
with folder.joinpath("openapi", "v1.yaml").open() as f:
specification = safe_load(f)
- return FlaskApi(
+
+ swagger_ui_options = SwaggerUIOptions(
Review Comment:
@RobbeSneyders -> here we switched back to the swagger UI that is built-in (we used to have an external one) - I guess you are committed to update the swagger in the Connexion 3 as it gets released (one of the main reason we used external swagger was that the connexion 2 embedded version at the time had security issue).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567028296
##########
airflow/www/app.py:
##########
@@ -70,7 +72,20 @@
def create_app(config=None, testing=False):
"""Create a new instance of Airflow WWW app."""
- flask_app = Flask(__name__)
+ connexion_app = connexion.FlaskApp(__name__)
+
+ @connexion_app.app.before_request
Review Comment:
Here the question is if that exemption here from CSRF is teh right one
TODO: also add FAB cc: @vincbeck ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570544359
##########
airflow/cli/commands/internal_api_command.py:
##########
@@ -102,7 +102,7 @@ def internal_api(args):
"--workers",
str(num_workers),
"--worker-class",
- str(args.workerclass),
+ "uvicorn.workers.UvicornWorker",
Review Comment:
yes. That's in the TODOs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "mik-laj (via GitHub)" <gi...@apache.org>.
mik-laj commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1568603690
##########
airflow/www/views.py:
##########
@@ -3574,7 +3573,7 @@ class RedocView(AirflowBaseView):
@expose("/redoc")
def redoc(self):
"""Redoc API documentation."""
- openapi_spec_url = url_for("/api/v1./api/v1_openapi_yaml")
+ openapi_spec_url = "/api/v1/openapi.yaml"
Review Comment:
I'm not sure if this won't cause problems when the `AIRFLOW__WEBSERVER__BASE_URL` env var is set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570515070
##########
airflow/api_connexion/endpoints/connection_endpoint.py:
##########
@@ -91,7 +91,7 @@ def get_connection(*, connection_id: str, session: Session = NEW_SESSION) -> API
@provide_session
def get_connections(
*,
- limit: int,
+ limit: int | None = None,
Review Comment:
We could do that, yes @jscheffl eventually, yes but I would like to avoid "polluting" current code without knowing how we are proceeding with the whole PR. For me this is more of POC for comments and deciding what's next rather than something we want to actively start merging `now`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570545750
##########
airflow/www/app.py:
##########
@@ -70,7 +72,20 @@
def create_app(config=None, testing=False):
"""Create a new instance of Airflow WWW app."""
- flask_app = Flask(__name__)
+ connexion_app = connexion.FlaskApp(__name__)
+
+ @connexion_app.app.before_request
Review Comment:
Yes. If we go that direction we should likely propose a better way than it is currently (it's a bit messy).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567011131
##########
airflow/www/extensions/init_views.py:
##########
@@ -220,90 +203,100 @@ def resolve(self, operation):
return _LazyResolution(self.resolve_function_from_operation_id, operation_id)
-class _CustomErrorRequestBodyValidator(RequestBodyValidator):
- """Custom request body validator that overrides error messages.
-
- By default, Connextion emits a very generic *None is not of type 'object'*
- error when receiving an empty request body (with the view specifying the
- body as non-nullable). We overrides it to provide a more useful message.
- """
-
- def validate_schema(self, data, url):
- if not self.is_null_value_valid and data is None:
- raise BadRequestProblem(detail="Request body must not be empty")
- return super().validate_schema(data, url)
+base_paths: list[str] = ["/auth/fab/v1"] # contains the list of base paths that have api endpoints
-base_paths: list[str] = [] # contains the list of base paths that have api endpoints
-
-
-def init_api_error_handlers(app: Flask) -> None:
+def init_api_error_handlers(connexion_app: connexion.FlaskApp) -> None:
"""Add error handlers for 404 and 405 errors for existing API paths."""
from airflow.www import views
- @app.errorhandler(404)
- def _handle_api_not_found(ex):
+ def _handle_api_not_found(error) -> ConnexionResponse | str:
+ from flask.globals import request
+
if any([request.path.startswith(p) for p in base_paths]):
# 404 errors are never handled on the blueprint level
# unless raised from a view func so actual 404 errors,
# i.e. "no route for it" defined, need to be handled
# here on the application level
- return common_error_handler(ex)
- else:
- return views.not_found(ex)
-
- @app.errorhandler(405)
- def _handle_method_not_allowed(ex):
- if any([request.path.startswith(p) for p in base_paths]):
- return common_error_handler(ex)
- else:
- return views.method_not_allowed(ex)
-
- app.register_error_handler(ProblemException, common_error_handler)
+ return connexion_app._http_exception(error)
+ return views.not_found(error)
+ def _handle_api_method_not_allowed(error) -> ConnexionResponse | str:
+ from flask.globals import request
-def init_api_connexion(app: Flask) -> None:
+ if any([request.path.startswith(p) for p in base_paths]):
+ return connexion_app._http_exception(error)
+ return views.method_not_allowed(error)
+
+ def _handle_redirect(
+ request: ConnexionRequest, ex: starlette.exceptions.HTTPException
+ ) -> ConnexionResponse:
+ return problem(
+ title=connexion.http_facts.HTTP_STATUS_CODES.get(ex.status_code),
+ detail=ex.detail,
+ headers={"Location": ex.detail},
+ status=ex.status_code,
+ )
+
+ # in case of 404 and 405 we handle errors at the Flask APP level in order to have access to
+ # context and be able to render the error page for the UI
+ connexion_app.app.register_error_handler(404, _handle_api_not_found)
+ connexion_app.app.register_error_handler(405, _handle_api_method_not_allowed)
+
+ # We should handle redirects at connexion_app level - the requests will be redirected to the target
+ # location - so they can return application/problem+json response with the Location header regardless
+ # ot the request path - does not matter if it is API or UI request
+ connexion_app.add_error_handler(301, _handle_redirect)
+ connexion_app.add_error_handler(302, _handle_redirect)
+ connexion_app.add_error_handler(307, _handle_redirect)
+ connexion_app.add_error_handler(308, _handle_redirect)
+
+ # Everything else we handle at the connexion_app level by default error handler
+ connexion_app.add_error_handler(ProblemException, problem_error_handler)
+
+
+def init_api_connexion(connexion_app: connexion.FlaskApp) -> None:
"""Initialize Stable API."""
base_path = "/api/v1"
base_paths.append(base_path)
with ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml").open() as f:
specification = safe_load(f)
- api_bp = FlaskApi(
+ swagger_ui_options = SwaggerUIOptions(
+ swagger_ui=conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
+ swagger_ui_template_dir=os.fspath(ROOT_APP_DIR.joinpath("www", "static", "dist", "swagger-ui")),
+ )
+
+ connexion_app.add_api(
specification=specification,
resolver=_LazyResolver(),
base_path=base_path,
- options={"swagger_ui": SWAGGER_ENABLED, "swagger_path": SWAGGER_BUNDLE.__fspath__()},
+ swagger_ui_options=swagger_ui_options,
strict_validation=True,
validate_responses=True,
- validator_map={"body": _CustomErrorRequestBodyValidator},
- ).blueprint
- api_bp.after_request(set_cors_headers_on_response)
-
- app.register_blueprint(api_bp)
- app.extensions["csrf"].exempt(api_bp)
+ )
-def init_api_internal(app: Flask, standalone_api: bool = False) -> None:
+def init_api_internal(connexion_app: connexion.FlaskApp, standalone_api: bool = False) -> None:
"""Initialize Internal API."""
if not standalone_api and not conf.getboolean("webserver", "run_internal_api", fallback=False):
return
base_paths.append("/internal_api/v1")
with ROOT_APP_DIR.joinpath("api_internal", "openapi", "internal_api_v1.yaml").open() as f:
specification = safe_load(f)
- api_bp = FlaskApi(
+ swagger_ui_options = SwaggerUIOptions(
+ swagger_ui=conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
+ swagger_ui_template_dir=os.fspath(ROOT_APP_DIR.joinpath("www", "static", "dist", "swagger-ui")),
+ )
Review Comment:
Same here with swagger
##########
airflow/cli/commands/internal_api_command.py:
##########
@@ -74,8 +74,8 @@ def internal_api(args):
log.info("Starting the Internal API server on port %s and host %s.", args.port, args.hostname)
app = create_app(testing=conf.getboolean("core", "unit_test_mode"))
app.run(
- debug=True, # nosec
- use_reloader=not app.config["TESTING"],
+ log_level="debug",
+ # reload=not app.app.config["TESTING"],
Review Comment:
I guess we need to do something with that
##########
airflow/www/extensions/init_views.py:
##########
@@ -220,90 +203,100 @@ def resolve(self, operation):
return _LazyResolution(self.resolve_function_from_operation_id, operation_id)
-class _CustomErrorRequestBodyValidator(RequestBodyValidator):
- """Custom request body validator that overrides error messages.
-
- By default, Connextion emits a very generic *None is not of type 'object'*
- error when receiving an empty request body (with the view specifying the
- body as non-nullable). We overrides it to provide a more useful message.
- """
-
- def validate_schema(self, data, url):
- if not self.is_null_value_valid and data is None:
- raise BadRequestProblem(detail="Request body must not be empty")
- return super().validate_schema(data, url)
+base_paths: list[str] = ["/auth/fab/v1"] # contains the list of base paths that have api endpoints
-base_paths: list[str] = [] # contains the list of base paths that have api endpoints
-
-
-def init_api_error_handlers(app: Flask) -> None:
+def init_api_error_handlers(connexion_app: connexion.FlaskApp) -> None:
"""Add error handlers for 404 and 405 errors for existing API paths."""
from airflow.www import views
- @app.errorhandler(404)
- def _handle_api_not_found(ex):
+ def _handle_api_not_found(error) -> ConnexionResponse | str:
+ from flask.globals import request
+
if any([request.path.startswith(p) for p in base_paths]):
# 404 errors are never handled on the blueprint level
# unless raised from a view func so actual 404 errors,
# i.e. "no route for it" defined, need to be handled
# here on the application level
- return common_error_handler(ex)
- else:
- return views.not_found(ex)
-
- @app.errorhandler(405)
- def _handle_method_not_allowed(ex):
- if any([request.path.startswith(p) for p in base_paths]):
- return common_error_handler(ex)
- else:
- return views.method_not_allowed(ex)
-
- app.register_error_handler(ProblemException, common_error_handler)
+ return connexion_app._http_exception(error)
+ return views.not_found(error)
+ def _handle_api_method_not_allowed(error) -> ConnexionResponse | str:
+ from flask.globals import request
-def init_api_connexion(app: Flask) -> None:
+ if any([request.path.startswith(p) for p in base_paths]):
+ return connexion_app._http_exception(error)
+ return views.method_not_allowed(error)
+
+ def _handle_redirect(
+ request: ConnexionRequest, ex: starlette.exceptions.HTTPException
+ ) -> ConnexionResponse:
+ return problem(
+ title=connexion.http_facts.HTTP_STATUS_CODES.get(ex.status_code),
+ detail=ex.detail,
+ headers={"Location": ex.detail},
+ status=ex.status_code,
+ )
+
+ # in case of 404 and 405 we handle errors at the Flask APP level in order to have access to
+ # context and be able to render the error page for the UI
+ connexion_app.app.register_error_handler(404, _handle_api_not_found)
+ connexion_app.app.register_error_handler(405, _handle_api_method_not_allowed)
+
+ # We should handle redirects at connexion_app level - the requests will be redirected to the target
+ # location - so they can return application/problem+json response with the Location header regardless
+ # ot the request path - does not matter if it is API or UI request
+ connexion_app.add_error_handler(301, _handle_redirect)
+ connexion_app.add_error_handler(302, _handle_redirect)
+ connexion_app.add_error_handler(307, _handle_redirect)
+ connexion_app.add_error_handler(308, _handle_redirect)
+
+ # Everything else we handle at the connexion_app level by default error handler
+ connexion_app.add_error_handler(ProblemException, problem_error_handler)
+
+
+def init_api_connexion(connexion_app: connexion.FlaskApp) -> None:
"""Initialize Stable API."""
base_path = "/api/v1"
base_paths.append(base_path)
with ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml").open() as f:
specification = safe_load(f)
- api_bp = FlaskApi(
+ swagger_ui_options = SwaggerUIOptions(
+ swagger_ui=conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
+ swagger_ui_template_dir=os.fspath(ROOT_APP_DIR.joinpath("www", "static", "dist", "swagger-ui")),
Review Comment:
We have a constants for Swagger in https://github.com/apache/airflow/blob/79603f9302b5344bc480a42ec31dee4be35fb1b8/airflow/www/constants.py
##########
airflow/cli/commands/webserver_command.py:
##########
@@ -356,11 +356,11 @@ def webserver(args):
print(f"Starting the web server on port {args.port} and host {args.hostname}.")
app = create_app(testing=conf.getboolean("core", "unit_test_mode"))
app.run(
- debug=True,
- use_reloader=not app.config["TESTING"],
Review Comment:
Same here with reloader
##########
airflow/cli/commands/webserver_command.py:
##########
@@ -384,7 +384,7 @@ def webserver(args):
"--workers",
str(num_workers),
"--worker-class",
- str(args.workerclass),
+ "uvicorn.workers.UvicornWorker",
Review Comment:
Same here about workerclass
##########
tests/conftest.py:
##########
@@ -1264,6 +1265,16 @@ def initialize_providers_manager():
ProvidersManager().initialize_providers_configuration()
+@pytest.fixture(autouse=True)
+def create_swagger_ui_dir_if_missing():
+ """
+ The directory needs to exist to satisfy starlette attempting to register it as middleware
+ :return:
+ """
+ swagger_ui_dir = AIRFLOW_SOURCES_ROOT_DIR / "airflow" / "www" / "static" / "dist" / "swagger-ui"
+ swagger_ui_dir.mkdir(exist_ok=True, parents=True)
Review Comment:
I think better propagate it in the CI and raise an error if `"static" / "dist"` not available, it might prevent some serious error rather than silence it
##########
airflow/cli/commands/internal_api_command.py:
##########
@@ -102,7 +102,7 @@ def internal_api(args):
"--workers",
str(num_workers),
"--worker-class",
- str(args.workerclass),
+ "uvicorn.workers.UvicornWorker",
Review Comment:
We should warn that provide `workerclass` has no affect anymore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567036132
##########
airflow/www/extensions/init_views.py:
##########
@@ -220,90 +203,100 @@ def resolve(self, operation):
return _LazyResolution(self.resolve_function_from_operation_id, operation_id)
-class _CustomErrorRequestBodyValidator(RequestBodyValidator):
Review Comment:
@RobbeSneyders - we removed the custom error request body validator completely and rely on validation done by Connexion 3- which i think should be the right call - since Connexion performs the validation now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1573342339
##########
airflow/www/app.py:
##########
@@ -49,35 +50,50 @@
)
from airflow.www.extensions.init_session import init_airflow_session_interface
from airflow.www.extensions.init_views import (
- init_api_auth_provider,
+ init_api_auth_manager,
init_api_connexion,
init_api_error_handlers,
init_api_experimental,
init_api_internal,
init_appbuilder_views,
+ init_cors_middleware,
init_error_handlers,
init_flash_views,
init_plugins,
)
from airflow.www.extensions.init_wsgi_middlewares import init_wsgi_middleware
app: Flask | None = None
-
+connexion_app: connexion.FlaskApp | None = None
# Initializes at the module level, so plugins can access it.
# See: /docs/plugins.rst
csrf = CSRFProtect()
-def create_app(config=None, testing=False):
+def create_connexion_app(config=None, testing=False):
"""Create a new instance of Airflow WWW app."""
- flask_app = Flask(__name__)
+ conn_app = connexion.FlaskApp(__name__)
+
+ @conn_app.app.before_request
+ def before_request():
+ """Exempts the view function associated with '/api/v1' requests from CSRF protection."""
+ if request.path.startswith("/api/v1"): # TODO: make sure this path is correct
+ view_function = conn_app.app.view_functions.get(request.endpoint)
+ if view_function:
+ # Exempt the view function from CSRF protection
+ conn_app.app.extensions["csrf"].exempt(view_function)
+
+ init_cors_middleware(conn_app)
+
+ flask_app = conn_app.app
Review Comment:
TODO: init_wsgl_middleware/proxy_fix does not work currently -> BASE_URL does not work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1573343176
##########
airflow/www/app.py:
##########
@@ -49,19 +50,20 @@
)
from airflow.www.extensions.init_session import init_airflow_session_interface
from airflow.www.extensions.init_views import (
- init_api_auth_provider,
+ init_api_auth_manager,
init_api_connexion,
init_api_error_handlers,
init_api_experimental,
init_api_internal,
init_appbuilder_views,
+ init_cors_middleware,
init_error_handlers,
init_flash_views,
init_plugins,
)
from airflow.www.extensions.init_wsgi_middlewares import init_wsgi_middleware
-app: Flask | None = None
+app: connexion.FlaskApp | None = None
Review Comment:
I fixed that by bringing back the old setup - now `app` is `Flask` app and we have `connexion_app` which is `connexion.FlaskApp`. This should be fully backwards compatible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1566983509
##########
.github/workflows/basic-tests.yml:
##########
@@ -148,6 +148,8 @@ jobs:
env:
HATCH_ENV: "test"
working-directory: ./clients/python
+ - name: Compile www assets
Review Comment:
We need to have assets compiled here to test Python client. Right now the API uses UI from Swagger that expects Javascript to be compiled in order to make API calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567013950
##########
airflow/cli/commands/internal_api_command.py:
##########
@@ -74,8 +74,8 @@ def internal_api(args):
log.info("Starting the Internal API server on port %s and host %s.", args.port, args.hostname)
app = create_app(testing=conf.getboolean("core", "unit_test_mode"))
app.run(
- debug=True, # nosec
- use_reloader=not app.config["TESTING"],
+ log_level="debug",
+ # reload=not app.app.config["TESTING"],
Review Comment:
We still need to sort out this reload option here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567040463
##########
newsfragments/37638.significant.rst:
##########
@@ -0,0 +1,4 @@
+Replaced test_should_respond_400_on_invalid_request with test_ignore_read_only_fields in the test_dag_endpoint.py.
Review Comment:
TODO: We need to add a few more notes likely here and change the newsfragment number to reflect the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1567021838
##########
airflow/cli/commands/webserver_command.py:
##########
@@ -356,11 +356,11 @@ def webserver(args):
print(f"Starting the web server on port {args.port} and host {args.hostname}.")
app = create_app(testing=conf.getboolean("core", "unit_test_mode"))
app.run(
- debug=True,
- use_reloader=not app.config["TESTING"],
+ log_level="debug",
Review Comment:
TODO: We need to fugr our reloading here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1570549535
##########
airflow/www/app.py:
##########
@@ -49,19 +50,20 @@
)
from airflow.www.extensions.init_session import init_airflow_session_interface
from airflow.www.extensions.init_views import (
- init_api_auth_provider,
+ init_api_auth_manager,
init_api_connexion,
init_api_error_handlers,
init_api_experimental,
init_api_internal,
init_appbuilder_views,
+ init_cors_middleware,
init_error_handlers,
init_flash_views,
init_plugins,
)
from airflow.www.extensions.init_wsgi_middlewares import init_wsgi_middleware
-app: Flask | None = None
+app: connexion.FlaskApp | None = None
Review Comment:
Yes. Good point, If we go that direction, likely we might want to move Connexion FlaskApp to `connexion_app` or similar field, and leave the `app` one as `Flask` app. Having two `app` is grossly misleading.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1578443041
##########
airflow/www/app.py:
##########
@@ -192,3 +209,8 @@ def purge_cached_app():
"""Remove the cached version of the app in global state."""
global app
app = None
+
+
+def cached_flask_app(config=None, testing=False):
Review Comment:
Replaced with both app cached.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [DRAFT] Switch to Connexion 3 framework [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #39055:
URL: https://github.com/apache/airflow/pull/39055#discussion_r1578442260
##########
airflow/www/package.json:
##########
@@ -141,7 +141,8 @@
"reactflow": "^11.7.4",
"redoc": "^2.0.0-rc.72",
"remark-gfm": "^3.0.1",
- "swagger-ui-dist": "4.1.3",
+ "sanitize-html": "^2.12.1",
Review Comment:
Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org