"Serializable" classes should use auto-generated version ids

java

(Jens Bannmann) #1

Hi all,

I would like to propose the following new rule.


Rule description & motivation
Providing a serialVersionUID field in Serializable classes is recommended, but blindly following that recommendation can be harmful.

First, there are applications that do not use serialization for long-term persistent storage, but only for short-term in-memory caching. Also, some classes like exceptions have to implement Serializable.

In these situations, backwards compatibility or interoperability with code compiled by other compilers is not a goal. However, requiring an explicit serialVersionUID makes it quite likely that developers forget to update it when they should. When this happens, incompatible versions of the class will be deserialized without any warning at runtime, leading to bugs later on that are hard to pinpoint.

Not specifying a serialVersionUID can be the safer approach for such applications: the compiler will automatically generate a version, and the serialization runtime will refuse to deserialize an instance if the versions do not match. This gives the developer a hint that e.g. something is wrong with the classpath or different versions of the application use the same cache server.

Impact to keep this code as it is
Specifying a serialVersionUID field poses the risk that it is not updated when it should, which introduces bugs that can be hard to diagnose.

Notes
This rule is the opposite of S2057 (“Serializable” classes should have a version id). Teams may use either that rule or this one depending on their requirements and application architecture. A team may even go so far as using both rules in their quality profile - then, using SonarQube’s capability of path-based rules, they can add an include entry for the “should have a version id” rule that limits it to those parts of their application where it’s relevant, and exclude issues from the new “use auto-generated version id” rule there.

If this rule is accepted, I suggest adding links to the other rule in the description of both rules.

Furthermore, independently of this rule suggestion, the description of S2057 should be changed: currently, it starts with “A serialVersionUID field is required”, but it should read “A serialVersionUID field is strongly recommended” to align with the wording given in the Serializable documentation:

However, it is strongly recommended that all serializable classes explicitly declare serialVersionUID values, since the default serialVersionUID computation is highly sensitive to class details that may vary depending on compiler implementations, and can thus result in unexpected InvalidClassExceptions during deserialization. Therefore, to guarantee a consistent serialVersionUID value across different java compiler implementations, a serializable class must declare an explicit serialVersionUID value.

The first sentence makes it quite clear it’s a recommendation; the word “must” in the last sentence only applies to the goal of compiler compatibility.

Noncompliant Code

public class Foo implements Serializable {
  private static final long serialVersionUID = 1;
}

public class SubsystemException extends RuntimeException {
  private static final long serialVersionUID = 8582433437601788991L;
}

References

Type
Code Smell

Tags
pitfall


Best regards


(Alexandre Gigleux) #4

Hello,

I updated the specification of S2057 so we say “recommended” instead of “required”.

Then for the rule you are suggesting, I’m so-so. I think the use case you are describing is perfectly valid and you want to enforce the idea that developers must review the serialVersionUID each time they change the “Serializable” class.

My concern is more about how this will work inside SonarQube. If SonarQube is raising an issue on each serialVersionUID with the message “Check if this serialVersionUID is still relevant”, that will work, but then what’s the next step? The issue will remain open for ever and if the developer do the manual review and close the issue as “Won’t Fix”, it will never come back again.
I’m afraid that today, we don’t have a type of rule/issue and an associated workflow that allows to implement such type of rule.

Regards


(Jens Bannmann) #5

Not exactly. It’s more like "allow teams to enforce the rule that no class has a serialVersionUID". A more detailed explanation of my use case would be the following:

  1. A team decides their application has no use for backwards compatibility or interoperability with code compiled by other compilers.
  2. They put a rule in their code conventions to only use serialVersionUID in exceptional cases approved by the architect. Also, they configure their quality profile to include the rule that I propose.
  3. A developer adds or edits a class. They don’t know about the fine details of Serializable and the team’s decision, so they add a serialVersionUID field to the class.
  4. SonarQube creates an issue for the rule violation, which alerts the other team members. The team investigates and comes to either of the following conclusions:
    • As for the majority of classes in their application, a manually maintained version ID is useless and even harmful here, so the team removes it.
    • This is indeed an exceptional case where they actually want a manually maintained version ID. They mark the SonarQube issue as “won’t fix” while entering their reasoning in a comment. To be sure, they also add a code comment explaining the situation.

In my view, this kind of process works perfectly fine using the existing SonarQube mechanisms. In fact, it is the same thing as many existing rules: the rule basically says “this looks weird, are you sure this is a good idea?” and the team either fixes it or closes the issue, effectively saying “thanks, but in this case it’s fine / we accept the risk / we can’t change it”.


(Alexandre Gigleux) #6

Hello Jens,

Can you review https://jira.sonarsource.com/browse/RSPEC-4926 and let me know if it matches your expectations?

Thanks


(Jens Bannmann) #7

Can you review https://jira.sonarsource.com/browse/RSPEC-4926 and let me know if it matches your expectations?

Hi Alexandre,

thanks for considering this rule! The spec captures the intention fairly well. I would only suggest the following wording and phrasing changes that I think improve style and precision:

Providing a serialVersionUID field on Serializable classes is strongly recommended by the Serializable documentation but blindly following that recommendation can be harmful.

serialVersionUID value is stored with the serialized data and this field is controlledverified when unserializingdeserializing the data to ensure that the code reading the data is compatible with the serialized data. In case of failure, it means the serialized data and the code are not in sync and this is fine because you know what’s wrong.
When the serialVersionUID is generated from the fields of the Serializable classby an IDE or blindly hard-coded, there is a high probability that one will forget to update the serialVersionUID value when the Serializable class is later enriched with additional fields. As a consequence, some old serialized data will incorrectly be considered as compatible with athe newer version of the code while it is not the case, creating situations which are hard to debug.

This is whyTherefore, defining serialVersionUID should be done with care. and this This rule is raisingraises an issue on each serialVersionUID field declared on Classesclasses implementing Serializable to be sure the presence and the value of the serialVersionUID field is challenged and validated by the team.

Best regards,
Jens


(Alexandre Gigleux) #8

Thanks for the review and here is the implementation ticket that you can follow: https://jira.sonarsource.com/browse/SONARJAVA-2926

Regards