Java 17 recipe add serialVersionUUID and @Serial by default

[melloware]

What's changed?

Adds serialVersionUUID and @Serial to java classes that implement Serializable as a best practice.

What's your motivation?

Many IDE's like IntelliJ and Eclipse report these issues need to be addressed.

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

[melloware]

The CI/CD log is very confusing on what is failing but doesn't look related to my change?

[timtebeek]

Thanks again @melloware ! Good to have these warnings cleared out.

[melloware]

@timtebeek i just tested this and for some reason its not working. It looks like its looking in the /target folder and not the /src folder for these two recipes? Does that make sense?

[timtebeek]

That would be quite surprising; did you try it with the latest version just released?
https://github.com/openrewrite/rewrite-migrate-java/releases/tag/v2.26.0

[melloware]

Yes I will put together a test tomorrow showing the issue.

[DidierLoiseau]

Hey @melloware and @timtebeek, I just ran the latest rewrite version and realized it was adding serialVersionUID to all our Serializable classes when running the Spring Boot 3.3 migration (since Java 17 upgrade is part of it).

This is absolutely not a safe change, as it will change the value of the (previously generated) serialVersionUID, preventing to read previously serialized objects (if you actually use serialization). I think it’s ok to add @Serial to the existing field (although I don’t know if it’s desirable), but not to add the field.

Moreover, there are a lot of cases in which you use Serializable without actually ever serializing your objects, simply because you inherit from it or some framework requires it (e.g. JPA). Adding an unmaintained serialVersionUID everywhere then just pollutes the code and can lead to more issues the day serialization actually becomes used.

[melloware]

@DidierLoiseau so you are saying it changed

private static final long serialVersionUID = 123456789L;

to

private static final long serialVersionUID = 1L;

??

That would be a bug in the Recipe if so?

Also if you had NO declared serialVersionUID you know the JVM randomly assigns it on each build right? So that wouldn't deserialize for you anyway on a version change would it?

[DidierLoiseau]

It only added it to classes which didn’t have it.

Also if you had NO declared serialVersionUID you know the JVM randomly assigns it on each build right? So that wouldn't deserialize for you anyway on a version change would it?

With this change, it would fail to deserialize WITHOUT version change, since the recipe adds a different value than the one that was previously generated based on the (same) class structure – it is not random by the way.

People tend to add the field just to get rid of the warning, and then they don’t maintain it, which will lead to troubles when the class changes without changing the value. It should only be added when people know what they are doing, not blindly by a Java 17 upgrade recipe (which runs even when your code is already in Java 17, btw). Do no harm 😉

AddSerialVersionUidToSerializable is also unrelated to a specific Java version, so it should not be part of the Java 17 upgrade recipe. It should stay a cleanup recipe that people may want to run, even on older Java versions.

[melloware]

I can live with that argument!! @DidierLoiseau i will submit a PR to pull it out.