feat: Update OpenAPI specs for reactive reactive results#28
feat: Update OpenAPI specs for reactive reactive results#28trawlinson-kainos wants to merge 14 commits intoNHSDigital:mainfrom
Conversation
*initial addition of FHIRBundle and FHIRCommuncation types * Some initial work around creating an example.
…ithub.com/trawlinson-kainos/hometest-supplier-integration-framework into feat/update-openapi-spec-reactive-results
🔬 FHIR Validation Results
|
| Severity | Location | Message |
|---|---|---|
warning |
OperationOutcome |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
observation_reactive_without_contact.example.json
| Severity | Location | Message |
|---|---|---|
warning |
Bundle.entry[0].resource/*DiagnosticReport/a1b2c3d4-e5f6-7890-abcd-ef1234567890*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Best Practice Recommendation: In general, all observations should have a performer |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Best Practice Recommendation: In general, all observations should have an effective[x] () |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle.entry[2].resource/*Communication/f47ac10b-58cc-4372-a567-0e02b2c3d479*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
ℹ️ information |
Bundle.entry[0].resource/*DiagnosticReport/a1b2c3d4-e5f6-7890-abcd-ef1234567890*/.code |
None of the codings provided are in the value set 'LOINC Diagnostic Report Codes' (http://hl7.org/fhir/ValueSet/report-codes |
observation_reactive_with_contact.example.json
| Severity | Location | Message |
|---|---|---|
warning |
Bundle.entry[0].resource/*DiagnosticReport/a1b2c3d4-e5f6-7890-abcd-ef1234567890*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Best Practice Recommendation: In general, all observations should have a performer |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Best Practice Recommendation: In general, all observations should have an effective[x] () |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle.entry[2].resource/*Communication/f47ac10b-58cc-4372-a567-0e02b2c3d479*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
ℹ️ information |
Bundle.entry[0].resource/*DiagnosticReport/a1b2c3d4-e5f6-7890-abcd-ef1234567890*/.code |
None of the codings provided are in the value set 'LOINC Diagnostic Report Codes' (http://hl7.org/fhir/ValueSet/report-codes |
get_test_results_non_reactive.example.json
| Severity | Location | Message |
|---|---|---|
warning |
Bundle.entry[0].resource/*Bundle/36716694-601b-4ad2-99e3-befb78d846ef*/.entry[0].resource/*DiagnosticReport/a1b2c3d4-e5f6-7890-abcd-ef1234567890*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle.entry[0].resource/*Bundle/36716694-601b-4ad2-99e3-befb78d846ef*/.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle.entry[0].resource/*Bundle/36716694-601b-4ad2-99e3-befb78d846ef*/.entry[2].resource/*Communication/f47ac10b-58cc-4372-a567-0e02b2c3d479*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle |
SearchSet bundles should have search modes on the entries |
ℹ️ information |
Bundle.entry[0].resource/*Bundle/36716694-601b-4ad2-99e3-befb78d846ef*/.entry[0].resource/*DiagnosticReport/a1b2c3d4-e5f6-7890-abcd-ef1234567890*/.code |
None of the codings provided are in the value set 'LOINC Diagnostic Report Codes' (http://hl7.org/fhir/ValueSet/report-codes |
ℹ️ information |
Bundle |
No types could be determined from the search string, so the types can't be checked |
ℹ️ information |
Bundle.entry[0].resource |
Unable to determine if this resource is a valid resource type for this search |
order_servicerequest.example.json
| Severity | Location | Message |
|---|---|---|
warning |
ServiceRequest |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
task_update_dispatched.example.json
| Severity | Location | Message |
|---|---|---|
warning |
Task |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
ℹ️ information |
Task.businessStatus |
Binding for path Task.businessStatus in profile StructureDefinition[http://hl7.org/fhir/StructureDefinition/Task |
observation_non_reactive.example.json
| Severity | Location | Message |
|---|---|---|
warning |
Bundle.entry[0].resource/*DiagnosticReport/a1b2c3d4-e5f6-7890-abcd-ef1234567890*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Best Practice Recommendation: In general, all observations should have a performer |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Best Practice Recommendation: In general, all observations should have an effective[x] () |
warning |
Bundle.entry[1].resource/*Observation/550e8400-e29b-41d4-a716-446655440001*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
warning |
Bundle.entry[2].resource/*Communication/f47ac10b-58cc-4372-a567-0e02b2c3d479*/ |
Constraint failed: dom-6: 'A resource should have narrative for robust management' (defined in http://hl7.org/fhir/StructureDefinition/DomainResource) (Best Practice Recommendation) |
ℹ️ information |
Bundle.entry[0].resource/*DiagnosticReport/a1b2c3d4-e5f6-7890-abcd-ef1234567890*/.code |
None of the codings provided are in the value set 'LOINC Diagnostic Report Codes' (http://hl7.org/fhir/ValueSet/report-codes |
Full HTML report available in the workflow run artifacts.
There was a problem hiding this comment.
@trawlinson-kainos do we need to update the schemas/supplier-api-spec.yaml results endpoint to reflect these changes. If so it might be good to add an example as well for that.
Yes, great point, I'd forgotten that there are two ways for the results to be sent, one from the suppliers to us (which is what I've worked on), but also from us making a request to the suppliers (through the 'supplier-api-spec'). The format will be the same, but I'll get that other API spec updated as well as part of this PR. |
jwankhalaf-cx
left a comment
jwankhalaf-cx
left a comment
There was a problem hiding this comment.
Left some comments to think about and double check.
| code: "260415000" | ||
| display: "Not detected" | ||
|
|
||
| code: "165816009" |
There was a problem hiding this comment.
FHIRObservation.valueCodeableConcept schema example uses a SNOMED code that does not exist.
The schema example specifies code: "165816009" with display "Human immunodeficiency virus not detected". This concept ID returns "No results" on the NHS Digital SNOMED CT Browser (termbrowser.nhs.uk) even with full-text search and both active and inactive components enabled, so it has never existed in SNOMED CT.
The docs/results.md and the FHIRBundleResultsExample in the same YAML file both use 165815009 for "HIV not detected", which is the real concept. The schema example appears to be a typo?
There was a problem hiding this comment.
Yes, a typo, have changed to '165815009', thank you.
| - description: Category of the diagnostic report (e.g., Laboratory) | ||
| example: | ||
| coding: | ||
| - system: "http://terminology.hl7.org/CodeSystem/observation-category" |
There was a problem hiding this comment.
DiagnosticReport.category uses the wrong code system.
The example for FHIRDiagnosticReport.category (and the same value in FHIRBundleResultsExample at line 541) uses system: http://terminology.hl7.org/CodeSystem/observation-category with code laboratory. That code system is the one for Observation.category, not DiagnosticReport.category.
Per the repo's own docs/results.md:
DiagnosticReport.category.coding.system = http://terminology.hl7.org/CodeSystem/v2-0074
DiagnosticReport.category.coding.code = LAB
This also matches the UK Core UKCore-DiagnosticReport-Lab profile, which has a mandatory laboratory slice fixing system: http://terminology.hl7.org/CodeSystem/v2-0074 and code: LAB.
Should be:
coding:
- system: "http://terminology.hl7.org/CodeSystem/v2-0074"
code: "LAB"
display: "Laboratory"
The Observation.category examples (lines 238 and 566) are correct as-is, observation-category / laboratory is the right system for Observation, just not for DiagnosticReport.
There was a problem hiding this comment.
Another great spot, thank you, agree it should be the 'v2-0074' CodeSystem with code 'LAB' for the Category field.
| - subject | ||
| - effectiveDateTime | ||
| - issued | ||
| - valueCodeableConcept |
There was a problem hiding this comment.
Schema cannot represent the documented error result types.
FHIRObservation.required lists valueCodeableConcept as mandatory, and dataAbsentReason is never declared anywhere in the schema. But docs/results.md in this same PR documents 7 error result types that must leave valueCodeableConcept empty and populate dataAbsentReason instead:
"the value field should be left empty in the Observation resource. Instead the
dataAbsentReasonfield should be populated."
| Test Supplier code | dataAbsentReason |
|---|---|
| Insufficient | insufficient |
| Haemolysed | invalid |
| Invalid sample | invalid |
| Out of Validation | invalid |
| Lab Error | invalid |
| Not Processed | invalid |
| Unknown | invalid |
This also matches FHIR R4, which has the constraint:
"dataAbsentReason SHALL only be present if Observation.value[x] is not present" (HL7 R4 Observation)
As written, the schema makes any of these 7 documented result types unsendable: a supplier following the spec strictly cannot validly construct an Observation without valueCodeableConcept, and there's no schema slot for dataAbsentReason even if they wanted to.
Suggested fix:
- Move
valueCodeableConceptout ofrequired(it should be conditionally required: present XORdataAbsentReasonpresent). - Add a
dataAbsentReasonproperty toFHIRObservationof typeFHIRCodeableConcept, with the documented codes from the value sethttp://terminology.hl7.org/CodeSystem/data-absent-reason. - Optionally express the XOR constraint as a description note (OpenAPI can't enforce it natively without
oneOf/not, but documenting the rule is useful).
There was a problem hiding this comment.
I think your point is totally valid around 'valueCodeableConcept' being required, and I've changed that.
However, the second point around using the 'data-absent-reason' value set I need to think about more deeply. All of the error cases there I think would fall under the 'not-performed' reason, and so we wouldn't have a way to distinguish between say a user error (insufficient sample) and other errors.
It's possible we need to agree an extension to that value set, just not 100% sure there. I will change the required part though for now, and also update the description note.
There was a problem hiding this comment.
Hiya,
To come back around this - we'll have all the error cases fall under 'not-performed' for now, but we will I think need to re-vist this later to understand how we address user errors vs lab errors (we want to show a different user screen if it's a user error to give them more guidance around how to do the test properly, get enough blood etc).
| description: Status of the diagnostic report result | ||
| enum: [final] | ||
| example: "final" | ||
| category: |
There was a problem hiding this comment.
category is modelled as a single object but FHIR R4 defines it as 0..* (an array).
Both FHIRDiagnosticReport.category (line 151) and FHIRObservation.category (line 232) use:
category:
allOf:
- $ref: '#/components/schemas/FHIRCodeableConcept'
- …That makes them single objects. But FHIR R4 specifies 0..* for both:
- DiagnosticReport.category — cardinality
0..* - Observation.category — cardinality
0..*
The FHIRBundleResultsExample in this same file (lines 539, 564) correctly renders category as an array, so the schema and the example contradict each other.
Suggested fix:
category:
type: array
items:
$ref: '#/components/schemas/FHIRCodeableConcept'
description: …
example:
- coding:
- system: "http://terminology.hl7.org/CodeSystem/v2-0074" # for DiagnosticReport
code: "LAB"
display: "Laboratory"There was a problem hiding this comment.
Yup, sorry, made a mistake in the schema there.
| items: | ||
| type: object | ||
| minItems: 3 | ||
| maxItems: 3 |
There was a problem hiding this comment.
FHIRBundleResults.entry items are typed as object, which orphans the FHIRDiagnosticReport, FHIRObservation and FHIRCommunication schemas.
entry:
type: array
description: The array should contain three FHIR resources - DiagnosticReport, Observation, and Communication
items:
type: object
minItems: 3
maxItems: 3The schemas for FHIRDiagnosticReport, FHIRObservation, and FHIRCommunication are defined further down but never $ref'd from FHIRBundleResults.entry. Consequences:
- Code-generators (e.g. NSwag for .NET clients) will produce a
Bundlewithentry: object[], and consumers will have to hand-roll typing on the resource union. - The schema validates any JSON object inside
entry, including{}, so suppliers can send schema-conformant nonsense. - Any IDE/tooling that drives off the schema for autocomplete or examples gets nothing useful for the entry payload.
Suggested fix (a typed Entry wrapper with a polymorphic resource):
entry:
type: array
minItems: 3
items:
type: object
required: [fullUrl, resource]
properties:
fullUrl:
type: string
example: "urn:uuid:…"
resource:
oneOf:
- $ref: '#/components/schemas/FHIRDiagnosticReport'
- $ref: '#/components/schemas/FHIRObservation'
- $ref: '#/components/schemas/FHIRCommunication'There was a problem hiding this comment.
Thank you - I was also thinking about how to avoid them being orphaned by didn't find anything easily on how to resolve that, but that's perfect - I agree, we should enforce through the schema that each one is one of those three types.
| example: "not-done" | ||
| reasonReference: | ||
| type: array | ||
| description: Reference to the ServiceRequest this communication is about |
There was a problem hiding this comment.
Communication.reasonReference description and example reference a ServiceRequest, but it should reference the Observation.
The schema currently says:
reasonReference:
description: Reference to the ServiceRequest this communication is about
items:
allOf:
- example:
reference: "ServiceRequest/550e8400-e29b-41d4-a716-446655440000"But this PR's own docs/clinical-contact-communication.md says explicitly:
"This field is used to relate the communication to the Observation that led to clinical contact being necessary."
And FHIRBundleResultsExample in the same YAML (line 591) correctly references the Observation by its urn:uuid. The schema's description and inline example are the two outliers and should be aligned to point at the Observation.
Suggested fix:
reasonReference:
description: Reference to the Observation that led to clinical contact being necessary
items:
allOf:
- $ref: '#/components/schemas/FHIRReference'
- example:
reference: "urn:uuid:550e8400-e29b-41d4-a716-446655440001"There was a problem hiding this comment.
Also agree, my auto-complete picking the wrong reference there
| type: string | ||
| description: Plain text representation of the concept | ||
| example: "HIV antigen test" | ||
| FHIRCoding: |
There was a problem hiding this comment.
FHIRCoding doesn't allow extension, but FHIRObservation.code example puts an extension on a coding.
The FHIRCoding schema (line 491) only declares system, code, display. But the Observation.code example (around line 244) embeds a valueset-reference extension directly on a coding:
code:
…
example:
coding:
- extension:
- url: "http://hl7.org/fhir/StructureDefinition/valueset-reference"
valueUri: "https://fhir.hl7.org.uk/ValueSet/UKCore-PathologyBoundedCodeListObservables"
system: "http://snomed.info/sct"
code: "31676001"
display: "HIV antigen test"(The extension URL itself is real — see the FHIR R4 valueset-reference extension, bound to the Coding type.)
So the example is conceptually fine, but it doesn't validate against the schema as written. Either:
- Add
extensiontoFHIRCoding(and probably the other FHIR data types — Identifier, Reference, etc.):FHIRCoding: properties: extension: type: array items: { $ref: '#/components/schemas/FHIRExtension' } system: …
- Or set
additionalProperties: trueon the FHIR data type schemas.
Without one of these, schema-validating suppliers will reject the canonical example.
There was a problem hiding this comment.
Agree - I've gone for the first option so that extensions in general are allowed in 'FHIRCodeableConcept'-type objects.
| items: | ||
| type: object | ||
| minItems: 3 | ||
| maxItems: 3 |
There was a problem hiding this comment.
Might maxItems: 3 be too rigid? This might block legitimate future additions.
entry:
…
minItems: 3
maxItems: 3Hard-coding "exactly 3 entries" forecloses a number of plausible near-term needs:
- Multiple Observations per Bundle (e.g. multi-analyte panels, or future kits that report more than one finding) — the docs already hint at "Beta stage of HomeTest, only HIV HomeTest Kits are used", implying more later.
- Adding a contained
Patient,Specimen, orPractitionerentry, which is normal in UK Pathology IG bundles. - Carrying an additional
OperationOutcomewith non-fatal warnings.
Any of those would require a breaking schema bump. minItems: 3 is fine as a floor, drop the maxItems: 3 cap.
There was a problem hiding this comment.
Mmm I thought about this, and it was a choice to narrow it down to just the three resources we're expecting. We don't have a use-case at the moment for handling more than one test at a time, and similarly we're not wanting to track the Patient, Specimen or Practitioner either.
I think it's good for it to be an explicit choice to have to handle those, including an explicit schema version bump for example, and so I'd like to keep the restriction narrow to just the three (DiagnosticReport, Observation and Communication) until we actually reach the point where more are needed. Is that okay?
lewisbirks
dismissed
ronanoconnor1’s stale review
Out of date with more updates required
Removal of fullURL from inside the FHIR resources, it should be instead part of the entry in each Bundle. Fixing of errors around the category field, where it sohuld be one code system for Observations, and a different one for DiagnosticReports Correction of type in SNOMED-CT code for HIV not detected. Inclusion of dataAbsentReason in spec. Correction of typo to reasonReference for Communication Add extension to the list of properies for FHIRCoding objects.
Hi @jwankhalaf-cx - thank you so much for your comments, and I believe I've resolved them now. If there's anything I've missed, do let me know. I'm also doing all this fairly manually, and it looked like you had some good way of validating what I'd done to get your comments - I'd love to be able to do that validation myself if it's not just manually going through (my attention to detail is clearly not perfect!). Thanks again! |
lewisbirks
changed the title
| @@ -0,0 +1,141 @@ | |||
| { | |||
| "resourceType": "Bundle", | |||
There was a problem hiding this comment.
Bundle (searchset) contains a bundle (collection). This does appear to match the YML
There was a problem hiding this comment.
Hiya,
The intention here is indeed to have a 'Bundle of Bundles' - so a 'searchset' Bundle that contains a 'collection' Bundle, with the DiganosticReport etc being inside the 'collection' bundle.
I think that's right and matches up between the JSON example here and the OpenAPI spec YAML (both the main spec and the example within the spec) - I just did a quick double check and all seems to line up to me.
I could well have missed something though, is there an issue here I haven't spotted?
| "code": { | ||
| "coding": [ | ||
| { | ||
| "system": "http://snomed.info/sct", |
There was a problem hiding this comment.
Would you expect this section to be static, and always contain the same code/display?
There was a problem hiding this comment.
Yes, my understanding is that for all Pathology results, the expected 'code' in the DiagnosticReport is always set to '721981007 - Diagnostic studies report'.
It's discussed here : https://simplifier.net/guide/pathology-fhir-implementation-guide/Home/FHIRAssets/AllAssets/AllProfiles/UKCore-DiagnosticReport-Lab?version=0.3.1 under 'Additional Guidance'.
Do you think we should enforce that in the spec?
…pi-spec * Enforce single-entry enums for all the resourceTypes consistently - we always know what they should be in each case. * The FHIRBundleResults needed to be nested inside a 'resource 'field.
| content: | ||
| application/fhir+json: | ||
| schema: | ||
| $ref: '#/components/schemas/FHIRBundleSearchsetObservations' |
There was a problem hiding this comment.
info/version number not changed (at the top of this file)
There was a problem hiding this comment.
Good spot, I've changed that to version 1.0.9, it just seemed best to align on the same version since we're modifying both at the same time to match up.
| description: FHIR Bundle resource for sending test results, containing DiagnosticReport, Observation and Communication resources | ||
| required: | ||
| - resourceType | ||
| - id |
There was a problem hiding this comment.
Missing link/total properties here
There was a problem hiding this comment.
Ahh, this is confusing, and is different between the 'home-test-supplier-api.yaml' and the supplier-api.yaml.
For the home-test-supplier-api.yaml - a single result is being POST'ed, and so I've used the UK Pathology implementation as my guide (https://simplifier.net/guide/pathology-fhir-implementation-guide/Home/FHIRAssets/AllAssets/AllProfiles/Examples/Bundles/C-Reactive-Protein-Report?version=0.3.1), and it doesn't use the link or total fields. It's a 'collection' bundle, and there's no searching going on.
However, for the 'supplier-api'.yaml, what gets returned from the GET /results endpoint is actually the response from a search. The result of the search is a 'searchset' bundle, and that's where the link and total fields come in. The 'link' field contains the request that led to this search result, with 'total' being the number of matches.
Each match is then a 'collection' bundle containing the DiagnosticReport etc. - the whole search response is a 'searchset' bundle containing 'collection' bundles
Does that make sense? So then I think it's right that this line in home-test-supplier-api.yaml doesn't talk about link/total, because there's no search happening.
Updated version number to match the home test version. It's not strictly necessary to keep them all in line, but seems easiest for this one.
|
5 participants
Description
This PR updates the OpenAPI spec to match the new examples for results.
This includes the use of a Bundle to communicate the results, containing a DiagnosticReport, an Observation, and a Communication.
An example has also been added within the OpenAPI spec for the 'non-reactive' case, which matches with the non-reactive JSON example.
Context
This aims to align the OpenAPI spec with the UK Pathology FHIR Implementation Guide (https://simplifier.net/guide/pathology-fhir-implementation-guide/Home/Design/Design-Overview?version=0.3.1) and to match with the examples that we've previously created following that guide.
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.