[AIT-316] feat: introduce support for message annotations #667
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds annotations support across REST and Realtime: new Annotation types and actions, REST and Realtime handlers (publish/delete/get/subscribe/unsubscribe), channel mode flags and attach-flag encoding/decoding, centralized data encoding helpers, transport routing for ANNOTATION messages, and tests covering 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)
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).
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}
- 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
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/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()
| 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?
… handling - Refactored to mandate the `Annotation` type across annotation-related methods in `RealtimeAnnotations` and `RestAnnotations`. - Introduced `_copy_with` in `Annotation` for simplified object cloning with modifications. - Enhanced data validation in `encode_data` to raise `AblyException` for unsupported payloads.
github-actions
bot
temporarily deployed
to
staging/pull/667/features
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ably/rest/annotations.py`:
- Around line 68-74: The error message thrown in the validation block for
annotations.publish is stale: the code only accepts Annotation instances
(checked by isinstance(annotation, Annotation)) but the AblyException message
still says "must be a dict or Annotation". Update the AblyException message in
that validation (the block referencing Annotation and raising AblyException) to
only mention Annotation (e.g., "Second argument of annotations.publish() must be
an Annotation (the intended annotation to publish)") while keeping the same
status_code and code.
In `@ably/types/message.py`:
- Around line 330-332: The code is directly mutating a name-mangled private
attribute (annotations._MessageAnnotations__summary = {}) which breaks
encapsulation; instead, update MessageAnnotations.from_dict to ensure the
instance's summary attribute defaults to {} when the parsed value is None (e.g.,
set self.summary = summary_value or {} during construction), and then remove the
external fallback lines that assign to _MessageAnnotations__summary; locate the
MessageAnnotations.from_dict factory/constructor and ensure it normalizes
summary to an empty dict so callers (and the code that currently sets
annotations._MessageAnnotations__summary) can be deleted.
🧹 Nitpick comments (5)
ably/util/encoding.py (2)
12-15: Redundantstr()call afterjson.dumps.
json.dumpsalready returns astr, sodata = str(data)on line 15 is a no-op.
31-34: Payload validation occurs after transformation — consider validating earlier.The type check on line 33 runs after all transformation branches. If the caller passes an unsupported type (e.g.,
int,float), it silently falls through every branch unchanged and only then raises. This works correctly but could be clearer if the unsupported-type guard were placed before the transformation logic.ably/types/message.py (1)
334-349: In-place mutation of summary entries to injectclippeddefaults.This deeply nested logic mutates the summary dict in-place during deserialization. While it's spec-driven (TM7c1c, TM7d1c), consider extracting this into a dedicated helper method on
MessageAnnotations(e.g.,_ensure_clipped_fields()) for readability and testability.test/ably/realtime/realtimeannotations_test.py (1)
78-85: Futures awaited without explicit timeout — risk of hanging tests.Several
awaitcalls onasyncio.Future(e.g., lines 80, 88, 107, 201) andReusableFuture.get()(e.g., lines 237, 252, 296, 309) lack explicit timeouts. If an annotation is never delivered, the test will hang indefinitely rather than fail cleanly. Consider wrapping withasyncio.wait_for(..., timeout=...).Also applies to: 252-253
ably/types/annotation.py (1)
54-66: Duplicate encoding storage —self.__encodingshadows the mixin's_encoding_array.Line 66 stores
encodingseparately asself.__encoding, whilesuper().__init__(encoding)already stores it in the mixin's_encoding_array. The_copy_withmethod reads fromself.__encoding(line 262), which won't reflect changes made through the mixin'sencodingsetter. For annotations (which aren't encrypted), this is low risk, but it's a latent inconsistency.
ably/types/message.py
Outdated
| # TM8a: Ensure summary exists | ||
| if annotations.summary is None: | ||
| annotations._MessageAnnotations__summary = {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Bypassing encapsulation via name-mangled private attribute access.
annotations._MessageAnnotations__summary = {} directly mutates a private field through Python's name mangling. This is fragile and breaks if MessageAnnotations is renamed or refactored. Consider adding a fallback in MessageAnnotations.from_dict to default summary to {} when it's None, which would eliminate the need for this external mutation.
Proposed fix in MessageAnnotations.from_dict
`@staticmethod`
def from_dict(obj):
"""Create MessageAnnotations from dictionary."""
if obj is None:
return MessageAnnotations()
return MessageAnnotations(
- summary=obj.get('summary'),
+ summary=obj.get('summary') or {},
)Then remove lines 330–332:
- # TM8a: Ensure summary exists
- if annotations.summary is None:
- annotations._MessageAnnotations__summary = {}🤖 Prompt for AI Agents
In `@ably/types/message.py` around lines 330 - 332, The code is directly mutating
a name-mangled private attribute (annotations._MessageAnnotations__summary = {})
which breaks encapsulation; instead, update MessageAnnotations.from_dict to
ensure the instance's summary attribute defaults to {} when the parsed value is
None (e.g., set self.summary = summary_value or {} during construction), and
then remove the external fallback lines that assign to
_MessageAnnotations__summary; locate the MessageAnnotations.from_dict
factory/constructor and ensure it normalizes summary to an empty dict so callers
(and the code that currently sets annotations._MessageAnnotations__summary) can
be deleted.
There was a problem hiding this comment.
Pull request overview
This PR introduces message annotations across REST and Realtime channels, including publish/get/delete APIs and realtime subscription support, and adds channel “mode” flags to explicitly request annotation capabilities during attach. It also refactors payload encoding into a shared utility and extends Message to surface annotation summaries.
Changes:
- Add
Annotationtypes plus REST (RestAnnotations) and realtime (RealtimeAnnotations) annotation APIs, wiring annotation protocol messages through the transport/channel stack. - Introduce
ChannelMode/mode flags and plumb requested/attached modes throughChannelOptionsandRealtimeChannelattach. - Centralize payload encoding logic into
ably.util.encoding.encode_dataand update message/presence encoding paths accordingly; add REST/realtime annotation test suites.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Bumps local editable package version reference. |
| test/ably/utils.py | Adds ReusableFuture helper for async tests. |
| test/ably/rest/restannotations_test.py | New REST annotation publish/get/delete coverage. |
| test/ably/realtime/realtimeconnection_test.py | Updates expected transport param key to clientId. |
| test/ably/realtime/realtimeannotations_test.py | New realtime annotation publish/subscribe/unsubscribe/delete coverage (plus mode requirements). |
| ably/util/helper.py | Adds shared to_text() helper. |
| ably/util/encoding.py | New centralized encode_data() used by message/presence/annotation encoding. |
| ably/types/presence.py | Switches to shared encode_data() implementation. |
| ably/types/message.py | Adds MessageAnnotations + message annotation summary parsing/serialization; uses encode_data(). |
| ably/types/flags.py | Adds annotation publish/subscribe flag bits. |
| ably/types/channeloptions.py | Extends options to carry requested channel modes. |
| ably/types/channelmode.py | New mode enum + encode/decode helpers for attach flags. |
| ably/types/annotation.py | New annotation model/types and response handlers. |
| ably/transport/websockettransport.py | Routes incoming ANNOTATION protocol messages to channel handler. |
| ably/rest/channel.py | Exposes .annotations on REST channels. |
| ably/rest/auth.py | Uses clientId transport param and omits wildcard *. |
| ably/rest/annotations.py | Implements REST annotations API (publish/delete/get). |
| ably/realtime/channel.py | Stores attached channel modes; encodes requested modes into attach flags; handles ANNOTATION protocol messages. |
| ably/realtime/annotations.py | Implements realtime annotation publish/subscribe/unsubscribe + delegates get() to REST. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ttypic
force-pushed
the
AIT-316/annotations-support
branch
from
db902dc to
42c0fd4
Compare
|
I compared realtime annotations spec, rest annotations spec with current PR taking ably-java as a reference implementation, found follwing issues 1. ❌ Missing
|
|
Few more issues to fix, first 9 ideally should be fixed, remaining are nice to have 8.
|
3 participants
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