[ECO-5518] Fix delta decoding#2083
Conversation
Not sure of the thinking behind the current assertion but it seems it was working by accident.
All but the first should be using deltas, so ensure this and assert it. The existing assertion ignored messages that weren't using deltas.
WalkthroughThe PR removes ARTDeltaCodec/ARTVCDiffDecoder from project/module maps and private headers, adjusts ARTDataEncoder to refine delta-base handling across encodings, deletes a VCDiff category implementation, adds a new default method in ARTClientOptions, and significantly updates tests to cover JSON and non-binary delta decoding. Changes
Sequence Diagram(s)sequenceDiagram
participant Publisher
participant Realtime
participant ARTDataEncoder as DataEncoder
participant DeltaCodec
Publisher->>Realtime: Publish messages (various payloads)
Realtime->>DataEncoder: decode(data, identifier, encodings)
alt encodings end not base64 and no vcdiff
DataEncoder->>DataEncoder: setDeltaCodecBase(from data)
end
alt encodings include base64
DataEncoder->>DataEncoder: base64 decode
alt last encoding and no vcdiff
DataEncoder->>DataEncoder: setDeltaCodecBase(from decoded)
end
end
alt encodings include vcdiff
DataEncoder->>DeltaCodec: applyDelta(delta, base)
DeltaCodec-->>DataEncoder: decoded data
DataEncoder->>DataEncoder: update base to decoded
end
DataEncoder-->>Realtime: decoded payload
Realtime-->>Publisher: deliver message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
💤 Files with no reviewable changes (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (11)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Fix the following scenarios: - delta decoding of JSON-valued message data: we were not setting the base payload to the message's `data` before decoding JSON - delta decoding when using the non-binary protocol: we were setting the base payload to the delta, not the result of vcdiff decoding Specification references based on [1] at adf8d0f. Resolves #2082. [1] ably/specification#356
0136189 to
4879b36
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
maratal
left a comment
There was a problem hiding this comment.
LGTM, what was Source/ARTDeltaCodec.m doing?
Nothing, as far as I could tell |
Improves the tests around delta decoding, and fixes the following scenarios:
databefore decoding JSONSpecification references based on ably/specification#356.
Resolves #2082.
Summary by CodeRabbit
Bug Fixes
Chores
Tests