[Proposal] Making all things Stapler more declarative

[Stephen Connolly]

I have been developing Jenkins plugins and changes to Jenkins core for more than 10 years now. As such, I have internalized a lot of the knowledge about how to develop against Jenkins and more specifically against Stapler.

When I talk to people trying to start out development against Jenkins, the magic of Stapler seems to be a big source of confusion - at least to the people I have talked to.

Prospective developers get confused:

• between primary facets and facet fragments.
  
• where to put facets and facet fragments.
  
• which facet fragments are optional and which ones are mandatory
  

(Did you even know that those jelly / groovy views are called facets and that there are two kinds?)

Some of those concerns affect me too... but I am a seasoned Jenkins developer CMD+SHIFT+P, *, ENTER and IntelliJ is showing me a full list of facets and facet fragments from the class hierarchy and I can search each one looking for the <st:include page="fragment" optional="true"> to discover that there is an optional facet fragment... and then try and dig through the Jelly / Groovy logic to determine when and why that fragment should be included.

It doesn't just stop there... There are those methods that we expect Stapler to invoke for us... typically they are the doFillXYZItems() or doCheckXYZ() methods. I find myself having to mark them up like:

@SuppressWarnings("unused") // stapler

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

So that my IDE stops complaining about the dead code. It's not a big deal in the grand scheme of things, but I'd much rather have an annotation on the method to signify that we expect the method to be used by stapler... Right now I could do that using

@WebMethod(name="checkWidgetValue")

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

If I don't like that, I guess I could use the newer HTTP verb annotations in stapler:

@GET // stapler

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

But the simple @GET is not screaming out stapler to me, so I feel compelled to add a // stapler comment to remind myself that the annotation is used by stapler

Then we hit the actual name that stapler exposes things on. There are some conventions that stapler follows... and they are magic conventions... so much so that I just keep http://stapler.kohsuke.org/reference.html as the first bookmark in my bookmark bar.

I think we can do better. I think we should do better, and here is my proposal.

We should introduce some annotations. Make them available against older versions of Jenkins so that we don't have to wait for everyone to update their plugin baseline. The annotations can be added as a plugin dependency in the parent pom, that everyone can use them just by upgrading the plugin parent pom.

Here are the class annotations I think we need:

• An annotation (I propose @StaplerObject) that says "This object is expected to bound to a URL by Stapler, the Stapler annotations are complete, check that there are no facets / fragments without a corresponding annotation and no magic on this class please" (so if there is, say a rename.jelly but no @StaplerFacet("rename") or @StaplerFragment("rename") then you get an error)
  
• An annotation (I propose @StaplerFacet and the repeatable container @StaplerFacets) that says "Here are the facets that this object is expected to have" (doesn't have to be on a @StaplerObject but when not on a @StaplerObject we can only check for missing expected facets, we cannot check for "unexpected" facets - i.e. I created a facet with a spelling mistake or typo in the name)
  
• An annotation (I propose @StaplerFragment and the repeatable container @StaplerFragments) that says "Here are the facet fragments that this object is required or may optionally have" (quite often will not be on a @StaplerObject)
  

So I envision something like this (pre-Java 8):

@StaplerObject

@StaplerFacets({

  @StaplerFacet("index"),

  @StaplerFacet("config")

})

@StaplerFragments({

  @StaplerFragment("main"),

  @StaplerFragment("side-panel"),

  @StaplerFragment(value="tasks",optional=true)

})

public class Widget { ... }

When compiled targeting Java 8 this would become:

@StaplerObject

@StaplerFacet("index")

@StaplerFacet("config")

@StaplerFragment("main")

@StaplerFragment("side-panel")

@StaplerFragment(value="tasks",optional=true)

public class Widget { ... }

There would be an annotation processor that could do some checks:

• If you add @StaplerFragment(..., optional=false) then any non-abstract class must have that fragment somewhere in the class hierarchy
  
• If you add @StaplerFacet then any non-abstract class must have that fragment somewhere in the class hierarchy
  
• If you add @StaplerObject then only the named facets and facet fragments must be present for that class (to catch somebody calling the facet resource with an incorrect name - it should be side-panel2.jelly but they called it sidepanel2.jelly)
  

Then, while we are at it, I'd also like to add method / field annotations:

• An annotation (I propose @StaplerPath and the repeatable container @StaplerPaths) that says "here is the URL segment names that this method / field matches when Stapler is binding the URL". This annotation can be used to enable easier refactoring without risk of breaking the URLs because we can rename the getters and keep the URL scheme as before or even keep the legacy URLs but direct to the new URLs via the UI.
  
• Annotations (I propose @StaplerHEAD / @StaplerGET / @StaplerPOST / @StaplerPUT / @StaplerPATCH / @StaplerDELETE plus non standard methods supported with @StaplerMethod("...") and then a repeatable container of @StaplerMethods - the repeatable container would only take @StaplerMethod annotations though) that cover the different HTTP methods (a.k.a. verbs) - we have some existing, but they need some extensions and I think it might be a good idea to consolidate the names, especially if we want to allow consumption on older baseline versions of Jenkins

So this could be something like this (Pre Java 8):

public class Widget {

  @StaplerPath

  public Manchu manchu;

 

  @StaplerPaths({@StaplerPath,@StaplerPath("fu")})

  public Foo foo;

 

  @StaplerPath

  public Bar getBar() { ... }

 

  @StaplerPath(StaplerPath.DYNAMIC)

  public Object getDynamic(StaplerRequest req) { ... }

 

  @StaplerPOST

  public HttpResponse doActivate(StaplerRequest req) { ... }

 

  @StaplerPath(StaplerPath.DYNAMIC)

  @StaplerPUT

  public HttpResponse doDynamic(StaplerRequest req) { ... }

}

Or targeting Java 8:

public class Widget {

  @StaplerPath

  public Manchu manchu;

 

  @StaplerPath

  @StaplerPath("fu")

  public Foo foo;

 

  @StaplerPath

  public Bar getBar() { ... }

 

  @StaplerPath(StaplerPath.DYNAMIC)

  public Object getDynamic(StaplerRequest req) { ... }

 

  @StaplerPOST

  public HttpResponse doActivate(StaplerRequest req) { ... }

 

  @StaplerPath(StaplerPath.DYNAMIC)

  @StaplerPUT

  public HttpResponse doDynamic(StaplerRequest req) { ... }

}

The annotation processor can then give additional checks:

• @StaplerPath annotated fields must be public
  
• @StaplerPath(altName) annotated fields are only allowed if targeting a new version Stapler that adds support for that (unless the name is the inferred name)
  
• @StaplerPath annotated methods must be public and either match one of the allowed getter signatures or also have a HTTP method annotation and match the allowed action method signatures.
  
• @StaplerPath(altName) annotated methods are only allowed if targeting a new version Stapler that adds support for that (unless the name is the inferred name)
  
• @StaplerHEAD / @StaplerGET / @StaplerPOST / @StaplerPUT / @StaplerPATCH / @StaplerDELETE annotated methods must be public and match one of the allowed action method signatures. The method name must match the convention for action method inference (subject to @WebMethod(name="...") overriding when compiling against older versions of Stapler - but it would be an error to use @WebMethod if compiling against newer versions of Stapler because we can move to the new annotations in those cases)
  
• If the class is @StaplerObject annotated we can do additional checks for overlapping facets, for example where an action method or a getter will hide a facet (it's ok to hide the facet for non HEAD/GET requests though)

There are probably additional checks that we can add as we figure them out.

Now there are some open questions:

• Does it make sense to start all these annotations with Stapler? My initial stab at this was to use @Staple in place of @StaplerPath and to re-use the @GET / @POST / @PUT / @DELETE verbs but mandating the @Staple annotation so that you would have:

@Staple @POST

public HttpResponse doActivate(StaplerRequest req) { ... }

rather than

@StaplerPOST

public HttpResponse doActivate(StaplerRequest req) { ... }

One of the issues I found with that is class conflicts when back-porting the @GET / @POST / etc annotations, so we could only use those if we were prepared to mark 2.7 as the oldest core that could use the annotations, whereas we can make new annotations in a separate package available as far back as Stapler 1.180 == Jenkins 1.455 (or worst case Stapler 1.237 == Jenkins 1.651)

Now we could introduce new annotations without the @Stapler prefix, but then your code complete would confuse you between @org.kohsuke.stapler.annotations.GET and @org.kohsuke.stapler.verbs.GET plus we still need something to give a hint that the annotation is implying Stapler's involvement (at least from my perspective)

• I had thought about saying that the class level @StaplerObject should be enough of a flag that you should expect the other annotations... but the class annotation will be inherited, so you may not see it until you look at the super-class... and @Path will conflict with java.nio.file.Path... that might work with @Staple (and @Staples as the container) but while cute they do not scream to the new developers that these are involved in url binding.

NOTE: In this context I see this change as being entirely opt-in. Plugin developers should not have to go adding the annotations - though we would probably apply them in Jenkins core to assist new developers.

• If we go with the Stapler prefix, would it make sense to consolidate all the annotations with that prefix? 
  
  
• I am not convinced for the case of @ExportedBean and @Exported, these reflect a different cross-cutting concern and as such the pair seem named well from my PoV
• The @JavaScriptMethod / @WithWellKnownURL annotations might be worthy of consolidation if we can determine better names. 
• The parameter binding annotations such as @AncestorInPath, @Header, @QueryParameter and @InjectedParameter I think are fine where they are as they will only be on an action method which already has the @StaplerPOST etc indicator. 
• The meta-annotation @InterceptorAnnotation should not be consolidated, in any case we could not consolidate it and retain usage against older cores
• The other annotations are likely rarely used, but if we can find good names and people think they are generically useful rather than single use-case hacks added into the stapler API, I think consolidation might make the features more widely used. 

So, over to you the community of Jenkins developers:

1. Would this change have helped you get started quicker?
   
2. Would this change help you even now?
   
3. How ugly are my proposed annotation names? Can you provide a less ugly scheme of names?
   
4. Anything else?

Stephen

[Robert Sandell]

> Would this change have helped you get started quicker?

Yes, If I can remember that far back I believe it would have helped, 90% of learning how to code plugins is looking at what others have done before.

> Would this change help you even now?

Yes, even though my brain usually reads "stapler method" on any doXXX method having them "marked up" would help more with code reviews and while planning an implementation and especially refactoring.

> How ugly are my proposed annotation names? Can you provide a less ugly scheme of names?

I think they are OK, can't come up with anything better :)

> Anything else?

So with this we could perhaps enhance the existing tooling even more, like some intelligent inspections in the IDEA plugin and perhaps even a "go to facet" and "create facet" :)

/B

--
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-dev+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMz-m49TK7Em%2BxBNb%2BV98dBCz9CrrPXg3uW6%2B_x3KX5gOQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--

Robert Sandell

Software Engineer

CloudBees Inc.

[Stephen Connolly]

Ulli,

What do you think on this proposal, given that you have contact with a lot of students who have been trying to get to grips with the Jenkins code base.

Would it make their task easier?

-Stephen

[Stephen Connolly]

Actually I nearly forgot Oleg you were driving the Google summer of code last year, what do you think?

--

Sent from my phone

[Stephen Connolly]

+jenkins-dev

On 5 July 2017 at 02:13, Ullrich Hafner <ullrich...@gmail.com> wrote:

Am 04.07.2017 um 18:44 schrieb Stephen Connolly <stephen.alan.connolly@gmail.com>:

Ulli,

What do you think on this proposal, given that you have contact with a lot of students who have been trying to get to grips with the Jenkins code base.

Would it make their task easier?

Up to now most students work in the area of testing (ATH), so this will not help them (they would love to see IDs for all entry fields, but that is a totally different topic).

So one aspect where this could help is in knowing what to test.

The annotations can be used to help identify the intent of the plugin author / core and we could perhaps even develop tooling to identify ATH coverage

 

-Stephen

On 3 July 2017 at 05:21, Stephen Connolly <stephen.alan.connolly@gmail.com> wrote:

I have been developing Jenkins plugins and changes to Jenkins core for more than 10 years now. As such, I have internalized a lot of the knowledge about how to develop against Jenkins and more specifically against Stapler.

When I talk to people trying to start out development against Jenkins, the magic of Stapler seems to be a big source of confusion - at least to the people I have talked to.

Prospective developers get confused:

• between primary facets and facet fragments.
  
• where to put facets and facet fragments.
  
• which facet fragments are optional and which ones are mandatory
  

This indeed is one of the most confusing things when developing plug-ins. It cost a lot of time to get these things right (mostly by copying and pasting code from another plugin). 

I'm not sure if annotations are the correct way of dealing with these problems. I think you then need to know where to put an annotation and where not, this will also result in searching for other plugins that do it right. 

Yes, I would also propose improving the stapler documentation in conjunction with these changes so that it should be easier to find the "recommended" patterns to follow.

Though how much energy I personally have to push documentation improvements is another question entirely ;-)

 

(Did you even know that those jelly / groovy views are called facets and that there are two kinds?)

Some of those concerns affect me too... but I am a seasoned Jenkins developer CMD+SHIFT+P, *, ENTER and IntelliJ is showing me a full list of facets and facet fragments from the class hierarchy and I can search each one looking for the <st:include page="fragment" optional="true"> to discover that there is an optional facet fragment... and then try and dig through the Jelly / Groovy logic to determine when and why that fragment should be included.

It doesn't just stop there... There are those methods that we expect Stapler to invoke for us... typically they are the doFillXYZItems() or doCheckXYZ() methods. I find myself having to mark them up like:

@SuppressWarnings("unused") // stapler

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

So that my IDE stops complaining about the dead code. It's not a big deal in the grand scheme of things, but I'd much rather have an annotation on the method to signify that we expect the method to be used by stapler... Right now I could do that using

@WebMethod(name="checkWidgetValue")

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

If I don't like that, I guess I could use the newer HTTP verb annotations in stapler:

@GET // stapler

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

But do these annotations help to get rid of the unused code warnings? I think you still need to add a @SuppressWarnings.

Well you can configure IDEs to say "this annotation is used" and we can even have the Stapler plugin for IntelliJ pre-register the annotations so that IntelliJ users (I'm selfish ;-) ) don't even need to add the setting

 

Would there be some checks, that check for the correct path for the corresponding index.jelly, config.jelly, global.jelly and so on? 

Yes, the idea is that when you annotate a non-abstract class with @StaplerFacet we would check you have the facet in your class hierarchy and error out.

If you annotated your non-abstract class with @StaplerObject then we can go one further and error out if you have an unexpected .jelly or .groovy file which gives you even more help to avoid creating resources in the wrong place.

Because of taglibs, and semi-static resources we cannot scan the classpath for .jelly and .groovy files and assume that they are either facets or fragments which is why the "is a complete set" checks require @StaplerObject

 

Now there are some open questions:

• Does it make sense to start all these annotations with Stapler? My initial stab at this was to use @Staple in place of @StaplerPath and to re-use the @GET / @POST / @PUT / @DELETE verbs but mandating the @Staple annotation so that you would have:

@Staple @POST

public HttpResponse doActivate(StaplerRequest req) { ... }

rather than

@StaplerPOST

public HttpResponse doActivate(StaplerRequest req) { ... }

One of the issues I found with that is class conflicts when back-porting the @GET / @POST / etc annotations, so we could only use those if we were prepared to mark 2.7 as the oldest core that could use the annotations, whereas we can make new annotations in a separate package available as far back as Stapler 1.180 == Jenkins 1.455 (or worst case Stapler 1.237 == Jenkins 1.651)

Now we could introduce new annotations without the @Stapler prefix, but then your code complete would confuse you between @org.kohsuke.stapler.annotations.GET and @org.kohsuke.stapler.verbs.GET plus we still need something to give a hint that the annotation is implying Stapler's involvement (at least from my perspective)

• I had thought about saying that the class level @StaplerObject should be enough of a flag that you should expect the other annotations... but the class annotation will be inherited, so you may not see it until you look at the super-class... and @Path will conflict with java.nio.file.Path... that might work with @Staple (and @Staples as the container) but while cute they do not scream to the new developers that these are involved in url binding.

NOTE: In this context I see this change as being entirely opt-in. Plugin developers should not have to go adding the annotations - though we would probably apply them in Jenkins core to assist new developers.

• If we go with the Stapler prefix, would it make sense to consolidate all the annotations with that prefix? 
  
  
• I am not convinced for the case of @ExportedBean and @Exported, these reflect a different cross-cutting concern and as such the pair seem named well from my PoV
• The @JavaScriptMethod / @WithWellKnownURL annotations might be worthy of consolidation if we can determine better names. 
• The parameter binding annotations such as @AncestorInPath, @Header, @QueryParameter and @InjectedParameter I think are fine where they are as they will only be on an action method which already has the @StaplerPOST etc indicator. 
• The meta-annotation @InterceptorAnnotation should not be consolidated, in any case we could not consolidate it and retain usage against older cores
• The other annotations are likely rarely used, but if we can find good names and people think they are generically useful rather than single use-case hacks added into the stapler API, I think consolidation might make the features more widely used. 

So, over to you the community of Jenkins developers:

1. Would this change have helped you get started quicker?
   
2. Would this change help you even now?
   

I’m not sure if it would help to get started quicker. I think the UI mapping in Jenkins is still (too?) complex even with these annotations. Maybe it would make sense if you can show for an example plug-in class (and jelly file) how it will look after using the annotations? 

Yes, my next step is to mock up the proposed annotations and apply to one or two sample plugins as a (do not merge) PR.

Do you have any plugins or plugin classes you would like to suggest as good examples to trial?

3. How ugly are my proposed annotation names? Can you provide a less ugly scheme of names?
   

The names are fine.

Thanks, one of the concerns I have is that we need some rough consensus on the names as they are liable to become a bikeshed painting exercise 

4. Anything else?

Stephen

[Ullrich Hafner]

Am 05.07.2017 um 11:27 schrieb Stephen Connolly <stephen.al...@gmail.com>:

+jenkins-dev

Uups, sorry… 

On 5 July 2017 at 02:13, Ullrich Hafner <ullrich...@gmail.com> wrote:

Am 04.07.2017 um 18:44 schrieb Stephen Connolly <stephen.alan.connolly@gmail.com>:

Ulli,

What do you think on this proposal, given that you have contact with a lot of students who have been trying to get to grips with the Jenkins code base.

Would it make their task easier?

Up to now most students work in the area of testing (ATH), so this will not help them (they would love to see IDs for all entry fields, but that is a totally different topic).

So one aspect where this could help is in knowing what to test.

The annotations can be used to help identify the intent of the plugin author / core and we could perhaps even develop tooling to identify ATH coverage

 

-Stephen

On 3 July 2017 at 05:21, Stephen Connolly <stephen.alan.connolly@gmail.com> wrote:

I have been developing Jenkins plugins and changes to Jenkins core for more than 10 years now. As such, I have internalized a lot of the knowledge about how to develop against Jenkins and more specifically against Stapler.

When I talk to people trying to start out development against Jenkins, the magic of Stapler seems to be a big source of confusion - at least to the people I have talked to.

Prospective developers get confused:

• between primary facets and facet fragments.
  
• where to put facets and facet fragments.
  
• which facet fragments are optional and which ones are mandatory
  

This indeed is one of the most confusing things when developing plug-ins. It cost a lot of time to get these things right (mostly by copying and pasting code from another plugin). 

I'm not sure if annotations are the correct way of dealing with these problems. I think you then need to know where to put an annotation and where not, this will also result in searching for other plugins that do it right. 

Yes, I would also propose improving the stapler documentation in conjunction with these changes so that it should be easier to find the "recommended" patterns to follow.

Though how much energy I personally have to push documentation improvements is another question entirely ;-)

