[ECO-5390] Encode and decode binary and JSON data per spec #18
Conversation
Have to add a bunch of boilerplate each time we introduce a new error type, to no current benefit.
lawrence-forooghian
changed the base branch from
main
to
initial-protocol-changes
github-actions
bot
temporarily deployed
to
staging/pull/18/AblyLiveObjects
We're going to introduce an additional decoding step in which the ObjectMessages sent and received over the wire need to have some binary and JSON data encoded per the rules of the specification. So introduce new types that will represent the ObjectMessage after it's gone through this upcoming processing. Most of this code was generated by Cursor at my instruction.
Introduces the `format` argument for encoding and decoding ObjectMessages; we'll use this argument shortly to handle binary data.
This provides us with a JSONValue-like type which also supports binary data. We'll use this in an upcoming commit for encoding and decoding the binary data contained within an ObjectMessage. (Note about naming: the term "wire object" is a bit overloaded now; it's used in protocols like WireObjectCodable and it also appears as a prefix to the name of WireObjectMessage, and the two meanings have nothing to do with each other. I didn't have a better idea for naming though and didn't want to get hung up on it.) This was initially implemented by asking Cursor to copy JSONValue; I then realised that I'd made some mistakes in what I'd asked it to do, so had to make a bunch of corrections myself and also introduced the ExtendedJSONValue type. The MessagePack decoding tests (the ones that make comparisons to hand-crafted byte arrays) were generated by Cursor, and I have not verified the correctness of these byte arrays; my main assurance that they're probably correct is that the tests pass, and that it feels unlikely that Cursor and ably-cocoa's MessagePack encoder contain overlapping mistakes in their handling of MessagePack data.
Based on [1] at 2e975cb. [1] ably/specification#335
Preparation for decoding and encoding JSON-valued ObjectData. We also introduce a type that represents a JSON value or array; as well as representing JSONSerialization's supported top-level objects, we'll also shortly use it to represent the kind of JSON data that you can put in ObjectData.
Based on [1] at 2e975cb. This was implemented by updating the code to reflect the internal API that I wanted to exist, and then asking Cursor to implement the rules of the spec and to write tests. I then edited the generated code to simplify it a bit and add things like @SPEC annotations and other explanatory comments. I wanted some round-trip tests that go through the Realtime backend but decided to leave them until later once I have a bit more knowledge of the LiveObjects protocol; have created #17. [1] ably/specification#335
lawrence-forooghian
force-pushed
the
12-encode-and-decode-ObjectMessage-data
branch
from
239a2fa to
2715aeb
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a major refactor to the object message encoding/decoding infrastructure, transitioning from JSON-specific serialization to a generalized "wire" format supporting both JSON and MessagePack, including binary data. New internal representations and conversion logic for object messages are added, error handling is unified, and comprehensive tests for wire and object message serialization are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefaultLiveObjects
participant DefaultInternalPlugin
participant AblyPlugin
participant Channel
Client->>DefaultLiveObjects: send/receive ObjectMessage(s)
DefaultLiveObjects->>DefaultInternalPlugin: encodeObjectMessage(s) (specify format)
DefaultInternalPlugin->>AblyPlugin: convert to wire format, send/receive
AblyPlugin->>Channel: transmit/receive wire data
Channel-->>AblyPlugin: deliver wire data
AblyPlugin-->>DefaultInternalPlugin: provide wire data
DefaultInternalPlugin->>DefaultLiveObjects: decodeObjectMessage(s) (handle format)
DefaultLiveObjects-->>Client: deliver/receive ObjectMessage(s)
Estimated code review effort5 (~2+ hours): Extensive refactoring and new features across many files, including complex encoding/decoding logic, error handling, and comprehensive new tests. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (21)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedFull review triggered. |
umair-ably
left a comment
umair-ably
left a comment
There was a problem hiding this comment.
LGTM - worth waiting to see if coderabbit comes up with anything useful before merging though
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
Tests/AblyLiveObjectsTests/.swiftformat (1)
1-2: Consider consolidating formatting configPlacing a dedicated
.swiftformatin the test bundle only disablesenumNamespacesfor tests; if the same rule should be off project-wide (e.g. to keep parity between production & tests), prefer a single config at repository root with a--enable/--disablesection per path.
Conversely, if the rule must be test-specific, add a clarifying comment so future contributors don’t delete the file thinking it’s redundant..cursor/rules/specification.mdc (1)
14-14: Typo: extra “the” in sentence- - The LiveObjects functionality is referred to the in the Specification simply as "Objects". + - The LiveObjects functionality is referred to in the Specification simply as "Objects".Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (1)
94-100: Update aligns with new API – looks correct.Optional: since
OutboundObjectMessageis the inferred type in this context, you could shorten to.init(to follow the freshly-added style guideline, but that’s purely cosmetic.Tests/AblyLiveObjectsTests/ObjectMessageTests.swift (1)
6-7: Well-documented Xcode workaround.Good practice to document the Xcode issue and version tested. Consider adding a TODO to revisit this in future Xcode versions.
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
46-46: Fix typo in documentation- /// The contract for what this will return are the same as those of `JSONValue.toJSONSerializationInputElemtn`, with one addition: any values in the input of case `.extra` will be passed to the `serializeExtraValue` function, and the result of this function call will be inserted into the output object. + /// The contract for what this will return are the same as those of `JSONValue.toJSONSerializationInputElement`, with one addition: any values in the input of case `.extra` will be passed to the `serializeExtraValue` function, and the result of this function call will be inserted into the output object.Sources/AblyLiveObjects/Utility/WireValue.swift (1)
4-6: Fix typo in documentation-/// A wire value that can be represents the kinds of data that we expect to find inside a deserialized wire object received from AblyPlugin, or which we may put inside a serialized wire object that we send to AblyPlugin. +/// A wire value that represents the kinds of data that we expect to find inside a deserialized wire object received from AblyPlugin, or which we may put inside a serialized wire object that we send to AblyPlugin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.cursor/rules/specification.mdc(1 hunks).cursor/rules/swift.mdc(1 hunks)Sources/AblyLiveObjects/DefaultLiveObjects.swift(2 hunks)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift(1 hunks)Sources/AblyLiveObjects/Protocol/ObjectMessage.swift(1 hunks)Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift(14 hunks)Sources/AblyLiveObjects/Utility/Data+Extensions.swift(1 hunks)Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift(1 hunks)Sources/AblyLiveObjects/Utility/InternalError.swift(2 hunks)Sources/AblyLiveObjects/Utility/JSONValue.swift(1 hunks)Sources/AblyLiveObjects/Utility/WireCodable.swift(17 hunks)Sources/AblyLiveObjects/Utility/WireValue.swift(1 hunks)Tests/AblyLiveObjectsTests/.swiftformat(1 hunks)Tests/AblyLiveObjectsTests/.swiftlint.yml(1 hunks)Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalErrorTests.swift(1 hunks)Tests/AblyLiveObjectsTests/JSONValueTests.swift(5 hunks)Tests/AblyLiveObjectsTests/ObjectMessageTests.swift(1 hunks)Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift(28 hunks)Tests/AblyLiveObjectsTests/WireValueTests.swift(1 hunks)ably-cocoa(1 hunks)
🧬 Code Graph Analysis (6)
Sources/AblyLiveObjects/DefaultLiveObjects.swift (1)
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
handleObjectProtocolMessage(100-110)handleObjectSyncProtocolMessage(112-123)
Tests/AblyLiveObjectsTests/InternalErrorTests.swift (1)
Sources/AblyLiveObjects/Utility/InternalError.swift (2)
toInternalError(28-30)toInternalError(34-36)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
Sources/AblyLiveObjects/Utility/Dictionary+Extensions.swift (1)
ablyLiveObjects_mapValuesWithTypedThrow(3-7)
Tests/AblyLiveObjectsTests/WireValueTests.swift (2)
Tests/AblyLiveObjectsTests/JSONValueTests.swift (2)
arguments(8-28)arguments(80-102)Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
serialized(47-64)
Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift (2)
Tests/AblyLiveObjectsTests/WireValueTests.swift (2)
arguments(9-31)arguments(197-221)Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (1)
arguments(27-124)
Sources/AblyLiveObjects/Utility/WireValue.swift (1)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (2)
serialized(47-64)map(78-99)
🧰 Additional context used
🧬 Code Graph Analysis (6)
Sources/AblyLiveObjects/DefaultLiveObjects.swift (1)
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
handleObjectProtocolMessage(100-110)handleObjectSyncProtocolMessage(112-123)
Tests/AblyLiveObjectsTests/InternalErrorTests.swift (1)
Sources/AblyLiveObjects/Utility/InternalError.swift (2)
toInternalError(28-30)toInternalError(34-36)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
Sources/AblyLiveObjects/Utility/Dictionary+Extensions.swift (1)
ablyLiveObjects_mapValuesWithTypedThrow(3-7)
Tests/AblyLiveObjectsTests/WireValueTests.swift (2)
Tests/AblyLiveObjectsTests/JSONValueTests.swift (2)
arguments(8-28)arguments(80-102)Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
serialized(47-64)
Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift (2)
Tests/AblyLiveObjectsTests/WireValueTests.swift (2)
arguments(9-31)arguments(197-221)Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (1)
arguments(27-124)
Sources/AblyLiveObjects/Utility/WireValue.swift (1)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (2)
serialized(47-64)map(78-99)
🔇 Additional comments (38)
ably-cocoa (1)
1-1: Confirm submodule bump doesn’t introduce breaking API changesI attempted to fetch commit differences in this environment, but SSH cloning isn’t supported here. Please manually verify:
- All AblyLiveObjects CI and unit/integration tests still pass against the new commit (
9b2bfe4).- The bumped SHA is on a stable branch or, ideally, tagged (to avoid drifting pins).
- No public API used by the plugin was removed or renamed between the old and new SHA.
Smoke-test locally using HTTPS:
#!/bin/bash set -e # Point the submodule to HTTPS so it can clone in any environment git submodule set-url ably-cocoa https://github.com/ably/ably-cocoa.git git submodule update --init ably-cocoa pushd ably-cocoa >/dev/null # Replace <old_sha> with the previous pointer git log --oneline <old_sha>..9b2bfe4 --abbrev-commit | head -20 popd >/dev/nullTests/AblyLiveObjectsTests/.swiftlint.yml (1)
1-3: Config looks fine.Selective rule-disable for the test target is reasonable and matches the comment.
.cursor/rules/swift.mdc (1)
10-13: Documentation addition LGTM.Tests/AblyLiveObjectsTests/InternalErrorTests.swift (1)
5-17: Solid test coverage.
Verifies the intended.errorInfocase; good addition.Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift (2)
25-25: LGTM! Consistent refactoring from JSON to wire format.The systematic replacement of
JSONValuewithWireValueand the corresponding updates to initializers (jsonObject:→wireObject:) and variable names (json→wire) are correctly implemented throughout the test file.Also applies to: 37-37, 51-51, 53-53, 68-68, 70-70, 77-77, 79-79, 86-86, 88-88, 95-95, 97-97, 127-127, 154-154, 168-168, 178-178, 199-199, 203-203, 217-217, 221-221, 241-241, 267-267, 278-278, 286-286, 300-300, 305-305, 337-337, 358-358, 370-370, 377-377, 387-387, 388-388, 407-407, 427-427, 435-435, 439-439, 446-446, 447-447, 458-458, 471-471, 481-481, 482-482, 489-489, 497-497, 504-504, 516-516, 517-517, 524-524, 527-527, 540-540, 556-556, 566-566, 567-567, 573-573, 574-574, 581-581, 588-588, 596-596, 601-601, 609-609, 610-610, 623-623, 638-638
163-195: Spec-compliant behavior change looks correct.The updated test expectations correctly reflect the specification requirements (OOP3g, OOP3h, OOP3i) that
nonce,initialValueEncoding, andinitialValueshould not be extracted during decoding.Tests/AblyLiveObjectsTests/WireValueTests.swift (3)
9-31: Comprehensive test coverage for WireValue conversions.The tests correctly cover all supported data types including binary data, which is a key enhancement over JSON-only support. The use of NSObject equality for comparison is appropriate for Foundation types.
Also applies to: 197-221
81-193: Excellent test coverage for MessagePack deserialization.The manually crafted MessagePack binary data with detailed format comments provides thorough testing of MessagePack-specific features, particularly binary data support that JSON cannot handle.
301-378: Smart approach to MessagePack encoding verification.Comparing the decoded objects rather than raw bytes is the correct approach, as it handles potential differences in map key ordering between MessagePack encoder implementations.
Tests/AblyLiveObjectsTests/ObjectMessageTests.swift (4)
52-68: Correct verification of MessagePack float64 encoding.Good use of
CFNumberGetTypeto verify that numbers are encoded as float64 type per specification OD4c3.
87-103: Well-designed JSON payload encoding tests.Smart use of single-element objects to ensure stable JSON string output for test assertions. The tests correctly verify that the encoding field is set to "json" for JSON object/array payloads.
Also applies to: 180-195
296-327: Comprehensive error handling test coverage.Good coverage of error cases including invalid JSON, non-object/array JSON values, and invalid Base64 decoding. The decision to throw
InternalErrorfor these cases is appropriate.Also applies to: 393-402, 424-455
460-500: Thorough round-trip and ObjectOperation tests.The round-trip tests provide excellent verification of encoding/decoding symmetry. The ObjectOperation tests correctly implement different initialValue encoding strategies for MessagePack (binary) vs JSON (Base64) per specifications.
Also applies to: 502-566
Sources/AblyLiveObjects/Utility/InternalError.swift (1)
10-13: Good simplification of error handling.The consolidation to a single
.generic(Error)case is appropriate given that all error types were being handled identically. The extension methods provide clean conversion toInternalError.Also applies to: 27-31, 33-37
Tests/AblyLiveObjectsTests/JSONValueTests.swift (1)
6-6: Consistent terminology update from AblyPlugin to JSONSerialization.The systematic renaming from "AblyPlugin data" to "JSONSerialization output/input" correctly reflects the new approach of working directly with Foundation's JSONSerialization types.
Also applies to: 10-27, 30-33, 54-55, 75-75, 78-78, 98-99, 104-107, 149-149
Sources/AblyLiveObjects/DefaultLiveObjects.swift (2)
11-14: Type refactoring looks goodThe transition from
InboundWireObjectMessagetoInboundObjectMessagealigns with the broader refactoring to support multiple encoding formats.
69-71: Consistent type updates throughout the interfaceAll method signatures have been properly updated to use the new
InboundObjectMessageandOutboundObjectMessagetypes, maintaining consistency across the public API.Also applies to: 77-78, 84-84
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (3)
1-12: Well-designed extensible JSON value typeThe
ExtendedJSONValueenum provides a clean abstraction for representing JSON-like data with custom extensions. The indirect enum properly handles recursive structures.
28-36: Correct boolean vs number distinctionThe careful handling of booleans using
kCFBooleanTrueandkCFBooleanFalseidentity checks is essential for preserving type information when round-tripping through Foundation types.
77-99: Robust transformation implementationThe
mapfunction correctly handles recursive transformation of theExtratype with proper error propagation using typed throws.Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (4)
49-58: Clean wrapper implementation for struct-to-protocol bridgingThe
ObjectMessageBoxclass provides a necessary bridge between Swift structs and the class-boundAblyPlugin.ObjectMessageProtocol. The documentation clearly explains the rationale.
60-82: Proper separation of wire and internal message formatsThe two-step decoding process cleanly separates wire format parsing from internal representation construction, supporting multiple encoding formats.
84-94: Type-safe encoding with appropriate runtime checksThe encoding method properly validates the message type and converts through the wire format layer.
127-146: Clean async/await bridge implementationThe method properly wraps messages and bridges the callback-based plugin API to Swift's async/await pattern.
Sources/AblyLiveObjects/Protocol/ObjectMessage.swift (8)
1-31: Well-structured internal message typesThe
InboundObjectMessageandOutboundObjectMessagestructs properly model the protocol fields with appropriate optionality and clear documentation references to the specification.
44-58: Appropriate modeling of dual string/JSON contentThe
StringPropertyContentenum clearly represents the two possible string field contents, supporting the encoding rules where JSON objects can be stringified.
151-156: Correct handling of outbound-only fieldsThe implementation properly sets
nonce,initialValue, andinitialValueEncodingto nil for inbound data as specified in OOP3g, OOP3h, and OOP3i.
164-187: Correct implementation of format-specific encoding rulesThe method properly implements OOP5 rules:
- MessagePack: Binary data as MessagePack binary type with "msgpack" encoding
- JSON: Base64-encoded string with "json" encoding
219-248: Robust format-aware decoding implementationThe decoding correctly handles:
- MessagePack binary data directly
- JSON Base64 decoding with error handling
- Appropriate precondition failure for logic errors
250-260: Proper JSON string decodingThe implementation correctly checks the encoding field and parses JSON strings when appropriate, following OD5a2 and OD5b3.
267-309: Comprehensive format-specific encodingThe encoding properly implements:
- Binary data: MessagePack binary vs JSON Base64
- Numbers: MessagePack float64 conversion
- JSON objects: Stringification with "json" encoding marker
322-442: Consistent conversion pattern across all message componentsAll remaining conversion methods (MapOp, MapEntry, Map, ObjectState) follow the same pattern of format-aware initialization and wire conversion, maintaining consistency throughout the codebase.
Sources/AblyLiveObjects/Utility/WireValue.swift (4)
18-79: Well-designed convenience accessorsThe convenience getters provide type-safe access to associated values with a consistent pattern. The
isNullproperty appropriately returns a Bool rather than an optional.
82-116: Comprehensive literal protocol supportThe literal protocol conformances enable clean, readable syntax for creating
WireValueinstances in tests and implementation code.
124-136: Robust conversion from AblyPlugin dataThe implementation correctly identifies binary data for MessagePack support and fails fast with precondition for unexpected types, helping catch integration issues early.
230-248: Appropriate error handling for JSON conversionThe implementation correctly throws an error when attempting to convert binary data to JSON, as JSON cannot represent binary data directly.
Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (1)
474-508: Well-designed StringOrData implementationThe
StringOrDataenum is a clean solution for handling both string and binary data in the wire format. The error handling and protocol conformance are properly implemented.Sources/AblyLiveObjects/Utility/WireCodable.swift (1)
233-267: Consistent implementation of data extraction methodsThe new data extraction methods follow the established pattern and properly support binary data handling in the wire format. Good error handling with typed throws.
✅ Actions performedFull review triggered. |
Note: This PR is based on top of #14; please review that one first.
This implements the encoding and decoding rules of ably/specification#335. See commit messages for more details.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores