Matthias247 / jawampa

Web Application Messaging Protocol (WAMP v2) support for Java
Apache License 2.0
148 stars 57 forks source link

Allow specifying supported/preferred serializations in the client/server #25

Closed jrogers closed 9 years ago

jrogers commented 9 years ago
Matthias247 commented 9 years ago

Hi. Sorry for the delay - was on vacation last week.

Do I understand it correctly that this is only for restricting the serialization on server or client side to a subset of all protocols? And not for also providing custom serialization methods (ObjectMappers) as we talked about in #25 ?

Most things are looking good to me. I would personally prefer a Set instead of a List for the Serializations and also having a static DefaultWampSerializations Set instead of abusing null for it, but both will work.

I think the WampServerWebsocketHandler really needs one more update:
serialization = WampSerialization.fromString(actualProtocol); will return any valid serialization even if it's not contained in supportedSerializations. We can't rely on Netty here to close the connection if the requested subprotocol is not in subProtocols - because the WebSocket spec says servers should accept connections even if they don't support it and send the subprotocol that they support back to the client.

So this

if (serialization == WampSerialization.Invalid) {

should be fixed into something like

if (serialization == WampSerialization.Invalid || !supportedSerializations.contains(serialization)) {

Ah and another small thing: WampClientBuilder should reject having WampSerialization.Invalid in the list. Like:

for (WampSerialization serialization : serializations) {
    if (serialization == WampSerialization.Invalid) throw new ApplicationError(ApplicationError.INVALID_SERIALIZATIONS);
    this.serializations.add(serialization);
}
Matthias247 commented 9 years ago

Merged with some small changes (default List instead of null and better validation). Didn't go for Sets as I realized that the ordering of the Serializations is important.

jrogers commented 9 years ago

Sorry about not replying to or updating this one, I have been very busy at work lately. Thanks for the change.