Enable resources to be sent to a generic FHIR store using the hapi client #30

pmanko · 2020-10-07T05:35:19Z

Closes #25 and #29.

TESTED

Batch mode using docker-compose
Atomfeed-streaming mode using the following command: mvn exec:java -pl streaming-atomfeed -Dexec.mainClass=org.openmrs.analytics.FhirStreaming -Dexec.args="http://18.158.139.243:8091/openmrs admin/Admin123 http://18.158.139.243:8098/hapi-fhir-jpaserver/fhir"

Some fixes based on these tests were put into this PR, while other will be added as issues.

See: https://github.com/pmanko/openmrs-fhir-analytics/tree/25-hapi-client-to-sink-endtoend

batch/src/main/java/org/openmrs/analytics/FhirEtl.java

ibacher · 2020-10-27T15:11:34Z

batch/src/main/java/org/openmrs/analytics/FhirEtl.java

+			return new GcpStoreUtil(fhirStoreUrl, fhirContext);
+		else
+			return new FhirStoreUtil(fhirStoreUrl, fhirContext);


The constructors here are defined to take an IGenericClient rather than FhirContext, so this should likely be something like:

return new FhirStoreUtil(fhirStoreUrl, fhirContext.newRestfulGenericClient());

Similarly, I tend to think that if ... else should always be blocks:

if (condition) { } else { }

This makes it easier if additional actions need to be taken.

The constructors here are defined to take an IGenericClient rather than FhirContext, so this should likely be something like:

return new FhirStoreUtil(fhirStoreUrl, fhirContext.newRestfulGenericClient());

Yeah, I accidentally committed in the middle of refactoring -_- thanks!

ibacher · 2020-10-27T15:13:20Z

batch/src/main/java/org/openmrs/analytics/FhirEtl.java

@@ -130,7 +138,7 @@ static OpenmrsUtil createOpenmrsUtil(String sourceUrl, String sourceUser, String
 	        throws CannotProvideCoderException {
 		FhirSearchUtil fhirSearchUtil = createFhirSearchUtil(options, fhirContext);
 		// TODO remove fhirContext from fhirStoreUtil.
-		FhirStoreUtil fhirStoreUtil = new FhirStoreUtil(options.getGcpFhirStore(), fhirContext);
+		FhirStoreUtil fhirStoreUtil = new FhirStoreUtil(options.getFhirStoreUrl(), fhirContext);


Suggested change

FhirStoreUtil fhirStoreUtil = new FhirStoreUtil(options.getFhirStoreUrl(), fhirContext);

FhirStoreUtil fhirStoreUtil = new FhirStoreUtil(options.getFhirStoreUrl(), fhirContext.newRestfulGenericClient());

ibacher · 2020-10-27T15:13:55Z

batch/src/main/java/org/openmrs/analytics/FhirEtl.java

@@ -194,15 +202,15 @@ public void Setup() {
 			fhirContext = FhirContext.forDstu3();
 			fhirStoreUtil = new FhirStoreUtil(fhirStoreUrl, fhirContext);


Suggested change

fhirStoreUtil = new FhirStoreUtil(fhirStoreUrl, fhirContext);

fhirStoreUtil = new FhirStoreUtil(fhirStoreUrl, fhirContext.newRestfulGenericClient());

mozzy11 · 2020-10-28T09:13:15Z

@pmanko Thanks for the great work.
do we need to entirely get rid of the http utils ??

would think it would be useful at some point dealing with non FHIR data .

What do you think

bashir2

Thanks @pmanko for this change; I like the unified interface of FhirStoreUtil. Have you tested your changes on a GCP FHIR store too?

bashir2 · 2020-10-30T04:48:59Z

batch/src/main/java/org/openmrs/analytics/FhirEtl.java

+		            options.getPassword(), fhirContext));
+	}
+
+	static FhirStoreUtil createFhirStoreUtil(String fhirStoreUrl, FhirContext fhirContext) {


I think this functionality is needed elsewhere too (e.g., in the streaming pipelines), so here is a suggestion: Create a factory method somewhere under common where internally it uses the pattern matching logic of matchesGcpPattern but do not expose the metchesGcpPattern function publicly.

BTW, please update all usages of FhirStoreUtil in the code; I am thinking that the constructor for FhirStoreUtil should be private and instances of that should only be created through the factory method, WDYT?

bashir2 · 2020-10-30T05:42:01Z

batch/src/main/java/org/openmrs/analytics/FhirSearchUtil.java

 	private OpenmrsUtil openmrsUtil;

-	FhirSearchUtil(OpenmrsUtil openmrsUtil) {
+	FhirSearchUtil(FhirStoreUtil fhirStoreUtil, OpenmrsUtil openmrsUtil) {


Please revert this change; the dependency between FhirSearchUtil and FhirStoreUtil was intentionally removed in this commit. The idea is that, FhirStoreUtil is any functionality for uploading data to a FHIR-store and the only reason we needed FhirStoreUtil here was uploadBundleToCloud for which the right place is actually FhirStoreUtil itself.

bashir2 · 2020-10-30T05:42:40Z

batch/src/main/java/org/openmrs/analytics/FhirSearchUtil.java

@@ -79,4 +84,12 @@ public String findBaseSearchUrl(Bundle searchBundle) {
 		}
 	}

+	public MethodOutcome uploadBundleToCloud(Bundle bundle) {


As I said above, this was intentionally moved to FhirStoreUtil, let's keep it that way.

common/src/main/java/org/openmrs/analytics/GcpStoreUtil.java

bashir2 · 2020-10-30T05:49:04Z

batch/src/main/java/org/openmrs/analytics/FhirEtl.java

@@ -192,17 +201,17 @@ static OpenmrsUtil createOpenmrsUtil(String sourceUrl, String sourceUser, String
 		@Setup
 		public void Setup() {
 			fhirContext = FhirContext.forDstu3();
-			fhirStoreUtil = new FhirStoreUtil(fhirStoreUrl, fhirContext);
+			fhirStoreUtil = new FhirStoreUtil(fhirStoreUrl, fhirContext.getRestfulClientFactory());


The above createFhirStoreUtil has a conditional logic for creating FhirStoreUtil (depending on flag values), shouldn't we have the same pattern here? I mean does this work if the fhirStoreUril is a GCP FHIR-store?

bashir2 · 2020-10-30T06:11:30Z

streaming-atomfeed/src/main/resources/hibernate.default.properties

-hibernate.connection.username=root
-hibernate.connection.password=test
+hibernate.connection.username=admin
+hibernate.connection.password=BHPmyMtaLj6S


If these are not standard in OpenMRS, let's revert these (to prevent the impression that there is naything specific with these).

@bashir2 should we use a .templates files / another approach to more easily allow individual user settings to be kept as a hibernate.default.properties file without checking it into github?

bashir2 · 2020-10-30T06:11:55Z

streaming-atomfeed/src/main/resources/hibernate.default.properties

 hibernate.connection.driver_class=com.mysql.jdbc.Driver
-hibernate.connection.url=jdbc:mysql://localhost:3306/atomfeed_client?autoReconnect=true
+hibernate.connection.url=jdbc:mysql://openmrs-hapi-db.chijnl5rwr2c.eu-central-1.rds.amazonaws.com:3306/atomfeed_client?autoReconnect=true&useSSL=false


Ditto, I feel it is better to keep localhost as the default.

bashir2 · 2020-10-30T06:13:13Z

streaming-binlog/src/main/java/org/openmrs/analytics/DebeziumListener.java

-		FhirStoreUtil fhirStoreUtil = new FhirStoreUtil(System.getProperty("cloud.gcpFhirStore"), fhirContext);
-		IParser parser = fhirContext.newJsonParser();
-		return new FhirConverter(openmrsUtil, fhirStoreUtil, parser);
+		FhirStoreUtil fhirStoreUtil = new FhirStoreUtil(System.getProperty("cloud.gcpFhirStore"),


This one should be conditional too; also change the property name from cloud.gcpFhirStore to something more generic.

bashir2 · 2020-10-30T06:17:15Z

common/src/main/java/org/openmrs/analytics/FhirStoreUtil.java

-	// https://github.com/GoogleCloudPlatform/java-docs-samples/healthcare/tree/master/healthcare/v1
-	// TODO: remove redundant resource information if passing a HAPI resource
-	public void uploadResourceToCloud(String resourceType, String resourceId, Resource resource) {
+	public MethodOutcome uploadResourceToCloud(Resource resource) {


Let's drop ToCloud from this method name as it is now more generic.

streaming-binlog/src/main/java/org/openmrs/analytics/FhirConverter.java

pmanko · 2020-11-03T20:19:07Z

@pmanko Thanks for the great work.
do we need to entirely get rid of the http utils ??

would think it would be useful at some point dealing with non FHIR data .

What do you think

@bashir2 @ibacher any thoughts on keeping HttpUtils in case generic requests are needed in the future?

ibacher · 2020-11-04T15:46:45Z

@pmanko I'm all for dropping the HttpUtil class until we have a concrete use-case for it. Generally, I don't love keeping code around just "in case".

mozzy11 · 2020-11-04T15:48:44Z

that makes sense @ibacher

kimaina · 2020-11-05T06:10:53Z

Great work @pmanko! I think we should have an alpha version of the entire pipeline once this PR is merged! Do we have a FHIR store container/docker-compose that I can test it out?

bashir2

Thanks for the updates @pmanko; comments are mostly minor points.

Also can you please add a TESTED field to your PR description. I am actually thinking of creating a template for PR descriptions including a TESTED field because given that we don't have an e2e test yet and three major way of running pipelines, it is good to know what e2e tests have been done manually or not.

bashir2 · 2020-11-05T05:45:32Z

batch/src/main/java/org/openmrs/analytics/FhirSearchUtil.java

@@ -29,6 +29,8 @@

 	private static final Logger log = LoggerFactory.getLogger(FhirSearchUtil.class);

+	FhirStoreUtil fhirStoreUtil;


I don't think this is needed anymore, is it?

bashir2 · 2020-11-05T05:47:40Z

batch/src/test/java/org/openmrs/analytics/FhirSearchUtilTest.java

@@ -14,7 +14,7 @@
 package org.openmrs.analytics;

 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.*;


Please avoid wildcard imports: style guide.

bashir2 · 2020-11-05T05:48:09Z

batch/src/test/java/org/openmrs/analytics/FhirSearchUtilTest.java

@@ -51,6 +52,9 @@
 	@Mock
 	private OpenmrsUtil openmrsUtil;

+	@Mock
+	private FhirStoreUtil fhirStoreUtil;


This should not be needed either.

bashir2 · 2020-11-05T05:49:36Z

batch/src/test/java/org/openmrs/analytics/FhirSearchUtilTest.java

@@ -66,6 +70,8 @@

 	private FhirSearchUtil fhirSearchUtil;

+	private MethodOutcome outcome;


bashir2 · 2020-11-05T05:51:04Z

common/src/main/java/org/openmrs/analytics/FhirStoreUtil.java

-	private static final NetHttpTransport HTTP_TRANSPORT = new NetHttpTransport();
-
-	private HttpUtil httpUtil;
+	FhirStoreUtil(String sinkUrl, IRestfulClientFactory clientFactory) throws IllegalArgumentException {


Can we make this private?

common/src/main/java/org/openmrs/analytics/FhirStoreUtil.java

bashir2 · 2020-11-05T06:15:33Z

@pmanko Thanks for the great work.
do we need to entirely get rid of the http utils ??
would think it would be useful at some point dealing with non FHIR data .
What do you think

@bashir2 @ibacher any thoughts on keeping HttpUtils in case generic requests are needed in the future?

I would say let's remove it if it is not used anywhere; we can always resurrect it from the history if it is needed in the future.

bashir2 · 2020-11-06T18:49:17Z

common/src/main/java/org/openmrs/analytics/GcpStoreUtil.java

+			updateFhirResource(sinkUrl, resource);
+		}
+		catch (Exception e) {
+			System.out.println(String.format("Exception while sending to sink: %s", e.toString()));


Please use log.error.

mozzy11 · 2020-11-10T13:11:01Z

@ibacher @kimaina @bashir2 ,this PR looks good to me.
seems its ready for merge and the PLIR work greatly depends on this.
can we merge it in ??

pmanko · 2020-11-11T18:06:07Z

@ibacher @kimaina @bashir2 ,this PR looks good to me.
seems its ready for merge and the PLIR work greatly depends on this.
can we merge it in ??

@mozzy11 I've been doing some end to end tests to make sure everything is working, so I need to bring some fixes back in. Should be ready to go then.

mozzy11 · 2020-11-11T19:02:03Z

oh sure @pmanko , thats ok

bashir2

Thanks @pmanko for the updates. From your other comments it seems that you are not done with the e2e tests yet but I went ahead and took another look to speed up the review process. It would be great if you can finish this soon as I think this is blocking @mozzy11 (please also update the PR description with a TESTED field).

bashir2 · 2020-11-12T04:55:04Z

common/src/main/java/org/openmrs/analytics/FhirStoreUtil.java

 		}
 	}

 	public MethodOutcome uploadResource(Resource resource) {
+		Collection<IClientInterceptor> interceptors = Collections.<IClientInterceptor> emptyList();
+
+		if (sinkUsername.isEmpty() && !sinkPassword.isEmpty()) {


It seems a ! is missing from the first condition, right?

bashir2 · 2020-11-12T04:57:24Z

common/src/main/java/org/openmrs/analytics/FhirStoreUtil.java

@@ -74,10 +92,10 @@ public MethodOutcome uploadResource(Resource resource) {
 		return responses;
 	}

-	protected MethodOutcome updateFhirResource(String sinkUrl, Resource resource,
+	protected MethodOutcome updateFhirResource(String targetUri, Resource resource,


nit: Is there a particular reason to call this "target" here but "sink" everywhere else? I don't mind either of these choices but I rather be consistent.

bashir2 · 2020-11-12T05:03:43Z

common/src/main/java/org/openmrs/analytics/GcpStoreUtil.java

-			throw new IllegalArgumentException(
-			        String.format("The gcpFhirStore %s does not match Google cloud fhir dataset pattern!", gcpFhirStore));
-		}
+	protected GcpStoreUtil(String sinkUrl, String sinkUsername, String sinkPassword, IRestfulClientFactory clientFactory) {


I don't think BasicAuth makes sense for the GCP FHIR store and regardless we are not using it here. So I suggest that you drop username/password from the constructor signature here to make it more clear and prevent confusion.

Communication with a FHIR-compliant endpoint - like a HAPI FHIR JPA server or Google Healthcare APIs - should follow the specifications of the FHIR standard, and as a result can be facilitated by leveraging a FHIR client implementation like the [HAPI client](https://hapifhir.io/hapi-fhir/docs/client/introduction.html). This commit updates the communication with target FHIR repositories to use this client library.

bashir2

Thanks @pmanko for the tests; LGTM. Just two minor points/questions.

bashir2 · 2020-11-13T17:59:57Z

common/src/main/java/org/openmrs/analytics/FhirStoreUtil.java

+	public MethodOutcome uploadResource(Resource resource) {
+		Collection<IClientInterceptor> interceptors = Collections.<IClientInterceptor> emptyList();
+
+		if (sinkUsername != null && !sinkUsername.isEmpty() && sinkPassword != null && !sinkPassword.isEmpty()) {


These cannot be null can they? Also isNullOrEmpty might be handy.

bashir2 · 2020-11-13T18:03:57Z

common/src/main/java/org/openmrs/analytics/FhirStoreUtil.java

 		}
-		catch (URISyntaxException e) {
-			log.error(String.format("URI syntax exception while using Google APIs: %s", e.toString()));
+		catch (Exception e) {


Are we expecting any exceptions here? Or put it another way: Are you trying to catch all runtime exceptions here? If yes, why? I think it is a better practice to only catch checked exceptions.

pmanko mentioned this pull request Oct 8, 2020

Implemented Parquet output generation for the batch mode. #31

Merged

mozzy11 mentioned this pull request Oct 22, 2020

Use HAPI Client to send resources to a generic FHIR store #29

Closed

pmanko force-pushed the 25-hapi-client-to-sink branch 2 times, most recently from 4be317d to 93a1f56 Compare October 23, 2020 20:37

pmanko marked this pull request as ready for review October 23, 2020 20:45

pmanko force-pushed the 25-hapi-client-to-sink branch from 93a1f56 to 0111f07 Compare October 23, 2020 20:55

pmanko commented Oct 23, 2020

View reviewed changes

batch/src/main/java/org/openmrs/analytics/FhirEtl.java Outdated Show resolved Hide resolved

pmanko changed the title ~~Enable resources to be sent to a generic FHIR store using the hapi client~~ Enable resources to be sent to a generic FHIR store using the hapi client for #25 #29 Oct 23, 2020

pmanko changed the title ~~Enable resources to be sent to a generic FHIR store using the hapi client for #25 #29~~ Enable resources to be sent to a generic FHIR store using the hapi client Oct 23, 2020

pmanko self-assigned this Oct 26, 2020

pmanko force-pushed the 25-hapi-client-to-sink branch from 0111f07 to a21d1e8 Compare October 26, 2020 22:40

pmanko requested review from bashir2 and mozzy11 October 26, 2020 22:40

ibacher reviewed Oct 27, 2020

View reviewed changes

pmanko force-pushed the 25-hapi-client-to-sink branch 2 times, most recently from d5f2565 to c17997b Compare October 28, 2020 00:01

bashir2 reviewed Oct 30, 2020

View reviewed changes

pmanko force-pushed the 25-hapi-client-to-sink branch 3 times, most recently from 74a77c7 to ccf7807 Compare November 3, 2020 20:16

pmanko force-pushed the 25-hapi-client-to-sink branch 2 times, most recently from c0909f9 to 690b080 Compare November 3, 2020 21:27

pmanko requested a review from bashir2 November 3, 2020 21:27

bashir2 reviewed Nov 5, 2020

View reviewed changes

bashir2 reviewed Nov 6, 2020

View reviewed changes

bashir2 mentioned this pull request Nov 6, 2020

Make exported resource IDs consistent #55

Closed

pmanko force-pushed the 25-hapi-client-to-sink branch from a1bb7ed to 0d4804c Compare November 9, 2020 22:27

pmanko force-pushed the 25-hapi-client-to-sink branch 2 times, most recently from 4cd0bdb to ac99269 Compare November 10, 2020 14:45

bashir2 reviewed Nov 12, 2020

View reviewed changes

mozzy11 mentioned this pull request Nov 12, 2020

standadize argument passing using commandline args #54

Closed

pmanko added 4 commits November 12, 2020 15:39

Fixes for @ibacher review

04c8a75

Fixes for @bashir2 review

eb92c84

Fixes for @bashir2 review

e3c9f8b

pmanko force-pushed the 25-hapi-client-to-sink branch from 1860c6e to aa2898f Compare November 12, 2020 23:42

Fixes for @bashir2 review

4597929

pmanko force-pushed the 25-hapi-client-to-sink branch from aa2898f to 4597929 Compare November 12, 2020 23:47

pmanko requested a review from bashir2 November 13, 2020 15:01

bashir2 reviewed Nov 13, 2020

View reviewed changes

Fixes for @bashir2 review

0e05cf4

pmanko force-pushed the 25-hapi-client-to-sink branch from 3da71fc to 0e05cf4 Compare November 13, 2020 22:22

pmanko requested a review from bashir2 November 13, 2020 22:39

Update FhirStoreUtil.java

b95c11c

bashir2 merged commit ebe1eb2 into google:master Nov 15, 2020

	FhirStoreUtil fhirStoreUtil = new FhirStoreUtil(options.getFhirStoreUrl(), fhirContext);
	FhirStoreUtil fhirStoreUtil = new FhirStoreUtil(options.getFhirStoreUrl(), fhirContext.newRestfulGenericClient());

		@@ -194,15 +202,15 @@ public void Setup() {
		fhirContext = FhirContext.forDstu3();
		fhirStoreUtil = new FhirStoreUtil(fhirStoreUrl, fhirContext);

		@@ -29,6 +29,8 @@

		private static final Logger log = LoggerFactory.getLogger(FhirSearchUtil.class);

		FhirStoreUtil fhirStoreUtil;

		@@ -66,6 +70,8 @@

		private FhirSearchUtil fhirSearchUtil;

		private MethodOutcome outcome;

Enable resources to be sent to a generic FHIR store using the hapi client #30

Enable resources to be sent to a generic FHIR store using the hapi client #30

Conversation

pmanko commented Oct 7, 2020 • edited

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

mozzy11 commented Oct 28, 2020

bashir2 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

pmanko commented Nov 3, 2020

ibacher commented Nov 4, 2020

mozzy11 commented Nov 4, 2020

kimaina commented Nov 5, 2020

bashir2 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

bashir2 commented Nov 5, 2020

Choose a reason for hiding this comment

mozzy11 commented Nov 10, 2020

pmanko commented Nov 11, 2020

mozzy11 commented Nov 11, 2020

bashir2 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

bashir2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmanko commented Oct 7, 2020 •

edited