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

DefaultBuilderDescriptorLister should not check buildstep class for @DataBoundConstructor

      Some exotic build steps (cloudbees templates ones, that are dynamically generated) aren't created using @DataBoundConstructor databinding, but by a custom newInstance implementation in constructor.

      DefaultBuilderDescriptorLister is checking the buildStep for a @DataBoundConstructor, for some unknown reason (I've searched the git history, but this whole code was added as a single commit), but shouldn't imho

          [JENKINS-15445] DefaultBuilderDescriptorLister should not check buildstep class for @DataBoundConstructor

          Code changed in jenkins
          User: Nicolas De Loof
          Path:
          src/main/java/org/jenkinsci/plugins/conditionalbuildstep/lister/DefaultBuilderDescriptorLister.java
          http://jenkins-ci.org/commit/conditional-buildstep-plugin/fcfc219e627afaa8b19395674a756fe4dda43e7d
          Log:
          [FIXED JENKINS-15445] use Functions#getBuilderDescriptors to select the matching descriptors
          don't check for @DataBoundConstructor. Descriptor may not use it to create instances

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Nicolas De Loof Path: src/main/java/org/jenkinsci/plugins/conditionalbuildstep/lister/DefaultBuilderDescriptorLister.java http://jenkins-ci.org/commit/conditional-buildstep-plugin/fcfc219e627afaa8b19395674a756fe4dda43e7d Log: [FIXED JENKINS-15445] use Functions#getBuilderDescriptors to select the matching descriptors don't check for @DataBoundConstructor. Descriptor may not use it to create instances

          bap added a comment -

          The history is not in this plugin because the code was copied from the Flexible Publish plugin.

          The original check is both necessary - and not aggressive enough. The descriptors should be filtered for those with newInstance methods too. Some plugins appear to have both DBC and newInstance!

          The reason they need to be filtered is that the Flexible publish creates instances using a data bound constructor. Any instances of build step that are contained within will fail to be configured corectly if it uses configuration from the form and also uses newInstance.

          • 100% of buildsteps I tested which used newInstance failed to be correctly configured when the form was submitted.*

          The descriptor listers are used to enable publishers as build steps and build steps as publishers (using the Any Build Step plugin). For this reason, build steps returned need to be configurable using both configuration mechanisms.

          If you believe that the newInstance will be correctly configured when contained within a DBC configured container, please add a unit test to the Any Build Step plugin showing successful configuration submission of a newInstance configured buildstep (builder) inside the Flexible Publish publisher.

          Maybe I've missed something, and you can help me undestand, but like I said, I did not find a single build step that configured correctly when using newInstance.

          bap added a comment - The history is not in this plugin because the code was copied from the Flexible Publish plugin. The original check is both necessary - and not aggressive enough. The descriptors should be filtered for those with newInstance methods too. Some plugins appear to have both DBC and newInstance! The reason they need to be filtered is that the Flexible publish creates instances using a data bound constructor. Any instances of build step that are contained within will fail to be configured corectly if it uses configuration from the form and also uses newInstance. 100% of buildsteps I tested which used newInstance failed to be correctly configured when the form was submitted.* The descriptor listers are used to enable publishers as build steps and build steps as publishers (using the Any Build Step plugin). For this reason, build steps returned need to be configurable using both configuration mechanisms. If you believe that the newInstance will be correctly configured when contained within a DBC configured container, please add a unit test to the Any Build Step plugin showing successful configuration submission of a newInstance configured buildstep (builder) inside the Flexible Publish publisher. Maybe I've missed something, and you can help me undestand, but like I said, I did not find a single build step that configured correctly when using newInstance.

          Code changed in jenkins
          User: imod
          Path:
          src/main/java/org/jenkinsci/plugins/conditionalbuildstep/lister/DefaultBuilderDescriptorLister.java
          http://jenkins-ci.org/commit/conditional-buildstep-plugin/022995b2e0e9744a382b553609f446c9625885bb
          Log:
          revert change for JENKINS-15445 until the issues with DataBoundConstructor and newInstance are sorted out

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: imod Path: src/main/java/org/jenkinsci/plugins/conditionalbuildstep/lister/DefaultBuilderDescriptorLister.java http://jenkins-ci.org/commit/conditional-buildstep-plugin/022995b2e0e9744a382b553609f446c9625885bb Log: revert change for JENKINS-15445 until the issues with DataBoundConstructor and newInstance are sorted out

          Nicolas, do you have any code/plugin where this would work with the 'newInstance' method?
          In my experience this does not (yet) work - it seams that Stapler is not falling back in every case to the 'newInstance' method if the DataBoundConstructor is not present.

          Dominik Bartholdi added a comment - Nicolas, do you have any code/plugin where this would work with the 'newInstance' method? In my experience this does not (yet) work - it seams that Stapler is not falling back in every case to the 'newInstance' method if the DataBoundConstructor is not present.

          @ndeloof Looking at JENKINS-18629 I think we ca implement this now with a check for the version of the core. So if core > 1.536 then we ignore the databoundconsrructor limitation.

          Dominik Bartholdi added a comment - @ndeloof Looking at JENKINS-18629 I think we ca implement this now with a check for the version of the core. So if core > 1.536 then we ignore the databoundconsrructor limitation.

          Code changed in jenkins
          User: Nicolas De Loof
          Path:
          pom.xml
          src/main/java/org/jenkinsci/plugins/conditionalbuildstep/lister/DefaultBuilderDescriptorLister.java
          http://jenkins-ci.org/commit/conditional-buildstep-plugin/132178a305c0d8ece4bd58622811497745816667
          Log:
          [FIXED JENKINS-15445] rely on 1.536 so JENKINS-18629 doesn’t prevent BuildStep without a @DataBoundConstructor to be used.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Nicolas De Loof Path: pom.xml src/main/java/org/jenkinsci/plugins/conditionalbuildstep/lister/DefaultBuilderDescriptorLister.java http://jenkins-ci.org/commit/conditional-buildstep-plugin/132178a305c0d8ece4bd58622811497745816667 Log: [FIXED JENKINS-15445] rely on 1.536 so JENKINS-18629 doesn’t prevent BuildStep without a @DataBoundConstructor to be used.

          Alex Taylor added a comment -

          ndeloof This PR does not seem to cause issues with the latest version of Jenkins anymore. Can we get it re-attempted or am I just missing something?

          Alex Taylor added a comment - ndeloof This PR does not seem to cause issues with the latest version of Jenkins anymore. Can we get it re-attempted or am I just missing something?

            Unassigned Unassigned
            ndeloof Nicolas De Loof
            Votes:
            1 Vote for this issue
            Watchers:
            6 Start watching this issue

              Created:
              Updated: