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.

Sign up for GitHub

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

Jump to bottom

Move ClassMatcher to core to have a consistent fix for addServerClasses in all environments #11566

Merged
merged 33 commits into from
Apr 12, 2024
Merged

Move ClassMatcher to core to have a consistent fix for addServerClasses in all environments #11566

merged 33 commits into from
Apr 12, 2024

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Mar 25, 2024

Updated to use a common ClassMatcher and to support customization of default, server, environment or context

@gregw gregw requested review from joakime and janbartel March 25, 2024 10:36
@gregw gregw changed the base branch from jetty-12.0.x to fix/12.0.x/addserverclasses-ee9 March 25, 2024 10:37
@gregw gregw changed the base branch from fix/12.0.x/addserverclasses-ee9 to jetty-12.0.x March 25, 2024 10:37
@gregw
Copy link
Contributor Author

gregw commented Mar 25, 2024

@joakime I was intending to make this a PR against your PR, but something is screwed up and 171 files are changed. This is based on your #11516 and is now an alternative.

@gregw gregw mentioned this pull request Mar 25, 2024
Closed
@joakime
Copy link
Contributor

joakime commented Mar 25, 2024

Ya know, this jetty-core/jetty-ee could just be called jetty-core/jetty-environment and even the Environment code (currently in util) could be in this common location too.

@gregw
System Server Classes
01f2903 
Renaming to Hidden/Protected
@gregw
Copy link
Contributor Author

gregw commented Mar 25, 2024

@joakime

Ya know, this jetty-core/jetty-ee could just be called jetty-core/jetty-environment and even the Environment code (currently in util) could be in this common location too.
Mmmmmmmaybe! We do have a core environment... but perhaps it too could optionally use a Classloader that uses hidden/protected classes.

How vital for 12.0.8 is this fix? Could we punt to 12.0.9 so we can get the naming and locations right.
Is there a minimal fix we can do for 12.0.8 (like removing the "ee9" from the attribute name)?

@joakime
Copy link
Contributor

joakime commented Mar 25, 2024

How vital for 12.0.8 is this fix? Could we punt to 12.0.9 so we can get the naming and locations right.
Is there a minimal fix we can do for 12.0.8 (like removing the "ee9" from the attribute name)?

Not vital, we can punt this to 12.0.9 easily.

@gregw
Merge branch 'jetty-12.0.x' into fix/12.0.x/addserverclasses-ee9-gw
0ddab40 
# Conflicts:
#	jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java
@gregw gregw requested review from janbartel and joakime March 26, 2024 17:22
@gregw gregw requested a review from janbartel April 4, 2024 08:46
@gregw
Copy link
Contributor Author

gregw commented Apr 4, 2024

@joakime does this PR work for your cross context dispatch testing?

@gregw
Copy link
Contributor Author

gregw commented Apr 4, 2024

@joakime @janbartel Can we get this reviewed and merged as a priority?

@joakime joakime changed the title Fix/12.0.x/addserverclasses ee9 gw Move ClassMatcher to core to have a consistent fix for addServerClasses in all environments Apr 4, 2024
joakime
joakime previously requested changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

A lot of signature changes (in methods and fields), new names partially introduced but not consistently across the use cases, deprecations on the wrong things (deprecating new signatures, but not keeping the old signatures and deprecating them)

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ClassVisibilityChecker.java Outdated Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ClassVisibilityChecker.java Outdated Show resolved Hide resolved
jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java Outdated Show resolved Hide resolved
jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java Outdated Show resolved Hide resolved
jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java Outdated Show resolved Hide resolved
jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/WebAppContext.java Outdated Show resolved Hide resolved
...y-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/AbstractConfiguration.java Outdated Show resolved Hide resolved
jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/Configuration.java Outdated
@@ -17,6 +17,7 @@
import java.util.Collections;
import java.util.ServiceLoader;

import org.eclipse.jetty.util.ClassMatcher;
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the signatures of methods using ClassMatcher as input or return.
Also, the naming change of server/system to hidden/protected hasn't been performed here yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to change ee9 naming more than necessary.

...e10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/AbstractConfiguration.java Outdated Show resolved Hide resolved
jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/Configuration.java Outdated Show resolved Hide resolved
@gregw gregw requested a review from joakime April 4, 2024 21:07
@gregw
Copy link
Contributor Author

gregw commented Apr 8, 2024

@janbartel @joakime nudge!

1 similar comment
@gregw
Copy link
Contributor Author

gregw commented Apr 10, 2024

@janbartel @joakime nudge!

@joakime
Copy link
Contributor

joakime commented Apr 10, 2024

@gregw It's looking good, I pushed 2 small commits to cleanup other things I noticed.

@gregw gregw merged commit 30bee71 into jetty-12.0.x Apr 12, 2024
10 checks passed
@gregw gregw deleted the fix/12.0.x/addserverclasses-ee9-gw branch April 12, 2024 07:31
@joakime joakime mentioned this pull request Apr 17, 2024
Closed
sbordet added a commit that referenced this pull request Apr 30, 2024
@sbordet
Addendum to #11566.
8f4fe8f 
Restored methods that were removed in WebAppClassLoader.Context.

Fixed method signatures for deprecated method -- they must take the deprecated ClassMatcher, not the newly introduced one.

Signed-off-by: Simone Bordet <[email protected]>
@gregw gregw mentioned this pull request May 1, 2024
Merged
sbordet added a commit that referenced this pull request May 2, 2024
@sbordet
Addendum to #11566. (#11722)
42ba415 
* Addendum to #11566.

Restored methods that were removed in WebAppClassLoader.Context.

Fixed method signatures for deprecated method -- they must take the deprecated ClassMatcher, not the newly introduced one.

Signed-off-by: Simone Bordet <[email protected]>
@gregw gregw mentioned this pull request May 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janbartel janbartel janbartel approved these changes

@joakime joakime joakime approved these changes

Assignees
No one assigned
Labels
build-no-cache High Priority
Projects
No open projects
Jetty 12.0.9 - FROZEN
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gregw @joakime @janbartel