Use Filter name to identify the WebSocketUpgradeFilter.

[lachlan-roberts]

• Don't allow configuration of WebSocketMapping attribute.
• The WebSocketUpgradeFilter is identified by it's name, which must be set as the fully qualified class name.

[joakime]

Where is the issue that caused this change?

How would a webapp have 2 WebSocketUpgradeFilters now?
How would they configure the second one?

I'm a big fat -1 on this change as it stands.

[lachlan-roberts]

@joakime There was no issue put on github, it was from verbal conversations with Simone from the failures he got on cometd.

I have pushed a test for the use of multiple WebSocketUpgradeFilters, using the special name for the filter only overrides the default upgrade filter being added, additional filters can still be added.

[gregw]

LGTM

[joakime]

We need more test cases for alternate WSUF usage.

1. Two filters, both as WSUF.
   one on default / dynamic at /* (auto discovered mappings)
   one on new url-pattern /alt/* (programmatic mappings)

2. Two filters, one overridden.
   one on default / dynamic at /* (auto discovered mappings)
   one with overridden WSUF with custom init() that configures the mappings.

3. One filter, replacing default/dynamic filter.
   one on /alt/*, which is the filter to use for discovered mappings (for both javax.websocket and jetty apis)

For the programmatic mappings, this should support WebAppContext, configuring mappings via servlet initialization phase (eg: in a ServletContextListener. or ServletContainerInitializer)
This needs to be tested from a WebAppContext to ensure that there is sufficient API exposed to allow a WebApp to customize the mappings programmatically.
In Jetty 9.4.x this was a generic API at MappedWebSocketCreator.
But in Jetty 10.0.x this feature does not seem to be exposed in an API that a WebAppContext can use. (You have the various .addMapping methods on WebSocketServlet, but not with websockets that are managed by a WebSocketUpgradeFilter)

[lachlan-roberts]

@joakime there is no distinction whether the mappings are auto-discovered or added programmatically. They both end up in the same WebSocketMappings instance which is shared between all of the WebSocketUpgradeFilters, and shared for both the jetty and javax websocket implementations.

We also do not currently allow mappings to be configured through the WebSocketUpgradeFilter directly, the mappings need to be configured through the ServerContainer in the jetty websocket server API. So you could do something like:

JettyWebSocketServerContainer container = JettyWebSocketServerContainer.getContainer(servletContext);
container.addMapping("/echo", EchoEndpoint.class);

[joakime]

@joakime there is no distinction whether the mappings are auto-discovered or added programmatically. They both end up in the same WebSocketMappings instance which is shared between all of the WebSocketUpgradeFilters, and shared for both the jetty and javax websocket implementations.

That's a problem.

And a distinct difference from Jetty 9.4.x

We need the ability to configure different mappings for different WSUFs.

We also do not currently allow mappings to be configured through the WebSocketUpgradeFilter directly, the mappings need to be configured through the ServerContainer in the jetty websocket server API. So you could do something like:

That's also a problem for those that want to use WSUF with the Jetty WebSocket APIs.
The mappings need to have a way to be programmatically configured during embedded-jetty context setup.
AND have options during the normal servlet context initialization phase.
This needs a few webappcontext based test cases (for each initialization phase option)

Not everyone wants to use the WebSocketServlet, as its incredibly limiting, and has undesired side effects related to filter execution.

The dynamically added WebSocketUpgradeFilter is typically the first filter executed. (which many people actually want/like)
This needs a a few webappcontext test cases. (add multiple filters in the WEB-INF/web.xml, and ensure that the dynamically added WSUF is executed first)

But sometimes the user wants the WebSocketUpgradeFilter to execute after other filters (such as those for spring security).
To do this, we manually add the WebSocketUpgradeFilter to the WEB-INF/web.xml.
This also needs a few test cases. (make sure that the user can control the WSUF order, and that the implementation doesn't undo any efforts)

Also, the WebSocketMappings are far more powerful than the url-pattern from the servlet spec.
There are many projects already using the Jetty WebSocket APIs against WSUF for their own purposes, and have to live in the same context as other libraries that use javax.websocket.
(Sometimes the direct WSUF usage is on /*, sometimes it's on alternate url-patterns like /api/* or *.ws)
With this current implementation, we are stepping the toes of the other API mappings, as you are assuming too many things.

We need to get all of these addressed.

[gregw]

@joakime i don't understand the use case (s) you are describing. WebsocketMappings has no state other than the map it self a reference to the shared components like buffer pool. What does having multiple instances of WebsocketMappings do for an app? What can't it do? I think they can still have different configurations on the different filters and different mappings and still share the WebsocketMappings.

[sbordet]

I think we should bake in the tests the following:
A) web.xml with non-WSUF filter => expect WSUF + non-WSUF
B) web.xml with non-WSUF and default WSUF in that order => expect non-WSUF + default WSUF

[joakime]

There's some test failures left to address.

no longer relevant

[lachlan-roberts]

Test failures seem unrelated, I have merged from 10.0.x to try to fix them.