Close JarFile #1970
Close JarFile #1970
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
trask and
tylerbenson
as code owners
| @@ -127,10 +128,9 @@ private static String getPackageName(String className) { | |||
| } | |||
|
|
|||
| private static Manifest getManifest(URL url) { | |||
| try { | |||
| JarFile jarFile = new JarFile(url.getFile()); | |||
| try (JarFile jarFile = new JarFile(url.toURI().getPath())) { | |||
There was a problem hiding this comment.
Do we need to change the url.getFile()?
There was a problem hiding this comment.
If path contains a space then url.getFile() will leave it as %20, url.toURI().getPath() decodes it as an actual space character
There was a problem hiding this comment.
Does it cause a problem? I'm surprised the more direct way of getting a File would cause JarFile constructor to fail, if you're seeing an exception let's add a comment above this line on what exception we are trying to prevent with the indirect code.
There was a problem hiding this comment.
URL.getFile returns a String. As the url is constructed with new File(exporterJar).toURI().toURL(); which encodes space -> %20 when getting file from url we should use a way that translates %20 back to space.
There was a problem hiding this comment.
So it sounds like there is a problem when the path has a space. If so yeah we need to fix it :) But it's confusing, so we need at least a comment documenting the failure case. Even better is a test that fails before the change and passes after, then no one will accidentally revert and regress.
Alternatively, we can keep the code similarly as before and file an issue - the PR title is about closing the jar file but is doing more than just that. It's ok but we need to do it completely, otherwise let's not make too many changes.
No description provided.