Skip to content

Drop serialVersionUID from AbstractPersistable. #2245

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Drop serialVersionUID from AbstractPersistable. #2245

wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Jun 22, 2021

Since serialVersionUID already exists

@quaff
Make AbstractPersistable serializable
f5165c4 
Since serialVersionUID already exists
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 22, 2021
@schauder
Copy link
Contributor

Could you explain what problem gets solved by making it Serializable? Apart from the obvious one of an otherwise out of place serialVersionUID?

@schauder schauder added the status: waiting-for-feedback We need additional information before we can continue label Jun 22, 2021
@quaff
Copy link
Contributor Author

quaff commented Jun 22, 2021

Could you explain what problem gets solved by making it Serializable? Apart from the obvious one of an otherwise out of place serialVersionUID?

serialVersionUID is present here, I guess someone missed Serializable here, Serializable has no sin, It's reasonable to mark model as serializable.
If project maintainer object this PR for security concern, please close it and remove unused serialVersionUID.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 22, 2021
@mp911de
Copy link
Member

mp911de commented Jun 22, 2021

Implementing Serializable would suddenly make all subclasses of AbstractPersistable serializable which isn't an intended effect. Having serialVersionUID helps with the object version and aids subclasses that want to rely on serialVersionUID and explicitly make their subclasses serializable. That being said, we should keep things as they are right now.

@quaff
Copy link
Contributor Author

quaff commented Jun 22, 2021

Implementing Serializable would suddenly make all subclasses of AbstractPersistable serializable which isn't an intended effect. Having serialVersionUID helps with the object version and aids subclasses that want to rely on serialVersionUID and explicitly make their subclasses serializable. That being said, we should keep things as they are right now.

It seems serialVersionUID doesn't inherited by subclasses.
https://stackoverflow.com/questions/23465613/is-serialversionuid-inherited-by-subclasses-if-i-have-default-serialversionuid

@mp911de
Copy link
Member

mp911de commented Jun 28, 2021

Okay, then let's remove the serial version UID altogether. ObjectStreamClass performs a cl.getDeclaredField("serialVersionUID") which uses the actual class.

@schauder schauder requested a review from gregturn July 1, 2021 15:11
gregturn
gregturn reviewed Jul 2, 2021
View reviewed changes
Copy link
Contributor

@gregturn gregturn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based upon @mp911de 's comments, we're actually going to drop serialVersionUID, and verify that doesn't break anything.

@gregturn gregturn self-assigned this Jul 2, 2021
@gregturn gregturn removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jul 2, 2021
@gregturn gregturn changed the title Make AbstractPersistable serializable Drop serialVersionUID from AbstractPersistable. Jul 2, 2021
@gregturn gregturn added this to the 2.6 M1 (2021.1.0) milestone Jul 2, 2021
@gregturn gregturn added the type: enhancement A general enhancement label Jul 2, 2021
gregturn added a commit that referenced this pull request Jul 2, 2021
@gregturn
Drop serialVersionUID from AbstractPersistable.
2e30a35 
Considering it's not serializable, there is no need to maintain serialVersionUID.

See #2245.
@gregturn
Copy link
Contributor

gregturn commented Jul 2, 2021

Resolved via 2e30a35.

@gregturn gregturn closed this Jul 2, 2021
gregturn added a commit that referenced this pull request Jul 2, 2021
@gregturn
Backport dropping of serialVersionUID from AbstractPersistable.
d3a1242 
Considering it's not serializable, there is no need to maintain serialVersionUID.

See #2245.
@gregturn
Copy link
Contributor

gregturn commented Jul 2, 2021 •
edited
Loading

Backported to 2.5.x with d3a1242.

gregturn added a commit that referenced this pull request Jul 2, 2021
@gregturn
Backport dropping of serialVersionUID from AbstractPersistable.
855bff6 
Considering it's not serializable, there is no need to maintain serialVersionUID.

See #2245.
@gregturn
Copy link
Contributor

gregturn commented Jul 2, 2021

Backported to 2.4.x with 855bff6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregturn gregturn gregturn left review comments

Assignees

@gregturn gregturn

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.6 M1 (2021.1.0)
Development

Successfully merging this pull request may close these issues.
5 participants
@quaff @schauder @mp911de @gregturn @spring-projects-issues