From 4d3972584b2e3e71d4a84d3f6dda20270d1c4ce8 Mon Sep 17 00:00:00 2001
From: Tim te Beek <tim@moderne.io>
Date: Wed, 26 Jul 2023 17:10:36 +0200
Subject: [PATCH] Ony selectively apply NoRequestMappingAnnotation (#396)

As `@RequestMapping` without `method` matches all methods.
---
 .../spring/NoRequestMappingAnnotation.java    | 31 +++++++++----------
 .../NoRequestMappingAnnotationTest.java       |  8 +++--
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/src/main/java/org/openrewrite/java/spring/NoRequestMappingAnnotation.java b/src/main/java/org/openrewrite/java/spring/NoRequestMappingAnnotation.java
index 32835415..29119b37 100644
--- a/src/main/java/org/openrewrite/java/spring/NoRequestMappingAnnotation.java
+++ b/src/main/java/org/openrewrite/java/spring/NoRequestMappingAnnotation.java
@@ -78,10 +78,13 @@ private static class NoRequestMappingAnnotationVisitor extends JavaIsoVisitor<Ex
         public J.Annotation visitAnnotation(J.Annotation annotation, ExecutionContext ctx) {
             J.Annotation a = super.visitAnnotation(annotation, ctx);
             if (REQUEST_MAPPING_ANNOTATION_MATCHER.matches(a) && getCursor().getParentOrThrow().getValue() instanceof J.MethodDeclaration) {
-
                 Optional<J.Assignment> requestMethodArg = requestMethodArgument(a);
-                Optional<String> requestType = requestMethodArg.isPresent() ? requestMethodArg.flatMap(this::requestMethodType) : Optional.of("GET");
+                Optional<String> requestType = requestMethodArg.map(this::requestMethodType);
                 String resolvedRequestMappingAnnotationClassName = requestType.map(this::associatedRequestMapping).orElse(null);
+                if (resolvedRequestMappingAnnotationClassName == null) {
+                    // Without a method argument @RequestMapping matches all request methods, so we can't safely convert
+                    return a;
+                }
 
                 maybeRemoveImport("org.springframework.web.bind.annotation.RequestMapping");
                 maybeRemoveImport("org.springframework.web.bind.annotation.RequestMethod");
@@ -138,37 +141,31 @@ private boolean methodArgumentHasSingleType(J.Assignment assignment) {
             return newArray.getInitializer() != null && newArray.getInitializer().size() == 1;
         }
 
-        private Optional<String> requestMethodType(@Nullable J.Assignment assignment) {
-            String method;
-            if (assignment == null) {
-                method = "GET";
-            } else if (assignment.getAssignment() instanceof J.Identifier) {
-                method = ((J.Identifier) assignment.getAssignment()).getSimpleName();
+        private String requestMethodType(@Nullable J.Assignment assignment) {
+            if (assignment.getAssignment() instanceof J.Identifier) {
+                return ((J.Identifier) assignment.getAssignment()).getSimpleName();
             } else if (assignment.getAssignment() instanceof J.FieldAccess) {
-                method = ((J.FieldAccess) assignment.getAssignment()).getSimpleName();
+                return ((J.FieldAccess) assignment.getAssignment()).getSimpleName();
             } else if (methodArgumentHasSingleType(assignment)) {
                 J.NewArray newArray = (J.NewArray) assignment.getAssignment();
                 assert newArray.getInitializer() != null;
-                method = ((J.FieldAccess) newArray.getInitializer().get(0)).getSimpleName();
-            } else {
-                method = null;
+                return ((J.FieldAccess) newArray.getInitializer().get(0)).getSimpleName();
             }
-            return Optional.ofNullable(method);
+            return null;
         }
 
         @Nullable
         private String associatedRequestMapping(String method) {
-            String methodName = null;
             switch (method) {
                 case "POST":
                 case "PUT":
                 case "DELETE":
                 case "PATCH":
                 case "GET":
-                    methodName = method.charAt(0) + method.toLowerCase().substring(1) + "Mapping";
-                    break;
+                    return method.charAt(0) + method.toLowerCase().substring(1) + "Mapping";
             }
-            return methodName;
+            // HEAD, OPTIONS, TRACE do not have associated RequestMapping variant
+            return null;
         }
     }
 }
diff --git a/src/testWithSpringBoot_2_1/java/org/openrewrite/java/spring/NoRequestMappingAnnotationTest.java b/src/testWithSpringBoot_2_1/java/org/openrewrite/java/spring/NoRequestMappingAnnotationTest.java
index 72f395e3..7e9d1b43 100644
--- a/src/testWithSpringBoot_2_1/java/org/openrewrite/java/spring/NoRequestMappingAnnotationTest.java
+++ b/src/testWithSpringBoot_2_1/java/org/openrewrite/java/spring/NoRequestMappingAnnotationTest.java
@@ -97,7 +97,7 @@ public ResponseEntity<String> getUser(@PathVariable("id") Long id) {
                       return null;
                   }
 
-                  @GetMapping
+                  @RequestMapping
                   public ResponseEntity<List<String>> getUsersNoRequestMethod() {
                       return null;
                   }
@@ -117,6 +117,7 @@ void postMapping() {
               import org.springframework.http.ResponseEntity;
               import org.springframework.web.bind.annotation.RequestMapping;
               import org.springframework.web.bind.annotation.RestController;
+              import static org.springframework.web.bind.annotation.RequestMethod.POST;
               
               @RestController
               @RequestMapping("/users")
@@ -157,6 +158,7 @@ void removeUnnecessaryAnnotation() {
               import org.springframework.http.ResponseEntity;
               import org.springframework.web.bind.annotation.RequestMapping;
               import org.springframework.web.bind.annotation.RestController;
+              import static org.springframework.web.bind.annotation.RequestMethod.POST;
               
               @RestController
               public class UsersController {
@@ -194,6 +196,7 @@ void hasValueParameter() {
               import java.util.*;
               import org.springframework.http.ResponseEntity;
               import org.springframework.web.bind.annotation.RequestMapping;
+              import org.springframework.web.bind.annotation.RequestMethod;
               
               @RestController
               @RequestMapping("/users")
@@ -296,10 +299,11 @@ void getMappingWithinNestedClass() {
           java(
             """
               import org.springframework.web.bind.annotation.RequestMapping;
+              import org.springframework.web.bind.annotation.RequestMethod;
               
               class Test {
                   class Inner {
-                      @RequestMapping("/api")
+                      @RequestMapping(method = RequestMethod.GET, value = "/api")
                       void test() {
                       }
                   }