Skip to content

Issue #12990 - Introduce static-deploy module #12998

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 34 commits into from
Apr 30, 2025

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Apr 11, 2025

  • And distribution tests

Fixes #12990

@joakime joakime added the Bug For general bugs on Jetty side label Apr 11, 2025
@joakime joakime requested a review from sbordet April 11, 2025 17:24
@joakime joakime self-assigned this Apr 11, 2025
@joakime joakime moved this to 👀 In review in Jetty 12.1.0 Apr 11, 2025
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've gone wrong somewhere with "foo.d" directories. These should never EVER be deployed by the deployer. They should be invisible to the scanner/deployer and are used to hold files for a app at "/foo/" or "/foo.war".

I think the issue here is that a "/foo/" directory should be deployed to deploy static resources if:

  • The default env is an EEn env and then it is just a servlet application with no servlets and just a default servlet.
  • If the default env is "core" then a resource handler should be created.
  • If there is a foo.properties file that sets the env to core, then it should be deployed with a resource handler.

@joakime joakime changed the title Issue #12990 - Restore static directory deployment for core-deploy Issue #12990 - Introduce static-deploy module Apr 15, 2025
@joakime joakime requested a review from gregw April 15, 2025 20:22
@joakime joakime requested a review from gregw April 18, 2025 15:09
@sbordet
Copy link
Contributor

sbordet commented Apr 23, 2025

@gregw @joakime so current status is that environment core is always enabled, so a static only deployment (only with static-deploy) does not work because it assumes the environment is core.

Caused by: java.lang.IllegalStateException: unable to create ContextHandler for /tmp/jetty.base/webapps/static-app
	at org.eclipse.jetty.deploy.StandardContextHandlerFactory.newContextHandler(StandardContextHandlerFactory.java:118)

A static deployment would therefore always require a properties file to specify the environment, which seems an exception to the rule that if we enable just one deployer, then it is that environment by default.

ClassLoaderDump also confuses matters because it uses the word core in its dump, but it's not the environment, it's something else 😄

Perhaps the right solution is to not have any environment setup by default -- they are only enabled if there is the correspondent module.

Thoughts?

@joakime
Copy link
Contributor Author

joakime commented Apr 23, 2025

Perhaps the right solution is to not have any environment setup by default -- they are only enabled if there is the correspondent module.

The start.jar always sets up the core environment via the XmlConfiguration command line arguments (you can see this in start.jar --dry-run).
Not sure we can eliminate the core environment from existing, esp in the jetty-home/jetty-base scenario.
That being said ... in embedded mode it's easy to not have a core environment.

@joakime
Copy link
Contributor Author

joakime commented Apr 25, 2025

@gregw a rereview on this one please.

@@ -127,12 +130,40 @@ public class DeploymentScanner extends ContainerLifeCycle implements Scanner.Bul
// old attributes prefix, now stripped.
private static final String ATTRIBUTE_PREFIX = "jetty.deploy.attribute.";

private static final Pattern EE_ENVIRONMENT_NAME_PATTERN = Pattern.compile("ee(\\d+)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we use "ee", but just in case, perhaps this should be

Suggested change
private static final Pattern EE_ENVIRONMENT_NAME_PATTERN = Pattern.compile("ee(\\d+)");
private static final Pattern EE_ENVIRONMENT_NAME_PATTERN = Pattern.compile("ee(\\d+)", Pattern.CASE_INSENSITIVE);

* EE names are compared by EE number, otherwise simple name comparison is used.
*/
protected static final Comparator<String> ENVIRONMENT_COMPARATOR = (e1, e2) ->
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparator will generate a lot of matchers, so perhaps take the returns where you can:

Suggested change
{
{
if (Objects.equals(e1, e2))
return 0;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better yet, why not just create an Index that is case insensitive and maps all the known environment names to an integer.

    Index<Integer> environmentOrdinals = new Index.Builder<Integer>()
        .caseSensitive(false)
        .with("static", 1)
        .with("core", 2)
        .with("ee9", 9)
        .with("ee10", 10)
        .with("ee11", 11)
        .build();

Copy link
Contributor Author

@joakime joakime Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a conversation with @sbordet and @lorban this will be fixed in a followup PR.

I have an approach that will push the weight of the tracked environments (the ones allowed to be deployed to) via a change to the DeploymentScanner.configureEnvironment(String name, int weight), putting the weights into the XMLs instead.

That way the weights are not hardcoded, and can be adjusted by any user, and even add new environments (with their own weights) without the need to modify code.

joakime and others added 6 commits April 28, 2025 11:00
+ `baseResource` to `jetty.deploy.baseResource`
+ `jetty.static.dirAllowed` to `jetty.deploy.baseResource.dirAllowed`
…tory'.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
…ectory' into fix/12.1.x/coredeploy-static-directory
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to start the followup PR

@joakime joakime merged commit 4809364 into jetty-12.1.x Apr 30, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Jetty 12.1.0 Apr 30, 2025
@joakime joakime deleted the fix/12.1.x/coredeploy-static-directory branch April 30, 2025 21:27
@olamy olamy mentioned this pull request Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Enhancement
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Jetty CoreContextHandler needs to support simple static directory deployment
4 participants