Skip to content

Commit

Permalink
INT-3698: HTTP MH: Remove Internal Converters
Browse files Browse the repository at this point in the history 
JIRA: https://jira.spring.io/browse/INT-3698

The `HttpRequestExecutingMessageHandler` added two internal `Converters` to the `ConversionService` before.
In case of parent-child environment it causes memory leak: closing of child context doesn't release
`HttpRequestExecutingMessageHandler` instances, because those internal `Converters` aren't `static`, hence bounded to outer instance.

Actually after introduction since `4.0` version to the `HttpRequestExecutingMessageHandler` the code like:

```
Assert.isTrue(expectedResponseType instanceof Class<?>
					|| expectedResponseType instanceof String
					|| expectedResponseType instanceof ParameterizedTypeReference,
					"'expectedResponseType' can be an instance of 'Class<?>', 'String' or 'ParameterizedTypeReference<?>'.");
if (expectedResponseType instanceof String && StringUtils.hasText((String) expectedResponseType)){
				expectedResponseType = ClassUtils.forName((String) expectedResponseType, ClassUtils.getDefaultClassLoader());
}
```

These converters are redundant.

In addition remove the `expected type` for the `uriVariablesExpression` evaluation to avoid `MapToMapConverter`.

**Cherry-pick to 4.1.x, 4.0.x**

INT-3698: Provide robust logic for the `uriExpression` and `httpMethodExpression`

Make some polishing around `Assert`s, when `IllegalStateException` must be presented instead of `IllegalArgumentException`
for the expression evaluation results.

Polishing
  • Loading branch information
@artembilan @garyrussell
artembilan authored and garyrussell committed Apr 16, 2015
1 parent 73522d2 commit 2aac441
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 123 deletions.
102 changes: 39 additions & 63 deletions ...ava/org/springframework/integration/http/outbound/HttpRequestExecutingMessageHandler.java
View file
Expand Up @@ -30,14 +30,9 @@

import org.springframework.beans.factory.BeanFactory;
import org.springframework.core.ParameterizedTypeReference;
import org.springframework.core.convert.ConversionService;
import org.springframework.core.convert.converter.Converter;
import org.springframework.core.convert.converter.ConverterRegistry;
import org.springframework.core.convert.support.GenericConversionService;
import org.springframework.expression.Expression;
import org.springframework.expression.common.LiteralExpression;
import org.springframework.expression.spel.support.StandardEvaluationContext;
import org.springframework.expression.spel.support.StandardTypeConverter;
import org.springframework.http.HttpEntity;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpMethod;
Expand All @@ -48,6 +43,7 @@
import org.springframework.http.converter.HttpMessageConverter;
import org.springframework.integration.expression.ExpressionEvalMap;
import org.springframework.integration.expression.ExpressionUtils;
import org.springframework.integration.expression.ValueExpression;
import org.springframework.integration.handler.AbstractReplyProducingMessageHandler;
import org.springframework.integration.http.support.DefaultHttpHeaderMapper;
import org.springframework.integration.mapping.HeaderMapper;
Expand Down Expand Up @@ -97,7 +93,7 @@ public class HttpRequestExecutingMessageHandler extends AbstractReplyProducingMe

private volatile boolean encodeUri = true;

private volatile Expression httpMethodExpression = new LiteralExpression(HttpMethod.POST.name());
private volatile Expression httpMethodExpression = new ValueExpression<HttpMethod>(HttpMethod.POST);

private volatile boolean expectReply = true;

Expand All @@ -123,7 +119,7 @@ public class HttpRequestExecutingMessageHandler extends AbstractReplyProducingMe
* @param uri The URI.
*/
public HttpRequestExecutingMessageHandler(URI uri) {
this(uri.toString());
this(new ValueExpression<URI>(uri));
}

/**
Expand Down Expand Up @@ -199,7 +195,8 @@ public void setHttpMethodExpression(Expression httpMethodExpression) {
* @param httpMethod The method.
*/
public void setHttpMethod(HttpMethod httpMethod) {
this.httpMethodExpression = new LiteralExpression(httpMethod.name());
Assert.notNull(httpMethod, "'httpMethod' must not be null");
this.httpMethodExpression = new ValueExpression<HttpMethod>(httpMethod);
}

/**
Expand Down Expand Up @@ -351,53 +348,14 @@ public String getComponentType() {
@Override
protected void doInit() {
this.evaluationContext = ExpressionUtils.createStandardEvaluationContext(this.getBeanFactory());

ConversionService conversionService = this.getConversionService();
if (conversionService == null){
conversionService = new GenericConversionService();
}
if (conversionService instanceof ConverterRegistry){
ConverterRegistry converterRegistry =
(ConverterRegistry) conversionService;

converterRegistry.addConverter(new ClassToStringConverter());
converterRegistry.addConverter(new ObjectToStringConverter());

this.evaluationContext.setTypeConverter(new StandardTypeConverter(conversionService));
}
else {
logger.warn("ConversionService is not an instance of ConverterRegistry therefore" +
"ClassToStringConverter and ObjectToStringConverter will not be registered");
}
}

private class ClassToStringConverter implements Converter<Class<?>, String> {
@Override
public String convert(Class<?> source) {
return source.getName();
}
}

/**
* Spring 3.0.7.RELEASE unfortunately does not trigger the ClassToStringConverter.
* Therefore, this converter will also test for Class instances and do a
* respective type conversion.
*
*/
private class ObjectToStringConverter implements Converter<Object, String> {
@Override
public String convert(Object source) {
if (source instanceof Class) {
return ((Class<?>) source).getName();
}
return source.toString();
}
}

@Override
protected Object handleRequestMessage(Message<?> requestMessage) {
String uri = this.uriExpression.getValue(this.evaluationContext, requestMessage, String.class);
Assert.notNull(uri, "URI Expression evaluation cannot result in null");
Object uri = this.uriExpression.getValue(this.evaluationContext, requestMessage);
Assert.state(uri instanceof String || uri instanceof URI,
"'uriExpression' evaluation must result in a 'String' or 'URI' instance, not: "
+ (uri == null ? "null" : uri.getClass()));
URI realUri = null;
try {
HttpMethod httpMethod = this.determineHttpMethod(requestMessage);
Expand All @@ -413,7 +371,10 @@ protected Object handleRequestMessage(Message<?> requestMessage) {

HttpEntity<?> httpRequest = this.generateHttpRequest(requestMessage, httpMethod);
Map<String, ?> uriVariables = this.determineUriVariables(requestMessage);
UriComponents uriComponents = UriComponentsBuilder.fromUriString(uri).buildAndExpand(uriVariables);
UriComponentsBuilder uriComponentsBuilder = uri instanceof String
? UriComponentsBuilder.fromUriString((String) uri)
: UriComponentsBuilder.fromUri((URI) uri);
UriComponents uriComponents = uriComponentsBuilder.buildAndExpand(uriVariables);
realUri = this.encodeUri ? uriComponents.toUri() : new URI(uriComponents.toUriString());
ResponseEntity<?> httpResponse;
if (expectedResponseType instanceof ParameterizedTypeReference<?>) {
Expand Down Expand Up @@ -448,8 +409,7 @@ protected Object handleRequestMessage(Message<?> requestMessage) {
}
catch (Exception e) {
throw new MessageHandlingException(requestMessage, "HTTP request execution failed for URI ["
+ (realUri == null ? uri : realUri.toString())
+ "]", e);
+ (realUri == null ? uri : realUri) + "]", e);
}
}

Expand Down Expand Up @@ -612,10 +572,22 @@ private boolean isFormData(Map<Object, ?> map) {
}

private HttpMethod determineHttpMethod(Message<?> requestMessage) {
String strHttpMethod = httpMethodExpression.getValue(this.evaluationContext, requestMessage, String.class);
Assert.isTrue(StringUtils.hasText(strHttpMethod) && !Arrays.asList(HttpMethod.values()).contains(strHttpMethod),
"The 'httpMethodExpression' returned an invalid HTTP Method value: " + strHttpMethod);
return HttpMethod.valueOf(strHttpMethod);
Object httpMethod = this.httpMethodExpression.getValue(this.evaluationContext, requestMessage);
Assert.state(httpMethod != null && (httpMethod instanceof String || httpMethod instanceof HttpMethod),
"'httpMethodExpression' evaluation must result in an 'HttpMethod' enum or its String representation, " +
"not: " + (httpMethod == null ? "null" : httpMethod.getClass()));
if (httpMethod instanceof HttpMethod) {
return (HttpMethod) httpMethod;
}
else {
try {
return HttpMethod.valueOf((String) httpMethod);
}
catch (Exception e) {
throw new IllegalStateException("The 'httpMethodExpression' returned an invalid HTTP Method value: "
+ httpMethod);
}
}
}

private Object determineExpectedResponseType(Message<?> requestMessage) throws Exception{
Expand All @@ -624,10 +596,11 @@ private Object determineExpectedResponseType(Message<?> requestMessage) throws E
expectedResponseType = this.expectedResponseTypeExpression.getValue(this.evaluationContext, requestMessage);
}
if (expectedResponseType != null) {
Assert.isTrue(expectedResponseType instanceof Class<?>
|| expectedResponseType instanceof String
|| expectedResponseType instanceof ParameterizedTypeReference,
"'expectedResponseType' can be an instance of 'Class<?>', 'String' or 'ParameterizedTypeReference<?>'.");
Assert.state(expectedResponseType instanceof Class<?>
|| expectedResponseType instanceof String
|| expectedResponseType instanceof ParameterizedTypeReference,
"'expectedResponseType' can be an instance of 'Class<?>', 'String' or 'ParameterizedTypeReference<?>'; "
+ "evaluation resulted in a" + expectedResponseType.getClass() + ".");
if (expectedResponseType instanceof String && StringUtils.hasText((String) expectedResponseType)){
expectedResponseType = ClassUtils.forName((String) expectedResponseType, ClassUtils.getDefaultClassLoader());
}
Expand All @@ -640,7 +613,10 @@ private Object determineExpectedResponseType(Message<?> requestMessage) throws E
Map<String, ?> expressions;

if (this.uriVariablesExpression != null) {
expressions = this.uriVariablesExpression.getValue(this.evaluationContext, requestMessage, Map.class);
Object expressionsObject = this.uriVariablesExpression.getValue(this.evaluationContext, requestMessage);
Assert.state(expressionsObject instanceof Map,
"The 'uriVariablesExpression' evaluation must result in a 'Map'.");
expressions = (Map<String, ?>) expressionsObject;
}
else {
expressions = this.uriVariableExpressions;
Expand Down
7 changes: 4 additions & 3 deletions .../src/test/java/org/springframework/integration/http/config/OutboundResponseTypeTests.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2014 the original author or authors.
* Copyright 2002-2015 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.
Expand All @@ -13,6 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.integration.http.config;

import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -161,12 +162,12 @@ public void testInt2706ResponseTypePrimitiveArrayClassAsString() throws Exceptio
public void testInt3052InvalidResponseType() throws Exception {
try {
this.invalidResponseTypeChannel.send(new GenericMessage<byte[]>("hello".getBytes()));
fail("IllegalArgumentException expected.");
fail("IllegalStateException expected.");
}
catch (Exception e) {
assertThat(e, Matchers.instanceOf(MessageHandlingException.class));
Throwable t = e.getCause();
assertThat(t, Matchers.instanceOf(IllegalArgumentException.class));
assertThat(t, Matchers.instanceOf(IllegalStateException.class));
assertThat(t.getMessage(), Matchers.containsString("'expectedResponseType' can be an instance of " +
"'Class<?>', 'String' or 'ParameterizedTypeReference<?>'"));
}
Expand Down
68 changes: 13 additions & 55 deletions ...rg/springframework/integration/http/outbound/HttpRequestExecutingMessageHandlerTests.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2013 the original author or authors.
* Copyright 2002-2015 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.
Expand All @@ -13,14 +13,13 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.integration.http.outbound;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand All @@ -35,26 +34,18 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

import javax.xml.transform.Source;

import org.aopalliance.intercept.MethodInterceptor;
import org.aopalliance.intercept.MethodInvocation;
import org.junit.Test;
import org.mockito.Mockito;

import org.springframework.aop.framework.ProxyFactory;
import org.springframework.beans.DirectFieldAccessor;
import org.springframework.beans.factory.BeanFactory;
import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
import org.springframework.beans.factory.support.DefaultListableBeanFactory;
import org.springframework.context.ApplicationContext;
import org.springframework.context.support.ClassPathXmlApplicationContext;
import org.springframework.core.ParameterizedTypeReference;
import org.springframework.core.convert.ConversionService;
import org.springframework.core.convert.converter.ConverterRegistry;
import org.springframework.expression.spel.standard.SpelExpressionParser;
import org.springframework.http.HttpEntity;
import org.springframework.http.HttpHeaders;
Expand Down Expand Up @@ -736,7 +727,7 @@ public void testHttpOutboundGatewayWithinChain() throws IOException, URISyntaxEx
.exchange(Mockito.eq(new URI("http://localhost:51235/%2f/testApps?param=http+Outbound+Gateway+Within+Chain")),
Mockito.eq(HttpMethod.POST), Mockito.any(HttpEntity.class), Mockito.eq(new ParameterizedTypeReference<List<String>>() {

}));
}));
}

@Test
Expand Down Expand Up @@ -773,47 +764,6 @@ public void testInt2455UriNotEncoded() {
assertEquals("http://my.RabbitMQ.com/api/queues/%2f/si.test.queue?foo#bar", restTemplate.actualUrl.get());
}

@Test
public void nonCompatibleConversionService() throws Exception {
HttpRequestExecutingMessageHandler handler =
new HttpRequestExecutingMessageHandler("http://www.springsource.org/spring-integration");
ConfigurableListableBeanFactory bf = new DefaultListableBeanFactory();
ConversionService mockConversionService = mock(ConversionService.class);
bf.registerSingleton("integrationConversionService", mockConversionService);
handler.setBeanFactory(bf);
try {
handler.afterPropertiesSet();
}
catch (Exception e) {
fail("Unexpected exception during initialization " + e.getMessage());
}
assertSame(mockConversionService, TestUtils.getPropertyValue(handler, "conversionService"));
}

@Test
public void compatibleConversionService() throws Exception {
HttpRequestExecutingMessageHandler handler =
new HttpRequestExecutingMessageHandler("http://www.springsource.org/spring-integration");
ConfigurableListableBeanFactory bf = new DefaultListableBeanFactory();
ProxyFactory pf = new ProxyFactory(new Class<?>[] {ConversionService.class, ConverterRegistry.class});
final AtomicInteger converterCount = new AtomicInteger();
pf.addAdvice(new MethodInterceptor() {

public Object invoke(MethodInvocation invocation) throws Throwable {
if (invocation.getMethod().getName().equals("addConverter")) {
converterCount.incrementAndGet();
}
return null;
}
});
ConversionService mockConversionService = (ConversionService) pf.getProxy();
bf.registerSingleton("integrationConversionService", mockConversionService);
handler.setBeanFactory(bf);
handler.afterPropertiesSet();
assertEquals(2, converterCount.get());
assertSame(mockConversionService, TestUtils.getPropertyValue(handler, "conversionService"));
}

@Test
public void acceptHeaderForSerializableResponse() throws Exception {
HttpRequestExecutingMessageHandler handler = new HttpRequestExecutingMessageHandler("http://www.springsource.org/spring-integration");
Expand Down Expand Up @@ -895,7 +845,7 @@ private HttpHeaders setUpMocksToCaptureSentHeaders(RestTemplate restTemplate) th
when(clientRequest.getHeaders()).thenReturn(headers);

when(requestFactory.createRequest(any(URI.class), any(HttpMethod.class)))
.thenReturn(clientRequest );
.thenReturn(clientRequest);

ClientHttpResponse response = mock(ClientHttpResponse.class);
when(response.getStatusCode()).thenReturn(HttpStatus.NOT_FOUND);
Expand All @@ -912,20 +862,25 @@ private HttpHeaders setUpMocksToCaptureSentHeaders(RestTemplate restTemplate) th
return headers;
}

public static class City{
public static class City {

private final String name;

public City(String name){
this.name = name;
}

@Override
public String toString(){
return name;
}

}

private static class MockRestTemplate extends RestTemplate {

private final AtomicReference<HttpEntity<?>> lastRequestEntity = new AtomicReference<HttpEntity<?>>();

private final AtomicReference<String> actualUrl = new AtomicReference<String>();

@Override
Expand All @@ -935,6 +890,7 @@ public <T> ResponseEntity<T> exchange(URI uri, HttpMethod method, HttpEntity<?>
this.lastRequestEntity.set(requestEntity);
throw new RuntimeException("intentional");
}

}

@SuppressWarnings("unused")
Expand All @@ -951,11 +907,13 @@ public <T> ResponseEntity<T> exchange(URI url, HttpMethod method, HttpEntity<?>
ParameterizedTypeReference<T> responseType) throws RestClientException {
return new ResponseEntity<T>(HttpStatus.OK);
}

}

private static class Foo implements Serializable {

private static final long serialVersionUID = 1L;

}

}

0 comments on commit 2aac441

Please sign in to comment.