Change in the REST WS Module PropertySetter Logic

ibewes · February 9, 2016, 11:15am

Has there been a change in the RESTWS module PropertySetter annotation logic in version 2.12? We have a resource which defines a property setter for a collection property which is no longer being used properly in version 2.12 and 2.13. In previous versions our property setter was called as expected to handle setting the property during a POST.

Also, it appears that the ordering logic of how the properties are handled has changed as of version 2.12 as well. Was this intentional? Normally I would not expect the ordering to be consistent, but there is a comment in the org.openmrs.module.webservices.rest.web.resource.impl.BaseDelegatingResource.setConvertedProperties method which says that it is.

I can provide more detail on our issue if needed; I just wanted to ask generally first in case there was a change that I just didn’t see.

dkayiwa · February 9, 2016, 11:28am

Regarding the ordering, is this in anyway related?

ibewes · February 9, 2016, 11:42am

I was actually referring to the order used on the server side. The comment I mentioned says the following:

Apply properties in the order specified in the resource description (necessary e.g. so the obs resource can apply “concept” before “value”); we have already excluded unchanged and ignored properties. some resources (e.g. any AttributeResource) require some properties to be set before others can be fetched, we apply each property in its iteration, rather than testing everything first and applying later.

This comment is found here:

github.com

openmrs/openmrs-module-webservices.rest/blob/master/omod-common/src/main/java/org/openmrs/module/webservices/rest/web/resource/impl/BaseDelegatingResource.java#L597


      
          		// Because some resources (e.g. any AttributeResource) require some properties to be set before others can
          		// be fetched, we apply each property in its iteration, rather than testing everything first and applying later.
          		for (String property : allowedProperties.keySet()) {
          			if (!propertiesToSet.containsKey(property)) {
          				continue;
          			}
          			if (propertiesToSet.containsKey(property)) {
          				// Ignore any properties that were not actually changed, also covering the case where you post back an
          				// incomplete rep of a complex property
          				Object oldValue = getProperty(delegate, property);
          				Object newValue = propertiesToSet.get(property);
          				if (unchangedValue(oldValue, newValue)) {
          					propertiesToSet.remove(property);
          					continue;
          				}
          				
          				setProperty(delegate, property, propertiesToSet.get(property));
          			}
          		}
          		
          		// If any non-settable properties remain after the above logic, fail

However, the ordering is a much less important question to us as compared to the PropertySetter logic which is currently resulting in exceptions for us in certain situations.

darius · February 9, 2016, 1:47pm

@ibewes I see that I did some refactoring / moving of code in this commit which happened for 2.12. I don’t remember if I intentionally changed anything.

Can you write a test case that fails when you run it against 2.12+ but passes on 2.11?

ibewes · February 10, 2016, 11:45am

@darius Thanks for looking into this. I have dug a little deeper and our issue appears to be a direct result of some changes to the ConversionUtil.convertMap method (this commit). Previous to this commit, the method would create a new object and then set each property as defined in the json. This commit changed the logic to now check for a ‘uuid’ field and, if found, use that to attempt to load the object from the database. While this change makes sense in many cases, it doesn’t really make sense when the resource in question does not have a data service to get data from the database (not to mention, adding a significant and often useless round-trip to the db).

In our situation, we have a parent model which has a collection of child models associated with it (Bill and BillLineItem). There is no way to save or load the child models apart from getting the parent model and so our code has been set up to throw an exception when an attempt is made given that this is not supported, as opposed to returning null which can be confusing and might result in duplicate records.

For the time being I think we can work around this change in behavior, but I question whether the updated convertMap logic is reasonable given what this method says it should be doing. I certainly didn’t expect (or want) this method to hit the db. Any thoughts?

darius · February 10, 2016, 7:11pm

@ibewes, it’s helpful to know that you had a use case that got broken by the code. So, the original thing we were trying to fix is https://issues.openmrs.org/browse/RESTWS-460 and you can see from this comment that I was thinking about scenarios that didn’t match our cookie cutter one, but I couldn’t actually think of the right ones.

Any chance that you can write a (failing) unit test as part of the webservices.rest module codebase to illustrate your scenario?

ibewes · February 24, 2016, 6:58am

@darius I will work on an example unit test for this today. However, my previous concern about how this change seems like a poor design choice remains.

While I understand the goal of the issue you linked, we designed our client-side code to work properly given the previous design. The change in version 2.12 fundamentally breaks that logic and we are now having to go back and put in a rather hacky fix in each child resource (overriding the getByUniqueId function to return null). I also think that implementing this change directly into the convertMap function is confusing as it greatly changes the logic of the function and seems to go way beyond simply converting a map to an object as the name would imply.

At the very least, it would be nice if this change were implemented so that it was backwards compatible with how things worked in version 2.11 and before. Hopefully a failing test that I will create will make it a bit more obvious what we have been doing that is now broken.

darius · February 24, 2016, 12:21pm

@ibewes obviously it’s bad that we broke your code, that was relying on a particular implementation of the base and utility classes.

Is there some automated test process that could have led to catching this earlier? E.g. some CI build that the latest versions of your modules against the latest snapshots of their module dependencies? Or you could put a suite of tests in the webservices.rest module showing a broad range of how you use the module?

ibewes · February 24, 2016, 1:05pm

@darius Digging into this further has shown that there is plenty of blame on our side as well. We wrote some base classes that tie resources to our data service framework so that we don’t have to write that code for each specific resource. That base code made some assumptions that broke with RESTWS version 2.12 but we’re working around that change now (see here). Here is a simple test (in patch format) which shows what we were doing that worked in 2.11 and is now broken as of 2.12 (it’s not exactly what we were doing simplifies it to show the core issue). Thanks for looking into this and let me know if you have any questions.

ibewes · February 24, 2016, 1:10pm

@darius Your question about tests that could catch this earlier is a good one. We are planning on adding REST tests and end-to-end integration tests in the relatively near future but those test would all be on our end. Do you have any process right now for including downstream module implementations in your tests?

We utilize REST for all our client-server interactions and are therefore quite sensitive to changes in the RESTWS module. If we can help to ensure that future changes don’t break our modules, or at least find out before your changes are released, that would be a great help to us. Let me know if you can think of a good way forward.