LEGLINK-887: Terminology FHIR endpoints return Problem Details on error (#1779)

* LEGLINK-887: Terminology FHIR endpoints return Problem Details on error

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.

* LEGLINK-887: Cover the NotFound and 500 Problem Details contracts

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.

Author: MikeAtPinnacle

DotNet/ServiceTests/UnitTests/Terminology/Controllers/FhirControllerTests.cs

@@ -1,10 +1,16 @@
 using Hl7.Fhir.Model;
+using LantanaGroup.Link.Terminology.Application.Extensions;
 using LantanaGroup.Link.Terminology.Application.Interfaces;
 using LantanaGroup.Link.Terminology.Application.Models;
 using LantanaGroup.Link.Terminology.Controllers;
 using LantanaGroup.Link.Terminology.Services;
+using Microsoft.AspNetCore.Hosting;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
 using Moq;
 using Xunit;
 using Code = LantanaGroup.Link.Terminology.Application.Models.Code;
@@ -53,6 +59,33 @@ private static Parameters AssertOkParameters(ActionResult<Parameters> result)
         return Assert.IsType<Parameters>(okResult.Value);
     }
 
+    /// <summary>
+    /// Asserts that the action produced an RFC 9457 Problem Details result with the given status, title,
+    /// type and detail.
+    /// </summary>
+    /// <remarks>
+    /// No <see cref="HttpContext"/> is wired up, so the controller's <c>ProblemDetailsFactory</c> is null and
+    /// <c>ControllerBase.Problem</c> builds a plain <see cref="ProblemDetails"/> from its arguments. The runtime
+    /// <c>traceId</c> extension and the scrubbing of 5xx detail are applied by the configured customization,
+    /// which is covered separately below (see ConfigControllerTests for the same note).
+    /// </remarks>
+    private static void AssertProblem(
+        ActionResult? result, int expectedStatus, string expectedTitle, string expectedType, string expectedDetail)
+    {
+        var objectResult = Assert.IsType<ObjectResult>(result);
+        Assert.Equal(expectedStatus, objectResult.StatusCode);
+
+        var problem = Assert.IsType<ProblemDetails>(objectResult.Value);
+        Assert.Equal(expectedTitle, problem.Title);
+        Assert.Equal(expectedStatus, problem.Status);
+        Assert.Equal(expectedType, problem.Type);
+        Assert.Equal(expectedDetail, problem.Detail);
+    }
+
+    private static void AssertBadRequestProblem(ActionResult<Parameters> result, string expectedDetail) =>
+        AssertProblem(result.Result, StatusCodes.Status400BadRequest, "Bad Request",
+            "https://tools.ietf.org/html/rfc9110#section-15.5.1", expectedDetail);
+
     [Fact]
     public void ValidateCodeInValueSet_WithDisplayContainingAmpersand_ReturnsTrue()
     {
@@ -99,8 +132,7 @@ public void ValidateCodeInValueSet_WithMarkupOnlyDisplay_ReturnsBadRequest()
 
         // Assert - the display sanitizes away to nothing; passing the empty value on would skip the
         // display check and answer result=true, so the request is rejected instead
-        var badRequest = Assert.IsType<BadRequestObjectResult>(result.Result);
-        Assert.Equal("Invalid value supplied for 'display'", badRequest.Value);
+        AssertBadRequestProblem(result, "Invalid value supplied for 'display'.");
     }
 
     [Fact]
@@ -116,8 +148,7 @@ public void ValidateCodeInCodeSystem_WithMarkupOnlyDisplay_ReturnsBadRequest()
             CodeSystemUrl, null, LoincCode, "<script>alert('x')</script>", null);
 
         // Assert
-        var badRequest = Assert.IsType<BadRequestObjectResult>(result.Result);
-        Assert.Equal("Invalid value supplied for 'display'", badRequest.Value);
+        AssertBadRequestProblem(result, "Invalid value supplied for 'display'.");
     }
 
     [Fact]
@@ -127,8 +158,7 @@ public void ValidateCodeInValueSet_WithNoUrlOrId_ReturnsBadRequest()
         var result = _controller.ValidateCodeInValueSet(null, null, null, LoincCode, null, null);
 
         // Assert
-        var badRequest = Assert.IsType<BadRequestObjectResult>(result.Result);
-        Assert.Equal("No id or url parameter specified", badRequest.Value);
+        AssertBadRequestProblem(result, "No id or url parameter specified.");
     }
 
     [Fact]
@@ -138,7 +168,110 @@ public void ValidateCodeInValueSet_WithEmptyUrl_ReturnsBadRequest()
         var result = _controller.ValidateCodeInValueSet(string.Empty, null, null, LoincCode, null, null);
 
         // Assert
