Confusing behavior with getAllObs method on Encounter

mogoodrich · February 23, 2019, 12:17am

(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:

github.com

openmrs/openmrs-core/blob/master/api/src/test/java/org/openmrs/EncounterTest.java#L369


		
		// make sure that the obs is the parent obs
		Obs obsInEncounter = (Obs) encounter.getObsAtTopLevel(false).toArray()[0];
		assertTrue(obsInEncounter.isObsGrouping());
	}
	
	/**
	 * @see Encounter#getAllObs(null)
	 */
	@Test
	public void getAllObs_shouldGetBothParentAndChildObs() {
		Encounter encounter = new Encounter();
		
		//create and add an Obs
		Obs parentObs = new Obs();
		encounter.addObs(parentObs);
		
		//add a child to the obs and make sure that the Obs is an ObsGroup with one child:
		Obs childObs = new Obs();
		parentObs.addGroupMember(childObs);

github.com

openmrs/openmrs-core/blob/master/api/src/test/java/org/openmrs/EncounterTest.java#L443


		// do the check
		assertEquals(1, encounter.getObsAtTopLevel(false).size());
		Obs obsInEncounter = (Obs) encounter.getObsAtTopLevel(false).toArray()[0];
		assertTrue(obsInEncounter.isObsGrouping());
	}
	
	/**
	 * @see Encounter#getAllObs(null)
	 */
	@Test
	public void getAllObs_shouldGetBothParentAndChildWithChildDirectlyOnEncounter() {
		Encounter enc = new Encounter();
		
		//create and add an Obs
		Obs parentObs = new Obs();
		enc.addObs(parentObs);
		
		//add a child to the obs and make sure that now that the Obs is an ObsGroup with one child:
		Obs childObs = new Obs();
		parentObs.addGroupMember(childObs);

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):

github.com

openmrs/openmrs-module-webservices.rest/blob/master/omod-1.9/src/test/java/org/openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/EncounterController1_9Test.java#L181


		encounter.put("encounterType", "61ae96f4-6afe-4351-b6f8-cd4fc383cce1");
		encounter.put("encounterDatetime", "2011-01-15");
		encounter.put("patient", "da7f524f-27ce-4bb2-86d6-6d1d05312bd5");
		encounter.put("obs", obsObject);
		
		MockHttpServletResponse response = handle(newPostRequest(getURI(), encounter));
		String newEncounterUuid = deserialize(response).get("uuid").toString();
		
		Encounter newEncounter = Context.getEncounterService().getEncounterByUuid(newEncounterUuid);
		Set<Obs> encounterObs = newEncounter.getAllObs();
		Assert.assertThat(encounterObs.size(), is(1));
		
		Obs parentObs = encounterObs.iterator().next();
		Assert.assertTrue(parentObs.hasGroupMembers());
		
		Set<Obs> parentGroupMembers = parentObs.getGroupMembers();
		Assert.assertThat(parentGroupMembers.size(), is(1));
		
		Obs childObs = parentGroupMembers.iterator().next();
		Assert.assertTrue(childObs.hasGroupMembers());

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 · March 1, 2019, 6:31pm

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.

mogoodrich · March 8, 2019, 6:40pm

@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 · March 8, 2019, 7:34pm

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.

mogoodrich · March 8, 2019, 8:07pm

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:

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/Encounter.java#L60


	
	private Form form;
	
	private EncounterType encounterType;
	
	private Set<Order> orders;


	private Set<Diagnosis> diagnoses;


	@AllowDirectAccess
	private Set<Obs> obs;
	
	private Visit visit;
	
	@DisableHandlers(handlerTypes = { VoidHandler.class })
	private Set<EncounterProvider> encounterProviders = new LinkedHashSet<>();
	
	// Constructors
	
	/** default constructor */
	public Encounter() {

And an Obs also has a reference to an Encounter:

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/Obs.java#L166


	protected String comment;
	
	protected transient Integer personId;
	
	protected Person person;
	
	protected Order order;
	
	protected Location location;
	
	protected Encounter encounter;
	
	private Obs previousVersion;
	
	private String formNamespaceAndPath;
	
	private Boolean dirty = Boolean.FALSE;
	
	private Interpretation interpretation;
	
	private Status status = Status.FINAL;

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:

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/api/impl/EncounterServiceImpl.java#L135


			// Therefore, encounter.patient must always equal
			// encounter.observations[0-n].patient
			
			// If we are changing encounter.encounterDatetime, then we need to
			// also apply that
			// to Obs that inherited their obsDatetime from the encounter in the
			// first place
			
			Date newDate = encounter.getEncounterDatetime();
			Location newLocation = encounter.getLocation();
			for (Obs obs : encounter.getAllObs(true)) {
				// if the date was changed
				if (OpenmrsUtil.compare(originalDate, newDate) != 0
				        && OpenmrsUtil.compare(obs.getObsDatetime(), originalDate) == 0) {
					
					// if the obs datetime is the same as the
					// original encounter datetime, fix it
					obs.setObsDatetime(newDate);
					
				}

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… .)

Take care, Mark

mogoodrich · March 21, 2019, 7:11pm

ping @burke

mogoodrich · August 26, 2019, 7:59pm

ping @burke… we will likely be working on this in our next sprint