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()
[email protected](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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
