Polish HttpMessageWriterView and view resolution

spring-webflux/src/main/java/org/springframework/web/reactive/result/view/HttpMessageWriterView.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 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,11 +17,12 @@
 package org.springframework.web.reactive.result.view;
 
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.reactivestreams.Publisher;
 import reactor.core.publisher.Mono;
@@ -37,53 +38,65 @@ import org.springframework.web.server.ServerWebExchange;
 
 
 /**
- * A {@link View} that delegates to an {@link HttpMessageWriter}.
+ * {@code View} that writes model attribute(s) with an {@link HttpMessageWriter}.
  *
  * @author Rossen Stoyanchev
  * @since 5.0
  */
 public class HttpMessageWriterView implements View {
 
-	private final HttpMessageWriter<?> messageWriter;
+	private final HttpMessageWriter<?> writer;
 
 	private final Set<String> modelKeys = new HashSet<>(4);
 
-	private final List<MediaType> mediaTypes;
+	private final boolean canWriteMap;
 
 
 	/**
-	 * Create a {@code View} with the given {@code Encoder} wrapping it as an
-	 * {@link EncoderHttpMessageWriter}.
+	 * Constructor with an {@code Encoder}.
 	 */
 	public HttpMessageWriterView(Encoder<?> encoder) {
 		this(new EncoderHttpMessageWriter<>(encoder));
 	}
 
 	/**
-	 * Create a View that delegates to the given message messageWriter.
+	 * Constructor with a fully initialized {@link HttpMessageWriter}.
 	 */
-	public HttpMessageWriterView(HttpMessageWriter<?> messageWriter) {
-		Assert.notNull(messageWriter, "'messageWriter' is required.");
-		this.messageWriter = messageWriter;
-		this.mediaTypes = messageWriter.getWritableMediaTypes();
+	public HttpMessageWriterView(HttpMessageWriter<?> writer) {
+		Assert.notNull(writer, "'writer' is required.");
+		this.writer = writer;
+		this.canWriteMap = writer.canWrite(ResolvableType.forClass(Map.class), null);
 	}
 
 
 	/**
-	 * Return the configured message messageWriter.
+	 * Return the configured message writer.
 	 */
 	public HttpMessageWriter<?> getMessageWriter() {
-		return this.messageWriter;
+		return this.writer;
 	}
 
 	/**
-	 * By default model attributes are filtered with
-	 * {@link HttpMessageWriter#canWrite} to find the ones that can be
-	 * rendered. Use this property to further narrow the list and consider only
-	 * attribute(s) under specific model key(s).
-	 * <p>If more than one matching attribute is found, than a Map is rendered,
-	 * or if the {@code Encoder} does not support rendering a {@code Map} then
-	 * an exception is raised.
+	 * {@inheritDoc}
+	 * <p>The implementation of this method for {@link HttpMessageWriterView}
+	 * delegates to {@link HttpMessageWriter#getWritableMediaTypes()}.
+	 */
+	@Override
+	public List<MediaType> getSupportedMediaTypes() {
+		return this.writer.getWritableMediaTypes();
+	}
+
+	/**
+	 * Set the attributes in the model that should be rendered by this view.
+	 * When set, all other model attributes will be ignored. The matching
+	 * attributes are further narrowed with {@link HttpMessageWriter#canWrite}.
+	 * The matching attributes are processed as follows:
+	 * <ul>
+	 * <li>0: nothing is written to the response body.
+	 * <li>1: the matching attribute is passed to the writer.
+	 * <li>2..N: if the writer supports {@link Map}, write all matches;
+	 * otherwise raise an {@link IllegalStateException}.
+	 * </ul>
 	 */
 	public void setModelKeys(Set<String> modelKeys) {
 		this.modelKeys.clear();
@@ -99,73 +112,50 @@ public class HttpMessageWriterView implements View {
 		return this.modelKeys;
 	}
 
-	@Override
-	public List<MediaType> getSupportedMediaTypes() {
-		return this.mediaTypes;
-	}
 
 	@Override
-	public Mono<Void> render(Map<String, ?> model, MediaType contentType,
-			ServerWebExchange exchange) {
-		Object value = extractObjectToRender(model);
-		return applyMessageWriter(value, contentType, exchange);
-	}
-
-	protected Object extractObjectToRender(Map<String, ?> model) {
-		Map<String, Object> map = new HashMap<>(model.size());
-		for (Map.Entry<String, ?> entry : model.entrySet()) {
-			if (isEligibleAttribute(entry.getKey(), entry.getValue())) {
-				map.put(entry.getKey(), entry.getValue());
-			}
-		}
-		if (map.isEmpty()) {
-			return null;
-		}
-		else if (map.size() == 1) {
-			return map.values().iterator().next();
-		}
-		else if (getMessageWriter().canWrite(ResolvableType.forClass(Map.class), null)) {
-			return map;
-		}
-		else {
-			throw new IllegalStateException(
-					"Multiple matching attributes found: " + map + ". " +
-							"However Map rendering is not supported by " + getMessageWriter());
-		}
-	}
-
-	/**
-	 * Whether the given model attribute key-value pair is eligible for encoding.
-	 * <p>The default implementation checks against the configured
-	 * {@link #setModelKeys model keys} and whether the Encoder supports the
-	 * value type.
-	 */
-	protected boolean isEligibleAttribute(String attributeName, Object attributeValue) {
-		ResolvableType type = ResolvableType.forClass(attributeValue.getClass());
-		if (getModelKeys().isEmpty()) {
-			return getMessageWriter().canWrite(type, null);
-		}
-		if (getModelKeys().contains(attributeName)) {
-			if (getMessageWriter().canWrite(type, null)) {
-				return true;
-			}
-			throw new IllegalStateException(
-					"Model object [" + attributeValue + "] retrieved via key " +
-							"[" + attributeName + "] is not supported by " + getMessageWriter());
-		}
-		return false;
-	}
-
 	@SuppressWarnings("unchecked")
-	private <T> Mono<Void> applyMessageWriter(Object value, MediaType contentType, ServerWebExchange exchange) {
-		if (value == null) {
-			return Mono.empty();
-		}
-		Publisher<? extends T> stream = Mono.just((T) value);
+	public Mono<Void> render(Map<String, ?> model, MediaType contentType, ServerWebExchange exchange) {
+		return getObjectToRender(model)
+				.map(value -> {
+					Publisher stream = Mono.justOrEmpty(value);
 					ResolvableType type = ResolvableType.forClass(value.getClass());
 					ServerHttpResponse response = exchange.getResponse();
-		return ((HttpMessageWriter<T>) getMessageWriter()).write(stream, type, contentType,
-				response, Collections.emptyMap());
+					return this.writer.write(stream, type, contentType, response, Collections.emptyMap());
+				})
+				.orElseGet(() -> exchange.getResponse().setComplete());
+	}
+
+	private Optional<Object> getObjectToRender(Map<String, ?> model) {
+
+		Map<String, ?> result = model.entrySet().stream()
+				.filter(this::isMatch)
+				.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
+		if (result.isEmpty()) {
+			return Optional.empty();
+		}
+		else if (result.size() == 1) {
+			return Optional.of(result.values().iterator().next());
+		}
+		else if (this.canWriteMap) {
+			return Optional.of(result);
+		}
+		else {
+			throw new IllegalStateException("Multiple matches found: " + result + " but " +
+					"Map rendering is not supported by " + getMessageWriter().getClass().getName());
+		}
+	}
+
+	private boolean isMatch(Map.Entry<String, ?> entry) {
+		if (entry.getValue() == null) {
+			return false;
+		}
+		if (!getModelKeys().isEmpty() && !getModelKeys().contains(entry.getKey())) {
+			return false;
+		}
+		ResolvableType type = ResolvableType.forInstance(entry.getValue());
+		return getMessageWriter().canWrite(type, null);
 	}
 
 }

spring-webflux/src/main/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandler.java

@@ -55,13 +55,13 @@ import org.springframework.web.server.support.HttpRequestPathHelper;
  * {@code HandlerResultHandler} that encapsulates the view resolution algorithm
  * supporting the following return types:
  * <ul>
- *     <li>String-based view name
- *     <li>Reference to a {@link View}
- *     <li>{@link Model}
- *     <li>{@link Map}
- *     <li>Return types annotated with {@code @ModelAttribute}
- *     <li>{@link BeanUtils#isSimpleProperty Non-simple} return types are
- *     treated as a model attribute
+ *     <li>{@link Void} or no value -- default view name</li>
+ *     <li>{@link String} -- view name unless {@code @ModelAttribute}-annotated
+ *     <li>{@link View} -- View to render with
+ *     <li>{@link Model} -- attributes to add to the model
+ *     <li>{@link Map} -- attributes to add to the model
+ *     <li>{@link ModelAttribute @ModelAttribute} -- attribute for the model
+ *     <li>Non-simple value -- attribute for the model
  * </ul>
  *
  * <p>A String-based view name is resolved through the configured
@@ -71,8 +71,9 @@ import org.springframework.web.server.support.HttpRequestPathHelper;
  *
  * <p>By default this resolver is ordered at {@link Ordered#LOWEST_PRECEDENCE}
  * and generally needs to be late in the order since it interprets any String
- * return value as a view name while others may interpret the same otherwise
- * based on annotations (e.g. for {@code @ResponseBody}).
+ * return value as a view name or any non-simple value type as a model attribute
+ * while other result handlers may interpret the same otherwise based on the
+ * presence of annotations, e.g. for {@code @ResponseBody}.
  *
  * @author Rossen Stoyanchev
  * @since 5.0
@@ -174,11 +175,16 @@ public class ViewResolutionResultHandler extends HandlerResultHandlerSupport
 		ReactiveAdapter adapter = getAdapter(result);
 
 		if (adapter != null) {
-			Assert.isTrue(!adapter.isMultiValue(), "Only single-value async return type supported.");
+			Assert.isTrue(!adapter.isMultiValue(), "Multi-value " +
+					"reactive types not supported in view resolution: " + result.getReturnType());
+
 			valueMono = result.getReturnValue()
-					.map(value -> Mono.from(adapter.toPublisher(value))).orElse(Mono.empty());
+					.map(value -> Mono.from(adapter.toPublisher(value)))
+					.orElse(Mono.empty());
+
 			valueType = adapter.isNoValue() ?
-					ResolvableType.forClass(Void.class) : result.getReturnType().getGeneric(0);
+					ResolvableType.forClass(Void.class) :
+					result.getReturnType().getGeneric(0);
 		}
 		else {
 			valueMono = Mono.justOrEmpty(result.getReturnValue());
@@ -224,7 +230,7 @@ public class ViewResolutionResultHandler extends HandlerResultHandlerSupport
 						viewsMono = resolveViews(getDefaultViewName(exchange), locale);
 					}
 
-					addBindingResult(result.getBindingContext(), exchange);
+					updateBindingContext(result.getBindingContext(), exchange);
 
 					return viewsMono.then(views -> render(views, model.asMap(), exchange));
 				});
@@ -259,11 +265,6 @@ public class ViewResolutionResultHandler extends HandlerResultHandlerSupport
 				});
 	}
 
-	/**
-	 * Return the name of a model attribute return value based on the method
-	 * {@code @ModelAttribute} annotation, if present, or derived from the type
-	 * of the return value otherwise.
-	 */
 	private String getNameForReturnValue(Class<?> returnValueType, MethodParameter returnType) {
 		ModelAttribute annotation = returnType.getMethodAnnotation(ModelAttribute.class);
 		if (annotation != null && StringUtils.hasText(annotation.value())) {
@@ -273,9 +274,7 @@ public class ViewResolutionResultHandler extends HandlerResultHandlerSupport
 		return ClassUtils.getShortNameAsProperty(returnValueType);
 	}
 
-
-
-	private void addBindingResult(BindingContext context, ServerWebExchange exchange) {
+	private void updateBindingContext(BindingContext context, ServerWebExchange exchange) {
 		Map<String, Object> model = context.getModel().asMap();
 		model.keySet().stream()
 				.filter(name -> isBindingCandidate(name, model.get(name)))

spring-webflux/src/test/java/org/springframework/web/reactive/result/view/HttpMessageWriterViewTests.java

@@ -20,16 +20,15 @@ import java.nio.charset.StandardCharsets;
 import java.time.Duration;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 
 import org.junit.Test;
 import reactor.test.StepVerifier;
 
 import org.springframework.core.codec.CharSequenceEncoder;
+import org.springframework.core.io.buffer.DataBuffer;
 import org.springframework.core.io.buffer.support.DataBufferTestUtils;
 import org.springframework.http.MediaType;
 import org.springframework.http.codec.json.Jackson2JsonEncoder;
@@ -38,12 +37,9 @@ import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest;
 import org.springframework.mock.http.server.reactive.test.MockServerWebExchange;
 import org.springframework.ui.ExtendedModelMap;
 import org.springframework.ui.ModelMap;
-import org.springframework.util.MimeType;
 
 import static junit.framework.TestCase.assertTrue;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.fail;
 
 
@@ -55,62 +51,65 @@ public class HttpMessageWriterViewTests {
 
 	private HttpMessageWriterView view = new HttpMessageWriterView(new Jackson2JsonEncoder());
 
-	private ModelMap model = new ExtendedModelMap();
+	private final ModelMap model = new ExtendedModelMap();
+
+	private final MockServerWebExchange exchange = MockServerHttpRequest.get("/").toExchange();
 
 
 	@Test
 	public void supportedMediaTypes() throws Exception {
-		List<MimeType> mimeTypes = Arrays.asList(
-				new MimeType("application", "json", StandardCharsets.UTF_8),
-				new MimeType("application", "*+json", StandardCharsets.UTF_8));
-
-		assertEquals(mimeTypes, this.view.getSupportedMediaTypes());
+		assertEquals(Arrays.asList(
+				MediaType.parseMediaType("application/json;charset=UTF-8"),
+				MediaType.parseMediaType("application/*+json;charset=UTF-8")),
+				this.view.getSupportedMediaTypes());
 	}
 
 	@Test
-	public void extractObject() throws Exception {
+	public void singleMatch() throws Exception {
 		this.view.setModelKeys(Collections.singleton("foo2"));
 		this.model.addAttribute("foo1", "bar1");
 		this.model.addAttribute("foo2", "bar2");
 		this.model.addAttribute("foo3", "bar3");
 
-		assertEquals("bar2", this.view.extractObjectToRender(this.model));
+		assertEquals("\"bar2\"", doRender());
 	}
 
 	@Test
-	public void extractObjectNoMatch() throws Exception {
+	public void noMatch() throws Exception {
 		this.view.setModelKeys(Collections.singleton("foo2"));
 		this.model.addAttribute("foo1", "bar1");
 
-		assertNull(this.view.extractObjectToRender(this.model));
+		assertEquals("", doRender());
 	}
 
 	@Test
-	public void extractObjectMultipleMatches() throws Exception {
+	public void noMatchBecauseNotSupported() throws Exception {
+		this.view = new HttpMessageWriterView(new Jaxb2XmlEncoder());
+		this.view.setModelKeys(new HashSet<>(Collections.singletonList("foo1")));
+		this.model.addAttribute("foo1", "bar1");
+
+		assertEquals("", doRender());
+	}
+
+	@Test
+	public void multipleMatches() throws Exception {
 		this.view.setModelKeys(new HashSet<>(Arrays.asList("foo1", "foo2")));
 		this.model.addAttribute("foo1", "bar1");
 		this.model.addAttribute("foo2", "bar2");
 		this.model.addAttribute("foo3", "bar3");
 
-		Object value = this.view.extractObjectToRender(this.model);
-		assertNotNull(value);
-		assertEquals(HashMap.class, value.getClass());
-
-		Map<?, ?> map = (Map<?, ?>) value;
-		assertEquals(2, map.size());
-		assertEquals("bar1", map.get("foo1"));
-		assertEquals("bar2", map.get("foo2"));
+		assertEquals("{\"foo1\":\"bar1\",\"foo2\":\"bar2\"}", doRender());
 	}
 
 	@Test
-	public void extractObjectMultipleMatchesNotSupported() throws Exception {
-		HttpMessageWriterView view = new HttpMessageWriterView(CharSequenceEncoder.allMimeTypes());
-		view.setModelKeys(new HashSet<>(Arrays.asList("foo1", "foo2")));
+	public void multipleMatchesNotSupported() throws Exception {
+		this.view = new HttpMessageWriterView(CharSequenceEncoder.allMimeTypes());
+		this.view.setModelKeys(new HashSet<>(Arrays.asList("foo1", "foo2")));
 		this.model.addAttribute("foo1", "bar1");
 		this.model.addAttribute("foo2", "bar2");
 
 		try {
-			view.extractObjectToRender(this.model);
+			doRender();
 			fail();
 		}
 		catch (IllegalStateException ex) {
@@ -119,22 +118,6 @@ public class HttpMessageWriterViewTests {
 		}
 	}
 
-	@Test
-	public void extractObjectNotSupported() throws Exception {
-		HttpMessageWriterView view = new HttpMessageWriterView(new Jaxb2XmlEncoder());
-		view.setModelKeys(new HashSet<>(Collections.singletonList("foo1")));
-		this.model.addAttribute("foo1", "bar1");
-
-		try {
-			view.extractObjectToRender(this.model);
-			fail();
-		}
-		catch (IllegalStateException ex) {
-			String message = ex.getMessage();
-			assertTrue(message, message.contains("[foo1] is not supported"));
-		}
-	}
-
 	@Test
 	public void render() throws Exception {
 		Map<String, String> pojoData = new LinkedHashMap<>();
@@ -143,18 +126,24 @@ public class HttpMessageWriterViewTests {
 		this.model.addAttribute("pojoData", pojoData);
 		this.view.setModelKeys(Collections.singleton("pojoData"));
 
-		MockServerWebExchange exchange = MockServerHttpRequest.get("/path").toExchange();
+		this.view.render(this.model, MediaType.APPLICATION_JSON, exchange).block(Duration.ZERO);
 
-		this.view.render(this.model, MediaType.APPLICATION_JSON, exchange).block(Duration.ofSeconds(5));
-
-		StepVerifier.create(exchange.getResponse().getBody())
-				.consumeNextWith( buf -> assertEquals("{\"foo\":\"f\",\"bar\":\"b\"}",
-						DataBufferTestUtils.dumpString(buf, StandardCharsets.UTF_8))
-				)
+		StepVerifier.create(this.exchange.getResponse().getBody())
+				.consumeNextWith(buf -> assertEquals("{\"foo\":\"f\",\"bar\":\"b\"}", dumpString(buf)))
 				.expectComplete()
 				.verify();
 	}
 
+	private String dumpString(DataBuffer buf) {
+		return DataBufferTestUtils.dumpString(buf, StandardCharsets.UTF_8);
+	}
+
+	private String doRender() {
+		this.view.render(this.model, MediaType.APPLICATION_JSON, this.exchange).block(Duration.ZERO);
+		return this.exchange.getResponse().getBodyAsString().block(Duration.ZERO);
+	}
+
+
 
 	@SuppressWarnings("unused")
 	private String handle() {