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

[java] Rename BeanMembersShouldSerialize to NonSerializableClass #4176

Closed
adangel opened this issue Oct 27, 2022 · 0 comments · Fixed by #4196
Closed

[java] Rename BeanMembersShouldSerialize to NonSerializableClass #4176

adangel opened this issue Oct 27, 2022 · 0 comments · Fixed by #4196
Labels
an:enhancement An improvement on existing features / rules
Milestone

Comments

@adangel
Copy link
Member

adangel commented Oct 27, 2022

There are a two issues around the rule BeanMembersShouldSerialize:

The rule exists since PMD 1.1 but doesn't really check for serialization despite its name:

The name of the rule is confusing too, since it's not checking that members should be serializable (which would be interesting on it's own), but rather if the getter / setters are in place. I suggest renaming this rule accordingly.

(#403 (comment))

I'm proposing the following changes:

  1. Rename the rule to "NonSerializableClass" (still category "Error Prone")
  2. Only consider classes, that are java.io.Serializable
  3. Verify, that each member (= field) of the class is again serializable. This is effectively implementing SonarSource's rule https://rules.sonarsource.com/java/RSPEC-1948
  4. Taking into account, that transient fields can be ignored
  5. Don't look at getters or setters - this is something for a different rule that actually checks Java Beans conformity
  6. Deprecate the property prefix - if a class is serializable, all members have to be serializable as well, regardless of their name

That way, we don't need to need to add a new property like #403 suggests.
And since we only consider serializable classes, the rule should be less noisy (#1668).

Note: BeanMembersShouldSerialize can be suppressed with @SuppressWarnings("serial"), which actually doesn't make sense, since the rule doesn't do serializability checks (originally implemented per request https://sourceforge.net/p/pmd/bugs/784/)

Alternative

We could actually just create a new rule "NonSerializableClass" and deprecate "BeanMembersShouldSerialize" entirely. While the implementation will be completely different, the original intention (serializability) is the same of both rules.
It probably makes not so much a difference, since the rule was probably not used at all because it's so noisy...

@adangel adangel added the an:enhancement An improvement on existing features / rules label Oct 27, 2022
@adangel adangel added this to the 6.52.0 milestone Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
an:enhancement An improvement on existing features / rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant