Issue #4450 - websocket-core exposes internal classes

[lachlan-roberts]

Issue #4450

First 3 commits directly fix the visibility error messages from #4450.

Last commit is a more general cleanup where I have moved some classes around.

• NullAppendable is now an internal class.
• Moved Configuration and CoreSession out of the FrameHandler interface to their own files.
• All the websocket exceptions are now in the org.eclipse.jetty.websocket.core.exception package (8 exception classes).

Closes #4450
Closes #4226

[sbordet]

Yuck!

protected fields, getter and setters with Integer and Long rather than their primitive counterparts, etc.

I'm sure we can do better.

[gregw]

The intention here is to tri-state the fields, because we have lots of sources of configuration - filters, individual mappings, endpoints, annotations, connection factories and ultimately defaults. These configurations have to be merged into a single configuration, so knowing if they have been set is important when doing the merge - else we end up with many "defaults" distributed through out the code.

[sbordet]

Yuck! If this class (which is internal) needs WebSocketCoreSession, give that, not a configuration.
The whole concept that WebSocketCoreSession is-a configuration is IMHO wrong.

[gregw]

The wrongness here is that if extensions need to know the coreSession, they should be able to get it without needing to down cast a configuration.
Why isn't this method just setCoreSession rather than setConfiguration?

[lachlan-roberts]

I could change it to setCoreSession using the API CoreSession, but for ValidationExtension we would still need to cast that to the implementation class WebSocketCoreSession if we want to get access to these validation methods which aren't in the CoreSession interface.

[gregw]

So is that saying we actually need a Validator class with static methods that take both a Configuration and a Frame, which can then be used in both places without down cast?

[sbordet]

@lachlan-roberts I think the configurability needs to be re-thought, as it is particularly weird that WebSocketCoreSession is-a Configuration - typically has-a configuration.

The default configuration should be created once, then passed around to those that need it.

I need an explanation of what are the major places in the code that need to be configured, and why the inheritance solution has been chosen over the composition solution.

[gregw]

So currently the architecture is as follows:

• There is a Configuration interface that is used by anything that can have [gs]etFooTimeout and [gs]MaxBar style methods called on them. This includes the session itself as well as many other components that can be configured (filters, mappings etc.)
• There is a ConfigurationCustomizer that has the tri-state fields, that allows a partial configuration to be applied to another configuration. It is currently implemented by a ConfigurationHolder, which I don't really like as it is really on the the customizer that needs the tri-state. Does anything use the holder directly??? Hmm there are some usages, but they look like they are final configurations rather than tristate ones... I'm not sure they should be using it. Eitherway, I'd like to see a clear split between a customizer with tristate and a final configuration that must have values for every field.
• The CoreSession interface is-a Configuration because it has all the setters and getters of a Configuration. Perhaps it should be called Configurable?
• The concrete WebSocketCoreSession class directly implements the Configuration method to fields with default values. It doesn't use the holder (and I'm more dubious we need that class)

The rest of it looks kind of expected with factories and metadata having or being Configurations

I think we should look at getting rid of the Holder class and just have the customizer.
I also think we should consider renaming Configuration to Configurable to see if @sbordet understands it a bit more that way.

[gregw]

This has become a bit of a mega PR, so a bit hard to review. But I like the Configuration cleanup and other goodness. I think there is probably more todo, but let's get this merged and fix issues in smaller new PRs

[sbordet]

@lachlan-roberts, in class JavaxWebSocketServletContainerInitializer method JavaxWebSocketServerContainer initialize(ServletContextHandler context): JavaxWebSocketServerContainer is internal, but needs to be exported because it's in the signature of this class.

[sbordet]

@lachlan-roberts I would like to see one more test that tests the WebSocketContainer.set* methods.
That is, create the container, call the setter method and verify that a Session has the right values.

[sbordet]

There is still a JPMS issue that is not solved, and I would like the additional test detailed in the comments.

[lachlan-roberts]

@sbordet this PR was only intended to fix restructure websocket-core to stop it from exposing internal classes. I was going to consider the jetty and javax APIs separately.

I will make sure we have this test and will add it here if you want to expand the scope of this PR, otherwise I will open another one for it.