[DTP-1138] Add object-level write API for LiveMap/LiveCounter creation #1950
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces significant enhancements to the LiveObjects system, focusing on creating and managing map and counter objects. The changes span multiple files, introducing new static methods for object creation, message generation, and state management. The modifications centralize object creation logic, improve type safety, and provide more flexible mechanisms for initializing and manipulating live objects across different platforms. 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/1950/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1950/features
github-actions
bot
temporarily deployed
to
staging/pull/1950/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1950/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1950/features
github-actions
bot
temporarily deployed
to
staging/pull/1950/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: 2
🧹 Nitpick comments (4)
src/platform/web/lib/util/bufferutils.ts (1)
119-122: Consider using a more efficient string replacement approach.While the current implementation is correct, using multiple
replacecalls with regex might not be the most efficient approach for high-frequency operations.Consider this alternative implementation:
- return this.base64Encode(buffer).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, ''); + const base64 = this.base64Encode(buffer); + let result = ''; + for (let i = 0; i < base64.length; i++) { + const char = base64[i]; + if (char === '+') result += '-'; + else if (char === '/') result += '_'; + else if (char === '=') break; + else result += char; + } + return result;src/plugins/liveobjects/objectid.ts (2)
18-34: Add TODO for temporary base64url encoding fixThe code includes a temporary fix for base64url encoding with '=' padding. It's important to track this with a TODO comment or issue to ensure it's addressed in the future.
Consider adding:
// temporary fix for realtime using base64url encoding with '=' padding. should be removed once fixed in realtime +// TODO: Remove padding workaround after fixing base64url encoding in realtime
44-63: Improve validation of 'msTimestamp' in 'fromString' methodThe current validation may not handle all edge cases. Consider parsing
msTimestamponce and usingNumber.isNaN()andNumber.isInteger()for more robust validation.Apply this diff to enhance validation:
const [hash, msTimestampStr] = rest.split('@'); if (!hash || !msTimestampStr) { throw new client.ErrorInfo('Invalid object id string', 50000, 500); } - if (!Number.isInteger(Number.parseInt(msTimestamp))) { + const msTimestamp = Number(msTimestampStr); + if (Number.isNaN(msTimestamp) || !Number.isInteger(msTimestamp)) { throw new client.ErrorInfo('Invalid object id string', 50000, 500); }Update the return statement accordingly:
- return new ObjectId(type as LiveObjectType, hash, Number.parseInt(msTimestamp)); + return new ObjectId(type as LiveObjectType, hash, msTimestamp);src/plugins/liveobjects/statemessage.ts (1)
244-255: Consider adding validation and error handling.While the implementation is functional, it could benefit from:
- Input validation
- Error handling for invalid JSON
static encodeInitialValue( utils: typeof Utils, initialValue: Partial<StateOperation>, ): { encodedInitialValue: string; format: Utils.Format; } { + if (!initialValue || typeof initialValue !== 'object') { + throw new Error('Initial value must be a valid object'); + } + try { return { encodedInitialValue: JSON.stringify(initialValue), format: utils.Format.json, }; + } catch (error) { + throw new Error(`Failed to encode initial value: ${error.message}`); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/common/lib/client/auth.ts(3 hunks)src/common/lib/client/baseclient.ts(1 hunks)src/common/types/IBufferUtils.ts(2 hunks)src/platform/nodejs/lib/util/bufferutils.ts(2 hunks)src/platform/web/lib/util/bufferutils.ts(3 hunks)src/platform/web/lib/util/hmac-sha256.ts(2 hunks)src/plugins/liveobjects/livecounter.ts(3 hunks)src/plugins/liveobjects/livemap.ts(6 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)src/plugins/liveobjects/liveobjectspool.ts(2 hunks)src/plugins/liveobjects/objectid.ts(3 hunks)src/plugins/liveobjects/statemessage.ts(5 hunks)test/realtime/live_objects.test.js(3 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/client/auth.ts
[error] 778-779: 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-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (33)
src/common/lib/client/baseclient.ts (3)
174-186: LGTM! Well-documented timestamp retrieval implementation.The function correctly implements the timestamp retrieval logic with proper handling of server time synchronization.
188-190: LGTM! Clean implementation of offset-based timestamp calculation.The function correctly handles the nullable server time offset using the nullish coalescing operator.
192-194: LGTM! Simple and clear offset initialization check.The function provides a clean way to check if the server time offset has been initialized.
src/common/lib/client/auth.ts (2)
778-778: LGTM! Proper usage of the new timestamp method.The code correctly uses the new
_getTimestampmethod with proper handling of optionalqueryTimeparameter.🧰 Tools
🪛 Biome (1.9.4)
[error] 778-779: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
1001-1007: LGTM! Well-documented timestamp retrieval wrapper.The function correctly wraps the client's timestamp retrieval while considering both the provided
queryTimeandauthOptions.queryTime.src/common/types/IBufferUtils.ts (1)
11-11: LGTM! Interface changes are well-designed.The new method signatures for
base64UrlEncodeandsha256are consistent with the existing patterns in the interface, maintaining type safety and following the established conventions.Also applies to: 23-23
src/platform/nodejs/lib/util/bufferutils.ts (3)
20-22: LGTM! Efficient implementation using Node.js built-in encoding.The implementation leverages Node.js's built-in
base64urlencoding, which is both efficient and secure.
77-81: LGTM! Secure implementation using Node.js crypto module.The implementation follows Node.js crypto best practices:
- Proper buffer conversion using
toBuffer- Uses the standard crypto module for SHA-256 hashing
87-87: LGTM! Clean and concise implementation.The simplified HMAC implementation maintains the existing functionality while improving code readability.
src/platform/web/lib/util/hmac-sha256.ts (1)
105-105: LGTM! Improved type safety with explicit type annotations.The addition of explicit type annotations and proper exports enhances type safety and module interoperability.
Also applies to: 188-188
src/platform/web/lib/util/bufferutils.ts (2)
3-3: LGTM! Clean import statement.The import statement properly includes both required functions from the hmac-sha256 module.
203-206: LGTM! Consistent implementation with proper buffer handling.The implementation:
- Properly converts input to Uint8Array using
toBuffer- Correctly converts the hash result back to ArrayBuffer
- Maintains consistency with the Node.js implementation
src/plugins/liveobjects/livecounter.ts (5)
3-3: Approved: Necessary import addedThe import of
ObjectIdis correctly added and is necessary for object ID generation within the class.
105-111: Approved: Encapsulation of initial value object creationThe
createInitialValueObjectmethod correctly encapsulates the creation of the initial value object for the counter, improving code modularity.
113-115: Ensure '_dataRef' is defined before accessing in 'value()' methodThe
value()method accessesthis._dataRef.data. Ensure that_dataRefis always defined to prevent potential runtime errors.Please confirm that
_dataRefis initialized beforevalue()is called in all cases. If there's a possibility of_dataRefbeing undefined, consider adding a check or default value.
126-129: Approved: Update to 'increment' methodThe
incrementmethod correctly utilizes the new staticcreateCounterIncMessageand appropriately publishes the message.
243-244: Approved: Proper handling of tombstoned objectsThe method correctly returns a no-op when the object is tombstoned, ensuring that terminal states are handled appropriately.
src/plugins/liveobjects/objectid.ts (3)
2-2: Approved: Import of 'Platform'The import of
Platformis necessary for object ID generation and is correctly added.
15-15: Approved: Addition of 'msTimestamp' propertyAdding the
msTimestampproperty to include timestamp information inObjectIdenhances the uniqueness and traceability of object IDs.
65-66: Approved: Addition of 'toString' methodThe
toStringmethod properly formats theObjectIdwith the newmsTimestampproperty, aiding in consistent object ID representation.src/plugins/liveobjects/liveobjectspool.ts (2)
71-71: Approved: Returning the newly created objectReturning the zero-value object enhances the method's usability, allowing immediate access to the object after creation.
Line range hint
51-71: Ensure all callers handle the new return type of 'createZeroValueObjectIfNotExists'The method now returns a
LiveObjectinstead ofvoid. Verify that all callers are updated to handle the returned object appropriately.Please run the following script to identify any usages of
createZeroValueObjectIfNotExiststhat may need updates:Ensure that each caller captures the returned
LiveObjector modifies its logic accordingly.src/plugins/liveobjects/liveobjects.ts (2)
57-75: LGTM! Well-documented async method for map creation.The implementation correctly:
- Generates a state message using LiveMap.createMapCreateMessage
- Publishes the message to the channel
- Returns a zero-value object for immediate interaction
77-95: LGTM! Well-documented async method for counter creation.The implementation correctly:
- Generates a state message using LiveCounter.createCounterCreateMessage
- Publishes the message to the channel
- Returns a zero-value object for immediate interaction
src/plugins/liveobjects/statemessage.ts (2)
24-24: LGTM! Type update improves compatibility.Replacing Uint8Array with ArrayBuffer is a good choice as it's more widely supported and can be used with various buffer implementations.
107-115: LGTM! Well-documented interface extension.The new properties are clearly documented and serve a clear purpose:
initialValuefor object creation verificationinitialValueEncodingfor proper value interpretationsrc/plugins/liveobjects/livemap.ts (5)
87-118: LGTM! Well-implemented message creation with proper validation.The implementation correctly:
- Validates inputs using validateKeyValue
- Handles both primitive values and LiveObject references
- Creates a properly structured state message
123-147: LGTM! Straightforward message creation with validation.The implementation correctly:
- Validates key type
- Creates a properly structured state message
152-172: LGTM! Thorough input validation.The implementation correctly validates:
- Key type must be string
- Value type must be string, number, boolean, Buffer, or LiveObject
177-215: LGTM! Well-implemented map creation with proper validation.The implementation correctly:
- Validates entries
- Generates object ID using ObjectId.fromInitialValue
- Creates a properly structured state message with initial value
220-240: LGTM! Clean implementation of initial value object creation.The implementation correctly:
- Handles both primitive values and LiveObject references
- Creates a properly structured initial value object
test/realtime/live_objects.test.js (2)
2374-2441: LGTM! Comprehensive tests for counter creation.The tests thoroughly cover:
- Counter creation with various initial values
- Counter assignment to state tree
- Error handling for invalid inputs
2443-2607: LGTM! Comprehensive tests for map creation.The tests thoroughly cover:
- Map creation with primitive values
- Map creation with LiveObject references
- Map assignment to state tree
- Error handling for invalid inputs
| await this.publish([stateMessage]); | ||
|
|
||
| // return zero-value object so that users can interact and subscribe to it immediately | ||
| const object = this._liveObjectsPool.createZeroValueObjectIfNotExists(objectId); |
There was a problem hiding this comment.
should this store the initial value too? if the user accesses this object's value before it is actaully created via the echoed message, I think they would expect it to have the value they specified
There was a problem hiding this comment.
Hmm, it gets a bit complicated due to CRDT rules and site time serials.
First: site timeserials. We don't have a timeserial for a CREATE operation at the moment an object is created, so if we were to apply an initial value to it, we wouldn't be able to assign a site timeserial for that CREATE operation. So far, it just means that whenever we actually receive a CREATE op by the operation application rules, we will try to apply it to the object (as there is no existing site timeserial on the object that would prevent it).
Ok, now second: so say we applied the initial value immediately, and now we received a CREATE op and there is not site timeserial to prevent application based on site timeserials. Objects have a flag _createOperationIsMerged to identify if the create operation was merged previously, to prevent double application (important for counters, as merging a CREATE op of a counter just adds the amount to the counter). So to guarantee that initial values for a counter are not double counted, we need to have the _createOperationIsMerged flag set to true when we assign an initial value on object creation during liveObjects.create* call. This means that the CREATE op won't be applied to objects when it is eventually received.
Third: map objects have additional CRDT rules for their map entries where each map entry stores a separate timeserial. When a CREATE op for a map is received, the map entries in it will have a timeserial set to the timeserial of the CREATE op. But if we applied the initial value on object creation client-side and prevented additional CREATE op application via the _createOperationIsMerged flag, we won't set timeserials for map entries when the CREATE op is received. That being said, maybe missing timeserials on map entries after the creation is not that big of a deal, as since this object was just created, there can't exist an operation on that object that is CGO earlier than the time serial of the CREATE operation, as we only obtain an object ID after the CREATE op has happened. However, it would lead to this weird state for a map object, where its entries are missing timeserials, and it seems like something that can cause unexpected bugs in the future.
This can be solved if we introduce different behavior in handling a _createOperationIsMerged flag between live object types. So both objects, map and counter, are set with their initial value on creation client-side, but the counter won't allow the CREATE op to be applied (has _createOperationIsMerged set to true), and the map will allow it to be applied (will have _createOperationIsMerged set to false).
Alternatively, if we implement our idea with operation application on ACKs, that ACK message will have a timeserial for the CREATE operation. Since create operations are currently async and wait for an ACK before resolving a promise and returning an object to a user, it would be trivial to apply an actual initial value on the object with the corresponding timeserial upon receipt of that ACK message.
There was a problem hiding this comment.
That being said, maybe missing timeserials on map entries after the creation is not that big of a deal, as since this object was just created, there can't exist an operation on that object that is CGO earlier than the time serial of the CREATE operation, as we only obtain an object ID after the CREATE op has happened.
I can only think of a situation when there there is a substantial clock skew between realtime regions, and the MAP_CREATE operation for a new object in region A ends up having a timestamp t1 which is greater than a following MAP_SET op t2 for that object id in region B. So that t1 > t2 and by LWW rules MAP_SET should not be applied, but if we don't have timeserials on map entries client-side due to not applying a CREATE op we will wrongly apply that MAP_SET operation. Is this even technically possible in the context of cross-region operations?
There was a problem hiding this comment.
First: site timeserials.
CREATE ops should be idempotent. The way we do this on the server side is by storing the create op on the object separately to the materialised state. This means that applying a CREATE op should not depend on the site timeserials (in realtime we still do the site timeserial check before applying the create, but we don't strictly need to). In the client we are instead relying on a boolean flag and materialising the state all together.
Ok, now second: ... This means that the CREATE op won't be applied to objects when it is eventually received.
I think this is fine - the payload of the create op that is eventually received is guaranteed to be the same as the one applied initially by the client for a given object ID.
Third: ... When a CREATE op for a map is received, the map entries in it will have a timeserial set to the timeserial of the CREATE op.
I actually don't think we need to do this (we don't do it on the server side). I think it is valid to say that a map entry specified as the initial value in a CREATE op should be have an "earliest possible" timeserial (probably just no timeserial, or any timeserial with a time component of 0), and any subsequent op applied to that key will always be applied.
As you point out - setting the map entry timeserial to the create op timeserial does allow the possibility of a concurrent MAP_SET operation with an earlier CGO timeserial. However, in reality, in order to send a MAP_SET operation you need the object ID, which you won't know until you have received the CREATE op, so the likelihood of a clock skew big enough to actually encounter this problem is basically nil. (Either way, this is moot if the entry initialised from the CREATE op doesn't have a timeserial in the first place)
There was a problem hiding this comment.
the likelihood of a clock skew big enough to actually encounter this problem is basically nil.
sgtm, then we can set initial value client-side and treat it as the CREATE op was merged into the object.
added setting initial value on object creation in https://github.com/ably/ably-js/compare/8dc5396a07d0b3357a0a961f99e4fa89e7555ea7..c89a9dc90a8fa737e5dd9b7518f13cd2fbb96e86
There was a problem hiding this comment.
fixed the race condition with the order of operations for this (discussed on the standup) in https://github.com/ably/ably-js/compare/c89a9dc90a8fa737e5dd9b7518f13cd2fbb96e86..d66a95f0b37a577a4800d7bda7545f135e73f142
| format: Utils.Format; | ||
| } { | ||
| return { | ||
| encodedInitialValue: JSON.stringify(initialValue), |
There was a problem hiding this comment.
should we use the default encoding specified on the client?
There was a problem hiding this comment.
yep, that's the plan. I think it's a client.options.useBinaryProtocol option to decide which encoding to use (msgpack if true).
For now it's hard coded to be always JSON as we're fixing the issue with msgpack encoding for the initial value on realtime.
Will update this part to depend on the client option once it's fixed and we know how initialValue field should look like when encoding is set to msgpack
VeskeR
force-pushed
the
DTP-1138/map-counter-creates
branch
from
4293a88 to
1c46209
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1950/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1950/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1950/features
VeskeR
force-pushed
the
liveobjects/object-mutation-api
branch
2 times, most recently
from
2db42fc to
f8563cb
Compare
…urn created object
VeskeR
force-pushed
the
DTP-1138/map-counter-creates
branch
from
1c46209 to
8dc5396
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1950/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1950/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1950/features
VeskeR
force-pushed
the
DTP-1138/map-counter-creates
branch
from
8dc5396 to
c89a9dc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1950/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1950/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1950/features
VeskeR
force-pushed
the
DTP-1138/map-counter-creates
branch
from
c89a9dc to
d66a95f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1950/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1950/features
github-actions
bot
temporarily deployed
to
staging/pull/1950/typedoc
|
Changes look good, just waiting now on the msgpack changes in realtime to unblock this |
VeskeR
force-pushed
the
DTP-1138/map-counter-creates
branch
from
d66a95f to
56d62a8
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1950/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1950/features
github-actions
bot
temporarily deployed
to
staging/pull/1950/typedoc
VeskeR
force-pushed
the
DTP-1138/map-counter-creates
branch
from
56d62a8 to
4a33d69
Compare
Resolves DTP-1138
Summary by CodeRabbit
New Features
Bug Fixes
Refactor