RemovalsServletJakarta10 Recipe

[ranuradh]

What's changed?

This PR consists of a Jakarta EE 10 migration recipe - RemovalsServletJakarta10. Below is the screenshot of what the rule does:

Anything in particular you'd like reviewers to focus on? -

Update: This is fixed in the latest
I was able to complete all the migrations except

Here I need to be able to remove one argument using org.openrewrite.java.DeleteMethodArgument and then proceed to follow it up with another recipe. Unfortunately outside the test or within the test if I run , it recursively deletes all arguments and does not provide an option to stop after one cycle. In the recipe I have provided the details

  - org.openrewrite.java.DeleteMethodArgument:
      methodPattern: "B foo(int, int, int)"
      argumentIndex: 1

and the test has

@Test
    void testY() {
        rewriteRun(
          //language=java
          java(b),
          java(
            "public class A {{ B.foo(0, 1, 2); }}",
            "public class A {{ B.foo(0, 2); }}"
          ));
    }

I would like the above result but without using cycles(1).expectedCyclesThatMakeChanges(1) I dont get the result and that wont work for me since I need to proceed and modify the result further.
Please let me know if you have further questions regarding the issue. Once I get past this I will be able to complete this recipe

Anyone you would like to review specifically?

@timtebeek @joanvr

Have you considered any alternatives or workarounds?

Yes

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've added the license header to any new files through ./gradlew licenseFormat
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Not quite sure what's going on, but I can't seem to get the exception test cases to pass, which use a combination of DeleteMethodArgument and ReorderMethodArguments. @joanvr you recently patched DeleteMethodArgument; any idea what might be going on here?

[ranuradh]

Hi @timtebeek when I run the test with my last changes I see it passing -

[timtebeek]

On my machine, and in CI, I'm getting

@@ -5,8 +5,8 @@ 
 class Test {
     void doGet(HttpServletRequest req, HttpServletResponse res) throws Exception {
         jakarta.servlet.Servlet servlet ;
-        UnavailableException unavailableEx1 = new UnavailableException("x", 0);
-        UnavailableException unavailableEx2 = new UnavailableException("x", 0);
+        UnavailableException unavailableEx1 = new UnavailableException(0, "x");
+        UnavailableException unavailableEx2 = new UnavailableException(0, "x");
         UnavailableException unavailableEx3 = new UnavailableException( "x");
     }
 }

I'd like to see this work in CI before a merge; did you refresh your dependencies locally?

[timtebeek]

Also; at some point we might want to update the README footer.

[ranuradh]

Hi @timtebeek will update the readme soon ..also I ran the same test in my branch ..

void testException() {
        rewriteRun(
          //language=java
          java("""
            package com.test;
             
            import java.io.IOException;
  
            import jakarta.servlet.ServletException;            
            import jakarta.servlet.SingleThreadModel;
            import jakarta.servlet.UnavailableException;
            import jakarta.servlet.http.HttpServlet;
            import jakarta.servlet.http.HttpServletRequest;
            import jakarta.servlet.http.HttpServletResponse;         
  
            public class Test {
            
                public void doGet(HttpServletRequest req, HttpServletResponse res) throws ServletException, IOException {
                
                 	   jakarta.servlet.Servlet servlet ;
                 	   UnavailableException unavailableEx2 = new UnavailableException("x",1);     
                 	   UnavailableException unavailableEx1 = new UnavailableException(0, null, "x");     
                 	   UnavailableException unavailableEx1 = new UnavailableException(0, servlet, "x");   
                           UnavailableException unavailableEx1 = new UnavailableException(servlet, "x");   
                 	    
                }
               
            }
            """, """
            package com.test;
             
            import java.io.IOException;
  
            import jakarta.servlet.ServletException;            
            import jakarta.servlet.SingleThreadModel;
            import jakarta.servlet.UnavailableException;
            import jakarta.servlet.http.HttpServlet;
            import jakarta.servlet.http.HttpServletRequest;
            import jakarta.servlet.http.HttpServletResponse;         
  
            public class Test {
            
                public void doGet(HttpServletRequest req, HttpServletResponse res) throws ServletException, IOException {
                
                 	   jakarta.servlet.Servlet servlet ;
                 	   UnavailableException unavailableEx2 = new UnavailableException("x");  
                 	   UnavailableException unavailableEx1 = new UnavailableException("x", 0);
                 	   UnavailableException unavailableEx1 = new UnavailableException("x", 0);    
                 	   UnavailableException unavailableEx1 = new UnavailableException("x");     
                }
               
            }
            """));
    }

