Merge pull request #1053 from fractal-analytics-platform/1051-move-pa…

…tch-apiv1jobjob_id-to-patch-adminjobjob_id Move `PATCH /api/v1/project/1/job/1/` to `/admin/job/1/` (close #1051)
fractal-analytics-platform · Dec 1, 2023 · 6e9d9ed · 6e9d9ed
2 parents b86410d + 14aadd6
commit 6e9d9ed
Show file tree

Hide file tree

Showing 5 changed files with 123 additions and 123 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,7 +9,7 @@
         * Add new `GET` endpoints `api/v1/job/` and `api/v1/project/{project_id}/workflow/{workflow_id}/job/` (\#969, \#1003).
         * Add new `GET` endpoints `api/v1/dataset/` and `api/v1/workflow/` (\#988, \#1003).
         * Add new `GET` endpoint `api/v1/project/{project_id}/dataset/` (\#993).
-        * Add superuser-only `PATCH /api/v1/job/{job_id}/` endpoint (\#1030).
+        * Add `PATCH /admin/job/{job_id}/` endpoint (\#1030, \#1053).
         * Move `GET /auth/whoami/` to `GET /auth/current-user/` (\#1013).
         * Move `PATCH /auth/users/me/` to `PATCH /auth/current-user/` (\#1013, \#1035).
         * Remove `DELETE /auth/users/{id}/` endpoint (\#994).

diff --git a/fractal_server/app/routes/admin.py b/fractal_server/app/routes/admin.py
@@ -6,6 +6,8 @@
 
 from fastapi import APIRouter
 from fastapi import Depends
+from fastapi import HTTPException
+from fastapi import status as fastapi_status
 from sqlalchemy import func
 from sqlmodel import select
 
@@ -18,6 +20,7 @@
 from ..models import Workflow
 from ..models.security import UserOAuth as User
 from ..schemas import ApplyWorkflowRead
+from ..schemas import ApplyWorkflowUpdate
 from ..schemas import DatasetRead
 from ..schemas import ProjectRead
 from ..schemas import WorkflowRead
@@ -212,3 +215,39 @@ async def view_job(
     await db.close()
 
     return job_list
+
+
+@router_admin.patch(
+    "/job/{job_id}/",
+    response_model=ApplyWorkflowRead,
+)
+async def update_job(
+    job_update: ApplyWorkflowUpdate,
+    job_id: int,
+    user: User = Depends(current_active_superuser),
+    db: AsyncSession = Depends(get_db),
+) -> Optional[ApplyWorkflowRead]:
+    """
+    Change the status of an existing job.
+
+    This endpoint is only open to superusers, and it does not apply
+    project-based access-control to jobs.
+    """
+    job = await db.get(ApplyWorkflow, job_id)
+    if job is None:
+        raise HTTPException(
+            status_code=fastapi_status.HTTP_404_NOT_FOUND,
+            detail=f"Job {job_id} not found",
+        )
+
+    if job_update.status != JobStatusType.FAILED:
+        raise HTTPException(
+            status_code=fastapi_status.HTTP_422_UNPROCESSABLE_ENTITY,
+            detail=f"Cannot set job status to {job_update.status}",
+        )
+
+    setattr(job, "status", job_update.status)
+    await db.commit()
+    await db.refresh(job)
+    await db.close()
+    return job
diff --git a/fractal_server/app/routes/api/v1/job.py b/fractal_server/app/routes/api/v1/job.py
@@ -15,12 +15,8 @@
 from .....syringe import Inject
 from ....db import AsyncSession
 from ....db import get_db
-from ....models import ApplyWorkflow
 from ....runner._common import SHUTDOWN_FILENAME
 from ....schemas import ApplyWorkflowRead
-from ....schemas import ApplyWorkflowUpdate
-from ....schemas import JobStatusType
-from ....security import current_active_superuser
 from ....security import current_active_user
 from ....security import User
 from ._aux_functions import _get_job_check_owner
@@ -200,39 +196,3 @@ async def stop_job(
         f.write(f"Trigger executor shutdown for {job.id=}, {project_id=}.")
 
     return Response(status_code=status.HTTP_204_NO_CONTENT)
-
-
-@router.patch(
-    "/job/{job_id}/",
-    response_model=ApplyWorkflowRead,
-)
-async def update_job(
-    job_update: ApplyWorkflowUpdate,
-    job_id: int,
-    user: User = Depends(current_active_superuser),
-    db: AsyncSession = Depends(get_db),
-) -> Optional[ApplyWorkflowRead]:
-    """
-    Change the status of an existing job.
-
-    This endpoint is only open to superusers, and it does not apply
-    project-based access-control to jobs.
-    """
-    job = await db.get(ApplyWorkflow, job_id)
-    if job is None:
-        raise HTTPException(
-            status_code=status.HTTP_404_NOT_FOUND,
-            detail=f"Job {job_id} not found",
-        )
-
-    if job_update.status != JobStatusType.FAILED:
-        raise HTTPException(
-            status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
-            detail=f"Cannot set job status to {job_update.status}",
-        )
-
-    setattr(job, "status", job_update.status)
-    await db.commit()
-    await db.refresh(job)
-    await db.close()
-    return job
diff --git a/tests/test_admin_api.py b/tests/test_admin_api.py
@@ -1,5 +1,7 @@
 from datetime import datetime
 
+from devtools import debug
+
 from fractal_server.app.models import JobStatusType
 
 PREFIX = "/admin"
@@ -369,3 +371,84 @@ async def test_view_job(
         )
         assert res.status_code == 200
         assert len(res.json()) == 1
+
+
+async def test_patch_job(
+    MockCurrentUser,
+    project_factory,
+    dataset_factory,
+    workflow_factory,
+    job_factory,
+    task_factory,
+    client,
+    registered_superuser_client,
+    db,
+    tmp_path,
+):
+    ORIGINAL_STATUS = JobStatusType.SUBMITTED
+    NEW_STATUS = JobStatusType.FAILED
+
+    async with MockCurrentUser(user_kwargs={"id": 111111}) as user:
+        project = await project_factory(user)
+        workflow = await workflow_factory(project_id=project.id)
+        task = await task_factory(name="task", source="source")
+        await workflow.insert_task(task_id=task.id, db=db)
+        dataset = await dataset_factory(project_id=project.id)
+        job = await job_factory(
+            working_dir=tmp_path.as_posix(),
+            project_id=project.id,
+            input_dataset_id=dataset.id,
+            output_dataset_id=dataset.id,
+            workflow_id=workflow.id,
+            status=ORIGINAL_STATUS,
+        )
+        # Read job as job owner (standard user)
+        res = await client.get(f"/api/v1/project/{project.id}/job/{job.id}/")
+        assert res.status_code == 200
+        assert res.json()["status"] == ORIGINAL_STATUS
+        # Patch job as job owner (standard user) and fail
+        res = await client.patch(
+            f"{PREFIX}/job/{job.id}/",
+            json={"status": NEW_STATUS},
+        )
+        assert res.status_code == 403
+        # Patch job as superuser
+        async with MockCurrentUser(
+            user_kwargs={"id": 222222, "is_superuser": True}
+        ):
+            # Fail due to invalid payload (missing attribute "status")
+            res = await registered_superuser_client.patch(
+                f"{PREFIX}/job/{job.id}/",
+                json={"working_dir": "/tmp"},
+            )
+            assert res.status_code == 422
+            # Fail due to invalid payload (status not part of JobStatusType)
+            res = await registered_superuser_client.patch(
+                f"{PREFIX}/job/{job.id}/",
+                json={"status": "something_invalid"},
+            )
+            assert res.status_code == 422
+            # Fail due to invalid payload (status not failed)
+            res = await registered_superuser_client.patch(
+                f"{PREFIX}/job/{job.id}/",
+                json={"status": "done"},
+            )
+            assert res.status_code == 422
+            # Fail due to non-existing job
+            res = await registered_superuser_client.patch(
+                f"{PREFIX}/job/{123456789}/",
+                json={"status": NEW_STATUS},
+            )
+            assert res.status_code == 404
+            # Successfully apply patch
+            res = await registered_superuser_client.patch(
+                f"{PREFIX}/job/{job.id}/",
+                json={"status": NEW_STATUS},
+            )
+            assert res.status_code == 200
+            debug(res.json())
+            assert res.json()["status"] == NEW_STATUS
+        # Read job as job owner (standard user)
+        res = await client.get(f"/api/v1/project/{project.id}/job/{job.id}/")
+        assert res.status_code == 200
+        assert res.json()["status"] == NEW_STATUS
diff --git a/tests/test_job_api.py b/tests/test_job_api.py
@@ -6,95 +6,13 @@
 
 from fractal_server.app.runner import _backends
 from fractal_server.app.runner._common import SHUTDOWN_FILENAME
-from fractal_server.app.schemas import JobStatusType
 
 
 PREFIX = "/api/v1"
 
 backends_available = list(_backends.keys())
 
 
-async def test_patch_job(
-    MockCurrentUser,
-    project_factory,
-    dataset_factory,
-    workflow_factory,
-    job_factory,
-    task_factory,
-    client,
-    registered_superuser_client,
-    db,
-    tmp_path,
-):
-    ORIGINAL_STATUS = JobStatusType.SUBMITTED
-    NEW_STATUS = JobStatusType.FAILED
-
-    async with MockCurrentUser(user_kwargs={"id": 111111}) as user:
-        project = await project_factory(user)
-        workflow = await workflow_factory(project_id=project.id)
-        task = await task_factory(name="task", source="source")
-        await workflow.insert_task(task_id=task.id, db=db)
-        dataset = await dataset_factory(project_id=project.id)
-        job = await job_factory(
-            working_dir=tmp_path.as_posix(),
-            project_id=project.id,
-            input_dataset_id=dataset.id,
-            output_dataset_id=dataset.id,
-            workflow_id=workflow.id,
-            status=ORIGINAL_STATUS,
-        )
-        # Read job as job owner (standard user)
-        res = await client.get(f"{PREFIX}/project/{project.id}/job/{job.id}/")
-        assert res.status_code == 200
-        assert res.json()["status"] == ORIGINAL_STATUS
-        # Patch job as job owner (standard user) and fail
-        res = await client.patch(
-            f"{PREFIX}/job/{job.id}/",
-            json={"status": NEW_STATUS},
-        )
-        assert res.status_code == 403
-        # Patch job as superuser
-        async with MockCurrentUser(
-            user_kwargs={"id": 222222, "is_superuser": True}
-        ):
-            # Fail due to invalid payload (missing attribute "status")
-            res = await registered_superuser_client.patch(
-                f"{PREFIX}/job/{job.id}/",
-                json={"working_dir": "/tmp"},
-            )
-            assert res.status_code == 422
-            # Fail due to invalid payload (status not part of JobStatusType)
-            res = await registered_superuser_client.patch(
-                f"{PREFIX}/job/{job.id}/",
-                json={"status": "something_invalid"},
-            )
-            assert res.status_code == 422
-            # Fail due to invalid payload (status not failed)
-            res = await registered_superuser_client.patch(
-                f"{PREFIX}/job/{job.id}/",
-                json={"status": "done"},
-            )
-            assert res.status_code == 422
-            # Fail due to non-existing job
-            res = await registered_superuser_client.patch(
-                f"{PREFIX}/job/{123456789}/",
-                json={"status": NEW_STATUS},
-            )
-            assert res.status_code == 404
-            # Successfully apply patch
-            res = await registered_superuser_client.patch(
-                f"{PREFIX}/job/{job.id}/",
-                json={"status": NEW_STATUS},
-            )
-            assert res.status_code == 200
-            debug(res.json())
-            assert res.json()["status"] == NEW_STATUS
-        # Read job as job owner (standard user)
-        res = await client.get(f"{PREFIX}/project/{project.id}/job/{job.id}/")
-        assert res.status_code == 200
-        assert res.json()["status"] == NEW_STATUS
-
-
 @pytest.mark.parametrize("backend", backends_available)
 async def test_stop_job(
     backend,