Fix spring webmvc latest dep test (#4687)

* Fix spring webmvc latest dep test

* review comments

* remove latest dep version restriction

instrumentation/spring/spring-webmvc-3.1/javaagent/build.gradle.kts

@@ -40,10 +40,6 @@ dependencies {
   testLibrary("org.springframework.boot:spring-boot-starter-web:1.5.17.RELEASE")
   testLibrary("org.springframework.boot:spring-boot-starter-security:1.5.17.RELEASE")
 
-  latestDepTestLibrary("org.springframework.boot:spring-boot-starter-test:2.5.+")
-  latestDepTestLibrary("org.springframework.boot:spring-boot-starter-web:2.5.+")
-  latestDepTestLibrary("org.springframework.boot:spring-boot-starter-security:2.5.+")
-
   testImplementation("org.springframework.security.oauth:spring-security-oauth2:2.0.16.RELEASE")
 
   // For spring security

instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/DispatcherServletInstrumentation.java

@@ -52,8 +52,7 @@ public void transform(TypeTransformer transformer) {
 
   /**
    * This advice creates a filter that has reference to the handlerMappings from DispatcherServlet
-   * which allows the mappings to be evaluated at the beginning of the filter chain. This evaluation
-   * is done inside the Servlet3Decorator.onContext method.
+   * which allows the mappings to be evaluated outside of regular request processing.
    */
   @SuppressWarnings("unused")
   public static class HandlerMappingAdvice {

instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/org/springframework/web/servlet/OpenTelemetryHandlerMappingFilter.java

@@ -12,6 +12,9 @@
 import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.javaagent.instrumentation.springwebmvc.SpringWebMvcServerSpanNaming;
 import java.io.IOException;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
@@ -28,18 +31,39 @@
 import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
 
 public class OpenTelemetryHandlerMappingFilter implements Filter, Ordered {
+  private static final String PATH_ATTRIBUTE = getRequestPathAttribute();
+  private static final MethodHandle usesPathPatternsMh = getUsesPathPatternsMh();
+  private static final MethodHandle parseAndCacheMh = parseAndCacheMh();
 
   private final ServerSpanNameSupplier<HttpServletRequest> serverSpanName =
       (context, request) -> {
-        if (findMapping(request)) {
-          // Name the parent span based on the matching pattern
-          // Let the parent span resource name be set with the attribute set in findMapping.
-          return SpringWebMvcServerSpanNaming.SERVER_SPAN_NAME.get(context, request);
+        Object previousValue = null;
+        if (this.parseRequestPath && PATH_ATTRIBUTE != null) {
+          previousValue = request.getAttribute(PATH_ATTRIBUTE);
+          // sets new value for PATH_ATTRIBUTE of request
+          parseAndCache(request);
+        }
+        try {
+          if (findMapping(request)) {
+            // Name the parent span based on the matching pattern
+            // Let the parent span resource name be set with the attribute set in findMapping.
+            return SpringWebMvcServerSpanNaming.SERVER_SPAN_NAME.get(context, request);
+          }
+        } finally {
+          // mimic spring DispatcherServlet and restore the previous value of PATH_ATTRIBUTE
+          if (this.parseRequestPath && PATH_ATTRIBUTE != null) {
+            if (previousValue == null) {
+              request.removeAttribute(PATH_ATTRIBUTE);
+            } else {
+              request.setAttribute(PATH_ATTRIBUTE, previousValue);
+            }
+          }
         }
         return null;
       };
 
-  @Nullable private volatile List<HandlerMapping> handlerMappings;
+  @Nullable private List<HandlerMapping> handlerMappings;
+  private boolean parseRequestPath;
 
   @Override
   public void init(FilterConfig filterConfig) {}
@@ -90,31 +114,16 @@ private boolean findMapping(HttpServletRequest request) {
   public void setHandlerMappings(List<HandlerMapping> mappings) {
     List<HandlerMapping> handlerMappings = new ArrayList<>();
     for (HandlerMapping mapping : mappings) {
-      // it may be enticing to add all HandlerMapping classes here, but DO NOT
-      //
-      // because we call getHandler() on them above, at the very beginning of the request
-      // and this can be a very invasive call with application-crashing side-effects
-      //
-      // for example: org.grails.web.mapping.mvc.UrlMappingsHandlerMapping.getHandler()
-      // 1. uses GrailsWebRequest.lookup() to get GrailsWebRequest bound to thread local
-      // 2. and populates the servlet request attribute "org.grails.url.match.info"
-      //    with GrailsControllerUrlMappingInfo
-      //
-      // which causes big problems if GrailsWebRequest thread local is leaked from prior request
-      // (which has been observed to happen in Grails 3.0.17 at least), because then our call to
-      // UrlMappingsHandlerMapping.getHandler() at the very beginning of the request:
-      // 1. GrailsWebRequest.lookup() gets the leaked GrailsWebRequest
-      // 2. servlet request attribute "org.grails.url.match.info" is populated based on this leaked
-      //    GrailsWebRequest (so in other words, most likely the wrong route is matched)
-      //
-      // and then GrailsWebRequestFilter creates a new GrailsWebRequest and binds it to the thread
-      //
-      // and then the application calls UrlMappingsHandlerMapping.getHandler() to route the request
-      // but it finds servlet request attribute "org.grails.url.match.info" already populated (by
-      // above) and so it short cuts the matching process and uses the wrong route that the agent
-      // populated caused to be populated into the request attribute above
+      // Originally we ran findMapping at the very beginning of the request. This turned out to have
+      // application-crashing side-effects with grails. That is why we don't add all HandlerMapping
+      // classes here. Although now that we run findMapping after the request, and only when server
+      // span name has not been updated by a controller, the probability of bad side-effects is much
+      // reduced even if we did add all HandlerMapping classes here.
       if (mapping instanceof RequestMappingHandlerMapping) {
         handlerMappings.add(mapping);
+        if (usePathPatterns(mapping)) {
+          this.parseRequestPath = true;
+        }
       }
     }
     if (!handlerMappings.isEmpty()) {
@@ -127,4 +136,68 @@ public int getOrder() {
     // Run after all HIGHEST_PRECEDENCE items
     return Ordered.HIGHEST_PRECEDENCE + 1;
   }
+
+  private static MethodHandle getUsesPathPatternsMh() {
+    // Method added in spring 5.3
+    try {
+      return MethodHandles.lookup()
+          .findVirtual(
+              HandlerMapping.class, "usesPathPatterns", MethodType.methodType(boolean.class));
+    } catch (NoSuchMethodException | IllegalAccessException exception) {
+      return null;
+    }
+  }
+
+  private static boolean usePathPatterns(HandlerMapping handlerMapping) {
+    if (usesPathPatternsMh == null) {
+      return false;
+    }
+    try {
+      return (boolean) usesPathPatternsMh.invoke(handlerMapping);
+    } catch (Throwable throwable) {
+      throw new IllegalStateException(throwable);
+    }
+  }
+
+  private static MethodHandle parseAndCacheMh() {
+    // ServletRequestPathUtils added in spring 5.3
+    try {
+      Class<?> pathUtilsClass =
+          Class.forName("org.springframework.web.util.ServletRequestPathUtils");
+      Class<?> requestPathClass = Class.forName("org.springframework.http.server.RequestPath");
+      return MethodHandles.lookup()
+          .findStatic(
+              pathUtilsClass,
+              "parseAndCache",
+              MethodType.methodType(requestPathClass, HttpServletRequest.class));
+    } catch (ClassNotFoundException | NoSuchMethodException | IllegalAccessException exception) {
+      return null;
+    }
+  }
+
+  private static void parseAndCache(HttpServletRequest request) {
+    if (parseAndCacheMh == null) {
+      return;
+    }
+    try {
+      parseAndCacheMh.invoke(request);
+    } catch (Throwable throwable) {
+      throw new IllegalStateException(throwable);
+    }
+  }
+
+  private static String getRequestPathAttribute() {
+    try {
+      Class<?> pathUtilsClass =
+          Class.forName("org.springframework.web.util.ServletRequestPathUtils");
+      return (String)
+          MethodHandles.lookup()
+              .findStaticGetter(pathUtilsClass, "PATH_ATTRIBUTE", String.class)
+              .invoke();
+    } catch (ClassNotFoundException | NoSuchFieldException | IllegalAccessException exception) {
+      return null;
+    } catch (Throwable throwable) {
+      throw new IllegalStateException(throwable);
+    }
+  }
 }