-        var badRequest = Assert.IsType<BadRequestObjectResult>(result.Result);
-        Assert.Equal("No id or url parameter specified", badRequest.Value);
+        AssertBadRequestProblem(result, "No id or url parameter specified.");
+    }
+
+    [Fact]
+    public void ValidateCodeInValueSet_WithEmptyValueUriInBody_ReturnsBadRequest()
+    {
+        // Arrange - LEGLINK-887's reported request: the url arrives in the POST body as an empty valueUri
+        var parameters = new Parameters();
+        parameters.Add("url", new FhirUri(string.Empty));
+        parameters.Add("code", new FhirString(LoincCode));
+
+        // Act
+        var result = _controller.ValidateCodeInValueSet(null, null, null, null, null, parameters);
+
+        // Assert
+        AssertBadRequestProblem(result, "No id or url parameter specified.");
+    }
+
+    [Fact]
+    public void GetValueSetById_WhenValueSetNotLoaded_ReturnsNotFoundProblem()
+    {
+        // Arrange - the cache has no value set under this id
+        _mockCacheService
+            .Setup(x => x.GetCodeGroupById(CodeGroup.CodeGroupTypes.ValueSet, "missing-vs", It.IsAny<string>()))
+            .Returns((CodeGroup?)null);
+
+        // Act
+        var result = _controller.GetValueSetById("missing-vs");
+
+        // Assert
+        AssertProblem(result.Result, StatusCodes.Status404NotFound, "Not Found",
+            "https://tools.ietf.org/html/rfc9110#section-15.5.5", "Value set not found with ID missing-vs.");
+    }
+
+    [Fact]
+    public void GetValueSets_WhenCachedResourceIsNotAValueSet_ReturnsInternalServerErrorProblem()
+    {
+        // Arrange - a code group cached under the ValueSet type whose resource is a CodeSystem
+        var mismatched = BuildCodeGroup(CodeGroup.CodeGroupTypes.ValueSet, CodeSystemUrl);
+        mismatched.Resource = new CodeSystem { Id = "not-a-value-set", Url = CodeSystemUrl };
+
+        _mockCacheService
+            .Setup(x => x.GetCodeGroup(CodeGroup.CodeGroupTypes.ValueSet, ValueSetUrl, It.IsAny<string>()))
+            .Returns(mismatched);
+
+        // Act
+        var result = _controller.GetValueSets(ValueSetUrl, null);
+
+        // Assert - the controller sets the 5xx contract; the customization scrubs the detail at runtime
+        AssertProblem(result.Result, StatusCodes.Status500InternalServerError, "Internal Server Error",
+            "https://tools.ietf.org/html/rfc9110#section-15.6.1", "Code group found is not a ValueSet.");
+    }
+
+    /// <summary>
+    /// Builds the <c>CustomizeProblemDetails</c> callback the service registers at startup, so the
+    /// runtime-only behaviour can be exercised without standing up a host or issuing an HTTP request.
+    /// </summary>
+    private static Action<ProblemDetailsContext> GetConfiguredCustomization()
+    {
+        var environment = new Mock<IWebHostEnvironment>();
+        environment.SetupGet(e => e.EnvironmentName).Returns(Environments.Production);
+
+        var options = new ServiceCollection()
+            .AddTerminologyProblemDetails(environment.Object)
+            .BuildServiceProvider()
+            .GetRequiredService<IOptions<ProblemDetailsOptions>>();
+
+        return Assert.IsType<Action<ProblemDetailsContext>>(options.Value.CustomizeProblemDetails);
+    }
+
+    private static ProblemDetailsContext BuildContext(int status, string detail) => new()
+    {
+        HttpContext = new DefaultHttpContext(),
+        ProblemDetails = new ProblemDetails { Status = status, Detail = detail }
+    };
+
+    [Fact]
+    public void ProblemDetailsCustomization_ForServerError_ReplacesRawExceptionDetail()
+    {
+        // Arrange - the raw message a 500 would otherwise carry out of the controller
+        var context = BuildContext(StatusCodes.Status500InternalServerError, "Value set could not be copied.");
+
+        // Act
+        GetConfiguredCustomization()(context);
+
+        // Assert - internal state is replaced by a generic message, and a traceId is added to correlate
+        Assert.Equal(
+            "An error occurred in our API. Please use the trace id when requesting assistance.",
+            context.ProblemDetails.Detail);
+        Assert.DoesNotContain("Value set could not be copied", context.ProblemDetails.Detail);
+        Assert.True(context.ProblemDetails.Extensions.ContainsKey("traceId"));
+    }
+
+    [Fact]
+    public void ProblemDetailsCustomization_ForClientError_PreservesDetail()
+    {
+        // Arrange - scrubbing must be limited to 5xx; a 4xx detail is actionable and must survive
+        var context = BuildContext(StatusCodes.Status404NotFound, "Value set not found with ID missing-vs.");
+
+        // Act
+        GetConfiguredCustomization()(context);
+
+        // Assert
+        Assert.Equal("Value set not found with ID missing-vs.", context.ProblemDetails.Detail);
+        Assert.True(context.ProblemDetails.Extensions.ContainsKey("traceId"));
     }
 }

DotNet/Terminology/Controllers/FhirController.cs

@@ -54,6 +54,40 @@ public class FhirController(FhirService fhirService) : Controller
         return sanitized;
     }
 
+    /// <summary>
+    /// Builds an RFC 9457 Problem Details result for a failed terminology request, matching the shape
+    /// <see cref="ConfigController"/> already returns. The <c>traceId</c> extension is added by the
+    /// service-wide customization in <c>TerminologyProblemDetailsExtensions</c>.
+    /// </summary>
+    /// <remarks>
+    /// Exception messages are written as fragments ("Value set not found with ID x") because they are
+    /// also read from logs and asserted in unit tests. <c>detail</c> is prose, so a terminating period
+    /// is added here rather than baked into every message at the throw site.
+    /// </remarks>
+    private ObjectResult TerminologyProblem(HttpStatusCode statusCode, string title, string type, string detail)
+    {
+        var sentence = detail.EndsWith('.') || detail.EndsWith('?') || detail.EndsWith('!')
+            ? detail
+            : detail + ".";
+
+        return Problem(detail: sentence, statusCode: (int)statusCode, title: title, type: type);
+    }
+
+    /// <summary>Client input failed validation. RFC 9110 section 15.5.1.</summary>
+    private ObjectResult BadRequestProblem(string detail) => TerminologyProblem(
+        HttpStatusCode.BadRequest, "Bad Request", "https://tools.ietf.org/html/rfc9110#section-15.5.1", detail);
+
+    /// <summary>The requested terminology resource is not loaded. RFC 9110 section 15.5.5.</summary>
+    private ObjectResult NotFoundProblem(string detail) => TerminologyProblem(
+        HttpStatusCode.NotFound, "Not Found", "https://tools.ietf.org/html/rfc9110#section-15.5.5", detail);
+
+    /// <summary>
+    /// A loaded code group could not be used as requested. RFC 9110 section 15.6.1. The customization
+    /// replaces <c>detail</c> with a generic message so internal state is not exposed to the caller.
+    /// </summary>
+    private ObjectResult InternalServerErrorProblem(string detail) => TerminologyProblem(
+        HttpStatusCode.InternalServerError, "Internal Server Error", "https://tools.ietf.org/html/rfc9110#section-15.6.1", detail);
+
     #region Value Sets
 
     /// <summary>
@@ -75,11 +109,11 @@ public ActionResult<ValueSet> GetValueSetById([FromRoute] string id)
         }
         catch (ArgumentException ex)
         {
-            return BadRequest(ex.Message);
+            return BadRequestProblem(ex.Message);
         }
         catch (KeyNotFoundException ex)
         {
-            return NotFound(ex.Message);
+            return NotFoundProblem(ex.Message);
         }
     }
 
@@ -105,11 +139,11 @@ public ActionResult<Bundle> GetValueSets([FromQuery] string? url,
         }
         catch (ArgumentException ex)
         {
-            return BadRequest(ex.Message);
+            return BadRequestProblem(ex.Message);
         }
         catch (InvalidOperationException ex)
         {
-            return StatusCode(StatusCodes.Status500InternalServerError, ex.Message);
+            return InternalServerErrorProblem(ex.Message);
         }
     }
 
@@ -134,15 +168,15 @@ public ActionResult<ValueSet> ExpandValueSet([FromRoute] string? id, [FromQuery]
         }
         catch (ArgumentException ex)
         {
-            return BadRequest(ex.Message);
+            return BadRequestProblem(ex.Message);
         }
         catch (KeyNotFoundException ex)
         {
-            return NotFound(ex.Message);
+            return NotFoundProblem(ex.Message);
         }
         catch (InvalidOperationException ex)
         {
-            return StatusCode(StatusCodes.Status500InternalServerError, ex.Message);
+            return InternalServerErrorProblem(ex.Message);
         }
     }
 
@@ -168,11 +202,11 @@ public ActionResult<CodeSystem> GetCodeSystemById([FromRoute] string id)
         }
         catch (ArgumentException ex)
         {
-            return BadRequest(ex.Message);
+            return BadRequestProblem(ex.Message);
         }
         catch (KeyNotFoundException ex)
         {
-            return NotFound(ex.Message);
+            return NotFoundProblem(ex.Message);
         }
     }
 
@@ -198,11 +232,11 @@ public ActionResult<Bundle> GetCodeSystems([FromQuery] string? url, [FromQuery(N
         }
         catch (ArgumentException ex)
         {
-            return BadRequest(ex.Message);
+            return BadRequestProblem(ex.Message);
         }
         catch (InvalidOperationException ex)
         {
-            return StatusCode(StatusCodes.Status500InternalServerError, ex.Message);
+            return InternalServerErrorProblem(ex.Message);
         }
     }
 
@@ -235,7 +269,7 @@ public ActionResult<Parameters> ValidateCodeInCodeSystem([FromQuery] string? url
         }
         catch (ArgumentException ex)
         {
-            return BadRequest(ex.Message);
+            return BadRequestProblem(ex.Message);
         }
     }
 
@@ -271,7 +305,7 @@ public ActionResult<Parameters> ValidateCodeInValueSet([FromQuery] string? url,
         }
         catch (ArgumentException ex)
         {
-            return BadRequest(ex.Message);
+            return BadRequestProblem(ex.Message);
         }
     }