[ECO-5035] Remove usage of Decodable for decoding data recieved from Realtime
#200
Conversation
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.
WalkthroughThe 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 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Sources/AblyChat/JSONValue.swift (1)
Line range hint
162-164: Consider enhancing error handling for paginated resultsThe 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) throwsSources/AblyChat/PaginatedResult.swift (2)
35-36: LGTM: Efficient decoding implementationThe 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 TODOWhile 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
nextproperty? 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 commentThe 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 numbersWhile the JSON parsing looks correct, consider adding validation to ensure:
- The numbers are non-negative (as negative occupancy doesn't make sense)
- 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
📒 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
PaginatedResultWrappershows items are properly converted usingJSONValue(ablyCocoaData:)before decoding - The error handling with
preconditionFailureis 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.
Switch to using
JSONDecodableinternally. This allows us to avoid the dance of encoding an ably-cocoa object to aDatajust so that we can decode it usingJSONDecoder. 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
JSONDecodableconformance using macros, or that we can find some other, better way of bridging with the compiler’s synthesisedDecodableconformance.)Resolves #84.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor