Compare commits
5 Commits
aff3a13016
...
f62e1a7881
| Author | SHA1 | Date | |
|---|---|---|---|
| f62e1a7881 | |||
| dc5bff617d | |||
| abaf23c122 | |||
| ce0df8ac24 | |||
| cc02cb99c5 |
@@ -7,3 +7,4 @@ __pycache__/
|
|||||||
venv/
|
venv/
|
||||||
build/
|
build/
|
||||||
dist/
|
dist/
|
||||||
|
var/
|
||||||
|
|||||||
@@ -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}",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -82,7 +82,6 @@ PERMISSIONS: dict[tuple[str, str], set[Role]] = {
|
|||||||
("POST", "/api/inbox/payer-rejected"): WRITE_ROLES,
|
("POST", "/api/inbox/payer-rejected"): WRITE_ROLES,
|
||||||
("POST", "/api/reconcile"): WRITE_ROLES,
|
("POST", "/api/reconcile"): WRITE_ROLES,
|
||||||
("POST", "/api/reconciliation"): WRITE_ROLES,
|
("POST", "/api/reconciliation"): WRITE_ROLES,
|
||||||
("POST", "/api/resubmit"): WRITE_ROLES,
|
|
||||||
("POST", "/api/acks"): WRITE_ROLES,
|
("POST", "/api/acks"): WRITE_ROLES,
|
||||||
("POST", "/api/batches"): WRITE_ROLES, # /export-837 regenerates X12 from DB rows
|
("POST", "/api/batches"): WRITE_ROLES, # /export-837 regenerates X12 from DB rows
|
||||||
("POST", "/api/eligibility"): WRITE_ROLES,
|
("POST", "/api/eligibility"): WRITE_ROLES,
|
||||||
|
|||||||
@@ -94,6 +94,22 @@ def _op_timeout_seconds() -> float:
|
|||||||
return value
|
return value
|
||||||
|
|
||||||
|
|
||||||
|
@dataclass(frozen=True)
|
||||||
|
class SftpStat:
|
||||||
|
"""File metadata returned by :meth:`SftpClient.stat`.
|
||||||
|
|
||||||
|
Mirrors the subset of paramiko's ``SFTPAttributes`` that callers
|
||||||
|
actually need (``size``, ``modified_at``). Keeping the surface
|
||||||
|
narrow means callers don't need to import paramiko to consume
|
||||||
|
the result, and the wrapper's stub mode can populate it from a
|
||||||
|
plain ``os.stat_result`` without any paramiko dance.
|
||||||
|
|
||||||
|
Frozen so callers can hash / cache the result if they need to.
|
||||||
|
"""
|
||||||
|
size: int
|
||||||
|
modified_at: datetime | None = None
|
||||||
|
|
||||||
|
|
||||||
@dataclass
|
@dataclass
|
||||||
class InboundFile:
|
class InboundFile:
|
||||||
"""A single file observed in the inbound MFT path."""
|
"""A single file observed in the inbound MFT path."""
|
||||||
@@ -216,6 +232,30 @@ class SftpClient:
|
|||||||
return self._read_file_stub(remote_path)
|
return self._read_file_stub(remote_path)
|
||||||
return self._read_file_paramiko(remote_path)
|
return self._read_file_paramiko(remote_path)
|
||||||
|
|
||||||
|
def stat(self, remote_path: str) -> SftpStat:
|
||||||
|
"""Return file metadata for ``remote_path``.
|
||||||
|
|
||||||
|
Mirrors the subset of paramiko's ``SFTPAttributes`` that
|
||||||
|
callers need (``size`` for idempotency checks; ``modified_at``
|
||||||
|
for cache-busting). The SP37 submission helper uses
|
||||||
|
``stat().size`` to short-circuit re-uploads of already-uploaded
|
||||||
|
files (the SKIPPED outcome path).
|
||||||
|
|
||||||
|
Stub mode: reads ``os.stat_result`` from
|
||||||
|
``{staging_dir}/{remote_path}``.
|
||||||
|
|
||||||
|
Real mode: calls ``sftp.stat(remote_path)`` on a paramiko
|
||||||
|
connection.
|
||||||
|
|
||||||
|
Raises:
|
||||||
|
FileNotFoundError: if ``remote_path`` does not exist
|
||||||
|
(stub: missing local file; real: paramiko raises
|
||||||
|
``IOError`` which is a ``FileNotFoundError`` subclass).
|
||||||
|
"""
|
||||||
|
if self._stub:
|
||||||
|
return self._stat_stub(remote_path)
|
||||||
|
return self._stat_paramiko(remote_path)
|
||||||
|
|
||||||
def _read_file_stub(self, remote_path: str) -> bytes:
|
def _read_file_stub(self, remote_path: str) -> bytes:
|
||||||
"""Read bytes from ``{staging_dir}/{remote_path}`` (SP16 stub)."""
|
"""Read bytes from ``{staging_dir}/{remote_path}`` (SP16 stub)."""
|
||||||
staging = Path(self._block.staging_dir).resolve()
|
staging = Path(self._block.staging_dir).resolve()
|
||||||
@@ -224,6 +264,25 @@ class SftpClient:
|
|||||||
raise FileNotFoundError(f"inbound stub file not found: {target}")
|
raise FileNotFoundError(f"inbound stub file not found: {target}")
|
||||||
return target.read_bytes()
|
return target.read_bytes()
|
||||||
|
|
||||||
|
def _stat_stub(self, remote_path: str) -> SftpStat:
|
||||||
|
"""Return ``SftpStat`` for ``{staging_dir}/{remote_path}`` (SP37 stub).
|
||||||
|
|
||||||
|
Mirrors what paramiko's ``sftp.stat()`` returns in real mode:
|
||||||
|
``size`` from ``st.st_size`` and ``modified_at`` from
|
||||||
|
``st.st_mtime``. Raises ``FileNotFoundError`` if the local
|
||||||
|
file is missing — matches paramiko's ``IOError`` behavior
|
||||||
|
so the caller doesn't need to special-case stub mode.
|
||||||
|
"""
|
||||||
|
staging = Path(self._block.staging_dir).resolve()
|
||||||
|
target = staging / remote_path.lstrip("/")
|
||||||
|
if not target.is_file():
|
||||||
|
raise FileNotFoundError(f"inbound stub file not found: {target}")
|
||||||
|
st = target.stat()
|
||||||
|
return SftpStat(
|
||||||
|
size=st.st_size,
|
||||||
|
modified_at=datetime.fromtimestamp(st.st_mtime),
|
||||||
|
)
|
||||||
|
|
||||||
def get_secret(self, name: str) -> Optional[str]:
|
def get_secret(self, name: str) -> Optional[str]:
|
||||||
"""Fetch the auth secret from Keychain. Returns the stub secret if absent."""
|
"""Fetch the auth secret from Keychain. Returns the stub secret if absent."""
|
||||||
value = secrets.get_secret(name)
|
value = secrets.get_secret(name)
|
||||||
@@ -506,6 +565,20 @@ class SftpClient:
|
|||||||
shutil.copyfileobj(f, buf, length=64 * 1024)
|
shutil.copyfileobj(f, buf, length=64 * 1024)
|
||||||
return buf.getvalue()
|
return buf.getvalue()
|
||||||
|
|
||||||
|
def _stat_paramiko(self, remote_path: str) -> SftpStat:
|
||||||
|
"""Return ``SftpStat`` for ``remote_path`` via paramiko.
|
||||||
|
|
||||||
|
Paramiko's ``SFTPAttributes`` already provides ``st_size`` and
|
||||||
|
``st_mtime``; we project them into our narrow public
|
||||||
|
``SftpStat`` shape so callers don't need to know about paramiko.
|
||||||
|
"""
|
||||||
|
with self._connect() as (ssh, sftp):
|
||||||
|
attr = sftp.stat(remote_path)
|
||||||
|
return SftpStat(
|
||||||
|
size=attr.st_size or 0,
|
||||||
|
modified_at=datetime.fromtimestamp(attr.st_mtime or 0),
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Module-level helper
|
# Module-level helper
|
||||||
|
|||||||
@@ -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,50 @@
|
|||||||
|
"""SP37 follow-up #1: Fail-closed for unregistered paths.
|
||||||
|
|
||||||
|
The permissions matrix is fail-closed — endpoints not listed are denied
|
||||||
|
(None). The pre-existing ``("POST", "/api/resubmit"): WRITE_ROLES``
|
||||||
|
entry was a dead prefix that ONLY matched the exact path ``/api/resubmit``
|
||||||
|
(which is not a registered route). The actual resubmit lives at
|
||||||
|
``/api/inbox/rejected/resubmit`` (already covered by its own entry).
|
||||||
|
|
||||||
|
This test pins the fail-closed invariant: ``/api/resubmit`` MUST have
|
||||||
|
no granted permission because no route is registered there. Without
|
||||||
|
this test, a future contributor could re-add the dead entry (or any
|
||||||
|
similar dead prefix) and the auth system would silently grant access
|
||||||
|
to a non-existent path.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from cyclone.auth.permissions import PERMISSIONS, allowed_roles
|
||||||
|
|
||||||
|
|
||||||
|
def test_api_resubmit_exact_path_not_in_matrix():
|
||||||
|
"""``/api/resubmit`` (exact, no children) must NOT be in the matrix.
|
||||||
|
|
||||||
|
The actual resubmit endpoint is ``/api/inbox/rejected/resubmit``,
|
||||||
|
which has its own entry. The pre-existing
|
||||||
|
``("POST", "/api/resubmit"): WRITE_ROLES`` entry only matched the
|
||||||
|
exact path ``/api/resubmit`` and never any real route.
|
||||||
|
"""
|
||||||
|
assert ("POST", "/api/resubmit") not in PERMISSIONS
|
||||||
|
|
||||||
|
|
||||||
|
def test_api_resubmit_returns_none_via_allowed_roles():
|
||||||
|
"""Fail-closed: ``allowed_roles("POST", "/api/resubmit")`` is None.
|
||||||
|
|
||||||
|
The matrix default is DENY. A registered entry that matches the
|
||||||
|
path would return a non-empty set of roles; the absence of any
|
||||||
|
entry should return None.
|
||||||
|
"""
|
||||||
|
assert allowed_roles("POST", "/api/resubmit") is None
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("path", [
|
||||||
|
"/api/resubmit",
|
||||||
|
"/api/resubmit/",
|
||||||
|
"/api/resubmit/anything",
|
||||||
|
])
|
||||||
|
def test_api_resubmit_prefix_variants_all_deny(path):
|
||||||
|
"""No path under ``/api/resubmit`` should match any permission entry."""
|
||||||
|
assert allowed_roles("POST", path) is None
|
||||||
@@ -14,6 +14,11 @@ the fresh DB up to v20, insert representative rows, then replay the
|
|||||||
exact UPDATE statement the migration uses. Replaying the UPDATE
|
exact UPDATE statement the migration uses. Replaying the UPDATE
|
||||||
proves the SQL works as intended even though the migration itself
|
proves the SQL works as intended even though the migration itself
|
||||||
already ran over an empty table.
|
already ran over an empty table.
|
||||||
|
|
||||||
|
The backfill UPDATE is loaded directly from the migration file (not
|
||||||
|
a hand-maintained copy) so the test cannot drift from production —
|
||||||
|
see :func:`_load_migration_0020_backfill_sql` and the regression
|
||||||
|
locks in ``test_migration_0020_no_drift.py``.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
@@ -27,18 +32,50 @@ import sqlalchemy as sa
|
|||||||
from cyclone import db_migrate
|
from cyclone import db_migrate
|
||||||
|
|
||||||
|
|
||||||
# The backfill UPDATE the migration executes (extracted so the test can
|
def _migration_0020_path() -> Path:
|
||||||
# replay it against rows that didn't exist when init_db ran).
|
return (
|
||||||
BACKFILL_SQL = (
|
Path(__file__).parent.parent / "src" / "cyclone" / "migrations"
|
||||||
"UPDATE batches "
|
/ "0020_add_batch_txn_set_control_number.sql"
|
||||||
"SET transaction_set_control_number = "
|
|
||||||
"json_extract(raw_result_json, '$.envelope.transaction_set_control_number') "
|
|
||||||
"WHERE raw_result_json IS NOT NULL "
|
|
||||||
"AND json_extract(raw_result_json, '$.envelope.transaction_set_control_number') "
|
|
||||||
"IS NOT NULL"
|
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _extract_update_statements(sql: str) -> list[str]:
|
||||||
|
"""Split a migration into statements; return the UPDATE ones.
|
||||||
|
|
||||||
|
Mirrors db_migrate.run()'s splitter (strip ``--`` comments,
|
||||||
|
split on ``;``) so the test extraction can never disagree with
|
||||||
|
what the runner actually executes.
|
||||||
|
"""
|
||||||
|
lines = [
|
||||||
|
line for line in sql.splitlines()
|
||||||
|
if not line.strip().startswith("--")
|
||||||
|
]
|
||||||
|
cleaned = "\n".join(lines)
|
||||||
|
return [
|
||||||
|
stmt.strip() for stmt in cleaned.split(";")
|
||||||
|
if stmt.strip()
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
def _load_migration_0020_backfill_sql() -> str:
|
||||||
|
"""Return migration 0020's UPDATE statement (the backfill).
|
||||||
|
|
||||||
|
Reads the migration file at test time and extracts its UPDATE.
|
||||||
|
Test code that needs to replay the backfill against rows that
|
||||||
|
didn't exist when init_db ran uses this helper — guarantees the
|
||||||
|
replayed SQL is byte-identical to what production will run.
|
||||||
|
"""
|
||||||
|
sql = _migration_0020_path().read_text()
|
||||||
|
updates = [
|
||||||
|
s for s in _extract_update_statements(sql)
|
||||||
|
if s.upper().startswith("UPDATE ")
|
||||||
|
]
|
||||||
|
assert len(updates) == 1, (
|
||||||
|
f"expected exactly 1 UPDATE in migration 0020, found {len(updates)}: {updates}"
|
||||||
|
)
|
||||||
|
return updates[0]
|
||||||
|
|
||||||
|
|
||||||
def _fresh_engine(path: Path) -> sa.Engine:
|
def _fresh_engine(path: Path) -> sa.Engine:
|
||||||
return sa.create_engine(f"sqlite:///{path}", future=True)
|
return sa.create_engine(f"sqlite:///{path}", future=True)
|
||||||
|
|
||||||
@@ -115,7 +152,7 @@ def test_migration_0020_backfills_when_key_present(
|
|||||||
"VALUES ('B-ST02-1', '837p', 'mig0020-st02.txt', '2026-07-07 00:00:00', ?)",
|
"VALUES ('B-ST02-1', '837p', 'mig0020-st02.txt', '2026-07-07 00:00:00', ?)",
|
||||||
(json.dumps(raw_json),),
|
(json.dumps(raw_json),),
|
||||||
)
|
)
|
||||||
conn.exec_driver_sql(BACKFILL_SQL)
|
conn.exec_driver_sql(_load_migration_0020_backfill_sql())
|
||||||
|
|
||||||
with migrated_engine.connect() as conn:
|
with migrated_engine.connect() as conn:
|
||||||
row = conn.exec_driver_sql(
|
row = conn.exec_driver_sql(
|
||||||
@@ -141,7 +178,7 @@ def test_migration_0020_backfill_conditional_on_key_present(
|
|||||||
"VALUES ('B-NOST-1', '837p', 'mig0020-nost.txt', '2026-07-07 00:00:00', ?)",
|
"VALUES ('B-NOST-1', '837p', 'mig0020-nost.txt', '2026-07-07 00:00:00', ?)",
|
||||||
(json.dumps(raw_json),),
|
(json.dumps(raw_json),),
|
||||||
)
|
)
|
||||||
conn.exec_driver_sql(BACKFILL_SQL)
|
conn.exec_driver_sql(_load_migration_0020_backfill_sql())
|
||||||
|
|
||||||
with migrated_engine.connect() as conn:
|
with migrated_engine.connect() as conn:
|
||||||
row = conn.exec_driver_sql(
|
row = conn.exec_driver_sql(
|
||||||
@@ -163,7 +200,7 @@ def test_migration_0020_backfill_handles_null_raw_result_json(
|
|||||||
"INSERT INTO batches (id, kind, input_filename, parsed_at) "
|
"INSERT INTO batches (id, kind, input_filename, parsed_at) "
|
||||||
"VALUES ('B-NULL-1', '837p', 'mig0020-null.txt', '2026-07-07 00:00:00')"
|
"VALUES ('B-NULL-1', '837p', 'mig0020-null.txt', '2026-07-07 00:00:00')"
|
||||||
)
|
)
|
||||||
conn.exec_driver_sql(BACKFILL_SQL)
|
conn.exec_driver_sql(_load_migration_0020_backfill_sql())
|
||||||
|
|
||||||
with migrated_engine.connect() as conn:
|
with migrated_engine.connect() as conn:
|
||||||
row = conn.exec_driver_sql(
|
row = conn.exec_driver_sql(
|
||||||
|
|||||||
@@ -0,0 +1,66 @@
|
|||||||
|
"""SP37 follow-up #4: detect drift between test BACKFILL_SQL and migration SQL.
|
||||||
|
|
||||||
|
Followup #1 of the SP37 final-state tracker. The previous test file
|
||||||
|
maintained a hand-copied ``BACKFILL_SQL`` constant alongside the
|
||||||
|
migration file. If a future contributor edited one but not the other,
|
||||||
|
the test would silently replay a different SQL than production —
|
||||||
|
defeating the regression test.
|
||||||
|
|
||||||
|
The fix: ``test_migration_0020.py`` now reads the migration file at
|
||||||
|
test time and extracts its UPDATE. This file imports the helper and
|
||||||
|
pins the invariant — so a future contributor who edits the migration
|
||||||
|
automatically gets the new SQL replayed in tests, and a contributor
|
||||||
|
who removes the backfill UPDATE gets a loud extraction failure.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from test_migration_0020 import (
|
||||||
|
_extract_update_statements,
|
||||||
|
_load_migration_0020_backfill_sql,
|
||||||
|
_migration_0020_path,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_migration_0020_backfill_sql_uses_migration_file():
|
||||||
|
"""The backfill SQL used in tests must come from the migration file.
|
||||||
|
|
||||||
|
Guards against the previous pattern of a hand-maintained
|
||||||
|
BACKFILL_SQL constant that could drift from the actual migration.
|
||||||
|
"""
|
||||||
|
sql = _load_migration_0020_backfill_sql()
|
||||||
|
# Sanity check: the SQL targets batches.transaction_set_control_number
|
||||||
|
# via json_extract on raw_result_json. If a contributor changes
|
||||||
|
# the column name or the JSON path, this assertion catches it.
|
||||||
|
assert "UPDATE batches" in sql
|
||||||
|
assert "SET transaction_set_control_number" in sql
|
||||||
|
assert "json_extract(raw_result_json" in sql
|
||||||
|
assert "$.envelope.transaction_set_control_number" in sql
|
||||||
|
|
||||||
|
|
||||||
|
def test_migration_0020_backfill_sql_is_non_empty_single_statement():
|
||||||
|
"""The helper returns a non-empty SQL string suitable for direct
|
||||||
|
execution via exec_driver_sql. The existing
|
||||||
|
``test_migration_0020.py`` tests use this helper to replay the
|
||||||
|
SQL against representative rows — so any successful replay
|
||||||
|
exercises the migration's actual UPDATE.
|
||||||
|
"""
|
||||||
|
sql = _load_migration_0020_backfill_sql()
|
||||||
|
assert sql # non-empty
|
||||||
|
assert ";" not in sql # already stripped by the splitter
|
||||||
|
|
||||||
|
|
||||||
|
def test_migration_0020_has_exactly_one_update():
|
||||||
|
"""Guardrail: if a future migration adds a second UPDATE, the
|
||||||
|
extraction fails loudly so the test author can decide which one
|
||||||
|
is the backfill.
|
||||||
|
"""
|
||||||
|
sql = _migration_0020_path().read_text()
|
||||||
|
updates = [
|
||||||
|
s for s in _extract_update_statements(sql)
|
||||||
|
if s.upper().startswith("UPDATE ")
|
||||||
|
]
|
||||||
|
assert len(updates) == 1, (
|
||||||
|
f"migration 0020 should have exactly 1 UPDATE (the backfill); "
|
||||||
|
f"found {len(updates)}. If you added a second UPDATE, update "
|
||||||
|
f"_load_migration_0020_backfill_sql() to pick the right one."
|
||||||
|
)
|
||||||
@@ -0,0 +1,150 @@
|
|||||||
|
"""SP37 follow-up #5: SftpClient.stat() method.
|
||||||
|
|
||||||
|
The SftpClient wrapper previously exposed ``write_file``,
|
||||||
|
``list_inbound``, ``list_inbound_names``, ``download_inbound``, and
|
||||||
|
``read_file`` — but NO ``stat()``. The canonical
|
||||||
|
``cyclone.submission.submit_file`` helper needs to compare the local
|
||||||
|
file size against the remote file size for idempotency; the lack of
|
||||||
|
``stat()`` forced the helper to bypass the wrapper and open paramiko
|
||||||
|
directly (see ``submission/core.py:_default_sftp_factory``).
|
||||||
|
|
||||||
|
This file adds a ``stat()`` method to the wrapper so the helper can
|
||||||
|
use ``SftpClient(block=b)`` instead of bypassing the wrapper. Tests
|
||||||
|
cover the stub implementation only (paramiko real-mode tests would
|
||||||
|
need a real SFTP server).
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from cyclone.clearhouse import SftpClient, SftpStat
|
||||||
|
from cyclone.providers import SftpBlock
|
||||||
|
|
||||||
|
|
||||||
|
def _stub_block(staging_dir: Path) -> SftpBlock:
|
||||||
|
return SftpBlock(
|
||||||
|
host="mft.example.com",
|
||||||
|
port=22,
|
||||||
|
username="user",
|
||||||
|
auth={"password_keychain_account": "x"},
|
||||||
|
paths={"inbound": "/inbound", "outbound": "/outbound"},
|
||||||
|
staging_dir=str(staging_dir),
|
||||||
|
stub=True,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def staging(tmp_path: Path) -> Path:
|
||||||
|
"""A staging dir pre-populated with one file. Mirrors what the
|
||||||
|
SP9 stub layout looks like when an operator has dropped a real
|
||||||
|
inbound file for testing."""
|
||||||
|
inbound = tmp_path / "inbound"
|
||||||
|
inbound.mkdir(parents=True)
|
||||||
|
(inbound / "test.txt").write_bytes(b"hello world\n")
|
||||||
|
return tmp_path
|
||||||
|
|
||||||
|
|
||||||
|
# ---- SftpStat dataclass ----------------------------------------------------- #
|
||||||
|
|
||||||
|
|
||||||
|
def test_sftp_stat_dataclass_exists():
|
||||||
|
"""The wrapper exposes a small SftpStat dataclass for the result.
|
||||||
|
|
||||||
|
Mirrors the paramiko ``SFTPAttributes`` shape (at least ``size``)
|
||||||
|
but keeps the public surface narrow so callers can use it
|
||||||
|
without depending on paramiko.
|
||||||
|
"""
|
||||||
|
assert SftpStat is not None
|
||||||
|
stat = SftpStat(size=42, modified_at=None)
|
||||||
|
assert stat.size == 42
|
||||||
|
assert stat.modified_at is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_sftp_stat_is_frozen():
|
||||||
|
"""SftpStat is a frozen dataclass so it can be hashed / used as a
|
||||||
|
cache key by future callers without surprise mutation."""
|
||||||
|
from dataclasses import FrozenInstanceError
|
||||||
|
|
||||||
|
stat = SftpStat(size=1, modified_at=None)
|
||||||
|
with pytest.raises(FrozenInstanceError):
|
||||||
|
stat.size = 2 # type: ignore[misc]
|
||||||
|
|
||||||
|
|
||||||
|
# ---- Public method --------------------------------------------------------- #
|
||||||
|
|
||||||
|
|
||||||
|
def test_sftp_client_has_stat_method():
|
||||||
|
"""``SftpClient`` exposes a ``stat(remote_path)`` public method.
|
||||||
|
|
||||||
|
Guards the API surface — a future contributor who renames or
|
||||||
|
removes ``stat()`` (forcing the helper to bypass the wrapper
|
||||||
|
again) would break this test, prompting them to update the
|
||||||
|
helper instead.
|
||||||
|
"""
|
||||||
|
assert hasattr(SftpClient, "stat")
|
||||||
|
assert callable(SftpClient.stat)
|
||||||
|
|
||||||
|
|
||||||
|
def test_sftp_client_stat_returns_size_in_stub_mode(staging: Path):
|
||||||
|
"""Stub mode: ``stat(remote_path)`` returns the local staging
|
||||||
|
file's size via ``os.stat_result.st_size``. Mirrors what a real
|
||||||
|
paramiko ``SFTPAttributes.st_size`` would return for a remote file.
|
||||||
|
"""
|
||||||
|
client = SftpClient(block=_stub_block(staging))
|
||||||
|
stat = client.stat("/inbound/test.txt")
|
||||||
|
assert stat.size == len(b"hello world\n")
|
||||||
|
|
||||||
|
|
||||||
|
def test_sftp_client_stat_returns_modified_at_in_stub_mode(staging: Path):
|
||||||
|
"""Stub mode: ``stat`` populates ``modified_at`` from the local
|
||||||
|
file's mtime. Mirrors paramiko's behavior for parity."""
|
||||||
|
client = SftpClient(block=_stub_block(staging))
|
||||||
|
stat = client.stat("/inbound/test.txt")
|
||||||
|
assert stat.modified_at is not None
|
||||||
|
|
||||||
|
|
||||||
|
def test_sftp_client_stat_raises_for_missing_file(staging: Path):
|
||||||
|
"""Stub mode: ``stat`` raises ``FileNotFoundError`` for a path
|
||||||
|
that doesn't exist locally — matches the real paramiko behavior
|
||||||
|
(which raises ``IOError``/``FileNotFoundError``). The helper's
|
||||||
|
SKIPPED-outcome short-circuit depends on this exception type.
|
||||||
|
"""
|
||||||
|
client = SftpClient(block=_stub_block(staging))
|
||||||
|
with pytest.raises(FileNotFoundError):
|
||||||
|
client.stat("/inbound/missing.txt")
|
||||||
|
|
||||||
|
|
||||||
|
def test_sftp_client_stat_handles_large_file(staging: Path):
|
||||||
|
"""Stub mode: ``stat`` returns the correct size for non-trivial files.
|
||||||
|
|
||||||
|
Sanity check: the size path doesn't truncate or accidentally
|
||||||
|
cast (e.g. to int8) on larger files.
|
||||||
|
"""
|
||||||
|
payload = b"x" * (1024 * 1024) # 1 MiB
|
||||||
|
(staging / "inbound" / "big.bin").write_bytes(payload)
|
||||||
|
client = SftpClient(block=_stub_block(staging))
|
||||||
|
stat = client.stat("/inbound/big.bin")
|
||||||
|
assert stat.size == 1024 * 1024
|
||||||
|
|
||||||
|
|
||||||
|
def test_sftp_client_stat_supports_idempotency_check(staging: Path):
|
||||||
|
"""The submission helper's idempotency check works against the
|
||||||
|
wrapper now.
|
||||||
|
|
||||||
|
Replays the SKIPPED-outcome check from
|
||||||
|
``cyclone.submission.submit_file`` (lines 178-182) using the
|
||||||
|
wrapper directly. Before this follow-up, the helper had to open
|
||||||
|
paramiko directly because the wrapper lacked ``stat()``.
|
||||||
|
"""
|
||||||
|
payload = b"identical contents"
|
||||||
|
(staging / "outbound").mkdir(parents=True, exist_ok=True)
|
||||||
|
(staging / "outbound" / "claim.x12").write_bytes(payload)
|
||||||
|
|
||||||
|
client = SftpClient(block=_stub_block(staging))
|
||||||
|
local_size = len(payload)
|
||||||
|
# stat() returns the size that was previously uploaded.
|
||||||
|
remote_stat = client.stat("/outbound/claim.x12")
|
||||||
|
assert remote_stat.size == local_size # idempotency check passes
|
||||||
|
# → SKIPPED outcome in the submission helper
|
||||||
@@ -196,13 +196,19 @@ def test_submit_file_uses_default_paramiko_factory(monkeypatch):
|
|||||||
|
|
||||||
|
|
||||||
def test_submit_batch_cli_help():
|
def test_submit_batch_cli_help():
|
||||||
"""`cyclone submit-batch --help` exits 0 and shows the flags."""
|
"""`cyclone submit-batch --help` exits 0 and shows the flags.
|
||||||
import subprocess
|
|
||||||
import sys
|
Uses ``click.testing.CliRunner`` instead of ``subprocess.run`` for
|
||||||
result = subprocess.run(
|
~10ms vs ~200ms per call. Same assertions — the CliRunner
|
||||||
[sys.executable, "-m", "cyclone.cli", "submit-batch", "--help"],
|
``result.exit_code`` matches the subprocess returncode, and
|
||||||
capture_output=True, text=True,
|
``result.output`` is the Click-rendered help text (subprocess's
|
||||||
cwd=str(Path(__file__).parent.parent),
|
stdout).
|
||||||
)
|
"""
|
||||||
assert result.returncode == 0
|
from click.testing import CliRunner
|
||||||
assert "--ingest-dir" in result.stdout
|
|
||||||
|
from cyclone.cli import main
|
||||||
|
|
||||||
|
runner = CliRunner()
|
||||||
|
result = runner.invoke(main, ["submit-batch", "--help"])
|
||||||
|
assert result.exit_code == 0, result.output
|
||||||
|
assert "--ingest-dir" in result.output
|
||||||
|
|||||||
@@ -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