Metadata Term Mappings unique on source/term?

1

It looks like within the database configuration, metadata term mappings are enforced to be unique within a single source:

I believe this is correct… and is, in fact, is a core intent of metadata mappings… to provide unique source/code pairs to reference metadata so we can get away from references them by uuid.

However, there doesn’t seem to be any code (or Hibernate) level enforcement of this. This seems incorrect?

Also, to make mappings truly easy-to-use, I would think the saveMetadataMappingTerm() method should be smart enough to update when required.

For instance, let’s say make the following two calls:

saveMetadataTermMapping(new MetadataTermMapping(emrApiSource, “defaultLocation”, Location.class.getName(), “uuid-of-original-default-location”)

saveMetadataTermMapping(new MetadataTermMapping(emrApiSource, “defaultLocation”, Location.class.getName(), “uuid-of-updated-default-location”)

I believe this currently will pass at the application level, and fail at the database level. At minimum, this should fail at the application level, but I believe it would be helpful to do the “right” think and update an existing mapping–ie, in this case you’d end up with a single mapping, mapping emrapi.defaultLocation to the location with uuid “uuid-of-updated-default-location”.

If not, I’m thinking we should at least provide a saveOrUpdateMetadataTermMapping method… or, at minimum, there should be some sort of utility method provided by the metadata mapping module to update the value of a mapping via a single line.

Thoughts? Am I misunderstanding anything here?

Thanks! Mark

@raff @kosmik @burke @darius @mseaton @gutkowski

2

I agree that the metadatamapping module should provide a helpful way that an implementation’s distro module can set configuration values with a one-line call.

I would have thought:

// assume emrapi has already created emrapi.defaultLocation with datatype Location
setMetadataMapping("emrapi", "defaultLocation", locationWithUuidSet);

But I haven’t looked at the underlying code, so this is just a handwavy guess.

Basically if PIH is writing a distro module that’s going to configure metadata mappings for a bunch of other modules, it should have a convenient method for doing so that works like “set configuration properties”, not like “create a metadata mapping”.

3

Currently, you need 3 lines to do the job of updating a mapping: MetadataTermMapping mapping = service.getMetadataTermMapping(“emrapi”, “defaultLocation”); mapping.setMetadataUuid(“new-uuid”); service.saveMetadataTermMapping(mapping);

I like the one-liner suggested by Darius. Created MAP-33.

I don’t like supporting overwriting with saveMetadataTermMapping(new MetadataTermMapping(…)) as it may introduce issues with attached/detached Hibernate objects, if you previously fetched a mapping you want to overwrite in the same session.

Adding duplicate check in code in saveMetadataTermMapping is needed too… Created MAP-34.

Thanks Mark!

4

Thanks @darius @raff.

Yeah, thinking it through a little more I agree with Rafal that a new method is best and leaving “saveMetadataTermMappings(MTM)” to follow the standard conventions of OpenMRS objects. I should be able to pick up MAP-33 and probably MAP-34 as well.

That being said, seems like for Metadata Mappings we should steer API consumers to use the new convenience methods we create? Although all the Metadata domain objects are just new OpenMRS Metadata, the I would think the average API consumer shouldn’t have to worry about that, as they should make accessing and mapping existing metadata easier, not more complicated?

For MAP-33, I believe it should also create a new MTM if one doesn’t exist, so I will implement it that way if there are no objections.

I also started thinking about Metadata groups, and was going to ask about a potential design, but I see you’ve included it in the ticket @raff and the design you came up with was what I was thinking, so I’ll just go with that… also adding the create-if-doesn’t exist, ie:

// fetch existing
mapping = service.getMetadataTermMapping("emrapi", "defaultLocations");
if mapping == null {{ // create new }}
if mapping != null && mapppedObject not instance of MetadataSet {{ // throw Exception }}
else {{ // update existing based on logic described in ticket -- add any new set members, retire old ones }}
5

Agree, we should steer API consumers to use the convenience methods. Shall we move the less convenient methods out to a different service to make it clear?

Yes, I meant supporting create-if-doesn’t exist for both new methods.