DATACMNS-683 - Tweaked the matching algorithm for DomainClassConverter.

DomainClassConverter as well as its internal ToEntity- and ToIdConverter implementations now actively refrain from matching if the source type is assignable to the target one to prevent unnecessary conversion attempts if the source value already actually is assignable to the target type.

Related ticket: DATACMNS-583.

Author: odrotbohm

src/main/java/org/springframework/data/repository/support/DomainClassConverter.java

@@ -166,12 +166,12 @@ public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor t
 		@Override
 		public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {
 
-			if (!repositories.hasRepositoryFor(targetType.getType())) {
+			if (sourceType.isAssignableTo(targetType)) {
 				return false;
 			}
 
-			if (sourceType.equals(targetType)) {
-				return true;
+			if (!repositories.hasRepositoryFor(targetType.getType())) {
+				return false;
 			}
 
 			Class<?> rawIdType = repositories.getRepositoryInformationFor(targetType.getType()).getIdType();
@@ -192,7 +192,7 @@ public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {
 	 * @author Oliver Gierke
 	 * @since 1.10
 	 */
-	private class ToIdConverter implements ConditionalGenericConverter {
+	class ToIdConverter implements ConditionalGenericConverter {
 
 		/*
 		 * (non-Javadoc)
@@ -232,12 +232,12 @@ public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor t
 		@Override
 		public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {
 
-			if (!repositories.hasRepositoryFor(sourceType.getType())) {
+			if (sourceType.isAssignableTo(targetType)) {
 				return false;
 			}
 
-			if (sourceType.equals(targetType)) {
-				return true;
+			if (!repositories.hasRepositoryFor(sourceType.getType())) {
+				return false;
 			}
 
 			Class<?> rawIdType = repositories.getRepositoryInformationFor(sourceType.getType()).getIdType();

src/test/java/org/springframework/data/repository/support/DomainClassConverterUnitTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2008-2014 the original author or authors.
+ * Copyright 2008-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +20,8 @@
 import static org.mockito.Matchers.*;
 import static org.mockito.Mockito.*;
 
+import java.lang.reflect.Method;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -30,10 +32,14 @@
 import org.springframework.beans.factory.support.DefaultListableBeanFactory;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.support.GenericApplicationContext;
+import org.springframework.core.MethodParameter;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.data.repository.CrudRepository;
 import org.springframework.data.repository.core.support.DummyRepositoryFactoryBean;
+import org.springframework.data.repository.support.DomainClassConverter.ToIdConverter;
+import org.springframework.test.util.ReflectionTestUtils;
+import org.springframework.web.bind.annotation.ModelAttribute;
 
 /**
  * Unit test for {@link DomainClassConverter}.
@@ -47,6 +53,7 @@ public class DomainClassConverterUnitTests {
 	static final User USER = new User();
 	static final TypeDescriptor STRING_TYPE = TypeDescriptor.valueOf(String.class);
 	static final TypeDescriptor USER_TYPE = TypeDescriptor.valueOf(User.class);
+	static final TypeDescriptor SUB_USER_TYPE = TypeDescriptor.valueOf(SubUser.class);
 	static final TypeDescriptor LONG_TYPE = TypeDescriptor.valueOf(Long.class);
 
 	@SuppressWarnings("rawtypes") DomainClassConverter converter;
@@ -144,11 +151,11 @@ public void discoversFactoryAndRepoFromParentApplicationContext() {
 	 * @see DATACMNS-583
 	 */
 	@Test
-	public void shouldReturnSourceObjectIfSourceAndTargetTypesAreTheSame() {
+	public void converterDoesntMatchIfTargetTypeIsAssignableFromSource() {
 
 		converter.setApplicationContext(initContextWithRepo());
 
-		assertThat(converter.matches(USER_TYPE, USER_TYPE), is(true));
+		assertThat(converter.matches(SUB_USER_TYPE, USER_TYPE), is(false));
 		assertThat((User) converter.convert(USER, USER_TYPE, USER_TYPE), is(USER));
 	}
 
@@ -186,6 +193,24 @@ public void supportsConversionFromEntityToString() {
 		assertThat(converter.matches(USER_TYPE, STRING_TYPE), is(true));
 	}
 
+	/**
+	 * @see DATACMNS-683
+	 */
+	@Test
+	public void toIdConverterDoesNotMatchIfTargetTypeIsAssignableFromSource() throws Exception {
+
+		converter.setApplicationContext(initContextWithRepo());
+
+		@SuppressWarnings("rawtypes")
+		DomainClassConverter.ToIdConverter toIdConverter = (ToIdConverter) ReflectionTestUtils.getField(converter,
+				"toIdConverter");
+
+		Method method = Wrapper.class.getMethod("foo", User.class);
+		TypeDescriptor target = TypeDescriptor.nested(new MethodParameter(method, 0), 0);
+
+		assertThat(toIdConverter.matches(SUB_USER_TYPE, target), is(false));
+	}
+
 	private ApplicationContext initContextWithRepo() {
 
 		BeanDefinitionBuilder builder = BeanDefinitionBuilder.rootBeanDefinition(DummyRepositoryFactoryBean.class);
@@ -199,10 +224,17 @@ private ApplicationContext initContextWithRepo() {
 		return ctx;
 	}
 
+	static interface Wrapper {
+
+		void foo(@ModelAttribute User user);
+	}
+
 	private static class User {
 
 	}
 
+	private static class SubUser extends User {}
+
 	private static interface UserRepository extends CrudRepository<User, Long> {
 
 	}