As I was working on TRUNK-5494, I ran into an issue where an Obs can’t get loaded. Obs is a subclass of BaseOpenmrsData which is a mapped superclass that already has dateChanged and changedBy properties mapped but the obs table doesn’t have changed_by and date_changed columns since observations are ‘immutable’.
I haven’t yet figured out how to get around this but I can’t help to think that probably we should never have mapped BaseOpenmrsObject , BaseOpenmrsData and pretty much any abstract class in the first place. Persistent subclasses should be able to exist without the superclasses being mapped.
Does anyone object removing these annotations from the base classes in core? I can create a ticket and do the work.
There are actually other options that come to mind, so we have these 3,
Option 1:
Remove all the mappings from the base classes including BaseOpenmrsObject. The downside to this option is that any module that currently expects these classes to be mapped will break but the fix should be simple, if they are ref modules, we can fix them with follow up tickets in the refapp project.
Option 2:
Since the controversial fields have always been dateChanged and changedBy, we can choose to only remove mappings from these fields in the base classes. This still would break some modules like option 1.
Option 3:
We could temporarily add the columns to the obs table but never populate them, and then later drop them as part of TRUNK-5232, I know the obs table is large for some implementations but adding nullable columns to any table should be fast. This seems to be the only backwards compatible option.
Options 2 and 3 seem to align with TRUNK-5232 when it eventually makes it into master.
Option 1 would be better to override option two inasmuch as the problem has been by those two fields; you can’t guarantee the other field won’t cause issues in the future… Quick question about option three… Does that imply persistence is still done via openmrsbaseobject?
Maybe the immutable class should never have extended a mutable one, after all immutable is not mutable (IS-A thrown out the window right there)! Anyhow maybe it is not a big deal but if you ask me the correct fix would be to have another super class that is actually immutable.
@willa you are correct, since Obs is immutable, it should have never extended BaseOpenmrsData, it’s the reason why we made some changes to the class hierarchy in TRUNK-4917 to introduce BaseChangeableOpenmrsData and BaseChangeableOpenmrsData in 2.2, we created a follow-up ticket TRUNK-5232 to eventually clean all this up, I think it’s too early to make such a backwards incompartible change in 2.3, but we were looking for a short term solution which is why am suggesting Option 3.
@reagan I don’t seem to understand your question, do you mind clarifying it please? If we add the columns to the table in the DB, they will have null values for all rows, there is no side effect except having redundant columns in the DB which should be no big deal.
In that case option 3 seems to be minimally invasive which is one of the main reasons I tend to consider when attempting to fix stuff in code. I know you already know this but just to emphasize, I think it will even be clearer to include the details of the intent on the Javadocs and the fact that they will be removed later.
The other two is more like nuking the code, I know you don’t wanna do that
@dkayiwa fetch the branch linked to this PR and try to load the patient dashboard.
The problem is that obs table has no changed_by and date_changed columns but because hibernate is seeing the mapped fields changedBy and dateChanged from the superclass, it is including them in generated SQL query when looking up obs.
There is also option 4 which I have applied to the PR, it’s similar to option 3 but rather than adding the columns directly to the obs table, I added a throwaway table containing changed_by and date_changed columns, them I mapped the 2 properties to the throwaway table’s columns using @AttributeOveride for dateChanged and @AssociationOverride for changedBy in the Obs class, this is actually the proper way for a subclass to customize fields from a mapped superclass. Then I updated AuditableInterceptor to not set changedBy and dateChanged for Obs instances. I have created a follow-up ticket TRUNK-5561 to drop this throwaway table when TRUNK-5232 is completed.
