CDI recipe #340

ranuradh · 2023-11-07T02:08:55Z

What's changed?

This recipe takes care of the following rules:

2 recipes were added -
UpdateAddAnnotatedType: Here an argument String with value null was added to the return type
UpdateBeanManagerMethod : Removes the deprecated method and substitutes with the right call

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

Anyone you would like to review specifically?

@cjobinabo , @timtebeek

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 IDEA auto-formatter on affected files

src/test/java/org/openrewrite/java/migrate/jakarta/DeprecatedCDIAPIsRemoved40Test.java

src/main/java/org/openrewrite/java/migrate/jakarta/UpdateAddAnnotatedType.java

src/test/java/org/openrewrite/java/migrate/jakarta/DeprecatedCDIAPIsRemoved40Test.java

cjobinabo · 2023-11-07T19:07:53Z

src/test/java/org/openrewrite/java/migrate/jakarta/DeprecatedCDIAPIsRemoved40Test.java

+            		beforeBeanDiscovery.addAnnotatedType(producerType, "my unique id");				// Not this one
+            		beforeBeanDiscovery.addAnnotatedType(String.class, "my other unique id");	// Not this one
+
+            		beanManager.getInjectionTargetFactory().createInjectionTarget(producerType);


getInjectionTargetFactory needs to take in the annotatedType which was previous passed to createInjectionTarget

createInjectionTarget would take in null. Here are some examples I've seen of this code being used https://www.tabnine.com/code/java/methods/javax.enterprise.inject.spi.BeanManager/getInjectionTargetFactory

I couldnt get this to work as well since if the arguments are right I was using the simpleName change and that worked..now we need to add different arguments to the methods as well. I updated the test

Okay I'll look into both updates

src/main/java/org/openrewrite/java/migrate/jakarta/UpdateAddAnnotatedType.java

timtebeek · 2023-11-16T18:17:01Z

What's the status on this PR? I see some unresolved conversations above, and partial in the title, as well as some small formatting deviations.

Let me know when it's ready to review!

cjobinabo · 2023-11-16T20:31:16Z

@timtebeek I see some of the formatting deviations you are referring to. I'll push some of those updates for Anu shortly.

I did leave a question for you about how to handle the removal of Bean.isNullable().
Ex:

if(!myFavoriteBean.isNullable()) {
   // do something;
}

I initially thought we could just update uses of myFavoriteBean.isNullable() to false but quickly realized that a method visitor cannot return a literal. So instead, I proposed updating myFavoriteBean.isNullable() to Boolean.valueOf("false"). I wanted to see if you had a better suggestion.

timtebeek · 2023-11-16T20:43:20Z

@timtebeek I see some of the formatting deviations you are referring to. I'll push some of those updates for Anu shortly.

Thanks for taking those changes on! Helps me focus on the function of the recipes in my review.

I did leave a question for you about how to handle the removal of Bean.isNullable(). Ex:
if(!myFavoriteBean.isNullable()) {
   // do something;
}
I initially thought we could just update uses of myFavoriteBean.isNullable() to false but quickly realized that a method visitor cannot return a literal. So instead, I proposed updating myFavoriteBean.isNullable() to Boolean.valueOf("false"). I wanted to see if you had a better suggestion.

If you implement JavaVisitor instead of JavaIsoVisitor then visitMethodInvocation can return a literal instead, which is probably what you'll want to do here. There's more information on (non-)isomorphic visitors in the docs.

After you replace the method invocation with false we can try to clean up any leftover conditionals with something similar to what I'm doing here. Note that that's not a finished implementation yet, but it could help clear out old code and not be left with if (false) { .. } or if (!false) { ... }

cjobinabo · 2023-11-16T20:49:13Z

If you implement JavaVisitor instead of JavaIsoVisitor then visitMethodInvocation can return a literal instead, which is probably what you'll want to do here. There's more information on (non-)isomorphic visitors in the docs.

This is good to know. I have a meeting coming up in 10 minutes. After that is done I'll look into using JavaVisitor.

cjobinabo · 2023-11-16T22:56:45Z

I've gone ahead and updated the code to return false and then simplified the logic as you recommended. Feel free to suggest any additional changes to the original rule logic that you think make sense. I tried not to deviate too far from the original code that was written.

Description should end with a dot; and reflect what the recipe does. Call super in the normal code path. Remove superfluous nullable annotations. Add missing space in case `false` is retained.

timtebeek · 2023-11-20T13:40:00Z

src/main/resources/META-INF/rewrite/jakarta-ee-10.yml

+  - org.openrewrite.java.migrate.jakarta.UpdateAddAnnotatedType:
+      methodPattern: jakarta.enterprise.inject.spi.BeforeBeanDiscovery addAnnotatedType(jakarta.enterprise.inject.spi.AnnotatedType)


