refactor(api): migrate GlobalExceptionHandler to RFC 9457 ProblemDetail #466

New Issue

marcel · 2026-05-07T17:25:41+02:00

marcel commented

2026-05-07 17:25:41 +02:00

Context

backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java returns a custom record ErrorResponse(ErrorCode code, String message) for every error path. RFC 9457 (application/problem+json) is the modern web-standard error format and is supported first-class in Spring Boot 4 via org.springframework.http.ProblemDetail.

This is not a security or correctness issue — current handler is consistent and prevents stack-trace leaks (audit verified). It's an interop and ergonomics improvement: third-party API consumers, AI tooling, and standardized error-handling middleware all expect RFC 9457.

The ErrorCode enum has 47 codes — they all become the type URI suffix, preserving the typed error info already used by the frontend in frontend/src/lib/shared/errors.ts.

Approach

Migrate every @ExceptionHandler to return ProblemDetail. Keep ErrorCode as the source of truth; expose it via a code property in the ProblemDetail's extensions.

Pattern

@ExceptionHandler(DomainException.class)
ProblemDetail handleDomain(DomainException ex) {
    var pd = ProblemDetail.forStatusAndDetail(ex.getStatus(), ex.getMessage());
    pd.setType(URI.create("https://errors.familienarchiv.app/" + ex.getCode().name()));
    pd.setTitle(ex.getCode().name());
    pd.setProperty("code", ex.getCode().name());
    return pd;
}

The frontend already extracts code from the response body (per CLAUDE.md API client convention). After migration, change the extractor to read result.error.code instead of treating the whole body as {code, message}. Both work because the new shape is {type, title, status, detail, code} — code is still present.

Validation handlers

MethodArgumentNotValidException and ConstraintViolationException should add a errors property (array of field-error objects) per RFC 9457's "violations" extension convention:

pd.setProperty("violations", ex.getBindingResult().getFieldErrors().stream()
    .map(e -> Map.of("field", e.getField(), "message", e.getDefaultMessage()))
    .toList());

`ResponseStatusException` handler

Spring Boot 4 already handles this — consider removing the explicit handler and letting the framework's default produce a ProblemDetail.

Generic Exception handler

Keep the catch-all but switch to ProblemDetail. Critically, do not include the exception class name or stack trace in the response — the audit verified this is currently safe; preserve that property.

Critical files

backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java
backend/src/main/java/org/raddatz/familienarchiv/exception/ErrorResponse.java — delete (dead after migration)
backend/src/test/java/.../exception/GlobalExceptionHandlerTest.java
frontend/src/lib/shared/api.server.ts or wherever the error extractor lives — read from new shape
frontend/src/lib/shared/errors.ts — confirm the getErrorMessage resolver still works (signature: input is { code?: string })

Verification

Backend test: hit a 404 endpoint, response body is {type, title, status, detail, code} with Content-Type: application/problem+json.
Backend test: hit a validation endpoint with bad input, response body has violations: [{field, message}, ...].
Frontend smoke: trigger any error path (e.g., delete a non-existent document); UI shows the translated error message — i18n still resolves from code.
OpenAPI: npm run generate:api produces the new response shape; existing fetch wrappers still compile.

Acceptance criteria

Every error response has Content-Type: application/problem+json.
Every error response includes a code property matching an ErrorCode enum value.
Validation errors include a violations array.
No stack traces or exception class names in response bodies.
Frontend getErrorMessage still produces translated messages from code.
OpenAPI spec regenerated; frontend types build cleanly.

Effort

S — half a day; the change is mechanical because every handler follows the same pattern.

Risk if not addressed

None operationally. Pure interop improvement.

Tracked in audit doc as F-16 (Medium).

## Context `backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java` returns a custom record `ErrorResponse(ErrorCode code, String message)` for every error path. RFC 9457 (`application/problem+json`) is the modern web-standard error format and is supported first-class in Spring Boot 4 via `org.springframework.http.ProblemDetail`. This is **not** a security or correctness issue — current handler is consistent and prevents stack-trace leaks (audit verified). It's an interop and ergonomics improvement: third-party API consumers, AI tooling, and standardized error-handling middleware all expect RFC 9457. The `ErrorCode` enum has 47 codes — they all become the `type` URI suffix, preserving the typed error info already used by the frontend in `frontend/src/lib/shared/errors.ts`. ## Approach Migrate every `@ExceptionHandler` to return `ProblemDetail`. Keep `ErrorCode` as the source of truth; expose it via a `code` property in the ProblemDetail's extensions. ### Pattern ```java @ExceptionHandler(DomainException.class) ProblemDetail handleDomain(DomainException ex) { var pd = ProblemDetail.forStatusAndDetail(ex.getStatus(), ex.getMessage()); pd.setType(URI.create("https://errors.familienarchiv.app/" + ex.getCode().name())); pd.setTitle(ex.getCode().name()); pd.setProperty("code", ex.getCode().name()); return pd; } ``` The frontend already extracts `code` from the response body (per `CLAUDE.md` API client convention). After migration, change the extractor to read `result.error.code` instead of treating the whole body as `{code, message}`. Both work because the new shape is `{type, title, status, detail, code}` — `code` is still present. ### Validation handlers `MethodArgumentNotValidException` and `ConstraintViolationException` should add a `errors` property (array of field-error objects) per RFC 9457's "violations" extension convention: ```java pd.setProperty("violations", ex.getBindingResult().getFieldErrors().stream() .map(e -> Map.of("field", e.getField(), "message", e.getDefaultMessage())) .toList()); ``` ### `ResponseStatusException` handler Spring Boot 4 already handles this — consider removing the explicit handler and letting the framework's default produce a ProblemDetail. ### Generic Exception handler Keep the catch-all but switch to ProblemDetail. Critically, **do not include the exception class name or stack trace** in the response — the audit verified this is currently safe; preserve that property. ## Critical files - `backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java` - `backend/src/main/java/org/raddatz/familienarchiv/exception/ErrorResponse.java` — delete (dead after migration) - `backend/src/test/java/.../exception/GlobalExceptionHandlerTest.java` - `frontend/src/lib/shared/api.server.ts` or wherever the error extractor lives — read from new shape - `frontend/src/lib/shared/errors.ts` — confirm the `getErrorMessage` resolver still works (signature: input is `{ code?: string }`) ## Verification 1. **Backend test**: hit a 404 endpoint, response body is `{type, title, status, detail, code}` with `Content-Type: application/problem+json`. 2. **Backend test**: hit a validation endpoint with bad input, response body has `violations: [{field, message}, ...]`. 3. **Frontend smoke**: trigger any error path (e.g., delete a non-existent document); UI shows the translated error message — i18n still resolves from `code`. 4. **OpenAPI**: `npm run generate:api` produces the new response shape; existing fetch wrappers still compile. ## Acceptance criteria - [ ] Every error response has `Content-Type: application/problem+json`. - [ ] Every error response includes a `code` property matching an `ErrorCode` enum value. - [ ] Validation errors include a `violations` array. - [ ] No stack traces or exception class names in response bodies. - [ ] Frontend `getErrorMessage` still produces translated messages from `code`. - [ ] OpenAPI spec regenerated; frontend types build cleanly. ## Effort S — half a day; the change is mechanical because every handler follows the same pattern. ## Risk if not addressed None operationally. Pure interop improvement. Tracked in audit doc as **F-16** (Medium).

marcel added the P2-medium refactor labels 2026-05-07 17:27:01 +02:00

marcel commented

2026-05-07 17:47:59 +02:00

🏛️ Markus Keller — Application Architect

Observations

The issue is well-scoped and the migration is genuinely mechanical. The existing GlobalExceptionHandler is a clean 70-line file; the new one will be roughly the same size.
ProblemDetail is a first-class citizen in Spring Boot 4 / Spring Framework 7. No new dependency needed — spring-web already ships it. The Content-Type: application/problem+json is set automatically when returning ProblemDetail from a @ExceptionHandler.
The current ErrorResponse record lives as an inner record of GlobalExceptionHandler (line 71 of the file). It is not imported anywhere else in the backend — safe to delete after migration.
ErrorCode drift already exists before this issue is even implemented. Backend has THUMBNAIL_BACKFILL_ALREADY_RUNNING and NOTIFICATION_NOT_FOUND that are absent from frontend/src/lib/shared/errors.ts. Frontend has MISSING_CREDENTIALS that is absent from the backend enum. This pre-existing gap should be fixed in the same PR — it is exactly the kind of hygiene this refactor motivates.
The ResponseStatusException handler currently uses ex.getReason() as the detail message. getReason() is nullable in Spring 6+; a null reason passed to ProblemDetail.forStatusAndDetail() will produce null in the detail field. Add a null guard.
The issue correctly flags the ResponseStatusException handler as potentially removable. In Spring Boot 4, the framework's ResponseEntityExceptionHandler already handles ResponseStatusException and produces a ProblemDetail — but only if the handler class extends it. Since GlobalExceptionHandler does not extend ResponseEntityExceptionHandler, the explicit handler must remain unless we change the class hierarchy. Recommendation: do not extend ResponseEntityExceptionHandler — doing so implicitly inherits 20+ exception mappings that may conflict with the existing custom handlers. Keep the explicit handler, just switch it to ProblemDetail.
No Flyway migration, no schema change, no new Spring service — doc update requirements from the checklist are minimal. Only CLAUDE.md error-handling section needs a note about the new response shape; no diagram changes required.

Recommendations

Use @ControllerAdvice + ResponseBodyAdvice or keep @RestControllerAdvice as-is — no class hierarchy change needed.
Set the type URI to an enum-keyed path: URI.create("https://errors.familienarchiv.app/" + code.name()). Even if the domain never resolves, RFC 9457 allows it to be a non-dereferenceable identifier.
Null-guard ex.getReason() in the ResponseStatusException handler: pd.setDetail(ex.getReason() != null ? ex.getReason() : ex.getMessage()).
Fix the pre-existing ErrorCode drift in the same PR: add THUMBNAIL_BACKFILL_ALREADY_RUNNING and NOTIFICATION_NOT_FOUND to errors.ts, remove MISSING_CREDENTIALS from the frontend type union (or add it to the backend enum if it is genuinely used).
Write an ADR entry — not a standalone ADR file, but a one-paragraph note in docs/ARCHITECTURE.md under a new "Error response format" section — capturing why RFC 9457 was chosen and what the code extension property preserves.

Open Decisions

violations array format for validation errors: The issue proposes [{field, message}]. For ConstraintViolationException, the property path includes the method name prefix (e.g., createPerson.dto.lastName). Decide whether to strip the prefix before exposing it in the violations array or expose the raw path. Raw is simpler; stripped is more consumer-friendly. Pick one and specify it in the issue before implementation starts.

## 🏛️ Markus Keller — Application Architect ### Observations - The issue is well-scoped and the migration is genuinely mechanical. The existing `GlobalExceptionHandler` is a clean 70-line file; the new one will be roughly the same size. - `ProblemDetail` is a first-class citizen in Spring Boot 4 / Spring Framework 7. No new dependency needed — `spring-web` already ships it. The `Content-Type: application/problem+json` is set automatically when returning `ProblemDetail` from a `@ExceptionHandler`. - The current `ErrorResponse` record lives as an inner record of `GlobalExceptionHandler` (line 71 of the file). It is not imported anywhere else in the backend — safe to delete after migration. - **ErrorCode drift already exists before this issue is even implemented.** Backend has `THUMBNAIL_BACKFILL_ALREADY_RUNNING` and `NOTIFICATION_NOT_FOUND` that are absent from `frontend/src/lib/shared/errors.ts`. Frontend has `MISSING_CREDENTIALS` that is absent from the backend enum. This pre-existing gap should be fixed in the same PR — it is exactly the kind of hygiene this refactor motivates. - The `ResponseStatusException` handler currently uses `ex.getReason()` as the detail message. `getReason()` is nullable in Spring 6+; a null reason passed to `ProblemDetail.forStatusAndDetail()` will produce `null` in the detail field. Add a null guard. - The issue correctly flags the `ResponseStatusException` handler as potentially removable. In Spring Boot 4, the framework's `ResponseEntityExceptionHandler` already handles `ResponseStatusException` and produces a `ProblemDetail` — but only if the handler class extends it. Since `GlobalExceptionHandler` does not extend `ResponseEntityExceptionHandler`, the explicit handler must remain unless we change the class hierarchy. **Recommendation: do not extend `ResponseEntityExceptionHandler`** — doing so implicitly inherits 20+ exception mappings that may conflict with the existing custom handlers. Keep the explicit handler, just switch it to `ProblemDetail`. - No Flyway migration, no schema change, no new Spring service — doc update requirements from the checklist are minimal. Only `CLAUDE.md` error-handling section needs a note about the new response shape; no diagram changes required. ### Recommendations - Use `@ControllerAdvice` + `ResponseBodyAdvice` or keep `@RestControllerAdvice` as-is — no class hierarchy change needed. - Set the `type` URI to an enum-keyed path: `URI.create("https://errors.familienarchiv.app/" + code.name())`. Even if the domain never resolves, RFC 9457 allows it to be a non-dereferenceable identifier. - Null-guard `ex.getReason()` in the `ResponseStatusException` handler: `pd.setDetail(ex.getReason() != null ? ex.getReason() : ex.getMessage())`. - Fix the pre-existing `ErrorCode` drift in the same PR: add `THUMBNAIL_BACKFILL_ALREADY_RUNNING` and `NOTIFICATION_NOT_FOUND` to `errors.ts`, remove `MISSING_CREDENTIALS` from the frontend type union (or add it to the backend enum if it is genuinely used). - Write an ADR entry — not a standalone ADR file, but a one-paragraph note in `docs/ARCHITECTURE.md` under a new "Error response format" section — capturing why RFC 9457 was chosen and what the `code` extension property preserves. ### Open Decisions - **`violations` array format for validation errors**: The issue proposes `[{field, message}]`. For `ConstraintViolationException`, the property path includes the method name prefix (e.g., `createPerson.dto.lastName`). Decide whether to strip the prefix before exposing it in the `violations` array or expose the raw path. Raw is simpler; stripped is more consumer-friendly. Pick one and specify it in the issue before implementation starts.

marcel commented

2026-05-07 17:48:19 +02:00

👨‍💻 Felix Brandt — Fullstack Developer

Observations

The migration is mechanical but has one non-trivial piece: the frontend error extractor currently uses two different patterns across the codebase. About 20 files use (result.error as unknown as { code?: string })?.code (the typed client path); a smaller set use parseBackendError(res) with raw fetch (the file upload and invite paths). Both read body.code — that field is preserved in the new {type, title, status, detail, code} shape, so both patterns continue to work after migration without any frontend changes. This is the correct call in the issue.
The handleResponseStatus handler on line 59 passes ErrorCode.VALIDATION_ERROR as the code for all ResponseStatusException instances. This is wrong for non-validation uses — e.g., a ResponseStatusException(HttpStatus.FORBIDDEN, ...) would emit VALIDATION_ERROR as the code. After migration to ProblemDetail this should be corrected: use a sentinel ErrorCode.INTERNAL_ERROR or derive it from the status code.
The current MethodArgumentNotValidException and ConstraintViolationException handlers concatenate field errors into a single comma-joined string. The issue proposes moving those to a structured violations array, which is the right call. The concatenated string is currently the message field that getErrorMessage() receives — but getErrorMessage() ignores the message and maps only on code, so consumers do not rely on this string. Safe to restructure.
The ErrorResponse inner record is self-contained on line 71 — no external import. Delete it and the compiler confirms all references are gone.
handleGeneric already guards against leak ("An unexpected error occurred") — preserve exactly that behaviour. Do not change the detail string.
There is no existing GlobalExceptionHandlerTest. The issue lists one as a critical file to create. This is the first test coverage for the handler — good motivation for TDD here.

Recommendations

Write the failing test first. Suggested test class: @WebMvcTest slice loading a minimal controller that throws each exception type, asserting Content-Type: application/problem+json, status code, and the presence of code in the body. One test per exception type.
Fix the ResponseStatusException handler's hardcoded ErrorCode.VALIDATION_ERROR: replace with a status-to-code mapping or use INTERNAL_ERROR as fallback. A simple helper errorCodeForStatus(HttpStatus) returning FORBIDDEN for 403, UNAUTHORIZED for 401, etc., is worth the 10 lines.
Keep handleGeneric message as-is. The detail field in ProblemDetail maps 1:1 to what was previously message — no information added or removed.
After the backend change, run npm run generate:api. The OpenAPI spec currently reflects ErrorResponse {code, message} as an error response shape. After migration the spec will reflect ProblemDetail. Confirm the frontend typed client still compiles — the result.error as unknown cast is already untyped so it will not break, but the generated types should be clean.
Extract the violations mapping into a private helper method to keep each @ExceptionHandler body under 10 lines: private List<Map<String,String>> toViolations(BindingResult result).

