Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-20262

Fix for JENKINS-18629 caused a regression in several plugins

    Details

    • Similar Issues:

      Description

      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.

        Attachments

          Issue Links

            Activity

            Hide
            slide_o_mix Alex Earl added a comment -

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

            Show
            slide_o_mix Alex Earl added a comment - It seems like the new BindingInterceptor is changing behavior for how classes that follow the Descriptor/Describable pattern are instantiated.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            Started evaluating.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - Started evaluating.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            Jesse Glick suggests we roll back JENKINS-18629 fix.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - Jesse Glick suggests we roll back JENKINS-18629 fix.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            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.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - 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.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            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.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - 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.
            Hide
            slide_o_mix Alex Earl added a comment -

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

            Show
            slide_o_mix Alex Earl added a comment - Correct, I fixed this and will be making a release, but there are other plugins that may have similar issues.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            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:

              <f:entry field="url" title="URL">
                <f:textbox/>
              </f:entry>
            

            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.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - 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: <f:entry field="url" title="URL"> <f:textbox/> </f:entry> 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.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            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.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - 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.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            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.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - 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.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            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

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - 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
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            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.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - 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.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

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

            Show
            kohsuke Kohsuke Kawaguchi added a comment - I have deprecated various StaplerRequest.bindParameters to point people to StaplerRequest.bindJSON . This change will be in 1.222.
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

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

            Show
            kohsuke Kohsuke Kawaguchi added a comment - Problematic commit reverted. I'm going to check with Jesse Glick before I proceed with the proposed change.
            Hide
            dogfood dogfood added a comment -

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

            Result = SUCCESS
            kohsuke : 17c868753df43a861e2c4f946897e8a54a90b63d
            Files :

            • changelog.html
            Show
            dogfood dogfood added a comment - Integrated in jenkins_main_trunk #2982 [FIEXED JENKINS-20262] (Revision 17c868753df43a861e2c4f946897e8a54a90b63d) Result = SUCCESS kohsuke : 17c868753df43a861e2c4f946897e8a54a90b63d Files : changelog.html
            Hide
            jglick Jesse Glick added a comment -

            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.

            Show
            jglick Jesse Glick added a comment - 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 .
            Hide
            slide_o_mix Alex Earl added a comment -

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

            Show
            slide_o_mix Alex Earl added a comment - So, is this going to be closed out now? Is there anything still outstanding we need to keep it open for?
            Hide
            jglick Jesse Glick added a comment -

            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”

            Show
            jglick Jesse Glick added a comment - 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”
            Hide
            danielbeck Daniel Beck added a comment -

            Kohsuke, Jesse: Any news here?

            Show
            danielbeck Daniel Beck added a comment - Kohsuke, Jesse: Any news here?
            Hide
            slide_o_mix Alex Earl added a comment -

            Bump, what's the status on this?

            Show
            slide_o_mix Alex Earl added a comment - Bump, what's the status on this?
            Hide
            kutzi kutzi added a comment - - edited

            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?

            Show
            kutzi kutzi added a comment - - edited 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?
            Hide
            slide_o_mix Alex Earl added a comment -

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

            Show
            slide_o_mix Alex Earl added a comment - Where does it show as a blocker for email-ext?
            Hide
            kutzi kutzi added a comment -

            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.

            Show
            kutzi kutzi added a comment - 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.
            Hide
            slide_o_mix Alex Earl added a comment -

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

            Show
            slide_o_mix Alex Earl added a comment - Just click the "refresh" button on the JIRA widget and it goes away.
            Hide
            nikki586 Nicole Jacobson added a comment -

            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.

            Show
            nikki586 Nicole Jacobson added a comment - 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.
            Hide
            danielbeck Daniel Beck added a comment -

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

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

            Show
            danielbeck Daniel Beck added a comment - define an interface in Stapler that allows an object to nominate its own replacement Jesse Glick Is https://github.com/stapler/stapler/blob/master/core/src/main/java/org/kohsuke/stapler/DataBoundResolvable.java what you meant here?
            Hide
            jglick Jesse Glick added a comment -

            Daniel Beck it was actually Kohsuke Kawaguchi who wrote that. From looking at DataBoundResolvable, it does sound like the same thing.

            Show
            jglick Jesse Glick added a comment - Daniel Beck it was actually Kohsuke Kawaguchi who wrote that. From looking at DataBoundResolvable , it does sound like the same thing.

              People

              • Assignee:
                Unassigned
                Reporter:
                slide_o_mix Alex Earl
              • Votes:
                5 Vote for this issue
                Watchers:
                16 Start watching this issue

                Dates

                • Created:
                  Updated: