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

Minimize readResolve visibility #2567

Merged
merged 2 commits into from Mar 31, 2018

Conversation

@daniel-beck
Member

daniel-beck commented Sep 26, 2016

I don't think plugin code should ever be calling readResolve, and since it doesn't need to be public to work, I'm restricting it as much as possible.

One is called by tests, so I @Restricted it. Another overrides the parent class's protected … readResolve, so I've kept that the same. The others are now private.

I'm actually not positive this is a good idea, I'm counting on @jenkinsci/code-reviewers to be more knowledgeable about this than I am.

@KostyaSha

@daniel-beck I would suggest firstly to do experiment with but that i saw before but had no time to debug:

  • ClassA contains ClassB as initialised (stored) field.
  • ClassB contains some fields and code in readResolve() that changes this field.
  • jenkins restarts and readResolve isn't called on nested field.

To bypass this issue i called readResolve() from readResolve() enforced.

@KostyaSha

This comment has been minimized.

Member

KostyaSha commented Sep 26, 2016

Wow, no way to edit comment.. Sorry for confused word.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 27, 2016

@KostyaSha No problem. It's not clear what you expect me to do here though. Make all readResolves protected to make sure subclasses can call them?

@KostyaSha

This comment has been minimized.

Member

KostyaSha commented Sep 27, 2016

It's not about subclasses, ClassA didn't inherit ClassB. It's just a field of other type.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 27, 2016

@KostyaSha Do you have a reference or reproducible case for this? I am pretty sure that readResolve is called on all Xstream deserialized classes, nested or not. Otherwise we'd never have had the GhprbTrigger issue a few weeks/months ago, as the deserialized object is the project.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 29, 2016

@kzantow I have no idea why a PR build shows up as you at unconfigured-jenkins-location, but please stop doing whatever it is you're doing.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Sep 30, 2016

@daniel-beck I've contacted @kzantow yesterday via a private message. He was going to cleanup the mess after getting some KT from @Vlatombe , who already did it a couple of weeks ago

@oleg-nenashev

I'd rather 🐛 this pull-request, because it formally breaks the binary compatibility and may also impact class implementations in plugins, which override readResolve to a whatever crazy reason

@@ -203,7 +203,7 @@ public void save() throws IOException {
SaveableListener.fireOnChange(this, config);
}
public Object readResolve() {
private Object readResolve() {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Sounds weird, but formally it's a compatibility breaking change. It could be a real case when you override the method in the inherited class, but this class is actually final. So I'm not sure it is important.

I would just add Restricted

This comment has been minimized.

@svanoort

This comment has been minimized.

@jglick

jglick Mar 12, 2018

Member

I do not think we care about formal compatibility when there is no plausible use case.

This comment has been minimized.

@svanoort

svanoort Mar 12, 2018

Member

Yes, but "no plausible use case" is unfortunately not the same as "Nobody is doing it and it won't break things."

I tend to prefer relying on the Restricted modifier -- it'll break builds of plugins reliant on APIs that they shouldn't have access to, but doesn't cause direct binary-compat breakages.

This comment has been minimized.

@svanoort

svanoort Mar 12, 2018

Member

Or alternately, @Restricted for one LTS to show if this raises plugin issues, then private.
This is a gentler way of doing binary-compat breaking changes IMO.

In this case it's probably safe to break since, uh, readResolve... but in general we've been bitten many times when we said "oh there's no user" (and in one case recently even when an API was Restricted due to a quirk).

@@ -41,7 +41,7 @@
return ChangeLogSet.createEmpty(build);
}
public Object readResolve() {
private Object readResolve() {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

🐛 not a final class, it may be a breaking change if there are overrides

@@ -138,7 +138,7 @@ public ArtifactArchiver(String artifacts, String excludes, boolean latestOnly, b
}
// Backwards compatibility for older builds
public Object readResolve() {
private Object readResolve() {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

🐛 not a private class

@@ -170,7 +170,7 @@ public InstallState(@Nonnull String name, boolean isSetupComplete) {
public void initializeState() {
}
public Object readResolve() {
private Object readResolve() {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Not a final class. Also it's an extension point, which may be potentially doing some migrations in custom implementations from plugins 🐛

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Oct 1, 2016

@oleg-nenashev

it formally breaks the binary compatibility and may also impact class implementations in plugins, which override readResolve to a whatever crazy reason

Please clarify, would you 👍 a variant of this that uses protected? If anyone relies on public readResolve they're pretty certainly doing it wrong.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 10, 2016

@daniel-beck I would

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 11, 2018

@oleg-nenashev I addressed your concern in 2d494cf

@daniel-beck daniel-beck requested a review from oleg-nenashev Mar 11, 2018

@daniel-beck daniel-beck removed their assignment Mar 11, 2018

@oleg-nenashev

Weak 👍 . I do not like the binary compat breakage much, but it should be fine taking the method kind into account

@daniel-beck daniel-beck requested review from jglick and dwnusbaum Mar 11, 2018

@jglick

jglick approved these changes Mar 12, 2018

@@ -203,7 +203,7 @@ public void save() throws IOException {
SaveableListener.fireOnChange(this, config);
}
public Object readResolve() {
private Object readResolve() {

This comment has been minimized.

@jglick

jglick Mar 12, 2018

Member

I do not think we care about formal compatibility when there is no plausible use case.

justification = "Null checks in readResolve are valid since we deserialize and upgrade objects")
public Object readResolve() {
protected Object readResolve() {

This comment has been minimized.

@jglick

jglick Mar 12, 2018

Member

We should just make classes like this final anyway. They were never designed for subclassing and any plugin attempting to do so is likely to get broken in all sorts of ways anyway.

@@ -221,7 +221,7 @@ public void initializeState() {
* {@code <installState class="jenkins.install.InstallState$CreateAdminUser" resolves-to="jenkins.install.InstallState">}
* @see #UNUSED_INNER_CLASSES
*/
public Object readResolve() {
protected Object readResolve() {

This comment has been minimized.

@jglick

jglick Mar 12, 2018

Member

Just make it private. There is no legitimate use case for overriding this—we want to use this specific implementation for all subclasses.

This comment has been minimized.

@daniel-beck

daniel-beck Mar 12, 2018

Member

Wouldn't the problem be that subclasses would override it without being able to super.readResolve()?

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Mar 17, 2018

@daniel-beck I'd guess it's ready to go, but maybe you want to finish the discussion with @jglick

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 27, 2018

@oleg-nenashev As far as I'm concerned, this is ready to go. It was public, I'm making it protected, @jglick suggests private.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Mar 28, 2018

Test failure is unrelated.

@dwnusbaum dwnusbaum removed their request for review Mar 28, 2018

@oleg-nenashev oleg-nenashev merged commit c82620d into jenkinsci:master Mar 31, 2018

0 of 2 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
continuous-integration/jenkins/pr-merge The build of this commit was aborted
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment