Add support for the _elements param #39

elsaperelli · 2024-03-27T19:22:49Z

Summary

This PR adds support for the _elements query parameter as defined by the Bulk Data Access IG. Some inspiration for this parameter's implementation was taken from the implementation of the _elements FHIR search parameter implementation in the measure-respository-service in this PR.

New behavior

Functionality for the _elements query parameter is now available! When the _elements query parameter is used, the server will return resources with only the elements specified in the parameter and the SUBSETTED tag (see the Bulk Data Access IG for more info).

Code changes

exportWorker.js - add elements as a parameter to call to exportToNDJson
export.service.js - add _elements parameter validation to validateExportParams function and add _elements as a recognized parameter
exportToNDJson.js - add a try/catch to the buildSearchParamList function so that it doesn't error out on some resources that the server supports but for some reason do not exist in the node-fhir-server-core parameters (ex. BiologicallyDerivedProduct), add _elements parameter handling to exportToNDJson and getDocuments
mongo.controller.js - add projection functionality to findResourcesWithAggregation so that we can use the _elements parameter with the _typeFilter parameter
exportToNDJson.test.js - adds two tests- one for _elements handling in getDocuments and the other to make sure that buildSearchParamList doesn't error out on one of the supportResources

Implementation Questions

The Bulk Data Access IG defines the _elements parameter to include the following SHOULD functionality: "A server is not obliged to return just the requested elements. A server SHOULD always return mandatory elements whether they are requested or not." I do not have this implemented currently. I feel like it is something I should implement; however, it is an interesting question of how useful that may be (and potentially something we suggest in the IG?). I think it's interesting because in my opinion, if we defined mandatory elements to be elements that have a cardinality of 1..1, then there are quite a few and it may defeat the purpose of a potential use case of the _elements parameter (i.e., if a user just wanted the ids of all of the Condition resources).
If JUST the element name is included, are we returning ALL resource types (and only include x where the element name x is included) OR are we returning ONLY resource types where the element name x is included (right now we do the former).

Testing guidance

Please read and consider my implementation questions!
npm run check
npm start to spin up the server at localhost:3001
Try lots of different HTTP request combinations! Try all three endpoints (System-level, Patient, Group) and combinations of the _elements parameter with other query parameters (_type, patient, _typeFilter, etc.).
I would try making your own HTTP requests in Insomnia first because it may bring up something that I missed, but I also have the following Insomnia tests attached!

ElementsParamTesting.json

github-actions · 2024-03-27T19:24:09Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	74.92% (-4.67% 🔻)	490/654
🟡	Branches	61.64% (-4.89% 🔻)	180/292
🟡	Functions	73.33% (-4.56% 🔻)	77/105
🟡	Lines	75.23% (-4.8% 🔻)	483/642

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / export.service.js	85.9% (-8.99% 🔻)	79.44% (-6.28% 🔻)	94.44% (-5.56% 🔻)	85.62% (-9.16% 🔻)
🟢	`...` / mongo.controller.js	91.07% (+0.16% 🔼)	85.71% (-14.29% 🔻)	78.57%	91.07% (+0.16% 🔼)
🟡	`...` / exportToNDJson.js	70.95% (-11.76% 🔻)	48.68% (-10.93% 🔻)	66.67% (-12.5% 🔻)	71.68% (-12.58% 🔻)

Test suite run success

78 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from 3ea331f

lmd59

Initial code changes make sense, though understood we're still waiting on implementation details. I think it makes sense to do a README update to the server endpoints section for this PR as well. (When you do, there are a couple of deqm-test-server references that might be nice to cleanup to reference bulk-export-server instead)

lmd59 · 2024-04-01T17:32:37Z

Implementation question 1:
My initial thoughts are that it might be worth implementing the "mandatory elements". The mandatory elements could be helpful in an _elements param request if the requester is expecting back valid FHIR resources, which I could see being a requirement in certain use cases. And while I think the option to not include mandatory elements also has valid use cases, I think at least attempting to implement it would give us more information in our goal of "exercising the spec". If we do end up making an IG recommendation, this would give us more data to go off of, whether we recommend in favor or against mandatory elements.

Implementation question 2:
This is potentially dependent on 1. If we're just returning elements requested (and not mandatory ones), then it probably wouldn't make sense to return resources that don't have at least one of the elements requested. If we're also returning mandatory elements, I would lean toward returning all resource types and assuming the burden of filtering should be on the requester.

elsaperelli · 2024-04-03T16:07:17Z

@lmd59 thank you for your thoughtful responses to the implementation questions! I have decided to go with the following, please let me know if that doesn't make sense or if I should change anything:

Update:
The Bulk Data Access IG says the following about the _elements parameter: "A server is not obliged to return just the requested elements. A server SHOULD always return mandatory elements whether they are requested or not." We have decided to use the USCore definition of mandatory elements for this (thanks Lauren!) (also worth noting that this should probably be detailed directly in the Bulk Data Access IG).

I have decided to get those mandatory elements for each of the resource types that our server supports by downloading the StructureDefinition.json for each of those resource types (using FHIR v4.0.1) and running the parseStructureDefinitions.js script so that we have a mandatory-elements.json file that is an object whose keys are resourceTypes and values are arrays of strings of mandatory elements.

