Bug 196452 - LayerGeneratingProcessor utility method to validate resource path
LayerGeneratingProcessor utility method to validate resource path
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Filesystems
7.0
All All
: P3 (vote)
: 7.1
Assigned To: Jesse Glick
issues@platform
: API, API_REVIEW_FAST
: 212077 (view as bug list)
Depends on: 194545 196933
Blocks: 200175
  Show dependency treegraph
 
Reported: 2011-03-08 19:59 UTC by Jesse Glick
Modified: 2012-05-07 21:22 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Running patch (23.68 KB, patch)
2011-07-18 22:31 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2011-03-08 19:59:05 UTC
ActionProcessor, OptionsPanelControllerProcessor, and maybe others have custom code to check for the existence of a named resource. This should be consolidated into some properly tested utility method in LayerGeneratingProcessor. For example:

protected final byte[] validateResource(Element originatingElement, String resource, boolean searchClasspath) throws LayerBuildingException;

SOURCE_PATH should always be checked, and CLASS_OUTPUT currently as well (to work around a javac bug affecting Maven projects). Optionally, also check CLASS_PATH and perhaps PLATFORM_CLASS_PATH (but definitely not ANNOTATION_PROCESSOR_PATH or SOURCE_OUTPUT). Should throw LBE if not found.

Can return byte contents in case the processor wishes to perform additional checks on the content (as e.g. bundlevalue does now). Returning an InputStream is slightly more efficient, though it forces processors which do not wish to perform any further checks to remember to close it. (@CheckReturnValue, if openide.filesystems could depend on that.)

May be useful to also add a parameter for the responsible annotation (or annotation type?), if bug #194545 is also implemented.

In principle it could be useful to have a boolean flag to permit the resource to be either absolute or relative to the package of the originatingElement, as e.g. Class.getResource (as opposed to e.g. ClassLoader.getResource). Currently I do not know of any NB annotations which permit relative resource paths, however. Even if they did, something would probably need to first compute the absolute resource path for emitting to a layer, so that code could pass the absolute path to the validate method.
Comment 1 Jesse Glick 2011-03-21 17:48:12 UTC
(In reply to comment #0)
> Optionally, also check CLASS_PATH

Unfortunately I have found that this does not work in JDK 6 javac - even if the JAR containing the icon is moved to the front of the classpath (ahead of build/classes and so on), the filer produces

java.io.FileNotFoundException: .../release/modules/ext/whatever.jar/path/to/icon.gif (Not a directory)

resulting in a "Cannot find iconBase file..." error from the processor. The same command invocation works in JDK 7 javac. Have not yet tracked down the javac bug, but this is a potentially blocking issue when using @ActionRegistration or any other annotation which permits a resource to be found on the classpath and not just in the sourcepath. (Encountered in a customer module including jlfgr-1_0.jar on the classpath for standard icons, which is a reasonable use case.)
Comment 2 Jesse Glick 2011-03-21 17:58:09 UTC
Found the fix in JDK 7. Probably need to escalate a backport.
Comment 3 Jesse Glick 2011-03-21 18:25:39 UTC
Filed this problem as bug #196933.
Comment 4 Jesse Glick 2011-07-18 22:31:27 UTC
Created attachment 109497 [details]
Running patch

'hg pdi validateResource_196452' to get current patch.
Comment 5 Jesse Glick 2011-07-18 22:33:41 UTC
Please review. Tests are pending, though ActionProcessorTest does some of the testing.
Comment 6 Jaroslav Tulach 2011-07-20 16:40:57 UTC
I don't like absolutizeResource much. Perhaps this is a way to avoid it:

Y01 Rather than having validateResource on LGP, you could have new method on LayerBuilder.File:

public void stringFromResourceValue(String attrName, String path, boolean classpath, boolean canBeRelative, ...)

The method would validate if the resource exists (either relative or absolute) and would generate the proper form of the attribute (absolutized by default I guess). In this system absolutizeResource method is not needed, as it is handled by the processor under the cover.

There is a lot of somevalue in LayerBuilder.File anyway, and new ones are appearing everyday, so I guess this would be fine approach.
Comment 7 Jesse Glick 2011-07-20 16:59:29 UTC
(In reply to comment #6)
> Y01 ... would generate the proper form of the attribute
> (absolutized by default I guess).

This style was my initial thought (see comment #0), but it did not work out well: the processor may need to do various things with the absolute result, including constructing URLs. Rather than trying to shoehorn two independent tasks - absolutizing a path, and validating a resource - into a single method, it seemed better to keep them separate and leave it up to the caller to combine them where and how desired.

Another thing to note is that validating a resource with classpath=true cannot work in JDK 6 javac (the impl will silently accept any value), so it is not in general possible to detect whether an ambiguous path format is in fact relative or absolute.

A variant method I have considered (but which is not proposed in this patch) would treat "o/n/m/x/resources/x.png" as absolute, but would also permit "./resources/x.png" or "../resources/x.png" as relative paths. This would provide a compatible enhancement for the many annotations which require an absolute resource path for e.g. iconBase, but where a relative path would be more convenient in some cases, since the forms with ./ or ../ segments would not be permitted by the current processors. Again it would be clearest for this to be a simple added utility method which could be used with validateResource, rather than complicating vR's signature with such an option.
Comment 8 Jaroslav Tulach 2011-07-21 12:18:42 UTC
(In reply to comment #7)
> (In reply to comment #6)
> A variant method I have considered (but which is not proposed in this patch)
> would treat "o/n/m/x/resources/x.png" as absolute, but would also permit
> "./resources/x.png" or "../resources/x.png" as relative paths. 

This way you don't need an option to ask whether stringFromAResource method accepts relative or absolute values. It would always accept both. 

> rather than complicating vR's signature with such an option.

The only option needed now would be whether the value should always be converted to absolute or can be left relative.

Btw. it would be more OS-like and Class.getResource()-like if absolute paths  started with / and relative would not.
Comment 9 Jesse Glick 2011-07-21 13:03:53 UTC
(In reply to comment #8)
> This way you don't need an option to ask whether stringFromAResource method
> accepts relative or absolute values. It would always accept both. 

Not good enough - some annotations specify that "resources/x.png" is permitted as a relative path. And as previously mentioned, it is not always possible to infer whether this is absolute or relative.

> The only option needed now would be whether the value should always be
> converted to absolute or can be left relative.

Not necessarily, because some processors need to do other things with the result than store it as a stringvalue, as previously mentioned.

> it would be more [...] Class.getResource()-like if absolute paths 
> started with / and relative would not.

That is what absolutizeResource does.
Comment 10 Jesse Glick 2011-07-25 21:02:07 UTC
core-main #2c2768cd93bb
Comment 11 Quality Engineering 2011-07-27 14:07:39 UTC
Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/2c2768cd93bb
User: Jesse Glick <jglick@netbeans.org>
Log: #196452: way for a LayerGeneratingProcessor to validate a resource path.
Comment 12 Jesse Glick 2012-05-07 21:22:31 UTC
*** Bug 212077 has been marked as a duplicate of this bug. ***


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo