Confusing behavior with getAllObs method on Encounter


(Mark Goodrich) #1

(Referencing https://issues.openmrs.org/browse/TRUNK-5438, and apologies to @burke who asked me months ago to create this Talk post to move the discussion out of JIRA and to Talk)

I’ve noticed some peculiar behavior regarding the getAllObs method on encounter. I would assume that it would fetch a flattened list of all the obs associated with the encounter but however that is not exactly what the method returns… it only returns obs that have been directly linked to the encounter, which may or may not be the case for nested obs. Some tests seem to suggest that this is expected behavior, for instance, see:

In practice, it looks like the connection between the nested obs and the encounter is only made at the time that the encounter is flushed to the database. Take a look a this test from REST web services, which creates an encounter with nested obs and then fetches the encounter back, expecting the size of getAllObs to be 1 (ie only the top-level obs):

If I change this test so that it does a Context.flushSession() and Context.closeSession() after the Encounter is posted but before the Encounter is reloaded, this assertion will start to fail as getAllObs().size will now be 3!

Besides being inconsistent, this seems problematic, because the getAllObs method is used by the EncounterService to update the obs associated with an encounter and to copy all obs from one encounter to another. It seems like this in practice this will only work if we are dealing with an object that has been persisted to the DB?

(The actual bug we are seeing is that if you try to update a encounter via REST and change the encounter datetime, that change is only propagated to the obs datetime of the top level obs).

Should we create a new method that explicitly flattens and fetches all the Obs on an encounter? Should we change the behavior of the existing getAllObs method? I’d worry about backwards compatibility, so maybe we should create a new method with a clearer name, and then deprecate getAllObs?

Take care, Mark


(Burke Mamlin) #2

I believe the intention was:

  • getObs() → all top level observations excluding obs groups
  • getObsAtTopLevel() → all top level observations including obs groups
  • getAllObs() → get all observations within encounter (full hierarchy)

Meaning that getAllObs() should (as the tests & documentation imply) return all observations within the encounter, no matter how deep they go.


(Mark Goodrich) #3

@burke so just to be clear, if you have an encounter with a single top-level obs that contains two child obs, getAllObs would return a set with size = 3?

I would tend to agree, but the tests suggested otherwise… take a close look at the tests, although the first one is called “getAllObs_shouldGetBothParentAndChildObs” the assertion expects that getAllObs().size() = 1…

@bwolfe


(Burke Mamlin) #4

Is the confusion is between “getting” vs “flattening”? If an encounter has a single top-level obs that contains two child obs, getAllObs could return a set with size = 1 if that obs contains the children (i.e., an object hierarchy) vs. flattening (and losing) the hierarchy. I searched for Encounter API documentation for “flatten” and didn’t see it mentioned anywhere.


(Mark Goodrich) #5

Yes, I think the confusion is around “getting” vs “flattening” but it has lead to some inconsistent behavior…

We can see that Encounter contains a set of obs:

And an Obs also has a reference to an Encounter:

When you add an Obs with nested members to an encounter, it only adds the top-level obs to the set, but it recursively sets the encounter on all the nested obs to the encounter you are adding it to. This would be the behavior I expect, but it also means that once you persist the encounter and then retrieve it again, Set will now contain all the obs in the encounter (ie the flattened set).

So what is in Set depends on whether the set has been persisted or not.

getAllObs() basically just returned that set (filtering out voided Obs).

My guess is that the original intent was for getAllObs to return the non-flattened set of obs, but after persistence and retrieval it essentially becomes a flattened set of obs, and I think in some cases the assumption has become that getAllObs is flattened. For instance, the saveEncounter method in the EncounterService:

So if 1) the common understanding has become that getAllObs returns a flattened list, and 2) when an encounter is retrieved from the DB it indeed returns a flattened list, do we want to change getAllObs (or even Set) to really contain the flattened list in all cases?

I think errors around this didn’t manifest themselves much in the past because previously you’d rarely be operating on a “detached” object after it was initially persisted. But in a RESTful world this isn’t the case.

If you post an encounter RESTfully to be updated, the saveEncounter API method is called before the encounter is reattached to the session (I think), and so when it calls getAllObs to make sure it updates all child obs with the correct date, it misses any obs not at the top level.

Hope that clarifies things… .:slight_smile:)

Take care, Mark


(Mark Goodrich) #6

ping @burke