We are currently trialing moving some of the concept configuration for one of our implementations out of Metadata Sharing and into Iniz, and ran into one potential problem/risk:
When referencing a concept (for example, when setting up concept sets or concept answers), Iniz supports referencing that concept by uuid, mapping, or name, and referencing by name is a common choice because it leads to the most “readable” CSV files.
Behind-the-scenes, Iniz delegates to the ConceptService.getConceptByName method to look up concepts based on name. We’ve noticed some perhaps unexpected behavior with this method when there are two concepts that share the same name. For example, if you have a concept with a Fully Specified Name in a locale, and that same name is also a Synonym for different concept, there’s no guarantee which of the two concepts will be returned. With Iniz, this could result with a wrong concept being associated with an concept set or an answer.
We should consider changing the getConceptByName so that if there are multiple concepts with the same name, it picks which one to return based on a standard set of rules (assumedly preferring the concept with the FSN = name)
We should consider changing Iniz so that it can set to a “strict” mode, in which case it will only match on Fully Specified Name, to avoid choosing an unintended name
(We are not blocked on this, and have worked around the problem by using concept mappings for the few cases in our system where this is a problem, but wanted to share and throw this out there for others to consider).
fyi @mseaton @burke @mksd @mksrom @dkayiwa @ibacher
Thanks @mogoodrich for raising these.
I agree with this, but also think we might consider going further and deprecating this method altogether, as there is no way to know whether the concept returned is the concept intended across all use cases. This is actually how the ConceptService.getConceptByMapping method works - if there are > 1 non-retired concepts that could match the given mapping, the method throws an Exception. Still, even here there may be scenarios where the retired concept is actually the one you want to refer to…
Agreed, and we should encourage implementations to adopt a “strict” mode in their configuration settings.
Thanks @mseaton … thinking about it, I tend to agree with deprecating the getConceptByName altogether in favor of a clearer method.
This breaks our concept validation rules. If a name can be used for multiple concepts, it is not fully specified:
- Fully specified names must be unique across all names (except short names and index terms) in a locale
Is this true @burke ? Isn’t @mogoodrich talking about a name that exists as an FSN on one concept and as an index term (synonym) on another?
Yes. Per @akanter here, if a name is fully specified, then it shouldn’t refer to something else.
@ball pointed out cases where even CIEL has challenges with this rule, when an FSN is used for general terms (like Absent), that may be useful as synonyms elsewhere. But I think we have fixed all of those cases in CIEL.
I assume Andy’s recommendation for when these conflicts arise would be to fix the FSN to be more fully specified – e.g., change “Grey” to “Grey color” and make “Grey” a synonym & preferred (so concept still shows as “Grey” but has a fully specified name that is less ambiguous).
Thanks @burke . I re-read that thread, which is useful. Not sure whether to comment further on this thread or on that one, but probably makes sense to do it on that one, and then we can try to pull back any further information here if needed. That said, this discussion is coming up because the validation you describe above is not enforced in OpenMRS. So dictionaries have situations where the same name is used as a fully specified name for one concept, and a synonym for another, and this caused issues in matching that concept in Iniz, which is the reason @mogoodrich raised this post.
@mseaton I have also run into a similar challenge related to a mix up of concepts that share the Fully Specified Names and the Short Names. I would think it wise indeed to have a solution that accommodates creating sets and coded questions with their members and answers as names in the CSVs because its easier to read for an implementor. We could have a custom method in Iniz to be able to have this strictness and if it is relevant elsewhere then we could probably modify the method in core to have a strict mode via a GP.
Or (as I thought you suggest on our internal Slack) Iniz could become configurable so that the concept names that are used as keys to identity concepts in CSVs:
- Can either be any name (this is what happens now); or
- Can only be FSNs (that would be the new configurable option).
I’d probably think that defaulting to FSNs makes the most sense and making the current behaviour the thing that can be enabled via configuration. That’s not backwards-compatible, but it seems more correct; even when we’re using human-readable identifiers (which should be the preference), it’s best if we’re using unambiguous ones.
Please see Iniz issue # 211.
Since this is primarily and pressingly intended for a Bahmni implementation, we suggest to stage the solution in Iniz for now, to avoid backporting as far as down to Core 2.1.x.