Improving the way we save immutable objects

Hi,

In core we have domain objects that we treat as immutable e.g Orders and Obs, when they’re changed and saved through the API we have a mechanism in place that voids and replaces them with new copies. We also have hibernate based interceptors that act as watchers to catch any edited entities that get flushed, this is useful when they get edited directly or indirectly and an explicit call to save them is made but hibernate goes ahead to commit the changes them when the session is flushed, but the problem is hibernate auto flushes can happen at anytime, in general this happens when a call to a transactional method is made which at times is within the API itself and the external code has nothing to do with it. One possible solution is to always return fully loaded and detached instances of these objects but this approach has performance implications. The other solution that I’m proposing is to disable auto commit in the save methods for these objects and reset it before they return.

What do others think?

I would not worry much about the performance implications of the first proposal, because they are most probably insignificant. The challenge i see with it is, some API consumers who already have code which may not be expecting detached instances. So this leads me to cast my vote for the second proposal.

But since platform 2.0 is not meant to be backwards compatible, i change my mind and go with the first proposal. :smile: If this is not acceptable, then we could have it in platform 3.0

Note the the second options is already being used by the validation mechanism, so it’s not something new and it has no known performance hits

Are you proposing to disable auto-flush upon save of some specific objects, or all objects?

Should we consider just turning flush mode to manual always? (This seems like it would need to happen in platform 3.0…)

I meant specific objects for now, and in 3.0 we can do it for all

I don’t think that will work.

Isn’t the issue that other hibernate calls having nothing to do with concept and order trigger the flush, e.g. more like this:

Concept concept = service.getConcept(...);
makeChangesTo(concept);
if (checkGlobalPropertySetting()) { // *this line triggers a flush*
    // ...
}
makeMoreChangesTo(concept);
service.saveConcept(concept);

Disabling autocommit in the save methods for concept and order won’t solve this problem.

We’re not disabling auto flush throughout the API, we just don’t want auto flushes to happen from when saveOrder(Order) and saveObs(Obs) are invoked until they return.

The solution you are proposing will not fix the scenario I described in my previous message, right?

Is there a different scenario that it will fix? (Or am I misunderstanding how things work?)

Your scenario is not what we’re are solving, what we are trying to solve is where an API consumer edits an Obs calls saveObs(Obs, String) and an exception is thrown by an interceptor because of an auto flush triggered by another call before the method returns. It’s okay for other auto flushes happening outside the boundaries of the save method since a developer can work around those.

Stepping back…

I think it’s generally always fine to change any particular XyzService.saveXyz method to do its work with manual flush mode. No API consumer should ever be expecting an automatic hibernate flush.

So I have no objection to the change you’re suggesting.

(I still don’t quite understand the scenario you are describing. It would help to see a specific example, or some pseudocode.)

@darius i think the example that @wyclif is referring to is when you call ObsService.saveObs with a complex obs. In the process of saving this obs, the API will internally call ConceptService.getConceptComplex() resulting into a flush. Some of the handlers may also call methods like AdministrativeService.getGlobalProperty() to get the directory where complex obs are stored, etc, resulting into more flushes. https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/obs/handler/AbstractHandler.java#L68

That’s right @dkayiwa , @darius take a look at the code snippet below:

ObsService service = Context.getObsService();
Obs obs = service.getObs(1);
obs.setComment("Changing comment to something else");
service.saveObs(obs, "updating comment");// We don't want any premature auto flush to happen  during execution of this method 

As @dkayiwa mentioned, we don’t want any auto flush to occur inside saveObs(Obs) due to calls to other transaction methods within saveObs(Obs);

The proper design for the future is to make fields, which should be immutable, truly immutable in Java by declaring no setters for them.

Changes to Obs may be persisted not only by calling saveObs, but virtually any method which starts a not-ready-only transaction e.g.

Obs obs = obsService.getObs(1);
obs.setComment("Changing comment to something else");
locationService.saveLocation(location); //it may also result in obs being flushed and persisted in the database

Wasn’t https://issues.openmrs.org/browse/TRUNK-50 addressing the issue?

In this case Hibernate will fail I’m afraid. Fields listed in the mapping file must have getters and setters following the Java Beans specification.

@lluismf, not if you use field access instead of getter/setter, documented at https://docs.jboss.org/hibernate/stable/annotations/reference/en/html_single/#entity-mapping-entity

You’re 100% right ! Excuse my ignorance :grinning: BTW maybe there are other frameworks/modules using reflection that are no so smart as Hibernate.

I feel like we are mixing up 2 things here, something being immutable from the point of view of client code vs within the API, the concept of Obs being immutable wasn’t implemented well for Obs right from the start, it was partially implemented and this is what makes it a little confusing, when a developer sees an error message that says ‘Some fields on Obs are not allowed to be changed’ it’s actually misleading. We can’t really get rid of the setters reason being that in theory from the client’s point of view Obs are actually mutable, it’s within the API that we treat them otherwise and this should never have to bubble out of the API and I think may be this was the mistake we made, one is allowed to set new values for fields on Obs therefore you need their setters.

On the other hand, Orders were better implemented since we provided a mechanism to revise existing Orders without a dev having to deal with the original Order instance directly and this is probably what we need to apply to Obs.

I would also like to agree that if a field is TRULY immutable i.e from both a client’s point of view and the API it should have no setter but rather a constructor argument.

@wyclif did you create a ticket for this? I would like to include it in platform 2.0.1 because it is a simple fix.

Nope! I doubt if we’ve agreed on this yet.