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

Split the large databound constructors into databound setters

      Inside EC2 plugin there is a lot of constructors with a large set of databound constructors.
      Databound constructor should only be used for mandatory/final parameters

      Where databound setters should be used for optional parameters.

      https://jenkins.io/doc/developer/plugin-development/pipeline-integration/#constructor-vs-setters

          [JENKINS-57513] Split the large databound constructors into databound setters

          Joseph Petersen (old) created issue -

          Joseph Petersen (old) added a comment - - edited

          https://github.com/jenkinsci/configuration-as-code-plugin/issues/885
          https://github.com/jenkinsci/configuration-as-code-plugin/issues/381

          we also had several users on gitter ask for help debugging null pointers and it all leads to those large data bound constructors that assume null does not exist.

          Joseph Petersen (old) added a comment - - edited https://github.com/jenkinsci/configuration-as-code-plugin/issues/885 https://github.com/jenkinsci/configuration-as-code-plugin/issues/381 we also had several users on gitter ask for help debugging null pointers and it all leads to those large data bound constructors that assume null does not exist.

          Joseph Petersen (old) added a comment - jvz raihaan

          It is one of the things in backlog, now the plugin it is quite complex in term of options.

          FABRIZIO MANFREDI added a comment - It is one of the things in backlog, now the plugin it is quite complex in term of options.

          Matt Sicker added a comment -

          I'd also like to consider some form of data bound builder class feature so we can avoid using poor Java patterns. Let me give you an example from the Log4j plugin system. The more typical use of DataBoundConstructor is equivalent to PluginFactory in Log4j: https://github.com/apache/logging-log4j2/blob/a6b1cbfc9d4257baaa6760e58426f0f4dfa99b0f/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/FailoverAppender.java#L183

          In order to add new options over time, we used to deprecate a factory method, remove the annotation, and move it to the new factory method that contained default values for new parameters (as well as validation; you could return null instead of the plugin object to signify an error without throwing exceptions which was important in this part of our design, but I digress). This pattern was extremely brittle and is fairly similar to DataBoundConstructor. However, in order to follow Effective Java and other similar best practices, we'd still like to prefer that our objects are fully constructed and ready to use as soon as they're "created" whether that be via a constructor, a factory method, a builder class, or whatever. Having to call setters after creating the object leaves the object in intermediary states that typically don't have meaning, and this can cause runtime bugs in practice.

          This is getting a bit long, so let me wrap it up. My preferred approach would be what we did in Log4j with newer plugins via a builder class pattern: https://github.com/apache/logging-log4j2/blob/a6b1cbfc9d4257baaa6760e58426f0f4dfa99b0f/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/HttpAppender.java#L48

          As you can see, due to syntactical features of Java, it is much easier to add and rename things in a builder class over time independently from the implementation details of the plugin class itself. I'd also note that there was a time in Log4j history before there was automatic type conversion in the annotated factory methods, so some old code still have all string parameters with manual type conversion.

          Also, why do I bring this up in EC2 and not structs or whatever the appropriate plugin is? Good question, but a new issue should likely be filed about it linked back here.

          Matt Sicker added a comment - I'd also like to consider some form of data bound builder class feature so we can avoid using poor Java patterns. Let me give you an example from the Log4j plugin system. The more typical use of DataBoundConstructor is equivalent to PluginFactory in Log4j: https://github.com/apache/logging-log4j2/blob/a6b1cbfc9d4257baaa6760e58426f0f4dfa99b0f/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/FailoverAppender.java#L183 In order to add new options over time, we used to deprecate a factory method, remove the annotation, and move it to the new factory method that contained default values for new parameters (as well as validation; you could return null instead of the plugin object to signify an error without throwing exceptions which was important in this part of our design, but I digress). This pattern was extremely brittle and is fairly similar to DataBoundConstructor. However, in order to follow Effective Java and other similar best practices, we'd still like to prefer that our objects are fully constructed and ready to use as soon as they're "created" whether that be via a constructor, a factory method, a builder class, or whatever. Having to call setters after creating the object leaves the object in intermediary states that typically don't have meaning, and this can cause runtime bugs in practice. This is getting a bit long, so let me wrap it up. My preferred approach would be what we did in Log4j with newer plugins via a builder class pattern: https://github.com/apache/logging-log4j2/blob/a6b1cbfc9d4257baaa6760e58426f0f4dfa99b0f/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/HttpAppender.java#L48 As you can see, due to syntactical features of Java, it is much easier to add and rename things in a builder class over time independently from the implementation details of the plugin class itself. I'd also note that there was a time in Log4j history before there was automatic type conversion in the annotated factory methods, so some old code still have all string parameters with manual type conversion. Also, why do I bring this up in EC2 and not structs or whatever the appropriate plugin is? Good question, but a new issue should likely be filed about it linked back here.

          Joseph Petersen (old) added a comment - - edited

          jvz I agree with you but for JCasC we only really care about the lovely data binding

          I would like to point out this lovely PR which could solve all our data binding problem and jelly problems at the same time: https://github.com/stapler/stapler/pull/147
          https://github.com/jenkinsci/jenkins/pull/3669

          Joseph Petersen (old) added a comment - - edited jvz I agree with you but for JCasC we only really care about the lovely data binding I would like to point out this lovely PR which could solve all our data binding problem and jelly problems at the same time: https://github.com/stapler/stapler/pull/147 https://github.com/jenkinsci/jenkins/pull/3669

          Matt Sicker added a comment -

          I'd love to get that merged, though my Stapler street cred hasn't been enough yet to gain committership yet. I hope to change that over the next couple months, though.

          Matt Sicker added a comment - I'd love to get that merged, though my Stapler street cred hasn't been enough yet to gain committership yet. I hope to change that over the next couple months, though.
          Oleg Nenashev made changes -
          Labels New: jcasc-compatibility
          Joseph Petersen made changes -
          Assignee Original: FABRIZIO MANFREDI [ thoulen ] New: Joseph Petersen [ jetersen ]
          Joseph Petersen made changes -
          Assignee Original: Joseph Petersen [ jetersen ] New: FABRIZIO MANFREDI [ thoulen ]

            thoulen FABRIZIO MANFREDI
            casz Joseph Petersen (old)
            Votes:
            1 Vote for this issue
            Watchers:
            8 Start watching this issue

              Created:
              Updated: