[ECO-5178] Fix sending and receiving of message headers and metadata #196
Conversation
Will be helpful when I add JSONEncodable support to other types.
We (erroneously) never read this argument, but it hasn’t been a problem because we never pass it either.
For consistency with JSONValue, which seems like the right thing to do API-wise and also will make some upcoming work easier.
In an upcoming commit, I intend to use these methods in a few more situations other than just presence.
We were not serializing or deserializing the Chat SDK’s Metadata and Headers types. This was a mistake in 8b6e56a. I was surprised to find that there were no existing tests for the request that ChatAPI makes when you call sendMessage, nor for DefaultMessages’s handling of messages received over a realtime channel, which meant that there wasn’t an obvious place to slot in the tests for the fixes introduced by this commit, nor did the mocks have any support for writing such tests. I’ve added tests for my fixes but, given my rush to get this fix done, the changes to the mocks aren’t great. Have created issue #195 for us come back and write the missing tests and tidy mine up. Note that I’ve removed the optionality of Metadata’s values. This optionality came from 20e7f5f when it was unclear what Metadata would be. We probably should have removed it in 8b6e56a when we introduced `null` as a MetadataValue case. Resolves #193.
WalkthroughThe pull request introduces significant modifications across multiple files in the AblyChat module, focusing on enhancing type safety and structured data handling, particularly with JSON. Key changes include updating method signatures to utilize a new 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: 4
🧹 Outside diff range and nitpick comments (9)
Sources/AblyChat/Headers.swift (1)
5-5: Potential Precision Loss withDoublefor Numeric ValuesChanging the
numbercase fromNSNumbertoDoublemight introduce precision loss, especially for large integer values that exceedDouble's precision. Consider whether it's necessary to represent all numeric values asDouble, or if preserving integer types is important for your use case.Sources/AblyChat/Metadata.swift (1)
6-6: Consideration Needed for ChangingnumberCase toDoubleSwitching from
InttoDoublein thenumbercase may lead to unintended precision issues with integer values. Evaluate whether this change may affect the integrity of integer metadata and if alternative solutions are needed.Tests/AblyChatTests/JSONValueTests.swift (1)
Line range hint
149-154: Consider adding error handling test casesWhile the happy path is well tested, consider adding test cases for error scenarios in JSON serialization.
+ @Test + func toAblyCocoaData_handlesInvalidJSON() { + // Test invalid JSON scenarios + let invalidValue: JSONValue = ["invalid": Double.infinity] + #expect(throws: JSONSerialization.InvalidJSONError.self) { + try JSONSerialization.data(withJSONObject: invalidValue.toAblyCocoaData) + } + }Sources/AblyChat/ChatAPI.swift (1)
31-40: LGTM! Consider simplifying the mapValues transformations.The type-safe JSON handling looks good and correctly follows the CHA-M3b spec for optional metadata/headers.
Consider simplifying the transformations using the
JSONValuetype's expressible protocol conformance:-body["metadata"] = .object(metadata.mapValues(\.toJSONValue)) +body["metadata"] = metadata -body["headers"] = .object(headers.mapValues(\.toJSONValue)) +body["headers"] = headersThis works because
DictionarywhereValue: JSONEncodableautomatically converts toJSONValue.Sources/AblyChat/JSONValue.swift (1)
Line range hint
131-161: LGTM! Consider adding @throws to documentation.The initialization logic and type handling look great. The documentation is clear but could be enhanced.
Consider adding
@throwsdocumentation to clarify when the preconditionFailure might occur:/// Creates a `JSONValue` from an ably-cocoa deserialized JSON object. /// /// Specifically, `ablyCocoaData` can be: /// /// - a non-`nil` value of `ARTPresenceMessage`'s `data` property /// - a non-`nil` value of `ARTMessage`'s `data` property /// - the return value of the `toJSON()` method of a non-`nil` value of `ARTMessage`'s `extras` property +/// +/// @throws preconditionFailure if the input cannot be converted to a valid JSON valueTests/AblyChatTests/ChatAPITests.swift (2)
55-76: LGTM! Consider strengthening the assertions.The test effectively verifies headers serialization with good test structure.
Consider adding assertions for the complete request body structure:
let requestBody = try #require(realtime.requestArguments.first?.body as? NSDictionary) #expect(try #require(requestBody["headers"] as? NSObject) == ["numberKey": 10, "stringKey": "hello"] as NSObject) +#expect(requestBody.allKeys.count == 2) // Verify only text and headers are present +#expect(try #require(requestBody["text"] as? String) == "")
78-99: LGTM! Consider strengthening the assertions similarly.The test effectively verifies metadata serialization with good consistency.
Consider adding assertions for the complete request body structure, similar to the headers test:
let requestBody = try #require(realtime.requestArguments.first?.body as? NSDictionary) #expect(try #require(requestBody["metadata"] as? NSObject) == ["numberKey": 10, "stringKey": "hello"] as NSObject) +#expect(requestBody.allKeys.count == 2) // Verify only text and metadata are present +#expect(try #require(requestBody["text"] as? String) == "")Tests/AblyChatTests/IntegrationTests.swift (1)
118-124: Good test coverage for headers and metadata.The test effectively verifies sending and receiving of message headers and metadata with different value types (numbers and strings). Consider adding test cases for:
- Empty metadata/headers
- Nested objects in metadata/headers
- Array values in metadata/headers
Sources/AblyChat/DefaultMessages.swift (1)
80-89: Consider adding error handling for invalid value types.While the parsing logic is solid, consider adding specific error messages when metadata or headers contain invalid value types. This would help developers identify issues during development.
Example improvement:
let metadata: Metadata? = if let metadataJSONObject = data["metadata"]?.objectValue { - try metadataJSONObject.mapValues { try MetadataValue(jsonValue: $0) } + try? metadataJSONObject.mapValues { jsonValue in + do { + return try MetadataValue(jsonValue: jsonValue) + } catch { + logger.log(message: "Invalid metadata value type: \(error)", level: .error) + return nil + } + } } else { nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Sources/AblyChat/ChatAPI.swift(3 hunks)Sources/AblyChat/DefaultMessages.swift(2 hunks)Sources/AblyChat/DefaultPresence.swift(4 hunks)Sources/AblyChat/Headers.swift(1 hunks)Sources/AblyChat/JSONCodable.swift(1 hunks)Sources/AblyChat/JSONValue.swift(2 hunks)Sources/AblyChat/Metadata.swift(1 hunks)Sources/AblyChat/PresenceDataDTO.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(2 hunks)Tests/AblyChatTests/JSONValueTests.swift(6 hunks)Tests/AblyChatTests/Mocks/MockRealtime.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(3 hunks)Tests/AblyChatTests/PresenceDataDTOTests.swift(3 hunks)
🔇 Additional comments (23)
Sources/AblyChat/JSONCodable.swift (1)
1-9: Well-Defined Protocols for JSON Encoding and Decoding
The introduction of JSONEncodable, JSONDecodable, and the JSONCodable typealias provides a clean and structured approach to JSON handling. This design promotes code reusability and clarity.
Sources/AblyChat/PresenceDataDTO.swift (1)
8-32: Implementation of JSONCodable Conformance is Correct
The conformance of PresenceDataDTO to JSONCodable is properly implemented. The initializer and the computed property handle JSON serialization and deserialization effectively.
Tests/AblyChatTests/PresenceDataDTOTests.swift (2)
5-5: LGTM: Improved section headers clarity
The section headers now better reflect the protocol names (JSONDecodable, JSONCodable), making the test organization clearer and more aligned with the implementation.
Also applies to: 25-25
37-39: LGTM: Enhanced type safety in JSON value comparison
The test method has been correctly updated to use toJSONValue and properly compares the result using .object(expectedJSONObject), ensuring type-safe JSON value comparison.
Tests/AblyChatTests/Mocks/MockRealtime.swift (3)
11-13: LGTM: Well-implemented thread safety mechanism
Good implementation of thread safety using NSLock and proper documentation of synchronization requirements.
88-91: LGTM: Proper mutex usage in request handling
The implementation correctly uses mutex locks to ensure thread-safe access to request arguments, both when recording new requests and when accessing the stored arguments.
Also applies to: 99-105
6-6: Verify thread safety implementation with @unchecked Sendable
The @unchecked Sendable attribute indicates that we're manually ensuring thread safety. While the implementation looks correct with mutex usage, we should verify all mutable state is properly synchronized.
✅ Verification successful
Let me gather more specific information about the mutable properties in MockRealtime.swift.
Thread safety implementation is properly verified
The implementation is correctly synchronized:
- The only mutable state
_requestArgumentsis properly protected with anNSLock - All access to this state is guarded by the mutex lock (in the
requestArgumentsgetter) - Other properties are either computed or immutable
- The implementation follows the correct pattern for
@unchecked Sendableconformance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any mutable properties in the class that might need synchronization
rg -l "var\s+\w+\s*(:|\=)" | xargs rg "class\s+\w+.*Sendable"
Length of output: 2352
Script:
#!/bin/bash
# Search for var declarations in MockRealtime.swift
rg "var " Tests/AblyChatTests/Mocks/MockRealtime.swift -A 2
# Search for mutex/lock usage
rg "NSLock|DispatchQueue|actor" Tests/AblyChatTests/Mocks/MockRealtime.swift
Length of output: 836
Tests/AblyChatTests/JSONValueTests.swift (2)
6-6: LGTM: Improved clarity in test organization
The section headers have been updated to reflect the broader scope of JSON conversion, making the test structure clearer and more maintainable.
Also applies to: 78-78
26-27: LGTM: Consistent renaming for broader applicability
The method names have been updated to remove "Presence" specificity, making them more generic and reusable. The implementation remains correct while becoming more versatile.
Also applies to: 32-32, 98-99
Sources/AblyChat/ChatAPI.swift (2)
66-75: LGTM! Clean type-safe conversion to Ably Cocoa format.
The body conversion and request execution are well-implemented with proper error handling.
102-102: LGTM! Good cleanup removing unused parameter.
Removing the unused body parameter improves the API clarity.
Sources/AblyChat/JSONValue.swift (1)
163-174: LGTM! Well-documented conversion logic.
The property documentation and implementation correctly handle all JSON value types.
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
72-104: Well-structured test for header extraction!
The test effectively validates the extraction and type-safe conversion of message headers, covering both numeric and string values.
106-137: Consistent test implementation for metadata extraction!
The test maintains consistency with the headers test pattern while properly validating metadata extraction from the message data field.
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (3)
34-42: LGTM! Clean initializer update.
The initializer properly integrates the new message emission capability while maintaining backward compatibility.
145-154: LGTM! Comprehensive message construction.
The subscribe method properly constructs the ARTMessage with all necessary fields from MessageToEmit.
20-27: Track the TODO for type safety improvements.
The @unchecked Sendable annotation is a temporary solution. The referenced issue #195 should address proper type constraints.
✅ Verification successful
Issue #195 is still open and needs attention for type safety improvements
The TODO comment references issue #195 which is currently open with the title "Improve testing for messages functionality". The temporary use of @unchecked Sendable should be addressed as part of this issue to improve type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if issue #195 is still open
gh issue view 195 --json state,title
Length of output: 108
Sources/AblyChat/DefaultPresence.swift (3)
118-118: LGTM! Improved JSON value handling.
The change properly utilizes the new JSON value conversion chain for presence data.
152-152: LGTM! Consistent JSON value handling across methods.
The update and leave methods maintain consistency with the new JSON value conversion approach.
Also applies to: 186-186
240-240: LGTM! Consistent decoder update.
The decoder properly adopts the new JSON value initialization approach.
Tests/AblyChatTests/IntegrationTests.swift (1)
104-108: LGTM! Improved code formatting.
The multi-line formatting enhances readability while maintaining the same functionality.
Sources/AblyChat/DefaultMessages.swift (2)
59-61: LGTM! Improved type safety with JSONValue.
The use of JSONValue for parsing message data enhances type safety and provides better error handling compared to direct dictionary casting.
66-68: LGTM! Consistent use of JSONValue for extras parsing.
The parsing of extras follows the same pattern as message data, maintaining consistency and type safety.
Resolves #193. See commit messages for more details.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation