-
Notifications
You must be signed in to change notification settings - Fork 3
Add spec for ObjectMessage encoding and decoding
#335
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
Conversation
textile/features.textile
Outdated
| * @(OD4)@ @ObjectData@ encoding: | ||
| ** @(OD4a)@ Payloads must be booleans, binary, numbers, strings, or objects capable of JSON representation. Any other data type must not be permitted and result in an error with code 40013. | ||
| ** @(OD4b)@ When the MessagePack protocol is used: | ||
| *** @(OD4b1)@ A boolean payload is encoded as a MessagePack boolean type, and the result is set on the @ObjectData.boolean@ attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| *** @(OD4b1)@ A boolean payload is encoded as a MessagePack boolean type, and the result is set on the @ObjectData.boolean@ attribute. | |
| *** @(OD4b1)@ A boolean payload is encoded as a MessagePack boolean type, and the result is set on the @ObjectData.boolean@ attribute. The @ObjectData.encoding@ attribute is then set to "msgpack" |
or you can add separate spec point for the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @ObjectData.encoding@ attribute is then set to "msgpack"
It must not be set in this case. Currently the ObjectData.encoding is only set when the payload is being JSON-stringified, in OD4b5 and OD4c5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then we should explicitly specify that it should not be set. Otherwise, it could easily be misinterpreted as to be set to either msgpack or json.
i.e. we set encoding field for OOP5a, OOP5b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then we should explicitly specify that it should not be set
I don't think it's reasonable to list all the things the client library should not do. ObjectData encoding/decoding spec should not be compared to encoding for ObjectOperation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, it shouldn't be compared directly. However, from a reader's perspective, it's reasonable to expect that the encoding field would have a specific value. It's always better to define things explicitly in the spec to avoid any implicit misunderstandings. In this case, the spec doesn't clearly state whether a value should or shouldn't be set—it's rather ambiguous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that the encoding field would have a specific value
I do not agree with this. The spec is explicit in its phrasing that ObjectOperation.initialValueEncoding OOP3i must be set by the client library when sending create ops to the server, while the ObjectData.encoding OD2b may be set to indicate that one of the value fields has an additional encoding (an argument can be made that it shouldn't mention bytes field at the moment, as there is a no procedure for additionally encoding anything that goes into bytes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not currently comparing this with initialValueEncoding; rather, I'm trying to share my understanding from a reader's perspective. Specifically, this spec doesn't clearly state whether a value should or should not be set.
For example, in the case of OD4b5, it explicitly says to set the encoding to json. In other cases, we describe how the value is encoded—for instance, "A string payload is encoded as a MessagePack string type"—but we neither state that the encoding field must be set, nor that it should be left unset.
If we were to feed this spec into a context-driven LLM model, it would likely infer or assume some default value. That’s why I’m suggesting we make this aspect explicit in the spec to avoid ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current encoding spec for ObjectMessage was written in the same format as the encoding spec for regular messages in RSL4, where a spec only specifies when to set the encoding field.
That being said, I don't see a huge issue with adding a single spec point that states that encoding field is left unset by default, unless specified by the encoding procedure. Added it in https://github.com/ably/specification/compare/826e4bddf506d3923dc746edc893591ce78d245d..7f30177752945dae89891c9146109fbfa1dcea08
we describe how the value is encoded—for instance, "A string payload is encoded as a MessagePack string type"—but we neither state that the encoding field must be set, nor that it should be left unset.
I should note that ObjectData.encoding (just like Message.encoding from RSL4) is not meant to tell the client library that the value at the ObjectData.string attribute is string-encoded. It exists to inform the client library that the value represents something else and should be further processed. For example, if client received ObjectData.string with ObjectData.encoding set to json, then the string is actually a JSON string - so if you JSON-parse it, you'll get the original object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ObjectData.encoding is no longer used and will be removed altogether in #360
textile/features.textile
Outdated
| *** @(OD4c1)@ A boolean payload is represented as a JSON boolean and set on the @ObjectData.boolean@ attribute. | ||
| *** @(OD4c2)@ A binary payload is Base64-encoded and represented as a JSON string; the result is set on the @ObjectData.bytes@ attribute. | ||
| *** @(OD4c3)@ A number payload is represented as a JSON number and set on the @ObjectData.number@ attribute. | ||
| *** @(OD4c4)@ A string payload is represented as a JSON string and set on the @ObjectData.string@ attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally ObjectData.encoding should be called ObjectData.isJsonString or ObjectData.isJson that is either set to true or false.
If it's true, we construct JsonObject or JsonArray from the @objectdata.string@ value.
Is it possible to be changed as a part of https://github.com/ably/specification/pull/335/files#r2144849490 changes
wdyt @lmars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ObjectData.encoding in itself feels misleading. When we apply msgpack encoding, ObjectData.encoding is set to either json or none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH if we make a change to the fields here, I'd suggest adding a separate json field which is explicitly used to send a JSON encoded value, rather than using the string field with encoding=json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so field ObjectData.json set to true or false ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sorry, I mean ObjectData.json would be set to the JSON encoded value, for example ObjectData.json = '{"this":"is some json"}'), rather than what would currently be string = '{"this":"is some json"}', encoding = 'json'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, understood, so new ObjectData will look like
class ObjectData // OD*, internal
objectId: String? // OD2a
boolean: Boolean? // OD2c
bytes: Binary? // OD2d
number: Number? // OD2e
string: String? // OD2f
json: String? // OD2g
This also makes logical sense—since the encoding field can be misleading. It implies that the encoding applies to the entire ObjectData( related discussion ), when in fact, it only refers to the encoding of a specific field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmars, I guess this can be addressed as a part of getting rid of initialValueEncoding => #335 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec for ObjectData.encoding and .json will be updated in #360
Based on [1] at 826e4bd. [1] ably/specification#335
2619ee2 to
21dda52
Compare
Based on the spec added in [1] [1] ably/specification#335
sacOO7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, just noticed, both of my msgpack and json were using enum string to send and receive data. Instead it should use code starting from 0, can you add separate spec point for the same?
Why? |
Apologies for the confusion — what I meant is that it should use the underlying |
|
Sorry, I don't think I know what you're referring to. What property are you talking about exactly, and what is the enum that you're referring to? |
Asked about this in standup — @sacOO7 just meant that the spec should be explicit that when sending an enum value (e.g. a |
Based on [1] at 2e975cb. [1] ably/specification#335
Based on [1] at 2e975cb. This was implemented by updating the code to reflect the internal API that I wanted to exist, and then asking Cursor to implement the rules of the spec and to write tests. I then edited the generated code to simplify it a bit and add things like @SPEC annotations and other explanatory comments. I wanted some round-trip tests that go through the Realtime backend but decided to leave them until later once I have a bit more knowledge of the LiveObjects protocol; have created #17. [1] ably/specification#335
Based on the spec added in [1] [1] ably/specification#335
The plugin will use this to apply the spec's rules (see [1]) regarding the encoding and decoding of binary data. This was initially generated by Cursor at my instructions, but I ended up rewriting most of the documentation it generated because it didn't have enough context to understand how the plugin would use the format. [1] ably/specification#335
Also improves the phrasing used for some of the related
2e975cb to
6b14482
Compare
This is the spec equivalent of the encoding/decoding logic implemented in ably-js for ObjectMessage