Support for fetching FHIR resources via Bulk export mode #1082

chandrashekar-s · 2024-06-03T12:36:53Z

Description of what I changed

Fixes #1031

Changes to support fetching of FHIR resources from FHIR server via Bulk Export API. Currently, changes are made only for FULL_RUN, INCREMENTAL_RUN changes will be made in a separate PR.
A parameter called fhirFetchMode has been added, which explicitly specifies which mode the application should use to fetch FHIR resources from source (this avoids the confusion of dynamically figuring out the mode from the parameters configured).

E2E test

Added a e2e test for BULK_EXPORT mode.
Added unit tests.

TESTED:

Manually tested the application by triggering a FULL run from the controller UI.

Checklist: I completed these to help reviewers :)

I have read and will follow the review process.
I am familiar with Google Style Guides for the language I have coded in.

No? Please take some time and review Java and Python style guides.
My IDE is configured to follow the Google code styles.

No? Unsure? -> configure your IDE.
I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)
I ran mvn clean package right before creating this pull request and added all formatting changes to my commit.
All new and existing tests passed.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

codecov-commenter · 2024-06-03T14:03:31Z

Codecov Report

Attention: Patch coverage is 26.59176% with 196 lines in your changes missing coverage. Please review.

Project coverage is 51.26%. Comparing base (1db3398) to head (739e8e7).
Report is 8 commits behind head on master.

Files	Patch %	Lines
...c/main/java/com/google/fhir/analytics/FhirEtl.java	0.00%	57 Missing ⚠️
...java/com/google/fhir/analytics/BulkExportUtil.java	0.00%	48 Missing ⚠️
...main/java/com/google/fhir/analytics/FetchUtil.java	5.40%	35 Missing ⚠️
...com/google/fhir/analytics/BulkExportApiClient.java	67.30%	11 Missing and 6 partials ⚠️
...ava/com/google/fhir/analytics/PipelineManager.java	37.50%	15 Missing ⚠️
...a/com/google/fhir/analytics/ReadJsonFromUrlFn.java	0.00%	7 Missing ⚠️
...java/com/google/fhir/analytics/DataProperties.java	22.22%	5 Missing and 2 partials ⚠️
...ain/java/com/google/fhir/analytics/ReadJsonFn.java	42.85%	2 Missing and 2 partials ⚠️
...e/fhir/analytics/converter/JsonDateSerializer.java	0.00%	4 Missing ⚠️
...fhir/analytics/converter/JsonDateDeserializer.java	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1082      +/-   ##
============================================
- Coverage     51.92%   51.26%   -0.67%     
+ Complexity      697      667      -30     
============================================
  Files            91       97       +6     
  Lines          5519     5569      +50     
  Branches        709      724      +15     
============================================
- Hits           2866     2855      -11     
- Misses         2390     2443      +53     
- Partials        263      271       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chandrashekar-s · 2024-06-05T03:27:53Z

Hi @bashir2, this PR contains changes to support FULL_RUN only. I will raise a separate PR for supporting INCREMENTAL_RUN (have created this issue for the same).

bashir2

Thanks @chandrashekar-s for the change and sorry for the delay caused by DevDays and issues around it. I had a look at most of the non-test code and my main/major comment is about whether we should create temporary files or not (I am suggesting not).

bashir2 · 2024-06-24T17:45:42Z

pipelines/batch/src/main/java/com/google/fhir/analytics/BulkExportUtil.java

+   * @throws BulkExportException
+   * @throws IOException
+   */
+  public static List<Path> triggerBulkExportAndDownloadFiles(


In general, it is better to avoid static methods for non-trivial tasks as it makes the modules more tightly coupled and harder to unit-test. For example, here I would make this a non-static method and call it on an instance of BulkExportUtil when needed. I would also make fhirSearchUtil as instance property.

Agreed, changes have been made as suggested.

pipelines/batch/src/main/java/com/google/fhir/analytics/BulkExportUtil.java

pipelines/batch/src/main/java/com/google/fhir/analytics/FhirEtl.java

pipelines/batch/src/main/java/com/google/fhir/analytics/BulkExportUtil.java

pipelines/batch/src/main/java/com/google/fhir/analytics/FhirSearchUtil.java

bashir2 · 2024-06-24T20:28:13Z

pipelines/batch/src/main/java/com/google/fhir/analytics/model/BulkExportHttpResponse.java

+import org.hl7.fhir.instance.model.api.IBaseOperationOutcome;
+
+/** Model class for holding the details of the bulk export job output */
+@Data


For pure data objects, I have found the "new" record construct is also useful.

Thanks for pointing this out, I didn't know about it. For time being, I have not made these changes as there are many fields in the Pojo and by default the record construct only creates all arguments constructor. I will revisit this later if this fine for you.

bashir2 · 2024-06-24T20:29:31Z

pipelines/batch/src/main/java/com/google/fhir/analytics/FhirSearchUtil.java

+    MethodOutcome methodOutcome =
+        fetchUtil.performServerOperation(
+            "export", fetchBulkExportParameters(fhirVersionEnum, resourceTypes), headers);
+    if ((methodOutcome.getResponseStatusCode() / 100) != 2) {


I would guess httpcore probably has a isStatusSuccess() method or something similar that is more readable; if not, maybe define one?

I didnt find this method in the apache http util classes, so I created one.

bashir2 · 2024-06-24T20:34:37Z

pipelines/batch/src/main/java/com/google/fhir/analytics/FhirSearchUtil.java

+  private IBaseParameters fetchBulkExportParameters(
+      FhirVersionEnum fhirVersionEnum, List<String> resourceTypes) throws BulkExportException {
+    switch (fhirVersionEnum) {
+      case R4:


Why we only have support for R4 here?

Added support for DSTU3 now.

bashir2 · 2024-06-24T20:34:54Z

pipelines/batch/src/main/java/com/google/fhir/analytics/model/BulkExportResponse.java

+import lombok.Data;
+
+/** The response body of the bulk export job status */
+@Data


Have provided my reasons in the other comment.

chandrashekar-s · 2024-07-04T18:08:38Z

Thanks @bashir2 for the review. I have addressed the comments in the latest commit.

chandrashekar-s force-pushed the trigger-bulk-export branch from 8c12502 to 9a7b464 Compare June 3, 2024 13:56

chandrashekar-s force-pushed the trigger-bulk-export branch 7 times, most recently from aaf488e to dbc61d0 Compare June 5, 2024 03:16

chandrashekar-s requested a review from bashir2 June 5, 2024 03:26

chandrashekar-s force-pushed the trigger-bulk-export branch from dbc61d0 to c779d2d Compare June 19, 2024 06:44

bashir2 reviewed Jun 24, 2024

View reviewed changes

Support for fetching FHIR resources via Bulk export mode

30b4fe9

chandrashekar-s force-pushed the trigger-bulk-export branch from c779d2d to 30b4fe9 Compare June 26, 2024 05:46

Addressed review comments

739e8e7

chandrashekar-s force-pushed the trigger-bulk-export branch from 5d36f23 to 739e8e7 Compare July 4, 2024 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for fetching FHIR resources via Bulk export mode #1082

Support for fetching FHIR resources via Bulk export mode #1082

chandrashekar-s commented Jun 3, 2024 •

edited

Loading

codecov-commenter commented Jun 3, 2024 •

edited

Loading

chandrashekar-s commented Jun 5, 2024

bashir2 left a comment

bashir2 Jun 24, 2024

chandrashekar-s Jul 4, 2024 •

edited

Loading

bashir2 Jun 24, 2024

chandrashekar-s Jul 4, 2024

bashir2 Jun 24, 2024

chandrashekar-s Jul 4, 2024

bashir2 Jun 24, 2024

chandrashekar-s Jul 4, 2024

bashir2 Jun 24, 2024

chandrashekar-s Jul 4, 2024

chandrashekar-s commented Jul 4, 2024

Support for fetching FHIR resources via Bulk export mode #1082

Are you sure you want to change the base?

Support for fetching FHIR resources via Bulk export mode #1082

Conversation

chandrashekar-s commented Jun 3, 2024 • edited Loading

Description of what I changed

E2E test

Checklist: I completed these to help reviewers :)

codecov-commenter commented Jun 3, 2024 • edited Loading

Codecov Report

chandrashekar-s commented Jun 5, 2024

bashir2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandrashekar-s Jul 4, 2024 • edited Loading

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

chandrashekar-s commented Jul 4, 2024

chandrashekar-s commented Jun 3, 2024 •

edited

Loading

codecov-commenter commented Jun 3, 2024 •

edited

Loading

chandrashekar-s Jul 4, 2024 •

edited

Loading