Migrating emrapi module from GlobalProperties to metadata mappings

Development
1

Hi everyone,

During Metadata Mapping Spring, as @raff announced here, we extended functionality of Metadata Mapping module, making it possible to get rid of most global properties in favour of metadata mappings. As proof of concept we choose to migrate emrapi module.

We tried to make implementing migration as easy as possible, and provide necessary backwards compatibility. Metadata Mapping module api provides convenient converter from global property:

    new GlobalPropertyToMappingConverter<Location>(emrapiMetadataSource) {
        public Location getMetadataByUuid(String s) {
            return Context.getLocationService().getLocationByUuid(s);
        }
    }.convert(EmrApiConstants.GP_UNKNOWN_LOCATION);

Example of saving new metadata term mapping:

    MetadataTermMapping termMapping = metadataMappingService.getMetadataTermMapping(sourceName, Mappingcode);
	termMapping.setMappedObject(metadataObject);
	metadataMappingService.saveMetadataTermMapping(termMapping);

Migration from global properties is major change, and require adjustments in Reference Application modules. There are PRs in emrapi-module, referencemetadata-module and referenceapplication-module.

We are aware that emrapi is widely used module, we would be very thankful for your thoughts about this design.

cheers, PaweƂ

1 Like
2

@gutkowski so where are the mappings to be stored now? Can you provide a link to an example

3

@ssmusoke mappings are stored in database and handled by MetadataMappingService. Mapped object can be accessed with this service’s method:

<T extends OpenmrsMetadata> T getMetadataItem(Class<T> type, String metadataSourceName, String metadataTermCode);

For example, EmrApiProperties uses it with wrapper method:

    protected <T extends OpenmrsMetadata> T getEmrApiMetadataByCode(Class<T> type, String code, boolean required){

    T metadataItem = metadataMappingService.getMetadataItem(type, EmrApiConstants.EMR_METADATA_SOURCE_NAME, code);
    if(required && metadataItem == null){
        throw new IllegalStateException("Configuration required: " + code);
    } else {
        return metadataItem;
    }
}

which is invoked to fetch specific metadata object:

public EncounterRole getClinicianEncounterRole() {

	return getEmrApiMetadataByCode(EncounterRole.class, EmrApiConstants.GP_CLINICIAN_ENCOUNTER_ROLE);
}

Besides, MetadataMappingTerm can be mapped with MappingSet. Effectively, it replaces storing multiple uuids in single GlobalProperty string, and parsing it every time, like it was done before. Instead, multiple items can be accessed in much more convenient manner:

protected List<PatientIdentifierType> getPatientIdentifierTypesByCode(String code) {

    MetadataSet metadataSet = getEmrApiMetadataByCode(MetadataSet.class, code);
    return metadataMappingService.getMetadataSetItems(PatientIdentifierType.class, metadataSet);
}

To sum up, it makes storing module’s metadata much more structured.

4

So in the big picture, you’re saying that we’ll start using the new metadatamapping module across various reference application => lots of distributions (including Bahmni, PIH-EMR, and the Uganda one) will need to start including the metadatamapping module.

5

Emrapi is already requiring metadatasharing (source), which is requiring metadatamapping (source), so if I understand this correctly, these distributions already have to include metadatamapping module?

6

True, they probably include this, but their developers have probably been ignoring it up to this point.

So, starting now we’re saying they’ll need to care.

7

@darius @gutkowski Uganda is using metadata mapping and sharing for configuration.

Can you point to any examples how to migrate from global properties to MDS?

8

@darius You’re right, that’s why we are waiting with commiting these changes and asking for opinions. All feedback is welcome.

@ssmusoke example of migration is PR in emrapi-module, (it is cluttered with changes in dependencies, because switch to new core version in Reference Application 2.5 caused some compatibility issues). GlobalPropertyToMappingConverter ensures that after upgrade to new version of module, its configuration is properly preserved.

Migrating module is quite easy if access to global properties is properly encapsulated, like in emrapi’s EmrApiProperties. There is some additional work to do if distribution module access module’s metadata directly, like Reference Application module do, but it is easy to fix : example

9

Thanks @gutkowski

Just to clear when you say there is a backwards-compatibility, this means that after an upgrade the EMR-API module ill allow referencing via mapping, but will support global property mapping, thereby allowing existing implementations to migrate from global properties to metadata mapping at their leisure?

Or does the upgrade actually change the existing, relevant global properties to metadata mappings?

Take care, Mark

10

Yes, it does. With current implementation of migration, backwards compatibility means that emrapi will automatically map objects which uuids where stored in GP before.

To explain it on example, let’s say that implementation has previous version of emrapi and custom clinician encounter role. When module is upgraded, it finds this EncounterRole object by uuid stored in GP and creates mapping to it, preserving configuration. If Global property is blank, empty mapping with no mapped object is created. From now on if system administrator wants to modify clinician encounter role, he would do it using new metadatamapping UI (it is in code review stage, issue: MAP-18).

11

I have a comment from the code snippets above. The ModuleProperties class in emrapi is supposed to be a utility class that could be extended by other modules to provide a config bean with good semantics. (Here’s a random example from a PIH module.)

The metadatamapping module is now a more appropriate place for this helper base class, so I suggest that you explicitly write a ModuleProperties class in metadatamapping that replaces the emrapi one (and deprecate the emrapi one).

12

Thanks for the explanation, @gutkowski This would require a little rework on our part, since we programmactically set global properties via a configuration module during startup. However, as long as there is a straightforward API call to make to set up metadata mappings, switching this up sounds like it shouldn’t be too big deal.

Does the module get rid of the global property after the change?

Mark

13

@darius that’s great idea, I will update the PR!

Good question, I’m not sure what we should do with these unnecessary global properties, currently they are left as they are. What’s your opinion?

14

Haha, I was going to offer my opinion when asking, but then realized I wasn’t 100% sure.

I don’t think it should delete them automatically–too risky. But it would nice if it could flag to an administrator that they are no longer needed. Not sure how that would work.

Maybe just something manually for now–a list in the README.md? Or, it looks like there’s a “description” field on the global property object–maybe a “replaced by metadmapping xyz” could be appended to that description?

Anyway, I’d say keep them, and make sure the code is smart enough not to remap them on the next startup.

Mark

15

Haha, yeah, that’s a tricky one.

Documentation in README won’t hurt, that’s sure. Modifying description field in global property object seems good idea as well, but problem is that eg. Reference Application UI doesn’t show descriptions in global property management section, so it is just partial solution


I agree that we need to keep them, and at the moment only thing that triggers creating new mapping is lack of term mapping for specific code.

Thanks for your help!

EDIT: @raff thinks we should keep the global property, but ‘break’ its value, setting it to migrated to metadata mapping or something. Considering pros and cons, maybe that’s the best solution, a bit risky, but other modules should access emrapi 's GP by EmrApiProperties anyway


16

Unfortunately, Bahmni is using these global properties at other places in our omod. emr.primaryIdentifierType is used in a lot of our reports (which use sql queries) and also emr.extraPatientIdentifierTypes. Also, the emr.admissionEncounterType is used.

Looks like after the emrapi is up-and-running, we are replacing the global_property value to the value “This global property had been migrated to metadata mapping in source ‘org.openmrs.module.emrapi’ with code ‘&s’”. See the code here and here

Sorry, its too late to comment. But following is a route we can take

  1. In short term, before the emrapi migration starts (i.e. as part of the deployment process and before tomcat starts), we need to run a migration to fill up metadatamapping_metadata_term_mapping with the migrated global_property values (obviously with all the associated mappings like metadatamapping_metadata_source, metadatamapping_metadata_set). If the mappings are available, there is a logic to skip it.
  2. In medium term, move away from these global properties to bahmni specific property names.

What do you think? Is this approach fine? Or we have a better solution? @darius @angshuonline @vinay @gutkowski @mogoodrich

17

For our situation, I believe we only access the Global Properties via the EmrApiProperties, so assuming we migrate all the global properties properly to metadata mapping we should be all set, though @bharatak’s point about directly accessing global properties is something we should definitely confirm we don’t do–and sounds like a definite blocker for Bahmni.

In terms of actually doing the migration, we currently set all our global properties programmatically at OpenMRS startup in our “PIH Core” module. My plan (I actually started working on it yesterday) is to change this code so that it sets things up as mappings instead of global properties and removes the old global properties. At that point, the code that was added to EMR API to migrate from global properties to mappings shouldn’t have to do anything, so if I watch what happens at startup in a test environment I should be able to confirm that I’ve properly migrated anything.

We use metadata deploy to set up our metadata, so to facilitate this yesterday I added some metadata deploy utility classes to metadata mapping, which I plan to commit and push up after I test–hopefully within the next couple days, assuming all goes well and I don’t get pulled in another distraction.

So that’s our plan–I’ll sent out links to any commits I make. Unfortunately, still doesn’t provide any help with the situation where GPs are accessed directly via things like SQL queries–don’t know if there’s really a way around that besides manually changing this.

Take care, Mark

1 Like
18

@bharatak, the motivation behind setting GPs to the “migrated” message was that it should be clear that the only place to edit mappings is now the metadata mapping module, because there is no way to make GPs read only.

Are those properties meant to be modified by implementations or are they only set in code? If it is the latter then in short term I would just let the migration to happen and then set the GPs to the values you had there originally. It should be easier.

In medium term I would really like you to pick up the new approach to manage mappings instead of global properties like @mogoodrich :slight_smile: Is there anything we can improve to help you adopt that approach?

@mogoodrich thanks for working on the metadata deploy!

19

Thanks @raff! I’ve added metadata deploy support to metadata mappings for MetadataSource and MetadataTermMapping (I haven’t committed or pushed it up yet thought).

I haven’t added anything for MetadataSet and MetadataSetMember
 not sure if I will or not, but, regardless, I have some questions on how to interact with Set and SetMembers, specifically regarding how you update a set


Looking at how the “createExtraPatientIdTypesSetIfMissing” method works in EmrApiActivator, it looks like one can create and populate a set like this:

MetadataSet set = metadataMappingService.saveMetadataSet(new MetadataSet());

PatientIdentifierType type1 =  // assume some existing type
PatientIdentifierType type2 = //
PatientIdentifierType type3 = //

metadataMappingService.saveMetadataSetMember(extraPatientIdTypesSet, type1);
metadataMappingService.saveMetadataSetMember(extraPatientIdTypesSet, type2);

But now say I want to remove “type1” and “type2” from the set (and therefore have them retired or just plain deleted) and replace them with type3. What would be the way to do this, or am I approaching the problem the wrong way? :slight_smile:

Basically, we have code in our distribution that populates the extra patient identfier GP on startup, and I need to switch that to create the equivalent set on each startup. So this method should be idempotence if there are no changes to the set, but should update the set if there are changes.

Thoughts?

Thanks! Mark

20

Also, I’m planning on modifying the EMR API code so that when it creates the MetadataSource for EMR API it assigns a uuid to the set, which I’m storing as a constant. I need this so that I can safely deploy and reference it via metadata deploy.

@raff do you see any issues with this?

Mark