Spec: ban warning on unknown fields for spec v3 #205
Conversation
SimonWoolf
force-pushed
the
forward-compatibility
branch
from
9314572 to
919bebc
Compare
There was a problem hiding this comment.
(I don't know which one, @mschristensen I think you mentioned it, do you know?)
@SimonWoolf I think Paddy mentioned it, and I think he said Ably Java
| @@ -1299,6 +1299,7 @@ h4. Message | |||
| ** @(TM2f)@ @timestamp@ time in milliseconds since epoch. If a message received from Ably does not contain a @timestamp@, it should be set to the @timestamp@ of the encapsulating @ProtocolMessage@ | |||
| * @(TM4)@ @Message@ has constructors @constructor(name: String?, data: Data?)@ and @constructor(name: String?, data: Data?, clientId: String?)@. | |||
| * @(TM3)@ @fromEncoded@ and @fromEncodedArray@ are alternative constructors that take an (already deserialized) @Message@-like object (or array of such objects), and optionally a @channelOptions@, and return a @Message@ (or array of such @Messages@) that's decoded and decrypted as specified in @RSL6@, using the cipher in the @channelOptions@ if the message is encrypted, with any residual transforms (ones that the library cannot decode or decrypt) left in the @encoding@ property per @RSL6b@. This is intended for users receiving messages other than from a REST or Realtime channel (for example, from a queue), to avoid them having to parse the @encoding@ string themselves. | |||
| * @(TM5)@ When decoding a @Message@, if the SDK encounters any fields it does not recognise, it must silently ignore them, and not print any kind of error or warning log. | |||
There was a problem hiding this comment.
Worth clarifying that debug/trace/info is ok?
|
Does RSF1 not already cover this? |
Oh, yes, good point, I missed that. |
|
Yep, I’d say it's an ably-java bug in that case. Do you have an example of the error message? I’ll create an ably-java bug. |
I don't, I have this second hand (Mike commented above that Paddy mentioned it). |
|
Yep, I’ll take a look. Does sandbox contain the realtime changes that trigger the warning? |
quick hack: delete this line: https://github.com/ably/ably-java/blob/e0e23176c717406f0c9a0e96752b6dc8d6663e47/settings.gradle#L3 |
See for example https://github.com/ably/ably-java/blob/main/lib/src/main/java/io/ably/lib/types/ProtocolMessage.java#L226 |
That's a |
But it does violate the existing RSF1, given that it doesn't "ignore" it per that spec item's language. So it still sounds like an ably-java bug. |
There's apparently an sdk that prints a warning message when it decodes a Message with an unknown field. This bans that for v3, for forward compatibility.
(I don't know which one, @mschristensen I think you mentioned it, do you know?)
I was thinking if I might have to bump the major version for this change, but looking at various SDKs, the only SDK so far to have implemented wire protocol v3 (and therefore spec v3) is ably-js, which doesn't complain about unknown fields, so already satisfied this. So I think I'm safe to add this as a requirement for v3. @ttypic wdyt?
(the purpose is to be able to use protocol v3 to trigger possible inclusion of new fields that are being added for various purposes like message interactions and data sync)