Skip to content
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

Jetty 12 FileID no longer recognizes "jar" extension as isWebArchiveFile #12889

Open
gonphsn opened this issue Mar 7, 2025 · 19 comments
Open
Labels
behavior-change Indicates that a change in behavior was introduced Enhancement

Comments

@gonphsn
Copy link

gonphsn commented Mar 7, 2025

Jetty version(s)
Compare Jetty 9/10/11 to Jetty 12.0.16

Jetty Environment
jetty-core

Java version/vendor (use: java -version)
openjdk version "21.0.6" 2025-01-21 LTS
OpenJDK Runtime Environment Zulu21.40+18-SA (build 21.0.6+7-LTS)
OpenJDK 64-Bit Server VM Zulu21.40+18-SA (build 21.0.6+7-LTS, mixed mode, sharing)

OS type/version
Windows, but behavior would be OS agnostic

Description
The Jetty 12 FIleID utility class no longer recognizes "jar" as a succesful case of isWebArchiveFile().

See Jetty 9/10/11:

    /**
     * Is the path a Web Archive File (not directory)
     *
     * @param path the path to test.
     * @return True if a .war or .jar file.
     * @see FileID#isWebArchive(File)
     */
    public static boolean isWebArchiveFile(File path)
    {
        if (!path.isFile())
        {
            return false;
        }

        String name = path.getName().toLowerCase(Locale.ENGLISH);
        return (name.endsWith(".war") || name.endsWith(".jar"));
    }    

See Jetty 12:

    /**
     * Is the filename a WAR file.
     *
     * @param filename the filename to test.
     * @return True if a war file.
     */
    public static boolean isWebArchive(String filename)
    {
        return isExtension(filename, "war");
    }

In practice this means I can no longer use ContextProvider (previously WebAppProvider) as a way to deploy webapp enabled jars:

            // Otherwise it must be a directory or an archive
            else if (!Files.isDirectory(path) && !FileID.isWebArchive(path))
            {
                throw new IllegalStateException("unable to create ContextHandler for " + app);
            }

I empathize with the new implementation of FileID to avoid conflating "jar" and "war" extensions; however, would it not be sensible to then modify the ContextProvider.java to:

            // Otherwise it must be a directory or an archive
            else if (!Files.isDirectory(path) && !FileID.isWebArchive(path) && !FileID.isJavaArchive(path))
            {
                throw new IllegalStateException("unable to create ContextHandler for " + app);
            }

To carry forward the Jetty 9/10/11 deployment capabilities of ContextProvider?

How to reproduce?

Call FileID.isWebArchive("foo.jar"), observe false return value. Or rather, observe that you cannot use "jar" files to deploy webapps via ContextProvider in the same manner as they were with WebAppProvider.

@gonphsn gonphsn added the Bug For general bugs on Jetty side label Mar 7, 2025
@joakime
Copy link
Contributor

joakime commented Mar 7, 2025

Call FileID.isWebArchive("foo.jar"), observe false return value. Or rather, observe that you cannot use "jar" files to deploy webapps via ContextProvider in the same manner as they were with WebAppProvider.

For the record, the deployment process is being rewritten in 12.1.x to fix various things.

See

The ContextProvider and WebAppProvider don't actually exist anymore, starting in 12.1.0

@joakime
Copy link
Contributor

joakime commented Mar 7, 2025

The support for JAR as a webarchive was deprecate the moment Java introduced Multi-Release JAR file format.
The WAR format does not, and cannot, support that, so the support for JAR as a WAR was dropped.

The replacement deployer in Jetty 12.1.0 already address your other concerns.

@joakime
Copy link
Contributor

joakime commented Mar 10, 2025

Closing. Not going to be fixed by adding jar back to FileID.isWebArchive methods in Jetty 12.0.x

There are other FileID methods present that better identify the role of the file (eg: isLibArchive, isJavaArchive, isArchive, etc)
The move of FileID from jetty-deploy to jetty-util also makes this request not sane for other areas that FileID is now used in.

As for the behavior in Deploy, please wait for Jetty 12.1.0 and test again, if you still have a feature request, file it against that codebase.

@joakime joakime closed this as completed Mar 10, 2025
@gregw
Copy link
Contributor

gregw commented Mar 10, 2025

@joakime I'm just going to reopen this for a little bit to find out exactly what the use-case is.
Also, exactly why can't a war file be a multi-release jar?

@gonphsn Can you detail your use-case a bit more. What exactly do you mean by "webapp enabled jars"? how are they produced and why are they not war files?

@gregw gregw reopened this Mar 10, 2025
@gonphsn
Copy link
Author

gonphsn commented Mar 10, 2025

@gonphsn Can you detail your use-case a bit more. What exactly do you mean by "webapp enabled jars"? how are they produced and why are they not war files?

Our jar files are built to contain the WEB-INF directory at the top level:

Image

These jar files were utilized by WebAppProvider to build the respective servlet that the jar provides when installed. They are not *war files because Jetty WebAppProvider facilitated using *.jar as the source file, and not exclusively *.war files. It worked, so we used it. Jars could be used in 9/10/11, and now they have been (purposefully) disabled in 12. We are prepared to wait for 12.1 for the more orthodox solution, but our current model of deployment will be dependent on Jetty 12.1 providing a way to use jar files as the source of the WEB-INF directory.

@joakime
Copy link
Contributor

joakime commented Mar 10, 2025

A WEB-INF should either be in a WAR file, or a web-fragment.
Using a WEB-INF from other sources shouldn't be looked at, that behavior is a red-flag to me, smells like a bug, and a servlet spec violation.

@janbartel
Copy link
Contributor

A WEB-INF should either be in a WAR file, or a web-fragment. Using a WEB-INF from other sources shouldn't be looked at, that behavior is a red-flag to me, smells like a bug, and a servlet spec violation.

Actually, in osgi we do deploy .jar files that contain a webapp. The naming convention is xxx-webbundle.jar. The osgi deployer looks at the .jar file to see if there's a WEB-INF, and if so, deploys it as a webapp. Bundles never have .war extensions in osgi.

@joakime
Copy link
Contributor

joakime commented Mar 10, 2025

A WEB-INF should either be in a WAR file, or a web-fragment. Using a WEB-INF from other sources shouldn't be looked at, that behavior is a red-flag to me, smells like a bug, and a servlet spec violation.

Actually, in osgi we do deploy .jar files that contain a webapp. The naming convention is xxx-webbundle.jar. The osgi deployer looks at the .jar file to see if there's a WEB-INF, and if so, deploys it as a webapp. Bundles never have .war extensions in osgi.

This issue was opened against the local filesystem scanner deployer, not osgi.

The osgi layers don't even use this local filesystem scanner deployer.

Also, the osgi layers in 12.1.0 don't have issue with FileID.isWebArchive excluding jar.

@gregw
Copy link
Contributor

gregw commented Mar 14, 2025

As 9/10/11 have accepted jar files as deployable files without any reported problems, then I'm inclined to consider supporting this. I.e. a jar file that does contain a WEB-INF should be OK to deploy, but one that doesn't should not be deployed (unlike a war file without a WEB-INF).

I.e the criteria to deploy an archive should be : named *.war OR contains WEB-INF.

@gregw gregw added Enhancement behavior-change Indicates that a change in behavior was introduced and removed Bug For general bugs on Jetty side labels Mar 14, 2025
@joakime
Copy link
Contributor

joakime commented Mar 14, 2025

@gregw that means cracking open and investigating every jar we see for that WEB-INF directory during deployment scanning, right?
Or are you saying it should fail AT deployment creation of the ContextHandler if it fails to have a WEB-INF?

I was going to say that an exploded JAR would need special attention, but that isn't really the case, a directory that came from an extracted JAR or WAR would look the same.

@gregw
Copy link
Contributor

gregw commented Mar 14, 2025

@gregw that means cracking open and investigating every jar we see for that WEB-INF directory during deployment scanning, right?

We'd only have to crack open any jar files that are dropped in the top level of webapps, which will more often than no be zero such files.

Or are you saying it should fail AT deployment creation of the ContextHandler if it fails to have a WEB-INF?

Yes - that would do it. Let the jar files be passed all the way in, then when creating the ContextHandler (when we crack open the archive anyway), if there is neither a war extension nor a WEB-INF then we throw an exception.

I was going to say that an exploded JAR would need special attention, but that isn't really the case, a directory that came from an extracted JAR or WAR would look the same.

We'd need to check as we exploded. If the user explodes, then OK

@joakime
Copy link
Contributor

joakime commented Mar 14, 2025

