Hi Everyone,
The equals method from Conditions as been overwrite and are not comparing the UUID as it is in the parent class BaseOpenmrsObject.
Condition equals overwrite:
BaseOpenmrsObject equals:
Is there any reason for this?
This is impacting I try to edit a new condition since the Set will not recognize it as already existing one and will create a new one.
Are your conditions pointing to the same patient and have the same clinical status?
I would think that the “correct” implementation of equality for Condition would be something like a combination of patient and concept (though that gets a bit challenging as Condition also supports free-text descriptions).
@luis.oliveira Condition does check UUID equality with the call to super.equals(), but as @dkayiwa if the patient or clinical status are different, the two Conditions will be regarded as different.
Same patient. But with the new development, (see PR TRUNK-5970 and PR HTML-747) we want to define that if the status is removed the conditions should be voided. so the status will change.
Yes you are right.
But that should not be reverted? Like:
if (super.equals(o)) {
return true;
}
Because if the UUID is the same the condition as to be the same even if you change any of the other attributes.
So, there’s a couple of considerations here.
I don’t think we want to depend on the UUID for condition identity because of the requirements documented here and here to maintain some kind of audit log and to create a new entry on condition changes. This means that UUID (which is really an identifier for a database row) isn’t sufficient to determine that two conditions are “the same”. (This also means it’s actually an error to have two conditions with the same UUID but different values).
Hence my proposal that we define condition equality on the basis of patient and the (coded or free text) condition field. That way, we can identify as “the same” references to the same condition for the same patient regardless of which row in the database represents them.
However, it’s entirely possible for a patient to have had, say TB, to have had that instance of TB cured, and then to be reinfected. In that case, would we want to have two TB entries, one for the past case and one for the active one or is it sufficient to just maintain the active one in the condition list?
@burke @jteich @mogoodrich @mksd Any thoughts on this?
What do we do generally with other pieces of OpenMRS Data and OpenMRS Metadata? I though we generally just relied on uuid, and so “equals” really just says this is the same “object”, not this is literally the same “condition”… ie, we tend not to include business logic in the equals method… if I’m correct about this I’d advocate for just using the super “equals()” method.
(We, of course, could add another business-logic method like “isSameCondition” or something if we need that functionality)
fyi @mseaton
You’re right that generally we don’t implement equals() for other domain classes. Alright, let’s go with eliminating the equals() method altogether then.
That equals() implementation for Condition is a weird outlier. It is implementing a business rule: two conditions are equals if they have the same clinical meaning, not necessarily if they actually point to the same database entity. As a general matter of fact of course, why not, but that is just unusual and leads to weird situations such as the one that @luis.oliveira has been facing with collections.
In short: either we always decide to implement equals() based on a business rule, or we never do.
I’m curious as to why that specific one was introduced though, @dkayiwa do you remember?
I’m not sure how much of a decision it was. Looking at the commit that added the equals() method, it looks like it was only used in the service layer to do nothing if saveCondition() was called without any changes.
This came from bahmni into the emrapi module and finally openmrs-core.
It looks like @sid just wanted to test if two objects have the same values for all properties.
@mogoodrich and @ian - I agree that equals/hashcode methods should be consistent with all other OpenmrsObject implementations, and inherit this from it’s superclass (use uuid), in order to test just that 2 condition objects refer to the same Condition in the system. If we have further business logic that needs to determine if 2 Condition instances refer to the same underlying medical condition, that should be a different method.
fyi @angshuonline… not sure if Bahmni Condition List relies on this equals implementation, (and so would need to tweaked after the change)
Take care, Mark
I assume we’re talking about equality and not just identity. An equals method could be useful to avoid having the same condition (i.e., same concept) appear twice in a patient’s condition list. For example, you shouldn’t be able to add “asthma” to a patient’s list of conditions if that list already contains “asthma”. But, I realize it gets messier, since conditions can be inactive or voided (e.g., an inactive condition of “asthma” from 5 years ago shouldn’t prevent “asthma” from being added to the condition list).
@burke we have the precedent of the Java “equals” method operating as a identity method for all other OpenMRS objects (via a Openmrs superclass), and so for consistency don’t want to make it into an “equality” method just for this object type.
Take care, Mark