Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep annotations for merged classes for WebSecurityConfigurerAdapter recipe (Spring Security 5.4) #652

Merged
merged 2 commits into from
Dec 23, 2024
Merged

Keep annotations for merged classes for WebSecurityConfigurerAdapter recipe (Spring Security 5.4) #652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
982aeec
Keep annotations for merged classes
jevanlingen Dec 23, 2024
84e8b0f
Light polish
timtebeek Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 18 additions & 41 deletions src/main/java/org/openrewrite/java/spring/security5/WebSecurityConfigurerAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,7 @@ public class WebSecurityConfigurerAdapter extends Recipe {
private static final String FQN_USER = "org.springframework.security.core.userdetails.User";
private static final String FQN_USER_DETAILS_BUILDER = "org.springframework.security.core.userdetails.User$UserBuilder";
private static final String FQN_USER_DETAILS = "org.springframework.security.core.userdetails.UserDetails";
private static final String BEAN_PKG = "org.springframework.context.annotation";
private static final String BEAN_SIMPLE_NAME = "Bean";
private static final String FQN_BEAN = BEAN_PKG + "." + BEAN_SIMPLE_NAME;
private static final String BEAN_ANNOTATION = "@" + BEAN_SIMPLE_NAME;
private static final String FQN_BEAN = "org.springframework.context.annotation.Bean";

private static final MethodMatcher CONFIGURE_HTTP_SECURITY_METHOD_MATCHER =
new MethodMatcher("org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter configure(org.springframework.security.config.annotation.web.builders.HttpSecurity)", true);
Expand Down Expand Up @@ -144,15 +141,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
}
// only applicable to former subclasses of WebSecurityConfigurerAdapter - other classes won't be flattened
classesToFlatten.add(classDecl);
// Remove imports for annotations being removed together with class declaration
// It is impossible in the general case to tell whether some of these annotations might apply to the bean methods
Copy link
Contributor Author

@jevanlingen jevanlingen Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this comment is true, choosing to not do anything with the annotations is a wrong move. This can lead to bugs, as I have shown in the description of this PR. It's better/safer to copy them and hope for the best 🤞🏻.

// However, a set of hardcoded annotations can be moved in the future
for (J.Annotation a : classDecl.getLeadingAnnotations()) {
JavaType.FullyQualified type = TypeUtils.asFullyQualified(a.getType());
if (type != null) {
maybeRemoveImport(type);
}
}
maybeRemoveImport(FQN_CONFIGURATION);
classDecl = null; // remove class
}
}
Expand Down Expand Up @@ -217,9 +206,12 @@ private J.ClassDeclaration processAnyClass(J.ClassDeclaration classDecl, Executi
continue;
}
String uniqueName = computeBeanNameFromClassName(fc.getSimpleName(), beanType.getClassName());
List<J.Annotation> fcLeadingAnnotations = ListUtils.map(fc.getLeadingAnnotations(),
anno -> TypeUtils.isOfClassType(anno.getType(), FQN_CONFIGURATION) ? null : anno);
s = m
.withName(m.getName().withSimpleName(uniqueName))
.withMethodType(m.getMethodType().withName(uniqueName));
.withMethodType(m.getMethodType().withName(uniqueName))
.withLeadingAnnotations(ListUtils.concatAll(m.getLeadingAnnotations(), fcLeadingAnnotations));
s = autoFormat(s, ctx, new Cursor(getCursor(), classDecl.getBody()));
}
}
Expand Down Expand Up @@ -285,18 +277,12 @@ private J.MethodDeclaration changeToBeanMethod(J.MethodDeclaration m, J.ClassDec
if (type != null) {
type = type.withName(newMethodName).withReturnType(inmemoryAuthConfigType);
if (!keepParams) {
type = type
.withParameterTypes(Collections.emptyList())
.withParameterNames(Collections.emptyList());
for (JavaType pt : type.getParameterTypes()) {
JavaType.FullyQualified fqt = TypeUtils.asFullyQualified(pt);
if (fqt != null) {
maybeRemoveImport(fqt);
}
maybeRemoveImport(TypeUtils.asFullyQualified(pt));
}
type = type.withParameterTypes(Collections.emptyList()).withParameterNames(Collections.emptyList());
}
}

Space returnPrefix = m.getReturnTypeExpression() == null ? Space.EMPTY : m.getReturnTypeExpression().getPrefix();
m = m.withLeadingAnnotations(ListUtils.map(m.getLeadingAnnotations(), anno -> {
if (TypeUtils.isOfClassType(anno.getType(), FQN_OVERRIDE)) {
Expand All @@ -315,9 +301,15 @@ private J.MethodDeclaration changeToBeanMethod(J.MethodDeclaration m, J.ClassDec
}

maybeAddImport(inmemoryAuthConfigType);
// not calling `updateCursor()` here because `visitBlock()` currently requires
// the original to be stored in the cursor
return addBeanAnnotation(m, new Cursor(getCursor().getParentOrThrow(), m));
maybeAddImport(FQN_BEAN);
return JavaTemplate.builder("@Bean")
.imports(FQN_BEAN)
.javaParser(JavaParser.fromJavaVersion().dependsOn(
"package org.springframework.context.annotation;public @interface Bean {}"))
.build()
// not calling `updateCursor()` here because `visitBlock()` currently requires the original to be stored in the cursor
.apply(new Cursor(getCursor().getParentOrThrow(), m),
m.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName)));
}

@Override
Expand Down Expand Up @@ -392,7 +384,6 @@ private J.Block handleWebSecurity(J.Block b, J.MethodDeclaration parentMethod) {

private J.Block handleAuthInMemory(J.Block b, J.MethodDeclaration parentMethod) {
Expression userExpr = findUserParameterExpression(b.getStatements().get(b.getStatements().size() - 1));
JavaType.FullyQualified type = userExpr == null ? null : TypeUtils.asFullyQualified(userExpr.getType());
String typeStr = "";
if (userExpr != null) {
if (userExpr.getType() instanceof JavaType.Primitive) {
Expand Down Expand Up @@ -452,19 +443,6 @@ private J.Block handleAuthInMemory(J.Block b, J.MethodDeclaration parentMethod)
maybeRemoveImport(FQN_AUTH_MANAGER_BUILDER);
return b;
}

private J.MethodDeclaration addBeanAnnotation(J.MethodDeclaration m, Cursor c) {
maybeAddImport(FQN_BEAN);
return JavaTemplate.builder(BEAN_ANNOTATION)
.imports(FQN_BEAN)
.javaParser(JavaParser.fromJavaVersion()
.dependsOn("package " + BEAN_PKG + "; public @interface " + BEAN_SIMPLE_NAME + " {}"))
.build()
.apply(
c,
m.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName))
);
}
});
}

Expand Down Expand Up @@ -535,7 +513,7 @@ private static AuthType getAuthType(J.MethodDeclaration m) {
}

private @Nullable Expression findUserParameterExpression(Statement s) {
AtomicReference<Expression> context = new AtomicReference<>();
AtomicReference<@Nullable Expression> context = new AtomicReference<>();
new JavaIsoVisitor<AtomicReference<Expression>>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, AtomicReference<Expression> ref) {
Expand All @@ -548,5 +526,4 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Atomi
}.visit(s, context);
return context.get();
}

}
Loading
Loading