jpmschweitzer
c34db66f51
DELETE /tasks/{name} issued a bare DELETE against scheduled_tasks. Any task
that had ever run owns rows in task_executions, so the foreign key rejected it
and the caller got:
psycopg2.errors.ForeignKeyViolation: update or delete on table
"scheduled_tasks" violates foreign key constraint
"task_executions_task_id_fkey" on table "task_executions"
surfaced as a bare 500 with nothing naming history as the obstacle. It read as
the service being broken rather than the request being refusable, and since
every task that has ever fired has history, the endpoint effectively worked
only for tasks that had never run. Found while removing a temporary probe task,
which then had to be deleted with hand-written SQL across two tables.
Refusing rather than cascading, because the outcomes are not equally
recoverable: a task definition can be recreated from the API in one call, its
execution history cannot be recreated at all. Defaulting to the destructive
reading of an ambiguous request is how audit trails disappear quietly.
The 409 carries what the caller needs to act -- how many records are at stake,
the flag that proceeds anyway, and PUT enabled=false, which is usually what was
actually wanted: it stops the task running and keeps the record. A bare
"conflict" would be little better than the 500 it replaces.
Purge deletes history and task in one transaction. Split across two, a failure
between them leaves the audit trail gone and the task alive -- the worst of both.
Mutation-checked: removing the guard fails the refusal tests. A test also pins
that a refused delete issues no DELETE at all, and that ?purge=true on a missing
task is still 404 rather than a success.
113 lines
4.5 KiB
Python
113 lines
4.5 KiB
Python
"""Deleting a task must not destroy its history by accident, or 500 by surprise.
|
|
|
|
DELETE /tasks/{name} used to issue a bare DELETE against scheduled_tasks. Any
|
|
task that had ever run owned rows in task_executions, so the foreign key
|
|
rejected it and the caller got "Internal Server Error" with nothing pointing at
|
|
history as the obstacle — it read as the service being broken rather than the
|
|
request being refusable. Since every task that has ever fired has history, the
|
|
endpoint effectively worked only for tasks that never ran.
|
|
|
|
It now refuses with 409 and takes ?purge=true to mean it. The asymmetry is the
|
|
argument: a task definition can be recreated from the API in one call, its
|
|
execution history cannot be recreated at all.
|
|
"""
|
|
import pytest
|
|
from fastapi.testclient import TestClient
|
|
from unittest.mock import MagicMock
|
|
|
|
from src.main import app, get_task_executor
|
|
|
|
|
|
def _fake_executor(task_row, execution_count=0):
|
|
"""A stand-in whose cursor answers the endpoint's two lookups in order."""
|
|
ex = MagicMock()
|
|
conn, cur = MagicMock(), MagicMock()
|
|
cur.fetchone.side_effect = (
|
|
[task_row, (execution_count,)] if task_row is not None else [None]
|
|
)
|
|
conn.cursor.return_value.__enter__ = MagicMock(return_value=cur)
|
|
conn.cursor.return_value.__exit__ = MagicMock(return_value=None)
|
|
conn.__enter__ = MagicMock(return_value=conn)
|
|
conn.__exit__ = MagicMock(return_value=None)
|
|
ex.get_db_connection.return_value = conn
|
|
return ex, cur
|
|
|
|
|
|
def _statements(cur):
|
|
return [c[0][0] for c in cur.execute.call_args_list]
|
|
|
|
|
|
@pytest.fixture
|
|
def override():
|
|
made = {}
|
|
|
|
def _install(task_row, execution_count=0):
|
|
ex, cur = _fake_executor(task_row, execution_count)
|
|
app.dependency_overrides[get_task_executor] = lambda: ex
|
|
made['cur'] = cur
|
|
return cur
|
|
|
|
yield _install
|
|
app.dependency_overrides.pop(get_task_executor, None)
|
|
|
|
|
|
@pytest.mark.unit
|
|
class TestDeleteTask:
|
|
|
|
def test_history_blocks_deletion_with_409(self, client: TestClient, auth_headers, override):
|
|
cur = override((46,), execution_count=12)
|
|
r = client.delete("/tasks/some_task", headers=auth_headers)
|
|
|
|
assert r.status_code == 409
|
|
detail = r.json()["detail"]
|
|
# The message has to carry the facts the caller needs to act: how much
|
|
# history is at stake, the flag that proceeds, and the option they
|
|
# probably actually wanted. A bare "conflict" would be no better than
|
|
# the 500 it replaces.
|
|
assert "12 execution record" in detail
|
|
assert "purge=true" in detail
|
|
assert "enabled=false" in detail
|
|
|
|
def test_a_refused_delete_deletes_nothing(self, client: TestClient, auth_headers, override):
|
|
cur = override((46,), execution_count=12)
|
|
client.delete("/tasks/some_task", headers=auth_headers)
|
|
assert not any("DELETE" in s.upper() for s in _statements(cur))
|
|
|
|
def test_purge_removes_history_then_the_task(self, client: TestClient, auth_headers, override):
|
|
cur = override((46,), execution_count=12)
|
|
r = client.delete("/tasks/some_task?purge=true", headers=auth_headers)
|
|
|
|
assert r.status_code == 200
|
|
assert r.json()["executions_purged"] == 12
|
|
deletes = [s for s in _statements(cur) if "DELETE" in s.upper()]
|
|
assert len(deletes) == 2
|
|
# History first: the foreign key points that way, and the reverse order
|
|
# is the failure this endpoint started with.
|
|
assert "task_executions" in deletes[0]
|
|
assert "scheduled_tasks" in deletes[1]
|
|
|
|
def test_a_task_that_never_ran_deletes_without_the_flag(
|
|
self, client: TestClient, auth_headers, override
|
|
):
|
|
cur = override((46,), execution_count=0)
|
|
r = client.delete("/tasks/fresh_task", headers=auth_headers)
|
|
|
|
assert r.status_code == 200
|
|
assert r.json()["executions_purged"] == 0
|
|
deletes = [s for s in _statements(cur) if "DELETE" in s.upper()]
|
|
assert len(deletes) == 1, "nothing to purge, so history must not be touched"
|
|
assert "scheduled_tasks" in deletes[0]
|
|
|
|
def test_unknown_task_is_404_not_409(self, client: TestClient, auth_headers, override):
|
|
override(None)
|
|
r = client.delete("/tasks/nope", headers=auth_headers)
|
|
assert r.status_code == 404
|
|
|
|
def test_purge_on_an_unknown_task_is_still_404(
|
|
self, client: TestClient, auth_headers, override
|
|
):
|
|
"""The flag must not turn a missing task into a success."""
|
|
override(None)
|
|
r = client.delete("/tasks/nope?purge=true", headers=auth_headers)
|
|
assert r.status_code == 404
|