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

strkkk · May 18, 2024 · c4bea27 · c4bea27
1 parent 6bb41a4
commit c4bea27
Show file tree

Hide file tree

Showing 13 changed files with 74 additions and 33 deletions.
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.createSource(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;
@@ -114,7 +113,7 @@ private void loadHeaderFile() throws CheckstyleException {
         }
         catch (final IOException ex) {
             throw new CheckstyleException(
-                    "unable to load header file " + headerFile, ex);
+                    "unable to load header file " + headerFile.toASCIIString(), ex);
         }
     }
 
@@ -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());
     }
 
     /**
@@ -303,7 +302,7 @@ public void setFile(URI uri) {
                 file = uri;
             }
             catch (CheckstyleException ex) {
-                throw new IllegalArgumentException(UNABLE_TO_LOAD + uri, ex);
+                throw new IllegalArgumentException(UNABLE_TO_LOAD + uri.toASCIIString(), ex);
             }
         }
     }

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlLoader.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlLoader.java
@@ -271,11 +271,11 @@ private static PkgImportControl load(InputSource source,
             return loader.getRoot();
         }
         catch (ParserConfigurationException | SAXException ex) {
-            throw new CheckstyleException("unable to parse " + uri
+            throw new CheckstyleException("unable to parse " + uri.toASCIIString()
                     + " - " + ex.getMessage(), ex);
         }
         catch (IOException ex) {
-            throw new CheckstyleException("unable to read " + uri, ex);
+            throw new CheckstyleException("unable to read " + uri.toASCIIString(), ex);
         }
     }
 
@@ -292,10 +292,10 @@ private static PkgImportControl loadUri(URI uri) throws CheckstyleException {
             return load(source, uri);
         }
         catch (MalformedURLException ex) {
-            throw new CheckstyleException("syntax error in url " + uri, ex);
+            throw new CheckstyleException("syntax error in url " + uri.toASCIIString(), ex);
         }
         catch (IOException ex) {
-            throw new CheckstyleException("unable to find " + uri, ex);
+            throw new CheckstyleException("unable to find " + uri.toASCIIString(), ex);
         }
     }
 

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.createSource(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.createSource(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;
 
 /**
@@ -594,4 +596,16 @@ public static boolean isCodePointWhitespace(int[] codePoints, int index) {
         return Character.isWhitespace(character);
     }
 
+    /**
+     * 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 createSource(String filename) throws CheckstyleException {
+        // figure out if this is a File or a URL
+        final URI uri = getUriByFilename(filename);
+        return new InputSource(uri.toASCIIString());
+    }
 }
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,20 @@ public void testLoadConfigurationFromClassPath() throws Exception {
             .isEqualTo(0);
     }
 
+    @Test
+    public void testLoadConfigurationFromClassPathWithNonAsciiSymbolsInPath() throws Exception {
+        final DefaultConfiguration config =
+                (DefaultConfiguration) ConfigurationLoader.loadConfiguration(
+                    getResourcePath("æ£µ¥/InputConfigurationLoaderModuleIgnoreSeverity.xml"),
+                        new PropertiesExpander(new Properties()), IgnoredModulesOptions.OMIT);
+
+        final Configuration[] children = config.getChildren();
+        final int length = children[0].getChildren().length;
+        assertWithMessage("Invalid children count")
+            .that(length)
+            .isEqualTo(0);
+    }
+
     @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())
@@ -576,6 +576,14 @@ public void testIsCodePointWhitespace() {
                 .isFalse();
     }
 
+    @Test
+    public void testIsCreatingInputSourceFromFilenameWithNonAsciiCharsInPath()
+            throws CheckstyleException {
+        final String filename =
+            "/" + getPackageLocation() + "/InputCommonUtilTest_empty_checks.xml";
+        assertThat(CommonUtil.createSource(filename)).isNotNull();
+    }
+
     @Test
     public void testLoadSuppressionsUriSyntaxException() throws Exception {
         final URL configUrl = mock();

diff --git a/...ools/checkstyle/configurationloader/æ£µ¥/InputConfigurationLoaderModuleIgnoreSeverity.xml b/...ools/checkstyle/configurationloader/æ£µ¥/InputConfigurationLoaderModuleIgnoreSeverity.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">
+    <module name="TreeWalker">
+        <module name="MemberName">
+            <property name="severity" value="ignore"/>
+        </module>
+    </module>
+</module>