[Proposal] Making DataBound components more declarative

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[Proposal] Making DataBound components more declarative

nicolas de loof-2
Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this : 

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}
(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

kinow
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

nicolas de loof-2


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznZKabShcA8N%2BctZo26D5uoK85v3u%2BzOUiUPv4oOfavcw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

Oleg Nenashev
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
  • It would be great to add support of custom Validator classes within the annotation
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="CqdosNKoAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkin...@googlegroups.com>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="CqdosNKoAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">nicolas...@...>
To: <a href="javascript:" target="_blank" gdf-obfuscated-mailto="CqdosNKoAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkin...@...
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented <a href="https://github.com/stapler/stapler/pull/140" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNG9oXGHu38gCBNue8M8iMFk_Q_0Ng&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNG9oXGHu38gCBNue8M8iMFk_Q_0Ng&#39;;return true;">https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="javascript:" target="_blank" gdf-obfuscated-mailto="CqdosNKoAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="javascript:" target="_blank" gdf-obfuscated-mailto="CqdosNKoAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

nicolas de loof-2


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

Oleg Nenashev

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

Baptiste MATHUS
In reply to this post by nicolas de loof-2
All for this. Great idea and initiative Nicolas!

Le jeu. 7 juin 2018 à 10:52, nicolas de loof <[hidden email]> a écrit :
Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this : 

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}
(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS4XkT-%2BTd6v_u%2BawEObnpRZW%3DMWzjNRN3P%2BpbjB4z9aKg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

nicolas de loof-2
In reply to this post by Oleg Nenashev
Got it, implemented as https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkc9DRa745jR%3DnxKa%3DQ-QvTV1LXcv1firUgTbTJSO9%3D5g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

James Nord-3

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as <a href="https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2F60945d14fe3da876ce5bd7da699e78f6062e8447\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNGqn9qWXUk1pkXjVmH62xRrdf-ZyQ&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2F60945d14fe3da876ce5bd7da699e78f6062e8447\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNGqn9qWXUk1pkXjVmH62xRrdf-ZyQ&#39;;return true;">https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="iY6Oc3KxAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">o.v.ne...@...>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

  • I meant <a href="https://github.com/ndeloof/stapler/blob/f905c693f174175cad0cf4478015591668a76a73/core/src/main/java/org/kohsuke/stapler/DataBound.java" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fndeloof%2Fstapler%2Fblob%2Ff905c693f174175cad0cf4478015591668a76a73%2Fcore%2Fsrc%2Fmain%2Fjava%2Forg%2Fkohsuke%2Fstapler%2FDataBound.java\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNH2tVyGP49782vMEBgEq2EUKAiCJg&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fndeloof%2Fstapler%2Fblob%2Ff905c693f174175cad0cf4478015591668a76a73%2Fcore%2Fsrc%2Fmain%2Fjava%2Forg%2Fkohsuke%2Fstapler%2FDataBound.java\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNH2tVyGP49782vMEBgEq2EUKAiCJg&#39;;return true;">https://github.com/ndeloof/stapler/blob/f905c693f174175cad0cf4478015591668a76a73/core/src/main/java/org/kohsuke/stapler/DataBound.java . This is not a javax.validation for sure :)
  • In order to get benefits from UI databinding, they will need to update the Jenkins core dependency. Same as DataBoundConstructor requires now
  • But, for the rest of use-cases (like JCasC), it will be possible to get benefit without updating the core
  • It is similar to the approach of @Symbol, which can be used in plugins without bumping the core to Pipeline/JobDSL-capable versions
BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="iY6Oc3KxAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">nicolas...@...> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="iY6Oc3KxAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">o.v.ne...@...>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (<a href="http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html" target="_blank" rel="nofollow" onmousedown="this.href=&#39;http://www.google.com/url?q\x3dhttp%3A%2F%2Fwww.captaindebug.com%2F2011%2F07%2Fwritng-jsr-303-custom-constraint_26.html\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNETlWMp6DmH9GJ0cgvJDrBoibSAYw&#39;;return true;" onclick="this.href=&#39;http://www.google.com/url?q\x3dhttp%3A%2F%2Fwww.captaindebug.com%2F2011%2F07%2Fwritng-jsr-303-custom-constraint_26.html\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNETlWMp6DmH9GJ0cgvJDrBoibSAYw&#39;;return true;">http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented <a href="https://github.com/stapler/stapler/pull/140" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNG9oXGHu38gCBNue8M8iMFk_Q_0Ng&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNG9oXGHu38gCBNue8M8iMFk_Q_0Ng&#39;;return true;">https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="javascript:" target="_blank" gdf-obfuscated-mailto="iY6Oc3KxAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium=email&amp;utm_source=footer" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit <a href="https://groups.google.com/d/optout" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit <a href="https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe&#39;;return true;">https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to <a href="javascript:" target="_blank" gdf-obfuscated-mailto="iY6Oc3KxAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium=email&amp;utm_source=footer" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit <a href="https://groups.google.com/d/optout" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="javascript:" target="_blank" gdf-obfuscated-mailto="iY6Oc3KxAgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium=email&amp;utm_source=footer" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit <a href="https://groups.google.com/d/optout" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

nicolas de loof-2


2018-06-07 15:23 GMT+02:00 James Nord <[hidden email]>:

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

so the question : is this a good thing to have "another piece of code" create a DataBound object ? Do you have any legitimate use case in mind ?
What you call "API" is just a public class within many because java don't let us do it any other way. Do we really want anything public to be considered "an API" ? (yes I know about @Restricted. Just would like this to be the default)

Another way to ask : Do you think JAX-B and JPA are all wrong in their design using field annotations ? 
I remember I had this exact debate with Hibernate-annotation folks some years ago, unfortunately can't remember the reasoning. Sounds to me some endless debate with various valuable argument on both sides.

So, some option I can offer :
  • allow this annotation both on fields and methods, just like JPA does.
  • make @DataBound a class level annotation, all fields (and setters) would then be considered for databinding (same as JPA's @Entity). DataBound would then imply a @Restricted annotation, to ensure nobody does bad things with this code.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.

So we have found this guy who read the doc :)
 
 
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

Part of this effort I've considered the jelly tags could be enhanced to integrate client side validation for comparable rules. Afaik the doCheck methods are mostly used to validate a parameter is valid within a specific context (like credentialsId), not to check they follow some formal design. But with custom jsr-330 annotation one could probably implement an alternative to doCheck methods. Not sure this would be a better design but feasible afaict
 

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzk-b-E1UwigfbMQ1SPJU3%2Br7hgPR-3Qtqf%3DBv4V%2BYtSzw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

nicolas de loof-2


2018-06-07 15:58 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:23 GMT+02:00 James Nord <[hidden email]>:

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

so the question : is this a good thing to have "another piece of code" create a DataBound object ? Do you have any legitimate use case in mind ?
What you call "API" is just a public class within many because java don't let us do it any other way. Do we really want anything public to be considered "an API" ? (yes I know about @Restricted. Just would like this to be the default)

Another way to ask : Do you think JAX-B and JPA are all wrong in their design using field annotations ? 
I remember I had this exact debate with Hibernate-annotation folks some years ago, unfortunately can't remember the reasoning. Sounds to me some endless debate with various valuable argument on both sides.

So, some option I can offer :
  • allow this annotation both on fields and methods, just like JPA does.


previous sample can be implemented when one prefer setters annotations as  :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}


 
  • make @DataBound a class level annotation, all fields (and setters) would then be considered for databinding (same as JPA's @Entity). DataBound would then imply a @Restricted annotation, to ensure nobody does bad things with this code.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.

So we have found this guy who read the doc :)
 
 
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

Part of this effort I've considered the jelly tags could be enhanced to integrate client side validation for comparable rules. Afaik the doCheck methods are mostly used to validate a parameter is valid within a specific context (like credentialsId), not to check they follow some formal design. But with custom jsr-330 annotation one could probably implement an alternative to doCheck methods. Not sure this would be a better design but feasible afaict
 

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.


--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkLS0DkMM%3DtXiKO65i93YRczdMOUtzebR2h10DsfZz2Tg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

nicolas de loof-2
b905c1fc allows use of @DataBound at class level, comparable to JPA's @Entity, also introducing @Required annotation

so you can use :

@DataBound
public static class DataBoundBean {

@Required @Positive
private int x;

@NotBlank
private String y;

@Trim
private String z;

which automatically makes the class @Restricted and excluded from any "API", considering such a class only is public to allow web UI data binding framework constraints

or if you want finer grain control :

public static class ValidatedBean {

@DataBound @Required @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}

with support both for field and setter based injection

2018-06-07 16:59 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:58 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:23 GMT+02:00 James Nord <[hidden email]>:

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

so the question : is this a good thing to have "another piece of code" create a DataBound object ? Do you have any legitimate use case in mind ?
What you call "API" is just a public class within many because java don't let us do it any other way. Do we really want anything public to be considered "an API" ? (yes I know about @Restricted. Just would like this to be the default)

Another way to ask : Do you think JAX-B and JPA are all wrong in their design using field annotations ? 
I remember I had this exact debate with Hibernate-annotation folks some years ago, unfortunately can't remember the reasoning. Sounds to me some endless debate with various valuable argument on both sides.

So, some option I can offer :
  • allow this annotation both on fields and methods, just like JPA does.


previous sample can be implemented when one prefer setters annotations as  :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}


 
  • make @DataBound a class level annotation, all fields (and setters) would then be considered for databinding (same as JPA's @Entity). DataBound would then imply a @Restricted annotation, to ensure nobody does bad things with this code.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.

So we have found this guy who read the doc :)
 
 
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

Part of this effort I've considered the jelly tags could be enhanced to integrate client side validation for comparable rules. Afaik the doCheck methods are mostly used to validate a parameter is valid within a specific context (like credentialsId), not to check they follow some formal design. But with custom jsr-330 annotation one could probably implement an alternative to doCheck methods. Not sure this would be a better design but feasible afaict
 

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.



--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmyYke%2B7VsGUOkAHzgL1ATGfz0JsfWYi-VDeKayhH7EVg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

Matt Sicker
I like the idea, but could it be more focused on annotating public members? Java 9+ discourages things like setAccessible() for reflection nonsense like this. I'd prefer to annotate the constructor and/or methods only. Or public fields I suppose, but why use those?

As for annotations for validations, since Java doesn't support higher kinded types, I think that's a nice compromise. Ideally the types themselves could enforce the constraints, but that's more of an academic topic still.

On Thu, Jun 7, 2018 at 10:24 AM, nicolas de loof <[hidden email]> wrote:
b905c1fc allows use of @DataBound at class level, comparable to JPA's @Entity, also introducing @Required annotation

so you can use :

@DataBound
public static class DataBoundBean {

@Required @Positive
private int x;

@NotBlank
private String y;

@Trim
private String z;

which automatically makes the class @Restricted and excluded from any "API", considering such a class only is public to allow web UI data binding framework constraints

or if you want finer grain control :

public static class ValidatedBean {

@DataBound @Required @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}

with support both for field and setter based injection


2018-06-07 16:59 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:58 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:23 GMT+02:00 James Nord <[hidden email]>:

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

so the question : is this a good thing to have "another piece of code" create a DataBound object ? Do you have any legitimate use case in mind ?
What you call "API" is just a public class within many because java don't let us do it any other way. Do we really want anything public to be considered "an API" ? (yes I know about @Restricted. Just would like this to be the default)

Another way to ask : Do you think JAX-B and JPA are all wrong in their design using field annotations ? 
I remember I had this exact debate with Hibernate-annotation folks some years ago, unfortunately can't remember the reasoning. Sounds to me some endless debate with various valuable argument on both sides.

So, some option I can offer :
  • allow this annotation both on fields and methods, just like JPA does.


previous sample can be implemented when one prefer setters annotations as  :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}


 
  • make @DataBound a class level annotation, all fields (and setters) would then be considered for databinding (same as JPA's @Entity). DataBound would then imply a @Restricted annotation, to ensure nobody does bad things with this code.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.

So we have found this guy who read the doc :)
 
 
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

Part of this effort I've considered the jelly tags could be enhanced to integrate client side validation for comparable rules. Afaik the doCheck methods are mostly used to validate a parameter is valid within a specific context (like credentialsId), not to check they follow some formal design. But with custom jsr-330 annotation one could probably implement an alternative to doCheck methods. Not sure this would be a better design but feasible afaict
 

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.



--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmyYke%2B7VsGUOkAHzgL1ATGfz0JsfWYi-VDeKayhH7EVg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.



--
Matt Sicker
Software Engineer, CloudBees

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAEot4oxtzuN2Kv%3DEFLzoyDZ41HEL3CCka5Btt30nWS11RjEkHg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

Liam Newman
This looks really cool.  Seems like there's also some interesting feedback on this. 

I'll look forward to seeing the resulting JEP for this.  

On Thursday, June 7, 2018 at 8:45:14 AM UTC-7, Matt Sicker wrote:
I like the idea, but could it be more focused on annotating public members? Java 9+ discourages things like setAccessible() for reflection nonsense like this. I'd prefer to annotate the constructor and/or methods only. Or public fields I suppose, but why use those?

As for annotations for validations, since Java doesn't support higher kinded types, I think that's a nice compromise. Ideally the types themselves could enforce the constraints, but that's more of an academic topic still.

On Thu, Jun 7, 2018 at 10:24 AM, nicolas de loof <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="px_4bU68AgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">nicolas...@...> wrote:
<a href="https://github.com/stapler/stapler/pull/140/commits/b905c1fc7f1129d80933b39c96beef0e235d8e11" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2Fb905c1fc7f1129d80933b39c96beef0e235d8e11\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFdnsyxDFZNCQKHQ55Lw3R2ys33sQ&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2Fb905c1fc7f1129d80933b39c96beef0e235d8e11\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFdnsyxDFZNCQKHQ55Lw3R2ys33sQ&#39;;return true;">b905c1fc allows use of @DataBound at class level, comparable to JPA's @Entity, also introducing @Required annotation

so you can use :

@DataBound
public static class DataBoundBean {

@Required @Positive
private int x;

@NotBlank
private String y;

@Trim
private String z;

which automatically makes the class @Restricted and excluded from any "API", considering such a class only is public to allow web UI data binding framework constraints

or if you want finer grain control :

public static class ValidatedBean {

@DataBound @Required @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}

with support both for field and setter based injection


2018-06-07 16:59 GMT+02:00 nicolas de loof <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="px_4bU68AgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">nicolas...@...>:


2018-06-07 15:58 GMT+02:00 nicolas de loof <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="px_4bU68AgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">nicolas...@...>:


2018-06-07 15:23 GMT+02:00 James Nord <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="px_4bU68AgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jn...@...>:

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

so the question : is this a good thing to have "another piece of code" create a DataBound object ? Do you have any legitimate use case in mind ?
What you call "API" is just a public class within many because java don't let us do it any other way. Do we really want anything public to be considered "an API" ? (yes I know about @Restricted. Just would like this to be the default)

Another way to ask : Do you think JAX-B and JPA are all wrong in their design using field annotations ? 
I remember I had this exact debate with Hibernate-annotation folks some years ago, unfortunately can't remember the reasoning. Sounds to me some endless debate with various valuable argument on both sides.

So, some option I can offer :
  • allow this annotation both on fields and methods, just like JPA does.

implemented as <a href="https://github.com/stapler/stapler/pull/140/commits/6c385a8d5bea147ccb6ffd7f87ad835c79e01b99" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2F6c385a8d5bea147ccb6ffd7f87ad835c79e01b99\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNEZhyQgApnQwl3fNIyfAS0ND4I1pw&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2F6c385a8d5bea147ccb6ffd7f87ad835c79e01b99\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNEZhyQgApnQwl3fNIyfAS0ND4I1pw&#39;;return true;">https://github.com/stapler/stapler/pull/140/commits/6c385a8d5bea147ccb6ffd7f87ad835c79e01b99

previous sample can be implemented when one prefer setters annotations as  :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}


 
  • make @DataBound a class level annotation, all fields (and setters) would then be considered for databinding (same as JPA's @Entity). DataBound would then imply a @Restricted annotation, to ensure nobody does bad things with this code.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.

So we have found this guy who read the doc :)
 
 
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

Part of this effort I've considered the jelly tags could be enhanced to integrate client side validation for comparable rules. Afaik the doCheck methods are mostly used to validate a parameter is valid within a specific context (like credentialsId), not to check they follow some formal design. But with custom jsr-330 annotation one could probably implement an alternative to doCheck methods. Not sure this would be a better design but feasible afaict
 

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as <a href="https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2F60945d14fe3da876ce5bd7da699e78f6062e8447\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNGqn9qWXUk1pkXjVmH62xRrdf-ZyQ&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2F60945d14fe3da876ce5bd7da699e78f6062e8447\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNGqn9qWXUk1pkXjVmH62xRrdf-ZyQ&#39;;return true;">https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

  • I meant <a href="https://github.com/ndeloof/stapler/blob/f905c693f174175cad0cf4478015591668a76a73/core/src/main/java/org/kohsuke/stapler/DataBound.java" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fndeloof%2Fstapler%2Fblob%2Ff905c693f174175cad0cf4478015591668a76a73%2Fcore%2Fsrc%2Fmain%2Fjava%2Forg%2Fkohsuke%2Fstapler%2FDataBound.java\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNH2tVyGP49782vMEBgEq2EUKAiCJg&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fndeloof%2Fstapler%2Fblob%2Ff905c693f174175cad0cf4478015591668a76a73%2Fcore%2Fsrc%2Fmain%2Fjava%2Forg%2Fkohsuke%2Fstapler%2FDataBound.java\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNH2tVyGP49782vMEBgEq2EUKAiCJg&#39;;return true;">https://github.com/ndeloof/stapler/blob/f905c693f174175cad0cf4478015591668a76a73/core/src/main/java/org/kohsuke/stapler/DataBound.java . This is not a javax.validation for sure :)
  • In order to get benefits from UI databinding, they will need to update the Jenkins core dependency. Same as DataBoundConstructor requires now
  • But, for the rest of use-cases (like JCasC), it will be possible to get benefit without updating the core
  • It is similar to the approach of @Symbol, which can be used in plugins without bumping the core to Pipeline/JobDSL-capable versions
BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (<a href="http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html" rel="nofollow" target="_blank" onmousedown="this.href=&#39;http://www.google.com/url?q\x3dhttp%3A%2F%2Fwww.captaindebug.com%2F2011%2F07%2Fwritng-jsr-303-custom-constraint_26.html\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNETlWMp6DmH9GJ0cgvJDrBoibSAYw&#39;;return true;" onclick="this.href=&#39;http://www.google.com/url?q\x3dhttp%3A%2F%2Fwww.captaindebug.com%2F2011%2F07%2Fwritng-jsr-303-custom-constraint_26.html\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNETlWMp6DmH9GJ0cgvJDrBoibSAYw&#39;;return true;">http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented <a href="https://github.com/stapler/stapler/pull/140" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNG9oXGHu38gCBNue8M8iMFk_Q_0Ng&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNG9oXGHu38gCBNue8M8iMFk_Q_0Ng&#39;;return true;">https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium=email&amp;utm_source=footer" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit <a href="https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe&#39;;return true;">https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium=email&amp;utm_source=footer" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium=email&amp;utm_source=footer" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="javascript:" target="_blank" gdf-obfuscated-mailto="px_4bU68AgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com?utm_medium=email&amp;utm_source=footer" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.

For more options, visit <a href="https://groups.google.com/d/optout" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.



--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="javascript:" target="_blank" gdf-obfuscated-mailto="px_4bU68AgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmyYke%2B7VsGUOkAHzgL1ATGfz0JsfWYi-VDeKayhH7EVg%40mail.gmail.com?utm_medium=email&amp;utm_source=footer" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmyYke%2B7VsGUOkAHzgL1ATGfz0JsfWYi-VDeKayhH7EVg%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmyYke%2B7VsGUOkAHzgL1ATGfz0JsfWYi-VDeKayhH7EVg%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmyYke%2B7VsGUOkAHzgL1ATGfz0JsfWYi-VDeKayhH7EVg%40mail.gmail.com.

For more options, visit <a href="https://groups.google.com/d/optout" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.



--
Matt Sicker
Software Engineer, CloudBees

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/26aa86e0-912c-4be4-82f0-556206c9dc43%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

nicolas de loof-2
In reply to this post by Matt Sicker


2018-06-07 17:45 GMT+02:00 Matt Sicker <[hidden email]>:
I like the idea, but could it be more focused on annotating public members? Java 9+ discourages things like setAccessible() for reflection nonsense like this. I'd prefer to annotate the constructor and/or methods only. Or public fields I suppose, but why use those?

Both are supported
Java 9 discourage setAccessible() but also provide a (better?) mechanism with variable handles. We probably could rely on this (need more investigations)
 

As for annotations for validations, since Java doesn't support higher kinded types, I think that's a nice compromise. Ideally the types themselves could enforce the constraints, but that's more of an academic topic still.

On Thu, Jun 7, 2018 at 10:24 AM, nicolas de loof <[hidden email]> wrote:
b905c1fc allows use of @DataBound at class level, comparable to JPA's @Entity, also introducing @Required annotation

so you can use :

@DataBound
public static class DataBoundBean {

@Required @Positive
private int x;

@NotBlank
private String y;

@Trim
private String z;

which automatically makes the class @Restricted and excluded from any "API", considering such a class only is public to allow web UI data binding framework constraints

or if you want finer grain control :

public static class ValidatedBean {

@DataBound @Required @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}

with support both for field and setter based injection


2018-06-07 16:59 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:58 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:23 GMT+02:00 James Nord <[hidden email]>:

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

so the question : is this a good thing to have "another piece of code" create a DataBound object ? Do you have any legitimate use case in mind ?
What you call "API" is just a public class within many because java don't let us do it any other way. Do we really want anything public to be considered "an API" ? (yes I know about @Restricted. Just would like this to be the default)

Another way to ask : Do you think JAX-B and JPA are all wrong in their design using field annotations ? 
I remember I had this exact debate with Hibernate-annotation folks some years ago, unfortunately can't remember the reasoning. Sounds to me some endless debate with various valuable argument on both sides.

So, some option I can offer :
  • allow this annotation both on fields and methods, just like JPA does.


previous sample can be implemented when one prefer setters annotations as  :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}


 
  • make @DataBound a class level annotation, all fields (and setters) would then be considered for databinding (same as JPA's @Entity). DataBound would then imply a @Restricted annotation, to ensure nobody does bad things with this code.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.

So we have found this guy who read the doc :)
 
 
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

Part of this effort I've considered the jelly tags could be enhanced to integrate client side validation for comparable rules. Afaik the doCheck methods are mostly used to validate a parameter is valid within a specific context (like credentialsId), not to check they follow some formal design. But with custom jsr-330 annotation one could probably implement an alternative to doCheck methods. Not sure this would be a better design but feasible afaict
 

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.



--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmyYke%2B7VsGUOkAHzgL1ATGfz0JsfWYi-VDeKayhH7EVg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.



--
Matt Sicker
Software Engineer, CloudBees

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAEot4oxtzuN2Kv%3DEFLzoyDZ41HEL3CCka5Btt30nWS11RjEkHg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkJkfGqxE3KxKVZKMw%3DVTcDW_r8GMS2yqx2Hh7C_KvAbA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

Matt Sicker
I haven't looked at var handles in a while, but from what I recall, those are replacements for using Unsafe.compareAndSet along with the other ops there. Personally, I try to use AtomicReference instead where possible, but that's not always the best way to go about implementing atomic operations (hence VarHandle).

On Thu, Jun 7, 2018 at 11:28 AM, nicolas de loof <[hidden email]> wrote:


2018-06-07 17:45 GMT+02:00 Matt Sicker <[hidden email]>:
I like the idea, but could it be more focused on annotating public members? Java 9+ discourages things like setAccessible() for reflection nonsense like this. I'd prefer to annotate the constructor and/or methods only. Or public fields I suppose, but why use those?

Both are supported
Java 9 discourage setAccessible() but also provide a (better?) mechanism with variable handles. We probably could rely on this (need more investigations)
 

As for annotations for validations, since Java doesn't support higher kinded types, I think that's a nice compromise. Ideally the types themselves could enforce the constraints, but that's more of an academic topic still.

On Thu, Jun 7, 2018 at 10:24 AM, nicolas de loof <[hidden email]> wrote:
b905c1fc allows use of @DataBound at class level, comparable to JPA's @Entity, also introducing @Required annotation

so you can use :

@DataBound
public static class DataBoundBean {

@Required @Positive
private int x;

@NotBlank
private String y;

@Trim
private String z;

which automatically makes the class @Restricted and excluded from any "API", considering such a class only is public to allow web UI data binding framework constraints

or if you want finer grain control :

public static class ValidatedBean {

@DataBound @Required @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}

with support both for field and setter based injection


2018-06-07 16:59 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:58 GMT+02:00 nicolas de loof <[hidden email]>:


2018-06-07 15:23 GMT+02:00 James Nord <[hidden email]>:

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

so the question : is this a good thing to have "another piece of code" create a DataBound object ? Do you have any legitimate use case in mind ?
What you call "API" is just a public class within many because java don't let us do it any other way. Do we really want anything public to be considered "an API" ? (yes I know about @Restricted. Just would like this to be the default)

Another way to ask : Do you think JAX-B and JPA are all wrong in their design using field annotations ? 
I remember I had this exact debate with Hibernate-annotation folks some years ago, unfortunately can't remember the reasoning. Sounds to me some endless debate with various valuable argument on both sides.

So, some option I can offer :
  • allow this annotation both on fields and methods, just like JPA does.


previous sample can be implemented when one prefer setters annotations as  :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

private String z;

@DataBound @Trim
public void setZ(String z) {
this.z = z;
}


 
  • make @DataBound a class level annotation, all fields (and setters) would then be considered for databinding (same as JPA's @Entity). DataBound would then imply a @Restricted annotation, to ensure nobody does bad things with this code.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.

So we have found this guy who read the doc :)
 
 
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

Part of this effort I've considered the jelly tags could be enhanced to integrate client side validation for comparable rules. Afaik the doCheck methods are mostly used to validate a parameter is valid within a specific context (like credentialsId), not to check they follow some formal design. But with custom jsr-330 annotation one could probably implement an alternative to doCheck methods. Not sure this would be a better design but feasible afaict
 

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]om.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.



--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmyYke%2B7VsGUOkAHzgL1ATGfz0JsfWYi-VDeKayhH7EVg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.



--
Matt Sicker
Software Engineer, CloudBees

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAEot4oxtzuN2Kv%3DEFLzoyDZ41HEL3CCka5Btt30nWS11RjEkHg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkJkfGqxE3KxKVZKMw%3DVTcDW_r8GMS2yqx2Hh7C_KvAbA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.



--
Matt Sicker
Software Engineer, CloudBees

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAEot4owUc%2BcDveh0JaDBL%2Bq_fXFWG8VLqvYvUNNP430u%3D0G%3DOA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

Keith Zantow-2
In reply to this post by nicolas de loof-2
Sorry to be the dissonant here, but @PostConstruct is invaluable/necessary in certain contexts, so use it!

On Thu, Jun 7, 2018 at 4:52 AM, nicolas de loof <[hidden email]> wrote:
Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this : 

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}
(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.



--

Keith Zantow
Senior Software Engineer
CloudBees, Inc.

CloudBees-Logo.png


E: [hidden email]

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAJTHQaHdtQ-54wEou18eNWxbcDcWpS4r7GtBM1kzuW0U9yg-1w%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

Jesse Glick-4
In reply to this post by nicolas de loof-2
On Thu, Jun 7, 2018 at 9:58 AM, nicolas de loof
<[hidden email]> wrote:
> Part of this effort I've considered the jelly tags could be enhanced to
> integrate client side validation for comparable rules.

Some controls _do_ have client-side validation already, though today
of course you must explicitly select that option in your
`config.jelly`. See for example `f:number` and CSS classes like
`INPUT.positive-number-required`. It would be nice if either client-
or server-side validation to match bean annotations were automatic.

Thinking a bit more broadly, it would be very helpful if we could
mechanically generate a basic form for a given bean just based on its
element structure and annotations. For example, last I checked the
Blue Ocean Pipeline editor hard-codes support for a bunch of `Step`s,
since the plugin code implementing the step only defines a
`config.jelly` and the B.O. UI cannot embed those controls in any way,
which means you are out of luck if your plugin is not in that special
list. While we could urge the maintainer of every plugin which defines
a `Step` to also define a B.O. view for its configuration, we could
get a lot more traction by having B.O. introspect the `Step`’s
parameters and their constraints and offer a way to configure each
automatically.

Really that is not so different from what we already need to do for
JEP-201 to offer good IDE completion schemas, static validation, and
the like; or what the static step reference

https://jenkins.io/doc/pipeline/steps/

does by ignoring `config.jelly` but paying attention to
`Descriptor.displayName` and `help*.html` files.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr2s748BNGBQFhxbQLE_iq-N6X9FNpAWjKn45TOQs8_UrA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

nicolas de loof-2
Your perfectly right, this experiment was initially driven by the need to "document" the UI binding model in some discoverable way for use in Configuration-as-Code (JEP-201). The fact it allows to reduce boiler-plate coding is a bonus (but a nice one). I've been frustrated with DataBound model for a while, but here I have good reasons to propose an alternative :D

I haven't yet explored jelly tag integration, but I expect using:
<f:entry field="foo">
would allow to discover many details about target attribute (or setter) then add adequate classes, or maybe even use specific input type (number, date, time, range, etc).

I also like your idea to dynamically generate a "default" config.jelly file for a DataBound component, sounds perfectly feasible.

2018-06-07 20:44 GMT+02:00 Jesse Glick <[hidden email]>:
On Thu, Jun 7, 2018 at 9:58 AM, nicolas de loof
<[hidden email]> wrote:
> Part of this effort I've considered the jelly tags could be enhanced to
> integrate client side validation for comparable rules.

Some controls _do_ have client-side validation already, though today
of course you must explicitly select that option in your
`config.jelly`. See for example `f:number` and CSS classes like
`INPUT.positive-number-required`. It would be nice if either client-
or server-side validation to match bean annotations were automatic.

Thinking a bit more broadly, it would be very helpful if we could
mechanically generate a basic form for a given bean just based on its
element structure and annotations. For example, last I checked the
Blue Ocean Pipeline editor hard-codes support for a bunch of `Step`s,
since the plugin code implementing the step only defines a
`config.jelly` and the B.O. UI cannot embed those controls in any way,
which means you are out of luck if your plugin is not in that special
list. While we could urge the maintainer of every plugin which defines
a `Step` to also define a B.O. view for its configuration, we could
get a lot more traction by having B.O. introspect the `Step`’s
parameters and their constraints and offer a way to configure each
automatically.

Really that is not so different from what we already need to do for
JEP-201 to offer good IDE completion schemas, static validation, and
the like; or what the static step reference

https://jenkins.io/doc/pipeline/steps/

does by ignoring `config.jelly` but paying attention to
`Descriptor.displayName` and `help*.html` files.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr2s748BNGBQFhxbQLE_iq-N6X9FNpAWjKn45TOQs8_UrA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmw7hqemwQbcPhvNV3vZd_2fRQ4VL2CuTiYfC1dkXRKxg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Making DataBound components more declarative

James Nord-3
In reply to this post by nicolas de loof-2


On Thursday, June 7, 2018 at 2:59:08 PM UTC+1, nicolas de loof wrote:


2018-06-07 15:23 GMT+02:00 James Nord <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="3O3tXoS2AgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jn...@...>:

I didn't look at the implementation but IMO the annotations must go on the setters (or parameters) and not on private fields, and then the checks need to be done on the setters or (injected into the constructor)

Why?

Because things other than DataBinding create these objects (ie calling the API directly from another piece of code).  and if I can vilaote a requirement from code that would cause the UI or stapler to fail a round trip then that is a bad bug.

so the question : is this a good thing to have "another piece of code" create a DataBound object ?

Why not?  Why should I not be able to programatically generate a collection of some things based on some runtime / environmental state in a plugin?

Do you have any legitimate use case in mind ?

several all in a proprietary code base you have access to :)

But also, unit tests (JenkinsRule), why should a unit test not fail with an error IllegalArgumentExcetion "MyClass.foo 10 > 4" if it tries to set a value greater than the allowed version?
 
What you call "API" is just a public class within many because java don't let us do it any other way. Do we really want anything public to be considered "an API" ? (yes I know about @Restricted. Just would like this to be the default)


That is what Jenkins project defines as an API as that is why we are stuck with not removing anything for the past 10 years ;0
 
Another way to ask : Do you think JAX-B and JPA are all wrong in their design using field annotations ? 

Well yes.  If the annotation is of interest to an external party then there is no excuse for not documenting it.  (the colum name etc are of interest to hiberante only and not code so setting a column name of bob on an field is fair game).
 
I remember I had this exact debate with Hibernate-annotation folks some years ago, unfortunately can't remember the reasoning. Sounds to me some endless debate with various valuable argument on both sides.

So, some option I can offer :
  • allow this annotation both on fields and methods, just like JPA does.
  • make @DataBound a class level annotation, all fields (and setters) would then be considered for databinding (same as JPA's @Entity). DataBound would then imply a @Restricted annotation, to ensure nobody does bad things with this code.

Also the public setters are documented as part of the javadoc but private fields are not, and as such the annotaions if defined on something public would then also be documented.

So we have found this guy who read the doc :)

I'm sure there are others but yes - I find it quicker to go to javadoc.jenkins.io and to read a formatted page than decipher raw HTML, or find the bits in an IDE.
 
 
 
Tied to this I was wondering how this ties in the any doCheckXYZ() methods?  does it need to be repeated will they be generated for formbinding?

Part of this effort I've considered the jelly tags could be enhanced to integrate client side validation for comparable rules. Afaik the doCheck methods are mostly used to validate a parameter is valid within a specific context (like credentialsId), not to check they follow some formal design.

They can be for anything you want :-)  (I think the JNLP port uses a do check to check the number is not negative, there is certainly something somewhere that does something like that)
 
But with custom jsr-330 annotation one could probably implement an alternative to doCheck methods. Not sure this would be a better design but feasible afaict
 

but +100 for writing less code.
Regards

/James

On Thursday, June 7, 2018 at 1:26:14 PM UTC+1, nicolas de loof wrote:
Got it, implemented as <a href="https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2F60945d14fe3da876ce5bd7da699e78f6062e8447\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNGqn9qWXUk1pkXjVmH62xRrdf-ZyQ&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140%2Fcommits%2F60945d14fe3da876ce5bd7da699e78f6062e8447\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNGqn9qWXUk1pkXjVmH62xRrdf-ZyQ&#39;;return true;">https://github.com/stapler/stapler/pull/140/commits/60945d14fe3da876ce5bd7da699e78f6062e8447

As a result, my initial sample would now look like :

public static class DataBoundBean {

@DataBound(required = true) @Positive
private int x;

@DataBound @NotBlank
private String y;

@DataBound @Trim
private String z;




2018-06-07 13:34 GMT+02:00 Oleg Nenashev <[hidden email]>:

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway

@NonNull makes no sense for primitive types, so they are quite orthogonal IMHO.
I would rather add "required" and maybe "defaultValue" annotations similarly to what Jackson databind and args4j do in Jenkins.

  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?