Open Decisions

(none — the implementation path is clear)

## 👨‍💻 Felix Brandt — Fullstack Developer ### Observations - The migration is mechanical but has one non-trivial piece: the frontend error extractor currently uses **two different patterns** across the codebase. About 20 files use `(result.error as unknown as { code?: string })?.code` (the typed client path); a smaller set use `parseBackendError(res)` with raw `fetch` (the file upload and invite paths). Both read `body.code` — that field is preserved in the new `{type, title, status, detail, code}` shape, so both patterns continue to work after migration without any frontend changes. This is the correct call in the issue. - The `handleResponseStatus` handler on line 59 passes `ErrorCode.VALIDATION_ERROR` as the code for all `ResponseStatusException` instances. This is wrong for non-validation uses — e.g., a `ResponseStatusException(HttpStatus.FORBIDDEN, ...)` would emit `VALIDATION_ERROR` as the code. After migration to `ProblemDetail` this should be corrected: use a sentinel `ErrorCode.INTERNAL_ERROR` or derive it from the status code. - The current `MethodArgumentNotValidException` and `ConstraintViolationException` handlers concatenate field errors into a single comma-joined string. The issue proposes moving those to a structured `violations` array, which is the right call. The concatenated string is currently the `message` field that `getErrorMessage()` receives — but `getErrorMessage()` ignores the message and maps only on `code`, so consumers do not rely on this string. Safe to restructure. - The `ErrorResponse` inner record is self-contained on line 71 — no external import. Delete it and the compiler confirms all references are gone. - `handleGeneric` already guards against leak (`"An unexpected error occurred"`) — preserve exactly that behaviour. Do not change the detail string. - There is no existing `GlobalExceptionHandlerTest`. The issue lists one as a critical file to create. This is the first test coverage for the handler — good motivation for TDD here. ### Recommendations - **Write the failing test first.** Suggested test class: `@WebMvcTest` slice loading a minimal controller that throws each exception type, asserting `Content-Type: application/problem+json`, status code, and the presence of `code` in the body. One test per exception type. - Fix the `ResponseStatusException` handler's hardcoded `ErrorCode.VALIDATION_ERROR`: replace with a status-to-code mapping or use `INTERNAL_ERROR` as fallback. A simple helper `errorCodeForStatus(HttpStatus)` returning `FORBIDDEN` for 403, `UNAUTHORIZED` for 401, etc., is worth the 10 lines. - Keep `handleGeneric` message as-is. The detail field in ProblemDetail maps 1:1 to what was previously `message` — no information added or removed. - After the backend change, run `npm run generate:api`. The OpenAPI spec currently reflects `ErrorResponse {code, message}` as an error response shape. After migration the spec will reflect `ProblemDetail`. Confirm the frontend typed client still compiles — the `result.error as unknown` cast is already untyped so it will not break, but the generated types should be clean. - Extract the `violations` mapping into a private helper method to keep each `@ExceptionHandler` body under 10 lines: `private List<Map<String,String>> toViolations(BindingResult result)`. ### Open Decisions _(none — the implementation path is clear)_

marcel commented

2026-05-07 17:48:27 +02:00

🔧 Tobias Wendt — DevOps & Platform Engineer

Observations

This is a pure application-layer change. No Docker Compose modification, no new infrastructure, no CI pipeline impact. From a platform perspective it's low risk.
The Content-Type header change from application/json to application/problem+json is the only observable difference at the HTTP layer. Caddy does not inspect or filter response bodies, so the reverse proxy config requires no update.
If there are any external health-check scripts or monitoring rules that assert on the error response body shape (e.g., Uptime Kuma checks or custom smoke test assertions), those would need updating. Worth checking — though for a family archive this is unlikely to be in place.
The OpenAPI spec at /v3/api-docs (dev profile only) will reflect the new error shape after generate:api. If any CI job caches a snapshot of the spec, clear it. If not — no action needed.
There are no environment variables or secrets touched by this change.

Recommendations

After merge, run the post-deploy smoke test (or trigger a manual check) against the health endpoint and one error path to confirm application/problem+json is returned in the real environment.
If the team ever adds API contract testing (e.g., Pact or a Dredd check against the OpenAPI spec), this migration makes those tests more meaningful — the RFC 9457 shape is widely supported by contract-testing tools. Not a blocking concern for this PR, but worth noting in the project roadmap.
No Compose file changes needed. No infra cost change. Ship it.

Open Decisions

(none)

## 🔧 Tobias Wendt — DevOps & Platform Engineer ### Observations - This is a pure application-layer change. No Docker Compose modification, no new infrastructure, no CI pipeline impact. From a platform perspective it's low risk. - The `Content-Type` header change from `application/json` to `application/problem+json` is the only observable difference at the HTTP layer. Caddy does not inspect or filter response bodies, so the reverse proxy config requires no update. - If there are any external health-check scripts or monitoring rules that assert on the error response body shape (e.g., Uptime Kuma checks or custom smoke test assertions), those would need updating. Worth checking — though for a family archive this is unlikely to be in place. - The OpenAPI spec at `/v3/api-docs` (dev profile only) will reflect the new error shape after `generate:api`. If any CI job caches a snapshot of the spec, clear it. If not — no action needed. - There are no environment variables or secrets touched by this change. ### Recommendations - After merge, run the post-deploy smoke test (or trigger a manual check) against the health endpoint and one error path to confirm `application/problem+json` is returned in the real environment. - If the team ever adds API contract testing (e.g., Pact or a Dredd check against the OpenAPI spec), this migration makes those tests more meaningful — the RFC 9457 shape is widely supported by contract-testing tools. Not a blocking concern for this PR, but worth noting in the project roadmap. - No Compose file changes needed. No infra cost change. Ship it. ### Open Decisions _(none)_

