RESTWS-901: Call property setters only once on map conversion (#581)

openmrs · Aug 24, 2023 · cbcf07f · cbcf07f
1 parent a15b055
commit cbcf07f
Show file tree

Hide file tree

Showing 2 changed files with 117 additions and 1 deletion.
diff --git a/omod-common/src/main/java/org/openmrs/module/webservices/rest/web/ConversionUtil.java b/omod-common/src/main/java/org/openmrs/module/webservices/rest/web/ConversionUtil.java
@@ -314,6 +314,7 @@ public static Object convertMap(Map<String, ?> map, Class<?> toClass) throws Con
 		}
 
 		// If the converter is a resource handler use the order of properties of its default representation
+		Set<String> alreadySetProperties = new HashSet<String>();
 		if (converter instanceof DelegatingResourceHandler) {
 
 			DelegatingResourceHandler handler = (DelegatingResourceHandler) converter;
@@ -324,13 +325,14 @@ public static Object convertMap(Map<String, ?> map, Class<?> toClass) throws Con
 				for (Map.Entry<String, Property> prop : resDesc.getProperties().entrySet()) {
 					if (map.containsKey(prop.getKey()) && !RestConstants.PROPERTY_FOR_TYPE.equals(prop.getKey())) {
 						converter.setProperty(ret, prop.getKey(), map.get(prop.getKey()));
+						alreadySetProperties.add(prop.getKey());
 					}
 				}
 			}
 		}
 
 		for (Map.Entry<String, ?> prop : map.entrySet()) {
-			if (RestConstants.PROPERTY_FOR_TYPE.equals(prop.getKey()))
+			if (RestConstants.PROPERTY_FOR_TYPE.equals(prop.getKey()) || alreadySetProperties.contains(prop.getKey()))
 				continue;
 			converter.setProperty(ret, prop.getKey(), prop.getValue());
 		}

diff --git a/omod-common/src/test/java/org/openmrs/module/webservices/rest/web/ConversionUtilTest.java b/omod-common/src/test/java/org/openmrs/module/webservices/rest/web/ConversionUtilTest.java
@@ -12,6 +12,8 @@
 import static org.hamcrest.core.Is.is;
 import org.junit.Assert;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 import java.lang.reflect.Method;
 import java.lang.reflect.Type;
@@ -20,14 +22,25 @@
 import java.util.Arrays;
 import java.util.Calendar;
 import java.util.Date;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.TimeZone;
 
 import org.apache.commons.beanutils.PropertyUtils;
 import org.junit.Test;
+import org.mockito.Matchers;
+import org.openmrs.User;
 import org.openmrs.api.ConceptNameType;
+import org.openmrs.module.webservices.rest.web.annotation.Resource;
 import org.openmrs.module.webservices.rest.web.representation.Representation;
+import org.openmrs.module.webservices.rest.web.resource.api.Converter;
+import org.openmrs.module.webservices.rest.web.resource.impl.DelegatingResourceDescription;
+import org.openmrs.module.webservices.rest.web.resource.impl.DelegatingResourceHandler;
+import org.openmrs.module.webservices.rest.web.resource.impl.MetadataDelegatingCrudResource;
+import org.openmrs.module.webservices.rest.web.response.ConversionException;
+import org.openmrs.module.webservices.rest.web.response.ResponseException;
 import org.openmrs.web.test.BaseModuleWebContextSensitiveTest;
 
 public class ConversionUtilTest extends BaseModuleWebContextSensitiveTest {
@@ -289,6 +302,47 @@ public void getTypeVariableClass_shouldThrowIllegalArgumentExceptionWhenTypeVari
 		ConversionUtil.getTypeVariableClass(Temp.class, null);
 	}
 
+	@Test
+	@SuppressWarnings({ "rawtypes" })
+	public void setProperty_onlyCalledOnceWhenDelegatingResourceDescriptionIsPresent() {
+		String expectedKey = "someKey";
+		String expectedValue = "someValue";
+		String expectedUUID = "12345";
+
+		Map<String, Object> mapToConvert = new HashMap<String, Object>();
+		mapToConvert.put(expectedKey, expectedValue);
+		mapToConvert.put(RestConstants.PROPERTY_UUID, expectedUUID);
+		mapToConvert.put(RestConstants.PROPERTY_FOR_TYPE, "someType");
+
+		Converter<DummyUser> converter = ConversionUtil.getConverter(DummyUser.class);
+		Assert.assertNotNull(converter);
+
+		DelegatingResourceHandler handler = mock(DelegatingResourceHandler.class);
+		DelegatingResourceDescription resDesc = mock(DelegatingResourceDescription.class);
+
+		Map<String, DelegatingResourceDescription.Property> resourceDescProperties = new HashMap<String, DelegatingResourceDescription.Property>();
+		DelegatingResourceDescription.Property mockProperty = mock(DelegatingResourceDescription.Property.class);
+		resourceDescProperties.put(expectedKey, mockProperty);
+
+		DelegatingResourceDescription representationDesc = ((DelegatingResourceHandler) converter)
+		        .getRepresentationDescription(Representation.DEFAULT);
+
+		Map<String, DelegatingResourceDescription.Property> repDescProperties = new HashMap<String, DelegatingResourceDescription.Property>();
+		repDescProperties.put(expectedKey, mockProperty);
+
+		DummyUser expectedUser = mock(DummyUser.class);
+		((DummyUserConverter) converter).addUser(expectedUUID, expectedUser);
+
+		when(handler.getRepresentationDescription(Matchers.<Representation> any())).thenReturn(resDesc);
+		when(resDesc.getProperties()).thenReturn(resourceDescProperties);
+		when(representationDesc.getProperties()).thenReturn(repDescProperties);
+
+		DummyUser actualuser = (DummyUser) ConversionUtil.convertMap(mapToConvert, DummyUser.class);
+
+		Assert.assertEquals(expectedUser, actualuser);
+		Assert.assertEquals(1, (int) ((DummyUserConverter) converter).getPropertyKeyToNbTimesSet().get(expectedKey));
+	}
+
 	public abstract class BaseGenericType<T> {
 
 		private T value;
@@ -348,4 +402,64 @@ public class GrandchildGenericType_Int extends ChildGenericType_Int {}
 	public class GreatGrandchildGenericType_Int extends GrandchildGenericType_Int {}
 
 	public class ChildMultiGenericType extends BaseMultiGenericType<Integer, String, Temp> {}
+
+	@Resource(name = "dummyUserConverter", supportedClass = DummyUser.class, supportedOpenmrsVersions = { "1.0.* - 9.*" })
+	public static class DummyUserConverter extends MetadataDelegatingCrudResource<DummyUser> {
+
+		private final DelegatingResourceDescription mockRepDesc = mock(DelegatingResourceDescription.class);
+
+		private final Map<String, Object> properties = new HashMap<String, Object>();
+
+		private final Map<String, Integer> propertyKeyToNbTimesSet = new HashMap<String, Integer>();
+
+		private final Map<String, DummyUser> uniqueIdToUser = new HashMap<String, DummyUser>();
+
+		public void addUser(String uniqueId, DummyUser user) {
+			uniqueIdToUser.put(uniqueId, user);
+		}
+
+		@Override
+		public void setProperty(Object instance, String propertyName, Object value) throws ConversionException {
+			properties.put(propertyName, value);
+			Integer nbTimesCalled = propertyKeyToNbTimesSet.get(propertyName);
+			propertyKeyToNbTimesSet.put(propertyName, nbTimesCalled == null ? 1 : nbTimesCalled + 1);
+		}
+
+		@Override
+		public DummyUser getByUniqueId(String uniqueId) {
+			return uniqueIdToUser.get(uniqueId);
+		}
+
+		@Override
+		public DelegatingResourceDescription getRepresentationDescription(Representation rep) {
+			return mockRepDesc;
+		}
+
+		@Override
+		public void purge(DummyUser delegate, RequestContext context) throws ResponseException {
+			// Do nothing
+		}
+
+		@Override
+		public DummyUser newDelegate() {
+			return null;
+		}
+
+		@Override
+		public DummyUser save(DummyUser delegate) {
+			return delegate;
+		}
+
+		public Map<String, Object> getProperties() {
+			return properties;
+		}
+
+		public Map<String, Integer> getPropertyKeyToNbTimesSet() {
+			return propertyKeyToNbTimesSet;
+		}
+	}
+
+	protected static class DummyUser extends User {
+		// No overrides needed
+	}
 }