fix #4357: spoon doesn't handle paths containinig spaces correctly #4358

xzel23 · 2021-12-16T07:44:21Z

please review and apply

slarse · 2021-12-16T22:30:58Z

Could you add a test case to demonstrate that this fix solves the problem? As per the contributing guidelines, bugfix PRs must contain a test case that reproduces the bug.

I-Al-Istannen · 2021-12-17T10:00:42Z

src/main/java/spoon/support/StandardEnvironment.java

@@ -418,7 +420,7 @@ public void setInputClassLoader(ClassLoader aClassLoader) {
 				if (onlyFileURLs) {
 					List<String> classpath = new ArrayList<>();
 					for (URL url : urls) {
-						classpath.add(url.getPath());
+						classpath.add(URLDecoder.decode(url.getPath(), StandardCharsets.UTF_8));


I am not sure if this is better or something like

classpath.add(Path.of(url.toURI()).toAbsolutePath().toString());

I am unconvinced. Now that you remind me: Path.of(URL) will throw an exception if the URL is not pointing to something that lies on a filesystem or if no FileSystemProvider supporting the URL's protocol is present. For example to create a Path instance that points to a file located inside a Jar file (not the Jar itself), there has to be a FileSystem instance for the Jar file. In the case of the "jar:" protocol, Path.of(URL) will make a disk access and try to read the Jar content and fail if the file is not present or cannot be read. You can try this with jshell:

jshell> Path.of(new URL("jar:file:/hello.jar!/hello.txt").toURI()) | Exception java.nio.file.FileSystemNotFoundException | at ZipFileSystemProvider.getFileSystem (ZipFileSystemProvider.java:156) | at ZipFileSystemProvider.getPath (ZipFileSystemProvider.java:142) | at Path.of (Path.java:208) | at (#3515

I doubt the existing code can handle files inside JARs either? URL-Decoding the getPath() would return file:/hello.jar!/hello.txt which likely isn't handled correctly. I'd need to have a look at it, but failing early instead of maybe failing later on sounds like a plus to me?

Hmm, this assumes utf8 encoding. How does this work on Windows when there's a non-ASCII character in the filepath?

@slarse Maybe someone with access to a windows box could test. In any case, URLs should use UTF-8. Qutoe taken from W3.org:

Note. Some older user agents trivially process URIs in HTML using the bytes of the character encoding in which the document was received. Some older HTML documents rely on this practice and break when transcoded. User agents that want to handle these older documents should, on receiving a URI containing characters outside the legal set, first use the conversion based on UTF-8. Only if the resulting URI does not resolve should they try constructing a URI based on the bytes of the character encoding in which the document was received.

I am not fully convinced, that JDT accepts file paths referring to files inside a jar or other weird things correctly, so I am not sure raw URL decoding really is a benefit here over stricter Path.of validation/conversion.

Either way, I tried it out @slarse and the URLDecode javadoc states that it only uses the charset for percent-encoded data. new URL("öäüß") is not stored percent encoded, so the string is returned unchanged in the default JVM windows charset. The UTF-8 setting has no relevance for it, but if the URL does contain other percent encoded data it could maybe end up with a String using two encodings.

@xzel23 I did not know that, good to know what the spec says (although I doubt all file systems follow that, a filepath is not necessarily a URI). I just don't trust Windows not to be the odd one out on.. everything.

@I-Al-Istannen Thanks for checking it out, that's 100% confusing :D

xzel23 · 2021-12-20T13:05:19Z

Could you add a test case to demonstrate that this fix solves the problem? As per the contributing guidelines, bugfix PRs must contain a test case that reproduces the bug.

Test case added.

monperrus · 2021-12-22T08:26:09Z

src/test/java/spoon/test/issue4357/TestIssue4357.java

+        ClassLoader classLoader = new URLClassLoader(classpath);
+        launcher.getEnvironment().setInputClassLoader(classLoader);
+        launcher.getEnvironment().setNoClasspath(false);
+        assertDoesNotThrow(() -> launcher.buildModel());     


a stronger assert would be checking for the presence of CtType Foo in the model. WDYT?

OK, that's next on my list.

Ah, this was a bit more complicated than expected because sppon doesn't load java files via classloaders. So I added a jar file that is used during the test. After some trial and error (I accidentally compiled the jar using JDK 17 and other oversights) the test finally passed in all tested configurations.

…out the fix

slarse

Production code looks fine to me. As URL.toURI() throws a checked exception, using URLDecoder.decode() seems more convenient to use.

The test should be moved to an existing test class (required change), and I strongly suggest making the assertion more specific to the scenario being tested (optional change).

Ah, this was a bit more complicated than expected because sppon doesn't load java files via classloaders. So I added a jar file that is used during the test. After some trial and error (I accidentally compiled the jar using JDK 17 and other oversights) the test finally passed in all tested configurations.

Class loaders are only for binary code (e.g. jars and class files), not for source code. You could have added a single .class file as well. I don't think it matters if it's a .jar or a .class, both are easy to inspect, so you can keep it as-is.

slarse · 2021-12-30T15:05:56Z

src/test/java/spoon/test/issue4357/TestIssue4357.java

+        CtModel model = launcher.buildModel();
+
+        assertTrue(model.getAllTypes().stream().anyMatch(ct -> ct.getQualifiedName().equals("Foo")), 
+                "CtTxpe 'Foo' not present in model");


Optional change: This only indirectly verifies that we correctly handle spaces in classpath URLs. I'd suggest actually verifying that the reference to bar.Bar is correctly resolved.

Suggested change

CtModel model = launcher.buildModel();

assertTrue(model.getAllTypes().stream().anyMatch(ct -> ct.getQualifiedName().equals("Foo")),

"CtTxpe 'Foo' not present in model");

launcher.buildModel();

CtType<?> foo = launcher.getFactory().Type().get("Foo");

CtMethod<?> methodWithBarReturnType = foo.getMethod("foo");

CtTypeReference<?> barRef = methodWithBarReturnType.getType();

assertThat(barRef.getQualifiedName(), equalTo("bar.Bar"));

assertThat(barRef.getTypeDeclaration(), notNullValue());

assertTrue(barRef.getTypeDeclaration().isShadow(), "expected bar.Bar to be a shadow class");

Another alternative would be to test this with a unit test of StandardEnvironment instead, which is where the problem actually lies. Since this is tested so "far away" from the problem, the assertion should be very specific to IMO.

slarse · 2021-12-30T15:07:47Z

src/test/java/spoon/test/issue4357/TestIssue4357.java

+public class TestIssue4357 {
+
+    @Test
+    public void testClasspathURLWithSpaces() throws MalformedURLException {


Required change: Put this test in an existing test class instead of creating a new one. Due to the phrasing of the contract, I'd put this in LauncherTest.

@slarse done.

slarse

LGTM, thanks @xzel23

fix INRIA#4357: spoon doesn't handle paths containinig spaces correctly

5987064

I-Al-Istannen reviewed Dec 17, 2021

View reviewed changes

add testcase

636bdd9

monperrus reviewed Dec 22, 2021

View reviewed changes

xzel23 added 5 commits December 29, 2021 09:16

change the test case to make sure that the model cannot be built with…

f6caec6

…out the fix

change the test case to make sure that the model cannot be built with…

bb92e4f

…out the fix

recompile with JDK 8 compatibility

f540fbb

recompile with JDK 8 compatibility

61e78b7

remove source file

af3b36c

xzel23 mentioned this pull request Dec 29, 2021

[Bug] StandardEnvironment does not work if path contains spaces #4357

Closed

slarse suggested changes Dec 30, 2021

View reviewed changes

move test to LauncherTest

f00ec5d

slarse approved these changes Jan 6, 2022

View reviewed changes

slarse merged commit fa6b014 into INRIA:master Jan 6, 2022

monperrus mentioned this pull request Mar 14, 2022

WIP release: next release #4636

Closed

I-Al-Istannen mentioned this pull request Nov 17, 2022

review: fix: Properly accept urls with plus signs (+) #5014

Merged

xzel23 deleted the bug4357 branch October 4, 2023 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #4357: spoon doesn't handle paths containinig spaces correctly #4358

fix #4357: spoon doesn't handle paths containinig spaces correctly #4358

xzel23 commented Dec 16, 2021 •

edited by slarse

Loading

slarse commented Dec 16, 2021

I-Al-Istannen Dec 17, 2021

xzel23 Dec 17, 2021 •

edited

Loading

I-Al-Istannen Dec 22, 2021 •

edited

Loading

slarse Dec 23, 2021

xzel23 Dec 23, 2021

I-Al-Istannen Dec 23, 2021

slarse Dec 25, 2021 •

edited

Loading

xzel23 commented Dec 20, 2021

monperrus Dec 22, 2021

xzel23 Dec 23, 2021

xzel23 Dec 29, 2021

slarse left a comment

slarse Dec 30, 2021

slarse Dec 30, 2021

xzel23 Jan 5, 2022

slarse left a comment

-        CtModel model = launcher.buildModel();
-        assertTrue(model.getAllTypes().stream().anyMatch(ct -> ct.getQualifiedName().equals("Foo")),
-                "CtTxpe 'Foo' not present in model");
+        launcher.buildModel();
+        CtType<?> foo = launcher.getFactory().Type().get("Foo");
+        CtMethod<?> methodWithBarReturnType = foo.getMethod("foo");
+        CtTypeReference<?> barRef = methodWithBarReturnType.getType();
+        assertThat(barRef.getQualifiedName(), equalTo("bar.Bar"));
+        assertThat(barRef.getTypeDeclaration(), notNullValue());
+        assertTrue(barRef.getTypeDeclaration().isShadow(), "expected bar.Bar to be a shadow class");

fix #4357: spoon doesn't handle paths containinig spaces correctly #4358

fix #4357: spoon doesn't handle paths containinig spaces correctly #4358

Conversation

xzel23 commented Dec 16, 2021 • edited by slarse Loading

slarse commented Dec 16, 2021

Choose a reason for hiding this comment

xzel23 Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

I-Al-Istannen Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse Dec 25, 2021 • edited Loading

Choose a reason for hiding this comment

xzel23 commented Dec 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse left a comment

Choose a reason for hiding this comment

xzel23 commented Dec 16, 2021 •

edited by slarse

Loading

xzel23 Dec 17, 2021 •

edited

Loading

I-Al-Istannen Dec 22, 2021 •

edited

Loading

slarse Dec 25, 2021 •

edited

Loading