JIRA 5.2.8 breaks web resource transformers

Symptoms: in JIRA 5.2.8, web resources provided by plugins (via WebResourceTransformer) are broken and don't work, because the meaning of DownloadableResource.isResourceModified() has been reversed. An example is Structure Plugin, which serves localized JS strings through web transformer. In 5.2.8, the transformer gives zero content, resulting in https://jira.almworks.com/browse/HJ-1182

The cause of the change is likely JRA-31665

------

Investigation:

1. The code that serves DownloadableResource – PluginResourceDownload.serveFile()

atlassian-plugins-webresource:2.10.1

            if(downloadableResource.isResourceModified(request, response))
            {
                log.info("Plugin Resource has been modified since plugin was loaded. Skipping: " + requestUri);
                return;
            }

atlassian-plugins-webresource:2.13.4

            // Don't try and calculate isResourceModified if it's a cacheable resource - answer is always false.
            if(cacheableResource && !downloadableResource.isResourceModified(request, response))
            {
                // 304
                return;
            }

As you can see:

• the meaning is reversed in 2.13.4, NOT is added to the condition - this breaks plugins;
• in version 2.10.1, the physical meaning is in fact "is resource not modified", because it shouldn't be served if it's NOT been modified
• in version 2.10.1, the author of the log message seems to have no idea what's this all about, the message does not make sense whatsoever
• in version 2.13.4, the author of the comment also has problems describing the logic, because it contradicts the code that follows

2. Let's take a look at the API - DownloadableResource.isResourceModified(). Funny thing, it hasn't changed from 2.10.1 to 2.13.4:

    /**
     * Returns true if the plugin resource has been modified. The implementing class is responsible for
     * setting any appropriate response codes or headers on the response.
     *
     * If the resource has been modified, the resource shouldn't be served. 
     */
    boolean isResourceModified(HttpServletRequest request, HttpServletResponse response);

• Clearly, the same problems with logic here. If the resource has been modified, then it should be served. But the comment says the opposite.
• But, this broken logic was supported by the correct implementation of the broken logic in 2.10.1 - so everything worked for plugins whose authors had worked this out. In 2.13.4, you have broken implementation of the broken logic.

3. Now, looking at a standard implementation of a DownloadableResource - AbstractDownloadableResource.isResourceModified(). The mess continues.

atlassian-plugins-webresource:2.10.1

    /**
     * Checks any "If-Modified-Since" header from the request against the
     * plugin's loading time, since plugins can't be modified after they've been
     * loaded this is a good way to determine if a plugin resource has been
     * modified or not. If this method returns true, don't do any more
     * processing on the request -- the response code has already been set to
     * "304 Not Modified" for you, and you don't need to serve the file.
     */
    public boolean isResourceModified(final HttpServletRequest httpServletRequest, final HttpServletResponse httpServletResponse)
    {
        final Date resourceLastModifiedDate = (plugin.getDateLoaded() == null) ? new Date() : plugin.getDateLoaded();
        final LastModifiedHandler lastModifiedHandler = new LastModifiedHandler(resourceLastModifiedDate);
        return lastModifiedHandler.checkRequest(httpServletRequest, httpServletResponse);
    }

atlassian-plugins-webresource:2.13.4

    /**
     * Checks any "If-Modified-Since" header from the request against the
     * plugin's loading time, since plugins can't be modified after they've been
     * loaded this is a good way to determine if a plugin resource has been
     * modified or not. If this method returns true, don't do any more
     * processing on the request -- the response code has already been set to
     * "304 Not Modified" for you, and you don't need to serve the file.
     */
    public boolean isResourceModified(final HttpServletRequest httpServletRequest, final HttpServletResponse httpServletResponse)
    {
        final Date resourceLastModifiedDate = (plugin.getDateLoaded() == null) ? new Date() : plugin.getDateLoaded();
        final LastModifiedHandler lastModifiedHandler = new LastModifiedHandler(resourceLastModifiedDate);
        return !lastModifiedHandler.checkRequest(httpServletRequest, httpServletResponse);
    }

I intentionally added the whole text with the comments so you can see that:

• the result of the method is reversed in 2.13.4 (so this implementation works with the reversed logic)
• but the comment and the description stays the same, adding to the confusion

--------

Overall, this is a total mess, both working and non-working versions.

As a plugin developer, I find it a painful change in the API, which happens to be introduced in a patch version. If this change is going to stay, it will require us to produce multiple binaries, for 5.0-5.2.7 and for 5.2.8+. So please please fix this somehow!

As version 2.13.4 is broken in logic, I would suggest the following approach:

• Please deprecate isResourceModified() and reverse its meaning and implementation back to 2.10.1, so the plugins functionality is restored.
• Please create a new method with a good, conventional name, which correctly relates what the method does - for example processUnmodifiedResource(). Don't call it "isSomething" because of the expected side-effect (setting the headers). Don't call it "isXYZ" if it returns false in case of XYZ.
• Please update javadocs, comments, log messages or share the stuff you smoke ;)

Thank you!
Igor