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

simplify the usage of WebSocketUpgradeFilter in jetty 10 #5413

Merged
merged 6 commits into from Nov 6, 2020

Conversation

lachlan-roberts
Copy link
Contributor

@lachlan-roberts lachlan-roberts commented Oct 9, 2020

Now any WebSocketUpgradeFilter will use the default WebSocketMapping attribute by default. And we will not add a WebSocketUpgradeFilter automatically in the JettyWebSocketServletContainerInitializer if we detect there is already one added.

  • Change WebSocketUpgradeFilter mapping init param name and default value.
  • If the WebSocketMapping Attribute init parameter is not set on the WebSocketUpgradeFilter it will throw in its init() method.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts added this to To Do in Jetty 10.0.0 via automation Oct 9, 2020
@lachlan-roberts lachlan-roberts requested review from gregw and removed request for gregw October 12, 2020 09:06
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 not really convinced this is workable as I'm not sure you can always convert a className into a class at the point you need to.

They might extend the jetty servlet in their application, so the classname may be for something in the webapp, which might not be loadable until started.

Looking at the init param feels safer to me.

Let's discuss.

Jetty 10.0.0 automation moved this from To Do to In Review Oct 12, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
If a user wants to use the WebSocketUpgradeFilter with a different WebSocketMapping,
they can add the ServerContainer manually and put their own WebSocketMapping in.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
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.

So this is really now just a clean up and rename, rather than a substantive change.

…WebSocketUpgradeFilter

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

@gregw yes but also a change in behaviour, the WebSocketUpgradeFilter will now throw in its init() method if the mapping init parameter has not been set.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts merged commit b2bfe7e into jetty-10.0.x Nov 6, 2020
Jetty 10.0.0 automation moved this from In Review to Done Nov 6, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-WebSocketUpgradeFilter branch November 6, 2020 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Jetty 10.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants