refactor(whitelist): map domain errors via central exception registry

[abdulrafey1]

Closes: Whitelist routes map domain errors with inline per-route try/except

What

Migrate sparkth/api/v1/whitelist off inline try/except → HTTPException onto the centralized exception→HTTP registry from #493 (PR #513), so HTTP-status decisions live in one API-layer mapping instead of being duplicated (and message-string-matched) inside each route.

Changes

• refactor(whitelist): split the two single-file whitelist modules into packages — sparkth/api/v1/whitelist/ (routes.py + __init__.py) and sparkth/services/whitelist/ (service.py, exceptions.py, __init__.py) — with back-compat re-exports so existing import paths are unchanged.
• refactor(whitelist): add HTTP-agnostic domain exceptions (WhitelistError base + InvalidWhitelistValue, WhitelistEntryAlreadyExists, WhitelistEntryNotFound); the service raises them instead of ValueError.
• refactor(whitelist): move the IntegrityError unique-index race backstop into the service (rollback + log-with-context + re-raise as WhitelistEntryAlreadyExists), fixing the old inline catch that logged nothing.
• refactor(api): register the three concrete exception→status mappings (409/404/422) at import in the whitelist API package via register_exception_handler; drop all try/except / HTTPException from the route handlers.
• test(whitelist): switch service tests to the new exception types, add an IntegrityError race-path test (asserts rollback + the conflict log), assert exact API error bodies, and assert the mappings are registered.

How to Test

1. make test.backend.pytest — full suite passes (1364 passed). New/updated coverage in tests/services/test_whitelist.py and tests/api/v1/test_whitelist.py.
2. make mypy — clean (--strict).
3. make lint.backend and make lint.format.backend check=1 — both clean.
4. Behaviour check against a running backend (as a user with the whitelist permissions):
   • POST /api/v1/whitelist/ with a value already present → 409 {"detail": "Entry already exists: <value>"}.
   • POST /api/v1/whitelist/ with an invalid value (e.g. not-valid) → 422 {"detail": "Invalid email format: not-valid"}.
   • DELETE /api/v1/whitelist/{missing_id} → 404 {"detail": "Whitelist entry not found: <id>"}.

Notes

