Skip to content
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

Loading config.xml from classpath doesn't work properly #6232

Closed
peterdemaeyer opened this issue Nov 25, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@peterdemaeyer
Copy link
Contributor

commented Nov 25, 2018

According to the Command Line Usage documentation, it should be possible to load config.xml from classpath using the same -c option as when loading it from file directly.
This is a nice feature, and I'd like to exploit it for all my projects, except that it doesn't work properly.

Steps to reproduce:

  1. Assume a standard Maven project structure with sources, resources, and test sources.
  2. Define a Checkstyle config file src/main/java/resources/my_checks.xml:
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
		"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
		"http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd">
<module name="Checker"/>
  1. Define a unit test src/test/java/CheckstyleTest.java:
import java.io.File;
import java.io.FileOutputStream;

import org.junit.Test;

public class CheckstyleTest {

	@Test
	public void loadConfigFromClasspath() throws Throwable {
		String configurationResource = "my_checks.xml";
		String javaSource = "class MyClass {}";
		File javaSourceFile = File.createTempFile("MyClass", ".java");
		try (FileOutputStream out = new FileOutputStream(javaSourceFile)) {
			out.write(javaSource.getBytes("UTF-8"));
		}
		com.puppycrawl.tools.checkstyle.Main.main("-c", configurationResource, javaSourceFile.getCanonicalPath());
	}
}
  1. Run the test.
  • Expected: test is green.
  • Actual: test fails with "Could not find config XML file 'my_checks.xml'.", "Process finished with exit code 255".

Root cause

The root cause is a bug in the method com.puppycrawl.tools.checkstyle.utils.CommonUtil.getUriByFilename, line 286 in version 8.14:

URL configUrl = CommonUtil.class.getResource(filename);

That call loads filename relative to the package of CommonUtil, which is com/puppycrawl/tools/checkstyle/utils. I can hardly imagine this is the intent, it would make much more sense to load it relative to the "root" of the classpath. The fix is very easy (and I hope will be included in the one of the next versions of Checkstyle soon):

URL configUrl = CommonUtil.class.getClassLoader().getResource(filename);

Workaround

Now that we know the root cause, it's easy to come up with a workaround.

The first workaround is to use an absolute (resource) path to load the resource in my test:

		String configurationResource = "/my_checks.xml";

This forces the class loader to look in the root of the classpath. It's not so elegant, but I can live with it.

The second workaround is to move src/main/resources/my_checks.xml to src/main/resources/com/puppycrawl/tools/checkstyle/utils/my_checks.xml, which is not so elegant either.

@rnveach

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

You can load configuration from absolute path like we do.
https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L582

hat call loads filename relative to the package of CommonUtil, which is com/puppycrawl/tools/checkstyle/utils. I can hardly imagine this is the intent, it would make much more sense to load it relative to the "root" of the classpath.
This forces the class loader to look in the root of the classpath.

Aren't these 2 statements conflicting. You say it is looking only in our package, but you show it is also looking through all classpaths.
This, I believe, is the original intent. Loading as a resource means it has to be available on the classpath somewhere, like the src/main/resources just like where our google_checks.xml resides and should be loadable by defining its path as /google_checks.xml .

fix
URL configUrl = CommonUtil.class.getClassLoader().getResource(filename);

I looked at the source of my getResource that we are calling and it is:

    public java.net.URL getResource(String name) {
        name = resolveName(name);
        ClassLoader cl = getClassLoader0();
        if (cl==null) {
            // A system class.
            return ClassLoader.getSystemResource(name);
        }
        return cl.getResource(name);
    }

and getClassLoader is basically a wrapper (plus security) for getClassLoader0, so I don't see how our code is any different then yours.

String configurationResource = "my_checks.xml";

One thing to note, since we are using CommonUtil.class.getResource, you do have to specify / to start the resource. resolveName that I showed in the source above does add the package name to the resource since it thinks it is a relative resource path and not an absolute one.

@romani Should we make this change? I personally don't think we should take all resources as relative and instead require user to only use absolute.

@peterdemaeyer

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2018

  1. I know I can load it using an absolute path, that's precisely what I say in Workaround. But it remains a workaround. The point is that it can easily be fixed to work with relative and absolute classpath paths, such that one is not forced to use absolute paths.
  2. These statements are not conflicting, because you've taken them out of context: the first comes from my problem description and the second from the workaround. The whole point is that I'd like to eliminate the need for the workaround.
  3. I see you struck through part of your own comment - I presume that means you do understand why my suggestion makes a difference? Because really, it does.

Please understand that my suggestion will not "suddenly" cause all resources to be interpreted relative. In all cases, both absolute resources and relative resources work, the only difference is what the relative resources are relative to.

  • For absolute resources, it makes no difference, they continue to work in exactly the same way as they do now in all cases.
  • For relative resources, they will be interpreted relative to the classpath root instead of /com/puppycrawl/tools/checkstyle/utils.

The question you need to ask yourself is "Is it reasonable to assume that relative classpath resources are loaded relative to <classpath-root>/com/puppycrawl/tools/checkstyle/utils? Or does it make much more sense to load them relative to <classpath-root>?"

The latter allows greater flexibility in defining and placing configurations, and is much more intuitive to users.

@rnveach

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Yes, I started to see somethings after my original post when I started looking into the resource loading source, hence why I made edits.

I know I can load it using an absolute path, that's precisely what I say in Workaround

I was stating to use the absolute file path, not the absolute resource path, which is what your workaround stated. IE: C:\my_config.xml instead of /my_config.xml.
In Checkstyle, you can load a configuration by URL, file path, and resource.

URL configUrl = CommonUtil.class.getClassLoader().getResource(filename);
For absolute resources, it makes no difference, they continue to work in exactly the same way as they do now in all cases.

That is not entirely accurate with the change you recommended. Some tests fail when I make this change because resolveName also removes the starting / which is not there with your change. See MainTest#testDebugOption as an example. We would have to mimick the old classloader, along with the change you recommend, to continue compatibility by always dropping the starting / if it exists.

my suggestion will not "suddenly" cause all resources to be interpreted relative

Based on my previous comment, it looks like this is what exactly it does, hence why the starting / has to be removed making it a relative path. But it probably still is relative to the root.

The latter allows greater flexibility in defining and placing configurations, and is much more intuitive to users.

Can you describe what you mean? Absolute versus relative to the root, it all seems the same, just +/- 1 character.

@romani

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

I think it make sense to make resource loading to be from root, it is easier for user to predict what path is required from him.

@rnveach , I think it is reasonable to mark issue as "approved" and discuss details(starting / , ....) at PR. We should keep existing functionality. If you are agree, please put label and lets give @peterdemaeyer a way to show how implementation will looks like.

@rnveach rnveach added the approved label Nov 27, 2018

@rnveach

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Since they seem the same to me I am fine.

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 10, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 10, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 10, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 19, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 20, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 21, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 21, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 21, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 25, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Mar 27, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 27, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 27, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 28, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Apr 28, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue May 19, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue May 19, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue May 19, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue May 19, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue May 25, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue May 25, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue May 25, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Jun 6, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Jun 6, 2019

romani added a commit that referenced this issue Jun 7, 2019

@romani romani added the bug label Jun 7, 2019

@romani romani added this to the 8.22 milestone Jun 7, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Fix is merged

@romani romani closed this Jun 7, 2019

peterdemaeyer added a commit to peterdemaeyer/checkstyle that referenced this issue Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.