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

0 views
Skip to first unread message

josephp90@gmail.com (JIRA)

unread,
May 17, 2019, 12:35:03 AM5/17/19
to jenkinsc...@googlegroups.com
Joseph Petersen created an issue
 
Jenkins / Task JENKINS-57513
Split the large databound constructors into databound setters
Issue Type: Task Task
Assignee: FABRIZIO MANFREDI
Components: ec2-plugin
Created: 2019-05-17 00:34
Priority: Minor Minor
Reporter: Joseph Petersen

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

Add Comment Add Comment
 
This message was sent by Atlassian Jira (v7.11.2#711002-sha1:fdc329d)

josephp90@gmail.com (JIRA)

unread,
May 17, 2019, 12:38:03 AM5/17/19
to jenkinsc...@googlegroups.com

josephp90@gmail.com (JIRA)

unread,
May 17, 2019, 12:39:01 AM5/17/19
to jenkinsc...@googlegroups.com
Joseph Petersen edited a comment on Task JENKINS-57513
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.

josephp90@gmail.com (JIRA)

unread,
May 17, 2019, 12:50:02 AM5/17/19
to jenkinsc...@googlegroups.com

fabrizio.manfredi@gmail.com (JIRA)

unread,
May 17, 2019, 6:41:03 AM5/17/19
to jenkinsc...@googlegroups.com

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

boards@gmail.com (JIRA)

unread,
May 17, 2019, 3:32:03 PM5/17/19
to jenkinsc...@googlegroups.com

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.

josephp90@gmail.com (JIRA)

unread,
May 17, 2019, 4:04:02 PM5/17/19
to jenkinsc...@googlegroups.com

Matt Sicker 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

boards@gmail.com (JIRA)

unread,
May 17, 2019, 4:07:03 PM5/17/19
to jenkinsc...@googlegroups.com

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.

josephp90@gmail.com (JIRA)

unread,
May 17, 2019, 4:11:02 PM5/17/19
to jenkinsc...@googlegroups.com
Joseph Petersen edited a comment on Task JENKINS-57513
[~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

o.v.nenashev@gmail.com (JIRA)

unread,
May 24, 2019, 5:53:02 PM5/24/19
to jenkinsc...@googlegroups.com

josephp90@gmail.com (JIRA)

unread,
Mar 7, 2020, 6:10:19 AM3/7/20
to jenkinsc...@googlegroups.com
Joseph Petersen assigned an issue to Joseph Petersen
Change By: Joseph Petersen
Assignee: FABRIZIO MANFREDI Joseph Petersen
This message was sent by Atlassian Jira (v7.13.12#713012-sha1:6e07c38)
Atlassian logo

josephp90@gmail.com (JIRA)

unread,
Mar 7, 2020, 6:24:03 AM3/7/20
to jenkinsc...@googlegroups.com
Joseph Petersen assigned an issue to FABRIZIO MANFREDI
Change By: Joseph Petersen
Assignee: Joseph Petersen FABRIZIO MANFREDI

josephp90@gmail.com (JIRA)

unread,
Mar 16, 2020, 8:27:03 AM3/16/20
to jenkinsc...@googlegroups.com
Joseph Petersen updated an issue
Change By: Joseph Petersen
Attachment: ec2-slave-template-2020-03-16 092547.png

josephp90@gmail.com (JIRA)

unread,
Mar 16, 2020, 8:27:03 AM3/16/20
to jenkinsc...@googlegroups.com

josephp90@gmail.com (JIRA)

unread,
Mar 16, 2020, 8:29:02 AM3/16/20
to jenkinsc...@googlegroups.com

raihaan.shouhell@autodesk.com (JIRA)

unread,
Mar 16, 2020, 9:06:02 AM3/16/20
to jenkinsc...@googlegroups.com

Hey Joseph, I agree we really need to fix this issue.

The whole class(SlaveTemplate) needs a rework so that not everything is final so setters would work.

 

Matt Sicker I'm not sure how builder patterns will help this since I don't think casc will support a builder

msicker@cloudbees.com (JIRA)

unread,
Mar 16, 2020, 2:57:03 PM3/16/20
to jenkinsc...@googlegroups.com

A builder pattern is the proper Java way to do it. Since nobody has implemented support for that in the JSON binding in Stapler, it seems like the suggested pattern is to use 90's-style JavaBeans with mutable getters/setters. Essentially, any data binding parameters that are required should be in the constructor, and any that are optional (i.e., every single new option added since the first release technically) should be added as setters so that the class can still be designed to work if that setter isn't invoked (i.e., providing default behavior).

bochenski.kuba+jenkins@gmail.com (JIRA)

unread,
Mar 16, 2020, 3:19:03 PM3/16/20
to jenkinsc...@googlegroups.com

Can you please leave the old constructors in place. I'm already using groovys scripts to configure the plugin.
Splitting out into setters is a welcome change, but it sucks to have to update your scripts for a new release.

josephp90@gmail.com (JIRA)

unread,
Mar 16, 2020, 7:53:03 PM3/16/20
to jenkinsc...@googlegroups.com

Why not offer a builder pattern and proper data bound setters, so that those in groovy can use the builder pattern and JCasC and stapler can use data binding.

msicker@cloudbees.com (JIRA)

unread,
Mar 16, 2020, 7:57:03 PM3/16/20
to jenkinsc...@googlegroups.com

The point of the builder pattern was to allow for a single (private) mega-constructor that continues to change as the class changes, but centralizing the configuration in its own builder class. Alternatively, if all these parameters are moved into their own anemic domain model class, then having everything be set up via getters and setters wouldn't be as painful. The general idea here is about setting up the state of the object by the time it's constructed so that you can avoid representations of invalid state.

raihaan.shouhell@autodesk.com (JIRA)

unread,
Mar 18, 2020, 8:47:03 AM3/18/20
to jenkinsc...@googlegroups.com

We might have to make a breaking change here since everything is public to give jcasc its databinding we need to remove the final keyword but since fields are public they now become directly modifiable. We probably should bite the bullet and change them to be private and provide setters / getters? WDYT: Matt Sicker FABRIZIO MANFREDI

msicker@cloudbees.com (JIRA)

unread,
Mar 18, 2020, 2:16:04 PM3/18/20
to jenkinsc...@googlegroups.com

Due to how many people already have pipelines referring to those constructors, it might be worth trying to deprecate that class entirely with a compatibility layer to the new class.

Reply all
Reply to author
Forward
0 new messages