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 On 3 July 2017 at 05:21, 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/msgid/jenkinsci-dev/CA%2BnPnMwHaGOVxav%2B%3D%3DTSej_J%3DKb5XMCSTN0aXp_DCVLjTBVQBA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
