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

Reintroduce Cross Context Dispatch in Jetty 12 #11451

Merged
merged 61 commits into from
Mar 25, 2024

Conversation

janbartel
Copy link
Contributor

Implement cross context dispatch.

@joakime
Copy link
Contributor

joakime commented Feb 27, 2024

This PR is on track for 12.0.8 release, right?

@gregw
Copy link
Contributor

gregw commented Mar 11, 2024

@joakime @lachlan-roberts @olamy This is a big PR that we want in this release cycled (which may be rushed). Can you start reviewing this ASAP so it makes it in.

The key areas to consider are:

  • How we find other contexts
  • How we wrap other contexts as ServletContexts
  • How we create a special RequestDispatcher from the wrapped cross context ServletContext
  • The wrapping done before dispatch to turn eeX request/response back into core request/response
  • The slight changes to wrapping done in ContextHandler and/or ServletContentHandler to handle cross context dispatch.
  • Cross context params work because of the request attribute used by Fields.from, even if EE8/9 do their own parsing, they now use the Fields data structure
  • Multipart params are a bit limited, specifically as the data structure is different, we can't do cross environment dispatch
  • Cross context sessions work because of cross context cookies and and shared session id manager
  • Cross context bypasses security handler

@joakime any thoughts on better testing all of this?

@joakime
Copy link
Contributor

joakime commented Mar 14, 2024

PR #11516 is needed for the testing additions I'm working on.

@olamy
Copy link
Member

olamy commented Mar 19, 2024

this is passing TCK. Need some Arquillian changes though. arquillian/arquillian-container-jetty#186

joakime
joakime previously approved these changes Mar 20, 2024
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.

Merge this, it is quite good as it currently stands.
We'll keep adding testing it and refining over time.

@@ -339,6 +339,7 @@ protected void service(HttpServletRequest request, HttpServletResponse resp)
assertThat(connection.getResponse(), containsString(" 200 OK"));
}

cookieHistory.forEach(System.err::println);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should remove this System.err.println

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.

Lets get this merged. It's a good change.

@janbartel janbartel merged commit 2fc7ad8 into jetty-12.0.x Mar 25, 2024
8 checks passed
@janbartel janbartel deleted the jetty-12.0.x-cross-context-dispatch branch March 25, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Specification For all industry Specifications (IETF / Servlet / etc) Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants