[ECO-5456] feat: add VCDiff support for delta message decoding #620
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
b15ae82 to
32ba990
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
github-actions
bot
temporarily deployed
to
staging/pull/620/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/check.yml (1)
23-26: Upgrade checkout to actions/checkout@v4.v2 runs on deprecated Node runtime and may break CI.
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4
🧹 Nitpick comments (14)
test/ably/utils.py (3)
186-198: Consider improving exception handling practices.The current implementation catches all exceptions and re-raises a generic
TimeoutError. While functional, this approach loses the original exception context which could be valuable for debugging.Apply this diff to improve exception handling:
async def assert_waiter(block: Callable[[], Awaitable[bool]], timeout: float = 10) -> None: """ Polls a condition until it succeeds or times out. Args: block: A callable that returns a boolean indicating success timeout: Maximum time to wait in seconds (default: 10) Raises: - TimeoutError: If condition not met within timeout + asyncio.TimeoutError: If condition not met within timeout """ try: await asyncio.wait_for(_poll_until_success(block), timeout=timeout) except asyncio.TimeoutError: - raise asyncio.TimeoutError(f"Condition not met within {timeout}s") + raise asyncio.TimeoutError(f"Condition not met within {timeout}s") from None
201-210: Consider adding debug logging for polling failures.The current implementation silently ignores exceptions during polling, which can make debugging difficult. Consider logging failures while maintaining the polling behavior.
Apply this diff to add optional debug logging:
async def _poll_until_success(block: Callable[[], Awaitable[bool]]) -> None: while True: try: success = await block() if success: break - except Exception: - pass + except Exception as e: + # Log debug info but continue polling + import logging + logging.getLogger(__name__).debug(f"Polling condition failed: {e}") await asyncio.sleep(0.1)
213-235: Consider applying similar improvements to sync version.The sync version has similar exception handling patterns. Consider applying the same improvements for consistency.
Apply this diff to improve the sync version:
def assert_waiter_sync(block: Callable[[], bool], timeout: float = 10) -> None: """ Blocking version of assert_waiter that polls a condition until it succeeds or times out. Args: block: A callable that returns a boolean indicating success timeout: Maximum time to wait in seconds (default: 10) Raises: TimeoutError: If condition not met within timeout """ start_time = time.time() while True: try: success = block() if success: break - except Exception: - pass + except Exception as e: + # Log debug info but continue polling + import logging + logging.getLogger(__name__).debug(f"Polling condition failed: {e}") if time.time() - start_time >= timeout: - raise TimeoutError(f"Condition not met within {timeout}s") + raise TimeoutError(f"Condition not met within {timeout}s") from None time.sleep(0.1)ably/types/options.py (2)
24-26: Type-hint the new parameter for clarityAnnotate
vcdiff_decoderto improve readability and editor support.Apply:
+from typing import Optional @@ - channel_retry_timeout=Defaults.channel_retry_timeout, add_request_ids=False, - vcdiff_decoder=None, **kwargs): + channel_retry_timeout=Defaults.channel_retry_timeout, add_request_ids=False, + vcdiff_decoder: Optional['VCDiffDecoder'] = None, **kwargs):
271-274: Expose a setter to allow runtime swap of the decoderOptional quality-of-life improvement to support swapping the decoder without reconstructing Options.
Apply:
- @property - def vcdiff_decoder(self): - return self.__vcdiff_decoder + @property + def vcdiff_decoder(self) -> Optional['VCDiffDecoder']: + return self.__vcdiff_decoder + + @vcdiff_decoder.setter + def vcdiff_decoder(self, decoder: Optional['VCDiffDecoder']): + self.__vcdiff_decoder = decoderably/types/message.py (1)
193-194: Nit: trim exception message per lint hint (TRY003)Shorten the message; the ID can go into
causeor logs.Apply:
- raise AblyException( - f"Delta message decode failure - previous message not available. Message id = {id}", + raise AblyException( + "Delta message decode failure - previous message not available", 400, 40018, )ably/vcdiff_plugin.py (1)
46-50: Prefer log.exception for import failuresCaptures traceback and reduces debugging time.
- except ImportError as e: - log.error("vcdiff library not found. Install with: pip install ably[vcdiff]") + except ImportError as e: + log.exception("vcdiff library not found. Install with: pip install ably[vcdiff]") raise ImportError( "VCDiff plugin requires vcdiff library. " "Install with: pip install ably[vcdiff]" ) from eably/types/mixins.py (3)
71-74: Avoid one-line compound statementsKeeps style consistent and satisfies linters (E701).
- data = bytearray(base64.b64decode(data)) if isinstance(data, bytes) \ - else bytearray(base64.b64decode(data.encode('utf-8'))) - if not encoding_list: last_payload = data + data = bytearray(base64.b64decode(data)) if isinstance(data, bytes) \ + else bytearray(base64.b64decode(data.encode('utf-8'))) + if not encoding_list: + last_payload = data
123-123: Avoid one-line ifMinor readability/style improvement.
- if context: context.base_payload = last_payload + if context: + context.base_payload = last_payload
23-28: Consider dataclass for DecodingContextReduces boilerplate and clarifies intent.
Example:
from dataclasses import dataclass from typing import Optional, Any @dataclass class DecodingContext: base_payload: Optional[Any] = None last_message_id: Optional[str] = None vcdiff_decoder: Optional["VCDiffDecoder"] = Noneably/realtime/realtime_channel.py (2)
545-548: Consider optimizing the equality check for ChannelOptions.The method correctly checks state and performs equality comparison. However, for better performance when options are identical objects, consider adding an identity check.
def should_reattach_to_set_options(self, old_options: ChannelOptions, new_options: ChannelOptions) -> bool: if self.state != ChannelState.ATTACHING and self.state != ChannelState.ATTACHED: return False + # Fast path for identical objects + if old_options is new_options: + return False return old_options != new_options
607-612: Nitpick: Consider extracting error messages to constants.The static analysis tool suggests avoiding long error messages inline. While not critical, extracting these to constants would improve maintainability.
# At module level INVALID_OPTIONS_TYPE_ERROR = "options must be ChannelOptions instance or dictionary" REATTACH_OPTIONS_ERROR = ( 'Channels.get() cannot be used to set channel options that would cause the channel to reattach. ' 'Please, use RealtimeChannel.setOptions() instead.' ) # Then use in the method: raise AblyException(INVALID_OPTIONS_TYPE_ERROR, 40000, 400)ably/scripts/unasync.py (2)
242-243: Limit test-only token replacement to the test unasync pass.
assert_waiterexists in tests; mapping it during the source pass is unnecessary risk. Move this replacement into the “Test files” section alongside other test helpers.Apply within this hunk:
- _TOKEN_REPLACE["assert_waiter"] = "assert_waiter_sync"Add near the test replacements (see next comment) to keep scope tight.
262-268: Addassert_waiter→assert_waiter_syncmapping here (test pass).Keeps all test helper rewrites together and avoids touching src code.
Apply near existing test token replacements:
_TOKEN_REPLACE["asyncSetUp"] = "setUp" _TOKEN_REPLACE["asyncTearDown"] = "tearDown" _TOKEN_REPLACE["AsyncMock"] = "Mock" + _TOKEN_REPLACE["assert_waiter"] = "assert_waiter_sync"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/check.yml(1 hunks)ably/__init__.py(1 hunks)ably/realtime/realtime_channel.py(8 hunks)ably/scripts/unasync.py(1 hunks)ably/types/message.py(3 hunks)ably/types/mixins.py(4 hunks)ably/types/options.py(3 hunks)ably/vcdiff_plugin.py(1 hunks)pyproject.toml(3 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(1 hunks)test/ably/rest/restchannelpublish_test.py(2 hunks)test/ably/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
ably/vcdiff_plugin.py (4)
ably/types/options.py (2)
VCDiffDecoder(11-14)decode(13-14)ably/util/exceptions.py (1)
AblyException(9-84)ably/types/mixins.py (1)
decode(47-125)test/ably/realtime/realtimechannel_vcdiff_test.py (2)
decode(20-23)decode(29-30)
ably/types/options.py (3)
ably/types/mixins.py (1)
decode(47-125)ably/vcdiff_plugin.py (1)
decode(52-77)test/ably/realtime/realtimechannel_vcdiff_test.py (2)
decode(20-23)decode(29-30)
ably/types/message.py (2)
ably/types/mixins.py (5)
EncodeDataMixin(30-129)DeltaExtras(14-20)decode(47-125)encoding(36-37)encoding(40-44)ably/util/exceptions.py (1)
AblyException(9-84)
ably/types/mixins.py (4)
ably/util/exceptions.py (1)
AblyException(9-84)ably/types/message.py (3)
extras(97-98)data(69-70)from_encoded(181-206)ably/types/options.py (2)
vcdiff_decoder(272-273)decode(13-14)ably/vcdiff_plugin.py (1)
decode(52-77)
test/ably/realtime/realtimechannel_vcdiff_test.py (7)
ably/vcdiff_plugin.py (2)
VCDiffPlugin(27-77)decode(52-77)ably/realtime/realtime_channel.py (7)
ChannelOptions(25-99)get(596-628)attach(166-204)name(552-554)subscribe(285-337)params(54-56)params(59-63)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)test/ably/utils.py (4)
BaseAsyncTestCase(43-58)WaitableEvent(238-252)finish(251-252)wait(248-249)ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/types/options.py (3)
VCDiffDecoder(11-14)decode(13-14)vcdiff_decoder(272-273)ably/types/mixins.py (1)
decode(47-125)
ably/realtime/realtime_channel.py (4)
ably/rest/channel.py (8)
ably(143-144)Channel(22-174)Channels(177-229)cipher(155-156)get(182-193)name(147-148)options(159-160)options(167-174)ably/types/mixins.py (2)
DecodingContext(23-27)from_encoded_array(128-129)ably/util/eventemitter.py (1)
once_async(168-181)ably/util/crypto.py (1)
CipherParams(16-42)
test/ably/rest/restchannelpublish_test.py (2)
ably/rest/channel.py (4)
ably(143-144)publish(106-132)get(182-193)history(32-39)test/ably/utils.py (2)
BaseAsyncTestCase(43-58)assert_waiter(186-198)
🪛 Ruff (0.12.2)
ably/vcdiff_plugin.py
46-46: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
47-50: Avoid specifying long messages outside the exception class
(TRY003)
67-67: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Consider moving this statement to an else block
(TRY300)
76-76: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
ably/types/message.py
193-194: Avoid specifying long messages outside the exception class
(TRY003)
ably/types/mixins.py
73-73: Multiple statements on one line (colon)
(E701)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
81-81: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Do not catch blind exception: Exception
(BLE001)
103-103: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
104-104: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
104-104: Avoid specifying long messages outside the exception class
(TRY003)
123-123: Multiple statements on one line (colon)
(E701)
test/ably/realtime/realtimechannel_vcdiff_test.py
29-29: Unused method argument: delta
(ARG002)
29-29: Unused method argument: base
(ARG002)
30-30: Create your own exception
(TRY002)
30-30: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Use raise without specifying exception name
Remove exception name
(TRY201)
86-86: Use of assert detected
(S101)
88-88: Use of assert detected
(S101)
117-117: Use raise without specifying exception name
Remove exception name
(TRY201)
127-127: Use of assert detected
(S101)
129-129: Use of assert detected
(S101)
150-150: Use of assert detected
(S101)
167-167: Use raise without specifying exception name
Remove exception name
(TRY201)
179-179: Use of assert detected
(S101)
ably/realtime/realtime_channel.py
41-41: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Avoid specifying long messages outside the exception class
(TRY003)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
440-440: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
612-612: Avoid specifying long messages outside the exception class
(TRY003)
620-625: Avoid specifying long messages outside the exception class
(TRY003)
test/ably/rest/restchannelpublish_test.py
405-405: Use of assert detected
(S101)
411-411: Function definition does not bind loop variable encoding
(B023)
412-412: Function definition does not bind loop variable encoding
(B023)
413-413: Function definition does not bind loop variable msg_data
(B023)
415-415: Function definition does not bind loop variable msg_data
(B023)
421-421: Use of assert detected
(S101)
426-426: Function definition does not bind loop variable expected_value
(B023)
426-426: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
426-426: Function definition does not bind loop variable expected_type
(B023)
test/ably/utils.py
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
207-208: try-except-pass detected, consider logging the exception
(S110)
207-207: Do not catch blind exception: Exception
(BLE001)
229-230: try-except-pass detected, consider logging the exception
(S110)
229-229: Do not catch blind exception: Exception
(BLE001)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
🪛 actionlint (1.7.7)
.github/workflows/check.yml
27-27: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
39-39: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (7)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.8)
- GitHub Check: check (3.9)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.7)
🔇 Additional comments (25)
pyproject.toml (4)
59-59: LGTM - Optional dependency configuration.The VCDiff dependency is correctly configured as optional with the experimental source. This allows users to install the VCDiff functionality when needed without making it a mandatory dependency.
64-64: LGTM - Extras configuration.The extras configuration allows users to install VCDiff support via
pip install ably[vcdiff], which is a clean way to provide optional functionality.
78-78: LGTM - Development dependency.Adding VCDiff as a development dependency ensures that tests and development workflows can use the VCDiff functionality without requiring explicit installation steps.
30-33: Experimental package availability verified.The vcdiff package versions 0.1.0a1 and 0.1.0a2 are present on TestPyPI, so the
^0.1.0a2constraint from theexperimentalsource is satisfied.test/ably/utils.py (2)
1-9: LGTM - Import additions.The new imports support the async waiter functionality and are appropriately scoped.
238-252: LGTM - WaitableEvent implementation.The
WaitableEventclass provides a clean synchronization primitive for async tests. The implementation is straightforward and appropriate for the test utility context.test/ably/rest/restchannelpublish_test.py (2)
22-22: LGTM - Import update for assert_waiter.The import correctly adds
assert_waiterfrom the updated test utilities.
407-417: Excellent improvement replacing sleep with polling.The replacement of fixed
asyncio.sleep()calls withassert_waiter()based polling is a significant improvement. This makes the tests more reliable by waiting for actual conditions rather than arbitrary time delays, while also making them potentially faster when conditions are met early.Also applies to: 423-428
ably/types/options.py (2)
11-14: Good abstraction for pluggable VCDiff decodersDefining a small ABC with a single
decodemethod keeps the contract clear. LGTM.
88-88: No action: vcdiff_decoder is correctly propagated
Options.vcdiff_decoderis passed intoDecodingContextinably/realtime/realtime_channel.py:143and exercised bytest/ably/realtime/realtimechannel_vcdiff_test.py.ably/__init__.py (2)
8-8: Re-exporting VCDiffDecoder via top-level API looks goodKeeps the public surface cohesive. No issues.
11-11: Safe to import VCDiffPlugin at package initSince the plugin imports the optional dependency only in
__init__, top-level import won’t hard-require the extra. LGTM.ably/types/message.py (2)
181-181: Signature extension preserves backwards compatibilityAdding
context=Noneis safe; existing callers remain valid.
196-196: Passingcontextthrough to the decoder is correctEnsures base payload propagation and delta decoding work as intended.
ably/vcdiff_plugin.py (1)
27-34: Overall plugin structure looks solidClear interface adherence, input validation, and export surface.
test/ably/realtime/realtimechannel_vcdiff_test.py (2)
51-89: Happy path coverage looks goodEnd-to-end delta flow validated; decoder call count assertion is valuable.
133-181: Recovery path assertion is on-pointChecks RTL18c behavior by validating reason.code and reattach events.
ably/realtime/realtime_channel.py (6)
25-100: LGTM! Well-structured ChannelOptions implementation.The
ChannelOptionsclass is well-designed with proper encapsulation, validation, and utility methods. The implementation follows Python best practices with proper type hints and documentation.
152-163: LGTM! Proper implementation of dynamic option updates.The
set_optionsmethod correctly handles dynamic channel option updates with appropriate state management and error propagation.
417-418: Verify attach serial handling for delta recovery.The code correctly updates both
attach_serialandchannel_serialfrom the ATTACHED message. This is crucial for RTL18 recovery after delta decode failures.
572-582: LGTM! Correct implementation of RTL18 delta decode failure recovery.The recovery procedure properly implements the RTL18 specification by logging the error, transitioning to ATTACHING state with the error reason, and triggering the pending state check.
127-144: No issues with VCDiff decoder setup. The initializer correctly passesoptions.vcdiff_decoderintoDecodingContext, and existing tests cover both plugin-enabled and plugin-absent flows.
212-212: Params are correctly passed as a dict and serialized per spec; no changes required.ably/scripts/unasync.py (1)
281-295: No action needed on assert_waiter_sync mapping.assert_waiter_syncis defined intest/ably/utils.pyand, per the first-round unasync Rule, will be copied unchanged intotest/ably/sync/utils.py, satisfying the tests’ import paths..github/workflows/check.yml (1)
45-47: Ensure VCDiff extra is installed if tests rely on it.If the new tests require the optional VCDiff dependency, install it in CI.
- run: poetry install -E crypto + run: poetry install -E crypto -E vcdiffIf multiple extras are expected, consider
poetry install --all-extras.
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
d0ddba0 to
4dd472d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/check.yml (1)
39-44: Upgrade cache action to v4 and key by OS + Python to avoid mixing virtualenvs.Also add restore-keys for partial hits. This addresses the actionlint warning and prevents cross-Python contamination.
- - uses: actions/cache@v3 + - uses: actions/cache@v4 name: Define a cache for the virtual environment based on the dependencies lock file with: - path: ./.venv - key: venv-${{ hashFiles('poetry.lock') }} + path: ./.venv + key: venv-${{ runner.os }}-${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }} + restore-keys: | + venv-${{ runner.os }}-${{ matrix.python-version }}- + venv-${{ runner.os }}-
🧹 Nitpick comments (2)
.github/workflows/check.yml (2)
27-30: Good: upgraded to setup-python@v5. Add check-latest for fresher patch releases.v5 resolves the Node 20/runtime warning. To reduce cache misses and ensure latest patch for each minor, enable check-latest.
- name: Set up Python ${{ matrix.python-version }} uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} + check-latest: true
32-33: Pin Poetry version for reproducibility.Unpinned action is fine, but pinning Poetry itself avoids surprise breaks in lock/install behavior.
- - name: Setup poetry - uses: abatilo/actions-poetry@v4 + - name: Setup poetry + uses: abatilo/actions-poetry@v4 + with: + poetry-version: '1.8.3'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/check.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/check.yml
39-39: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (7)
- GitHub Check: check (3.13)
- GitHub Check: check (3.10)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
- GitHub Check: check (3.7)
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
4dd472d to
24bce80
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
feat/channel-options
branch
from
16f3ec3 to
5bec2a7
Compare
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
2 times, most recently
from
0d16430 to
793d07a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
793d07a to
72ae4d6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
feat/channel-options
branch
from
5bec2a7 to
3671ec8
Compare
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
72ae4d6 to
c8dbe6d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
c8dbe6d to
36381ca
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
feat/channel-options
branch
2 times, most recently
from
b0d7781 to
9032c63
Compare
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
36381ca to
bad7343
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
bad7343 to
8074178
Compare
ttypic
force-pushed
the
feat/channel-options
branch
from
a06a6da to
85c8e70
Compare
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
a0c096c to
833cc1a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
ably/types/presence.py (1)
89-113: Use decoding context and validate delta 'from' to avoid wrong-base decodes
- context arg is currently unused; pass it to decode.
- Mirror Message.from_encoded delta 'from' guard to prevent corrupt decodes when base/id mismatch; raise AblyException 400/40018.
Apply within this method:
@staticmethod -def from_encoded(obj, cipher=None, context=None): +def from_encoded(obj, cipher=None, context=None): id = obj.get('id') action = obj.get('action', PresenceAction.ENTER) client_id = obj.get('clientId') connection_id = obj.get('connectionId') data = obj.get('data') encoding = obj.get('encoding', '') timestamp = obj.get('timestamp') # member_key = obj.get('memberKey', None) extras = obj.get('extras', None) if timestamp is not None: timestamp = _dt_from_ms_epoch(timestamp) - decoded_data = PresenceMessage.decode(data, encoding, cipher) + # Ensure correct base sequencing for delta payloads (aligns with Message.from_encoded) + from ably.types.mixins import DeltaExtras # local import to avoid cycles + from ably.util.exceptions import AblyException + delta_extra = DeltaExtras(extras) + if delta_extra.from_id: + last_id = getattr(context, 'last_message_id', None) if context else None + if last_id != delta_extra.from_id: + raise AblyException( + f"Delta message decode failure - previous message not available. Message id = {id}", + 400, + 40018, + ) + + decoded_data = PresenceMessage.decode(data, encoding, cipher, context)Outside this range, ensure imports are available if you prefer module-level imports:
from ably.util.exceptions import AblyException from ably.types.mixins import EncodeDataMixin, DeltaExtrasably/vcdiff_plugin.py (1)
1-83: Normalize AblyException HTTP status code for VCDiff failures
In ably/vcdiff_plugin.py (line 78) and ably/types/mixins.py (lines 82 & 105), changeAblyException(..., 40018, 40018)to use status_code 400 and error_code 40018.ably/realtime/realtime_channel.py (3)
39-41: Fix AblyException argument order (ChannelOptions.params validation)Order should be (status_code=400, code=40000).
Apply:
- raise AblyException("params must be a dictionary", 40000, 400) + raise AblyException("params must be a dictionary", 400, 40000)
77-85: Fix AblyException argument order (ChannelOptions.from_dict validation)Order should be (400, 40000).
Apply:
- if not isinstance(options_dict, dict): - raise AblyException("options must be a dictionary", 40000, 400) + if not isinstance(options_dict, dict): + raise AblyException("options must be a dictionary", 400, 40000)
241-258: Fix AblyException argument order in detach pathTwo raises pass (code, status) instead of (status, code).
Apply:
- elif self.state == ChannelState.FAILED: - raise AblyException("Unable to detach; channel state = failed", 90001, 400) + elif self.state == ChannelState.FAILED: + raise AblyException("Unable to detach; channel state = failed", 400, 90001) ... - elif new_state == ChannelState.ATTACHING: - raise AblyException("Detach request superseded by a subsequent attach request", 90000, 409) + elif new_state == ChannelState.ATTACHING: + raise AblyException("Detach request superseded by a subsequent attach request", 409, 90000)
♻️ Duplicate comments (10)
ably/types/message.py (1)
191-196: Guard against None context before accessing last_message_idAvoid AttributeError when context is None; fail fast with 400/40018 per RTL18.
- delta_extra = DeltaExtras(extras) - if delta_extra.from_id and delta_extra.from_id != context.last_message_id: - raise AblyException(f"Delta message decode failure - previous message not available. " - f"Message id = {id}", 400, 40018) + delta_extra = DeltaExtras(extras) + if delta_extra.from_id: + last_id = getattr(context, 'last_message_id', None) if context else None + if last_id != delta_extra.from_id: + raise AblyException( + f"Delta message decode failure - previous message not available. Message id = {id}", + 400, + 40018, + )ably/vcdiff_plugin.py (1)
72-78: Use HTTP 400 for status_code and preserve traceback/cause on decode failuresPer Ably conventions, status_code must be 400 with code 40018. Also log with traceback and avoid duplicating the low-level error text in the public message; use cause to retain detail.
- try: - # Use the vcdiff library to decode - result = self._vcdiff.decode(base, delta) - return result - except Exception as e: - log.error(f"VCDiff decode failed: {e}") - raise AblyException(f"VCDiff decode failure: {e}", 40018, 40018) from e + try: + # Use the vcdiff library to decode + return self._vcdiff.decode(base, delta) + except Exception as e: + log.exception("VCDiff decode failed") + raise AblyException("VCDiff decode failure", 400, 40018, cause=e) from eably/types/mixins.py (3)
103-106: Don’t catch blanket Exception; preserve AblyException and chain othersUse log.exception for stacktrace and raise with cause.
[ suggest_essential_refactor ]Apply:
- except Exception as e: - log.error(f'VCDiff decode failed: {e}') - raise AblyException('VCDiff decode failure', 40018, 40018) + except AblyException: + raise + except Exception as e: + log.exception('VCDiff decode failed') + raise AblyException('VCDiff decode failure', 400, 40018, cause=e) from e
51-52: Only persist bytes-like payloads to context.base_payloadAvoid saving dict/list/str as base payload; track last bytes separately.
[ suggest_essential_refactor ]Apply:
- last_payload = data + last_bytes_payload = None ... - if not encoding_list: - last_payload = data + if not encoding_list: + last_bytes_payload = data ... - data = bytearray(context.vcdiff_decoder.decode(delta_data, base_data)) - last_payload = data + data = bytearray(context.vcdiff_decoder.decode(delta_data, base_data)) + last_bytes_payload = data ... - if context: - context.base_payload = last_payload + if context and last_bytes_payload is not None: + context.base_payload = last_bytes_payloadAlso applies to: 71-75, 100-102, 124-126
76-82: Fix AblyException argument order and HTTP statusstatus_code must be an HTTP status (400); code is the Ably error code (40019/40018).
Apply:
- if not context or not context.vcdiff_decoder: - log.error('Message cannot be decoded as no VCDiff decoder available') - raise AblyException('VCDiff decoder not available', 40019, 40019) + if not context or not context.vcdiff_decoder: + log.error('Message cannot be decoded as no VCDiff decoder available') + raise AblyException('VCDiff decoder not available', 400, 40019) - if not context.base_payload: - log.error('VCDiff decoding requires base payload') - raise AblyException('VCDiff decode failure', 40018, 40018) + if not context.base_payload: + log.error('VCDiff decoding requires base payload') + raise AblyException('VCDiff decode failure', 400, 40018)test/ably/realtime/realtimechannel_vcdiff_test.py (3)
1-10: Guard tests on optional vcdiff extraSkip suite when vcdiff isn’t installed to avoid hard failures.
[ suggest_optional_refactor ]Apply:
+import pytest ... +pytest.importorskip("vcdiff", reason="Install with: pip install 'ably[vcdiff]'")
8-9: Use canonical ConnectionState import pathImport from types module, not realtime.connection.
[ suggest_nitpick ]Apply:
-from ably.realtime.connection import ConnectionState +from ably.types.connectionstate import ConnectionState
146-153: Subscribe using ChannelState enum, not stringEvent emitter uses ChannelState values.
[ suggest_optional_refactor ]Apply:
- def on_attaching(state_change): + from ably.types.channelstate import ChannelState + def on_attaching(state_change): attaching_events.append(state_change) # RTL18c - Check error code if state_change.reason and state_change.reason.code: assert state_change.reason.code == 40018, "Check error code passed through per RTL18c" - channel.on('attaching', on_attaching) + channel.on(ChannelState.ATTACHING, on_attaching)ably/realtime/realtime_channel.py (2)
421-430: Harden message decode error handling (preserve stack, avoid partial processing)
- Use raw_messages var for logging.
- On error: for 40018 start recovery and return immediately; for others log.exception and return.
[ suggest_essential_refactor ]Apply:
- messages = [] - try: - messages = Message.from_encoded_array(proto_msg.get('messages'), context=self.__decoding_context) - self.__decoding_context.last_message_id = messages[-1].id - self.__channel_serial = channel_serial - except AblyException as e: - if e.code == 40018: # Delta decode failure - start recovery - self._start_decode_failure_recovery(e) - else: - log.error(f"Message processing error {e}. Skip messages {proto_msg.get('messages')}") + messages = [] + raw_messages = proto_msg.get('messages', []) + try: + messages = Message.from_encoded_array(raw_messages, context=self.__decoding_context) + if messages: + self.__decoding_context.last_message_id = messages[-1].id + self.__channel_serial = channel_serial + except AblyException as e: + if e.code == 40018: # Delta decode failure - start recovery + self._start_decode_failure_recovery(e) + return + else: + log.exception(f"Message processing error: {e}. Skipping {len(raw_messages)} messages") + return
570-586: RTL18 recovery: log with traceback and reset decode contextClear base_payload and last_message_id to avoid stale bases; keep existing flow.
[ suggest_essential_refactor ]Apply:
def _start_decode_failure_recovery(self, error: AblyException) -> None: """Start RTL18 decode failure recovery procedure""" if self.__decode_failure_recovery_in_progress: log.info('VCDiff recovery process already started, skipping') return self.__decode_failure_recovery_in_progress = True - # RTL18a: Log error with code 40018 - log.error(f'VCDiff decode failure: {error}') + # RTL18a: Log error with code 40018 + log.exception(f'VCDiff decode failure: {error}') + + # Reset delta decode context to avoid applying stale base after recovery starts + self.__decoding_context.base_payload = None + self.__decoding_context.last_message_id = None # RTL18b: Message is already discarded by not processing it # RTL18c: Send ATTACH with previous channel serial and transition to ATTACHING self._notify_state(ChannelState.ATTACHING, reason=error) self._check_pending_state()
🧹 Nitpick comments (4)
ably/types/presence.py (1)
170-174: Consider threading context through presence response handler (optional)If presence history ever carries deltas, expose an optional context in make_presence_response_handler and pass it into from_encoded_array.
Proposed shape (outside this hunk):
def make_presence_response_handler(cipher, context=None): def encrypted_presence_response_handler(response): messages = response.to_native() return PresenceMessage.from_encoded_array(messages, cipher=cipher, context=context) return encrypted_presence_response_handlerably/types/message.py (1)
234-238: Optional: allow passing decoding context in message response handlerREST usually isn’t delta-encoded, but for consistency and future-proofing, accept an optional context and pass it through.
-def make_message_response_handler(cipher): - def encrypted_message_response_handler(response): +def make_message_response_handler(cipher, context=None): + def encrypted_message_response_handler(response): messages = response.to_native() - return Message.from_encoded_array(messages, cipher=cipher) + return Message.from_encoded_array(messages, cipher=cipher, context=context) return encrypted_message_response_handlerably/vcdiff_plugin.py (2)
47-51: Log import failure with traceback; message can be shorterUse log.exception to capture stack trace; keep guidance concise.
- except ImportError as e: - log.error("vcdiff library not found. Install with: pip install ably[vcdiff]") + except ImportError as e: + log.exception("vcdiff library not found") raise ImportError( - "VCDiff plugin requires vcdiff library. " - "Install with: pip install ably[vcdiff]" + "VCDiff plugin requires the optional vcdiff dependency; install via: pip install 'ably[vcdiff]'" ) from e
1-18: Keep module header succinct (optional)Trim usage block or move to docs to reduce noise in the runtime module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
ably/__init__.py(1 hunks)ably/realtime/realtime_channel.py(5 hunks)ably/types/message.py(3 hunks)ably/types/mixins.py(4 hunks)ably/types/options.py(3 hunks)ably/types/presence.py(1 hunks)ably/vcdiff_plugin.py(1 hunks)pyproject.toml(3 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(1 hunks)test/ably/rest/restauth_test.py(3 hunks)test/ably/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/ably/rest/restauth_test.py
- test/ably/utils.py
- ably/types/options.py
- ably/init.py
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-08T15:37:26.765Z
Learnt from: lmars
PR: ably/ably-python#571
File: ably/util/exceptions.py:74-74
Timestamp: 2024-10-08T15:37:26.765Z
Learning: In `AblyException.decode_error_response`, exceptions are caught and re-thrown as an `AblyException` in `AblyException.raise_for_response`, so there's no need to raise an `AblyException` within `decode_error_response`.
Applied to files:
ably/types/mixins.py
🧬 Code graph analysis (6)
ably/types/message.py (3)
ably/types/mixins.py (5)
EncodeDataMixin(30-131)DeltaExtras(14-20)decode(47-127)encoding(36-37)encoding(40-44)ably/util/exceptions.py (1)
AblyException(9-84)ably/vcdiff_plugin.py (1)
decode(53-78)
ably/types/mixins.py (4)
ably/util/exceptions.py (1)
AblyException(9-84)ably/types/message.py (3)
extras(97-98)data(69-70)from_encoded(181-206)ably/types/options.py (2)
vcdiff_decoder(272-273)decode(13-14)ably/vcdiff_plugin.py (1)
decode(53-78)
ably/realtime/realtime_channel.py (4)
ably/types/message.py (3)
Message(24-231)id(77-78)id(81-82)ably/types/mixins.py (2)
DecodingContext(23-27)from_encoded_array(130-131)ably/types/options.py (1)
vcdiff_decoder(272-273)ably/util/exceptions.py (1)
AblyException(9-84)
ably/types/presence.py (1)
ably/types/message.py (1)
from_encoded(181-206)
ably/vcdiff_plugin.py (4)
ably/types/options.py (2)
VCDiffDecoder(11-14)decode(13-14)ably/util/exceptions.py (1)
AblyException(9-84)ably/types/mixins.py (1)
decode(47-127)test/ably/realtime/realtimechannel_vcdiff_test.py (2)
decode(20-23)decode(29-30)
test/ably/realtime/realtimechannel_vcdiff_test.py (7)
ably/vcdiff_plugin.py (2)
VCDiffPlugin(28-78)decode(53-78)ably/realtime/realtime_channel.py (6)
ChannelOptions(24-85)get(601-633)params(48-50)params(566-568)attach(154-192)name(545-547)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)test/ably/utils.py (4)
BaseAsyncTestCase(43-58)WaitableEvent(238-252)finish(251-252)wait(248-249)ably/types/options.py (3)
VCDiffDecoder(11-14)decode(13-14)vcdiff_decoder(272-273)ably/types/mixins.py (1)
decode(47-127)ably/util/eventemitter.py (2)
once_async(168-181)on(44-86)
🪛 Ruff (0.12.2)
ably/types/message.py
193-194: Avoid specifying long messages outside the exception class
(TRY003)
ably/types/mixins.py
78-78: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
103-103: Do not catch blind exception: Exception
(BLE001)
104-104: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
105-105: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
105-105: Avoid specifying long messages outside the exception class
(TRY003)
ably/realtime/realtime_channel.py
430-430: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
ably/types/presence.py
89-89: Unused static method argument: context
(ARG004)
ably/vcdiff_plugin.py
47-47: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
48-51: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Consider moving this statement to an else block
(TRY300)
77-77: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
78-78: Avoid specifying long messages outside the exception class
(TRY003)
test/ably/realtime/realtimechannel_vcdiff_test.py
29-29: Unused method argument: delta
(ARG002)
29-29: Unused method argument: base
(ARG002)
30-30: Create your own exception
(TRY002)
30-30: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Use raise without specifying exception name
Remove exception name
(TRY201)
117-117: Use raise without specifying exception name
Remove exception name
(TRY201)
167-167: Use raise without specifying exception name
Remove exception name
(TRY201)
⏰ 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). (7)
- GitHub Check: check (3.11)
- GitHub Check: check (3.9)
- GitHub Check: check (3.7)
- GitHub Check: check (3.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.10)
- GitHub Check: check (3.8)
🔇 Additional comments (10)
ably/vcdiff_plugin.py (3)
67-71: Type validation is fineExplicit bytes checks for delta and base look good.
28-36: Docstring aligns with behaviorInterface and error semantics are clear.
81-82: all export is correctPublic surface is explicit.
ably/types/mixins.py (3)
11-21: LGTM: ENC_VCDIFF constant and DeltaExtras helper look correctNaming and extraction of delta.from are consistent with usage in Message.from_encoded.
23-28: LGTM: DecodingContext fields are appropriateCarries base payload, last message id, and decoder reference as needed.
130-131: LGTM: Context propagation through from_encoded_arrayPassing context into each element decode is required for deltas.
test/ably/realtime/realtimechannel_vcdiff_test.py (2)
73-76: Re-raise original exception with tracebackUse bare raise inside except to preserve traceback.
[ suggest_nitpick ]
Apply:- except Exception as e: - waitable_event.finish() - raise e + except Exception: + waitable_event.finish() + raiseAlso applies to: 115-118, 165-168
35-37: Unused variable: valid_key_formatRemove if not used.
[ suggest_nitpick ]
Apply:- self.valid_key_format = "api:key"ably/realtime/realtime_channel.py (2)
128-132: Nit: simplify vcdiff_decoder assignmentThe ternary is redundant.
[ suggest_nitpick ]
Apply:- vcdiff_decoder = self.__realtime.options.vcdiff_decoder if self.__realtime.options.vcdiff_decoder else None - self.__decoding_context = DecodingContext(vcdiff_decoder=vcdiff_decoder) + self.__decoding_context = DecodingContext(vcdiff_decoder=self.__realtime.options.vcdiff_decoder)
472-474: LGTM: recovery flag reset when leaving ATTACHINGPrevents recovery lockout on future failures.
ttypic
force-pushed
the
feat/channel-options
branch
2 times, most recently
from
a5e34dc to
204138f
Compare
- Introduced `VCDiffDecoder` abstract class and `VCDiffPlugin` implementation. - Enhanced delta message processing with proper support for VCDiff decoding. - Updated `Options` to accept a `vcdiff_decoder`. - Handle delta failures with recovery mechanisms (RTL18-RTL20 compliance).
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
64e1e1c to
d893fb7
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
d893fb7 to
3293f3a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
ttypic
force-pushed
the
ECO-5456/vcdiff-support
branch
from
3293f3a to
0966233
Compare
github-actions
bot
temporarily deployed
to
staging/pull/620/features
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ably/realtime/realtime_channel.py (4)
39-41: Fix AblyException argument order (HTTP status, then Ably code)Should be
(400, 40000). Current(40000, 400)misclassifies errors.- raise AblyException("params must be a dictionary", 40000, 400) + raise AblyException("params must be a dictionary", 400, 40000)
247-247: Same issue: wrong AblyException argument orderDetach error should use HTTP 400, code 90001.
- raise AblyException("Unable to detach; channel state = failed", 90001, 400) + raise AblyException("Unable to detach; channel state = failed", 400, 90001)
261-261: Same issue: use 409 status, Ably code 90000- raise AblyException("Detach request superseded by a subsequent attach request", 90000, 409) + raise AblyException("Detach request superseded by a subsequent attach request", 409, 90000)
605-631: Channels.get: accept dict options safely; fix reattach check and setterPassing a dict currently breaks (
to_dict()call). Convert dicts viaChannelOptions.from_dict.- def get(self, name: str, options: Optional[ChannelOptions] = None) -> RealtimeChannel: + def get(self, name: str, options: Optional[ChannelOptions] = None) -> RealtimeChannel: @@ - if name not in self.__all: - channel = self.__all[name] = RealtimeChannel(self.__ably, name, options) + if name not in self.__all: + if isinstance(options, dict): + options = ChannelOptions.from_dict(options) + channel = self.__all[name] = RealtimeChannel(self.__ably, name, options) else: channel = self.__all[name] # Update options if channel is not attached or currently attaching - if options and channel.should_reattach_to_set_options(options): + if isinstance(options, dict): + options = ChannelOptions.from_dict(options) + if options and channel.should_reattach_to_set_options(options): raise AblyException( 'Channels.get() cannot be used to set channel options that would cause the channel to ' 'reattach. Please, use RealtimeChannel.setOptions() instead.', 400, 40000 ) elif options: channel.set_options_without_reattach(options)
♻️ Duplicate comments (9)
pyproject.toml (1)
30-33: TestPyPI source in published metadata — clarify install story or removeIncluding an explicit TestPyPI source may confuse downstream consumers; pip won’t use Poetry sources. If
vcdiff-decoderisn’t on PyPI,pip install ably[vcdiff]will fail. Either document the exact install (with --extra-index-url) or drop the source and the extra until it’s on PyPI.Run to verify availability on PyPI:
#!/bin/bash set -euo pipefail for name in vcdiff-decoder vcdiff_decoder; do echo "Checking $name on PyPI…" if curl -fsSL "https://pypi.org/pypi/${name}/json" >/dev/null; then echo "Found $name on PyPI" else echo "NOT found: $name on PyPI" fi doneably/types/message.py (1)
191-196: Guard against None or stale context to avoid AttributeError and comply with RTL18Accessing
context.last_message_idunguarded will crash on delta messages whencontextis None. Fail with 400/40018 per spec.- delta_extra = DeltaExtras(extras) - if delta_extra.from_id and delta_extra.from_id != context.last_message_id: - raise AblyException(f"Delta message decode failure - previous message not available. " - f"Message id = {id}", 400, 40018) + delta_extra = DeltaExtras(extras) + if delta_extra.from_id: + last_id = getattr(context, 'last_message_id', None) if context else None + if last_id != delta_extra.from_id: + raise AblyException( + f"Delta message decode failure - previous message not available. Message id = {id}", + 400, + 40018, + )ably/vcdiff/ably_vcdiff_decoder.py (1)
72-79: Normalize error: use HTTP 400 status, Ably code 40018; chain the causeCurrent code raises 40018 as both status and code.
- except Exception as e: - log.error(f"VCDiff decode failed: {e}") - raise AblyException(f"VCDiff decode failure: {e}", 40018, 40018) from e + except AblyException: + raise + except Exception as e: + log.exception("VCDiff decode failed") + raise AblyException("VCDiff decode failure", 400, 40018, cause=e) from eably/types/mixins.py (2)
76-83: Fix AblyException status/code ordering for decoder availability and missing baseUse HTTP 400 as status; keep Ably codes 40019 (no decoder) and 40018 (decode failure).
- if not context or not context.vcdiff_decoder: - log.error('Message cannot be decoded as no VCDiff decoder available') - raise AblyException('VCDiff decoder not available', 40019, 40019) + if not context or not context.vcdiff_decoder: + log.error('Message cannot be decoded as no VCDiff decoder available') + raise AblyException('VCDiff decoder not available', 400, 40019) - if not context.base_payload: - log.error('VCDiff decoding requires base payload') - raise AblyException('VCDiff decode failure', 40018, 40018) + if not context.base_payload: + log.error('VCDiff decoding requires base payload') + raise AblyException('VCDiff decode failure', 400, 40018)
103-106: Don’t catch blanket Exception; preserve AblyException and chain othersImproves diagnostics and satisfies linters.
- except Exception as e: - log.error(f'VCDiff decode failed: {e}') - raise AblyException('VCDiff decode failure', 40018, 40018) + except AblyException: + raise + except Exception as e: + log.exception('VCDiff decode failed') + raise AblyException('VCDiff decode failure', 400, 40018, cause=e) from etest/ably/realtime/realtimechannel_vcdiff_test.py (2)
1-10: Skip module if optional decoder isn’t installed; fix importsGuard tests so environments without the extra don’t hard-fail; import
ConnectionStatefrom canonical location.+import pytest import asyncio import json -from ably import AblyVCDiffDecoder +from ably import AblyVCDiffDecoder from ably.realtime.realtime_channel import ChannelOptions from test.ably.testapp import TestApp from test.ably.utils import BaseAsyncTestCase, WaitableEvent -from ably.realtime.connection import ConnectionState +from ably.types.connectionstate import ConnectionState from ably.types.options import VCDiffDecoder +from ably.types.channelstate import ChannelState + +# Skip if optional decoder isn’t present +pytest.importorskip("vcdiff_decoder", reason="vcdiff extra not installed")
146-153: Subscribe using enum value, not stringChannel emits
ChannelStateenums.- channel.on('attaching', on_attaching) + channel.on(ChannelState.ATTACHING, on_attaching)ably/realtime/realtime_channel.py (2)
575-592: RTL18: log with traceback and reset decode context before reattachPrevents stale bases on recovery.
def _start_decode_failure_recovery(self, error: AblyException) -> None: """Start RTL18 decode failure recovery procedure""" @@ - # RTL18a: Log error with code 40018 - log.error(f'VCDiff decode failure: {error}') + # RTL18a: Log error with code 40018 + log.exception(f'VCDiff decode failure: {error}') + + # Reset delta decode context to avoid stale bases after recovery starts + self.__decoding_context.base_payload = None + self.__decoding_context.last_message_id = None @@ - self._notify_state(ChannelState.ATTACHING, reason=error) + self._notify_state(ChannelState.ATTACHING, reason=error) self._check_pending_state()
425-434: Robust message batch handling; don’t lose traceback; skip batch on errorUse a local
raw_messages, update last_message_id only when non-empty, and log exceptions.- messages = [] - try: - messages = Message.from_encoded_array(proto_msg.get('messages'), context=self.__decoding_context) - self.__decoding_context.last_message_id = messages[-1].id - self.__channel_serial = channel_serial - except AblyException as e: - if e.code == 40018: # Delta decode failure - start recovery - self._start_decode_failure_recovery(e) - else: - log.error(f"Message processing error {e}. Skip messages {proto_msg.get('messages')}") + messages = [] + raw_messages = proto_msg.get('messages', []) + try: + messages = Message.from_encoded_array(raw_messages, context=self.__decoding_context) + if messages: + self.__decoding_context.last_message_id = messages[-1].id + self.__channel_serial = channel_serial + except AblyException as e: + if e.code == 40018: # Delta decode failure - start recovery + self._start_decode_failure_recovery(e) + return # Skip processing during recovery + else: + log.exception(f"Message processing error: {e}. Skipping {len(raw_messages)} messages") + return
🧹 Nitpick comments (7)
pyproject.toml (3)
59-59: Optional dependency naming looks fine; keep prerelease allowance explicitUsing
^0.1.0a1is fine for Poetry. Ensure docs state that this is a pre-release and how to enable it if users pin via other tools.
64-64: Extras wiring good, contingent on PyPI availability
vcdiff = ["vcdiff-decoder"]is correct for extras. It still depends on the package being on PyPI (see earlier comment).
66-86: Fix deprecated section: move dev-dependencies to Poetry groupsGA warns:
[tool.poetry.dev-dependencies]is deprecated. Rename to[tool.poetry.group.dev.dependencies]. No behavior change.-[tool.poetry.dev-dependencies] +[tool.poetry.group.dev.dependencies] pytest = "^7.1" mock = "^4.0.3" ... vcdiff-decoder = { version = "^0.1.0a1" }ably/types/presence.py (1)
89-105: Pass context through to decode; removes unused-arg and enables delta if ever neededCurrently
contextis accepted but not used. Forward it todecode.- decoded_data = PresenceMessage.decode(data, encoding, cipher) + decoded_data = PresenceMessage.decode(data, encoding, cipher, context)ably/vcdiff/ably_vcdiff_decoder.py (1)
47-51: Prefer logging.exception for import failure; tighten guidance textPreserve traceback and keep a single clear user action.
- except ImportError as e: - log.error("vcdiff library not found. Install with: pip install ably[vcdiff]") - raise ImportError( - "VCDiff plugin requires vcdiff library. " - "Install with: pip install ably[vcdiff]" - ) from e + except ImportError as e: + log.exception("vcdiff-decoder not found") + raise ImportError("Install optional decoder with: pip install 'ably[vcdiff]'") from etest/ably/realtime/realtimechannel_vcdiff_test.py (2)
73-76: Re-raise without shadowing to keep tracebackUse bare
raisein handlers.- except Exception as e: - waitable_event.finish() - raise e + except Exception: + waitable_event.finish() + raiseAlso applies to: 115-117, 165-167
59-61: Redundant attach before subscribe
subscribe()awaitsattach()(RTL7c). You can drop the explicitawait channel.attach()to reduce chatter.Also applies to: 101-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
ably/__init__.py(1 hunks)ably/realtime/realtime_channel.py(5 hunks)ably/types/message.py(3 hunks)ably/types/mixins.py(4 hunks)ably/types/options.py(3 hunks)ably/types/presence.py(1 hunks)ably/vcdiff/ably_vcdiff_decoder.py(1 hunks)pyproject.toml(3 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(1 hunks)test/ably/rest/restauth_test.py(3 hunks)test/ably/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/ably/utils.py
- ably/init.py
- ably/types/options.py
- test/ably/rest/restauth_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-01T18:02:27.016Z
Learnt from: lmars
PR: ably/ably-python#571
File: ably/util/exceptions.py:74-74
Timestamp: 2024-10-01T18:02:27.016Z
Learning: In `AblyException.decode_error_response`, exceptions are caught and re-thrown as an `AblyException` in `AblyException.raise_for_response`, so there's no need to raise an `AblyException` within `decode_error_response`.
Applied to files:
ably/types/mixins.py
🧬 Code graph analysis (6)
ably/vcdiff/ably_vcdiff_decoder.py (3)
ably/types/options.py (3)
VCDiffDecoder(11-14)vcdiff_decoder(272-273)decode(13-14)ably/util/exceptions.py (1)
AblyException(9-84)test/ably/realtime/realtimechannel_vcdiff_test.py (2)
decode(20-23)decode(29-30)
ably/types/message.py (2)
ably/types/mixins.py (5)
EncodeDataMixin(30-131)DeltaExtras(14-20)decode(47-127)encoding(36-37)encoding(40-44)ably/util/exceptions.py (1)
AblyException(9-84)
ably/realtime/realtime_channel.py (5)
ably/transport/websockettransport.py (1)
ProtocolMessageAction(29-41)ably/types/message.py (3)
Message(24-231)id(77-78)id(81-82)ably/types/mixins.py (2)
DecodingContext(23-27)from_encoded_array(130-131)ably/types/options.py (1)
vcdiff_decoder(272-273)ably/util/exceptions.py (1)
AblyException(9-84)
ably/types/mixins.py (3)
ably/util/exceptions.py (1)
AblyException(9-84)ably/types/message.py (3)
extras(97-98)data(69-70)from_encoded(181-206)ably/types/options.py (2)
vcdiff_decoder(272-273)decode(13-14)
ably/types/presence.py (3)
ably/types/message.py (1)
from_encoded(181-206)ably/realtime/realtime_channel.py (1)
cipher(43-45)ably/rest/channel.py (1)
cipher(155-156)
test/ably/realtime/realtimechannel_vcdiff_test.py (8)
ably/vcdiff/ably_vcdiff_decoder.py (2)
AblyVCDiffDecoder(28-78)decode(53-78)ably/realtime/realtime_channel.py (7)
ChannelOptions(24-85)get(606-631)params(48-50)params(571-573)attach(158-196)name(550-552)subscribe(277-329)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)test/ably/utils.py (4)
BaseAsyncTestCase(43-58)WaitableEvent(238-252)finish(251-252)wait(248-249)ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/types/options.py (3)
VCDiffDecoder(11-14)vcdiff_decoder(272-273)decode(13-14)ably/types/mixins.py (1)
decode(47-127)ably/util/eventemitter.py (2)
once_async(168-181)on(44-86)
🪛 Ruff (0.12.2)
ably/vcdiff/ably_vcdiff_decoder.py
47-47: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
48-51: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Consider moving this statement to an else block
(TRY300)
77-77: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
78-78: Avoid specifying long messages outside the exception class
(TRY003)
ably/types/message.py
193-194: Avoid specifying long messages outside the exception class
(TRY003)
ably/realtime/realtime_channel.py
434-434: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
ably/types/mixins.py
78-78: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
103-103: Do not catch blind exception: Exception
(BLE001)
104-104: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
105-105: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
105-105: Avoid specifying long messages outside the exception class
(TRY003)
ably/types/presence.py
89-89: Unused static method argument: context
(ARG004)
test/ably/realtime/realtimechannel_vcdiff_test.py
29-29: Unused method argument: delta
(ARG002)
29-29: Unused method argument: base
(ARG002)
30-30: Create your own exception
(TRY002)
30-30: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Use raise without specifying exception name
Remove exception name
(TRY201)
117-117: Use raise without specifying exception name
Remove exception name
(TRY201)
167-167: Use raise without specifying exception name
Remove exception name
(TRY201)
🪛 GitHub Actions: .github/workflows/check.yml
pyproject.toml
[warning] 1-1: The 'poetry.dev-dependencies' section is deprecated and will be removed in a future version. Use 'poetry.group.dev.dependencies' instead.
🔇 Additional comments (1)
ably/realtime/realtime_channel.py (1)
416-416: Use logging.warning instead of deprecated warn[Suggest_nitpick]
- log.warn("RealtimeChannel._on_message(): ATTACHED received while not attaching") + log.warning("RealtimeChannel._on_message(): ATTACHED received while not attaching")
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/ably/realtime/realtimechannel_vcdiff_test.py (3)
1-10: Skip suite when optional extra is missing.Guard tests so CI without ably[vcdiff] doesn’t hard-fail.
import asyncio import json from ably import AblyVCDiffDecoder from ably.realtime.realtime_channel import ChannelOptions from test.ably.testapp import TestApp from test.ably.utils import BaseAsyncTestCase, WaitableEvent -from ably.realtime.connection import ConnectionState +from ably.types.connectionstate import ConnectionState from ably.types.options import VCDiffDecoder +import pytest + +# Skip if the optional decoder backend is not installed +pytest.importorskip("vcdiff_decoder", reason="vcdiff extra not installed")
146-153: Subscribe to enum state, not a string literal.Channel emits ChannelState values; string "attaching" won’t match.
- def on_attaching(state_change): + from ably.types.channelstate import ChannelState # move to top-level if preferred + def on_attaching(state_change): attaching_events.append(state_change) # RTL18c - Check error code if state_change.reason and state_change.reason.code: assert state_change.reason.code == 40018, "Check error code passed through per RTL18c" - channel.on('attaching', on_attaching) + channel.on(ChannelState.ATTACHING, on_attaching)
214-214: Use ChannelState enum here as well.- channel.on('attaching', on_attaching) + from ably.types.channelstate import ChannelState # ensure imported at top + channel.on(ChannelState.ATTACHING, on_attaching)
🧹 Nitpick comments (6)
setup.cfg (1)
11-11: Confirm widened Flake8 exclude (could hide defects).Excluding ably/sync and test/ably/sync removes those paths from linting. If ably/sync contains runtime code, this may mask issues. If it’s intentional (generated/legacy), add a brief rationale comment above to avoid future confusion; otherwise prefer narrowing or using extend-exclude just for tests.
-# Exclude virtual environment check -exclude = .venv,venv,env,.env,.git,__pycache__,.pytest_cache,build,dist,*.egg-info,ably/sync,test/ably/sync +# Exclude virtual envs, build artifacts, and (intentional) sync dirs (non-linted/legacy) +exclude = .venv,venv,env,.env,.git,__pycache__,.pytest_cache,build,dist,*.egg-info,ably/sync,test/ably/syncably/vcdiff/default_vcdiff_decoder.py (2)
7-18: Fix example imports in docstring.AblyVCDiffDecoder is re-exported at ably, and AblyRealtime is not under ably.vcdiff. Update the example to reflect actual imports.
-Usage: - from ably.vcdiff import AblyVCDiffDecoder, AblyRealtime +Usage: + from ably import AblyVCDiffDecoder, AblyRealtime
47-51: Use logging.exception and name the optional dependency correctly.This captures the traceback and clarifies the missing package.
- log.error("vcdiff library not found. Install with: pip install ably[vcdiff]") + log.exception("vcdiff-decoder not found. Install with: pip install ably[vcdiff]") raise ImportError( - "VCDiff plugin requires vcdiff library. " + "VCDiff plugin requires the vcdiff-decoder library. " "Install with: pip install ably[vcdiff]" ) from etest/ably/realtime/realtimechannel_vcdiff_test.py (3)
73-76: Use bareraiseto preserve original traceback.Re-raising the caught exception by name rewrites the traceback.
- except Exception as e: - waitable_event.finish() - raise e + except Exception: + waitable_event.finish() + raise- except Exception as e: - waitable_event.finish() - raise e + except Exception: + waitable_event.finish() + raise- except Exception as e: - waitable_event.finish() - raise e + except Exception: + waitable_event.finish() + raiseAlso applies to: 115-118, 165-168
36-37: Remove unused test variable.valid_key_format is never used.
- self.valid_key_format = "api:key" -
59-61: Avoid redundant attach (subscribe already awaits attach).Keeps tests shorter and avoids double state transitions.
- await channel.attach() + # subscribe() will attach if needed- await channel.attach() + # subscribe() will attach if needed- await channel.attach() + # subscribe() will attach if neededAlso applies to: 101-104, 192-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
ably/__init__.py(1 hunks)ably/types/options.py(3 hunks)ably/vcdiff/default_vcdiff_decoder.py(1 hunks)setup.cfg(1 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ably/types/options.py
- ably/init.py
🧰 Additional context used
🧬 Code graph analysis (2)
ably/vcdiff/default_vcdiff_decoder.py (3)
ably/types/options.py (3)
VCDiffDecoder(11-24)vcdiff_decoder(282-283)decode(23-24)ably/util/exceptions.py (1)
AblyException(9-84)test/ably/realtime/realtimechannel_vcdiff_test.py (2)
decode(20-23)decode(29-30)
test/ably/realtime/realtimechannel_vcdiff_test.py (7)
ably/vcdiff/default_vcdiff_decoder.py (2)
AblyVCDiffDecoder(28-78)decode(53-78)ably/realtime/realtime_channel.py (7)
ChannelOptions(24-85)get(606-631)params(48-50)params(571-573)attach(158-196)name(550-552)subscribe(277-329)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)test/ably/utils.py (4)
BaseAsyncTestCase(43-58)WaitableEvent(238-252)finish(251-252)wait(248-249)ably/types/connectionstate.py (1)
ConnectionState(8-16)ably/types/options.py (3)
VCDiffDecoder(11-24)vcdiff_decoder(282-283)decode(23-24)ably/types/mixins.py (1)
decode(47-127)
🪛 Ruff (0.12.2)
ably/vcdiff/default_vcdiff_decoder.py
47-47: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
48-51: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Consider moving this statement to an else block
(TRY300)
77-77: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
78-78: Avoid specifying long messages outside the exception class
(TRY003)
test/ably/realtime/realtimechannel_vcdiff_test.py
29-29: Unused method argument: delta
(ARG002)
29-29: Unused method argument: base
(ARG002)
30-30: Create your own exception
(TRY002)
30-30: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Use raise without specifying exception name
Remove exception name
(TRY201)
117-117: Use raise without specifying exception name
Remove exception name
(TRY201)
167-167: Use raise without specifying exception name
Remove exception name
(TRY201)
⏰ 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). (7)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.7)
- GitHub Check: check (3.8)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
🔇 Additional comments (1)
ably/vcdiff/default_vcdiff_decoder.py (1)
73-75: Confirm vcdiff-decoder call signature (base, delta).Some libraries expect (delta, base). Please confirm to avoid silent corruption.
Resolves #602
VCDiffDecoderabstract class andVCDiffPluginimplementation.Optionsto accept avcdiff_decoder.Summary by CodeRabbit
New Features
Tests
Chores