feat: introduce ChannelOptions for enhanced channel configuration #622
Conversation
WalkthroughAdds ChannelOptions for per-channel cipher/params, persists options in RealtimeChannel, includes params in ATTACH, tracks attach/channel serials, exposes set_options and params, and updates Channels.get to accept/validate ChannelOptions or dicts and enforce safe option changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant Channels
participant RealtimeChannel
participant AblyServer
Developer->>Channels: get(name, options)
alt channel does not exist
Channels->>RealtimeChannel: new RealtimeChannel(name, ChannelOptions)
RealtimeChannel->>AblyServer: ATTACH { params: ChannelOptions.params }
AblyServer-->>RealtimeChannel: ATTACHED { attachSerial, channelSerial, params }
RealtimeChannel->>RealtimeChannel: store __attach_serial, __channel_serial, __params
else channel exists
Channels->>RealtimeChannel: compare options
alt reattach required
Channels-->>Developer: AblyException (use set_options)
else safe to update without reattach
Channels->>RealtimeChannel: set_options_without_reattach(newOptions)
else set_options triggers reattach
RealtimeChannel->>AblyServer: ATTACH { params: newOptions.params }
AblyServer-->>RealtimeChannel: ATTACHED { attachSerial, channelSerial, params }
RealtimeChannel->>RealtimeChannel: update serials/params
end
end
Note over AblyServer,RealtimeChannel: On MESSAGE, update __channel_serial before emitting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
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. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/622/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: 3
🧹 Nitpick comments (2)
ably/realtime/realtime_channel.py (2)
530-534: Consider more granular reattachment logicThe current implementation triggers reattachment whenever options differ. Consider if certain option changes (like cipher changes) might not require reattachment depending on the protocol requirements.
If the protocol allows certain option changes without reattachment, consider implementing more granular logic:
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 - return old_options != new_options + # Only reattach if params changed - cipher changes might not require reattachment + if old_options.params != new_options.params: + return True + # Add other conditions as per protocol requirements + return False
38-38: Consider moving error messages to exception classThe static analysis suggests avoiding long messages outside the exception class. While not critical, consider defining these as constants in the AblyException class or using shorter inline messages.
Consider refactoring to use exception message constants or factory methods in the AblyException class to improve maintainability and consistency of error messages across the codebase.
Also applies to: 59-59, 91-91, 586-586, 594-599
📜 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)
ably/realtime/realtime_channel.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ably/realtime/realtime_channel.py (5)
ably/rest/channel.py (7)
ably(143-144)cipher(155-156)get(182-193)name(147-148)Channel(22-174)options(159-160)options(167-174)ably/util/crypto.py (1)
CipherParams(16-42)ably/util/exceptions.py (1)
AblyException(9-84)ably/realtime/realtime.py (1)
AblyRealtime(12-140)ably/util/eventemitter.py (1)
once_async(168-181)
🪛 Ruff (0.12.2)
ably/realtime/realtime_channel.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
586-586: Avoid specifying long messages outside the exception class
(TRY003)
594-599: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Linting check
ably/realtime/realtime_channel.py
[error] 63-63: flake8: E303 too many blank lines (2). Command: poetry run flake8
[error] 595-595: flake8: E501 line too long (117 > 115 characters). Command: poetry run flake8
⏰ 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.11)
- GitHub Check: check (3.12)
- GitHub Check: check (3.10)
- GitHub Check: check (3.7)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (5)
ably/realtime/realtime_channel.py (5)
22-97: Well-structured ChannelOptions implementation with proper validationThe ChannelOptions class is well-designed with appropriate validation, property access patterns, and conversion utilities. The implementation correctly validates
paramsas a dictionary and provides both equality checking and hashability for consistent behavior.
136-144: Clean integration of ChannelOptions with inheritance hierarchyThe channel options are properly initialized with a default instance, and correctly passed to the parent Channel class as a dictionary. This maintains backward compatibility while adding the new functionality.
199-215: Channel parameters correctly included in ATTACH messageThe implementation properly includes channel parameters in the ATTACH protocol message, ensuring they're sent to the server during channel attachment.
410-411: Consistent serial tracking implementationThe serial tracking for ATTACHED and MESSAGE actions is implemented correctly. When ATTACHED is received in an already attached state with the resumed flag, both
__attach_serialand__channel_serialare updated appropriately. For MESSAGE actions, only__channel_serialis updated as expected.Also applies to: 425-425
145-157: Add proper async implementation for set_optionsThe
set_optionsmethod should be properly marked as async since it contains await statements and handles asynchronous state changes.- async def set_options(self, channel_options: ChannelOptions) -> None: + async def set_options(self, channel_options: ChannelOptions) -> None: """Set channel options""" old_channel_options = self.__channel_options self.__channel_options = channel_options # Update parent class options self.options = channel_options.to_dict() if self.should_reattach_to_set_options(old_channel_options, channel_options): - self._attach_impl() + await self._attach_impl() state_change = await self.__internal_state_emitter.once_async() if state_change.current in (ChannelState.SUSPENDED, ChannelState.FAILED): raise state_change.reasonAlso,
_attach_implneeds to be made async to match this usage pattern, or the call should be wrapped appropriately.Likely an incorrect or invalid review comment.
ttypic
force-pushed
the
feat/channel-options
branch
from
16f3ec3 to
5bec2a7
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
ttypic
force-pushed
the
fix/fix-tests-update-ci
branch
from
3cc9a9a to
9bfa4db
Compare
ttypic
force-pushed
the
feat/channel-options
branch
from
5bec2a7 to
3671ec8
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
ttypic
force-pushed
the
feat/channel-options
branch
2 times, most recently
from
b0d7781 to
9032c63
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
ttypic
force-pushed
the
feat/channel-options
branch
from
9032c63 to
f84e7e6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
ttypic
force-pushed
the
fix/fix-tests-update-ci
branch
from
490767a to
0659f89
Compare
ttypic
force-pushed
the
feat/channel-options
branch
from
f84e7e6 to
22aecd6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
ttypic
force-pushed
the
fix/fix-tests-update-ci
branch
from
0659f89 to
a1dfdd7
Compare
ttypic
force-pushed
the
feat/channel-options
branch
from
22aecd6 to
29bb3a3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
ttypic
force-pushed
the
fix/fix-tests-update-ci
branch
from
a1dfdd7 to
05054a8
Compare
ttypic
force-pushed
the
feat/channel-options
branch
from
29bb3a3 to
01c3725
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Since we are introducing ChannelOptions, can we add tests for the same.
- Add
paramsproperty toRealtimeChannelclass and set it from receivedProtocolMessagesimilar to channel.params = protoMessage.params. Add test for the same, check if sent params fromattachMessageare received and set on channel params. - Optional, but you can also introduce modes to
ChannelOptionssimilar to ChannelOptions#modes. Add test for the same.
|
thanks @sacOO7, I'll add tests for params, but I think modes is out of the scope, we'll add them later |
ttypic
force-pushed
the
fix/fix-tests-update-ci
branch
2 times, most recently
from
23d1036 to
30fdc5d
Compare
ttypic
force-pushed
the
feat/channel-options
branch
from
01c3725 to
28135ae
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ably/realtime/realtimechannel_test.py (1)
521-533: Tighten the negative case assertionOptionally assert on the exception’s code/message to avoid false positives (any AblyException would currently pass).
Example:
with pytest.raises(AblyException) as exc: ably.channels.get(channel_name, {"params": {"delta": "vcdiff"}}) assert getattr(exc.value, "code", None) in {40000, 40010, 400} # adjust to the actual code raised
📜 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 selected for processing (2)
ably/realtime/realtime_channel.py(9 hunks)test/ably/realtime/realtimechannel_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/realtime/realtime_channel.py
🧰 Additional context used
🪛 GitHub Actions: Linting check
test/ably/realtime/realtimechannel_test.py
[error] 475-475: flake8 E251: unexpected spaces around keyword / parameter equals.
[error] 475-475: flake8 E251: unexpected spaces around keyword / parameter equals.
[error] 486-486: flake8 E251: unexpected spaces around keyword / parameter equals.
[error] 486-486: flake8 E251: unexpected spaces around keyword / parameter equals.
[error] 500-500: flake8 E201: whitespace after '{'.
[error] 500-500: flake8 E202: whitespace before '}'.
[error] 511-511: flake8 E201: whitespace after '{'.
[error] 511-511: flake8 E202: whitespace before '}'.
[error] 515-515: flake8 E201: whitespace after '{'.
[error] 515-515: flake8 E202: whitespace before '}'.
[error] 525-525: flake8 E201: whitespace after '{'.
[error] 525-525: flake8 E202: whitespace before '}'.
[error] 528-528: flake8 W293: blank line contains whitespace.
[error] 530-530: flake8 F841: local variable 'same_channel' is assigned to but never used.
[error] 531-531: flake8 E201: whitespace after '{'.
[error] 531-531: flake8 E202: whitespace before '}'.
[error] 534-534: flake8 E201: whitespace after '{'.
[error] 534-534: flake8 E202: whitespace before '}'.
[error] 537-537: flake8 W391: blank line at end of file.
⏰ 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.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.7)
- GitHub Check: check (3.8)
🔇 Additional comments (1)
test/ably/realtime/realtimechannel_test.py (1)
3-3: Confirm public import path for ChannelOptionsMake sure
ChannelOptionsis actually defined/exported fromably.realtime.realtime_channel; if it moved (e.g.,ably.types), the tests will fail to import.Run to verify definition and import sites:
#!/bin/bash # Locate ChannelOptions definition and any public exports rg -nP 'class\s+ChannelOptions\b|ChannelOptions\s*=' -C2 # Check all import statements referencing ChannelOptions rg -nP 'from\s+ably\.[\w\.]+\s+import\s+.*\bChannelOptions\b' -C1
ttypic
force-pushed
the
feat/channel-options
branch
from
28135ae to
a06a6da
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
ably/realtime/realtime_channel.py (1)
589-597: Fix AblyException code/status order and await async set_options in Channels.get
- Arguments are reversed (should be code=40000, status=400).
set_optionsis async and currently not awaited, so updates won’t apply. Schedule it.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 + 40000, + 400 ) elif options: - channel.set_options(options) + asyncio.create_task(channel.set_options(options))
🧹 Nitpick comments (4)
ably/realtime/realtime_channel.py (4)
4-4: Import Mapping for safer type hintsChannelOptions would benefit from
Mappingto accept any mapping-like object and to support copying.-from typing import Optional, TYPE_CHECKING, Dict, Any, Union +from typing import Optional, TYPE_CHECKING, Dict, Any, Union, Mapping
111-132: Initialize RealtimeChannel params from ChannelOptions for immediate visibilityOptional: seed
__paramsfrom provided options sochannel.paramsreflects configured params before first ATTACH.- self.__channel_options = channel_options or ChannelOptions() - self.__params: Optional[Dict[str, str]] = None + self.__channel_options = channel_options or ChannelOptions() + self.__params: Optional[Dict[str, str]] = ( + dict(self.__channel_options.params) if self.__channel_options.params else None + )
415-416: Guard channelSerial update on MESSAGEPrevent resetting to None when the field is absent.
- self.__channel_serial = channel_serial + if channel_serial is not None: + self.__channel_serial = channel_serial
520-524: Simplify and clarify reattach predicateEquivalent logic, more readable.
- def should_reattach_to_set_options(self, new_options: ChannelOptions) -> bool: - if self.state != ChannelState.ATTACHING and self.state != ChannelState.ATTACHED: - return False - return self.__channel_options != new_options + def should_reattach_to_set_options(self, new_options: ChannelOptions) -> bool: + return ( + self.state in (ChannelState.ATTACHING, ChannelState.ATTACHED) + and self.__channel_options != new_options + )
📜 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 selected for processing (2)
ably/realtime/realtime_channel.py(9 hunks)test/ably/realtime/realtimechannel_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ably/realtime/realtimechannel_test.py
⏰ 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.13)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
- GitHub Check: check (3.12)
- GitHub Check: check (3.10)
- GitHub Check: check (3.7)
🔇 Additional comments (2)
ably/realtime/realtime_channel.py (2)
387-390: Guard serial/params updates; don’t overwrite with None; use attachSerial if availableSetting
__attach_serialtochannelSeriallooks incorrect; also avoid clobbering existing values with None.- self.__attach_serial = channel_serial - self.__channel_serial = channel_serial - self.__params = proto_msg.get('params') + # Prefer attachSerial when provided, and avoid overwriting with None + attach_serial = proto_msg.get('attachSerial') + if attach_serial is not None: + self.__attach_serial = attach_serial + if channel_serial is not None: + self.__channel_serial = channel_serial + if 'params' in proto_msg: + self.__params = proto_msg.get('params')Would you like me to verify the Ably Realtime protocol fields for ATTACHED messages and confirm attachSerial/channelSerial semantics?
547-551: Fix type hint and copy-out for RealtimeChannel.paramsThe property may be None; return a copy to avoid external mutation.
@property - def params(self) -> Dict[str, str]: + def params(self) -> Optional[Dict[str, str]]: """Get channel parameters""" - return self.__params + return dict(self.__params) if self.__params is not None else NoneLikely an incorrect or invalid review comment.
ttypic
force-pushed
the
feat/channel-options
branch
from
a06a6da to
85c8e70
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
ably/realtime/realtime_channel.py (6)
565-582: Consider accepting only ChannelOptions for consistencyAs per the spec RTS3c, the signature should be
get(String, ChannelOptions) -> ChannelType. The current implementation accepts both dict and ChannelOptions which may lead to inconsistency.Consider strictly accepting
ChannelOptionsto align with the specification and maintain consistency, especially since future fields likeattachOnSubscribe(boolean) can't be properly represented in a plain dict.
65-73: to_dict() exposes internal mutable referenceThe
to_dict()method returns the internal__paramsdict directly, allowing external code to mutate the channel options.def to_dict(self) -> Dict[str, Any]: """Convert to dictionary representation""" result = {} if self.__cipher is not None: result['cipher'] = self.__cipher if self.__params: - result['params'] = self.__params + result['params'] = dict(self.__params) return result
133-145: set_options() can hang indefinitely when channel is ATTACHEDCalling
_attach_impl()directly won't emit a state change when the current state is ATTACHED, causingonce_async()to wait forever. Use_request_state(ATTACHING)to properly trigger state transitions.async def set_options(self, channel_options: ChannelOptions) -> None: """Set channel options""" should_reattach = self.should_reattach_to_set_options(channel_options) self.__channel_options = channel_options # Update parent class options self.options = channel_options.to_dict() if should_reattach: - self._attach_impl() + self._request_state(ChannelState.ATTACHING) state_change = await self.__internal_state_emitter.once_async() if state_change.current in (ChannelState.SUSPENDED, ChannelState.FAILED): raise state_change.reason
588-597: Async method called without awaitThe
set_optionsmethod is async but called without await on line 596, which will create an unawaited coroutine warning.# Update options if channel is not attached or currently attaching 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(options) + asyncio.create_task(channel.set_options(options))
22-39: Potential mutability issues with ChannelOptions paramsThe
paramsparameter accepts a mutable dict but stores it by reference, which could lead to external mutations affecting the channel's behavior unexpectedly. Additionally, the type hint for theparamsproperty suggests it's never None, but it can be.Apply these changes to ensure immutability:
- def __init__(self, cipher: Optional[CipherParams] = None, params: Optional[dict] = None): + def __init__(self, cipher: Optional[CipherParams] = None, params: Optional[Dict[str, str]] = None): self.__cipher = cipher - self.__params = params + # Copy params to prevent external mutation + self.__params = dict(params) if params else None # Validate params if self.__params and not isinstance(self.__params, dict): raise AblyException("params must be a dictionary", 40000, 400)And update the property:
@property - def params(self) -> Dict[str, str]: + def params(self) -> Optional[Dict[str, str]]: """Get channel parameters""" - return self.__params + return dict(self.__params) if self.__params else None
58-63: Unsafe hash implementation with mutable paramsThe
__hash__method can crash at runtime ifcipheris unhashable, and having__hash__with mutableparamsviolates Python's hashability contract (objects that compare equal must have the same hash value).Since
paramscan be mutated externally (current implementation) andCipherParamsmay not be hashable, remove the__hash__method:- def __hash__(self): - """Make ChannelOptions hashable""" - return hash(( - self.__cipher, - tuple(sorted(self.__params.items())) if self.__params else None, - ))If hashability is required, ensure both immutability and that all components are hashable.
🧹 Nitpick comments (2)
ably/realtime/realtime_channel.py (2)
191-195: Avoid sending null params in ATTACH messageThe ATTACH message always includes
paramseven when it's None, which may not be desired.attach_msg = { "action": ProtocolMessageAction.ATTACH, - "params": self.__channel_options.params, "channel": self.name, } + if self.__channel_options.params: + attach_msg["params"] = self.__channel_options.params
547-551: params property potentially returns NoneThe
paramsproperty can return None but the type hint suggests it always returns a Dict.@property - def params(self) -> Dict[str, str]: + def params(self) -> Optional[Dict[str, str]]: """Get channel parameters""" return self.__params
📜 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 selected for processing (2)
ably/realtime/realtime_channel.py(9 hunks)test/ably/realtime/realtimechannel_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ably/realtime/realtimechannel_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
ably/realtime/realtime_channel.py (5)
ably/rest/channel.py (7)
ably(143-144)cipher(155-156)get(182-193)name(147-148)Channel(22-174)options(159-160)options(167-174)ably/util/crypto.py (1)
CipherParams(16-42)ably/util/exceptions.py (2)
AblyException(9-84)from_dict(83-84)ably/util/eventemitter.py (1)
once_async(168-181)ably/types/channelstate.py (1)
ChannelState(7-14)
🪛 Ruff (0.12.2)
ably/realtime/realtime_channel.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
78-78: Avoid specifying long messages outside the exception class
(TRY003)
581-581: Avoid specifying long messages outside the exception class
(TRY003)
589-594: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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.7)
- GitHub Check: check (3.8)
- GitHub Check: check (3.11)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
- GitHub Check: check (3.12)
- GitHub Check: check (3.13)
🔇 Additional comments (2)
ably/realtime/realtime_channel.py (2)
387-389: Correct implementation of attach serial handlingThe code correctly updates both
__attach_serialand__channel_serialwhen receiving an ATTACHED message, and stores the server-provided params.
520-524: LGTM - Clean reattachment logicThe
should_reattach_to_set_optionsmethod properly checks if reattachment is needed only when the channel is ATTACHING or ATTACHED and options have changed.
ttypic
force-pushed
the
feat/channel-options
branch
from
85c8e70 to
a5e34dc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/622/features
ttypic
force-pushed
the
feat/channel-options
branch
from
a5e34dc to
204138f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
ably/realtime/realtime_channel.py (4)
4-4: Fix AblyException arg order; make params immutable; drop unsafe hash; tighten types.
- AblyException calls use wrong arg order at Lines 37-38 and 77-78 (status_code, code).
- Expose/retain params by reference; later mutation breaks equality semantics.
- hash is unsafe (mutable params; cipher may be unhashable).
- Type of params should be Mapping[str, str]; property should return Optional[Dict[str, str]] and copy.
Apply:
-from typing import Optional, TYPE_CHECKING, Dict, Any +from typing import Optional, TYPE_CHECKING, Dict, Any, Mapping @@ -class ChannelOptions: +class ChannelOptions: @@ - def __init__(self, cipher: Optional[CipherParams] = None, params: Optional[dict] = None): - self.__cipher = cipher - self.__params = params - # Validate params - if self.__params and not isinstance(self.__params, dict): - raise AblyException("params must be a dictionary", 40000, 400) + def __init__(self, cipher: Optional[CipherParams] = None, params: Optional[Mapping[str, str]] = None): + self.__cipher = cipher + if params is not None and not isinstance(params, Mapping): + raise AblyException("params must be a dictionary", 400, 40000) + # store a copy to prevent external mutation + self.__params = dict(params) if params is not None else None @@ - def params(self) -> Dict[str, str]: - """Get channel parameters""" - return self.__params + def params(self) -> Optional[Dict[str, str]]: + """Get channel parameters (copy)""" + return dict(self.__params) if self.__params is not None else None @@ - def __hash__(self): - """Make ChannelOptions hashable""" - return hash(( - self.__cipher, - tuple(sorted(self.__params.items())) if self.__params else None, - )) + # Note: __hash__ intentionally omitted due to mutability and potential unhashable cipher. @@ - def to_dict(self) -> Dict[str, Any]: + def to_dict(self) -> Dict[str, Any]: """Convert to dictionary representation""" result = {} if self.__cipher is not None: result['cipher'] = self.__cipher - if self.__params: - result['params'] = self.__params + if self.__params: + result['params'] = dict(self.__params) return result @@ - def from_dict(cls, options_dict: Dict[str, Any]) -> 'ChannelOptions': + def from_dict(cls, options_dict: Dict[str, Any]) -> 'ChannelOptions': """Create ChannelOptions from dictionary""" if not isinstance(options_dict, dict): - raise AblyException("options must be a dictionary", 40000, 400) + raise AblyException("options must be a dictionary", 400, 40000) return cls( cipher=options_dict.get('cipher'), params=options_dict.get('params'), )I can add unit tests asserting immutability and correct AblyException codes. Want me to open an issue and provide tests?
Also applies to: 22-39, 45-49, 58-63, 65-73, 74-83
133-143: set_options() can hang; request ATTACHING instead of calling _attach_impl() directly.Calling _attach_impl() while ATTACHED won’t emit a state change; once_async() may await forever. Trigger the state machine.
if should_reattach: - self._attach_impl() - state_change = await self.__internal_state_emitter.once_async() + self._request_state(ChannelState.ATTACHING) + state_change = await self.__internal_state_emitter.once_async() if state_change.current in (ChannelState.SUSPENDED, ChannelState.FAILED): raise state_change.reason
195-199: Avoid sending"params": Nonein ATTACH frame.Only include params when present.
attach_msg = { "action": ProtocolMessageAction.ATTACH, - "params": self.__channel_options.params, "channel": self.name, } + if self.__channel_options.params: + attach_msg["params"] = self.__channel_options.params
552-556: params property should be Optional and return a copy.Prevents accidental external mutation and matches None default.
- def params(self) -> Dict[str, str]: - """Get channel parameters""" - return self.__params + def params(self) -> Optional[Dict[str, str]]: + """Get channel parameters""" + return dict(self.__params) if self.__params is not None else None
🧹 Nitpick comments (2)
ably/realtime/realtime_channel.py (2)
578-580: Docstring: accept ChannelOptions only (per spec).The signature is strict; update docs.
- options: ChannelOptions or dict, optional - Channel options for the channel + options: ChannelOptions, optional + Channel options for the channel
586-594: Nit: quell TRY003 by extracting the long error message.Optional style tweak for Ruff; logic is fine.
- 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 - ) + if options and channel.should_reattach_to_set_options(options): + msg = ( + 'Channels.get() cannot be used to set channel options that would cause the channel to ' + 'reattach. Please, use RealtimeChannel.setOptions() instead.' + ) + raise AblyException(msg, 400, 40000)
📜 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 selected for processing (2)
ably/realtime/realtime_channel.py(9 hunks)test/ably/realtime/realtimechannel_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ably/realtime/realtimechannel_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
ably/realtime/realtime_channel.py (4)
ably/rest/channel.py (7)
ably(143-144)cipher(155-156)get(182-193)name(147-148)Channel(22-174)options(159-160)options(167-174)ably/util/crypto.py (1)
CipherParams(16-42)ably/util/exceptions.py (2)
AblyException(9-84)from_dict(83-84)ably/util/eventemitter.py (1)
once_async(168-181)
🪛 Ruff (0.12.2)
ably/realtime/realtime_channel.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
78-78: Avoid specifying long messages outside the exception class
(TRY003)
587-592: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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.8)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
- GitHub Check: check (3.7)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
ChannelOptionsclass to handle channel parameters and cipher configurations.RealtimeChannelto supportChannelOptionsSummary by CodeRabbit
New Features
Bug Fixes
Tests