LEGLINK-822: Add Mock DMRP API for reporting-plan integration testing - #1800
Open
MikeAtPinnacle wants to merge 25 commits into
Open
LEGLINK-822: Add Mock DMRP API for reporting-plan integration testing#1800MikeAtPinnacle wants to merge 25 commits into
MikeAtPinnacle wants to merge 25 commits into
Conversation
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Adds DotNet/MockDmrpApi, a service that stands in for the DMRP (Digital Monthly Reporting Plan) API that Leidos is building for CDC. The real endpoint is reachable only from the CDC Dev environment, so local and QA testing in the LCG environments cannot exercise code that depends on it. This phase establishes the contract and the code generation that binds to it. No endpoints are implemented yet; the service serves only /health. Contracts/dmrp-openapi.yaml is the source of truth. It is written as a production contract -- base path /, production operation names, no "test" or "mock" vocabulary anywhere in it -- so that replacing it with the published contract is a file swap rather than a translation exercise. It is marked provisional: LCG has no published OpenAPI description, so every path, field and status code in it is a reconstruction and none of it should be treated as authoritative. NSwag regenerates the abstract controller base and the DTOs from that spec into obj/ before every compile where the spec has changed. The generated file is deliberately not committed: keeping it in obj/ makes it impossible for the C# and the spec to drift, and keeps a contract change reviewable as a spec diff rather than thousands of lines of generated churn. Once the controllers exist, replacing the spec will surface every difference as a compile error. Three spec shapes are load-bearing and should not be "tidied" later: - Every request body is application/json, including the token endpoint. RFC 6749 specifies form encoding there; a consistent encoding across the contract is preferred, and it is also what the generated [FromBody] parameter actually binds -- a form-encoded body would be rejected with 415. - Path parameters are declared per-operation, ahead of query parameters. Declaring them once at the path-item level is equivalent OpenAPI, but generators emit operation-level parameters first, which places an optional query parameter ahead of a required path one and produces uncompilable C#. - sortBy is a closed enum rather than a free string. The repository layer resolves it to a property by name, so an unconstrained value would be a server fault rather than a client error. Testing Performed: built the project and link-cloud.sln. Confirmed codegen runs on a clean build, is skipped when the spec is unchanged, re-runs when the spec is touched, and is skipped under /p:RunNSwagCodeGen=false. Confirmed the spec contains no "test" or "mock" occurrences. Inspected the generated output: the base type is DmrpControllerBase, it carries no class-level route attribute, and IssueToken binds its request from the body.
Second phase of the Mock DMRP API. Adds the reporting plan entry table, its EF Core context and mapping, the initial migration, and the local database provisioning entry. Still no endpoints; the service continues to serve only /health. MockDmrpEntry derives from the shared BaseEntityExtended, so Id, CreateDate and ModifyDate come from the shared base and the timestamps are maintained by UpdateBaseEntityInterceptor rather than by hand. The entity is deliberately kept separate from the generated ReportingPlanEntry contract type: persisting the generated type would tie the schema to the API contract and turn every revision of Contracts/dmrp-openapi.yaml into a migration. IsReporting is stored as a string rather than a bool. The upstream field is only known to carry "Y", and it is not known to be boolean, so narrowing it now would be a guess baked into the schema. One composite unique index, (FacilityId, ReportingYear, ReportingMonth, Measure), does three jobs: it makes the by-facility lookup a seek, makes the reporting plan lookup a seek on its first three columns, and enforces the natural key. The last of those matters most -- without it the table could hold two contradictory rows for the same facility, measure and period, and produce a nonsensical reporting plan. Column order is by selectivity for those two queries rather than the order the fields read in. The design-time context factory reads ConnectionStrings__DatabaseConnection from the environment before falling back to a local default. EF prefers a design-time factory over the application's service provider, so a hardcoded string there is what "dotnet ef database update" would actually target -- quietly pointing schema changes at the wrong server. Testing Performed: applied the migration to a local SQL Server instance and confirmed the table shape, the primary key and the unique index. Inserted a row, confirmed a duplicate natural key is rejected with error 2601, and confirmed a differing measure for the same facility and period is accepted. Rolled the migration back with "database update 0", confirmed the table was dropped, and re-applied it. Built the project and link-cloud.sln.
Third phase of the Mock DMRP API. Adds the two services that hold the behaviour, the mapper between the entity and the generated contract types, and 84 unit tests. Still no endpoints; the service continues to serve only /health. Also renames the types introduced in the previous phase. "Mock" reads as "test double" to any .NET developer, so a production MockDmrpEntryService is actively misleading. No implementation type carries the prefix now. The project, namespace, configuration section, database and routes keep it, because there it accurately warns whoever is reading a config store or a connection string that this service is a stand-in rather than the real DMRP API. MockDmrpEntry -> ReportingPlanEntryEntity MockDmrpDbContext -> ReportingPlanDbContext MockDmrpEntryMap -> ReportingPlanEntryMap IMockDmrpEntryService -> IReportingPlanService MockDmrpEntryService -> ReportingPlanService IMockAuthTokenService -> IAuthTokenService MockAuthTokenService -> AuthTokenService MockDmrpApiSettings -> DmrpApiSettings MockDmrpApiConstants -> DmrpApiConstants MockDmrpSearchCriteria -> ReportingPlanSearchCriteria The entity takes an Entity suffix because the natural domain name is already taken by the generated ReportingPlanEntry contract type. The table remains MockDmrpEntries, so the schema is untouched and no migration is needed. ReportingPlanService owns storage and querying. Reporting plan lookups filter to entries actively being reported, because enrollment is conveyed by presence -- an entry marked as not reporting is equivalent to no entry at all, and letting one through would tell a caller a facility is enrolled in a measure it has opted out of. Writes guard the natural key before touching the store and translate a unique-index violation into the same failure. The pre-check gives a clean answer; the index is the actual guarantee, and both are needed because the pre-check has a race the index does not. Update is update-only: a missing entry returns null rather than being created, so a caller cannot create entries through a verb that promises not to. Sorting is restricted to a closed enum mapped onto property names. The shared repository resolves the sort field by building an Expression.Property from the supplied name and throws for anything that is not a property, so an unconstrained string would turn ordinary client input into a server fault. Paging is clamped rather than rejected. AuthTokenService issues real signed JSON Web Tokens rather than opaque strings -- standard claims, HS512, a genuine expiry -- so a caller's acquire, cache and refresh-on-expiry path is exercised instead of trivially satisfied. Validation checks issuer, audience, lifetime and signature, and answers false for every rejection reason rather than throwing, since a malformed header is a client error and not a fault. The signing key is validated at construction: HS512 needs 512 bits, and catching a short key at startup beats an opaque 500 on the first token request. Note that every replica must be configured with the same signing key. Tokens are validated by the same service that issues them, so a per-instance key would mean a token minted by one instance is rejected by another -- an intermittent 401 that would read as a bug in the caller. EntryMapper is the seam that keeps the contract out of the database. Everything below it deals in entities, everything above it in generated types, so replacing Contracts/dmrp-openapi.yaml produces compile errors here rather than in the service layer or a migration. It reads stored timestamps as UTC explicitly; the column carries no offset, so left unqualified the conversion would apply the host's local offset and a developer would see different values than CI. The entry service tests use a hand-written in-memory repository rather than a mock. Most of what is under test is the predicates and sort expressions the service builds, and a mock would record that a search happened with "some expression" while asserting nothing about whether that expression selects the right rows. Testing Performed: 84 new unit tests covering plan projection and its absence semantics, filter composition, the sort whitelist, paging clamps, duplicate detection on create and update, update-only behaviour, the delete variants, token claims and every validation rejection path, and mapper round-tripping. Ran the full unit suite after the rename: 1188 passed, 0 failed. Confirmed EF still resolves the renamed context and lists the existing migration.
Fourth phase of the Mock DMRP API. Implements all ten operations the
contract declares and adds a stand-in for the NHSN Auth API. The service
now answers requests rather than only /health.
DmrpController overrides the generated abstract base, so the routes,
response types and validation attributes all come from
Contracts/dmrp-openapi.yaml. Each action sanitizes its string input,
forwards its cancellation token, and maps domain outcomes onto the status
codes the contract documents: 201 with a Location header on create, 202
on update, 409 on a duplicate natural key, 404 when update is asked to
change something that does not exist, 204 for an empty page, and 400 with
"Invalid Id format" for a malformed identifier.
Two behaviours are deliberate departures worth not "fixing" later. Update
returns 404 rather than creating, so a caller cannot create entries
through a verb that promises not to. And the reporting plan query returns
200 with an empty measures array for a facility enrolled in nothing --
not 204 and not 404 -- because an empty plan is a meaningful answer,
unlike an empty search result.
Overriding a generated controller has two traps, both now covered by
GeneratedControllerBindingTests. Binding source attributes do survive the
override, because MVC resolves them through the base declaration. Default
parameter values do not: an override that omits "= 10" silently binds
null and unpages the endpoint. Nullability does not either: NSwag types
optional string filters as non-nullable, which [ApiController] treats as
required, so the contract's optional filters would return 400 until the
override restates them as nullable. Every override therefore repeats both.
The tests exercise routing, binding, status codes and serialization over
real HTTP rather than calling the methods directly, which is what makes
the routing precedence assertion possible -- /search and /reporting-plans
have to win over /{id}, or they would be read as identifiers.
NhsnAuthController serves the auth-test route from the ticket. It stands
in for the NHSN Auth API, which is a separate service a caller
authenticates against rather than a DMRP operation, so it sits outside
the contract document. It delegates to the token action so there is one
token implementation, and the tests assert the token it issues is
accepted by the contract operation.
The ticket also named an api-test route, which this does not implement.
The reporting plan query belongs to the real API and is already served at
GET /dmrp/mock/reporting-plans; a second path to the same operation would
only invite a consumer to integrate against a route the real service does
not have.
No LinkSdk client ships with this service. Nothing in the repository
needs typed access to a stand-in, and adding one would put a Mock DMRP
URL on ServiceRegistry, a shared type every service compiles against.
Callers use curl, Postman, or a plain HttpClient from a test.
MockFhirServer has no SDK client either.
Testing Performed: 26 controller tests over real HTTP covering every
operation, the auth simulation route, the status-code contract, routing
precedence, duplicate handling, update-only behaviour, the
auth-then-query sequence and the empty-plan case; 11 binding tests
pinning the generated-base behaviour above. Ran the full unit suite: 1225
passed, 0 failed. Built link-cloud.sln.
Fifth phase of the Mock DMRP API. Adds the switch that keeps this stand-in from serving traffic where it does not belong, and the info endpoint the switch keeps available. Availability is decided in two layers, and the outer one is absolute. Production never serves the mock, whatever any configuration source says; everywhere else MockDmrpApi:Enabled decides, defaulting to enabled so a bare "dotnet run" works. The environment block exists because Azure App Configuration is appended last in the configuration chain, so a row provisioned against a production label would silently outrank appsettings and environment variables. That failure would be invisible -- a running mock looks exactly like a healthy service -- so it is closed off in code rather than left to configuration hygiene alone. Hygiene still applies as a second line: appsettings.Production.json ships Enabled false, and no production row should ever be created. Both the request pipeline and startup consult the same decision, so a deployment cannot end up serving traffic while skipping migrations, or the reverse. When disabled, the service logs a warning naming the environment and the routes that remain, and skips EF migration entirely -- a dormant deployment has no business creating or altering a schema. The gate is middleware registered before routing, so nothing added later can be reached while the service is disabled. It answers 503 with problem details carrying a traceId. The alternatives were worse: refusing to start crash-loops the pod and pages someone, and skipping route registration yields a 404 indistinguishable from a typo'd path or a misconfigured ingress. 503 is also the honest answer -- the service is reachable, it just will not serve this environment. /health and /api/mock-dmrp/info answer either way. Health has to stay up or the container reports unhealthy and restarts, which reads as an outage rather than a service that is deliberately dormant; info lets an operator confirm which build is deployed without enabling anything. Testing Performed: 30 unit tests covering the decision and the gate, including that Production refuses an explicit Enabled=true, that the allow-list does not leak onto the API surface, and that a refused request never reaches the rest of the pipeline. Also ran the service itself in three configurations against a local SQL Server and confirmed by request: Production refuses the whole surface with 503 and skips migration while health and info answer 200; the same build in Development serves search, issues a token and returns a reporting plan; and Development with MockDmrpApi__Enabled=false refuses the surface while health still answers. Full unit suite: 1255 passed, 0 failed. Built link-cloud.sln.
Sixth phase of the Mock DMRP API. Adds the container image, the compose service, and integration tests that run against a real database. This is the first phase where the service runs as a deployed artifact rather than from dotnet run, which is what surfaced the two fixes below. The image regenerates the API contract during build. obj/ is excluded by .dockerignore, so the generated controller and DTOs are produced inside the image from Contracts/dmrp-openapi.yaml rather than copied in -- a build that cannot reach the spec or the generator fails there rather than shipping a stale contract. The compose service depends on mssql_init, unlike mock-fhir-server: this one has a database, and migration runs at startup against a catalog create-dbs.sql has to create first. Credentials and the signing key come from environment variables with local defaults, so a developer needs no setup and nothing sensitive is committed. Two fixes found by running it: Swagger was configured but never wired. appsettings carried EnableSwagger while Program.cs registered neither the generator nor the middleware, so /swagger returned 404. It is now registered after the availability gate, so a disabled deployment does not advertise a surface it will not serve. Note the reflected document is not the contract: it shows the routes as this service hosts them, under /dmrp/mock, while Contracts/dmrp-openapi.yaml describes the API rooted at / as it is expected to be published. Issued tokens carried no iat claim. JwtSecurityToken writes nbf and exp from its constructor arguments but not iat, and callers commonly read it to reason about token age. It is now added explicitly and asserted, so the token carries the full set: iss, aud, sub, scope, iat, nbf, exp, jti. The integration tests cover what the in-memory fake cannot show: the save interceptor stamping CreateDate and ModifyDate, predicates and orderings that have to translate to SQL, paging against real data, and the unique index rejecting a duplicate natural key when written straight through the context, bypassing the service's pre-check. The reporting plan semantics get their own file because presence is the entire signal -- a measure wrongly appearing or wrongly vanishing misleads a consumer about what a facility reports. The fixture uses SQLite with EnsureCreated, so the EF migration itself is not exercised there; that is covered by bringing the service up against SQL Server. Testing Performed: 18 integration tests and the updated unit suite; full test run 1806 passed, 1 skipped, 0 failed. Built the image and brought it up in compose, reaching healthy, and confirmed by request: swagger and its document serve; create returns 201 with a Location that resolves; a duplicate returns 409; PUT on a missing entry returns 404 and on an existing one 202; a bad sortBy returns 400 rather than 500; a non-GUID id returns 400; an unknown facility returns 204. Acquired a token, decoded it to confirm all eight claims and a 3600s lifetime, confirmed the reporting plan lists only enrolled measures, and confirmed a facility enrolled in nothing returns 200 with an empty array rather than 204 or 404. Ran the same image with MockDmrpApi__Enabled=false and confirmed the whole surface answers 503 while health and info answer 200 and migration is skipped.
Seventh phase of the Mock DMRP API. Adds the pieces needed to deploy the service to the AKS cluster and to configure it per environment. The service now loads external configuration. This was a gap rather than plumbing: it previously read only appsettings and environment variables, while every other deployed service selects its own Azure App Configuration label. It loads that label before the availability check runs, so the switch that keeps this stand-in out of production sees the same values every other consumer of configuration sees. The CD pipeline follows the existing per-service template, path-filtered on DotNet/MockDmrpApi and DotNet/Shared, publishing link-mock-dmrp. Its build step regenerates the API contract from Contracts/dmrp-openapi.yaml, so a pipeline run fails if the spec and the implementation have diverged. Its test step points at ServiceTests, which holds this service's unit tests alongside every other service's, filtered to this service and excluding the integration suite -- that suite needs Docker and a database the build agent does not have. Note the Terminology pipeline points at DotNet/TerminologyTests/TerminologyTests.csproj, which does not exist; that is pre-existing and untouched here. The deploy matrix row and the kubectl line follow the established shape: deployment mock-dmrp-deploy, container mock-dmrp, image link-mock-dmrp. The health check the matrix runs answers even when the mock is disabled by configuration, so a deliberately dormant deployment passes rather than being reported as an outage. Four keys are catalogued in app-config.yaml: the master switch and the three auth values. All are required: false, because this service runs only in the lower environments and its keys should be provisioned only where it is deployed. The signing key description records the constraint that every replica must share one value -- tokens are validated by the service that issues them, so a per-instance key means a token minted by one pod is rejected by another. The catalog entries use only key, description and required, which is what the schema in that file actually defines. Not done, and worth knowing: docs/config-key-inventory.md does not exist in this repository, and the scripts that would generate it (extract_config_keys.py, check_required_config.py, validate_app_config_schema.py, dump_config_symbols.cs) are not present either, nor is a workflow enforcing the catalog. CLAUDE.md documents all of it, so it is either stale or lives elsewhere. The catalog entry above was written against the schema that is actually in the file and validated against it. Deploying this still depends on work outside this repository: the AKS Deployment, Service and Ingress objects named above do not exist here, so the pipeline's kubectl set image has nothing to target until someone creates them. The link-mock-dmrp database and the App Configuration rows also need provisioning per environment. Testing Performed: validated app-config.yaml parses and that every entry in the file, including the new ones, uses only the fields its schema defines. Built link-cloud.sln and ran the unit suite: 1255 passed, 0 failed. The pipeline and deploy matrix changes are not exercisable locally.
Eighth and final phase of the Mock DMRP API. Adds the service README. The centrepiece is how the project is built, because contract-first code generation is not a pattern used anywhere else in this repository and several of its consequences are not obvious from reading the code: - Validation rules live in the spec, not the C#. A range change is a yaml edit. sortBy is a closed enum there for a reason -- the repository resolves the sort field by property name and throws for anything that is not one, so an unconstrained value would turn client input into a server fault. - Generated code is not committed. In obj/ the C# and the spec cannot drift, and a contract change stays reviewable as a spec diff rather than thousands of lines of mechanical churn. - After a fresh clone the generated types do not exist until the first build, so the IDE shows errors that are not a broken checkout. - Overriding the generated base has two traps that produce no compiler warning: default parameter values are not inherited, and NSwag types optional string filters as non-nullable, which [ApiController] treats as required. It also documents the procedure for swapping in Leidos's contract when it arrives -- replace wholesale, rebuild, fix the compile errors -- and a troubleshooting table for the ways codegen can fail. Two warnings are called out rather than left to be discovered. The Swagger page is not the contract: it is a reflected OpenAPI 2.0 view of the prefixed routes, while the contract is 3.0.3 rooted at /. And the token this service issues is symmetric with no discovery document or key set, so a consumer cannot write signature validation the way it will be written against real NHSN Auth -- the one seam where mock-tested integration code is not the code that ships. The open questions section lists every invented field, path and shape, so a reader can see the size of the guess they are building on. Testing Performed: ran every command the README gives. The test filter returns 169 passing tests; /p:RunNSwagCodeGen=false skips codegen; and deleting obj/NSwag and rebuilding regenerates the contract, which is the fresh-clone behaviour the README describes. Confirmed every file path it references exists. Full test run 1806 passed, 1 skipped, 0 failed.
The contract's server URL now names the running stand-in
(http://localhost:6159/dmrp/mock) rather than a bare root, so its paths
resolve against something reachable and tooling generated from it can
actually call the service. This is a testing convenience and will be
replaced along with the rest of the document when Leidos publishes theirs.
That has a side effect worth knowing about. Because the server URL now has
a path component, NSwag emits a class-level [Route("dmrp/mock")] on the
generated base -- it did not when the URL was "/". DmrpController declares
the same route itself, so the prefix is now stated twice.
It resolves correctly: attribute routing takes the most-derived
declaration rather than combining them, so the prefix is applied once.
Nothing was asserting that, so ThePrefixIsAppliedExactlyOnce now checks
both halves -- that /dmrp/mock/search resolves, and that
/dmrp/mock/dmrp/mock/search does not. If that behaviour ever inverts,
every endpoint moves and this test is what says so.
The explicit route on the controller stays, and is what actually decides
the served path. Without it, shortening the server URL to
http://localhost:6159 would empty the generated route and silently move
every endpoint to /. It also insulates the service from whatever base path
the real contract eventually carries.
The README and the DmrpController remarks described the contract as
"rooted at /", which is no longer true. Both now describe paths as
relative to the server URL, which carries the prefix. The Swagger warning
is restated more accurately too: the meaningful distinction is a
hand-authored 3.0.3 document versus a reflected 2.0 one generated from the
controllers, not which path they are rooted at.
Testing Performed: rebuilt from a cleared obj/NSwag and confirmed the
generated routes are unchanged apart from the new class-level attribute.
Ran the controller suite (26 tests) and the new prefix guard. Built
link-cloud.sln and ran the unit suite: 1256 passed, 0 failed.
docs/config-key-inventory.md is generated from the code by
Scripts/AzureAppConfig, and the committed copy had drifted from what the
source actually reads. Regenerating it picks up that drift along with the
keys the Mock DMRP API added.
Most of this diff is not about the Mock DMRP API. Regenerating on an
unmodified dev produces 253 insertions and 248 deletions on its own --
mostly call-site line numbers that moved, chiefly in
Automation.UI/Program.cs, and rows shifting between service sections. Only
eight added rows are the new service's, which is why this is a separate
TECH_DEBT commit rather than part of LEGLINK-822: a reviewer can skip it
whole rather than reading 500 lines of unrelated churn to find eight.
The new rows are MockDmrpApi:Enabled, AuthClientId, AuthClientSecret and
SigningKey, each marked as catalogued, plus Audience, Issuer and
TokenLifetimeSeconds, which are not -- they have safe shipped defaults and
are not provisioned per environment. No key shows a store, correctly: the
service is not deployed yet.
The file says not to edit it by hand, so adding only the eight relevant
rows was not an option.
Regenerated with:
dotnet run --file Scripts/AzureAppConfig/dump_config_symbols.cs -- \
DotNet Scripts/AzureAppConfig/config_symbols.json
python Scripts/AzureAppConfig/extract_config_keys.py
Testing Performed: ran both generator steps against .NET 10, parsing 1377
source files. Confirmed the drift is pre-existing by running the same
regeneration in a clean worktree checked out at origin/dev, with none of
this branch's code present, and seeing the same 253/248 change. The two
JSON side-outputs remain gitignored, as the tooling's README specifies.
… surface The service was built on a wrong premise: that the CRUD, search and token endpoints in Contracts/dmrp-openapi.yaml were the third-party DMRP contract. They are not. They are our own endpoints, for seeding and inspecting test data. The real surface is two endpoints, /msc and /ps/annual, returning the same ReportingPlanResponse for different NHSN components. This matters beyond tidiness. The contract as published told a consumer that DMRP offers CRUD over reporting plan entries, and someone is coding against it. Contract surface -- /msc and /ps/annual, at the root, in the spec: - The spec now describes only those two operations, and its server URL carries no path component, so DmrpController declares no [Route] of its own. Pointing a consumer at real DMRP becomes a base-URL change and nothing else. - Both take the third party's bearer token, not Link's, matching the real topology where DMRP sits behind a separate authorization server. - Kept deliberately thin -- validate, select by component, project -- because both are placeholders whose shape is expected to change. Support surface -- everything under /mock, hand-written, outside the spec: - MockController carries the CRUD, search and token operations, with the same status-code contract as before, and its own request/response models so that replacing the third party's contract cannot disturb it. - Guarded by Link's standard scheme via AddLinkBearerServiceAuthentication and IsLinkAdmin, as every other Link service is. Anonymous in the local stack, matching the other ten services; enforced everywhere else. - NhsnAuthController is removed; the token endpoint moves here. It hands out a contract-surface credential from behind Link's authentication, so a caller performs the same acquire-then-use sequence it will for real. The two components differ in cadence, and that reaches the schema: - MSC is monthly and carries a reporting month; PS is annual and does not, so ReportingMonth becomes nullable and Component joins the natural key. - Whether a month is required depends on the component, which no column constraint can express, so the service enforces it. Both failure modes are silent otherwise: a PS row with a stray month is returned for every request, and an MSC row without one is returned for none. - The unique index sets HasFilter(null). EF's default for a unique index over a nullable column is "WHERE [ReportingMonth] IS NOT NULL", which would drop every annual row out of the index and permit duplicate patient-safety entries -- the exact thing the index exists to prevent. Nothing is deployed, so the migration is regenerated rather than layered. No new configuration keys. The Link authentication keys are already global in app-config.yaml; the inventory is regenerated for the moved line numbers. Testing performed: - Full solution builds; full ServiceTests suite passes (1927 passed, 1 skipped, 0 failed), of which 236 cover this service. - Unit tests split into contract-surface and support-surface suites. The latter authenticates only when a credential is present, so both the 200 and the 401 paths are exercised; every /mock route is enumerated against an anonymous client. - GeneratedControllerBindingTests retargeted at the two-operation base, confirming that binding sources, [BindRequired] and routes all inherit into the override while default parameter values do not. - Integration tests gain component isolation, null-month round-tripping, and a model-level assertion that the unique index is unfiltered. Noted in the fixture that SQLite treats NULLs as distinct where SQL Server does not, so the service pre-check is what the tests assert. - Verified against the local stack on SQL Server: migration applies, rolls back and re-applies; the shipped index is unique with no filter and ReportingMonth is nullable; a duplicate annual row is rejected with error 2601 while the same measure under another component is accepted. - Exercised end to end over HTTP: seeding both components, all three cadence rejections, 401 without a token, /msc and /ps/annual each returning only their own component, an annual response omitting reportingMonth, an empty plan as 200 with [], 409 on a duplicate, and every old route now 404. Documentation updated: README rewritten around the two-surface split, the route map, the cadence rule, the filtered-index hazard, and the two authentication systems.
Three changes that arrived together, committed as one because each rewrites
the contract the previous one's tests query. Splitting them would leave
intermediate commits that do not build.
Configurable response delay
QA needs to exercise a caller's timeout and retry path, which only happens
against an upstream that is actually slow. PUT /mock/delay sets one, GET
reads it, DELETE clears it.
- Held in memory and never persisted. The delay describes what a test is
doing right now, not how the service is configured, so a forgotten one
must not outlive the run that set it. A restart always clears it.
- It reaches the contract endpoints ONLY. /mock, /health and /api are never
delayed, and that scoping is load-bearing rather than tidy: a delayed
/mock would mean turning a five-minute delay off takes five minutes,
because the endpoint that clears it would be delayed too, and a delayed
/health would push the container past its probe timeout and get it
restarted mid-test.
- The rule is written as "everything except our own namespaced paths"
rather than a list of contract routes, so an endpoint added to the
contract is delayed automatically. That inversion is only safe because
the contract endpoints sit at the root and everything of ours is
prefixed.
- Capped at five minutes, and the wait honours the request's cancellation
token, so a caller that disconnects releases its request instead of
holding it for the full delay.
PUT answers 200 rather than the 202 used elsewhere for updates: the change
has already taken effect by the time the response is written.
Request parameters
Both endpoints take nhsnorgid (required), name, year and month. facilityId
becomes nhsnorgid; reportingMonth and reportingYear become month and year,
and are strings rather than integers. name is new -- the NHSN module.
Only nhsnorgid is required, so the query is filter-based: a caller
supplying nothing else gets the facility's whole plan for that component
across every period. month is accepted on /ps/annual and ignored, because
annual entries carry no month and narrowing by one would exclude every row
the endpoint exists to return.
A year or month that is present but not a whole number, or a month outside
1-12, is a 400. Answering 200 with an empty plan would let a typo convey
"enrolled in nothing", which is the exact conclusion this API exists to
convey and the one it must not convey by accident.
Response body
Reshaped to the ADR's example: psDMRptPlanID, orgid, year, month,
modifyDate, createDate and a plans array of {name, nhsnorgid, month, year,
reporting, rptSeq}.
Two properties of that shape are reproduced deliberately rather than
tidied up, because normalising either would let a consumer write code that
passes here and fails on first contact with the real endpoint -- which is
the one failure this service exists to prevent:
- The same values appear twice with different types. orgid, year and month
are numeric on the root object; nhsnorgid, year and month are strings
inside plans. The generated C# carries both: int? Month on the response,
string Month on the item.
- The timestamps are not RFC 3339. They are 2023-09-09 11:12:12.59 -- a
space separator, two fractional digits, no timezone -- so they are typed
as plain strings and formatted by hand. Binding them as date-time would
emit 2023-09-09T11:12:12.59+00:00, which is well formed and not what a
consumer will have to parse.
One consequence of the numeric orgid: a facility identifier that is not
numeric cannot be represented there and comes back null, though the string
form survives in plans[].nhsnorgid. Link facility identifiers are not
always numeric.
psDMRptPlanID is derived from the query with FNV-1a rather than
string.GetHashCode(), which is randomised per process -- the same query
would otherwise return a different identifier after every restart.
The optional filters bring back a trap that had no live example: NSwag
types them as non-nullable string, which [ApiController] treats as
required, so DmrpController restates all three as nullable.
GeneratedControllerBindingTests regains the probe that demonstrates it and
the one showing the correct shape.
New dependency: Microsoft.Extensions.TimeProvider.Testing, test projects
only, so the delay's unit tests drive a fake clock and a five-minute delay
finishes instantly.
Testing performed:
- Full solution builds; full ServiceTests suite passes (1978 passed, 1
skipped, 0 failed), of which 287 cover this service.
- Delay unit tests drive a FakeTimeProvider: boundaries, clearing mid-wait,
cancellation, and the path predicate in both directions including that
/mocked is still delayed while /mock is not.
- Delay pipeline tests run the middleware in a real pipeline against a real
clock, because a fake one races there -- SendAsync returns before the
request reaches the middleware. Assertions use a lower bound for a
configured delay and a five-minute delay for the never-delayed paths, so
a loaded agent cannot flip either. Ran three times under the full
parallel suite to confirm.
- Contract tests cover each filter combination, the whole-plan case, the
malformed-period rejections, and that month does not narrow the annual
plan.
- Verified end to end against the local stack that the live response is a
field-for-field match with the ADR's example: root values numeric, plans
values strings, timestamps in the real format, plan entry fields exactly
the six documented, reporting only ever Y, and psDMRptPlanID stable
across identical requests. A non-numeric facility returns a null orgid
with the string preserved in plans[].nhsnorgid.
- A 2s delay held /msc for 2.03s while /mock/delay, /mock/search and
/health all answered inside 17ms.
Documentation updated: README gains the parameter table, the two response
quirks and why they must not be normalised, a section on the delay and its
scoping, a note that the components are separately deployed APIs in
production, and a rewritten open-questions list separating what the ADR
settled from what is still guessed.
Every error this service returned was a well-formed problem document and an
unhelpful one. A caller got a status code and, on the paths that set one, a
detail -- but no title worth branching on, no type, and a traceId only where
the framework happened to add one. Several distinct failures were
indistinguishable apart from their prose.
Follows the pattern Terminology established.
AddDmrpProblemDetails, mirroring AddTerminologyProblemDetails:
- A traceId on every problem response, so a report of "it returned 500" can
be traced without asking the reporter to reproduce it.
- A detail on responses that would otherwise carry only a status code, with
wording chosen per status rather than one generic line.
- A 500's detail replaced wholesale rather than filtered, so an exception
message cannot reach a caller by accident.
- An API extension naming the service in Development, or when
ProblemDetails:IncludeExceptionDetails is set; the exception extension is
removed otherwise.
Program.cs also adopts Terminology's split on the exception handler: the
developer page renders a stack trace, which is what you want on a
workstation and never what a caller should receive.
Both controllers now pass a title and a type alongside their detail:
- Invalid Id, Entry Not Found, Missing Facility Id, Id Mismatch, Invalid
Reporting Plan Entry, Duplicate Reporting Plan Entry, Invalid Delay on the
support surface.
- Unauthorized and Invalid Reporting Period on the contract surface.
The type values are named in DmrpProblemTypes rather than inlined, so the
same status always carries the same type. Bare NotFound() calls become
problems that name the id they looked for -- a 404 that does not say what
was missing is indistinguishable from a bad route, which matters on a
service where /mock/{id} sits beside several literal routes.
The 401 on the contract endpoints is deliberately vague about which check
failed. Missing, malformed and expired get one answer, because
distinguishing them helps someone probing the endpoint more than it helps a
caller fixing their client.
Two deliberate exceptions:
- POST /mock/oauth2/token keeps its OAuth 2.0 error shape. It stands in for
an authorization server and client libraries parse those codes; problem
details would break them.
- An oversized delay is still the framework's validation problem. The
[Range] annotation shares its bounds with the service guard, so
[ApiController] rejects the request before the action runs and the
controller's own catch is unreachable over HTTP -- defence in depth for
callers reaching the service directly, not a shape anyone will see. The
test says so rather than asserting a title that cannot occur.
This applies to the contract endpoints too. The real DMRP API has not been
observed to define an error shape, so matching Link's house style is the
better default -- but it is a divergence, and the README says a consumer
should not read these error bodies as evidence of what the real service
returns.
Testing performed:
- Full solution builds; full ServiceTests suite passes (1990 passed, 1
skipped, 0 failed), of which 299 cover this service.
- New ProblemDetailsShapeTests asserts status, title, type, a non-empty
detail and a traceId on every problem both surfaces produce, that a 404
names the id it looked for, that a PUT against an absent entry says it
never creates, and that the two 401 details are identical whether the
token was absent or invalid.
- Also asserts the token endpoint keeps the OAuth shape and carries no
traceId or title, so a future tidy-up cannot quietly convert it.
- Verified against the local stack that the emitted documents match
Terminology's field for field, including type URIs and key order, and that
the customization's fallback detail reaches the framework's own validation
problem.
Documentation updated: README gains an error-responses section with a
worked example, the OAuth exception, and the note about the contract
surface.
Three of the seven audit decisions went against the service. The other four
are edits to the cases and need no code.
Trimming key fields
facilityId, component, measure and isReporting are now trimmed before they
are stored or compared, on writes and on lookups alike.
The sanitizer keeps the space character, so " HOB" and "HOB" were two
distinct rows in the natural key. Both stored happily, and a plan seeded
with the padded one silently omitted the measure a consumer was looking
for -- no error anywhere, just a short plan. That is the failure this
service exists to prevent, arriving through the back door. A padded create
against an existing entry is now a visible 409.
Lookups are trimmed too. Trimming writes alone would have moved the problem
rather than fixed it: a padded query would stop matching the row a padded
create now stores trimmed.
A guard rejects a key field that is empty once trimmed. Trimming opened
that hole -- " " used to be stored verbatim and was merely useless, but
it now trims to "", and an entry with no measure at all satisfies every
other rule here. Found by a test written for the trim, not by inspection.
Paging rejected rather than clamped
pageSize must be 1-100 and pageNumber at least 1. Anything outside that is
a 400 from the request annotations.
Clamping was the friendlier behaviour and the wrong one for a stand-in: a
caller asking for 5,000 rows and receiving 100 has no way to tell that
happened, and a test written against the clamped result would pass while
proving nothing. The service still clamps internally for callers that
bypass HTTP, so a bad value cannot reach the repository as a negative Skip,
and says that is what it is for.
Get-by-facility answers 404
GET /mock/facilities/{facilityId} returns 404 when the facility has no
entries, where it previously returned 204.
The rule is where the identifier sits. A facility named in the path that
matches nothing is an absent resource; GET /mock/search keeps its 204
because its filters are query parameters, so no matches is an empty result
set. A test asserts the two side by side -- if they ever agreed, one of
them would be wrong.
This service keeps no facility registry, so "a facility with no entries"
and "a facility that does not exist" are the same observation. The detail
claims only what it can support: nothing is stored under that identifier.
Delete-by-facility stays an idempotent 204. It is a teardown convenience,
and failing it for an already-empty facility would make cleanup fragile.
Testing performed:
- Full solution builds; full ServiceTests suite passes (2009 passed, 1
skipped, 0 failed), of which 318 cover this service.
- Trimming covered at both levels: every key field trimmed, a padded
measure colliding with its trimmed twin, padded queries and padded
facility paths still finding trimmed rows, and a whitespace-only measure
rejected.
- Paging covered at the boundaries -- 0, -1 and 101 rejected, 1 and 100
accepted -- on both the search and by-facility endpoints.
- The 404 is covered alongside search's 204 in one test, so the distinction
cannot drift.
- Verified against the local stack: a create with " 100 "/" MSC "/" HOB "
stores 100/MSC/HOB, the duplicate is a 409, a whitespace-only measure is
a 400, a padded query finds the trimmed row, pageSize=101 and
pageNumber=-1 are 400 while pageSize=100 is 200, an unknown facility is a
404 naming it, search on the same unknown facility is a 204, and
delete-by-facility on it is still 204.
Documentation updated: README covers trimming, paging rejection, and the
404-versus-204 rule with the reasoning for each.
MikeAtPinnacle
force-pushed
the
users/mtherien/leglink-822/dmrp-mock
branch
from
August 7, 2026 19:24
56e20cf to
c82b366
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
DotNet/ServiceTests/UnitTests/MockDmrpApi/FakeEntryRepository.cs (1)
145-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the null
sortOrdercase match the shared repository.When
sortOrderisnull, this fake sorts descending. If the sharedEntityRepositorydefaults to ascending, a test that omitsSortOrderobserves the wrong order from the fake. Confirm the shared default and mirror it explicitly.#!/bin/bash # Inspect the shared repository SearchAsync sort-order handling. fd -t f -i 'entityrepository.cs' -o -t f -i 'baseentityrepository.cs' 2>/dev/null rg -nP -C6 'SortOrder\s*\?\s*sortOrder' --type=cs -g '!**/ServiceTests/**'🤖 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/MockDmrpApi/FakeEntryRepository.cs` around lines 145 - 147, Update the sort selection in the fake repository method containing the Queryable.OrderBy call so a null sortOrder explicitly follows the shared EntityRepository default, confirmed as ascending. Preserve descending behavior only for SortOrder.Descending and use ascending for null or SortOrder.Ascending.DotNet/ServiceTests/IntegrationTests/MockDmrpApi/ReportingPlanPersistenceTests.cs (1)
230-243: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the resulting order, not only the record count.
The test name states that each sort field orders the results. The assertion only checks that two records return, so it proves SQL translation but not ordering. A reversed comparator would still pass. Add an ordering assertion for at least the deterministic fields.
♻️ Proposed change
var (records, _) = await WithServiceAsync(s => s.SearchAsync(criteria, CancellationToken.None)); records.Should().HaveCount(2); + + if (sortBy == ReportingPlanSortBy.Measure) + { + records.Select(r => r.Measure).Should().BeInAscendingOrder(); + }🤖 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/IntegrationTests/MockDmrpApi/ReportingPlanPersistenceTests.cs` around lines 230 - 243, Update EverySortFieldOrdersAgainstTheDatabase to assert the returned record sequence, not just records.Count. For deterministic sort fields, verify the records are ordered according to the requested ReportingPlanSortBy and ascending SortOrder, while preserving the existing translation/count assertion and avoiding order assertions for nondeterministic fields.DotNet/MockDmrpApi/README.md (1)
161-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code blocks.
markdownlint reports MD040 for the blocks at Line 161, Line 223, and Line 701. Use
textfor the two diagrams andbashfor the build command block.Also applies to: 223-223, 701-701
🤖 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/MockDmrpApi/README.md` at line 161, Add explicit language identifiers to the fenced code blocks in the README: use text for the diagrams at the referenced blocks and bash for the build command block. Ensure all three fences satisfy markdownlint MD040 without changing their contents.Source: Linters/SAST tools
DotNet/ServiceTests/UnitTests/MockDmrpApi/ProblemDetailsShapeTests.cs (1)
70-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the production problem-details registration if it is reachable from the test project.
This host re-implements the
CustomizeProblemDetailshook instead of callingAddDmrpProblemDetails. The tests then pin a copy of the behavior, not the shipped behavior. IfAddDmrpProblemDetailslater drops thetraceIdextension or the fallbackdetail, these tests still pass.The test project already consumes public types from the same assembly (
MockController,ReportingPlanService,DmrpApiSettings). Confirm whetherDmrpProblemDetailsExtensionsis public; if it is, call it here and remove the duplicate hook.#!/bin/bash # Check accessibility and shape of the production problem-details registration. fd -i 'DmrpProblemDetailsExtensions.cs' --exec cat -n {} rg -n 'AddDmrpProblemDetails' -C3🤖 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/MockDmrpApi/ProblemDetailsShapeTests.cs` around lines 70 - 87, Check whether DmrpProblemDetailsExtensions and AddDmrpProblemDetails are publicly accessible from the test project; if so, replace the local CustomizeProblemDetails implementation in the test host with the production registration and remove the duplicated fallback-detail and traceId logic. If the production registration is not reachable, retain the current test setup.DotNet/MockDmrpApi/Presentation/Controllers/DmrpController.cs (1)
113-121: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider bounding the
yearvalue.
TryParsePeriodpassesnullforminandmaxwhen it parsesyear. The method then accepts0,-5, and999999. Each of these reachesGetReportingPlanAsyncand returns an empty plan, which is the exact conclusion the remarks onParseFailuresay the API must not convey by accident.A plausible range (for example, 2000 to 2100) would make a mistyped year a 400 instead of an empty plan.
♻️ Proposed range for the year filter
- if (!TryParsePeriod(year, nameof(year), null, null, out var reportingYear, out var invalidYear)) + if (!TryParsePeriod(year, nameof(year), 2000, 2100, out var reportingYear, out var invalidYear))🤖 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/MockDmrpApi/Presentation/Controllers/DmrpController.cs` around lines 113 - 121, Update the year validation in the controller flow around TryParsePeriod to pass explicit minimum and maximum bounds instead of null values, using the intended supported year range (such as 2000 through 2100). Preserve the existing invalidYear response path so out-of-range years return a validation error before calling GetReportingPlanAsync.DotNet/MockDmrpApi/Application/Services/AuthTokenService.cs (1)
143-153: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueCompare fixed-length digests instead of raw credential bytes.
CryptographicOperations.FixedTimeEqualsreturns immediately when the two spans have different lengths. The comparison is therefore constant-time only for equal-length inputs, and the runtime still reveals the configured secret length. Hash both values to a fixed size first, then compare the digests.🔒 Proposed fix
private static bool MatchesConfigured(string supplied, string? configured) { if (string.IsNullOrEmpty(configured)) { return false; } return CryptographicOperations.FixedTimeEquals( - Encoding.UTF8.GetBytes(supplied), - Encoding.UTF8.GetBytes(configured)); + SHA256.HashData(Encoding.UTF8.GetBytes(supplied)), + SHA256.HashData(Encoding.UTF8.GetBytes(configured))); }🤖 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/MockDmrpApi/Application/Services/AuthTokenService.cs` around lines 143 - 153, Update MatchesConfigured to hash both supplied and configured credential values with the established cryptographic hash before calling CryptographicOperations.FixedTimeEquals, ensuring the compared digests are always fixed length. Preserve the existing false result for null or empty configured values.DotNet/MockDmrpApi/Application/Models/MockTokenModels.cs (1)
25-52: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin the OAuth field names with
[JsonPropertyName].The snake_case wire format currently depends on the default camelCase naming policy, which lowercases only the first character. If the service ever sets
PropertyNamingPolicy = null, or switches to Newtonsoft.Json, these fields serialize asGrant_typeandAccess_token, and OAuth clients break. Explicit attributes make the contract independent of the global policy.♻️ Proposed change (same pattern for every field)
+using System.Text.Json.Serialization; + [Required] + [JsonPropertyName("grant_type")] public string Grant_type { get; set; } = string.Empty; [Required] [StringLength(200)] + [JsonPropertyName("client_id")] public string Client_id { get; set; } = string.Empty;#!/bin/bash # Description: Check the JSON serializer configuration for MockDmrpApi. fd -t f 'Program.cs' -p 'MockDmrpApi' --exec rg -n 'AddControllers|JsonOptions|PropertyNamingPolicy|NewtonsoftJson' {}🤖 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/MockDmrpApi/Application/Models/MockTokenModels.cs` around lines 25 - 52, Apply explicit JsonPropertyName attributes to every OAuth DTO property in MockTokenRequest, MockTokenResponse, and MockTokenErrorResponse, using the required snake_case wire names such as grant_type, client_id, access_token, expires_in, issued_at, and error_description. Ensure all fields’ serialized names are independent of the global JSON naming policy.DotNet/MockDmrpApi/Application/Services/ReportingPlanService.cs (1)
179-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the cadence documentation onto
GuardComponentAndPeriod.Two
<summary>blocks now precedeTrim. The first block describes the cadence guard, but it documentsTriminstead.GuardComponentAndPeriodat line 243 has no documentation. The compiler also reports CS1571 for the duplicate tag.♻️ Proposed documentation move
- /// <summary> - /// Rejects an entry whose reporting period does not match its component's cadence. - /// </summary> - /// <remarks> - /// This cannot be a column constraint or a range annotation, because whether a month is - /// required depends on the component. It has to be enforced, not merely documented: a - /// patient-safety entry saved with a stray month satisfies the unique index perfectly - /// well, but the annual query does not filter on month, so the row would be returned for - /// every month -- or, with the month wrong on a monthly entry, returned for none. Both - /// failures are silent. - /// </remarks> /// <summary> /// Trims a value that takes part in the natural key. /// </summary>Then add the removed block directly above
GuardComponentAndPeriod.🤖 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/MockDmrpApi/Application/Services/ReportingPlanService.cs` around lines 179 - 200, Move the cadence-related XML documentation block from immediately before Trim to directly above GuardComponentAndPeriod. Leave Trim documented only by its trimming summary and remarks, preserving a single summary per member and eliminating the duplicate XML summary warning.DotNet/MockDmrpApi/Dockerfile (1)
1-5: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the repository's standard runtime base image.
Other service Dockerfiles in this repository use
lantanagroup/aspnet8-runand its runtime-user pattern. This file uses the upstreammcr.microsoft.com/dotnet/aspnet:8.0-jammy-amd64image and declares noUSER, so the container runs as root. Align the base stage with the convention, or state why this service must differ.Based on learnings: "Service Dockerfiles in the lantanagroup/link-cloud repository should retain the existing
lantanagroup/aspnet8-runbase image and runtime-user pattern for consistency across services."#!/bin/bash # Description: Compare the base images used across service Dockerfiles. fd -t f 'Dockerfile' --exec sh -c 'echo "== $1"; rg -n "^FROM|^USER" "$1"' _ {}🤖 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/MockDmrpApi/Dockerfile` around lines 1 - 5, Update the Dockerfile’s base stage to use the repository-standard lantanagroup/aspnet8-run image and apply its established runtime-user pattern, including the appropriate USER declaration. Preserve the curl installation and existing application setup.
🤖 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 `@Azure_Pipelines/azure-pipelines.mockdmrpapi.cd.yaml`:
- Line 97: Remove the condition: always() from the Docker push step, or replace
it with the default succeeded() behavior, so the push runs only when preceding
dotnet test steps succeed and failed-test commits cannot publish latest or
$(MyTag) images.
In `@DotNet/MockDmrpApi/Application/Extensions/DmrpProblemDetailsExtensions.cs`:
- Around line 87-94: Update the API extension assignment in
CustomizeProblemDetails to use the Extensions indexer instead of Add, ensuring
repeated customization safely overwrites an existing API key without throwing.
In `@DotNet/MockDmrpApi/Contracts/dmrp-openapi.yaml`:
- Line 101: Update both descriptions in
DotNet/MockDmrpApi/Contracts/dmrp-openapi.yaml at lines 101-101 and 133-133 to
state that plans may be empty instead of measures.
In `@DotNet/MockDmrpApi/Presentation/Controllers/MockController.cs`:
- Around line 34-38: Update MockController’s route from “mock” to the
service-prefixed entries resource route “api/mock-dmrp/entries”, preserving the
existing authorization and response metadata. Ensure the controller’s entry
operations use this plural collection path, while keeping delay and oauth2/token
on their sibling routes; update all corresponding Postman collections, HTTP
tests, and README references.
In `@DotNet/MockDmrpApi/Program.cs`:
- Around line 74-92: Register
AddDbContextCheck<ReportingPlanDbContext>("database") only when enabled is true,
while keeping AddHealthChecks available in all modes so /health remains
responsive for disabled instances. Update the startup service-registration flow
before builder.Build and preserve the existing migration and disabled-instance
behavior.
In `@DotNet/MockDmrpApi/README.md`:
- Around line 296-298: Update DotNet/MockDmrpApi/README.md to use the current
contract vocabulary: replace measures with plans in the response descriptions at
lines 297 and 336, and replace ReportingPlanMeasure with ReportingPlanItem in
the generated-DTO list at lines 168-169.
In `@DotNet/MockDmrpApi/Settings/DmrpApiSettings.cs`:
- Around line 11-28: Update DmrpApiSettings to default Enabled to false and
remove committed credential and signing-key defaults, requiring explicit
configuration for AuthClientSecret and SigningKey; enable the API explicitly in
the Development and Docker environment settings, and update app-config.yaml
entries to required: false with any retained defaults recorded in defaultValue.
In `@DotNet/ServiceTests/UnitTests/MockDmrpApi/DmrpAvailabilityTests.cs`:
- Around line 55-63: Remove the trailing space from the alternate configuration
key in the configuration dictionary used by DmrpAvailability.IsEnabled,
replacing it with a valid differently-cased variant of
DmrpAvailability.EnabledConfigurationKey so the test exercises a real alternate
signal.
In `@DotNet/ServiceTests/UnitTests/MockDmrpApi/ResponseDelayPipelineTests.cs`:
- Around line 161-167: Update
WithNoDelayConfigured_TheContractEndpointAnswersImmediately to send one
unmeasured request through GetPlanAsync before calling ElapsedAsync, so the
assertion measures a warmed pipeline while preserving the existing LowerBound
check.
---
Nitpick comments:
In `@DotNet/MockDmrpApi/Application/Models/MockTokenModels.cs`:
- Around line 25-52: Apply explicit JsonPropertyName attributes to every OAuth
DTO property in MockTokenRequest, MockTokenResponse, and MockTokenErrorResponse,
using the required snake_case wire names such as grant_type, client_id,
access_token, expires_in, issued_at, and error_description. Ensure all fields’
serialized names are independent of the global JSON naming policy.
In `@DotNet/MockDmrpApi/Application/Services/AuthTokenService.cs`:
- Around line 143-153: Update MatchesConfigured to hash both supplied and
configured credential values with the established cryptographic hash before
calling CryptographicOperations.FixedTimeEquals, ensuring the compared digests
are always fixed length. Preserve the existing false result for null or empty
configured values.
In `@DotNet/MockDmrpApi/Application/Services/ReportingPlanService.cs`:
- Around line 179-200: Move the cadence-related XML documentation block from
immediately before Trim to directly above GuardComponentAndPeriod. Leave Trim
documented only by its trimming summary and remarks, preserving a single summary
per member and eliminating the duplicate XML summary warning.
In `@DotNet/MockDmrpApi/Dockerfile`:
- Around line 1-5: Update the Dockerfile’s base stage to use the
repository-standard lantanagroup/aspnet8-run image and apply its established
runtime-user pattern, including the appropriate USER declaration. Preserve the
curl installation and existing application setup.
In `@DotNet/MockDmrpApi/Presentation/Controllers/DmrpController.cs`:
- Around line 113-121: Update the year validation in the controller flow around
TryParsePeriod to pass explicit minimum and maximum bounds instead of null
values, using the intended supported year range (such as 2000 through 2100).
Preserve the existing invalidYear response path so out-of-range years return a
validation error before calling GetReportingPlanAsync.
In `@DotNet/MockDmrpApi/README.md`:
- Line 161: Add explicit language identifiers to the fenced code blocks in the
README: use text for the diagrams at the referenced blocks and bash for the
build command block. Ensure all three fences satisfy markdownlint MD040 without
changing their contents.
In
`@DotNet/ServiceTests/IntegrationTests/MockDmrpApi/ReportingPlanPersistenceTests.cs`:
- Around line 230-243: Update EverySortFieldOrdersAgainstTheDatabase to assert
the returned record sequence, not just records.Count. For deterministic sort
fields, verify the records are ordered according to the requested
ReportingPlanSortBy and ascending SortOrder, while preserving the existing
translation/count assertion and avoiding order assertions for nondeterministic
fields.
In `@DotNet/ServiceTests/UnitTests/MockDmrpApi/FakeEntryRepository.cs`:
- Around line 145-147: Update the sort selection in the fake repository method
containing the Queryable.OrderBy call so a null sortOrder explicitly follows the
shared EntityRepository default, confirmed as ascending. Preserve descending
behavior only for SortOrder.Descending and use ascending for null or
SortOrder.Ascending.
In `@DotNet/ServiceTests/UnitTests/MockDmrpApi/ProblemDetailsShapeTests.cs`:
- Around line 70-87: Check whether DmrpProblemDetailsExtensions and
AddDmrpProblemDetails are publicly accessible from the test project; if so,
replace the local CustomizeProblemDetails implementation in the test host with
the production registration and remove the duplicated fallback-detail and
traceId logic. If the production registration is not reachable, retain the
current test setup.
🪄 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: 84b13743-29cd-4c8d-a156-b7ec57f563e5
📒 Files selected for processing (65)
.docker/create-dbs.sqlAzure_Pipelines/_deploy_all_services.ymlAzure_Pipelines/azure-pipelines.mockdmrpapi.cd.yamlDirectory.Packages.propsDotNet/MockDmrpApi/Application/Extensions/DmrpProblemDetailsExtensions.csDotNet/MockDmrpApi/Application/Mapping/EntryMapper.csDotNet/MockDmrpApi/Application/Middleware/DmrpAvailability.csDotNet/MockDmrpApi/Application/Middleware/DmrpDisabledMiddleware.csDotNet/MockDmrpApi/Application/Middleware/ResponseDelayMiddleware.csDotNet/MockDmrpApi/Application/Models/MockDelayModels.csDotNet/MockDmrpApi/Application/Models/MockEntryModels.csDotNet/MockDmrpApi/Application/Models/MockTokenModels.csDotNet/MockDmrpApi/Application/Models/ReportingPlanSearchCriteria.csDotNet/MockDmrpApi/Application/Services/AuthTokenService.csDotNet/MockDmrpApi/Application/Services/IAuthTokenService.csDotNet/MockDmrpApi/Application/Services/IReportingPlanService.csDotNet/MockDmrpApi/Application/Services/IResponseDelayService.csDotNet/MockDmrpApi/Application/Services/ReportingPlanService.csDotNet/MockDmrpApi/Application/Services/ResponseDelayService.csDotNet/MockDmrpApi/Contracts/dmrp-openapi.yamlDotNet/MockDmrpApi/DockerfileDotNet/MockDmrpApi/Domain/Context/Mappings/ReportingPlanEntryMap.csDotNet/MockDmrpApi/Domain/Context/ReportingPlanDbContext.csDotNet/MockDmrpApi/Domain/Entities/ReportingPlanEntryEntity.csDotNet/MockDmrpApi/Migrations/20260806172339_InitMockDmrp.Designer.csDotNet/MockDmrpApi/Migrations/20260806172339_InitMockDmrp.csDotNet/MockDmrpApi/Migrations/ReportingPlanDbContextModelSnapshot.csDotNet/MockDmrpApi/MockDmrpApi.csprojDotNet/MockDmrpApi/Presentation/Controllers/DmrpController.csDotNet/MockDmrpApi/Presentation/Controllers/MockController.csDotNet/MockDmrpApi/Program.csDotNet/MockDmrpApi/Properties/launchSettings.jsonDotNet/MockDmrpApi/README.mdDotNet/MockDmrpApi/Settings/DmrpApiConstants.csDotNet/MockDmrpApi/Settings/DmrpApiSettings.csDotNet/MockDmrpApi/appsettings.Development.jsonDotNet/MockDmrpApi/appsettings.Docker.jsonDotNet/MockDmrpApi/appsettings.Production.jsonDotNet/MockDmrpApi/appsettings.jsonDotNet/MockDmrpApi/nswag.jsonDotNet/MockDmrpApi/packages.lock.jsonDotNet/ServiceTests/IntegrationTests/IntegrationTestCollection.csDotNet/ServiceTests/IntegrationTests/MockDmrpApi/MockDmrpApiIntegrationTestFixture.csDotNet/ServiceTests/IntegrationTests/MockDmrpApi/ReportingPlanPersistenceTests.csDotNet/ServiceTests/IntegrationTests/MockDmrpApi/ReportingPlanSemanticsTests.csDotNet/ServiceTests/ServiceTests.csprojDotNet/ServiceTests/UnitTests/MockDmrpApi/AuthTokenServiceTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/DmrpAvailabilityTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/DmrpControllerTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/DmrpDisabledMiddlewareTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/EntryMapperTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/FakeEntryRepository.csDotNet/ServiceTests/UnitTests/MockDmrpApi/GeneratedControllerBindingTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/MockControllerTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/MockEntryMapperTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/ProblemDetailsShapeTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/ReportingPlanServiceTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/ResponseDelayPipelineTests.csDotNet/ServiceTests/UnitTests/MockDmrpApi/ResponseDelayServiceTests.csDotNet/ServiceTests/packages.lock.jsonScripts/set_kubernetes_services.batapp-config.yamldocker-compose.ymldocs/config-key-inventory.mdlink-cloud.sln
Raised by CodeRabbit on PR #1800. The Docker buildAndPush step carried `condition: always()`, directly after the step that runs the test suite. A failed test run would still publish the image -- tagged both `$(MyTag)` and `latest`, the tag every environment pulls by default. Removing the condition restores the default `succeeded()`, so a red build stops before the push. The condition was copied from the sibling CD pipelines rather than written for this one; a comment now records why this pipeline differs, so it does not get "corrected" back into line with them. Worth a separate ticket: 17 of the 19 CD pipelines carry the same condition, and the four spot-checked (terminology, census, tenant, account) all run a test step before the push. Left alone here to keep this change minimal. Testing performed: the pipeline YAML parses, and every step now resolves to the default succeeded() condition, verified by walking the parsed step list.
Raised by CodeRabbit on PR #1800. IDictionary.Add throws ArgumentException when the key is already present, and nothing here owns the extensions dictionary exclusively. Throwing while building an error response is the worst place to throw -- it replaces a useful 404 with an opaque 500. The indexer overwrites instead, and rewriting our own value with the same value is harmless. The file was already inconsistent about this: traceId a few lines above is guarded with ContainsKey, this key was not. No duplicate-key throw was reproduced -- each request builds its own ProblemDetails, so the customization runs once per instance. This is hardening against a second pass, not a fix for an observed crash. Verified by running the service with ProblemDetails:IncludeExceptionDetails enabled, which reaches this branch for the first time; it was never exercised before, since it is skipped outside Development unless that flag is set. Both traceId and API are present and correctly shaped, and repeated requests stay 400 rather than 500. 318 MockDmrpApi tests pass.
…ract Raised by CodeRabbit on PR #1800. Both 200 descriptions still referred to a `measures` array. The schema has no such field -- the array is `plans`. The two descriptions were left behind when the response was realigned to the ADR and `measures` became `plans`. Not purely cosmetic: NSwag copies the response description into the generated abstract base as the <returns> doc comment, so the stale name was visible to anyone implementing against it. The remaining "measure" in the file is the NHSN measure a facility is enrolled in, which is correct and unchanged. Verified by rebuilding, which regenerates from the spec: both <returns> comments in obj/NSwag/DmrpApi.Generated.cs now read `plans`. 318 MockDmrpApi tests pass.
Raised by CodeRabbit on PR #1800. MockController sat at [Route("mock")] -- the only production controller in the repo outside api/. The three other non-api routes are test-host fixtures in ServiceTests. The service also already served /api/mock-dmrp/info, so it straddled two bases. POST /mock -> /api/mock-dmrp/entries GET PUT DEL /mock/{id} -> .../entries/{id} GET /mock/search -> .../entries/search GET DEL /mock/facilities/{id} -> .../facilities/{id}/entries GET PUT DEL /mock/delay -> .../delay POST /mock/oauth2/token -> .../oauth2/token The facility routes nest entries under the facility rather than the reverse, following the /facilities/{id}/configs shape the API guidance documents. Fixing one convention breach by introducing an unidiomatic path would be a poor trade. The contract surface does not move. /msc and /ps/annual stay at the root so that repointing a consumer at the real DMRP is a base-URL change, and the support surface no longer squats a root segment the real service might use. Dropped "/mock" from the delay middleware's exemption list: "/api" already covers the whole support surface, including the endpoints that clear a delay. Three test changes that are not mechanical: - AppliesTo("/MOCK/delay") asserted BeFalse. Uppercase, so no rename rule touched it, and it would have started failing once /mock left the exemption list. Now /API/Mock-Dmrp/delay. - AppliesTo("/mocked") guarded a StartsWithSegments boundary that no longer exists. It would still have passed while testing nothing. Retargeted to /apiary against /api, which is the live hazard. - DmrpAvailabilityTests gained cases for /api/mock-dmrp/entries and its siblings. They share a prefix with the exempt .../info, so a sloppy StartsWithSegments("/api") would now fail a test rather than quietly open the support surface on a disabled deployment. DmrpControllerTests still asserts /dmrp/mock/msc returns 404 -- the pre- refactor prefix must stay gone. It is the only /dmrp/mock reference left. Verified with 320 unit tests (up 2, from the new availability cases) and against a live container: the old paths 404, every new path answers, the Location header follows CreatedAtAction automatically, and /msc, /ps/annual, /health and /info are unmoved. The Postman collection and the TestRail case audit were updated to match.
… creates Raised by CodeRabbit on PR #1800. A disabled deployment deliberately skips EF migration, but the only registered health check probed the DbContext regardless. So /health on a disabled instance answered 503 Unhealthy -- and under a Kubernetes or ACA probe that is a restart loop, which is exactly the "looks like an outage" failure the disabled path exists to prevent. The comment three lines below already claimed the container "reports healthy rather than looking like an outage"; the code disagreed with it, and the comment was right. AddHealthChecks stays unconditional so /health always answers. Only the database check moves inside the enabled branch. Migration and every other disabled-instance behaviour are untouched. Reproduced against a container with Enabled=false pointing at a database that was never created: before /health -> 503 {"status":"Unhealthy","entries":{"database":...}} after /health -> 200 {"status":"Healthy","entries":{}} and with Enabled=true against the same connection string: /health -> 200 {"status":"Healthy","entries":{"database":...}} The entries payload is what shows the check is conditional rather than removed: present and executing when enabled, absent when disabled. The enabled run reports Healthy because AutoMigrateEF creates the schema, which is that path working as designed. /msc stays 503 while disabled either way. Not covered by a unit test. The behaviour lives in Program.cs top-level statements, which ServiceTests cannot reach without a WebApplicationFactory and a bootable database, so this was verified by container in both modes. 320 existing tests still pass.
Raised by CodeRabbit on PR #1800. Three leftovers from the ADR realignment, which renamed the response array `measures` to `plans` and the item type ReportingPlanMeasure to ReportingPlanItem. Same root cause as 6ac44df, which fixed the two descriptions in the contract itself. - The generated-DTO list named ReportingPlanMeasure. That type exists nowhere in the repo; the generated DTOs are ReportingPlanResponse and ReportingPlanItem. - Two places described an empty `measures` array. The field is `plans`. Two other occurrences of "measures" are left alone deliberately. "the set of measures a facility is enrolled to report" and "each returns only its own component's measures" are domain prose about NHSN measures, not the field name -- the array is `plans` and its contents are measures. Renaming those would make the README wrong in the other direction. Since this is the second stale-name finding, the whole file was checked rather than the three reported lines: every backticked field name resolves to a field in dmrp-openapi.yaml, and every ReportingPlan* type resolves -- two in the generated code, five hand-written. Documentation only. 320 tests still pass.
Raised by CodeRabbit on PR #1800. AuthClientSecret and SigningKey shipped with working defaults compiled into DmrpApiSettings, so a deployed environment that failed to provision them would silently sign tokens with a value published in this repository -- anyone reading the repo could forge a token for that environment. Both now default to string.Empty. The workstation values move to appsettings.Development.json, where a known value is harmless; docker-compose already supplies its own and parameterises all three for override. Nothing is deployed yet, so this is the cheapest possible moment to make the change. AuthClientId keeps its default. It is an identifier, not a credential, and the catalog already records it in defaultValue. Catalog descriptions for both keys corrected to say there is no longer a code default. Left required: false deliberately -- no environment has been provisioned, so required: true would fail check_required_config.py against all three stores and turn this PR red. Both descriptions now say to make them required once rows exist. Verified against a container. With no signing key the token endpoint and /msc answer 500 and the log names the key: System.InvalidOperationException: MockDmrpApi:SigningKey must be at least 64 bytes to sign with HMAC-SHA512; it is 0. With the key supplied as docker-compose supplies it, both answer 200. No test relied on either default -- all five fixtures set them explicitly. 320 tests pass, and both catalog checks are green. Two things the verification turned up, recorded rather than changed: - The failure is NOT at startup, which both the old catalog description and the first draft of these doc comments claimed. AuthTokenService is a lazily-constructed singleton, so a deployment missing the key starts, reports healthy, and only fails when a token is first issued or validated. Making it eager would be wrong for a disabled deployment, which must stay dormant rather than crash -- see 01644e4. Both descriptions now state where the failure actually surfaces. - DmrpApiSettings.Enabled is never read. The availability decision comes from DmrpAvailability.IsEnabled, which reads configuration directly and also applies the Production block. The property carries a doc warning now, because reading it would bypass that block.
Raised by CodeRabbit on PR #1800. The test is named "even when every other signal says otherwise", but its second signal was ["MockDmrpApi:Enabled "] -- with a trailing space. Configuration keys are not whitespace-trimmed, so that is a key nothing reads. The test was passing on one signal while claiming two. The suggested remedy, a differently-cased variant, does not work. Keys are case-insensitive, so a case variant is the SAME key, and MemoryConfigurationProvider copies into an OrdinalIgnoreCase dictionary with Add, which rejects the duplicate: System.ArgumentException : An item with the same key has already been added. Key: mockdmrpapi:enabled Ranking sources is the only way one key can arrive by two routes, and it is also the hazard DmrpAvailability's own remarks describe. The test now layers two in-memory sources: a base standing in for appsettings saying false, and a second appended after it standing in for Azure App Configuration saying true. Without the environment block the later source would win and the mock would run in production. Added an assertion that the later source really does win. Without it the test could go vacuous again exactly as it had -- passing because nothing reached the code under test rather than because the code is right. Verified by mutation: with the IsProduction block disabled this test fails, along with the three theory cases that cover the same guarantee. Restored and re-run; 320 tests pass.
Raised by CodeRabbit on PR #1800. WithNoDelayConfigured_TheContractEndpointAnswersImmediately measured the very first request through a freshly built host against an upper bound of 300ms. That first request pays for JIT, routing and the auth handler before any of this service's own code runs. Measured by forcing the elapsed value into the assertion message, three runs each, on an unloaded workstation: cold 219ms 256ms 264ms margin of 36-81ms against the 300ms bound warm 3ms 4ms 4ms margin of ~296ms So it was passing with as little as 12% headroom. A loaded CI agent would have broken it intermittently, and the failure would have read as a regression in the delay middleware rather than as a cold start. The fix is one unmeasured request before the measured one. The bound itself is unchanged. This is the only upper-bound assertion in the file, which is why it is the only one affected: every other test asserts a lower bound, which a slow agent can only overshoot, or measures against a five-minute delay. Proving that nothing holds a request is inherently an upper bound, so it needs the warm-up instead. The class remarks claimed every assertion here was written to survive a slow agent. That was true of all of them except this one. They now say so, and carry the measured numbers so the next person does not have to rediscover them. Verified with the class run three times consecutively and the full suite: 320 tests pass.
Raised by CodeRabbit on PR #1800. Three opening fences carried no language: the NSwag codegen flow diagram -> text the dotnet build command -> bash the project structure tree -> text Contents are unchanged; only the fence markers. The finding cites markdownlint MD040, but markdownlint is not set up in this repository -- no config file, not in CI, not in .coderabbit.yaml -- so nothing was going to flag these. The change is still worth making for a different reason: an unlabelled fence leaves GitHub to guess the language, and it guesses badly on ASCII diagrams, colouring box-drawing characters as though they were syntax. Marking them text turns that off. Verified by pairing fences rather than counting them, since closing fences never carry a language and a naive count reports twelve false positives. All 12 pairs now have a language on the opening fence, no closing fence carries stray text, and the diff is exactly the three markers.
MikeAtPinnacle
marked this pull request as ready for review
August 7, 2026 21:17
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
Adds
MockDmrpApi, a stand-in for the third-party DMRP API that Leidos is building for CDC, so local development and the LCG environments can exercise reporting-plan integration without reaching the CDC Dev network.GET /msc,GET /ps/annual/mockContracts/dmrp-openapi.yaml?IsLinkAdmin)A consumer integrates against the contract surface only. The support surface exists so a test can set up an exact scenario, and it is kept out of the contract so that replacing that document with Leidos's published one cannot disturb it.
Contract-first with NSwag
Contracts/dmrp-openapi.yamldrives codegen at build time intoobj/NSwag/, which is gitignored — the C# and the spec cannot drift, and a contract change reviews as a spec diff rather than thousands of lines of mechanical churn.Persistence
MockDmrpEntriesin a newlink-mock-dmrpcatalog, EF Core, with anInitMockDmrpmigration verified to apply, roll back and re-apply.(facilityId, component, reportingYear, reportingMonth, measure), unique.HasFilter(null). EF's default for a unique index over a nullable column isWHERE [ReportingMonth] IS NOT NULL, which would drop every annual row out of the index and silently permit duplicate patient-safety entries.Support surface
CRUD, filtered search, an OAuth2-shaped token endpoint that issues the credential the contract endpoints accept, and configurable response-delay endpoints for timeout and retry testing.
Availability
MockDmrpApi:Enableddisables the service, and production never serves it whatever configuration says. Azure App Configuration is appended last in the chain, so a row provisioned against a production label would silently outrank appsettings and environment variables — and a running mock looks exactly like a healthy service, so that failure would be invisible. When disabled, every route answers 503,/healthand/api/mock-dmrp/infokeep answering, and migration is skipped.Also included
docker-compose.ymlon port 6159, a CD pipeline,app-config.yamlentries, and a regenerated config-key inventory.Three things that look like bugs and are not:
orgid/year/monthare numeric on the root object,nhsnorgid/year/monthare strings insideplans. That is how the real API behaves.2023-09-09 11:12:12.59, so they are typed as strings and formatted by hand. Binding them as dates would emit ISO 8601, which is well formed and not what a consumer will have to parse./mockor/health— a delayed/mockwould mean turning a five-minute delay off takes five minutes, and a delayed/healthwould get the container restarted mid-test.Normalising any of these would let a consumer write code that passes here and fails on first contact with the real endpoint, which is the one failure this service exists to prevent.
🧪 Testing Performed
Automated — full solution builds; the full
ServiceTestssuite passes (2009 passed, 1 skipped, 0 failed), of which 318 cover this service.Against the local docker-compose stack, verified by hand:
ReportingMonthis nullable.plansvalues strings, timestamps in the real format,reportingonly everY, andpsDMRptPlanIDstable across identical requests. A non-numeric facility returns a nullorgidwith the string preserved inplans[].nhsnorgid.nhsnorgid/name/year/month, includingnhsnorgidalone returning the whole plan, and malformed periods returning 400 rather than an empty plan./mscfor 2.03s while/mock/delay,/mock/searchand/healthall answered inside 17ms./mockanswers 401 unauthenticated,/healthand/api/mock-dmrp/infostay open, and the contract endpoints keep working with a third-party token — the two schemes do not interfere.Postman — the
Mock DMRP APIcollection in the BOTW workspace runs top to bottom against a live instance, with assertions on the response quirks, component isolation, the cadence rule, absence semantics, token errors and delay scoping.🧑🔬 Unit Testing
Worth calling out, because each pins something that fails silently otherwise:
GeneratedControllerBindingTests— pins how the NSwag base behaves once overridden. Binding sources,[BindRequired]and routes do inherit; default values and nullability do not, so an optional filter left as generated becomes mandatory and returns 400 for requests the contract says are valid.ProblemDetailsShapeTests— every problem carries status, title, type, a non-empty detail and a traceId; the token endpoint keeps its OAuth shape so a future tidy-up cannot quietly convert it.ResponseDelayPipelineTests— proves the delay reaches the contract endpoints and never/mockor/health.ReportingPlanSemanticsTests— component isolation and absence semantics against a real database.Two notes on test technique. The delay's unit tests drive a
FakeTimeProviderso a five-minute delay finishes instantly; its pipeline tests use the real clock, because a fake one races throughTestServer— assertions there use a lower bound for a configured delay and a five-minute delay for the never-delayed paths, so a loaded agent cannot flip either. Ran repeatedly under the full parallel suite to confirm.New test-only dependency:
Microsoft.Extensions.TimeProvider.Testing.📓 Documentation Updated
DotNet/MockDmrpApi/README.md(~730 lines) — the two surfaces, the contract-first workflow and how to replace the spec, the query parameters, the response quirks and why they must not be tidied up, the cadence rule, the filtered-index hazard, the availability switch, the two authentication systems, error responses, and an open-questions list separating what the ADR settled from what is still guessed.app-config.yaml— service entries; the Link authentication keys are already global, so nothing service-specific was added for them.docs/config-key-inventory.md— regenerated.Summary by CodeRabbit