Overuse of FileID.isArchive() and inability to deal with packed jars without .jar extension

[janbartel]

jetty-12.0.x

    default Resource newJarFileResource(URI uri)
    {
        if (!FileID.isArchive(uri))
            throw new IllegalArgumentException("Path is not a Java Archive: " + uri);
        if (!uri.getScheme().equalsIgnoreCase("file"))
            throw new IllegalArgumentException("Not an allowed path: " + uri);
        return newResource(URIUtil.toJarFileUri(uri));
    }

Line 240 of ResourceFactory we test if the uri has an appropriate file extension with FileID.isArchive.
Then at line 244 we call URIUtil.toJarFileUri, which also tests if the uri has an appropriate extension:

    public static URI toJarFileUri(URI uri)
    {
        Objects.requireNonNull(uri, "URI");
        String scheme = Objects.requireNonNull(uri.getScheme(), "URI scheme");

        if (!FileID.isArchive(uri))
            return uri;

        boolean hasInternalReference = uri.getRawSchemeSpecificPart().indexOf("!/") > 0;

        if (scheme.equalsIgnoreCase("jar"))
        {
            if (uri.getRawSchemeSpecificPart().startsWith("file:"))
            {
                // Looking good as a jar:file: URI
                if (hasInternalReference)
                    return uri; // is all good, no changes needed.
                else
                    // add the internal reference indicator to the root of the archive
                    return URI.create(uri.toASCIIString() + "!/");
            }
        }
        else if (scheme.equalsIgnoreCase("file"))
        {
            String rawUri = uri.toASCIIString();
            if (hasInternalReference)
                return URI.create("jar:" + rawUri);
            else
                return URI.create("jar:" + rawUri + "!/");
        }

        // shouldn't be possible to reach this point
        throw new IllegalArgumentException("Cannot make %s into `jar:file:` URI".formatted(uri));
    }

We don't need to do the test multiple times.

Moreover, the FileID.isArchive method does not work with files that are actually packed jar format, but missing the .jar extension, for example as seen with Equinox in the osgi environment. I tried modifying the method to also try the Files.probeContentType method, but it failed to detect a filetype for these types of files. Do we really need to check the file extension? If you make a uri into a jar:file uri and its the wrong type of file, then using it will fail. Same thing for the JarFileResource.

[gregw]

I'm conflicted about how to fix this one....

If Files.probeContentType actually worked like the unix file command, then part of me thinks we should just use that in isArchive, but then the other part of me thinks that is an expensive test to call several times as we process the same resource. So do we really need to check if a resource is an archive before mounting it? If it is not, then the mount will fail anyway.

But the probe doesn't work anyway, so going for an expensive isArchive and calling it less often is not going to be a solution.

Maybe we rename it to isArchiveExtension to be clear it is not actually looking at the magic number in the file? We then don't need to call it in the mount and other archive related methods, because they can be applied to archives that don't have the extension. It is up to the caller to get the logic right. In this case if it passed as a war file, but it is not a directory, then it is worthwhile trying it as an archive regardless of the extension.

[joakime]

This is Jetty 12.

Packed jars, aka Pack200, is unsupported by the JVM.

Pack200 was removed in JDK 14 - https://bugs.openjdk.org/browse/JDK-8232022

Also, Eclipse Equinox no longer downloads / uses *.pack.gz on Java 14 or newer.
https://bugs.eclipse.org/bugs/show_bug.cgi?id=536106

[gregw]

@joakime not sure how your comment is related? This issue is that we have a war file that we need to deploy, that just happens to not have a ".war" nor ".jar" extension. We could deploy that in jetty<=11, because we would try any non-directory file as a jar:file resource. This now fails in jetty-12 because of all the new FileID.isArchive() checks. So we need to either make isArchive correctly test for archives (with more than just filename extension... and t hen I worry about the cost of it), or we have to stop calling from within the resource factories. If we are asked to try a resource as a jar:file, then we should try regardless of the extension. We can still use a FileID.isArchiveExtension(String) method to help us decide if we want to try or not, but unless isArchive really works, we need to stop calling it from within the factories.

[joakime]

From the original comment ...

Moreover, the FileID.isArchive method does not work with files that are actually packed jar format

That, "packed jar format" (aka Pack200) is not supported anymore in JDK 17.

Supporting WAR files is a useful thing for sure.
isArchive() used to contain war, but that lead to war files being added to the URLClassLoader.

[joakime]

Lets strip isArchive() from the ResourceFactory implementations and see what happens.

[joakime]

Opened PR #9002

[janbartel]

Sorry if my phraseology confused you. I did not mean pack200, just an assembled jar that does not have a .jar extension.

From the original comment ...

Moreover, the FileID.isArchive method does not work with files that are actually packed jar format

That, "packed jar format" (aka Pack200) is not supported anymore in JDK 17.

Supporting WAR files is a useful thing for sure. isArchive() used to contain war, but that lead to war files being added to the URLClassLoader.

[janbartel]

Fixed.