GH-3938: Fix HTTP XML configuration for ambiguity (#3939)

* GH-3938: Fix HTTP XML configuration for ambiguity

Fixes https://github.com/spring-projects/spring-integration/issues/3938

The `encoding-mode` is a property of the `RestTemplate`.
Therefore, it cannot be set on the component configuration together with
an externally injected `rest-template`

Even if `HttpRequestExecutingMessageHandler` has an assertion for such
an ambiguity, the XML parser just ignores this `encoding-mode`
when it encounters the `rest-template`

* Fix `HttpAdapterParsingUtils.verifyNoRestTemplateAttributes()` to check for not allowed
attributes with a `encoding-mode` as well
* Remove a `default` from the `encoding-mode` to not cause an ambiguity in the parser
* Fix some typos in the `spring-integration-http.xsd`
* Rework `OutboundResponseTypeTests` to JUnit 5

**Cherry-pick to `5.5.x`**

* * Fix error handling for `encoding-mode` in the `HttpAdapterParsingUtils`
* Cover `encoding-mode` and `rest-template` ambiguity with a test against failing XML configuration

* * Improve error message for ambiguous attributes in the `HttpAdapterParsingUtils`

spring-integration-http/src/main/java/org/springframework/integration/http/config/HttpAdapterParsingUtils.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2022 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -36,9 +36,10 @@ import org.springframework.util.xml.DomUtils;
  * @author Gary Russell
  * @author Artem Bilan
  * @author Shiliang Li
+ *
  * @since 2.0.2
  */
-abstract class HttpAdapterParsingUtils {
+final class HttpAdapterParsingUtils {
 
 	static final String[] SYNC_REST_TEMPLATE_REFERENCE_ATTRIBUTES = {
 			"request-factory", "error-handler", "message-converters"
@@ -48,14 +49,22 @@ abstract class HttpAdapterParsingUtils {
 		for (String attributeName : SYNC_REST_TEMPLATE_REFERENCE_ATTRIBUTES) {
 			if (element.hasAttribute(attributeName)) {
 				parserContext.getReaderContext().error("When providing a 'rest-template' reference, the '"
-								+ attributeName + "' attribute is not allowed.",
+								+ attributeName + "' attribute is not allowed, " +
+								"it must be set on the provided template instead",
 						parserContext.extractSource(element));
 			}
 		}
+
+		if (element.hasAttribute("encoding-mode")) {
+			parserContext.getReaderContext().error("When providing a 'rest-template' reference, " +
+							"the 'encoding-mode' must be set on the 'RestTemplate.uriTemplateHandler' property.",
+					parserContext.extractSource(element));
+		}
 	}
 
 	static void configureUriVariableExpressions(BeanDefinitionBuilder builder, ParserContext parserContext,
 			Element element) {
+
 		String uriVariablesExpression = element.getAttribute("uri-variables-expression");
 
 		List<Element> uriVariableElements = DomUtils.getChildElementsByTagName(element, "uri-variable");
@@ -73,7 +82,7 @@ abstract class HttpAdapterParsingUtils {
 		}
 
 		if (hasUriVariableExpressions) {
-			ManagedMap<String, Object> uriVariableExpressions = new ManagedMap<String, Object>();
+			ManagedMap<String, Object> uriVariableExpressions = new ManagedMap<>();
 			for (Element uriVariableElement : uriVariableElements) {
 				String name = uriVariableElement.getAttribute("name");
 				String expression = uriVariableElement.getAttribute("expression");
@@ -146,8 +155,8 @@ abstract class HttpAdapterParsingUtils {
 
 		if (hasExpectedResponseType && hasExpectedResponseTypeExpression) {
 			parserContext.getReaderContext()
-					.error("The 'expected-response-type' and 'expected-response-type-expression' are mutually exclusive. " +
-							"You can only have one or the other", element);
+					.error("The 'expected-response-type' and 'expected-response-type-expression' " +
+							"are mutually exclusive. You can only have one or the other", element);
 		}
 
 		RootBeanDefinition expressionDef = null;
@@ -164,4 +173,7 @@ abstract class HttpAdapterParsingUtils {
 		}
 	}
 
+	private HttpAdapterParsingUtils() {
+	}
+
 }

spring-integration-http/src/main/resources/org/springframework/integration/http/config/spring-integration-http.xsd

@@ -73,8 +73,7 @@
 						By default 'status-code-expression' is null, meaning that the default '200 OK' response status
 						will be returned.
 						The 'http:inbound-gateway' resolves the 'status code' from the 'http_statusCode' header of the
-						reply
-						Message.
+						reply Message.
 					</xsd:documentation>
 				</xsd:annotation>
 			</xsd:attribute>
@@ -412,7 +411,7 @@
 				<xsd:annotation>
 					<xsd:documentation>
 						Specify whether the outbound message's payload should be extracted
-						when preparing the request body. Otherwise the Message instance itself
+						when preparing the request body. Otherwise, the Message instance itself
 						will be serialized.
 						The default value is 'true'.
 					</xsd:documentation>
@@ -484,7 +483,7 @@
 						<xsd:annotation>
 							<xsd:documentation>
 								Specifies whether the outbound message's payload should be extracted
-								when preparing the request body. Otherwise the Message instance itself
+								when preparing the request body. Otherwise, the Message instance itself
 								will be serialized.
 								The default value is 'true'.
 							</xsd:documentation>
@@ -493,8 +492,8 @@
 					<xsd:attribute name="transfer-cookies" type="xsd:string" default="false">
 						<xsd:annotation>
 							<xsd:documentation><![CDATA[
-	When set to "true", if a response contains a 'Set-Cookie' header, it will be mapped to a 'Cookie' header. This enables simple
-	cookie handling where subsequent HTTP interactions in the same message flow can use a cookie
+	When set to "true", if a response contains a 'Set-Cookie' header, it will be mapped to a 'Cookie' header.
+	This enables simple cookie handling where subsequent HTTP interactions in the same message flow can use a cookie
 	supplied by the server. Default is "false".
 							]]></xsd:documentation>
 						</xsd:annotation>
@@ -602,7 +601,8 @@
 	<xsd:complexType name="headerType">
 		<xsd:annotation>
 			<xsd:documentation><![CDATA[
-				Expression to be evaluated against the ServletRequest(makes BODY and Headers available) as well as URI Variables (e.g., foo/bar/{id}).
+				Expression to be evaluated against the ServletRequest(makes BODY and Headers available)
+				as well as URI Variables (e.g., foo/bar/{id}).
 			]]></xsd:documentation>
 		</xsd:annotation>
 		<xsd:attribute name="name" use="required">
@@ -759,7 +759,7 @@
 		<xsd:attribute name="allow-credentials" default="true">
 			<xsd:annotation>
 				<xsd:documentation>
-					Set to "true" if the the browser should include any cookies associated to the domain
+					Set to "true" if the browser should include any cookies associated to the domain
 					of the request being annotated, or "false" if it should not. Empty string "" means undefined.
 					If true, the pre-flight response will include the header
 					'Access-Control-Allow-Credentials=true'. Default value is "true".
@@ -826,7 +826,7 @@
 			<xsd:annotation>
 				<xsd:documentation>
 					Provide a reference to a list of HttpMessageConverter instances. If specified,
-					these converters will replace all of the default converters that would normally
+					these converters will replace all default converters that would normally
 					be present on the underlying RestTemplate.
 				</xsd:documentation>
 			</xsd:annotation>
@@ -852,10 +852,11 @@
 					]]></xsd:documentation>
 			</xsd:annotation>
 		</xsd:attribute>
-		<xsd:attribute name="encoding-mode" default="TEMPLATE_AND_VALUES">
+		<xsd:attribute name="encoding-mode">
 			<xsd:annotation>
 				<xsd:documentation>
 					Set the encoding mode during URI building.
+					Defaults to 'TEMPLATE_AND_VALUES'
 				</xsd:documentation>
 			</xsd:annotation>
 			<xsd:simpleType>
@@ -948,7 +949,7 @@
 		<xsd:attribute name="uri-variables-expression" type="xsd:string">
 			<xsd:annotation>
 				<xsd:documentation><![CDATA[
-	Specifies the SpEL expression to be evaluate as a Map for URI variable placeholders within 'url'.
+	Specifies the SpEL expression to be evaluated as a Map for URI variable placeholders within 'url'.
 	This attribute is mutually exclusive with 'uri-variable' sub-elements.
 					]]></xsd:documentation>
 			</xsd:annotation>

spring-integration-http/src/test/java/org/springframework/integration/http/config/OutboundResponseTypeTests-context-encoding-mode-fail.xml

@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans xmlns="http://www.springframework.org/schema/beans"
+	   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	   xmlns:int-http="http://www.springframework.org/schema/integration/http"
+	   xsi:schemaLocation="http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd
+		http://www.springframework.org/schema/integration/http https://www.springframework.org/schema/integration/http/spring-integration-http.xsd">
+
+	<bean id="restTemplate" class="org.mockito.Mockito" factory-method="mock">
+		<constructor-arg value="org.springframework.web.client.RestTemplate"/>
+	</bean>
+
+	<int-http:outbound-gateway url="/fake"
+							   rest-template="restTemplate"
+							   encoding-mode="NONE"/>
+
+</beans>

spring-integration-http/src/test/java/org/springframework/integration/http/config/OutboundResponseTypeTests-context.xml

@@ -3,15 +3,12 @@
 	   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 	   xmlns:int="http://www.springframework.org/schema/integration"
 	   xmlns:int-http="http://www.springframework.org/schema/integration/http"
-	   xmlns:util="http://www.springframework.org/schema/util"
 	   xsi:schemaLocation="http://www.springframework.org/schema/integration/http
 	    https://www.springframework.org/schema/integration/http/spring-integration-http.xsd
 		http://www.springframework.org/schema/integration
 		https://www.springframework.org/schema/integration/spring-integration.xsd
 		http://www.springframework.org/schema/beans
-		https://www.springframework.org/schema/beans/spring-beans.xsd
-		http://www.springframework.org/schema/util
-		https://www.springframework.org/schema/util/spring-util.xsd">
+		https://www.springframework.org/schema/beans/spring-beans.xsd">
 
 	<bean id="restTemplate" class="org.springframework.web.client.RestTemplate"/>
 

spring-integration-http/src/test/java/org/springframework/integration/http/config/OutboundResponseTypeTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2022 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,20 +17,17 @@
 package org.springframework.integration.http.config;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.fail;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.springframework.test.web.client.match.MockRestRequestMatchers.header;
 import static org.springframework.test.web.client.match.MockRestRequestMatchers.method;
 import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestTo;
 import static org.springframework.test.web.client.response.MockRestResponseCreators.withSuccess;
 
 import java.util.Collections;
-import java.util.Map;
 
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 
-import org.springframework.beans.BeansException;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.parsing.BeanDefinitionParsingException;
 import org.springframework.context.support.ClassPathXmlApplicationContext;
@@ -44,8 +41,7 @@ import org.springframework.messaging.MessageChannel;
 import org.springframework.messaging.MessageHandlingException;
 import org.springframework.messaging.support.GenericMessage;
 import org.springframework.test.annotation.DirtiesContext;
-import org.springframework.test.context.ContextConfiguration;
-import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
 import org.springframework.test.web.client.MockRestServiceServer;
 import org.springframework.web.client.RestTemplate;
 
@@ -53,13 +49,10 @@ import org.springframework.web.client.RestTemplate;
  * @author Oleg Zhurakousky
  * @author Artem Bilan
  * @author Gary Russell
- * @since 2.2
  *
- * <p>
- * see https://jira.springsource.org/browse/INT-2397
+ * @since 2.2
  */
-@ContextConfiguration
-@RunWith(SpringJUnit4ClassRunner.class)
+@SpringJUnitConfig
 @DirtiesContext
 public class OutboundResponseTypeTests {
 
@@ -95,18 +88,18 @@ public class OutboundResponseTypeTests {
 
 	private MockRestServiceServer mockServer;
 
-	@Before
+	@BeforeEach
 	public void setup() {
 		this.mockServer = MockRestServiceServer.createServer(this.restTemplate);
 	}
 
 	@Test
-	public void testDefaultResponseType() throws Exception {
+	public void testDefaultResponseType() {
 		this.mockServer.expect(requestTo("/testApps/outboundResponse"))
 				.andExpect(method(HttpMethod.POST))
 				.andRespond(withSuccess(HttpMethod.POST.name(), MediaType.TEXT_PLAIN));
 
-		this.requestChannel.send(new GenericMessage<String>("Hello"));
+		this.requestChannel.send(new GenericMessage<>("Hello"));
 		Message<?> message = this.replyChannel.receive(5000);
 		assertThat(message).isNotNull();
 		assertThat(message.getPayload() instanceof ResponseEntity).isTrue();
@@ -115,12 +108,12 @@ public class OutboundResponseTypeTests {
 	}
 
 	@Test
-	public void testWithResponseTypeSet() throws Exception {
+	public void testWithResponseTypeSet() {
 		this.mockServer.expect(requestTo("/testApps/outboundResponse"))
 				.andExpect(method(HttpMethod.POST))
 				.andRespond(withSuccess(HttpMethod.POST.name(), MediaType.TEXT_PLAIN));
 
-		this.resTypeSetChannel.send(new GenericMessage<String>("Hello"));
+		this.resTypeSetChannel.send(new GenericMessage<>("Hello"));
 		Message<?> message = this.replyChannel.receive(5000);
 		assertThat(message).isNotNull();
 		assertThat(message.getPayload() instanceof String).isTrue();
@@ -129,12 +122,12 @@ public class OutboundResponseTypeTests {
 	}
 
 	@Test
-	public void testWithResponseTypeExpressionSet() throws Exception {
+	public void testWithResponseTypeExpressionSet() {
 		this.mockServer.expect(requestTo("/testApps/outboundResponse"))
 				.andExpect(method(HttpMethod.POST))
 				.andRespond(withSuccess(HttpMethod.POST.name(), MediaType.TEXT_PLAIN));
 
-		this.resTypeExpressionSetChannel.send(new GenericMessage<String>("java.lang.String"));
+		this.resTypeExpressionSetChannel.send(new GenericMessage<>("java.lang.String"));
 		Message<?> message = this.replyChannel.receive(5000);
 		assertThat(message).isNotNull();
 		assertThat(message.getPayload() instanceof String).isTrue();
@@ -143,7 +136,7 @@ public class OutboundResponseTypeTests {
 	}
 
 	@Test
-	public void testWithResponseTypeExpressionSetAsClass() throws Exception {
+	public void testWithResponseTypeExpressionSetAsClass() {
 		this.mockServer = MockRestServiceServer.createServer(this.restTemplateWithConverters);
 		this.mockServer.expect(requestTo("/testApps/outboundResponse"))
 				.andExpect(method(HttpMethod.POST))
@@ -158,12 +151,12 @@ public class OutboundResponseTypeTests {
 	}
 
 	@Test
-	public void testInt2706ResponseTypeExpressionAsPrimitive() throws Exception {
+	public void testInt2706ResponseTypeExpressionAsPrimitive() {
 		this.mockServer.expect(requestTo("/testApps/outboundResponse"))
 				.andExpect(method(HttpMethod.POST))
 				.andRespond(withSuccess(HttpMethod.POST.name(), MediaType.TEXT_PLAIN));
 
-		this.resTypeExpressionSetChannel.send(new GenericMessage<String>("byte[]"));
+		this.resTypeExpressionSetChannel.send(new GenericMessage<>("byte[]"));
 		Message<?> message = this.replyChannel.receive(5000);
 		assertThat(message).isNotNull();
 		assertThat(message.getPayload() instanceof byte[]).isTrue();
@@ -172,12 +165,12 @@ public class OutboundResponseTypeTests {
 	}
 
 	@Test
-	public void testInt2706ResponseTypePrimitiveArrayClassAsString() throws Exception {
+	public void testInt2706ResponseTypePrimitiveArrayClassAsString() {
 		this.mockServer.expect(requestTo("/testApps/outboundResponse"))
 				.andExpect(method(HttpMethod.POST))
 				.andRespond(withSuccess(HttpMethod.POST.name(), MediaType.TEXT_PLAIN));
 
-		this.resPrimitiveStringPresentationChannel.send(new GenericMessage<byte[]>("hello".getBytes()));
+		this.resPrimitiveStringPresentationChannel.send(new GenericMessage<>("hello".getBytes()));
 		Message<?> message = this.replyChannel.receive(5000);
 		assertThat(message).isNotNull();
 		assertThat(message.getPayload() instanceof byte[]).isTrue();
@@ -186,44 +179,43 @@ public class OutboundResponseTypeTests {
 	}
 
 	@Test
-	public void testInt3052InvalidResponseType() throws Exception {
-		try {
-			this.invalidResponseTypeChannel.send(new GenericMessage<byte[]>("hello".getBytes()));
-			fail("IllegalStateException expected.");
-		}
-		catch (Exception e) {
-			assertThat(e).isInstanceOf(MessageHandlingException.class);
-			Throwable t = e.getCause();
-			assertThat(t).isInstanceOf(IllegalStateException.class);
-			assertThat(t.getMessage()).contains("'expectedResponseType' can be an instance of " +
-					"'Class<?>', 'String' or 'ParameterizedTypeReference<?>'");
-		}
+	public void testInt3052InvalidResponseType() {
+		assertThatExceptionOfType(MessageHandlingException.class)
+				.isThrownBy(() -> this.invalidResponseTypeChannel.send(new GenericMessage<>("hello".getBytes())))
+				.withCauseInstanceOf(IllegalStateException.class)
+				.withStackTraceContaining("'expectedResponseType' can be an instance of " +
+						"'Class<?>', 'String' or 'ParameterizedTypeReference<?>'");
 	}
 
 	@Test
-	public void testMutuallyExclusivityInMethodAndMethodExpression() throws Exception {
-		try {
-			new ClassPathXmlApplicationContext("OutboundResponseTypeTests-context-fail.xml", this.getClass()).close();
-			fail("Expected BeansException");
-		}
-		catch (BeansException e) {
-			assertThat(e instanceof BeanDefinitionParsingException).isTrue();
-			assertThat(e.getMessage().contains("The 'expected-response-type' " +
-					"and 'expected-response-type-expression' are mutually exclusive")).isTrue();
-		}
+	public void testMutuallyExclusivityInMethodAndMethodExpression() {
+		assertThatExceptionOfType(BeanDefinitionParsingException.class)
+				.isThrownBy(() -> new ClassPathXmlApplicationContext("OutboundResponseTypeTests-context-fail.xml",
+						getClass()))
+				.withMessageContaining("The 'expected-response-type' " +
+						"and 'expected-response-type-expression' are mutually exclusive");
 	}
 
 	@Test
-	public void testContentTypePropagation() throws Exception {
+	public void testContentTypePropagation() {
 		this.mockServer.expect(requestTo("/testApps/outboundResponse"))
 				.andExpect(method(HttpMethod.POST))
 				.andExpect(header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON.toString()))
 				.andRespond(withSuccess(HttpMethod.POST.name(), MediaType.TEXT_PLAIN));
 
 		this.contentTypePropagationChannel
-				.send(new GenericMessage<Map<String, String>>(Collections.singletonMap("foo", "bar")));
+				.send(new GenericMessage<>(Collections.singletonMap("foo", "bar")));
 
 		this.mockServer.verify();
 	}
 
+	@Test
+	public void notAllowedEncodingModeWhenExternalRestTemplate() {
+		assertThatExceptionOfType(BeanDefinitionParsingException.class)
+				.isThrownBy(() -> new ClassPathXmlApplicationContext(
+						"OutboundResponseTypeTests-context-encoding-mode-fail.xml", getClass()))
+				.withMessageContaining("When providing a 'rest-template' reference, " +
+						"the 'encoding-mode' must be set on the 'RestTemplate.uriTemplateHandler' property.");
+	}
+
 }