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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Jan 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5987064
fix #4357: spoon doesn't handle paths containinig spaces correctly
xzel23 Dec 16, 2021
636bdd9
add testcase
xzel23 Dec 20, 2021
f6caec6
change the test case to make sure that the model cannot be built with…
xzel23 Dec 29, 2021
bb92e4f
change the test case to make sure that the model cannot be built with…
xzel23 Dec 29, 2021
f540fbb
recompile with JDK 8 compatibility
xzel23 Dec 29, 2021
61e78b7
recompile with JDK 8 compatibility
xzel23 Dec 29, 2021
af3b36c
remove source file
xzel23 Dec 29, 2021
f00ec5d
move test to LauncherTest
xzel23 Jan 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/main/java/spoon/support/StandardEnvironment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;
import java.net.URLDecoder;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is better or something like

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

Copy link
Contributor Author

@xzel23 xzel23 Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@slarse slarse Dec 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

}
setSourceClasspath(classpath.toArray(new String[0]));
} else {
Expand Down
7 changes: 5 additions & 2 deletions src/test/java/spoon/support/visitor/java/JavaReflectionTreeBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import java.lang.annotation.Retention;
import java.net.CookieManager;
import java.net.URLClassLoader;
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
import java.time.format.TextStyle;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -603,10 +605,11 @@ public void testPartialShadow() {
@Test
public void testInnerClassWithConstructorParameterAnnotated() {
Launcher launcher = new Launcher();
launcher.addInputResource(JavaReflectionTreeBuilderTest.class
launcher.addInputResource(URLDecoder.decode(JavaReflectionTreeBuilderTest.class
.getClassLoader()
.getResource("annotated-parameter-on-nested-class-constructor/Caller.java")
.getPath());
.getPath(),
StandardCharsets.UTF_8));
launcher.getEnvironment().setSourceClasspath(
new String[]{
"src/test/resources"
Expand Down
33 changes: 33 additions & 0 deletions src/test/java/spoon/test/issue4357/TestIssue4357.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package spoon.test.issue4357;

import org.junit.jupiter.api.Test;
import spoon.Launcher;
import spoon.reflect.CtModel;

import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Paths;

import static org.junit.jupiter.api.Assertions.assertTrue;

public class TestIssue4357 {

@Test
public void testClasspathURLWithSpaces() throws MalformedURLException {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slarse done.

// contract: launcher can handle spaces in classpath URL
Launcher launcher = new Launcher();
URL[] classpath = {
Paths.get("./src/test/resources/path with spaces/lib/bar.jar").toAbsolutePath().toUri().toURL()
};
launcher.getEnvironment().setNoClasspath(false);
launcher.getEnvironment().setShouldCompile(true);
ClassLoader classLoader = new URLClassLoader(classpath);
launcher.getEnvironment().setInputClassLoader(classLoader);
launcher.addInputResource(Paths.get("./src/test/resources/path with spaces/Foo.java").toAbsolutePath().toString());
CtModel model = launcher.buildModel();

assertTrue(model.getAllTypes().stream().anyMatch(ct -> ct.getQualifiedName().equals("Foo")),
"CtTxpe 'Foo' not present in model");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
10 changes: 10 additions & 0 deletions src/test/resources/path with spaces/Foo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import bar.Bar;

public class Foo {
Foo() {
}

Bar foo() {
return new Bar();
}
}
Binary file added src/test/resources/path with spaces/lib/bar.jar
View file
Open in desktop
Binary file not shown.