Finally, these mandatory elements are utilized in exportToNDJson.js so that they are included in the response when the _elements parameter is used.

lmd59

I have a couple of general comments and questions in code and then I'll also add:

It seems like resourceType is implicitly mandatory on the base resource element, but the structure definition itself doesn't indicate that - https://hl7.org/fhir/R4/resource.html . Do you think we would consider that mandatory for all resources?
I would add a description of the _elements param to the README

src/scripts/parseStructureDefinitions.js

src/services/export.service.js

src/scripts/parseStructureDefinitions.js

elsaperelli · 2024-04-11T20:33:51Z

I have a couple of general comments and questions in code and then I'll also add:

It seems like resourceType is implicitly mandatory on the base resource element, but the structure definition itself doesn't indicate that - https://hl7.org/fhir/R4/resource.html . Do you think we would consider that mandatory for all resources?

I would add a description of the _elements param to the README

Good question about resourceType! Yeah I am confused on this as well- it looks like it is required in the TypeScript FHIR definition which is interesting, but as you said, does not appear in the structure definition? I am going to defer to @hossenlopp and @p9g for this

p9g · 2024-04-12T14:03:22Z

https://hl7.org/fhir/R4/json.html#resources says "A resource is a JSON object with a property resourceType which informs the parser which resource type this is" .
So if it does not have resourceType, it is not a FHIR resource. Sounds mandatory.
http://hl7.org/fhir/R4/json.html#xml comparison of json and xml forms of a resource: "The type of the resource is represented differently in JSON - instead of being the name of the base object (there is none in JSON), it is carried as the property resourceType"

The json schema http://hl7.org/fhir/R4/fhir.schema.json.zip has resourceType as required for all of the (duh) resources.

elsaperelli · 2024-04-24T14:07:45Z

Huge thank you for the guidance as well as Karl's! Looks like our current implementation makes the most sense although it certainly can be subject to change.

For testing now, please refer to Karl's guidance and make sure this functionality aligns with his suggestions. Also please check that my README explanation makes sense and if there should be anything else added.

I have an updated Insomnia testing collection attached! Please use this one now that resourceType is a mandatory element and the endpoint is localhost:3000.

ElementsParamTesting.json

lmd59

Just some recommendations for the README. Otherwise, looks good!

README.md

hossenlopp

This looks like it works well for the most part, but if you reference a choice type parameter such as DiagnosticReport.effective you do not get the FHIR JSON effectiveDateTime and effectivePeriod choice elements. The spec isn't clear if it should reference the choice types by the name of the element or the element and the specific type.

elsaperelli · 2024-05-14T14:09:25Z

This looks like it works well for the most part, but if you reference a choice type parameter such as DiagnosticReport.effective you do not get the FHIR JSON effectiveDateTime and effectivePeriod choice elements. The spec isn't clear if it should reference the choice types by the name of the element or the element and the specific type.

Good call! I reached out to Karl and confirmed that this is indeed the functionality that we want for our current interpretation of the spec. I added two files: getChoiceTypesForResource.js which is a script for parsing the FHIR R4 StructureDefinitions for each resource type supported by the server and getting the choice type elements and their types for each and choice-types.json which is a lookup object that is a result of running the getChoiceTypesForResource.js script.

New behavior for the server now includes the following: for _elements parameter inputs, if the element name is a choice type for a resource, then the server will return any of the choice types. For example, DiagnosticReport.effective should return DiagnosticReport resources that contain effectiveDateTime and effectivePeriod. The ecqm-content-r4-2021 content seems to not have effectivePeriod on the DiagnosticReport resources, so a better test may be MedicationRequest.medication should return medicationCodeableConcept. I will update the Insomnia collection to include GET requests for this cases.

src/util/exportToNDJson.js

hossenlopp

More null checks on the choice type stuff is needed.

hossenlopp · 2024-05-15T13:58:18Z

src/util/exportToNDJson.js

