From a8704d426ea40bf592da41d29f2810045dd43b00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Sat, 15 Nov 2014 02:15:56 -0200 Subject: [PATCH 1/9] Adding a test to check converters priority Closes #876. When a converter B with more priority than A are registered, vraptor is getting only the first converter, ignoring priority. --- .../vraptor/core/DefaultConvertersTest.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java b/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java index 675bff7cf..d22b5cda7 100644 --- a/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java +++ b/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java @@ -17,11 +17,14 @@ package br.com.caelum.vraptor.core; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.typeCompatibleWith; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.when; +import javax.annotation.Priority; + import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -66,6 +69,18 @@ public void convertingANonAnnotatedConverterEndsUpComplaining() { converters.register(WrongConverter.class); } + @Test + public void shouldChooseConverterWithGreaterPriority() { + converters.register(MyConverter.class); + converters.register(MySecondConverter.class); + + when(container.instanceFor(MyConverter.class)).thenReturn(new MyConverter()); + when(container.instanceFor(MySecondConverter.class)).thenReturn(new MySecondConverter()); + + Object converter = converters.to(MyData.class); + assertThat(converter, instanceOf(MySecondConverter.class)); + } + class WrongConverter implements Converter { @Override @@ -78,7 +93,7 @@ class MyData { } @Convert(MyData.class) - class MyConverter implements Converter { + private class MyConverter implements Converter { @Override public MyData convert(String value, Class type) { return null; @@ -86,7 +101,8 @@ public MyData convert(String value, Class type) { } @Convert(MyData.class) - class MySecondConverter implements Converter { + @Priority(javax.interceptor.Interceptor.Priority.APPLICATION) + private class MySecondConverter implements Converter { @Override public MyData convert(String value, Class type) { return null; From 829f8ad7488525c462e044cb466b1961f737305f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Sat, 15 Nov 2014 02:17:32 -0200 Subject: [PATCH 2/9] Fixing converter priority When a converter with more priority are registered, the DefaultConverters override the current converter registered, allowing users to override the interface Converter instead of override the LongConverter. --- .../vraptor/core/DefaultConverters.java | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java index 732b1faf6..fe9e950eb 100644 --- a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java +++ b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java @@ -20,7 +20,9 @@ import static com.google.common.base.Preconditions.checkState; import java.util.LinkedList; +import java.util.List; +import javax.annotation.Priority; import javax.enterprise.context.ApplicationScoped; import javax.inject.Inject; @@ -34,15 +36,11 @@ import br.com.caelum.vraptor.converter.Converter; import br.com.caelum.vraptor.ioc.Container; -import com.google.common.base.Predicate; -import com.google.common.base.Supplier; -import com.google.common.collect.FluentIterable; - @ApplicationScoped public class DefaultConverters implements Converters { private final Logger logger = LoggerFactory.getLogger(DefaultConverters.class); - private final LinkedList>> classes = new LinkedList<>(); + private final List>> classes = new LinkedList<>(); @LRU private final CacheStore, Class>> cache; @@ -67,37 +65,48 @@ public void register(Class> converterClass) { Convert type = converterClass.getAnnotation(Convert.class); checkState(type != null, "The converter type %s should have the Convert annotation", converterClass.getName()); + Class> other = findConverterType(type.value()); + if (!other.equals(NullConverter.class)) { + int priority = getConverterPriority(converterClass); + int priorityOther = getConverterPriority(other); + + checkState(priority != priorityOther, "Converter %s have same priority than %s", converterClass, other); + + if (priority > priorityOther) { + logger.debug("Overriding converter {} with {} because have more priority", other, converterClass); + classes.remove(other); + classes.add(converterClass); + } + } + logger.debug("adding converter {} to {}", converterClass, type.value()); classes.add(converterClass); } + private int getConverterPriority(Class> converter) { + Priority priority = converter.getAnnotation(Priority.class); + return priority == null ? 0 : priority.value(); + } + @SuppressWarnings("unchecked") @Override public Converter to(Class clazz) { Class> converterType = findConverterType(clazz); checkState(!converterType.equals(NullConverter.class), "Unable to find converter for %s", clazz.getName()); + logger.debug("found converter {} to {}", converterType.getName(), clazz.getName()); return (Converter) container.instanceFor(converterType); } private Class> findConverterType(final Class clazz) { - return cache.fetch(clazz, new Supplier>>() { - @Override - public Class> get() { - return FluentIterable.from(classes).filter(matchConverter(clazz)) - .first().or(NullConverter.class); + for (Class> current : classes) { + Class boundType = current.getAnnotation(Convert.class).value(); + if (boundType.isAssignableFrom(clazz)) { + return current; } - }); - } + } - private Predicate> matchConverter(final Class clazz) { - return new Predicate>() { - @Override - public boolean apply(Class input) { - Class boundType = input.getAnnotation(Convert.class).value(); - return boundType.isAssignableFrom(clazz); - } - }; + return NullConverter.class; } private interface NullConverter extends Converter {}; From 92be772231625a5d5e74cb2a34a1cc6c2dc36f0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Sat, 15 Nov 2014 02:32:36 -0200 Subject: [PATCH 3/9] Testing converters with same priority --- .../vraptor/core/DefaultConvertersTest.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java b/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java index d22b5cda7..ae69ef9a8 100644 --- a/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java +++ b/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java @@ -81,6 +81,15 @@ public void shouldChooseConverterWithGreaterPriority() { assertThat(converter, instanceOf(MySecondConverter.class)); } + @Test + public void shouldForbidConverterWithSamePriority() { + exception.expect(IllegalStateException.class); + exception.expectMessage(String.format("Converter %s have same priority than %s", MyThirdConverter.class, MySecondConverter.class)); + + converters.register(MySecondConverter.class); + converters.register(MyThirdConverter.class); + } + class WrongConverter implements Converter { @Override @@ -109,6 +118,15 @@ public MyData convert(String value, Class type) { } } + @Convert(MyData.class) + @Priority(javax.interceptor.Interceptor.Priority.APPLICATION) + private class MyThirdConverter implements Converter { + @Override + public MyData convert(String value, Class type) { + return null; + } + } + @Test public void registersAndUsesTheConverterInstaceForTheSpecifiedType() { converters.register(MyConverter.class); From f3adacf9a5f1d087bde2177cb9770c82ed7a2aad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Sat, 15 Nov 2014 03:00:29 -0200 Subject: [PATCH 4/9] Improving docs --- .../br/com/caelum/vraptor/core/Converters.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java index 664e7e8c2..f2ec2314e 100644 --- a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java +++ b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java @@ -21,9 +21,7 @@ import br.com.caelum.vraptor.converter.Converter; /** - * Represents a collection of converters.
- * Note: This interface will probably change in the near future to allow - * annotation support. + * Represents a collection of converters. * * @author Guilherme Silveira */ @@ -31,14 +29,19 @@ public interface Converters { /** * Extracts a converter for this specific type. - * - * @param type - * @return */ Converter to(Class type); + /** + * Register a converter. Implementations must guarantee that converters + * are registered always keeping its priority. + * @param converterClass + */ void register(Class> converterClass); + /** + * Returns true if a converter exists for the type. + */ boolean existsFor(Class type); boolean existsTwoWayFor(Class type); From f78dc59dd69933f8858e86a42c206170fda86070 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Sat, 15 Nov 2014 12:37:27 -0200 Subject: [PATCH 5/9] Adding more log messages and renaming objects --- .../caelum/vraptor/core/DefaultConverters.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java index fe9e950eb..dbf29d656 100644 --- a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java +++ b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java @@ -65,17 +65,19 @@ public void register(Class> converterClass) { Convert type = converterClass.getAnnotation(Convert.class); checkState(type != null, "The converter type %s should have the Convert annotation", converterClass.getName()); - Class> other = findConverterType(type.value()); - if (!other.equals(NullConverter.class)) { + Class> currentConverter = findConverterType(type.value()); + if (!currentConverter.equals(NullConverter.class)) { int priority = getConverterPriority(converterClass); - int priorityOther = getConverterPriority(other); + int priorityCurrent = getConverterPriority(currentConverter); - checkState(priority != priorityOther, "Converter %s have same priority than %s", converterClass, other); + checkState(priority != priorityCurrent, "Converter %s have same priority than %s", converterClass, currentConverter); - if (priority > priorityOther) { - logger.debug("Overriding converter {} with {} because have more priority", other, converterClass); - classes.remove(other); + if (priority > priorityCurrent) { + logger.debug("Overriding converter {} with {} because have more priority", currentConverter, converterClass); + classes.remove(currentConverter); classes.add(converterClass); + } else { + logger.debug("Converter {} not registered because have less priority than {}", converterClass, currentConverter); } } @@ -106,6 +108,7 @@ private Class> findConverterType(final Class clazz) { } } + logger.debug("Unable to find a converter for {}. Returning NullConverter.", clazz); return NullConverter.class; } From 251498cf7be32fa3078265c0b952a750059e87c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Sat, 15 Nov 2014 12:47:38 -0200 Subject: [PATCH 6/9] Adding more docs about behavior And some information about people who have contributed with this class. --- .../main/java/br/com/caelum/vraptor/core/Converters.java | 6 +++++- .../br/com/caelum/vraptor/core/DefaultConverters.java | 8 ++++++++ .../br/com/caelum/vraptor/core/DefaultConvertersTest.java | 2 ++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java index f2ec2314e..23d078eaf 100644 --- a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java +++ b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java @@ -21,7 +21,11 @@ import br.com.caelum.vraptor.converter.Converter; /** - * Represents a collection of converters. + * Represents a collection of all converters registered by VRaptor. When the application + * {@link #register(Class)} method is called to register all converters found. Before + * registration the default implementation check the priority of each converter, and + * in the case of multiple converter for the same type, only the converter with highest + * priority will registered. * * @author Guilherme Silveira */ diff --git a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java index dbf29d656..d2bba22e5 100644 --- a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java +++ b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java @@ -36,6 +36,14 @@ import br.com.caelum.vraptor.converter.Converter; import br.com.caelum.vraptor.ioc.Container; +/** + * Default implementation for {@link Converters}. + * + * @author Guilherme Silveira + * @author Rodrigo Turini + * @author Lucas Cavalcanti + * @author Otávio Scherer Garcia + */ @ApplicationScoped public class DefaultConverters implements Converters { diff --git a/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java b/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java index ae69ef9a8..19de46b9d 100644 --- a/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java +++ b/vraptor-core/src/test/java/br/com/caelum/vraptor/core/DefaultConvertersTest.java @@ -24,6 +24,7 @@ import static org.mockito.Mockito.when; import javax.annotation.Priority; +import javax.interceptor.Interceptor; import org.junit.Before; import org.junit.Rule; @@ -102,6 +103,7 @@ class MyData { } @Convert(MyData.class) + @Priority(Interceptor.Priority.LIBRARY_BEFORE) private class MyConverter implements Converter { @Override public MyData convert(String value, Class type) { From 63358942fb248536185cf7bf0fc7d9707b7dcf36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Sat, 15 Nov 2014 12:51:07 -0200 Subject: [PATCH 7/9] Getting back the cache --- .../vraptor/core/DefaultConverters.java | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java index d2bba22e5..a1716003a 100644 --- a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java +++ b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java @@ -29,6 +29,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.base.Supplier; + import br.com.caelum.vraptor.Convert; import br.com.caelum.vraptor.TwoWayConverter; import br.com.caelum.vraptor.cache.CacheStore; @@ -109,15 +111,21 @@ public Converter to(Class clazz) { } private Class> findConverterType(final Class clazz) { - for (Class> current : classes) { - Class boundType = current.getAnnotation(Convert.class).value(); - if (boundType.isAssignableFrom(clazz)) { - return current; + return cache.fetch(clazz, new Supplier>>() { + + @Override + public Class> get() { + for (Class> current : classes) { + Class boundType = current.getAnnotation(Convert.class).value(); + if (boundType.isAssignableFrom(clazz)) { + return current; + } + } + + logger.debug("Unable to find a converter for {}. Returning NullConverter.", clazz); + return NullConverter.class; } - } - - logger.debug("Unable to find a converter for {}. Returning NullConverter.", clazz); - return NullConverter.class; + }); } private interface NullConverter extends Converter {}; From 53a75addb6fcc5b256dd5b505aa5f22c5d7c0529 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Sat, 15 Nov 2014 13:08:45 -0200 Subject: [PATCH 8/9] Fixing cache store when register a converter --- .../vraptor/core/DefaultConverters.java | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java index a1716003a..43d83555f 100644 --- a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java +++ b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/DefaultConverters.java @@ -29,8 +29,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.google.common.base.Supplier; - import br.com.caelum.vraptor.Convert; import br.com.caelum.vraptor.TwoWayConverter; import br.com.caelum.vraptor.cache.CacheStore; @@ -38,6 +36,8 @@ import br.com.caelum.vraptor.converter.Converter; import br.com.caelum.vraptor.ioc.Container; +import com.google.common.base.Supplier; + /** * Default implementation for {@link Converters}. * @@ -103,47 +103,52 @@ private int getConverterPriority(Class> converter) { @SuppressWarnings("unchecked") @Override public Converter to(Class clazz) { - Class> converterType = findConverterType(clazz); + Class> converterType = findConverterTypeFromCache(clazz); checkState(!converterType.equals(NullConverter.class), "Unable to find converter for %s", clazz.getName()); logger.debug("found converter {} to {}", converterType.getName(), clazz.getName()); return (Converter) container.instanceFor(converterType); } - private Class> findConverterType(final Class clazz) { + private Class> findConverterTypeFromCache(final Class clazz) { return cache.fetch(clazz, new Supplier>>() { @Override public Class> get() { - for (Class> current : classes) { - Class boundType = current.getAnnotation(Convert.class).value(); - if (boundType.isAssignableFrom(clazz)) { - return current; - } - } - - logger.debug("Unable to find a converter for {}. Returning NullConverter.", clazz); - return NullConverter.class; + return findConverterType(clazz); } + }); } + private Class> findConverterType(final Class clazz) { + for (Class> current : classes) { + Class boundType = current.getAnnotation(Convert.class).value(); + if (boundType.isAssignableFrom(clazz)) { + return current; + } + } + + logger.debug("Unable to find a converter for {}. Returning NullConverter.", clazz); + return NullConverter.class; + } + private interface NullConverter extends Converter {}; @Override public boolean existsFor(Class type) { - return !findConverterType(type).equals(NullConverter.class); + return !findConverterTypeFromCache(type).equals(NullConverter.class); } @Override public boolean existsTwoWayFor(Class type) { - return TwoWayConverter.class.isAssignableFrom(findConverterType(type)); + return TwoWayConverter.class.isAssignableFrom(findConverterTypeFromCache(type)); } @Override public TwoWayConverter twoWayConverterFor(Class type) { checkState(existsTwoWayFor(type), "Unable to find two way converter for %s", type.getName()); - return (TwoWayConverter) container.instanceFor(findConverterType(type)); + return (TwoWayConverter) container.instanceFor(findConverterTypeFromCache(type)); } } From c299c0857f37d65790a6370fbfe07be73e6f0ebb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Garcia?= Date: Mon, 17 Nov 2014 10:27:11 -0200 Subject: [PATCH 9/9] Typo --- .../main/java/br/com/caelum/vraptor/core/Converters.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java index 23d078eaf..ace4319c5 100644 --- a/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java +++ b/vraptor-core/src/main/java/br/com/caelum/vraptor/core/Converters.java @@ -22,9 +22,9 @@ /** * Represents a collection of all converters registered by VRaptor. When the application - * {@link #register(Class)} method is called to register all converters found. Before - * registration the default implementation check the priority of each converter, and - * in the case of multiple converter for the same type, only the converter with highest + * starts, the {@link #register(Class)} method is called to register all converters found. + * Before registration the default implementation check the priority of each converter, + * and in the case of multiple converter for the same type, only the converter with highest * priority will registered. * * @author Guilherme Silveira