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

Fix for JENKINS-18629 caused a regression in several plugins

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major
    • core

      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.

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

          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.

          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.

          Started evaluating.

          Kohsuke Kawaguchi added a comment - Started evaluating.

          jglick suggests we roll back JENKINS-18629 fix.

          Kohsuke Kawaguchi added a comment - jglick suggests we roll back JENKINS-18629 fix.

          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.

          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.

          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.

          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.

          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.

          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.

          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.

          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.

          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.

          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.

          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.

          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.

          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

          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

          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.

          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.

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

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

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

          Kohsuke Kawaguchi added a comment - Problematic commit reverted. I'm going to check with jglick before I proceed with the proposed change.

          dogfood added a comment -

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

          Result = SUCCESS
          kohsuke : 17c868753df43a861e2c4f946897e8a54a90b63d
          Files :

          • changelog.html

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

          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.

          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 .

          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?

          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?

          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”

          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”

          Daniel Beck added a comment -

          Kohsuke, Jesse: Any news here?

          Daniel Beck added a comment - Kohsuke, Jesse: Any news here?

          Alex Earl added a comment -

          Bump, what's the status on this?

          Alex Earl added a comment - Bump, what's the status on this?

          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?

          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?

          Alex Earl added a comment -

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

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

          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.

          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.

          Alex Earl added a comment -

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

          Alex Earl added a comment - Just click the "refresh" button on the JIRA widget and it goes away.

          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.

          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.

          Daniel Beck added a comment -

          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?

          Daniel Beck added a comment - 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?

          Jesse Glick added a comment -

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

          Jesse Glick added a comment - danielbeck it was actually kohsuke who wrote that. From looking at DataBoundResolvable , it does sound like the same thing.

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

              Created:
              Updated: