TRUNK 5144:Concept save for coded concept throws hibernate exception

Unable to save code concept in Openmrs (>=2.1.0). Issue :TRUNK-5144

On investigation, we found that in Concept.java public void setAnswers(Collection answers) { this.answers = answers; } Here we are directly assigning the collection that is passed in the argument. If the argument is a different collection object than the one existing in this.answers we get the hibernate exception as the older collection is being orphaned.

This is an issue only from 2.1.x onwards, as there is a change in: public getAnswers(boolean includeRetired) where we previously returned the same collection object that is in concept.answers and now with commit Use of streams and lambdas we return a different collection object . The object returned here is used in openmrs-module-legacyui in ConceptFormController.

We can either revert the changes to the function: public getAnswers(boolean includeRetired) that were done as part of Use of streams and lambdas or modify public void setAnswers(Collection answers) like this

Any thoughts on what is the right approach to fix this? @dkayiwa and @raff

There is may be a not well documented practice that we recommend when working with collections in OpenMRS, in general in the core api for every collection there is an associated addXXX(Object) method, e.g. there is Concept.addAnswer(ConceptAnswer) that client code should be calling to add all the answers one by one and not setAnswers(Collection) to make client code safer, setAnswers() is there for historical reasons for hibernate to use probably, in fact going forward, for newer domain objects we don’t recommend exposing the setter for collections on the public API and set the access to field in the hibernate mapping file.

@wyclif, the offending code is in the legacyui module, so though perhaps this is not the recommended usage, it’s what OpenMRS has been doing “forever”.

I think that the quick-fix to this problem is to change this line in the legacyui to do getAnswers() instead of getAnswers(true).

Or we could change the openmrs-core code so it still returns the original list when you want to include retired ones, e.g.

getAnswers(boolean includeRetired) {
    if (includeRetired == true) {
        return getAnswers();
    } else {
        // do the stream stuff here
    }
}

Thanks everyone. @darius will make the changes as per your second suggestion so that if getAnswers(boolean includeRetired) is called from some other omod as well it gets the original list.

Made changes as per suggestion. PR link: https://github.com/openmrs/openmrs-core/pull/2133.