Well, this is always a problem ;-) But we already have some good examples in the wiki so the documentation of these new concepts might grow in the same way by some motivated plugin authors ...

 

(Did you even know that those jelly / groovy views are called facets and that there are two kinds?)

Some of those concerns affect me too... but I am a seasoned Jenkins developer CMD+SHIFT+P, *, ENTER and IntelliJ is showing me a full list of facets and facet fragments from the class hierarchy and I can search each one looking for the <st:include page="fragment" optional="true"> to discover that there is an optional facet fragment... and then try and dig through the Jelly / Groovy logic to determine when and why that fragment should be included.

It doesn't just stop there... There are those methods that we expect Stapler to invoke for us... typically they are the doFillXYZItems() or doCheckXYZ() methods. I find myself having to mark them up like:

@SuppressWarnings("unused") // stapler

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

So that my IDE stops complaining about the dead code. It's not a big deal in the grand scheme of things, but I'd much rather have an annotation on the method to signify that we expect the method to be used by stapler... Right now I could do that using

@WebMethod(name="checkWidgetValue")

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

If I don't like that, I guess I could use the newer HTTP verb annotations in stapler:

@GET // stapler

public FormValidation doCheckWidgetValue(@QueryParameter String value) {

    ...

}

But do these annotations help to get rid of the unused code warnings? I think you still need to add a @SuppressWarnings.

Well you can configure IDEs to say "this annotation is used" and we can even have the Stapler plugin for IntelliJ pre-register the annotations so that IntelliJ users (I'm selfish ;-) ) don't even need to add the setting

That would be very helpful! (Isn’t everybody using IntelliJ to develop plugins?;-)

Maybe improving the Stapler plug-in would be a good candidate for a bachelor/master thesis… (Code completion in jelly views would be awesome)

 

But the simple @GET is not screaming out stapler to me, so I feel compelled to add a // stapler comment to remind myself that the annotation is used by stapler

Then we hit the actual name that stapler exposes things on. There are some conventions that stapler follows... and they are magic conventions... so much so that I just keep http://stapler.kohsuke.org/reference.html as the first bookmark in my bookmark bar.

I think we can do better. I think we should do better, and here is my proposal.

We should introduce some annotations. Make them available against older versions of Jenkins so that we don't have to wait for everyone to update their plugin baseline. The annotations can be added as a plugin dependency in the parent pom, that everyone can use them just by upgrading the plugin parent pom.

Here are the class annotations I think we need:

• An annotation (I propose @StaplerObject) that says "This object is expected to bound to a URL by Stapler, the Stapler annotations are complete, check that there are no facets / fragments without a corresponding annotation and no magic on this class please" (so if there is, say a rename.jelly but no @StaplerFacet("rename") or @StaplerFragment("rename")then you get an error)
  

My plugins are typically too complex as a starting point for beginners (due to the analysis-core dependency and the number of implemented extension points), so it might be better to start with a simpler one. 

[Kanstantsin Shautsou]

On Tuesday, July 4, 2017 at 8:25:06 PM UTC+3, Stephen Connolly wrote:

Actually I nearly forgot Oleg you were driving the Google summer of code last year, what do you think?

And as usual seems you forgot github-plugin and Kirill that was GSOC mentor for some UI changes and already covered with annotations some pieces https://github.com/jenkinsci/github-plugin/blob/93d40692ff3866705175624e93ec584d4ac88132/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java#L182-L186  

[Jesse Glick]

On Wed, Jul 5, 2017 at 6:50 PM, Kanstantsin Shautsou
<kanstan...@gmail.com> wrote:
> https://github.com/jenkinsci/github-plugin/blob/93d40692ff3866705175624e93ec584d4ac88132/src/main/java/org/jenkinsci/plugins/github/admin/GitHubHookRegisterProblemMonitor.java#L182-L186

These are not really related; they are just other interceptor
annotations. (Which I do not really advocate, either—you should not
use annotations where plain old method calls are just as readable, and
far more flexible and debuggable.)

[Oleg Nenashev]

Hi,

Generally +1 from me for Stapler annotations. It adds many extra opportunities:

1. Self-documentation and Jenkins.io Documentation, e.g. via Annotation Indexer
• It will also allow to generate some Swagger specs for Stapler-annotated API calls if somebody is ready to work on that
  
2. Extra static analysis capabilities in default Parent POM
   
• I have a draft of a Jenkins-specific detectors library, but I put it on hold to see how FindBugs/SpotBugs story finishes
3. Ability to automatically generate some injected tests in JTH
   

Regarding the GSoC context, I would say that we had 95% of organizational issues and 5% of technical issues in the project. So I do not expect it to help it much, but OTOH new documentation would help with onboarding new developers.

So +1 from me

Best regards,
Oleg

четверг, 6 июля 2017 г., 0:50:59 UTC+2 пользователь Kanstantsin Shautsou написал:

[Kanstantsin Shautsou]

Not really, but imho almost all plugins had/has security issues with unclear stapler endpoints and simple annotations for stapler method may be better for automated security checks. I think it related for stapler/binding understanding also. 

--
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/UrVVT8wbHIE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr2ShkBiyLgxO18eK%2BKtecKMR5tKWbNfYHiOcc-HqgEG4g%40mail.gmail.com.

[Kohsuke Kawaguchi]

FWIW, I've already discussed this offline with Stephen and I'm generally +1.

One suggestion I had for him is to consider using a Java method definition instead of @StaplerFacet and @StaplerFragment. That is, instead of this:

@StaplerFacets({

  @StaplerFacet("index"),

  @StaplerFacet("config")

})

@StaplerFragments({

  @StaplerFragment("main"),

  @StaplerFragment("side-panel"),

  @StaplerFragment(value="tasks",optional=true)

})

public class Widget { ... }

Consider that:

public class Widget {

    @StaplerFacet protected void index() {}

    @StaplerFacet protected void config() {}

    @StaplerFragment protected abstract void main();

    @StaplerFragment("side-panel") protected abstract void sidepanel();

    @StaplerFragment protected void tasks();

}

I liked this better because ...

• It fits with the original design thinking behind Stapler, which is that views are like methods. Their inheritance and override semantics are all modeled after methods.
• It avoids a long list of @StaplerFragment/@StaplerFacet at the top of the class declaration. We have some objects in the core that has quite a few views.
• It creates a nice to place to describe view as javadoc.
• 'abstract' and 'final' provides convenient semantics and javac does that work for us. One less thing to do for our annotation processor

Obvious downside is that those are not real methods.

--

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/CA%2BnPnMz-m49TK7Em%2BxBNb%2BV98dBCz9CrrPXg3uW6%2B_x3KX5gOQ%40mail.gmail.com.

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

--

Kohsuke Kawaguchi

[Stephen Connolly]

On 5 July 2017 at 18:10, Kohsuke Kawaguchi <k...@kohsuke.org> wrote:

FWIW, I've already discussed this offline with Stephen and I'm generally +1.

One suggestion I had for him is to consider using a Java method definition instead of @StaplerFacet and @StaplerFragment. That is, instead of this:

@StaplerFacets({

  @StaplerFacet("index"),

  @StaplerFacet("config")

})

@StaplerFragments({

  @StaplerFragment("main"),

  @StaplerFragment("side-panel"),

  @StaplerFragment(value="tasks",optional=true)

})

public class Widget { ... }

Consider that:

public class Widget {

    @StaplerFacet protected void index() {}

    @StaplerFacet protected void config() {}

    @StaplerFragment protected abstract void main();

    @StaplerFragment("side-panel") protected abstract void sidepanel();

    @StaplerFragment protected void tasks();

}

Well at least this is a better explanation than you gave me verbally ;-)

 

I liked this better because ...

• It fits with the original design thinking behind Stapler, which is that views are like methods. Their inheritance and override semantics are all modeled after methods.
• It avoids a long list of @StaplerFragment/@StaplerFacet at the top of the class declaration. We have some objects in the core that has quite a few views.

So let's see:

$ ( cd core/src/main/resources ; find . -name \*.jelly -exec dirname {} \; ; find . -name \*.groovy -exec dirname {} \; ) | sort | uniq -c | grep -v ./lib/ | sort -n | tail -n 10

   7 ./hudson/model/AbstractProject

   7 ./hudson/slaves/SlaveComputer

   7 ./jenkins/install/SetupWizard

   9 ./hudson/model/Run

  11 ./hudson/PluginManager

  11 ./hudson/security/HudsonPrivateSecurityRealm

  12 ./hudson/model/Job

  13 ./hudson/model/View

  14 ./hudson/model/Computer

  25 ./jenkins/model/Jenkins 

So there are not altogether too many classes in Jenkins having lots of fragments / facets

I think an argument based on a couple of really big classes like Jenkins having 25 lines of 

@StaplerObject

@StaplerFacet("_restart")

@StaplerFacet("_safeRestart")

@StaplerFacet("_script")

@StaplerFacet("_scriptText")

@StaplerFacet("accessDenied")

