LEGLINK-888: ValueSet $validate-code rejects a blank "system" with a 400

[MikeAtPinnacle]

🛠️ Description of Changes

ValueSet/$validate-code treated a blank system as if none had been supplied. string.IsNullOrEmpty sent both "" and null down the same branch, so a malformed request was answered by searching every code system in the value set and reporting 200 result=false/result=true — an answer to a broader question than the caller asked, with no indication in the payload of which system matched.

An absent system is legitimate FHIR and keeps its "search every system" meaning. An empty string is not valid for any FHIR primitive, so the request is now rejected rather than reinterpreted.

• Adds NormalizeSystem and validates every client-supplied system up front in ValidateCodeInValueSet: the query parameter, the body system parameter, coding.system, and each codeableConcept.coding.system. Validating before the merges matters because they treat an empty string as "not supplied" and would overwrite a blank before anything could see it; doing it up front also keeps the verdict for a malformed codeableConcept independent of which coding happens to match first.
• Whitespace-only is treated as blank, matching the FHIR rule that a primitive carries at least one character of non-whitespace content.
• The string.IsNullOrEmpty check in ValidateCodeInCodeGroup is deliberately left alone. It is shared with ValidateCodeInCodeSystem, which passes the code group's own Url into that slot — cache content, not client input — so an empty one must keep falling back to a search rather than being blamed on the caller.

Two things worth a reviewer's attention:

1. Deliberate non-FHIR leniency. The literal strings "null" and "undefined" are treated as an omitted parameter, for clients that interpolate an unset variable into a request. This is not FHIR behaviour and no spec text will explain it, so it is pinned by tests and called out here. Agreed with QA on LEGLINK-888.

2. A service-wide MVC change. PreserveEmptyStringMetadataProvider turns off MVC's ConvertEmptyStringToNull. Without it, ?system= arrives at the action as null and is indistinguishable from an omitted parameter, so the validation above could never fire for the query-string form — validation cannot restore a distinction the binder has already erased. MvcOptions exposes no setting for this and DisplayFormatAttribute cannot target an action parameter, so a display-metadata provider is the supported route.

Blast radius is contained: 8 string query parameters across the two Terminology controllers, all guarded by IsNullOrEmpty/IsNullOrWhiteSpace, and ConfigController's version already normalises blank to null itself. Body binding is unaffected — FhirModelBinder deserialises with System.Text.Json and never consults MVC metadata.

🧪 Testing Performed

Unit tests: 1185 pass, 0 fail (dotnet test DotNet/ServiceTests/ServiceTests.csproj --filter FullyQualifiedName~UnitTests). Terminology suite is 97, up from 77.

Confirmed the metadata provider is load-bearing by removing it and re-running: exactly one test fails, the ?system= case. Without it that scenario silently returns 200.

Verified against the local docker-compose stack, before and after rebuilding the Terminology image:

Request	Before	After
coding.system blank in body	200 result=true	400 — The 'coding.system' parameter cannot be blank.
?system= on the query string	200 result=true	400 — The 'system' parameter cannot be blank.
system blank in body	200 result=true	400 — The 'system' parameter cannot be blank.
inside codeableConcept	200 result=true	400 — The 'codeableConcept.coding.system' parameter cannot be blank.
omitted system	200 result=true	unchanged
valid system	200 result=true	unchanged
?system=null	200 result=false	200 result=true (now searches all systems)

Each 400 returns application/problem+json carrying type, title, status, detail and a W3C traceId.

QA cases: TestRail 10992, 11015, 11342 and 11343 were updated by QA to match these responses, and all four have been exercised end to end from Postman against the DEV terminology service.

🧑‍🔬 Unit Testing

• I have written or updated unit tests to cover my changes
• Coverage: 96.3%

📓 Documentation Updated

No documentation changes are required by this PR:

• No new configuration keys, so app-config.yaml is unchanged.
• No entity changes, so no EF migration.
• No REST route or status-code changes beyond the corrected 400, which the API guidance already prescribes for input that fails validation.

The "null"/"undefined" leniency in item 1 above is behaviour the spec will not document. It is recorded on LEGLINK-888 so QA can cover it; there is currently no TestRail case for it.

Summary by CodeRabbit

• Bug Fixes

  • Improved $validate-code handling for blank, omitted, "null", and "undefined" system values.
  • Blank system values now return standardized 400 responses instead of being treated as missing.
  • Omitted or placeholder systems continue searching across all systems and validate successfully when appropriate.
  • Applied consistent behavior across coding, codeable-concept, query, and request-body inputs.

• Tests

  • Added comprehensive coverage for system-value validation and HTTP responses.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 55e0fda2-b69e-46d2-b3df-9969df53afa9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the primary change: rejecting blank system values with HTTP 400.
Description check	✅ Passed	The description covers the changes, testing, unit-test updates, coverage, and documentation status in the required sections.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

DotNet/Terminology/Services/FhirService.cs (1)

597-611: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Consider trimming the system before comparison.

NormalizeSystem rejects a whitespace-only system, but it keeps surrounding whitespace on a non-blank value. A request with ?system=%20http://x therefore reaches the lookup with a leading space and returns result=false instead of matching the system. A trim also makes the placeholder comparison catch " null ".

♻️ Proposed refactor

-        // FHIR requires at least one character of non-whitespace content, so "   " is as malformed as "".
-        if (string.IsNullOrWhiteSpace(system))
-        {
-            throw new ArgumentException($"The '{parameterName}' parameter cannot be blank");
-        }
-
-        return SystemPlaceholders.Contains(system, StringComparer.OrdinalIgnoreCase) ? null : system;
+        // FHIR requires at least one character of non-whitespace content, so "   " is as malformed as "".
+        if (string.IsNullOrWhiteSpace(system))
+        {
+            throw new ArgumentException($"The '{parameterName}' parameter cannot be blank");
+        }
+
+        var trimmed = system.Trim();
+
+        return SystemPlaceholders.Contains(trimmed, StringComparer.OrdinalIgnoreCase) ? null : trimmed;

🤖 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/Terminology/Services/FhirService.cs` around lines 597 - 611, Update
NormalizeSystem to trim the validated system value before comparing it against
SystemPlaceholders and returning it, while preserving null and whitespace-only
handling. Ensure values such as " http://x " match lookups and whitespace-padded
placeholders resolve to null.

DotNet/ServiceTests/UnitTests/Terminology/Controllers/FhirControllerHttpTests.cs (1)

250-254: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the result value, not only the presence of the parameter.

The placeholder test states that ?system=null is treated as an absent system. The assertions only check the status code and that the payload contains "result". A response with result=false also passes, so the test does not prove the stated behavior. Parse the payload and assert the boolean value. The same weakness applies to Line 224-225, where Assert.Contains("true", payload) matches any occurrence of true in the JSON.

💚 Proposed fix

         var (status, payload) = await PostAsync(query, body);
 
         Assert.Equal(HttpStatusCode.OK, status);
-        Assert.Contains("\"result\"", payload);
+        AssertValidationResult(payload, true);
     }

Add a shared helper next to AssertBadRequestDetail, and use it in ValidateCodeInValueSet_AbsentSystem_SearchesAllSystemsAndSucceeds as well:

private static void AssertValidationResult(string body, bool expected)
{
    using var document = JsonDocument.Parse(body);

    var parameter = document.RootElement
        .GetProperty("parameter")
        .EnumerateArray()
        .Single(p => p.GetProperty("name").GetString() == "result");

    Assert.Equal(expected, parameter.GetProperty("valueBoolean").GetBoolean());
}

🤖 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/FhirControllerHttpTests.cs`
around lines 250 - 254, Add a shared AssertValidationResult helper beside
AssertBadRequestDetail that parses the JSON payload and asserts the result
parameter’s valueBoolean equals the expected value. Replace the broad payload
assertions in ValidateCodeInValueSet_AbsentSystem_SearchesAllSystemsAndSucceeds
and the test around the existing Assert.Contains("true", payload) with this
helper, passing the expected boolean.

🤖 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/FhirControllerHttpTests.cs`:
- Around line 250-254: Add a shared AssertValidationResult helper beside
AssertBadRequestDetail that parses the JSON payload and asserts the result
parameter’s valueBoolean equals the expected value. Replace the broad payload
assertions in ValidateCodeInValueSet_AbsentSystem_SearchesAllSystemsAndSucceeds
and the test around the existing Assert.Contains("true", payload) with this
helper, passing the expected boolean.

In `@DotNet/Terminology/Services/FhirService.cs`:
- Around line 597-611: Update NormalizeSystem to trim the validated system value
before comparing it against SystemPlaceholders and returning it, while
preserving null and whitespace-only handling. Ensure values such as " http://x "
match lookups and whitespace-padded placeholders resolve to null.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 247fb1f8-12c5-44e3-863d-58b8cb5058a2

📥 Commits

Reviewing files that changed from the base of the PR and between 908f46f and 7635e5d.

📒 Files selected for processing (6)

• DotNet/ServiceTests/UnitTests/Terminology/Controllers/FhirControllerHttpTests.cs
• DotNet/ServiceTests/UnitTests/Terminology/Controllers/FhirControllerTests.cs
• DotNet/ServiceTests/UnitTests/Terminology/Services/FhirServiceTests.cs
• DotNet/Terminology/Application/Formatters/PreserveEmptyStringMetadataProvider.cs
• DotNet/Terminology/Program.cs
• DotNet/Terminology/Services/FhirService.cs