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

DefaultBuilderDescriptorLister should not check buildstep class for @DataBoundConstructor

    Details

    • Similar Issues:

      Description

      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

        Attachments

          Issue Links

            Activity

            ndeloof Nicolas De Loof created issue -
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            scm_issue_link SCM/JIRA link daemon made changes -
            Field Original Value New Value
            Status Open [ 1 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            Hide
            bap 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.

            Show
            bap 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 bap made changes -
            Resolution Fixed [ 1 ]
            Status Resolved [ 5 ] Reopened [ 4 ]
            Assignee domi [ domi ] Nicolas De Loof [ ndeloof ]
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            domi 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.

            Show
            domi 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.
            domi Dominik Bartholdi made changes -
            Link This issue depends on JENKINS-18629 [ JENKINS-18629 ]
            Hide
            domi 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.

            Show
            domi 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.
            domi Dominik Bartholdi made changes -
            Link This issue is blocking JENKINS-16259 [ JENKINS-16259 ]
            domi Dominik Bartholdi made changes -
            Link This issue is related to JENKINS-6797 [ JENKINS-6797 ]
            Hide
            scm_issue_link 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.

            Show
            scm_issue_link 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.
            rtyler R. Tyler Croy made changes -
            Workflow JNJira [ 146179 ] JNJira + In-Review [ 186085 ]
            cloudbees CloudBees Inc. made changes -
            Remote Link This issue links to "CloudBees Internal CJP-6237 (Web Link)" [ 19176 ]
            Hide
            ataylor Alex Taylor added a comment -

            Nicolas De Loof 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?

            Show
            ataylor Alex Taylor added a comment - Nicolas De Loof 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?
            ndeloof Nicolas De Loof made changes -
            Assignee Nicolas De Loof [ ndeloof ]

              People

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

                Dates

                • Created:
                  Updated: