[PUB-1530] Update ably-js objects tests to use new REST API for Objects #1990
Conversation
WalkthroughThe changes refactor operation handling in the codebase to transition from integer-based action constants to string-based representations. In Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant O as ObjectsHelper
participant R as REST Server
C->>O: createAndSetOnMap(channelName, opts)
O->>O: Select appropriate REST operation (e.g., counterCreateRestOp/mapCreateRestOp)
O->>R: operationRequest(channelName, opBody)
R-->>O: REST Response
O-->>C: Return Result
Assessment against linked issues
Suggested reviewers
Poem
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/objects.test.js🪧 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/1990/bundle-report
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/common/modules/objects_helper.js (3)
17-24: Introduce string-based action constants carefully to avoid redundancy.
These string-basedACTION_STRINGSconstants duplicate the meaning of the existing numeric-basedACTIONS. If numeric constants are no longer needed, consider removing them to avoid confusion and reduce potential maintenance overhead.
312-342: Refactor repeated REST operation logic to eliminate duplication.
Each REST operation method follows a near-identical pattern of returning an object with common fields. Consider centralizing this logic in a helper function to keep the code DRY and reduce potential for future errors.+ function buildRestOpBody(type, { objectId, nonce, data }) { + const opBody = { operation: type }; + if (objectId != null) opBody.objectId = objectId; + if (nonce != null) opBody.nonce = nonce; + if (data) opBody.data = data; + return opBody; + } function mapCreateRestOp(opts) { const { objectId, nonce, data } = opts ?? {}; - const opBody = { - operation: ACTION_STRINGS.MAP_CREATE, - }; - if (data) { opBody.data = data; } - if (objectId != null) { opBody.objectId = objectId; opBody.nonce = nonce; } - return opBody; + return buildRestOpBody(ACTION_STRINGS.MAP_CREATE, { objectId, nonce, data }); }Also applies to: 344-355, 357-384
411-417: Potential collision in ID generation under high concurrency.
UsingDate.now()andrandomString()may suffice for most test scenarios, but be aware that extremely rapid calls in short succession could theoretically produce collisions. If you need higher guarantees, consider using a unique identifier library or a higher-precision time source.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/common/modules/objects_helper.js(5 hunks)test/realtime/objects.test.js(41 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
test/realtime/objects.test.js (2)
src/plugins/objects/batchcontextlivecounter.ts (1)
increment(23-28)src/plugins/objects/livecounter.ts (1)
increment(139-143)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (31)
test/common/modules/objects_helper.js (6)
82-97: Creation of extended map entries appears consistent.
The updated structure formapCreateRestOpcalls, including various data types (string, bytes, number, boolean), looks properly formatted for the new REST approach.
100-100: Region markers.
These comment lines neatly separate code sections. There are no issues with these changes.Also applies to: 298-298, 300-300
302-310: Implementation ofcreateAndSetOnMapis correct and straightforward.
The async flow (creating the object, retrievingobjectId, and then setting it on the parent map) is logically sound.
386-386: Single-operation array check is valid.
Raising an error when multiple operations are passed is helpful for preventing unexpected behaviors.Also applies to: 388-388
397-397: Clear inline comments.
These comments clarify assumptions about the response items, aiding maintainability. No other concerns.Also applies to: 399-399
409-409: Region end marker is fine.
No changes required here; it improves readability.test/realtime/objects.test.js (25)
624-646: Added proper REST API structure for primitive key data.The code now includes
restDataproperty alongside the originaldataproperty for each item in theprimitiveKeyDataarray. This change properly maps the original data to the new REST API format which uses type-specific field names (string,bytes,number,boolean) instead of the genericvalueproperty.
656-659: Updated primitiveMapsFixtures to include REST API data structure.Added a new
restDataproperty to fixtures that properly aggregates therestDatafromprimitiveKeyData. This ensures consistency with the new REST API format throughout the tests.
735-736: Migrated to REST API for counter creation.Changed from the legacy operation method to the new REST API endpoint for counter creation, using the type-specific
numberproperty in the payload.
786-787: Migrated to REST API for counter creation in createAndSetOnMap.Updated another instance of counter creation to use the new REST API format with the proper
numberproperty.
857-858: Migrated to REST API for map creation.Updated from the legacy
mapCreateOpmethod to the new REST-basedmapCreateRestOpmethod, with the data structure adapted for the new API format.
916-919: Updated operation requests to use REST API format.Changed the request structure to use the new REST API format for map creation, including the proper data structure with type-specific fields.
920-923: Migrated counter creation to REST API.Updated the counter creation request to use the new REST-based operation with the proper
numberparameter instead of the legacy approach.
927-932: Updated map creation to REST API format.Modified the map creation operation to use the REST API structure with the appropriate data format for references to other objects.
1073-1080: Migrated to REST API for map set operations.Updated from the legacy
mapSetOpto the new REST-basedmapSetRestOpmethod, with the correct parameter structure including thevalueproperty that contains the type-specific data.
1269-1275: Migrated to REST API for map remove operations.Updated from the legacy
mapRemoveOpto the new REST-basedmapRemoveRestOpmethod, maintaining the same parameters but with the updated API structure.
1390-1391: Updated counter creation across multiple fixtures.Changed the counter creation to use the new REST API format with the
numberparameter for all counters in the test fixtures.
1500-1501: Migrated to REST API for counter creation in a specific test.Updated another instance of counter creation to use the REST API format with the appropriate
numberparameter.
1534-1540: Updated counter increment to use REST API.Changed from the legacy counter increment operation to the REST-based version with the appropriate
numberparameter instead of the previousamountparameter.
1761-1766: Migrated map creation with complex data to REST API.Updated map creation with nested key data to use the REST API format, properly structuring multi-key data with type-specific fields.
1771-1772: Updated counter creation with initial value to REST API.Changed counter creation with an initial value to use the REST API format with the proper
numberparameter.
2239-2246: Migrated map set operations to REST API in another test scenario.Updated map set operations to use the REST API approach with type-specific field (
stringin this case) for the value.
3669-3677: Updated subscription test to use REST API for map operations.Modified the map set operation in a subscription test to use the new REST API format with type-specific fields.
3704-3710: Migrated map remove operation in subscription test to REST API.Updated the map remove operation in a subscription test to use the new REST-based approach.
3751-3757: Updated multiple map operations in subscription test to REST API.Changed the first of multiple map set operations in a subscription test to use the REST API format with the proper structure.
3760-3766: Migrated additional map set operation to REST API.Updated another map set operation in the subscription test to use the REST-based approach.
3769-3775: Updated map remove operation in subscription test to REST API.Changed the map remove operation in the subscription test to use the REST-based method.
3777-3784: Migrated final map set operation to REST API.Updated the last map set operation in the subscription test to use the REST API format.
3786-3792: Updated final map remove operation to REST API.Changed the final map remove operation in the subscription test to use the REST-based approach.
3817-3823: Migrated counter increment in subscription test to REST API.Updated the counter increment operation in a subscription test to use the REST API approach with the proper
numberparameter.
3855-3861: Updated counter increment operations in multiple subscription tests.Changed the counter increment operations in subscription tests to use the REST-based approach with the proper parameter structure.
VeskeR
force-pushed
the
PUB-1530/update-rest-api
branch
from
24e7feb to
480ff4c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1990/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/common/modules/objects_helper.js (1)
213-213:⚠️ Potential issueUpdate remaining legacy method references in this file.
There are still references to the legacy methods in
mapObjectandcounterObjectfunctions (lines 213 and 233). These should be updated to use the new REST-based methods for consistency.Apply this fix:
- obj.object.createOp = this.mapCreateOp({ objectId, entries: initialEntries }).operation; + obj.object.createOp = this.mapCreateRestOp({ objectId, data: initialEntries }); - obj.object.createOp = this.counterCreateOp({ objectId, count: initialCount }).operation; + obj.object.createOp = this.counterCreateRestOp({ objectId, number: initialCount });Also applies to: 233-233
🧹 Nitpick comments (1)
test/common/modules/objects_helper.js (1)
98-296: Consider plan for removing legacy methods.The file now contains both legacy and REST-based operation methods. Consider documenting a plan for deprecating and eventually removing the legacy methods once the transition is complete.
You could add a comment at the beginning of the legacy methods section:
// #region Wire Object Messages +/** + * @deprecated These methods are being replaced by their REST counterparts. + * They will be removed in a future version once all code has been migrated. + */Also applies to: 298-407
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
test/common/modules/objects_helper.js(5 hunks)test/realtime/objects.test.js(41 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
test/realtime/objects.test.js (2)
src/plugins/objects/batchcontextlivecounter.ts (1)
increment(23-28)src/plugins/objects/livecounter.ts (1)
increment(139-143)
⏰ 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 (41)
test/common/modules/objects_helper.js (7)
17-24: Good addition of string constants to match numeric ones.Adding the string-based
ACTION_STRINGSconstant alongside the numericACTIONSis a good approach for the transition to REST-based operations. This makes the code more readable and maintainable.
56-56: Verify all references to legacy methods have been updated.The switch from
counterCreateOp/mapCreateOptocounterCreateRestOp/mapCreateRestOplooks correct here, but ensure all references are updated consistently across the codebase.#!/bin/bash # Search for any remaining references to the old operation methods rg "counterCreateOp" --type js rg "mapCreateOp" --type jsAlso applies to: 61-61, 66-66, 72-72, 77-94
82-94: Good structured data format using typed values.The updated map creation operation with typed values (
string,number,boolean, etc.) is more explicit and type-safe than the previous implementation, which is a good improvement.
300-308: Method refactored to use REST operations.The
createAndSetOnMapmethod has been properly updated to use the new REST operations. The flow is clear and the variable names are descriptive.
310-326: Well-structured REST operation creator for maps.The
mapCreateRestOpmethod is well-structured and includes proper handling of optional parameters. The conditional logic for adding data, objectId, and nonce is clear.
384-405: Updated error message in operationRequest.The error message has been updated to reflect that only single object operation requests are supported, which is more specific and helpful than the previous message.
409-415: Good helper methods for generating fake object IDs.The addition of helper methods to generate fake object IDs for maps and counters is a good improvement for test readability and maintenance.
test/realtime/objects.test.js (34)
625-646: AddedrestDataproperty to improve test compatibility with the new REST API.This change adds a new
restDataproperty to all objects in theprimitiveKeyDataarray. The property transforms the existingdatastructure into a format compatible with the REST API, using more specific type keys likestring,bytes,number, andbooleaninstead of the genericvalueproperty.This modification facilitates testing with the updated REST API format while maintaining backward compatibility.
656-659: Added REST-compatible data structure to map fixtures.This change adds
restDataaccumulation for theprimitiveMapsFixturesarray, ensuring that the nested map structures are properly represented in the REST API format. The implementation correctly aggregates therestDataobjects from theprimitiveKeyDataentries for consistent representation.
735-736: Updated to use REST API for counter creation.Changed from
createOp: objectsHelper.counterCreateOp(...)tocreateOp: objectsHelper.counterCreateRestOp(...)to use the REST-based operation method. This is part of the larger effort to transition from legacy operation methods to REST-based ones for the Objects feature.
786-787: Switched to REST API for counter creation in test fixtures.Consistent with the change pattern, updated the counter creation operation to use the REST API format in this test case as well.
857-858: Updated map creation to use REST API format.Changed from
createOp: objectsHelper.mapCreateOp(...)tocreateOp: objectsHelper.mapCreateRestOp({ data: fixture.restData })to utilize the REST API format for map creation. Thedataparameter now uses therestDataproperty created earlier, which contains the appropriate structure for the REST API.
916-919: Converted map creation to use REST API in reference object tests.Changed the map creation operation to use
objectsHelper.mapCreateRestOp()with the appropriate REST data structure. This ensures consistent usage of the REST API across different test scenarios.
920-923: Updated counter creation to use REST API format.Modernized the counter creation operation to use
objectsHelper.counterCreateRestOp()with the explicitnumberparameter, maintaining consistency with other REST API modifications in the tests.
927-933: Converted map reference creation to use REST API format.Updated the map creation with references to use
mapCreateRestOp()with the appropriatedatastructure containing object references. The new format specifies object references using theobjectIdfield within the appropriate data structure.
1073-1079: Updated map set operations to use REST API format.Changed from direct operation requests to using
objectsHelper.mapSetRestOp()with the appropriate structure. Thevalueparameter now uses therestDataproperty instead ofdata, which contains the REST API compatible structure.
1126-1127: Switched to REST API for counter creation in increment tests.Updated counter creation to use
counterCreateRestOp()with thenumberparameter instead of the legacy operation format. This maintains consistency with the REST API transition throughout the test suite.
1132-1136: Updated map creation with initial data to use REST API format.Changed the map creation to use
mapCreateRestOp()with a properly structureddataobject. The data now contains fields with specific type indicators (e.g.,string) instead of the genericvalueproperty.
1242-1249: Converted map creation with initial data to use REST API format.Updated the map creation operation to use
mapCreateRestOp()with a properly structureddataobject containing string values in the REST API format.
1269-1275: Updated map remove operation to use REST API format.Changed from direct operation requests to using
objectsHelper.mapRemoveRestOp()with the appropriate parameters. This maintains consistency with the REST API transition throughout the test suite.
1390-1391: Updated counter creation to use REST API format in multiple tests.Changed the counter creation operation to use
counterCreateRestOp()with thenumberparameter in the counter creation fixture tests. This maintains consistency with the REST API transition throughout the test suite.
1500-1501: Switched to REST API for counter creation in increment tests.Updated the counter creation to use
counterCreateRestOp()with thenumberparameter. This is part of the effort to transition all object operations to the REST API format.
1534-1540: Updated counter increment operation to use REST API format.Changed the counter increment operation to use
counterIncRestOp()with thenumberparameter instead ofamount. This naming change reflects the REST API's parameter naming conventions.
1610-1616: Converted map creation with empty data to use REST API format.Updated the map creation to use
mapCreateRestOp()without parameters, maintaining consistency with the REST API transition throughout the test suite.
1761-1766: Updated map creation with initial data to use REST API format.Changed the map creation to use
mapCreateRestOp()with a properly structureddataobject containing both string and number values in the REST API format.
1771-1772: Switched to REST API for counter creation with initial value.Updated counter creation to use
counterCreateRestOp()with thenumberparameter. This maintains consistency with the REST API transition throughout the test suite.
3593-3599: Updated counter increment operation to use REST API format.Changed the counter increment operation to use
counterIncRestOp()with thenumberparameter instead ofamount. This naming change reflects the REST API's parameter naming conventions.
3636-3642: Switched counter increment operations to use REST API format.Updated multiple counter increment operations to use
counterIncRestOp()with thenumberparameter. This maintains consistency with the REST API transition throughout the test suite.
3670-3677: Updated map set operation to use REST API format.Changed the map set operation to use
mapSetRestOp()with the appropriate parameters and adjusted the value structure to use the type-specific key format (stringin this case).
3704-3710: Converted map remove operation to use REST API format.Updated the map remove operation to use
mapRemoveRestOp()with the appropriate parameters. This maintains consistency with the REST API transition throughout the test suite.
3751-3758: Updated map set operation to use REST API format in multiple operation tests.Changed the map set operation to use
mapSetRestOp()with the appropriate parameters and adjusted the value structure to use the type-specific key format (stringin this case).
3760-3767: Updated additional map set operation to use REST API format.Changed another map set operation to use
mapSetRestOp()with the appropriate parameters. This maintains consistency with the REST API transition throughout the test suite.
3769-3775: Converted map remove operation to use REST API format.Updated another map remove operation to use
mapRemoveRestOp()with the appropriate parameters. This is part of the systematic transition to the REST API format.
3777-3784: Updated map set operation to use REST API format.Changed the map set operation to use
mapSetRestOp()with the appropriate parameters and adjusted the value structure to use the type-specific key format.
3786-3792: Converted final map remove operation to use REST API format.Updated the last map remove operation in this test section to use
mapRemoveRestOp()with the appropriate parameters. This completes the consistent transition to REST API operations in this test section.
3817-3823: Updated counter increment operation to use REST API format.Changed the counter increment operation to use
counterIncRestOp()with thenumberparameter instead ofamount. This naming change aligns with the REST API parameter naming conventions.
3855-3861: Converted counter increment operation to use REST API format.Updated another counter increment operation to use
counterIncRestOp()with thenumberparameter. This maintains consistency with the REST API transition throughout the test suite.
3895-3901: Updated counter increment operation in unsubscribe tests to use REST API format.Changed the counter increment operation in the subscription tests to use
counterIncRestOp()with thenumberparameter. This ensures consistency in the REST API approach across different test scenarios.
3935-3944: Updated map set operation in subscription tests to use REST API format.Changed the map set operation to use
mapSetRestOp()with the appropriate parameters and value structure in the subscription test scenarios.
3979-3988: Converted map set operation to use REST API format.Updated another map set operation in subscription tests to use
mapSetRestOp()with the appropriate parameters. This maintains consistency with the REST API transition throughout the test suite.
4025-4034: Updated map set operation in unsubscribe tests to use REST API format.Changed the map set operation to use
mapSetRestOp()with the appropriate parameters in the unsubscribe test scenarios. This maintains consistency across all test cases.
VeskeR
force-pushed
the
PUB-1530/update-rest-api
branch
from
480ff4c to
918a0a6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1990/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
test/common/modules/objects_helper.js (1)
212-214:⚠️ Potential issueLegacy code references need updating
The
mapObjectandcounterObjectmethods still use the legacy operationsmapCreateOpandcounterCreateOprather than their REST counterparts.Update these methods to use the new REST operations for consistency:
- obj.object.createOp = this.mapCreateOp({ objectId, entries: initialEntries }).operation; + obj.object.createOp = this.mapCreateRestOp({ objectId, data: initialEntries }).operation;- obj.object.createOp = this.counterCreateOp({ objectId, count: initialCount }).operation; + obj.object.createOp = this.counterCreateRestOp({ objectId, number: initialCount }).operation;Also applies to: 232-234
♻️ Duplicate comments (2)
test/common/modules/objects_helper.js (2)
56-56: Update Legacy Operation Method ReferencesThe update to use REST-based operations is good, but there are still references to the legacy methods in the codebase.
Your switch to
counterCreateRestOpandmapCreateRestOpis correct, but ensure that all other references to the old methods are updated for consistency.Also applies to: 61-61, 66-66, 72-72, 77-77
356-362: Parameter name inconsistency: 'count' vs 'number'The parameter for counter creation has been changed from
counttonumberin the REST operations.Make sure this change is consistently applied throughout the codebase, especially in test cases that may still be using the old parameter name.
🧹 Nitpick comments (2)
test/common/modules/objects_helper.js (2)
384-405: Updated error message for operation requestThe error message has been updated to reflect that only single object operation requests are supported, which is clearer than the previous message.
Consider adding more detailed information about what constitutes a valid operation request to help with debugging.
- throw new Error(`Only single object operation requests are supported`); + throw new Error(`Only single object operation requests are supported. Please provide a single operation object instead of an array.`);
100-196: Consider removing or deprecating legacy methodsThe old operation methods (
mapCreateOp,counterCreateOp, etc.) are still present in the code. If these are being replaced by the REST operations, consider adding deprecation notices or removing them entirely to prevent confusion.If keeping them for backward compatibility, add documentation comments explaining this:
+ /** + * @deprecated Use mapCreateRestOp instead. Maintained for backward compatibility. + */ mapCreateOp(opts) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
test/common/modules/objects_helper.js(5 hunks)test/realtime/objects.test.js(41 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/realtime/objects.test.js (2)
src/plugins/objects/batchcontextlivecounter.ts (1)
increment(23-28)src/plugins/objects/livecounter.ts (1)
increment(139-143)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (14)
test/common/modules/objects_helper.js (6)
17-24: Good addition of string-based action constantsAdding
ACTION_STRINGSwith string representations of actions improves code readability and aligns with the REST API approach. This is a good practice for API clarity.
82-93: Good restructuring of map data formatThe new structure using typed property values (
string,bytes,number,boolean,objectId) is more explicit and aligns well with the REST API requirements. This addresses the issues mentioned in the PR objectives regarding MAP_CREATE failures with initial values.
98-98: Improved code organization with regionsAdding region comments improves code organization by clearly separating the wire object messages from the REST API operations. This makes the code more maintainable and easier to navigate.
Also applies to: 296-298
300-308: Well refactored method to use REST operationsThe
createAndSetOnMapmethod has been properly refactored to use the new REST operations. The flow is cleaner and aligns with the REST API approach.
409-415: Useful utility methods for object ID generationAdding helper methods for generating fake object IDs with the appropriate format for maps and counters improves code organization and readability.
310-326:✅ Verification successful
Good implementation of REST operation methods
The new REST operation methods follow a consistent pattern and provide a clean interface for creating REST API operation requests. The separation of concerns is clear.
However, ensure parameter naming is consistent across the codebase.
Also applies to: 328-340, 342-353, 355-371, 373-382
🏁 Script executed:
#!/bin/bash # Check for inconsistencies between count and number in counter-related code echo "Searching for references to the old parameter name 'count' in counter-related code:" rg "count.*counter" --type js | grep -v "materialisedCount"Length of output: 12390
Excellent implementation of REST operation methods; parameter naming verified as consistent.
The code in
test/common/modules/objects_helper.js(lines 310–326) is clean and follows a consistent pattern for creating REST API requests. The automated check for counter-related parameter usage shows that the parameter name"count"is uniformly used in the test suite, suggesting that the current naming convention is consistently applied.Please double-check that similar naming (across sections at lines 328–340, 342–353, 355–371, and 373–382) aligns with this convention, ensuring that there isn’t any unintended mix-up with alternative names like
"number".test/realtime/objects.test.js (8)
625-647: Good addition ofrestDataproperty to support the new REST API formatThe addition of the
restDataproperty to each entry in theprimitiveKeyDataarray correctly maps the existing data structures to the new REST API format. This change maintains backward compatibility while enabling the transition to the new API.String data: { value: 'stringValue' } → restData: { string: 'stringValue' } Bytes data: { value: '...', encoding: 'base64' } → restData: { bytes: '...', encoding: 'base64' } Number data: { value: 1 } → restData: { number: 1 } Boolean data: { value: true } → restData: { boolean: true }
657-660: Good update toprimitiveMapsFixturesAdding the
restDataproperty toprimitiveMapsFixturesensures consistency with the updatedprimitiveKeyDatastructure. The implementation correctly aggregates therestDataproperties from theprimitiveKeyDataentries, which maintains test data integrity.
736-736: Consistent update of operation methods to use REST-based equivalentsAll operations have been consistently updated throughout the codebase from the legacy format to the new REST API format (e.g.,
counterCreateOp→counterCreateRestOp). This change aligns well with the PR objective to transition the tests to use the new REST API for Objects.Also applies to: 789-789, 820-820, 860-860, 1128-1128, 1245-1245, 1397-1397, 1442-1442, 1614-1614, 1817-1817, 1884-1884
919-926: Properly refactored operation request callsThe operation request calls have been updated to use the REST format with the appropriate data structure. This change ensures compatibility with the new REST API implementation while maintaining the same test functionality.
930-934: Correctly updated map creation operation parametersThe map creation operation parameters have been updated to use the new REST format, changing from the old format with
entriesto the new format withdatacontaining object IDs. This change reflects the newer structure expected by the REST API.
1071-1082: Well-structured update to map set operationsThe map set operations have been refactored to use the REST API format, with the
valueparameter now properly structured with type-specific fields (e.g.,string,number,boolean) instead of the genericvaluefield. This change correctly implements the new REST API requirements.
1272-1278: Properly updated map remove operationsThe map remove operations have been refactored to use the REST API format with
mapRemoveRestOp. The parameter structure has been maintained, which ensures the tests continue to function as expected with the new API.
1537-1543: Updated counter increment operations to use REST APIThe counter increment operations have been updated to use the REST API format, changing from
counterIncOptocounterIncRestOpand updating the parameter name fromamounttonumber. This change aligns with the new REST API specifications.
Update to use new API from [1] Resolves PUB-1530 [1] https://ably.atlassian.net/browse/PUB-1191
VeskeR
force-pushed
the
PUB-1530/update-rest-api
branch
from
918a0a6 to
2689b47
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
test/common/modules/objects_helper.js (2)
100-196: 🛠️ Refactor suggestionConsider deprecating legacy operation methods
The old operation methods (
mapCreateOp,counterCreateOp, etc.) still exist alongside the new REST methods. This could lead to confusion or incorrect usage. Consider marking these methods as deprecated with JSDoc comments or gradually removing them if they're no longer needed.You could add deprecation notices like:
+/** + * @deprecated Use mapCreateRestOp instead + */ mapCreateOp(opts) { // existing code }
212-214:⚠️ Potential issueUpdate mapObject and counterObject to use new REST operations
The
mapObjectandcounterObjectmethods still use the legacy operation methods (mapCreateOpandcounterCreateOp) in theircreateOpassignments. These should be updated to use the new REST operation methods for consistency.if (initialEntries) { - obj.object.createOp = this.mapCreateOp({ objectId, entries: initialEntries }).operation; + obj.object.createOp = this.mapCreateRestOp({ objectId, data: initialEntries }); }if (initialCount != null) { - obj.object.createOp = this.counterCreateOp({ objectId, count: initialCount }).operation; + obj.object.createOp = this.counterCreateRestOp({ objectId, number: initialCount }); }Also applies to: 232-234
♻️ Duplicate comments (2)
test/common/modules/objects_helper.js (2)
56-56: Consistent update to use new REST API operationsGreat work updating all create operations in the
initForChannelmethod to use the new REST API operations. The transition from legacy operation methods to REST counterparts is consistent across all instances.Also applies to: 61-61, 66-66, 72-72, 77-77
355-371: Parameter name change from 'count' to 'number'The parameter for counter creation has been renamed from
counttonumberin the REST operation method. Ensure this change is consistent across all counter-related code to avoid confusion.#!/bin/bash # Check if there are any references to both 'count' and 'number' in counter-related code # which might indicate inconsistent parameter naming rg "counterCreateRestOp.*count" --type js rg "counterCreateOp.*number" --type js
🧹 Nitpick comments (1)
test/common/modules/objects_helper.js (1)
310-326: Add JSDoc comments to the new REST operation methodsWhile the implementation of
mapCreateRestOpis good, it would be helpful to add JSDoc comments explaining the purpose of the method and its parameters, similar to other methods in this file.+/** + * Creates a REST API operation body for creating a new map + * @param {Object} opts - Options for the create operation + * @param {string} [opts.objectId] - Optional object ID to use + * @param {string} [opts.nonce] - Optional nonce to use + * @param {Object} [opts.data] - Optional initial data for the map + * @returns {Object} The operation body + */ mapCreateRestOp(opts) { const { objectId, nonce, data } = opts ?? {}; const opBody = { operation: ACTION_STRINGS.MAP_CREATE, }; if (data) { opBody.data = data; } if (objectId != null) { opBody.objectId = objectId; opBody.nonce = nonce; } return opBody; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
test/common/modules/objects_helper.js(5 hunks)test/realtime/objects.test.js(41 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/realtime/objects.test.js (2)
src/plugins/objects/livecounter.ts (1)
increment(139-143)src/plugins/objects/batchcontextlivecounter.ts (1)
increment(23-28)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (24)
test/common/modules/objects_helper.js (5)
17-24: Well-structured constants for string-based operationsGood addition of string-based operation constants that parallel the existing numeric ACTIONS. This change supports the transition to REST API operations and improves code readability.
82-94: Improved data structure for map creationThe new structure for map creation with explicit type fields (
string,bytes,number,boolean,objectId) is clearer and more consistent with REST API expectations. This should address the issues mentioned in the PR description regarding map creation with initial values.
300-308: Well-refactored map creation methodThe
createAndSetOnMapmethod has been properly refactored to use the new REST API operations. The flow is clear and follows the same logical structure as before while using the updated API.
384-405: Improved error handling in operationRequestGood update to the error message in
operationRequestto clarify that only single object operations are supported. The additional comments also improve code readability.
409-415: Useful utility methods for generating fake object IDsThe new helper methods for generating fake object IDs are a good addition. They abstract the ID generation logic and make the code more readable.
test/realtime/objects.test.js (19)
625-647: Changes add REST API compatibility to primitiveKeyDataThe additions of
restDataproperty to each entry in theprimitiveKeyDataarray provides proper formatting for the new REST API. The mapping between data types is consistent:
- String values:
{ value: 'stringValue' }→{ string: 'stringValue' }- Bytes values:
{ value: 'base64', encoding: 'base64' }→{ bytes: 'base64' }- Number values:
{ value: number }→{ number: number }- Boolean values:
{ value: boolean }→{ boolean: boolean }
657-660: Adds REST data aggregation to primitiveMapsFixturesThis change adds a
restDataproperty to the map fixtures by aggregating therestDatafromprimitiveKeyData, ensuring consistency with the new API structure across the test suite.
789-790: Updates createAndSetOnMap to use counterCreateRestOpThe operation is properly updated to use the new REST API operation method, maintaining consistent operation behavior while adapting to the new API format.
836-861: Updates operation to use mapCreateRestOp with restDataThis change properly migrates the map creation operation to leverage the REST API format, using the previously defined
restDataproperty from the fixtures.
919-936: Updates operationRequest to use REST operation methodsChanged from directly calling operations to using
operationRequestwith REST-formatted operations, which helps adapt the tests to use the new API properly. The object structure has been properly transformed from the old API format to the new REST API format.
1075-1085: Updates mapSetOp to use REST operation formatChanges the operation to use
mapSetRestOpand references the newrestDataproperty correctly, maintaining functional equivalence while adopting the new API structure.
1129-1130: Updates counter and map creation operations to use REST APIBoth counter and map creation operations have been properly migrated to use the REST API format, maintaining test functionality while adapting to the updated API.
Also applies to: 1135-1139
1243-1252: Updates map creation to use REST API with nested structureThe operation now uses
mapCreateRestOpwith a properly structured nested data object conforming to the REST API requirements.
1272-1278: Updates mapRemoveOp to use REST operation formatProperly changes the map removal operation to use the REST API format, maintaining the same functionality while adapting to the new API structure.
1393-1394: Updates counter creation to use REST API formatThe counter creation operation has been updated to use
counterCreateRestOpwith appropriate parameters, correctly adapting to the new API requirements.
1503-1504: Updates counter creation operation parameterThe operation parameter structure has been properly updated to match the new REST API requirements while maintaining the same functionality.
1537-1543: Updates counter increment operation to use REST APICounter increment operation has been migrated to use
counterIncRestOpwith the updated parameter structure, correctly implementing the new API format.
1613-1619: Updates createAndSetOnMap to use REST API operationsBoth map and counter creation operations within the test method have been updated to use the REST API format, ensuring consistency throughout the test.
1764-1775: Updates map and counter creation with complex data structuresThe map creation now properly uses the REST API format with a nested data structure, and the counter creation uses the updated REST API format, maintaining test functionality.
2242-2249: Updates map set operation to use REST API formatMap set operation properly migrated to use
mapSetRestOpwith the appropriate parameter structure for the REST API, maintaining proper behavior.
3596-3602: Updates operationRequest for counter operationsCounter increment operation is now correctly using the REST API format with
counterIncRestOpand updated parameter structure, ensuring the subscription test works as expected.
3754-3795: Updates map operations to use REST API format in subscription testsAll map operations (set and remove) in the subscription test have been updated to use the REST API format, ensuring that subscription callbacks work correctly with the new operation format.
3817-3827: Updates counter increment operation in unsubscribe testCounter operation has been properly migrated to use the REST API format in the unsubscribe test, maintaining the test's functionality.
3857-3866: Updates counter operations in multi-increment testAll counter increment operations in the multi-increment test have been updated to use the REST API format, ensuring the test functions correctly with the updated API.
Update to use new API from https://ably.atlassian.net/browse/PUB-1191.
Based on the REST API docs from ably/docs#2490.
Current issues (will be marked if resolved):
MAP_CREATEoperation with any initial value for a map fails with "invalid object message: state data required" error. Quite a lot of tests rely on the ability to create a map over the REST API, so those are broken for now.MAP_SEToperation which sets bytes value for a key on map succeeds, but then the double encoded bytes value is received over the wire for a connected client.Resolves PUB-1530
Summary by CodeRabbit
Refactor
Tests
restDataproperty for better alignment with updated operations, ensuring greater consistency during validation.These improvements streamline internal workflows and ensure that data processing remains clear and reliable across operations and testing scenarios.