Skip to content

Fix zip parsing and add zipfile parsing test #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 10, 2025

Conversation

alanocallaghan
Copy link
Contributor

The javadocs claim we can parse models without unzipping, but as far as I can tell this is not the case (hence the broken unit test).

When parsing a zip, we try to resolve model names with respect to the root directory of the model zip. However for me this returns "/", so we're looking for files in the filesystem root and this will fail unless somebody's done something really strange.

This PR aims to fix parsing from zipped files without unzipping.

@alanocallaghan
Copy link
Contributor Author

Relatedly, it'd be good to have better reporting from the unit tests here... Not obvious what's failing

@petebankhead
Copy link
Member

Not entirely sure I follow, but there is some weirdness around how zip files are handled generally - because there is a Zip File System Provider I'm less sure that looking relative to "/" is necessarily wrong.

Adding to the fun, since the modularisation of Java you'll need to ensure that jdk.zipfs is available - otherwise it might all compile fine, but fail at the end. I vaguely recall that causing problems with tests or distributions in the past.

@alanocallaghan
Copy link
Contributor Author

alanocallaghan commented Nov 20, 2024

I'm less sure that looking relative to "/" is necessarily wrong.

In theory, if there was a way to pass the filesystem to Files.exists, but otherwise we're just ignoring where the zip is and checking the system/user's filesystem root. The paths returned from fs.getRootDirectories should include the zip path since that's where we're likely to find stuff within the zip.

It's probably that there wrong type of filesystem is being create here:

var fs = FileSystems.newFileSystem(path, (ClassLoader)null);
for (var dir : fs.getRootDirectories()) {
for (var name : MODEL_NAMES) {
var p = dir.resolve(name);
if (Files.exists(p))
return p;
}
}
}

but convincing Filesystems.newFileSystem to make a ZipFileSystem seems to be another story entirely.

@alanocallaghan
Copy link
Contributor Author

alanocallaghan commented Nov 20, 2024

Actually, I spoke too soon. It is a zip file system, but we're not resolving paths within the zip.

eg, changing the above quoted block to this:

			// Check zip file
			var fs = FileSystems.newFileSystem(path, null);
			System.out.println(fs.getClass());
			for (var dir : fs.getRootDirectories()) {
				for (var name : MODEL_NAMES) {
					var p = dir.resolve(name);
					System.out.println(p);
					if (Files.exists(p))
						return p;
				}
			}

I get:

class jdk.nio.zipfs.ZipFileSystem
/model.yaml
/model.yml
/rdf.yaml
/rdf.yml

because dir is /, so Files.exists(p) fails for all of these because I don't have any of these files in /.

This stackoverflow post suggests exactly this method, so it's not clear to me if/when something changed here.

@alanocallaghan alanocallaghan changed the title Fix parsing from zip files Add zipfile parsing test Nov 20, 2024
@alanocallaghan
Copy link
Contributor Author

My mistake, the code was right, my zip files were wrong. Could merge this to test zip parsing, or not, doesn't matter much

@alanocallaghan
Copy link
Contributor Author

Going to can this, might be useful to automatically generate a zipfile then parse it but there's little utility in this

@alanocallaghan
Copy link
Contributor Author

I've broken zip parsing somehow which I guess demonstrates the utility of this PR

@alanocallaghan alanocallaghan changed the title Add zipfile parsing test Fix zip parsing and add zipfile parsing test Feb 10, 2025
@alanocallaghan alanocallaghan marked this pull request as ready for review February 10, 2025 13:23
@alanocallaghan alanocallaghan merged commit def809e into qupath:main Feb 10, 2025
1 check passed
@alanocallaghan alanocallaghan deleted the test-zip branch February 10, 2025 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants