@Override
public Obs getByUniqueId(String uniqueId) {
return Context.getObsService().getObsByUuid(uniqueId);
}
I am guessing that the above method does not return a complex obs when it should.
Am I correct to assume that either ObsResource1_8.getByUniqueId(String uniqueId) or ObsServiceImpl.getObsByUuid(String uuid) should be modified to handle complex obs?
I believe the bug rather lies in Coreâs ObsServiceImpl.saveObs(Obs obs, String changeMessage).
I woud like to suggest the following change where changeMessage conveniently (and bit hackily) could be used to specify the complex obsâs âviewâ:
public Obs saveObs(Obs obs, String changeMessage) throws APIException {
if (null != obs && null != obs.getConcept() && obs.getConcept().isComplex()) {
String view = changeMessage;
Obs complexObs = getComplexObs(obs.getId(), view);
if (null != complexObs && null != obs.getComplexData()) {
// save or update complexData object on this obs
// this is done before the database save so that the obs.valueComplex
// can be filled in by the handler.
ComplexObsHandler handler = getHandler(obs);
if (null != handler) {
handler.saveObs(obs);
} else {
throw new APIException("Unknown handler for " + obs.getConcept());
}
}
}
...
}
I believe the NullPointerException mentioned in my previous post should definitely have been avoided in the first place. In the sense that we want to check that complexData is not null but not that its inner data isnât. One could imagine that someone attempts to save a complex obs with a null inner data, and it is up to the handler to take care of this.
P.S. I believe this ancient ticket reported the same symptom: ERR-391.
I wouldnât change the meaning of changeMessage, from the API client point of view will be confusing (even if the javadoc explains the hack). If there is a specific method to save a complex obs, I would change the logic in the resource class and invoke the appropriate method from there.
I get the same error when I try to add new obs to the encounter which has complex obs. When obs is loaded with encounter, complexData will be null and it throws NullPointerException at this line.
Below is the failing test:
@Test
@Verifies(value="should save encounter with complex obs", method="saveEncounter(Encounter)")
public void saveEncounter_shouldSaveEncounterWithComplexObs() throws Exception {
executeDataSet(ENC_OBS_HIERARCHY_DATA_XML);
EncounterService es = Context.getEncounterService();
Encounter encounter = es.getEncounter(101);
Obs observation = buildObs();
observation.setLocation(encounter.getLocation());
observation.setPerson(encounter.getPatient());
encounter.addObs(observation);
es.saveEncounter(encounter);
Context.flushSession();
Context.clearSession();
encounter = es.getEncounter(101);
assertEquals(2, encounter.getObsAtTopLevel(true).size());
}
It makes sense to check if complexData is not null, though it makes me wonder why one is posting it when itâs null, the REST API currently is not designed to support posting of complex Obs so I would think this is technically not a bug
It makes sense to check that the complex data member is not null but not, IMHO, to check that the inner data itself is not null (thatâs up to the complex data handlers to deal with that kind of situations).
And that is what is happening in ObsServiceImpl and that causes troubles:
@mksd i do not see much value in trying to avoid a NPE at this point. If it is a complex obs but with getComplexData() returning null, then we should not have reached here in the very first place. Something like the validators should have prevented the call to saveObs() from happening.
Otherwise, by preventing this NPE, you would be saving meaningless data.
Sounds right for the validator to reject a complex Obs with null complex data, checking if complexData is null is pointless in the API because it isnât expected to be null
Iâm not necessarily trying to avoid the NPE though, I just thought that whatever is inside the complex data is the business of the ad-hoc complex data handler, and not of ObsService itself.
I would rather see a handler throwing an APIException when there is an attempt to save a complex obs with an invalid complex data (such as one with a null inner data for instance).
On my end I worked around this problem anyway, and for the rest I trust your opinions then
Itâs the role of the API to ensure that we are saving clean data, why would you save a complex obs if the data is null? The handlerâs job is to save and retrieve the complex data
In my case, I am not trying to save complex object without complex data. Infact, am not touching the obs at all. All I am doing is, adding a new obs to the encounter which contains complex obs. Since complex data is not loaded by default, when encounter is saved, it tries to save all the obs in it and it finds complexData as null for complex obs.
Wyclif, I donât know how to interpret this statement.
The OpenMRS platform needs to support all aspects of complex obs through its REST API. Whether we call this a defect or an enhancement doesnât matter: we should treat all limitations of the REST API as high-priority issues for the community to address.
I suggest we get this fixed in Platform 2.0.1, that @preethi_s is actively working on now. So, please letâs create any relevant tickets and get them ready-for-work.
