Issue #13012: fix loading with non-ascii symbols in path

strkkk · nrmancuso · commit 8a4b40600c1c · 2024-05-26T09:19:22.000-04:00
diff --git a/config/signatures.txt b/config/signatures.txt
@@ -3,3 +3,4 @@ com.puppycrawl.tools.checkstyle.DefaultConfiguration#getAttribute(java.lang.Stri
 com.puppycrawl.tools.checkstyle.DefaultConfiguration#addAttribute(java.lang.String,java.lang.String) @ Usage of deprecated API is forbidden. Please use DefaultConfiguration.addProperty(String name, String value) instead.
 com.puppycrawl.tools.checkstyle.api.AbstractCheck#log(int,int,java.lang.String,java.lang.Object[]) @ Use of this log method is forbidden, please use AbstractCheck#log(DetailAST ast, String key, Object... args)
 com.puppycrawl.tools.checkstyle.api.AbstractCheck#log(int,java.lang.String,java.lang.Object[]) @ Use of this log method is forbidden, please use AbstractCheck#log(DetailAST ast, String key, Object... args)
+java.net.URI#toString() @ This method can return garbage for non-ascii data, please use URI#toASCIIString()
diff --git a/config/suppressions.xml b/config/suppressions.xml
@@ -194,4 +194,8 @@
   <!-- The file is generated -->
   <suppress checks="FileTabCharacter"
     files="src[\\/]xdocs[\\/]checks[\\/]whitespace[\\/]filetabcharacter\.xml"/>
+
+  <!-- Uses non-ascii symbols intentionally -->
+  <suppress id="checkASCII"
+            files="[\\/]src[\\/]test[\\/]java[\\/]com[\\/]puppycrawl[\\/]tools[\\/]checkstyle[\\/]ConfigurationLoaderTest.java"/>
 </suppressions>
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java b/src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java
@@ -20,7 +20,6 @@
 package com.puppycrawl.tools.checkstyle;
 
 import java.io.IOException;
