RestServiceImpl behavior

teleivo · September 29, 2016, 2:18pm

Hi there

while writing tests for the RestServiceImpl I’ve seen the following, which I would like to get your opinion on whether you think the behavior is ok (and if so why) or should be changed:

ONE: List<DelegatingResourceHandler<?>> RestServiceImpl.getResourceHandlers()

Returns only DelegatingResourceHandler which support the current Openmrs version but it returns DelegatingSubclassHandler regardless of their supportedOpenmrsVersions.

This test shows the described behavior:

github.com

openmrs/openmrs-module-webservices.rest/blob/master/omod-common/src/test/java/org/openmrs/module/webservices/rest/web/api/impl/RestServiceImplTest.java#L808


      
          		when(restHelperService.getRegisteredSearchHandlers()).thenReturn(asList(searchHandler1));
          		
          		RestUtil.disableContext(); //to avoid a Context call
          		
          		Map<String, String[]> parameters = new HashMap<String, String[]>();
          		parameters.put("source", new String[] { "some source" });
          		
          		assertThat(restService.getSearchHandler("v1/order", parameters), is(nullValue()));
          	}
          	
          	/**
          	 * @verifies return null if no search handler is found for current openmrs version
          	 * @see RestServiceImpl#getSearchHandler(String, Map)
          	 */
          	@Test
          	public void getSearchHandler_shouldReturnNullIfNoSearchHandlerIsFoundForCurrentOpenmrsVersion() throws Exception {
          		
          		SearchHandler searchHandler1 = mock(SearchHandler.class);
          		SearchConfig searchConfig1 = new SearchConfig("default", "v1/concept", "1.8.*", new SearchQuery.Builder(
          		        "description").withRequiredParameters("source").withOptionalParameters("code").build());
          		when(searchHandler1.getSearchConfig()).thenReturn(searchConfig1);

TWO: Resource RestServiceImpl.getResourceBySupportedClass(Class<?> resourceClass)

An example hierarchy:

Animal → Bird → MockingBird

Animal has a Resource AnimalResource_1_9 and Bird has its own Resource BirdResource_1_9 (which is not a SubclassHandler but its own Resource! This is the point, so read on

MockingBird does not have its own Resource and also no SubclassHandler.

In this case:

getResourceBySupportedClass(MockingBird.class) returns BirdResource_1_9

This test shows the described behavior:

github.com

openmrs/openmrs-module-webservices.rest/blob/master/omod-common/src/test/java/org/openmrs/module/webservices/rest/web/api/impl/RestServiceImplTest.java#L454


      
          		setCurrentOpenmrsVersion("1.9.10");
          		
          		assertThat(restService.getResourceBySupportedClass(Cat.class), instanceOf(AnimalResource_1_9.class));
          	}
          	
          	/**
          	 * @verifies return resource supporting direct superclass of given class if no resource
          	 *           supporting given class was found but multiple resources supporting multiple
          	 *           superclasses exist
          	 * @see RestServiceImpl#getResourceBySupportedClass(Class) <p>
          	 */
          	@Test
          	public void getResourceBySupportedClass_shouldReturnResourceSupportingDirectSuperclassOfGivenClassIfNoResourceSupportingGivenClassWasFoundButMultipleResourcesSupportingMultipleSuperclassesExist()
          	        throws Exception {
          		
          		List<Class<? extends Resource>> resources = new ArrayList<Class<? extends Resource>>();
          		resources.add(AnimalResource_1_9.class);
          		resources.add(AnimalResource_1_11.class);
          		resources.add(BirdResource_1_9.class);
          		
          		when(openmrsClassScanner.getClasses(Resource.class, true)).thenReturn(resources);

Now my question to this behavior is why is this implemented in such a way? I mean we advocate pro Subclasshandler if a domain object is a subclass of a domain object which already has a Resource. So why is this supported?

wyclif · September 29, 2016, 6:59pm

For the first one, my assumption is that in the module’s config.xml file we should have conditional resource entries for each omod that contains classes that require a specific version of core or later which is pretty much all except those for 1.8 and 1.9, currently there is one entry for the 2.0 omod. I also believe that it’s these conditional resource entries that currently determine which resource is available or not when a module is deployed based on the openmrs version, I haven’t touched that code in a while so I might be wrong. But If my assumptions are correct then I guess the unit tests are the ones that are wrong and blind to the fact that conditional resource loading doesn’t happen in tests. BTW why do we still have the 1.8 sub module? If the required version is now 1.9.10, shouldn’t we merge it with the the 1.9 sub module? Or are we just being lazy since it requires a fair amount of refactoring?

As for the second case, I would say it’s kind of historical based on the initial code we wrote to make it possible for a resource to work for an entire class hierarchy before subclass handlers were introduced, although this is only 100% achievable for sub classes within core because we can bake their fields within the parent class’ resource since their fields are known which is not the case for module sub classes. If you have ever noticed, this is the reason why ConceptNumeric, a subclass of Concept has no resource nor a subclass handler and is handled by the ConceptResource. But I’m more inclined to believe that this behavior is more historical than intentional

teleivo · September 30, 2016, 9:48am

aha, so do you think that the config.xml conditionalResources should be changed?

I dont think that the unit tests are wrong, they just test this unit of work which simply behaves this way. A release test might be good which does know about the config.xml

interesting who could answer this?

Sure, but the current implementation allows several Resource’s in a class hierarchy and gives you the most specific for a class without Resource. In your example of ConceptNumeric there is only a ConceptResource.

wyclif · September 30, 2016, 5:56pm

I would think the unit test is not exactly appropriate and should not test that behavior because it’s testing a method that doesn’t encapsulate the filtering logic. From my assumptions, filtering by openmrs version is done by the conditional resource loading mechanism and not the RestServiceImpl which happens to just return all loaded handlers.

As for the sub classes issue, as I mentioned that behavior is historic so it’s possible that later changes were made without it in mind.

darius · October 3, 2016, 1:50pm

I think Wyclif is correct that we should consider merging the 1.8 resources into the 1.9 submodule. (I don’t know if we definitely want to do this or not.) We have not prioritized refactoring this. Not because we’re lazy, but because we’re smart. I would personally rate it low priority (it doesn’t provide any direct value to an implementation), but if someone wants to take this up, they are welcome!

This seems pretty contrived. What would it take to run into this issue in real life? E.g. Order has a Resource, RadiologyOrder is also a Resource, and then you have an XyzRadiologyOrder without a resource or subclass handler. Why would this happen?

wyclif · October 3, 2016, 2:25pm

I do agree that merging 1.8 into the 1.9 sub module is of low priority