Whenever a child obs is edited, ObsService.saveObs removes the old child obs from parent’s groupmembers and add new child obs to it.
In the above calls, when group member is added or removed, the parent is made dirty, which means when the parent obs is saved again at any point of time, the entire hierarchy is voided and created again.
So the question would also be whether to make an obs as dirty when a group member is added or removed from it?
The intended behavior (as I understand it from a quick comment by Burke) is that if you change the “set of questions” that are answered as part of an obs group, this changes the meaning of the obs group, and thus it should follow the void+create new flow. (I.e. adding or removing a group member should lead to the parent being marked as dirty, whereas editing an obs within the group shouldn’t mark the parent as dirty.)
I guess a side-effect of this is that if you have an obs group with 2 members, and you add a 3rd member, this voids and recreates the parent, so you end up voiding the original 3 obs, and creating 4 new ones. I can see why that might be problematic if you’re following a workflow where you frequently add one obs to a group (because of the point-of-care approach Bahmni takes, and the encounter transaction REST API), and you don’t want to explode your obs table with voided rows that don’t really add info.
We’re open to changing how things behave, but we’d like to understand the situation better. Could you explain a bit more why you are raising this question? I presume because there’s a real Bahmni use case that this behavior causes problems for…
Does changing parent required that the children are voided and rewritten? If I add an obs to a parent, I would understand the parent changing, but does this need to cascade to other unchanged children? Or could the new parent point to the same children that were unchanged?
I haven’t looked at the code, but I presume what is happening is that “the parent changing” => we need to void and recreate it. (And when we void and recreate, we need to void and recreate the children too, or else the history of voided obs wouldn’t make sense.) Perhaps we could try allowing a direct change to the parent obs in the db if the only fields you’re changing are changedBy and dateChanged, but that has other issues.
(Personally, my leaning is to not touch the parent obs when we’re modifying its children, and if the user/client wants to know the “date changed” of an obs group, they can look at all children.)
@darius We have not really encountered this issue in bahmni yet. As part of fixing group members getting duplicated on saving even when there is no change, @wyclif and I encountered this scenario and it looked weird. So thought of asking opinions here.
Also I think that the issue TRUNK-4944 can be fixed irrespective of the approach we take for the other scenario.
Can you do a quick estimate of the impact this will have on Bahmni? For
example, are there deeply nested obs groups because of the way that concept
sets are done, so editing a single form field is going to create like 50
voided obs?
In Bahmni, we have lot of big nested forms. Entering any data for a new field after the first save of the form will void the entire hierarchy and create the new hierarchy again. This is applicable to diagnoses as well which will explode the obs table over a period of time.
However adding the group member to top level obs voids and creates the new hierarchy whereas adding a group member to nested level obs throws unique constraint exception on ‘previous version’ field.
Lets say we have obs hierarchy like below:
Obs#16
Obs#17 (group member of Obs#16)
* Obs#18 (group member of Obs#17)
When I try to add a group member to Obs#16 and save the encounter, it voids and create a new hierarchy.
But when I try to add group member to Obs#17 (only obs#17 is marked as dirty, not obs#16) and save the encounter, it throws exception because, encounter uses getAllObs(true) for saving the obs which will return all the obs irrespective of the hierarchy not just the one at top level. i.e. it will return [Obs#16, Obs#17, Obs#18]
Now iterating the obs list,
During the first iteration, when the first obs Obs#16 is saved, its group member Obs#17 hierarchy is already voided and new hierarchy is created something like below:
Obs#16
Obs#19 (its previous version is Obs#17)
* Obs#20
During the second iteration, Obs#17 is attempted to save again and its trying to use Obs#17 as previous version for the newly created hierarchy and it fails.
This is another side effect of making parent as dirty when group member is added or removed. Am not sure of the approach to fix this. Need some inputs on it.
When you edit a parent obs the child obs technically won’t get voided unless they were edited too, if it’s happening then it’s a bug.
Encounter.getAllObs(boolean) doesn’t return nested child obs so in theory I don’t think it is the cause of the bug @preethi_s mentioned, it might be something else causing it. It would be nice to write up unit tests demonstrating the bug. With that said, I think it’s key that we write up more unit tests around editing obs groups.
@wyclif [quote=“wyclif, post:10, topic:8049”]
When you edit a parent obs the child obs technically won’t get voided unless they were edited too, if it’s happening then it’s a bug.
[/quote]
Obs.newInstance(obsToCopy) method which is used while voiding the dirty obs and creating a new one, creates new instance of group members as well.
It is true that getAllObs(boolean) returns all the obs, not just the top level obs. But that is the one which is causing the problem. In my above example, it returns [Obs#16, Obs#17, Obs#18] but still Obs#17 can be accessed from group members list of Obs#16.
I have already written the tests in my forked repo and mentioned it in my previous reply. Can you please take a look at it?
I see, I guess we need to add a new newInstance() method that has an argument that specifies if child obs should be cloned or not and we set it to false where applicable.
Looking at Obs.getAllObs() code here, it only returns top level child obs but getObs() returns all including all nested.
But on debugging through the code, found that getAllObs(true) returns all the obs with child obs not being removed from the parent group. May be i’l look at my tests once again.
I looked into this issue and the tests from @preethi_s’ fork and realized that the tests are failing because of a mix of several things, first all the tests and test dataset are not exactly well set up, below are some the issues specific to how the tests are set up.
Encounter.getAllObs() technically returns only top level obs but the test data has the encounter column of the children which is why they get returned when you call encounter.getAllObs() because hibernate adds them to the collection automatically being immediate children, my assumption is that the child obs in the test dataset should have a null encounter_id and have it default to that of the parent.
It would also be nice to assign the obs in the test dataset obs_ids and uuids different from those in the standardTestDataset file, the in memory DB overwrites a row if you duplicate in another test dataset and that gets executed at a later point which can mess up references to and from it of other entries in the other dataset.
The API has some historic logic that we might have to revisit since it appears contradictory to the behavior we want, we could consider making the changes below:
This commit for TRUNK-4940 seems problematic since it’s dependent on if the obs is voided or not which doesn’t always seem correct e.g when cloning an Obs for replacement.
Encounter.getAllObs() has a strange behavior i.e. it always returns all child Obs, do we really want to have child Obs to be linked to the encounter directly? Why can’t we infer the encounter from the parent? This solves some problems but I believe some will argue that it should also be okay for a child to directly reference the encounter which I’d not really against, I think there should be a work around to get the correct behavior.
ObsValidator validates voided obs, it should skip them since they are as good as deleted anways.
Obs.newInstance seems to be cloning all child Obs multiple times, so we probably need another that has a argument that specifies if it should apply to the child Obs or not. This is the reason why we end up with new Observations for every child obs even if the child obs have no changes.
Also the AOP around voiding openmrs objects is the reason why the entire tree of an Obs group ends up getting voided, the work around is to avoid calling ObsService.voidObs() and instead just set the void fields within the service. A combination of this issue and the prior is what makes it seem like the entire Obs tree gets voided and replaced with new ones.
I guess the reason why I was against resetting the dirty flag was because it was just going to mask some of these issues which to me seem like they need to be revisited since some don’t apply given the new behavior and anways it wouldn’t have fixed them.
The test data has been setup in such a way to exactly represent the actual way the obs are stored. All the obs would have encounter_id and I would think that is correct because we need not traverse up the obs hierarchy to find out which encounter a particular obs belongs to.
getAllObs method behaves differently in the scenario when there are already nested obs saved in the database for the encounter i.e when we fetch nested obs from the db in the current hibernate session and in the scenario when we create new nested obs in the current hiberante session.
Also I see this as description in getAllObs method
/**
* Returns all Obs where Obs.encounterId = Encounter.encounterId In practice, this method should
* not be used very often...
*
* @param includeVoided specifies whether or not to include voided Obs
* @return Returns the all Obs.
* @should not return null with null obs set
* @should get obs
* @should get both parent and child obs
* @should **get both parent and child with child directly on encounter**
* @should get both child and parent obs after removing child from parent grouping
*/
Now am not really sure what is the intended behaviour this method.
So I would suggest using getObsAtTopLevel() method while iterating the obs during encounter save because when we save an obs, ObsService.saveObs method takes care of saving its group members as well . Using this will eliminate the error thrown whild adding a group member to nested child.
Also, I would like to understand this concept:
Why do the parent obs needs to be voided and created again when a new group member is added or removed from it, because technically there is not going to be any change in parent obs entry. Isn’t it like adding one more info to the question set? Can someone please explain this?
@preethi_s in the case of Encounter.getAllObs() I understand that’s how the test data is setup, I was just explaining why it returns the entire tree of child Obs and that it’s not because of the logic inside the method. In theory if those nested child obs don’t have the encounter id set in the DB, they won’t get included when you call Encounter.getAllObs() but they will get included if you call Encounter.getObs() which includes nested ones because of the logic inside it.
As for why we have to void an Obs Group when a member is added or removed, that notion and behavior comes from the medical experts and @burke can best explain more, my recollection from what he said is that it changes the meaning of the Obs group, different implementations have different use cases for Obs grouping and in some cases it can put the patient at risk. E.g historically implementations used to employ obs grouping to represent order groups for medications since Obs has an order field, in their case adding or removing a group member would be equivalent to editing a drug order group and that can affect the associated patient, I might have understood it wrongly but that’s how I understood it, therefore because we don’t know all the use cases for order groups out there, we choose to void and replace the original.
@wyclif Also, as @preethi_s pointed out, the result from Encounter.getAllObs() is different based on its staleness. If a save operation is performed on the Encounter, the encounter is stale and needs to be evicted from the session and reloaded for it to be usable. This is concerning. The testcase is available. here Does this mean that after every encounter save, we need to re-query to get the encounter loaded?
I agree with @preethi_s. I would assume that all obs should reference the encounter to which they belong. In fact, putting obr (what HL7 calls an obs group) in the obs table was a hack that I have never liked. The initial intent was to have a separate obs_group table that would have an entry for every groups of obs. In any case, a value of obs.obs_group_id can be used to identify a lower-level obs.
In general, we take a write-once approach to obs – i.e., any change to obs voids and inserts instead of replacing. This is to preserve an audit trail. I believe the reason children are getting voided & inserted when a change is made to the parent (e.g., adding a new child), is two-fold:
The API doesn’t have a way to distinguish a void-to-revise vs. void-to-delete – i.e., if we are revising a parent, it needs to get voided as the new revision is inserted and this void operation cannot be distinguished from a void-to-delete, so can only assume children should be voided as well.
The reference to the parent is in the child obs (obs.obs_group_id). If we changed this value in a child in place (without void+insert of the child) then any history of a link to the old parent obs would be lost.
To the main question on this topic: Should editing the child make its parent dirty?
I agree that the changes should (ideally) just affect the tail (i.e., the child). Since the children point to the parent (via obs.obs_group_id) and not vice versa, we should be able to make changes at the tail without having to void & insert the parent.
I’m not saying that child obs shouldn’t reference the encounter, I was just explaining why Encounter.getAllObs returns all Obs and that it has nothing to do with some ‘mysterious’ logic inside the method.
@bharatak that test seems strange to me, my assumption is that it should be failing because no change is made to the encounter so the obs count couldn’t have magically changed from 1 to 4 just because it has been evicted, are you saying that the test is passing? If yes, that’s a very serious bug.