-import java.net.URI;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -260,10 +259,7 @@ public static Configuration loadConfiguration(String config,
                                                   IgnoredModulesOptions ignoredModulesOptions,
                                                   ThreadModeSettings threadModeSettings)
             throws CheckstyleException {
-        // figure out if this is a File or a URL
-        final URI uri = CommonUtil.getUriByFilename(config);
-        final InputSource source = new InputSource(uri.toString());
-        return loadConfiguration(source, overridePropsResolver,
+        return loadConfiguration(CommonUtil.sourceFromFilename(config), overridePropsResolver,
                 ignoredModulesOptions, threadModeSettings);
     }
 
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/header/AbstractHeaderCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/header/AbstractHeaderCheck.java
@@ -29,7 +29,6 @@
 import java.nio.charset.Charset;
 import java.nio.charset.UnsupportedCharsetException;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 import java.util.regex.Pattern;
@@ -201,10 +200,10 @@ public Set<String> getExternalResourceLocations() {
         final Set<String> result;
 
         if (headerFile == null) {
-            result = Collections.emptySet();
+            result = Set.of();
         }
         else {
-            result = Collections.singleton(headerFile.toString());
+            result = Set.of(headerFile.toASCIIString());
         }
 
         return result;
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlCheck.java
@@ -20,7 +20,6 @@
 package com.puppycrawl.tools.checkstyle.checks.imports;
 
 import java.net.URI;
-import java.util.Collections;
 import java.util.Set;
 import java.util.regex.Pattern;
 
@@ -253,7 +252,7 @@ else if (currentImportControl != null) {
 
     @Override
     public Set<String> getExternalResourceLocations() {
-        return Collections.singleton(file.toString());
+        return Set.of(file.toASCIIString());
     }
 
     /**
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoader.java b/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressionsLoader.java
@@ -21,7 +21,6 @@
 
 import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.net.URI;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Locale;
@@ -218,10 +217,7 @@ private static XpathFilterElement getXpathFilter(Attributes attributes) throws S
      */
     public static FilterSet loadSuppressions(String filename)
             throws CheckstyleException {
-        // figure out if this is a File or a URL
-        final URI uri = CommonUtil.getUriByFilename(filename);
-        final InputSource source = new InputSource(uri.toString());
-        return loadSuppressions(source, filename);
+        return loadSuppressions(CommonUtil.sourceFromFilename(filename), filename);
     }
 
     /**
@@ -247,10 +243,7 @@ private static FilterSet loadSuppressions(
      */
     public static Set<TreeWalkerFilter> loadXpathSuppressions(String filename)
             throws CheckstyleException {
-        // figure out if this is a File or a URL
-        final URI uri = CommonUtil.getUriByFilename(filename);
-        final InputSource source = new InputSource(uri.toString());
-        return loadXpathSuppressions(source, filename);
+        return loadXpathSuppressions(CommonUtil.sourceFromFilename(filename), filename);
     }
 
     /**
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/utils/CommonUtil.java b/src/main/java/com/puppycrawl/tools/checkstyle/utils/CommonUtil.java
@@ -36,6 +36,8 @@
 import java.util.regex.Pattern;
 import java.util.regex.PatternSyntaxException;
 
+import org.xml.sax.InputSource;
+
 import com.puppycrawl.tools.checkstyle.api.CheckstyleException;
 
 /**
@@ -330,6 +332,19 @@ public static void close(Closeable closeable) {
         }
     }
 
+    /**
+     * Creates an input source from a file.
+     *
+     * @param filename name of the file.
+     * @return input source.
+     * @throws CheckstyleException if an error occurs.
+     */
+    public static InputSource sourceFromFilename(String filename) throws CheckstyleException {
+        // figure out if this is a File or a URL
+        final URI uri = getUriByFilename(filename);
+        return new InputSource(uri.toASCIIString());
+    }
+
     /**
      * Resolve the specified filename to a URI.
      *
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java
@@ -654,6 +654,21 @@ public void testLoadConfigurationFromClassPath() throws Exception {
             .isEqualTo(0);
     }
 
+    @Test
+    public void testLoadConfigurationFromClassPathWithNonAsciiSymbolsInPath() throws Exception {
+        final DefaultConfiguration config =
+                (DefaultConfiguration) ConfigurationLoader.loadConfiguration(
+                    getResourcePath("æ£µ¥/InputConfigurationLoaderDefaultProperty.xml"),
+                        new PropertiesExpander(new Properties()));
+
+        final Properties expectedPropertyValues = new Properties();
+        expectedPropertyValues.setProperty("tabWidth", "2");
+        expectedPropertyValues.setProperty("basedir", ".");
+        // charset property uses 2 variables, one is not defined, so default becomes a result value
+        expectedPropertyValues.setProperty("charset", "ASCII");
+        verifyConfigNode(config, "Checker", 0, expectedPropertyValues);
+    }
+
     @Test
     public void testParsePropertyString() throws Exception {
         final List<String> propertyRefs = new ArrayList<>();
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java
@@ -295,7 +295,7 @@ public void testExternalResource() throws Exception {
             .isEqualTo(1);
         assertWithMessage("Invalid resource location")
             .that(results.iterator().next())
-            .isEqualTo(uri.toString());
+            .isEqualTo(uri.toASCIIString());
     }
 
     @Test
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtilTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtilTest.java
@@ -79,7 +79,7 @@ public void testMetadataFilesGenerationAllFiles(@SystemOutGuard.SysOut Capturabl
 
         final String[] expectedErrorMessages = {
             "31: " + getCheckMessage(MSG_DESC_MISSING, "AbstractSuperCheck"),
-            "44: " + getCheckMessage(MSG_DESC_MISSING, "AbstractHeaderCheck"),
+            "43: " + getCheckMessage(MSG_DESC_MISSING, "AbstractHeaderCheck"),
             "43: " + getCheckMessage(MSG_DESC_MISSING, "AbstractJavadocCheck"),
             "45: " + getCheckMessage(MSG_DESC_MISSING, "AbstractClassCouplingCheck"),
             "26: " + getCheckMessage(MSG_DESC_MISSING, "AbstractAccessControlNameCheck"),
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java
@@ -491,7 +491,7 @@ public void testGetUriByFilenameFindsAbsoluteResourceOnClasspath() throws Except
         final URI uri = CommonUtil.getUriByFilename(filename);
 
         final Properties properties = System.getProperties();
-        final Configuration config = ConfigurationLoader.loadConfiguration(uri.toString(),
+        final Configuration config = ConfigurationLoader.loadConfiguration(uri.toASCIIString(),
             new PropertiesExpander(properties));
         assertWithMessage("Unexpected config name!")
             .that(config.getName())
@@ -505,7 +505,7 @@ public void testGetUriByFilenameFindsRelativeResourceOnClasspath() throws Except
         final URI uri = CommonUtil.getUriByFilename(filename);
 
         final Properties properties = System.getProperties();
-        final Configuration config = ConfigurationLoader.loadConfiguration(uri.toString(),
+        final Configuration config = ConfigurationLoader.loadConfiguration(uri.toASCIIString(),
             new PropertiesExpander(properties));
         assertWithMessage("Unexpected config name!")
             .that(config.getName())
@@ -531,10 +531,10 @@ public void testGetUriByFilenameFindsResourceRelativeToRootClasspath() throws Ex
                 "com/puppycrawl/tools/checkstyle/utils/"
                         + getPackageLocation() + "/InputCommonUtilTest_resource.txt";
         assertWithMessage("URI is relative to package " + uriRelativeToPackage)
-            .that(uri.toString())
+            .that(uri.toASCIIString())
             .doesNotContain(uriRelativeToPackage);
         final String content = IOUtils.toString(uri.toURL(), StandardCharsets.UTF_8);
-        assertWithMessage("Content mismatches for: " + uri)
+        assertWithMessage("Content mismatches for: " + uri.toASCIIString())
             .that(content)
             .startsWith("good");
     }
@@ -546,7 +546,7 @@ public void testGetUriByFilenameClasspathPrefixLoadConfig() throws Exception {
         final URI uri = CommonUtil.getUriByFilename(filename);
 
         final Properties properties = System.getProperties();
-        final Configuration config = ConfigurationLoader.loadConfiguration(uri.toString(),
+        final Configuration config = ConfigurationLoader.loadConfiguration(uri.toASCIIString(),
             new PropertiesExpander(properties));
         assertWithMessage("Unexpected config name!")
             .that(config.getName())
@@ -560,7 +560,7 @@ public void testGetUriByFilenameFindsRelativeResourceOnClasspathPrefix() throws
         final URI uri = CommonUtil.getUriByFilename(filename);
 
         final Properties properties = System.getProperties();
-        final Configuration config = ConfigurationLoader.loadConfiguration(uri.toString(),
+        final Configuration config = ConfigurationLoader.loadConfiguration(uri.toASCIIString(),
             new PropertiesExpander(properties));
         assertWithMessage("Unexpected config name!")
             .that(config.getName())
diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/configurationloader/æ£µ¥/InputConfigurationLoaderDefaultProperty.xml b/src/test/resources/com/puppycrawl/tools/checkstyle/configurationloader/æ£µ¥/InputConfigurationLoaderDefaultProperty.xml
@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!DOCTYPE module PUBLIC
+    "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
+    "https://checkstyle.org/dtds/configuration_1_3.dtd">
+
+<module name="Checker">
+    <property name="tabWidth" value="${checkstyle.tabsize}" default="2"/>
+    <property name="basedir" value="src/${checkstyle.baseDirectory}" default="."/>
+    <property name="charset"
+              value="${checkstyle.charset.base}-${checkstyle.charset.number}" default="ASCII"/>
+
+</module>

Original file line number	Diff line number	Diff line change
`@@ -295,7 +295,7 @@ public void testExternalResource() throws Exception {`
`295`	`295`	`.isEqualTo(1);`
`296`	`296`	`assertWithMessage("Invalid resource location")`
`297`	`297`	`.that(results.iterator().next())`
`298`		`- .isEqualTo(uri.toString());`
	`298`	`+ .isEqualTo(uri.toASCIIString());`
`299`	`299`	`}`
`300`	`300`
`301`	`301`	`@Test`