-
Notifications
You must be signed in to change notification settings - Fork 4
Message/PresenceMessage/Annotation size #331
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
base: main
Are you sure you want to change the base?
Conversation
textile/features.textile
Outdated
| ** @(TM6a)@ The size is the sum of the sizes of the @name@, @data@, @clientId@, and @extras@ properties | ||
| ** @(TM6b)@ The size of an @Object@ or @Array@ @data@ property is its string length after being JSON-stringified | ||
| ** @(TM6c)@ The size of a @binary@ @data@ property is its size in bytes (of the actual binary, not the base64 representation, regardless of whether the binary protocol is in use) | ||
| ** @(TM6f)@ The size of a @string@ @data@ property is its length |
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.
If data has type boolean or number then what should be it's size?
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.
Same will be valid for PresenceMessage and Annotation
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.
Also, couldn't find ably-js impl. for calculating PresenceMessage size, seems it's handled in a generic way as a part of
message.ts#getMessageSize
function getMessageSize(msg: WireMessage) {
let size = 0;
if (msg.name) {
size += msg.name.length;
}
if (msg.clientId) {
size += msg.clientId.length;
}
if (msg.extras) {
size += JSON.stringify(msg.extras).length;
}
if (msg.data) {
size += Utils.dataSizeBytes(msg.data);
}
return size;
}
// TM6b => This seems wrong. : (
// dataSizeBytes only checks for number, boolean, string or binary
export function dataSizeBytes(data: string | number | boolean | Bufferlike): number {
if (Platform.BufferUtils.isBuffer(data)) {
return Platform.BufferUtils.byteLength(data);
}
if (typeof data === 'string') {
return Platform.Config.stringByteSize(data);
}
if (typeof data === 'number') {
return 8;
}
if (typeof data === 'boolean') {
return 1;
}
throw new Error(
`Expected input of Utils.dataSizeBytes to be a string, a number, a boolean or a buffer, but was: ${typeof data}`,
);
}
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.
Calculation for msg.data seems wrong, because as per TM6b => The size of an @Object@ or @Array@ @data@ property is its string length after being JSON-stringified
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.
If data has type boolean or number then what should be it's size?
This is not an accepted data type for regular Messages or PresenceMessage.
Don't know about Annotation messages, but the message size calculation for it should be discussed in a separate conversation below.
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.
Calculation for msg.data seems wrong, because as per TM6b => The size of an @object@ or @array@ @DaTa@ property is its string length after being JSON-stringified
It is calculated correctly. The message size is calculated on the encoded (and encrypted) data, as it requires data to be represented as a string or buffer:
https://github.com/ably/ably-js/blob/d55afe78340f1f16d17f7ec9eeffc15bf6563667/src/common/lib/types/message.ts#L91-L93
That said, I can see the spec is missing the part that the size calculation should take place after encoding and encryption. This is something that should be added.
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.
If data field after encoding is either string or buffer, then we don't need TM6b right
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.
Uhhh, it seems like it's not that straightforward.
Based on the ably-js implementation comment, the message should be encoded and encrypted before calculating its size (which happens here). However, it should not apply protocol-specific encoding - such as representing binary data as a Base64 string in the JSON protocol (TM6c). That kind of encoding takes place at a later stage in ably-js, after message size calculation.
So currently, the spec for message size calculation kind of dances around this logic. It says that Object and Array data should be JSON-stringified in TM6b (per RSL4c3 and RSL4d3), but states that Base64 encoding should not be applied for binary data (RSL4d1 should not apply). At the same time, it doesn’t mention that the data should be encrypted (if applicable) before size calculation if client was configured to do so.
Ideally, all of these nuances should be clarified and stated explicitly in the spec for message size calculation. However, I’m not sure this should be our priority right now. Based on Simon’s comment below, it’s not a requirement to have this implemented for Annotations, and we haven’t planned to update this for Message and PresenceMessage.
Would opening a separate issue that outlines the scope of the needed changes suffice in this case?
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.
Created separate issue #336, meanwhile you can review updated spec PR.
3824174 to
e328e38
Compare
e328e38 to
f6ac11d
Compare
f6ac11d to
0891737
Compare
2. Added spec for calculating Presence message size 3. Added spec for calculating Annotation message size
0891737 to
5419c41
Compare
textile/features.textile
Outdated
| ** @(TM6a)@ The size is the sum of the sizes of the @name@, @data@, @clientId@, and @extras@ properties | ||
| ** @(TM6b)@ The size of an @Object@ or @Array@ @data@ property is its string length after being JSON-stringified | ||
| ** @(TM6c)@ The size of a @binary@ @data@ property is its size in bytes (of the actual binary, not the base64 representation, regardless of whether the binary protocol is in use) | ||
| ** @(TM6f)@ The size of a @string@ @data@ property is its length |
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.
If data has type boolean or number then what should be it's size?
This is not an accepted data type for regular Messages or PresenceMessage.
Don't know about Annotation messages, but the message size calculation for it should be discussed in a separate conversation below.
textile/features.textile
Outdated
| ** @(TM6a)@ The size is the sum of the sizes of the @name@, @data@, @clientId@, and @extras@ properties | ||
| ** @(TM6b)@ The size of an @Object@ or @Array@ @data@ property is its string length after being JSON-stringified | ||
| ** @(TM6c)@ The size of a @binary@ @data@ property is its size in bytes (of the actual binary, not the base64 representation, regardless of whether the binary protocol is in use) | ||
| ** @(TM6f)@ The size of a @string@ @data@ property is its length |
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.
Calculation for msg.data seems wrong, because as per TM6b => The size of an @object@ or @array@ @DaTa@ property is its string length after being JSON-stringified
It is calculated correctly. The message size is calculated on the encoded (and encrypted) data, as it requires data to be represented as a string or buffer:
https://github.com/ably/ably-js/blob/d55afe78340f1f16d17f7ec9eeffc15bf6563667/src/common/lib/types/message.ts#L91-L93
That said, I can see the spec is missing the part that the size calculation should take place after encoding and encryption. This is something that should be added.
textile/features.textile
Outdated
| ** @(TM6f)@ The size of a @string@ @data@ property is its UTF-8 encoded length in bytes (because of Non-ASCII characters) | ||
| ** @(TM6g)@ The size of a @string@ @name@ property is its length | ||
| ** @(TM6h)@ The size of a @string@ @clientId@ property is its length | ||
| ** @(TM6d)@ The size of the @extras@ property is the string length of its JSON representation |
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.
We don't have Non-ASCII characters included in extras right
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.
we definitely can, they can contain arbitrary data, eg in a push payload
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.
Thanks will update for all string spec points 👍
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.
updated
textile/features.textile
Outdated
| ** @(TM6c)@ The size of a @binary@ @data@ property is its size in bytes (of the actual binary, not the base64 representation, regardless of whether the binary protocol is in use). | ||
| ** @(TM6f)@ The size of a @string@ @data@ property is its UTF-8 encoded length in bytes (because of Non-ASCII characters) | ||
| ** @(TM6g)@ The size of a @string@ @name@ property is its length | ||
| ** @(TM6h)@ The size of a @string@ @clientId@ property is its length |
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.
There's no reason that name, clientId, data, and json-encoded-extras strings should use different calculations for their sizes. If we want to specify "UTF-8 encoded length in bytes" for a string data (which I agree is the correct interpretation, we want to bound the encoded size over the wire), we should use that same wording on the others for consistency.
(if ably-js doesn't do this that's technically a bug, though not one that matters much. if the difference changes the result then it wasn't grossy over the limit and it can just get rejected by the server, shrug)
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 it will be better to be specific for each spec point 👍
I guess we will need to update all string specific spec points.
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.
updated
97aade3 to
f540477
Compare
f540477 to
3ed8569
Compare
Extension to https://github.com/ably/specification/pull/326/files
Added a clear specification to eliminate ambiguity when calculating sizes for each Message type.
This also aids in annotating the code with the correct specification.
Link to internal conversation => https://ably-real-time.slack.com/archives/C03JDBVM5MY/p1749642052882309