Clarify search order of Encoders / Decoders

[joakime]

As outlined in #39 (comment) ...

Danny Coward states ...

then try to find a Decoder.Text or a Decoder.TextStream. Look, in order, through developer provided ones (single list, may contain both Text and TextStream types). Then look through platform provided ones. Use the first match to decode it. If the decode works, call the MessageHandler and stop, if not, call the onError/@websocket method with the decode exception and stop.

What is the search order when the "developer provided ones" comes from 2 places (annotations and configuration)?

@ServerEndpoint(
    value = "/purchase",
    decoders = {AppleDecoder.class, PearDecoder.class, FruitDecoder.class},
    configurator = FruitStandConfigurator.class
)
public class PurchaseEndpoint
{
}

and in the container init ...

        ServerContainer container = servletContext.getAttribuet(javax.websocket.server.ServerContainer.class.getName());
        ServerEndpointConfig config = ServerEndpointConfig.Builder.create(PurchaseEndpoint.class, "/floo")
                .decoders(Arrays.asList(CherryDecoder.class, BananaDecoder.class)).build();
        container.addEndpoint(config);

What is the resulting search order of decoders?

Option 1: (EndpointConfig wins over Annotations)

1. CherryDecoder.class
2. BananaDecoder.class
3. AppleDecoder.class
4. PearDecoder.class
5. FruitDecoder.class
6. Platform Provided Decoders

Option 2: (Annotation wins over EndpointConfig)

1. AppleDecoder.class
2. PearDecoder.class
3. FruitDecoder.class
4. CherryDecoder.class
5. BananaDecoder.class
6. Platform Provided Decoders

(Does the order of the Platform Decoders matter? I don't think so)

[joakime]

Then there is the issue with merging decoders in common between the annotations and configuration.

It could be argued that just like @ServerEndpoint(value="/echo") is overridden if using ServerEndpointConfig.getPath(), why wouldn't the decoders/encoders from the config also override/replace the ones from the annotation.

The javadoc is unclear on this behavior.
And the TCK doesn't seem to have a test for this scenario as well.

[markt-asf]

Clarification is (potentially) a change in behaviour so this will need to wait for Jakarta EE 10. As a note to self I don't think the API will need to change (but haven't thought about this much) but don't forget to update the spec document and the Javadoc.

[joakime]

I don't think the API will need to change (but haven't thought about this much)

I agree. This seems like a need for updated documentation and/or javadoc, along with some new TCK tests.

[volosied]

To me, it makes sense for the "/purchase" endpoint to use "AppleDecoder.class, PearDecoder.class, FruitDecoder.class" while the "/floo" endpoint to use "CherryDecoder.class, BananaDecoder.class". Shouldn't each URI path be mapped to a particular config?

However, I'm not familiar with this scenario: "could be argued that just like @serverendpoint(value="/echo") is overridden if using ServerEndpointConfig.getPath()". @joakime Could you provide an example? Overriding encoders / decoders could make sense.

[joakime]

@volosied In the original example ...

ServerEndpointConfig.Builder.create(PurchaseEndpoint.class, "/floo")

The PurchaseEndpoint.class has an annotation defining the the decoders.
Then when you use the ServerEndpointConfig.Builder.create(class, ....) you have the ability to change the values coming from the annotation.

Since the annotation doesn't support multiple paths, or having multiple annotations, the end user has to create the config manually to add.
The annotation entries for decoders/encoders/subprotocols/extensions/configurator are viewed as requirements for the class to function.

I wish we had either defined the annotation in a way that multiple configurations could be defined (use annotations only), or that it only applied to 1 configuration (never mix with manual endpoint config).
But we didn't, and this scenario (using annotated endpoint in ServerEndpointConfig.Builder) is now quite common, so we cannot fix it.

Frankly, the "platform" decoders / encoders should have also been mandatory to declare in your decoders/encoders to use them (as their behaviors have never been fully defined)
OR, in my opinion, the ENTIRE decoders / encoders concept shouldn't even be here on the websocket spec/api (that's a higher level concern)

[volosied]

I see. Thanks for the explanation and the background information.

I'm not sure what the official proposal is, but It makes sense to use the Annotated Endpoints values for the encoders and decoders by default as you suggested before. If any encoders / decoders are set via the ServerEndpointConfig.Builder then they should override the ones set via annotation.

Since you mentioned the subprotocols, extensions, and configurator. Should this overriding process be extended to them, too?

[markt-asf]

How about this then:

• EndpointConfig Encoders/Decoders are in addition to those configured via Annotation
• EndpointConfig Encoders/Decoders take priority over those configured via Annotation
• Annotation Encoders/Decoders take priority over the platform defaults

Same rules for sub-protocols, extensions and configurators (except there is only a single configurator)

Is deprecating everything encode and decoder related a step too far?

[markt-asf]

See also #213 and make sure any fix for this issue also addresses the problems described in #213

[jansupol]

I'd expect the programmatic API to override annotations (so that the annotated class can be reused if the customer wants to). That I can see in the most Specs.

The Javadoc to ServerEndpointConfig.Builder#decoders says:

Sets the decoder implementation classes

So the decoders(encoders,...) are set, not added. If addition would be a requirement, that would need to be done some other way:

• addDecoders() - too many new methods,
• ServerEndpointConfig.Builder.create(...).add(/*noargs - switch to addition mode*/).decoders(...).encoders(...).build().
• Some other way

In JAX-RS Spec (Section 4.2.4), the user-defined providers always have priority over the platform providers. I imagine the same behaviour for Encoders/Decoders here.

just my $0.02

[joakime]

How about this then:

• EndpointConfig Encoders/Decoders are in addition to those configured via Annotation
• EndpointConfig Encoders/Decoders take priority over those configured via Annotation
• Annotation Encoders/Decoders take priority over the platform defaults

Same rules for sub-protocols, extensions and configurators (except there is only a single configurator)

I'm good with this.

Are we going to have the primitive decoders in the WebSocket spec too?

Is deprecating everything encode and decoder related a step too far?

Yeah, that ship has sailed.

[joakime]

We need to be careful about the order of the resulting set of encoders / decoders when we have these new rules.

[jansupol]

Are we going to have the primitive decoders in the WebSocket spec too?

Do you mean to allow users to override the platform primitive types decoders?

[joakime]

Are we going to have the primitive decoders in the WebSocket spec too?

Do you mean to allow users to override the platform primitive types decoders?

This is part of the discussion at #213 .

Basically, this statement means that the primitive encoders / decoders are defined and implemented in the WebSocket spec for every websocket implementation to use.
This codifies the behavior of the encoders / decoders in a standard way (that doesn't exist right now).

There's so many behavior differences in things like ...

• what is a valid value for a Boolean? (true, TRUE, t, on, yes, etc)
• how many decimal places are represented? (double, float, etc)
• should long be represented with the L suffix? to distinguish it from other number types?
• should integer / long / short have full prefix support? (we all seem to support -, but what about + prefix? like what Integer.parseInt does?)
• etc ...

As for having a requirement to declare their use in the encoders / decoders for each endpoint, that is still undecided.

I personally think it was a mistake to have support for anything but String / ByteBuffer, but that ship has long since sailed.

[markt-asf]

Is the typical user expectation with the current API that EndpointConfig Encoders/Decoders are in addition to those configured via Annotation or that they replace them? The current Javadoc suggests replacement so on reflection I'd prefer to do that if possible. If we need to handle addition and replacement then so be it.

The current spec is pretty clear about how String <-> primitive (or class equivalent) is meant to behave. There shouldn't be that much difference. No objection to adding a utility class to do the conversion.

Looking at the current spec text I think the platform encoders always have to be used unless overridden by EndpointConfig or Annotation.

[joakime]

Is the typical user expectation with the current API that EndpointConfig Encoders/Decoders are in addition to those configured via Annotation or that they replace them? The current Javadoc suggests replacement so on reflection I'd prefer to do that if possible. If we need to handle addition and replacement then so be it.

I cannot find a TCK test for this. Can you?

Speaking for Jetty ...
If the EndpointConfig (eg: from a ServerEndpointConfig) has encoders / decoders declared (not-empty list), then those are the only ones used.
If the EndpointConfig.getEndpointClass() happens to be an annotation, then this is the fall-back for encoders / decoders if the EndpointConfig does not have them declared (null, or empty).

This produces an interesting side effect though.
It is not possible, for example, to have an annotated endpoint with an encoder declared ...

@ServerEndpoint(value = "/time", encoders = TimeEncoder.class)
public class TimeSocket {
 // ...
}

and later use a ServerEndpointConfig with that TimeSocket to remove the encoders (to null them out, empty set, no encoders).

ServerEndpointConfig timeconfig = ServerEndpointConfig.Builder
    .create(TimeSocket.class, "/alt-time")
    .encoders(List.of()) // want to have no encoders, want to rely on primitives, but we are instead relying on TimeEncoder.class from the annotation
    .build();

you can only replace them with a no-op implementation.

ServerEndpointConfig timeconfig = ServerEndpointConfig.Builder
    .create(TimeSocket.class, "/alt-time")
    .encoders(List.of(NoOpTimeEncoder.class))
    .build();

[jansupol]

I do not think the API should come with the primitive-type coders. The Spec would need to force the implementation to use them (otherwise they are there for no reason), and the implementation will try to find its way to override them if it wants to support additional conversions from String representation (as mentioned true, TRUE, t, on, yes, +, -, etc).