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

investigate jetty 12 #2158

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

investigate jetty 12 #2158

wants to merge 2 commits into from

Conversation

tipsy
Copy link
Member

@tipsy tipsy commented Feb 28, 2024

No description provided.

@joakime
Copy link

joakime commented Feb 28, 2024

How important is maintaining Servlet support in this effort?
Meaning, if this migration is made to Jetty Core only, with no Servlet support, is that acceptable?

<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-servlet</artifactId>
<groupId>org.eclipse.jetty.ee9</groupId>
<artifactId>jetty-ee9-servlet</artifactId>
Copy link

Choose a reason for hiding this comment

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

I would recommend using ee10, not ee9 in this effort.

Copy link
Member Author

@tipsy tipsy Feb 28, 2024

Choose a reason for hiding this comment

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

Tried that first, but were missing most of these classes:

private fun Handler?.attachHandler(servletContextHandler: ServletContextHandler) = when {
    this == null -> servletContextHandler // server has no handler, just use Javalin handler
    this is HandlerCollection -> this.apply { addHandler(servletContextHandler) } // user is using a HandlerCollection, add Javalin handler to it
    this is HandlerWrapper -> this.apply {
        (this.unwrap() as? HandlerCollection)?.addHandler(servletContextHandler) // if HandlerWrapper unwraps as HandlerCollection, add Javalin handler
        (this.unwrap() as? HandlerWrapper)?.handler = servletContextHandler // if HandlerWrapper unwraps as HandlerWrapper, add Javalin last
    }

    else -> throw IllegalStateException("Server has unsupported Handler attached to it (must be HandlerCollection or HandlerWrapper)")
}

private fun HandlerWrapper.unwrap(): Handler = when (this.handler) {
    null -> this // current HandlerWrapper is last element, return the HandlerWrapper itself
    is HandlerCollection -> this.handler // HandlerWrapper wraps HandlerCollection, return HandlerCollection
    is HandlerWrapper -> (this.handler as HandlerWrapper).unwrap() // HandlerWrapper wraps another HandlerWrapper, recursive call required
    else -> throw IllegalStateException("HandlerWrapper has unsupported Handler type (must be HandlerCollection or HandlerWrapper")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Are there straightforward equivalents available?

Copy link

@joakime joakime Feb 28, 2024

Choose a reason for hiding this comment

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

The classes org.eclipse.jetty.ee9.nested.HandlerCollection and org.eclipse.jetty.ee9.nested.HandlerWrapper are internal nested classes, not meant to be used directly like you are doing.
Basically, if you find yourself wanting to use org.eclipse.jetty.ee9.nested.* classes, take a pause, and ask why you are using it?

Use one of the following core handlers ...

  • org.eclipse.jetty.server.handler.ContextHandlerCollection if you have a list of ContextHandler to lookup within. (the ee10 ServletContextHandler is a valid ContextHandler)
  • org.eclipse.jetty.server.Handler.Sequence for a raw list of Handler.
  • org.eclipse.jetty.server.Handler.Wrapper for a Handler wrapper.
  • org.eclipse.jetty.server.handler.PathMappingsHandler is a way to map multiple Handlers via a url path specs / patterns (you can mix/match with servlet, regex, or uri-template path specs)

There is no concept of a loose collection of just raw handlers. (what HandlerCollection did)

Copy link
Member Author

Choose a reason for hiding this comment

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

The use-case is that we have (had?) users passing a configured jetty.Server to Javalin. We then needed to attach Javalin to that jetty.Server.

We have removed the option to do this though, so perhaps we can remove this part of the code 🤔

Copy link

Choose a reason for hiding this comment

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

If Javalin can produce a Handler (be it a Handler.Wrapper, a ContextHandler, a ServletContextHandler, etc), then it would be trivial to just add it wherever the user wants it to be in the server handler tree.

Copy link
Member Author

Choose a reason for hiding this comment

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

If Javalin can produce a Handler (be it a Handler.Wrapper, a ContextHandler, a ServletContextHandler, etc), then it would be trivial to just add it wherever the user wants it to be in the server handler tree.

Do you mean for us to expose our Handler through an API, and put it on our users to attach this Handler to their jetty.Server where they want it?

@tipsy
Copy link
Member Author

tipsy commented Feb 28, 2024

How important is maintaining Servlet support in this effort?

I would say it's required. Personally, I am quite married to Jetty, but we have a lot of people who attach Javalin to a different server as a servlet, and we have some maintainers who want the project to become server independent.

@tipsy
Copy link
Member Author

tipsy commented Feb 28, 2024

Ripped out the handler-wrapper stuff and changed to ee10. Only resource handler isn't compiling, but many tests will fail once we get it compiling.

@tipsy tipsy force-pushed the master branch 4 times, most recently from 765b7d0 to 6faa186 Compare May 8, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants