Quietly ignore interfaces in IndexHtmlDispatcher, avoiding NPE. #110
Conversation
There was a problem hiding this comment.
🐛 I I understand the issue correctly, it used to work. And since there is no documentation about this case AFAIK, there may be regressions in other Jenkins plugins.
If there is no plan about restoring support of Resources from interfaces, there should be at least explicit log message for it and maybe a notice in https://github.com/stapler/stapler/blob/master/CHANGELOG.md.
| @@ -36,7 +36,7 @@ public String toString() { | |||
| * Returns a {@link IndexHtmlDispatcher} if and only if the said class has {@code index.html} as a side-file | |||
| */ | |||
| static Dispatcher make(ServletContext context, Class c) { | |||
There was a problem hiding this comment.
I would suggest documenting the null case in Javadoc/Annotations here and in Calling methods just to make sure nobody misuses the API.
There was a problem hiding this comment.
Do you mean this method returning null? That should be a valid scenario in case no index.html is found.
There was a problem hiding this comment.
Nope, I just mean the documentation.
E.g.
/**
* Returns a {@link IndexHtmlDispatcher} if and only if the said class has {@code index.html} as a side-file
* @param c Class to be used. For {@code null} (e.g. interface) the returned value will be {@code null} as well
* @return Index dispatcher or {@code null} if it cannot be found
*/
@CheckForNull
static Dispatcher make(ServletContext context, @CheckForNull Class c) {
|
@oleg-nenashev I might be wrong, but this does not ignore the interface per se but avoids the inspection of the class hierarchy, so my understanding is this should restore the previous behaviour. |
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
|
@varyvol It is great if it fixes the issue :) Just misunderstood the PR then. Added the Javadoc enhancement proposal above. If you write a direct unit test for the case, it will be perfect |
| ServletContext context = mock(ServletContext.class); | ||
| when(context.getResource(any(String.class))).thenReturn(null); | ||
|
|
||
| IndexHtmlDispatcher.make(context, TestInterface.class); |
|
@oleg-nenashev @jglick comments addressed. |
|
@reviewbybees done |
|
@oleg-nenashev @jglick do you know who might merge this? |
|
@varyvol The merge can be done by several people, @vivek and @kohsuke are the maintainers AFAIK. I pinged them both on Friday regarding the release. We may want to also backport the fix to 2.46.3, but it will require a custom branch of Stapler (to be also decided by the maintainers). The release can be done by @kohsuke only. |
|
I am able to cut releases. Anyway this PR needs no immediate release, much less backport, since it is merely a robustness fix and the plugin bug was already corrected. |
Issue #109
Interfaces do not have a superclass and thus a NPE exception was happening.
@reviewbybees @jglick @oleg-nenashev