[GitHub] [skywalking] wu-sheng commented on a change in pull request #5084: #4907 - Duplicate Spring MVC endpoint fix

[GitBox <gi...@apache.org>]

wu-sheng commented on a change in pull request #5084:
URL: https://github.com/apache/skywalking/pull/5084#discussion_r453255696



##########
File path: apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInterceptor.java
##########
@@ -82,11 +82,9 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
             EnhanceRequireObjectCache pathMappingCache = (EnhanceRequireObjectCache) objInst.getSkyWalkingDynamicField();
             String requestURL = pathMappingCache.findPathMapping(method);
             if (requestURL == null) {
-                requestURL = getRequestURL(method);
-                pathMappingCache.addPathMapping(method, requestURL);
-                requestURL = getAcceptedMethodTypes(method) + pathMappingCache.findPathMapping(method);
+                pathMappingCache.addPathMapping(method, getRequestURL(method));
             }
-            operationName = requestURL;
+            operationName = getAcceptedMethodTypes(method) + pathMappingCache.findPathMapping(method);

Review comment:
       > The requestURL is updated after addPathMapping. This means it will use {POST}/url the first time the endpoint is hit and then fall back to /url every time it's hit afterward.
   
   I think you just need move original L86 after L87. The way you changing right now, will make the cache invalid. RIght?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org