UpdateAddAnnotatedType is only used once, yet the implementation seems to switch between two types of method patterns. Should it be used once more? Or can we simplify the implementation to only support a single case?

@timtebeek I added the javax case as well .Should be there in the final commit.

src/main/java/org/openrewrite/java/migrate/jakarta/UpdateBeanManagerMethod.java

timtebeek · 2023-11-20T13:43:58Z

src/test/java/org/openrewrite/java/migrate/jakarta/DeprecatedCDIAPIsRemoved40Test.java

Rather than one big test to verify three different Java recipes, as well as some yaml configuration, I'd prefer to see a unit test class per Java recipe; That helps make it clear what each recipe contributes, and allows one to jump from the implementation directly to the associated tests rather than having to deduce that from usage in yaml files.

@timtebeek will do that from the next recipe. This one just would up like that

timtebeek · 2023-11-20T13:45:58Z

src/main/java/org/openrewrite/java/migrate/jakarta/UpdateBeanManagerMethod.java

+                    if (type != null) {
+                        type = type.withName(newMethodName);
+                    }
+                    return method.withName(method.getName().withSimpleName(newMethodName)).withMethodType(type);


This won't work well with our type system I think, to have a methodName containing getEvent().fire. Instead we should likely use a JavaTemplate here to create the chain of method invocations.

This was also not addressed.

@timtebeek I tried to update the code with the template..no matter what I tried it keeps failing.Hence I used this way

// maybeRemoveImport("jakarta.enterprise.inject.spi.BeanManager"); // return JavaTemplate.builder("#{any(jakarta.enterprise.inject.spi.BeanManager)}.getEvent().fire(#{any()})") // .build() // .apply(updateCursor(mi), mi.getCoordinates().replace(), mi.getSelect(), mi.getArguments().get(0));

Not sure why the template approach didnt work for getEvent()

src/test/java/org/openrewrite/java/migrate/jakarta/DeprecatedCDIAPIsRemoved40Test.java

timtebeek · 2023-11-27T16:45:49Z

@ranuradh There's still unresolved remarks above; could you respond to and address the earlier feedback?

ranuradh · 2023-11-28T19:35:46Z

@timtebeek I fixed all the concerns other than the template usage for getEvent().

melloware · 2023-12-21T19:38:34Z

Looks like there are still conflicts with this branch?

timtebeek · 2024-01-09T21:15:07Z

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

timtebeek · 2024-01-11T20:44:21Z

The CI logs seem to be filled with stacktraces on classes added in this PR:

org.openrewrite.config.RecipeIntrospectionException: Unable to call primary constructor for Recipe class org.openrewrite.java.migrate.jakarta.UpdateAddAnnotatedType
org.openrewrite.config.RecipeIntrospectionException: Unable to call primary constructor for Recipe class org.openrewrite.java.migrate.jakarta.UpdateBeanManagerMethod

Can we fix those issues?

And has issues with whitespace that we don't want

timtebeek

Thanks for adding this one @ranuradh ! I've pushed changes in the last few commits that would be good to repeat in other PRs

have individual unit tests per Java recipe; this helps flush out validation issues
no need to make recipes configurable if they are specific
don't match on method simple name; use dedicated method matchers instead
use wildcards if you want to match similar classes in both javax and jakarta
minimize your tests to only show the recipe relevant code changes, not any common surrounding context

Hope that helps, and look forward to what you'll produce next! :)

partial cdi recipe

ac34dd2

ranuradh self-assigned this Nov 7, 2023

timtebeek requested a review from cjobinabo November 7, 2023 08:53

timtebeek added the recipe Recipe requested label Nov 7, 2023

completed CDI recipe

6c5e0cd

ranuradh requested a review from timtebeek November 7, 2023 16:17

timtebeek reviewed Nov 7, 2023

View reviewed changes

src/test/java/org/openrewrite/java/migrate/jakarta/DeprecatedCDIAPIsRemoved40Test.java Outdated Show resolved Hide resolved

timtebeek reviewed Nov 7, 2023

View reviewed changes

src/main/java/org/openrewrite/java/migrate/jakarta/UpdateAddAnnotatedType.java Outdated Show resolved Hide resolved

AnuRam123 added 2 commits November 7, 2023 12:08

add back ticks for code

48061f9

remove public class from test classes

e1072c8

cjobinabo reviewed Nov 7, 2023

View reviewed changes

AnuRam123 and others added 4 commits November 8, 2023 13:41

updated tests with isNullable and BeanDiscovery method

730d337

updated recipes based on my comments

954f8b6

Merge branch 'main' into jakarta_ee10_CDI

b624d7c

