feat!: answer every error with RFC 9457 problem details - #184
Merged
Conversation
Failures were encoded in the response type: each domain carried its own XxxErrorDTO next to its success DTO, and every controller tested that union with instanceof before picking a status. Ten near-identical error records and 27 such checks later, the API still spoke three different error shapes, and ExceptionHandlerAdvice turned every unhandled throwable into a 404 carrying exception.getMessage() - which for MariaDB and Hibernate means table names, column names and SQL fragments on the wire (CWE-209). Errors are now raised, not returned. ApiException names an ErrorCode, which carries the HTTP status, the title and the problem type; ApiExceptionHandler turns it into the response. ProblemErrorResponseProcessor replaces Micronaut's stock processor - the hook every built-in handler routes its body through - so validation failures, unbindable path variables, malformed JSON, 405 and 415 come out in the same shape as the rest. Two rules keep it from leaking. Details for 5xx are a fixed constant, never the exception message. Details for 4xx are written at the throw site from data the caller already sent us. Where the honest reason differs from what the caller may learn - a cross-project access - it goes to the log as internalDetail and is never serialized. What the frontend gains: - a stable machine-readable `code` to branch and localize on, reaching the Dart client as an enum, instead of matching on English prose - `errors[]` naming the rejected fields on a validation failure, so a form can mark the matching input; that shape was previously not modelled at all - honest statuses: 400 for an update without an id (a client bug, not a missing resource), 409 for a uniqueness violation, 500 for a server fault - a `traceId` on every error to quote in support The service layer got smaller on the way: CrudService loses a type parameter, AbstractCrudService loses the errorMapper from all four constructors, deleteAll returns void where it only ever returned an empty list, and the sub-resource lookups in ItemServiceImpl and FontServiceImpl no longer scan the whole table. BREAKING CHANGE: error responses change shape and status. All XxxErrorDTO records and the ErrorResponse interface are gone, replaced by a single ProblemDetail served as application/problem+json. Unhandled server faults now answer 500 instead of 404, an update without an id answers 400, and a uniqueness violation answers 409. CrudService.deleteAll returns void and its endpoints answer 204 No Content instead of 200 with an empty array. Service methods raise ApiException instead of returning an error DTO, and the CrudService type parameters drop from five to four. Consumers of the generated Dart client must be regenerated against the new spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TwxDNT3VW1WKvieWA1NUdt
Contributor
Test results 48 files 48 suites 5s ⏱️ Results for commit 4f548b9. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Failures were encoded in the response type: each domain carried its own
XxxErrorDTOnext to its success DTO, and every controller tested that union withinstanceofbefore picking a status. Ten near-identical error records and 27 such checks later, the API still spoke three different error shapes:instanceofin the controller{"errorMessage": "..."}{"errorMessage": "..."}@Validatedfailure (Micronaut'sConstraintExceptionHandler){"message":"...","_embedded":{...},"_links":{...}}The third shape was never modelled in the generated Dart client, so the frontend could not render form validation at all. And
ExceptionHandlerAdviceanswered 404 for every unhandled throwable while echoingexception.getMessage()— which for MariaDB and Hibernate means table names, column names and SQL fragments on the wire (CWE-209).What changes
Errors are now raised, not returned.
ApiExceptionnames anErrorCode, which carries the HTTP status, the title and the problem type;ApiExceptionHandlerturns it into the response.ProblemErrorResponseProcessorreplaces Micronaut's stockErrorResponseProcessor— the hook every built-in handler routes its body through — so validation failures, unbindable path variables, malformed JSON, 405 and 415 come out in the same shape as everything else. The stock one is annotated@Requires(missingBeans = ErrorResponseProcessor.class), so declaring ours is enough to take over.{ "type": "https://vulpes.onelitefeather.net/errors/resource-not-found", "title": "Resource not found", "status": 404, "detail": "Attribute not found.", "instance": "/project/6f1c.../attribute/update", "code": "RESOURCE_NOT_FOUND", "traceId": "4bf92f3577b34da6a3ce929d0e0e4736", "errors": [] }Two rules keep it from leaking: details for 5xx are a fixed constant, never the exception message; details for 4xx are written at the throw site from data the caller already sent us. Where the honest reason differs from what the caller may learn — a cross-project access — it goes to the log as
internalDetailand is never serialized.What the frontend gains
codeto branch and localize on, reaching the Dart client as an enum, instead of matching on English proseerrors[]naming the rejected fields on a validation failure ({field, code, message},fieldbeing the request property path such asdisplayName), so a form can mark the matching inputtraceIdon every error to quote in supportStandards this follows
statusmust mirror the response statusUniqueness violations are detected by walking the cause chain for SQLSTATE class 23, which is standard SQL, so it works on MariaDB and PostgreSQL without reading a vendor error message.
micronaut-problem-json5.0.0 exists and fits the platform, but was not used: it pulls in Zalando'sproblem, whoseThrowableProblemdoes not generate a clean Dart model. A 25-line record givesvulpes-clienta first-classProblemDetailclass instead.Incidental cleanups in the same code
CrudServiceloses a type parameter (the success/error union collapses)AbstractCrudServiceloses theerrorMapperfrom all four constructorsdeleteAllreturnsvoidwhere it only ever returned an empty list; those endpoints answer 204ItemServiceImplandFontServiceImpluse the scoped repository finders instead offindAll().stream().filter(...)over the whole tableNet: -27
instanceofblocks, -10 error records, +6 small files.Types of changes
Breaking: all
XxxErrorDTOrecords and theErrorResponseinterface are gone, replaced by a singleProblemDetailserved asapplication/problem+json. Unhandled server faults answer 500 instead of 404, an update without an id answers 400, a uniqueness violation answers 409.CrudService.deleteAllreturnsvoidand its endpoints answer 204 instead of 200-with-[]. Service methods raiseApiExceptioninstead of returning an error DTO, and theCrudServicetype parameters drop from five to four. The Dart client must be regenerated against the new spec.Checklist
Tests added:
ErrorCodeTest(status/type mapping, the 4xx/5xx fallback split),ApiExceptionHandlerTest(status from the code, problem+json content type,internalDetailnever reaching the body),ProblemErrorResponseProcessorTest(a 5xx never echoing a SQL message, SQLSTATE 23 → 409, a self-referencing cause chain terminating, validation violations reported as client-facing field names via the real Hibernate Validator executable validator). Existing controller tests that asserted404on an error DTO now assert the raisedErrorCodeinstead. Docs: an "Error handling" section in the README covering the contract for clients and the no-leak rules for contributors.Further comments
Verification, and its limit. This session could not run
./gradlew build:repo.onelitefeather.devis blocked by the sandbox's network policy, sovulpes-modelis unresolvable, and Maven Central rate-limits (429) the plugin classpath. What was verified locally, on a JDK 25 matching the CI toolchain:javacover all ofsrc/mainandsrc/testagainst hand-written stubs of thevulpes-modelAPI, and the full unit suite — 106 tests, all green. Annotation processing (micronaut-serde, micronaut-openapi) and the OpenAPI/Dart generation are therefore unverified; CI on this PR is the first real run of those. I'm watching it and will push fixes for whatever it finds.Two pre-existing issues left alone, since fixing them would widen this PR beyond error handling:
FontController.updatecarries no@Validated, so the font update endpoint is unvalidated. It is the only mutating endpoint without a documented 400 for that reason.ItemServiceImpl.updateFlagById/updateEnchantmentByIdandFontServiceImpl.updateCharByFontIdset the parent and callupdate()without first checking the child belongs to that parent — a child id from another item can be reassigned. The delete paths do check. Worth a follow-up.One judgement call worth flagging: validation failures stay on 400 rather than moving to 422. RFC 9110 would allow 422, but Zalando explicitly discourages it, and the distinction the frontend actually needs is carried by
code: VALIDATION_FAILED— which costs nothing and does not fight the framework's default.🤖 Generated with Claude Code
https://claude.ai/code/session_01TwxDNT3VW1WKvieWA1NUdt
Generated by Claude Code