LEGLINK-887: Terminology FHIR endpoints return Problem Details on error - #1779
Merged
Conversation
FhirController answered every error with a bare string body and text/plain, so a rejected request carried no type, title, status or traceId. LEGLINK-886 corrected the status code for a request naming no value set, but the payload was still a plain string; this makes the body match the RFC 9457 format ConfigController has returned since LEGLINK-591. - Adds TerminologyProblem plus BadRequestProblem, NotFoundProblem and InternalServerErrorProblem helpers, and routes all 13 error returns across the six actions through them (7 BadRequest, 3 NotFound, 3 raw 500). - traceId comes from the existing AddTerminologyProblemDetails customization. - detail is prose, so the helper appends a terminating period. Exception messages stay fragments because they are also read from logs and asserted in unit tests. - 500 responses no longer place the exception message in the body; the customization substitutes a generic detail so internal state is not exposed to the caller. Testing: 73 unit tests pass in UnitTests.Terminology, one new covering LEGLINK-887's reported request (empty valueUri in the POST body). Verified against the local docker-compose stack that an empty valueUri in the body, an empty url query parameter, a markup-only display and an unknown ValueSet id all return application/problem+json carrying type, title, status, detail and a W3C traceId, and that the $validate-code success and failure payloads are unchanged.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DotNet/ServiceTests/UnitTests/Terminology/Controllers/FhirControllerTests.cs (1)
57-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover 404 and 500 Problem Details contracts.
AssertBadRequestProblemonly verifies 400 responses. The controller now also returnsNotFoundProblemandInternalServerErrorProblem. Add focused XUnit cases for both contracts. For 500, verify that the configured customization removes the raw exception detail without making an HTTP network call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DotNet/ServiceTests/UnitTests/Terminology/Controllers/FhirControllerTests.cs` around lines 57 - 75, Add focused xUnit tests alongside AssertBadRequestProblem for the controller’s NotFoundProblem and InternalServerErrorProblem responses, validating their RFC 9457 status, title, type, and detail contracts. For the 500 case, exercise the configured customization directly or through the existing test setup and assert the raw exception detail is removed, without making an HTTP network call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@DotNet/ServiceTests/UnitTests/Terminology/Controllers/FhirControllerTests.cs`:
- Around line 57-75: Add focused xUnit tests alongside AssertBadRequestProblem
for the controller’s NotFoundProblem and InternalServerErrorProblem responses,
validating their RFC 9457 status, title, type, and detail contracts. For the 500
case, exercise the configured customization directly or through the existing
test setup and assert the raw exception detail is removed, without making an
HTTP network call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 507f1eea-306e-4015-9515-50a0cba662ff
📒 Files selected for processing (2)
DotNet/ServiceTests/UnitTests/Terminology/Controllers/FhirControllerTests.csDotNet/Terminology/Controllers/FhirController.cs
Follow-up to review feedback on #1779: AssertBadRequestProblem was the only assertion helper, so NotFoundProblem and InternalServerErrorProblem shipped untested. - Generalises the helper to AssertProblem(result, status, title, type, detail) and keeps AssertBadRequestProblem as a wrapper, leaving the existing call sites unchanged. - Adds 404 coverage via GetValueSetById with an id the cache does not hold, and 500 coverage via a code group cached under the ValueSet type whose resource is a CodeSystem. - Exercises the configured CustomizeProblemDetails callback directly, with no host and no HTTP call, to assert a 5xx detail is replaced by the generic message and a traceId is added. A paired client-error test pins the scrubbing to 5xx so an over-broad change cannot silently erase the actionable 4xx detail this change exists to deliver. The controller-level 500 test asserts status, title and type only: ProblemDetailsFactory is null in unit tests, so ControllerBase.Problem builds a plain ProblemDetails and the scrubbing belongs to the customization test. Testing: 77 unit tests pass in UnitTests.Terminology, up from 73. Test project only; no production code changed.
MikeAtPinnacle
marked this pull request as ready for review
August 4, 2026 15:52
smailliwcs
approved these changes
Aug 6, 2026
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
2 participants
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.
🛠️ Description of Changes
Fixes LEGLINK-887.
FhirControlleranswered every error with a bare string body andtext/plain:No
type,title,statusortraceId— nothing a client can branch on, and nothing to correlate against a trace. LEGLINK-886 (#1777, merged) corrected the status code for a request naming no value set, but left the payload a plain string. This PR fixes the format, so the same request now returns:{ "type": "https://tools.ietf.org/html/rfc9110#section-15.5.1", "title": "Bad Request", "status": 400, "detail": "No id or url parameter specified.", "traceId": "00-af3d283aeb5f6bf14e01ac47f6bc2e6b-aea917848a69b991-01" }with
Content-Type: application/problem+json.This is not new infrastructure.
AddTerminologyProblemDetailswas already registered inProgram.cs, andConfigControllerhas returned RFC 9457 Problem Details since LEGLINK-591 —FhirControllerwas simply the straggler.traceIdcomes free from the existing customization, in W3C traceparent format.Changes:
TerminologyProblemplusBadRequestProblem/NotFoundProblem/InternalServerErrorProblemhelpers, and routes all 13 error returns across the six actions through them (7 ×BadRequest, 3 ×NotFound, 3 × rawStatusCode(500, ...)).detailis prose, so the helper appends a terminating period. Exception messages stay fragments because they are also read from logs and asserted in unit tests — this avoids rewriting every message string and its assertions.🧪 Testing Performed
Unit:
dotnet test DotNet/ServiceTests/ServiceTests.csproj --filter FullyQualifiedName~UnitTests.Terminology→ 73 passed, 0 failed.Manual against the local docker-compose stack (
link-terminologyon :8076, real terminology data — 98 value sets, 91 code systems):valueUriin POST body (887's reported request)400text/plainNo id or url parameter specified400problem+json, detailNo id or url parameter specified.urlquery parameterdisplay400text/plain400problem+json, detailInvalid value supplied for 'display'.GET ValueSet/does-not-exist404text/plain404problem+json, type#section-15.5.5, titleNot Found$validate-codewith an unknown code200 {result:false, message:...}$validate-codehappy path200 {result:true}Every 4xx response was confirmed to carry
type,title,status,detailand a W3C-formattraceIdunderContent-Type: application/problem+json.A Postman post-response script asserting the exact payload above (status, content type, all five members,
typeshape, traceparent format, and absence ofexception/stackTrace) passes against this build.🧑🔬 Unit Testing
📓 Documentation Updated
No documentation changes. No config keys were added, and the endpoints' success payloads are unchanged, so
docs/command-validatevaluesetcode.htmlanddocs/command-validatecodesystemcode.htmlremain accurate.Summary by CodeRabbit
valueUrivalues.