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 jenkinsci-dev+unsubscr...@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.
