Rework HttpMessageConverters

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfiguration.java

@@ -96,7 +96,7 @@ public class EndpointWebMvcAutoConfiguration implements ApplicationContextAware,
 	public EndpointHandlerAdapter endpointHandlerAdapter(
 			final HttpMessageConverters messageConverters) {
 		EndpointHandlerAdapter adapter = new EndpointHandlerAdapter();
-		adapter.setMessageConverters(messageConverters.getMessageConverters());
+		adapter.setMessageConverters(messageConverters.getConverters());
 		return adapter;
 	}
 

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

@@ -17,95 +17,141 @@
 package org.springframework.boot.autoconfigure.web;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.LinkedHashSet;
+import java.util.Iterator;
 import java.util.List;
 
+import org.springframework.beans.factory.InitializingBean;
 import org.springframework.http.converter.HttpMessageConverter;
 import org.springframework.http.converter.xml.AbstractXmlHttpMessageConverter;
+import org.springframework.util.Assert;
 import org.springframework.web.servlet.config.annotation.WebMvcConfigurationSupport;
 
 /**
- * Convenient utility for adding and merging additional {@link HttpMessageConverter} in an
+ * Bean used to manage the {@link HttpMessageConverter}s used in a Spring Boot
- * application context. It also modifies the default converters a bit (putting XML
+ * application. Provides a convenient way to add and merge additional
- * converters at the back of the list if they are present).
+ * {@link HttpMessageConverter}s to a web application.
+ * <p>
+ * An instance of this bean can be registered with specific
+ * {@link #HttpMessageConverters(HttpMessageConverter...) additional converters} if
+ * needed, otherwise default converters will be used.
+ * <p>
+ * NOTE: The default converters used are the same as standard Spring MVC (see
+ * {@link WebMvcConfigurationSupport#getMessageConverters} with some slight re-ordering to
+ * put XML converters at the back of the list.
  * 
  * @author Dave Syer
+ * @author Phillip Webb
+ * @see #HttpMessageConverters(HttpMessageConverter...)
+ * @see #HttpMessageConverters(Collection)
+ * @see #getConverters()
+ * @see #setConverters(List)
  */
-public class HttpMessageConverters {
+public class HttpMessageConverters implements Iterable<HttpMessageConverter<?>>,
-
+		InitializingBean {
-	private List<HttpMessageConverter<?>> defaults;
-
-	private List<HttpMessageConverter<?>> overrides;
-
-	private Object lock = new Object();
 
 	private List<HttpMessageConverter<?>> converters;
 
-	public HttpMessageConverters() {
+	private boolean initialized;
-		this(Collections.<HttpMessageConverter<?>> emptyList());
+
+	/**
+	 * 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.
+	 */
+	public HttpMessageConverters(HttpMessageConverter<?>... additionalConverters) {
+		this(Arrays.asList(additionalConverters));
 	}
 
-	public HttpMessageConverters(Collection<HttpMessageConverter<?>> overrides) {
+	/**
-		this.overrides = new ArrayList<HttpMessageConverter<?>>(overrides);
+	 * Create a new {@link HttpMessageConverters} instance with the specified additional
-	}
+	 * converters.
-
+	 * @param additionalConverters additional converters to be added. New converters will
-	public List<HttpMessageConverter<?>> getMessageConverters() {
+	 * be added to the front of the list, overrides will replace existing items without
-		if (this.converters == null) {
+	 * changing the order. The {@link #getConverters()} methods can be used for further
-			synchronized (this.lock) {
+	 * converter manipulation.
-				if (this.converters == null) {
+	 */
-					getDefaultMessageConverters(); // ensure they are available
+	public HttpMessageConverters(Collection<HttpMessageConverter<?>> additionalConverters) {
-					Collection<HttpMessageConverter<?>> fallbacks = new LinkedHashSet<HttpMessageConverter<?>>();
+		this.converters = new ArrayList<HttpMessageConverter<?>>();
-					for (HttpMessageConverter<?> fallback : this.defaults) {
+		List<HttpMessageConverter<?>> defaultConverters = getDefaultConverters();
-						boolean overridden = false;
+		for (HttpMessageConverter<?> converter : additionalConverters) {
-						for (HttpMessageConverter<?> converter : this.overrides) {
+			int defaultConverterIndex = indexOfItemClass(defaultConverters, converter);
-							if (fallback.getClass()
+			if (defaultConverterIndex == -1) {
-									.isAssignableFrom(converter.getClass())) {
+				this.converters.add(converter);
-								if (!fallbacks.contains(converter)) {
+			}
-									fallbacks.add(converter);
+			else {
-									overridden = true;
+				defaultConverters.set(defaultConverterIndex, converter);
-								}
-							}
-						}
-						if (!overridden) {
-							fallbacks.add(fallback);
-						}
-					}
-					Collection<HttpMessageConverter<?>> converters = new LinkedHashSet<HttpMessageConverter<?>>(
-							this.overrides);
-					converters.addAll(fallbacks);
-					this.converters = new ArrayList<HttpMessageConverter<?>>(converters);
-				}
 			}
 		}
+		this.converters.addAll(defaultConverters);
+	}
+
+	private List<HttpMessageConverter<?>> getDefaultConverters() {
+		List<HttpMessageConverter<?>> converters = new ArrayList<HttpMessageConverter<?>>();
+		converters.addAll(new WebMvcConfigurationSupport() {
+			public List<HttpMessageConverter<?>> defaultMessageConverters() {
+				return super.getMessageConverters();
+			}
+		}.defaultMessageConverters());
+		reorderXmlConvertersToEnd(converters);
+		return converters;
+	}
+
+	private void reorderXmlConvertersToEnd(List<HttpMessageConverter<?>> converters) {
+		List<HttpMessageConverter<?>> xml = new ArrayList<HttpMessageConverter<?>>();
+		for (Iterator<HttpMessageConverter<?>> iterator = converters.iterator(); iterator
+				.hasNext();) {
+			HttpMessageConverter<?> converter = iterator.next();
+			if (converter instanceof AbstractXmlHttpMessageConverter) {
+				xml.add(converter);
+				iterator.remove();
+			}
+		}
+		converters.addAll(xml);
+	}
+
+	private <E> int indexOfItemClass(List<E> list, E item) {
+		Class<? extends Object> itemClass = item.getClass();
+		for (int i = 0; i < list.size(); i++) {
+			if (list.get(i).getClass().isAssignableFrom(itemClass)) {
+				return i;
+			}
+		}
+		return -1;
+	}
+
+	@Override
+	public Iterator<HttpMessageConverter<?>> iterator() {
+		return getConverters().iterator();
+	}
+
+	/**
+	 * Return a mutable list of the converters in the order that they will be registered.
+	 * Values in the list cannot be modified once the bean has been initialized.
+	 * @return the converters
+	 */
+	public List<HttpMessageConverter<?>> getConverters() {
 		return this.converters;
 	}
 
-	public List<HttpMessageConverter<?>> getDefaultMessageConverters() {
+	/**
-		if (this.defaults == null) {
+	 * Set the converters to use, replacing any existing values. This method can only be
-			synchronized (this.lock) {
+	 * called before the bean has been initialized.
-				if (this.defaults == null) {
+	 * @param converters the converters to set
-					this.defaults = new ArrayList<HttpMessageConverter<?>>();
+	 */
-					this.defaults.addAll(new WebMvcConfigurationSupport() {
+	public void setConverters(List<HttpMessageConverter<?>> converters) {
-						public List<HttpMessageConverter<?>> defaultMessageConverters() {
+		Assert.state(!this.initialized, "Unable to set converters once initialized");
-							return super.getMessageConverters();
+		this.converters = converters;
-						}
-					}.defaultMessageConverters());
-					List<HttpMessageConverter<?>> xmls = new ArrayList<HttpMessageConverter<?>>();
-					for (HttpMessageConverter<?> converter : this.defaults) {
-						// Shift XML converters to the back of the list so they only get
-						// used if nothing else works...
-						if (converter instanceof AbstractXmlHttpMessageConverter) {
-							xmls.add(converter);
-						}
-					}
-					this.defaults.removeAll(xmls);
-					this.defaults.addAll(xmls);
-				}
-			}
-		}
-		return Collections.unmodifiableList(this.defaults);
 	}
 
-}
+	@Override
+	public void afterPropertiesSet() throws Exception {
+		this.initialized = true;
+		this.converters = Collections.unmodifiableList(this.converters);
+	}
+}

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

@@ -133,7 +133,7 @@ public class WebMvcAutoConfiguration {
 
 		@Override
 		public void configureMessageConverters(List<HttpMessageConverter<?>> converters) {
-			converters.addAll(this.messageConverters.getMessageConverters());
+			converters.addAll(this.messageConverters.getConverters());
 		}
 
 		@Bean

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

@@ -42,6 +42,8 @@ import static org.mockito.Matchers.argThat;
 import static org.mockito.Mockito.verify;
 
 /**
+ * Tests for {@link HttpMessageConvertersAutoConfiguration}.
+ * 
  * @author Dave Syer
  */
 public class HttpMessageConvertersAutoConfigurationTests {
@@ -67,7 +69,7 @@ public class HttpMessageConvertersAutoConfigurationTests {
 				converter.getObjectMapper());
 		HttpMessageConverters converters = this.context
 				.getBean(HttpMessageConverters.class);
-		assertTrue(converters.getMessageConverters().contains(converter));
+		assertTrue(converters.getConverters().contains(converter));
 	}
 
 	@Test

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

@@ -16,43 +16,96 @@
 
 package org.springframework.boot.autoconfigure.web;
 
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.List;
 
+import org.junit.Rule;
 import org.junit.Test;
-import org.mockito.Mockito;
+import org.junit.rules.ExpectedException;
+import org.springframework.http.converter.ByteArrayHttpMessageConverter;
 import org.springframework.http.converter.HttpMessageConverter;
+import org.springframework.http.converter.ResourceHttpMessageConverter;
+import org.springframework.http.converter.StringHttpMessageConverter;
 import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter;
+import org.springframework.http.converter.support.AllEncompassingFormHttpMessageConverter;
+import org.springframework.http.converter.xml.Jaxb2RootElementHttpMessageConverter;
+import org.springframework.http.converter.xml.SourceHttpMessageConverter;
 
+import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
 
 /**
  * @author Dave Syer
+ * @author Phillip Webb
  */
 public class HttpMessageConvertersTests {
 
+	@Rule
+	public ExpectedException thrown = ExpectedException.none();
+
 	@Test
-	public void defaultsCreated() {
+	public void containsDefaults() throws Exception {
-		HttpMessageConverters messageConverters = new HttpMessageConverters();
+		HttpMessageConverters converters = new HttpMessageConverters();
-		assertFalse(messageConverters.getDefaultMessageConverters().isEmpty());
+		List<Class<?>> converterClasses = new ArrayList<Class<?>>();
+		for (HttpMessageConverter<?> converter : converters) {
+			converterClasses.add(converter.getClass());
+		}
+		assertThat(converterClasses, equalTo(Arrays.<Class<?>> asList(
+				ByteArrayHttpMessageConverter.class, StringHttpMessageConverter.class,
+				ResourceHttpMessageConverter.class, SourceHttpMessageConverter.class,
+				AllEncompassingFormHttpMessageConverter.class,
+				MappingJackson2HttpMessageConverter.class,
+				Jaxb2RootElementHttpMessageConverter.class)));
+	}
+
+	@Test
+	public void canModifyBeforeInitialize() throws Exception {
+		HttpMessageConverters converters = new HttpMessageConverters();
+		HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
+		converters.getConverters().add(converter);
+		assertThat(converters.getConverters().contains(converter), equalTo(true));
+	}
+
+	@Test
+	public void cannotModifyAfterInitialize() throws Exception {
+		HttpMessageConverters converters = new HttpMessageConverters();
+		converters.afterPropertiesSet();
+		this.thrown.expect(UnsupportedOperationException.class);
+		converters.getConverters().add(mock(HttpMessageConverter.class));
+	}
+
+	@Test
+	public void canSetBeforeInitialize() throws Exception {
+		HttpMessageConverters converters = new HttpMessageConverters();
+		converters.setConverters(new ArrayList<HttpMessageConverter<?>>());
+		assertThat(converters.getConverters().size(), equalTo(0));
+	}
+
+	@Test
+	public void cannotSetAfterInitailzie() throws Exception {
+		HttpMessageConverters converters = new HttpMessageConverters();
+		converters.afterPropertiesSet();
+		this.thrown.expect(IllegalStateException.class);
+		converters.setConverters(new ArrayList<HttpMessageConverter<?>>());
 	}
 
 	@Test
 	public void overrideExistingConverter() {
 		MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter();
-		HttpMessageConverters messageConverters = new HttpMessageConverters(
+		HttpMessageConverters converters = new HttpMessageConverters(converter);
-				Arrays.<HttpMessageConverter<?>> asList(converter));
+		assertTrue(converters.getConverters().contains(converter));
-		assertTrue(messageConverters.getMessageConverters().contains(converter));
 	}
 
 	@Test
-	public void addNewOne() {
+	public void addNewConverter() {
-		HttpMessageConverter<?> converter = Mockito.mock(HttpMessageConverter.class);
+		HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
-		HttpMessageConverters messageConverters = new HttpMessageConverters(
+		HttpMessageConverters converters = new HttpMessageConverters(converter);
-				Arrays.<HttpMessageConverter<?>> asList(converter));
+		assertTrue(converters.getConverters().contains(converter));
-		assertTrue(messageConverters.getMessageConverters().contains(converter));
+		assertEquals(converter, converters.getConverters().get(0));
-		assertEquals(converter, messageConverters.getMessageConverters().get(0));
 	}
 
 }

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

@@ -83,8 +83,7 @@ public class WebMvcAutoConfigurationTests {
 		assertEquals(3, this.context.getBeanNamesForType(HandlerAdapter.class).length);
 		assertFalse(this.context.getBean(RequestMappingHandlerAdapter.class)
 				.getMessageConverters().isEmpty());
-		assertEquals(this.context.getBean(HttpMessageConverters.class)
+		assertEquals(this.context.getBean(HttpMessageConverters.class).getConverters(),
-				.getMessageConverters(),
 				this.context.getBean(RequestMappingHandlerAdapter.class)
 						.getMessageConverters());
 	}