[DTP-1118] Apply ConnectionDetails.maxMessageSize limit to state messages
#1963
Conversation
|
Warning Rate limit exceeded@VeskeR has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughThis pull request introduces enhancements to message size handling and encoding across multiple files in the Ably Realtime library. The changes focus on implementing message size validation, particularly for state messages, by adding new methods to calculate message sizes, updating error handling, and extending utility functions to support more data types. The modifications aim to improve message transmission reliability by enforcing size limits and providing more detailed error information. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1963/features
github-actions
bot
temporarily deployed
to
staging/pull/1963/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1963/typedoc
|
@mschristensen Please let me know if these two changes are in-line with your idea for DTP-1136:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
test/realtime/live_objects.test.js (7)
75-76: State message creator function
stateMessageFromValues()is a neat helper to generateStateMessageinstances. If used extensively, consider adding validations (e.g., type checks) for the inputvaluesobject to preempt unexpected runtime errors.
458-459: Repeated buffer instrumentation callsRepeated calls to the private API for buffer operations confirm consistent usage. Consider grouping them if code duplication occurs frequently in extended tests.
919-919: Context object destructuring repeatedThis pattern is repeated in multiple scenarios. Consider a single utility function that returns the context to reduce repetition if more logic is needed in the future.
946-947: Instrumenting buffer equality checksConsistent with other instrumentation lines. Keep an eye on performance impact if large amounts of data are tested in the future.
2073-2074: Ensuring up-to-date references when tests runAfter buffering operations, code references the root object again. Consider verifying that references to local variables do not become stale if concurrency is introduced.
2302-2302: Expanding test coverage on edge casesLine 2302 indicates we set up primitive data in the root map. Consider adding extreme edge cases (e.g., zero-length strings, extremely large strings, null bytes) to solidify coverage.
2661-2662: Multiple calls to BufferUtilsFrequent usage is probably expected in these tests. If performance becomes an issue, caching or grouping tests might help.
src/common/lib/util/utils.ts (1)
282-298: LGTM! Consider adding JSDoc comments.The function has been correctly extended to handle additional data types with appropriate size calculations. The error message is clear and descriptive.
Consider adding JSDoc comments to document the function's purpose, parameters, return value, and the size calculation rules for each data type:
+/** + * Calculates the size in bytes of the input data. + * @param data The input data to calculate the size for + * @returns The size in bytes: + * - For strings: Uses platform-specific string byte size calculation + * - For numbers: Returns 8 bytes + * - For booleans: Returns 1 byte + * - For Buffer/ArrayBuffer: Returns the buffer length + * @throws Error if the input type is not supported + */ export function dataSizeBytes(data: string | number | boolean | Buffer | ArrayBuffer): number {src/plugins/liveobjects/statemessage.ts (2)
453-464: LGTM! Consider using reduce for clarity.The method correctly calculates the map size, but could be more concise using reduce.
Consider using reduce for a more functional approach:
private _getStateMapSize(map: StateMap): number { - let size = 0; - - Object.entries(map.entries ?? {}).forEach(([key, entry]) => { - size += key?.length ?? 0; - if (entry) { - size += this._getStateMapEntrySize(entry); - } - }); - - return size; + return Object.entries(map.entries ?? {}).reduce( + (size, [key, entry]) => + size + (key?.length ?? 0) + (entry ? this._getStateMapEntrySize(entry) : 0), + 0 + ); }
474-482: LGTM! Consider simplifying the implementation.The method correctly calculates the entry size, but could be more concise.
Consider simplifying the implementation:
private _getStateMapEntrySize(entry: StateMapEntry): number { - let size = 0; - - if (entry.data) { - size += this._getStateDataSize(entry.data); - } - - return size; + return entry.data ? this._getStateDataSize(entry.data) : 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/common/lib/client/defaultrealtime.ts(2 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/restchannel.ts(1 hunks)src/common/lib/util/utils.ts(1 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)src/plugins/liveobjects/statemessage.ts(1 hunks)test/common/modules/private_api_recorder.js(2 hunks)test/realtime/live_objects.test.js(16 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/common/lib/client/realtimechannel.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (29)
test/realtime/live_objects.test.js (14)
12-13: Use of Ably Realtime internal propertiesUsing
Ably.Realtime.UtilsandAbly.Realtime._MessageEncodingaligns with the new message encoding logic. However,_MessageEncodingis underscored, hinting it's an internal property. If it's stable and intentionally exposed, consider renaming or documenting so it’s clear it's officially supported.
63-73: Ensure robust error validation in async testsReplacing the boolean flag with
savedErrorclarifies error handling. This block might benefit from validating the entire error object, e.g., verifying error code, stack trace, or additional fields where relevant.
449-450: Buffer decoding instrumentationRecording private API calls for buffer decoding fosters better test coverage and debugging capabilities. No further changes suggested.
703-703: Clarify scenario context parameterDestructuring
{ root, liveObjectsHelper, channelName, helper }helps keep code readable. Ensure the context is well-documented so new team members understand each property’s usage and scope in these test scenarios.
744-745: Helper methods for buffer comparisonsSimilar to lines 449-450, these calls are consistent. No issues found.
1804-1804: Buffered operation application in testsLine 1804 logs the test arrangement for applying buffered operations after STATE_SYNC. The approach is clear, and the test is thorough.
1833-1834: Private API usage for buffer checksNo concerns. Maintains consistency with prior lines.
2034-2034: Manage scenario context meticulouslyWhen injecting additional state operations, always confirm the channel state is valid, especially under concurrency scenarios or potential race conditions.
2306-2306: Verifying base64 decode callsCoordinates well with line 2302’s setup. No immediate issues.
2315-2316: Further instrumentation on buffer operationsAcross the file, instrumentation is consistent. This approach is suitable for diagnosing issues quickly without cluttering production code.
2628-2628: Creating maps with initial valuesThis code sets the stage for thorough testing of
LiveMapobjects. Consider logging or validating final states to catch any silent logic regressions.
2634-2634: Handling base64-encoded values upon map creationNo issues. If additional encodings are introduced, ensure they’re handled consistently across the entire codebase.
3567-3614: Test for enforcing max message sizeGreat scenario verifying that an error is thrown when exceeding
connectionDetails.maxMessageSize. This ensures state message publish adheres to the size limit. The error code40009is well-defined. Consider adding boundary tests at exactly the size limit to confirm off-by-one correctness.
3616-3789: Comprehensive tests for message size calculationsThis block thoroughly validates data size measurement across different object types (maps, counters, booleans, etc.). The coverage is high. For maintainability, ensure this remains in sync if the underlying size calculation logic evolves.
src/common/lib/client/defaultrealtime.ts (2)
22-22: Introducing MessageEncoding importThe newly imported
MessageEncodingexpands the encoding handling. Confirm that no existing logic depends on older encoding constants, and ensure this seamlessly integrates with_MessageEncoding.
75-75: New static property: _MessageEncodingExporting
_MessageEncodingfor external use is helpful for testing. If it’s truly part of the public API, consider removing the underscore prefix; otherwise, it’s best to clarify through documentation that this is intended for internal use/testing only.test/common/modules/private_api_recorder.js (3)
19-19: Tracking LiveObject getObjectIdNewly tracking
'call.LiveObject.getObjectId'in the recorder ensures deeper introspection of object IDs. No concerns unless performance overhead arises from many calls.
29-31: StateMessage-related instrumentationAdding
'call.StateMessage.encode','call.StateMessage.fromValues', and'call.StateMessage.getMessageSize'extends your private API recording. This ensures test coverage for newly introduced logic inStateMessageand helps identify usage patterns.
33-33: Expanded coverage for dataSizeBytesCapturing
'call.Utils.dataSizeBytes'is consistent with the broader instrumentation strategy. No immediate issues found.src/plugins/liveobjects/liveobjects.ts (1)
226-234: LGTM! Size validation is correctly implemented.The implementation correctly validates the total size of state messages before publishing:
- Retrieves the maximum size limit from client options
- Calculates total size by summing individual message sizes
- Throws a descriptive error if the limit is exceeded
src/plugins/liveobjects/statemessage.ts (8)
401-416: LGTM! Message size calculation is comprehensive.The method correctly calculates the total message size by:
- Summing the sizes of all relevant fields
- Handling optional fields safely
- Delegating complex calculations to specialized methods
418-435: LGTM! State operation size calculation is well-structured.The method correctly calculates the operation size by delegating to specialized methods for each field type.
437-451: LGTM! State object size calculation is well-structured.The method correctly calculates the object size by delegating to specialized methods for each field type.
466-472: LGTM! Counter size calculation is correct.The method correctly returns:
- 8 bytes for a defined count (number size)
- 0 bytes for undefined/null count
484-494: LGTM! Map operation size calculation is correct.The method correctly calculates the size by:
- Including the key length (if present)
- Adding the data size (if present)
496-502: LGTM! Counter operation size calculation is correct.The method correctly returns:
- 8 bytes for a defined amount (number size)
- 0 bytes for undefined/null amount
504-515: LGTM! State data size calculation is correct.The method correctly calculates the size by:
- Including the objectId size using dataSizeBytes
- Adding the value size using specialized method
517-519: LGTM! State value size calculation is correct.The method correctly uses the enhanced dataSizeBytes utility which supports all StateValue types.
src/common/lib/client/restchannel.ts (1)
120-120: LGTM! Error message readability improved.The error message has been updated to use template literals, making it more readable while maintaining the same information.
VeskeR
force-pushed
the
DTP-1118/apply-maxMessageSize-for-state
branch
from
f48f2af to
908ddae
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1963/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1963/features
github-actions
bot
temporarily deployed
to
staging/pull/1963/typedoc
|
Removed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/plugins/liveobjects/statemessage.ts (3)
400-416: Consider using_utils.dataSizeBytes()for consistency when measuring clientId and extrasCurrently,
clientIdis measured withthis.clientId?.length, andextrasis measured viaJSON.stringify(this.extras).length. Meanwhile, other fields (e.g., map data, counters) use_utils.dataSizeBytes(...). For coherence and to correctly handle multi-byte characters and buffer data, consider measuring these fields using_utils.dataSizeBytes(...)as well.
466-472: Validate fixed 8-byte assumption for counters
_getStateCounterSizereturns8ifcount != null. This implies all counter values, regardless of magnitude, are stored as 8 bytes (often a float64). Confirm that this is consistent with how counters are serialized elsewhere in the system.
484-494: Handle references in_getStateMapOpSizeThis method calculates the size of
mapOp.keyand, when present, the data’s value. IfmapOp.data?.objectIdis set, confirm it should be excluded or included. The PR suggests possibly counting object references in some contexts.test/realtime/live_objects.test.js (1)
449-450: Instrumentation calls to record private API usageThese lines log calls to
BufferUtilsfunctions and other internals. Ensure these logging statements don’t impair performance or clutter logs in production.Also applies to: 458-459, 946-947, 1833-1834, 2073-2074, 2302-2306, 2315-2316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/common/lib/client/defaultrealtime.ts(2 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)src/plugins/liveobjects/statemessage.ts(1 hunks)test/common/modules/private_api_recorder.js(2 hunks)test/realtime/live_objects.test.js(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/lib/client/defaultrealtime.ts
- test/common/modules/private_api_recorder.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (13)
src/plugins/liveobjects/statemessage.ts (6)
418-435: Verify omission ofobjectId,nonce, and initial values in_getStateOperationSizeThe current size calculation omits
operation.objectId,nonce,initialValue, andinitialValueEncoding. The PR objectives mention excluding object IDs in most cases except potentially for references (e.g., MAP_SET referencing another object). Confirm that these fields are indeed intentionally omitted or if you need to incorporate them under specific conditions.
437-451: Check for consistency when omittingobjectIdin_getStateObjectSize
_getStateObjectSizeincludes map, counter, and createOp fields but does not account forobj.objectId. According to the PR discussion, object IDs are generally excluded from the size calculation, which may be correct. Ensure this is fully aligned with the updated design.
453-464: Confirm ignoringsemanticsin_getStateMapSize
_getStateMapSizeskipsmap.semantics. If this property was discussed for exclusion from size calculations, that’s fine; just verify it aligns with the requirements from [DTP-1136].
474-482: Tombstone consideration in_getStateMapEntrySize
_getStateMapEntrySizeonly accounts forentry.data. If tombstone or timeserial fields must be included in the size count, ensure that skipping them is intentional.
496-502: Fixed 8-byte assumption for_getStateCounterOpSizeSimilar to
_getStateCounterSize, this returns a fixed 8 bytes for anyamount. Confirm that the numeric type is consistently stored as an 8-byte float or if you need more nuanced handling (e.g., big integer scenarios).
504-516: Confirm ignoringobjectIdin_getStateDataSize
_getStateDataSizefocuses ondata.value, calling_getStateValueSize(value). Ifdata.objectIdis present, it’s not counted. Verify this aligns with the latest decision to exclude object IDs except in certain MAP_SET references.src/plugins/liveobjects/liveobjects.ts (1)
226-234: Verify undefined or zero edge cases when enforcingmaxMessageSizeThe code throws an error if
size > maxMessageSize, assumingmaxMessageSizeis a positive number. Ifthis._client.options.maxMessageSizeis missing or zero, a publish may fail unexpectedly (e.g.,size > undefinedis false, butsize > 0is always true if size is positive). Consider short-circuiting if this value is not set or is invalid.test/realtime/live_objects.test.js (6)
12-13: Imports forUtilsandMessageEncodinglook goodThe references to
Ably.Realtime.UtilsandAbly.Realtime._MessageEncodingare well-placed.
63-73: Enhanced error-check helperThis new
expectRejectedWith(fn, errorStr)utility improves test readability by returning the captured error. Consider expanding it to validate error codes or properties if needed.
75-76: Convenient factory forStateMessage
stateMessageFromValuesneatly wrapsStateMessage.fromValues. This should simplify test logic.
3568-3571: Coverage references in code commentsReferences to
@spec TO3l8and@spec RSL1iare helpful for traceability of specification coverage. No issues spotted.
3572-3614: Test forconnectionDetails.maxMessageSizelimitThis test thoroughly validates that exceeding the configured limit yields the correct error code and message. Good job ensuring the code 40009 is checked.
3616-3837: Comprehensive tests forStateMessage.getMessageSizeThe scenario-driven approach covers various data types, verifying that
getMessageSizeignores object IDs and handles strings, buffers, booleans, and numbers. This is excellent coverage.
VeskeR
force-pushed
the
DTP-1118/apply-maxMessageSize-for-state
branch
from
908ddae to
7206725
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1963/features
…d more generic Buffer type This is in preparation for message size calculation for StateMessages, which can have numbers and booleans as user provided data [1]. Also should use platform agnostic `Bufferlike` type provided by the Platform module to have better support for node.js/browser buffers. [1] https://ably.atlassian.net/browse/DTP-1118
…messages Resolves DTP-1118
VeskeR
force-pushed
the
DTP-1118/apply-maxMessageSize-for-state
branch
from
7206725 to
187473e
Compare
Message size calculation algorithm is based on https://github.com/ably/realtime/blob/d0557b715a5dbace6dce456b14127eea75d6f019/go/realtime/lib/ablyrpc/state.go#L452, with changes:
MAP_SEToperation is referencing another object (TBD)createOpin the object messages also contributes to the message size (TBD)Message size calculation tests are based on https://github.com/ably/realtime/blob/d0557b715a5dbace6dce456b14127eea75d6f019/nodejs/realtime/test/unit/encoding.ts#L1055
Resolves DTP-1118
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests