Skip to content

Add scope to injected classes #248

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

Jump to bottom
Merged
merged 7 commits into from
Jul 10, 2023
Merged

Add scope to injected classes #248

merged 7 commits into from
Jul 10, 2023

Conversation

m-brophy
Copy link
Contributor

@m-brophy m-brophy commented Jun 29, 2023 •
edited
Loading

What's changed?

A recipe to add scope to an injected class

What's your motivation?

when beans.xml is ignored (eg in Quarkus) or not present (eg from CDI 1.1 onwards), bean injection requires a scope-defining annotation to enable discovery of a bean

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

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

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 auto-formatter on affected files
  • I've updated the documentation (if applicable)

@timtebeek timtebeek added the recipe Recipe requested label Jun 29, 2023
@timtebeek
Copy link
Member

@jdelobel Following on from our earlier discussion, this PR should interest you.

timtebeek and others added 2 commits June 29, 2023 14:44
@timtebeek
Update src/main/java/org/openrewrite/java/migrate/AddScopeToInjectedC…
373039b 
…lass.java
@timtebeek
Extract AnnotateTypesVisitor for reuse
079ffcb 
Use `JavaIsoVisitor` instead of `TreeVisitor`
Use `ScanningRecipe<Set<JavaType.FullyQualified>>` instead of `ScanningRecipe<Accumulator>`
Pull `variableTypeRequiresScope` into scanner visitor
@timtebeek
Copy link
Member

@m-brophy I've just pushed 079ffcb ; hope you agree with those changes! Let me know before we do a final review and merge.

@m-brophy
Copy link
Contributor Author

Hi @timtebeek, thanks for those changes, they look good to me.

@timtebeek
Copy link
Member

Hi @timtebeek, thanks for those changes, they look good to me.

Great! Then I'm only wondering if we should move these classes into the javax pacakge under https://github.com/openrewrite/rewrite-migrate-java/tree/main/src/main/java/org/openrewrite/java/migrate ; what do you think?

@m-brophy
Copy link
Contributor Author

m-brophy commented Jul 3, 2023

Hi @timtebeek, thanks for those changes, they look good to me.

Great! Then I'm only wondering if we should move these classes into the javax pacakge under https://github.com/openrewrite/rewrite-migrate-java/tree/main/src/main/java/org/openrewrite/java/migrate ; what do you think?

I didn't do that initially because I wasn't sure if the changes would only be useful for migrations from javax or if it could be used more generally but I think that's probably the best place for it, yes.

@m-brophy
Copy link
Contributor Author

m-brophy commented Jul 5, 2023

Hi @timtebeek I've refactored the recipe to the javax space

knutwannheden
knutwannheden reviewed Jul 5, 2023
View reviewed changes
src/main/java/org/openrewrite/java/migrate/javax/AddScopeToInjectedClass.java Outdated
return false;
}

for (JavaType.FullyQualified annotation : memberVariable.getAnnotations()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Detail: You can instead declare an AnnotationMatcher for javax.inject.Inject. That would be slightly more OpenRewrite idiomatic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea behind storing the annotation to look for in a Collection was that the recipe might be parameterized in future, allowing to search for more than one annotation at once. However this isn't happening at the moment so it's probably best to code for the now

Copy link
Contributor

Choose a reason for hiding this comment

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

Even then you could have a collection of AnnotationMatchers which you match in a loop.

Copy link
Contributor Author

@m-brophy m-brophy Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm trying to implement this now but I'm struggling a bit because JavaType.Variable getAnnotations() method returns a collection of FullyQualified and when I apply the matcher to that it doesn't compile because it expects J.Annotation. Is there a way to construct a J.Annotation from a FullyQualified? Or will it mean entirely rewriting to use cd.getAnnotations()?

Copy link
Contributor

Choose a reason for hiding this comment

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

AnnotationMatcher has a method called matchesAnnotationOrMetaAnnotation(JavaType.FullyQualified). That is probably what you are looking for, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/main/java/org/openrewrite/java/migrate/javax/AddScopeToInjectedClass.java Outdated
public J.CompilationUnit visitCompilationUnit(J.CompilationUnit compilationUnit, ExecutionContext executionContext) {
J.CompilationUnit cu = super.visitCompilationUnit(compilationUnit, executionContext);
for (J.ClassDeclaration aClass : cu.getClasses()) {
if (injectedTypes.contains(aClass.getType())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that JavaType.Class does not implement hashCode() so you cannot really add these to a HashSet. This could be problematic when the recipe scheduler executes a recipe against a large repository, because then it breaks up the sources into "chunks", which each have their own set of JavaType objects (i.e. not referential equality).

For the same reason it isn't really a good idea to add JavaType objects to the recipe's accumulator in the first place. You should instead get the type's fully qualified name using JavaType.FullyQualified#getFullyQualifiedName() and add these strings to the set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

joanvr
joanvr approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@joanvr joanvr left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Let me know if you are done and I shall merge it :)

@m-brophy
Copy link
Contributor Author

m-brophy commented Jul 10, 2023 via email

@joanvr joanvr merged commit d73534b into openrewrite:main Jul 10, 2023
@joanvr
Copy link
Contributor

joanvr commented Jul 10, 2023

Thank you very much for your contribution! Merged it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knutwannheden knutwannheden knutwannheden left review comments

@timtebeek timtebeek timtebeek approved these changes

+1 more reviewer

@joanvr joanvr joanvr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@m-brophy m-brophy

Labels

recipe Recipe requested

Projects

OpenRewrite
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@m-brophy @timtebeek @joanvr @knutwannheden