Proposed change to the class hierarchy of openmrs objects

Hi,

In OpenMRS all domain objects are categorized as being either OpenmrsData or OpenmrsMetadata, they both implement the Auditable interface which assumes all sub classes are changeable which really isn’t the case always.

Historically, when mapping classes for ‘immutable’ domain objects i.e ones that don’t have changedBy and dateChanged fields, the work around has always been to exclude the fields in the DB and the hibernate mapping file. With more developers gravitating towards annotation based mappings and requesting support for them in core, we decided to add annotations to the base classes BaseOpenmrsData and BaseOpenmrsMetadata and back ported the changes to older maintenance branches up to 1.9.x that way a developer can be able to extend these classes and still be able to annotate them. But there is a problem that arises when you use annotations with the current class hierarchy, basically you can’t have an annotated persistent subclass of BaseOpenmrsData or BaseOpenmrsMetadata which doesn’t have some of the annotated fields in the DB because it freaks out hibernate otherwise you’d have to add the columns in the DB which would make them redundant and you’d possibly still have to set the fields mapped as not nullable anyways, an example of this is the Order class which has no changedBy and dateChanged fields since orders are considered to be ‘immutable’.

Therefore because of the above issue we’re proposing a change to the class hierarchy to introduce 2 new interfaces i.e Creatable and Changeable which will be extended by the Auditable interface, for more details see TRUNK-4922. That way a class can choose to implement the appropriate interface without the baggage of redundant fields that come with the other, as per the solution proposed in the ticket Orders would end up not being instances of OpenmrsData nor OpenmrsMetadata anymore which would break existing code that was being applied to all instances of OpenmrsData e.g code inside an if (object instanceof OpenmrsData){…} clause. As per the notes from this design forum , it means we’d also need to make other changes to the class hierarchy to keep all domain objects as still being either OpenmrsData or OpenmrsMetadata and the option we agreed on would require the changes below:

  • OpenmrsData and OpenmrsMetadata to be changed to implement the new Creatable interface and not Auditable which means by default everything would be immutable so classes like Order would directly extend base classes for these.

  • Then we introduce OpenmrsChageableData and OpenmrsChangeableMetadata which extend OpenmrsData and OpenmrsMetadata respectively while implementing Changeable to make them mutable, of course we would also add base classes for them i.e BaseOpenmrsChangeableData and BaseOpenmrsChangeableMetadata.

The above changes would imply pretty much 90% or more of our domain objects would have be changed to extend BaseOpenmrsChangeableData or BaseOpenmrsChangeableMetadata and module developers would definitely need to change their domain objects to extend the same accordingly.

We want to know what you think about the proposed changes, our guess is that for sure this affects module developers but what they have to change is minimal even though it would be a must.

3 Likes

In order to reduce the immediate impact for module developers, how about simply deprecating the current BaseOpenmrsData and BaseOpenmrsMetadata and then have them extend BaseOpenmrsChangeableData and BaseOpenmrsChangeableMetadata respectively? That way it would take a couple of platform releases before module developers have to hassle with the complexity of supporting multiple versions of the platform in regard to this.

@dkayiwa’s idea makes sense to me.

Overall, assumedly this will only be in the 2.0 core release, so I guess one big consideration is what other major changes are module developers going to have to make to support 2.0? If there’s a lot, adding this in seemed reasonable. If this is going to add a big overhead not otherwise incurred, I’d be hesitant.

Regardless, I suspect the last 1.x release is going to last a long time, and there will be several modules that don’t make the jump to 2.0.

Mark

Actually, since Platform 2.0 was already released, this change would happen in Platform 2.1.

So, part of the purpose of this talk thread is to see what people like @mogoodrich think about this.

(@wyclif I think it would be important to do a spike to see what a module has to do to support both the pre- and post-refactor versions of the code. I know that the change would be pretty trivial, but I fear backwards-compatibility is very hard.)

I totally agree with this idea. Anything we can do to limit how much stuff will be broken by this change will be very helpful. We’ve got a whole service framework (from the db up through REST resources and unit tests) built around the three core omrs objects; I really hope that this change can be implemented in a way that we don’t have to change too much of that.

Agree with the approach suggested by @dkayiwa.

Seems like @dkayiwa’s proposed solution is worth considering

dear @wyclif, @dkayiwa and others should we discuss this at the design forum?

I originally wanted to discuss

but since this depends on this issue might be better to talk about this first.

TRUNK-4903 is one of the reasons as to why this came up

I know, I worked on it and found this anomaly :wink:

Just wanted to know if you want to discuss this on the design forum, since working/discussing the annotations issue doesn’t make much sense before this issue is resolved. If so we just make a topic switch otherwise I’ll cancel it for now.

@wyclif @dkayiwa @darius

should we now discuss this on

instead of the Order annotations?

can anyone from @wyclif @dkayiwa @darius

please tell me if we should discuss this topic today on the design forum

instead of the Order annotations as originally planned? I dont think discussing Order annotations makes any sense at the moment.

(To close the loop).

We discussed this on today’s Design Forum. We talked about a couple alternative approaches to make things easier on API consumers who want backwards and forwards compatibility across this change:

  1. what @dkayiwa had suggested (deprecating, but not removing, BaseOpenmrsData/Metadata)
  2. introducing OpenmrsChangeableData/Metadata in maintenance versions of supported release versions

