pierrejeambrun commented on code in PR #72026:
URL: https://github.com/apache/airflow/pull/72026#discussion_r3879319485
##########
airflow-core/tests/unit/api_fastapi/test_app.py:
##########
@@ -88,6 +89,17 @@ def test_all_apps(mock_create_task_exec_api,
mock_init_plugins, mock_init_views,
mock_create_task_exec_api.assert_called_once_with()
[email protected]("apps", ["all", "core", "execution"])
+def test_access_log_middleware_installed_for_every_apps_selection(apps,
client):
+ """Both server backends disable their own access logger, so a selection
that skips this
+ middleware has no access logging at all; it must also stay outermost so
the duration it
+ logs covers every inner middleware."""
+ installed = [m.cls for m in client(apps=apps).app.user_middleware]
+
+ assert installed.count(HttpAccessLogMiddleware) == 1
+ assert installed[0] is HttpAccessLogMiddleware
Review Comment:
```suggestion
```
##########
airflow-core/src/airflow/api_fastapi/app.py:
##########
@@ -152,6 +153,11 @@ def create_app(apps: str = "all") -> FastAPI:
init_error_handlers(app)
init_middlewares(app)
+ # Every server backend disables its own access logger, so this is the only
producer of access
+ # records and has to be installed whatever ``apps`` selects. Added last to
stay outermost and
+ # time the full request lifecycle.
Review Comment:
I would remove or trim down this comment. It's really verbose.
Also to be consistent with other middleware initialisation, i would put this
in `init_log()` or something function. Put this there, so you can document the
function properly without polluting the `create_app`
##########
airflow-core/newsfragments/72026.bugfix.rst:
##########
Review Comment:
This file needs to be removed. We do not use newsfragment unless it's
'significant' one. This is a regular bugfix so we remove 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
