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

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

@@ -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.

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

@@ -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;

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

@@ -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.

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

@@ -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;
 	}
 
-
 }

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

@@ -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 {