Commit fd85725
authored
LEGLINK-888: ValueSet $validate-code rejects a blank "system" with a 400 (#1794)
* LEGLINK-888: ValueSet $validate-code rejects a blank "system" with a 400
An empty "system" was folded into the absent case by string.IsNullOrEmpty and answered
by searching every code system in the value set, so TestRail 10992's request returned
200 result=true. An absent system is legitimate FHIR and keeps that meaning; an empty
string is not valid for any FHIR primitive, so the request is malformed and is now
rejected rather than reinterpreted into a broader question than the caller asked.
- Adds NormalizeSystem and validates every client-supplied system up front in
ValidateCodeInValueSet: the query parameter, the body "system" parameter,
coding.system and each codeableConcept.coding.system. Validating before the merges
matters because they treat an empty string as "not supplied" and would overwrite a
blank before anything could see it, and it keeps the verdict for a malformed
codeableConcept independent of which coding happens to match first.
- Whitespace-only is treated as blank, matching the FHIR rule that a primitive carries
at least one character of non-whitespace content.
- The literal "null" and "undefined" are treated as an omitted parameter. This is
deliberate leniency for clients that interpolate an unset variable into a request
rather than FHIR behavior, so it is pinned by tests.
- The string.IsNullOrEmpty check in ValidateCodeInCodeGroup is left alone: it is shared
with ValidateCodeInCodeSystem, which passes the code group's own Url there, and that
is cache content rather than client input.
- Registers PreserveEmptyStringMetadataProvider. MVC converts an empty query value to
null before an action runs, so "?system=" reached the action indistinguishable from an
omitted parameter and returned 200 regardless of the validation above. MvcOptions
exposes no setting for this and DisplayFormatAttribute cannot target a parameter, so a
display metadata provider is the supported route. Body binding is unaffected;
FhirModelBinder deserializes with System.Text.Json and never consults MVC metadata.
- Adds FhirControllerHttpTests, which drives the four QA requests (TestRail 10992,
11015, 11342, 11343) over real HTTP through the configured binding pipeline. Calling
the action directly cannot reach the query-string case: a direct call passing
string.Empty exercises a state real traffic cannot produce.
Testing: 97 unit tests pass in UnitTests.Terminology, up from 77, 20 of them new; the
full .NET unit suite passes at 1128. Confirmed the metadata provider is load-bearing by
removing it and observing only the "?system=" test fail. Verified against the local
docker-compose stack: before the change all four QA requests returned 200 result=true;
after rebuilding the image each returns application/problem+json carrying type, title,
status, the expected detail and a W3C traceId. An omitted system and a valid system are
unchanged at 200; "?system=null" now searches all systems rather than being looked up as
a system URL, so it returns result=true where it previously returned result=false.
* LEGLINK-888: Trim a client-supplied "system" before matching it
CodeRabbit finding on PR #1794. NormalizeSystem returned the value verbatim, but the lookup in
ValidateCodeInSystem is an exact dictionary match, so a whitespace-padded " http://x " answered
"Code system not found in ValueSet" for a system that is present -- the same confidently-wrong
answer to a malformed request that this method exists to prevent. A padded placeholder likewise
failed to resolve to null.
Reproduced against the running service before changing anything: "?system=%20<sys>%20", a padded
coding.system in the body, and "?system=%20null%20" all returned result=false with "Code system
not found in ValueSet".
- Trims after the whitespace-only guard, so " " is still rejected with a 400 rather than
trimmed to empty and slipping through as "not supplied".
- The trimmed value is what gets placeholder-matched and returned, so " null " resolves to null
and " http://x " matches the loaded code system.
Padded values are well-formed FHIR rather than malformed input, so this is normalization and not
the silent repair of a bad request that LEGLINK-888 otherwise argues against: the Firely
validating deserializer accepts " http://hl7.org/fhir/address-type " without complaint.
Not addressed here: "url", "code" and "display" share the same exact-match behavior --
"?url=%20<valueset>%20" returns "Value set not found". Widening the treatment to those changes
behavior across the endpoint and belongs in its own ticket with QA coverage.
Testing: 1188 unit tests pass, 3 of them new -- a padded system still matching its code system,
and the placeholder theory extended with " null " and "\tundefined\t".
* LEGLINK-888: Assert the validation verdict instead of matching the payload text
CodeRabbit finding on PR #1794. Both 200-response tests in FhirControllerHttpTests checked the
raw payload with Assert.Contains rather than reading the result parameter.
The placeholder test was the one that mattered. It asserted only that the body contained
"result", never the value -- but "?system=null" being treated as an omitted parameter (search
every system, code found, result=true) differs from it being looked up as a code system literally
named "null" (result=false) in exactly that boolean. The assertion passed either way, so a test
named TreatedAsAbsent was not testing that the value was treated as absent. That leniency is the
one deliberately non-FHIR behaviour in this change and has no TestRail coverage, so this unit
test was its only guard.
- Adds AssertValidationResult beside AssertBadRequestDetail: parses the payload, checks it is a
FHIR Parameters resource, locates the result parameter and asserts its valueBoolean. Reports a
missing result parameter as a named failure rather than throwing on a null reference.
- Both call sites now use it, replacing Assert.Contains("\"result\"", payload) and the
Assert.Contains("true", payload) substring match over the whole body.
Verified the new assertion can fail for the right reason: with the placeholder branch temporarily
removed from NormalizeSystem, both PlaceholderSystem_TreatedAsAbsent cases fail where the previous
assertions passed.
Testing: 1188 unit tests pass, unchanged in count. Test-only change; no production code touched.
* LEGLINK-888: Scope the empty-string binder override to the one parameter that needs it
Reviewer feedback on PR #1794. PreserveEmptyStringMetadataProvider turned
ConvertEmptyStringToNull off for every bound model in the service, when only
ValueSet/$validate-code's "system" needs the distinction between an omitted
parameter and a blank one.
- Adds PreserveEmptyStringAttribute, a parameter-only marker, and narrows the
provider to parameters carrying it. Every other bound value in the service
goes back to MVC's default behavior.
- Marks FhirController.ValidateCodeInValueSet's "system" with it. The
requirement is now visible on the parameter itself rather than in a provider
the reader has to go and find, and it survives a rename that a reflection
match on the parameter name would not.
- Drops the provider's dependency on the Controllers namespace, so the
Application layer no longer reaches into Presentation.
ValidateCodeInCodeSystem takes no "system" parameter -- the code system's
system is its url -- so ValueSet/$validate-code is the whole surface.
Verified the attribute is load-bearing: removing it fails exactly one test,
ValidateCodeInValueSet_BlankSystemQueryParameter_Returns400, and restoring it
passes again.
Testing: full ServiceTests suite passes -- 1761 passed, 1 skipped, 0 failed.1 parent 9f2bdc7 commit fd85725
8 files changed
Lines changed: 610 additions & 10 deletions
File tree
- DotNet
- ServiceTests/UnitTests/Terminology
- Controllers
- Services
- Terminology
- Application/Formatters
- Controllers
- Services
Lines changed: 288 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
Lines changed: 52 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
186 | 186 | | |
187 | 187 | | |
188 | 188 | | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
189 | 241 | | |
190 | 242 | | |
191 | 243 | | |
| |||
0 commit comments