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

Add constructors accepting the handler to wrap to all core handler wrappers #9988

Merged
merged 5 commits into from
Jun 30, 2023

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Jun 28, 2023

Add constructors that accept the wrapped handler to all core Handler.Wrapper subclasses

This allows the chaining of hander wrappers in a more elegant way; this is how we currently do it:

GzipHandler handler = new GzipHandler();
handler.setHandler(new EchoHandler());
server.setHandler(handler);

and this is what this change allows us to write:

server.setHandler(new GzipHandler(new EchoHandler()));

@sbordet
Copy link
Contributor

sbordet commented Jun 28, 2023

Seems reasonable.
However, newly introduced parameterless constructors should be implemented by calling this(null), so that if the other constructor is changed, we properly delegate.
Also, I would like the Handler parameter to be the first, as it is canonical in Java (subclass has the same list of parameters as superclass, plus eventually others).

@janbartel
Copy link
Contributor

I have a little bit of a doubt after the very long history of (at least some) handler constructors taking the parent handler as the argument, whereas this flips it around and passes the child. But I do think it looks neat. Maybe we solve that with very good documentation.

@lorban
Copy link
Contributor Author

lorban commented Jun 29, 2023

@janbartel this PR is a followup to #9948 that changed all handlers to only take the child handler as an argument as it was decided in #9946.

Said differently, passing the child handler to the constructor of a Handler.Wrapper is the new standard.

@janbartel
Copy link
Contributor

@lorban hhhmm'ok. So you're saying that if I look in the documentation I'll find this difference with foregoing versions of jetty clearly spelt out? :)

@lorban
Copy link
Contributor Author

lorban commented Jun 29, 2023

@janbartel now you should :-)

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.

I'm +1 on this once it actually compiles.

@lorban lorban changed the title PROPOSAL: Add constructors accepting the handler to wrap to all core handler wrappers Add constructors accepting the handler to wrap to all core handler wrappers Jun 29, 2023
@lorban lorban marked this pull request as ready for review June 29, 2023 14:08
@lorban lorban requested a review from gregw June 29, 2023 14:30
lorban added 5 commits June 30, 2023 10:09
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban force-pushed the jetty-12-add-handler-wrapper-ctors branch from 2aed6ab to ae45c23 Compare June 30, 2023 08:10
@lorban lorban merged commit 457d41c into jetty-12.0.x Jun 30, 2023
@lorban lorban deleted the jetty-12-add-handler-wrapper-ctors branch June 30, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants