Retain default order in HttpMessageConverters

The original fix for gh-1293 (commit 05e6af2) 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

Author: Phillip Webb

spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/HttpMessageConverters.java

@@ -67,16 +67,27 @@ public HttpMessageConverters(HttpMessageConverter<?>... additionalConverters) {
 	/**
 	 * 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<HttpMessageConverter<?>> additionalConverters) {
 		List<HttpMessageConverter<?>> converters = new ArrayList<HttpMessageConverter<?>>();
-		List<HttpMessageConverter<?>> defaultConverters = getDefaultConverters();
-		converters.addAll(additionalConverters);
-		converters.addAll(defaultConverters);
+		List<HttpMessageConverter<?>> processing = new ArrayList<HttpMessageConverter<?>>(
+				additionalConverters);
+		for (HttpMessageConverter<?> defaultConverter : getDefaultConverters()) {
+			Iterator<HttpMessageConverter<?>> 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);
 	}
 

spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/HttpMessageConvertersTests.java

@@ -34,6 +34,7 @@
 
 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 void containsDefaults() throws Exception {
 	}
 
 	@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<MappingJackson2HttpMessageConverter> httpConverters = new ArrayList<MappingJackson2HttpMessageConverter>();
+		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));
 	}
 
 }