• Stacked PR: based on rafey/exception-to-http-code-mapping (PR feat(api): add centralized exception-to-http mapping mechanism #513), not main. Merge feat(api): add centralized exception-to-http mapping mechanism #513 first.
• Minor behaviour change (intentional): the 404 detail body changes from the old hard-coded "Whitelist entry not found" to "Whitelist entry not found: <id>". The old delete route discarded the service message; the new path renders str(exc), which is consistent with the 409/422 bodies (they already echo the offending value). Non-breaking — status is still 404 and no consumer reads this body.
• Test removed from feat(api): add centralized exception-to-http mapping mechanism #513's suite: tests/core/exceptions/test_handlers.py::test_global_registry_ships_empty is deleted here. Its premise ("the registry ships empty — this issue delivers only the mechanism") is deliberately ended by this PR: whitelist is the registry's first consumer, registering mappings at import. The _restore_registry fixture docstring was updated to match.
• No migration, no new env var, no dependency bump.

This description was written with the assistance of an LLM (Claude).

[abdulrafey1]

@claude-review

[claude]

Claude finished @abdulrafey1's task in 3m 6s —— View job

---

Review: PR #517 — map whitelist errors via central exception registry

• Read the PR diff vs origin/rafey/exception-to-http-code-mapping
• Review whitelist service + exceptions
• Review API routes + registry registration
• Review tests
• Check for unresolved review threads (none exist)
• Post review feedback

---

This is a clean, well-scoped refactor that lands exactly what the linked issue asks for. The inline try/except → HTTPException (with fragile "already exists" in msg string-matching) is gone, HTTP status decisions now live in the API layer via register_exception_handler, and the service raises HTTP-agnostic domain exceptions. It follows the CLAUDE.md "Domain exceptions → HTTP responses" guidance faithfully. No blocking issues found.

What's good

• Fixes the silent catch. Moving the IntegrityError unique-index backstop into service.add_entry with rollback() + a contextual logger.warning + re-raise (service.py:61-66) genuinely improves on the old inline catch that logged nothing.
• 1-to-1 exception→status design. WhitelistEntryAlreadyExists→409, WhitelistEntryNotFound→404, InvalidWhitelistValue→422 are each unambiguous, exactly as the guidance requires.
• Back-compat holds. Splitting both modules into packages with re-exporting __init__.py keeps from sparkth.services.whitelist import WhitelistService working for api/v1/auth.py and existing tests — verified no callers broke.
• Registration is guaranteed, not fragile. TestWhitelistExceptionRegistration doesn't import the API package itself, but sparkth.main (imported by the sparkth.lib.testing pytest plugin at session start) pulls in api/v1/api.py → the whitelist package __init__, so the import-time registration always fires before any test runs.
• Strong tests. Exact error-body assertions, the IntegrityError race-path test asserting both rollback and the conflict log, and the registration test. Removing test_global_registry_ships_empty with a documented rationale is the right call now that whitelist is the registry's first consumer.

Suggestions (non-blocking)

• Medium — IntegrityError translated to 409 unconditionally (service.py:63). Any integrity violation (FK on added_by_id, NOT NULL, …) becomes a misleading 409 Entry already exists. Realistically only the unique-value index races here, so it's not a regression, but consider narrowing to the specific constraint and re-raising the rest. Details in the inline comment. Fix this →
• Low — duplicated conflict message. f"Entry already exists: {normalized}" appears in both the pre-check (service.py:53-ish) and the IntegrityError path (service.py:66). A shared local/helper would prevent the two bodies drifting apart.
• Low — LLM-notice consistency. services/whitelist/exceptions.py carries the "generated with LLM (Claude) assistance" module notice, but the other new files (api/v1/whitelist/__init__.py, services/whitelist/__init__.py) don't. Worth aligning per the project's LLM-code convention.
• Nit — API coverage. The API tests exercise invalid-email → 422, but not invalid-domain → 422. The service-level test covers domains, so this is minor.

Assessment: Approve in spirit — solid refactor, good test coverage, matches the codebase's exception-handling pattern. Only the IntegrityError breadth is worth a second look; everything else is polish. Also confirmed: no migration, no new env var, docstrings updated.
· rafey/migrate-whitelist-routes-to-central-exception-management

[claude]

Medium — IntegrityError is translated to "already exists" unconditionally.

IntegrityError covers any integrity constraint, not just the unique-value index — e.g. an added_by_id foreign-key violation or a NOT NULL breach would also land here and be reported to the client as 409 Entry already exists, which is misleading. In practice added_by_id comes from an authenticated user so an FK failure is unlikely, and the unique index is the only realistic racing constraint, so this isn't a regression (the old inline catch did the same). But since this path is being reworked, consider narrowing it — e.g. inspect exc.orig / the constraint name and only map the unique-value violation to WhitelistEntryAlreadyExists, re-raising anything else so it surfaces as a 500 rather than a wrong 409.

Not blocking. The rollback + contextual logger.warning added here is a real improvement over the old catch that logged nothing.

[hamza-56]

Now that value has unique=True and the commit below translates IntegrityError into WhitelistEntryAlreadyExists, this pre-check SELECT is a redundant round trip.

[hamza-56]

This blanket translation assumes every IntegrityError is a duplicate value, but the insert can also violate the FK on added_by_id (e.g. the user was deleted mid-request). On Postgres that failure would surface as a 409 "Entry already exists" and get logged as an insert conflict, which is misleading.

[hamza-56]

nit: select(...).where(WhitelistedEmail.id == entry_id) plus one_or_none() is the long way to spell await session.get(WhitelistedEmail, entry_id), which is the idiomatic primary-key lookup and also uses the identity map.