Merge branch 'main' into jakarta_ee10_CDI

92bbacf

cjobinabo changed the title ~~partial cdi recipe~~ CDI recipe Nov 16, 2023

minor refactor

7ba16f4

convert isNullable to false and simplify logic

cbfe7e7

timtebeek added 2 commits November 20, 2023 11:22

Merge branch 'main' into jakarta_ee10_CDI

19bce8b

Test RemoveBeanIsNullable separately and simplify implementation

1a5524b

Description should end with a dot; and reflect what the recipe does. Call super in the normal code path. Remove superfluous nullable annotations. Add missing space in case `false` is retained.

timtebeek reviewed Nov 20, 2023

View reviewed changes

src/main/java/org/openrewrite/java/migrate/jakarta/UpdateBeanManagerMethod.java Outdated Show resolved Hide resolved

timtebeek reviewed Nov 20, 2023

View reviewed changes

src/test/java/org/openrewrite/java/migrate/jakarta/DeprecatedCDIAPIsRemoved40Test.java Outdated Show resolved Hide resolved

AnuRam123 and others added 5 commits November 28, 2023 11:29

remove annotation and clean up code

3b93dfd

fixes the diff failure.

48914e8

reverting to not use templete in getEvent

4448fe4

fixed test

bce6039

remove template comment

4cccdbf

Merge branch 'main' into jakarta_ee10_CDI

ddd40a4

timtebeek requested review from timtebeek and removed request for joanvr December 27, 2023 10:39

Merge branch 'main' into jakarta_ee10_CDI

fbc45b9

timtebeek and others added 13 commits January 18, 2024 00:15

Merge branch 'main' into jakarta_ee10_CDI

9e26317

Merge branch 'main' into jakarta_ee10_CDI

41e54de

should resolve tests

e8f44e7

Add no args constructors to reduce test log output

360b481

Merge branch 'main' into jakarta_ee10_CDI

2938411

Merge branch 'main' into jakarta_ee10_CDI

1da61b2

Minor polish

701fb68

Add individual unit tests to flush out issues

69b6659

Add individual unit tests to flush out issues

64fb591

Remove DeprecatedCDIAPIsRemoved40Test as it duplicates others

f3f82f9

And has issues with whitespace that we don't want

Drop FacesManagedBeansRemoved that had moved to jakarta-faces-4.yml

83b6afa

Further minimize tests

701e46b

Only keep one copy of classpath resource jar

a466098

timtebeek approved these changes Jan 30, 2024

View reviewed changes

Merge branch 'main' into jakarta_ee10_CDI

65f1728

timtebeek merged commit aa43f7b into openrewrite:main Jan 30, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDI recipe #340

CDI recipe #340

ranuradh commented Nov 7, 2023 •

edited

Loading

cjobinabo Nov 7, 2023

ranuradh Nov 8, 2023

cjobinabo Nov 8, 2023

timtebeek commented Nov 16, 2023

cjobinabo commented Nov 16, 2023 •

edited

Loading

timtebeek commented Nov 16, 2023

cjobinabo commented Nov 16, 2023

cjobinabo commented Nov 16, 2023

timtebeek Nov 20, 2023

ranuradh Nov 28, 2023

timtebeek Nov 20, 2023

ranuradh Nov 28, 2023

timtebeek Nov 20, 2023

timtebeek Nov 27, 2023

ranuradh Nov 28, 2023 •

edited

Loading

timtebeek commented Nov 27, 2023

ranuradh commented Nov 28, 2023

melloware commented Dec 21, 2023

timtebeek commented Jan 9, 2024

timtebeek commented Jan 11, 2024

timtebeek left a comment

		- org.openrewrite.java.migrate.jakarta.UpdateAddAnnotatedType:
		methodPattern: jakarta.enterprise.inject.spi.BeforeBeanDiscovery addAnnotatedType(jakarta.enterprise.inject.spi.AnnotatedType)

CDI recipe #340

CDI recipe #340

Conversation

ranuradh commented Nov 7, 2023 • edited Loading

What's changed?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtebeek commented Nov 16, 2023

cjobinabo commented Nov 16, 2023 • edited Loading

timtebeek commented Nov 16, 2023

cjobinabo commented Nov 16, 2023

cjobinabo commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranuradh Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

timtebeek commented Nov 27, 2023

ranuradh commented Nov 28, 2023

melloware commented Dec 21, 2023

timtebeek commented Jan 9, 2024

timtebeek commented Jan 11, 2024

timtebeek left a comment

Choose a reason for hiding this comment

ranuradh commented Nov 7, 2023 •

edited

Loading

cjobinabo commented Nov 16, 2023 •

edited

Loading

ranuradh Nov 28, 2023 •

edited

Loading