I was wondering if systematically adding a concept-descriptor member to BaseOpenmrsMetadata could not be a simple way to delegate i18n support for virtually all metadata classes?
Very much in the way it is in fact done to a certain extent with Program, ProgramWorkflow, … etc.
In a very basic nutshell, something like this:
public abstract class BaseOpenmrsMetadata ... {
...
private String name;
...
private Concept descriptor;
...
@Override
public String getName() {
if (descriptor != null) {
return descriptor.getName(Context.getLocale()).getName();
}
return name;
}
...
}
The problem of course is that this will break a number of code bases out there that currently rely on the name as a primary key to fetch metadata instances through using getName() … which I think is never a great idea to be honest. But perhaps such use of the getter could be progressively discouraged after introducing the above logic. Just throwing the idea, I’d be curious to know whether you think this is a big no-no or on the contrary whether this could be the way to go.
My gut reaction is that I’d be reluctant to introduce the complexity of linking a concept (and the complexity of concept names in particular) to every type of OpenMRS metadata, but I do agree there’s some appeal in it… would be interested in what others think.
I like the push towards consistency across our API. I had actually suggested the opposite approach years ago (namely to deprecate and remove concept from program/workflow/state), and there is still an active ticket for this that gets bounced around a lot today, and which I’ve been recently advocating to abandon due to the fact that I fear it will cause more problems (due ot backwards-compatibility headaches) than is worth the benefit: https://issues.openmrs.org/browse/TRUNK-1748.
I think we should be careful about your design proposal for a few reasons:
This would create a situation with a persisted “name” property on an object, but for which getName() and setName() do not operate like simple getters and setters and/or have side-effects.
This would create a situation with a property “getter” hitting the Context and possibly (as in this case) requiring a hibernate session and database query to successfully work.
Concepts have a lot of baggage in the API - they have lots of lazy-loaded properties (including concept names I believe), and they have a history of resulting in annoying to debug LazyInitializationExceptions and can be tricky with some common paradigms like object serialization (eg for reporting and REST web services). So there might be some unknown compatibility issues and downstream problems that this introduces that will cause additional work to sort out.
I’ve actually been thinking a lot more these days about what the minimum is that we actually need to store in the database within metadata tables. Given the way many implementations (and many of our best practices) are leading us towards first starting with configuration files (eg. Initializer) and loading all of our database metadata from these configuration files, the question is worth asking - can we just standardize on some configuration file structures that might have a richer set of information and some APIs that access this directly from there, rather than try to force this information into existing database tables and domain objects? What is it solving by putting this into the database?
You rightfully pointed out that the hacky part is to mess up with the getter (your points 1 and 2). I was wondering if people would tolerate that to the benefit of letting a complete i18n mechanism sneak in with a certain level of backward compatibility. The hope would be that most of the Ref App and Bahmni would not break and that the “enhanced” getName() would work as expected in most places when switching locales.
I am much less concerned about 3 I must say. Perhaps there is some downside when using concepts, but what I know is that Concept is the only thing that i18n fully, and we have all the tools to manage and load concepts now (cfr Iniz… etc).
I tend to lean towards the fact that stuff should be defined/described by concepts. It’s rather those i18n-incompatible names and descriptions that are problematic when it comes to metadata in OpenMRS.
What was the rationale at the time when program/workflows/states were made to encapsulate a concept then?
Do you advocate to persist names and descriptions as message properties keys? I know it works well enough in the Ref App and you guys have extensively done this at PIH with success. It can work in Bahmni as well, but it is a lot more tricky, it’s quite a rabbit hole actually.
@mksd - fair enough about my 3rd point - I threw that in at the end as an afterthought, but it’s still worth being aware of. I like the practicality of your solution, and I like your point that stuff should be defined/described by concepts, I’m just wary for the reasons I listed.
My sense is that you are proposing this because there are existing UI layers that refer extensively to the “name” property already (in rest representations, etc), and this will allow all of those UI layers to “just work” as-is. If that’s your reasoning, that makes a ton of sense. I’d personally be more amenable to doing what you propose, if we did not using the “getName()” method to do so. If we create a new method on BaseOpenmrsMetadata called something like “displayName()”, which can use the standard “name” and “descriptor” properties as you describe to come up with the best display name for a given piece of metadata, maybe that would be an alternative to consider? Then, we would just need to adjust our Rest Web Services (for JS apps) and ui:format tag (for GSP apps) to refer to this new method and most would immediately see the benefits.
Regarding your question about our use of message properties, for my part, I really do not like the pattern we established of using message properties that include uuids in the key, because I feel that UUID should be an internal property (used for things like sync) and not something that developers or implementers should have to concern themselves with. I also don’t like having to independently maintain message property files separately from the metadata definitions themselves. That said, one benefit they do have is that they can be easily managed in Transifex.
I do see one other key benefit in keeping the translations in the database rather than external configuration files when it comes to SQL-based reporting.
If the translations are to be changed at runtime, then i would prefer keeping them in the database. But if not, then i would go for external configuration files.
True, however the idea is to leverage Concept since it’s got all that’s needed right out of the box. Nevertheless with tools like Iniz concepts become part of configuration files as well. So, here we go
So could this kind of stuff make consensus?
public abstract class BaseOpenmrsMetadata ... {
...
private String name;
...
private Concept descriptor;
@Override
public String getName() {
return name;
}
public String getDisplayName() {
if (descriptor != null) {
return descriptor.getName(Context.getLocale()).getName();
}
return getName();
}
...
}
And we would be left to search and replace all those getName() with getDisplayName() where appropriate in both the Ref App and Bahmni?
This seems ok to me. I would be interested in hearing from @burke on it, in case there are any red flags on his end. Or from @dkayiwa, @raff, @darius, or @wyclif in case they have thoughts about whether we considered this solution in the past.
On the particulars, I don’t know if it is bad practice to name the method “getDisplayName()” since it isn’t a true getter (that was the reason I called it just “displayName”), but I do think it would be the most practical to name it as you have (allows you to access the property directly in jstl, etc).
What would you propose to do in the case of Program/Workflow/State? These already have a property named “concept” on them. How would we evolve these to match this new naming pattern?
The other thing that this will enable and we’ll need to think through, is that this descriptor will give access to other properties of the Concept besides name, in particular Concept Reference Terms. If we end up going this route, that might make our creation and (limited) adoption of the metadatamapping module somewhat unnecessary and we might want to evolve over to just the Concept Mapping mechanism.
One complication that I can see is that some implementations (I think) use CIEL and just CIEL - they do not allow any custom concepts in their distribution. So there may be challenges for them to gain the flexibility they would need in creating these concepts which might be hard to justify putting in CIEL.
I’m hoping others weigh in critically so we can try to gain consensus or hear about other reasons not to do this. Given we have gone through several rounds of attempts at metadatalocalization in the past that have used much less elegant mechanisms, and we did go through the effort to create the metadatamapping module, I fear that this option was considered and ruled out for particular reasons in the past. So I’d like to hear about this from others if they recall.
Generally, i find it unintuitive and hacky to use the Concept class for this. If it were a new class, that would be easier to take in. I do not think Erick Evans would find this to be in line with Domain-Driven Design.
I’m not enough of a medical informatics expert to settle whether it is hacky or appropriate to use terminology concepts to describe pieces of EMR metadata.
I would say yes, unless there was a specific and yet other reason for them to be described by a concept.
I agree with the idea that we should keep getters and setters simple, but this is actually how we implement Attributable#getPossibleValues(). Granted that goes through the service layer, so it likely avoids some of the LazyInitializationExceptions.
What do you think should happen in the case that we commonly have where translations would be only very rarely changed at runtime? For example, the number of VisitTypes and their corresponding names are likely to change only very infrequently, but I’m not sure that keeping a link between a possibly implementation-specific VisitType and a configuration file is the best idea.
This is the argument that really gets me. This really does end up changing what a concept is in OpenMRS (I read a “concept” in OpenMRS as a semantic label for a piece of data). Plus, it seems a little odd to me that we end up with a concept for determining what to call a “concept” in a given implementation.
That said, I take the overall point: i18n support is pretty limited in OpenMRS. I think it might be better to approach this from the perspective of creating a i18n framework from which we could (re-)build support for localization of concept names and metadata items, e.g., by implementing a version of ResourceBundle that can read from the DB (or wherever). Of course, this does get away from being a “simple way” to implement i18n for metadata.
We could introduce a specific data class for this. For instance “metadata descriptor”, to segregate them away. Again, such ad-hoc classes had been introduced for programs, workflows and states.
I’m not sure I understood this, would you mind rephrasing?
I was indeed contemplating a relatively inexpensive way to solve this old problem. Concept is great because it’s been there forever and it will plug nicely without too much overhead. Every implementer by now has a way to handle them.
Nor me. My understanding is that Concepts have the explicit name structure that they have due to more than just localization, and that it is an important feature of the system to be able to record exactly what concept name was asked and answered (eg. the specific wording) on a clinical observation. This is why, for example, Obs has a reference to valueCodedName in addition to valueCoded. As @dkayiwa pointed out, there may be value in trying to limit Concepts to clinical terms and value sets rather than a generic multi-lingual dictionary. Though I defer to @burke or @akanter on this.
Taking this a step further, one could ask why we have different types of metadata at all, if everything is ultimately able to link back to Concept. What design decisions led us to create distict “drug” table and independent Drug object rather to have Drug extends Concept and an explicit relationship between the “concept” and “concept_drug” table? The answers to that might be informative.
At Regenstrief, the concept dictionary started out much like in OpenMRS but later evolved to a model where the “local” concept dictionary (equivalent to OpenMRS’ Concept) became just one source (one dictionary) and other sources/dictionaries could easily be added & used as needed for things like metadata.
In effect, imagine adding a concept.source attribute and populating it with “1” for all existing concepts. In turn, what we see as the dictionary now becomes the “primary dictionary” and code that uses concepts evolves from asking for “concept 123” to asking for “concept 123 from the primary dictionary.” When you need a new value set or metadata dictionary, you add a new source (e.g., “2” or “3”). Eventually, a universal concept ID was added that could map to a source + concept ID (similar to how we use UUIDs to map to concepts).
The challenge with using concepts for metadata (and why there is ongoing debate on our choice to leverage them for i18n of programs/workflows/states) is conflating local metadata with a central/primary concept dictionary that typically needs higher rigor than local metadata.
I bring this up because this thread reminds me of my attempts many years ago to evolve the OpenMRS model toward Regenstrief’s – i.e., adding a “source” column to our concept table. Unfortunately, the extra complexity wasn’t justifiable at the time. That said, if we had managed to do it, I don’t think we’d be having this conversation; rather, people would just be adding a new source for metadata when needed.
In the absence of the source column, do you think it would be acceptable enough to introduce ad-hoc classes (again, this is what was done with programs/workflows/states)? See here. That would be the discriminator between first class metadata and local/second class metadata.
If yes then the proposition would be:
Each piece of metadata could be described by a concept.
Such concepts would belong to a “metadata descriptor” class.
I would even be ok to assume that BaseOpenmrsMetadata#name could be used as the i18n code rather than actually adding a new member to that class (and it’s myriad of subclasses). Hackier of course, but quite affordable IMO.
Unless we are changing the implementation of getName() to be aware of the i18n code, i would not be in favour of this. And even then, i would rather introduce a new method like getLocalizedName(), than change getName().
A much more lightweight approach could be to: