websocket icon indicating copy to clipboard operation
websocket copied to clipboard

Define that user provided encoders and decoders override the platform provided ones (e.g. for Integer-> String conversions)

Open glassfishrobot opened this issue 11 years ago • 13 comments

Otherwise, what would be the point ?

For encoders, the javadoc on the RemoteEndpoint send operations does define the override for primitive types, but doesn't mention String-> String encoders which should be allowed.

For decoders, there isn't anything, so currently its undefined.

Affected Versions

[1.0]

glassfishrobot avatar Aug 19 '13 23:08 glassfishrobot

  • Issue Imported From: https://github.com/javaee/websocket-spec/issues/213
  • Original Issue Raised By:@glassfishrobot
  • Original Issue Assigned To: @pavelbucek

glassfishrobot avatar Feb 12 '18 08:02 glassfishrobot

@glassfishrobot Commented Reported by dannycoward

glassfishrobot avatar Aug 19 '13 23:08 glassfishrobot

@glassfishrobot Commented This issue was imported from java.net JIRA WEBSOCKET_SPEC-213

glassfishrobot avatar Apr 24 '17 11:04 glassfishrobot

Otherwise, what would be the point ?

Indeed. I think this is sufficiently obvious that we can clarify it in 2.0. I'll work on some updates.

markt-asf avatar May 07 '20 11:05 markt-asf

Ah. As I look into this it gets more complex. I'm using String throughout here but the issue also includes Reader, ByteBuffer, InputStream and any other type explicitly mentioned when discussing message handling.

First of all Encoders. These are used with sendObject() but they are also implicitly used with the return value from a MessageHandler or a method annotated with @OnMessage.

The sendObject() case is relatively simple. The only container provided encoders are the Java primitives and equivalents and updating the Javadoc is simple enough for this case. String is just another object in this case so if there is an Encoder it will be used and if there isn't there will be an error. I have a Javadoc update for this that I'll submit a PR for shortly.

Return values are less obvious. If the method returns a String and there is an Encoder registered for String should sendObject() be used with the Encoder or should sendText() be used?

Similar questions exist for Decoders. Does MessageHandler.Whole<String> use a Encoder registered for String or not? And essentially the same question when considering methods annotated with @OnMessage.

The final part of the puzzle is the search order of annotations and configurations. See #289. I mention that here for completeness but I think it can be handled in that issue.

For this issue I am currently thinking that a registered Encoder always takes priority when determining how to handle a message. This allows developers to override the default container provided handling for any primitive or object. My thinking is that this is the logical extension of what we already have for sendObject(). Thoughts?

Whatever we decide for this will have implications for the specification document, Javadoc and the TCK so this is definitely a Jakarta EE 10 issue.

markt-asf avatar May 07 '20 17:05 markt-asf

I really wish this spec never had Encoders and Decoders.

I've always interpreted the spec as the implementation always having primitive encoders as default implementations.

  • java.lang.Boolean (boolean) as TEXT
  • java.lang.Byte (byte) as TEXT
  • java.lang.Character (char) as TEXT
  • java.lang.Double (double) as TEXT
  • java.lang.Float (float) as TEXT
  • java.lang.Integer (int) as TEXT
  • java.lang.Long (long) as TEXT
  • java.lang.Short (short) as TEXT
  • java.lang.String (String) as TEXT
  • java.nio.ByteBuffer as BINARY
  • byte[] as BINARY

Then when a Endpoint is registered, the declared Encoders replace the previous set for the same type. So if they had a MyStringEncoder extends Encoder<String> then that would be the new String encoder for all Object based actions.

The use of sendObject() and the return types from MessageHandler and the return types from methods annotated with @OnMessage are always Object based actions, never using sendBinary() or sendText().

joakime avatar May 07 '20 17:05 joakime

In fact, wouldn't it be swell to just have each of the primitive encoders and decoders declared in the websocket API and ask the implementations to use them?

That way the other vague pieces about supporting primitives could be standardized as well (such as how many decimal places do we support in double or float, what does a "byte" look like in text form? what about out of bands usage for that format?)

joakime avatar May 07 '20 17:05 joakime

I like this idea. It is simple and clear.

markt-asf avatar May 07 '20 18:05 markt-asf

Can someone elaborate on parts of this issue post as I don't quite follow it.

Then when a Endpoint is registered, the declared Encoders replace the previous set for the same type.
So if they had a MyStringEncoder extends Encoder<String> then that would be the new String encoder for all Object based actions.

How is this different from the new current spec? The primitive would be autoboxed via the sendObject​(Object data) call, so wouldn't the custom encoder be picked first?

  1. In the original issue text. when could a Integer -> String or a String -> String encoder be needed? Isn't the toString called on the Object before it converted to byte data?

Once again, wouldn't the custom encoder be used first (assuming the class type is applicable) before the platform encoder, per spec?

The 2.1 javadoc for sendObject​ states that: A developer-provided encoder for a Java primitive type and its object equivalent overrides the container default encoder.


As for the other comments: The use of sendObject() and the return types from MessageHandler and the return types from methods annotated with @OnMessage are always Object based actions, never using sendBinary() or sendText(). I also agree here.

Thanks!

volosied avatar Aug 30 '23 20:08 volosied

  1. Should this be titled Define that user provided encoders and decoders for primitive types override the platform provided ones

  2. Testing on Open Liberty and Tomcat, I saw that custom primitives encoders are handled before platform encoders, but platform decoders for primitives are handled first. Custom / Platform decoders should be switched around?

Tomcat Decoder: https://github.com/apache/tomcat/blob/a1cc4e94a17589f2edb28c4d6807fb28252130e9/java/org/apache/tomcat/websocket/pojo/PojoMessageHandlerWholeText.java#L93-L109

Tomcat Encoder: https://github.com/apache/tomcat/blob/a1cc4e94a17589f2edb28c4d6807fb28252130e9/java/org/apache/tomcat/websocket/WsRemoteEndpointImplBase.java#L604-L614

volosied avatar Sep 19 '23 14:09 volosied

Have you filed this as a bug at Tomcat?

joakime avatar Sep 19 '23 15:09 joakime

I have not. Before I do, let me ask @markt-asf. Would this be considered a bug -- looks like it's been this way for 10 years?

Looked at this more, and I don't see decoders registered for primitives or Strings (decoders only added when nothing else has a match) https://github.com/apache/tomcat/blob/f4fb5a923d061d45794dfa64eec91af213c77c8d/java/org/apache/tomcat/websocket/pojo/PojoMethodMapping.java#L451

volosied avatar Sep 20 '23 19:09 volosied

Jetty doesn't add the Primitive decoders to the decoder list on the either the EndpointConfig.decoders or the ServerEndpointConfig.decoders

Those are used internally, when nothing else will handle the object.

joakime avatar Sep 20 '23 19:09 joakime