Add Transient annotation to private accessor methods #425

evie-lau · 2024-02-22T14:10:15Z

What's changed?

OpenJPA -> EclipseLink recipe
Add @Transient annotation to private accessor methods

What's your motivation?

https://wiki.eclipse.org/EclipseLink/Examples/JPA/Migration/OpenJPA/Mappings#Private_Accessor_Methods

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

Question for this recipe, and maybe even all the other EclipseLink migration recipes...
Do all the JPA migration recipes require the class being tagged with @Entity or some JPA annotation? @cjobinabo

Checklist

I've added unit tests to cover both positive and negative cases
I've read and applied the recipe conventions and best practices
I've used the IntelliJ IDEA auto-formatter on affected files

src/main/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessor.java

timtebeek · 2024-02-24T13:55:35Z

src/main/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessor.java

+                     */
+                    private boolean methodReturnsFieldFromClass(J.Return returnStatement) {
+                        J.ClassDeclaration classDecl = getCursor().dropParentUntil(parent -> parent instanceof J.ClassDeclaration).getValue();
+                        JavaType.Variable returnedVar = ((J.Identifier)returnStatement.getExpression()).getFieldType();


I'm not sure about matching on the field type here ; I'd expect we'd want to match that the identifier or accessed field name are the same as what's returned.

It seemed like the Identifier sometimes had different object numbers when I was debugging, but getting it's FieldType object always matched up (which also contains the variable name).

the return expression can be a lot of things, but I imagine we'd only want to add an annotation when it's exactly an identifier or field access that references a field in the surrounding class; I think that were we were headed already right?

For visual reference:

The J.Identifier object (name) didn't match up between the class's field Identifier and the method's returned Identifier. simpleName and fieldType were the highest level matches I found, and comparing the objects seemed like a better choice than comparing strings.

…avior

...est/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessorTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

src/main/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessor.java

...est/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessorTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

...est/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessorTest.java

src/main/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessor.java

timtebeek · 2024-05-06T14:49:44Z

...est/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessorTest.java

+    }
+
+    @Test
+    void doNotChangePublicOrProtectedGetter() {


The spec and tests don't seem to mention package private cases. Would we want to make explicit what we do with those in the tests? Right now it seems we only change private cases.

I think since the spec doesn't mention it, it may be best to only stick with the explicit private case.

timtebeek · 2024-05-06T16:32:18Z

src/main/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessor.java

+                                    if (expression instanceof J.FieldAccess) {
+                                        return ((J.FieldAccess) expression).getName().getFieldType();
+                                    } else if (expression instanceof J.Identifier) { // ie: return field;
+                                        return ((J.Identifier) expression).getFieldType();
+                                    } else {
+                                        return null;
+                                    }


In a way we're iterating over returns twice; first as we visit each return, and then again to extract the returned field type; would it make sense to fold those into one by doing this extract in the visitor just above?

Great catch, will update to do that. Also I noticed that I'm collecting the class variables in each method, so I'll extract that to a higher scope variable to be reused.

timtebeek

Approved already, and you're welcome to merge as is; I just had two small suggestions that I'd like your thoughts on, but they are not a requirement to address before we merge.

cjobinabo

Thanks @timtebeek for helping me review this. Thanks @evie-lau for making these updates.

Initial implementation with test

9d0ecdb

evie-lau added the recipe Recipe requested label Feb 22, 2024

evie-lau self-assigned this Feb 22, 2024

evie-lau commented Feb 22, 2024

View reviewed changes

src/main/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessor.java Outdated Show resolved Hide resolved

evie-lau and others added 4 commits February 22, 2024 13:07

Polishing and add another test

7b13dc6

Check if the method return value is a field from the parent class

8cb83a6

Merge branch 'main' into transientGetter

9c611b5

Prevent applying annotation twice; add unhandled case

e196629

timtebeek reviewed Feb 24, 2024

View reviewed changes

timtebeek and others added 2 commits February 24, 2024 14:59

Add test to show annotated field not changed again

932ab06

Handle FieldAccess and Literal returns, add test for Literal

b975a34

evie-lau marked this pull request as ready for review February 26, 2024 17:42

evie-lau requested a review from cjobinabo February 26, 2024 18:37

evie-lau and others added 6 commits February 26, 2024 15:20

Cleanup

a1092f7

Merge branch 'main' into transientGetter

b0f9e2a

Small improvements and comments

17c703c

Merge branch 'main' into transientGetter

c2a51b5

Refactor, add test

58893dd

Handle private accessor in inner class of an entity to match WAMT beh…

bf9dab1

…avior

github-actions bot reviewed Apr 10, 2024

View reviewed changes

...est/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessorTest.java Outdated Show resolved Hide resolved

evie-lau and others added 2 commits April 10, 2024 14:37

Fix test formatting

ef81876

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into transientGetter

8720c86

This comment was marked as resolved.

Sign in to view

Merge branch 'main' into transientGetter

2c72bf8

github-actions bot reviewed Apr 25, 2024

View reviewed changes

src/main/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessor.java Outdated Show resolved Hide resolved

...est/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessorTest.java Outdated Show resolved Hide resolved

timtebeek and others added 2 commits April 26, 2024 11:10

Apply suggestions from code review

32a6cfe

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into transientGetter

f35e327

cjobinabo reviewed May 1, 2024

View reviewed changes

...est/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessorTest.java Show resolved Hide resolved

Add protected getter to test

f42384d

evie-lau commented May 2, 2024

View reviewed changes

src/main/java/org/openrewrite/java/migrate/javax/AddTransientAnnotationToPrivateAccessor.java Outdated Show resolved Hide resolved

evie-lau added 2 commits May 3, 2024 10:11

Check all returns in method for class field variables

d0a9660

Add complex logic test without field access

d6e3b11

evie-lau requested review from cjobinabo and timtebeek May 3, 2024 19:01

timtebeek reviewed May 6, 2024

View reviewed changes

timtebeek approved these changes May 6, 2024

View reviewed changes

evie-lau added 2 commits May 6, 2024 15:30

Refactor for efficiency

3479ecd

Add logic comments

2613275

cjobinabo approved these changes May 6, 2024

View reviewed changes

cjobinabo merged commit bc8e2f7 into openrewrite:main May 6, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Transient annotation to private accessor methods #425

Add Transient annotation to private accessor methods #425

evie-lau commented Feb 22, 2024 •

edited

Loading

timtebeek Feb 24, 2024

evie-lau Feb 26, 2024 •

edited

Loading

timtebeek Feb 26, 2024

evie-lau Feb 26, 2024 •

edited

Loading

This comment was marked as resolved.

timtebeek May 6, 2024

evie-lau May 6, 2024

timtebeek May 6, 2024

evie-lau May 6, 2024 •

edited

Loading

timtebeek left a comment

cjobinabo left a comment

Add Transient annotation to private accessor methods #425

Add Transient annotation to private accessor methods #425

Conversation

evie-lau commented Feb 22, 2024 • edited Loading

What's changed?

What's your motivation?

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

Checklist

timtebeek Feb 24, 2024

Choose a reason for hiding this comment

evie-lau Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

timtebeek Feb 26, 2024

Choose a reason for hiding this comment

evie-lau Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

timtebeek May 6, 2024

Choose a reason for hiding this comment

evie-lau May 6, 2024

Choose a reason for hiding this comment

timtebeek May 6, 2024

Choose a reason for hiding this comment

evie-lau May 6, 2024 • edited Loading

Choose a reason for hiding this comment

timtebeek left a comment

Choose a reason for hiding this comment

cjobinabo left a comment

Choose a reason for hiding this comment

evie-lau commented Feb 22, 2024 •

edited

Loading

evie-lau Feb 26, 2024 •

edited

Loading

evie-lau Feb 26, 2024 •

edited

Loading

evie-lau May 6, 2024 •

edited

Loading