[AIT-196] feat: Rest mutable message support plus protocol v5 changes #659
Conversation
- Introduced `MessageVersion` class to handle metadata related to message versions, such as serial, timestamp, client_id, description, and metadata. - Updated `Message` class to include a `version` property and support serialization/deserialization of message versions.
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (4)test/ably/rest/restchannelpublish_test.py (1)
test/ably/rest/resthttp_test.py (3)
ably/types/message.py (2)
test/ably/rest/restchannelmutablemessages_test.py (6)
🪛 Gitleaks (8.30.0)test/ably/rest/restchannelmutablemessages_test.py[high] 158-158: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) ⏰ 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)
🔇 Additional comments (13)
✏️ Tip: You can disable this entire section by setting 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/659/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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably/rest/channel.py (1)
102-124: Critical breaking change:publish_messagesreturn type incompatible with existing tests and callers.The method now returns
PublishResultinstead of the rawResponseobject. This breaks existing tests that expectresponse.status_code == 201(will raiseAttributeErrorsincePublishResulthas only aserialsproperty) and any code relying onResponseproperties likestatus_code,headers, etc.Either update the implementation to return the wrapped Response (preserving status_code access), or fix all tests and code paths expecting the old API. Additionally, avoid silently swallowing parse exceptions; at minimum log the failure.
🤖 Fix all issues with AI agents
In `@ably/rest/channel.py`:
- Around line 165-223: In _send_update, avoid accessing
operation.client_id/description/metadata when operation is None: compute a local
variable (e.g., version) that is MessageVersion(client_id=..., description=...,
metadata=...) only if operation is not None, otherwise set version = None, and
pass that version variable into the Message constructor instead of constructing
MessageVersion unconditionally; this preserves existing behavior when an
operation is provided and prevents AttributeError when operation is omitted.
In `@ably/types/message.py`:
- Around line 368-372: The single-message handler omits a context when calling
Message.from_encoded causing an AttributeError for delta-encoded messages;
update make_single_message_response_handler so
encrypted_message_response_handler calls Message.from_encoded(message,
cipher=cipher, context=None) (or change make_single_message_response_handler to
accept an optional context parameter and forward it) to explicitly supply a
context and match the behavior of make_message_response_handler.
In `@ably/types/operations.py`:
- Around line 4-12: The Operation class constructor accepts and stores client_id
but as_dict() and from_dict() omit it; update the Operation docstring to
document client_id and modify as_dict() to include "client_id" when
self.__client_id is not None, and update from_dict(cls, data) to read
data.get("client_id") and pass it into the constructor (or set the private
attribute) so serialization/deserialization round-trips client_id; if client_id
is intentionally server-only instead, instead add a clear docstring note that
client_id is read-only and deliberately omitted from as_dict() and from_dict().
In `@test/ably/rest/restchannelmutablemessages_test.py`:
- Around line 221-253: The test test_complete_workflow_publish_update_delete is
brittle because it asserts exactly 3 versions; change the check to wait for at
least 3 versions (use wait_until_get_all_message_version to poll until
len(versions) >= 3) and then assert the last three entries match the published
serial and the update/delete result serials (e.g. verify
versions[-3].version.serial == serial, versions[-2].version.serial ==
update_result.version_serial, versions[-1].version.serial ==
delete_result.version_serial) so the test tolerates extra older/newer entries
while still validating the workflow.
- Around line 295-304: The waiter in wait_until_get_all_message_version uses a
strict equality check (return len(versions) == count) which flakes if the API
returns more than count; change the condition to tolerate "at least N" items by
using len(versions) >= count when checking the result of await
channel.get_message_versions(serial) in the check_message_action closure so the
waiter succeeds once there are count or more versions.
- Around line 25-57: The Message created in test_update_message_success omits
the name and the test doesn't assert that update_result.version_serial is
present; to fix, supply the channel name when constructing the Message (e.g.,
set message.name to the channel's name) or explicitly assert that name is None
if omission is intentional, then add an assertion like assert
update_result.version_serial is not None (or truthy) before relying on it, and
optionally assert updated_message.name matches the message.name to ensure the
server returned the expected name.
- Around line 161-167: The test test_update_message_with_encryption currently
uses a hard-coded secret_key string in CipherParams which triggers gitleaks;
replace the literal with a generated synthetic key inside the test (e.g., derive
a 16-byte deterministic key via a utility or from a fixed seed using hashlib or
os.urandom seeded) and pass that variable to CipherParams(secret_key=...).
Update references in this test to use the new variable instead of the literal so
the key is not embedded as a string constant in the file.
In `@test/unit/mutable_message_test.py`:
- Around line 1-2: Imports are unsorted causing linter failure; reorder them so
third-party modules are grouped and alphabetized: place "from ably import
MessageAction, MessageOperation, MessageVersion, UpdateDeleteResult" before
"from ably.types.message import Message" and alphabetize the names in the
from-abyl import list (MessageAction, MessageOperation, MessageVersion,
UpdateDeleteResult).
- Around line 46-55: The test fails because Message.__init__ stores the provided
action value as-is and does not convert integer values to the MessageAction
enum; update Message.__init__ in ably/types/message.py so that if the incoming
action is an int (or already a MessageAction), it gets normalized to
MessageAction by doing a conversion like MessageAction(action) before assigning
to self.action (keep Message.from_encoded behavior consistent); ensure you
reference the Message.__init__ constructor and MessageAction enum and adjust any
type hints or tests if needed.
- Around line 108-123: The test does not verify that MessageOperation.client_id
is intentionally excluded from serialization; update
test_message_operation_serialization to construct a MessageOperation with
client_id (e.g., 'client_123'), call MessageOperation.as_dict() and assert that
'client_id' is not present in the returned dict while also asserting
operation.client_id == 'client_123' to ensure the attribute remains accessible;
keep the existing checks for description/metadata and ensure
MessageOperation.from_dict still round-trips correctly for the serialized
fields.
🧹 Nitpick comments (6)
test/assets/testAppSpec.json (1)
30-33: Inconsistent indentation style.The new namespace entry uses spaces while the rest of the file uses tabs for indentation. Consider using tabs to match the existing style.
🔧 Proposed fix
{ "id": "canpublish", "pushEnabled": true }, - { - "id": "mutable", - "mutableMessages": true - } + { + "id": "mutable", + "mutableMessages": true + } ],ably/types/message.py (1)
308-316: Redundant else clause.Line 316 (
action = None) is redundant sinceactionis alreadyNonewhen the condition on line 309 is false.♻️ Suggested simplification
# Handle action - can be MessageAction enum, int, or None if action is not None: try: action = MessageAction(action) except ValueError: # If it's not a valid action value, store as None action = None - else: - action = Nonetest/ably/rest/restchannelmutablemessages_test.py (1)
70-101: Delete test: avoid sendingdata/nameunless required by the API.
Keeping delete minimal (Message(serial=serial)) reduces the chance of server-side validation changes breaking the test for unrelated reasons.ably/rest/channel.py (3)
165-223: Addtimeoutsupport to update/delete/append (parity with publish/get).
PATCH currently has notimeoutpath, which makes these calls harder to control in production code.
224-262: Public API: consider allowingserial: strinput (ergonomics), not onlyMessage.
You already acceptserial_or_messagefor getters; symmetry for mutations would reduce boilerplate and avoid users constructing partially-filledMessageobjects.
263-343: Refactor: deduplicate serial extraction & validation betweenget_messageandget_message_versions.
This logic is repeated and will drift (error text / error code / accepted types). A tiny private helper would keep it consistent.
📜 Review details
Configuration used: Organization 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 (7)
ably/__init__.pyably/rest/channel.pyably/types/message.pyably/types/operations.pytest/ably/rest/restchannelmutablemessages_test.pytest/assets/testAppSpec.jsontest/unit/mutable_message_test.py
🧰 Additional context used
🧬 Code graph analysis (3)
ably/__init__.py (2)
ably/types/message.py (1)
MessageVersion(25-97)ably/types/operations.py (3)
MessageOperation(1-43)PublishResult(46-65)UpdateDeleteResult(68-87)
ably/types/operations.py (1)
ably/types/message.py (7)
client_id(63-64)client_id(165-166)description(67-68)metadata(71-72)as_dict(74-84)as_dict(236-285)from_dict(87-97)
test/ably/rest/restchannelmutablemessages_test.py (4)
ably/util/exceptions.py (1)
AblyException(9-90)ably/types/message.py (10)
MessageAction(100-107)Message(110-359)serial(55-56)serial(197-198)data(161-162)description(67-68)metadata(71-72)version(193-194)name(157-158)action(201-202)ably/types/operations.py (3)
MessageOperation(1-43)description(19-20)metadata(23-24)test/ably/utils.py (1)
assert_waiter(179-191)
🪛 GitHub Actions: Linting check
test/unit/mutable_message_test.py
[error] 1-1: ruff check failed: Import block is un-sorted or un-formatted. 1 error (organize imports). 1 fixable with the --fix option. Command: uv run ruff check
🪛 Gitleaks (8.30.0)
test/ably/rest/restchannelmutablemessages_test.py
[high] 165-165: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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.14)
- GitHub Check: check (3.12)
- GitHub Check: check (3.9)
- GitHub Check: check (3.11)
- GitHub Check: check (3.8)
- GitHub Check: check (3.10)
🔇 Additional comments (9)
ably/types/message.py (3)
25-97: LGTM!The
MessageVersionclass is well-structured with properto_textconversions for string fields and cleanas_dict/from_dictserialization helpers.
100-107: LGTM!The
MessageActionenum correctly defines all message action types with appropriate integer values.
318-321: Verify intent of fallbackMessageVersionconstruction.When
versionisNonein the response, a newMessageVersionis constructed using onlyserialandtimestamp. This means newly created messages will always have a non-Noneversionproperty, even if the server didn't provide version data. Confirm this is the intended behavior for backward compatibility.ably/__init__.py (2)
10-11: LGTM!New types are correctly exported from the public API surface.
20-20: API version bump from 3 to 5.This is a significant protocol change. Ensure the changelog and migration notes document any breaking changes or new capabilities that require API version 5.
ably/types/operations.py (2)
46-65: LGTM!
PublishResultis straightforward with proper handling of theserialslist.
68-87: LGTM!
UpdateDeleteResultcorrectly mapsversionSerialfrom the server response toversion_serial.test/ably/rest/restchannelmutablemessages_test.py (2)
15-24: Add teardown/cleanup for the REST client (if BaseAsyncTestCase doesn’t already).
The autouse fixture createsself.ablybut doesn’t explicitly close it; if the base class doesn’t handle this, tests may leak connections across the parametrized runs.
281-304: Test suite Python compatibility: avoidMessage | Noneunless repo is 3.10+.
If this project still runs tests on Python 3.9 (or earlier),Message | Nonewill SyntaxError. UseOptional[Message]instead.Proposed change
-from typing import List +from typing import List, Optional ... - message: Message | None = None + message: Optional[Message] = None
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ttypic
force-pushed
the
AIT-196/rest-edits-deletes
branch
from
d385246 to
737d738
Compare
github-actions
bot
temporarily deployed
to
staging/pull/659/features
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/ably/rest/restchannelmutablemessages_test.py`:
- Around line 18-23: The fixture setup creates an Ably client via
TestApp.get_ably_rest but does not tear it down; modify the async pytest fixture
setup to yield after assigning self.test_vars and self.ably, and after the yield
ensure the Ably client is closed (call the client's close/async close method
used elsewhere) to release resources—mirror the pattern used in
TestRestChannelPublish.setup in restchannelpublish_test.py when locating the
change.
- Around line 281-293: The type annotation uses Python 3.10 union syntax; change
the declaration in wait_until_message_with_action_appears from "message: Message
| None" to "message: Optional[Message]" and add "from typing import Optional" at
the top of the file; ensure references in nested check_message_action and the
return type remain valid and keep using channel.get_message and assert_waiter
as-is for compatibility with Python 3.7+.
♻️ Duplicate comments (8)
test/unit/mutable_message_test.py (2)
108-123: Test doesn't verifyclient_idexclusion from serialization.Per the past review,
MessageOperation.as_dict()intentionally excludesclient_id. Add a test case to verify this behavior:def test_message_operation_client_id_exclusion(): """Test that client_id is excluded from serialization but accessible as property.""" operation = MessageOperation( client_id='client_123', description='Test operation', metadata={'key': 'value'} ) op_dict = operation.as_dict() assert 'clientId' not in op_dict # Intentionally excluded assert operation.client_id == 'client_123' # Still accessible
46-55: Test relies on IntEnum equality semantics but doesn't verify type conversion.This test may pass due to
IntEnumequality (where1 == MessageAction.MESSAGE_UPDATE), butMessage.__init__stores the raw integer without converting toMessageAction. Consider adding a type assertion to verify the expected behavior:assert isinstance(message.action, MessageAction) # Will fail currentlyIf integer storage is acceptable, update the docstring to clarify this. Otherwise, add int-to-enum conversion in
Message.__init__.ably/types/message.py (1)
367-371: Context parameter not explicitly provided tofrom_encoded.As flagged in the past review,
Message.from_encodedis called withoutcontext. If a delta-encoded message is received, line 302 (context.last_message_id) will raiseAttributeError. While REST single-message responses likely don't contain delta messages, explicitly passingcontext=Nonewould clarify intent.Can Ably REST API single message responses contain delta-encoded messages?ably/rest/channel.py (1)
180-192: Critical:_send_updatecrashes whenoperationisNone.Lines 188-190 access
operation.client_id,operation.description, andoperation.metadata, butoperationdefaults toNone. Sinceupdate_message,delete_message, andappend_messageallowoperation=None, this will raiseAttributeError.🐛 Proposed fix
async def _send_update( self, message: Message, action: MessageAction, operation: MessageOperation = None, params: dict = None, ): """Internal method to send update/delete/append operations.""" if not message.serial: raise AblyException( "Message serial is required for update/delete/append operations", 400, 40000 ) + # Handle None operation + version = None + if operation: + version = MessageVersion( + client_id=operation.client_id, + description=operation.description, + metadata=operation.metadata, + ) + # Create a new message with the operation fields update_message = Message( name=message.name, data=message.data, client_id=message.client_id, serial=message.serial, action=action, - version=MessageVersion( - client_id=operation.client_id, - description=operation.description, - metadata=operation.metadata, - ), + version=version, )test/ably/rest/restchannelmutablemessages_test.py (4)
46-56: Add assertion thatupdate_result.version_serialis not None before depending on it.The test asserts on
update_result.version_serialat line 53 without first confirming it's set. A missing orNonevalue would cause a confusing failure rather than a clear diagnostic.
161-182: Replace the hardcoded secret key to avoid triggering secret scanners.The literal
'keyfordecrypt_16'is flagged by Gitleaks. Use a generated key instead.Proposed fix
+ # Generate a deterministic test key to avoid secret scanner triggers + TEST_KEY = b'\x00' * 16 # 16 zero bytes for AES-128 + async def test_update_message_with_encryption(self): """Test updating an encrypted message""" # Create channel with encryption channel_name = self.get_channel_name('mutable:update_encrypted') - cipher_params = CipherParams(secret_key='keyfordecrypt_16', algorithm='aes') + cipher_params = CipherParams(secret_key=self.TEST_KEY, algorithm='aes') channel = self.ably.channels.get(channel_name, cipher=cipher_params)
247-252: Exact version count assertion may cause flaky tests.Using
len(versions) == 3is brittle if the API returns additional entries. Consider>= 3and validate the specific entries you care about.
295-304: Waiter condition should use>=to avoid flaky timeouts.If the API returns more than
countversions, the exact equality check will never succeed.Proposed fix
async def wait_until_get_all_message_version(self, channel, serial, count): versions: List[Message] = [] async def check_message_versions(): nonlocal versions versions = (await channel.get_message_versions(serial)).items - return len(versions) == count + return len(versions) >= count await assert_waiter(check_message_versions) return versions
🧹 Nitpick comments (4)
test/ably/rest/restrequest_test.py (1)
196-198: Consider asserting on the publish result for additional coverage.The test correctly validates the 503 fallback behavior via
default_route.called. Sincepublishnow returns aPublishResult, you could optionally verify the result contains expected data from the fallback response to strengthen the test.♻️ Optional enhancement
- await ably.channels['test'].publish('test', 'data') + result = await ably.channels['test'].publish('test', 'data') assert default_route.called + # Optionally verify result from fallback + assert result is not Noneably/types/message.py (1)
122-139: Consider normalizingactiontoMessageActionin constructor.The
actionparameter is stored as-is without conversion toMessageAction. Whilefrom_encoded(line 308-315) normalizes the action, direct construction with an integer won't produce aMessageActioninstance. This inconsistency may cause type-checking issues.♻️ Optional: Normalize action in constructor
self.__serial = serial - self.__action = action + if action is not None and not isinstance(action, MessageAction): + try: + self.__action = MessageAction(action) + except ValueError: + self.__action = None + else: + self.__action = action self.__version = versionably/rest/channel.py (2)
116-123: Consider narrowing exception handling or logging failures.The bare
except Exceptionsilently swallows all errors during response parsing. This could hide legitimate issues. Consider logging the exception or catching specific expected exceptions.♻️ Optional: Log parsing failures
try: result_data = response.to_native() if result_data and isinstance(result_data, dict): return PublishResult.from_dict(result_data) return PublishResult() - except Exception: + except Exception as e: # If response parsing fails, return empty PublishResult for backwards compatibility + log.debug("Failed to parse publish response: %s", e) return PublishResult()
216-222: Same exception handling concern aspublish_messages.Consider logging parsing failures here as well for consistency and debugging.
📜 Review details
Configuration used: Organization 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 (10)
ably/__init__.pyably/rest/channel.pyably/types/message.pyably/types/operations.pytest/ably/rest/restchannelmutablemessages_test.pytest/ably/rest/restchannelpublish_test.pytest/ably/rest/resthttp_test.pytest/ably/rest/restrequest_test.pytest/assets/testAppSpec.jsontest/unit/mutable_message_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- test/assets/testAppSpec.json
- ably/types/operations.py
- ably/init.py
🧰 Additional context used
🧬 Code graph analysis (5)
test/unit/mutable_message_test.py (2)
ably/types/message.py (18)
MessageAction(100-107)MessageVersion(25-97)Message(110-358)version(193-194)serial(55-56)serial(197-198)timestamp(59-60)timestamp(185-186)client_id(63-64)client_id(165-166)as_dict(74-84)as_dict(236-285)description(67-68)metadata(71-72)name(157-158)data(161-162)action(201-202)from_dict(87-97)ably/types/operations.py (10)
MessageOperation(1-45)UpdateDeleteResult(70-89)client_id(15-16)as_dict(26-34)description(19-20)metadata(23-24)from_dict(37-45)from_dict(63-67)from_dict(85-89)version_serial(81-82)
test/ably/rest/restrequest_test.py (1)
ably/rest/channel.py (2)
ably(345-346)publish(129-155)
ably/rest/channel.py (8)
ably/types/message.py (15)
Message(110-358)from_dict(87-97)action(201-202)serial(55-56)serial(197-198)name(157-158)data(161-162)client_id(63-64)client_id(165-166)version(193-194)description(67-68)metadata(71-72)encrypt(204-220)as_dict(74-84)as_dict(236-285)ably/types/operations.py (10)
MessageOperation(1-45)PublishResult(48-67)UpdateDeleteResult(70-89)from_dict(37-45)from_dict(63-67)from_dict(85-89)client_id(15-16)description(19-20)metadata(23-24)as_dict(26-34)ably/types/presence.py (6)
Presence(240-291)action(62-63)data(74-75)client_id(66-67)encrypt(125-145)get(253-263)ably/util/exceptions.py (4)
AblyException(9-90)IncompatibleClientIdException(109-110)catch_all(93-102)from_dict(89-90)ably/http/http.py (5)
response(110-111)to_native(95-107)version(83-84)patch(251-254)get(246-249)ably/rest/rest.py (2)
http(108-109)client_id(95-96)ably/types/channelsubscription.py (3)
from_dict(47-49)client_id(27-28)as_dict(34-44)ably/types/tokenrequest.py (1)
client_id(82-83)
test/ably/rest/restchannelmutablemessages_test.py (1)
ably/rest/channel.py (10)
ably(345-346)publish(129-155)update_message(224-235)name(349-350)delete_message(237-248)append_message(250-261)get(384-395)cipher(357-358)get_message(263-299)get_message_versions(301-342)
test/ably/rest/resthttp_test.py (3)
ably/rest/rest.py (1)
request(119-139)ably/http/http.py (1)
headers(71-72)ably/http/paginatedresult.py (1)
headers(133-134)
🪛 Gitleaks (8.30.0)
test/ably/rest/restchannelmutablemessages_test.py
[high] 165-165: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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.14)
- GitHub Check: check (3.10)
- GitHub Check: check (3.13)
- GitHub Check: check (3.11)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (21)
test/ably/rest/resthttp_test.py (1)
183-183: LGTM!The test expectation correctly updated to match the API version bump to protocol v5.
test/unit/mutable_message_test.py (6)
1-2: Imports appear correctly ordered.The imports are alphabetically sorted within the
from ably importstatement. If the linter still fails, verify the linter configuration expectsfrom ably.types.message import Messageto come before or after thefrom ably importline based on your project's import grouping rules.
5-17: LGTM!Test correctly verifies that
Nonevalues are filtered out from the serialized dictionary output.
18-27: LGTM!Test correctly verifies
Nonevalue filtering forMessageOperation.as_dict().
29-44: LGTM!Test correctly verifies that
Messagestores and serializesactionandserialfields when initialized with the enum value.
57-70: LGTM!Tests correctly verify
UpdateDeleteResult.from_dict()handles both valid dictionaries andNoneinput gracefully.
73-80: LGTM!Test correctly validates all
MessageActionenum values match the expected integer mappings.ably/types/message.py (4)
25-97: LGTM!
MessageVersionclass is well-implemented with proper serialization/deserialization,Nonefiltering, and consistent property accessors.
100-107: LGTM!
MessageActionenum correctly usesIntEnumfor wire-protocol compatibility with appropriate action type values.
274-276: LGTM!Serialization correctly handles the new fields:
versionis serialized viaas_dict(),serialis included directly, andactionis converted tointfor wire compatibility.
308-320: LGTM!The action normalization with
try/exceptgracefully handles invalid values, and the version construction has appropriate fallback logic when the version dict is not present.ably/rest/channel.py (4)
13-27: LGTM!Import statements correctly include the new types and utilities needed for mutable message operations.
224-261: LGTM on method signatures and documentation.The public API methods are well-documented with clear parameter descriptions. Note: the
operation=Noneissue will be resolved by fixing_send_update.
263-299: LGTM!Method correctly handles both string serial and
Messageobject inputs, validates the serial presence, and provides a helpful error message directing users to enable the feature in dashboard settings.
301-342: LGTM!Method correctly implements paginated version history retrieval with consistent serial validation and proper use of
PaginatedResult.test/ably/rest/restchannelpublish_test.py (1)
402-424: LGTM! Test correctly updated to reflect new publish() return type.The test no longer captures or asserts on HTTP response status codes since
publish()now returns aPublishResultobject. The correctness validation is appropriately delegated to thecheck_dataandcheck_historypolling functions that verify the message was persisted correctly.test/ably/rest/restchannelmutablemessages_test.py (5)
58-68: LGTM! Error handling test correctly verifies the expected status code and error message when serial is missing.
70-100: LGTM! The delete test correctly validates the MESSAGE_DELETE action, version serial, and operation metadata.
114-147: LGTM! The append test correctly validates that the resulting message action isMESSAGE_UPDATEwith concatenated data, which aligns with the expected behavior.
184-219: LGTM! Both tests appropriately validate the params passthrough and batch publish serial results.
254-279: LGTM! The test validates string data append with concatenation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ttypic
force-pushed
the
AIT-196/rest-edits-deletes
branch
from
737d738 to
a63e968
Compare
github-actions
bot
temporarily deployed
to
staging/pull/659/features
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/ably/rest/restchannelmutablemessages_test.py`:
- Around line 41-51: The test calls channel.update_message(message) but later
asserts the version.description and version.metadata, so create and pass a
MessageOperation with the expected values when updating; e.g. construct a
MessageOperation with description 'Fixed typo' and metadata {'reason':
'correction'} and pass it to update_message (the same call that leads to
MessageAction.MESSAGE_UPDATE and wait_until_message_with_action_appears) so
update_result.version_serial/description/metadata match the assertions.
♻️ Duplicate comments (3)
test/ably/rest/restchannelmutablemessages_test.py (3)
18-23: Missing teardown: addyieldand close the Ably client.The fixture creates an Ably client but doesn't clean it up, which may cause resource leaks during test runs.
156-160: Replace the hard-codedsecret_keyliteral.Hard-coded key-like strings trigger secret scanners and can block merges. Use a generated synthetic key instead.
275-287: UseOptional[Message]instead ofMessage | Nonefor Python 3.7+ compatibility.The project supports Python 3.7+ but the
|union syntax requires Python 3.10+. AddOptionalto the imports and change the type annotation.
🧹 Nitpick comments (6)
test/ably/rest/restrequest_test.py (1)
196-197: Consider adding an assertion on the publish result.The test now only verifies that
default_route.calledis True, but no longer asserts that the fallback response was actually processed correctly. Sincepublishnow returns aPublishResult, you could assert on the returned value to strengthen coverage.💡 Optional enhancement
- await ably.channels['test'].publish('test', 'data') + result = await ably.channels['test'].publish('test', 'data') assert default_route.called + # Optionally verify the fallback response was processed + assert result is not Noneably/types/operations.py (1)
4-12: Docstring incomplete:client_idparameter not documented.The constructor accepts
client_id(line 4) and it's now properly serialized inas_dict/from_dict, but the docstring (lines 6-8) doesn't document this parameter. Consider adding documentation to clarify its purpose.📝 Add client_id to docstring
def __init__(self, client_id=None, description=None, metadata=None): """ Args: + client_id: The client ID that performed the operation (typically read-only, populated by server). description: Optional description of the operation. metadata: Optional dict of metadata key-value pairs associated with the operation. """ably/types/message.py (1)
308-315: Minor: redundantaction = Noneassignment.The
elsebranch (lines 314-315) assignsaction = None, butactionis alreadyNonewhen this branch is reached (per the condition on line 308).♻️ Simplify
if action is not None: try: action = MessageAction(action) except ValueError: # If it's not a valid action value, store as None action = None - else: - action = Noneably/rest/channel.py (3)
116-123: Bareexcept Exceptionmay mask real errors.This broad exception handler silently returns an empty
PublishResultfor any parsing failure, which could mask legitimate errors (e.g., malformed response, network issues during parsing). Consider catching more specific exceptions or at least logging the error.♻️ Add logging for debugging
try: result_data = response.to_native() if result_data and isinstance(result_data, dict): return PublishResult.from_dict(result_data) return PublishResult() except Exception: # If response parsing fails, return empty PublishResult for backwards compatibility + log.debug("Failed to parse publish response, returning empty PublishResult") return PublishResult()
221-227: Same concern: broadexcept Exceptionfor response parsing.Similar to
publish_messages, this silently swallows all exceptions. Consider adding debug logging for consistency and easier debugging.♻️ Add logging
try: result_data = response.to_native() if result_data and isinstance(result_data, dict): return UpdateDeleteResult.from_dict(result_data) return UpdateDeleteResult() except Exception: + log.debug("Failed to parse update/delete response, returning empty UpdateDeleteResult") return UpdateDeleteResult()
268-294: Consider extracting serial extraction to a helper method.The serial extraction logic (lines 281-286 and 320-325) is duplicated between
get_messageandget_message_versions. A small helper could reduce duplication.♻️ Extract helper method
def _extract_serial(self, serial_or_message): """Extract serial from string or Message object.""" if isinstance(serial_or_message, str): return serial_or_message elif isinstance(serial_or_message, Message): return serial_or_message.serial return NoneThen use in both methods:
- if isinstance(serial_or_message, str): - serial = serial_or_message - elif isinstance(serial_or_message, Message): - serial = serial_or_message.serial - else: - serial = None + serial = self._extract_serial(serial_or_message)Also applies to: 306-333
📜 Review details
Configuration used: Organization 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 (10)
ably/__init__.pyably/rest/channel.pyably/types/message.pyably/types/operations.pytest/ably/rest/restchannelmutablemessages_test.pytest/ably/rest/restchannelpublish_test.pytest/ably/rest/resthttp_test.pytest/ably/rest/restrequest_test.pytest/assets/testAppSpec.jsontest/unit/mutable_message_test.py
🚧 Files skipped from review as they are similar to previous changes (4)
- test/ably/rest/resthttp_test.py
- test/assets/testAppSpec.json
- test/ably/rest/restchannelpublish_test.py
- test/unit/mutable_message_test.py
🧰 Additional context used
🧬 Code graph analysis (5)
ably/types/operations.py (1)
ably/types/message.py (7)
client_id(63-64)client_id(165-166)description(67-68)metadata(71-72)as_dict(74-84)as_dict(236-285)from_dict(87-97)
test/ably/rest/restrequest_test.py (2)
ably/rest/channel.py (2)
ably(350-351)publish(129-155)ably/realtime/realtime_channel.py (1)
publish(393-493)
test/ably/rest/restchannelmutablemessages_test.py (8)
ably/rest/channel.py (9)
ably(350-351)publish(129-155)update_message(229-240)name(354-355)delete_message(242-253)append_message(255-266)get(389-400)get_message(268-304)get_message_versions(306-347)ably/util/exceptions.py (1)
AblyException(9-90)ably/util/crypto.py (2)
CipherParams(16-42)algorithm(25-26)ably/types/message.py (10)
MessageAction(100-107)Message(110-358)serial(55-56)serial(197-198)data(161-162)version(193-194)description(67-68)metadata(71-72)name(157-158)action(201-202)ably/types/operations.py (3)
MessageOperation(1-45)description(19-20)metadata(23-24)test/ably/testapp.py (2)
TestApp(37-115)get_test_vars(41-70)test/ably/utils.py (1)
assert_waiter(179-191)test/ably/rest/restrequest_test.py (1)
setup(16-27)
ably/types/message.py (2)
ably/types/presence.py (8)
timestamp(78-79)client_id(66-67)get(253-263)action(62-63)id(58-59)connection_id(70-71)extras(87-88)from_encoded(206-230)ably/types/operations.py (7)
client_id(15-16)description(19-20)metadata(23-24)as_dict(26-34)from_dict(37-45)from_dict(63-67)from_dict(85-89)
ably/__init__.py (2)
ably/types/message.py (2)
MessageAction(100-107)MessageVersion(25-97)ably/types/operations.py (3)
MessageOperation(1-45)PublishResult(48-67)UpdateDeleteResult(70-89)
⏰ 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.10)
- GitHub Check: check (3.12)
- GitHub Check: check (3.14)
- GitHub Check: check (3.11)
- GitHub Check: check (3.9)
- GitHub Check: check (3.13)
🔇 Additional comments (21)
ably/types/operations.py (2)
48-67: LGTM!
PublishResultclass is well-structured with proper null handling - defaults to empty list whenserialsisNoneor whenobjisNoneinfrom_dict.
70-89: LGTM!
UpdateDeleteResultclass follows the same clean pattern asPublishResultwith appropriate null-safefrom_dictimplementation.ably/types/message.py (2)
25-97: LGTM!
MessageVersionis well-implemented with proper string normalization viato_text(), consistent with patterns in the codebase. Theas_dict/from_dictmethods correctly handle None values.
100-107: LGTM!
MessageActionenum values align with the expected protocol message action types.ably/__init__.py (2)
10-11: LGTM!New types are correctly imported and exposed in the public API surface.
20-21: API version bumped from '3' to '5' — intentional and properly scoped.The version bump is confirmed by the commit message and aligns with the introduction of message update, delete, and append operations. Ably's platform supports mutable messages natively, and the implementation (MessageOperation, PublishResult, UpdateDeleteResult classes) properly handles the spec requirements for message mutations.
ably/rest/channel.py (4)
13-27: LGTM!Imports are correctly organized and include all necessary types for the new message operations.
180-197: LGTM! Proper guard againstNoneoperation.The check at lines 180-187 correctly handles the case when
operationisNone, preventing theAttributeErrorthat was raised in the previous version.
229-266: LGTM!The
update_message,delete_message, andappend_messagepublic methods are clean delegations to_send_updatewith appropriate action types. Documentation is clear and consistent.
218-218: Thehttp.patchmethod exists and is properly implemented as an async method inably/http/http.py. The code usage is correct.test/ably/rest/restchannelmutablemessages_test.py (11)
1-12: LGTM!Imports are appropriate for the test module's needs.
53-63: LGTM!Good negative test validating the 400 error when serial is missing.
65-95: LGTM!Well-structured test with proper operation passing and comprehensive assertions.
97-107: LGTM!Consistent error handling test for delete operation.
109-142: LGTM!Good test for append functionality with proper verification of concatenated data.
144-154: LGTM!Consistent error handling test.
179-198: LGTM!Test validates that params can be passed without error. Consider adding server-side verification if the params have observable effects.
200-214: LGTM!Good test verifying that
PublishResultreturns the expected number of serials.
242-246: Verify assertion semantics: comparing version serial to message serial.Line 244 compares
versions[0].version.serial(the version serial) toserial(the message serial fromresult.serials[0]). In other tests, version serials are compared to*_result.version_serialwhile message serials are compared separately (e.g., line 92 vs 95 intest_delete_message_success).If the initial version serial equals the message serial by design, this is fine. Otherwise, consider:
- Comparing message serials:
versions[0].serial == serial- Or storing the initial version serial from the publish result
248-273: LGTM!Good test specifically validating string data concatenation in append operations.
289-298: LGTM!The waiter correctly uses
>= countto tolerate eventual consistency scenarios where more versions than expected may be present.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Introduced `_send_update` method to handle message operations (update, delete, append) on the channel. - Added `update_message`, `delete_message`, and `append_message` methods to enable respective operations via the API. - Implemented `MessageOperation`, `PublishResult`, and `UpdateDeleteResult` classes for handling operation metadata and results. - Bumped `api_version` to 5.
ttypic
force-pushed
the
AIT-196/rest-edits-deletes
branch
from
a63e968 to
8ff2552
Compare
update_message,delete_message, andappend_messagemethods to enable respective operations via the API.MessageOperation,PublishResult, andUpdateDeleteResultclasses for handling operation metadata and results.api_versionto 5Summary by CodeRabbit
New Features
Updates
Tests
✏️ Tip: You can customize this high-level summary in your review settings.