You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geronimo.apache.org by GitBox <gi...@apache.org> on 2022/12/11 16:42:03 UTC
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045261031
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +43,52 @@ public class JarArchive implements Archive {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
+ this.loader = loader;
+ this.url = url;
+ File jf;
+ int idx;
+
+ /*
+ * True URL.getPath(...) wiping out protocol prefixes:
+ *
+ * 1. 'jar:file:/c:/temp/...' -> 'jar' + 'file:/c:/temp/...'
+ * 2. 'file:/c:/temp/...' -> 'file' + '/c:/temp/...'
+ *
+ * assuming we accept 'file:/...${xxx.jar}!/' URLs
+ * AND 'zip:file:/...!/' would be cool too, if
+ * allowed by new URL(...)
+ */
try {
- this.loader = loader;
- this.url = url;
- URL u = url;
-
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.lastIndexOf("!"));
- u = new URL(jarPath);
+ // Underlying JarFile(...) requires RandomAccessFile,
+ // so only local file URLs here...
+ while(!"file".equalsIgnoreCase(url.getProtocol())) {
Review Comment:
looks too relaxed, `jar:foo:file` wouldn't fail but it should using this new implementation.
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +43,52 @@ public class JarArchive implements Archive {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
+ this.loader = loader;
+ this.url = url;
+ File jf;
+ int idx;
+
+ /*
+ * True URL.getPath(...) wiping out protocol prefixes:
+ *
+ * 1. 'jar:file:/c:/temp/...' -> 'jar' + 'file:/c:/temp/...'
+ * 2. 'file:/c:/temp/...' -> 'file' + '/c:/temp/...'
+ *
+ * assuming we accept 'file:/...${xxx.jar}!/' URLs
+ * AND 'zip:file:/...!/' would be cool too, if
+ * allowed by new URL(...)
+ */
try {
- this.loader = loader;
- this.url = url;
- URL u = url;
-
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.lastIndexOf("!"));
- u = new URL(jarPath);
+ // Underlying JarFile(...) requires RandomAccessFile,
+ // so only local file URLs here...
+ while(!"file".equalsIgnoreCase(url.getProtocol())) {
+ // no need here in .getQuery() tail, appended by getFile()
+ url = new URL(url.getPath());
}
- jar = new JarFile(FileArchive.decode(u.getFile())); // no more an url
- } catch (IOException e) {
- throw new IllegalStateException(e);
+ }catch(MalformedURLException ex){
+ // No more protocol(s) in path
+ throw new UnsupportedOperationException("Please provide 'file:/...' or 'jar:file:/...!/' URL");
+ }
+
+ // TODO: drop FileArchive.decode?
+ // URLDecoder.decode(url.getPath(), "UTF-8");
+ String jarPath = FileArchive.decode(url.getPath());
+
+ while(!(jf = new File(jarPath)).exists()){
+ if((idx = jarPath.lastIndexOf('!')) > 0){
+ jarPath = jarPath.substring(0, idx);
+ }else{
+ throw new IllegalStateException("Cannot find any files by '%s' url".formatted(url));
+ }
+ }
+
+ try{
+ this.jar = new JarFile(jf);
+ }catch(IOException e){
+ throw new IllegalStateException("Cannot open jar '%s'".formatted(jf.getAbsolutePath()), e);
Review Comment:
same
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -85,34 +87,53 @@ public void testLoadClass() throws Exception {
@Test
public void testIterator() throws Exception {
- List<String> actual = new ArrayList<>();
- for (Archive.Entry entry : this.archive) {
+ List<String> actual = new ArrayList<String>();
+ for (Archive.Entry entry : archive) {
actual.add(entry.getName());
}
Assert.assertFalse(0 == actual.size());
- for (Class clazz : JarArchiveTest.classes) {
+ for (Class clazz : classes) {
Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
-
@Test
public void testXBEAN337() throws Exception {
+ // Virtual path
+ JarArchive jar;
+
String path = "/this!/!file!/does!/!not/exist.jar";
- URL[] urls = {new URL("jar:file:" + path + "!/")};
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
Review Comment:
do you want to test a nested resource with a `!`
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +43,52 @@ public class JarArchive implements Archive {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
+ this.loader = loader;
+ this.url = url;
+ File jf;
+ int idx;
+
+ /*
+ * True URL.getPath(...) wiping out protocol prefixes:
+ *
+ * 1. 'jar:file:/c:/temp/...' -> 'jar' + 'file:/c:/temp/...'
+ * 2. 'file:/c:/temp/...' -> 'file' + '/c:/temp/...'
+ *
+ * assuming we accept 'file:/...${xxx.jar}!/' URLs
+ * AND 'zip:file:/...!/' would be cool too, if
+ * allowed by new URL(...)
+ */
try {
- this.loader = loader;
- this.url = url;
- URL u = url;
-
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.lastIndexOf("!"));
- u = new URL(jarPath);
+ // Underlying JarFile(...) requires RandomAccessFile,
+ // so only local file URLs here...
+ while(!"file".equalsIgnoreCase(url.getProtocol())) {
+ // no need here in .getQuery() tail, appended by getFile()
+ url = new URL(url.getPath());
}
- jar = new JarFile(FileArchive.decode(u.getFile())); // no more an url
- } catch (IOException e) {
- throw new IllegalStateException(e);
+ }catch(MalformedURLException ex){
+ // No more protocol(s) in path
+ throw new UnsupportedOperationException("Please provide 'file:/...' or 'jar:file:/...!/' URL");
+ }
+
+ // TODO: drop FileArchive.decode?
Review Comment:
URLDecoder != FileArchive#decode so no need of this comment
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -42,27 +44,27 @@ public class JarArchiveTest {
@BeforeClass
public static void classSetUp() throws Exception {
- JarArchiveTest.classpath = Archives.jarArchive(JarArchiveTest.classes);
+ classpath = Archives.jarArchive(classes);
Review Comment:
change not needed
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +43,52 @@ public class JarArchive implements Archive {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
+ this.loader = loader;
+ this.url = url;
+ File jf;
+ int idx;
+
+ /*
+ * True URL.getPath(...) wiping out protocol prefixes:
+ *
+ * 1. 'jar:file:/c:/temp/...' -> 'jar' + 'file:/c:/temp/...'
+ * 2. 'file:/c:/temp/...' -> 'file' + '/c:/temp/...'
+ *
+ * assuming we accept 'file:/...${xxx.jar}!/' URLs
+ * AND 'zip:file:/...!/' would be cool too, if
+ * allowed by new URL(...)
+ */
try {
- this.loader = loader;
- this.url = url;
- URL u = url;
-
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.lastIndexOf("!"));
- u = new URL(jarPath);
+ // Underlying JarFile(...) requires RandomAccessFile,
+ // so only local file URLs here...
+ while(!"file".equalsIgnoreCase(url.getProtocol())) {
+ // no need here in .getQuery() tail, appended by getFile()
+ url = new URL(url.getPath());
}
- jar = new JarFile(FileArchive.decode(u.getFile())); // no more an url
- } catch (IOException e) {
- throw new IllegalStateException(e);
+ }catch(MalformedURLException ex){
+ // No more protocol(s) in path
+ throw new UnsupportedOperationException("Please provide 'file:/...' or 'jar:file:/...!/' URL");
+ }
+
+ // TODO: drop FileArchive.decode?
+ // URLDecoder.decode(url.getPath(), "UTF-8");
+ String jarPath = FileArchive.decode(url.getPath());
+
+ while(!(jf = new File(jarPath)).exists()){
+ if((idx = jarPath.lastIndexOf('!')) > 0){
Review Comment:
should be indexOf, we start from the start othewise you eat too quickly resources (exact same comment than in previous review)
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -42,27 +44,27 @@ public class JarArchiveTest {
@BeforeClass
public static void classSetUp() throws Exception {
- JarArchiveTest.classpath = Archives.jarArchive(JarArchiveTest.classes);
+ classpath = Archives.jarArchive(classes);
}
@Before
public void setUp() throws Exception {
- URL[] urls = {new URL("jar:" + JarArchiveTest.classpath.toURI().toURL() + "!/")};
+ URL[] urls = {new URL("jar:" + classpath.toURI().toURL() + "!/")};
- this.archive = new JarArchive(new URLClassLoader(urls), urls[0]);
+ archive = new JarArchive(new URLClassLoader(urls), urls[0]);
}
@Test
public void testGetBytecode() throws Exception {
- for (Class clazz : JarArchiveTest.classes) {
Review Comment:
change not needed
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +43,52 @@ public class JarArchive implements Archive {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
+ this.loader = loader;
+ this.url = url;
+ File jf;
+ int idx;
+
+ /*
+ * True URL.getPath(...) wiping out protocol prefixes:
+ *
+ * 1. 'jar:file:/c:/temp/...' -> 'jar' + 'file:/c:/temp/...'
+ * 2. 'file:/c:/temp/...' -> 'file' + '/c:/temp/...'
+ *
+ * assuming we accept 'file:/...${xxx.jar}!/' URLs
+ * AND 'zip:file:/...!/' would be cool too, if
+ * allowed by new URL(...)
+ */
try {
- this.loader = loader;
- this.url = url;
- URL u = url;
-
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.lastIndexOf("!"));
- u = new URL(jarPath);
+ // Underlying JarFile(...) requires RandomAccessFile,
+ // so only local file URLs here...
+ while(!"file".equalsIgnoreCase(url.getProtocol())) {
+ // no need here in .getQuery() tail, appended by getFile()
+ url = new URL(url.getPath());
}
- jar = new JarFile(FileArchive.decode(u.getFile())); // no more an url
- } catch (IOException e) {
- throw new IllegalStateException(e);
+ }catch(MalformedURLException ex){
+ // No more protocol(s) in path
+ throw new UnsupportedOperationException("Please provide 'file:/...' or 'jar:file:/...!/' URL");
+ }
+
+ // TODO: drop FileArchive.decode?
+ // URLDecoder.decode(url.getPath(), "UTF-8");
+ String jarPath = FileArchive.decode(url.getPath());
+
+ while(!(jf = new File(jarPath)).exists()){
+ if((idx = jarPath.lastIndexOf('!')) > 0){
+ jarPath = jarPath.substring(0, idx);
+ }else{
+ throw new IllegalStateException("Cannot find any files by '%s' url".formatted(url));
Review Comment:
keep in mind we should stay at least java 8 compatible so formatted is not an option, String.format (my 2cts being a plain concatenation is surely more relevant there since faster and more common in terms of code style).
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -42,27 +44,27 @@ public class JarArchiveTest {
@BeforeClass
public static void classSetUp() throws Exception {
- JarArchiveTest.classpath = Archives.jarArchive(JarArchiveTest.classes);
+ classpath = Archives.jarArchive(classes);
}
@Before
public void setUp() throws Exception {
- URL[] urls = {new URL("jar:" + JarArchiveTest.classpath.toURI().toURL() + "!/")};
+ URL[] urls = {new URL("jar:" + classpath.toURI().toURL() + "!/")};
- this.archive = new JarArchive(new URLClassLoader(urls), urls[0]);
+ archive = new JarArchive(new URLClassLoader(urls), urls[0]);
Review Comment:
change not needed
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -42,27 +44,27 @@ public class JarArchiveTest {
@BeforeClass
public static void classSetUp() throws Exception {
- JarArchiveTest.classpath = Archives.jarArchive(JarArchiveTest.classes);
+ classpath = Archives.jarArchive(classes);
}
@Before
public void setUp() throws Exception {
- URL[] urls = {new URL("jar:" + JarArchiveTest.classpath.toURI().toURL() + "!/")};
+ URL[] urls = {new URL("jar:" + classpath.toURI().toURL() + "!/")};
Review Comment:
change not needed
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -71,12 +73,12 @@ public void testGetBytecode() throws Exception {
@Test
public void testLoadClass() throws Exception {
- for (Class clazz : JarArchiveTest.classes) {
Review Comment:
change not needed
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -85,34 +87,53 @@ public void testLoadClass() throws Exception {
@Test
public void testIterator() throws Exception {
- List<String> actual = new ArrayList<>();
- for (Archive.Entry entry : this.archive) {
Review Comment:
change not needed
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -85,34 +87,53 @@ public void testLoadClass() throws Exception {
@Test
public void testIterator() throws Exception {
- List<String> actual = new ArrayList<>();
- for (Archive.Entry entry : this.archive) {
+ List<String> actual = new ArrayList<String>();
+ for (Archive.Entry entry : archive) {
actual.add(entry.getName());
}
Assert.assertFalse(0 == actual.size());
- for (Class clazz : JarArchiveTest.classes) {
Review Comment:
change not needed
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -85,34 +87,53 @@ public void testLoadClass() throws Exception {
@Test
public void testIterator() throws Exception {
- List<String> actual = new ArrayList<>();
- for (Archive.Entry entry : this.archive) {
+ List<String> actual = new ArrayList<String>();
+ for (Archive.Entry entry : archive) {
actual.add(entry.getName());
}
Assert.assertFalse(0 == actual.size());
- for (Class clazz : JarArchiveTest.classes) {
+ for (Class clazz : classes) {
Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
-
Review Comment:
change not needed
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -42,27 +44,27 @@ public class JarArchiveTest {
@BeforeClass
public static void classSetUp() throws Exception {
- JarArchiveTest.classpath = Archives.jarArchive(JarArchiveTest.classes);
+ classpath = Archives.jarArchive(classes);
}
@Before
public void setUp() throws Exception {
- URL[] urls = {new URL("jar:" + JarArchiveTest.classpath.toURI().toURL() + "!/")};
+ URL[] urls = {new URL("jar:" + classpath.toURI().toURL() + "!/")};
- this.archive = new JarArchive(new URLClassLoader(urls), urls[0]);
+ archive = new JarArchive(new URLClassLoader(urls), urls[0]);
}
@Test
public void testGetBytecode() throws Exception {
- for (Class clazz : JarArchiveTest.classes) {
+ for (Class clazz : classes) {
Assert.assertNotNull(clazz.getName(), this.archive.getBytecode(clazz.getName()));
}
try {
- this.archive.getBytecode("Fake");
Review Comment:
change not needed
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -71,12 +73,12 @@ public void testGetBytecode() throws Exception {
@Test
public void testLoadClass() throws Exception {
- for (Class clazz : JarArchiveTest.classes) {
+ for (Class clazz : classes) {
Assert.assertEquals(clazz.getName(), clazz, this.archive.loadClass(clazz.getName()));
}
try {
- this.archive.loadClass("Fake");
+ archive.loadClass("Fake");
Review Comment:
change not needed
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -85,34 +87,53 @@ public void testLoadClass() throws Exception {
@Test
public void testIterator() throws Exception {
- List<String> actual = new ArrayList<>();
- for (Archive.Entry entry : this.archive) {
+ List<String> actual = new ArrayList<String>();
+ for (Archive.Entry entry : archive) {
actual.add(entry.getName());
}
Assert.assertFalse(0 == actual.size());
- for (Class clazz : JarArchiveTest.classes) {
+ for (Class clazz : classes) {
Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
-
@Test
public void testXBEAN337() throws Exception {
+ // Virtual path
+ JarArchive jar;
+
String path = "/this!/!file!/does!/!not/exist.jar";
- URL[] urls = {new URL("jar:file:" + path + "!/")};
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
try {
- this.archive = new JarArchive(new URLClassLoader(urls), urls[0]);
+ jar = new JarArchive(new URLClassLoader(urls), urls[0]);
}catch(Exception ex){
Assert.assertTrue(
"Muzz never fail on '/this', but try full path with exclamations('%s') instead"
.formatted(path),
- ex.getCause().getMessage().contains("exist.jar"));
+ ex.getMessage().contains("exist.jar"));
}
+
+ // Real file
+
+ Path tmpDir = Files.createTempDirectory("!" + JarArchiveTest.class.getSimpleName() + "!-");
Review Comment:
can be worth testing with `!` not being at index 0 of the dir name too?
personally i would use junit temporaryfolder rule instead of creating a leaking temp file like that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org