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.
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.
If this is not acceptable, then we could have it in platform 3.0
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.
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.
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
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.