Issue #11514 - Cleanup jetty.webapp.addServerClasses property behavior for ee10/ee9/ee8
#11516
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -486,7 +486,7 @@ public void postConfigure() throws Exception | |
| protected void doStart() throws Exception | ||
| { | ||
| ClassLoader old = Thread.currentThread().getContextClassLoader(); | ||
| Thread.currentThread().setContextClassLoader(ServletContextHandler.ENVIRONMENT.getClassLoader()); | ||
| try | ||
| { | ||
| _metadata.setAllowDuplicateFragmentNames(isAllowDuplicateFragmentNames()); | ||
|
|
@@ -728,20 +728,20 @@ public void setServer(Server server) | |
| { | ||
| if (__dftSystemClasses.equals(_systemClasses)) | ||
| { | ||
| Object systemClasses = ServletContextHandler.ENVIRONMENT.getAttribute(SERVER_SYS_CLASSES); | ||
| if (systemClasses instanceof String[] patterns) | ||
| _systemClasses.add(patterns); | ||
| if (systemClasses instanceof ClassMatcher classMatcher) | ||
| _systemClasses.add(classMatcher.getPatterns()); | ||
| } | ||
|
|
||
| if (__dftServerClasses.equals(_serverClasses)) | ||
| { | ||
| Object serverClasses = ServletContextHandler.ENVIRONMENT.getAttribute(SERVER_SRV_CLASSES); | ||
| if (serverClasses instanceof String[] patterns) | ||
| _serverClasses.add(patterns); | ||
| if (serverClasses instanceof ClassMatcher classMatcher) | ||
| _serverClasses.add(classMatcher.getPatterns()); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1383,24 +1383,62 @@ public MetaData getMetaData() | |
| return _metadata; | ||
| } | ||
|
|
||
| /** | ||
| * @param server ignored. | ||
| * @param patterns the patterns to add | ||
| * @deprecated use {@link #addServerClasses(String...)} instead, will be removed in Jetty 12.1.0 | ||
|
There was a problem hiding this comment. Remove when it will be removed. There was a problem hiding this comment. The method that doesn't take a server does a different job. That sets the default for all servers rather than this method, which sets the default for a specific server. What is the replacement for setting the default for a specific server? That should be linked here. There was a problem hiding this comment. That's at the heart of what caused the issue. The Now with this PR the Do we have something like a jetty-core level How about in a followup PR I move the 2 There was a problem hiding this comment. @joakime note that the server classes are set on a static constant There was a problem hiding this comment. The ServletContextHandler.ENVIRONMENT (that exists in ee10, ee9, ee8) are JVM statics ATM (in HEAD). |
||
| */ | ||
| @Deprecated(since = "12.0.8", forRemoval = true) | ||
| public static void addServerClasses(Server server, String... patterns) | ||
| { | ||
| addServerClasses(patterns); | ||
| } | ||
|
|
||
| /** | ||
| * Add a Server Class pattern to use for all ee10 WebAppContexts. | ||
| * @param patterns the patterns to use | ||
| * @see #getServerClassMatcher() | ||
| * @see #getServerClasses() | ||
| */ | ||
| public static void addServerClasses(String... patterns) | ||
| { | ||
| if (LOG.isDebugEnabled()) | ||
| LOG.debug("Adding {} ServerClasses: {}", ServletContextHandler.ENVIRONMENT, String.join(", ", patterns)); | ||
| addClasses(__dftServerClasses, SERVER_SRV_CLASSES, patterns); | ||
| } | ||
|
|
||
| /** | ||
| * @param server ignored. | ||
| * @param patterns the patterns to add | ||
| * @deprecated use {@link #addSystemClasses(String...)} instead, will be removed in Jetty 12.1.0 | ||
|
There was a problem hiding this comment. Remove when it will be removed. |
||
| */ | ||
| @Deprecated(since = "12.0.8", forRemoval = true) | ||
| public static void addSystemClasses(Server server, String... patterns) | ||
| { | ||
| addSystemClasses(patterns); | ||
| } | ||
|
|
||
| /** | ||
| * Add a System Class pattern to use for all ee10 WebAppContexts. | ||
| * @param patterns the patterns to use | ||
| * @see #getSystemClassMatcher() | ||
| * @see #getSystemClasses() | ||
| */ | ||
| public static void addSystemClasses(String... patterns) | ||
| { | ||
| if (LOG.isDebugEnabled()) | ||
| LOG.debug("Adding {} SystemClasses: {}", ServletContextHandler.ENVIRONMENT, String.join(", ", patterns)); | ||
| addClasses(__dftSystemClasses, SERVER_SYS_CLASSES, patterns); | ||
| } | ||
|
|
||
| private static void addClasses(ClassMatcher matcher, String attributeKey, String... pattern) | ||
| { | ||
| if (pattern == null || pattern.length == 0) | ||
| return; | ||
|
|
||
| // look for a ClassMatcher attribute with the list of Server / System classes | ||
| // to apply to every ee10 web application. If not present, use our defaults. | ||
| Object o = ServletContextHandler.ENVIRONMENT.getAttribute(attributeKey); | ||
| if (o instanceof ClassMatcher) | ||
| { | ||
| ((ClassMatcher)o).add(pattern); | ||
|
|
@@ -1415,6 +1453,6 @@ private static void addClasses(ClassMatcher matcher, String attribute, Server se | |
| int l = classes.length; | ||
| classes = Arrays.copyOf(classes, l + pattern.length); | ||
| System.arraycopy(pattern, 0, classes, l, pattern.length); | ||
| ServletContextHandler.ENVIRONMENT.setAttribute(attributeKey, classes); | ||
gregw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the part related to when it will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, this is super common in use on open source java projects (spring-boot, spring-framework, dropwizard, tomcat, etc) and benefits developers.
Our own codebase already has many such usages before this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joakime then we should remove them all.
Spring has a policy (2 minor versions).
We do not know if we ever will have a 12.1.0, maybe we jump to 13 immediately due to the move to Java 21 required by JEE 11.
Or maybe we will have a policy similar to Spring.
Or maybe we retain it for sponsored reasons.
So the comment about 12.1.0 is wrong, in all cases, and must be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence here. I like seeing the deprecated for removal. I also don't mind being given info that will let me know I can keep using it (e.g. knowing it will always be there in 12.0.x). But I also can see that naming a specific future version is not always correct (although we have frequently done it before and survived!).
So perhaps just say it is deprecated for removal "after 12.0.x", and that could be 12.1.0 or 13.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with the "must be removed" part of your reply.
but I'm totally on board with having a minimum 2 minor releases policy.
That would change these references to say "12.2.0" instead, which means even if 13.0.0 gets released, they are removed, as 13.0.0 is after 12.2.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joakime then the policy is outlined elsewhere, e.g. in the docs.
Leave the code clean, without comments that are going to rot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The policy on deprecated doesn't exist in any documentation in Jetty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let's add it, then. But it is orthogonal to this issue.