Preserve ordering when auto-configuring WebSocket MessageConverters

Previously, WebSocketMessagingAutoConfiguration added a single
additional converter. This was a MappingJackson2MessageConverter
configured with the auto-configured ObjectMapper.
AbstractMessageBrokerConfiguration places additional converters before
any of the default converters. This meant that the auto-configuration
had the unwanted side-effect of changing the ordering of the
converters. A MappingJackson2MessageConverter was now first in the
list, whereas, by default, it's last in the list after a
StringMessageConverter and a ByteArrayMessageConverter.

This commit updates WebSocketMessagingAutoConfiguration so that it
switches off the registration of the default converters and registers
a StringMessageConverter, ByteArrayMessageConverter and
MappingJackson2MessageConverter in that order. A test has been
added to verify that the types of these three converters match
the types of the default converters. A second test that verifies
that String responses are converted correctly has also been added
alongside the existing test that verified the behaviour for JSON
responses.

Closes gh-5123

Author: wilkinsona

spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/websocket/WebSocketMessagingAutoConfiguration.java

@@ -28,9 +28,11 @@
 import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication;
 import org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.messaging.converter.ByteArrayMessageConverter;
 import org.springframework.messaging.converter.DefaultContentTypeResolver;
 import org.springframework.messaging.converter.MappingJackson2MessageConverter;
 import org.springframework.messaging.converter.MessageConverter;
+import org.springframework.messaging.converter.StringMessageConverter;
 import org.springframework.messaging.simp.config.AbstractMessageBrokerConfiguration;
 import org.springframework.util.MimeTypeUtils;
 import org.springframework.web.socket.config.annotation.AbstractWebSocketMessageBrokerConfigurer;
@@ -72,8 +74,10 @@ public boolean configureMessageConverters(
 			DefaultContentTypeResolver resolver = new DefaultContentTypeResolver();
 			resolver.setDefaultMimeType(MimeTypeUtils.APPLICATION_JSON);
 			converter.setContentTypeResolver(resolver);
+			messageConverters.add(new StringMessageConverter());
+			messageConverters.add(new ByteArrayMessageConverter());
 			messageConverters.add(converter);
-			return true;
+			return false;
 		}
 
 	}

spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/websocket/WebSocketMessagingAutoConfigurationTests.java

@@ -17,12 +17,15 @@
 package org.springframework.boot.autoconfigure.websocket;
 
 import java.lang.reflect.Type;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Iterator;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -39,6 +42,8 @@
 import org.springframework.boot.test.EnvironmentTestUtils;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.messaging.converter.CompositeMessageConverter;
+import org.springframework.messaging.converter.MessageConverter;
 import org.springframework.messaging.converter.SimpleMessageConverter;
 import org.springframework.messaging.simp.annotation.SubscribeMapping;
 import org.springframework.messaging.simp.config.MessageBrokerRegistry;
@@ -49,9 +54,11 @@
 import org.springframework.messaging.simp.stomp.StompSessionHandler;
 import org.springframework.messaging.simp.stomp.StompSessionHandlerAdapter;
 import org.springframework.stereotype.Controller;
+import org.springframework.test.util.ReflectionTestUtils;
 import org.springframework.web.client.RestTemplate;
 import org.springframework.web.socket.client.standard.StandardWebSocketClient;
 import org.springframework.web.socket.config.annotation.AbstractWebSocketMessageBrokerConfigurer;
+import org.springframework.web.socket.config.annotation.DelegatingWebSocketMessageBrokerConfiguration;
 import org.springframework.web.socket.config.annotation.EnableWebSocket;
 import org.springframework.web.socket.config.annotation.EnableWebSocketMessageBroker;
 import org.springframework.web.socket.config.annotation.StompEndpointRegistry;
@@ -62,6 +69,7 @@
 import org.springframework.web.socket.sockjs.client.WebSocketTransport;
 
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
@@ -92,7 +100,49 @@ public void tearDown() {
 	}
 
 	@Test
-	public void basicMessagingWithJson() throws Throwable {
+	public void basicMessagingWithJsonResponse() throws Throwable {
+		Object result = performStompSubscription("/app/json");
+		assertThat(new String((byte[]) result),
+				is(equalTo(String.format("{%n  \"foo\" : 5,%n  \"bar\" : \"baz\"%n}"))));
+	}
+
+	@Test
+	public void basicMessagingWithStringResponse() throws Throwable {
+		Object result = performStompSubscription("/app/string");
+		assertThat(new String((byte[]) result),
+				is(equalTo(String.format("string data"))));
+	}
+
+	@Test
+	public void customizedConverterTypesMatchDefaultConverterTypes() {
+		List<MessageConverter> customizedConverters = getCustomizedConverters();
+		List<MessageConverter> defaultConverters = getDefaultConverters();
+		assertThat(customizedConverters.size(), is(equalTo(defaultConverters.size())));
+		Iterator<MessageConverter> customizedIterator = customizedConverters.iterator();
+		Iterator<MessageConverter> defaultIterator = defaultConverters.iterator();
+		while (customizedIterator.hasNext()) {
+			assertThat(customizedIterator.next(),
+					is(instanceOf(defaultIterator.next().getClass())));
+		}
+	}
+
+	private List<MessageConverter> getCustomizedConverters() {
+		List<MessageConverter> customizedConverters = new ArrayList<MessageConverter>();
+		WebSocketMessagingAutoConfiguration.WebSocketMessageConverterConfiguration configuration = new WebSocketMessagingAutoConfiguration.WebSocketMessageConverterConfiguration();
+		ReflectionTestUtils.setField(configuration, "objectMapper", new ObjectMapper());
+		configuration.configureMessageConverters(customizedConverters);
+		return customizedConverters;
+	}
+
+	@SuppressWarnings("unchecked")
+	private List<MessageConverter> getDefaultConverters() {
+		CompositeMessageConverter compositeDefaultConverter = new DelegatingWebSocketMessageBrokerConfiguration()
+				.brokerMessageConverter();
+		return (List<MessageConverter>) ReflectionTestUtils
+				.getField(compositeDefaultConverter, "converters");
+	}
+
+	private Object performStompSubscription(final String topic) throws Throwable {
 		EnvironmentTestUtils.addEnvironment(this.context, "server.port:0",
 				"spring.jackson.serialization.indent-output:true");
 		this.context.register(WebSocketMessagingConfiguration.class);
@@ -107,7 +157,7 @@ public void basicMessagingWithJson() throws Throwable {
 			@Override
 			public void afterConnected(StompSession session,
 					StompHeaders connectedHeaders) {
-				session.subscribe("/app/data", new StompFrameHandler() {
+				session.subscribe(topic, new StompFrameHandler() {
 
 					@Override
 					public void handleFrame(StompHeaders headers, Object payload) {
@@ -155,8 +205,8 @@ public void handleTransportError(StompSession session, Throwable exception) {
 				fail("Response was not received within 30 seconds");
 			}
 		}
-		assertThat(new String((byte[]) result.get()),
-				is(equalTo(String.format("{%n  \"foo\" : 5,%n  \"bar\" : \"baz\"%n}"))));
+
+		return result.get();
 	}
 
 	@Configuration
@@ -201,11 +251,16 @@ public TomcatWebSocketContainerCustomizer tomcatCustomizer() {
 	@Controller
 	static class MessagingController {
 
-		@SubscribeMapping("/data")
-		Data getData() {
+		@SubscribeMapping("/json")
+		Data json() {
 			return new Data(5, "baz");
 		}
 
+		@SubscribeMapping("/string")
+		String string() {
+			return "string data";
+		}
+
 	}
 
 	static class Data {