From ac0e71745b1de86e293382cce64e94f04e0ab6af Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 21 May 2021 11:54:06 +0200 Subject: [PATCH] Set best matching pattern attribute in WebMvc.fn Prior to this commit, the `RouterFunctionMapping` WebFlux.fn variant would set the `HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE` as an exchange attribute. This is useful for instrumentation purposes. The WebMvc.fn variant would not do the same; this would lead to "UNKNOWN" path metrics tags. This commit ensures that the `RouterFunctionMapping` WebMvc.fn variant does set the `BEST_MATCHING_PATTERN_ATTRIBUTE` and `BEST_MATCHING_HANDLER_ATTRIBUTE` request attributes. Closes gh-26963 --- .../support/RouterFunctionMappingTests.java | 52 ++++++++++++++++--- .../support/RouterFunctionMapping.java | 18 +++++-- .../support/RouterFunctionMappingTests.java | 48 ++++++++++++----- 3 files changed, 93 insertions(+), 25 deletions(-) diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/support/RouterFunctionMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/support/RouterFunctionMappingTests.java index a3f7cfdf9d..a7d5fce5a1 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/support/RouterFunctionMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/support/RouterFunctionMappingTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 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. @@ -21,33 +21,37 @@ import reactor.core.publisher.Mono; import reactor.test.StepVerifier; import org.springframework.http.codec.ServerCodecConfigurer; +import org.springframework.web.reactive.HandlerMapping; import org.springframework.web.reactive.function.server.HandlerFunction; import org.springframework.web.reactive.function.server.RouterFunction; import org.springframework.web.reactive.function.server.RouterFunctions; +import org.springframework.web.reactive.function.server.ServerRequest; import org.springframework.web.reactive.function.server.ServerResponse; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; import org.springframework.web.testfixture.server.MockServerWebExchange; +import org.springframework.web.util.pattern.PathPattern; + +import static org.assertj.core.api.Assertions.assertThat; /** * @author Arjen Poutsma + * @author Brian Clozel */ public class RouterFunctionMappingTests { - private final ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("https://example.com/match")); - private final ServerCodecConfigurer codecConfigurer = ServerCodecConfigurer.create(); @Test - public void normal() { + void normal() { HandlerFunction handlerFunction = request -> ServerResponse.ok().build(); RouterFunction routerFunction = request -> Mono.just(handlerFunction); RouterFunctionMapping mapping = new RouterFunctionMapping(routerFunction); mapping.setMessageReaders(this.codecConfigurer.getReaders()); - Mono result = mapping.getHandler(this.exchange); + Mono result = mapping.getHandler(createExchange("https://example.com/match")); StepVerifier.create(result) .expectNext(handlerFunction) @@ -56,12 +60,12 @@ public class RouterFunctionMappingTests { } @Test - public void noMatch() { + void noMatch() { RouterFunction routerFunction = request -> Mono.empty(); RouterFunctionMapping mapping = new RouterFunctionMapping(routerFunction); mapping.setMessageReaders(this.codecConfigurer.getReaders()); - Mono result = mapping.getHandler(this.exchange); + Mono result = mapping.getHandler(createExchange("https://example.com/match")); StepVerifier.create(result) .expectComplete() @@ -69,7 +73,7 @@ public class RouterFunctionMappingTests { } @Test - public void changeParser() throws Exception { + void changeParser() throws Exception { HandlerFunction handlerFunction = request -> ServerResponse.ok().build(); RouterFunction routerFunction = RouterFunctions.route() .GET("/foo", handlerFunction) @@ -90,4 +94,36 @@ public class RouterFunctionMappingTests { } + @Test + void mappedRequestShouldHoldAttributes() { + HandlerFunction handlerFunction = request -> ServerResponse.ok().build(); + RouterFunction routerFunction = RouterFunctions.route() + .GET("/match", handlerFunction).build(); + + RouterFunctionMapping mapping = new RouterFunctionMapping(routerFunction); + mapping.setMessageReaders(this.codecConfigurer.getReaders()); + + ServerWebExchange exchange = createExchange("https://example.com/match"); + Mono result = mapping.getHandler(exchange); + + StepVerifier.create(result) + .expectNext(handlerFunction) + .expectComplete() + .verify(); + + PathPattern matchingPattern = exchange.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); + assertThat(matchingPattern).isNotNull(); + assertThat(matchingPattern.getPatternString()).isEqualTo("/match"); + + ServerRequest serverRequest = exchange.getAttribute(RouterFunctions.REQUEST_ATTRIBUTE); + assertThat(serverRequest).isNotNull(); + + HandlerFunction handler = exchange.getAttribute(HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE); + assertThat(handler).isEqualTo(handlerFunction); + } + + private ServerWebExchange createExchange(String urlTemplate) { + return MockServerWebExchange.from(MockServerHttpRequest.get(urlTemplate)); + } + } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/function/support/RouterFunctionMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/function/support/RouterFunctionMapping.java index ebaae4460d..59d21f4a70 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/function/support/RouterFunctionMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/function/support/RouterFunctionMapping.java @@ -34,10 +34,12 @@ import org.springframework.http.converter.support.AllEncompassingFormHttpMessage import org.springframework.http.converter.xml.SourceHttpMessageConverter; import org.springframework.lang.Nullable; import org.springframework.util.CollectionUtils; +import org.springframework.web.servlet.function.HandlerFunction; import org.springframework.web.servlet.function.RouterFunction; import org.springframework.web.servlet.function.RouterFunctions; import org.springframework.web.servlet.function.ServerRequest; import org.springframework.web.servlet.handler.AbstractHandlerMapping; +import org.springframework.web.util.pattern.PathPattern; import org.springframework.web.util.pattern.PathPatternParser; /** @@ -50,6 +52,7 @@ import org.springframework.web.util.pattern.PathPatternParser; * * @author Arjen Poutsma * @author Sebastien Deleuze + * @author Brian Clozel * @since 5.2 */ public class RouterFunctionMapping extends AbstractHandlerMapping implements InitializingBean { @@ -206,16 +209,23 @@ public class RouterFunctionMapping extends AbstractHandlerMapping implements Ini protected Object getHandlerInternal(HttpServletRequest servletRequest) throws Exception { if (this.routerFunction != null) { ServerRequest request = ServerRequest.create(servletRequest, this.messageConverters); - setAttributes(servletRequest, request); - return this.routerFunction.route(request).orElse(null); + HandlerFunction handlerFunction = this.routerFunction.route(request).orElse(null); + setAttributes(servletRequest, request, handlerFunction); + return handlerFunction; } else { return null; } } - private void setAttributes(HttpServletRequest servletRequest, ServerRequest request) { - servletRequest.removeAttribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE); + private void setAttributes(HttpServletRequest servletRequest, ServerRequest request, + HandlerFunction handlerFunction) { + PathPattern matchingPattern = (PathPattern) servletRequest.getAttribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE); + if (matchingPattern != null) { + servletRequest.removeAttribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE); + servletRequest.setAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE, matchingPattern.getPatternString()); + } + servletRequest.setAttribute(BEST_MATCHING_HANDLER_ATTRIBUTE, handlerFunction); servletRequest.setAttribute(RouterFunctions.REQUEST_ATTRIBUTE, request); } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/function/support/RouterFunctionMappingTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/function/support/RouterFunctionMappingTests.java index 72a5c528f9..165e346b79 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/function/support/RouterFunctionMappingTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/function/support/RouterFunctionMappingTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 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. @@ -24,6 +24,7 @@ import org.junit.jupiter.api.Test; import org.springframework.http.converter.HttpMessageConverter; import org.springframework.web.servlet.HandlerExecutionChain; +import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.servlet.function.HandlerFunction; import org.springframework.web.servlet.function.RouterFunction; import org.springframework.web.servlet.function.RouterFunctions; @@ -36,43 +37,42 @@ import static org.assertj.core.api.Assertions.assertThat; /** * @author Arjen Poutsma + * @author Brian Clozel */ class RouterFunctionMappingTests { - private final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/match"); - private List> messageConverters = Collections.emptyList(); @Test - public void normal() throws Exception { + void normal() throws Exception { HandlerFunction handlerFunction = request -> ServerResponse.ok().build(); RouterFunction routerFunction = request -> Optional.of(handlerFunction); RouterFunctionMapping mapping = new RouterFunctionMapping(routerFunction); mapping.setMessageConverters(this.messageConverters); - ServletRequestPathUtils.parseAndCache(this.request); - HandlerExecutionChain result = mapping.getHandler(this.request); + MockHttpServletRequest request = createTestRequest("/match"); + HandlerExecutionChain result = mapping.getHandler(request); assertThat(result).isNotNull(); assertThat(result.getHandler()).isSameAs(handlerFunction); } @Test - public void noMatch() throws Exception { + void noMatch() throws Exception { RouterFunction routerFunction = request -> Optional.empty(); RouterFunctionMapping mapping = new RouterFunctionMapping(routerFunction); mapping.setMessageConverters(this.messageConverters); - ServletRequestPathUtils.parseAndCache(this.request); - HandlerExecutionChain result = mapping.getHandler(this.request); + MockHttpServletRequest request = createTestRequest("/match"); + HandlerExecutionChain result = mapping.getHandler(request); assertThat(result).isNull(); } @Test - public void changeParser() throws Exception { + void changeParser() throws Exception { HandlerFunction handlerFunction = request -> ServerResponse.ok().build(); RouterFunction routerFunction = RouterFunctions.route() .GET("/foo", handlerFunction) @@ -86,13 +86,35 @@ class RouterFunctionMappingTests { mapping.setPatternParser(patternParser); mapping.afterPropertiesSet(); - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/FOO"); - ServletRequestPathUtils.parseAndCache(request); - + MockHttpServletRequest request = createTestRequest("/FOO"); HandlerExecutionChain result = mapping.getHandler(request); assertThat(result).isNotNull(); assertThat(result.getHandler()).isSameAs(handlerFunction); } + @Test + void mappedRequestShouldHoldAttributes() throws Exception { + HandlerFunction handlerFunction = request -> ServerResponse.ok().build(); + RouterFunction routerFunction = RouterFunctions.route() + .GET("/match", handlerFunction) + .build(); + + RouterFunctionMapping mapping = new RouterFunctionMapping(routerFunction); + mapping.setMessageConverters(this.messageConverters); + + MockHttpServletRequest request = createTestRequest("/match"); + HandlerExecutionChain result = mapping.getHandler(request); + + assertThat(result).isNotNull(); + assertThat(request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE)).isEqualTo("/match"); + assertThat(request.getAttribute(HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE)).isEqualTo(handlerFunction); + } + + private MockHttpServletRequest createTestRequest(String path) { + MockHttpServletRequest request = new MockHttpServletRequest("GET", path); + ServletRequestPathUtils.parseAndCache(request); + return request; + } + }