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

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

Open
glassfishrobot opened this issue Aug 19, 2013 · 13 comments
Labels
API (Both) Impacts the client and server API enhancement Adding a new feature or improving an existing one
Milestone

Comments

@glassfishrobot
Copy link

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
Copy link
Author

@glassfishrobot Commented
Reported by dannycoward

@glassfishrobot
Copy link
Author

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

@glassfishrobot
Copy link
Author

@markt-asf
Copy link
Contributor

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
Copy link
Contributor

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.

@joakime
Copy link
Contributor

joakime commented May 7, 2020

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
Copy link
Contributor

joakime commented May 7, 2020

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?)

@markt-asf
Copy link
Contributor

I like this idea. It is simple and clear.

@markt-asf markt-asf added API (Both) Impacts the client and server API enhancement Adding a new feature or improving an existing one Jakarta EE 10 and removed Priority: Minor Type: Bug labels May 7, 2020
@markt-asf markt-asf modified the milestones: backlog, 2.2 May 4, 2022
@volosied
Copy link

volosied commented Aug 30, 2023

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
Copy link

  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

@joakime
Copy link
Contributor

joakime commented Sep 19, 2023

Have you filed this as a bug at Tomcat?

@volosied
Copy link

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

@joakime
Copy link
Contributor

joakime commented Sep 20, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API (Both) Impacts the client and server API enhancement Adding a new feature or improving an existing one
Projects
None yet
Development

No branches or pull requests

4 participants