[AIT-96] feat: add msgpack suppport for websocket transport #648
Conversation
WalkthroughAdds format-aware WebSocket framing (json/msgpack) driven by transport params; Message.from_encoded_array gains an optional cipher parameter and is called with channel cipher for inbound MESSAGE decoding; connection params set format=msgpack when binary protocol is enabled; tests parametrized for json/msgpack and include an encryption round‑trip. Changes
(*Exact path for sequenceDiagram
participant Client
participant ConnMgr as ConnectionManager
participant WS as WebSocketTransport
participant Network
participant Channel as RealtimeChannel
Client->>ConnMgr: create connection (use_binary_protocol flag)
ConnMgr->>WS: init transport params (format='msgpack' if binary)
Client->>Channel: publish(message)
Channel->>WS: send(payload)
alt format == msgpack
WS->>WS: msgpack.packb(payload) → bytes
WS->>Network: send binary frame
else format == json
WS->>WS: json.dumps(payload) → string
WS->>Network: send text frame
end
Network->>WS: receive frame (raw)
WS->>WS: decode_raw_websocket_frame(raw)
alt msgpack frame
WS->>WS: msgpack.unpackb(raw) → array/dict
else json frame
WS->>WS: json.loads(raw) → array/dict
end
WS->>Channel: deliver decoded message array
Channel->>Channel: Message.from_encoded_array(array, cipher=self.cipher)
Channel-->>Client: decoded (and possibly decrypted) Message objects
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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/648/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)
test/ably/realtime/realtimechannel_publish_test.py (2)
936-966: Encryption test hardcodesuse_binary_protocol=True, ignoring the parameterized transport.This test always uses binary protocol regardless of the
transportparameter, which means it won't test encryption with JSON transport.Consider using the parameterized transport value:
async def test_publish_with_encryption(self): """Verify that encrypted messages can be published and received correctly""" - # Create connection with binary protocol enabled - self.ably = await TestApp.get_ably_realtime(use_binary_protocol=True) + # Use the existing self.ably from fixture which respects the transport paramOr if you specifically need binary protocol for this test, document why and skip for JSON:
if self.ably.connection.connection_manager.transport.format == 'json': pytest.skip('Encryption test requires binary protocol')
889-891: Address the TODO or add a tracking issue.The test is skipped for msgpack with a "todo fix this" comment. Consider:
- Creating an issue to track this
- Adding more context about what needs fixing
if self.ably.connection.connection_manager.transport.format == 'msgpack': - pytest.skip('todo fix this') + pytest.skip('Idempotent publishing test fails with msgpack - see issue #XXX')Do you want me to help investigate the idempotent publishing issue with msgpack or create an issue to track it?
📜 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/realtime/connectionmanager.py(1 hunks)ably/realtime/realtime_channel.py(1 hunks)ably/transport/websockettransport.py(4 hunks)test/ably/realtime/realtimechannel_publish_test.py(26 hunks)test/ably/realtime/realtimeconnection_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
test/ably/realtime/realtimeconnection_test.py (2)
ably/types/options.py (2)
use_binary_protocol(161-162)use_binary_protocol(165-166)ably/transport/websockettransport.py (1)
decode_raw_websocket_frame(202-205)
ably/realtime/connectionmanager.py (3)
ably/http/http.py (1)
options(275-276)ably/rest/rest.py (1)
options(112-113)ably/types/options.py (2)
use_binary_protocol(161-162)use_binary_protocol(165-166)
test/ably/realtime/realtimechannel_publish_test.py (5)
ably/realtime/realtime_channel.py (6)
ChannelOptions(26-87)get(741-766)attach(160-198)state(691-693)state(696-697)cipher(45-47)ably/types/channelstate.py (1)
ChannelState(8-15)ably/util/crypto.py (1)
CipherParams(16-42)ably/util/eventemitter.py (1)
once_async(169-182)ably/types/connectionstate.py (1)
ConnectionState(8-16)
ably/realtime/realtime_channel.py (3)
ably/types/message.py (1)
Message(24-231)ably/types/mixins.py (1)
from_encoded_array(129-130)ably/rest/channel.py (2)
get(184-195)cipher(157-158)
⏰ 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.14)
- GitHub Check: check (3.11)
- GitHub Check: check (3.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.8)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
🔇 Additional comments (7)
ably/transport/websockettransport.py (2)
243-250: LGTM! Format-aware encoding is correctly implemented.The conditional encoding based on
self.formatis clean. The asymmetric logging (length for msgpack, full payload for JSON) makes sense for binary data.
76-76: LGTM!The format extraction with 'json' as default correctly aligns with the connection manager's behavior.
ably/realtime/connectionmanager.py (1)
157-159: LGTM! RTN2a implementation is correct.The conditional addition of
format=msgpackonly whenuse_binary_protocolis enabled aligns with the Ably spec and correctly integrates with the WebSocketTransport's default JSON handling.ably/realtime/realtime_channel.py (1)
561-562: LGTM! Cipher parameter correctly passed for message decryption.The
cipher=self.cipherparameter enables proper decryption of incoming encrypted messages, aligning with thefrom_encoded_arraysignature inably/types/mixins.py.test/ably/realtime/realtimeconnection_test.py (2)
404-432: LGTM! Comprehensive test for msgpack format verification.The test correctly verifies:
- Transport format is set to 'msgpack'
- Params include
format=msgpack- Received frames are bytes (binary)
The frame interception approach is clean and doesn't interfere with actual protocol handling.
434-462: LGTM! Good complementary test for JSON format.The test properly verifies the default JSON behavior and confirms frames are strings. The assertion
transport_format is None or transport_format == 'json'correctly handles both cases where the param might not be set.test/ably/realtime/realtimechannel_publish_test.py (1)
15-15: LGTM! Good parameterization for transport format coverage.The
@pytest.mark.parametrize("transport", ["json", "msgpack"])decorator ensures all tests run against both serialization formats, which is excellent for verifying the new msgpack support.
ttypic
force-pushed
the
AIT-96/msgpack-suppport
branch
from
06fbd9b to
2a7fc20
Compare
github-actions
bot
temporarily deployed
to
staging/pull/648/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/ably/realtime/realtimechannel_publish_test.py (3)
22-22: Simplify boolean assignment.The ternary expression can be simplified to a direct comparison.
- self.use_binary_protocol = True if transport == 'msgpack' else False + self.use_binary_protocol = transport == 'msgpack'
942-944: Track the TODO for msgpack idempotent publishing.The skip indicates idempotent publishing doesn't work correctly with msgpack, but the comment lacks detail on the root cause. Consider adding a more descriptive comment or linking to a tracking issue.
Would you like me to open an issue to track this known limitation, or can you provide more context on what's broken with msgpack for idempotent publishing?
1010-1010: Remove commented-out code.This appears to be a debug artifact. If
message.decode()isn't needed, remove the comment to keep the test clean.try: - # message.decode() received_data = message.data
📜 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 (1)
test/ably/realtime/realtimechannel_publish_test.py(31 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/ably/realtime/realtimechannel_publish_test.py (3)
ably/realtime/realtime_channel.py (3)
ChannelOptions(26-87)cipher(45-47)get(741-766)ably/util/crypto.py (1)
CipherParams(16-42)test/ably/testapp.py (2)
get_test_vars(41-70)get_ably_realtime(80-83)
⏰ 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.9)
- GitHub Check: check (3.13)
- GitHub Check: check (3.14)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.12)
- GitHub Check: check (3.8)
🔇 Additional comments (1)
test/ably/realtime/realtimechannel_publish_test.py (1)
993-1025: Good addition for encryption round-trip validation.This test effectively validates that encrypted messages work correctly with both JSON and msgpack transports, ensuring binary cipher data is properly serialized and deserialized.
ttypic
force-pushed
the
AIT-96/msgpack-suppport
branch
from
2a7fc20 to
0a93f11
Compare
github-actions
bot
temporarily deployed
to
staging/pull/648/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ably/realtime/realtimechannel_publish_test.py (1)
999-999: Consider using transport-specific channel name to prevent test interference.Unlike
test_idempotent_realtime_publishing(line 945), which includesself.use_binary_protocolin the channel name to ensure uniqueness across parametrized test runs, this test uses a hardcoded channel name. While pytest may run parametrized tests sequentially, using a unique name would be safer.Apply this diff:
- channel = ably.channels.get('encrypted_channel', channel_options) + channel = ably.channels.get(f'encrypted_channel_{self.use_binary_protocol}', channel_options)
📜 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 (1)
test/ably/realtime/realtimechannel_publish_test.py(31 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/ably/realtime/realtimechannel_publish_test.py (4)
ably/realtime/realtime_channel.py (3)
ChannelOptions(26-87)get(741-766)cipher(45-47)ably/util/crypto.py (1)
CipherParams(16-42)ably/util/exceptions.py (2)
AblyException(9-84)IncompatibleClientIdException(103-104)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)
⏰ 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.8)
- GitHub Check: check (3.14)
- GitHub Check: check (3.9)
- GitHub Check: check (3.11)
- GitHub Check: check (3.13)
- GitHub Check: check (3.10)
- GitHub Check: check (3.12)
🔇 Additional comments (4)
test/ably/realtime/realtimechannel_publish_test.py (4)
6-6: LGTM! Imports correctly support the new encryption test.The addition of
ChannelOptionsandCipherParamsimports enables the encryption functionality tested intest_publish_with_encryption.Also applies to: 9-9
15-15: LGTM! Parametrization correctly enables dual-format testing.The decorator ensures all tests run with both JSON and MsgPack transports, fulfilling the PR's objective of comprehensive serialization testing.
20-22: LGTM! Setup fixture correctly derives binary protocol flag.The fixture properly accepts the parametrized transport and sets
use_binary_protocolaccordingly. Since no resources are created here, the absence of cleanup is appropriate.
27-27: LGTM! All existing tests correctly updated for parametrized transports.Each test now passes
use_binary_protocol=self.use_binary_protocolto ensure proper testing with both JSON and MsgPack serialization formats. Tests properly manage client lifecycle with local variables and explicit cleanup.Also applies to: 40-40, 82-82, 114-114, 159-159, 201-201, 242-242, 291-291, 328-328, 366-366, 389-389, 443-443, 494-494, 546-546, 595-595, 687-687, 708-708, 726-728, 744-746, 759-761, 781-783, 804-804, 817-817, 836-836, 850-850, 863-863, 876-876, 895-895, 918-918, 942-942
ttypic
force-pushed
the
AIT-96/msgpack-suppport
branch
from
0a93f11 to
e807ee8
Compare
github-actions
bot
temporarily deployed
to
staging/pull/648/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/ably/realtime/realtimeconnection_test.py (1)
404-462: Format/msgpack WebSocket tests are well‑targetedThese two tests nicely validate both the negotiated format (
transport.formatandparams['format']) and the actual frame type (bytesfor msgpack,strfor JSON) by interceptingdecode_raw_websocket_frame. The interception pattern (capturing the bound method and reassigning an instance attribute) is correct for this use case.If you want extra robustness, you could wrap the body in a
try/finallythat restorestransport.decode_raw_websocket_frameand always closesably, but it isn’t strictly necessary given each test disposes its own client.test/ably/realtime/realtimechannel_publish_test.py (1)
990-1021: Encryption round‑trip test correctly exercises the new cipher‑aware path
test_publish_with_encryptionusesCipherParamsandChannelOptions(cipher=...)to enable encryption on a realtime channel, then verifies thatmessage.dataafter subscribe matches the original"sensitive data". This is exactly the path that depends on passingcipher=self.cipherintoMessage.from_encoded_array, and running it under both JSON and msgpack (via the class parametrization) gives good coverage.Only tiny nit: the comment “Create connection with binary protocol enabled” is slightly misleading when
transport="json"(whereuse_binary_protocolisFalse), but that’s cosmetic and doesn’t affect behavior.
📜 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/realtime/connectionmanager.py(1 hunks)ably/realtime/realtime_channel.py(1 hunks)ably/transport/websockettransport.py(4 hunks)test/ably/realtime/realtimechannel_publish_test.py(31 hunks)test/ably/realtime/realtimeconnection_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/transport/websockettransport.py
🧰 Additional context used
🧬 Code graph analysis (4)
ably/realtime/connectionmanager.py (5)
ably/rest/rest.py (1)
options(112-113)ably/http/http.py (1)
options(275-276)ably/rest/channel.py (2)
options(161-162)options(169-176)ably/types/options.py (2)
use_binary_protocol(161-162)use_binary_protocol(165-166)ably/realtime/realtime_channel.py (2)
params(50-52)params(706-708)
ably/realtime/realtime_channel.py (2)
ably/types/message.py (1)
Message(24-231)ably/types/mixins.py (1)
from_encoded_array(129-130)
test/ably/realtime/realtimechannel_publish_test.py (3)
ably/realtime/realtime_channel.py (4)
ChannelOptions(26-87)get(741-766)cipher(45-47)publish(388-488)ably/util/crypto.py (1)
CipherParams(16-42)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)
test/ably/realtime/realtimeconnection_test.py (3)
ably/types/options.py (2)
use_binary_protocol(161-162)use_binary_protocol(165-166)ably/transport/websockettransport.py (1)
decode_raw_websocket_frame(203-210)ably/types/message.py (1)
data(69-70)
⏰ 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.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
- GitHub Check: check (3.14)
🔇 Additional comments (3)
ably/realtime/connectionmanager.py (1)
151-160: Binary protocol → msgpack format wiring looks correctConditionally setting
params["format"] = "msgpack"whenoptions.use_binary_protocolis enabled is consistent with the new WebSocketTransport behavior and RTN2a, and keeps the non‑binary path unchanged (noformatparam, defaulting to JSON on the transport). The override of any pre‑existingformatin the auth params also looks intentional, with the client options being the single source of truth.ably/realtime/realtime_channel.py (1)
559-563: Inbound decryption wiring viacipheris soundPassing
cipher=self.cipherintoMessage.from_encoded_arrayaligns inbound decoding with the encryption used on publish and keeps the existing delta‑decoding context intact. Non‑encrypted channels still get the previous behavior (cipherisNone), so this is a safe, focused enhancement.test/ably/realtime/realtimechannel_publish_test.py (1)
15-23: Parametrizing over JSON/MsgPack and threadinguse_binary_protocollooks solidThe class‑level
@pytest.mark.parametrize("transport", ["json", "msgpack"])plus the autousesetupfixture that derivesself.use_binary_protocolgives you a clean test matrix for both formats. Consistently passinguse_binary_protocol=self.use_binary_protocolinto allTestApp.get_ably_realtime(...)calls (and into variants withqueue_messagesorclient_id) keeps behavior explicit and makes it clear which transport each test is exercising.The tweak to the idempotent publishing test’s channel name (
f'test_idempotent_channel_{self.use_binary_protocol}') is a nice touch to avoid any potential cross‑scenario interference, even though each test already uses its own client instance.Also applies to: 27-28, 40-41, 82-83, 114-115, 159-160, 201-202, 242-243, 291-292, 328-329, 367-368, 389-390, 443-444, 495-496, 547-548, 595-596, 687-688, 709-710, 726-728, 744-746, 760-762, 782-783, 804-805, 817-818, 837-838, 851-852, 863-864, 877-878, 895-896, 919-920, 943-946
owenpearson
left a comment
owenpearson
left a comment
There was a problem hiding this comment.
looks good! one minor suggestion:
ably/transport/websockettransport.py
Outdated
| task.add_done_callback(self.on_protcol_message_handled) | ||
| # Decode based on format | ||
| msg = self.decode_raw_websocket_frame(raw) | ||
| if msg is not None: |
There was a problem hiding this comment.
I think it would be better if decode_raw_websocket_frame raised an exception and we caught it here, rather than catching the exception and then returning None
| from test.ably.utils import BaseAsyncTestCase, WaitableEvent, assert_waiter | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("transport", ["json", "msgpack"], ids=["JSON", "MsgPack"]) |
There was a problem hiding this comment.
look great :) we should do this for other tests too in a separate PR
github-actions
bot
temporarily deployed
to
staging/pull/648/features
ttypic
force-pushed
the
AIT-96/msgpack-suppport
branch
from
68a70bb to
6c9be35
Compare
github-actions
bot
temporarily deployed
to
staging/pull/648/features
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably/transport/websockettransport.py (1)
62-77: Validateformatearly and handle str/bytes mismatches in decodeThe
formatparameter is not validated in__init__, anddecode_raw_websocket_frame()has no type checking before callingjson.loads()ormsgpack.unpackb(). When an unexpectedformatvalue is encountered, or the server sends frames with mismatched types (text vs binary), decode failures are silently caught by the broadexcept Exceptionat lines 200–203, causing frames to be dropped without any warning.Add explicit format validation in
__init__and type handling indecode_raw_websocket_frame():class WebSocketTransport(EventEmitter): def __init__(self, connection_manager: ConnectionManager, host: str, params: dict): @@ self.host = host self.params = params - self.format = params.get('format', 'json') + fmt = params.get('format', 'json') + if fmt not in ('json', 'msgpack'): + log.warning(f"WebSocketTransport.__init__(): unsupported format={fmt!r}; falling back to 'json'") + fmt = 'json' + self.format = fmt super().__init__() def decode_raw_websocket_frame(self, raw: str | bytes) -> dict: if self.format == 'msgpack': ... - return json.loads(raw) + if isinstance(raw, (bytes, bytearray, memoryview)): + raw = bytes(raw).decode('utf-8') + return json.loads(raw)
📜 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 (1)
ably/transport/websockettransport.py(4 hunks)
⏰ 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.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.11)
- GitHub Check: check (3.8)
- GitHub Check: check (3.14)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
- Implement format detection (`json`/`msgpack`) in WebSocket transport. - Add `use_binary_protocol` option to enable `msgpack` encoding/decoding. - Ensure compatibility with protocol parameters and set appropriate formats during connection. - Add tests to verify `msgpack` and `json` behavior based on `use_binary_protocol` setting.
Move exception handling for `decode_raw_websocket_frame` to the calling function Move exception handling for `decode_raw_websocket_frame` to the calling function
ttypic
force-pushed
the
AIT-96/msgpack-suppport
branch
from
6c9be35 to
78d8233
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/ably/realtime/realtimeconnection_test.py (1)
404-433: Restore the monkeypatch (and consider asserting outbound framing if that’s the goal).
These tests currently validate inbound raw-frame types viadecode_raw_websocket_frame; if you intended to validate outbound framing/serialization, consider intercepting the websocket send path too. Also, restoring the monkeypatch makes test failures less leaky.async def test_connection_format_msgpack_with_binary_protocol(self): @@ - transport.decode_raw_websocket_frame = intercepted_websocket_frame + transport.decode_raw_websocket_frame = intercepted_websocket_frame + try: @@ - await ably.close() + finally: + transport.decode_raw_websocket_frame = original_decode_raw_websocket_frame + await ably.close() @@ async def test_connection_format_json_without_binary_protocol(self): @@ - transport.decode_raw_websocket_frame = intercepted_websocket_frame + transport.decode_raw_websocket_frame = intercepted_websocket_frame + try: @@ - await ably.close() + finally: + transport.decode_raw_websocket_frame = original_decode_raw_websocket_frame + await ably.close()Also applies to: 434-462
test/ably/realtime/realtimechannel_publish_test.py (2)
25-28: Good: running the same publish/ACK semantics across json/msgpack.
Optional: consider a small helper/fixture to create+closeably(ortry/finally) so failures don’t leak clients mid-test.Also applies to: 40-41, 82-83, 114-115, 159-160, 201-203, 242-244, 291-293, 328-330, 366-368, 389-391, 443-445, 494-496, 546-548, 595-597, 687-689, 708-710, 726-729, 744-748, 759-763, 781-785, 804-806, 817-819, 836-838, 850-852, 863-865, 876-878, 895-897, 918-920, 942-947
990-1021: Encryption round-trip test is valuable; fix the misleading comment and consider a unique channel name.
Right now the comment says “binary protocol enabled” but this test runs for both parameterizations.- # Create connection with binary protocol enabled + # Create connection (transport-dependent: json vs msgpack) @@ - channel = ably.channels.get('encrypted_channel', channel_options) + channel = ably.channels.get(f'encrypted_channel_{self.use_binary_protocol}', channel_options)
📜 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/realtime/connectionmanager.py(1 hunks)ably/realtime/realtime_channel.py(1 hunks)ably/transport/websockettransport.py(4 hunks)test/ably/realtime/realtimechannel_publish_test.py(31 hunks)test/ably/realtime/realtimeconnection_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ably/realtime/connectionmanager.py
- ably/transport/websockettransport.py
🧰 Additional context used
🧬 Code graph analysis (3)
ably/realtime/realtime_channel.py (3)
ably/types/message.py (1)
Message(24-231)ably/types/mixins.py (1)
from_encoded_array(129-130)ably/rest/channel.py (2)
get(184-195)cipher(157-158)
test/ably/realtime/realtimechannel_publish_test.py (3)
ably/realtime/realtime_channel.py (3)
ChannelOptions(26-87)get(741-766)cipher(45-47)ably/util/crypto.py (1)
CipherParams(16-42)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_realtime(80-83)
test/ably/realtime/realtimeconnection_test.py (2)
ably/types/options.py (2)
use_binary_protocol(161-162)use_binary_protocol(165-166)ably/transport/websockettransport.py (1)
decode_raw_websocket_frame(207-210)
⏰ 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.14)
- GitHub Check: check (3.8)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.11)
| messages = Message.from_encoded_array(proto_msg.get('messages'), | ||
| cipher=self.cipher, context=self.__decoding_context) | ||
| self.__decoding_context.last_message_id = messages[-1].id | ||
| self.__channel_serial = channel_serial |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n ably/realtime/realtime_channel.py | sed -n '555,575p'Repository: ably/ably-python
Length of output: 1352
🏁 Script executed:
rg -n --type=py '\.from_encoded_array\s*\(' --max-count=20Repository: ably/ably-python
Length of output: 487
🏁 Script executed:
# Find the Message.from_encoded_array definition to understand its signature
ast-grep --pattern 'def from_encoded_array($$$)'Repository: ably/ably-python
Length of output: 262
🏁 Script executed:
# Search for tests or comments about MESSAGE action with empty/missing messages
rg -n --type=py 'ProtocolMessageAction\.MESSAGE|proto_msg.*messages' -A 3 -B 3 | head -50Repository: ably/ably-python
Length of output: 3967
🏁 Script executed:
# Check if there are any proto message construction examples in tests
rg -n --type=py 'messages.*\[\]|messages.*None' -B 2 -A 2 | head -40Repository: ably/ably-python
Length of output: 2758
🏁 Script executed:
# Look at exception handling patterns in similar code sections
cat -n ably/realtime/realtime_channel.py | sed -n '540,580p'Repository: ably/ably-python
Length of output: 2486
🏁 Script executed:
# Check if there's any validation or protocol specification for MESSAGE action messages field
rg -n --type=py 'MESSAGE|messages' ably/realtime/realtime_channel.py | grep -i 'protocol\|spec\|doc' | head -20Repository: ably/ably-python
Length of output: 283
🏁 Script executed:
# Look at how proto_msg is constructed/parsed to understand if messages key is always present
rg -n --type=py 'def _on_message|proto_msg.*=' ably/realtime/realtime_channel.py | head -10Repository: ably/ably-python
Length of output: 115
🏁 Script executed:
# Check if there are any edge cases in tests that exercise empty message scenarios
rg -n --type=py 'messages.*=.*\[\]' test/ -B 2 -A 2 | head -30Repository: ably/ably-python
Length of output: 2124
Guard against missing/empty messages when updating last_message_id.
Line 561–563 can crash with TypeError (if proto_msg.get('messages') is None) or IndexError (if the list is empty). The exception handler at line 565 only catches AblyException, so these errors will propagate uncaught.
- messages = Message.from_encoded_array(proto_msg.get('messages'),
- cipher=self.cipher, context=self.__decoding_context)
- self.__decoding_context.last_message_id = messages[-1].id
+ raw_messages = proto_msg.get('messages') or []
+ messages = Message.from_encoded_array(
+ raw_messages,
+ cipher=self.cipher,
+ context=self.__decoding_context,
+ )
+ if messages:
+ self.__decoding_context.last_message_id = messages[-1].id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| messages = Message.from_encoded_array(proto_msg.get('messages'), | |
| cipher=self.cipher, context=self.__decoding_context) | |
| self.__decoding_context.last_message_id = messages[-1].id | |
| self.__channel_serial = channel_serial | |
| raw_messages = proto_msg.get('messages') or [] | |
| messages = Message.from_encoded_array( | |
| raw_messages, | |
| cipher=self.cipher, | |
| context=self.__decoding_context, | |
| ) | |
| if messages: | |
| self.__decoding_context.last_message_id = messages[-1].id | |
| self.__channel_serial = channel_serial |
| from ably.realtime.realtime_channel import ChannelOptions, ChannelState | ||
| from ably.transport.websockettransport import ProtocolMessageAction | ||
| from ably.types.message import Message | ||
| from ably.util.crypto import CipherParams | ||
| from ably.util.exceptions import AblyException, IncompatibleClientIdException | ||
| from test.ably.testapp import TestApp | ||
| from test.ably.utils import BaseAsyncTestCase, WaitableEvent, assert_waiter | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("transport", ["json", "msgpack"], ids=["JSON", "MsgPack"]) | ||
| class TestRealtimeChannelPublish(BaseAsyncTestCase): | ||
| """Tests for RTN7 spec - Message acknowledgment""" | ||
|
|
||
| @pytest.fixture(autouse=True) | ||
| async def setup(self): | ||
| async def setup(self, transport): | ||
| self.test_vars = await TestApp.get_test_vars() | ||
| self.use_binary_protocol = True if transport == 'msgpack' else False | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check whether msgpack is declared in repo dependency files
rg -n --hidden -S '\bmsgpack\b' pyproject.toml poetry.lock requirements*.txt setup.cfg setup.py Pipfile 2>/dev/null | head -20Repository: ably/ably-python
Length of output: 104
🏁 Script executed:
# Check if ably.types.channelstate exists and what it exports
find . -name "channelstate.py" -type f | head -5Repository: ably/ably-python
Length of output: 88
🏁 Script executed:
# Search for current ChannelState imports to see the pattern
rg -n "from.*import.*ChannelState|import.*ChannelState" --type py | head -20Repository: ably/ably-python
Length of output: 616
🏁 Script executed:
# Check the structure of ably.realtime.realtime_channel to see what's exported
rg -n "^class ChannelState|^ChannelState\s*=" . --type py | head -10Repository: ably/ably-python
Length of output: 179
🏁 Script executed:
# Check if ably.realtime.realtime_channel exports ChannelState explicitly
rg -n "__all__|^from.*ChannelState|^import.*ChannelState" ably/realtime/realtime_channel.py | head -20Repository: ably/ably-python
Length of output: 131
Prefer ChannelState import from ably.types.channelstate.
-from ably.realtime.realtime_channel import ChannelOptions, ChannelState
+from ably.realtime.realtime_channel import ChannelOptions
+from ably.types.channelstate import ChannelStateThe ChannelState class is defined in ably.types.channelstate and should be imported from there rather than from ably.realtime.realtime_channel, which only re-exports it. This aligns with the pattern already used in ably/realtime/realtime_channel.py and avoids importing from an indirect source. Additionally, msgpack is properly declared as a dependency in pyproject.toml, so the parametrized transport tests will have access to it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from ably.realtime.realtime_channel import ChannelOptions, ChannelState | |
| from ably.transport.websockettransport import ProtocolMessageAction | |
| from ably.types.message import Message | |
| from ably.util.crypto import CipherParams | |
| from ably.util.exceptions import AblyException, IncompatibleClientIdException | |
| from test.ably.testapp import TestApp | |
| from test.ably.utils import BaseAsyncTestCase, WaitableEvent, assert_waiter | |
| @pytest.mark.parametrize("transport", ["json", "msgpack"], ids=["JSON", "MsgPack"]) | |
| class TestRealtimeChannelPublish(BaseAsyncTestCase): | |
| """Tests for RTN7 spec - Message acknowledgment""" | |
| @pytest.fixture(autouse=True) | |
| async def setup(self): | |
| async def setup(self, transport): | |
| self.test_vars = await TestApp.get_test_vars() | |
| self.use_binary_protocol = True if transport == 'msgpack' else False | |
| from ably.realtime.realtime_channel import ChannelOptions | |
| from ably.types.channelstate import ChannelState | |
| from ably.transport.websockettransport import ProtocolMessageAction | |
| from ably.types.message import Message | |
| from ably.util.crypto import CipherParams | |
| from ably.util.exceptions import AblyException, IncompatibleClientIdException | |
| from test.ably.testapp import TestApp | |
| from test.ably.utils import BaseAsyncTestCase, WaitableEvent, assert_waiter | |
| @pytest.mark.parametrize("transport", ["json", "msgpack"], ids=["JSON", "MsgPack"]) | |
| class TestRealtimeChannelPublish(BaseAsyncTestCase): | |
| """Tests for RTN7 spec - Message acknowledgment""" | |
| @pytest.fixture(autouse=True) | |
| async def setup(self, transport): | |
| self.test_vars = await TestApp.get_test_vars() | |
| self.use_binary_protocol = True if transport == 'msgpack' else False |
🤖 Prompt for AI Agents
In test/ably/realtime/realtimechannel_publish_test.py around lines 6 to 23, the
test imports ChannelState from ably.realtime.realtime_channel but it should be
imported directly from its defining module; update the import to use
ably.types.channelstate for ChannelState (keep other imports the same) so the
file imports ChannelState from ably.types.channelstate instead of
ably.realtime.realtime_channel.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.