Finally we decided that the potential disruption of this change (so soon after platform 2.0, which was supposed to be the last non-backwards-compatible release for a while) is significant, and may outweigh the benefits. Wyclif is going to look more deeply into whether we can sequence the changes more carefully, to avoid compatibility problems. (E.g. introduce new interfaces immediately, introduce new classes a bit later, and make breaking changes much later.)

2 Likes

Hi,

I took a closer look at the options proposed on this design call, specially I tried to look into the actual steps to be taken to get us where we want to be? And below is how each option would be implemented:

OPTION 1:

I think this is the approach closer to what @dkayiwa was suggesting i.e. we keep BaseOpenmrsData and BaseOpenmrsMetadata so that in 3.0 they default to being changeable, below is what we’d do in 2.1 and possibly back port the changes to 2.0.1.

  • Introduce the i.e Creatable, Changeable interfaces.
  • Override and deprecate the getters and setters for changeBy and dateChanged in OpenmrsData and OpenmrsMetadata
  • The getters and setters for changeBy and dateChanged need to be overriden in OpenmrsChangeableData and OpenmrsChangeableMetadata so that they don’t appear as deprecated yet since they will eventually stay around.
  • Update Auditable to extend Creatable and Changeable interfaces.
  • Introduce OpenmrsChangeableData and OpenmrsChangeableMetadata along with sibling interfaces for immutable data/metadata i.e OpenmrsUnchangeableData and OpenmrsUnchangeableMetadata. I think alternatively we can choose to not add OpenmrsChangeableData and OpenmrsChangeableMetadata and just assume all sub classes of BaseOpenmrsData and BaseOpenmrsMetadata are mutable.
  • Update BaseOpenmrsData and BaseOpenmrsMetadata to implement the new interfaces for changeable data/metadata if we choose to add them.

I also think we might consider deprecating and eventually removing Auditable in 3.0 but it’s very likely to affect module developers.

Then in 3.0 we do the following:

  • OpenmrsData/OpenmrsMetadata should extend Creatable, Voidable/Retireable and no longer extend Auditable.
  • Update OpenmrsChangeableData to extend OpenmrsData and Auditable, technically it should be Changeable instead of Auditable if we decide to drop the Auditable interface.
  • Update OpenmrsChangeableMetadata to extend OpenmrsMetadata and Auditable.
  • Remove the overrides for getters and setters for changedBy/dateChanged from OpenmrsChangeableData and OpenmrsChangeableMetadata since they’d be meaningless at this point.
  • Add the necessary base classes i.e BaseOpenmrsChangeableData, BaseOpenmrsChangeableMetadata, BaseOpenmrsUnchangeableData and BaseOpenmrsUnchangeableMetadata.
  • Immutable types like Order get updated to extend BaseOpenmrsUnchangeableData.
  • Update AuditableInterceptor to check for Changeable instances instead of Auditable when a persistent object is flushed in case we keep the Auditable interface around.

OPTION 2:

With this approach we’ll eventually get rid BaseOpenmrsData and BaseOpenmrsMetadata in 3.0, what needs to be done in 2.1 and possibly 2.0.1 is the same as that in option 1 but we’d deprecate BaseOpenmrsData and BaseOpenmrsMetadata.

Then in 3.0, again the changes would be more like those in option-1, but we’d have to get rid of BaseOpenmrsData and BaseOpenmrsMetadata by renaming them to BaseOpenmrsChangeableData and BaseOpenmrsChangeableMetadata respectively.

From above, you can tell option2 requires more refactoring with a direct effect to module developers whereas with option1 possibly most modules might require no changes.

1 Like

Thanks @wyclif for writing it down. A few of your points are inconsistent:

yet you said

Next:

I think you meant “mutable”.

Next:

Shouldn’t they also extend Changeable and still extend Auditable?

A few opinions:

  1. I’d prefer we stick to OpenmrsMetadata and OpenmrsData for mutable objects instead of introducing OpenmrsChangeableMetadata and OpenmrsChangeableData since if I understand correctly they will be just synonyms.
  2. OpenmrsImmutableData and OpenmrsImmutableMetadata sounds better for me.
  3. I’d rather we do not remove Auditable. It’s still a useful shortcut for Creatable and Changeable.

By the way it would be easier to comment on the changes seeing a pull request (even one, which does not compile). It’s harder to capture code changes in prose than in code :wink:

Yes I meant to say BaseOpenmrsData and BaseOpenmrsMetadata should end up being mutable and I have corrected it :slight_smile: Thanks!. I think the other is still consistent, we want OpenmrsData and OpenmrsMetadata to be immutable by default but BaseOpenmrsData and BaseOpenmrsMetadata will be mutable since they will implement the OpenmrsChangeableData and OpenmrsChangeableMetadata interfaces. OpenmrsData/OpenmrsMetadata can’t extend Changeable and Auditable because they are the root super interfaces so they can’t dictate the behavior of their sub classes when it comes to changeability, instead it’s their immediate sub interfaces i.e. OpenmrsChangeableData, OpenmrsUnchangeadbleData, OpenmrsChangeableMetadata and OpenmrsUnchageableMetadata that should dictate it.

So i have 2 branches in my fork for each option i.e option-1 and option-2, each branch has 2 commits where the first commit contains the deprecation and introduction of new interfaces we would’ve to make/add now and back port while the second commits contain the changes to be made in 3.0.

FYI for option 2 I didn’t bother to rename BaseOpenmrsData and BaseOpenmrsMetadata to BaseOpenmrsChangeableData and BaseOpenmrsChangeableMetadata but we’d have to.

What do others think about the proposed options?