From 69c22482d10a9e658d04ea9bd56a659c17406eda Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Fri, 5 May 2017 09:22:23 +0100 Subject: [PATCH] Add more subtle content negotiation in web layer So that single Strings can be POSTed without JSON conversion. There's still some work to do to support single POJOs in JSON, and to reach parity with the WebFlux reactive type handlers, but it's now closer to what we had before we moved the String conversion out of the function layer. --- ...ntextFunctionCatalogAutoConfiguration.java | 24 +++++-- .../com/example/SampleApplicationTests.java | 4 +- .../com/example/SampleApplicationTests.java | 1 + .../web/flux/ReactorAutoConfiguration.java | 4 +- .../FluxHandlerMethodArgumentResolver.java | 29 +++++++-- .../flux/response/FluxReturnValueHandler.java | 62 +++++++++++++++---- .../function/mvc/MvcRestApplicationTests.java | 5 -- .../function/web/RestApplicationTests.java | 37 +++++++---- 8 files changed, 124 insertions(+), 42 deletions(-) diff --git a/spring-cloud-function-context/src/main/java/org/springframework/cloud/function/context/ContextFunctionCatalogAutoConfiguration.java b/spring-cloud-function-context/src/main/java/org/springframework/cloud/function/context/ContextFunctionCatalogAutoConfiguration.java index 3595fd3c5..8f191b63b 100644 --- a/spring-cloud-function-context/src/main/java/org/springframework/cloud/function/context/ContextFunctionCatalogAutoConfiguration.java +++ b/spring-cloud-function-context/src/main/java/org/springframework/cloud/function/context/ContextFunctionCatalogAutoConfiguration.java @@ -396,13 +396,19 @@ public class ContextFunctionCatalogAutoConfiguration { String.class))); } - private Class findType(AbstractBeanDefinition definition, int index) { + private Class findType(AbstractBeanDefinition definition, ParamType paramType) { Object source = definition.getSource(); Type param; + // Start by assuming output -> Function + int index = paramType==ParamType.OUTPUT ? 1 : 0; if (source instanceof StandardMethodMetadata) { ParameterizedType type; type = (ParameterizedType) ((StandardMethodMetadata) source) .getIntrospectedMethod().getGenericReturnType(); + if (type.getActualTypeArguments().length==1) { + // There's only one + index = 0; + } Type typeArgumentAtIndex = type.getActualTypeArguments()[index]; if (typeArgumentAtIndex instanceof ParameterizedType) { param = ((ParameterizedType) typeArgumentAtIndex) @@ -441,7 +447,10 @@ public class ContextFunctionCatalogAutoConfiguration { if (resolvable != null) { param = resolvable.getGeneric(index).getGeneric(0).getType(); } - else return Object.class; + else { + // TODO: compiled functions only work as String -> String + return String.class; + } } if (param instanceof ParameterizedType) { ParameterizedType concrete = (ParameterizedType) param; @@ -461,14 +470,19 @@ public class ContextFunctionCatalogAutoConfiguration { if (!registry.containsBeanDefinition(name)) { return Object.class; } - return findType((AbstractBeanDefinition) registry.getBeanDefinition(name), 0); + return findType((AbstractBeanDefinition) registry.getBeanDefinition(name), ParamType.INPUT); } private Class findOutputType(String name) { - if (!registry.containsBeanDefinition(name)) { + if (name==null || !registry.containsBeanDefinition(name)) { return Object.class; } - return findType((AbstractBeanDefinition) registry.getBeanDefinition(name), 1); + BeanDefinition definition = registry.getBeanDefinition(name); + return findType((AbstractBeanDefinition) definition, ParamType.OUTPUT); + } + + static enum ParamType { + INPUT, OUTPUT; } } } diff --git a/spring-cloud-function-samples/spring-cloud-function-sample-compiler/src/test/java/com/example/SampleApplicationTests.java b/spring-cloud-function-samples/spring-cloud-function-sample-compiler/src/test/java/com/example/SampleApplicationTests.java index 3744500c8..1bfd3d432 100644 --- a/spring-cloud-function-samples/spring-cloud-function-sample-compiler/src/test/java/com/example/SampleApplicationTests.java +++ b/spring-cloud-function-samples/spring-cloud-function-sample-compiler/src/test/java/com/example/SampleApplicationTests.java @@ -41,8 +41,8 @@ public class SampleApplicationTests { @Test public void lowercase() { assertThat(new TestRestTemplate().postForObject( - "http://localhost:" + port + "/test", "[\"it works\"]", - String.class)).isEqualTo("[\"it works!!!\"]"); + "http://localhost:" + port + "/test", "it works", + String.class)).isEqualTo("it works!!!"); } } diff --git a/spring-cloud-function-samples/spring-cloud-function-sample-pojo/src/test/java/com/example/SampleApplicationTests.java b/spring-cloud-function-samples/spring-cloud-function-sample-pojo/src/test/java/com/example/SampleApplicationTests.java index 474e578a9..83a2c7166 100644 --- a/spring-cloud-function-samples/spring-cloud-function-sample-pojo/src/test/java/com/example/SampleApplicationTests.java +++ b/spring-cloud-function-samples/spring-cloud-function-sample-pojo/src/test/java/com/example/SampleApplicationTests.java @@ -46,6 +46,7 @@ public class SampleApplicationTests { @Test public void uppercase() { + // TODO: make this work with a JSON stream as well (like in WebFlux) assertThat(new TestRestTemplate().postForObject( "http://localhost:" + port + "/uppercase", "[{\"value\":\"foo\"}]", String.class)).isEqualTo("[{\"value\":\"FOO\"}]"); diff --git a/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/ReactorAutoConfiguration.java b/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/ReactorAutoConfiguration.java index 80ff896d3..82ed9cb23 100644 --- a/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/ReactorAutoConfiguration.java +++ b/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/ReactorAutoConfiguration.java @@ -65,9 +65,9 @@ public class ReactorAutoConfiguration { @ConditionalOnMissingClass("org.springframework.core.ReactiveAdapter") protected static class FluxReturnValueConfiguration { @Bean - public FluxReturnValueHandler fluxReturnValueHandler( + public FluxReturnValueHandler fluxReturnValueHandler(FunctionInspector inspector, HttpMessageConverters converters) { - return new FluxReturnValueHandler(converters.getConverters()); + return new FluxReturnValueHandler(inspector, converters.getConverters()); } } diff --git a/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/request/FluxHandlerMethodArgumentResolver.java b/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/request/FluxHandlerMethodArgumentResolver.java index 8ece77565..722b1b1e2 100644 --- a/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/request/FluxHandlerMethodArgumentResolver.java +++ b/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/request/FluxHandlerMethodArgumentResolver.java @@ -16,7 +16,9 @@ package org.springframework.cloud.function.web.flux.request; +import java.nio.charset.Charset; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import javax.servlet.http.HttpServletRequest; @@ -26,6 +28,8 @@ import com.fasterxml.jackson.databind.ObjectMapper; import org.springframework.cloud.function.context.FunctionInspector; import org.springframework.core.MethodParameter; import org.springframework.core.Ordered; +import org.springframework.http.MediaType; +import org.springframework.util.StreamUtils; import org.springframework.web.bind.support.WebDataBinderFactory; import org.springframework.web.context.request.NativeWebRequest; import org.springframework.web.method.support.HandlerMethodArgumentResolver; @@ -67,13 +71,30 @@ public class FluxHandlerMethodArgumentResolver if (type == null) { type = Object.class; } - List body = mapper.readValue( - webRequest.getNativeRequest(HttpServletRequest.class).getInputStream(), - mapper.getTypeFactory().constructCollectionLikeType(ArrayList.class, - type)); + List body; + if (isPlainText(webRequest) && CharSequence.class.isAssignableFrom(type)) { + body = Arrays.asList(StreamUtils.copyToString(webRequest + .getNativeRequest(HttpServletRequest.class).getInputStream(), + Charset.forName("UTF-8"))); + } + else { + body = mapper.readValue( + webRequest.getNativeRequest(HttpServletRequest.class) + .getInputStream(), + mapper.getTypeFactory().constructCollectionLikeType(ArrayList.class, + type)); + } return new FluxRequest(body); } + private boolean isPlainText(NativeWebRequest webRequest) { + String value = webRequest.getHeader("Content-Type"); + if (value!=null) { + return MediaType.valueOf(value).isCompatibleWith(MediaType.TEXT_PLAIN); + } + return false; + } + @Override public boolean supportsParameter(MethodParameter parameter) { return FluxRequest.class.isAssignableFrom(parameter.getParameterType()); diff --git a/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/response/FluxReturnValueHandler.java b/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/response/FluxReturnValueHandler.java index 1a8902684..0a0d2b433 100644 --- a/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/response/FluxReturnValueHandler.java +++ b/spring-cloud-function-web/src/main/java/org/springframework/cloud/function/web/flux/response/FluxReturnValueHandler.java @@ -17,12 +17,15 @@ package org.springframework.cloud.function.web.flux.response; import java.time.Duration; +import java.util.Arrays; import java.util.List; import javax.servlet.http.HttpServletResponse; import org.reactivestreams.Publisher; +import org.springframework.cloud.function.context.FunctionInspector; +import org.springframework.cloud.function.web.flux.request.FluxHandlerMethodArgumentResolver; import org.springframework.core.MethodParameter; import org.springframework.core.ResolvableType; import org.springframework.http.MediaType; @@ -49,8 +52,12 @@ public class FluxReturnValueHandler implements AsyncHandlerMethodReturnValueHand private long timeout = 1000L; private static final MediaType EVENT_STREAM = MediaType.valueOf("text/event-stream"); - public FluxReturnValueHandler(List> messageConverters) { - delegate = new ResponseBodyEmitterReturnValueHandler(messageConverters); + private FunctionInspector inspector; + + public FluxReturnValueHandler(FunctionInspector inspector, + List> messageConverters) { + this.inspector = inspector; + this.delegate = new ResponseBodyEmitterReturnValueHandler(messageConverters); } /** @@ -108,23 +115,52 @@ public class FluxReturnValueHandler implements AsyncHandlerMethodReturnValueHand } Publisher flux = (Publisher) adaptFrom; + Object handler = webRequest.getAttribute( + FluxHandlerMethodArgumentResolver.HANDLER, + NativeWebRequest.SCOPE_REQUEST); + Class type = inspector.getOutputType(inspector.getName(handler)); + MediaType mediaType = null; - if (webRequest.getHeader("Accept") != null) { - for (MediaType type : MediaType - .parseMediaTypes(webRequest.getHeader("Accept"))) { - if (!MediaType.ALL.equals(type) - && MediaType.APPLICATION_JSON.isCompatibleWith(type)) { - mediaType = MediaType.APPLICATION_JSON; - break; - } else if (mediaType==null) { - mediaType = type; - } - } + if (isPlainText(webRequest) && CharSequence.class.isAssignableFrom(type)) { + mediaType = MediaType.TEXT_PLAIN; + } else { + mediaType = findMediaType(webRequest); } delegate.handleReturnValue(getEmitter(timeout, flux, mediaType), returnType, mavContainer, webRequest); } + private MediaType findMediaType(NativeWebRequest webRequest) { + List accepts = Arrays.asList(MediaType.ALL); + MediaType mediaType = null; + if (webRequest.getHeader("Accept") != null) { + accepts = MediaType.parseMediaTypes(webRequest.getHeader("Accept")); + for (MediaType accept : accepts) { + if (!MediaType.ALL.equals(accept) + && MediaType.APPLICATION_JSON.isCompatibleWith(accept)) { + mediaType = MediaType.APPLICATION_JSON; + // Prefer JSON if that is acceptable + break; + } + else if (mediaType == null) { + mediaType = accept; + } + } + } + if (mediaType == null) { + mediaType = MediaType.APPLICATION_JSON; + } + return mediaType; + } + + private boolean isPlainText(NativeWebRequest webRequest) { + String value = webRequest.getHeader("Content-Type"); + if (value != null) { + return MediaType.valueOf(value).isCompatibleWith(MediaType.TEXT_PLAIN); + } + return false; + } + private ResponseBodyEmitter getEmitter(Long timeout, Publisher flux, MediaType mediaType) { Publisher exported = flux instanceof Mono ? Mono.from(flux) diff --git a/spring-cloud-function-web/src/test/java/org/springframework/cloud/function/mvc/MvcRestApplicationTests.java b/spring-cloud-function-web/src/test/java/org/springframework/cloud/function/mvc/MvcRestApplicationTests.java index 5cfb5112d..c7b6075d1 100644 --- a/spring-cloud-function-web/src/test/java/org/springframework/cloud/function/mvc/MvcRestApplicationTests.java +++ b/spring-cloud-function-web/src/test/java/org/springframework/cloud/function/mvc/MvcRestApplicationTests.java @@ -296,11 +296,6 @@ public class MvcRestApplicationTests { return Mono.just(id).map(value -> "[" + value.trim().toUpperCase() + "]"); } - @PostMapping("/wrap") - public Flux wrap(@RequestBody Flux flux) { - return flux.log().map(value -> ".." + value + ".."); - } - @GetMapping("/wrap/{id}") public Mono wrapGet(@PathVariable int id) { return Mono.just(id).log().map(value -> ".." + value + ".."); diff --git a/spring-cloud-function-web/src/test/java/org/springframework/cloud/function/web/RestApplicationTests.java b/spring-cloud-function-web/src/test/java/org/springframework/cloud/function/web/RestApplicationTests.java index a26baafc2..42acbd908 100644 --- a/spring-cloud-function-web/src/test/java/org/springframework/cloud/function/web/RestApplicationTests.java +++ b/spring-cloud-function-web/src/test/java/org/springframework/cloud/function/web/RestApplicationTests.java @@ -244,14 +244,29 @@ public class RestApplicationTests { ResponseEntity result = rest.exchange(RequestEntity .post(new URI("/uppercase")).contentType(MediaType.APPLICATION_JSON) .body("[\"foo\",\"bar\"]"), String.class); - assertThat(result.getBody()).isEqualTo("[\"[FOO]\",\"[BAR]\"]"); + assertThat(result.getBody()).isEqualTo("[\"(FOO)\",\"(BAR)\"]"); + } + + @Test + public void uppercaseSingleValue() throws Exception { + ResponseEntity result = rest.exchange(RequestEntity + .post(new URI("/uppercase")).contentType(MediaType.TEXT_PLAIN) + .body("foo"), String.class); + assertThat(result.getBody()).isEqualTo("(FOO)"); + } + + @Test + @Ignore("WebFlux would split the request body into lines: TODO make this work the same") + public void uppercasePlainText() throws Exception { + ResponseEntity result = rest.exchange(RequestEntity + .post(new URI("/uppercase")).contentType(MediaType.TEXT_PLAIN) + .body("foo\nbar"), String.class); + assertThat(result.getBody()).isEqualTo("(FOO)(BAR)"); } @Test public void uppercaseFoos() throws Exception { ResponseEntity result = rest.exchange(RequestEntity - // TODO: does not require a content type header, but the plain MVC version - // does .post(new URI("/upFoos")).contentType(MediaType.APPLICATION_JSON) .body("[{\"value\":\"foo\"},{\"value\":\"bar\"}]"), String.class); assertThat(result.getBody()) @@ -263,7 +278,7 @@ public class RestApplicationTests { ResponseEntity result = rest.exchange(RequestEntity .post(new URI("/bareUppercase")).contentType(MediaType.APPLICATION_JSON) .body("[\"foo\",\"bar\"]"), String.class); - assertThat(result.getBody()).isEqualTo("[\"[FOO]\",\"[BAR]\"]"); + assertThat(result.getBody()).isEqualTo("[\"(FOO)\",\"(BAR)\"]"); } @Test @@ -271,7 +286,7 @@ public class RestApplicationTests { ResponseEntity result = rest.exchange(RequestEntity .post(new URI("/transform")).contentType(MediaType.APPLICATION_JSON) .body("[\"foo\",\"bar\"]"), String.class); - assertThat(result.getBody()).isEqualTo("[\"[FOO]\",\"[BAR]\"]"); + assertThat(result.getBody()).isEqualTo("[\"(FOO)\",\"(BAR)\"]"); } @Test @@ -279,17 +294,17 @@ public class RestApplicationTests { ResponseEntity result = rest.exchange(RequestEntity .post(new URI("/post/more")).contentType(MediaType.APPLICATION_JSON) .body("[\"foo\",\"bar\"]"), String.class); - assertThat(result.getBody()).isEqualTo("[\"[FOO]\",\"[BAR]\"]"); + assertThat(result.getBody()).isEqualTo("[\"(FOO)\",\"(BAR)\"]"); } @Test public void postMoreFoo() { - assertThat(rest.getForObject("/post/more/foo", String.class)).isEqualTo("[FOO]"); + assertThat(rest.getForObject("/post/more/foo", String.class)).isEqualTo("(FOO)"); } @Test public void uppercaseGet() { - assertThat(rest.getForObject("/uppercase/foo", String.class)).isEqualTo("[FOO]"); + assertThat(rest.getForObject("/uppercase/foo", String.class)).isEqualTo("(FOO)"); } @Test @@ -327,7 +342,7 @@ public class RestApplicationTests { assertThat(rest.exchange(RequestEntity.post(new URI("/uppercase")) .accept(EVENT_STREAM).contentType(MediaType.APPLICATION_JSON) .body("[\"foo\",\"bar\"]"), String.class).getBody()) - .isEqualTo(sse("[FOO]", "[BAR]")); + .isEqualTo(sse("(FOO)", "(BAR)")); } private String sse(String... values) { @@ -343,12 +358,12 @@ public class RestApplicationTests { @Bean({ "uppercase", "transform", "post/more" }) public Function, Flux> uppercase() { return flux -> flux.log() - .map(value -> "[" + value.trim().toUpperCase() + "]"); + .map(value -> "(" + value.trim().toUpperCase() + ")"); } @Bean public Function bareUppercase() { - return value -> "[" + value.trim().toUpperCase() + "]"; + return value -> "(" + value.trim().toUpperCase() + ")"; } @Bean