Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ECO-5035] Remove usage of Decodable for decoding data recieved from Realtime #200

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Dec 17, 2024

Switch to using JSONDecodable internally. This allows us to avoid the dance of encoding an ably-cocoa object to a Data just so that we can decode it using JSONDecoder. It also gives us consistency in how we handle data objects received from ably-cocoa.

This means that we now have to write a bit more code in order to decode objects manually, but I think we can live with it. (It might also be that we can revisit in the future and either automatically generate the JSONDecodable conformance using macros, or that we can find some other, better way of bridging with the compiler’s synthesised Decodable conformance.)

Resolves #84.

Summary by CodeRabbit

  • New Features

    • Enhanced JSON handling capabilities with new protocols for encoding and decoding.
    • Custom initializers for various structs and enums to improve JSON object handling.
  • Bug Fixes

    • Improved error handling in JSON decoding processes.
  • Documentation

    • Updated comments for clarity in JSON extraction methods.
  • Refactor

    • Removed automatic Codable conformance from several entities to implement a more manual JSON handling approach.

Missed some formatting in 8b8a084.
Switch to using JSONDecodable internally. This allows us to avoid the
dance of encoding an ably-cocoa object to a Data just so that we can
decode it using JSONDecoder. It also gives us consistency in how we
handle data objects received from ably-cocoa.

This means that we now have to write a bit more code in order to decode
objects manually, but I think we can live with it. (It might also be
that we can revisit in the future and either automatically generate the
JSONDecodable conformance using macros, or that we can find some other,
better way of bridging with the compiler’s synthesised Decodable
conformance.)

Resolves #84.
Copy link

coderabbitai bot commented Dec 17, 2024

Walkthrough

The pull request introduces a comprehensive refactoring of JSON decoding and encoding across multiple Swift files in the AblyChat module. The primary focus is transitioning from Swift's standard Codable protocol to custom JSONDecodable and JSONEncodable protocols. This change aims to provide more granular control over JSON parsing, removing the dependency on JSONSerialization and DictionaryDecoder. The modifications affect several key structs and enums, including Message, OccupancyEvent, MessageAction, and PaginatedResult, enhancing the flexibility of JSON handling within the codebase.

Changes

File Changes
Sources/AblyChat/ChatAPI.swift - Updated SendMessageResponse to use JSONObjectDecodable
- Modified makeRequest and makePaginatedRequest methods to use JSONDecodable
- Removed DictionaryDecoder
Sources/AblyChat/Events.swift - Removed Codable from MessageAction enum
Sources/AblyChat/Headers.swift - Removed Codable from HeadersValue enum
Sources/AblyChat/JSONCodable.swift - Added new protocols: JSONEncodable, JSONDecodable, JSONObjectEncodable, JSONObjectDecodable
- Added new type aliases
- Added new error case and dictionary value extraction methods
Sources/AblyChat/Message.swift - Removed Codable from Message struct
- Added JSONObjectDecodable extension with custom initializer
Sources/AblyChat/Metadata.swift - Removed Codable from MetadataValue enum
Sources/AblyChat/Occupancy.swift - Removed Encodable/Decodable from OccupancyEvent struct
- Added JSONObjectDecodable extension
Sources/AblyChat/PaginatedResult.swift - Updated PaginatedResult protocol and related types to use JSONDecodable

Assessment against linked issues

Objective Addressed Explanation
Improve decoding to a dictionary within getRequest
Remove dependency on JSONSerialization

Possibly related PRs

Suggested reviewers

  • maratal
  • umair-ably

Poem

🐰 Hop, hop, through JSON's maze,
Decoding data in clever ways,
Codable's gone, new protocols shine,
Swift's parsing now looks mighty fine!
A rabbit's dance of type delight! 🌟


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?

❤️ 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.
    • Generate unit testing code for this file.
    • 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 generate unit testing code for this file.
    • @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 generate unit testing code.
    • @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.

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. (Beta)
  • @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

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (5)
Sources/AblyChat/JSONValue.swift (1)

Line range hint 162-164: Consider enhancing error handling for paginated results

The current error handling uses preconditionFailure, which is appropriate for development-time checks. However, with the addition of paginated results handling, consider if runtime error handling might be more suitable for cases where item types are unknown until runtime.

Consider this alternative approach:

-            preconditionFailure("JSONValue(ablyCocoaData:) was given \(ablyCocoaData)")
+            throw JSONValueError.unsupportedType(String(describing: type(of: ablyCocoaData)))

This would require adding:

enum JSONValueError: Error {
    case unsupportedType(String)
}

And updating the initializer signature:

init(ablyCocoaData: Any) throws
Sources/AblyChat/PaginatedResult.swift (2)

35-36: LGTM: Efficient decoding implementation

The new decoding implementation efficiently handles the conversion from Ably Cocoa data to the target type through JSONValue, eliminating the need for intermediate encoding/decoding steps.

Consider adding a performance test to measure the impact of this change, especially for large datasets. This could help validate the optimization and serve as a benchmark for future improvements mentioned in the PR objectives (e.g., macro-generated conformance).


Line range hint 11-11: Consider addressing the unwrap TODO

While updating the decoding implementation, this might be a good opportunity to address the TODO regarding unwrap necessity.

Would you like me to propose a solution for avoiding the unwrap in the next property? This could involve using Swift's new optional binding patterns or a different approach to handling the optional PaginatedResult.

Sources/AblyChat/Headers.swift (1)

1-1: Consider tracking the TODO comment

The TODO comment references issue #13 for potential improvements to this type.

Would you like me to help track this by opening a new GitHub issue or linking it to the existing one?

Sources/AblyChat/Occupancy.swift (1)

68-75: Consider adding validation for occupancy numbers

While the JSON parsing looks correct, consider adding validation to ensure:

  1. The numbers are non-negative (as negative occupancy doesn't make sense)
  2. Graceful handling of number parsing failures

Here's a suggested implementation:

 internal init(jsonObject: [String: JSONValue]) throws {
+    let connections = Int(jsonObject.numberValueForKey("connections"))
+    let presenceMembers = Int(jsonObject.numberValueForKey("presenceMembers"))
+    
+    guard connections >= 0, presenceMembers >= 0 else {
+        throw JSONDecodingError.invalidValue("Occupancy counts must be non-negative")
+    }
+    
     try self.init(
-        connections: Int(jsonObject.numberValueForKey("connections")),
-        presenceMembers: Int(jsonObject.numberValueForKey("presenceMembers"))
+        connections: connections,
+        presenceMembers: presenceMembers
     )
 }
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 315d162 and 526a220.

📒 Files selected for processing (9)
  • Sources/AblyChat/ChatAPI.swift (4 hunks)
  • Sources/AblyChat/Events.swift (1 hunks)
  • Sources/AblyChat/Headers.swift (1 hunks)
  • Sources/AblyChat/JSONCodable.swift (8 hunks)
  • Sources/AblyChat/JSONValue.swift (1 hunks)
  • Sources/AblyChat/Message.swift (2 hunks)
  • Sources/AblyChat/Metadata.swift (1 hunks)
  • Sources/AblyChat/Occupancy.swift (2 hunks)
  • Sources/AblyChat/PaginatedResult.swift (4 hunks)
🔇 Additional comments (18)
Sources/AblyChat/JSONValue.swift (1)

139-139: Verify handling of ARTHTTPPaginatedResult items

The documentation addition is clear and aligns with the PR's objective of improving JSON handling. Let's verify that the implementation correctly handles all possible item types from ARTHTTPPaginatedResult.

✅ Verification successful

Let me gather more information about the implementation to verify the handling of ARTHTTPPaginatedResult items.


Let me try one more search to find any usage of ARTHTTPPaginatedResult to understand how its items are handled.


Let me check the implementation of PaginatedResult and its tests to understand how items are handled.


Documentation accurately reflects ARTHTTPPaginatedResult items handling

The implementation in JSONValue.swift correctly handles all possible JSON types from ARTHTTPPaginatedResult's items array, as evidenced by:

  • The init(ablyCocoaData:) implementation handles all JSON-compatible types (dictionaries, arrays, strings, numbers, booleans, null)
  • The actual usage in PaginatedResultWrapper shows items are properly converted using JSONValue(ablyCocoaData:) before decoding
  • The error handling with preconditionFailure is appropriate as it helps catch any unexpected types during development
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for ARTHTTPPaginatedResult usage to verify item types
# Expected: Find all places where ARTHTTPPaginatedResult items are converted to JSONValue

# Search for ARTHTTPPaginatedResult usage
rg -A 5 "ARTHTTPPaginatedResult" 

# Search for potential item type declarations
ast-grep --pattern 'let $item: $type = $paginatedResult.items[$_]'

Length of output: 583


Script:

#!/bin/bash
# Search for the complete implementation of init(ablyCocoaData:) to verify type handling
ast-grep --pattern 'init(ablyCocoaData: Any) {
  $$$
}'

# Search for any tests involving ARTHTTPPaginatedResult
rg -l "ARTHTTPPaginatedResult.*test"

# Search for actual usage of JSONValue with paginated results
rg "JSONValue.*ARTHTTPPaginatedResult" -A 5

Length of output: 2238


Script:

#!/bin/bash
# Search for ARTHTTPPaginatedResult definition or usage
rg "ARTHTTPPaginatedResult" -B 2 -A 5

# Search for any files that might contain tests or usage of paginated results
fd -e swift -x grep -l "PaginatedResult"

Length of output: 1217


Script:

#!/bin/bash
# Check PaginatedResult implementation
cat Sources/AblyChat/PaginatedResult.swift

# Check relevant test implementations
rg -A 10 "PaginatedResult.*items" Tests/AblyChatTests/

# Look for actual conversion of paginated items to JSONValue
rg -A 5 "items.*JSONValue" 

Length of output: 5354

Sources/AblyChat/PaginatedResult.swift (3)

16-16: LGTM: Type constraint change aligns with project goals

The transition from Codable to JSONDecodable while maintaining Sendable & Equatable constraints is appropriate and maintains thread safety and comparison capabilities.


50-50: LGTM: Consistent type constraint updates

The generic constraints are consistently updated throughout the type hierarchy, maintaining type safety and protocol conformance.

Also applies to: 100-100


16-16: Verify all pagination implementations

Let's ensure all code using these pagination classes has been updated to use JSONDecodable.

Also applies to: 50-50

✅ Verification successful

All pagination implementations correctly use JSONDecodable

Based on the search results, all pagination-related implementations consistently use JSONDecodable & Sendable & Equatable as their type constraints:

  • ARTHTTPPaginatedCallbackWrapper<Response: JSONDecodable & Sendable & Equatable>
  • PaginatedResultWrapper<T: JSONDecodable & Sendable & Equatable>
  • makePaginatedRequest<Response: JSONDecodable & Sendable & Equatable>
  • toPaginatedResult<T: JSONDecodable & Sendable>

There are no instances of Codable being used with pagination-related types, confirming that all implementations have been properly updated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining Codable usage in pagination-related code
rg -l "PaginatedResult|Paginated.*Wrapper" | xargs rg "Codable"

# Search for implementations of PaginatedResult to verify they're updated
ast-grep --pattern 'class $_ : $$$PaginatedResult$$$'

Length of output: 119


Script:

#!/bin/bash
# Let's try a different approach to find pagination-related code and their type constraints

# Search for files containing PaginatedResult
rg -l "PaginatedResult"

# Search for any type definitions with Paginated prefix
ast-grep --pattern 'struct $name: $$$' --lang swift | rg "Paginated"
ast-grep --pattern 'class $name: $$$' --lang swift | rg "Paginated"

