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

Add @Serial annotation to serialVersionUID not working #344

Open
melloware opened this issue Sep 24, 2024 · 3 comments
Open

Add @Serial annotation to serialVersionUID not working #344

melloware opened this issue Sep 24, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@melloware
Copy link

Using the latest version of this recipe: https://docs.openrewrite.org/recipes/staticanalysis/addserialannotationtoserialversionuid

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-static-analysis:RELEASE -Drewrite.activeRecipes=org.openrewrite.staticanalysis.AddSerialAnnotationToserialVersionUID -Drewrite.exportDatatables=true

Just tested that Standalone on two Java projects and it did not add the annotation to any private static final long serialVersionUID = 1L;

Does it do some detection that the pom.xml is already Java14 or higher? Because we are trying to run this as part of the JDK17 Recipe?

@melloware melloware added the bug Something isn't working label Sep 24, 2024
@timtebeek timtebeek moved this to Backlog in OpenRewrite Sep 24, 2024
@timtebeek
Copy link
Contributor

timtebeek commented Sep 24, 2024

hi! The recipe does indeed require that you're running at least Java 17, as determined by a marker that we upgrade as part of a Java version bump. Linking the relevant components here.

return Preconditions.check(
Preconditions.and(
new UsesJavaVersion<>(14),
new UsesType<>("java.io.Serializable", true)

https://github.com/openrewrite/rewrite-migrate-java/blob/1931a6fd3f3999dc20d57b0907d1251447640b07/src/main/java/org/openrewrite/java/migrate/UpgradeJavaVersion.java#L83-L88

Earlier today I'd also added a test to see if we do indeed add the annotation if the field is new: that seems to work: 5c43d94

And updating the Java version is pretty much the first thing we do on any Java migration: https://github.com/openrewrite/rewrite-migrate-java/blob/1931a6fd3f3999dc20d57b0907d1251447640b07/src/main/resources/META-INF/rewrite/java-version-17.yml#L28-L30

@melloware
Copy link
Author

OK test case:

  1. git clone https://github.com/melloware/quarkus-faces
  2. The whole \src\main\java\org\primefaces\showcase\domain has Serializable POJO's missing both serialversionId and @Serial.
  3. Run mvn -U org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-migrate-java:RELEASE -Drewrite.activeRecipes=org.openrewrite.java.migrate.UpgradeToJava17 -Drewrite.exportDatatables=true which is the latest versions of the plugin.

Notice nothing gets touched in the domain package when all of them should have had serialversionID and @Serial added.

@timtebeek
Copy link
Contributor

Thanks for the reproducer steps.. Can't yet look in detail as I'm traveling/PTO for the next week or so, followed by conference. Any help appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

2 participants