Improve compliance of FastAPIJobFiles with the HTTP spec (files endpoint)

This commit introduces the following differences in behavior:
- GET and HEAD requests to `/api/jobs/{job_id}/files` return HTTP status code 400 when the given path does not refer to a file or the input datasets for the job have been purged and 404 when the given path does not exist.
- PROPFIND requests to `/api/jobs/{job_id}/files` are answered with HTTP status code 501 (read motivation for this change below).
- POST requests to `/api/jobs/{job_id}/files` are answered with HTTP status code 400 when no file is provided.

The reason behind the code explicitly answering `PROPFIND` requests with status code 501 is an unfortunate interaction between the ARC remote job runner that is under development, the behavior of legacy API endpoints and how they are integrated within the FastAPI app.

The ARC remote job runner (which will be implemented as `lib.galaxy.jobs.runners.pulsar.PulsarARCJobRunner`) expects this endpoint to return HTTP codes other than 404 when `PROPFIND` requests are issued. They are not part of the HTTP spec, but they are used in the WebDAV protocol. The correct answer to such requests is likely 501 (not implemented).

FastAPI returns HTTP 405 (method not allowed) for `PROPFIND`, which maybe is not fully correct but tolerable because it is one less quirk to maintain.

However, because of the way legacy WSGI endpoints are injected into the FastAPI app (using `app.mount("/", wsgi_handler)`), the built-in support for returning HTTP 405 for `PROPFIND` breaks, because such requests are passed to the `wsgi_handler` sub-application. This means that the endpoint still needs to include some code to handle this behavior.

When ALL routes have been migrated to ASGI (no WSGI handler sub-application needed anymore), some lines of code can be removed, they are labeled using comments.

Author: kysrpex

client/src/api/schema/schema.ts

@@ -31259,8 +31259,26 @@ export interface operations {
                     "application/octet-stream": unknown;
                 };
             };
-            /** @description File not found, path does not refer to a file, or input dataset(s) for job have been purged. */
+            /** @description Path does not refer to a file, or input dataset(s) for job have been purged. */
             400: {
+                headers: {
+                    [name: string]: unknown;
+                };
+                content: {
+                    "application/json": unknown;
+                };
+            };
+            /** @description File not found. */
+            404: {
+                headers: {
+                    [name: string]: unknown;
+                };
+                content: {
+                    "application/json": unknown;
+                };
+            };
+            /** @description Input dataset(s) for job have been purged. */
+            500: {
                 headers: {
                     [name: string]: unknown;
                 };
@@ -31319,6 +31337,13 @@ export interface operations {
                     "application/json": unknown;
                 };
             };
+            /** @description Bad request (including no file provided). */
+            400: {
+                headers: {
+                    [name: string]: unknown;
+                };
+                content?: never;
+            };
             /** @description Request Error */
             "4XX": {
                 headers: {
@@ -31368,8 +31393,26 @@ export interface operations {
                     "application/octet-stream": unknown;
                 };
             };
-            /** @description File not found, path does not refer to a file, or input dataset(s) for job have been purged. */
+            /** @description Path does not refer to a file, or input dataset(s) for job have been purged. */
             400: {
+                headers: {
+                    [name: string]: unknown;
+                };
+                content: {
+                    "application/json": unknown;
+                };
+            };
+            /** @description File not found. */
+            404: {
+                headers: {
+                    [name: string]: unknown;
+                };
+                content: {
+                    "application/json": unknown;
+                };
+            };
+            /** @description Input dataset(s) for job have been purged. */
+            500: {
                 headers: {
                     [name: string]: unknown;
                 };

lib/galaxy/webapps/galaxy/api/__init__.py

@@ -265,6 +265,10 @@ def environ(self) -> Environ:
             self.__environ = build_environ(self.__request.scope, None)  # type: ignore[arg-type]
         return self.__environ
 
+    @property
+    def method(self):
+        return self.__request.method
+
     @property
     def headers(self):
         return self.__request.headers

lib/galaxy/webapps/galaxy/api/job_files.py

@@ -36,6 +36,7 @@
     DependsOnTrans,
     Router,
 )
+from galaxy.work.context import SessionRequestContext
 from . import BaseGalaxyAPIController
 
 __all__ = ("FastAPIJobFiles", "JobFilesAPIController", "router")
@@ -128,16 +129,48 @@ class FastAPIJobFiles:
                         "content": {"application/json": None, "application/octet-stream": {"example": None}},
                     },
                     400: {
-                        "description": (
-                            "File not found, path does not refer to a file, or input dataset(s) for job have been purged."
-                        )
+                        "description": "Path does not refer to a file, or input dataset(s) for job have been purged.",
+                        "content": {
+                            "application/json": {
+                                "example": {
+                                    "detail": (
+                                        "Path does not refer to a file, or input dataset(s) for job have been purged."
+                                    )
+                                },
+                            }
+                        },
                     },
+                    404: {
+                        "description": "File not found.",
+                        "content": {
+                            "application/json": {
+                                "example": {"detail": "File not found."},
+                            }
+                        },
+                    },
+                    500: {"description": "Input dataset(s) for job have been purged."},
                 },
             )
         ),
     )
     @router.head(*_args, **_kwargs)  # type: ignore[name-defined]
     # remove `@router.head(...)` when ALL endpoints have been migrated to FastAPI
+    @router.api_route(
+        *_args,  # type: ignore[name-defined]
+        **{key: value for key, value in _kwargs.items() if key != "responses"},  # type: ignore[name-defined]
+        responses={501: {"description": "Not implemented."}},
+        methods=["PROPFIND"],
+        include_in_schema=False,
+    )
+    # remove `@router.api_route(..., methods=["PROPFIND"])` when ALL endpoints have been migrated to FastAPI
+    # The ARC remote job runner (`lib.galaxy.jobs.runners.pulsar.PulsarARCJobRunner`) expects this to return HTTP codes
+    # other than 404 when `PROPFIND` requests are issued. They are not part of the HTTP spec, but they are used in the
+    # WebDAV protocol. The correct answer to such requests is likely 501 (not implemented). FastAPI returns HTTP 405
+    # (method not allowed) for `PROPFIND`, which maybe is not fully correct but tolerable because it is one less quirk
+    # to maintain. However, because of the way legacy WSGI endpoints are injected into the FastAPI app (using
+    # `app.mount("/", wsgi_handler)`), the built-in support for returning HTTP 405 for `PROPFIND` breaks, because such
+    # requests are passed to the `wsgi_handler` sub-application. This means that the endpoint still needs to include
+    # some code to handle this behavior.
     def index(
         self,
         job_id: Annotated[str, Path(description="Encoded id string of the job.")],
@@ -155,14 +188,19 @@ def index(
                 ),
             ),
         ],
-        trans: ProvidesAppContext = DependsOnTrans,
+        trans: SessionRequestContext = DependsOnTrans,
     ) -> GalaxyFileResponse:
         """
         Get a file required to staging a job (proper datasets, extra inputs, task-split inputs, working directory
         files).
 
         This API method is intended only for consumption by job runners, not end users.
         """
+        # PROPFIND is not implemented, but the endpoint needs to return a non-404 error code for it
+        # remove me when ALL endpoints have been migrated to FastAPI
+        if trans.request.method == "PROPFIND":
+            raise exceptions.NotImplemented()
+
         path = unquote(path)
 
         job = self.__authorize_job_access(trans, job_id, path=path, job_key=job_key)
