[ECO-5181] Fix sending and receiving of reaction metadata and headers #199
Conversation
Mistake in 80e8585.
Mistake in 80e8585.
Add a JSONObjectCodable protocol which represents a type that can be serialised to or deserialised from a [String : JSONValue] dictionary. As well as slightly simplifying the deserialisation of a type (you don't have to do the "is it an object" check yourself), this will be necessary when, in an upcoming commit, we try to pass a serialised value to the `extras` parameter of ably-cocoa’s publish, which expects an `ARTJsonCompatible` value.
Will use these in an upcoming commit.
Similar issue to the one which was fixed in 7fcab5c. Haven’t added tests for the handling of the incoming message, since there were no existing tests to update and I didn’t feel like writing them; will leave this for whoever writes these tests in #88. The approach that I’ve taken here of using a DTO is consistent with the approach that we use for presence data. I should have done this in 7fcab5c too; will do it separately. The spec doesn’t describe how to behave if the user doesn’t pass headers or metadata; I’ve taken the behaviour of passing an empty object from the JS Chat SDK at 6d1b04a. Have created spec issue [1] for specifying this. Resolves #198. [1] ably/specification#256
|
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)Sources/AblyChat/DefaultMessages.swiftAn unexpected error occurred while running ast-grep. WalkthroughThe pull request introduces significant changes to the JSON handling and reaction mechanisms in the AblyChat module. The modifications focus on enhancing the robustness of JSON encoding, decoding, and reaction processing. Key changes include introducing new protocols for JSON object handling, creating a structured Changes
Assessment against linked issues
Possibly related PRs
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 (2)
Tests/AblyChatTests/RoomReactionDTOTests.swift (2)
4-62: LGTM! Well-structured test suite for RoomReactionDTO.Data.The test coverage is comprehensive, including:
- Error handling for invalid inputs
- Handling of missing optional fields
- JSON encoding/decoding validation
Consider adding these edge cases:
- Test with empty string for type
- Test with empty object for metadata
- Test with null values in metadata
+ @Test + func initWithJSONValue_withEmptyType() throws { + #expect(throws: JSONValueDecodingError.self) { + try RoomReactionDTO.Data(jsonValue: ["type": ""]) + } + } + + @Test + func initWithJSONValue_withEmptyMetadata() throws { + let data = try RoomReactionDTO.Data( + jsonValue: [ + "type": "someType", + "metadata": [:], + ] + ) + #expect(data.metadata?.isEmpty == true) + }
64-112: LGTM! Thorough test coverage for RoomReactionDTO.Extras.The test suite properly validates the Extras structure with similar thoroughness to the Data tests.
Consider adding similar edge cases for headers:
- Test with empty object for headers
- Test with null values in headers
+ @Test + func initWithJSONValue_withEmptyHeaders() throws { + let data = try RoomReactionDTO.Extras( + jsonValue: [ + "headers": [:], + ] + ) + #expect(data.headers?.isEmpty == true) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/DefaultRoomReactions.swift(3 hunks)Sources/AblyChat/JSONCodable.swift(1 hunks)Sources/AblyChat/JSONValue.swift(3 hunks)Sources/AblyChat/PresenceDataDTO.swift(1 hunks)Sources/AblyChat/RoomReactionDTO.swift(1 hunks)Sources/AblyChat/RoomReactions.swift(0 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/PresenceDataDTOTests.swift(1 hunks)Tests/AblyChatTests/RoomReactionDTOTests.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/RoomReactions.swift
🔇 Additional comments (18)
Sources/AblyChat/DefaultRoomReactions.swift (4)
26-33: Proper construction and publishing of RoomReactionDTO
The send method now correctly constructs a RoomReactionDTO using the provided parameters and publishes it on the channel with the appropriate data and extras. This aligns with the new data structures introduced and ensures that reactions are sent in the expected format.
47-48: Robust error handling for incoming messages
The added guard statements ensure that essential components of the incoming message (data, clientId, timestamp, and extras) are present before processing. This improves the robustness of the message handling by preventing the processing of incomplete or malformed messages.
Also applies to: 59-59
63-66: Correct parsing of message data into RoomReactionDTO
The code correctly initializes a RoomReactionDTO from the message's data and extras, handling any potential errors during deserialization. This allows for a structured and reliable way to convert incoming messages into reaction objects.
70-72: Accurate creation of Reaction object
The Reaction object is accurately constructed using the data from dto, with appropriate default values for metadata and headers when they are nil. This ensures that all necessary information is included when emitting reactions to subscribers.
Sources/AblyChat/JSONCodable.swift (4)
10-21: Introduction of JSONObjectEncodable improves JSON serialization
The addition of the JSONObjectEncodable protocol and its default implementation enhances the JSON encoding process by providing a standardized way to convert objects to JSON dictionaries. This promotes code reuse and consistency across the module.
22-41: Addition of JSONObjectDecodable streamlines JSON deserialization
By introducing the JSONObjectDecodable protocol with a default initializer, the code simplifies the deserialization of JSON objects. The JSONValueDecodingError enum provides clear error cases, improving error handling during the decoding process.
43-44: Typealias JSONObjectCodable enhances code clarity
The JSONObjectCodable typealias consolidates the encodable and decodable protocols, making it easier to declare types that conform to both, thereby improving code readability.
46-230: Comprehensive helper methods for JSON value extraction
The extension on [String: JSONValue] provides robust helper methods for extracting typed values from a JSON dictionary. This reduces boilerplate code and standardizes JSON parsing throughout the codebase.
Sources/AblyChat/PresenceDataDTO.swift (1)
6-24: Updated conformance to JSONObjectCodable simplifies JSON handling
Changing the conformance from JSONCodable to JSONObjectCodable and adjusting the initializer and serialization methods enhances the clarity and simplicity of the PresenceDataDTO structure. The code now directly works with JSON objects, reducing complexity in JSON transformations.
Sources/AblyChat/RoomReactionDTO.swift (1)
1-70: Well-structured implementation of RoomReactionDTO
The introduction of RoomReactionDTO along with its nested Data and Extras structs provides a clear and organized way to represent room reactions. The implementations of JSONObjectCodable for these structs facilitate reliable JSON serialization and deserialization.
Tests/AblyChatTests/PresenceDataDTOTests.swift (1)
19-24: LGTM! Good addition of error handling test.
The new test case properly validates that non-object JSON values are rejected with the appropriate error type. This aligns with the broader changes to standardize JSON handling across the codebase.
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (2)
33-34: LGTM! More descriptive test data.
The test data now uses clearer key names that better indicate their purpose (e.g., "someMetadataKey", "someHeadersKey").
42-43: Verify the message format matches the API specification.
The test expectations now properly separate the reaction data and extras into their respective structures. However, let's verify this matches the API specification.
✅ Verification successful
Let me gather more information about the message format specification by looking at the test file mentioned in the results.
Let me check the implementation of the reaction data structure to fully verify the format.
Message format matches the API specification
The test expectations correctly match the internal message format defined in RoomReactionDTO, which separates reaction data (type and metadata) from extras (headers) as per specification CHA-ER3a. The format aligns with the Reaction type structure and its JSON encoding implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for API documentation or interface definitions that specify the expected message format
# Look for API interface definitions
rg -A 5 "interface.*Reaction|type.*Reaction|protocol.*Reaction"
# Look for documentation comments about reaction message format
rg -B 2 -A 2 "@spec CHA-ER"
Length of output: 5044
Script:
#!/bin/bash
# Look at the test file content to understand the specification requirements
cat Tests/AblyChatTests/DefaultRoomReactionsTests.swift
Length of output: 3671
Script:
#!/bin/bash
# Look at the RoomReactionDTO and Reaction implementations
cat Sources/AblyChat/RoomReactionDTO.swift Sources/AblyChat/Reaction.swift
Length of output: 2786
Sources/AblyChat/JSONValue.swift (2)
170-174: LGTM: Improved code organization
The change to use toAblyCocoaDataDictionary improves code organization by extracting the dictionary conversion logic into a dedicated method.
189-205: LGTM: Well-structured extension with clear separation of concerns
The new extension provides two well-documented methods:
toAblyCocoaDataDictionary: Handles conversion to Ably Cocoa's data formattoARTJsonCompatible: Provides type-safe casting to ARTJsonCompatible
The implementation is clean and follows Swift best practices.
Sources/AblyChat/DefaultMessages.swift (1)
Line range hint 80-89: LGTM: Improved error handling for metadata and headers extraction
The changes enhance robustness by:
- Using
optionalObjectValueForKeyfor safer extraction - Properly handling optional values with clear nil-coalescing
- Maintaining consistent error handling patterns across metadata and headers
Tests/AblyChatTests/IntegrationTests.swift (2)
168-174: LGTM: Comprehensive reaction sending test
The test properly includes metadata and headers in the reaction parameters, maintaining consistency with the message sending tests above.
177-178: LGTM: Clear assertions for metadata and headers
The assertions properly verify that both metadata and headers are correctly received and match the sent data.
Resolves #198. See commit messages for more details.
Summary by CodeRabbit
New Features
RoomReactionDTO.Bug Fixes
Tests
RoomReactionDTOand updated existing tests forDefaultRoomReactionsandPresenceDataDTO.Chores