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.

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

Remove Temporal annotation from attributes to avoid EclipseLink error #420

Merged
merged 10 commits into from
Apr 24, 2024

Conversation

evie-lau
Copy link
Contributor

What's changed?

OpenJPA -> EclipseLink recipe to remove @Temporal annotation on attributes where data does not need to be converted.

What's your motivation?

https://wiki.eclipse.org/EclipseLink/Examples/JPA/Migration/OpenJPA/Mappings#.40Temporal_on_java.sql.Date.2FTime.2FTimestamp_fields

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

@evie-lau evie-lau added the recipe Recipe requested label Feb 20, 2024
@evie-lau evie-lau self-assigned this Feb 20, 2024
import java.sql.Time;
import java.sql.Timestamp;

public class TemporalDates {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you port over the following fields:

  • TemporalType.TIMESTAMP used with java.sql.Date
  • TemporalType.DATE used with java.sql.Timestamp
  • TemporalType.TIME used with java.sql.Timestamp

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think the help we have for this issue is incorrect. It sounds like this recipes only needs to remove annotation for the following combinations:

  • TemporalType.TIMESTAMP and java.sql.Timestamp
  • TemporalType.DATE and java.sql.Date
  • TemporalType.TIME and java.sql.Time

Screenshot 2024-03-07 at 3 27 24 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did notice the rule is overly broad, but the other cases that we remove the annotation would have been an invalid combination in either OpenJPA or EclipseLink anyway. So it would just fix the code error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For clarity, the current implementation handles/fixes all the cases except the ones that require a converter.
Do we want to include the converter in this recipe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not worry about the converter for now. But I'll create an issue to look into it later.

)
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably makes sense to have a test that uses Date, Time, and Timestamp classes from other packages such as java.util.Date, java.security.Timestamp, org.omg.Security.Time to make it clear those are valid cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the Eclipse page, I only see that java.util.Date and java.util.Calendar are allowed. I added a test case with these to show no change occurs.

Copy link
Contributor

@cjobinabo cjobinabo Apr 24, 2024

Choose a reason for hiding this comment

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

Yeah the idea behind the tests using Date, Time, and Timestamp classes from other packages was to make it clear the recipe should only apply to java.sql.Date, java.sql.Timestamp, java.sql.Time. I see I had a typo in my original comment.

Update:
It looks like your allowTemporalOnValidClasses test covers this

@timtebeek timtebeek removed their request for review March 15, 2024 09:37
github-actions[bot]

This comment was marked as resolved.

@cjobinabo cjobinabo merged commit f7dfb14 into openrewrite:main Apr 24, 2024
2 checks passed
@evie-lau evie-lau deleted the temporal branch April 25, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants