New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Version#fromClasspathProperties() when loading from JAR plugin #2535

Merged
merged 2 commits into from Jul 26, 2016

Conversation

Projects
None yet
2 participants
@bernd
Member

bernd commented Jul 26, 2016

Loading a version.properties file from a JAR plugin didn't work before
because Resources.getResource() didn't look into the plugin's class
loader.

This changes the main #fromClasspathProperties() method to accept a
Class parameter which will be used to lookup the resource in the correct
class loader.

It also adds #fromPluginProperties() a convenience method for plugins.

Fix Version#fromClasspathProperties() when loading from JAR plugin
Loading a `version.properties` file from a JAR plugin didn't work before
because `Resources.getResource()` didn't look into the plugin's class
loader.

This changes the main `#fromClasspathProperties()` method to accept a
Class parameter which will be used to lookup the resource in the correct
class loader.

It also adds `#fromPluginProperties()` a convenience method for plugins.

@bernd bernd added this to the 2.1.0 milestone Jul 26, 2016

@@ -217,6 +243,12 @@ public static Version fromClasspathProperties(String path, String propertyName,
return defaultVersion;
}
private static URL getResource(Class<?> clazz, String path) {
final URL url = requireNonNull(clazz, "Class argument is null!").getClassLoader().getResource(path);

This comment has been minimized.

@joschi

joschi Jul 26, 2016

Contributor

I think the getClassLoader() call is not required here, as Class#getResource(String) already is using the object's class loader.

There's also a variant of Resources#getResource() for this.

This comment has been minimized.

@bernd

bernd Jul 26, 2016

Member

There's also a variant of Resources#getResource() for this.

I tried the Resources#getResource() variant but it didn't work... No idea why.

I will try again.

This comment has been minimized.

@bernd

bernd Jul 26, 2016

Member

When I change the code like the following or use the Resources#getResource(class, path) method, I am getting the error below. It works fine with the current code in this PR.

diff --git graylog2-server/src/main/java/org/graylog2/plugin/Version.java graylog2-server/src/main/java/org/graylog2/plugin/Version.java
index 7f13e36..3a0be0a 100644
--- graylog2-server/src/main/java/org/graylog2/plugin/Version.java
+++ graylog2-server/src/main/java/org/graylog2/plugin/Version.java
@@ -244,7 +244,7 @@ public class Version implements Comparable<Version> {
     }

     private static URL getResource(Class<?> clazz, String path) {
-        final URL url = requireNonNull(clazz, "Class argument is null!").getClassLoader().getResource(path);
+        final URL url = requireNonNull(clazz, "Class argument is null!").getResource(path);

         return requireNonNull(url, "Resource <" + path + "> not found.");
     }
2016-07-26 17:03:11,877 ERROR: org.graylog2.plugin.Version - Unable to read version.properties, this build has no version number.
java.lang.NullPointerException: Resource <version.properties> not found.
    at java.util.Objects.requireNonNull(Objects.java:228) ~[?:1.8.0_101]
    at org.graylog2.plugin.Version.getResource(Version.java:249) ~[classes/:?]
    at org.graylog2.plugin.Version.fromClasspathProperties(Version.java:215) [classes/:?]
    at org.graylog2.plugin.Version.fromClasspathProperties(Version.java:199) [classes/:?]
    at org.graylog2.plugin.Version.fromClasspathProperties(Version.java:173) [classes/:?]
    at org.graylog2.plugin.Version.<clinit>(Version.java:66) [classes/:?]
    at org.graylog2.bootstrap.CmdLineTool.<clinit>(CmdLineTool.java:90) [classes/:?]
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:1.8.0_101]
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) [?:1.8.0_101]
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) [?:1.8.0_101]
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423) [?:1.8.0_101]
    at io.airlift.airline.ParserUtil.createInstance(ParserUtil.java:19) [airline-0.7.jar:0.7]
    at io.airlift.airline.ParserUtil.createInstance(ParserUtil.java:37) [airline-0.7.jar:0.7]
    at io.airlift.airline.Cli.parse(Cli.java:120) [airline-0.7.jar:0.7]
    at io.airlift.airline.Cli.parse(Cli.java:97) [airline-0.7.jar:0.7]
    at org.graylog2.bootstrap.Main.main(Main.java:43) [classes/:?]

Resources#getResource()

2016-07-26 17:38:36,485 ERROR: org.graylog2.plugin.Version - Unable to read version.properties, this build has no version number.
java.lang.IllegalArgumentException: resource version.properties relative to org.graylog2.plugin.Version not found.
    at com.google.common.base.Preconditions.checkArgument(Preconditions.java:146) ~[guava-19.0.jar:?]
    at com.google.common.io.Resources.getResource(Resources.java:209) ~[guava-19.0.jar:?]
    at org.graylog2.plugin.Version.getResource(Version.java:248) ~[classes/:?]
    at org.graylog2.plugin.Version.fromClasspathProperties(Version.java:216) [classes/:?]
    at org.graylog2.plugin.Version.fromClasspathProperties(Version.java:200) [classes/:?]
    at org.graylog2.plugin.Version.fromClasspathProperties(Version.java:174) [classes/:?]
    at org.graylog2.plugin.Version.<clinit>(Version.java:67) [classes/:?]
    at org.graylog2.bootstrap.CmdLineTool.<clinit>(CmdLineTool.java:90) [classes/:?]
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:1.8.0_101]
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) [?:1.8.0_101]
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) [?:1.8.0_101]
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423) [?:1.8.0_101]
    at io.airlift.airline.ParserUtil.createInstance(ParserUtil.java:19) [airline-0.7.jar:0.7]
    at io.airlift.airline.ParserUtil.createInstance(ParserUtil.java:37) [airline-0.7.jar:0.7]
    at io.airlift.airline.Cli.parse(Cli.java:120) [airline-0.7.jar:0.7]
    at io.airlift.airline.Cli.parse(Cli.java:97) [airline-0.7.jar:0.7]
    at org.graylog2.bootstrap.Main.main(Main.java:43) [classes/:?]

This comment has been minimized.

@joschi

joschi Jul 26, 2016

Contributor

Alright, thanks for checking! Let's keep it the original way.

@joschi joschi self-assigned this Jul 26, 2016

@joschi joschi added the bug label Jul 26, 2016

@joschi

This comment has been minimized.

Contributor

joschi commented Jul 26, 2016

LGTM. 👍

@joschi joschi merged commit c796870 into master Jul 26, 2016

1 check passed

continuous-integration/travis-ci/push The Travis CI build passed
Details

@joschi joschi deleted the fix-version-properties branch Jul 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment