From 5e1552b71867b657a4985fcfb356ffc489acc48e Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 28 Jul 2014 15:31:46 -0700 Subject: [PATCH] Retain default order in HttpMessageConverters The original fix for gh-1293 (commit 05e6af23) caused test failures due to the fact that Spring Boot's MappingJackson2HttpMessageConverter was added before Spring's default StringHttpMessageConverter. This commit changes the HttpMessageConverters logic so that additional converts are added just before any default converter of the same type. This allows additional converters to be added whilst still retaining the sensible ordering of the default converters. Fixes gh-1293 --- .../web/HttpMessageConverters.java | 25 ++++++++---- .../web/HttpMessageConvertersTests.java | 39 ++++++++++++------- 2 files changed, 42 insertions(+), 22 deletions(-) diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/HttpMessageConverters.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/HttpMessageConverters.java index ac52b71fd98..f779ffff6b7 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/HttpMessageConverters.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/HttpMessageConverters.java @@ -67,16 +67,27 @@ public class HttpMessageConverters implements Iterable> /** * Create a new {@link HttpMessageConverters} instance with the specified additional * converters. - * @param additionalConverters additional converters to be added. New converters will - * be added to the front of the list, overrides will replace existing items without - * changing the order. The {@link #getConverters()} methods can be used for further - * converter manipulation. + * @param additionalConverters additional converters to be added. Items are added just + * before any default converter of the same type (or at the front of the list if no + * default converter is found) The {@link #getConverters()} methods can be used for + * further converter manipulation. */ public HttpMessageConverters(Collection> additionalConverters) { List> converters = new ArrayList>(); - List> defaultConverters = getDefaultConverters(); - converters.addAll(additionalConverters); - converters.addAll(defaultConverters); + List> processing = new ArrayList>( + additionalConverters); + for (HttpMessageConverter defaultConverter : getDefaultConverters()) { + Iterator> iterator = processing.iterator(); + while (iterator.hasNext()) { + HttpMessageConverter candidate = iterator.next(); + if (ClassUtils.isAssignableValue(defaultConverter.getClass(), candidate)) { + converters.add(candidate); + iterator.remove(); + } + } + converters.add(defaultConverter); + } + converters.addAll(0, processing); this.converters = Collections.unmodifiableList(converters); } diff --git a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/HttpMessageConvertersTests.java b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/HttpMessageConvertersTests.java index 49ad34abf80..91ae26af35e 100644 --- a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/HttpMessageConvertersTests.java +++ b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/HttpMessageConvertersTests.java @@ -34,6 +34,7 @@ import org.springframework.http.converter.xml.SourceHttpMessageConverter; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; @@ -65,27 +66,35 @@ public class HttpMessageConvertersTests { } @Test - public void overrideExistingConverter() { - MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter(); - HttpMessageConverters converters = new HttpMessageConverters(converter); - assertTrue(converters.getConverters().contains(converter)); - int count = 0; - for (HttpMessageConverter httpMessageConverter : converters) { - if (httpMessageConverter instanceof MappingJackson2HttpMessageConverter) { - count++; + public void addBeforeExistingConverter() { + MappingJackson2HttpMessageConverter converter1 = new MappingJackson2HttpMessageConverter(); + MappingJackson2HttpMessageConverter converter2 = new MappingJackson2HttpMessageConverter(); + HttpMessageConverters converters = new HttpMessageConverters(converter1, + converter2); + assertTrue(converters.getConverters().contains(converter1)); + assertTrue(converters.getConverters().contains(converter2)); + List httpConverters = new ArrayList(); + for (HttpMessageConverter candidate : converters) { + if (candidate instanceof MappingJackson2HttpMessageConverter) { + httpConverters.add((MappingJackson2HttpMessageConverter) candidate); } } // The existing converter is still there, but with a lower priority - assertEquals(2, count); - assertEquals(0, converters.getConverters().indexOf(converter)); + assertEquals(3, httpConverters.size()); + assertEquals(0, httpConverters.indexOf(converter1)); + assertEquals(1, httpConverters.indexOf(converter2)); + assertNotEquals(0, converters.getConverters().indexOf(converter1)); } @Test - public void addNewConverter() { - HttpMessageConverter converter = mock(HttpMessageConverter.class); - HttpMessageConverters converters = new HttpMessageConverters(converter); - assertTrue(converters.getConverters().contains(converter)); - assertEquals(converter, converters.getConverters().get(0)); + public void addNewConverters() { + HttpMessageConverter converter1 = mock(HttpMessageConverter.class); + HttpMessageConverter converter2 = mock(HttpMessageConverter.class); + HttpMessageConverters converters = new HttpMessageConverters(converter1, + converter2); + assertTrue(converters.getConverters().contains(converter1)); + assertEquals(converter1, converters.getConverters().get(0)); + assertEquals(converter2, converters.getConverters().get(1)); } }