Integration with OpenEMPI

[mogoodrich (Mark Goodrich)]

@shaoyuan and since I don’t think 2.1.4 is out yet, you can try setting the war to 2.1.4-SNAPSHOT

[shaoyuan (Shao Yuan Chew Chia)]

Thanks for your input @mogoodrich and @dkayiwa,

That seems to have fixed it. So I used the sdk to set up the reference application 2.8.0 distribution, then used openmrs-sdk:deploy to deploy the platform 2.1.4-SNAPSHOT to the server and then added in my merged files and the conditions table was set up so I didn’t get that error anymore. thanks!

[shaoyuan (Shao Yuan Chew Chia)]

hi @pgesek,

So we were wondering about the mllp sender (openmrs-module-registrationcore/api/src/main/java/org/openmrs/module/registrationcore/api/mpi/pixpdq/MllpSender.java).

So we could change the global property: registrationcore.mpi.Hl7implementation from registrationcore.mpiHl7v2HttpSender to registrationcore.mpiHl7v2MllpSender in order to use it, but under what circumstances would someone want to do this? Is it for talking directly to OpenEMPI or also for routing through OpenHIM? and if so what other configuration/setup needs to be done to get this to work? We could test it and help create some documentation for it. Thank you!

Best regards,

Shao Yuan

[pgesek (Paweł Gesek)]

Hello,

the MLLP sender will use the MLLP protocol as the name suggests. Yes, MLLP can be used to directly interface with OpenEMPI or other PIX/PDQ implementation through a TCP socket. Since MLLP does not provide any security on its own, additional methods for securing the channel need to be employed, a VPN or an SSH tunnel for example.

We opted for HTTP and basic authentication instead. The HTTP sender sends the HL7v2 message through HTTP to OpenHIM, which then routes the message to OpenEMPI using MLLP. In our case the OpenEMPI ports that accept MLLP messages (3600 & 3700) are not open to the world.

Regards, Paweł

[shaoyuan (Shao Yuan Chew Chia)]

Hi Pawel,

Thanks for that clarification!

So right now I’m looking over the tests and beginning to understand some of the code better and I have a few questions. I have three suggestions marked by the but I’m not sure whether there are considerations that I am not seeing…

1. Currently there is only one test class in the pixpdq folder, org.openmrs.module.registrationcore.api.mpi.pixpdq.PdqSimilarPatientsSearcherTest ideally there should be about one test class for each class in that folder correct?

2. Referring to org.openmrs.module.registrationcore.api.mpi.pixpdq.PdqSimilarPatientsSearcher

   1. Referring to the find method, for constructing the identifierQueryParams for the PDQ message, only the last identifier to be added to the set of patient identifiers is used for the query, this leads to the following behaviour in the iSantePlus SEDISH Demo:

      1. Start Registering a patient, set ST Code to 6757, press enter, it submits the PDQ and gets back one similar patient, Jean Barron.

         

      2. Next, set NAT990999283 for Code National, press enter, it will submit this new ID as the identifierQueryParams.

         

      3. However, this is where I think the undesirable behaviour is: if you go back and change the ST Code and press enter, for example, changing it to 98761, it will submit a PDQ message but the identifier it sends is the Code National and not the recently changed ST Code, thus the matching patient it gets back from the MPI is still Jean Paul and not Jean loubeau

      
      
      • I also noticed that this pattern is different than for the given name and family name, where it will send one if it only has one, and will send both it has both.
        • Tangent: I think it would make sense to add Middle Name to the PDQ under @PID.5.3 if the patient has it, since the reference application form comes with it and toMatchList checks for it.
          
      • Might it be better to send all available Identifiers? According to the documentation, you can send multiple sets of identifiers in the query
        
        And I verified this by testing with a local OpenEMPI instance by sending a PDQ with both the ST Code and Code National.
      • In fact, I was wondering about the design considerations for having distinct identifierQueryParams vs nameQueryParams and sending the query with preference for name, and if no name is present then for identifier. (See lines 77 to 87 in find method)
      • Would it be better to send all available demographic data? So as the registration form gets filled up you send more and more data of all the types? Right now, for example gender and addresses are not currently used for PDQ.

   2. In the toMatchList method, is there a reason that there are no checks for matching identifiers? Or phrased differently, that matching identifiers do not get added to the getMatchedFields List?

Thanks for your help in clarifying these things

• Shao Yuan

[shaoyuan (Shao Yuan Chew Chia)]

Also, a question about PixPatientExporter: (org.openmrs.module.registrationcore.api.mpi.pixpdq.PixPatientExporter)

Currently, the method exportPatient assumes that if the mpiPatient that is returned is not null, it means that a patient with the sam IDs already exists. What we have found is that if a patient with the same MPI Global Identifier Domain ID exists, it will not create the patient that was sent, but the PdqPatientFetcher will get back the patient originally in the MPI. I have explained the situation here: https://wiki.openmrs.org/display/docs/Registration+Module (that I am in the process of editing) under the section: Errors -> Patient Identifier Collision

given that there is no error in the PIX response (as shown in my screen shots in the other page) I think we can get rid of this undefined behaviour by doing a check to see if the name, gender and address fields of the patient passed as input to exportPatient is the same as that of the mpiPatient returned by pdqPatientFetcher.fetchMpiPatient? If they do not match we can throw a new MPI exception.

Does this seem like a good approach?

[shaoyuan (Shao Yuan Chew Chia)]

Hi @pgesek,

Sorry to be asking you so many questions, but here is another one regarding org.openmrs.module.registrationcore.api.mpi.pixpdq.PdqPatientFetcher

What is the purpose of the method: toPatientFromMpiPatient? It has the comment “it is a hack in order to save the MpiPatient class to DB (converting to the Patient class)” but I don’t quite understand this.

First, in line 54, the mpiPatients is a list of Patients and not MpiPatients, so I don’t see where the MpiPatient class is involved or what needs to be converted to the Patient class? Second, I dont understand what it means by saving the class to DB?

I tried commenting out

line 58: return toPatientFromMpiPatient(mpiPatients.get(0)) and just returned return mpiPatients.get(0);

and ran openmrs and created a patient and everything seemed to work fine? Hope you can shed some light on the thinking behind this thank you!

Best regards,

Shao Yuan

[shaoyuan (Shao Yuan Chew Chia)]

Hi Everyone,

I went ahead and made the changes I proposed and everything seems to be working. We found some inconsistent behaviour/ behaviour that doesn’t seem to comply with the HL7v2 standard and have some other problems with the way OpenEMPI responds to PDQ messages and generates matches which @jiahaochua tried to post on the OpenEMPI forum but his posts haven’t been approved and it seems like no one is watching those forums so…

Anyways, what I would like to field your collective advice about is a specific issue which we’ve been experiencing since the start:

Description

The registerPatient method in RegistrationCoreServiceImpl saves the patient using the patient service, then creates the patient registration message then fires the message. The PatientActionListener listens to the message and tries to retrieve the patient using the patient service via the patient’s uuid which was received via the message. There are instances when the PatientActionListener tries to retrieve the patient and it returns a null patient. Usually the first (or first and second) patient that is registered after starting OpenMRS results in the Null Pointer Exception occurring.

Diagnosis

Since these two things happen asynchronously, we guessed that the problem is that there are instances when the PatientActionListener tries to retrieve the patient before it is fully saved, resulting in it returning a null patient.

In other words, we assumed it was a Hibernate session issue.

Attempted Solution

Attached are the changes which constitute the first attempted fix suggested by @mogoodrich in RegistrationCoreServicesImpl and PatientActionListener, as well as the console output when i used the registration app to create the patient for the first time after booting up OpenMRS. After one or two patients, it usually fixes itself and starts working (meaning properly registering the patients in the MPI, in all cases the patient is created locally)

So its interesting to note that it seems like the patientservice works in getting back the patient in RegistrationCoreServicesImpl but not in the PatientActionListener (which assumedly is running in a different thread), even after flushing the session.

Do you guys have any other suggestions for how to fix this issue? this is the main blocker and after this i think i can submit the pull request.

• Shao Yuan

[shaoyuan (Shao Yuan Chew Chia)]

Note that this is what the console log of a successful MPI upload looks like:

[wyclif (Wyclif Luyima)]

Is this happening in a unit test or when you’re running the application?

[shaoyuan (Shao Yuan Chew Chia)]

hi @wyclif,

This is happening when I’m running the application, I have made some changes as per @mseaton’s suggestion

one (probably more correct) option would be to change the event listeners from keying off the PatientRegistrationEvent and instead key off of the more generic OpenmrsObject create/update events, and move the listener to a SubscribableEventListener that subscribes to Patient objects that are CREATED or UPDATED. This will have the benefits of:

• Only firing off once the transaction is completed and the data is in the DB and accessible to all Sessions
• Only firing off if the transaction succeeds and doesn’t get rolled back
• Will update the MPI regardless of how the Patient is modified (currently this is only happening during the Registration process)

Of course, if this results in hundreds of updates to the MPI due to excessive Patient UPDATE events, we’ll need to re-think this. But might be worth a quick attempt.

And this has fixed the problem but created some side effects:

First

The sequence of events is like this: RegCoreServiceImpl.registerPatient → savePatient → PatientCreatedListener.onMessage → PDQ to check whether patient exits on MPI → PIX to save patient to MPI → PDQ to check what the created MPI ID is → savePatient to update the patient’s local record → PatientUpdatedListener.onMessage → PIX to update patient in MPI Previously the last two steps didn’t take place, but since our listener now listens to all OpenMRS Patient Object CREATED and UPDATED events, it does this slightly redundant bit. It doesn’t create any problems, but in this specific case it is slightly redundant? thoughts on whether we should/what we should do about this?

Second

When importing a patient from the MPI, it uses savePatient as well which also activates PatientCreatedListener which then tries to create the patient on the MPI but is stopped before it sends the PIX message by the first PQD check. ERROR - PixPatientExporter.exportPatient(62) |2018-07-18 11:16:54,524| org.openmrs.module.registrationcore.api.mpi.common.MpiException: Patient with that identifier is already present on MPI. Unable to create patient Exception in thread “Thread-976” org.openmrs.module.registrationcore.api.mpi.common.MpiException: PIX patient push exception occurred And this error is captured by the error handler and logged into the PIX Exceptions which then will keep retrying it…

Another Bug: Unable to Import MPI Patient without ECID Identifier

There is a hard coded reference to ‘ECID’ in openmrs-module-registrationapp/omod/src/main/webapp/resources/scripts/registerPatient.js

Thus if the identifier name that is set for the MPI Global Domain Identifier is not ‘ECID’ it will break and not be able to import the patient from the MPI.

What we need to do is to reference the global property that says what the MPI Global Domain Identifier is/ or a service that has access to it, e.g. mpiProperties.getMpiPersonIdentifierTypeUuid() and hopefully there is a way to match that with the javascript identifier object?

How can we do this?

• Shao Yuan

[wyclif (Wyclif Luyima)]

It appears like the listener is listening for the wrong events i.e. those fired from the registrationcore module’s service method as seen here, this code fires transactions before the method returns which implies before the transaction is committed, that’s an application event which differs from the hibernate based ones fired from the event module after a commit has happened, so yes it sounds like the listener needs to be changed to listen for the generic patient created events from the events module to avoid that issue.

[mseaton (Mike Seaton)]

@shaoyuan / @wyclif, it seems the main question is how to deal with Event handling situations where recursion can occur. In the above scenario:

1. Patient Save -> Hibernate Interceptor -> Fires Update Event
2. In a different thread, Event Listener handles Update Event, retrieves and sets additional information on the patient, and saves the Patient with this new information. This leads to step 1 again.

Is there a pattern or solution that has been developed to detect/avoid this infinite recursion? @SolDevelo, was this something that you had to deal with for Sync2? @darius / @dkayiwa / @wyclif , thoughts?

Thanks, Mike

[wyclif (Wyclif Luyima)]

@mseaton that can also be an issue too, to avoid that it goes back to something I suggested when we were addressing an issue SolDevelo was running into with sync2, we should possibly have two call backs (events) from the interceptor that could be synchronous and asynchronous, the current one fires events asynchronously outside of original transaction after it completes, we need one that fires them synchronously within the same transaction before it completes, this would solve the issue you described.

But based on the current state of the events module, the work around to the recursion issue has to be implemented in the listening code, you can use a flag that you turn off before you save the updates in the listener and clear it afterwards.

[mseaton (Mike Seaton)]

@wyclif, from earlier comments this can’t be synchronous within the transaction because we are hitting an external service (MPI) which may or may not be available in real-time. Basically:

1. Patient is saved -> We update the MPI
2. MPI is updated -> We update the Patient
3. recurse

Seems like what we would need to do is actually look at what specifically is being changed on the Patient at the OpenMRS or MPI level and only initiate a subsequent update if we confirm that meaningful data has changed… Other thoughts - not entirely sure how setting a flag would work…

Thanks, Mike

[wyclif (Wyclif Luyima)]

Then in that case, in step 2 you need to use a flag that you check first to avoid recursion, I’d think the code snippet below would work.

//probably this is a thread local if onSavePatient() is accessible by multiple threads
boolean busy = false;

onSavePatient() {
    if(!busy){
        updatePatientInMPI();
        busy = true;//This will avoid recursion when you call updatePatientInOpenMRS
        try{
            updatePatientInOpenMRS();
        }finally{
            busy = false;
       }
    }
}

[shaoyuan (Shao Yuan Chew Chia)]

@mseaton So just to quickly clarify, there is no case where Infinite recursion occurs because

Patient Registration:

1. Patient Save -> Hibernate Interceptor -> Fires CREATED Event
2. In a different thread, Event Listener handles CREATED Event, creates patient in the MPI. Retrieves additional information from the MPI and saves the Patient locally with this new information. This leads to step 1 again. (because the patient is already created)
3. Hibernate Interceptor -> Fires UPDATED Event
4. In a different thread, Event Listener handles the UPDATED Event, pushes the update to the MPI because it doesn’t know that it came from the MPI.

@wyclif, I’m not sure if I have a firm grasp on this but I feel like since PatientCreatedListener is distinct from PatientUpdatedListener this wouldn’t work because they wouldn’t be accessing the same instance and reference the same busy variable. So the key pieces of the puzzle are:

1. org.openmrs.module.registrationcore.api.impl.RegistrationCoreServiceImpl#registerPatient
   • That initiates the first patientService.savePatient and once that transaction is completed the hibernate interceptor fires the CREATED Event
2. org.openmrs.module.registrationcore.api.impl.PatientCreatedListener#performMpiAction
3. org.openmrs.module.registrationcore.api.impl.PatientUpdatedListener#performMpiAction

My understanding is that the flag needs to be set in either the registerPatient method, or in the PatientCreatedListener and then checked in the PatientUpdatedListener

Patient Import:

So this is a completely different case where during the process of filling out the registration form, the “import and open” button is clicked and has the following key pieces:

1. org.openmrs.module.registrationcore.api.impl.RegistrationCoreServiceImpl#importMpiPatient
   • That fetches the patient from the MPI using a PDQ and then initiates patientService.savePatient to persist the patient locally.
2. org.openmrs.module.registrationcore.api.impl.PatientCreatedListener#performMpiAction

The problem is that patientService.savePatient in importMpiPatient triggers the step 1 in the Patient Registration flow which errors out because we have a check to make sure that you can’t create a patient on the MPI that already exists on the MPI.

So we need a flag that is set in importMpiPatient and then checked in PatientCreatedListener to know that the patient came from the MPI and this is not the registration process and so should be ignored.

With these two propositions (the stars) I’m not sure about the scope/ what structure/ context should be used so that these disparate elements can talk to each other

Also a quick bump to “Another Bug: Unable to Import MPI Patient without ECID Identifier” I’m pretty clear on what the solution should be I’m just not sure how to do it… so some thoughts would be helpful!

• Shao Yuan

[wyclif (Wyclif Luyima)]

@shaoyuan

@mseaton So just to quickly clarify, there is no case where Infinite recursion occurs because

What @mseaton is saying is that recursion would occur only when a patient is updated not created, note that you would need to update their record in the MPI whenever they are updated in OpenMRS and that’s when it becomes an issue. Also this implies that the pseudo code I posted only applies to the update listener.

[shaoyuan (Shao Yuan Chew Chia)]

Mm sorry i’m Not sure if I’m misunderstanding but When Updates are pushed to the Mpi nothing is returned from the mpi that needs to be saved so it doesn’t trigger another update?

[wyclif (Wyclif Luyima)]

@shaoyuan

Mm sorry i’m Not sure if I’m misunderstanding but When Updates are pushed to the Mpi nothing is returned from the mpi that needs to be saved so it doesn’t trigger another update?

Then it’s a non issue, one of @mseaton’s responses implied that on update you update the MPI and get back something to update the user in OpenMRS.

@mseaton

@shaoyuan / @wyclif, it seems the main question is how to deal with Event handling situations where recursion can occur. In the above scenario:

Patient Save -> Hibernate Interceptor -> Fires Update Event In a different thread, Event Listener handles Update Event, retrieves and sets additional information on the patient, and saves the Patient with this new information. This leads to step 1 again.

That’s where I got the impression