diff --git a/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/HandlerMethodArgumentResolver.java b/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/HandlerMethodArgumentResolver.java index a9539b1ae3b..1df3064b1fd 100644 --- a/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/HandlerMethodArgumentResolver.java +++ b/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/HandlerMethodArgumentResolver.java @@ -27,14 +27,18 @@ import org.springframework.http.server.reactive.ServerHttpRequest; */ public interface HandlerMethodArgumentResolver { + boolean supportsParameter(MethodParameter parameter); /** - * The returned Publisher must produce a single value. As Reactive Streams - * does not allow publishing null values, if the value may be {@code null} - * use {@link java.util.Optional#ofNullable(Object)} to wrap it. + * The returned Publisher is expected to produce a single value -- i.e. the + * value to use to invoke the handler method. Any additional values will be + * ignored. + * + *

The publisher may also produce zero values if the argument does not + * resolve to any value which will result in passing {@code null} as the + * argument value. */ Publisher resolveArgument(MethodParameter parameter, ServerHttpRequest request); - } diff --git a/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/InvocableHandlerMethod.java b/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/InvocableHandlerMethod.java index 491ac774ff7..5b89c254e43 100644 --- a/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/InvocableHandlerMethod.java +++ b/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/InvocableHandlerMethod.java @@ -19,21 +19,28 @@ package org.springframework.web.reactive.method; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.ArrayList; -import java.util.Collections; +import java.util.Arrays; import java.util.List; -import java.util.Optional; +import java.util.function.Function; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import java.util.stream.Stream; import org.reactivestreams.Publisher; import reactor.Publishers; import reactor.fn.tuple.Tuple; +import reactor.rx.Streams; import org.springframework.core.DefaultParameterNameDiscoverer; import org.springframework.core.GenericTypeResolver; import org.springframework.core.MethodParameter; import org.springframework.core.ParameterNameDiscoverer; +import org.springframework.core.ResolvableType; import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.util.ObjectUtils; import org.springframework.util.ReflectionUtils; import org.springframework.web.method.HandlerMethod; +import org.springframework.web.reactive.HandlerResult; /** @@ -41,7 +48,12 @@ import org.springframework.web.method.HandlerMethod; */ public class InvocableHandlerMethod extends HandlerMethod { - private List argumentResolvers = new ArrayList<>(); + public static final Publisher NO_ARGS = Publishers.just(new Object[0]); + + private final static Object NO_VALUE = new Object(); + + + private List resolvers = new ArrayList<>(); private ParameterNameDiscoverer parameterNameDiscoverer = new DefaultParameterNameDiscoverer(); @@ -52,179 +64,132 @@ public class InvocableHandlerMethod extends HandlerMethod { public void setHandlerMethodArgumentResolvers(List resolvers) { - this.argumentResolvers.clear(); - this.argumentResolvers.addAll(resolvers); + this.resolvers.clear(); + this.resolvers.addAll(resolvers); } + @Override + protected Method getBridgedMethod() { + return super.getBridgedMethod(); + } - public Publisher invokeForRequest(ServerHttpRequest request, - Object... providedArgs) { - - List> argPublishers = getMethodArguments(request, providedArgs); - Publisher argValues = (!argPublishers.isEmpty() ? - Publishers.zip(argPublishers, this::unwrapOptionalArgValues) : - Publishers.just(new Object[0])); + /** + * + * @param request + * @param providedArgs + * @return Publisher that produces a single HandlerResult or an error signal; + * never throws an exception. + */ + public Publisher invokeForRequest(ServerHttpRequest request, Object... providedArgs) { - return Publishers.map(argValues, args -> { - if (logger.isTraceEnabled()) { - logger.trace("Invoking [" + getBeanType().getSimpleName() + "." + - getMethod().getName() + "] method with arguments " + - Collections.singletonList(argPublishers)); + Publisher argsPublisher = NO_ARGS; + try { + if (!ObjectUtils.isEmpty(getMethodParameters())) { + List> publishers = resolveArguments(request, providedArgs); + argsPublisher = Publishers.zip(publishers, this::initArgs); + argsPublisher = first(argsPublisher); } - Object returnValue = null; + } + catch (Throwable ex) { + return Publishers.error(ex); + } + + return Publishers.concatMap(argsPublisher, args -> { try { - returnValue = doInvoke(args); - if (logger.isTraceEnabled()) { - logger.trace("Method [" + getMethod().getName() + "] returned " + - "[" + returnValue + "]"); - } + Object value = doInvoke(args); + + HandlerMethod handlerMethod = InvocableHandlerMethod.this; + ResolvableType type = ResolvableType.forMethodParameter(handlerMethod.getReturnType()); + HandlerResult handlerResult = new HandlerResult(handlerMethod, value, type); + + return Publishers.just(handlerResult); + } + catch (InvocationTargetException ex) { + return Publishers.error(ex.getTargetException()); } - catch (Exception ex) { - // TODO: how to best handle error inside map? (also wrapping hides original ex) - throw new IllegalStateException(ex); + catch (Throwable ex) { + String s = getInvocationErrorMessage(args); + return Publishers.error(new IllegalStateException(s)); } - return returnValue; }); } - private List> getMethodArguments(ServerHttpRequest request, - Object... providedArgs) { - - MethodParameter[] parameters = getMethodParameters(); - List> valuePublishers = new ArrayList<>(parameters.length); - for (int i = 0; i < parameters.length; i++) { - MethodParameter parameter = parameters[i]; - parameter.initParameterNameDiscovery(this.parameterNameDiscoverer); - GenericTypeResolver.resolveParameterType(parameter, getBean().getClass()); - Object value = resolveProvidedArgument(parameter, providedArgs); - if (value != null) { - valuePublishers.add(Publishers.just(value)); - continue; - } - boolean resolved = false; - for (HandlerMethodArgumentResolver resolver : this.argumentResolvers) { - if (resolver.supportsParameter(parameter)) { + private List> resolveArguments(ServerHttpRequest request, Object... providedArgs) { + return Stream.of(getMethodParameters()) + .map(parameter -> { + parameter.initParameterNameDiscovery(this.parameterNameDiscoverer); + GenericTypeResolver.resolveParameterType(parameter, getBean().getClass()); + if (!ObjectUtils.isEmpty(providedArgs)) { + for (Object providedArg : providedArgs) { + if (parameter.getParameterType().isInstance(providedArg)) { + return Publishers.just(providedArg); + } + } + } + HandlerMethodArgumentResolver resolver = this.resolvers.stream() + .filter(r -> r.supportsParameter(parameter)) + .findFirst() + .orElseThrow(() -> getArgError("No resolver for ", parameter, null)); try { - valuePublishers.add(resolver.resolveArgument(parameter, request)); - resolved = true; - break; + Publisher publisher = resolver.resolveArgument(parameter, request); + publisher = mapError(publisher, ex -> getArgError("Error resolving ", parameter, ex)); + return Streams.wrap(publisher).defaultIfEmpty(NO_VALUE); } catch (Exception ex) { - String msg = buildArgErrorMessage("Error resolving argument", i); - valuePublishers.add(Publishers.error(new IllegalStateException(msg, ex))); - break; + throw getArgError("Error resolving ", parameter, ex); } - } - } - if (!resolved) { - String msg = buildArgErrorMessage("No suitable resolver for argument", i); - valuePublishers.add(Publishers.error(new IllegalStateException(msg))); - break; - } - } - return valuePublishers; + }) + .collect(Collectors.toList()); } - private String buildArgErrorMessage(String message, int index) { - MethodParameter param = getMethodParameters()[index]; - message += " [" + index + "] [type=" + param.getParameterType().getName() + "]"; - return getDetailedErrorMessage(message); + private IllegalStateException getArgError(String message, MethodParameter param, Throwable cause) { + return new IllegalStateException(message + + "argument [" + param.getParameterIndex() + "] " + + "of type [" + param.getParameterType().getName() + "] " + + "on method [" + getBridgedMethod().toGenericString() + "]", cause); } - protected String getDetailedErrorMessage(String message) { - return message + "\n" + "HandlerMethod details: \n" + - "Controller [" + getBeanType().getName() + "]\n" + - "Method [" + getBridgedMethod().toGenericString() + "]\n"; - } - - private Object resolveProvidedArgument(MethodParameter parameter, Object... providedArgs) { - if (providedArgs == null) { - return null; + private Object doInvoke(Object[] args) throws Exception { + if (logger.isTraceEnabled()) { + String target = getBeanType().getSimpleName() + "." + getMethod().getName(); + logger.trace("Invoking [" + target + "] method with arguments " + Arrays.toString(args)); } - for (Object providedArg : providedArgs) { - if (parameter.getParameterType().isInstance(providedArg)) { - return providedArg; - } + ReflectionUtils.makeAccessible(getBridgedMethod()); + Object returnValue = getBridgedMethod().invoke(getBean(), args); + if (logger.isTraceEnabled()) { + String target = getBeanType().getSimpleName() + "." + getMethod().getName(); + logger.trace("Method [" + target + "] returned [" + returnValue + "]"); } - return null; + return returnValue; } - private void unwrapOptionalArgValues(Object[] args) { - for (int i = 0; i < args.length; i++) { - if (args[i] instanceof Optional) { - Optional optional = (Optional) args[i]; - args[i] = optional.isPresent() ? optional.get() : null; - } - } + private String getInvocationErrorMessage(Object[] args) { + String argumentDetails = IntStream.range(0, args.length) + .mapToObj(i -> (args[i] != null ? + "[" + i + "][type=" + args[i].getClass().getName() + "][value=" + args[i] + "]" : + "[" + i + "][null]")) + .collect(Collectors.joining(",", " ", " ")); + return "Failed to invoke controller with resolved arguments:" + argumentDetails + + "on method [" + getBridgedMethod().toGenericString() + "]"; } - private Object[] unwrapOptionalArgValues(Tuple tuple) { - Object[] args = new Object[tuple.size()]; - for (int i = 0; i < tuple.size(); i++) { - args[i] = tuple.get(i); - if (args[i] instanceof Optional) { - Optional optional = (Optional) args[i]; - args[i] = optional.isPresent() ? optional.get() : null; - } - } - return args; + private Object[] initArgs(Tuple tuple) { + return Stream.of(tuple.toArray()).map(o -> o != NO_VALUE ? o : null).toArray(); } - protected Object doInvoke(Object... args) throws Exception { - ReflectionUtils.makeAccessible(getBridgedMethod()); - try { - return getBridgedMethod().invoke(getBean(), args); - } - catch (IllegalArgumentException ex) { - assertTargetBean(getBridgedMethod(), getBean(), args); - throw new IllegalStateException(getInvocationErrorMessage(ex.getMessage(), args), ex); - } - catch (InvocationTargetException ex) { - // Unwrap for HandlerExceptionResolvers ... - Throwable targetException = ex.getTargetException(); - if (targetException instanceof RuntimeException) { - throw (RuntimeException) targetException; - } - else if (targetException instanceof Error) { - throw (Error) targetException; - } - else if (targetException instanceof Exception) { - throw (Exception) targetException; - } - else { - String msg = getInvocationErrorMessage("Failed to invoke controller method", args); - throw new IllegalStateException(msg, targetException); - } - } - } - private void assertTargetBean(Method method, Object targetBean, Object[] args) { - Class methodDeclaringClass = method.getDeclaringClass(); - Class targetBeanClass = targetBean.getClass(); - if (!methodDeclaringClass.isAssignableFrom(targetBeanClass)) { - String msg = "The mapped controller method class '" + methodDeclaringClass.getName() + - "' is not an instance of the actual controller bean instance '" + - targetBeanClass.getName() + "'. If the controller requires proxying " + - "(e.g. due to @Transactional), please use class-based proxying."; - throw new IllegalStateException(getInvocationErrorMessage(msg, args)); - } + private static Publisher first(Publisher source) { + return Publishers.lift(source, (e, subscriber) -> { + subscriber.onNext(e); + subscriber.onComplete(); + }); } - private String getInvocationErrorMessage(String message, Object[] resolvedArgs) { - StringBuilder sb = new StringBuilder(getDetailedErrorMessage(message)); - sb.append("Resolved arguments: \n"); - for (int i=0; i < resolvedArgs.length; i++) { - sb.append("[").append(i).append("] "); - if (resolvedArgs[i] == null) { - sb.append("[null] \n"); - } - else { - sb.append("[type=").append(resolvedArgs[i].getClass().getName()).append("] "); - sb.append("[value=").append(resolvedArgs[i]).append("]\n"); - } - } - return sb.toString(); + private static Publisher mapError(Publisher source, Function function) { + return Publishers.lift(source, null, (throwable, subscriber) -> { + subscriber.onError(function.apply(throwable)); + }, null); } } diff --git a/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/annotation/RequestMappingHandlerAdapter.java b/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/annotation/RequestMappingHandlerAdapter.java index 144bf93ac60..215718afa80 100644 --- a/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/annotation/RequestMappingHandlerAdapter.java +++ b/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/annotation/RequestMappingHandlerAdapter.java @@ -92,10 +92,7 @@ public class RequestMappingHandlerAdapter implements HandlerAdapter, Initializin InvocableHandlerMethod handlerMethod = new InvocableHandlerMethod((HandlerMethod) handler); handlerMethod.setHandlerMethodArgumentResolvers(this.argumentResolvers); - ResolvableType type = ResolvableType.forMethodParameter(handlerMethod.getReturnType()); - - Publisher resultPublisher = handlerMethod.invokeForRequest(request); - return Publishers.map(resultPublisher, result -> new HandlerResult(handlerMethod, result, type)); + return handlerMethod.invokeForRequest(request); } } \ No newline at end of file diff --git a/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/annotation/RequestParamArgumentResolver.java b/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/annotation/RequestParamArgumentResolver.java index 7c26ae4692a..b50c5ebe8c2 100644 --- a/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/annotation/RequestParamArgumentResolver.java +++ b/spring-web-reactive/src/main/java/org/springframework/web/reactive/method/annotation/RequestParamArgumentResolver.java @@ -50,7 +50,7 @@ public class RequestParamArgumentResolver implements HandlerMethodArgumentResolv String name = (annotation.value().length() != 0 ? annotation.value() : param.getParameterName()); UriComponents uriComponents = UriComponentsBuilder.fromUri(request.getURI()).build(); String value = uriComponents.getQueryParams().getFirst(name); - return Publishers.just(Optional.ofNullable(value)); + return (value != null ? Publishers.just(value) : Publishers.empty()); } } diff --git a/spring-web-reactive/src/test/java/org/springframework/web/reactive/method/InvocableHandlerMethodTests.java b/spring-web-reactive/src/test/java/org/springframework/web/reactive/method/InvocableHandlerMethodTests.java new file mode 100644 index 00000000000..8f27b173c37 --- /dev/null +++ b/spring-web-reactive/src/test/java/org/springframework/web/reactive/method/InvocableHandlerMethodTests.java @@ -0,0 +1,236 @@ +/* + * Copyright 2002-2015 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.web.reactive.method; + + +import java.lang.reflect.Method; +import java.net.URI; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.TimeUnit; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.junit.Before; +import org.junit.Test; +import org.reactivestreams.Publisher; +import reactor.Publishers; +import reactor.rx.Streams; +import reactor.rx.action.Signal; + +import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.method.HandlerMethod; +import org.springframework.web.reactive.HandlerResult; +import org.springframework.web.reactive.method.annotation.RequestParamArgumentResolver; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * @author Rossen Stoyanchev + */ +@SuppressWarnings("ThrowableResultOfMethodCallIgnored") +public class InvocableHandlerMethodTests { + + private static Log logger = LogFactory.getLog(InvocableHandlerMethodTests.class); + + + private ServerHttpRequest request; + + + @Before + public void setUp() throws Exception { + this.request = mock(ServerHttpRequest.class); + } + + + @Test + public void noArgsMethod() throws Exception { + InvocableHandlerMethod hm = createHandlerMethod("noArgs"); + + Publisher publisher = hm.invokeForRequest(this.request); + Object value = awaitValue(publisher); + + assertEquals("success", value); + } + + @Test + public void resolveArgToZeroValues() throws Exception { + when(this.request.getURI()).thenReturn(new URI("http://localhost:8080/path")); + InvocableHandlerMethod hm = createHandlerMethod("singleArg", String.class); + hm.setHandlerMethodArgumentResolvers(Collections.singletonList(new RequestParamArgumentResolver())); + + Publisher publisher = hm.invokeForRequest(this.request); + Object value = awaitValue(publisher); + + assertEquals("success:null", value); + } + + @Test + public void resolveArgToOneValue() throws Exception { + InvocableHandlerMethod hm = createHandlerMethod("singleArg", String.class); + addResolver(hm, Publishers.just("value1")); + + Publisher publisher = hm.invokeForRequest(this.request); + Object value = awaitValue(publisher); + + assertEquals("success:value1", value); + } + + @Test + public void resolveArgToMultipleValues() throws Exception { + InvocableHandlerMethod hm = createHandlerMethod("singleArg", String.class); + addResolver(hm, Publishers.from(Arrays.asList("value1", "value2", "value3"))); + + Publisher publisher = hm.invokeForRequest(this.request); + List> signals = awaitSignals(publisher); + + assertEquals("Expected only one value: " + signals.toString(), 2, signals.size()); + assertEquals(Signal.Type.NEXT, signals.get(0).getType()); + assertEquals(Signal.Type.COMPLETE, signals.get(1).getType()); + assertEquals("success:value1", signals.get(0).get().getValue()); + } + + @Test + public void noResolverForArg() throws Exception { + InvocableHandlerMethod hm = createHandlerMethod("singleArg", String.class); + + Publisher publisher = hm.invokeForRequest(this.request); + Throwable ex = awaitErrorSignal(publisher); + + assertEquals(IllegalStateException.class, ex.getClass()); + assertEquals("No resolver for argument [0] of type [java.lang.String] on method " + + "[" + hm.getMethod().toGenericString() + "]", ex.getMessage()); + } + + @Test + public void resolveArgumentWithThrownException() throws Exception { + HandlerMethodArgumentResolver resolver = mock(HandlerMethodArgumentResolver.class); + when(resolver.supportsParameter(any())).thenReturn(true); + when(resolver.resolveArgument(any(), any())).thenThrow(new IllegalStateException("boo")); + + InvocableHandlerMethod hm = createHandlerMethod("singleArg", String.class); + hm.setHandlerMethodArgumentResolvers(Collections.singletonList(resolver)); + + Publisher publisher = hm.invokeForRequest(this.request); + Throwable ex = awaitErrorSignal(publisher); + + assertEquals(IllegalStateException.class, ex.getClass()); + assertEquals("Exception not wrapped with helpful argument details", + "Error resolving argument [0] of type [java.lang.String] on method " + + "[" + hm.getMethod().toGenericString() + "]", ex.getMessage()); + } + + @Test + public void resolveArgumentWithErrorSignal() throws Exception { + InvocableHandlerMethod hm = createHandlerMethod("singleArg", String.class); + addResolver(hm, Publishers.error(new IllegalStateException("boo"))); + + Publisher publisher = hm.invokeForRequest(this.request); + Throwable ex = awaitErrorSignal(publisher); + + assertEquals(IllegalStateException.class, ex.getClass()); + assertEquals("Exception not wrapped with helpful argument details", + "Error resolving argument [0] of type [java.lang.String] on method " + + "[" + hm.getMethod().toGenericString() + "]", ex.getMessage()); + } + + @Test + public void illegalArgumentExceptionIsWrappedWithHelpfulDetails() throws Exception { + InvocableHandlerMethod hm = createHandlerMethod("singleArg", String.class); + addResolver(hm, Publishers.just(1)); + + Publisher publisher = hm.invokeForRequest(this.request); + Throwable ex = awaitErrorSignal(publisher); + + assertEquals(IllegalStateException.class, ex.getClass()); + assertEquals("Failed to invoke controller with resolved arguments: " + + "[0][type=java.lang.Integer][value=1] " + + "on method [" + hm.getMethod().toGenericString() + "]", ex.getMessage()); + } + + @Test + public void invocationTargetExceptionIsUnwrapped() throws Exception { + InvocableHandlerMethod hm = createHandlerMethod("exceptionMethod"); + + Publisher publisher = hm.invokeForRequest(this.request); + Throwable ex = awaitErrorSignal(publisher); + + assertEquals(IllegalStateException.class, ex.getClass()); + assertEquals("boo", ex.getMessage()); + } + + + private InvocableHandlerMethod createHandlerMethod(String methodName, Class... argTypes) throws Exception { + Object controller = new TestController(); + Method method = controller.getClass().getMethod(methodName, argTypes); + return new InvocableHandlerMethod(new HandlerMethod(controller, method)); + } + + private void addResolver(InvocableHandlerMethod handlerMethod, Publisher resolvedValue) { + HandlerMethodArgumentResolver resolver = mock(HandlerMethodArgumentResolver.class); + when(resolver.supportsParameter(any())).thenReturn(true); + when(resolver.resolveArgument(any(), any())).thenReturn(resolvedValue); + handlerMethod.setHandlerMethodArgumentResolvers(Collections.singletonList(resolver)); + } + + private Object awaitValue(Publisher publisher) throws Exception { + Object object = awaitSignal(publisher, Signal.Type.NEXT).get(); + assertEquals(HandlerResult.class, object.getClass()); + return ((HandlerResult) object).getValue(); + } + + private Throwable awaitErrorSignal(Publisher publisher) throws Exception { + return awaitSignal(publisher, Signal.Type.ERROR).getThrowable(); + } + + @SuppressWarnings("unchecked") + private Signal awaitSignal(Publisher publisher, Signal.Type type) throws Exception { + Signal signal = awaitSignals(publisher).get(0); + if (!type.equals(signal.getType()) && signal.isOnError()) { + logger.error("Unexpected error: ", signal.getThrowable()); + } + assertEquals("Unexpected signal: " + signal, type, signal.getType()); + return signal; + } + + private List> awaitSignals(Publisher publisher) throws InterruptedException { + return Streams.wrap(publisher).materialize().toList().await(5, TimeUnit.SECONDS); + } + + + @SuppressWarnings("unused") + private static class TestController { + + public String noArgs() { + return "success"; + } + + public String singleArg(@RequestParam(required=false) String q) { + return "success:" + q; + } + + public void exceptionMethod() { + throw new IllegalStateException("boo"); + } + } + + +}