@StaplerFacet("configure")

@StaplerFacet("configureExecutors")

@StaplerFacet("fingerprintCheck")

@StaplerFacet("legend")

@StaplerFacet("load-statistics")

@StaplerFacet("login")

@StaplerFacet("loginError")

@StaplerFacet("manag_")

@StaplerFacet("manage")

@StaplerFacet("newView")

@StaplerFacet("noPrincipal")

@StaplerFacet("oops")

@StaplerFacet("opensearch.xml")

@StaplerFacet("projectRelationship-help")

@StaplerFacet("projectRelationship")

@StaplerFacet("systemInfo")

@StaplerFacet("threadDump")

@StaplerFragment("_api")

@StaplerFragment("downgrade")

@StaplerFragment("sidepanel")

public class Jenkins { 

  ...

}

compared with

@StaplerObject

public class Jenkins { 

  @StaplerFacet protected void _restart() {}

  @StaplerFacet protected void _safeRestart() {}

  @StaplerFacet protected void _script() {}

  @StaplerFacet protected void _scriptText() {}

  @StaplerFacet protected void accessDenied() {}

  @StaplerFacet protected void configure() {}

  @StaplerFacet protected void configureExecutors() {}

  @StaplerFacet protected void fingerprintCheck() {}

  @StaplerFacet protected void legend() {}

  @StaplerFacet @StaplerPath("load-statistics") protected void loadStatistics() {}

  @StaplerFacet protected void login() {}

  @StaplerFacet protected void loginError() {}

  @StaplerFacet protected void manag_() {}

  @StaplerFacet protected void manage() {}

  @StaplerFacet protected void newView() {}

  @StaplerFacet protected void noPrincipal() {}

