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
2017-07-06 13:16 GMT+02:00 Stephen Connolly <[email protected]
>:
>
>
> On 5 July 2017 at 18:10, Kohsuke Kawaguchi <[email protected]> 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.
>>
>> On Mon, Jul 3, 2017 at 5:21 AM Stephen Connolly <
>> [email protected]> 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
>>>
>>> (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
>>>
>>> --
>>> 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 [email protected].
>>> To view this discussion on the web visit https://groups.google.com/d/ms
>>> gid/jenkinsci-dev/CA%2BnPnMz-m49TK7Em%2BxBNb%2BV98dBCz9CrrP
>>> Xg3uW6%2B_x3KX5gOQ%40mail.gmail.com
>>> <https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMz-m49TK7Em%2BxBNb%2BV98dBCz9CrrPXg3uW6%2B_x3KX5gOQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
>>> .
>>> 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 [email protected].
>> To view this discussion on the web visit https://groups.google.com/d/ms
>> gid/jenkinsci-dev/CAN4CQ4wDS34DE-PQVK6tsCQD5AXWcxCMEVkCgh5MN
>> ymRNVcLPw%40mail.gmail.com
>> <https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4wDS34DE-PQVK6tsCQD5AXWcxCMEVkCgh5MNymRNVcLPw%40mail.gmail.com?utm_medium=email&utm_source=footer>
>> .
>>
>> 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/
> msgid/jenkinsci-dev/CA%2BnPnMwio9PeC3uhC_i4XqQ_7OY2dN2Hx34e6hSmHVAeG7RiRA%
> 40mail.gmail.com
> <https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMwio9PeC3uhC_i4XqQ_7OY2dN2Hx34e6hSmHVAeG7RiRA%40mail.gmail.com?utm_medium=email&utm_source=footer>
> .
>
> For more options, visit https://groups.google.com/d/optout.
>
--
Robert Sandell
*Software Engineer*
*CloudBees Inc.*
--
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 [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/jenkinsci-dev/CALzHZS10%2BzMD58BLJBiksxY7ch7sDQ7rPQEN9y-SJACGhREoTA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
