ConceptResource 'name' property not correctly handled by the available converter

[samuel34 (Samuel Male)]

Hi all,

Background

The appModelContext, holds reference to patientPrograms at-least on the patient clinicianfacing dashboard context. As part of work on AF-59, we want to consume the available patientPrograms property. However, the patientPrograms was serialized to a SimpleObject format and in the process de-serializing it back(java), conversion of the associated concepts fails: See in unit test below

@Test
public void testConceptConverter() {
    // Setup
	List<Concept> concepts = Context.getConceptService().getAllConcepts();
	List<SimpleObject> serialisedData = (List<SimpleObject>) ConversionUtil.convertToRepresentation(concepts, Representation.DEFAULT);
	List<Concept> deserialisedData = new ArrayList<Concept>();
    // Replay
	for (SimpleObject obj : serialisedData) {
		// Fails here
		// The converter seems to treat concept.name property as a String, hence will fail to find the `ConceptName` converter to process the name
		deserialisedData.add((Concept) ConversionUtil.convert(obj, Concept.class));
	}
}

View on github here.

The Issue

Can’t de-serialize a concept from it’s SimpleObject Representation to java using the available utility method: ConversionUtil.convert(...)(converter handler under the hood). The above test fails with this error.

Bug Explained

The conversion utility method that serializes a concept works based on the DelegatingResourceDescription depending on the Representation, like you can see, it contains a name property which is a SimpleObject and not a string value(see here ). That’s how we end-up with the name’s property value as an object(Simple object map of name properties) and not a string value.This becomes an issue when de-serialising, the converter looks at the setter method of the property it’s currently processing to determine the target datatype we are converting to, for the case of the name property, it will try to converting it to the String type/class; Since the name value is an instance of a map , it will look for a converter of type String which of course doesn’t exist hence the failing(see here)

Possible Fix

We need to have an end to end compatibility of these two operations ie. Serializing a Resource to its Representation and de-serializing it back by the same API.

• I would put a String value, (Either the name’s ‘uuid’ or ‘actual name value’ or ‘something better’) here.

• Support more than one property setter depending on the candidate method’s(this could be one of the setters of the same property) signature(Using reflection, we can inspect a method’s parameters and compare it with the current property value of instance we are processing)
  See pseudo code below:

/**
	 * Sets the name property to be the fully specified name of the Concept in the current locale
	 * 
	 * <p>
	 *   This will be invoked if property value is an instanceof String
	 * 
	 * @param instance
	 * @param name
	 */
	@PropertySetter("name")
	public static void setFullySpecifiedName(Concept instance, String name) {
		ConceptName fullySpecifiedName = new ConceptName(name, Context.getLocale());
		instance.setFullySpecifiedName(fullySpecifiedName);
	}
	
	/**
	 * This would be invoked if the property value is an instanceof a Map
	 * 
	 * @param instance
	 * @param name
	 */
	@PropertySetter("name")
	public static void setFullySpecifiedName(Concept instance, ConceptName name) {
		instance.setFullySpecifiedName(name);
	}
 

–> 2 may introduce a slightly major change in the way @PropertySetter annotations are processed while 1 is minor and simpler.

NB: This is the same issue that happens for other properties like concept.answers.

Thoughts??

/cc: @mksd, @mogoodrich, @dkayiwa

[mksd (Dimitri R)]

@dkayiwa this is wrong IMO:

@PropertySetter("name")
public static void setFullySpecifiedName(Concept instance, String name) {
  ConceptName fullySpecifiedName = new ConceptName(name, Context.getLocale());
  instance.setFullySpecifiedName(fullySpecifiedName);
}

This is not ok, it always assumes that the target locale is that out of the user’s session, aka Context.getLocale(). There is no way to specify the locale for the FSN being set.

My take is that this should be deprecated in favour of a setter that takes a ConceptName as an input, but obviously that might break a lot of existing code out there (or not).

Anyone, thoughts? @burke, @ibacher, @mogoodrich, @mseaton @wyclif

---

@samuel34 will explore a little what it’d take to make REST WS able to analyse the best match out of multiple possible @PropertySetter candidates when deserializing.

[ssmusoke (Stephen Senkomago Musoke)]

@mksd Shouldn’t the approach be (if I remember what I have seen somewhere)

1. Fully specified name for Locale (defined) - one should not be able to have multiple FULLY specified names for the same Locale
2. Short name for Locale
3. Description

[mksd (Dimitri R)]

@ssmusoke you’re stating business rules that may or may not be appropriate given an implementation context. But neither Bahmni nor the Ref App currently follows this guideline anyway.

What I’m saying is that the REST WS support for deserializing the name property of Concept is making an assumption that it shouldn’t.

[mogoodrich (Mark Goodrich)]

@mksd @samuel34 thanks!

Yes, @mksd I agreed that this method should really be:

	@PropertySetter("name")
	public static void setName(Concept instance, ConceptName name) 

But like you said, I don’t have a good sense of the potential ramifications… at PIH I know of no case where we actually are updating concepts via the REST endpoint, so it’s possible that there will be no ramifications, but… We’d also have to figure out exactly what the above method should do… assumedy look for any names within the concept with that uuid and update if found, and then add otherwise?

I’d be fine with (cautiously) doing some testing around this.

Also, looking at the Concept resource I see that the “name” property is not creatable but is updatable. Since “name” is really a convenience reference more than anything else, I don’t know if it would make sense to make it non-updatable?

Take care, Mark