fix(sp37-followup): SubmitOutcome.UNEXPECTED_ERROR for non-typed failures
Followup #3 from the SP37 final-state tracker. The router's per-file try/except previously coerced every unexpected exception to SubmitOutcome.SFTP_FAILED — misleading because a RuntimeError from cycl_store.add has nothing to do with SFTP. Changes: * submission/result.py — add UNEXPECTED_ERROR = 'unexpected_error' enum value. Docstring distinguishes it from the typed SFTP_FAILED. * api_routers/submission.py — exception handler now returns UNEXPECTED_ERROR (was SFTP_FAILED). * test_api_submit_batch.py::test_submit_batch_unexpected_exception_recorded_in_results — updated assertion (was sftp_failed, now unexpected_error). * test_unexpected_error_outcome.py (new, 4 tests): - enum value exists - distinct from typed failure values - API surfaces 'unexpected_error' for uncaught exceptions - typed SFTP_FAILED path still surfaces 'sftp_failed' (regression lock) Tests: 40/40 pass in 1.41s (full SP37 chain + new file). Live curl: POST /api/submit-batch in stub mode → HTTP 409 (unchanged behavior, no regression on the config-level guards).
This commit is contained in:
@@ -160,7 +160,7 @@ def submit_batch(body: SubmitBatchRequest):
|
|||||||
)
|
)
|
||||||
r = SubmitResult(
|
r = SubmitResult(
|
||||||
file=src.name,
|
file=src.name,
|
||||||
outcome=SubmitOutcome.SFTP_FAILED,
|
outcome=SubmitOutcome.UNEXPECTED_ERROR,
|
||||||
error=f"{exc.__class__.__name__}: {exc}",
|
error=f"{exc.__class__.__name__}: {exc}",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -16,8 +16,15 @@ class SubmitOutcome(str, Enum):
|
|||||||
audit event; ``SKIPPED`` does NOT (re-running on an already-uploaded
|
audit event; ``SKIPPED`` does NOT (re-running on an already-uploaded
|
||||||
file must not flood the audit log with duplicate events). All
|
file must not flood the audit log with duplicate events). All
|
||||||
failure outcomes (``PARSE_FAILED``, ``PAYER_MISMATCH``, ``DB_FAILED``,
|
failure outcomes (``PARSE_FAILED``, ``PAYER_MISMATCH``, ``DB_FAILED``,
|
||||||
``SFTP_FAILED``) emit no event either — failures should be observable
|
``SFTP_FAILED``, ``UNEXPECTED_ERROR``) emit no event either —
|
||||||
via the helper's return value, not the audit log.
|
failures should be observable via the helper's return value, not
|
||||||
|
the audit log.
|
||||||
|
|
||||||
|
``UNEXPECTED_ERROR`` is reserved for uncaught exceptions in the
|
||||||
|
helper's own code (e.g. a bug in ``cycl_store.add``) — distinct
|
||||||
|
from the typed ``SFTP_FAILED`` which means the SFTP layer itself
|
||||||
|
raised. Operators reading ``outcome="unexpected_error"`` know to
|
||||||
|
look at the bug tracker, not the SFTP server.
|
||||||
"""
|
"""
|
||||||
SUBMITTED = "submitted"
|
SUBMITTED = "submitted"
|
||||||
SKIPPED = "skipped"
|
SKIPPED = "skipped"
|
||||||
@@ -25,6 +32,7 @@ class SubmitOutcome(str, Enum):
|
|||||||
PAYER_MISMATCH = "payer_mismatch"
|
PAYER_MISMATCH = "payer_mismatch"
|
||||||
DB_FAILED = "db_failed"
|
DB_FAILED = "db_failed"
|
||||||
SFTP_FAILED = "sftp_failed"
|
SFTP_FAILED = "sftp_failed"
|
||||||
|
UNEXPECTED_ERROR = "unexpected_error"
|
||||||
|
|
||||||
|
|
||||||
@dataclass(frozen=True)
|
@dataclass(frozen=True)
|
||||||
|
|||||||
@@ -213,11 +213,10 @@ def test_submit_batch_unexpected_exception_recorded_in_results(
|
|||||||
"""If submit_file raises, the file lands in results[] as a failed row.
|
"""If submit_file raises, the file lands in results[] as a failed row.
|
||||||
|
|
||||||
The router catches the exception, builds a SubmitResult with
|
The router catches the exception, builds a SubmitResult with
|
||||||
``outcome=SubmitOutcome.SFTP_FAILED`` (per Task 6's instruction
|
``outcome=SubmitOutcome.UNEXPECTED_ERROR`` (followup #3 — distinct
|
||||||
that "any enum value works as a marker; choose whichever is least
|
from the typed SFTP_FAILED path so operators can tell "real bug"
|
||||||
misleading"), and counts it as ``failed``. The full error string
|
from "SFTP server down"), and counts it as ``failed``. The full
|
||||||
(including exception class name) is in ``error`` so the operator
|
error string (including exception class name) is in ``error``.
|
||||||
can tell it was a real exception, not a typed failure path.
|
|
||||||
"""
|
"""
|
||||||
_stage_batch(tmp_path, 1)
|
_stage_batch(tmp_path, 1)
|
||||||
|
|
||||||
@@ -235,10 +234,10 @@ def test_submit_batch_unexpected_exception_recorded_in_results(
|
|||||||
assert resp.status_code == 200, resp.text
|
assert resp.status_code == 200, resp.text
|
||||||
body = resp.json()
|
body = resp.json()
|
||||||
assert body["failed"] == 1
|
assert body["failed"] == 1
|
||||||
# The marker is the SFTP_FAILED enum value (per Task 6 design
|
# Followup #3: marker is UNEXPECTED_ERROR, NOT SFTP_FAILED — the
|
||||||
# choice). The full exception class+message is in ``error`` so the
|
# exception class+message in ``error`` tells the operator this
|
||||||
# operator can distinguish a real exception from a typed failure.
|
# was a real exception (not a typed failure).
|
||||||
assert body["results"][0]["outcome"] == "sftp_failed"
|
assert body["results"][0]["outcome"] == SubmitOutcome.UNEXPECTED_ERROR.value
|
||||||
assert "kaboom" in body["results"][0]["error"]
|
assert "kaboom" in body["results"][0]["error"]
|
||||||
assert "RuntimeError" in body["results"][0]["error"]
|
assert "RuntimeError" in body["results"][0]["error"]
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,139 @@
|
|||||||
|
"""SP37 follow-up #3: SubmitOutcome.UNEXPECTED_ERROR for non-typed failures.
|
||||||
|
|
||||||
|
The router's per-file try/except previously coerced every unexpected
|
||||||
|
exception to ``SubmitOutcome.SFTP_FAILED`` because the helper had no
|
||||||
|
"unknown failure" enum value. That was misleading — a ``RuntimeError``
|
||||||
|
from ``cycl_store.add`` (e.g. ``RuntimeError("db down")``) is
|
||||||
|
definitely not an SFTP failure.
|
||||||
|
|
||||||
|
This test pins the new behavior: ``SubmitOutcome.UNEXPECTED_ERROR``
|
||||||
|
exists, the API surfaces it under the JSON key ``"unexpected_error"``,
|
||||||
|
and the typed SFTP_FAILED path is reserved for actual SFTP failures.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from fastapi.testclient import TestClient
|
||||||
|
|
||||||
|
|
||||||
|
_FIXTURE = Path(__file__).parent / "fixtures" / "submit-batch" / "single-claim.x12"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def client(tmp_path) -> TestClient:
|
||||||
|
"""Standard TestClient; conftest.py autouse handles DB + auth gate.
|
||||||
|
|
||||||
|
Seeds the clearhouse then flips sftp_block.stub to False so the
|
||||||
|
file-walking tests reach the walker. Matches the fixture shape in
|
||||||
|
test_api_submit_batch.py — local because only these two test files
|
||||||
|
need it (no conftest move yet).
|
||||||
|
"""
|
||||||
|
from cyclone import db as db_mod
|
||||||
|
from cyclone.store import store as cycl_store
|
||||||
|
|
||||||
|
db_mod._reset_for_tests()
|
||||||
|
db_mod.init_db()
|
||||||
|
cycl_store.ensure_clearhouse_seeded()
|
||||||
|
ch = cycl_store.get_clearhouse()
|
||||||
|
cycl_store.update_clearhouse(
|
||||||
|
ch.model_copy(update={"sftp_block": ch.sftp_block.model_copy(update={"stub": False})}),
|
||||||
|
)
|
||||||
|
|
||||||
|
from cyclone.api import app
|
||||||
|
|
||||||
|
return TestClient(app)
|
||||||
|
|
||||||
|
|
||||||
|
def test_unexpected_error_enum_value_exists():
|
||||||
|
"""The enum MUST have UNEXPECTED_ERROR so the API can distinguish
|
||||||
|
a non-typed exception from a real SFTP failure.
|
||||||
|
"""
|
||||||
|
from cyclone.submission.result import SubmitOutcome
|
||||||
|
|
||||||
|
assert hasattr(SubmitOutcome, "UNEXPECTED_ERROR")
|
||||||
|
assert SubmitOutcome.UNEXPECTED_ERROR.value == "unexpected_error"
|
||||||
|
|
||||||
|
|
||||||
|
def test_unexpected_error_is_distinct_from_typed_failures():
|
||||||
|
"""The new value must NOT collide with any existing failure value."""
|
||||||
|
from cyclone.submission.result import SubmitOutcome
|
||||||
|
|
||||||
|
# Compare against the typed failure values only (not UNEXPECTED_ERROR
|
||||||
|
# itself, which would trivially be in any set including it).
|
||||||
|
typed_failure_values = {
|
||||||
|
SubmitOutcome.SUBMITTED.value,
|
||||||
|
SubmitOutcome.SKIPPED.value,
|
||||||
|
SubmitOutcome.PARSE_FAILED.value,
|
||||||
|
SubmitOutcome.PAYER_MISMATCH.value,
|
||||||
|
SubmitOutcome.DB_FAILED.value,
|
||||||
|
SubmitOutcome.SFTP_FAILED.value,
|
||||||
|
}
|
||||||
|
assert SubmitOutcome.UNEXPECTED_ERROR.value not in typed_failure_values
|
||||||
|
|
||||||
|
|
||||||
|
def test_router_maps_unexpected_exception_to_unexpected_error(client: TestClient, monkeypatch, tmp_path):
|
||||||
|
"""If submit_file raises an unexpected exception, the API surfaces
|
||||||
|
``outcome="unexpected_error"`` (not ``"sftp_failed"``).
|
||||||
|
"""
|
||||||
|
from cyclone.submission.result import SubmitResult, SubmitOutcome
|
||||||
|
|
||||||
|
# Stage one file so the walker has something to submit.
|
||||||
|
batch_dir = tmp_path / "batch-test-claims"
|
||||||
|
batch_dir.mkdir()
|
||||||
|
(batch_dir / "claim-0.x12").write_bytes(_FIXTURE.read_bytes())
|
||||||
|
|
||||||
|
def _explode(path, **_kw):
|
||||||
|
raise RuntimeError("kaboom")
|
||||||
|
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"cyclone.api_routers.submission.submit_file", _explode,
|
||||||
|
)
|
||||||
|
|
||||||
|
resp = client.post(
|
||||||
|
"/api/submit-batch",
|
||||||
|
json={"ingest_dir": str(tmp_path), "validate": False},
|
||||||
|
)
|
||||||
|
assert resp.status_code == 200, resp.text
|
||||||
|
body = resp.json()
|
||||||
|
assert body["failed"] == 1
|
||||||
|
# The operator-visible outcome MUST be "unexpected_error", not "sftp_failed".
|
||||||
|
assert body["results"][0]["outcome"] == SubmitOutcome.UNEXPECTED_ERROR.value
|
||||||
|
# The full exception class + message stays in ``error`` so the
|
||||||
|
# operator can tell it was a real exception, not a typed failure.
|
||||||
|
assert "kaboom" in body["results"][0]["error"]
|
||||||
|
assert "RuntimeError" in body["results"][0]["error"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_typed_sftp_failure_still_uses_sftp_failed(client: TestClient, monkeypatch, tmp_path):
|
||||||
|
"""Typed SFTP failure (returned by submit_file) MUST still surface as
|
||||||
|
``outcome="sftp_failed"`` — the new enum value is for *unexpected*
|
||||||
|
exceptions only, not the typed SFTP_FAILED path.
|
||||||
|
"""
|
||||||
|
from cyclone.submission.result import SubmitResult, SubmitOutcome
|
||||||
|
|
||||||
|
batch_dir = tmp_path / "batch-test-claims"
|
||||||
|
batch_dir.mkdir()
|
||||||
|
(batch_dir / "claim-0.x12").write_bytes(_FIXTURE.read_bytes())
|
||||||
|
|
||||||
|
def _fake_submit(path, **_kw):
|
||||||
|
return SubmitResult(
|
||||||
|
file=path.name,
|
||||||
|
outcome=SubmitOutcome.SFTP_FAILED,
|
||||||
|
error="sftp connection refused",
|
||||||
|
)
|
||||||
|
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"cyclone.api_routers.submission.submit_file", _fake_submit,
|
||||||
|
)
|
||||||
|
|
||||||
|
resp = client.post(
|
||||||
|
"/api/submit-batch",
|
||||||
|
json={"ingest_dir": str(tmp_path), "validate": False},
|
||||||
|
)
|
||||||
|
assert resp.status_code == 200, resp.text
|
||||||
|
body = resp.json()
|
||||||
|
assert body["failed"] == 1
|
||||||
|
assert body["results"][0]["outcome"] == SubmitOutcome.SFTP_FAILED.value
|
||||||
|
assert body["results"][0]["error"] == "sftp connection refused"
|
||||||
Reference in New Issue
Block a user