We check if it is a *.war file in many other places, not just the deploy scanner.
Some of the webapp Configuration implementations do the same check.
Example, the WebInfConfiguration will behave poorly if the foo.jar and foo/ (dir) both exist. (i'm sure there are more)

@joakime
Copy link
Contributor

joakime commented Mar 14, 2025

Basically, if we intend to support this, then the change will be in many more places than just the deploy scanner.

@gregw
Copy link
Contributor

gregw commented Mar 14, 2025

@joakime but we have supported it in the past, so I'm assuming most code will be OK with it. I.e. unless we explicitly changed it in those many places when we removed it from the supported paths check, then we should be fine. It's an easy thing to test.

@joakime
Copy link
Contributor

joakime commented Mar 14, 2025

Just tested this on Jetty 10.0.24, you cannot have a JAR in ${jetty.base}/webapps/ be recognized as a deployable artifact, nothing happens.

Looking at the WebAppProvider back then, the FileFilter for the Scanner has doesn't support JAR, so there wouldn't be a fileAdded/fileChanged/fileRemoved event for it.

Link to WebAppProvider.FileFilter in 10.0.24
https://github.com/jetty/jetty.project/blob/jetty-10.0.24/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/WebAppProvider.java#L74

The statement from the OP that it works in Jetty 10/11 isn't accurate.

@gonphsn
Copy link
Author

gonphsn commented Mar 14, 2025

Just tested this on Jetty 10.0.24, you cannot have a JAR in ${jetty.base}/webapps/ be recognized as a deployable artifact, nothing happens.

Looking at the WebAppProvider back then, the FileFilter for the Scanner has doesn't support JAR, so there wouldn't be a fileAdded/fileChanged/fileRemoved event for it.

Link to WebAppProvider.FileFilter in 10.0.24 https://github.com/jetty/jetty.project/blob/jetty-10.0.24/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/WebAppProvider.java#L74

The statement from the OP that it works in Jetty 10/11 isn't accurate.

The code we use in 9/10/11 is:

    DeploymentManager deployer = new DeploymentManager();
    WebAppProvider provider = new WebAppProvider();

    // Iterate through all modules that are Web Archives and load them into Jetty.
    Arrays.stream((ModuleInfo[])Sys.getRegistry().getModules())
      .forEach(info ->
      {
        SecurityUtil.doPrivileged((PrivilegedAction<Void>)() ->
        {
          try
          {
            // Create the Handler (a.k.a the WebAppContext).
            App app = new App(deployer, provider, module.getFile().getAbsolutePath());
            WebAppContext context = (WebAppContext)app.getContextHandler();

This functions to create a WebAppContext because getConextHandler considers jar files as satisfying the "isWebArchiveFile":

        // Otherwise it must be a directory or an archive
        else if (!file.isDirectory() && !FileID.isWebArchiveFile(file))
        {
            throw new IllegalStateException("unable to create ContextHandler for " + app);
        }

The parallel effort in Jetty 12 with WebAppProvider -> ContextProvider:

    DeploymentManager deployer = new DeploymentManager();
    ContextProvider provider = new ContextProvider();

    // Iterate through all modules that are Web Archives and load them into Jetty.
    Arrays.stream((ModuleInfo[])Sys.getRegistry().getModules())
      .forEach(info ->
      {
        SecurityUtil.doPrivileged((PrivilegedAction<Void>)() ->
        {
          try
          {
            // Create the Handler (a.k.a the WebAppContext).
            App app = new App(deployer, provider, module.getFile().toPath());
            WebAppContext context = (WebAppContext)app.getContextHandler();

no longer functions because the Jetty 12 ContextProvider continues to use:

            // Otherwise it must be a directory or an archive
            else if (!Files.isDirectory(path) && !FileID.isWebArchive(path))
            {
                throw new IllegalStateException("unable to create ContextHandler for " + app);
            }

Meaning that our workflow permitted the creation of a context handler with jar files in 9/10/11 and now it no longer functions in 12 because ContextProvider no longer considers a "jar" file as a candidate.

I could re-compile and test, but my initial impression was yes, it makes sense to update the definition of isWebArchive to exclusively apply to "war" files; however, if that is the case, is it reasonable that Jetty 12 ContextProvider is modified to:

            // Otherwise it must be a directory or an archive
            else if (!Files.isDirectory(path) && !FileID.isWebArchive(path) && !FileID.isJavaArchive(path))
            {
                throw new IllegalStateException("unable to create ContextHandler for " + app);
            }

This is almost certainly moot if you say that you will be completely overhauling the ContextProvider in 12.1 (which we will absolutely adopt), but just trying to raise awareness that:

  1. This is a workflow that we use
  2. This workflow worked in 9/10/11 but has no parallel in 12
  3. I did not see anything about this in the migration documents

We will have to significantly restructure our servlet deployment if this is something that can't be supported, but my impression of the code is that this was not explicitly prevented in any way by how 9/10/11 ContextProvider was implemented. I just got the impression that isWebArchive was changed (which I believe makes total sense) but that then ContextProvider was not updated to reflect that it needed an extra conditional.

I think our use case was abnormal and something that fell through the cracks, but it definitely does function - even perhaps unintentionally - for 9/10/11 just fine.

@joakime
Copy link
Contributor

joakime commented Mar 14, 2025

You are not using the WebAppProvider in the code examples you provided, nor are you seemingly using the DeploymentManager.

The behavior you have is more akin to a custom AppProvider, one that relies on the ContextHandler creation, but not the scanner from the WebAppProvider.

When PR #12583 merges for Jetty 12.1.x then you have other options to make it work.
And the integration is far easier, as the scanner and the ContextHandler creation are separated there.

@joakime
Copy link
Contributor

joakime commented Mar 19, 2025

The Jetty 12.1.0.alpha2 release is now available on Maven Central.

Your code would look something like this with that version of Jetty ...

Deployer deployer = server.getBean(Deployer.class);

Environment environment = Environment.ensure("ee11", WebAppContext.class);

ContextHandlerFactory contextHandlerFactory = new StandardContextHandlerFactory();

// Iterate through all modules that are Web Archives and load them into Jetty.
Arrays.stream((ModuleInfo[])Sys.getRegistry().getModules())
    .forEach(info ->
    {
        SecurityUtil.doPrivileged((PrivilegedAction<Void>)() ->
        {
            try
            {
                ContextHandler contextHandler = contextHandlerFactory.newContextHandler(
                    server,
                    environment,
                    module.getFile().toPath()
                    module.getOtherFilePaths()
                    attributes);
                deployer.deploy(contextHandler);

The module.getFile().toPath() is the main deployable path. (XML, or WAR, or Directory)
Where module.getOtherFilePaths() is the other files that represent the deployment (eg: the WAR, the Directory, properties files, other environment XMLs, etc)

The attributes is the attributes you want to use for that context. (cannot be null, must declare CONTEXT_HANDLER_CLASS_DEFAULT_ATTRIBUTE and ENVIRONMENT_ATTRIBUTE at least)
See: https://github.com/jetty/jetty.project/blob/jetty-12.1.x/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/ContextHandlerFactory.java

You can optionally make your entire Arrays.forEach a Jetty bean and have it be part of the lifecycle of Jetty as well.

@gonphsn
Copy link
Author

gonphsn commented Mar 21, 2025

Ok, we will re-evaluate against the "jetty-12.1.x" artifacts and re-submit under that context if necessary.

For posterity, the exception we see at present in 12.0 is:

DEBUG [10:39:50 21-Mar-25 EDT][org.eclipse.jetty.deploy.providers.ContextProvider] createContextHandler App@47a782a1[ee10,null,D:\acme\modules\module.jar] in oejuc.Environment$Named@0{ee10}
DEBUG [10:39:50 21-Mar-25 EDT][org.eclipse.jetty.deploy.providers.ContextProvider] Environment property files []
WARNING [10:39:50 21-Mar-25 EDT][web] Could not load Web App for: ModuleInfo (module)
java.lang.IllegalStateException: unable to create ContextHandler for App@47a782a1[ee10,null,D:\acme\modules\module.jar]
        at org.eclipse.jetty.deploy.providers.ContextProvider.createContextHandler(ContextProvider.java:472)
        at org.eclipse.jetty.deploy.App.getContextHandler(App.java:108)
        at com.acme.jetty.WebServer.lambda$addModuleWebAppHandlers$18(WebServer.java:1778)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(Direct5MethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at javax.baja.nre.util.SecurityUtil.lambda$doPrivileged$0(SecurityUtil.java:425)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:319)
        at javax.baja.nre.util.SecurityUtil.doPrivileged(SecurityUtil.java:420)
        at com.acme.jetty.WebServer.lambda$addModuleWebAppHandlers$19(WebServer.java:1769)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
        at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
        at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:1024)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
        
        ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior-change Indicates that a change in behavior was introduced Enhancement
Projects
Status: No status
Development

No branches or pull requests

4 participants