[JENKINS-20262] Fix for JENKINS-18629 caused a regression in several plugins

[timja]

https://github.com/jenkinsci/jenkins/commit/c01333c346176c93e8cbf93b5822978d8d2c0bff introduced a new behavior that breaks several plugins. The issue seems to arise only when a class that extends Descriptor implements its own newInstance method.

---

Originally reported by slide_o_mix, imported from: Fix for JENKINS-18629 caused a regression in several plugins

• status: Open
• priority: Major
• resolution: Unresolved
• imported: 2022/01/10

[timja]

slide_o_mix:

It seems like the new BindingInterceptor is changing behavior for how classes that follow the Descriptor/Describable pattern are instantiated.

[timja]

kohsuke:

Started evaluating.

[timja]

kohsuke:

jglick suggests we roll back JENKINS-18629 fix.

[timja]

kohsuke:

I looked at JENKINS-20198, but this one is the case of "a bug in a plugin was masked previously but it's now exposed by the core change" situation.

EmailTriggerDescriptor in email-ext 1.32.1 has the following code:

    public EmailTrigger newInstance(StaplerRequest req, JSONObject formData) {
EmailTrigger res = null;
try {
    res = clazz.newInstance();
    res.configure(req, formData);
} catch(Exception e) {
    // should do something here?
}
return res;
    }

clazz.newInstance() fails because clazz in this case refers to a Describable subtype like FailureTrigger, and there's no default constructor.

Before we fixed JENKINS-18629, this newInstance method was never called, so this problem was masked.

[timja]

kohsuke:

I looked at JENKINS-20201. This one is a regression caused by the behaviour difference in the bindJSON method.

GithubProjectProperty has the following code:

@Override
public JobProperty newInstance(StaplerRequest req,
JSONObject formData) throws FormException {
    GithubProjectProperty tpp = req.bindJSON(
    GithubProjectProperty.class, formData);
    if (tpp.projectUrl == null) {
tpp = null; // not configured
    }
    return tpp;
}

Before JENKINS-18629 fix, The bindJSON method always just called a constructor, so the return value is guaranteed to be non-null. After JENKINS-18629, the bindJSON returns the result of the nested newInstance invocation, so it returns null from it.

[timja]

slide_o_mix:

Correct, I fixed this and will be making a release, but there are other plugins that may have similar issues.

[timja]

kohsuke:

I looked at JENKINS-20186. This is also a case of "a bug in a plugin was masked previously but it's now exposed by the core change" situation.

The BitBucket class has the following newInstance method:

public @Override BitBucket newInstance(StaplerRequest req, JSONObject json) throws FormException {
    return req.bindParameters(BitBucket.class,"bitbucket.");
}

This style of parameter injection is legacy code, where Stapler will look for a parameter named "bitbucket.url" to insert into the "url" parameter of the BitBucket constructor. Sine BitBucket/config.jelly has the following, there's no such parameter:

Before JENKINS-18629, when a BitBucket is instantiated indirectly, this newInstance method is never called. So this problem was masked. After JENKINS-18629, this method is called, exposing the problem.

[timja]

kohsuke:

After looking at three issues, I think it's clear that this regression cannot be fixed by adjusting the JENKINS-18629 fix in the core.

While a good number of the regressions do appear to be problems in plugins that were previously masked, the fact still remains that this will catch any user upgrading to 1.536 in surprise, and the impact is severe.

Given this, I think we need to roll back JENKINS-18629 fix.

[timja]

kohsuke:

The original problem raised in JENKINS-18629 is that if a Descriptor defines a custom logic in the newInstance method, such logic doesn't take effect when the object appears in the middle of data-binding a tree of objects.

The original fix that had caused the mess tried to fix this by having Stapler call into newInstance method, instead of instantiating @DataBoundConstructor directly like it did before.

Instead, I propose we deprecate Descriptor.newInstance(StaplerRequest,JSONObject) method, then define an interface in Stapler that allows an object to nominate its own replacement, much like how readReplace() method works in Java Serialization.

This allows likes of GithubProjectProperty (see above) to return null, and encourage likes of this and this to be eliminated going forward.

Gradle class, which is mentioned in the initial description of JENKINS-18629, overrides the newInstance method just to manipulate the JSON tree structure to flatten some of the intermediates objects. This is best handled by introducing the inline attribute to the radioBlock tag much like the attribute of the same name in the optionalBlock tag.

[timja]

scm_issue_link:

Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
core/src/main/java/hudson/model/Descriptor.java
test/src/test/java/hudson/model/DescriptorTest.java
http://jenkins-ci.org/commit/jenkins/b3225944d127412a21e9763394bd90a1c870adcf
Log:
Revert "[FIXED JENKINS-18629]"

This reverts commit c01333c346176c93e8cbf93b5822978d8d2c0bff.
See
https://issues.jenkins-ci.org/browse/JENKINS-20262?focusedCommentId=188404&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-188404
for the discussion why this should be reverted.

Conflicts:

core/pom.xml

[timja]

scm_issue_link:

Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
changelog.html
http://jenkins-ci.org/commit/jenkins/17c868753df43a861e2c4f946897e8a54a90b63d
Log:
[FIEXED JENKINS-20262]

Describing the previous fix.

[timja]

kohsuke:

I have deprecated various StaplerRequest.bindParameters to point people to StaplerRequest.bindJSON. This change will be in 1.222.

[timja]

kohsuke:

Problematic commit reverted. I'm going to check with jglick before I proceed with the proposed change.

[timja]

dogfood:

Integrated in jenkins_main_trunk #2982
[FIEXED JENKINS-20262] (Revision 17c868753df43a861e2c4f946897e8a54a90b63d)

Result = SUCCESS
kohsuke : 17c868753df43a861e2c4f946897e8a54a90b63d
Files :

• changelog.html

[timja]

jglick:

Makes sense to me. I do not think overriding Descriptor.newInstance was ever clearly documented anyway. So can this issue be fixed, its plugin dependents left open for plugin authors to clean up form handling, and JENKINS-18629 closed as “will not fix”? Any new feature in forms can be introduced at any time; if the Gradle plugin really needs it, it can make its own temporary copy of radioBlock.jelly.

[timja]

slide_o_mix:

So, is this going to be closed out now? Is there anything still outstanding we need to keep it open for?

[timja]

jglick:

I think there is still outstanding work. @kohsuke proposed that we

deprecate Descriptor.newInstance(StaplerRequest,JSONObject) method, then define an interface in Stapler that allows an object to nominate its own replacement

and

[introduce] the inline attribute to the radioBlock tag

and I proposed that we

[close] JENKINS-18629 as “will not fix”

[timja]

danielbeck:

Kohsuke, Jesse: Any news here?

[timja]

slide_o_mix:

Bump, what's the status on this?

[timja]

kutzi:

Bump bump. This is still listed as a 'Blocker' in the issues of the email-ext plugin. So if one would read the priority as it's meant to be, it would mean that the plugin would be barely usable, if at all.
Is this correct? If not could we have the priority adjusted - or at least a understandable description (for users not developers) what the actual regression is?

[timja]

slide_o_mix:

Where does it show as a blocker for email-ext?

[timja]

kutzi:

Currently still here: https://wiki.jenkins-ci.org/display/JENKINS/Email-ext+plugin
But Jesse has now removed the email-ext component from this issue, so it will eventually disappear from the Wiki page, too.

[timja]

slide_o_mix:

Just click the "refresh" button on the JIRA widget and it goes away.

[timja]

nikki586:

Any news on this? Believe this is related to ruby-runtime defect ( https://github.com/jenkinsci/jenkins.rb/issues/112 ). If the recommendation is not to use newInstance() then what should the ruby-runtime lib be updated to use? Many ruby-runtime dependent plugins are broken with the build unable to save any post build actions.

[timja]

danielbeck:

define an interface in Stapler that allows an object to nominate its own replacement

jglick Is https://github.com/stapler/stapler/blob/master/core/src/main/java/org/kohsuke/stapler/DataBoundResolvable.java what you meant here?

[timja]

jglick:

danielbeck it was actually kohsuke who wrote that. From looking at DataBoundResolvable, it does sound like the same thing.