Merge branch 'job_files_fastapi_http_compliance' into arc

Author: kysrpex

client/src/api/schema/schema.ts

@@ -31590,8 +31590,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;
                 };
@@ -31650,6 +31668,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: {
@@ -31699,8 +31724,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