Skip to content

Commit

Permalink
MigrateHandlerInterceptor recipe migrates too much (#622)
Browse files Browse the repository at this point in the history 
* Extend MigrateHandlerInterceptorTest with superclass case

* Improve MigrateHandlerInterceptor with superclass case

* Apply formatter

* Split up test into two sources, to show unchanged clearly

Remove test that did not extend HandlerInterceptorAdapter at all

* Remove temporary workaround not needed for tests

* Apply suggestions from code review

* Improve MigrateHandlerInterceptor with better implementation

* Fix MigrateDatabaseCredentials

* Improve MigrateHandlerInterceptorTest

* Revert copyright date

* Use method matcher instead of string comparisons

---------

Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@jevanlingen @timtebeek
jevanlingen and timtebeek authored Nov 12, 2024
1 parent e50acf3 commit 2cf117d
Show file tree
Hide file tree
Showing 2 changed files with 134 additions and 39 deletions.
72 changes: 38 additions & 34 deletions src/main/java/org/openrewrite/java/spring/framework/MigrateHandlerInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,31 @@
*/
package org.openrewrite.java.spring.framework;

import org.jspecify.annotations.Nullable;
import org.openrewrite.ExecutionContext;
import org.openrewrite.Preconditions;
import org.openrewrite.Recipe;
import org.openrewrite.TreeVisitor;
import org.openrewrite.internal.ListUtils;
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.RemoveImport;
import org.openrewrite.java.JavaTemplate;
import org.openrewrite.java.JavaVisitor;
import org.openrewrite.java.MethodMatcher;
import org.openrewrite.java.search.UsesType;
import org.openrewrite.java.tree.*;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.TypeTree;
import org.openrewrite.java.tree.TypeUtils;

import static java.util.Collections.singletonList;
import static java.util.Objects.requireNonNull;

public class MigrateHandlerInterceptor extends Recipe {

private static final String HANDLER_INTERCEPTOR_ADAPTER = "org.springframework.web.servlet.handler.HandlerInterceptorAdapter";
private static final String HANDLER_INTERCEPTOR_INTERFACE = "org.springframework.web.servlet.HandlerInterceptor";

private static final MethodMatcher PRE_HANDLE = new MethodMatcher("org.springframework.web.servlet.HandlerInterceptor preHandle(..)");
private static final MethodMatcher POST_HANDLE = new MethodMatcher("org.springframework.web.servlet.HandlerInterceptor postHandle(..)");
private static final MethodMatcher AFTER_COMPLETION = new MethodMatcher("org.springframework.web.servlet.HandlerInterceptor afterCompletion(..)");

@Override
public String getDisplayName() {
return "Migrate `HandlerInterceptorAdapter` to `HandlerInterceptor`";
Expand All @@ -40,46 +50,40 @@ public String getDescription() {
return "Deprecated as of 5.3 in favor of implementing `HandlerInterceptor` and/or `AsyncHandlerInterceptor`.";
}

// It will therefore be necessary to use the interfaces of these classes, respectively org.springframework.web.servlet.HandlerInterceptor and org.springframework.web.servlet.config.annotation.WebMvcConfigurer.
//In order to replace these Adapter classes with interfaces you would have to:
//- Edit import
//- Replace extends <className> by implements <interfaceName>
//- Replace super.<MethodName> calls by <InterfaceName>.super.<MethodName>

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(new UsesType<>("org.springframework.web.servlet.handler.HandlerInterceptorAdapter", false), new JavaIsoVisitor<ExecutionContext>() {
return Preconditions.check(new UsesType<>(HANDLER_INTERCEPTOR_ADAPTER, false), new JavaVisitor<ExecutionContext>() {
@Override
public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) {
J.ClassDeclaration cd = super.visitClassDeclaration(classDecl, ctx);
if (classDecl.getExtends() != null && TypeUtils.isOfClassType(classDecl.getExtends().getType(),
"org.springframework.web.servlet.handler.HandlerInterceptorAdapter")) {
cd = cd.withExtends(null);
cd = cd.withImplements(singletonList(TypeTree.build("HandlerInterceptor")
.withType(JavaType.buildType("org.springframework.web.servlet.HandlerInterceptor")))
);
J.ClassDeclaration cd = (J.ClassDeclaration) super.visitClassDeclaration(classDecl, ctx);
if (cd.getExtends() == null || !TypeUtils.isOfClassType(cd.getExtends().getType(), HANDLER_INTERCEPTOR_ADAPTER)) {
return cd;
}

// temporary
cd = cd.getPadding().withImplements(JContainer.withElements(requireNonNull(cd.getPadding().getImplements()),
ListUtils.mapFirst(cd.getPadding().getImplements().getElements(),
anImplements -> anImplements.withPrefix(Space.format(" ")))));
maybeAddImport(HANDLER_INTERCEPTOR_INTERFACE);
maybeRemoveImport(HANDLER_INTERCEPTOR_ADAPTER);

maybeAddImport("org.springframework.web.servlet.HandlerInterceptor");
doAfterVisit(new RemoveImport<>("org.springframework.web.servlet.handler.HandlerInterceptorAdapter", true));
return autoFormat(cd, requireNonNull(cd.getImplements()).get(0), ctx, getCursor().getParentOrThrow());
}
return cd;
TypeTree implments = TypeTree.build("HandlerInterceptor")
.withType(JavaType.buildType(HANDLER_INTERCEPTOR_INTERFACE));
cd = cd.withExtends(null).withImplements(singletonList(implments));
return autoFormat(cd, implments, ctx, getCursor().getParentOrThrow());
}

@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
if (method.getSelect() instanceof J.Identifier) {
if ("super".equals(((J.Identifier) method.getSelect()).getSimpleName())) {
return method.withSelect(TypeTree.build("HandlerInterceptor.super")
.withType(JavaType.buildType("org.springframework.web.servlet.HandlerInterceptor")));
public @Nullable J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
J.MethodInvocation mi = (J.MethodInvocation) super.visitMethodInvocation(method, ctx);
if (mi.getMethodType() != null &&
TypeUtils.isOfClassType(mi.getMethodType().getDeclaringType(), HANDLER_INTERCEPTOR_INTERFACE) &&
mi.getSelect() instanceof J.Identifier && "super".equals(((J.Identifier) mi.getSelect()).getSimpleName())) {
if (PRE_HANDLE.matches(mi)) {
// No need to call super for the hardcoded `true` return value there
return JavaTemplate.apply("true", getCursor(), mi.getCoordinates().replace());
}
if (POST_HANDLE.matches(mi) || AFTER_COMPLETION.matches(mi)) {
return null; // No need to call super for empty methods there
}
}
return super.visitMethodInvocation(method, ctx);
return mi;
}
});
}
Expand Down
101 changes: 96 additions & 5 deletions ...estWithSpringBoot_2_4/java/org/openrewrite/java/spring/MigrateHandlerInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.Issue;
import org.openrewrite.java.JavaParser;
import org.openrewrite.java.spring.framework.MigrateHandlerInterceptor;
import org.openrewrite.test.RecipeSpec;
Expand All @@ -41,25 +42,115 @@ void migrateHandlerInterceptorAdapter() {
java(
"""
import javax.servlet.http.*;
import org.springframework.web.servlet.handler.HandlerInterceptorAdapter;
import org.springframework.web.servlet.ModelAndView;
class MyInterceptor extends HandlerInterceptorAdapter {
@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
return super.preHandle(request, response, handler);
}
@Override
public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView modelAndView) throws Exception {
super.postHandle(request, response, handler, modelAndView);
}
@Override
public void afterCompletion(HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex) throws Exception {
super.afterCompletion(request, response, handler, ex);
}
}
""",
"""
import javax.servlet.http.*;
import org.springframework.web.servlet.HandlerInterceptor;
import org.springframework.web.servlet.ModelAndView;
class MyInterceptor implements HandlerInterceptor {
@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
return HandlerInterceptor.super.preHandle(request, response, handler);
return true;
}
@Override
public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView modelAndView) throws Exception {
}
@Override
public void afterCompletion(HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex) throws Exception {
}
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-spring/issues/620")
@Test
void doesNotReplaceInterceptorsExtendingOwnInterceptors() {
//language=java
rewriteRun(
// Do change classes that directly extend HandlerInterceptorAdapter
java(
"""
import javax.servlet.http.*;
import org.springframework.web.servlet.handler.HandlerInterceptorAdapter;
class MySuperInterceptor extends HandlerInterceptorAdapter {
@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
return super.preHandle(request, response, handler);
}
}
""",
"""
import javax.servlet.http.*;
import org.springframework.web.servlet.HandlerInterceptor;
class MySuperInterceptor implements HandlerInterceptor {
@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
return true;
}
}
"""
),
// But do not change classes that transitively extend HandlerInterceptorAdapter
java(
"""
import javax.servlet.http.*;
import org.springframework.web.servlet.handler.HandlerInterceptorAdapter; // Unused but untouched
class MyInterceptor extends MySuperInterceptor {
@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
return super.preHandle(request, response, handler);
}
}
"""
),
// And do not change any other super call when HandlerInterceptorAdapter is used
java(
"""
import org.springframework.web.servlet.handler.HandlerInterceptorAdapter;
class B extends A {
@Override
public String test() {
new HandlerInterceptorAdapter() {};
return super.test();
}
}
class A {
public String test() {
return "test";
}
}
"""
Expand Down

0 comments on commit 2cf117d

Please sign in to comment.