  • I meant <a href="https://github.com/ndeloof/stapler/blob/f905c693f174175cad0cf4478015591668a76a73/core/src/main/java/org/kohsuke/stapler/DataBound.java" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fndeloof%2Fstapler%2Fblob%2Ff905c693f174175cad0cf4478015591668a76a73%2Fcore%2Fsrc%2Fmain%2Fjava%2Forg%2Fkohsuke%2Fstapler%2FDataBound.java\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNH2tVyGP49782vMEBgEq2EUKAiCJg&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fndeloof%2Fstapler%2Fblob%2Ff905c693f174175cad0cf4478015591668a76a73%2Fcore%2Fsrc%2Fmain%2Fjava%2Forg%2Fkohsuke%2Fstapler%2FDataBound.java\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNH2tVyGP49782vMEBgEq2EUKAiCJg&#39;;return true;">https://github.com/ndeloof/stapler/blob/f905c693f174175cad0cf4478015591668a76a73/core/src/main/java/org/kohsuke/stapler/DataBound.java . This is not a javax.validation for sure :)
  • In order to get benefits from UI databinding, they will need to update the Jenkins core dependency. Same as DataBoundConstructor requires now
  • But, for the rest of use-cases (like JCasC), it will be possible to get benefit without updating the core
  • It is similar to the approach of @Symbol, which can be used in plugins without bumping the core to Pipeline/JobDSL-capable versions
BR, Oleg







On Thu, Jun 7, 2018 at 1:15 PM, nicolas de loof <[hidden email]> wrote:


2018-06-07 13:06 GMT+02:00 Oleg Nenashev <[hidden email]>:
Yes, such annotation would help a lot with our current issues in the code.
So +1 from me to implement it.

Some notes:
  • I would suggest a "required" attribute in @DataBound which defaults to true
I also considered this, but wouldn't this be equivalent to jsr-303 annotation @NotNull ? Can do both anyway
  • I would recommend moving annotations to a separate library which we could include into plugins so that they do not require new core dependency to start putting annotations
    • New core will be still required to do binding from Web UI forms
    • Configuration-as-Code plugin and Pipeline can start using the annotations even on older cores
this is plain javax.validation API, so already a separate library
So you suggest plugin developer will mark fields with those annotations, without getting benefits for UI databinding ?
 
