From 8593270b5bedaf1a958e95e95c2883f874a81bec Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Sat, 21 Mar 2020 15:11:50 +0100 Subject: [PATCH] Only remove trailing slash from URI value This commit upgrades the algorithm when trailing slash are to be ignored. Previously a root URI (i.e. "/") would result to to empty string which is an issue for monitoring system that requires tag values to be non empty. If the URI is a single character, the trailing is not applied and "/" is left as is. Closes gh-20536 --- .../web/reactive/server/WebFluxTags.java | 2 +- .../metrics/web/servlet/WebMvcTags.java | 2 +- .../endpoint/web/servlet/WebMvcTagsTests.java | 18 ++++++++++++++-- .../web/reactive/server/WebFluxTagsTests.java | 21 +++++++++++++++++-- 4 files changed, 37 insertions(+), 6 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java index ecbcf6ec70..b0412d3bd9 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java @@ -107,7 +107,7 @@ public final class WebFluxTags { PathPattern pathPattern = exchange.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); if (pathPattern != null) { String patternString = pathPattern.getPatternString(); - if (ignoreTrailingSlash) { + if (ignoreTrailingSlash && patternString.length() > 1) { patternString = TRAILING_SLASH_PATTERN.matcher(patternString).replaceAll(""); } return Tag.of("uri", patternString); diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java index d6b89e809d..4b734847ed 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java @@ -112,7 +112,7 @@ public final class WebMvcTags { if (request != null) { String pattern = getMatchingPattern(request); if (pattern != null) { - if (ignoreTrailingSlash) { + if (ignoreTrailingSlash && pattern.length() > 1) { pattern = TRAILING_SLASH_PATTERN.matcher(pattern).replaceAll(""); } return Tag.of("uri", pattern); diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/WebMvcTagsTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/WebMvcTagsTests.java index 55e27a6b91..44f9f1a5a2 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/WebMvcTagsTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/WebMvcTagsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 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. @@ -52,12 +52,26 @@ class WebMvcTagsTests { @Test void uriTagValueIsBestMatchingPatternWhenAvailable() { - this.request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "/spring"); + this.request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "/spring/"); this.response.setStatus(301); Tag tag = WebMvcTags.uri(this.request, this.response); + assertThat(tag.getValue()).isEqualTo("/spring/"); + } + + @Test + void uriTagValueWithBestMatchingPatternAndIgnoreTrailingSlashRemoveTrailingSlash() { + this.request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "/spring/"); + Tag tag = WebMvcTags.uri(this.request, this.response, true); assertThat(tag.getValue()).isEqualTo("/spring"); } + @Test + void uriTagValueWithBestMatchingPatternAndIgnoreTrailingSlashKeepSingleSlash() { + this.request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "/"); + Tag tag = WebMvcTags.uri(this.request, this.response, true); + assertThat(tag.getValue()).isEqualTo("/"); + } + @Test void uriTagValueIsRootWhenRequestHasNoPatternOrPathInfo() { assertThat(WebMvcTags.uri(this.request, null).getValue()).isEqualTo("root"); diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTagsTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTagsTests.java index 6bc090c6f2..033d4d7932 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTagsTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTagsTests.java @@ -39,12 +39,13 @@ import static org.mockito.Mockito.mock; * @author Brian Clozel * @author Michael McFadyen * @author Madhura Bhave + * @author Stephane Nicoll */ class WebFluxTagsTests { private MockServerWebExchange exchange; - private PathPatternParser parser = new PathPatternParser(); + private final PathPatternParser parser = new PathPatternParser(); @BeforeEach void setup() { @@ -53,12 +54,28 @@ class WebFluxTagsTests { @Test void uriTagValueIsBestMatchingPatternWhenAvailable() { - this.exchange.getAttributes().put(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, this.parser.parse("/spring")); + this.exchange.getAttributes().put(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, + this.parser.parse("/spring/")); this.exchange.getResponse().setStatusCode(HttpStatus.MOVED_PERMANENTLY); Tag tag = WebFluxTags.uri(this.exchange); + assertThat(tag.getValue()).isEqualTo("/spring/"); + } + + @Test + void uriTagValueWithBestMatchingPatternAndIgnoreTrailingSlashRemoveTrailingSlash() { + this.exchange.getAttributes().put(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, + this.parser.parse("/spring/")); + Tag tag = WebFluxTags.uri(this.exchange, true); assertThat(tag.getValue()).isEqualTo("/spring"); } + @Test + void uriTagValueWithBestMatchingPatternAndIgnoreTrailingSlashKeepSingleSlash() { + this.exchange.getAttributes().put(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, this.parser.parse("/")); + Tag tag = WebFluxTags.uri(this.exchange, true); + assertThat(tag.getValue()).isEqualTo("/"); + } + @Test void uriTagValueIsRedirectionWhenResponseStatusIs3xx() { this.exchange.getResponse().setStatusCode(HttpStatus.MOVED_PERMANENTLY);