Allow loading TMX maps from jars in libtiled-java

[ahornace]

fixes #2102

Hello,

one of my students was working on a school project https://github.com/Naimad1CZ/LeapingElements and while checking the code I noticed that the resources could not be loaded from a jar file which did not allow to have a simple java distribution in a single jar file.

This implementation follows the approach discussed in #2170

I was not able to find any simple library that would allow URL modification (something like apache commons) so I had to add a few helper methods because https://docs.oracle.com/javase/8/docs/api/java/net/URI.html#resolve-java.lang.String- did not work as expected when URI pointed to a jar file.

Testing done:
I've added resources.jar which contains all the previous test resources and (re)implemented a few unit tests. I've made some changes to https://github.com/Naimad1CZ/LeapingElements and was able to load the resources from jar file (only by running java -jar <file>.jar).

I was not sure how the copyright works, so feel free to delete my name or add the maintainer names :)

There are still some corner cases that probably require some checks but I wanted to get feedback first.

Thanks :)

[bjorn]

one of my students was working on a school project https://github.com/Naimad1CZ/LeapingElements and while checking the code I noticed that the resources could not be loaded from a jar file which did not allow to have a simple java distribution in a single jar file.

Thank you so much for helping to fix this problem!

I was not able to find any simple library that would allow URL modification (something like apache commons) so I had to add a few helper methods because https://docs.oracle.com/javase/8/docs/api/java/net/URI.html#resolve-java.lang.String- did not work as expected when URI pointed to a jar file.

That's unfortunate. Just curious, but which behavior did you observe with using resolve with URIs pointing to a jar?

I've added resources.jar which contains all the previous test resources and (re)implemented a few unit tests.

This adds a 498 KB binary file. Would it be possible to produce this .jar file as part of the build instead?

I was not sure how the copyright works, so feel free to delete my name or add the maintainer names :)

You added your name to all files you touched or added, that's great!

Finally, regarding the implementation, I noticed you're converting a lot between URL and URI. Would it make sense to just drop the usage of URL and use URI everywhere?

I left two minor comments inline.

[ahornace]

Thanks for the feedback :)

That's unfortunate. Just curious, but which behavior did you observe with using resolve with URIs pointing to a jar?

new URL("jar:file:/path/to/my.jar!/dir1/file1").toURI().resolve("..");

throws an exception:

java.lang.IllegalArgumentException: URI is not absolute
	at java.base/java.net.URL.fromURI(URL.java:719)
	at java.base/java.net.URI.toURL(URI.java:1139)

Maybe there is a workaround. I've tried to Google but I was not able to find anything.

This adds a 498 KB binary file. Would it be possible to produce this .jar file as part of the build instead?

Done.

Finally, regarding the implementation, I noticed you're converting a lot between URL and URI. Would it make sense to just drop the usage of URL and use URI everywhere?

Yes, I thought about it as well but decided against it. Main reasons:

• according to https://danielmiessler.com/study/difference-between-uri-url/ we are working with URLs
• URI does not have openStream() method -> I would have to convert to URL before each call
• getResource() method returns URL so it makes for nicer API

Therefore, I would not be able to avoid the conversion between the 2 either way.

I was thinking about adding some more checks to the URLHelper methods. For example a check that resolve would not be able to go out of jar file -> navigate only path after !. What do you think?

[bjorn]

Maybe there is a workaround. I've tried to Google but I was not able to find anything.

Judging by the string representation, I would expect a possible workaround to be to strip the part until the "!" from the string before turning it into a URI, then to resolve and then adding back the stripped part before turning it into a URL. It's a bit involved, but when it works then it would require less custom code.

Thanks for producing the resources.jar at build time!

Therefore, I would not be able to avoid the conversion between the 2 either way.

Indeed, seems like it make sense just to convert back and forth to be able to use the URI.resolve method then.

I was thinking about adding some more checks to the URLHelper methods. For example a check that resolve would not be able to go out of jar file -> navigate only path after !. What do you think?

Probably a good idea, but if we go with the method of stripping and then adding back jar:...! then it can't actually go outside of the jar file, right? I've played around a bit with this possibility:

import java.net.URL;
import java.net.URI;
import java.net.MalformedURLException;
import java.net.URISyntaxException;

public class YourClassNameHere
{
    public static URL resolve(final URL url, final String path)
        throws URISyntaxException, MalformedURLException
    {
        if ("jar".equals(url.getProtocol())) {
            final String urlStr = url.toString();
            final int indexBeyondBang = urlStr.indexOf("!") + 1;
            
            final URI withinJarURI = new URI(urlStr.substring(indexBeyondBang));
            final String withinJarResolvedStr = withinJarURI.resolve(path).toString();
            return new URL(urlStr.substring(0, indexBeyondBang) + withinJarResolvedStr);
        }
        
        return url.toURI().resolve(path).toURL();
    }
    
    public static URL getDir(final URL url)
        throws URISyntaxException, MalformedURLException
    {
        return resolve(url, ".");
    }
    
    public static void main(String[] args)
        throws URISyntaxException, MalformedURLException
    {
        System.out.println(getDir(new URL("file:/path/to/dir1/file1")));
        System.out.println(getDir(new URL("jar:file:/path/to/my.jar!/dir1/file1")));
    }
}

I haven't coded Java in a long time (used https://www.tutorialspoint.com/compile_java_online.php to get this working), so I think it's better if you adjust your solution to the final version.

[ahornace]

Probably a good idea, but if we go with the method of stripping and then adding back jar:...! then it can't actually go outside of the jar file, right? I've played around a bit with this possibility

Great suggestion, I don't know why I thought that I would need to provide the protocol and then again strip it.

[bjorn]

This looks fine to me now. Shall I squash-merge it?

[ahornace]

This looks fine to me now. Shall I squash-merge it?

Please, definitely do a squash. If you think it's okay, then we can merge and improve upon it when issues arise.

[bjorn]

Hmm, first this test failure on AppVeyor will need to be looked into:

[INFO] Running org.mapeditor.io.MapReaderTest
[ERROR] Tests run: 23, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 2.844 s <<< FAILURE! - in org.mapeditor.io.MapReaderTest
[ERROR] testReadingFlippedTilesJar(org.mapeditor.io.MapReaderTest)  Time elapsed: 0.078 s  <<< ERROR!
java.lang.IllegalArgumentException: Illegal character in path at index 2: ..\desert\desert.tsx
	at org.mapeditor.io.MapReaderTest.testReadingFlippedTilesJar(MapReaderTest.java:341)
Caused by: java.net.URISyntaxException: Illegal character in path at index 2: ..\desert\desert.tsx
	at org.mapeditor.io.MapReaderTest.testReadingFlippedTilesJar(MapReaderTest.java:341)

Edit: Ah, I see you've already pushed a fix for that!

[bjorn]

Thank you so much for your this!