From 172afb510a7e8e0daca10753fe965b0dfb53f6f7 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 16 May 2019 14:37:04 -0400 Subject: [PATCH 1/2] Polish AbstractView in WebFlux --- .../reactive/result/view/AbstractView.java | 72 +++++++++---------- .../result/view/AbstractViewTests.java | 7 +- 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/AbstractView.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/AbstractView.java index 9a32fb19ca..5335adb725 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/AbstractView.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/AbstractView.java @@ -20,9 +20,9 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -211,30 +211,27 @@ public abstract class AbstractView implements View, BeanNameAware, ApplicationCo *

The default implementation creates a combined output Map that includes * model as well as static attributes with the former taking precedence. */ - protected Mono> getModelAttributes(@Nullable Map model, - ServerWebExchange exchange) { + protected Mono> getModelAttributes( + @Nullable Map model, ServerWebExchange exchange) { int size = (model != null ? model.size() : 0); - - Map attributes = new LinkedHashMap<>(size); + Map attributes = new ConcurrentHashMap<>(size); if (model != null) { attributes.putAll(model); } - - return resolveAsyncAttributes(attributes).then(Mono.just(attributes)); + return resolveAsyncAttributes(attributes).thenReturn(attributes); } /** - * By default, resolve async attributes supported by the - * {@link ReactiveAdapterRegistry} to their blocking counterparts. - *

View implementations capable of taking advantage of reactive types - * can override this method if needed. - * @return {@code Mono} for the completion of async attributes resolution + * Use the configured {@link ReactiveAdapterRegistry} to adapt asynchronous + * model attributes to {@code Mono} or {@code Mono>} and resolve + * them to actual values via {@link Mono#zip(Mono, Mono)}, so that when + * the returned result {@code Mono} completes, the model has its asynchronous + * attributes replaced with synchronous values. + * @return result {@code Mono} that completes when the model is ready */ protected Mono resolveAsyncAttributes(Map model) { - List names = new ArrayList<>(); - List> valueMonos = new ArrayList<>(); - + List> asyncAttributes = null; for (Map.Entry entry : model.entrySet()) { Object value = entry.getValue(); if (value == null) { @@ -242,35 +239,34 @@ public abstract class AbstractView implements View, BeanNameAware, ApplicationCo } ReactiveAdapter adapter = this.adapterRegistry.getAdapter(null, value); if (adapter != null) { - names.add(entry.getKey()); + if (asyncAttributes == null) { + asyncAttributes = new ArrayList<>(); + } + String name = entry.getKey(); if (adapter.isMultiValue()) { - Flux fluxValue = Flux.from(adapter.toPublisher(value)); - valueMonos.add(fluxValue.collectList().defaultIfEmpty(Collections.emptyList())); + asyncAttributes.add( + Flux.from(adapter.toPublisher(value)) + .collectList() + .doOnSuccess(result -> { + result = result != null ? result : Collections.emptyList(); + model.put(name, result); + })); } else { - Mono monoValue = Mono.from(adapter.toPublisher(value)); - valueMonos.add(monoValue.defaultIfEmpty(NO_VALUE)); + asyncAttributes.add( + Mono.from(adapter.toPublisher(value)) + .doOnSuccess(result -> { + if (result != null) { + model.put(name, result); + } + else { + model.remove(name); + } + })); } } } - - if (names.isEmpty()) { - return Mono.empty(); - } - - return Mono.zip(valueMonos, - values -> { - for (int i=0; i < values.length; i++) { - if (values[i] != NO_VALUE) { - model.put(names.get(i), values[i]); - } - else { - model.remove(names.get(i)); - } - } - return NO_VALUE; - }) - .then(); + return asyncAttributes != null ? Mono.when(asyncAttributes) : Mono.empty(); } /** diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/AbstractViewTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/AbstractViewTests.java index 2f6519091a..0a9c239a7c 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/AbstractViewTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/AbstractViewTests.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. @@ -16,6 +16,7 @@ package org.springframework.web.reactive.result.view; +import java.time.Duration; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -54,8 +55,8 @@ public class AbstractViewTests { TestBean testBean1 = new TestBean("Bean1"); TestBean testBean2 = new TestBean("Bean2"); Map attributes = new HashMap<>(); - attributes.put("attr1", Mono.just(testBean1)); - attributes.put("attr2", Flux.just(testBean1, testBean2)); + attributes.put("attr1", Mono.just(testBean1).delayElement(Duration.ofMillis(10))); + attributes.put("attr2", Flux.just(testBean1, testBean2).delayElements(Duration.ofMillis(10))); attributes.put("attr3", Single.just(testBean2)); attributes.put("attr4", Observable.just(testBean1, testBean2)); attributes.put("attr5", Mono.empty()); From da4430e27ef13a42a47c954386a3a1f66e4f6cd2 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 16 May 2019 20:41:37 -0400 Subject: [PATCH 2/2] BindingResult for resolved async attributes ViewResolutionResultHandler no longer adds BindingResult objects for async model attributes. Instead AbstractView adds them later when those attributes are resolved to concrete values. Closes gh-22933 --- .../reactive/result/view/AbstractView.java | 56 ++++++++++++++----- .../web/reactive/result/view/View.java | 14 ++++- .../view/ViewResolutionResultHandler.java | 46 +++++++++------ .../result/view/AbstractViewTests.java | 51 +++++++++-------- 4 files changed, 113 insertions(+), 54 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/AbstractView.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/AbstractView.java index 5335adb725..a4d0801e5f 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/AbstractView.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/AbstractView.java @@ -19,6 +19,7 @@ package org.springframework.web.reactive.result.view; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; @@ -29,6 +30,7 @@ import org.apache.commons.logging.LogFactory; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import org.springframework.beans.BeanUtils; import org.springframework.beans.factory.BeanNameAware; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; @@ -37,6 +39,8 @@ import org.springframework.core.ReactiveAdapterRegistry; import org.springframework.http.MediaType; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.validation.BindingResult; +import org.springframework.web.reactive.BindingContext; import org.springframework.web.server.ServerWebExchange; /** @@ -54,9 +58,6 @@ public abstract class AbstractView implements View, BeanNameAware, ApplicationCo /** Logger that is available to subclasses. */ protected final Log logger = LogFactory.getLog(getClass()); - private static final Object NO_VALUE = new Object(); - - private final ReactiveAdapterRegistry adapterRegistry; private final List mediaTypes = new ArrayList<>(4); @@ -219,18 +220,23 @@ public abstract class AbstractView implements View, BeanNameAware, ApplicationCo if (model != null) { attributes.putAll(model); } - return resolveAsyncAttributes(attributes).thenReturn(attributes); + //noinspection deprecation + return resolveAsyncAttributes(attributes) + .then(resolveAsyncAttributes(attributes, exchange)) + .doOnTerminate(() -> exchange.getAttributes().remove(BINDING_CONTEXT_ATTRIBUTE)) + .thenReturn(attributes); } /** * Use the configured {@link ReactiveAdapterRegistry} to adapt asynchronous - * model attributes to {@code Mono} or {@code Mono>} and resolve - * them to actual values via {@link Mono#zip(Mono, Mono)}, so that when - * the returned result {@code Mono} completes, the model has its asynchronous - * attributes replaced with synchronous values. + * attributes to {@code Mono} or {@code Mono>} and then wait to + * resolve them into actual values. When the returned {@code Mono} + * completes, the asynchronous attributes in the model would have been + * replaced with their corresponding resolved values. * @return result {@code Mono} that completes when the model is ready + * @since 5.1.8 */ - protected Mono resolveAsyncAttributes(Map model) { + protected Mono resolveAsyncAttributes(Map model, ServerWebExchange exchange) { List> asyncAttributes = null; for (Map.Entry entry : model.entrySet()) { Object value = entry.getValue(); @@ -247,10 +253,7 @@ public abstract class AbstractView implements View, BeanNameAware, ApplicationCo asyncAttributes.add( Flux.from(adapter.toPublisher(value)) .collectList() - .doOnSuccess(result -> { - result = result != null ? result : Collections.emptyList(); - model.put(name, result); - })); + .doOnSuccess(result -> model.put(name, result))); } else { asyncAttributes.add( @@ -258,6 +261,7 @@ public abstract class AbstractView implements View, BeanNameAware, ApplicationCo .doOnSuccess(result -> { if (result != null) { model.put(name, result); + addBindingResult(name, result, model, exchange); } else { model.remove(name); @@ -269,6 +273,32 @@ public abstract class AbstractView implements View, BeanNameAware, ApplicationCo return asyncAttributes != null ? Mono.when(asyncAttributes) : Mono.empty(); } + private void addBindingResult(String name, Object value, Map model, ServerWebExchange exchange) { + BindingContext context = exchange.getAttribute(BINDING_CONTEXT_ATTRIBUTE); + if (context == null || value.getClass().isArray() || value instanceof Collection || + value instanceof Map || BeanUtils.isSimpleValueType(value.getClass())) { + return; + } + BindingResult result = context.createDataBinder(exchange, value, name).getBindingResult(); + model.put(BindingResult.MODEL_KEY_PREFIX + name, result); + } + + /** + * Use the configured {@link ReactiveAdapterRegistry} to adapt asynchronous + * attributes to {@code Mono} or {@code Mono>} and then wait to + * resolve them into actual values. When the returned {@code Mono} + * completes, the asynchronous attributes in the model would have been + * replaced with their corresponding resolved values. + * @return result {@code Mono} that completes when the model is ready + * @deprecated as of 5.1.8 this method is still invoked but it is a no-op. + * Please, use {@link #resolveAsyncAttributes(Map, ServerWebExchange)} + * instead. It is invoked after this one and does the actual work. + */ + @Deprecated + protected Mono resolveAsyncAttributes(Map model) { + return Mono.empty(); + } + /** * Create a RequestContext to expose under the specified attribute name. *

The default implementation creates a standard RequestContext instance diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/View.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/View.java index 3b0e23e2f7..4df2b19424 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/View.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/View.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. @@ -44,6 +44,18 @@ import org.springframework.web.server.ServerWebExchange; */ public interface View { + /** + * The name of the exchange attribute that contains the + * {@link org.springframework.web.reactive.BindingContext BindingContext} + * for the request which can be used to create + * {@link org.springframework.validation.BindingResult BindingResult} + * instances for objects in to the model. + *

Note: This attribute is not required and may not be present. + * @since 5.1.8 + */ + String BINDING_CONTEXT_ATTRIBUTE = View.class.getName() + ".bindingContext"; + + /** * Return the list of media types this View supports, or an empty list. */ diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandler.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandler.java index 859c78a4e6..691fac712b 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandler.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandler.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. @@ -245,10 +245,9 @@ public class ViewResolutionResultHandler extends HandlerResultHandlerSupport model.addAttribute(name, returnValue); viewsMono = resolveViews(getDefaultViewName(exchange), locale); } - - updateBindingContext(result.getBindingContext(), exchange); - - return viewsMono.flatMap(views -> render(views, model.asMap(), exchange)); + BindingContext bindingContext = result.getBindingContext(); + updateBindingResult(bindingContext, exchange); + return viewsMono.flatMap(views -> render(views, model.asMap(), bindingContext, exchange)); }); } @@ -288,39 +287,42 @@ public class ViewResolutionResultHandler extends HandlerResultHandlerSupport .orElseGet(() -> Conventions.getVariableNameForParameter(returnType)); } - private void updateBindingContext(BindingContext context, ServerWebExchange exchange) { + private void updateBindingResult(BindingContext context, ServerWebExchange exchange) { Map model = context.getModel().asMap(); - model.keySet().stream() - .filter(name -> isBindingCandidate(name, model.get(name))) - .filter(name -> !model.containsKey(BindingResult.MODEL_KEY_PREFIX + name)) - .forEach(name -> { - WebExchangeDataBinder binder = context.createDataBinder(exchange, model.get(name), name); + for (Map.Entry entry : model.entrySet()) { + String name = entry.getKey(); + Object value = entry.getValue(); + if (isBindingCandidate(name, value)) { + if (!model.containsKey(BindingResult.MODEL_KEY_PREFIX + name)) { + WebExchangeDataBinder binder = context.createDataBinder(exchange, value, name); model.put(BindingResult.MODEL_KEY_PREFIX + name, binder.getBindingResult()); - }); + } + } + } } private boolean isBindingCandidate(String name, @Nullable Object value) { return (!name.startsWith(BindingResult.MODEL_KEY_PREFIX) && value != null && - !value.getClass().isArray() && !(value instanceof Collection) && - !(value instanceof Map) && !BeanUtils.isSimpleValueType(value.getClass())); + !value.getClass().isArray() && !(value instanceof Collection) && !(value instanceof Map) && + getAdapterRegistry().getAdapter(null, value) == null && + !BeanUtils.isSimpleValueType(value.getClass())); } private Mono render(List views, Map model, - ServerWebExchange exchange) { + BindingContext bindingContext, ServerWebExchange exchange) { for (View view : views) { if (view.isRedirectView()) { - return view.render(model, null, exchange); + return renderWith(view, model, null, exchange, bindingContext); } } - List mediaTypes = getMediaTypes(views); MediaType bestMediaType = selectMediaType(exchange, () -> mediaTypes); if (bestMediaType != null) { for (View view : views) { for (MediaType mediaType : view.getSupportedMediaTypes()) { if (mediaType.isCompatibleWith(bestMediaType)) { - return view.render(model, mediaType, exchange); + return renderWith(view, model, mediaType, exchange, bindingContext); } } } @@ -328,6 +330,14 @@ public class ViewResolutionResultHandler extends HandlerResultHandlerSupport throw new NotAcceptableStatusException(mediaTypes); } + private Mono renderWith(View view, Map model, + @Nullable MediaType mediaType, ServerWebExchange exchange, BindingContext bindingContext) { + + exchange.getAttributes().put(View.BINDING_CONTEXT_ATTRIBUTE, bindingContext); + return view.render(model, mediaType, exchange) + .doOnTerminate(() -> exchange.getAttributes().remove(View.BINDING_CONTEXT_ATTRIBUTE)); + } + private List getMediaTypes(List views) { return views.stream() .flatMap(view -> view.getSupportedMediaTypes().stream()) diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/AbstractViewTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/AbstractViewTests.java index 0a9c239a7c..b3cfa1395b 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/AbstractViewTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/AbstractViewTests.java @@ -17,8 +17,8 @@ package org.springframework.web.reactive.result.view; import java.time.Duration; +import java.util.Arrays; import java.util.HashMap; -import java.util.List; import java.util.Map; import io.reactivex.Observable; @@ -29,14 +29,15 @@ import reactor.core.publisher.Mono; import reactor.test.StepVerifier; import org.springframework.http.MediaType; +import org.springframework.lang.Nullable; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; import org.springframework.mock.web.test.server.MockServerWebExchange; import org.springframework.tests.sample.beans.TestBean; +import org.springframework.validation.BindingResult; +import org.springframework.web.reactive.BindingContext; import org.springframework.web.server.ServerWebExchange; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.*; /** * Unit tests for {@link AbstractView}. @@ -54,25 +55,31 @@ public class AbstractViewTests { TestBean testBean1 = new TestBean("Bean1"); TestBean testBean2 = new TestBean("Bean2"); - Map attributes = new HashMap<>(); - attributes.put("attr1", Mono.just(testBean1).delayElement(Duration.ofMillis(10))); - attributes.put("attr2", Flux.just(testBean1, testBean2).delayElements(Duration.ofMillis(10))); - attributes.put("attr3", Single.just(testBean2)); - attributes.put("attr4", Observable.just(testBean1, testBean2)); - attributes.put("attr5", Mono.empty()); + + Map inMap = new HashMap<>(); + inMap.put("attr1", Mono.just(testBean1).delayElement(Duration.ofMillis(10))); + inMap.put("attr2", Flux.just(testBean1, testBean2).delayElements(Duration.ofMillis(10))); + inMap.put("attr3", Single.just(testBean2)); + inMap.put("attr4", Observable.just(testBean1, testBean2)); + inMap.put("attr5", Mono.empty()); + + this.exchange.getAttributes().put(View.BINDING_CONTEXT_ATTRIBUTE, new BindingContext()); TestView view = new TestView(); - StepVerifier.create( - view.render(attributes, null, this.exchange)).verifyComplete(); - - Map actual = view.attributes; - assertEquals(testBean1, actual.get("attr1")); - assertArrayEquals(new TestBean[] { testBean1, testBean2 }, - ((List) actual.get("attr2")).toArray()); - assertEquals(testBean2, actual.get("attr3")); - assertArrayEquals(new TestBean[] { testBean1, testBean2 }, - ((List) actual.get("attr4")).toArray()); - assertNull(actual.get("attr5")); + StepVerifier.create(view.render(inMap, null, this.exchange)).verifyComplete(); + + Map outMap = view.attributes; + assertEquals(testBean1, outMap.get("attr1")); + assertEquals(Arrays.asList(testBean1, testBean2), outMap.get("attr2")); + assertEquals(testBean2, outMap.get("attr3")); + assertEquals(Arrays.asList(testBean1, testBean2), outMap.get("attr4")); + assertNull(outMap.get("attr5")); + + assertNotNull(outMap.get(BindingResult.MODEL_KEY_PREFIX + "attr1")); + assertNotNull(outMap.get(BindingResult.MODEL_KEY_PREFIX + "attr3")); + assertNull(outMap.get(BindingResult.MODEL_KEY_PREFIX + "attr2")); + assertNull(outMap.get(BindingResult.MODEL_KEY_PREFIX + "attr4")); + assertNull(outMap.get(BindingResult.MODEL_KEY_PREFIX + "attr5")); } private static class TestView extends AbstractView { @@ -81,7 +88,7 @@ public class AbstractViewTests { @Override protected Mono renderInternal(Map renderAttributes, - MediaType contentType, ServerWebExchange exchange) { + @Nullable MediaType contentType, ServerWebExchange exchange) { this.attributes = renderAttributes; return Mono.empty();