Skip to content

Conversation

@johnycho
Copy link

@johnycho johnycho commented Jan 10, 2026

Fixes gh-17729

Description

The allSerializableClassesShouldHaveSerialVersionOrSuppressWarnings test logic had two issues that caused it to pass incorrectly (false positives):

1. Logic Error

It permitted classes to pass if the annotation was null (suppressWarnings == null).
Since @SuppressWarnings has RetentionPolicy.SOURCE, it is not visible at runtime via reflection, causing getAnnotation to always return null. Consequently, the test was skipping validation for all classes.

2. Typo

It checked for "Serial" (case-sensitive) instead of "serial".

Fix

This PR updates the validation logic to:

  • Correct the value to "serial".
  • Enforce a strict check by ensuring the class is ignored only if the annotation is explicitly present (non-null) and contains the correct value.

Verification

To demonstrate the fix, I temporarily removed serialVersionUID from a class (e.g., SimpleGrantedAuthority) and ran the test.

State Screenshot Result
Before Fix Before fix screenshot PASSED (False Positive) ❌
The test incorrectly passed even though serialVersionUID was missing, because the verification logic was broken.
After Fix After fix screenshot FAILED (Correct Behavior) ✅
The test now correctly detects the missing serialVersionUID and fails, enforcing the strict check as intended.

Note

This change fixes the test logic to be correct. However, due to the RetentionPolicy.SOURCE limitation of @SuppressWarnings, this strict check might cause the test to fail for classes that rely on @SuppressWarnings("serial") without a serialVersionUID. This reveals that the previous test was not actually verifying those classes.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: waiting-for-triage An issue we've not yet triaged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

allSerializableClassesShouldHaveSerialVersionOrSuppressWarnings should correctly check for SuppressWarnings

2 participants