Merge pull request #346 from lsst-sqre/tickets/DM-47928

DM-47928: Use case-insensitive form middleware for UWS support

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.

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:
 

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",
 ]

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(

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