@@ -176,6 +214,9 @@ def index(
                 # This looks like a galaxy dataset, check if any job input has been deleted.
                 if any(jtid.dataset.dataset.purged for jtid in job.input_datasets):
                     raise exceptions.ItemDeletionException("Input dataset(s) for job have been purged.")
+            raise exceptions.ObjectNotFound("File not found.")
+        elif not os.path.isfile(path):
+            raise exceptions.RequestParameterInvalidException("Path does not refer to a file.")
 
         return GalaxyFileResponse(path)
 
@@ -184,6 +225,7 @@ def index(
         summary="Populate an output file.",
         responses={
             200: {"description": "An okay message.", "content": {"application/json": {"example": {"message": "ok"}}}},
+            400: {"description": "Bad request (including no file provided)."},
         },
     )
     def create(

lib/galaxy/work/context.py

@@ -97,6 +97,11 @@ def url_path(self) -> str:
     def host(self) -> str:
         """The host address."""
 
+    @property
+    @abc.abstractmethod
+    def method(self) -> str:
+        """The request's HTTP method."""
+
     @property
     @abc.abstractmethod
     def is_secure(self) -> bool:

test/integration/test_job_files.py

@@ -48,6 +48,7 @@ class TestJobFilesIntegration(integration_util.IntegrationTestCase):
     initialized = False
     dataset_populator: DatasetPopulator
 
+    hist_id: int  # cannot use `history_id` as name, it collides with a pytest fixture
     input_hda: model.HistoryDatasetAssociation
     input_hda_dict: Dict[str, Any]
     _nginx_upload_job_files_store: str
@@ -61,7 +62,6 @@ def handle_galaxy_config_kwds(cls, config):
         config["server_name"] = "files"
         config["nginx_upload_job_files_store"] = tempfile.mkdtemp()
         cls._nginx_upload_job_files_store = config["nginx_upload_job_files_store"]
-        cls.initialized = False
 
     @classmethod
     def tearDownClass(cls):
@@ -70,17 +70,14 @@ def tearDownClass(cls):
 
     def setUp(self):
         super().setUp()
-        cls = TestJobFilesIntegration
-        cls.dataset_populator = DatasetPopulator(self.galaxy_interactor)
-        if not cls.initialized:
-            history_id = cls.dataset_populator.new_history()
-            sa_session = self.sa_session
-            stmt = select(model.HistoryDatasetAssociation)
-            assert len(sa_session.scalars(stmt).all()) == 0
-            cls.input_hda_dict = cls.dataset_populator.new_dataset(history_id, content=TEST_INPUT_TEXT, wait=True)
-            assert len(sa_session.scalars(stmt).all()) == 1
-            cls.input_hda = sa_session.scalars(stmt).all()[0]
-            cls.initialized = True
+        self.dataset_populator = DatasetPopulator(self.galaxy_interactor)
+        history_id_encoded = self.dataset_populator.new_history()
+        self.hist_id = self._app.security.decode_id(history_id_encoded)
+        self.input_hda_dict = self.dataset_populator.new_dataset(history_id_encoded, content=TEST_INPUT_TEXT, wait=True)
+        sa_session = self.sa_session
+        stmt = select(model.HistoryDatasetAssociation).where(model.HistoryDatasetAssociation.history_id == self.hist_id)
+        assert len(sa_session.scalars(stmt).all()) == 1
+        self.input_hda = sa_session.scalars(stmt).first()
 
     def test_read_by_state(self):
         job, _, _ = self.create_static_job_with_state("running")
@@ -115,9 +112,57 @@ def test_read_fails_if_input_file_purged(self):
             self.input_hda_dict["history_id"], content_id=self.input_hda_dict["id"], purge=True, wait_for_purge=True
         )
         assert delete_response.status_code == 200
-        head_response = requests.get(get_url, params=data)
+        response = requests.get(get_url, params=data)
+        assert response.status_code == 400
+        assert response.json()["err_msg"] == "Input dataset(s) for job have been purged."
+
+    def test_read_missing_file(self):
+        job, _, _ = self.create_static_job_with_state("running")
+        job_id, job_key = self._api_job_keys(job)
+        data = {"path": self.input_hda.get_file_name() + "_missing", "job_key": job_key}
+        get_url = self._api_url(f"jobs/{job_id}/files", use_key=True)
+
+        head_response = requests.head(get_url, params=data)
+        assert head_response.status_code == 404
+
+        response = requests.get(get_url, params=data)
+        assert response.status_code == 404
+
+    def test_read_folder(self):
+        job, _, _ = self.create_static_job_with_state("running")
+        job_id, job_key = self._api_job_keys(job)
+        data = {"path": os.path.dirname(self.input_hda.get_file_name()), "job_key": job_key}
+        get_url = self._api_url(f"jobs/{job_id}/files", use_key=True)
+
+        head_response = requests.head(get_url, params=data)
         assert head_response.status_code == 400
-        assert head_response.json()["err_msg"] == "Input dataset(s) for job have been purged."
+
+        response = requests.get(get_url, params=data)
+        assert response.status_code == 400
+
+    def test_write_no_file(self):
+        job, output_hda, working_directory = self.create_static_job_with_state("running")
+        job_id, job_key = self._api_job_keys(job)
+        path = self._app.object_store.get_filename(output_hda.dataset)
+        assert path
+        data = {"path": path, "job_key": job_key}
+
+        post_url = self._api_url(f"jobs/{job_id}/files", use_key=False)
+        response = requests.post(post_url, data=data)
+        assert response.status_code == 400
+
+    def test_propfind(self):
+        # remove this test when ALL Galaxy endpoints have been migrated to FastAPI; it will then be FastAPI's
+        # responsibility to return a status code other than 404
+        job, output_hda, working_directory = self.create_static_job_with_state("running")
+        job_id, job_key = self._api_job_keys(job)
+        path = self._app.object_store.get_filename(output_hda.dataset)
+        assert path
+        data = {"path": path, "job_key": job_key}
+
+        propfind_url = self._api_url(f"jobs/{job_id}/files", use_key=False)
+        response = requests.request("PROPFIND", propfind_url, params=data)
+        assert response.status_code == 501
 
     def test_write_by_state(self):
         job, output_hda, working_directory = self.create_static_job_with_state("running")
@@ -269,9 +314,13 @@ def sa_session(self):
     def create_static_job_with_state(self, state):
         """Create a job with unknown handler so its state won't change."""
         sa_session = self.sa_session
-        hda = sa_session.scalars(select(model.HistoryDatasetAssociation)).all()[0]
+        stmt_hda = select(model.HistoryDatasetAssociation).where(
+            model.HistoryDatasetAssociation.history_id == self.hist_id
+        )
+        hda = sa_session.scalars(stmt_hda).first()
         assert hda
-        history = sa_session.scalars(select(model.History)).all()[0]
+        stmt_history = select(model.History).where(model.History.id == self.hist_id)
+        history = sa_session.scalars(stmt_history).first()
         assert history
         user = sa_session.scalars(select(model.User)).all()[0]
         assert user