From 14aadd6f0961f0ae5f44ec3ba5b0e36c8238f6d9 Mon Sep 17 00:00:00 2001 From: Tommaso Comparin <3862206+tcompa@users.noreply.github.com> Date: Fri, 1 Dec 2023 09:58:48 +0100 Subject: [PATCH] Move `PATCH /api/v1/project/1/job/1/` to `/admin/job/1/` (close #1051) --- CHANGELOG.md | 2 +- fractal_server/app/routes/admin.py | 39 ++++++++++++ fractal_server/app/routes/api/v1/job.py | 40 ------------ tests/test_admin_api.py | 83 +++++++++++++++++++++++++ tests/test_job_api.py | 82 ------------------------ 5 files changed, 123 insertions(+), 123 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63e478cd67..466c54b401 100644 --- 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 index d640a23784..367aa0575e 100644 --- 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 index 7228ced4d4..2ee93d9db1 100644 --- 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 index aa96e909d1..492e61b326 100644 --- 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 index 1751a2227a..799c5f4872 100644 --- a/tests/test_job_api.py +++ b/tests/test_job_api.py @@ -6,7 +6,6 @@ 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" @@ -14,87 +13,6 @@ 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,