-
Notifications
You must be signed in to change notification settings - Fork 31
[ECO-5518] Fix delta decoding #2083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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