root
/
spring-framework
mirror of https://github.com/spring-projects/spring-framework.git
1
0
Fork 0
Code Issues Actions 6 Packages Projects Releases Wiki Activity

Forbid null converters in RestTemplate & HttpMessageConverterExtractor 

Prior to this commit, RestTemplate and HttpMessageConverterExtractor did
not validate that the supplied HttpMessageConverter list contained no
null elements, which can lead to a NullPointerException when the
converters are accessed.

This commit improves the user experience by failing immediately if the
supplied HttpMessageConverter list contains a null element. This applies
to constructors for RestTemplate and HttpMessageConverterExtractor as
well as to RestTemplate#setMessageConverters().

Note, however, that RestTemplate#getMessageConverters() returns a mutable
list. Thus, if a user modifies that list so that it contains null values,
that will still lead to a NullPointerException when the converters are
accessed.

This commit also introduces noNullElements() variants for collections in
org.springframework.util.Assert.

Closes gh-23151
This commit is contained in:
Sam Brannen 2019-06-18 15:56:50 +03:00
parent 8ceac9c015
commit 4000b244ff
5 changed files with 102 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
spring-core/src/main/java/org/springframework/util/Assert.java
View File

@ -495,6 +495,47 @@ public abstract class Assert {
"[Assertion failed] - this collection must not be empty: it must contain at least 1 element");
}
/**
* Assert that a collection contains no {@code null} elements.
* <p>Note: Does not complain if the collection is empty!
* <pre class="code">Assert.noNullElements(collection, "Collection must contain non-null elements");</pre>
* @param collection the collection to check
* @param message the exception message to use if the assertion fails
* @throws IllegalArgumentException if the collection contains a {@code null} element
* @since 5.2
*/
public static void noNullElements(@Nullable Collection<?> collection, String message) {
if (collection != null) {
for (Object element : collection) {
if (element == null) {
throw new IllegalArgumentException(message);
}
}
}
}
/**
* Assert that a collection contains no {@code null} elements.
* <p>Note: Does not complain if the collection is empty!
* <pre class="code">
* Assert.noNullElements(collection, () -&gt; "Collection " + collectionName + " must contain non-null elements");
* </pre>
* @param collection the collection to check
* @param messageSupplier a supplier for the exception message to use if the
* assertion fails
* @throws IllegalArgumentException if the collection contains a {@code null} element
* @since 5.2
*/
public static void noNullElements(@Nullable Collection<?> collection, Supplier<String> messageSupplier) {
if (collection != null) {
for (Object element : collection) {
if (element == null) {
throw new IllegalArgumentException(nullSafeGet(messageSupplier));
}
}
}
}
/**
* Assert that a Map contains entries; that is, it must not be {@code null}
* and must contain at least one entry.

4
spring-web/src/main/java/org/springframework/web/client/HttpMessageConverterExtractor.java
View File

@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2019 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.
@ -37,6 +37,7 @@ import org.springframework.util.Assert;
* to convert the response into a type {@code T}.
*
* @author Arjen Poutsma
* @author Sam Brannen
* @since 3.0
* @param <T> the data type
* @see RestTemplate
@ -73,6 +74,7 @@ public class HttpMessageConverterExtractor<T> implements ResponseExtractor<T> {
HttpMessageConverterExtractor(Type responseType, List<HttpMessageConverter<?>> messageConverters, Log logger) {
Assert.notNull(responseType, "'responseType' must not be null");
Assert.notEmpty(messageConverters, "'messageConverters' must not be empty");
Assert.noNullElements(messageConverters, "'messageConverters' must not contain null elements");
this.responseType = responseType;
this.responseClass = (responseType instanceof Class ? (Class<T>) responseType : null);
this.messageConverters = messageConverters;

11
spring-web/src/main/java/org/springframework/web/client/RestTemplate.java
View File

@ -84,6 +84,7 @@ import org.springframework.web.util.UriTemplateHandler;
* @author Brian Clozel
* @author Roy Clarkson
* @author Juergen Hoeller
* @author Sam Brannen
* @since 3.0
* @see HttpMessageConverter
* @see RequestCallback
@ -198,11 +199,12 @@ public class RestTemplate extends InterceptingHttpAccessor implements RestOperat
* @since 3.2.7
*/
public RestTemplate(List<HttpMessageConverter<?>> messageConverters) {
Assert.notEmpty(messageConverters, "At least one HttpMessageConverter required");
validateConverters(messageConverters);
this.messageConverters.addAll(messageConverters);
this.uriTemplateHandler = initUriTemplateHandler();
}
private static DefaultUriBuilderFactory initUriTemplateHandler() {
DefaultUriBuilderFactory uriFactory = new DefaultUriBuilderFactory();
uriFactory.setEncodingMode(EncodingMode.URI_COMPONENT); // for backwards compatibility..
@ -215,7 +217,7 @@ public class RestTemplate extends InterceptingHttpAccessor implements RestOperat
* <p>These converters are used to convert from and to HTTP requests and responses.
*/
public void setMessageConverters(List<HttpMessageConverter<?>> messageConverters) {
Assert.notEmpty(messageConverters, "At least one HttpMessageConverter required");
validateConverters(messageConverters);
// Take getMessageConverters() List as-is when passed in here
if (this.messageConverters != messageConverters) {
this.messageConverters.clear();
@ -223,6 +225,11 @@ public class RestTemplate extends InterceptingHttpAccessor implements RestOperat
}
}
private void validateConverters(List<HttpMessageConverter<?>> messageConverters) {
Assert.notEmpty(messageConverters, "At least one HttpMessageConverter is required");
Assert.noNullElements(messageConverters, "The HttpMessageConverter list must not contain null elements");
}
/**
* Return the list of message body converters.
* <p>The returned {@link List} is active and may get appended to.

84
spring-web/src/test/java/org/springframework/web/client/HttpMessageConverterExtractorTests.java
View File

@ -19,7 +19,7 @@ package org.springframework.web.client;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import org.junit.Test;
@ -34,8 +34,10 @@ import org.springframework.http.converter.GenericHttpMessageConverter;
import org.springframework.http.converter.HttpMessageConverter;
import org.springframework.http.converter.HttpMessageNotReadableException;
import static java.util.Arrays.asList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;
@ -46,18 +48,30 @@ import static org.mockito.Mockito.mock;
*
* @author Arjen Poutsma
* @author Brian Clozel
* @author Sam Brannen
*/
public class HttpMessageConverterExtractorTests {
private HttpMessageConverterExtractor<?> extractor;
@SuppressWarnings("unchecked")
private final HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
private final HttpMessageConverterExtractor<?> extractor = new HttpMessageConverterExtractor<>(String.class, asList(converter));
private final MediaType contentType = MediaType.TEXT_PLAIN;
private final HttpHeaders responseHeaders = new HttpHeaders();
private final ClientHttpResponse response = mock(ClientHttpResponse.class);
@Test
public void constructorPreconditions() {
assertThatIllegalArgumentException()
.isThrownBy(() -> new HttpMessageConverterExtractor<>(String.class, (List<HttpMessageConverter<?>>) null))
.withMessage("'messageConverters' must not be empty");
assertThatIllegalArgumentException()
.isThrownBy(() -> new HttpMessageConverterExtractor<>(String.class, Arrays.asList(null, this.converter)))
.withMessage("'messageConverters' must not contain null elements");
}
@Test
public void noContent() throws IOException {
HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.NO_CONTENT.value());
Object result = extractor.extractData(response);
@ -66,8 +80,6 @@ public class HttpMessageConverterExtractorTests {
@Test
public void notModified() throws IOException {
HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.NOT_MODIFIED.value());
Object result = extractor.extractData(response);
@ -76,8 +88,6 @@ public class HttpMessageConverterExtractorTests {
@Test
public void informational() throws IOException {
HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.CONTINUE.value());
Object result = extractor.extractData(response);
@ -86,10 +96,7 @@ public class HttpMessageConverterExtractorTests {
@Test
public void zeroContentLength() throws IOException {
HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
HttpHeaders responseHeaders = new HttpHeaders();
responseHeaders.setContentLength(0);
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.OK.value());
given(response.getHeaders()).willReturn(responseHeaders);
@ -98,11 +105,7 @@ public class HttpMessageConverterExtractorTests {
}
@Test
@SuppressWarnings("unchecked")
public void emptyMessageBody() throws IOException {
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
HttpHeaders responseHeaders = new HttpHeaders();
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.OK.value());
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream("".getBytes()));
@ -112,11 +115,7 @@ public class HttpMessageConverterExtractorTests {
}
@Test // gh-22265
@SuppressWarnings("unchecked")
public void nullMessageBody() throws IOException {
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
HttpHeaders responseHeaders = new HttpHeaders();
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.OK.value());
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(null);
@ -126,14 +125,9 @@ public class HttpMessageConverterExtractorTests {
}
@Test
@SuppressWarnings("unchecked")
public void normal() throws IOException {
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
HttpHeaders responseHeaders = new HttpHeaders();
MediaType contentType = MediaType.TEXT_PLAIN;
responseHeaders.setContentType(contentType);
String expected = "Foo";
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.OK.value());
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
@ -145,32 +139,26 @@ public class HttpMessageConverterExtractorTests {
}
@Test
@SuppressWarnings("unchecked")
public void cannotRead() throws IOException {
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
HttpHeaders responseHeaders = new HttpHeaders();
MediaType contentType = MediaType.TEXT_PLAIN;
responseHeaders.setContentType(contentType);
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.OK.value());
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream("Foobar".getBytes()));
given(converter.canRead(String.class, contentType)).willReturn(false);
assertThatExceptionOfType(RestClientException.class).isThrownBy(() ->
extractor.extractData(response));
assertThatExceptionOfType(RestClientException.class).isThrownBy(() -> extractor.extractData(response));
}
@Test
@SuppressWarnings("unchecked")
public void generics() throws IOException {
GenericHttpMessageConverter<String> converter = mock(GenericHttpMessageConverter.class);
HttpHeaders responseHeaders = new HttpHeaders();
MediaType contentType = MediaType.TEXT_PLAIN;
responseHeaders.setContentType(contentType);
String expected = "Foo";
ParameterizedTypeReference<List<String>> reference = new ParameterizedTypeReference<List<String>>() {};
Type type = reference.getType();
extractor = new HttpMessageConverterExtractor<List<String>>(type, createConverterList(converter));
GenericHttpMessageConverter<String> converter = mock(GenericHttpMessageConverter.class);
HttpMessageConverterExtractor<?> extractor = new HttpMessageConverterExtractor<List<String>>(type, asList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.OK.value());
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream(expected.getBytes()));
@ -182,48 +170,28 @@ public class HttpMessageConverterExtractorTests {
}
@Test // SPR-13592
@SuppressWarnings("unchecked")
public void converterThrowsIOException() throws IOException {
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
HttpHeaders responseHeaders = new HttpHeaders();
MediaType contentType = MediaType.TEXT_PLAIN;
responseHeaders.setContentType(contentType);
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.OK.value());
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream("Foobar".getBytes()));
given(converter.canRead(String.class, contentType)).willReturn(true);
given(converter.read(eq(String.class), any(HttpInputMessage.class))).willThrow(IOException.class);
assertThatExceptionOfType(RestClientException.class).isThrownBy(() ->
extractor.extractData(response))
assertThatExceptionOfType(RestClientException.class).isThrownBy(() -> extractor.extractData(response))
.withMessageContaining("Error while extracting response for type [class java.lang.String] and content type [text/plain]")
.withCauseInstanceOf(IOException.class);
}
@Test // SPR-13592
@SuppressWarnings("unchecked")
public void converterThrowsHttpMessageNotReadableException() throws IOException {
HttpMessageConverter<String> converter = mock(HttpMessageConverter.class);
HttpHeaders responseHeaders = new HttpHeaders();
MediaType contentType = MediaType.TEXT_PLAIN;
responseHeaders.setContentType(contentType);
extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
given(response.getRawStatusCode()).willReturn(HttpStatus.OK.value());
given(response.getHeaders()).willReturn(responseHeaders);
given(response.getBody()).willReturn(new ByteArrayInputStream("Foobar".getBytes()));
given(converter.canRead(String.class, contentType)).willThrow(HttpMessageNotReadableException.class);
assertThatExceptionOfType(RestClientException.class).isThrownBy(() ->
extractor.extractData(response))
assertThatExceptionOfType(RestClientException.class).isThrownBy(() -> extractor.extractData(response))
.withMessageContaining("Error while extracting response for type [class java.lang.String] and content type [text/plain]")
.withCauseInstanceOf(HttpMessageNotReadableException.class);
}
private List<HttpMessageConverter<?>> createConverterList(HttpMessageConverter<?> converter) {
List<HttpMessageConverter<?>> converters = new ArrayList<>(1);
converters.add(converter);
return converters;
}
}

23
spring-web/src/test/java/org/springframework/web/client/RestTemplateTests.java
View File

@ -49,6 +49,7 @@ import org.springframework.web.util.DefaultUriBuilderFactory;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;
@ -65,9 +66,12 @@ import static org.springframework.http.HttpMethod.PUT;
import static org.springframework.http.MediaType.parseMediaType;
/**
* Unit tests for {@link RestTemplate}.
*
* @author Arjen Poutsma
* @author Rossen Stoyanchev
* @author Brian Clozel
* @author Sam Brannen
*/
@SuppressWarnings("unchecked")
public class RestTemplateTests {
@ -98,6 +102,25 @@ public class RestTemplateTests {
template.setErrorHandler(errorHandler);
}
@Test
public void constructorPreconditions() {
assertThatIllegalArgumentException()
.isThrownBy(() -> new RestTemplate((List<HttpMessageConverter<?>>) null))
.withMessage("At least one HttpMessageConverter is required");
assertThatIllegalArgumentException()
.isThrownBy(() -> new RestTemplate(Arrays.asList(null, this.converter)))
.withMessage("The HttpMessageConverter list must not contain null elements");
}
@Test
public void setMessageConvertersPreconditions() {
assertThatIllegalArgumentException()
.isThrownBy(() -> template.setMessageConverters((List<HttpMessageConverter<?>>) null))
.withMessage("At least one HttpMessageConverter is required");
assertThatIllegalArgumentException()
.isThrownBy(() -> template.setMessageConverters(Arrays.asList(null, this.converter)))
.withMessage("The HttpMessageConverter list must not contain null elements");
}
@Test
public void varArgsTemplateVariables() throws Exception {