Skip to content

Conversation

@lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Jul 4, 2025

Note: This PR is based on top of #21; please review that one first.

Simple PR that implements the APIs specified in ably/specification#341. See commit messages for more details.

Summary by CodeRabbit

  • New Features

    • Implemented throwable accessors for size, entries, keys, and values properties on Live Maps, providing more robust error handling when channel state is invalid.
  • Bug Fixes

    • Standardized error handling for Live Map and Live Counter operations, ensuring consistent error codes and status codes when channel state is detached or failed.
  • Tests

    • Expanded and refined test coverage for Live Map property access, error handling, and value type support.
    • Updated tests to validate both error code and status code, and to handle new throwable property signatures.

@github-actions github-actions bot temporarily deployed to staging/pull/22/AblyLiveObjects July 4, 2025 19:25 Inactive
Motivation as in 3f6de86; the new spec points in [1] tell us these can
throw.

[1] ably/specification#341
@lawrence-forooghian lawrence-forooghian force-pushed the ECO-5330-LiveMap-access-API branch from 1a74632 to 9093395 Compare July 7, 2025 15:00
@github-actions github-actions bot temporarily deployed to staging/pull/22/AblyLiveObjects July 7, 2025 15:01 Inactive
I didn't do this in cb427d8 because the specification hadn't yet
specified the status code (it was an outstanding question on the PR at
time of implementing), but the newly-written spec [1] for other LiveMap
getter methods _does_ specify the status code as being 400. So DRY up
the creation of these errors, and supply a status code (assuming that
the spec will be updated to specify 400 for these existing ones too).

[1] ably/specification#341
Based on [1] at 7d4c215. A few outstanding questions on the PR; have
implemented based on my current understanding of what's there.

Development approach similar to that described in cb427d8. Also, have
not implemented the specification points related to RTO2's channel mode
checking for same reasons as mentioned there.

[1] ably/specification#341
@umair-ably
Copy link
Collaborator

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Jul 24, 2025

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link

coderabbitai bot commented Jul 24, 2025

Walkthrough

The changes introduce a new internal error type, LiveObjectsError, to standardize error handling for invalid channel states across several classes. Properties and methods in DefaultLiveMap, DefaultLiveCounter, and DefaultRealtimeObjects now throw this error (converted to ARTErrorInfo) when accessed in detached or failed states. The LiveMap protocol and related tests are updated to reflect these throwable properties and stricter error assertions. Additional tests are added to verify property behavior and value filtering in DefaultLiveMap.

Changes

Files/Paths Change Summary
Sources/AblyLiveObjects/Utility/Errors.swift Introduced LiveObjectsError enum with conversion to ARTErrorInfo for standardized error handling.
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift
Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift
Refactored error handling to throw LiveObjectsError (converted to ARTErrorInfo) for invalid channel states.
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift Refactored value conversion, implemented throwable properties (size, entries, keys, values), and unified error handling.
Sources/AblyLiveObjects/Public/PublicTypes.swift Updated LiveMap protocol: properties (size, entries, keys, values) are now throwable.
Tests/AblyLiveObjectsTests/DefaultLiveCounterTests.swift
Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift
Strengthened error assertions in tests to check both error code and HTTP status code.
Tests/AblyLiveObjectsTests/DefaultLiveMapTests.swift Added comprehensive property and value tests for DefaultLiveMap, including error and filtering checks.
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift Updated test code to use try for throwable .size property accesses.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant LiveMap
    participant Channel

    Client->>LiveMap: Access property (e.g., size, entries, keys, values)
    LiveMap->>Channel: Check channel state
    alt Channel state is attached
        LiveMap-->>Client: Return requested data
    else Channel state is detached or failed
        LiveMap->>LiveObjectsError: Create error with operation and state
        LiveObjectsError->>LiveMap: Convert to ARTErrorInfo
        LiveMap-->>Client: Throw ARTErrorInfo
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Poem

In the warren of code where errors once grew,
A rabbit hopped in, with fixes anew.
Now maps throw with care, and counters do too,
Each channel state checked, each error in view.
With tests all enhanced and logic refined,
This code hops ahead, robustly designed!
🐇✨

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa255c1 and 8d881e2.

📒 Files selected for processing (9)
  • Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (1 hunks)
  • Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (1 hunks)
  • Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (3 hunks)
  • Sources/AblyLiveObjects/Public/PublicTypes.swift (1 hunks)
  • Sources/AblyLiveObjects/Utility/Errors.swift (1 hunks)
  • Tests/AblyLiveObjectsTests/DefaultLiveCounterTests.swift (1 hunks)
  • Tests/AblyLiveObjectsTests/DefaultLiveMapTests.swift (2 hunks)
  • Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift (1 hunks)
  • Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (1)
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (1)
  • get (107-129)
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (1)
Sources/AblyLiveObjects/Utility/Errors.swift (1)
  • toARTErrorInfo (34-40)
Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (1)
Sources/AblyLiveObjects/Utility/Errors.swift (1)
  • toARTErrorInfo (34-40)
Tests/AblyLiveObjectsTests/DefaultLiveMapTests.swift (5)
Tests/AblyLiveObjectsTests/DefaultLiveCounterTests.swift (1)
  • arguments (10-23)
Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift (1)
  • arguments (652-665)
Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (1)
  • createZeroValued (54-63)
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (1)
  • createZeroValued (89-102)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
  • mapEntry (392-402)
Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (4)
Sources/AblyLiveObjects/Utility/Errors.swift (1)
  • toARTErrorInfo (34-40)
Sources/AblyLiveObjects/Utility/Assertions.swift (1)
  • notYetImplemented (2-7)
Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (1)
  • getObjectFromPool (81-85)
Tests/AblyLiveObjectsTests/Mocks/MockLiveMapObjectPoolDelegate.swift (1)
  • getObjectFromPool (22-24)
🔇 Additional comments (20)
Tests/AblyLiveObjectsTests/DefaultLiveCounterTests.swift (1)

21-21: LGTM: Enhanced error validation for consistency

The test now validates both the error code (90001) and HTTP status code (400), which aligns with the new LiveObjectsError structure. This provides more comprehensive error validation and ensures consistency with the structured error handling introduced across the SDK.

Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (1)

298-298: LGTM: Correctly adapted to throwable LiveMap.size property

The addition of try keywords before accessing .size property is necessary and correct, reflecting that the LiveMap protocol's size property is now throwable as part of the error handling standardization. These changes maintain the existing test logic while properly handling potential errors from the updated API.

Also applies to: 322-322, 394-394, 495-495, 498-498, 504-504, 516-516, 546-546

Tests/AblyLiveObjectsTests/DefaultRealtimeObjectsTests.swift (1)

663-663: LGTM: Strengthened error validation for consistency

The test enhancement to validate both error code (90001) and HTTP status code (400) ensures proper validation of the new LiveObjectsError structure. This change maintains consistency with similar updates across other test files and provides more robust error checking.

Sources/AblyLiveObjects/Internal/DefaultLiveCounter.swift (1)

72-76: LGTM: Excellent error handling standardization

The refactoring to use LiveObjectsError.objectsOperationFailedInvalidChannelState instead of directly creating ARTErrorInfo provides several benefits:

  1. Type safety: Uses a structured error enum instead of manual error creation
  2. Consistency: Aligns with error handling patterns across the SDK
  3. Maintainability: Centralizes error creation logic in the LiveObjectsError enum
  4. Clarity: The operation description "LiveCounter.value" makes debugging easier

The conversion via .toARTErrorInfo() ensures the final error format remains compatible while providing the structured approach internally.

Sources/AblyLiveObjects/DefaultRealtimeObjects.swift (1)

93-97: LGTM: Consistent error handling standardization

This change mirrors the excellent refactoring in DefaultLiveCounter.swift, replacing direct ARTErrorInfo creation with the structured LiveObjectsError.objectsOperationFailedInvalidChannelState. The benefits include:

  1. Consistency: Maintains the same error handling pattern across the SDK
  2. Type safety: Uses structured error types instead of manual creation
  3. Centralization: Leverages the centralized error logic in LiveObjectsError
  4. Debugging: The "getRoot" operation description aids in troubleshooting

This standardization effort across the codebase demonstrates good architectural decision-making.

Sources/AblyLiveObjects/Public/PublicTypes.swift (1)

214-223: LGTM! Excellent consistency improvement for error handling.

The change to make size, entries, keys, and values properties throwable aligns perfectly with the existing get(key:) method and follows the same error handling pattern. This ensures consistent behavior across all LiveMap access methods when the channel is in an invalid state.

Sources/AblyLiveObjects/Utility/Errors.swift (3)

6-16: Well-designed error enum with proper error code mapping.

The LiveObjectsError enum provides a clean abstraction for SDK-specific errors with proper mapping to ARTErrorCode. The use of channelOperationFailedInvalidState (90001) is appropriate for channel state validation failures.


18-32: Good error structure with consistent status codes and descriptive messages.

The computed properties provide consistent error information:

  • Status code 400 is appropriate for client errors
  • The localized description interpolates operation context effectively
  • Error message format is clear and actionable

34-41: Excellent conversion utility for framework integration.

The toARTErrorInfo() method properly bridges the custom error type to the Ably framework's expected error format, maintaining all necessary error details (code, status, message).

Tests/AblyLiveObjectsTests/DefaultLiveMapTests.swift (5)

21-21: Enhanced error assertion improves test precision.

The updated assertion now checks both error code (90001) and HTTP status code (400), ensuring complete validation of the standardized error structure introduced by LiveObjectsError.


266-297: Excellent comprehensive test coverage for new throwable properties.

The AccessPropertiesTests struct provides thorough validation of all four properties (size, entries, keys, values) with:

  • Proper error throwing validation for invalid channel states
  • Consistent error code and status code assertions
  • Clean test structure using parameterized actions

306-342: Thorough tombstone filtering validation across all properties.

This test effectively verifies that all properties correctly implement the RTLM14 specification for filtering tombstoned entries. The test data setup with mixed tombstone states (nil, false, true) provides comprehensive coverage.


349-378: Great consistency validation between related properties.

This test ensures that size, entries, keys, and values maintain logical consistency with each other, which is crucial for API reliability. The assertions properly verify that counts match and collections contain equivalent data.


383-431: Comprehensive value type testing aligns with RTLM5d2 specification.

This test excellently covers all supported value types (boolean, bytes, number, string, mapRef, counterRef) and verifies that the entries property correctly handles each type through the shared conversion logic. The test setup with mock objects is well-structured.

Sources/AblyLiveObjects/Internal/DefaultLiveMap.swift (6)

111-116: Excellent adoption of standardized error handling.

The refactoring to use LiveObjectsError.objectsOperationFailedInvalidChannelState provides consistent error reporting across the SDK. The conversion to ARTErrorInfo maintains compatibility with the Ably framework.


127-129: Smart refactoring to use shared conversion logic.

Delegating to the new convertEntryToLiveMapValue helper eliminates code duplication and ensures consistent value conversion across all access methods.


131-152: Proper implementation of size property with error handling and tombstone filtering.

The implementation correctly:

  • Validates channel state per RTLM10c
  • Uses standardized error handling
  • Counts only non-tombstoned entries per RTLM10d and RTLM14
  • Maintains thread safety with mutex locking

154-181: Well-implemented entries property with comprehensive filtering.

The implementation effectively:

  • Validates channel state per RTLM11c
  • Filters tombstoned entries per RTLM11d1 and RTLM14
  • Reuses the shared conversion logic for consistency
  • Returns proper key-value pairs as specified

183-195: Elegant implementation leveraging entries property.

Both keys and values properties smartly delegate to the entries property and map the results, ensuring complete consistency and avoiding code duplication. This follows the DRY principle perfectly.


447-502: Excellent consolidation of value conversion logic.

The convertEntryToLiveMapValue helper method provides:

  • Proper tombstone filtering per RTLM5d2a/RTLM14
  • Complete coverage of all value types per RTLM5d2b-g
  • Correct object pool delegation for referenced objects
  • Consistent null handling for invalid/missing data

This eliminates duplication and ensures all access methods use identical conversion logic.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ECO-5330-LiveMap-access-API

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Collaborator

@umair-ably umair-ably left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Base automatically changed from ECO-5332-object-sync to main July 24, 2025 16:26
@lawrence-forooghian lawrence-forooghian merged commit 36982fe into main Jul 24, 2025
17 checks passed
@lawrence-forooghian lawrence-forooghian deleted the ECO-5330-LiveMap-access-API branch July 24, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants