Skip to content

LEGLINK-889: Add cached ValueSet member lookup and harden CSV status parsing - #1819

Merged
MikeAtPinnacle merged 14 commits into
devfrom
users/mtherien/leglink-889-fix-test-endpoint
Aug 12, 2026
Merged

LEGLINK-889: Add cached ValueSet member lookup and harden CSV status parsing#1819
MikeAtPinnacle merged 14 commits into
devfrom
users/mtherien/leglink-889-fix-test-endpoint

Conversation

@MikeAtPinnacle

@MikeAtPinnacle MikeAtPinnacle commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

🛠️ Description of Changes

Adds the missing cached-ValueSet-member lookup that LEGLINK-889 turned out to need, hardens CSV status parsing so one bad cell cannot drop a whole code group, and brings along the in-memory CSV upload endpoints so QA can set up terminology fixtures without editing the shared data volume.

LEGLINK-889 — what the ticket actually was

The report was that marking a code inactive and reading it back through GET /config/code-systems/{id}/codes/{code} always answers Active. Reproducing it from TestRail 10977 showed the case marks postal inactive in the ValueSet CSV and then reads the CodeSystem back. Those are two separate cached code groups from two separate directories, and the code system's own codes are never touched by that case — so Active was the correct answer. Ingestion, lookup and serialization were each verified to handle Inactive correctly and are covered by existing tests.

The real gap was that nothing could read a cached value set member's own membership status back.

  • Add GET /api/terminology/config/value-sets/{id}/codes/{code}, with optional system and version. It reports membershipStatus (what the value set itself declares, null when its CSV has no status column) alongside effectiveStatus (what actually applies). Two values rather than one because conflating them is what produced this ticket: null is not Active, it means the value set said nothing and the question defers to the code system.
  • Member selection mirrors ValueSet/$validate-code exactly — an explicit system narrows the search, otherwise the first system listing the code wins, and within a system the last occurrence wins (LEGLINK-599/814).
  • Make FhirService.ResolveIsActive public as ResolveCodeStatus, so the new endpoint and $validate-code resolve status through one implementation rather than two that can drift apart.
  • Declare CodeSystemCode as the code system lookup's response type, so Swagger documents the status field it has always returned on the wire.
  • Sanitize/decode consistently across all three config endpoints, so a code or system URI containing a reserved character still matches the cache. FhirController already did this; the two disagreeing is the class of defect this ticket is about.

CSV status parsing

  • Read the status column as a string and interpret it in the loader. CsvHelper's enum converter threw on any unrecognized value, and because records enumerate lazily, that throw escaped ProcessCodeSystemCsv, was swallowed by LoadCache, and cost the entire code group — thousands of good codes lost to one typo, with the group then answering 404. Both loaders now default the row to Active and log one warning per file naming the offending codes. The ValueSet loader already defaulted, but silently.
  • The warning is aggregated per file rather than per row: these loops run once per code and a large code system carries hundreds of thousands.

Terminology CSV upload (rides along)

  • Add PUT /api/terminology/config/value-sets/{id}/codes and .../code-systems/{id}/codes (multipart, part name file), answering 202 with the resulting code, system, and inactive-code counts. Nothing is written to Terminology:Path, the FHIR resource metadata is preserved exactly, and POST $reload-cache reverts.
  • Gated by Terminology:EnableCodeUploadEndpoint, default false, reporting 404 rather than 403 while off, so a disabled instance is indistinguishable from one that never shipped the routes. Enabled in the Development and Docker settings only, and cataloged in app-config.yaml.
  • Fixes a latent bug in SetCodeGroup: CacheKey overrides neither Equals nor GetHashCode, so the ConcurrentBag.Contains guard was reference equality and never matched. Harmless while LoadCache (which clears first) was the only caller, but every replace would leak a duplicate key.

The two halves compose well: ReplaceCodesFromCsv calls the same Process*Csv methods this PR hardens, so an uploaded CSV carrying an unrecognized status now defaults that row and warns instead of throwing the whole upload away.

Also included

  • TECH_DEBT: refresh DotNet/MockDmrpApi/packages.lock.json. It was the only project whose committed lock file predated a package graph shift that every other project already carries, so a restore regenerated it and left the file dirty for anyone building that project. No package reference changes.

🧪 Testing Performed

  • dotnet test DotNet/ServiceTests/ServiceTests.csproj --filter FullyQualifiedName~UnitTests1584 unit tests pass, and the solution builds clean.
  • Serialization was ruled out as a cause for LEGLINK-889 by standing up a minimal MVC host with the same shape (derived object returned through ActionResult<Code>); the response body carries "status":"Inactive", confirming MVC serializes by runtime type and only the documented Swagger type was wrong.
  • CsvHelper 33.1.0's behavior on an unrecognized status was confirmed standalone before the change: it throws TypeConverterException mid-enumeration, which is what cost the whole file.
  • The upload endpoints were verified previously against the local docker-compose stack: uploading one code to v3-ActEncounterCode took its expansion from 11 codes to 1, a former member stopped validating while the injected code validated with its inactive warning, the resource metadata and the CSV on disk were both untouched, empty/wrong-column/unknown-id/non-csv/wrong-type requests returned 400 or 404 without disturbing the override, and $reload-cache restored all 11 codes.

🧑‍🔬 Unit Testing

  • I have written or updated unit tests to cover my changes
  • Coverage: 96.7%
    • ConfigControllerTests — 13 new cases for GetValueSetCode (400/404 shapes, declared membership status wins in both directions, rejoin from the code system when the value set declares nothing, null vs Active, system filtering, first-system-wins, last-occurrence-wins, blank-parameter handling), plus existing GetCodeSystemCode cases updated for the CodeSystemCode response type. The cache is now mocked at ICodeGroupCacheService with a real FhirService sharing it, so the status reported is resolved by the same code $validate-code uses.
    • CodeGroupCacheServiceTests — a theory over three kinds of unrecognized status asserting the code group survives and only the bad row defaults, the equivalent for the ValueSet loader, and verification that the warning is logged once per file. Plus the upload branch's ReplaceCodesFromCsv coverage (unknown target, wrong column count leaves the cache intact, repeated replaces do not accumulate cache keys, malformed-CSV handling).

📓 Documentation Updated

  • app-config.yaml — catalogued Terminology:EnableCodeUploadEndpoint (required: false, defaultValue: "false").
  • docs/config-key-inventory.md — regenerated for the new key.
  • XML documentation on every new public member, including why ValueSetCodeLookupResult reports two statuses and why the CSV status column is read as a string.

Summary by CodeRabbit

  • New Features
    • Added ValueSet code lookup with system, membership, and effective-status details.
    • Added configurable CSV replacement endpoints for ValueSets and CodeSystems, including validation and replacement summaries.
    • Added upload controls that are disabled by default and enabled in supported environments.
  • Bug Fixes
    • Improved handling of invalid or missing CSV statuses by defaulting them to Active.
    • Prevented duplicate cached entries and preserved code-group metadata during replacements.
  • Documentation
    • Updated configuration documentation with the new upload setting.

Terminology loads every code group once at boot from the shared data volume, so changing a
single code to set up a test meant editing the volume the whole dev environment reads and
restarting the service. This adds two endpoints that swap one code group's codes in the
serving instance's memory:

    PUT /api/terminology/config/value-sets/{id}/codes?version=
    PUT /api/terminology/config/code-systems/{id}/codes?version=

multipart/form-data, part name "file", answering 202 with the resulting code, system and
inactive-code counts. Nothing is written to Terminology:Path, the FHIR resource metadata
(url, version, id, name, identifiers) is preserved exactly, and POST $reload-cache reverts.

- Gated by Terminology:EnableCodeUploadEndpoint, default false, so a missing key anywhere
  leaves the feature off. While false the routes report 404 rather than 403, making a
  disabled instance indistinguishable from one that never shipped the endpoints. Enabled in
  the Development and Docker settings only, and catalogued in app-config.yaml.
- ReplaceCodesFromCsv builds a new CodeGroup and lets SetCodeGroup swap it in with a single
  cache.Set. The service is a singleton and FhirService enumerates CodeGroup.Codes while
  serving requests, so an in-place edit could tear a reader's enumeration; a fresh group is
  also required because Process*Csv appends without clearing. Both Process*Csv methods call
  SetCodeGroup last and enumerate the CSV lazily, so a parse failure leaves the previously
  loaded codes intact with no rollback needed.
- Fixes a latent bug in SetCodeGroup: CacheKey overrides neither Equals nor GetHashCode, so
  the ConcurrentBag.Contains guard was reference equality and never matched. Harmless while
  LoadCache, which clears the cache first, was the only caller, but every replace would leak
  a duplicate key that lengthens each subsequent lookup scan. It now compares on the
  composite key.
- The 32 MB upload cap clears the largest shipped artifact, SNOMED CT at 16.4 MB.
- Malformed-CSV responses report the failing row but never echo CsvHelper's message, which
  embeds the caller-supplied field text.

The endpoints are unauthenticated, matching the rest of the Terminology service, which
declares no authorization anywhere. The config flag is the only thing gating them, and they
should only be enabled where a single instance runs, since an upload reaches one instance.

Testing: 101 unit tests pass in UnitTests.Terminology. Verified against the local
docker-compose stack that uploading one code to v3-ActEncounterCode took its expansion from
11 codes to 1, that a former member stopped validating while the injected code validated
with its inactive warning, that the resource metadata and the CSV on disk were both
untouched, that empty, wrong-column, unknown-id, non-csv and wrong-type requests returned
400 or 404 without disturbing the override, and that $reload-cache restored all 11 codes.

A LEGLINK ticket still needs to be created; the PR title will need it.
…parsing

Marking a code inactive in a value set CSV and reading it back through
GET /config/code-systems/{id}/codes/{code} returns the code system's own
status, which looks like the edit was ignored. Value set membership status is
deliberately independent of code system status - it overrides the code system
when a code is validated, but never writes back to it - and there was no
endpoint that read a cached value set member's status back at all.

- Add GET /api/terminology/config/value-sets/{id}/codes/{code}, with optional
  system and version. It reports membershipStatus (what the value set itself
  declares, null when its CSV has no status column) alongside effectiveStatus
  (what applies). Member selection mirrors ValueSet/$validate-code: an explicit
  system narrows the search, otherwise the first system listing the code wins,
  and within a system the last occurrence wins.

- Make FhirService.ResolveIsActive public as ResolveCodeStatus, so the new
  endpoint and $validate-code resolve status through one implementation rather
  than two that can drift.

- Read the CSV status column as a string and interpret it in the loader.
  CsvHelper's enum converter threw on any unrecognized value, and because the
  records are enumerated lazily that throw escaped ProcessCodeSystemCsv and was
  swallowed by LoadCache, costing the entire code group over one bad cell. Both
  loaders now default the row to Active and log one warning per file naming the
  offending codes; the value set loader already defaulted, but silently.

- Declare CodeSystemCode as the code system lookup's response type so Swagger
  documents the status field it has always returned.

- Decode after sanitizing in both config lookups, so a code or system URI
  containing a reserved character still matches the cache. FhirController
  already does this, and the two disagreeing is the class of defect this ticket
  is about.

Testing: dotnet test DotNet/ServiceTests - 1556 unit tests pass, including 15
new or updated cases covering the new endpoint's status resolution and a
regression guard proving an unrecognized status no longer drops a code group.
…ien/leglink-889-fix-test-endpoint

Brings the in-memory CSV upload endpoints alongside the LEGLINK-889 work so QA can set up
terminology fixtures without editing the data volume the whole environment reads.

The two branches touch the same three Terminology files. Resolution:

- ConfigController takes both new dependencies: FhirService for the value set lookup's effective
  status, IOptions<TerminologyConfig> for the upload feature flag. The routes do not collide -
  GET value-sets/{id}/codes/{code} and PUT value-sets/{id}/codes differ in both verb and shape.

- ConfigControllerTests was rebuilt. The upload tests were written against the old constructor and
  a concrete-class mock; they now use the interface mock and the BuildController(enableCodeUpload:)
  helper. Their ValueSetId constant collided with the lookup tests' and is now UploadValueSetId.

- ReplaceCodesFromCsv_MalformedStatus_ThrowsAndLeavesCacheIntact asserted that an unrecognized
  status throws CsvHelperException mid-enumeration, which is the behaviour LEGLINK-889 deliberately
  removed. It now asserts the replacement behaviour - every row loads and the bad one defaults to
  Active - while atomicity on a genuine parse failure stays covered by the WrongColumnCount test.

- The upload path's id is sanitized through the same SanitizeLookupValue helper as the lookups, so
  all three endpoints in the file treat a reserved character in an id identically.

The combination is worth more than either side alone: ReplaceCodesFromCsv calls the same Process*Csv
methods LEGLINK-889 hardened, so an uploaded CSV carrying an unrecognized status now defaults that
row and warns instead of throwing the whole upload away.

Testing: dotnet test DotNet/ServiceTests - 1584 unit tests pass, and the solution builds clean.
MockDmrpApi is the only project whose committed lock file predates the package
graph shift that every other project already carries - RESPite in place of
Pipelines.Sockets.Unofficial, and the Microsoft.Extensions.* transitives at
10.0.10. Its lock file was written once by LEGLINK-822 and never refreshed, so
a restore regenerates it and leaves the file dirty for anyone who builds the
project.

No package reference changes; this is the restore output catching up to the
rest of the repo.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds ValueSet code lookup and gated CSV replacement endpoints. It introduces replacement models and configuration, preserves metadata during cache updates, normalizes CSV statuses, exposes effective status resolution, and adds controller and service test coverage.

Changes

Terminology flow

Layer / File(s) Summary
Contracts and settings
DotNet/Terminology/Application/Interfaces/ICodeGroupCacheService.cs, DotNet/Terminology/Application/Models/*, DotNet/Terminology/Application/Settings/TerminologyConfig.cs
Adds the CSV replacement contract, lookup and replacement response models, upload configuration, and nullable raw CSV status handling.
Cache replacement and status processing
DotNet/Terminology/Services/CodeGroupCacheService.cs, DotNet/Terminology/Services/FhirService.cs, DotNet/ServiceTests/UnitTests/Terminology/Services/CodeGroupCacheServiceTests.cs
Replaces cached codes without mutating prior instances, preserves metadata, removes duplicate cache keys, normalizes invalid statuses to Active, aggregates warnings, and resolves effective code status.
Lookup and upload endpoints
DotNet/Terminology/Controllers/ConfigController.cs
Adds ValueSet lookup, updates CodeSystem responses, validates decoded lookup inputs, and processes gated multipart CSV replacements with structured errors and summaries.
Controller behavior coverage
DotNet/ServiceTests/UnitTests/Terminology/Controllers/ConfigControllerTests.cs
Tests ValueSet matching, system and version selection, status precedence, CodeSystem response types, upload validation, feature gating, error mapping, successful replacement, and version normalization.
Runtime configuration
DotNet/Terminology/appsettings*.json, app-config.yaml, docs/config-key-inventory.md
Configures upload endpoints for development and Docker environments, keeps the default disabled, and updates the configuration inventory.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ConfigController
  participant ICodeGroupCacheService
  participant CodeGroupCacheService
  Client->>ConfigController: Upload CodeSystem or ValueSet CSV
  ConfigController->>ConfigController: Validate feature flag, file, identifier, and CSV shape
  ConfigController->>ICodeGroupCacheService: ReplaceCodesFromCsv(type, id, version, csvContent)
  ICodeGroupCacheService->>CodeGroupCacheService: Parse CSV and publish replacement
  CodeGroupCacheService-->>ConfigController: Return replaced CodeGroup
  ConfigController-->>Client: Return ReplaceCodesResponse
Loading

Possibly related PRs

Suggested reviewers: seanmcilvenna, johnbritton

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.38% 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 identifies cached ValueSet member lookup and CSV status parsing as the primary changes.
Description check ✅ Passed The description completes all template sections and provides detailed change scope, testing evidence, unit coverage, and documentation updates.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch users/mtherien/leglink-889-fix-test-endpoint

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

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

@MikeAtPinnacle
MikeAtPinnacle marked this pull request as ready for review August 12, 2026 18:01
@MikeAtPinnacle
MikeAtPinnacle requested review from a team as code owners August 12, 2026 18:01

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (5)
DotNet/Terminology/Controllers/ConfigController.cs (2)

108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Declare the Problem Details response types in the Swagger annotations.

The 400 and 404 responses return ValidationProblemDetails or ProblemDetails, but the attributes declare no Type. The generated OpenAPI document therefore describes these responses without a schema, so clients cannot generate error models. The controller path instructions require Swagger/OpenAPI documentation for every API.

♻️ Proposed change for the lookup endpoints
     [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(CodeSystemCode))]
-    [ProducesResponseType(StatusCodes.Status400BadRequest)]
-    [ProducesResponseType(StatusCodes.Status404NotFound)]
+    [ProducesResponseType(StatusCodes.Status400BadRequest, Type = typeof(ValidationProblemDetails))]
+    [ProducesResponseType(StatusCodes.Status404NotFound, Type = typeof(ProblemDetails))]

As per coding guidelines: "Implement Swagger/OpenAPI documentation for every API".

Also applies to: 183-185, 321-323, 364-366

🤖 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/Controllers/ConfigController.cs` around lines 108 - 110,
Update the Swagger annotations for the lookup endpoints in ConfigController,
including the response declarations near the referenced blocks, to specify
ValidationProblemDetails for 400 responses and ProblemDetails for 404 responses.
Apply the same response Type metadata consistently to every affected endpoint
while preserving the existing status codes and success response documentation.

Source: Path instructions


432-436: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Bound the read by the validated length instead of reading an unbounded stream.

file.Length was already checked against MaxCsvUploadBytes, so this read is bounded in practice. One detail is worth confirming: StreamReader decodes without a byte order mark check argument, so a UTF-8 BOM stays in the first field of the first row. The header row is skipped, so the BOM is harmless today. If a caller ever posts a headerless file, the first system value contains the BOM and the match fails silently.

Consider new StreamReader(file!.OpenReadStream(), Encoding.UTF8, detectEncodingFromByteOrderMarks: true) to remove the ambiguity.

🤖 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/Controllers/ConfigController.cs` around lines 432 - 436,
Update the StreamReader setup used to populate csvContent so reading is
explicitly limited to the already validated file.Length (capped by
MaxCsvUploadBytes), rather than relying on an unbounded stream. Configure UTF-8
decoding with BOM detection enabled so a leading BOM is removed before parsing
system values, while preserving the existing cancellation-aware read.
DotNet/ServiceTests/UnitTests/Terminology/Controllers/ConfigControllerTests.cs (1)

533-559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for the oversize-file branch.

The upload validation has four file branches: null, empty, oversize, and non-.csv extension. Tests cover three. The file.Length > MaxCsvUploadBytes branch at ConfigController.cs Line 408 has no test, so a regression in the limit or the message would not be detected.

A stubbed IFormFile with a reported Length above the limit avoids allocating 32 MB in the test.

💚 Proposed test
[Fact]
public async Task ReplaceValueSetCodes_FileTooLarge_Returns400WithFileError()
{
    var file = new Mock<IFormFile>();
    file.SetupGet(x => x.Length).Returns(33L * 1024 * 1024);
    file.SetupGet(x => x.FileName).Returns("codes.csv");

    var result = await _controller.ReplaceValueSetCodes(UploadValueSetId, file.Object);

    var problem = AssertProblem(result, StatusCodes.Status400BadRequest);
    Assert.Contains("file", Assert.IsType<ValidationProblemDetails>(problem).Errors.Keys);
    _mockCacheService.Verify(
        x => x.ReplaceCodesFromCsv(It.IsAny<CodeGroup.CodeGroupTypes>(), It.IsAny<string>(), It.IsAny<string?>(), It.IsAny<string>()),
        Times.Never);
}

As per path instructions: "If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test."

🤖 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/ConfigControllerTests.cs`
around lines 533 - 559, Add a unit test alongside the existing
ReplaceValueSetCodes file-validation tests covering the oversize branch in
ReplaceValueSetCodes. Use a mocked IFormFile reporting a Length above
MaxCsvUploadBytes with a .csv filename, assert a 400 response containing a file
validation error, and verify _mockCacheService.ReplaceCodesFromCsv is never
called.

Source: Path instructions

DotNet/ServiceTests/UnitTests/Terminology/Services/CodeGroupCacheServiceTests.cs (1)

678-782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case with several unrecognized rows to prove aggregation.

Every unrecognized-status test uses exactly one bad row. Times.Once therefore passes even if the loader logged one warning per bad row. Add a CSV with two or more unrecognized statuses and keep Times.Once. That assertion is the one that verifies the per-file aggregation this change introduces. Asserting that the message names both offending codes also covers CodeStatusParser.Examples.

Based on path instructions: "If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test."

💚 Proposed additional test
[Fact]
public async Task LoadCache_MultipleUnrecognizedStatuses_LogsOneWarningNamingEachCode()
{
    using var memoryCache = new MemoryCache(new MemoryCacheOptions());
    var mockConfig = new Mock<IOptions<TerminologyConfig>>();
    mockConfig.Setup(x => x.Value).Returns(_config);

    var directoryFiles = new Dictionary<string, string[]>
    {
        ["/test/path/cs"] = new[] { "cs.json", "cs.csv" }
    };
    var fileContents = new Dictionary<string, string>
    {
        ["cs.json"] = "{ \"resourceType\": \"CodeSystem\", \"id\": \"test-cs\", " +
                      "\"url\": \"http://test.codesystem\", \"version\": \"1.0\" }",
        ["cs.csv"] = "code,display,status\r\n" +
                     "123,One,Retired\r\n" +
                     "456,Two,Bogus\r\n" +
                     "789,Three,Inactive\r\n"
    };

    var service = new TestableCodeGroupCacheService(
        _loggerMock.Object, memoryCache, mockConfig.Object, directoryFiles, fileContents);

    await service.LoadCache();

    // One warning for the whole file, naming both offending codes.
    _loggerMock.Verify(
        x => x.Log(
            LogLevel.Warning,
            It.IsAny<EventId>(),
            It.Is<It.IsAnyType>((v, _) =>
                v.ToString()!.Contains("unrecognized status") &&
                v.ToString()!.Contains("123") &&
                v.ToString()!.Contains("456")),
            It.IsAny<Exception>(),
            It.IsAny<Func<It.IsAnyType, Exception?, string>>()),
        Times.Once);
}
🤖 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/Services/CodeGroupCacheServiceTests.cs`
around lines 678 - 782, Add a test near
LoadCache_CodeSystemUnrecognizedStatus_KeepsTheCodeSystemAndDefaultsTheRow with
multiple unrecognized status rows in the same CSV, such as codes 123 and 456,
plus a valid row. Verify both malformed rows default to Active, the file remains
cached, and the warning is logged exactly once with a message containing
“unrecognized status” and both offending codes, covering per-file aggregation
and CodeStatusParser.Examples.

Source: Path instructions

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

195-207: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Replace the check-then-add guard with a keyed concurrent collection.

The duplicate-key fix is correct in intent, but the guard is not atomic. _cacheKeys.Any(...) followed by _cacheKeys.Add(...) is a check-then-act sequence on a ConcurrentBag. Two concurrent SetCodeGroup calls for the same composite key can both observe "absent" and both add. The upload endpoint makes concurrent calls reachable, because this service is a singleton and ReplaceCodesFromCsv runs on request threads.

The scan is also O(n) per call, so LoadCache becomes O(n²) in the number of cached groups.

A ConcurrentDictionary<string, CacheKey> keyed on CacheKey.Key removes both problems. It needs matching updates in GetCodeGroupById, GetCodeGroup, GetAllCodeGroups, and ClearCache, so it is a follow-up rather than a blocker.

♻️ Sketch of the keyed-collection change
-    private readonly ConcurrentBag<CacheKey> _cacheKeys = new ConcurrentBag<CacheKey>();
+    private readonly ConcurrentDictionary<string, CacheKey> _cacheKeys = new(StringComparer.Ordinal);
     protected internal virtual void SetCodeGroup(CodeGroup codeGroup)
     {
         CacheKey urlKey = new CacheKey((CodeGroup.CodeGroupTypes)codeGroup.Type!, codeGroup.Url!, codeGroup.Version!, codeGroup.Id!, codeGroup.Identifiers);
         cache.Set(urlKey.Key, codeGroup, _cacheOptions);
-
-        if (!_cacheKeys.Any(k => string.Equals(k.Key, urlKey.Key, StringComparison.Ordinal)))
-            _cacheKeys.Add(urlKey);
+        _cacheKeys[urlKey.Key] = urlKey;
     }

Reads then iterate _cacheKeys.Values, and ClearCache uses _cacheKeys.Clear().

🤖 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/CodeGroupCacheService.cs` around lines 195 - 207,
Replace the ConcurrentBag-based _cacheKeys collection with a
ConcurrentDictionary<string, CacheKey> keyed by CacheKey.Key, and update
SetCodeGroup to assign via the dictionary indexer so duplicate inserts are
atomic and constant-time. Adjust GetCodeGroupById, GetCodeGroup, and
GetAllCodeGroups to iterate _cacheKeys.Values, and update ClearCache to call
_cacheKeys.Clear() while preserving existing cache behavior.
🤖 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.

Inline comments:
In
`@DotNet/ServiceTests/UnitTests/Terminology/Controllers/ConfigControllerTests.cs`:
- Around line 404-422: Update
GetValueSetCode_NoSystemSupplied_TakesFirstSystemListingTheCode so it does not
depend on Dictionary enumeration order: assert that lookup.System is either
otherSystem or AddressTypeSystem, then assert MembershipStatus matches the
corresponding system’s expected status (Active for otherSystem, Inactive for
AddressTypeSystem). Only change the cache structure if the surrounding
implementation explicitly requires load-order semantics.

In
`@DotNet/ServiceTests/UnitTests/Terminology/Services/CodeGroupCacheServiceTests.cs`:
- Around line 1267-1284: The regression test does not directly verify that
duplicate cache keys are avoided. Add an internal CacheKeyCount member to
CodeGroupCacheService exposing _cacheKeys.Count, then update
ReplaceCodesFromCsv_RepeatedReplaces_DoNotAccumulateCacheKeys to assert the
expected single key after repeated replacements while retaining the existing
behavior assertions.

In `@DotNet/Terminology/appsettings.Docker.json`:
- Around line 4-5: Update the shared Docker configuration’s
EnableCodeUploadEndpoint setting to false, and provide any required test-only
override separately so code-upload routes are enabled only explicitly during
tests.

In `@DotNet/Terminology/Controllers/ConfigController.cs`:
- Around line 374-390: Protect the terminology upload actions
ReplaceValueSetCodes and ReplaceCodeSystemCodes with authorization that always
requires an authenticated user and the required permission, independent of
Authentication:EnableAnonymousAccess and permissive shared policies. Add the
authorization metadata or explicit checks at the controller/action level, while
preserving the existing disabled-endpoint 404 behavior in ReplaceCodesAsync.

In `@DotNet/Terminology/Services/CodeGroupCacheService.cs`:
- Around line 504-526: Update the Parse method’s invalid-status example
construction to truncate both code and rawStatus before appending, ensuring each
formatted entry remains bounded and the overall _examples value stays within
MaxExampleLength. Preserve the existing separator, invalid-count increment,
fallback status, and LogInvalidStatusWarning sanitization behavior, and align
the implementation with the XML remark describing truncated examples.

---

Nitpick comments:
In
`@DotNet/ServiceTests/UnitTests/Terminology/Controllers/ConfigControllerTests.cs`:
- Around line 533-559: Add a unit test alongside the existing
ReplaceValueSetCodes file-validation tests covering the oversize branch in
ReplaceValueSetCodes. Use a mocked IFormFile reporting a Length above
MaxCsvUploadBytes with a .csv filename, assert a 400 response containing a file
validation error, and verify _mockCacheService.ReplaceCodesFromCsv is never
called.

In
`@DotNet/ServiceTests/UnitTests/Terminology/Services/CodeGroupCacheServiceTests.cs`:
- Around line 678-782: Add a test near
LoadCache_CodeSystemUnrecognizedStatus_KeepsTheCodeSystemAndDefaultsTheRow with
multiple unrecognized status rows in the same CSV, such as codes 123 and 456,
plus a valid row. Verify both malformed rows default to Active, the file remains
cached, and the warning is logged exactly once with a message containing
“unrecognized status” and both offending codes, covering per-file aggregation
and CodeStatusParser.Examples.

In `@DotNet/Terminology/Controllers/ConfigController.cs`:
- Around line 108-110: Update the Swagger annotations for the lookup endpoints
in ConfigController, including the response declarations near the referenced
blocks, to specify ValidationProblemDetails for 400 responses and ProblemDetails
for 404 responses. Apply the same response Type metadata consistently to every
affected endpoint while preserving the existing status codes and success
response documentation.
- Around line 432-436: Update the StreamReader setup used to populate csvContent
so reading is explicitly limited to the already validated file.Length (capped by
MaxCsvUploadBytes), rather than relying on an unbounded stream. Configure UTF-8
decoding with BOM detection enabled so a leading BOM is removed before parsing
system values, while preserving the existing cancellation-aware read.

In `@DotNet/Terminology/Services/CodeGroupCacheService.cs`:
- Around line 195-207: Replace the ConcurrentBag-based _cacheKeys collection
with a ConcurrentDictionary<string, CacheKey> keyed by CacheKey.Key, and update
SetCodeGroup to assign via the dictionary indexer so duplicate inserts are
atomic and constant-time. Adjust GetCodeGroupById, GetCodeGroup, and
GetAllCodeGroups to iterate _cacheKeys.Values, and update ClearCache to call
_cacheKeys.Clear() while preserving existing cache behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6c98e5b5-6486-4dc8-8b02-b37b2e1374e2

📥 Commits

Reviewing files that changed from the base of the PR and between 8e15e7d and be496be.

📒 Files selected for processing (15)
  • DotNet/ServiceTests/UnitTests/Terminology/Controllers/ConfigControllerTests.cs
  • DotNet/ServiceTests/UnitTests/Terminology/Services/CodeGroupCacheServiceTests.cs
  • DotNet/Terminology/Application/Interfaces/ICodeGroupCacheService.cs
  • DotNet/Terminology/Application/Models/CsvCodeSystemRecord.cs
  • DotNet/Terminology/Application/Models/ReplaceCodesResponse.cs
  • DotNet/Terminology/Application/Models/ValueSetCodeLookupResult.cs
  • DotNet/Terminology/Application/Settings/TerminologyConfig.cs
  • DotNet/Terminology/Controllers/ConfigController.cs
  • DotNet/Terminology/Services/CodeGroupCacheService.cs
  • DotNet/Terminology/Services/FhirService.cs
  • DotNet/Terminology/appsettings.Development.json
  • DotNet/Terminology/appsettings.Docker.json
  • DotNet/Terminology/appsettings.json
  • app-config.yaml
  • docs/config-key-inventory.md

Comment thread DotNet/Terminology/appsettings.Docker.json
Comment thread DotNet/Terminology/Controllers/ConfigController.cs
Comment thread DotNet/Terminology/Services/CodeGroupCacheService.cs
…nfig endpoints

A CodeRabbit-style review of PR #1819 raised four findings against the new
cached-code lookups and the CSV upload endpoints. All four are addressed here.

- A CodeSystem CSV carrying a header and no data rows never created the system
  key, so ProcessCodeSystemCsv's trailing LogDebug indexed a missing key and
  threw - after SetCodeGroup had already swapped the emptied group into the
  cache. Log arguments are evaluated eagerly, so it threw whatever the
  configured level. Counting via TryGetValue fixes it; emptying a code group is
  a legitimate outcome of an upload, not an error. The comment claiming both
  Process*Csv methods call SetCodeGroup last was wrong for the CodeSystem path,
  which is what let this hide, and is corrected.

- ReplaceCodesAsync caught bare KeyNotFoundException, so any failed dictionary
  lookup under the call became a 404 with the internal message echoed to the
  client. Added CodeGroupNotFoundException, thrown at the one intentional site
  and caught alone; anything else now surfaces as a 500 with a traceId. It
  derives from KeyNotFoundException so the interface contract stays
  source-compatible.

- Plumbed a CancellationToken through ICodeGroupCacheService.LoadCache and
  ReplaceCodesFromCsv, checked per row in both Process*Csv loops and forwarded
  from ReloadCache, Startup and ReplaceCodesAsync. The parse is synchronous and
  walks up to 32 MB row by row, so without this a client disconnect left the
  work running to completion.

- A supplied 'system' that sanitizes away to nothing is now rejected with a 400
  rather than treated as omitted. Omitting the system widens the search to every
  system in the value set, so silently dropping an unusable one could answer
  with a member under a system the caller never asked about.
  FhirController.SanitizeTerminologyValue refuses an emptied value for the same
  reason. The blank check now runs on the sanitized value, not the raw one.

- Sanitized cleanId with SanitizeForLog() before it reaches the logger, matching
  what the service layer one call down already did.

Testing: dotnet test DotNet/ServiceTests - 1594 unit tests pass, build clean.
The header-only regression guard was widened from a Fact covering ValueSet to a
Theory over both group types, and verified to bite by reverting the fix and
confirming only the CodeSystem case failed. New tests cover the exact
cancellation token reaching the cache, and both sides of the system-sanitizing
boundary: input the sanitizer strips entirely is a 400, while an allow-listed
tag survives and is a 404 - so the guard cannot drift into rejecting real
system URIs.
Comment thread DotNet/Terminology/Controllers/ConfigController.cs Dismissed
@MikeAtPinnacle MikeAtPinnacle self-assigned this Aug 12, 2026
…Dictionary ordering

GetValueSetCode_NoSystemSupplied_TakesFirstSystemListingTheCode named the
expected system directly, which only held because Dictionary<TKey,TValue>
happens to enumerate in insertion order. That is an implementation detail, not
a documented contract.

The expectation is now read from the same walk FindValueSetMember performs
rather than naming a key, so the ordering assumption is gone while the
behaviour under test is still pinned: an implementation that picked the last
matching system, or an arbitrary one, still fails. Asserting only that the
result is one of the two systems would have removed the ordering dependency as
well, but it would also have stopped the test from checking the thing it exists
for - that this endpoint and $validate-code resolve to the same occurrence.

Note the ordering dependency itself is in the production code, not just the
test: FindValueSetMember and FhirService.ValidateCodeAcrossSystems both choose
between systems by walking Codes.Keys. It is left as-is because the two agree
with each other, which is what correctness rests on here, and defining a real
ordering would mean changing $validate-code too.

Testing: dotnet test DotNet/ServiceTests - 1596 unit tests pass, build clean.
…duplicates

ReplaceCodesFromCsv_RepeatedReplaces_DoNotAccumulateCacheKeys could not fail
for the reason its name gives. Duplicate keys are invisible through the public
surface: GetAllCodeGroups collapses them with its group-by on id, and the
lookups still return the right group and merely scan further. Both of its
assertions passed whether or not the guard in SetCodeGroup worked.

- Add an internal CacheKeyCount to CodeGroupCacheService exposing the tracked
  key count. Test-only, and consistent with how this class is already reached
  from tests - Process*Csv are internal and the file-system seams are
  protected internal virtual for the same reason.
- Assert the replaces leave the key count unchanged. Compared against the
  post-load count rather than a literal: the fixture loads both a CodeSystem
  and a ValueSet, so the count after loading is two, and the invariant worth
  stating is "replacing adds no keys" rather than "there is exactly one".
- Keep the two original assertions, with a comment recording that they check
  the replaces worked and are not evidence about key accumulation, so they are
  not mistaken for the guard again.

Verified the new assertion bites by reverting SetCodeGroup to the pre-fix
_cacheKeys.Contains(urlKey) and rerunning: expected 2, actual 5 - two loaded
groups plus one leaked key per replace. The two pre-existing assertions still
passed during that run, which is the direct confirmation they were vacuous. The
guard was then restored.

Testing: dotnet test DotNet/ServiceTests - 1596 unit tests pass, build clean.
…endpoints

The upload endpoints replace the codes every downstream validation is judged
against, and until now the only thing gating them was
Terminology:EnableCodeUploadEndpoint. Anything able to reach the service in an
environment where that flag is on could rewrite cached terminology.

- Apply [Authorize("IsLinkAdmin")] to ReplaceValueSetCodes and
  ReplaceCodeSystemCodes, and document 401 and 403 on both. The
  disabled-endpoint 404 is unaffected: the flag check still runs first, so a
  disabled instance stays indistinguishable from one that never shipped the
  routes.
- Add UploadEndpoints_RequireTheAdminPolicy, a reflection theory over both
  actions. Every other test in that region invokes the action directly, so all
  of them pass with the attribute deleted; this is the only thing that would
  notice. It asserts the policy name rather than the mere presence of an
  AuthorizeAttribute, since a bare [Authorize] behaves differently.

What this does and does not buy, recorded on the policy constant so the
attribute is not read as a stronger guarantee than it is:

It protects wherever authentication is actually configured, which is every
deployed environment - Authentication:EnableAnonymousAccess is False for
Terminology in dev, qa, qa2 and test, so the real IsLinkAdmin policy applies.

It does not hold under anonymous access. AddLinkBearerServiceAuthentication
returns before registering any authentication scheme in that mode and defines
every named policy as RequireAssertion(context => true), so the attribute
passes for anyone - as does every other endpoint in the service, none of which
declares a policy. A bare [Authorize] would not close that gap either: with no
default challenge scheme it throws rather than returning 401. Closing it
properly needs an explicit authenticated-user check, which would make these
endpoints permanently unusable under docker-compose, the environment they exist
to serve. There the remaining protection is the flag, which ships false.

Testing: dotnet test DotNet/ServiceTests - 1598 unit tests pass, build clean.
CodeStatusParser bounded the accumulated sample before appending but never
bounded the entry being appended. Both halves of an entry are raw CSV content
and neither is length-limited by the format, so a single row could put an
arbitrarily long value into the log line - and the upload endpoints accept up
to 32 MB. The Examples property already documented the sample as truncated, so
the code and its own contract disagreed.

- Cap each half of an entry at MaxExampleValueLength before appending, marking
  a cut with an ellipsis. The separator, the invalid-count increment, the
  Active fallback and the SanitizeForLog call in LogInvalidStatusWarning are
  unchanged.
- Cap the parts rather than clamping the finished string to MaxExampleLength.
  Clamping would cut the final entry mid-token for no real benefit; capping
  each half keeps every entry readable and still bounds the result, which the
  Examples doc now states rather than leaving "truncated" to interpretation.
- Add LoadCache_UnrecognizedStatusWithOverlongValues_TruncatesTheLoggedExamples:
  a 400-character code and status, asserting neither reaches the log line and
  that the ellipsis is present. It also asserts the row still loads and still
  defaults to Active, since truncation must affect the log sample only and not
  the cached codes.

Verified the test bites by reverting the Truncate calls and rerunning: it
failed, then passed again once restored.

Not addressed here: LogScientificNotationWarning's accumulator a few lines up
has the same unbounded-entry shape. It predates this work and is untouched by
this branch, so it is left alone rather than widening the diff.

Testing: dotnet test DotNet/ServiceTests - 1599 unit tests pass, build clean.
@MikeAtPinnacle
MikeAtPinnacle merged commit da5f51e into dev Aug 12, 2026
18 checks passed
@MikeAtPinnacle
MikeAtPinnacle deleted the users/mtherien/leglink-889-fix-test-endpoint branch August 12, 2026 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants