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

Rename Handler Nested & Collection #9305

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Feb 3, 2023

Fixes #8765 #8759 . Part of #9072

There is now a Handler interface hierarchy:

  • Container is a Handler that has 1 or more contained Handlers.
  • Wrapper is a Container with only 1 handler and a setHandler method.
  • Collection is a Container with n handlers and a addHandler method

class are now:

  • Abstract implements Handler
  • AbstractContainer extends Abstract implements Container
  • BaseWrapper extends AbstractContainer implements Wrapper
  • Sequence extends AbstractContainer implements Collection

Lots of other associated cleanups

There is now a Handler interface hierarchy:
 + Container is a Handler that has 1 or more contained Handlers.
 + Wrapper is a Container with only 1 handler and a setHandler method.
 + Collection is a Container with n handlers and a addHandler method

class are now:
 + Abstract implements Handler
 + AbstractContainer extends Abstract implements Container
 + BaseWrapper extends AbstractContainer implements Wrapper
 + Sequence extends AbstractContainer implements Collection

 Lots of other associated cleanups
@gregw
Copy link
Contributor Author

gregw commented Feb 4, 2023

@sbordet @joakime @janbartel Can I get a review on this ASAP as I really don't want this to go stale and have to do it again.

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.

I'm good with this.

@gregw gregw merged commit 60a08f5 into jetty-12.0.x Feb 6, 2023
@gregw gregw deleted the jetty-12-handler-rename branch February 6, 2023 01:15
@sbordet
Copy link
Contributor

sbordet commented Feb 6, 2023

@gregw sorry I don't like some parts of this PR.

In every other Jetty class, Wrapper is a class and it is used to "wrap" an instance of an interface, so we should stick with this semantic.

BaseWrapper is not a good name.

I would rather have Wrapper back as a class, and if we really need it use Single as the interface opposed to Collection.

"Single" is also used in Java's Collections.singletonList() to return a list of 1 element ("Singleton" cannot be used as it remembers the well-known pattern which has a different meaning than what we want here).

@gregw
Copy link
Contributor Author

gregw commented Feb 6, 2023

@sbordet how about Delegate rather than Single?

@gregw
Copy link
Contributor Author

gregw commented Feb 6, 2023

Actually, I don't mind Singleton as it is now just as much associated with Collections.singletonXxx() than the pattern. Thus a Wrapper is a concrete implementation of a Singleton Container.

@gregw gregw mentioned this pull request Feb 6, 2023
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this pull request Feb 6, 2023
…x-documentation-operations-logging

* upstream/jetty-12.0.x: (42 commits)
  fixed style
  cleanup TODOs for decoration
  Issue jetty#9300 - Rename RetainableByteBufferPool to ByteBufferPool
  Removed TODOs that will not be done.
  Rename Handler Nested & Collection (jetty#9305)
  fix surefire jpms configuration
  fix merge
  fix merge
  Bump maven.surefire.plugin.version from 3.0.0-M5 to 3.0.0-M8 (jetty#9255)
  Rename RetainableByteBufferPool to ByteBufferPool
  Fixed merge
  Fix jetty#9285 use possibly wrapper response for redirection (jetty#9286)
  Issue jetty#9293 - Jetty 12 - Relax JPMS dependencies (quic) (jetty#9307)
  Issue jetty#9293 - Jetty 12 - Relax JPMS dependencies (fcgi) (jetty#9306)
  Jetty 10 - Configurable Unsafe Host Header (jetty#9283)
  Issue jetty#9293 - Jetty 12 - Relax JPMS dependencies. (jetty#9296)
  Issue jetty#9293 - Jetty 12 - Relax JPMS dependencies. (jetty#9299)
  fix dependency
  add used dependency
  this dependency is used in test scope
  ...
Comment on lines -71 to -75
Handler.Collection handlers = server.getDescendant(Handler.Collection.class);
if (handlers == null)
server.setHandler(new Handler.Collection(contexts, new DefaultHandler()));
else
handlers.addHandler(contexts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Causes #12207, also decreasing the accuracy of the Javadoc a few lines above, which (now incorrectly) states:

Also put in a DefaultHandler so we get a nicer page than a 404 if we hit the root and the webapp's context isn't at root.

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.

4 participants