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

Removed variables with duplicate definitions in Ticket/Request #595

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

afm497
Copy link

@afm497 afm497 commented Jun 25, 2023

Fixes #573

Very simple/small fix, but will allow library to work correctly in an environment where GSON is used.

…lds were defined both here in Ticket and in it's parent class, Request. The fact that these had different access modifers in each definition led to serialiation/marshalling problems in some libraries (GSON for instance). Relying now on the definition in the parent class and fix is seamless.
@afm497 afm497 requested review from duemir and PierreBtz as code owners June 25, 2023 02:26
@PierreBtz PierreBtz added enhancement breaking Breaking change labels Jun 26, 2023
@afm497
Copy link
Author

afm497 commented Jul 3, 2023

Hey, sorry I totally missed this comment. I did test this, but I did only test it in the context of JSON marshalling. It's presumable that this could break backwards compat with binary serialization, but I do not believe there is any way around that. The error in the class is there, and there is no other way to resolve that I am aware of, at least that is relatively simple. I think the bottom line is that a bug has existed for a while here (redundant definitions of the same class member with differing accessibility. One is private, one is protected and they are exposing the same piece of data) and this change should happen at some point so the library works correctly. Maybe it is a major version revision and can be communicated that there is some issues with backward compatibility when doing binary serialization?

@PierreBtz
Copy link
Collaborator

I agree with you, but without my suggestion of changing the serialVersionUID, a code de-serializing a Ticket serialized with an older version would silently work, giving undefined behavior. I'd rather see such code fail, therefore we should change the serialVersionUID.

@afm497
Copy link
Author

afm497 commented Jul 7, 2023

Ahhh, definitely I agree wholeheartedly. Didn't see that suggestion, apologies. I'll update accordingly.

Thanks,
Ray

Copy link
Collaborator

@PierreBtz PierreBtz left a comment

Choose a reason for hiding this comment

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

great, thanks a lot for your contribution!

@PierreBtz PierreBtz merged commit bc1fb90 into cloudbees-oss:master Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot marshal Ticket to JSON
2 participants