getBuildCauses(...) performs safer checks for determining if the specified cause type is a subclass of the current build Cause

[mikecirioli]

getBuildCauses(...) now uses the uber classloader so that classes provided by other extension points can be found. Also provides sane error handling when a CNF exception does happen (logs a warning and returns an empty JSONArray)

Removed the use of class loading altogether in favor of doing direct comparisons against class names, recursively checking parent class. Feedback appreciated if there is a better way to handle inner$classes

[mikecirioli]

@jtnord @svanoort @jglick

[jtnord]

return causeClassName.equals(classNameToCheck) incase the user enters "java.lang.Object")?

[mikecirioli]

And also to exit the recursion...

[dwnusbaum]

Object.class.getSuperclass will return null, so maybe easier to use that as the base case and handle Object with the next if just like other classes.

[jtnord]

if the cause is foo.bar.Wibble$Cause would you not expect the user to enter that? Are there any instances of this, worth writing a test for?

[mikecirioli]

I'll look into writing a test but i think its already covered. Take the case of hudson.build.Cause$UserCause - if the user wants to check to see if a cause is of that type, they have to enter that string. I found that clazz.getClass().getSuperClass().getName() was returning java.lang.Object when it was passed to the next level of recursion (which makes sense i believe) which is why i added the check to pass clazz.getEnclosingClass() when needed. It is a bit odd to me though, because class.getClass().getSuperClass().getName() returns the enclosing class when evaluated in the ide (prior to being passed to the next level of the recursion).

[mikecirioli]

test:

workflow-support-plugin/src/test/java/org/jenkinsci/plugins/workflow/support/steps/build/RunWrapperTest.java

Line 268 in 152dcd6

+ "assert currentBuild.getBuildCauses('hudson.model"

[dwnusbaum]

I'm also confused by this check. It seems like you'd never want to check the enclosing class, since that is just about source-level organization, not about the actual sub/superclass relationships which should be handled by the recursive calls to getSuperclass().

It is a bit odd to me though, because class.getClass().getSuperClass().getName() returns the enclosing class when evaluated in the ide

In the case of hudson.build.Cause$UserCause, UserCause directly extends hudson.build.Cause, so hudson.build.Cause is both the superclass and the enclosing class. Is that what was happening?

Also, you can have a class with $ in the name literally, which would be broken by this check (e.g. public class User$Cause { ... } in User$Cause.java), although I'd be surprised to see it in practice.

[jglick]

I think this clause can be dropped.

[jtnord]

looks like it will work, but left a comment or two

[jglick]

I think this clause can be dropped.

[jglick]

Note that you are not checking implemented interfaces here. Which is fine IMO—I am not sure even checking superclasses is necessary, since I know of no meaningful hierarchy among Causes.

[mikecirioli]

I am starting to think that this has become overly complex, and that the most likely use case would be for checking for an exact match (ie. show me only causes of type hudson.model.Cause$UserIdCause)

[mikecirioli]

In the spirit of YAGNI, i'm closing this PR in favor of #85, which provides for simple filtering of build causes by a specific type.