# Look for actual usage of ARTHTTPPaginatedCallbackWrapper
rg "ARTHTTPPaginatedCallbackWrapper" -A 2 -B 2

# Search for any pagination-related protocol conformance
rg "JSONDecodable|Decodable" -A 1 -B 1 | rg -A 1 -B 1 "Paginated"

Length of output: 5816

Sources/AblyChat/ChatAPI.swift (4)

18-25: Update SendMessageResponse to conform to JSONObjectDecodable

The SendMessageResponse struct now conforms to JSONObjectDecodable with a custom initializer that parses the JSON object. This change aligns with the new JSON decoding strategy and improves consistency across the codebase.


70-70: Modify makeRequest to accept Response: JSONDecodable

Updating the makeRequest function to accept a generic Response type conforming to JSONDecodable is a necessary adjustment for the new JSON handling approach. This enhances the flexibility and type safety of the decoding process.


92-93: Handle JSON decoding using JSONDecodable initializer

By converting ablyCocoaData to a JSONValue and initializing the Response using the JSONDecodable protocol, the code effectively utilizes the new custom decoding mechanism. This change ensures more precise control over the decoding process.


Line range hint 105-113: Update makePaginatedRequest to use Response: JSONDecodable

The makePaginatedRequest function now requires Response to conform to JSONDecodable, which is consistent with the updated decoding strategy. This enhances uniformity in how paginated responses are handled throughout the API.

Sources/AblyChat/JSONCodable.swift (4)

1-2: Include necessary import for Foundation types

Adding import Foundation is appropriate to access Date and other Foundation types used in the new extensions. This ensures all dependencies are correctly imported.


32-32: Add failedToDecodeFromRawValue to JSONValueDecodingError

Introducing the failedToDecodeFromRawValue case enhances error handling when decoding raw representable values. This provides clearer feedback on decoding failures due to invalid raw values.


234-263: Implement date extraction methods from JSON

The new methods ablyProtocolDateValueForKey and optionalAblyProtocolDateValueForKey facilitate extracting Date objects from JSON values representing milliseconds since the Unix epoch. This aligns with Ably's date format and improves date handling consistency.


265-300: Add methods for decoding RawRepresentable types

The added methods rawRepresentableValueForKey and optionalRawRepresentableValueForKey enhance the ability to decode enums and other RawRepresentable types from JSON strings. This makes the decoding process more robust and type-safe.

Sources/AblyChat/Events.swift (1)

6-6: Remove Codable conformance from MessageAction enum

Eliminating the Codable conformance from MessageAction reflects the shift towards manual JSON handling using JSONDecodable. This change prevents unnecessary automatic encoding/decoding and provides more control over the process.

Sources/AblyChat/Metadata.swift (1)

3-3: Remove Codable conformance from MetadataValue

The removal of Codable conformance from MetadataValue aligns with the new custom JSON encoding and decoding approach using JSONEncodable and JSONDecodable. This modification enhances consistency and control over metadata serialization.

Sources/AblyChat/Headers.swift (1)

3-7: LGTM: Removal of Codable conformance aligns with project goals

The change to remove Codable while maintaining Sendable and Equatable is appropriate, as it aligns with the PR's objective to transition away from Decodable for realtime data handling.

Sources/AblyChat/Occupancy.swift (1)

Line range hint 51-66: LGTM: Clean removal of Codable conformance

The removal of Codable conformance while maintaining Sendable aligns with the project's goal to handle JSON decoding manually.

Sources/AblyChat/Message.swift (2)

16-16: LGTM: Clean removal of Codable conformance

The removal of Codable while maintaining Sendable, Identifiable, and Equatable is appropriate and aligns with the project goals.


92-103: LGTM: Robust JSON parsing implementation

The implementation correctly:

  • Maps JSON keys (clientId/roomId) to Swift properties (clientID/roomID)
  • Handles optional date parsing
  • Properly propagates errors from nested JSON parsing

However, consider verifying the impact of this change on the message publishing flow.

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.

Improve decoding to a dictionary within getRequest
1 participant