Commit ebb2f70e authored by Brian Clozel's avatar Brian Clozel

Make WebMvgTags use matched patterns for HTTP 404

Prior to this commit, `WebMvcTags' would always mark as "NOT_FOUND" or
"REDIRECTION" *any* exchange with responses of 404 and 3xx status, even
if those responses are actually returned by Controller handlers.

This commit checks inverts those checks and first considers if the
"BEST_MATCHING_PATTERN_ATTRIBUTE" request attribute is present and uses
it - then falls back to "NOT_FOUND" and "REDIRECTION" to avoid
cardinality explosion.

Fixes gh-12577
parent cca5c0d2
...@@ -31,10 +31,16 @@ import org.springframework.web.servlet.HandlerMapping; ...@@ -31,10 +31,16 @@ import org.springframework.web.servlet.HandlerMapping;
* *
* @author Jon Schneider * @author Jon Schneider
* @author Andy Wilkinson * @author Andy Wilkinson
* @author Brian Clozel
* @since 2.0.0 * @since 2.0.0
*/ */
public final class WebMvcTags { public final class WebMvcTags {
private static final Tag URI_NOT_FOUND = Tag.of("uri", "NOT_FOUND");
private static final Tag URI_REDIRECTION = Tag.of("uri", "REDIRECTION");
private WebMvcTags() { private WebMvcTags() {
} }
...@@ -69,21 +75,24 @@ public final class WebMvcTags { ...@@ -69,21 +75,24 @@ public final class WebMvcTags {
* @return the uri tag derived from the request * @return the uri tag derived from the request
*/ */
public static Tag uri(HttpServletRequest request, HttpServletResponse response) { public static Tag uri(HttpServletRequest request, HttpServletResponse response) {
if (response != null) { if (request != null) {
HttpStatus status = extractStatus(response); String pattern = getMatchingPattern(request);
if (status != null && status.is3xxRedirection()) { if (pattern != null) {
return Tag.of("uri", "REDIRECTION"); return Tag.of("uri", pattern);
} }
if (status != null && status.equals(HttpStatus.NOT_FOUND)) { else if (response != null) {
return Tag.of("uri", "NOT_FOUND"); HttpStatus status = extractStatus(response);
if (status != null && status.is3xxRedirection()) {
return URI_REDIRECTION;
}
if (status != null && status.equals(HttpStatus.NOT_FOUND)) {
return URI_NOT_FOUND;
}
} }
String pathInfo = getPathInfo(request);
return Tag.of("uri", pathInfo.isEmpty() ? "root" : pathInfo);
} }
if (request == null) { return Tag.of("uri", "UNKNOWN");
return Tag.of("uri", "UNKNOWN");
}
String uri = getUri(request);
uri = uri.replaceAll("//+", "/").replaceAll("/$", "");
return Tag.of("uri", uri.isEmpty() ? "root" : uri);
} }
private static HttpStatus extractStatus(HttpServletResponse response) { private static HttpStatus extractStatus(HttpServletResponse response) {
...@@ -95,11 +104,16 @@ public final class WebMvcTags { ...@@ -95,11 +104,16 @@ public final class WebMvcTags {
} }
} }
private static String getUri(HttpServletRequest request) { private static String getMatchingPattern(HttpServletRequest request) {
String uri = (String) request return (String) request
.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); .getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE);
uri = (uri != null ? uri : request.getPathInfo()); }
return (StringUtils.hasText(uri) ? uri : "/");
private static String getPathInfo(HttpServletRequest request) {
String uri = StringUtils.hasText(request.getPathInfo()) ?
request.getPathInfo() : "/";
return uri.replaceAll("//+", "/")
.replaceAll("/$", "");
} }
/** /**
......
...@@ -30,6 +30,7 @@ import static org.assertj.core.api.Assertions.assertThat; ...@@ -30,6 +30,7 @@ import static org.assertj.core.api.Assertions.assertThat;
* Tests for {@link WebMvcTags}. * Tests for {@link WebMvcTags}.
* *
* @author Andy Wilkinson * @author Andy Wilkinson
* @author Brian Clozel
*/ */
public class WebMvcTagsTests { public class WebMvcTagsTests {
...@@ -39,9 +40,17 @@ public class WebMvcTagsTests { ...@@ -39,9 +40,17 @@ public class WebMvcTagsTests {
@Test @Test
public void uriTrailingSlashesAreSuppressed() { public void uriTrailingSlashesAreSuppressed() {
this.request.setPathInfo("//spring/");
assertThat(WebMvcTags.uri(this.request, null).getValue()).isEqualTo("/spring");
}
@Test
public void uriTagValueIsBestMatchingPatternWhenAvailable() {
this.request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, this.request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE,
"//foo/"); "/spring");
assertThat(WebMvcTags.uri(this.request, null).getValue()).isEqualTo("/foo"); this.response.setStatus(301);
Tag tag = WebMvcTags.uri(this.request, this.response);
assertThat(tag.getValue()).isEqualTo("/spring");
} }
@Test @Test
...@@ -65,4 +74,9 @@ public class WebMvcTagsTests { ...@@ -65,4 +74,9 @@ public class WebMvcTagsTests {
assertThat(tag.getValue()).isEqualTo("root"); assertThat(tag.getValue()).isEqualTo("root");
} }
@Test
public void uriTagIsUnknownWhenRequestIsNull() {
Tag tag = WebMvcTags.uri(null, null);
assertThat(tag.getValue()).isEqualTo("UNKNOWN");
}
} }
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment