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 00:54:09 UTC
[GitHub] [geronimo-xbean] powerbroker opened a new pull request, #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
powerbroker opened a new pull request, #34:
URL: https://github.com/apache/geronimo-xbean/pull/34
Extracting '/fu!ll/!path/!to/!archive.jar' instead of '/fu' when path contains '!'s
--
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
[GitHub] [geronimo-xbean] LuciferYang commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1150765329
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -113,35 +114,75 @@ public void testIterator() throws Exception {
assertEquals(classes.length, actual.size());
}
- private List<String> list(final JarArchive archive) {
- final List<String> actual = new ArrayList<>();
- for (final Archive.Entry entry : archive) {
- actual.add(entry.getName());
+ @Test
+ public void testXBEAN337() throws Exception {
+
+
+ // Virtual path
+
+ String path = "/this!/!file!/does!/!not/exist.jar";
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ }catch(Exception ex){
+ Assert.assertTrue(String.format(
+ "Muzz never fail on '/this', but try full path with exclamations('%s') instead",
+ path),
+ ex.getCause().getMessage().contains("exist.jar"));
+ }
+
+
+ // Real file
+
+ File tmpDir = testTmpDir.newFolder("!" + JarArchiveTest.class.getSimpleName() + "!-temp!");
+
+ File exclamated = Files.copy(JarArchiveTest.classpath.toPath(),
+ tmpDir.toPath().resolve(
+ JarArchiveTest.classpath.getName()))
+ .toFile();
+
+ urls[0] = new URL("jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ Assert.assertEquals(String.format("Muzz successfully open '%s'", exclamated.getAbsolutePath()),
+ this.archive.iterator().hasNext(),
+ jar.iterator().hasNext());
+ }
+
+
+ // Unsupported protocols stack
+
+ urls[0] = new URL("http:ftp:jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+ Assert.fail(String.format("Muzz eat only local file URLs:"
+ + " 'file:/...' or 'jar:file:/...!/' but not '%s'",
+ urls[0]));
+ }catch(UnsupportedOperationException ex){
Review Comment:
I have a pr to fix this https://github.com/apache/geronimo-xbean/pull/37, but @rmannibucau told me that the current catch `UnsupportedOperationException ` can also be passed ...
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045314578
##########
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:
thought and thought how to start the check from the beginning, and not from the tail ... and finally didn’t come up with anything valuable. cannot even guess, when trash-prefixes may appear after protocol name and before path substring...
IMHO this little *internal* difference seems invisible from user API level. code works, there are some tests proving - let us leave concept as is. anyway, the if-file-exists check is fundamental here, not suffixes-cutting direction
--
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
[GitHub] [geronimo-xbean] dblevins commented on pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
dblevins commented on PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#issuecomment-1345609708
I'll take a look at this tomorrow as well.
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045496483
##########
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:
Thing is you allow other urls than "jar:file" with this loop so all the mentionned cases.
Side note: foo can really be used, just requires to register a handler. Famous example: pax-url and mvn handler.
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045271076
##########
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:
should JarArchive always throw IllegalStateException(FileNotFoundException("...${url}..."))?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045269988
##########
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:
what do you mean?
start from beginning FAILS, start from end SUCCEEDS. yes, success may cost some resources, that's the price.
here we (almost) always meet ! at the end, since new URL() requires it in 'jar:file:/...!/'. so, we try access file, cut ! off, and - if everything is near default scenario - meet available JAR file(even if it has !s in path or not - doesn't matter) => profit! once.
things become worse when there is a tail full of !s after 'file.jar!/'. definitely non-standard case, and each extra '!' cost new String, new File and extra file system access.
if i missed something...?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051477609
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
Review Comment:
agree in general, and specifically what kind of RuntimeException it will i don't care
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1052682163
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
+ @Test
+ public void testXBEAN337() throws Exception {
Review Comment:
> Just pushed on trunk branch with `@Ignore("PR-34")`.
thanks. thanks a lot!
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051445859
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -63,12 +64,12 @@ public void setUp() throws Exception {
public void testGetBytecode() throws Exception {
for (Class clazz : classes) {
- assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
+ Assert.assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
Review Comment:
Sadly I think geronimo misses some shared config (to be frank I think it is/was also a good compromise but understand it can miss for contributions)
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1150767755
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -113,35 +114,75 @@ public void testIterator() throws Exception {
assertEquals(classes.length, actual.size());
}
- private List<String> list(final JarArchive archive) {
- final List<String> actual = new ArrayList<>();
- for (final Archive.Entry entry : archive) {
- actual.add(entry.getName());
+ @Test
+ public void testXBEAN337() throws Exception {
+
+
+ // Virtual path
+
+ String path = "/this!/!file!/does!/!not/exist.jar";
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ }catch(Exception ex){
+ Assert.assertTrue(String.format(
+ "Muzz never fail on '/this', but try full path with exclamations('%s') instead",
+ path),
+ ex.getCause().getMessage().contains("exist.jar"));
+ }
+
+
+ // Real file
+
+ File tmpDir = testTmpDir.newFolder("!" + JarArchiveTest.class.getSimpleName() + "!-temp!");
+
+ File exclamated = Files.copy(JarArchiveTest.classpath.toPath(),
+ tmpDir.toPath().resolve(
+ JarArchiveTest.classpath.getName()))
+ .toFile();
+
+ urls[0] = new URL("jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ Assert.assertEquals(String.format("Muzz successfully open '%s'", exclamated.getAbsolutePath()),
+ this.archive.iterator().hasNext(),
+ jar.iterator().hasNext());
+ }
+
+
+ // Unsupported protocols stack
+
+ urls[0] = new URL("http:ftp:jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+ Assert.fail(String.format("Muzz eat only local file URLs:"
+ + " 'file:/...' or 'jar:file:/...!/' but not '%s'",
+ urls[0]));
+ }catch(UnsupportedOperationException ex){
Review Comment:
```
Apache Maven 3.8.7 (b89d5959fcde851dcb1c8946a785a163f14e1e29)
Maven home: /home/rmannibucau/.sdkman/candidates/mvnd/0.9.0
Java version: 1.8.0_312, vendor: Azul Systems, Inc., runtime: /home/rmannibucau/.sdkman/candidates/java/8.0.312-zulu/jre
Default locale: fr_FR, platform encoding: UTF-8
OS name: "linux", version: "5.19.0-35-generic", arch: "amd64", family: "unix"
```
master, no diff and build is green, we should likely refine why to ensure we dont hide an issue in tests IMHO
will try to look in a few hours why it passes for me
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051445155
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
+ @Test
+ public void testXBEAN337() throws Exception {
Review Comment:
well, i don't completely understand where i should look to... looking into pulled tests in branch and see nothing.
could you please point?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051444831
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
Review Comment:
i think since test for XBEAN-337 passes, it is time to close it as fixed. doing some cosmetic cleanup, if necessary
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#issuecomment-1346090560
If it helps, here is a proposal a bit simpler, does not do validation but explicitly only only `file:` and `jar:file:`: https://gist.github.com/rmannibucau/ced8a809672a12a6f026e5ccac58a679
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045267541
##########
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:
ok
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1053960992
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -80,7 +78,7 @@ public void testGetBytecode() throws Exception {
try {
archive.getBytecode("Fake");
- fail("ClassNotFoundException should have been thrown");
+ Assert.fail("ClassNotFoundException should have been thrown");
Review Comment:
yep))
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1053961070
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,65 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.endsWith("!/") ?
+ jarPath.substring(0, jarPath.lastIndexOf("!/"))
+ : jarPath);
+ }catch(MalformedURLException ex){
+ throw new UnsupportedOperationException(
Review Comment:
ok
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051446076
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
+ @Test
+ public void testXBEAN337() throws Exception {
Review Comment:
this test (https://gist.github.com/rmannibucau/ced8a809672a12a6f026e5ccac58a679#file-xbean-exclamation-mark-diff-L200) was covering 5 use cases, I don't think them all in this test
--
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
[GitHub] [geronimo-xbean] rmannibucau merged pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau merged PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1056614919
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -54,7 +54,8 @@ public class JarArchiveTest {
private JarArchive archive;
@Rule
- public final TemporaryFolder tmp = new TemporaryFolder();
+ public TemporaryFolder testTmpDir = new TemporaryFolder();
Review Comment:
final was better ;)
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,65 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.endsWith("!/") ?
+ jarPath.substring(0, jarPath.lastIndexOf("!/"))
+ : jarPath);
+ }catch(MalformedURLException ex){
+ throw new IllegalArgumentException(
+ "Please provide 'file:/...' or 'jar:file:/...!/' URL"
+ + " instead of '" + FileArchive.decode(String.valueOf(url)) + "'");
}
- jar = new JarFile(FileArchive.decode(u.getFile())); // no more an url
- } catch (IOException e) {
- throw new IllegalStateException(e);
+ }
+
+ try{
+ // handle 'file:/...' URL
+ if("file".equalsIgnoreCase(url.getProtocol())){
+
+ // Testing if file DOEN't exists AND trying
+ // substrings up to every '!/{...}' as path
+ idx = 0;
+ jarPath = FileArchive.decode(url.getPath());
+ for(String jp = jarPath; !(jarFile = new File(jp)).exists()
+ && (idx = jarPath.indexOf("!/", idx + 1)) > 0;
+ jp = jarPath.substring(0, idx)){}
+
+ // All substrings attempted, but referenced file wasn't discovered
+ if(!jarFile.exists()){
+
+ // To be caught later and wrapped into IllegalStateEx - default behavior
+ throw new FileNotFoundException(FileArchive.decode(String.valueOf(url)));
+ }
+
+ }else{
+ throw new UnsupportedOperationException(
Review Comment:
Didn't we prefer IllegalArgumentException over UnsupportedOperationException which has another meaning?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1056038202
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
in general, everything is correct, impossible to disagree. but in particular we are dealing with extremely entertaining moments. such as the lack of a clear style definition in the project and the need to fall back to manual labor in already well automated tasks.
would like to hope the imports section is the only obstacle preventing the fix from being accomplished
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1056038202
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
in general, everything is correct, impossible to disagree. but in particular we are dealing with extremely entertaining moments. such as the lack of a clear style definition in the project and the need to fall back to manual labor in already well automated tasks.
would like to hope the importы section is the only obstacle preventing the fix is accomplished
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045268414
##########
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:
no, be sure that such tails - with !s - don't break jar file access
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045269358
##########
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:
doesn't matter where it is in 4.22 - everything fails. think, test should prove it works in any position
thx, good idea - @Rule TemporaryFolder. those temp files... ((
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#issuecomment-1345631321
Open point to maybe discuss (can be on the list): do we want to deprecate `public JarArchive(ClassLoader loader, URL url) {` and promote a `public JarArchive(ClassLoader loader, File jarPath) {`? Can solve this issue by not letting the `JarArchive` handle this unwrapping which is highly caller related.
For standalone we can move the logic to `org.apache.xbean.finder.archive.ClasspathArchive#archive`, sounds more relevant IMHO.
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045269988
##########
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:
what do you mean?
start from beginning FAILS, start from end SUCCEEDS. yes, success may cost some resources, that's the price.
here we (almost) always meet ! at the end, since new URL() requires it in 'jar:file:/...!/'. so, we try access file, cut ! off, and - if everything is near default scenario - meet available JAR file(even if it has !s in path or not - doesn't matter) => profit! once.
things become worst when there is a tail full of !s after 'file.jar!/'. definitely non-standard case, and each extra '!' cost new String, new File and extra file system access.
if i missed something...?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045307959
##########
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:
> > jar:foo:file is actually prohibited by new URL(), why not allow this impossible scenario?
>
> Prohibited by who? Thing is that we actually only support `jar` and `file`, now you allow `jar:(whatever:)*file` - this is allowed by `URL` and `URLHandler` API. Don't think it is what we want so we should just unwrap once or let the caller unwrap it more before passing the path to the archive IMHO.
i cannot do e.g. new URL("jar:foa:file:/c:/Temp"), but - wow - can new URL("http:rtsp:ftp:jar:file:/c:/Temp")!
that's technically bullshit, but you're right - possible.
btw, absolhuck-o-lutely have no idea why don't URL.getProtocol() just say once "jar:file" instead of nesting URLs. or, if nesting, not to be e.g. "file:/usr/lib.jar:jar:/inner/file.txt"!
> Nested jar support is done through other `Archive` implementation like https://github.com/Talend/component-runtime/blob/master/component-runtime-manager/src/main/java/org/talend/sdk/component/runtime/manager/xbean/NestedJarArchive.java but it requires some knowledge about the enclosing structure so better to not try to be too clever there IMHO.
yep, too clever with restrictions as well ;) can't shake the feeling that this is what I'm doing...
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051446012
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
Review Comment:
yep but we don't want to get on main branch code with unexpected comments or exception so I think it is important to not only care of the passing case but more of error cases which are always the one costing the most ;)
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1150208611
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -113,35 +114,75 @@ public void testIterator() throws Exception {
assertEquals(classes.length, actual.size());
}
- private List<String> list(final JarArchive archive) {
- final List<String> actual = new ArrayList<>();
- for (final Archive.Entry entry : archive) {
- actual.add(entry.getName());
+ @Test
+ public void testXBEAN337() throws Exception {
+
+
+ // Virtual path
+
+ String path = "/this!/!file!/does!/!not/exist.jar";
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ }catch(Exception ex){
+ Assert.assertTrue(String.format(
+ "Muzz never fail on '/this', but try full path with exclamations('%s') instead",
+ path),
+ ex.getCause().getMessage().contains("exist.jar"));
+ }
+
+
+ // Real file
+
+ File tmpDir = testTmpDir.newFolder("!" + JarArchiveTest.class.getSimpleName() + "!-temp!");
+
+ File exclamated = Files.copy(JarArchiveTest.classpath.toPath(),
+ tmpDir.toPath().resolve(
+ JarArchiveTest.classpath.getName()))
+ .toFile();
+
+ urls[0] = new URL("jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ Assert.assertEquals(String.format("Muzz successfully open '%s'", exclamated.getAbsolutePath()),
+ this.archive.iterator().hasNext(),
+ jar.iterator().hasNext());
+ }
+
+
+ // Unsupported protocols stack
+
+ urls[0] = new URL("http:ftp:jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+ Assert.fail(String.format("Muzz eat only local file URLs:"
+ + " 'file:/...' or 'jar:file:/...!/' but not '%s'",
+ urls[0]));
+ }catch(UnsupportedOperationException ex){
Review Comment:
Guess cause the error is thrown internally by the jvm but you probably spotted an issue in the test too if true
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by "powerbroker (via GitHub)" <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1150759772
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -113,35 +114,75 @@ public void testIterator() throws Exception {
assertEquals(classes.length, actual.size());
}
- private List<String> list(final JarArchive archive) {
- final List<String> actual = new ArrayList<>();
- for (final Archive.Entry entry : archive) {
- actual.add(entry.getName());
+ @Test
+ public void testXBEAN337() throws Exception {
+
+
+ // Virtual path
+
+ String path = "/this!/!file!/does!/!not/exist.jar";
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ }catch(Exception ex){
+ Assert.assertTrue(String.format(
+ "Muzz never fail on '/this', but try full path with exclamations('%s') instead",
+ path),
+ ex.getCause().getMessage().contains("exist.jar"));
+ }
+
+
+ // Real file
+
+ File tmpDir = testTmpDir.newFolder("!" + JarArchiveTest.class.getSimpleName() + "!-temp!");
+
+ File exclamated = Files.copy(JarArchiveTest.classpath.toPath(),
+ tmpDir.toPath().resolve(
+ JarArchiveTest.classpath.getName()))
+ .toFile();
+
+ urls[0] = new URL("jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ Assert.assertEquals(String.format("Muzz successfully open '%s'", exclamated.getAbsolutePath()),
+ this.archive.iterator().hasNext(),
+ jar.iterator().hasNext());
+ }
+
+
+ // Unsupported protocols stack
+
+ urls[0] = new URL("http:ftp:jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+ Assert.fail(String.format("Muzz eat only local file URLs:"
+ + " 'file:/...' or 'jar:file:/...!/' but not '%s'",
+ urls[0]));
+ }catch(UnsupportedOperationException ex){
Review Comment:
it's my mistake, definitely IllegalArgumentException should be instead of UnsupportedOperationException.
the test fails with UnsupportedOperationException, i wonder how i could miss it.
guys, my project configuration is now broken. can i kindly ask you to include this little fix into your commits?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045307959
##########
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:
> > jar:foo:file is actually prohibited by new URL(), why not allow this impossible scenario?
>
> Prohibited by who? Thing is that we actually only support `jar` and `file`, now you allow `jar:(whatever:)*file` - this is allowed by `URL` and `URLHandler` API. Don't think it is what we want so we should just unwrap once or let the caller unwrap it more before passing the path to the archive IMHO.
i cannot do e.g. new URL("jar:foa:file:/c:/Temp"), but - wow - can new URL("http:rtsp:ftp:jar:file:/c:/Temp")!
that's technically bullshit, but you're right - possible.
btw, absolhuck-o-lutely have no idea why don't URL.getProtocol() just say once "jar:file" instead of nesting URLs. or, if nesting, not to be e.g. "file:/usr/lib.jar:jar:/inner/file.txt"!
> Nested jar support is done through other `Archive` implementation like https://github.com/Talend/component-runtime/blob/master/component-runtime-manager/src/main/java/org/talend/sdk/component/runtime/manager/xbean/NestedJarArchive.java but it requires some knowledge about the enclosing structure so better to not try to be too clever there IMHO.
yep, too clever with restrictions as well ;) can't shake the feeling that this is what I'm doing...
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045267857
##########
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:
+2
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045267812
##########
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:
+1
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051448994
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
Review Comment:
absolutely have no idea why it should be indexOf. at the beginning of story single-place change 'indexOf -> lastIndexOf' was fixing the issue. test passes with lastIndexOf, as well as with indexOf with some modifications...
well, if you so want indexOf - here it is.
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051477609
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
Review Comment:
agree in general, and specifically what kind of RuntimeException it will i don't care. i don't like the exception zoo, leading to try/multi catch leading in real life to logging one OhHuck$#itHappened error message
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051438054
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
+ throw new UnsupportedOperationException(
+ "Please provide 'file:/...' or 'jar:file:/...!/' URL"
+ + " instead of '" + FileArchive.decode(String.valueOf(url)) + "'");
}
- jar = new JarFile(FileArchive.decode(u.getFile())); // no more an url
- } catch (IOException e) {
- throw new IllegalStateException(e);
+ }
+
+ try{
+ // handle 'file:/...' URL
+ if("file".equalsIgnoreCase(url.getProtocol())){
+
+ // Testing if file DOEN't exists AND cutting !/{...}'
+ // suffix-by-suffix until file discovered or run out of suffixes
+ for(jarPath = FileArchive.decode(url.getPath());
+ !(jarFile = new File(jarPath)).exists()
+ && (idx = jarPath.lastIndexOf("!/")) > 0;
+ jarPath = jarPath.substring(0, idx)){ }
+
+ // No more suffixes to cut, but referenced file wasn't discovered
+ if(!jarFile.exists()){
+
+ // To be caught later and wrapped into IllegalStateEx - default behavior
+ throw new FileNotFoundException(FileArchive.decode(String.valueOf(url)));
+ }
+
+ }else{
+ throw new UnsupportedOperationException(
+ "Please provide 'file:/...' or 'jar:file:/...!/' URL"
+ + " instead of '" + FileArchive.decode(String.valueOf(url)) + "'");
+ }
+
+ jar = new JarFile(jarFile);
+
+ }catch(IOException e){
+ throw new IllegalStateException("Cannot open jar(zip) '"
+ + jarFile != null ? // why can it be null? but since compiler thinks so...
Review Comment:
ok, it knows better
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1056038202
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
in general, everything is correct, impossible to disagree. but in particular we are dealing with extremely entertaining moments. such as lack of a clear style definition in the project and need to fall back to manual labor in already well automated tasks.
would like to hope the imports section is the only obstacle preventing the fix from being accomplished
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1054091723
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
You can drop unused but please dont change the sorting in an unrelated pr
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051453129
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
Review Comment:
> at the beginning of story single-place change 'indexOf -> lastIndexOf' was fixing the issue
this is why I explained you and added also the example that it does *not* fix the issue, it fixes your particular case and breaks others
overall both are wrong but indexOf has the advantage to stick on the common case in terms of medium complexity whereas lastIndexOf changes this so I prefer to be conservator on that aspect since it does not help to reverse it on the issue the PR fixes.
the thing is that then the file can be wrong so the algorithm is maybe no more correct (using url), this is why I insist we cover all the potential cases to ensure we don't introduce regressions trying to fix one
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1053025654
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
style ;)
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,65 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.endsWith("!/") ?
+ jarPath.substring(0, jarPath.lastIndexOf("!/"))
+ : jarPath);
+ }catch(MalformedURLException ex){
+ throw new UnsupportedOperationException(
Review Comment:
dont forget to move to IllegalArgumentException please
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -80,7 +78,7 @@ public void testGetBytecode() throws Exception {
try {
archive.getBytecode("Fake");
- fail("ClassNotFoundException should have been thrown");
+ Assert.fail("ClassNotFoundException should have been thrown");
Review Comment:
back?
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1055299704
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
Here the goal is to reduce the patch size and side effect of a global IDE shortcut for other dev, nothing related to disk or repository infrastructure. Long story short the rational is:
1. limit the number of lines touched by the patch to the needed ones (once again no issue to drop useless imports but re-sorting it is a bad practise IMHO and breaks 2)
2. ensure that blaming the class we don't get the last person who sorted the import which kind of breaks the blame feature. We want the actual person who coded the line last and when I say coded here, I mean with some IP and not just some IDE refactoring.
Both are important and this is why I'm asking you to comply to the existing style - note: I didn't pick it myself and I'm not judging it is good or bad ;), just to ensure it works in time.
In terms of sorting, you picked alphabetical one but there are other common strategies which are valid:
* java then javax then others then static
* static then java then javax then others
* java then others
* alphabetical
* ...
None is bad and none is worse than another.
Generally speaking when you contribute to a project you respect its style you like it or not - and I'm the first one to complain about some ASF style ;) - so please just ensure git diff is minimal and highlights the change you did without side effects.
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1053962119
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
and here I would try to understand and forgive - all removed imports were 'unused'
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#issuecomment-1345503365
This actually hits a java bug where nested '!' are not supported in urls.
Can you at least add a boolean toggle in JarArchuve to switch the behavior cause opposite bug exists too or better, use a fallback strategy (while file path does not exist, use next exclamation mark if exists)?
Also please fix test style to reduce the patch surface.
Thks
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045267258
##########
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:
jar:foo:file is actually prohibited by new URL(), why not allow this impossible scenario?
from other side, if they let e.g. tar:gz:file, this will work here and, if fail - deeper, where e.g. 'tar' format should be supported.
in general, AFAIK we are interested here in url, pointing to (available, because of possible '!'s in path) local zip file: '...file:/.../some.jar....'. i think:
1. a File parameter, instead of URL would be appropriate
2. extra prefixes and suffixes may cause warning, but shouldn't ruin jar access.
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045268118
##########
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:
+4
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045269988
##########
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:
what do you mean?
start from beginning FAILS, start from end SUCCEEDS. yes, success may cost some resources, that's the price.
if i missed something...?
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045281610
##########
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:
ok, works for me since it shouldn't affect jar scanning much
--
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
[GitHub] [geronimo-xbean] LuciferYang commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1149965749
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -113,35 +114,75 @@ public void testIterator() throws Exception {
assertEquals(classes.length, actual.size());
}
- private List<String> list(final JarArchive archive) {
- final List<String> actual = new ArrayList<>();
- for (final Archive.Entry entry : archive) {
- actual.add(entry.getName());
+ @Test
+ public void testXBEAN337() throws Exception {
+
+
+ // Virtual path
+
+ String path = "/this!/!file!/does!/!not/exist.jar";
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ }catch(Exception ex){
+ Assert.assertTrue(String.format(
+ "Muzz never fail on '/this', but try full path with exclamations('%s') instead",
+ path),
+ ex.getCause().getMessage().contains("exist.jar"));
+ }
+
+
+ // Real file
+
+ File tmpDir = testTmpDir.newFolder("!" + JarArchiveTest.class.getSimpleName() + "!-temp!");
+
+ File exclamated = Files.copy(JarArchiveTest.classpath.toPath(),
+ tmpDir.toPath().resolve(
+ JarArchiveTest.classpath.getName()))
+ .toFile();
+
+ urls[0] = new URL("jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ Assert.assertEquals(String.format("Muzz successfully open '%s'", exclamated.getAbsolutePath()),
+ this.archive.iterator().hasNext(),
+ jar.iterator().hasNext());
+ }
+
+
+ // Unsupported protocols stack
+
+ urls[0] = new URL("http:ftp:jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+ Assert.fail(String.format("Muzz eat only local file URLs:"
+ + " 'file:/...' or 'jar:file:/...!/' but not '%s'",
+ urls[0]));
+ }catch(UnsupportedOperationException ex){
Review Comment:
Sorry, a late question, why does it catch `UnsupportedOperationException` instead of `IllegalArgumentException` here?
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051356926
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
+ throw new UnsupportedOperationException(
+ "Please provide 'file:/...' or 'jar:file:/...!/' URL"
+ + " instead of '" + FileArchive.decode(String.valueOf(url)) + "'");
}
- jar = new JarFile(FileArchive.decode(u.getFile())); // no more an url
- } catch (IOException e) {
- throw new IllegalStateException(e);
+ }
+
+ try{
+ // handle 'file:/...' URL
+ if("file".equalsIgnoreCase(url.getProtocol())){
+
+ // Testing if file DOEN't exists AND cutting !/{...}'
+ // suffix-by-suffix until file discovered or run out of suffixes
+ for(jarPath = FileArchive.decode(url.getPath());
+ !(jarFile = new File(jarPath)).exists()
+ && (idx = jarPath.lastIndexOf("!/")) > 0;
+ jarPath = jarPath.substring(0, idx)){ }
+
+ // No more suffixes to cut, but referenced file wasn't discovered
+ if(!jarFile.exists()){
+
+ // To be caught later and wrapped into IllegalStateEx - default behavior
+ throw new FileNotFoundException(FileArchive.decode(String.valueOf(url)));
+ }
+
+ }else{
+ throw new UnsupportedOperationException(
+ "Please provide 'file:/...' or 'jar:file:/...!/' URL"
+ + " instead of '" + FileArchive.decode(String.valueOf(url)) + "'");
+ }
+
+ jar = new JarFile(jarFile);
+
+ }catch(IOException e){
+ throw new IllegalStateException("Cannot open jar(zip) '"
+ + jarFile != null ? // why can it be null? but since compiler thinks so...
Review Comment:
Can if none of protocols match so no compiler issue ;)
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -63,12 +64,12 @@ public void setUp() throws Exception {
public void testGetBytecode() throws Exception {
for (Class clazz : classes) {
- assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
+ Assert.assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
Review Comment:
Shouldnt change, please fix the unexpected diff parts
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ 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 + "!/some!/!inner!/.jar!/file.jar")};
+
+ try {
+ jar = new JarArchive(new URLClassLoader(urls), urls[0]);
Review Comment:
Dont forget to close jar files
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
+ @Test
+ public void testXBEAN337() throws Exception {
Review Comment:
Maybe get the missing cases from the diff, it covers a bit more the pr cases IIRC
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
Review Comment:
Is it really the cause or just the contract which enforces it so it is impossible to get?
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
Review Comment:
indexOf and not lastI indexOf as shown by shared tests
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051440363
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
Review Comment:
Yes, it is really about the URL contract and potentially a wrong sub-protocol (not file nor a supported protocol)
Overall the point is that the comment is misleading so shouldn't be written this way and likely not rethrow it as a `UnsupportedOperationException` but more an `IllegalArgumentException` maybe, wdyt?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051440953
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ 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 + "!/some!/!inner!/.jar!/file.jar")};
+
+ try {
+ jar = new JarArchive(new URLClassLoader(urls), urls[0]);
Review Comment:
thx
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051477609
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
Review Comment:
agree in general, and specifically what kind of RuntimeException it will i don't care. i don't like the exception zoo, leading in real life to try/multi catch for logging one OhHuck$#itHappened error message
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051476696
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
Review Comment:
so be it. I almost look only at the JarArchive code and its unit test
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051443644
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -63,12 +64,12 @@ public void setUp() throws Exception {
public void testGetBytecode() throws Exception {
for (Class clazz : classes) {
- assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
+ Assert.assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
Review Comment:
If I'm not mistaken it is the exact opposite, you added Assert everywhere (maybe your IDE tricked you?) instead of following the pattern in place in the class so I'm just asking you to not change the code you don't modify and instead of forcing the existing code to comply to your settings, to align your settings/coding style on the existing code.
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051444664
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -63,12 +64,12 @@ public void setUp() throws Exception {
public void testGetBytecode() throws Exception {
for (Class clazz : classes) {
- assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
+ Assert.assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
Review Comment:
there is a setting in my ide "Alter ONLY manually edited code", and seems it doesn't work as i expect.
btw is there an Eclipse config file with code style settings for this project?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1056038202
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
in general, everything is correct, impossible to disagree. but in particular we are dealing with extremely entertaining moments. such as the lack of a clear style definition in the project and the need to fall back to manual labor in already well automated tasks.
would like to hope the imports section is the only obstacle preventing the fix is accomplished
--
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
[GitHub] [geronimo-xbean] powerbroker commented on pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#issuecomment-1346563119
> If it helps, here is a proposal a bit simpler, does not do validation but explicitly only only `file:` and `jar:file:`: https://gist.github.com/rmannibucau/ced8a809672a12a6f026e5ccac58a679
well,why not. the main thing is the test for exclamation marks in jar path must pass.
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045497969
##########
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:
Just replace lastindexof by indexof and keep track in the loop of "current" starting index - otherwise the loop is kot that useful.
So idea is to use indexOf(char,int)
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045282353
##########
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:
> start from beginning FAILS, start from end SUCCEEDS
For your test case but you can have the opposite case as well.
This is why I insist to test the file presence as a validation.
With such validation, we just need to decide if we start from start or end to find `!`.
Since the 99% of cases is there is no `!` in folders - JVM does not support it properly, there are numerous bugs on JDK bugtracker about that - it is likely saner to start from the beginning and keep consistent with the current behavior.
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045267775
##########
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:
ok
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051439651
##########
xbean-finder/src/main/java/org/apache/xbean/finder/archive/JarArchive.java:
##########
@@ -41,22 +44,63 @@ public class JarArchive implements Archive, AutoCloseable {
private final JarFile jar;
private final MJarSupport mjar = new MJarSupport();
- public JarArchive(ClassLoader loader, URL url) {
+ /*
+ * Supports only 'file:/...' or 'jar:file:/...!/' URLs
+ */
+ public JarArchive(ClassLoader loader, URL url){
// if (!"jar".equals(url.getProtocol())) throw new IllegalArgumentException("not a jar url: " + url);
- try {
- this.loader = loader;
- this.url = url;
- URL u = url;
+ this.loader = loader;
+ this.url = url;
+ File jarFile = null;
+ String jarPath;
+ int idx;
- String jarPath = url.getFile();
- if (jarPath.contains("!")) {
- jarPath = jarPath.substring(0, jarPath.indexOf("!"));
- u = new URL(jarPath);
+ // Wipe out 'jar:' prefix AND '!/{...}' suffix(if any)
+ if("jar".equalsIgnoreCase(url.getProtocol())){
+
+ try{
+ jarPath = url.getPath();
+ url = new URL(jarPath.substring(0, jarPath.lastIndexOf("!/")));
+ }catch(MalformedURLException ex){
+ // Probably CPU overheat and/or DRAM undervoltage
Review Comment:
trying and trying - with no success... in THIS CASE i think just a contract
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051486174
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
+ @Test
+ public void testXBEAN337() throws Exception {
Review Comment:
thanks, they helped to find/fix 1 issue.
can i ask you to integrate them?
cannot force my git to apply patch, so have to do it manually. and afraid it will turn in too much cosmetics to fix...
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051587029
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
+ @Test
+ public void testXBEAN337() throws Exception {
Review Comment:
well, what is easier for you - disabled on main branch so i sync, merge locally, enable them and run; or to this branch, as enabled.
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045267357
##########
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:
ok
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045269358
##########
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:
doesn't matter where it is in 4.22 - everything fails. think, test should prove it works in any position
thx, good idea - @Rule TemporaryFolder. oh, those temp files... ((
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045269988
##########
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:
what do you mean?
start from beginning FAILS, start from end SUCCEEDS. yes, success may cost some resources, that's the price.
here we (almost) always meet ! at the end, since new URL() requires it in 'jar:file:/...!/'. so, we try access file, cut ! off, and - if everything is near default scenario - meet available JAR file(even if it has !s in path or not - doesn't matter) => profit! once.
things become worse when there is a tail full of !s after 'file.jar!/'. definitely non-standard case, and each extra '!' cost new String, new File and *extra file system access*.
if i missed something...?
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045281235
##########
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:
> jar:foo:file is actually prohibited by new URL(), why not allow this impossible scenario?
Prohibited by who? Thing is that we actually only support `jar` and `file`, now you allow `jar:(whatever:)*file` - this is allowed by `URL` and `URLHandler` API. Don't think it is what we want so we should just unwrap once or let the caller unwrap it more before passing the path to the archive IMHO.
Nested jar support is done through other `Archive` implementation like https://github.com/Talend/component-runtime/blob/master/component-runtime-manager/src/main/java/org/talend/sdk/component/runtime/manager/xbean/NestedJarArchive.java but it requires some knowledge about the enclosing structure so better to not try to be too clever there IMHO.
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045282542
##########
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:
Not sure what you mean but we should probably keep current exception behavior, we can change messages if you think it is relevant
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by "powerbroker (via GitHub)" <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1150773605
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -113,35 +114,75 @@ public void testIterator() throws Exception {
assertEquals(classes.length, actual.size());
}
- private List<String> list(final JarArchive archive) {
- final List<String> actual = new ArrayList<>();
- for (final Archive.Entry entry : archive) {
- actual.add(entry.getName());
+ @Test
+ public void testXBEAN337() throws Exception {
+
+
+ // Virtual path
+
+ String path = "/this!/!file!/does!/!not/exist.jar";
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ }catch(Exception ex){
+ Assert.assertTrue(String.format(
+ "Muzz never fail on '/this', but try full path with exclamations('%s') instead",
+ path),
+ ex.getCause().getMessage().contains("exist.jar"));
+ }
+
+
+ // Real file
+
+ File tmpDir = testTmpDir.newFolder("!" + JarArchiveTest.class.getSimpleName() + "!-temp!");
+
+ File exclamated = Files.copy(JarArchiveTest.classpath.toPath(),
+ tmpDir.toPath().resolve(
+ JarArchiveTest.classpath.getName()))
+ .toFile();
+
+ urls[0] = new URL("jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ Assert.assertEquals(String.format("Muzz successfully open '%s'", exclamated.getAbsolutePath()),
+ this.archive.iterator().hasNext(),
+ jar.iterator().hasNext());
+ }
+
+
+ // Unsupported protocols stack
+
+ urls[0] = new URL("http:ftp:jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+ Assert.fail(String.format("Muzz eat only local file URLs:"
+ + " 'file:/...' or 'jar:file:/...!/' but not '%s'",
+ urls[0]));
+ }catch(UnsupportedOperationException ex){
Review Comment:
i just have no idea how i could commit a failing test 3 months ago... and how @rmannibucau could accept it...
but now in my(obsolete & broken) context the test fails - there is 'throw IllegalArgument..' in constructor AND 'catch UnsupportedOp...' in test
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045269988
##########
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:
what do you mean?
start from beginning FAILS, start from end SUCCEEDS. yes, success may cost some resources, that's the price.
here we (almost) always meet ! at the end, since new URL() requires it in 'jar:file:/...!/'. so, we try access file, cut ! off, and - if everything is near default scenario - meet available JAR file(even if it has !s in path or not - doesn't matter) => profit! once.
things become worse when there is a tail full of !s after 'file.jar!/'. definitely non-standard case, and each extra '!' costs new String, new File and *extra file system access*.
if i missed something...?
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045268062
##########
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:
.formatted(), i guess
##########
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:
+3
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045268414
##########
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:
no, be sure that such tails don't break jar file access
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1045307959
##########
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:
> > jar:foo:file is actually prohibited by new URL(), why not allow this impossible scenario?
>
> Prohibited by who? Thing is that we actually only support `jar` and `file`, now you allow `jar:(whatever:)*file` - this is allowed by `URL` and `URLHandler` API. Don't think it is what we want so we should just unwrap once or let the caller unwrap it more before passing the path to the archive IMHO.
i cannot do e.g. new URL("jar:foa:file:/c:/Temp"), but - wow - can new URL("http:rtsp:ftp:jar:file:/c:/Temp")!
that's technically bullshit, but you're right - possible.
btw, absolhuck-o-lutely have no idea why don't URL.getProtocol() just say once "jar:file" instead of nesting URLs. or, if nesting, not to be e.g. "file:/usr/lib.jar:jar:/inner/file.txt"!
> Nested jar support is done through other `Archive` implementation like https://github.com/Talend/component-runtime/blob/master/component-runtime-manager/src/main/java/org/talend/sdk/component/runtime/manager/xbean/NestedJarArchive.java but it requires some knowledge about the enclosing structure so better to not try to be too clever there IMHO.
yep, too clever with restrictions as well ;) can't shake the feeling that this is what I'm doing...
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051562968
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
+ @Test
+ public void testXBEAN337() throws Exception {
Review Comment:
Sure - but will likely wait tomorrow.
Just to be sure: you want I integrate the test as disabled on main branch?
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051642787
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -95,14 +96,65 @@ public void testIterator() throws Exception {
actual.add(entry.getName());
}
- assertFalse(0 == actual.size());
+ Assert.assertFalse(0 == actual.size());
for (Class clazz : classes) {
- assertTrue(clazz.getName(), actual.contains(clazz.getName()));
+ Assert.assertTrue(clazz.getName(), actual.contains(clazz.getName()));
}
- assertEquals(classes.length, actual.size());
+ Assert.assertEquals(JarArchiveTest.classes.length, actual.size());
}
+ @Test
+ public void testXBEAN337() throws Exception {
Review Comment:
Just pushed on trunk branch with `@Ignore("PR-34")`.
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1051442994
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -63,12 +64,12 @@ public void setUp() throws Exception {
public void testGetBytecode() throws Exception {
for (Class clazz : classes) {
- assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
+ Assert.assertNotNull(clazz.getName(), archive.getBytecode(clazz.getName()));
Review Comment:
this would be single line without 'Assert.' prefix in whole test, let us leave it as is
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#issuecomment-1364486933
Thanks a lot for the hard work, really appreciated you did the work to comply to existing codebase, kudo!
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1056038202
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
in general, everything is correct, impossible to disagree. but in particular we are dealing with extremely entertaining moments. such as the lack of a clear style definition in the project and the need to roll back to manual labor in already well automated tasks.
would like to hope the importы section is the only obstacle preventing the fix is accomplished
--
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
[GitHub] [geronimo-xbean] powerbroker commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by GitBox <gi...@apache.org>.
powerbroker commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1055292190
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -16,15 +16,10 @@
*/
package org.apache.xbean.finder.archive;
-import org.acme.foo.Blue;
Review Comment:
but what are we fighting for in this place, for what values?
in the main code it is clear: the shorter changes are, the faster they will be reviewed and comprehended.
and here - do we save space in repository on the size of patches?
in addition, the IDE sorts imports alphabetically. from my side, even if it happens from time to time it's more good than bad.
p.s. can we discuss project structure by e-mail? spamcollector_@t_yandex.ru, if you don't mind
--
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
[GitHub] [geronimo-xbean] rmannibucau commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1150767755
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -113,35 +114,75 @@ public void testIterator() throws Exception {
assertEquals(classes.length, actual.size());
}
- private List<String> list(final JarArchive archive) {
- final List<String> actual = new ArrayList<>();
- for (final Archive.Entry entry : archive) {
- actual.add(entry.getName());
+ @Test
+ public void testXBEAN337() throws Exception {
+
+
+ // Virtual path
+
+ String path = "/this!/!file!/does!/!not/exist.jar";
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ }catch(Exception ex){
+ Assert.assertTrue(String.format(
+ "Muzz never fail on '/this', but try full path with exclamations('%s') instead",
+ path),
+ ex.getCause().getMessage().contains("exist.jar"));
+ }
+
+
+ // Real file
+
+ File tmpDir = testTmpDir.newFolder("!" + JarArchiveTest.class.getSimpleName() + "!-temp!");
+
+ File exclamated = Files.copy(JarArchiveTest.classpath.toPath(),
+ tmpDir.toPath().resolve(
+ JarArchiveTest.classpath.getName()))
+ .toFile();
+
+ urls[0] = new URL("jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ Assert.assertEquals(String.format("Muzz successfully open '%s'", exclamated.getAbsolutePath()),
+ this.archive.iterator().hasNext(),
+ jar.iterator().hasNext());
+ }
+
+
+ // Unsupported protocols stack
+
+ urls[0] = new URL("http:ftp:jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+ Assert.fail(String.format("Muzz eat only local file URLs:"
+ + " 'file:/...' or 'jar:file:/...!/' but not '%s'",
+ urls[0]));
+ }catch(UnsupportedOperationException ex){
Review Comment:
```
Apache Maven 3.8.7 (b89d5959fcde851dcb1c8946a785a163f14e1e29)
Maven home: /home/rmannibucau/.sdkman/candidates/mvnd/0.9.0
Java version: 1.8.0_312, vendor: Azul Systems, Inc., runtime: /home/rmannibucau/.sdkman/candidates/java/8.0.312-zulu/jre
Default locale: fr_FR, platform encoding: UTF-8
OS name: "linux", version: "5.19.0-35-generic", arch: "amd64", family: "unix"
```
master, no diff and build is green, we should likely refine why to ensure we dont hide an issue in tests IMHO
will try to look in a few hours why it passes for me
ok, found my issue, got some intermediary state of previous PR, mea culpa, ok to drop UnsupportedOpEx
--
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
[GitHub] [geronimo-xbean] LuciferYang commented on a diff in pull request #34: XBEAN-337 jar path with exclamation signs(!) should be resolved correctly
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #34:
URL: https://github.com/apache/geronimo-xbean/pull/34#discussion_r1150777460
##########
xbean-finder/src/test/java/org/apache/xbean/finder/archive/JarArchiveTest.java:
##########
@@ -113,35 +114,75 @@ public void testIterator() throws Exception {
assertEquals(classes.length, actual.size());
}
- private List<String> list(final JarArchive archive) {
- final List<String> actual = new ArrayList<>();
- for (final Archive.Entry entry : archive) {
- actual.add(entry.getName());
+ @Test
+ public void testXBEAN337() throws Exception {
+
+
+ // Virtual path
+
+ String path = "/this!/!file!/does!/!not/exist.jar";
+ URL[] urls = {new URL("jar:file:" + path + "!/some!/!inner!/.jar!/file.jar")};
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ }catch(Exception ex){
+ Assert.assertTrue(String.format(
+ "Muzz never fail on '/this', but try full path with exclamations('%s') instead",
+ path),
+ ex.getCause().getMessage().contains("exist.jar"));
+ }
+
+
+ // Real file
+
+ File tmpDir = testTmpDir.newFolder("!" + JarArchiveTest.class.getSimpleName() + "!-temp!");
+
+ File exclamated = Files.copy(JarArchiveTest.classpath.toPath(),
+ tmpDir.toPath().resolve(
+ JarArchiveTest.classpath.getName()))
+ .toFile();
+
+ urls[0] = new URL("jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+
+ Assert.assertEquals(String.format("Muzz successfully open '%s'", exclamated.getAbsolutePath()),
+ this.archive.iterator().hasNext(),
+ jar.iterator().hasNext());
+ }
+
+
+ // Unsupported protocols stack
+
+ urls[0] = new URL("http:ftp:jar:" + exclamated.toURI().toURL() + "!/");
+
+ try(JarArchive jar = new JarArchive(new URLClassLoader(urls), urls[0])){
+ Assert.fail(String.format("Muzz eat only local file URLs:"
+ + " 'file:/...' or 'jar:file:/...!/' but not '%s'",
+ urls[0]));
+ }catch(UnsupportedOperationException ex){
Review Comment:
It doesn't matter. Let's fix it together :)
--
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