+            Object.keys(choiceTypeElements[resourceType]).includes(`${elementName}[x]`)
+          ) {
+            const rootElem = elementName.split('[x]')[0];
+            choiceTypeElements[resourceType][rootElem].forEach(e => {


This doesn't carefully check the choice type element structure exists before doing the forEach.

It blows up on the following request

http://localhost:3000/$export?_type=DiagnosticReport&_elements=DiagnosticReport.effective

Additionally this area of the code could use a bit more explanation of what it is doing. If possible, unit tests too.

Sounds good! I think I have a fix that I will push up. The issue was it wasn't finding the key with just the elementName in the lookup object (needed the [x] as well). I will add more documentation but will push that fix for now!

I think it also needs to be a bit more resilient to garbage being passed in. Like a Resource or element that doesn't exist.

How so? Should I check every resourceType and element that is passed in to make sure it's valid FHIR? If so, how does that work for elements passed in without resourceType (i.e. just id rather than Conditiion.id)? Would I have to check that it's valid on every single resourceType (often not the case)? These are things I have brought up but it seemed like the server should not error out.

If you wanted to do a base check that something of the form Condition.id has id as a valid property on Condition, you could use the propertyPaths file/script here: projecttacoma/fqm-testify@815c4fc
Hasn't been pulled into fqm-testify because it's mostly just been used for the data requirements work so far.

Do we have an idea of the expected error output according to the spec if there is malformed parameters?

@hossenlopp @lmd59 also keep in mind that we don't do any sort of checks for inputs passed into _typeFilter. For example, a GET request to $export?_typeFilter=Condition?invalid=invalid does not return any errors.

I think maybe some of this validation is out of scope for this tasking, but if we're not responding with the correct errors, we should know what the spec indicates we need and use that for potential follow-up tasking.

Probably just 400 Bad Request, right?

hossenlopp

This works well!

elsaperelli marked this pull request as ready for review March 27, 2024 20:45

elsaperelli requested review from lmd59 and hossenlopp March 28, 2024 13:12

elsaperelli assigned lmd59 and hossenlopp Mar 28, 2024

lmd59 reviewed Apr 1, 2024

View reviewed changes

elsaperelli requested a review from lmd59 April 3, 2024 15:56

elsaperelli mentioned this pull request Apr 10, 2024

Add docker setup for bulk export #41

Merged

lmd59 reviewed Apr 10, 2024

View reviewed changes

elsaperelli added 10 commits April 15, 2024 12:00

Initial _elements param work

1d4973e

Majority _elements parameter functionality

ad4029f

Add subsetted tag to meta.tag on resources returned by _elements param

cef2578

Comment out elementName variable for now

45358b6

Actually remove elementName for now

13b65b3

Add mandatory elements support and script

4c79b8b

Update unit test

633e82a

Remove comments and TODOs and change min check to >= 1

de1341e

Add link to where structuredef can be found

6281a95

Add resourceType as a mandatory element for resources

8d13fdd

elsaperelli force-pushed the elements-param branch from 060df2e to 8d13fdd Compare April 15, 2024 16:00

Add _elements description to README

95e7f0c

elsaperelli requested a review from lmd59 April 24, 2024 14:07

lmd59 reviewed Apr 25, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README fixes

dff935d

Additional README changes

680d427

elsaperelli requested a review from lmd59 April 25, 2024 20:39

lmd59 approved these changes Apr 26, 2024

View reviewed changes

hossenlopp reviewed May 2, 2024

View reviewed changes

Add functionality for choice types as elements parameter inputs

57d6f41

elsaperelli requested review from hossenlopp and lmd59 May 14, 2024 14:09

elsaperelli added 2 commits May 14, 2024 11:32

Remove console.logs, fix capitalization problem

e806fa5

Condense code, fix bugs found through Insomnia testing

1905d5a

lmd59 reviewed May 15, 2024

View reviewed changes

src/util/exportToNDJson.js Outdated Show resolved Hide resolved

hossenlopp requested changes May 15, 2024

View reviewed changes

Fix DiagnosticReport.effective error

3ea331f

hossenlopp approved these changes May 16, 2024

View reviewed changes

lmd59 approved these changes May 16, 2024

View reviewed changes

hossenlopp merged commit abb78ce into main May 18, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the _elements param #39

Add support for the _elements param #39

elsaperelli commented Mar 27, 2024 •

edited

Loading

github-actions bot commented Mar 27, 2024 •

edited

Loading

lmd59 left a comment

lmd59 commented Apr 1, 2024

elsaperelli commented Apr 3, 2024

lmd59 left a comment

elsaperelli commented Apr 11, 2024

p9g commented Apr 12, 2024

elsaperelli commented Apr 24, 2024

lmd59 left a comment

hossenlopp left a comment

elsaperelli commented May 14, 2024

hossenlopp left a comment

hossenlopp May 15, 2024

elsaperelli May 15, 2024

hossenlopp May 15, 2024

elsaperelli May 15, 2024

lmd59 May 15, 2024

lmd59 May 16, 2024

elsaperelli May 16, 2024

lmd59 May 16, 2024

lmd59 May 16, 2024

hossenlopp left a comment

Add support for the _elements param #39

Add support for the _elements param #39

Conversation

elsaperelli commented Mar 27, 2024 • edited Loading

Summary

New behavior

Code changes

Implementation Questions

Testing guidance

github-actions bot commented Mar 27, 2024 • edited Loading

Coverage report

Test suite run success

lmd59 left a comment

Choose a reason for hiding this comment

lmd59 commented Apr 1, 2024

elsaperelli commented Apr 3, 2024

lmd59 left a comment

Choose a reason for hiding this comment

elsaperelli commented Apr 11, 2024

p9g commented Apr 12, 2024

elsaperelli commented Apr 24, 2024

lmd59 left a comment

Choose a reason for hiding this comment

hossenlopp left a comment

Choose a reason for hiding this comment

elsaperelli commented May 14, 2024

hossenlopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hossenlopp left a comment

Choose a reason for hiding this comment

elsaperelli commented Mar 27, 2024 •

edited

Loading

github-actions bot commented Mar 27, 2024 •

edited

Loading