and it ran without issues.. I will pull all ur changes next and run to see..

[ranuradh]

@timtebeek seeing the same issues like you ..trying to understand what could have caused this.

[ranuradh]

@timtebeek & @joanvr when I ran the debugger with just this

java(
            """
              import jakarta.servlet.UnavailableException;
              import jakarta.servlet.http.HttpServletRequest;
              import jakarta.servlet.http.HttpServletResponse;
                
              class Test {
                  void doGet(HttpServletRequest req, HttpServletResponse res) throws Exception {
                      jakarta.servlet.Servlet servlet ;
                      UnavailableException unavailableEx1 = new UnavailableException(0, null, "x");
                    
                  }
              }
              """,
            """
              import jakarta.servlet.UnavailableException;
              import jakarta.servlet.http.HttpServletRequest;
              import jakarta.servlet.http.HttpServletResponse;
                
              class Test {
                  void doGet(HttpServletRequest req, HttpServletResponse res) throws Exception {
                      jakarta.servlet.Servlet servlet ;
                      UnavailableException unavailableEx1 = new UnavailableException("x", 0);
                     
                  }
              }

what I noticed is that after calling the DeleteMethodArgument class and getting upto deleting one argument, the test returns and fails instead of going to the next step of ReoderMethodArguments which inturn fails the test. I put breakpoints in ReorderMethodArguments but the code never reached there.

[ranuradh]

@joanvr and @timtebeek could you look at this failure? Thanks!

[timtebeek]

@joanvr and @timtebeek could you look at this failure? Thanks!

Figured out why the reordering of arguments isn't yet working: the visitor we use there does not yet support constructors, only method invocations. I'll see if I can add support there, such that we can use that here.

[timtebeek]

Now waiting for review and merge of

• Support order of constructor arguments rewrite#3710

[joanvr]

I'm going to re-run CI on this PR when it finishes publishing the changes on ReorderMethodArguments

[joanvr]

Now that the CI tests pass, what's the status of this PR? is it ready to review/merge?

[ranuradh]

@joanvr I feel this can be merged as well.. @timtebeek any updates? Thanks!

[timtebeek]

Would be great if we could get these conflicts resolved before a more thorough review and merge.

[timtebeek]

Copying the test failure message here to save folks a click

RemovalsServletJakarta10Test > servletReplacements() FAILED
    java.lang.IllegalStateException: LST contains missing or invalid type information
    MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit

    /*~~(MethodInvocation#name#type is not the same instance as the MethodType of MethodInvocation.)~~>*/req.getContext().getRealPath("")

    MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
    /*~~(MethodInvocation#name#type is not the same instance as the MethodType of MethodInvocation.)~~>*/reqWrapper.getContext().getRealPath("")
        at org.openrewrite.java.Assertions.assertValidTypes(Assertions.java:87)
        at org.openrewrite.java.Assertions.validateTypes(Assertions.java:57)
        at org.openrewrite.java.Assertions$$Lambda$431/0x00007f9978162370.accept(Unknown Source)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:489)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at org.openrewrite.java.migrate.jakarta.RemovalsServletJakarta10Test.servletReplacements(RemovalsServletJakarta10Test.java:36)

This is a new validation we added recently, meaning the element types aren't quite right yet. When run in the debugger you should be able to spot the old types quite quickly to make the necessary changes.

[timtebeek]

The move of the faces recipes complicated the review a bit here, but I think I've resolved all the conflicts correctly now by comparing between jakarta-ee-10.yml and jakarta-faces-4.yml. Let me know if I missed any, but that greatly simplfied the review.

Thanks again!