  @StaplerFacet protected void oops() {}

  @StaplerFacet @StaplerPath("opensearch.xml") protected void opensearchXml() {}

  @StaplerFacet @StaplerPath("projectRelationship-help") protected void projectRelationshipHelp()

  @StaplerFacet protected void projectRelationship() {}

  @StaplerFacet protected void systemInfo() {}

  @StaplerFacet protected void threadDump() {}

  @StaplerFragment protected void _api() {}

  @StaplerFragment protected void downgrade() {}

  @StaplerFragment protected void sidepanel() {}

  ...

}

just to get the javadoc comments and leverage abstract and final... I really do not like the extra effort of typing in all those method names...

Perhaps the biggest issue for me is that to retrofit that into Jenkins we would need to take great care that these methods we are adding as facet and fragment markers do not have names that may potentially conflict with subclasses...

If we add

@StaplerObject

public abstract class Descriptor ... {

  ...

  @StaplerFragment protected abstract void config(); // doesn't really matter if abstract or not here

  ...

}

we may make it impossible for a concrete subclass to have a 

public MyDescriptor extends Descriptor ... {

  private MyConfig config;

  ...

  public MyConfig config() {

    return config.clone(); 

  }

}

as they will be unable to retain binary compatibility with their existing "config" method when they upgrade...

So that means we will need to do

@StaplerObject

public abstract class Descriptor ... {

  ...

  @StaplerFragment @StaplerPath("config") protected abstract void _retrofit_config();

  ...

}

Thinking some more, I see issues with plugin authors being thereby forced to add implementations when they upgrade core... ok so they are quick and the IDE will help... but having to add all those

public MyDescriptor extends Descriptor ... {

  private MyConfig config;

  ...

  public MyConfig config() {

    return config.clone(); 

  }

  @StaplerFragment @StaplerPath("config") protected void _retrofit_config() {}

}

All the while hoping that the IDE has copied the method annotations (otherwise the method is not annotated) and the plugin author has not opted into @StaplerObject yet

That, to me, does not make for a compelling developer experience.

WDYT?

I am inclined to reject your method based idea and stick with class level annotations... there are only 11 classes in all of Jenkins core with more than 5 facets / fragments

• It creates a nice to place to describe view as javadoc.
• 'abstract' and 'final' provides convenient semantics and javac does that work for us. One less thing to do for our annotation processor

Obvious downside is that those are not real methods.

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMz-m49TK7Em%2BxBNb%2BV98dBCz9CrrPXg3uW6%2B_x3KX5gOQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--

Kohsuke Kawaguchi

--

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-dev+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4wDS34DE-PQVK6tsCQD5AXWcxCMEVkCgh5MNymRNVcLPw%40mail.gmail.com.

[Robert Sandell]

I agree with Stephen here; adding methods seems risky and could break existing plugins, at least in a backwards compat sense.

But I at the same time do like the idea of getting javadoc for the facets and fragments. So can we get both somehow? 

E.g. if there is a @StaplerFacet("login") or @StaplerFragment("main") there should be a corresponding 

@facet login the login page.

@fragment main add this to change the main part of the configuration page.

in the javadoc of the class?

/B

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMwio9PeC3uhC_i4XqQ_7OY2dN2Hx34e6hSmHVAeG7RiRA%40mail.gmail.com.

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

--

[Stephen Connolly]

Yes I was thinking that a custom javadoc handler would be useful. Especially as there are inheritance of facets and fragments that we'd want to see documented correctly.

On 7 July 2017 at 02:37, Robert Sandell <rsan...@cloudbees.com> wrote:

I agree with Stephen here; adding methods seems risky and could break existing plugins, at least in a backwards compat sense.

But I at the same time do like the idea of getting javadoc for the facets and fragments. So can we get both somehow? 

E.g. if there is a @StaplerFacet("login") or @StaplerFragment("main") there should be a corresponding 

@facet login the login page.

@fragment main add this to change the main part of the configuration page.

in the javadoc of the class?

/B

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CALzHZS10%2BzMD58BLJBiksxY7ch7sDQ7rPQEN9y-SJACGhREoTA%40mail.gmail.com.

[Stephen Connolly]

Ok, so that people can see code rather than just yapping on the M-L (and given that we seem to be OK with the annotation names)

https://github.com/stapler/stapler/pull/121

Is where I am working on this proposal. We can continue the discussion in either place as appropriate (general comments here, specific comments on specific details of the code in the PR)