From f3b78611270ee100ef28577f7d0a36ea805d5681 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Sat, 1 Jun 2024 10:07:04 +0200 Subject: [PATCH] Add missing matching pattern attribute Prior to this commit, we introduced in gh-906 custom `RequestPredicates` for faster and more efficient matching. These custom WebFlux and MVC predicates did not set the matching `PathPattern` as a request attribute for positive matches. This value is used by observability conventions for metrics and traces KeyValues; as a result, observation metadata is missing the "uri" metadata and is using the "UNKNOWN" value instead. This commit adds the missing request attribute in our custom request predicates. Fixes gh-987 --- .../webflux/GraphQlRequestPredicates.java | 4 ++ .../webmvc/GraphQlRequestPredicates.java | 4 ++ .../GraphQlRequestPredicatesTests.java | 40 +++++++++++++++++++ .../webmvc/GraphQlRequestPredicatesTests.java | 40 +++++++++++++++++++ 4 files changed, 88 insertions(+) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/server/webflux/GraphQlRequestPredicates.java b/spring-graphql/src/main/java/org/springframework/graphql/server/webflux/GraphQlRequestPredicates.java index e63f3f69..3f80729b 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/server/webflux/GraphQlRequestPredicates.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/server/webflux/GraphQlRequestPredicates.java @@ -31,6 +31,7 @@ import org.springframework.util.Assert; import org.springframework.util.MimeTypeUtils; import org.springframework.web.cors.reactive.CorsUtils; import org.springframework.web.reactive.function.server.RequestPredicate; +import org.springframework.web.reactive.function.server.RouterFunctions; import org.springframework.web.reactive.function.server.ServerRequest; import org.springframework.web.util.pattern.PathPattern; import org.springframework.web.util.pattern.PathPatternParser; @@ -165,6 +166,9 @@ public final class GraphQlRequestPredicates { PathContainer pathContainer = request.requestPath().pathWithinApplication(); boolean pathMatch = pattern.matches(pathContainer); traceMatch("Pattern", pattern.getPatternString(), request.path(), pathMatch); + if (pathMatch) { + request.attributes().put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, pattern); + } return pathMatch; } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/server/webmvc/GraphQlRequestPredicates.java b/spring-graphql/src/main/java/org/springframework/graphql/server/webmvc/GraphQlRequestPredicates.java index d9b14601..a6667e3a 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/server/webmvc/GraphQlRequestPredicates.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/server/webmvc/GraphQlRequestPredicates.java @@ -31,6 +31,7 @@ import org.springframework.util.Assert; import org.springframework.util.MimeTypeUtils; import org.springframework.web.cors.CorsUtils; import org.springframework.web.servlet.function.RequestPredicate; +import org.springframework.web.servlet.function.RouterFunctions; import org.springframework.web.servlet.function.ServerRequest; import org.springframework.web.util.pattern.PathPattern; import org.springframework.web.util.pattern.PathPatternParser; @@ -165,6 +166,9 @@ public final class GraphQlRequestPredicates { PathContainer pathContainer = request.requestPath().pathWithinApplication(); boolean pathMatch = pattern.matches(pathContainer); traceMatch("Pattern", pattern.getPatternString(), request.path(), pathMatch); + if (pathMatch) { + request.attributes().put(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE, pattern); + } return pathMatch; } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/server/webflux/GraphQlRequestPredicatesTests.java b/spring-graphql/src/test/java/org/springframework/graphql/server/webflux/GraphQlRequestPredicatesTests.java index 132147d9..1f20a1f6 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/server/webflux/GraphQlRequestPredicatesTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/server/webflux/GraphQlRequestPredicatesTests.java @@ -28,8 +28,10 @@ import org.springframework.http.MediaType; import org.springframework.mock.http.server.reactive.MockServerHttpRequest; import org.springframework.mock.web.server.MockServerWebExchange; import org.springframework.web.reactive.function.server.RequestPredicate; +import org.springframework.web.reactive.function.server.RouterFunctions; import org.springframework.web.reactive.function.server.ServerRequest; import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.util.pattern.PathPatternParser; import static org.assertj.core.api.Assertions.assertThat; @@ -107,6 +109,25 @@ class GraphQlRequestPredicatesTests { assertThat(httpPredicate.test(serverRequest)).isFalse(); } + @Test + void shouldSetMatchingPatternAttribute() { + ServerWebExchange exchange = createMatchingHttpExchange(); + ServerRequest serverRequest = ServerRequest.create(exchange, Collections.emptyList()); + httpPredicate.test(serverRequest); + assertThat(serverRequest.attribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE)) + .hasValue(PathPatternParser.defaultInstance.parse("/graphql")); + } + + @Test + void shouldNotSetAttributeWhenNoMatch() { + ServerWebExchange exchange = createMatchingHttpExchange() + .mutate().request(req -> req.path("/invalid")).build(); + ServerRequest serverRequest = ServerRequest.create(exchange, Collections.emptyList()); + httpPredicate.test(serverRequest); + assertThat(serverRequest.attribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE)) + .isEmpty(); + } + private MockServerWebExchange createMatchingHttpExchange() { MockServerHttpRequest request = MockServerHttpRequest.post("/graphql") .contentType(MediaType.APPLICATION_JSON) @@ -172,6 +193,25 @@ class GraphQlRequestPredicatesTests { assertThat(ssePredicate.test(serverRequest)).isFalse(); } + @Test + void shouldSetMatchingPatternAttribute() { + ServerWebExchange exchange = createMatchingSseExchange(); + ServerRequest serverRequest = ServerRequest.create(exchange, Collections.emptyList()); + ssePredicate.test(serverRequest); + assertThat(serverRequest.attribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE)) + .hasValue(PathPatternParser.defaultInstance.parse("/graphql")); + } + + @Test + void shouldNotSetAttributeWhenNoMatch() { + ServerWebExchange exchange = createMatchingSseExchange() + .mutate().request(req -> req.path("/invalid")).build(); + ServerRequest serverRequest = ServerRequest.create(exchange, Collections.emptyList()); + ssePredicate.test(serverRequest); + assertThat(serverRequest.attribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE)) + .isEmpty(); + } + private MockServerWebExchange createMatchingSseExchange() { MockServerHttpRequest request = MockServerHttpRequest.post("/graphql") .contentType(MediaType.APPLICATION_JSON) diff --git a/spring-graphql/src/test/java/org/springframework/graphql/server/webmvc/GraphQlRequestPredicatesTests.java b/spring-graphql/src/test/java/org/springframework/graphql/server/webmvc/GraphQlRequestPredicatesTests.java index 77a04714..fb9171dd 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/server/webmvc/GraphQlRequestPredicatesTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/server/webmvc/GraphQlRequestPredicatesTests.java @@ -25,7 +25,9 @@ import org.junit.jupiter.api.Test; import org.springframework.http.HttpHeaders; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.web.servlet.function.RequestPredicate; +import org.springframework.web.servlet.function.RouterFunctions; import org.springframework.web.servlet.function.ServerRequest; +import org.springframework.web.util.pattern.PathPatternParser; import static org.assertj.core.api.Assertions.assertThat; @@ -99,6 +101,25 @@ class GraphQlRequestPredicatesTests { assertThat(httpPredicate.test(serverRequest)).isFalse(); } + @Test + void shouldSetMatchingPatternAttribute() { + MockHttpServletRequest request = createMatchingHttpRequest(); + ServerRequest serverRequest = ServerRequest.create(request, Collections.emptyList()); + httpPredicate.test(serverRequest); + assertThat(serverRequest.attribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE)) + .hasValue(PathPatternParser.defaultInstance.parse("/graphql")); + } + + @Test + void shouldNotSetAttributeWhenNoMatch() { + MockHttpServletRequest request = createMatchingHttpRequest(); + request.setRequestURI("/invalid"); + ServerRequest serverRequest = ServerRequest.create(request, Collections.emptyList()); + httpPredicate.test(serverRequest); + assertThat(serverRequest.attribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE)) + .isEmpty(); + } + private MockHttpServletRequest createMatchingHttpRequest() { MockHttpServletRequest request = new MockHttpServletRequest("POST", "/graphql"); request.setContentType("application/json"); @@ -163,6 +184,25 @@ class GraphQlRequestPredicatesTests { assertThat(ssePredicate.test(serverRequest)).isFalse(); } + @Test + void shouldSetMatchingPatternAttribute() { + MockHttpServletRequest request = createMatchingSseRequest(); + ServerRequest serverRequest = ServerRequest.create(request, Collections.emptyList()); + ssePredicate.test(serverRequest); + assertThat(serverRequest.attribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE)) + .hasValue(PathPatternParser.defaultInstance.parse("/graphql")); + } + + @Test + void shouldNotSetAttributeWhenNoMatch() { + MockHttpServletRequest request = createMatchingSseRequest(); + request.setRequestURI("/invalid"); + ServerRequest serverRequest = ServerRequest.create(request, Collections.emptyList()); + ssePredicate.test(serverRequest); + assertThat(serverRequest.attribute(RouterFunctions.MATCHING_PATTERN_ATTRIBUTE)) + .isEmpty(); + } + private MockHttpServletRequest createMatchingSseRequest() { MockHttpServletRequest request = new MockHttpServletRequest("POST", "/graphql"); request.addHeader("Content-Type", "application/json");