From 4330520bf841f71c2c07e8f480e54e44524f8679 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Tue, 3 Dec 2024 13:19:04 -0800 Subject: [PATCH] Use case-insensitive form middleware for UWS support Convert the UWS support to the newly-added case-insensitive form middleware, which removes the need for annoying workarounds for case-insensitive form parameters. Update the application documentation accordingly. --- changelog.d/20241203_132727_rra_DM_47928.md | 3 + docs/user-guide/uws/define-inputs.rst | 31 +++----- safir/src/safir/uws/__init__.py | 2 - safir/src/safir/uws/_app.py | 13 ++- safir/src/safir/uws/_dependencies.py | 58 +------------- safir/src/safir/uws/_handlers.py | 88 +++------------------ 6 files changed, 38 insertions(+), 157 deletions(-) create mode 100644 changelog.d/20241203_132727_rra_DM_47928.md diff --git a/changelog.d/20241203_132727_rra_DM_47928.md b/changelog.d/20241203_132727_rra_DM_47928.md new file mode 100644 index 00000000..d23fc7d9 --- /dev/null +++ b/changelog.d/20241203_132727_rra_DM_47928.md @@ -0,0 +1,3 @@ +### Backwards-incompatible changes + +- Case-insensitivity of form `POST` parameters to UWS routes is now handled by middleware, and the `uws_post_params_dependency` function has been removed. Input parameter dependencies for UWS applications can now assume that all parameter keys will be in lowercase. diff --git a/docs/user-guide/uws/define-inputs.rst b/docs/user-guide/uws/define-inputs.rst index cec1826f..4a543a88 100644 --- a/docs/user-guide/uws/define-inputs.rst +++ b/docs/user-guide/uws/define-inputs.rst @@ -20,8 +20,8 @@ See :doc:`define-models` for how to do that. All FastAPI dependencies provided by your application must return a list of `UWSJobParameter` objects. The ``parameter_id`` attribute is the key and the ``value`` attribute is the value. -The key (the ``parameter_id``) is case-insensitive in the input. -When creating a `UWSJobParameter` object, it should be converted to lowercase (by using ``.lower()``, for example) so that the rest of the service can assume the lowercase form. +The key (the ``parameter_id``) is case-insensitive in the input, but it will be lowercased by middleware installed by Safir. +You will therefore always see lowercase query and form parameters in your dependency and do not have to handle other case possibilities. UWS allows the same ``parameter_id`` to occur multiple times with different values. For example, multiple ``id`` parameters may specify multiple input objects for a bulk operation that processes all of the input objects at the same time. @@ -67,7 +67,7 @@ Here is an example for a SODA service that performs circular cutouts: async def post_params_dependency( *, id: Annotated[ - str | list[str] | None, + list[str] | None, Form( title="Source ID", description=( @@ -77,7 +77,7 @@ Here is an example for a SODA service that performs circular cutouts: ), ] = None, circle: Annotated[ - str | list[str] | None, + list[str] | None, Form( title="Cutout circle positions", description=( @@ -88,23 +88,21 @@ Here is an example for a SODA service that performs circular cutouts: ), ), ] = None, - params: Annotated[ - list[UWSJobParameter], Depends(uws_post_params_dependency) - ], ) -> list[UWSJobParameter]: """Parse POST parameters into job parameters for a cutout.""" - return [p for p in params if p.parameter_id in {"id", "circle"}] + params = [] + for i in id: + params.append(UWSJobParameter(paramater_id="id", value=i)) + for c in circle: + params.append(UWSJobParameter(parameter_id="circle", value=c)) + return params This first declares the input parameters, with full documentation, as FastAPI ``Form`` parameters. -Note that the type is ``str | list[str]``, which allows the parameter to be specified multiple times. -Unfortunately, supporting UWS's case-insensitivity is obnoxious in FastAPI. -This is the purpose for the extra ``params`` argument that uses `uws_post_params_dependency`. -The explicitly-declared parameters are there only to generate API documentation and are not used directly. -Instead, the ``params`` argument collects all of the input form parameters and converts them into a canonical form for you, regardless of the case used for the key. -The body of the function then only needs to filter those parameters down to the ones that are relevant for your application and return them. +Note that the type is ``list[str]``, which allows the parameter to be specified multiple times. +If the parameters for your service cannot be repeated, change this to `str` (or another appropriate basic type, such as `int`). -You do not need to do any input validation here. +You do not need to do any input validation of the parameter values here. This will be done later as part of converting the input parameters to your parameter model, as defined in :doc:`define-models`. Async POST configuration @@ -213,9 +211,6 @@ Here is an example dependency for a cutout service: if k in {"id", "circle"} ] -This code is somewhat simpler and doesn't need `uws_post_params_dependency`. -The UWS library installs FastAPI middleware that canonicalizes the case of all query parameter keys, so your application can assume they are lowercase. - As in the other cases, you will then need to pass a `UWSRoute` object as the ``sync_get_route`` argument to `UWSAppSettings.build_uws_config`. Here is an example: diff --git a/safir/src/safir/uws/__init__.py b/safir/src/safir/uws/__init__.py index 2c1601b9..309c342e 100644 --- a/safir/src/safir/uws/__init__.py +++ b/safir/src/safir/uws/__init__.py @@ -2,7 +2,6 @@ from ._app import UWSApplication from ._config import ParametersModel, UWSAppSettings, UWSConfig, UWSRoute -from ._dependencies import uws_post_params_dependency from ._exceptions import ( DatabaseSchemaError, MultiValuedParameterError, @@ -38,5 +37,4 @@ "UWSRoute", "UWSSchemaBase", "UsageError", - "uws_post_params_dependency", ] diff --git a/safir/src/safir/uws/_app.py b/safir/src/safir/uws/_app.py index 889af80f..6661f396 100644 --- a/safir/src/safir/uws/_app.py +++ b/safir/src/safir/uws/_app.py @@ -20,7 +20,10 @@ is_database_current, stamp_database_async, ) -from safir.middleware.ivoa import CaseInsensitiveQueryMiddleware +from safir.middleware.ivoa import ( + CaseInsensitiveFormMiddleware, + CaseInsensitiveQueryMiddleware, +) from ._config import UWSConfig from ._constants import UWS_DATABASE_TIMEOUT, UWS_EXPIRE_JOBS_SCHEDULE @@ -260,15 +263,17 @@ def install_handlers(self, router: APIRouter) -> None: def install_middleware(self, app: FastAPI) -> None: """Install FastAPI middleware needed by UWS. - UWS unfortunately requires that the key portion of query parameters be - case-insensitive, so UWS FastAPI applications need to add custom - middleware to lowercase query parameter keys. This method does that. + UWS unfortunately requires that the key portion of query and form + parameters be case-insensitive, so UWS FastAPI applications need to + add custom middleware to lowercase parameter keys. This method does + that. Parameters ---------- app FastAPI app. """ + app.add_middleware(CaseInsensitiveFormMiddleware) app.add_middleware(CaseInsensitiveQueryMiddleware) async def is_schema_current( diff --git a/safir/src/safir/uws/_dependencies.py b/safir/src/safir/uws/_dependencies.py index 6edaed0d..417ea760 100644 --- a/safir/src/safir/uws/_dependencies.py +++ b/safir/src/safir/uws/_dependencies.py @@ -8,7 +8,7 @@ from collections.abc import AsyncIterator from typing import Annotated, Literal -from fastapi import Depends, Form, Query, Request +from fastapi import Depends, Form, Query from sqlalchemy.ext.asyncio import AsyncEngine, async_scoped_session from structlog.stdlib import BoundLogger @@ -17,8 +17,6 @@ from safir.dependencies.logger import logger_dependency from ._config import UWSConfig -from ._exceptions import ParameterError -from ._models import UWSJobParameter from ._responses import UWSTemplates from ._results import ResultStore from ._service import JobService @@ -30,7 +28,6 @@ "create_phase_dependency", "runid_post_dependency", "uws_dependency", - "uws_post_params_dependency", ] @@ -167,35 +164,6 @@ def override_arq_queue(self, arq_queue: ArqQueue) -> None: uws_dependency = UWSDependency() -async def uws_post_params_dependency( - request: Request, -) -> list[UWSJobParameter]: - """Parse POST parameters. - - UWS requires that all POST parameters be case-insensitive, which is not - supported by FastAPI or Starlette. POST parameters therefore have to be - parsed by this dependency and then extracted from the resulting - `~safir.uws.UWSJobParameter` list (which unfortunately also means - revalidating their types). - - The POST parameters can also be (and should be) listed independently as - dependencies using the normal FastAPI syntax, in order to populate the - OpenAPI schema, but unfortunately they all have to be listed as optional - from FastAPI's perspective because they may be present using different - capitalization. - """ - if request.method != "POST": - raise ValueError("uws_post_params_dependency used for non-POST route") - parameters = [] - for key, value in (await request.form()).multi_items(): - if not isinstance(value, str): - raise TypeError("File upload not supported") - parameters.append( - UWSJobParameter(parameter_id=key.lower(), value=value) - ) - return parameters - - async def create_phase_dependency( *, get_phase: Annotated[ @@ -206,22 +174,13 @@ async def create_phase_dependency( Literal["RUN"] | None, Form(title="Immediately start job", alias="phase"), ] = None, - params: Annotated[ - list[UWSJobParameter], Depends(uws_post_params_dependency) - ], ) -> Literal["RUN"] | None: """Parse the optional phase parameter to an async job creation. Allow ``phase=RUN`` to be specified in either the query or the POST parameters, which says that the job should be immediately started. """ - for param in params: - if param.parameter_id != "phase": - continue - if param.value != "RUN": - raise ParameterError(f"Invalid phase {param.value}") - return "RUN" - return get_phase + return post_phase or get_phase async def runid_post_dependency( @@ -237,17 +196,6 @@ async def runid_post_dependency( ), ), ] = None, - params: Annotated[ - list[UWSJobParameter], Depends(uws_post_params_dependency) - ], ) -> str | None: - """Parse the run ID from POST parameters. - - This is annoyingly complex because DALI defines all parameters as - case-insensitive, so we have to list the field as a dependency but then - parse it out of the case-canonicalized job parameters. - """ - for param in params: - if param.parameter_id == "runid": - runid = param.value + """Parse the run ID from POST parameters.""" return runid diff --git a/safir/src/safir/uws/_handlers.py b/safir/src/safir/uws/_handlers.py index c94500a9..a079c296 100644 --- a/safir/src/safir/uws/_handlers.py +++ b/safir/src/safir/uws/_handlers.py @@ -15,12 +15,13 @@ from vo_models.uws import Jobs, Results from vo_models.uws.types import ExecutionPhase -from safir.datetime import isodatetime, parse_isodatetime +from safir.datetime import isodatetime from safir.dependencies.gafaelfawr import ( auth_delegated_token_dependency, auth_dependency, auth_logger_dependency, ) +from safir.pydantic import IvoaIsoDatetime from safir.slack.webhook import SlackRouteErrorHandler from ._config import UWSRoute @@ -29,9 +30,8 @@ create_phase_dependency, runid_post_dependency, uws_dependency, - uws_post_params_dependency, ) -from ._exceptions import DataMissingError, ParameterError +from ._exceptions import DataMissingError from ._models import UWSJobParameter uws_router = APIRouter(route_class=SlackRouteErrorHandler) @@ -164,7 +164,6 @@ async def delete_job( async def delete_job_via_post( job_id: str, *, - request: Request, action: Annotated[ Literal["DELETE"] | None, Form( @@ -172,26 +171,10 @@ async def delete_job_via_post( description="Mandatory, must be set to DELETE", ), ] = None, - params: Annotated[ - list[UWSJobParameter], Depends(uws_post_params_dependency) - ], + request: Request, user: Annotated[str, Depends(auth_dependency)], uws_factory: Annotated[UWSFactory, Depends(uws_dependency)], ) -> str: - # Work around the obnoxious requirement for case-insensitive parameters, - # which is also why the action parameter is declared as optional (but is - # listed to help with API documentation generation). - saw_delete = False - for param in params: - if param.parameter_id != "action" or param.value != "DELETE": - msg = f"Unknown parameter {param.parameter_id}={param.value}" - raise ParameterError(msg) - if param.parameter_id == "action" and param.value == "DELETE": - saw_delete = True - if not saw_delete: - raise ParameterError("No action given") - - # Do the actual deletion. job_service = uws_factory.create_job_service() await job_service.delete(user, job_id) return str(request.url_for("get_job_list")) @@ -222,34 +205,18 @@ async def get_job_destruction( async def post_job_destruction( job_id: str, *, - request: Request, destruction: Annotated[ - datetime | None, + IvoaIsoDatetime, Form( title="New destruction time", description="Must be in ISO 8601 format.", examples=["2021-09-10T10:01:02Z"], ), - ] = None, - params: Annotated[ - list[UWSJobParameter], Depends(uws_post_params_dependency) ], + request: Request, user: Annotated[str, Depends(auth_dependency)], uws_factory: Annotated[UWSFactory, Depends(uws_dependency)], ) -> str: - # Work around the obnoxious requirement for case-insensitive parameters. - for param in params: - if param.parameter_id != "destruction": - msg = f"Unknown parameter {param.parameter_id}={param.value}" - raise ParameterError(msg) - try: - destruction = parse_isodatetime(param.value) - except Exception as e: - raise ParameterError(f"Invalid date {param.value}") from e - if not destruction: - raise ParameterError("No new destruction time given") - - # Update the destruction time. job_service = uws_factory.create_job_service() await job_service.update_destruction(user, job_id, destruction) return str(request.url_for("get_job", job_id=job_id)) @@ -303,39 +270,19 @@ async def get_job_execution_duration( async def post_job_execution_duration( job_id: str, *, - request: Request, executionduration: Annotated[ - int | None, + int, Form( title="New execution duration", description="Integer seconds of wall clock time.", examples=[14400], + ge=1, ), - ] = None, - params: Annotated[ - list[UWSJobParameter], Depends(uws_post_params_dependency) ], + request: Request, user: Annotated[str, Depends(auth_dependency)], uws_factory: Annotated[UWSFactory, Depends(uws_dependency)], ) -> str: - # Work around the obnoxious requirement for case-insensitive parameters. - for param in params: - if param.parameter_id != "executionduration": - msg = f"Unknown parameter {param.parameter_id}={param.value}" - raise ParameterError(msg) - try: - executionduration = int(param.value) - except Exception as e: - raise ParameterError(f"Invalid duration {param.value}") from e - if executionduration <= 0: - raise ParameterError(f"Invalid duration {param.value}") - if not executionduration: - raise ParameterError("No new execution duration given") - - # Update the execution duration. Note that the policy layer may modify - # the execution duration, so the duration set may not match the input. - # update_execution_duration returns the new execution duration set, or - # None if it was not changed. job_service = uws_factory.create_job_service() duration = timedelta(seconds=executionduration) await job_service.update_execution_duration(user, job_id, duration) @@ -401,7 +348,6 @@ async def get_job_phase( async def post_job_phase( job_id: str, *, - request: Request, phase: Annotated[ Literal["RUN", "ABORT"] | None, Form( @@ -409,26 +355,12 @@ async def post_job_phase( summary="RUN to start the job, ABORT to abort the job.", ), ] = None, - params: Annotated[ - list[UWSJobParameter], Depends(uws_post_params_dependency) - ], + request: Request, user: Annotated[str, Depends(auth_dependency)], access_token: Annotated[str, Depends(auth_delegated_token_dependency)], uws_factory: Annotated[UWSFactory, Depends(uws_dependency)], logger: Annotated[BoundLogger, Depends(auth_logger_dependency)], ) -> str: - # Work around the obnoxious requirement for case-insensitive parameters. - for param in params: - if param.parameter_id != "phase": - msg = f"Unknown parameter {param.parameter_id}={param.value}" - raise ParameterError(msg) - if param.value not in ("RUN", "ABORT"): - raise ParameterError(f"Invalid phase {param.value}") - phase = param.value # type: ignore[assignment] - if not phase: - raise ParameterError("No new phase given") - - # If told to abort the job, tell arq to do so. job_service = uws_factory.create_job_service() if phase == "ABORT": await job_service.abort(user, job_id)