marcel commented

2026-05-07 17:48:43 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue is well-specified for a refactor ticket: it has a clear context, a concrete approach with code examples, a verification section, and numbered acceptance criteria. It satisfies the Definition of Ready for this project's workflow.
Acceptance criterion coverage is complete for the happy path. The five ACs map cleanly to the five observable behaviours. No ambiguous language.
One gap: the AC for "Frontend getErrorMessage still produces translated messages from code" is phrased as an outcome but has no corresponding verification step in the "Verification" section. Section 3 (Frontend smoke) is the implicit test, but it is described as a manual check. For a solo developer this is fine, but it could be strengthened by specifying it as an automated test assertion.
The issue references frontend/src/lib/shared/api.server.ts as a file to change ("change the extractor to read result.error.code"). Inspection of the actual codebase shows that api.server.ts is only the client factory; the error extraction happens in individual +page.server.ts files using (result.error as unknown as { code?: string })?.code. The issue's claim that "both work" is correct — the code field is present in both shapes — so no extractor change is actually required. The issue slightly overstates the frontend scope; the implementation should not change those extraction sites unnecessarily.
Pre-existing ErrorCode drift (see Markus's comment) is a requirements gap that should be captured as an acceptance criterion if it is to be fixed in this PR, or as a separate issue if deferred.

Recommendations

Add a sixth AC: "The ErrorCode type in frontend/src/lib/shared/errors.ts is in sync with the backend ErrorCode enum (no missing or extra values)." This closes the pre-existing drift and keeps the PR self-contained.
Remove the mention of changing api.server.ts from the issue body to avoid confusion — the typed client pattern does not need to change.
The Effort estimate of S (half a day) is accurate given the mechanical nature of the change. Agree with the risk assessment of "None operationally."

Open Decisions

(none — the spec is ready to implement as-is with the minor clarification above)

## 📋 Elicit — Requirements Engineer ### Observations - The issue is well-specified for a refactor ticket: it has a clear context, a concrete approach with code examples, a verification section, and numbered acceptance criteria. It satisfies the Definition of Ready for this project's workflow. - **Acceptance criterion coverage is complete** for the happy path. The five ACs map cleanly to the five observable behaviours. No ambiguous language. - One gap: the AC for "Frontend `getErrorMessage` still produces translated messages from `code`" is phrased as an outcome but has no corresponding verification step in the "Verification" section. Section 3 (Frontend smoke) is the implicit test, but it is described as a manual check. For a solo developer this is fine, but it could be strengthened by specifying it as an automated test assertion. - The issue references `frontend/src/lib/shared/api.server.ts` as a file to change ("change the extractor to read `result.error.code`"). Inspection of the actual codebase shows that `api.server.ts` is only the client factory; the error extraction happens in individual `+page.server.ts` files using `(result.error as unknown as { code?: string })?.code`. The issue's claim that "both work" is correct — the `code` field is present in both shapes — so no extractor change is actually required. The issue slightly overstates the frontend scope; the implementation should not change those extraction sites unnecessarily. - **Pre-existing `ErrorCode` drift** (see Markus's comment) is a requirements gap that should be captured as an acceptance criterion if it is to be fixed in this PR, or as a separate issue if deferred. ### Recommendations - Add a sixth AC: "The `ErrorCode` type in `frontend/src/lib/shared/errors.ts` is in sync with the backend `ErrorCode` enum (no missing or extra values)." This closes the pre-existing drift and keeps the PR self-contained. - Remove the mention of changing `api.server.ts` from the issue body to avoid confusion — the typed client pattern does not need to change. - The Effort estimate of S (half a day) is accurate given the mechanical nature of the change. Agree with the risk assessment of "None operationally." ### Open Decisions _(none — the spec is ready to implement as-is with the minor clarification above)_

marcel commented

2026-05-07 17:49:00 +02:00

🔒 Nora "NullX" Steiner — Security Engineer

Observations

The existing handler correctly suppresses stack traces and exception class names in error responses. The generic catch-all returns only "An unexpected error occurred" — this is the right baseline. The audit conclusion ("prevents stack-trace leaks") is confirmed by reading the code.
Information disclosure via type URI: the issue proposes https://errors.familienarchiv.app/DOCUMENT_NOT_FOUND as the type URI. This exposes the ErrorCode enum value in the response body via the URI, which is intentional and already exposed via the code property. No new information is leaked. However, if the type URI ever resolves (i.e., the domain becomes real), ensure the endpoint returns only documentation — not anything that reveals internal structure beyond the enum name.
detail field in ProblemDetail for DomainException: the detail will be ex.getMessage(), which is the developer message from DomainException's constructor. Audit a sample: "Document not found: " + id — this exposes a UUID to the API consumer. This was already the behaviour with the previous message field, and is acceptable for this application (family archive, no sensitive UUIDs), but note it for awareness.
violations array for validation errors: exposing field names is standard RFC 9457 practice and already effectively exposed by the current comma-joined message string. No regression here.
ResponseStatusException with null reason: ex.getReason() can return null. If this null flows into ProblemDetail.setDetail(null), the serialised JSON will have "detail": null. Not a security issue, but a subtle functional regression — the previous handler serialised null as the message field in ErrorResponse without issue, but consumers expecting a string detail field will receive null. Null-guard as Markus recommends.
The handleGeneric catch-all is the most security-sensitive handler — it must never expose the original exception. The proposed migration preserves the safe static string. Confirmed safe.

Recommendations

In handleGeneric, explicitly do not set pd.setDetail(ex.getMessage()) or pd.setTitle(ex.getClass().getSimpleName()) — both would be information disclosure. Use only the static "An unexpected error occurred" string as the detail.
Add a @WebMvcTest assertion that hitting an endpoint that throws a raw RuntimeException returns a body that does not contain the exception message or class name. This is a permanent regression guard for the most sensitive handler.
Before deleting ErrorResponse.java (actually the inner record), confirm with grep -r ErrorResponse backend/src that it has no external references. From my reading it is self-contained, but confirm mechanically.
The type URI approach is sound from a security standpoint — it follows the RFC and avoids leaking internal system details beyond the enum name which is already public API.

Open Decisions

(none — no genuine security tradeoffs here; all recommendations above are unambiguous)

## 🔒 Nora "NullX" Steiner — Security Engineer ### Observations - The existing handler correctly suppresses stack traces and exception class names in error responses. The generic catch-all returns only `"An unexpected error occurred"` — this is the right baseline. The audit conclusion ("prevents stack-trace leaks") is confirmed by reading the code. - **Information disclosure via `type` URI**: the issue proposes `https://errors.familienarchiv.app/DOCUMENT_NOT_FOUND` as the `type` URI. This exposes the `ErrorCode` enum value in the response body via the URI, which is intentional and already exposed via the `code` property. No new information is leaked. However, if the `type` URI ever resolves (i.e., the domain becomes real), ensure the endpoint returns only documentation — not anything that reveals internal structure beyond the enum name. - **`detail` field in ProblemDetail** for `DomainException`: the `detail` will be `ex.getMessage()`, which is the developer message from `DomainException`'s constructor. Audit a sample: `"Document not found: " + id` — this exposes a UUID to the API consumer. This was already the behaviour with the previous `message` field, and is acceptable for this application (family archive, no sensitive UUIDs), but note it for awareness. - **`violations` array for validation errors**: exposing field names is standard RFC 9457 practice and already effectively exposed by the current comma-joined `message` string. No regression here. - **`ResponseStatusException` with null reason**: `ex.getReason()` can return null. If this null flows into `ProblemDetail.setDetail(null)`, the serialised JSON will have `"detail": null`. Not a security issue, but a subtle functional regression — the previous handler serialised `null` as the message field in `ErrorResponse` without issue, but consumers expecting a string detail field will receive null. Null-guard as Markus recommends. - The `handleGeneric` catch-all is the most security-sensitive handler — it must never expose the original exception. The proposed migration preserves the safe static string. Confirmed safe. ### Recommendations - In `handleGeneric`, explicitly **do not** set `pd.setDetail(ex.getMessage())` or `pd.setTitle(ex.getClass().getSimpleName())` — both would be information disclosure. Use only the static `"An unexpected error occurred"` string as the detail. - Add a `@WebMvcTest` assertion that hitting an endpoint that throws a raw `RuntimeException` returns a body that does **not** contain the exception message or class name. This is a permanent regression guard for the most sensitive handler. - Before deleting `ErrorResponse.java` (actually the inner record), confirm with `grep -r ErrorResponse backend/src` that it has no external references. From my reading it is self-contained, but confirm mechanically. - The `type` URI approach is sound from a security standpoint — it follows the RFC and avoids leaking internal system details beyond the enum name which is already public API. ### Open Decisions _(none — no genuine security tradeoffs here; all recommendations above are unambiguous)_

marcel commented

2026-05-07 17:49:26 +02:00

🧪 Sara Holt — QA Engineer

Observations

There is no existing GlobalExceptionHandlerTest in the codebase. This is the first time the handler will have direct test coverage. The issue correctly lists it as a critical file to create — this is the most important deliverable of the PR alongside the handler itself.
The issue lists four verification steps but they are described as manual checks. At minimum, steps 1 and 2 (body shape assertions) must be automated tests that run in CI.
The correct test slice for this handler is @WebMvcTest with a minimal test controller that throws each exception type. This avoids loading the full Spring context, MinIO, PostgreSQL, etc. Five exception types × two assertions each (status code + body shape) = ~10 test methods. This is a 10-second test class.
The Content-Type: application/problem+json assertion is the most important new test. It is the primary observable change and the thing most likely to regress if a future change reverts the handler. One assertion per handler method: andExpect(content().contentType(MediaType.APPLICATION_PROBLEM_JSON)).
The validation handlers (MethodArgumentNotValidException, ConstraintViolationException) must be tested for the presence of the violations array in the response body. Use jsonPath("$.violations").isArray() and jsonPath("$.violations[0].field").exists().
Coverage gate: The backend currently enforces 88% branch coverage via JaCoCo. Adding GlobalExceptionHandlerTest without the handler test class would lower coverage (new lines without tests). The PR must include the test class or the coverage gate will fail.

Recommendations

Create GlobalExceptionHandlerTest as a @WebMvcTest slice. Test method naming convention: should_return_problemDetail_with_code_when_domainException_is_thrown(), should_include_violations_array_when_validation_fails(), should_not_include_stack_trace_when_generic_exception_is_thrown().
Add one specific @Test per @ExceptionHandler method — do not combine them into one mega-test.
For the generic exception handler test, assert both the absence of a stack trace (use jsonPath("$.detail").value("An unexpected error occurred")) and the presence of code (jsonPath("$.code").value("INTERNAL_ERROR")).
Verify Content-Type header in every test, not just a representative sample: andExpect(content().contentType(MediaType.APPLICATION_PROBLEM_JSON)).
For the frontend smoke check (Verification step 3), consider promoting it from "manual" to an E2E Playwright test that triggers a 404 (visit a non-existent document) and asserts the UI displays a localised error message rather than raw JSON.

Open Decisions

Test fixture for ConstraintViolationException: triggering this in a @WebMvcTest requires a @Validated controller with a bean validation constraint. Decide whether to use a real controller method with @Valid on a simple DTO, or to mock the exception in a @MockBean service. The former is more realistic; the latter is simpler to set up. Both are valid — pick one and be consistent with how other controller tests in the project are structured.

## 🧪 Sara Holt — QA Engineer ### Observations - **There is no existing `GlobalExceptionHandlerTest`** in the codebase. This is the first time the handler will have direct test coverage. The issue correctly lists it as a critical file to create — this is the most important deliverable of the PR alongside the handler itself. - The issue lists four verification steps but they are described as manual checks. At minimum, steps 1 and 2 (body shape assertions) must be automated tests that run in CI. - The correct test slice for this handler is `@WebMvcTest` with a minimal test controller that throws each exception type. This avoids loading the full Spring context, MinIO, PostgreSQL, etc. Five exception types × two assertions each (status code + body shape) = ~10 test methods. This is a 10-second test class. - **The `Content-Type: application/problem+json` assertion is the most important new test.** It is the primary observable change and the thing most likely to regress if a future change reverts the handler. One assertion per handler method: `andExpect(content().contentType(MediaType.APPLICATION_PROBLEM_JSON))`. - The validation handlers (`MethodArgumentNotValidException`, `ConstraintViolationException`) must be tested for the presence of the `violations` array in the response body. Use `jsonPath("$.violations").isArray()` and `jsonPath("$.violations[0].field").exists()`. - **Coverage gate**: The backend currently enforces 88% branch coverage via JaCoCo. Adding `GlobalExceptionHandlerTest` without the handler test class would lower coverage (new lines without tests). The PR must include the test class or the coverage gate will fail. ### Recommendations - Create `GlobalExceptionHandlerTest` as a `@WebMvcTest` slice. Test method naming convention: `should_return_problemDetail_with_code_when_domainException_is_thrown()`, `should_include_violations_array_when_validation_fails()`, `should_not_include_stack_trace_when_generic_exception_is_thrown()`. - Add one specific `@Test` per `@ExceptionHandler` method — do not combine them into one mega-test. - For the generic exception handler test, assert both the absence of a stack trace (use `jsonPath("$.detail").value("An unexpected error occurred")`) and the presence of `code` (`jsonPath("$.code").value("INTERNAL_ERROR")`). - Verify `Content-Type` header in every test, not just a representative sample: `andExpect(content().contentType(MediaType.APPLICATION_PROBLEM_JSON))`. - For the frontend smoke check (Verification step 3), consider promoting it from "manual" to an E2E Playwright test that triggers a 404 (visit a non-existent document) and asserts the UI displays a localised error message rather than raw JSON. ### Open Decisions - **Test fixture for `ConstraintViolationException`**: triggering this in a `@WebMvcTest` requires a `@Validated` controller with a bean validation constraint. Decide whether to use a real controller method with `@Valid` on a simple DTO, or to mock the exception in a `@MockBean` service. The former is more realistic; the latter is simpler to set up. Both are valid — pick one and be consistent with how other controller tests in the project are structured.

marcel commented

2026-05-07 17:49:44 +02:00

🎨 Leonie Voss — UI/UX & Accessibility

Observations

This is a pure backend/API change. It has no direct impact on Tailwind classes, component markup, or visual design. No UI review concerns for the migration itself.
The one user-visible surface is error message display — and the issue explicitly verifies that getErrorMessage(code) continues to work and i18n resolution is preserved. The code field is present in both old and new shapes, so Paraglide-translated messages will continue to render correctly. Users will see no change.
The existing errors.ts has OCR_TRAINING_CONFLICT mapped to m.error_internal_error() (line 132 of the file) rather than a dedicated message. This is a pre-existing gap unrelated to this issue, but worth noting: if a user triggers an OCR training conflict, they see a generic "internal error" message. Not a blocker for this PR.
The structured violations array in validation error responses is backend-only at this stage. If in future a form wants to display per-field server-side validation errors (rather than the current generic "validation error" toast), the structured violations array makes that significantly easier. This is a design enabler, not a current requirement.

Recommendations

No UI changes needed for this PR.
When the structured violations array is eventually surfaced in form components, use the field value to match to a visible <label> and display the error adjacent to the relevant input — consistent with the existing inline validation pattern. Plan for this when a future issue adds server-side field-level error display.

Open Decisions

(none — no UX tradeoffs in a backend wire format change)

## 🎨 Leonie Voss — UI/UX & Accessibility ### Observations - This is a pure backend/API change. It has no direct impact on Tailwind classes, component markup, or visual design. No UI review concerns for the migration itself. - **The one user-visible surface is error message display** — and the issue explicitly verifies that `getErrorMessage(code)` continues to work and i18n resolution is preserved. The `code` field is present in both old and new shapes, so Paraglide-translated messages will continue to render correctly. Users will see no change. - The existing `errors.ts` has `OCR_TRAINING_CONFLICT` mapped to `m.error_internal_error()` (line 132 of the file) rather than a dedicated message. This is a pre-existing gap unrelated to this issue, but worth noting: if a user triggers an OCR training conflict, they see a generic "internal error" message. Not a blocker for this PR. - The structured `violations` array in validation error responses is backend-only at this stage. If in future a form wants to display per-field server-side validation errors (rather than the current generic "validation error" toast), the structured `violations` array makes that significantly easier. This is a design enabler, not a current requirement. ### Recommendations - No UI changes needed for this PR. - When the structured `violations` array is eventually surfaced in form components, use the `field` value to match to a visible `<label>` and display the error adjacent to the relevant input — consistent with the existing inline validation pattern. Plan for this when a future issue adds server-side field-level error display. ### Open Decisions _(none — no UX tradeoffs in a backend wire format change)_

marcel commented

2026-05-07 17:50:18 +02:00

Decision Queue

Two open decisions were raised across the review. Both need to be resolved before implementation starts.

D-1: `violations` field path format for `ConstraintViolationException` (Markus + Sara)

Context: ConstraintViolationException.getConstraintViolations() gives property paths that include the method name prefix — e.g., createPerson.dto.lastName instead of just lastName. The violations array will expose these paths to consumers.

Options:

A — Raw path (simpler): emit createPerson.dto.lastName as-is. No transformation code. Clients receive the full path.
B — Stripped path (cleaner for consumers): strip the method prefix, emit only lastName. Requires a small string transform (path.toString().replaceAll("^[^.]+\\.", "")).

Recommendation: B (stripped). The method name is an implementation detail with no value to API consumers. A 10-character regex is a cheap transformation.

Decision needed by: before implementation starts.

D-2: Test fixture strategy for `ConstraintViolationException` in `@WebMvcTest` (Sara)

Context: ConstraintViolationException is thrown when a @Validated controller parameter fails a bean validation constraint (e.g., @NotBlank). Triggering it in a @WebMvcTest slice requires either (A) a real @Validated DTO on a test controller endpoint, or (B) mocking the exception from a @MockBean service.

Options:

A — Real @Valid DTO on a stub endpoint: more realistic, exercises the Spring validation pipeline end-to-end, but slightly more setup.
B — Mock the exception: simpler fixture, but only tests the handler's response logic, not the full validation→handler path.

Recommendation: A (real @Valid DTO). The intent of the test is to confirm the full path — Spring validation firing, exception propagating, handler formatting the violations array. Mocking bypasses the validation step and gives less confidence.

Decision needed by: before writing GlobalExceptionHandlerTest.

## Decision Queue Two open decisions were raised across the review. Both need to be resolved before implementation starts. --- ### D-1: `violations` field path format for `ConstraintViolationException` (Markus + Sara) **Context:** `ConstraintViolationException.getConstraintViolations()` gives property paths that include the method name prefix — e.g., `createPerson.dto.lastName` instead of just `lastName`. The `violations` array will expose these paths to consumers. **Options:** - **A — Raw path** (simpler): emit `createPerson.dto.lastName` as-is. No transformation code. Clients receive the full path. - **B — Stripped path** (cleaner for consumers): strip the method prefix, emit only `lastName`. Requires a small string transform (`path.toString().replaceAll("^[^.]+\\.", "")`). **Recommendation:** B (stripped). The method name is an implementation detail with no value to API consumers. A 10-character regex is a cheap transformation. **Decision needed by:** before implementation starts. --- ### D-2: Test fixture strategy for `ConstraintViolationException` in `@WebMvcTest` (Sara) **Context:** `ConstraintViolationException` is thrown when a `@Validated` controller parameter fails a bean validation constraint (e.g., `@NotBlank`). Triggering it in a `@WebMvcTest` slice requires either (A) a real `@Validated` DTO on a test controller endpoint, or (B) mocking the exception from a `@MockBean` service. **Options:** - **A — Real `@Valid` DTO on a stub endpoint**: more realistic, exercises the Spring validation pipeline end-to-end, but slightly more setup. - **B — Mock the exception**: simpler fixture, but only tests the handler's response logic, not the full validation→handler path. **Recommendation:** A (real `@Valid` DTO). The intent of the test is to confirm the full path — Spring validation firing, exception propagating, handler formatting the `violations` array. Mocking bypasses the validation step and gives less confidence. **Decision needed by:** before writing `GlobalExceptionHandlerTest`.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#466

refactor(api): migrate GlobalExceptionHandler to RFC 9457 ProblemDetail #466

Context

Approach

Pattern

Validation handlers

ResponseStatusException handler

Generic Exception handler

Critical files

Verification

Acceptance criteria

Effort

Risk if not addressed

🏛️ Markus Keller — Application Architect

Observations

Recommendations

Open Decisions

👨‍💻 Felix Brandt — Fullstack Developer

Observations

Recommendations

Open Decisions

🔧 Tobias Wendt — DevOps & Platform Engineer

Observations

Recommendations

Open Decisions

📋 Elicit — Requirements Engineer

Observations

Recommendations

Open Decisions

🔒 Nora "NullX" Steiner — Security Engineer

Observations

Recommendations

Open Decisions

🧪 Sara Holt — QA Engineer

Observations

Recommendations

Open Decisions

🎨 Leonie Voss — UI/UX & Accessibility

Observations

Recommendations

Open Decisions

Decision Queue

D-1: violations field path format for ConstraintViolationException (Markus + Sara)

D-2: Test fixture strategy for ConstraintViolationException in @WebMvcTest (Sara)

`ResponseStatusException` handler

D-1: `violations` field path format for `ConstraintViolationException` (Markus + Sara)

D-2: Test fixture strategy for `ConstraintViolationException` in `@WebMvcTest` (Sara)