Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FHIR export resource type filter #1178

Merged
merged 6 commits into from
Nov 30, 2022
Merged

FHIR export resource type filter #1178

merged 6 commits into from
Nov 30, 2022

Conversation

dehall
Copy link
Contributor

@dehall dehall commented Oct 19, 2022

Addresses #1104

Adds new config settings to support filtering the resource types that are exported in the FHIR exporter, by either an include list or exclude list. (Similar to the existing feature for CSVs)
This approach checks each resource type before instantiating the relevant HAPI resource class.

A few notes on current status:

  • this leaves the Patient and Encounter resources as always included, even if specifically excluded, primarily because of the number of changes it would require.
  • this doesn't currently attempt to be "friendly" and include related resources, for example a DiagnosticReport will reference Observations, but if you include DiagnosticReport without including Observation, you will get a DiagnosticReport with no outgoing references.
  • this is only FHIR R4, but I'd like to get any feedback before applying this to the other exporters. (Or maybe we don't even want to spend time on the DSTU2 and STU3 exporters anymore?)
  • I'd also like to explore some changes to work with resource type enums instead of strings

This also includes a tweak to build.gradle to fix some error messages I was seeing in Eclipse. The issue was that JSBCL includes JUnit 4.10 as a transitive dependency, but we use JUnit 4.13 as a direct dependency for tests. Eclipse was getting confused and using JUnit 4.10 at compile time and it didn't recognize some method calls. The tweak removes the transitive dependency on JUnit from JSBCL because I don't think there's any need for the main build to include JUnit.

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #1178 (70dd608) into master (8a2e87d) will increase coverage by 1%.
The diff coverage is 88%.

@@            Coverage Diff             @@
##             master   #1178     +/-   ##
==========================================
+ Coverage        79%     80%     +1%     
- Complexity     3407    4017    +610     
==========================================
  Files           135     137      +2     
  Lines         23028   26003   +2975     
  Branches       3125    4148   +1023     
==========================================
+ Hits          18239   20862   +2623     
- Misses         3809    4097    +288     
- Partials        980    1044     +64     
Impacted Files Coverage Δ
src/main/java/org/mitre/synthea/export/FhirR4.java 83% <88%> (+4%) ⬆️
...in/java/org/mitre/synthea/engine/Distribution.java 51% <0%> (-14%) ⬇️
...org/mitre/synthea/export/ClinicalNoteExporter.java 71% <0%> (-8%) ⬇️
...ehaviors/providerfinder/ProviderFinderNearest.java 90% <0%> (-5%) ⬇️
...re/synthea/helpers/ConsolidatedServicePeriods.java 94% <0%> (-3%) ⬇️
...in/java/org/mitre/synthea/export/TextExporter.java 90% <0%> (-3%) ⬇️
...in/java/org/mitre/synthea/export/JSONExporter.java 91% <0%> (-2%) ⬇️
.../main/java/org/mitre/synthea/engine/Generator.java 78% <0%> (-1%) ⬇️
...in/java/org/mitre/synthea/world/agents/Person.java 85% <0%> (-1%) ⬇️
...tre/synthea/export/FhirPractitionerExporterR4.java 94% <0%> (-1%) ⬇️
... and 26 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sonatype-lift
Copy link
Contributor

sonatype-lift bot commented Oct 19, 2022

⚠️ 26 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

@dehall dehall changed the title WIP: FHIR export resource type filter FHIR export resource type filter Nov 3, 2022
@dehall
Copy link
Contributor Author

dehall commented Nov 3, 2022

Marking this as ready to review per separate conversation -- Patient and Encounter will always be required, and if people like this approach and want it for FHIR DSTU2 or STU3 I'll copy it over, pending any other suggestions.

@dehall dehall marked this pull request as ready for review November 3, 2022 16:41
@KevinCranmer
Copy link

@dehall this is awesome stuff that I would love to begin using! Would it be possible to include a filter for Provenance as well, or is that a required resource similar to Patient and Encounter? I'm decently new to fhir myself.

@dehall
Copy link
Contributor Author

dehall commented Nov 14, 2022

@KevinCranmer thanks and good catch, that was an unintentional omission. I've now added a check for Provenance. I'll remind our team this is ready for review and hopefully we can get it merged soon

Copy link
Member

@jawalonoski jawalonoski left a comment

Choose a reason for hiding this comment

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

The only thing I would say is that many reasons are now lost, even if they are known.

For example, assume we are exporting only MedicationRequest resources.

If a MedicationRequest has a reason, but Condition is filtered out, then MedicationRequest.reasonReference is not populated (as expected)... however, because the reasonCode is known, you should be able to populate the MedicationRequest.reasonCode.

Do you want to make a change now or in a separate PR?

@dehall
Copy link
Contributor Author

dehall commented Nov 16, 2022

Sounds good, I'll make that change now unless it turns out to be a lot more complex than it seems

Copy link
Collaborator

@hadleynet hadleynet left a comment

Choose a reason for hiding this comment

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

Just one suggestion about switching to using Classes instead of Strings for you lists of resources to include or exclude.

// normalize filenames by trimming
files = files.stream().map(f -> f.trim()).collect(Collectors.toList());

// TODO: convert these into FHIR resource enums
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using an enum you could use the class. E.g. Class clazz = Class.forName("org.hl7.fhir.r4.model." + className).
This function could return a List<Class> instead of List<String>
The Class.forName method can throw a ClassNotFound exception. You could use that to catch spelling errors in the list of resources to include/exclude.

@@ -267,75 +320,108 @@ public static Bundle convertToFHIR(Person person, long stopTime) {
for (Encounter encounter : person.record.encounters) {
BundleEntryComponent encounterEntry = encounter(person, personEntry, bundle, encounter);

for (HealthRecord.Entry condition : encounter.conditions) {
condition(person, personEntry, bundle, encounterEntry, condition);
if (shouldExport("Condition")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you take my suggestion above about using classes instead of strings, this would become shouldExport(Condition.class) which would make it less prone to typo errors.

@dehall
Copy link
Contributor Author

dehall commented Nov 30, 2022

@hadleynet I like the idea but there are a couple ways the Java code winds up being a little less elegant in practice. 1) To prevent compiler warnings, we can't use just Class, we need to parameterize it, which becomes Class<? extends Resource>. 2) Because of the overlap between FHIR resource type names and our HealthRecord class names, we can only import one class with the same name at the same time, so some of the shouldExport(ClassName.class) s become shouldExport(org.hl7.fhir.r4.model.ClassName.class).

Neither is the end of the world obviously but if people think this is ugly then I can try something else. I've pushed that change to a separate branch resource_type_filter_with_classes . Feel free to tell me I'm overthinking it and I'll move that back to this branch. See diff here: resource_type_filter...resource_type_filter_with_classes

@hadleynet
Copy link
Collaborator

You might be able to get away with Class<?> instead of Class<? extends Resource> but the latter is more specific/correct. If you don't like the shouldExport(org.hl7.fhir.r4.model.ClassName.class) you could switch which class you import and fully qualify the Synthea class name instead.

PersonalIy, think the type-safety aspect is worth the extra effort.

@dehall
Copy link
Contributor Author

dehall commented Nov 30, 2022

Ok I've gone ahead and added the type-safe changes here. Rather than change more unrelated lines I'll keep the fully qualified FHIR class names where necessary

@hadleynet hadleynet merged commit 75f417f into master Nov 30, 2022
@hadleynet hadleynet deleted the resource_type_filter branch November 30, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants