[AIT-316] feat: introduce support for message annotations #667
Conversation
WalkthroughAdds full annotations support: new Annotation type and actions, channel modes and flags, centralized data encoding helpers, REST and Realtime annotation handlers (publish/delete/get/subscribe/unsubscribe), attach-flag encoding/decoding for channels, transport routing for ANNOTATION messages, and tests for REST/Realtime flows and pagination. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RealtimeAnnotations
participant ConnectionManager
participant WebSocket
participant Channel
Client->>RealtimeAnnotations: publish(msg_or_serial, annotation)
RealtimeAnnotations->>RealtimeAnnotations: validate & convert to wire format
RealtimeAnnotations->>ConnectionManager: send ProtocolMessage(ANNOTATION)
ConnectionManager->>WebSocket: transmit ANNOTATION
WebSocket-->>Channel: deliver ANNOTATION message
Channel->>RealtimeAnnotations: _process_incoming(incoming_annotations)
RealtimeAnnotations->>Client: emit to subscribers
sequenceDiagram
participant Client
participant RestAnnotations
participant HTTPClient
participant RestAPI
Client->>RestAnnotations: publish(msg_or_serial, annotation)
RestAnnotations->>RestAnnotations: validate serial & encode annotation
RestAnnotations->>HTTPClient: POST encoded payload
HTTPClient->>RestAPI: create annotation
RestAPI-->>HTTPClient: response
HTTPClient-->>RestAnnotations: return result
RestAnnotations-->>Client: return/resolve
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 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)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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/667/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: 5
🤖 Fix all issues with AI agents
In `@ably/realtime/annotations.py`:
- Around line 162-171: The AblyException instantiation in the annotation
listener check passes the error code and HTTP status in the wrong order; update
the raise to call AblyException with (message, status_code, code) so the HTTP
status is 400 and the Ably error code is 93001. Locate the block that checks
self.__channel.state == ChannelState.ATTACHED and uses
self.__channel._has_flag(Flag.ANNOTATION_SUBSCRIBE) and change the arguments to
AblyException accordingly (message, 400, 93001).
In `@ably/rest/annotations.py`:
- Around line 74-81: When annotation is a dict the code mutates the caller's
dict by assigning to annotation_values['message_serial']; change this to work on
a shallow copy instead: detect dict input (the branch where annotation_values =
annotation), replace that assignment with a copy operation (e.g. create a new
dict from annotation) before adding message_serial, then return
Annotation.from_values(annotation_values); keep the existing handling for
Annotation instances (using annotation.as_dict()) and the return via
Annotation.from_values.
In `@ably/types/annotation.py`:
- Around line 125-141: The request body currently includes a 'dataType' field
set from encoded.get('type'), but encode_data (in ably/util/encoding.py) never
returns a 'type' key so this is dead and always filtered out; remove the
"'dataType': encoded.get('type')" entry (and its comment) from the request_body
construction in the Annotation serialization (where encode_data is called) so
the payload no longer contains a redundant key, or alternatively update
encode_data to return a 'type' key if the API truly needs dataType—pick one
approach and apply it consistently.
In `@test/ably/realtime/realtimeannotations_test.py`:
- Around line 322-334: The test has a race: it asserts annotations_received
counts immediately after publish/delete instead of waiting for the async
listener; replace the immediate asserts after the create and delete steps with
an await that waits for the condition (e.g. use asyncio.wait_for with a short
timeout and a small-poll loop or a test helper like wait_for_condition) to
assert len(annotations_received) == 1 after the create and
len(annotations_received) == 2 after the delete; keep the checks for
annotations_received[0].action == AnnotationAction.ANNOTATION_CREATE and
annotations_received[1].action == AnnotationAction.ANNOTATION_DELETE but only
evaluate them after the respective await has confirmed the expected length, and
reference the existing annotations_received list and channel.annotations.delete
call when making the changes.
- Around line 263-281: The assertions on annotations_received are racing with
async callbacks; modify the test around channel.annotations.publish and
channel.annotations.unsubscribe to wait for the listener to process by either
awaiting a small sleep (e.g., asyncio.sleep(0.05)) after publishing or, better,
use an asyncio.Event set inside on_annotation and await that event before
asserting; update uses of annotations_received and on_annotation to coordinate
with the event (clear/reset the event before the second publish) so the first
assert waits for the first callback and the second assert reliably verifies no
new event fired after unsubscribe.
🧹 Nitpick comments (8)
ably/util/encoding.py (3)
11-14: Redundantstr()conversion afterjson.dumps().
json.dumps()already returns a string, so the subsequentdata = str(data)on line 14 is unnecessary.🔧 Suggested fix
if isinstance(data, (dict, list)): encoding.append('json') data = json.dumps(data) - data = str(data)
15-16: Explicit no-op is acceptable but could use a comment.The
passstatement documents that string data in non-binary mode intentionally passes through unchanged. A brief comment would improve clarity.
8-32: Consider handlingNonedata explicitly.When
dataisNone, no branch matches and it passes through unchanged with the original encoding. This appears intentional (the caller filters outNonevalues), but an explicit branch or comment would make this clearer.ably/rest/annotations.py (1)
143-145: Preferisinstance()overtype()for type checking.Using
isinstance(v, bool)is more Pythonic and handles subclasses correctly.Suggested fix
if params: - params = {k: str(v).lower() if type(v) is bool else v for k, v in params.items()} + params = {k: str(v).lower() if isinstance(v, bool) else v for k, v in params.items()} path += '?' + parse.urlencode(params)ably/realtime/channel.py (1)
88-89: Name-mangled attribute override is unconventional but functional.Using
self._Channel__annotationsto override the parent class's private__annotationsattribute works but is fragile. If the parent class implementation changes, this could break silently. Consider making the parent's attribute protected (_annotations) instead of private (__annotations) for cleaner subclass access.test/ably/rest/restannotations_test.py (1)
82-87: Redundant annotation retrieval afterassert_waiter.The
assert_waiteralready fetches and verifies annotations exist. The subsequentawait channel.annotations.get(serial)on line 84 duplicates the fetch. Consider reusingannotations_resultfrom the waiter.Suggested cleanup
await assert_waiter(check_annotations, timeout=10) - # Verify - annotations_result = await channel.annotations.get(serial) + # Verify (annotations_result is already populated by check_annotations) annotations = annotations_result.items assert len(annotations) >= 1 assert annotations[0].name == '😕'test/ably/realtime/realtimeannotations_test.py (1)
216-217: Anti-flake sleep is a code smell; consider a more robust synchronization.The comment acknowledges this is temporary. A future improvement could use event-based waiting or server-side confirmation rather than arbitrary sleep.
ably/realtime/annotations.py (1)
154-160: Listener is registered before channel attach completes.The subscription is registered (lines 155-158) before
await self.__channel.attach()(line 160). If attach fails, the listener remains registered. Consider registering after successful attach, or cleaning up on attach failure.Suggested improvement
- # Register subscription - if annotation_type is not None: - self.__subscriptions.on(annotation_type, listener) - else: - self.__subscriptions.on(listener) - await self.__channel.attach() + # Register subscription after successful attach + if annotation_type is not None: + self.__subscriptions.on(annotation_type, listener) + else: + self.__subscriptions.on(listener) + # Check if ANNOTATION_SUBSCRIBE mode is enabled
- Added `RealtimeAnnotations` class to manage annotation creation, deletion, and subscription on realtime channels. - Introduced `Annotation` and `AnnotationAction` types to encapsulate annotation details and actions. - Extended flags to include `ANNOTATION_PUBLISH` and `ANNOTATION_SUBSCRIBE`. - Refactored data encoding logic into `ably.util.encoding`. - Integrated annotation handling into `RealtimeChannel` and `RestChannel`.
ttypic
force-pushed
the
AIT-316/annotations-support
branch
from
20c2c55 to
70701af
Compare
github-actions
bot
temporarily deployed
to
staging/pull/667/features
ttypic
force-pushed
the
AIT-316/annotations-support
branch
2 times, most recently
from
7c8dff4 to
c95c0f4
Compare
github-actions
bot
temporarily deployed
to
staging/pull/667/features
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ably/realtime/channel.py (2)
711-717: Ensure modes are cleared when ATTACHED has no flags.If
flagsis missing/0,__modesretains old values, which can misreport channel modes after reattach.Suggested fix
- flags = proto_msg.get('flags') + flags = proto_msg.get('flags') or 0 @@ - if flags: - resumed = has_flag(flags, Flag.RESUMED) - # RTP1: Check for HAS_PRESENCE flag - has_presence = has_flag(flags, Flag.HAS_PRESENCE) - # Store channel attach flags - self.__modes = decode_channel_mode(flags) + resumed = has_flag(flags, Flag.RESUMED) + # RTP1: Check for HAS_PRESENCE flag + has_presence = has_flag(flags, Flag.HAS_PRESENCE) + # Store channel attach flags (empty list if no flags) + self.__modes = decode_channel_mode(flags)
3-25: Fix import ordering to resolve ruff I001 violation.Line 24 (
from ably.types.channelmode import...) is out of order and should be grouped with the otherably.typesimports before theably.utilimports.ably/types/channeloptions.py (1)
3-7: Fix import order to satisfy CI lint (ruff I001).CI reports unsorted imports for this file. Organize the third-party imports alphabetically by module path.
Suggested fix
-from ably.util.crypto import CipherParams -from ably.util.exceptions import AblyException -from ably.types.channelmode import ChannelMode +from ably.types.channelmode import ChannelMode +from ably.util.crypto import CipherParams +from ably.util.exceptions import AblyException
🤖 Fix all issues with AI agents
In `@ably/realtime/annotations.py`:
- Around line 43-86: construct_validate_annotation currently passes camelCase
keys from Annotation.as_dict into Annotation.from_values causing TypeError
(unexpected keyword) when publish() or delete() call
construct_validate_annotation with an Annotation instance; fix by normalizing
keys to snake_case inside construct_validate_annotation when annotation is an
Annotation (or add a snake_case serializer on Annotation) so keys like
messageSerial -> message_serial and clientId -> client_id are converted before
calling Annotation.from_values; update construct_validate_annotation (and
mention it will affect publish() and delete()) to perform this mapping for any
camelCase keys returned by Annotation.as_dict.
- Around line 117-176: The subscribe method registers the listener via
self.__subscriptions.on before awaiting self.__channel.attach(), so if attach
fails or the ANNOTATION_SUBSCRIBE mode check raises the listener remains
registered; update subscribe (and the branches that call
self.__subscriptions.on) to remove the listener on any failure: after calling
self.__subscriptions.on(...) keep a reference to the subscription key/listener
and in the exception paths (attach raising or the subsequent
ChannelState/Flag.ANNOTATION_SUBSCRIBE check) call the corresponding unsubscribe
method (e.g. self.__subscriptions.off(...) or the appropriate removal API) to
rollback the registration before re-raising the error, ensuring cleanup whether
annotation_type is set or None.
In `@ably/realtime/channel.py`:
- Around line 69-71: Replace the call to list() with the empty list literal for
the channel mode field: change the initialization of self.__modes (the Channel
mode flags list used for ATTACHED messages) from list() to [] so it follows
idiomatic Python and satisfies the ruff/pyupgrade rule; leave the surrounding
attributes (self.__channel_options and self.__params) unchanged.
In `@ably/rest/annotations.py`:
- Around line 21-49: The function serial_from_msg_or_serial currently only
accepts Message instances but the docstring/error promises any object with a
string `serial` property; update serial_from_msg_or_serial to duck-type by
replacing the isinstance(msg_or_serial, Message) branch with a
hasattr(msg_or_serial, "serial") check and validate that getattr(msg_or_serial,
"serial") is a str before assigning message_serial, so objects with a `serial`
attribute (not just Message) are accepted while preserving the existing
AblyException path when missing or invalid.
In `@ably/types/annotation.py`:
- Around line 17-61: The constructor Annotation.__init__ currently coerces a
passed-in action=None back to AnnotationAction.ANNOTATION_CREATE, which loses
the explicit None produced by from_encoded(); change it to accept a sentinel
default (e.g., _UNSPECIFIED = object()) for the action parameter and only assign
AnnotationAction.ANNOTATION_CREATE when action is the sentinel, preserving an
explicit None value when callers pass None; update the parameter signature and
the assignment to self.__action accordingly to use the sentinel check instead of
action is not None.
In `@ably/types/channeloptions.py`:
- Around line 39-42: The docstring for the property modes is generic; update it
to specifically reference channel modes and the return type: state that modes
returns the channel's configured modes as a list of ChannelMode or None (e.g.,
"Return configured channel modes (list[ChannelMode]) or None if unset"). Edit
the docstring on the modes property to mention "channel modes" and the expected
return (list[ChannelMode] | None) so it's clear what the property represents.
- Around line 21-24: The __init__ method signature in ChannelOptions is too long
(127 chars); split the signature of __init__ across multiple lines so no line
exceeds 115 chars — for example put each parameter (cipher: CipherParams | None
= None, params: dict | None = None, modes: list[ChannelMode] | None = None) on
its own line aligned under def __init__(...) — keep the same parameter names and
types and retain the body assignments to self.__cipher, self.__params,
self.__modes.
- Around line 44-58: The equality/hash mismatch in ChannelOptions stems from
__eq__ comparing self.__modes to other.__modes order-sensitively while __hash__
sorts modes; update both to be order-insensitive: change ChannelOptions.__eq__
to compare modes using set(self.__modes) == set(other.__modes) (and keep params
and cipher comparisons as-is), and change __hash__ to use
frozenset(self.__modes) (and frozenset(self.__params.items()) or None) so
hashing and equality are consistent; this will stop
should_reattach_to_set_options() from triggering reattachment when modes differ
only by order.
In `@test/ably/rest/restannotations_test.py`:
- Around line 1-12: Reorder the imports in restannotations_test.py so standard
library modules (logging, random, string) appear grouped before third-party
imports (pytest) to satisfy ruff I001; keep the package imports (from ably...,
from test.ably...) after third-party imports and preserve existing imported
symbols (AblyException, AnnotationAction, Message, TestApp, BaseAsyncTestCase,
assert_waiter).
| def serial_from_msg_or_serial(msg_or_serial): | ||
| """ | ||
| Extract the message serial from either a string serial or a Message object. | ||
|
|
||
| Args: | ||
| msg_or_serial: Either a string serial or a Message object with a serial property | ||
|
|
||
| Returns: | ||
| str: The message serial | ||
|
|
||
| Raises: | ||
| AblyException: If the input is invalid or serial is missing | ||
| """ | ||
| if isinstance(msg_or_serial, str): | ||
| message_serial = msg_or_serial | ||
| elif isinstance(msg_or_serial, Message): | ||
| message_serial = msg_or_serial.serial | ||
| else: | ||
| message_serial = None | ||
|
|
||
| if not message_serial or not isinstance(message_serial, str): | ||
| raise AblyException( | ||
| message='First argument of annotations.publish() must be either a Message ' | ||
| '(or at least an object with a string `serial` property) or a message serial (string)', | ||
| status_code=400, | ||
| code=40003, | ||
| ) | ||
|
|
||
| return message_serial |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n ably/rest/annotations.py | head -60Repository: ably/ably-python
Length of output: 2155
🏁 Script executed:
# Search for the Message class to understand its structure
rg "class Message" --type py -A 5Repository: ably/ably-python
Length of output: 1291
🏁 Script executed:
# Find usages of serial_from_msg_or_serial to understand intended inputs
rg "serial_from_msg_or_serial" --type py -B 2 -A 2Repository: ably/ably-python
Length of output: 969
Align accepted inputs with the error message.
The error message promises to accept "an object with a string serial property," but the code only accepts Message instances. Update the condition to support duck typing with hasattr() to match the documented contract.
Suggested fix
- elif isinstance(msg_or_serial, Message):
- message_serial = msg_or_serial.serial
+ elif isinstance(msg_or_serial, Message) or hasattr(msg_or_serial, "serial"):
+ message_serial = msg_or_serial.serial🤖 Prompt for AI Agents
In `@ably/rest/annotations.py` around lines 21 - 49, The function
serial_from_msg_or_serial currently only accepts Message instances but the
docstring/error promises any object with a string `serial` property; update
serial_from_msg_or_serial to duck-type by replacing the
isinstance(msg_or_serial, Message) branch with a hasattr(msg_or_serial,
"serial") check and validate that getattr(msg_or_serial, "serial") is a str
before assigning message_serial, so objects with a `serial` attribute (not just
Message) are accepted while preserving the existing AblyException path when
missing or invalid.
| @property | ||
| def modes(self) -> list[ChannelMode] | None: | ||
| """Get channel parameters""" | ||
| return self.__modes |
There was a problem hiding this comment.
Docstring should reference channel modes.
Suggested fix
- """Get channel parameters"""
+ """Get channel modes"""📝 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.
| @property | |
| def modes(self) -> list[ChannelMode] | None: | |
| """Get channel parameters""" | |
| return self.__modes | |
| `@property` | |
| def modes(self) -> list[ChannelMode] | None: | |
| """Get channel modes""" | |
| return self.__modes |
🤖 Prompt for AI Agents
In `@ably/types/channeloptions.py` around lines 39 - 42, The docstring for the
property modes is generic; update it to specifically reference channel modes and
the return type: state that modes returns the channel's configured modes as a
list of ChannelMode or None (e.g., "Return configured channel modes
(list[ChannelMode]) or None if unset"). Edit the docstring on the modes property
to mention "channel modes" and the expected return (list[ChannelMode] | None) so
it's clear what the property represents.
c95c0f4 to
69ddea5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/667/features
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ably/rest/channel.py (1)
11-30: Remove duplicateRestAnnotationsimport (lint error). Line 29 re-importsRestAnnotations, triggering F811 and failing CI; also address the “organize imports” warning after removing it.🧹 Proposed fix
-from ably.rest.annotations import RestAnnotationsably/realtime/channel.py (1)
711-717: Reset modes when flags is 0/None to avoid stale modes. Line 711 usesif flags:, so a zero flags value skips decoding and leaves prior modes intact. Use an explicitNonecheck and reset to empty when absent.🛠️ Proposed fix
- if flags: - resumed = has_flag(flags, Flag.RESUMED) - # RTP1: Check for HAS_PRESENCE flag - has_presence = has_flag(flags, Flag.HAS_PRESENCE) - # Store channel attach flags - self.__modes = decode_channel_mode(flags) + if flags is not None: + resumed = has_flag(flags, Flag.RESUMED) + # RTP1: Check for HAS_PRESENCE flag + has_presence = has_flag(flags, Flag.HAS_PRESENCE) + # Store channel attach flags + self.__modes = decode_channel_mode(flags) + else: + self.__modes = []
🧹 Nitpick comments (3)
ably/realtime/channel.py (1)
7-25: Address import-order lint warning. Pipeline reports “organize imports” after adding new imports (Lines 7–25); please re-run the import sorter.test/ably/rest/restannotations_test.py (2)
88-92: Remove redundant API call.Line 89 fetches annotations again, but
annotations_resultis already populated bycheck_annotationsduring the wait. This extra call is unnecessary.Suggested fix
await assert_waiter(check_annotations, timeout=10) # Verify - annotations_result = await channel.annotations.get(serial) annotations = annotations_result.items assert len(annotations) >= 1 assert annotations[0].name == '😕'
134-140: Fix misleading comment.The comment says "Wait for annotation to appear" but this block is waiting for the delete annotation event. The original annotation was already verified in the previous wait.
Suggested fix
- # Wait for annotation to appear + # Wait for delete annotation to appear async def check_deleted_annotation():
ttypic
force-pushed
the
AIT-316/annotations-support
branch
from
69ddea5 to
072da64
Compare
github-actions
bot
temporarily deployed
to
staging/pull/667/features
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/ably/utils.py`:
- Around line 250-265: ReusableFuture.get currently awaits self.__future with
asyncio.wait_for but does not reset the internal future on timeout/exception and
does not return the awaited value; update ReusableFuture.get to await the
underlying future and capture its result, use a try/finally so that
self.__future is replaced with a fresh asyncio.Future() even if wait_for raises
(timeout or other), return the captured result on success, and re-raise any
exception after resetting so subsequent set_result/set_exception on the new
self.__future won't fail; refer to the ReusableFuture.get method and the
self.__future attribute (and existing set_result/set_exception helpers) when
applying this change.
🧹 Nitpick comments (3)
ably/rest/annotations.py (1)
146-153: Minor: Useisinstance()instead oftype() isfor type checking.Line 149 uses
type(v) is boolwhich works but is less idiomatic thanisinstance(v, bool):Suggested fix
# Build path path = self.__base_path_for_serial(annotation.message_serial) if params: - params = {k: str(v).lower() if type(v) is bool else v for k, v in params.items()} + params = {k: str(v).lower() if isinstance(v, bool) else v for k, v in params.items()} path += '?' + parse.urlencode(params)ably/realtime/channel.py (1)
69-91: Line 90: Direct name-mangled attribute override relies on implementation details.The code overrides the parent's
__annotationsusing Python's name mangling syntax (self._Channel__annotations). While this works, it's fragile—any rename of the parent's private attribute will break this.The parent
Channelclass initializesself.__annotations = RestAnnotations(self)in its__init__, and RealtimeChannel must override it withRealtimeAnnotationssince the parent's publicannotationsproperty has no setter.Consider refactoring the parent
Channelclass to use a protected attribute (_annotations) instead of private (__annotations), which would allow cleaner overrides without relying on name mangling.test/ably/realtime/realtimeannotations_test.py (1)
22-34: Add teardown to close clients in the fixture.The
setupfixture createsrealtime_clientandrest_clientbut lacks proper cleanup. Both clients implementasync def close()methods, and other tests in the codebase (e.g.,realtimepresence_test.py) follow the pattern of usingyieldfollowed by explicit cleanup. Update the fixture to properly close resources:Suggested teardown
`@pytest.fixture`(autouse=True) async def setup(self, transport): self.test_vars = await TestApp.get_test_vars() client_id = ''.join(random.choices(string.ascii_letters + string.digits, k=10)) self.realtime_client = await TestApp.get_ably_realtime( use_binary_protocol=True if transport == 'msgpack' else False, client_id=client_id, ) self.rest_client = await TestApp.get_ably_rest( use_binary_protocol=True if transport == 'msgpack' else False, client_id=client_id, ) + + yield + + await self.realtime_client.close() + await self.rest_client.close()
ttypic
force-pushed
the
AIT-316/annotations-support
branch
from
072da64 to
1ddefac
Compare
github-actions
bot
temporarily deployed
to
staging/pull/667/features
ttypic
force-pushed
the
AIT-316/annotations-support
branch
from
1ddefac to
d867def
Compare
github-actions
bot
temporarily deployed
to
staging/pull/667/features
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably/realtime/channel.py (1)
711-717: Reset modes when flags are absent/zero to avoid stale state.Line 711 only updates modes when
flagsis truthy. If the server omits flags or sends0, previousself.__modescan leak across attaches. Consider treating missing flags as zero and always decoding.✅ Suggested fix
- if flags: - resumed = has_flag(flags, Flag.RESUMED) - # RTP1: Check for HAS_PRESENCE flag - has_presence = has_flag(flags, Flag.HAS_PRESENCE) - # Store channel attach flags - self.__modes = decode_channel_mode(flags) + if flags is None: + flags = 0 + resumed = has_flag(flags, Flag.RESUMED) + # RTP1: Check for HAS_PRESENCE flag + has_presence = has_flag(flags, Flag.HAS_PRESENCE) + # Store channel attach flags + self.__modes = decode_channel_mode(flags)
🤖 Fix all issues with AI agents
In `@ably/rest/annotations.py`:
- Around line 68-79: construct_validate_annotation currently calls
annotation.copy() and fails for Annotation instances and also passes camelCase
dicts from Annotation.as_dict() into Annotation.from_values; update
construct_validate_annotation to: if annotation is an Annotation instance,
extract its dict via annotation.as_dict() (or a dedicated values method if
available), normalize its keys from camelCase to snake_case (e.g., messageSerial
-> message_serial) and then set message_serial; otherwise, if annotation is a
dict, shallow-copy it and ensure message_serial is set; finally call
Annotation.from_values with the normalized snake_case dict. Make this change so
publish()/delete() paths (which may pass Annotation.as_dict()) work with
Annotation.from_values.
🧹 Nitpick comments (3)
ably/realtime/channel.py (1)
914-921: Consider returning a copy ofmodesto prevent external mutation.Since
modesis a public property, returning the internal list allows callers to mutate internal state inadvertently.♻️ Suggested tweak
def modes(self): """Get the list of channel modes""" - return self.__modes + return list(self.__modes)test/ably/rest/restannotations_test.py (2)
88-92: Minor: Redundant fetch after waiter.Line 89 re-fetches annotations that were already retrieved by
check_annotations(). Theannotations_resultvariable is already populated from the waiter.Suggested simplification
await assert_waiter(check_annotations, timeout=10) # Verify - annotations_result = await channel.annotations.get(serial) annotations = annotations_result.items assert len(annotations) >= 1 assert annotations[0].name == '😕'
175-203: Consider asserting on thedataproperty.The test publishes an annotation with
'data': {'count': 5}but doesn't verify that this data is correctly retrieved. Adding an assertion would strengthen the test coverage for annotation properties.Suggested enhancement
assert annotation.type == 'reaction:distinct.v1' assert annotation.name == '❤️' assert annotation.serial is not None assert annotation.serial > serial + assert annotation.data == {'count': 5}
| if not annotation or (not isinstance(annotation, dict) and not isinstance(annotation, Annotation)): | ||
| raise AblyException( | ||
| message='Second argument of annotations.publish() must be a dict or Annotation ' | ||
| '(the intended annotation to publish)', | ||
| status_code=400, | ||
| code=40003, | ||
| ) | ||
|
|
||
| annotation_values = annotation.copy() | ||
| annotation_values['message_serial'] = message_serial | ||
|
|
||
| return Annotation.from_values(annotation_values) |
There was a problem hiding this comment.
construct_validate_annotation() breaks for Annotation inputs (and delete path).
Line 76 assumes .copy() exists, so passing an Annotation raises AttributeError. Additionally, delete() passes annotation.as_dict() (camelCase keys), which will fail Annotation.from_values() (snake_case args). This makes publish()/delete() unusable with Annotation instances.
🛠️ Suggested fix
- annotation_values = annotation.copy()
- annotation_values['message_serial'] = message_serial
+ if isinstance(annotation, Annotation):
+ annotation_values = {
+ "action": annotation.action,
+ "serial": annotation.serial,
+ "message_serial": annotation.message_serial,
+ "type": annotation.type,
+ "name": annotation.name,
+ "count": annotation.count,
+ "data": annotation.data,
+ "encoding": annotation.encoding,
+ "client_id": annotation.client_id,
+ "timestamp": annotation.timestamp,
+ "extras": annotation.extras,
+ }
+ else:
+ annotation_values = annotation.copy()
+ # normalize camelCase if the caller passed Annotation.as_dict()
+ if "messageSerial" in annotation_values:
+ annotation_values["message_serial"] = annotation_values.pop("messageSerial")
+ if "clientId" in annotation_values:
+ annotation_values["client_id"] = annotation_values.pop("clientId")
+
+ annotation_values["message_serial"] = message_serial🤖 Prompt for AI Agents
In `@ably/rest/annotations.py` around lines 68 - 79, construct_validate_annotation
currently calls annotation.copy() and fails for Annotation instances and also
passes camelCase dicts from Annotation.as_dict() into Annotation.from_values;
update construct_validate_annotation to: if annotation is an Annotation
instance, extract its dict via annotation.as_dict() (or a dedicated values
method if available), normalize its keys from camelCase to snake_case (e.g.,
messageSerial -> message_serial) and then set message_serial; otherwise, if
annotation is a dict, shallow-copy it and ensure message_serial is set; finally
call Annotation.from_values with the normalized snake_case dict. Make this
change so publish()/delete() paths (which may pass Annotation.as_dict()) work
with Annotation.from_values.
There was a problem hiding this comment.
This type error definitely needs to be fixed
There was a problem hiding this comment.
@owenpearson, I'll fix the type error in construct_validate_annotation() to properly handle both dict and Annotation instances, and normalize camelCase keys to snake_case.
Task: Fix construct_validate_annotation to handle Annotation instances
- Update construct_validate_annotation in ably/rest/annotations.py to handle both dict and Annotation inputs
- Normalize camelCase keys from as_dict() to snake_case for from_values()
- Ensure message_serial is properly set in all cases
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
There was a problem hiding this comment.
@owenpearson, understood — I've stopped the planning process. Let me know if you'd like to address this in a different way or revisit it later.
- Added `RealtimeAnnotations` class to manage annotation creation, deletion, and subscription on realtime channels. - Introduced `Annotation` and `AnnotationAction` types to encapsulate annotation details and actions. - Extended flags to include `ANNOTATION_PUBLISH` and `ANNOTATION_SUBSCRIBE`. - Refactored data encoding logic into `ably.util.encoding`. - Integrated annotation handling into `RealtimeChannel` and `RestChannel`.
ttypic
force-pushed
the
AIT-316/annotations-support
branch
from
d867def to
20288a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/ably/rest/restannotations_test.py`:
- Around line 183-203: The test publishes an annotation with data {'count': 5}
but never checks it; update the verification after fetching annotations (in the
async check_annotation/get flow using channel.annotations.get and the local
variables annotations_result and annotation) to assert that annotation.data
exists and equals {'count': 5} (or contains the expected count value), e.g., add
an assertion checking annotation.data['count'] == 5 to complete the coverage.
🧹 Nitpick comments (9)
ably/util/encoding.py (1)
11-14: Remove redundantstr()call.
json.dumps()already returns astr, so wrapping it withstr()on line 14 is unnecessary.♻️ Suggested fix
if isinstance(data, (dict, list)): encoding.append('json') data = json.dumps(data) - data = str(data)ably/types/annotation.py (1)
72-77:__ne__is redundant in Python 3.Python 3 automatically delegates
__ne__tonot __eq__()when__ne__is not defined. This explicit implementation can be removed.♻️ Suggested fix
- def __ne__(self, other): - if isinstance(other, Annotation): - result = self.__eq__(other) - if result != NotImplemented: - return not result - return NotImplementedably/realtime/channel.py (1)
82-90: Clarify the name-mangled attribute assignment.Line 90 uses
self._Channel__annotationsto directly set the parent class's private attribute. While this works for overriding REST annotations with realtime annotations, it relies on Python name mangling internals and may be confusing to maintainers.Consider adding a comment explaining why this pattern is used, or refactoring the parent class to expose a setter/protected method for annotation injection.
ably/realtime/annotations.py (3)
31-41: Docstring is incomplete.The docstring for
__init__only documents thechannelparameter but omitsconnection_manager.📝 Suggested fix
def __init__(self, channel: RealtimeChannel, connection_manager: ConnectionManager): """ Initialize RealtimeAnnotations. Args: channel: The Realtime Channel this annotations instance belongs to + connection_manager: The ConnectionManager for sending protocol messages """
43-43: API inconsistency with RestAnnotations.The type hint for
annotationisdict, butRestAnnotations.publish(inably/rest/annotations.pyline 116) acceptsdict | Annotation. Consider aligning the signatures for API consistency.♻️ Suggested fix
- async def publish(self, msg_or_serial, annotation: dict, params: dict | None = None): + async def publish(self, msg_or_serial, annotation: dict | Annotation, params: dict | None = None):You'll also need to import
Annotation:from ably.types.annotation import Annotation, AnnotationAction
87-112: Same API inconsistency applies here.Similar to
publish, thedeletemethod accepts onlydictwhileRestAnnotations.deleteacceptsdict | Annotation. The implementation also assumes dict withannotation.copy(), which would fail forAnnotationobjects.♻️ Suggested fix (align with RestAnnotations.delete pattern)
+from ably.types.annotation import Annotation, AnnotationAction + async def delete( self, msg_or_serial, - annotation: dict, + annotation: dict | Annotation, params: dict | None = None, ): - annotation_values = annotation.copy() + if isinstance(annotation, Annotation): + annotation_values = annotation.as_dict() + else: + annotation_values = annotation.copy() annotation_values['action'] = AnnotationAction.ANNOTATION_DELETEtest/ably/rest/restannotations_test.py (2)
19-26: Consider adding teardown to close the Ably client.The setup fixture creates an Ably REST client but doesn't close it after the test completes. This could lead to resource leaks during test runs.
♻️ Suggested fix
`@pytest.fixture`(autouse=True) async def setup(self, transport): self.test_vars = await TestApp.get_test_vars() client_id = ''.join(random.choices(string.ascii_letters + string.digits, k=10)) self.ably = await TestApp.get_ably_rest( use_binary_protocol=True if transport == 'msgpack' else False, client_id=client_id, ) + yield + await self.ably.close()
134-140: Misleading comment.Line 134 says "Wait for annotation to appear" but this is waiting for the delete annotation to appear. Consider updating for clarity.
📝 Suggested fix
- # Wait for annotation to appear + # Wait for delete annotation to appear async def check_deleted_annotation():test/ably/realtime/realtimeannotations_test.py (1)
22-34: Consider adding teardown to close both clients.The setup fixture creates both realtime and REST clients but doesn't close them after tests complete.
♻️ Suggested fix
`@pytest.fixture`(autouse=True) async def setup(self, transport): self.test_vars = await TestApp.get_test_vars() client_id = ''.join(random.choices(string.ascii_letters + string.digits, k=10)) self.realtime_client = await TestApp.get_ably_realtime( use_binary_protocol=True if transport == 'msgpack' else False, client_id=client_id, ) self.rest_client = await TestApp.get_ably_rest( use_binary_protocol=True if transport == 'msgpack' else False, client_id=client_id, ) + yield + await self.realtime_client.close() + await self.rest_client.close()
| # Publish annotation with various properties | ||
| await channel.annotations.publish(serial, { | ||
| 'type': 'reaction:distinct.v1', | ||
| 'name': '❤️', | ||
| 'data': {'count': 5} | ||
| }) | ||
|
|
||
| # Retrieve and verify | ||
| async def check_annotation(): | ||
| res = await channel.annotations.get(serial) | ||
| return len(res.items) > 0 | ||
|
|
||
| await assert_waiter(check_annotation, timeout=10) | ||
|
|
||
| annotations_result = await channel.annotations.get(serial) | ||
| annotation = annotations_result.items[0] | ||
| assert annotation.message_serial == serial | ||
| assert annotation.type == 'reaction:distinct.v1' | ||
| assert annotation.name == '❤️' | ||
| assert annotation.serial is not None | ||
| assert annotation.serial > serial |
There was a problem hiding this comment.
Missing assertion for data property.
The test publishes an annotation with 'data': {'count': 5} but never verifies that annotation.data contains this value. Add an assertion to complete the coverage.
💚 Suggested fix
assert annotation.message_serial == serial
assert annotation.type == 'reaction:distinct.v1'
assert annotation.name == '❤️'
+ assert annotation.data == {'count': 5}
assert annotation.serial is not None
assert annotation.serial > serial📝 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.
| # Publish annotation with various properties | |
| await channel.annotations.publish(serial, { | |
| 'type': 'reaction:distinct.v1', | |
| 'name': '❤️', | |
| 'data': {'count': 5} | |
| }) | |
| # Retrieve and verify | |
| async def check_annotation(): | |
| res = await channel.annotations.get(serial) | |
| return len(res.items) > 0 | |
| await assert_waiter(check_annotation, timeout=10) | |
| annotations_result = await channel.annotations.get(serial) | |
| annotation = annotations_result.items[0] | |
| assert annotation.message_serial == serial | |
| assert annotation.type == 'reaction:distinct.v1' | |
| assert annotation.name == '❤️' | |
| assert annotation.serial is not None | |
| assert annotation.serial > serial | |
| # Publish annotation with various properties | |
| await channel.annotations.publish(serial, { | |
| 'type': 'reaction:distinct.v1', | |
| 'name': '❤️', | |
| 'data': {'count': 5} | |
| }) | |
| # Retrieve and verify | |
| async def check_annotation(): | |
| res = await channel.annotations.get(serial) | |
| return len(res.items) > 0 | |
| await assert_waiter(check_annotation, timeout=10) | |
| annotations_result = await channel.annotations.get(serial) | |
| annotation = annotations_result.items[0] | |
| assert annotation.message_serial == serial | |
| assert annotation.type == 'reaction:distinct.v1' | |
| assert annotation.name == '❤️' | |
| assert annotation.data == {'count': 5} | |
| assert annotation.serial is not None | |
| assert annotation.serial > serial |
🤖 Prompt for AI Agents
In `@test/ably/rest/restannotations_test.py` around lines 183 - 203, The test
publishes an annotation with data {'count': 5} but never checks it; update the
verification after fetching annotations (in the async check_annotation/get flow
using channel.annotations.get and the local variables annotations_result and
annotation) to assert that annotation.data exists and equals {'count': 5} (or
contains the expected count value), e.g., add an assertion checking
annotation.data['count'] == 5 to complete the coverage.
| if not annotation or (not isinstance(annotation, dict) and not isinstance(annotation, Annotation)): | ||
| raise AblyException( | ||
| message='Second argument of annotations.publish() must be a dict or Annotation ' | ||
| '(the intended annotation to publish)', | ||
| status_code=400, | ||
| code=40003, | ||
| ) | ||
|
|
||
| annotation_values = annotation.copy() | ||
| annotation_values['message_serial'] = message_serial | ||
|
|
||
| return Annotation.from_values(annotation_values) |
There was a problem hiding this comment.
This type error definitely needs to be fixed
| if encoding: | ||
| result['encoding'] = '/'.join(encoding).strip('/') | ||
|
|
||
| return result |
There was a problem hiding this comment.
Why did you remove the data type validation when you copied this over?
if not (isinstance(data, (bytes, str, list, dict, bytearray)) or data is None):
raise AblyException("Invalid data payload", 400, 40011)
| encoding.append('base64') | ||
| elif isinstance(data, CipherData): | ||
| encoding.append(data.encoding_str) | ||
| data_type = data.type |
There was a problem hiding this comment.
the original code captured data_type here and set it as the type field on the result, is it safe to remove that?
There was a problem hiding this comment.
yeah, checked in ably-js, spec and other sdks, there are no such field (not sure it ever was). and this data_type is not used anywhere later on
| else: | ||
| raise ValueError('invalid subscribe arguments') | ||
|
|
||
| # Register subscription |
There was a problem hiding this comment.
we should probably validate channel mode before adding the subscription - in this code the listener would be attached even if the annotation_subscribe mode isn't enabled
| @@ -1,27 +1,16 @@ | |||
| import base64 | |||
There was a problem hiding this comment.
Are we intending to add Message.annotations as part of this change?
RealtimeAnnotationsclass to manage annotation creation, deletion, and subscription on realtime channels.AnnotationandAnnotationActiontypes to encapsulate annotation details and actions.ANNOTATION_PUBLISHandANNOTATION_SUBSCRIBE.ably.util.encoding.RealtimeChannelandRestChannel.Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.