  • It would be great to add support of custom Validator classes within the annotation
Already the case, jsr-303 do support custom validators (<a href="http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html" rel="nofollow" target="_blank" onmousedown="this.href=&#39;http://www.google.com/url?q\x3dhttp%3A%2F%2Fwww.captaindebug.com%2F2011%2F07%2Fwritng-jsr-303-custom-constraint_26.html\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNETlWMp6DmH9GJ0cgvJDrBoibSAYw&#39;;return true;" onclick="this.href=&#39;http://www.google.com/url?q\x3dhttp%3A%2F%2Fwww.captaindebug.com%2F2011%2F07%2Fwritng-jsr-303-custom-constraint_26.html\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNETlWMp6DmH9GJ0cgvJDrBoibSAYw&#39;;return true;">http://www.captaindebug.com/2011/07/writng-jsr-303-custom-constraint_26.html)
  • It would be great to also define some conversion logic on the top level, so that classes can implement migration logic should the data model change (like readResolve)
    • Use-case: support old submissions when a field is removed (add field converter class or so)

Good point. This would be needed for Configuraiton-as-Code backward compatibility as well. I guess a @PostConstruct method would be fine for this purpose. just mark legacy field as (transient) @Deprecated, but keep @DataBound annotation so that binding still happens.
 

BR, Oleg


On Thursday, June 7, 2018 at 11:48:11 AM UTC+2, nicolas de loof wrote:


2018-06-07 11:37 GMT+02:00 'Bruno P. Kinoshita' via Jenkins Developers <[hidden email]>:
The code looks much nicer, and developers probably will have already used jsr-303 in some project.

Had  a brief look at the code and it looks good (minor nit-pick for the @author tag in one of the files, maybe drop it or replace kohsuke by your user?).
good catch 

Would it also work if a plugin developer tried to implement a custom ConstraintValidator?

yes indeed. this is plain JSR 303 reference implementation used internally 
 


+1

Thanks
Bruno




________________________________
From: nicolas de loof <[hidden email]>
To: [hidden email]
Sent: Thursday, 7 June 2018 8:52 PM
Subject: [Proposal] Making DataBound components more declarative



Hi there

I've noticed most Jenkins components, when then don't parse json stream for UI data binding with human-written code, do only rely on a DataBoundConstructor with some @Nonnull annotation as sole definition of constraints on data (I don't even think Stapler will prevent passing null by the way).

Such constructor then has to invoke various Util.trimToNull / checkForNull / etc to convert and validate incoming data. As a result we don't expose a "model" that could benefit a better UI nor can be reused in another context (I have configuration-as-code in mind, but this would also be useful for Pipeline IIUC)

I suggest we embrace bean-validation (jsr-303) to declare model constraints, and as well offer some generic mechanisms so a maximum boiler-plate data filtering is managed by stappler based on declarative constraints.

For this purpose, I've implemented <a href="https://github.com/stapler/stapler/pull/140" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNG9oXGHu38gCBNue8M8iMFk_Q_0Ng&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Fstapler%2Fstapler%2Fpull%2F140\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNG9oXGHu38gCBNue8M8iMFk_Q_0Ng&#39;;return true;">https://github.com/stapler/stapler/pull/140

This allows to write a DataBound component as :

public static class DataBoundBean {

@DataBound @Positive @Max(100)
private int x;

@DataBound @NotBlank
private String y;

@DataBound(trim = DataBound.Trim.TONULL)
private String z;
}

The equivalent DataBound class with current Stapler codebase would look like this :

public static class DataBoundBean {

private int x;
private final String y;
private String z;

@DataBoundConstructor
public DataBoundBean(int x, @Nonnull String y, String z) {
if (x < 0 || x > 100) throw new IllegalArgumentException("x must be positive < 100");
if (StringUtils.isBlank(y)) throw new IllegalArgumentException("y can't be blank");
this.x = x;
this.y = y;
this.z = StringUtils.trimToNull(z);
}
}(or alternatively use DataBoundSetters and a @PostConstruct method for conversion/validation)

What do you think about this ?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzkhxCcspfcNxZJTwcw%3Dg1QcmEz9n9k4homP6rXvaqYSNA%40mail.gmail.com.
For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/1172002936.1614566.1528364231154%40mail.yahoo.com.
For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium=email&amp;utm_source=footer" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/8c888e0f-7fdc-4712-bc53-4ee66388a65e%40googlegroups.com.

For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Jenkins Developers" group.
To unsubscribe from this topic, visit <a href="https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe&#39;;return true;">https://groups.google.com/d/topic/jenkinsci-dev/Bb4pIdpMMIY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium=email&amp;utm_source=footer" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznpvHuQaE%2BODejhrv5H1OuzJVM26piimBirDn9j2bsf-A%40mail.gmail.com.

For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium=email&amp;utm_source=footer" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/CAPfivLByg0xPfzOOzjYDe_4i%3DmJC0nM1EjPsYYTduTHG_GmFiA%40mail.gmail.com.

For more options, visit <a href="https://groups.google.com/d/optout" rel="nofollow" target="_blank" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to <a href="javascript:" target="_blank" gdf-obfuscated-mailto="3O3tXoS2AgAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit <a href="https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com?utm_medium=email&amp;utm_source=footer" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com?utm_medium\x3demail\x26utm_source\x3dfooter&#39;;return true;">https://groups.google.com/d/msgid/jenkinsci-dev/99356e40-a6ab-483a-bcdc-ed25de0f0db0%40googlegroups.com.

For more options, visit <a href="https://groups.google.com/d/optout" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;" onclick="this.href=&#39;https://groups.google.com/d/optout&#39;;return true;">https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/77220835-5b84-4902-b2d6-9faafa4086cf%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
12