From d3b178a812b6c3b55f8060ba75679b3ad7b84f3b Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 20 Apr 2017 09:17:52 -0400 Subject: [PATCH] Consistent JSON array result for Flux in Spring MVC Issue: SPR-15456 --- .../annotation/ReactiveTypeHandler.java | 18 ++++---- .../annotation/ReactiveTypeHandlerTests.java | 43 ++++++++++--------- 2 files changed, 31 insertions(+), 30 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandler.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandler.java index cf36906a06..1f4cb2568b 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandler.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandler.java @@ -27,6 +27,7 @@ import org.apache.commons.logging.LogFactory; import org.reactivestreams.Publisher; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; + import org.springframework.core.MethodParameter; import org.springframework.core.ReactiveAdapter; import org.springframework.core.ReactiveAdapterRegistry; @@ -119,7 +120,6 @@ class ReactiveTypeHandler { Collection mediaTypes = getMediaTypes(request); Optional mediaType = mediaTypes.stream().filter(MimeType::isConcrete).findFirst(); - boolean jsonArrayOfStrings = isJsonArrayOfStrings(elementType, mediaType); if (adapter.isMultiValue()) { if (mediaTypes.stream().anyMatch(MediaType.TEXT_EVENT_STREAM::includes) || @@ -133,7 +133,7 @@ class ReactiveTypeHandler { new JsonEmitterSubscriber(emitter, this.taskExecutor).connect(adapter, returnValue); return emitter; } - if (CharSequence.class.isAssignableFrom(elementType) && !jsonArrayOfStrings) { + if (CharSequence.class.isAssignableFrom(elementType) && !isJsonStringArray(elementType, mediaType)) { ResponseBodyEmitter emitter = getEmitter(mediaType.orElse(MediaType.TEXT_PLAIN)); new TextEmitterSubscriber(emitter, this.taskExecutor).connect(adapter, returnValue); return emitter; @@ -142,7 +142,7 @@ class ReactiveTypeHandler { // Not streaming... DeferredResult result = new DeferredResult<>(); - new DeferredResultSubscriber(result, jsonArrayOfStrings).connect(adapter, returnValue); + new DeferredResultSubscriber(result, adapter).connect(adapter, returnValue); WebAsyncUtils.getAsyncManager(request).startDeferredResultProcessing(result, mav); return null; @@ -160,7 +160,7 @@ class ReactiveTypeHandler { } @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - private boolean isJsonArrayOfStrings(Class elementType, Optional mediaType) { + private boolean isJsonStringArray(Class elementType, Optional mediaType) { return CharSequence.class.isAssignableFrom(elementType) && mediaType.filter(type -> MediaType.APPLICATION_JSON.includes(type) || JSON_TYPE.includes(type)).isPresent(); } @@ -387,14 +387,14 @@ class ReactiveTypeHandler { private final DeferredResult result; - private final boolean jsonArrayOfStrings; + private final boolean multiValueSource; private final CollectedValuesList values = new CollectedValuesList(); - DeferredResultSubscriber(DeferredResult result, boolean jsonArrayOfStrings) { + DeferredResultSubscriber(DeferredResult result, ReactiveAdapter adapter) { this.result = result; - this.jsonArrayOfStrings = jsonArrayOfStrings; + this.multiValueSource = adapter.isMultiValue(); } @@ -421,14 +421,14 @@ class ReactiveTypeHandler { @Override public void onComplete() { - if (this.values.size() > 1) { + if (this.values.size() > 1 || this.multiValueSource) { this.result.setResult(this.values); } else if (this.values.size() == 1) { this.result.setResult(this.values.get(0)); } else { - this.result.setResult(this.jsonArrayOfStrings ? this.values : null); + this.result.setResult(null); } } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandlerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandlerTests.java index fb0517ef47..459945c088 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandlerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandlerTests.java @@ -16,7 +16,6 @@ package org.springframework.web.servlet.mvc.method.annotation; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Set; @@ -53,6 +52,7 @@ import org.springframework.web.servlet.HandlerMapping; import static junit.framework.TestCase.assertNull; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.springframework.web.method.ResolvableMethod.on; @@ -127,15 +127,8 @@ public class ReactiveTypeHandlerTests { @Test public void deferredResultSubscriberWithNoValues() throws Exception { - - // Empty -> null MonoProcessor monoEmpty = MonoProcessor.create(); testDeferredResultSubscriber(monoEmpty, Mono.class, monoEmpty::onComplete, null); - - // Empty -> List[0] when JSON is preferred - this.servletRequest.addHeader("Accept", "application/json"); - MonoProcessor monoEmpty2 = MonoProcessor.create(); - testDeferredResultSubscriber(monoEmpty2, Mono.class, monoEmpty2::onComplete, new ArrayList<>()); } @Test @@ -177,23 +170,31 @@ public class ReactiveTypeHandlerTests { public void jsonArrayOfStrings() throws Exception { // Empty -> null - testJsonPreferred("text/plain", null); - testJsonPreferred("text/plain, application/json", null); - testJsonPreferred("text/markdown", null); - testJsonPreferred("foo/bar", null); + testJsonNotPreferred("text/plain"); + testJsonNotPreferred("text/plain, application/json"); + testJsonNotPreferred("text/markdown"); + testJsonNotPreferred("foo/bar"); // Empty -> List[0] when JSON is preferred - testJsonPreferred("application/json", Collections.emptyList()); - testJsonPreferred("application/foo+json", Collections.emptyList()); - testJsonPreferred("application/json, text/plain", Collections.emptyList()); - testJsonPreferred("*/*, application/json, text/plain", Collections.emptyList()); + testJsonPreferred("application/json"); + testJsonPreferred("application/foo+json"); + testJsonPreferred("application/json, text/plain"); + testJsonPreferred("*/*, application/json, text/plain"); } - private void testJsonPreferred(String acceptHeaderValue, Object expected) throws Exception { + private void testJsonNotPreferred(String acceptHeaderValue) throws Exception { resetRequest(); this.servletRequest.addHeader("Accept", acceptHeaderValue); - MonoProcessor mono = MonoProcessor.create(); - testDeferredResultSubscriber(mono, Mono.class, mono::onComplete, expected); + EmitterProcessor processor = EmitterProcessor.create(); + ResponseBodyEmitter emitter = handleValue(processor, Flux.class); + assertNotNull(emitter); + } + + private void testJsonPreferred(String acceptHeaderValue) throws Exception { + resetRequest(); + this.servletRequest.addHeader("Accept", acceptHeaderValue); + EmitterProcessor processor = EmitterProcessor.create(); + testDeferredResultSubscriber(processor, Flux.class, processor::onComplete, Collections.emptyList()); } @Test @@ -212,8 +213,8 @@ public class ReactiveTypeHandlerTests { testSseResponse(false); // Requested media types are sorted - testJsonPreferred("text/plain;q=0.8, application/json;q=1.0", Collections.emptyList()); - testJsonPreferred("text/plain, application/json", null); + testJsonPreferred("text/plain;q=0.8, application/json;q=1.0"); + testJsonNotPreferred("text/plain, application/json"); } private void testSseResponse(boolean expectSseEimtter) throws Exception {