[DTP-1033] Add LiveObjects edit API #1948
Conversation
This reduces code duplication and prepares for the changes in next commits where a StateMessage from LiveObjects plugin will be using these encoding functions to encode its state.
…a `MessageEncoding`
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ast-grep (0.31.1)test/realtime/live_objects.test.jsAn unexpected error occurred while running ast-grep. WalkthroughThis pull request introduces enhancements to the message encoding and state management system across multiple files. Key modifications include the addition of a new Changes
Assessment against linked issues
Possibly related PRs
Poem
✨ Finishing Touches
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/1948/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1948/features
github-actions
bot
temporarily deployed
to
staging/pull/1948/typedoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
src/plugins/liveobjects/statemessage.ts (1)
165-179: Simplify Null Checks with Optional ChainingConsider using optional chaining to simplify the assignment of
message.operationandmessage.object.Apply this diff to refactor the code:
- message.operation = message.operation ? StateMessage._encodeStateOperation(message.operation, encodeFn) : undefined; - message.object = message.object ? StateMessage._encodeStateObject(message.object, encodeFn) : undefined; + message.operation = StateMessage._encodeStateOperation(message.operation, encodeFn); + message.object = StateMessage._encodeStateObject(message.object, encodeFn);Since the
_encodeStateOperationand_encodeStateObjectmethods likely handleundefinedinputs appropriately, this refactor simplifies the code.src/common/lib/types/message.ts (1)
171-181: Guard Against Null Cipher OptionsIn the
encodefunction, the checkif (cipherOptions != null && cipherOptions.cipher)may not properly handle all falsy values. Consider simplifying the condition.Apply this diff to improve the condition:
- if (cipherOptions != null && cipherOptions.cipher) { + if (cipherOptions?.cipher) { return encrypt(msg, cipherOptions); } else { return msg; }This uses optional chaining to make the code more concise.
🧰 Tools
🪛 Biome (1.9.4)
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/plugins/liveobjects/livemap.ts (2)
522-522: Update Log Message with Correct Object IDIn the
_applyMapCreatemethod, correct the object ID reference in the log message to provide accurate debugging information.Apply this diff to fix the log message:
`skipping applying MAP_CREATE op on a map instance as it was already applied before; objectId=${this.getObjectId()}`Ensure that the object ID is correctly included in the message.
601-601: Consistent Logging in_applyMapRemoveMethodIn the
_applyMapRemovemethod, ensure that the logging is consistent with other methods and includes all relevant information for debugging.Verify and adjust the log message as necessary.
src/common/lib/types/presencemessage.ts (1)
137-141: Consider removing the non-null assertion operator.The code assumes encoding will always be defined by using the non-null assertion operator (
!). Consider handling the undefined case explicitly for better type safety.- encoding: encoding!, + encoding: encoding ?? '',Also applies to: 151-155
src/common/lib/types/protocolmessage.ts (1)
183-185: Consider using array join for better performance.The string concatenation could be optimized by using array join instead of string concatenation.
- result += - '; state=' + toStringArray(liveObjectsPlugin.StateMessage.fromValuesArray(msg.state, Utils, MessageEncoding)); + const stateArray = liveObjectsPlugin.StateMessage.fromValuesArray(msg.state, Utils, MessageEncoding); + result += ['; state=', toStringArray(stateArray)].join('');src/plugins/liveobjects/livecounter.ts (2)
53-74: Consider adding error message constants.The error message and code are hardcoded. Consider extracting these to constants for better maintainability and reusability.
+const COUNTER_INCREMENT_ERROR = { + message: 'Counter value increment should be a number', + code: 40013, + statusCode: 400 +}; createCounterIncMessage(amount: number): StateMessage { if (typeof amount !== 'number') { - throw new this._client.ErrorInfo('Counter value increment should be a number', 40013, 400); + throw new this._client.ErrorInfo(COUNTER_INCREMENT_ERROR.message, COUNTER_INCREMENT_ERROR.code, COUNTER_INCREMENT_ERROR.statusCode); } // ... rest of the code
76-87: Consider reusing error constants for decrement.The error message for decrement is similar to increment. Consider reusing the same error constants with a different message.
+const COUNTER_DECREMENT_ERROR = { + message: 'Counter value decrement should be a number', + code: 40013, + statusCode: 400 +}; decrement(amount: number): Promise<void> { if (typeof amount !== 'number') { - throw new this._client.ErrorInfo('Counter value decrement should be a number', 40013, 400); + throw new this._client.ErrorInfo(COUNTER_DECREMENT_ERROR.message, COUNTER_DECREMENT_ERROR.code, COUNTER_DECREMENT_ERROR.statusCode); } return this.increment(-amount); }src/plugins/liveobjects/liveobjects.ts (1)
153-168: Consider adding batch size validation.The method accepts an array of state messages but doesn't validate the batch size. Consider adding a maximum batch size check to prevent memory issues with large batches.
+const MAX_BATCH_SIZE = 100; publish(stateMessages: StateMessage[]): Promise<void> { + if (stateMessages.length > MAX_BATCH_SIZE) { + throw this._client.ErrorInfo.fromValues({ + message: `Maximum batch size exceeded (was ${stateMessages.length}; limit is ${MAX_BATCH_SIZE})`, + code: 40009, + statusCode: 400 + }); + } if (!this._channel.connectionManager.activeState()) { throw this._channel.connectionManager.getError(); } // ... rest of the codesrc/common/lib/client/realtimechannel.ts (1)
629-629: Consider adding error handling for missing plugin.The ternary operation could throw if the plugin is missing. Consider adding a more descriptive error message.
- ? this.client._LiveObjectsPlugin.StateMessage.decode(msg, options, MessageEncoding) - : Utils.throwMissingPluginError('LiveObjects'), + ? this.client._LiveObjectsPlugin.StateMessage.decode(msg, options, MessageEncoding) + : Utils.throwMissingPluginError('LiveObjects', 'Cannot decode state message without LiveObjects plugin'),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/common/lib/client/baseclient.ts(2 hunks)src/common/lib/client/realtimechannel.ts(3 hunks)src/common/lib/transport/protocol.ts(1 hunks)src/common/lib/types/message.ts(6 hunks)src/common/lib/types/presencemessage.ts(3 hunks)src/common/lib/types/protocolmessage.ts(3 hunks)src/plugins/liveobjects/livecounter.ts(3 hunks)src/plugins/liveobjects/livemap.ts(5 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)src/plugins/liveobjects/statemessage.ts(5 hunks)test/realtime/live_objects.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/plugins/liveobjects/livecounter.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1897
File: src/plugins/liveobjects/livecounter.ts:93-93
Timestamp: 2024-11-12T07:31:53.691Z
Learning: In the `LiveCounter` class's `_applyCounterCreate` method, it's intentional to increment the counter's value using `+=` instead of initializing it with `=` because the counter may have a pre-existing non-zero value.
🪛 Biome (1.9.4)
src/common/lib/types/message.ts
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (21)
src/plugins/liveobjects/statemessage.ts (4)
1-2: Import Consistency: Ensure Correct PathsVerify that the import paths for
MessageEncodingandUtilsare correct and accessible in the project structure to prevent any module resolution issues.
5-8: Type Definition ClarityThe
StateDataEncodeFunctiontype is well-defined and clear. This enhances code readability and maintainability.
154-157: Constructor Parameter UpdateUpdating the constructor to accept
_utilsand_messageEncodingpromotes better dependency injection and modularity.
250-254: Ensure Proper Error Handling in Data DecodingIn the
_decodeStateDatamethod, after decoding the data, ensure that any errors are appropriately handled to prevent potential runtime exceptions.src/common/lib/types/message.ts (4)
134-137: Parameter Renaming for ClarityRenaming the parameter from
optionstocipherOptionsin theencryptfunction enhances readability by making the purpose of the parameter clear.
146-162: Proper Handling of Encoding StringsEnsure that the encoding string concatenation correctly handles cases where
encodingisnullor an empty string to prevent leading or trailing slashes.Test with different values of
encodingto confirm the correctness of the concatenated encoding string.
220-247: Handle Binary Data Consistently Across ProtocolsIn
encodeDataForWireProtocol, ensure that binary data is handled consistently for both JSON and MsgPack protocols, and that any required transformations are correctly applied.Review the handling of binary data to confirm that it adheres to protocol specifications.
445-451: ExposeMessageEncodingAppropriatelyExporting
MessageEncodingprovides a convenient way to access encoding functions, but ensure that this does not inadvertently expose internal implementations or create security risks.Review the accessibility of the encoding functions to confirm that they are intended for public use.
src/plugins/liveobjects/livemap.ts (4)
173-209: Validate Map Key and Value TypesThe
createMapSetMessagemethod correctly validates the key and value types, which is essential for data integrity.
228-246: Validate Map Key Type increateMapRemoveMessageThe validation of the key in
createMapRemoveMessageis appropriate, ensuring only string keys are accepted.
548-548: Ensure Correct Timeserial ComparisonIn the
_applyMapSetmethod, verify that the timeserial comparison logic accurately determines whether to apply the operation, accounting for potential edge cases.Review the timeserial comparison to confirm that it's consistent with the desired CRDT semantics.
267-267: Correct Log Message FormattingIn the
applyOperationmethod, ensure that template literals are used correctly to avoid any runtime errors.Check line 267 for any formatting issues in the log message.
Apply this diff if needed:
`skipping ${op.action} op: op timeserial ${opOriginTimeserial.toString()} <= site timeserial ${this._siteTimeserials[opSiteCode]?.toString()}; objectId=${this.getObjectId()}`Ensure that all variables used in the template string are correctly defined.
src/common/lib/transport/protocol.ts (1)
23-24: LGTM! Enhanced ack requirement logic.The changes improve type safety and readability while extending acknowledgment support to state messages.
src/common/lib/types/presencemessage.ts (1)
3-9: LGTM! Clean import organization.The imports are well-organized and properly grouped.
src/common/lib/client/baseclient.ts (1)
21-21: LGTM! Consistent export pattern.The MessageEncoding export follows the established pattern for exposing utilities to plugins.
Also applies to: 185-185
src/common/lib/types/protocolmessage.ts (1)
6-10: LGTM! Clean import organization.The imports are well-organized and properly grouped.
src/plugins/liveobjects/livecounter.ts (1)
39-51: LGTM! Well-documented increment method.The implementation is clean and follows the async pattern correctly. The documentation clearly explains that the change is applied when the operation is echoed back.
src/common/lib/client/realtimechannel.ts (1)
514-523: LGTM! Clean implementation of state message sending.The implementation follows the promise pattern correctly and uses the protocol message construction appropriately.
test/realtime/live_objects.test.js (3)
2063-2089: LGTM! Comprehensive test coverage for LiveCounter increment.The test cases cover a good range of values including edge cases like MAX_SAFE_INTEGER.
2102-2124: LGTM! Thorough input validation testing.The test cases cover all possible invalid input types for increment operation.
2343-2348: LGTM! Well-organized test scenarios.The test scenarios are well-organized into logical groups making the test suite maintainable.
VeskeR
force-pushed
the
liveobjects/object-mutation-api
branch
from
2412b3f to
6b66b7c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1948/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1948/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1948/features
VeskeR
force-pushed
the
liveobjects/object-mutation-api
branch
from
6b66b7c to
c986912
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1948/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1948/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1948/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/realtime/live_objects.test.js (1)
2074-2374: LGTM! Comprehensive test coverage for write API operations.The test scenarios thoroughly cover:
- LiveCounter increment/decrement operations
- LiveMap set/remove operations
- Error handling for invalid inputs
- Edge cases with various numeric values
Consider adding these test cases to improve coverage:
- For LiveCounter operations:
- Test with
Number.MIN_SAFE_INTEGER- Test with floating-point numbers
- Test with
Infinityand-Infinity- Test with
NaN- For LiveMap operations:
- Test with empty string keys
- Test with very long string keys
- Test setting circular references
- Test concurrent set/remove operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/realtime/live_objects.test.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (2)
test/realtime/live_objects.test.js (2)
60-69: LGTM! Well-structured error verification helper.The
expectRejectedWithhelper function is well-implemented with proper error handling and verification. It will be useful for testing error cases in the write API.
2378-2383: LGTM! Clean integration of write API scenarios.The write API scenarios are well-integrated into the existing test suite structure using the spread operator for clean array composition.
| expectedCounterValue += increment; | ||
| await counter.increment(increment); | ||
|
|
||
| expect(counter.value()).to.equal( |
There was a problem hiding this comment.
It looks like this could flake if the echoed message hasn't been received by the time we call value()?
There was a problem hiding this comment.
Yes, it can. Actually, there are several tests that can flake because they depend on a STATE message being received before checking the value. Now that the client implements all APIs (create/write/subscribe/access), I think it's time to refactor and fix the flakiness in all the tests. However, this will likely be a substantial change on its own.
I've created DTP-1147 to address the flakiness of tests that depend on a STATE message in a separate PR, if that's alright with you.
requested |
VeskeR
force-pushed
the
liveobjects/object-mutation-api
branch
from
c986912 to
2db42fc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1948/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1948/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1948/features
VeskeR
force-pushed
the
liveobjects/object-mutation-api
branch
from
2db42fc to
f8563cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/plugins/liveobjects/liveobjects.ts (3)
153-155: Add JSDoc description for thepublishmethod.The
@internaltag is present but there's no description of what the method does, its parameters, or return value. This would help other developers understand the method's purpose and usage.Add a description like:
/** * @internal + * Publishes state messages to the channel after encoding them. + * @param stateMessages - Array of state messages to publish + * @returns Promise that resolves when messages are sent + * @throws Error if connection is inactive or channel state is invalid */
157-163: Consider consolidating channel state checks.The code checks connection state and channel state separately. Consider combining these checks for better maintainability and to ensure all invalid states are handled consistently.
- if (!this._channel.connectionManager.activeState()) { - throw this._channel.connectionManager.getError(); - } - - if (this._channel.state === 'failed' || this._channel.state === 'suspended') { - throw this._client.ErrorInfo.fromValues(this._channel.invalidStateError()); - } + const channelError = this._validateChannelState(); + if (channelError) { + throw channelError; + } // Add this private method: + private _validateChannelState(): Error | null { + if (!this._channel.connectionManager.activeState()) { + return this._channel.connectionManager.getError(); + } + if (this._channel.state === 'failed' || this._channel.state === 'suspended') { + return this._client.ErrorInfo.fromValues(this._channel.invalidStateError()); + } + return null; + }
165-165: Consider batch encoding optimization.The
forEachloop creates a new function for each iteration. For large arrays, consider using a traditional for loop or adding batch encoding support toStateMessage.encode.- stateMessages.forEach((x) => StateMessage.encode(x, this._client.MessageEncoding)); + for (let i = 0; i < stateMessages.length; i++) { + StateMessage.encode(stateMessages[i], this._client.MessageEncoding); + }Or better, add batch support:
// In StateMessage class: static encodeBatch(messages: StateMessage[], encoding: MessageEncoding): void { for (let i = 0; i < messages.length; i++) { StateMessage.encode(messages[i], encoding); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/plugins/liveobjects/livecounter.ts(3 hunks)src/plugins/liveobjects/livemap.ts(5 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)test/realtime/live_objects.test.js(3 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- src/plugins/liveobjects/livecounter.ts
- src/plugins/liveobjects/livemap.ts
- test/realtime/live_objects.test.js
🧰 Additional context used
📓 Learnings (1)
src/plugins/liveobjects/livecounter.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1897
File: src/plugins/liveobjects/livecounter.ts:93-93
Timestamp: 2024-11-12T07:31:53.691Z
Learning: In the `LiveCounter` class's `_applyCounterCreate` method, it's intentional to increment the counter's value using `+=` instead of initializing it with `=` because the counter may have a pre-existing non-zero value.
🪛 Biome (1.9.4)
src/plugins/liveobjects/livecounter.ts
[error] 57-57: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
(lint/suspicious/noGlobalIsFinite)
[error] 82-82: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
(lint/suspicious/noGlobalIsFinite)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (1)
src/plugins/liveobjects/liveobjects.ts (1)
167-167: Verify error handling in sendState.The method awaits
this._channel.sendState()but doesn't catch potential errors. Ensure thatsendStatehas proper error handling or document that errors should be handled by the caller.
Adds common parts to support the write API for LiveObjects and implements the edit API part of the DTP-1033 (map set/remove, counter increment)
@lawrence-forooghian @ttypic Tagged you to review the encoding changes made in
common/libfiles (baseclient,realtimechanneland*messagefiles). You can skip reviewing the LiveObjects changes as they will be missing context for you.Resolves DTP-1033
Summary by CodeRabbit
Summary by CodeRabbit
New Features
sendStatemethod toRealtimeChannelfor sending state messages.incrementanddecrementmethods forLiveCounter.setandremovemethods forLiveMap.publishmethod inLiveObjectsfor state message publishing.Improvements
BaseClientwithMessageEncoding.Bug Fixes