[ECO-5065] feat: add support for managing message annotations #1096
Conversation
WalkthroughThis change introduces support for message annotations (such as reactions) in the Ably Java library. It adds new classes and methods for creating, publishing, retrieving, serializing, and subscribing to annotations on messages via both REST and realtime APIs. Comprehensive tests and serialization logic for annotations and summaries are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Channel
participant RealtimeAnnotations
participant RestAnnotations
participant AblyBackend
Client->>Channel: publish(message)
Channel->>RealtimeAnnotations: publish(messageSerial, annotation)
RealtimeAnnotations->>Channel: sendProtocolMessage(annotation)
Channel->>AblyBackend: send ProtocolMessage (annotation)
AblyBackend-->>Channel: ProtocolMessage (annotation)
Channel->>RealtimeAnnotations: onAnnotation(protocolMessage)
RealtimeAnnotations->>Client: notify AnnotationListener
Client->>RestAnnotations: get(messageSerial)
RestAnnotations->>AblyBackend: HTTP GET /annotations
AblyBackend-->>RestAnnotations: annotation data
RestAnnotations-->>Client: PaginatedResult<Annotation>
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.1)lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (1)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
137-145:⚠️ Potential issueMissing field count update for annotations.
The
fieldCountcalculation doesn't include theannotationsfield, which will cause incorrect MessagePack header size when annotations are present.Add this to the field count calculation:
int fieldCount = 1; //action if(channel != null) ++fieldCount; if(msgSerial != null) ++fieldCount; if(messages != null) ++fieldCount; if(presence != null) ++fieldCount; if(auth != null) ++fieldCount; if(flags != 0) ++fieldCount; if(params != null) ++fieldCount; if(channelSerial != null) ++fieldCount; +if(annotations != null) ++fieldCount; packer.packMapHeader(fieldCount);Also applies to: 181-184
🧹 Nitpick comments (3)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java (2)
33-34: Consider reducing the test timeout duration.A 300-second timeout seems excessive for these tests. Consider reducing it to align with other test timeouts in the suite.
143-143: Use assertFalse for boolean negation.For better readability, use
assertFalseinstead ofassertTruewith negation.-assertTrue(!result.hasNext()); +assertFalse(result.hasNext());lib/src/main/java/io/ably/lib/types/Summary.java (1)
124-124: Make constructor visibility consistentThe constructor has package-private visibility while other constructors in the inner classes are public.
-SummaryTotal(int total) { +public SummaryTotal(int total) { this.total = total; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotation.java(1 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(2 hunks)lib/src/main/java/io/ably/lib/rest/RestAnnotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/Annotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationAction.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/ChannelMode.java(1 hunks)lib/src/main/java/io/ably/lib/types/Message.java(7 hunks)lib/src/main/java/io/ably/lib/types/MessageAction.java(1 hunks)lib/src/main/java/io/ably/lib/types/ProtocolMessage.java(6 hunks)lib/src/main/java/io/ably/lib/types/Summary.java(1 hunks)lib/src/main/java/io/ably/lib/util/Serialisation.java(2 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java (1)
lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotation.java (1)
RealtimeAnnotation(31-284)
🔇 Additional comments (26)
lib/src/main/java/io/ably/lib/types/MessageAction.java (1)
7-7:Details
✅ Verification successful
Verify all references to the old constant name have been updated.
The rename from
META_OCCUPANCYtoMETAis semantically appropriate for supporting broader metadata functionality including annotations. The ordinal position is preserved, maintaining backward compatibility for serialized data.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old constant name rg "META_OCCUPANCY" --type javaLength of output: 31
To ensure we catch any stray references (in Java or elsewhere), let’s broaden the search across all files:
🏁 Script executed:
#!/bin/bash rg "META_OCCUPANCY"Length of output: 19
All references to
META_OCCUPANCYhave been removedNo occurrences of the old constant name were found across the codebase after renaming to
META. The change preserves the original ordinal (3), maintains backward compatibility for serialized data, and aligns with the expanded metadata use case.Approving the change.
lib/src/main/java/io/ably/lib/util/Serialisation.java (2)
14-15: LGTM: Imports are appropriately added for new annotation types.The imports for
Annotation,AnnotationAction, andSummaryare correctly placed and will be used in the serializer registration below.Also applies to: 21-21
54-56: LGTM: Serializer registrations follow established patterns.The custom serializers for annotation-related types are correctly registered using the same pattern as existing types. This ensures proper JSON and MessagePack serialization support for the new annotation functionality.
lib/src/main/java/io/ably/lib/types/ChannelMode.java (1)
27-35:Details
✅ Verification successful
LGTM: New annotation channel modes are properly implemented.
The new
annotation_publishandannotation_subscribeenum constants follow the established patterns with consistent documentation and proper flag mapping. This correctly extends the channel capabilities for annotation functionality.
🏁 Script executed:
#!/bin/bash # Verify that the referenced flags exist in ProtocolMessage.Flag ast-grep --pattern $'enum Flag { $$$ annotation_publish$_, $$$ annotation_subscribe$_, $$$ }'Length of output: 104
🏁 Script executed:
#!/bin/bash # Locate the Java source file containing `enum Flag` and verify the new flags flag_file=$(rg -l "enum Flag" -t java | head -n1) echo "Found Flag enum in: $flag_file" if [[ -n "$flag_file" ]]; then echo "--- Checking for annotation_publish ---" rg -n "annotation_publish" "$flag_file" || echo "annotation_publish not found" echo "--- Checking for annotation_subscribe ---" rg -n "annotation_subscribe" "$flag_file" || echo "annotation_subscribe not found" else echo "No `enum Flag` definition found in Java sources." fiLength of output: 784
✅ Annotation Channel Modes Correctly Implemented
Verified that
ProtocolMessage.Flagdefines bothannotation_publish(line 71) andannotation_subscribe(line 72). The new enum constants inChannelMode.javaare named, documented, and mapped consistently with existing patterns.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (2)
41-46: LGTM! Clean implementation following established patterns.The annotations field follows the same pattern as the existing
presencefield with proper documentation and type declaration.
325-325: Constructor initialization looks correct.The RestAnnotation initialization with appropriate parameters (name, HTTP client, options, channel options) follows the expected pattern.
lib/src/main/java/io/ably/lib/types/Message.java (5)
92-98: Excellent documentation and field declaration.The summary field is well-documented with clear specification references and usage guidelines.
277-277: MsgPack serialization implementation is correct.The field count increment and serialization logic follows the established pattern for other fields in the Message class.
Also applies to: 321-324
360-361: MsgPack deserialization is properly implemented.The summary field reading logic is consistent with other field deserialization patterns.
537-543: JSON deserialization includes proper type checking.Good defensive programming with explicit type checking and meaningful error messages.
580-582: JSON serialization is correctly implemented.The summary field serialization uses the appropriate method and follows the pattern of other fields.
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (5)
16-16: Import added for dependency.Required import for RestAnnotation usage in constructor.
95-95: Annotations field follows established pattern.Consistent with the presence field declaration pattern.
1301-1304: Constructor initialization is well-structured.The RealtimeAnnotation initialization properly composes with RestAnnotation, creating a clean separation between realtime and REST functionality.
1371-1373: Message handling follows established pattern.The annotation action handling delegates appropriately to the annotations instance, consistent with how presence messages are handled.
1400-1409: Clean abstraction for protocol message sending.The sendProtocolMessage method provides a clean interface for sending protocol messages with completion callbacks, properly delegating to the connection manager.
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (5)
31-52: Action enum additions look good.The new protocol actions are added with proper sequential ordinal values, maintaining backward compatibility.
69-75: Flag enum additions are properly implemented.The new channel mode flags use unique offsets and properly document reserved values, maintaining compatibility with the protocol specification.
88-94: Correct update to acknowledgment requirements.The addition of
objectandannotationactions to the acknowledgment-required list is appropriate for data-bearing protocol messages.
122-122: Field addition follows established patterns.The
annotationsarray field is correctly added, consistent with howmessagesandpresencearrays are handled.
244-246: Deserialization logic correctly implemented.The annotations deserialization follows the established pattern and properly uses
AnnotationSerializer.lib/src/main/java/io/ably/lib/types/Annotation.java (5)
20-71: Well-structured annotation class with comprehensive documentation.The class properly extends
BaseMessageand includes all necessary fields with clear specification references.
76-123: MessagePack serialization correctly implemented.The method properly counts fields including parent class fields and serializes all annotation data correctly.
125-158: Robust MessagePack deserialization with proper error handling.Good use of
tryFindByOrdinalfor enum deserialization and proper delegation to parent class.
160-234: Comprehensive JSON serialization with proper validation.The implementation includes appropriate type checking and error handling, especially for the extras field validation.
236-247: Correct enum serialization implementation.The
ActionSerializerproperly handlesAnnotationActionenum serialization using ordinal values.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java
Outdated
Show resolved
Hide resolved
lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java
Outdated
Show resolved
Hide resolved
|
Seems PR has few conflicts |
ttypic
force-pushed
the
ECO-5065/annotation-support
branch
from
f07c984 to
fd48a6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java (8)
24-26:⚠️ Potential issueException handling issue persists from previous review
The IOException is still being wrapped in RuntimeException, which prevents callers from handling I/O errors gracefully. This was previously flagged and remains unaddressed.
- } catch (IOException e) { - throw new RuntimeException(e.getMessage(), e); - } + } catch (IOException e) { + throw new AblyException(e.getMessage(), e); + }
45-47:⚠️ Potential issueSilent failure on serialization error still present
The method continues execution after logging IOException, potentially returning corrupted data. This critical issue from previous review remains unresolved.
} catch (IOException e) { Log.e(TAG, e.getMessage(), e); + throw new RuntimeException("Failed to serialize annotation", e); }
69-69:⚠️ Potential issueCharset specification still missing
Creating String from bytes without specifying charset can lead to encoding issues. This was flagged in previous review but not fixed.
- return Serialisation.gson.fromJson(new String(packed), Annotation[].class); + return Serialisation.gson.fromJson(new String(packed, StandardCharsets.UTF_8), Annotation[].class);Add the import:
+import java.nio.charset.StandardCharsets;
90-95:⚠️ Potential issuePartial data issue from decode errors remains unaddressed
The method logs decode errors but continues processing, potentially returning incomplete data without caller awareness. This critical issue from previous review is still present.
Consider collecting decode errors and either throwing an exception or returning a result object that includes both successful annotations and errors.
17-27: Exception handling strategy needs improvementThe current implementation wraps IOException in RuntimeException, which prevents callers from handling I/O errors gracefully.
37-49: Handle IOException properly instead of just loggingThe method logs the exception but continues execution, potentially returning a corrupted or incomplete request body.
68-70: Specify charset when creating String from bytesCreating a String from bytes without specifying the charset can lead to encoding issues.
88-95: Consider failing fast on decode errorsThe current implementation logs decode errors but continues processing remaining annotations. This could lead to partial data being returned without the caller knowing some annotations failed to decode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(6 hunks)lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotation.java(1 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(2 hunks)lib/src/main/java/io/ably/lib/rest/RestAnnotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/Annotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationAction.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/Message.java(7 hunks)lib/src/main/java/io/ably/lib/types/ProtocolMessage.java(7 hunks)lib/src/main/java/io/ably/lib/types/Summary.java(1 hunks)lib/src/main/java/io/ably/lib/util/Serialisation.java(2 hunks)lib/src/test/java/io/ably/lib/test/common/Setup.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java(2 hunks)lib/src/test/resources/ably-common(1 hunks)lib/src/test/resources/local/testAppSpec.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/src/test/resources/ably-common
- lib/src/test/java/io/ably/lib/test/common/Setup.java
🚧 Files skipped from review as they are similar to previous changes (12)
- lib/src/main/java/io/ably/lib/util/Serialisation.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java
- lib/src/main/java/io/ably/lib/rest/ChannelBase.java
- lib/src/main/java/io/ably/lib/types/Message.java
- lib/src/main/java/io/ably/lib/types/ProtocolMessage.java
- lib/src/main/java/io/ably/lib/types/AnnotationAction.java
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
- lib/src/main/java/io/ably/lib/types/Annotation.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java
- lib/src/main/java/io/ably/lib/rest/RestAnnotation.java
- lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotation.java
- lib/src/main/java/io/ably/lib/types/Summary.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (10)
lib/src/test/resources/local/testAppSpec.json (6)
2-19: Test configuration updates look appropriateThe addition of new capability patterns (
"cansubscribe:*","pushenabled:*","pushenabled:admin:*") and the revocable tokens configuration properly support the new annotation functionality.
20-24: Mutable namespace addition aligns with annotation featuresThe new
"mutable"namespace with"mutableMessages": truecorrectly supports the message annotation capabilities being introduced.
25-51: Channel consolidation improves test organizationConsolidating multiple presence channels into a single
"persisted:presence_fixtures"channel with diverse presence data provides better test coverage for annotation scenarios.
2-19: LGTM!The updated keys configuration properly supports the new annotation and push capabilities with well-formed capability patterns.
20-24: LGTM!The addition of the "mutable" namespace with mutableMessages support appropriately enables testing of annotation features that require message mutability.
25-51: LGTM!The consolidated channel configuration provides comprehensive test coverage with diverse presence data types and encoding scenarios, supporting robust testing of the annotation feature.
lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java (4)
29-35: LGTM!The method correctly handles IOException and has a clean implementation for reading annotation arrays.
51-53: LGTM!Simple and correct implementation for creating JSON request bodies.
55-57: LGTM!Clean factory method implementation.
59-66: LGTM!Proper exception handling using AblyException for I/O errors.
ttypic
force-pushed
the
ECO-5065/annotation-support
branch
from
fd48a6d to
510e69d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java (5)
17-27: Add input validation and improve exception handling
29-35: Add null safety check for unpacker parameter
88-96: Consider failing fast on decode errors
37-49: 🛠️ Refactor suggestionHandle exception properly and add input validation
The method has two issues:
- Logs IOException but continues execution, potentially returning corrupted data (as noted in previous review)
- Missing null check for annotations parameter
Add null validation:
public static HttpCore.RequestBody asMsgpackRequest(Annotation[] annotations) { + if (annotations == null) { + throw new IllegalArgumentException("Annotations array cannot be null"); + } ByteArrayOutputStream out = new ByteArrayOutputStream();
68-70: 🛠️ Refactor suggestionAdd charset specification and null check
Two issues need to be addressed:
- String creation from bytes without charset specification (as noted in previous review)
- Missing null check for packed parameter
Add null validation:
public static Annotation[] readMessagesFromJson(byte[] packed) throws MessageDecodeException { + if (packed == null) { + throw new IllegalArgumentException("Packed data cannot be null"); + } return Serialisation.gson.fromJson(new String(packed, StandardCharsets.UTF_8), Annotation[].class); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(6 hunks)lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotation.java(1 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(2 hunks)lib/src/main/java/io/ably/lib/rest/RestAnnotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/Annotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationAction.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/Message.java(7 hunks)lib/src/main/java/io/ably/lib/types/ProtocolMessage.java(7 hunks)lib/src/main/java/io/ably/lib/types/Summary.java(1 hunks)lib/src/main/java/io/ably/lib/util/Serialisation.java(2 hunks)lib/src/test/java/io/ably/lib/test/common/Setup.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java(2 hunks)lib/src/test/resources/ably-common(1 hunks)lib/src/test/resources/local/testAppSpec.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/src/main/java/io/ably/lib/util/Serialisation.java
- lib/src/test/resources/local/testAppSpec.json
🚧 Files skipped from review as they are similar to previous changes (12)
- lib/src/test/resources/ably-common
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java
- lib/src/main/java/io/ably/lib/rest/ChannelBase.java
- lib/src/test/java/io/ably/lib/test/common/Setup.java
- lib/src/main/java/io/ably/lib/types/AnnotationAction.java
- lib/src/main/java/io/ably/lib/types/ProtocolMessage.java
- lib/src/main/java/io/ably/lib/types/Message.java
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
- lib/src/main/java/io/ably/lib/types/Annotation.java
- lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotation.java
- lib/src/main/java/io/ably/lib/rest/RestAnnotation.java
- lib/src/main/java/io/ably/lib/types/Summary.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check (24)
- GitHub Check: build
- GitHub Check: check (19)
🔇 Additional comments (4)
lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java (2)
55-57: LGTM - Clean factory methodSimple and clear factory method implementation.
59-66: LGTM - Proper exception handlingGood exception handling that converts IOException to AblyException appropriately.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java (2)
33-90: LGTM - Improved synchronizationExcellent improvement! The test now uses proper synchronization with
CompletionWaiterandMessageWaiterinstead ofThread.sleep, making it more reliable and faster. The test logic comprehensively covers annotation publishing and subscription for both realtime and REST channels.
160-186: LGTM - Well-designed test helperExcellent test helper class that properly sets up channels with annotation modes and ensures proper cleanup. The channel configuration with annotation publish/subscribe modes is appropriate for testing annotation functionality.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java
Show resolved
Hide resolved
ttypic
force-pushed
the
ECO-5065/annotation-support
branch
from
510e69d to
3c5ed0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java (4)
17-27: Add null validation for parametersThe method should validate that both the annotations array and packer parameters are not null to prevent NullPointerExceptions and provide clearer error messages.
24-26: Reconsider exception handling strategyWrapping IOException in RuntimeException removes the ability for callers to handle I/O errors gracefully. Consider letting the IOException propagate or using AblyException.
29-35: Add null safety checkThe method doesn't validate the unpacker parameter and could fail with unclear error messages if null is passed.
51-53: Add input validation for null annotationsThe method should validate that the annotations parameter is not null to provide clearer error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitmodules(0 hunks)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(6 hunks)lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotation.java(1 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(2 hunks)lib/src/main/java/io/ably/lib/rest/RestAnnotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/Annotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationAction.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/Message.java(7 hunks)lib/src/main/java/io/ably/lib/types/ProtocolMessage.java(7 hunks)lib/src/main/java/io/ably/lib/types/Summary.java(1 hunks)lib/src/main/java/io/ably/lib/util/Serialisation.java(2 hunks)lib/src/test/java/io/ably/lib/test/common/Setup.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java(2 hunks)lib/src/test/resources/ably-common(0 hunks)lib/src/test/resources/local/testAppSpec.json(1 hunks)
💤 Files with no reviewable changes (2)
- lib/src/test/resources/ably-common
- .gitmodules
🚧 Files skipped from review as they are similar to previous changes (14)
- lib/src/test/resources/local/testAppSpec.json
- lib/src/main/java/io/ably/lib/util/Serialisation.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java
- lib/src/test/java/io/ably/lib/test/common/Setup.java
- lib/src/main/java/io/ably/lib/rest/ChannelBase.java
- lib/src/main/java/io/ably/lib/types/Message.java
- lib/src/main/java/io/ably/lib/types/AnnotationAction.java
- lib/src/main/java/io/ably/lib/types/ProtocolMessage.java
- lib/src/main/java/io/ably/lib/types/Annotation.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java
- lib/src/main/java/io/ably/lib/types/Summary.java
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
- lib/src/main/java/io/ably/lib/rest/RestAnnotation.java
- lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotation.java
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest
- GitHub Check: check (24)
- GitHub Check: check
- GitHub Check: check (29)
- GitHub Check: build
- GitHub Check: check (19)
- GitHub Check: check (21)
ttypic
force-pushed
the
ECO-5065/annotation-support
branch
from
3c5ed0e to
a8191ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (10)
lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java (10)
17-27: 🛠️ Refactor suggestionAdd input validation
The method should validate that the annotations array and packer are not null to provide clearer error messages.
public static void writeMsgpackArray(Annotation[] annotations, MessagePacker packer) { + if (annotations == null) { + throw new IllegalArgumentException("Annotations array cannot be null"); + } + if (packer == null) { + throw new IllegalArgumentException("MessagePacker cannot be null"); + } try {
24-26:⚠️ Potential issueReconsider exception handling strategy
Wrapping IOException in RuntimeException removes the ability for callers to handle I/O errors gracefully. Consider letting the IOException propagate or using AblyException.
-} catch (IOException e) { - throw new RuntimeException(e.getMessage(), e); -} +} catch (IOException e) { + throw new AblyException(e.getMessage(), e); +}
29-35: 🛠️ Refactor suggestionAdd null safety check
The method doesn't validate the unpacker parameter and could fail with unclear error messages if null is passed.
public static Annotation[] readMsgpackArray(MessageUnpacker unpacker) throws IOException { + if (unpacker == null) { + throw new IllegalArgumentException("MessageUnpacker cannot be null"); + } int count = unpacker.unpackArrayHeader();
37-49:⚠️ Potential issueAdd null validation and improve error handling
The method lacks null validation for the annotations parameter and has inadequate error handling. The current implementation logs IOException but continues execution, potentially returning corrupted data.
public static HttpCore.RequestBody asMsgpackRequest(Annotation[] annotations) { + if (annotations == null) { + throw new IllegalArgumentException("Annotations array cannot be null"); + } ByteArrayOutputStream out = new ByteArrayOutputStream(); try { MessagePacker packer = Serialisation.msgpackPackerConfig.newPacker(out); int count = annotations.length; packer.packArrayHeader(count); for (Annotation annotation : annotations) annotation.writeMsgpack(packer); packer.flush(); } catch (IOException e) { Log.e(TAG, e.getMessage(), e); + throw new RuntimeException("Failed to serialize annotations", e); } return new HttpUtils.ByteArrayRequestBody(out.toByteArray(), "application/x-msgpack"); }
51-53: 🛠️ Refactor suggestionAdd input validation for null annotations
The method should validate that the annotations parameter is not null to provide clearer error messages.
public static HttpCore.RequestBody asJsonRequest(Annotation[] annotations) { + if (annotations == null) { + throw new IllegalArgumentException("Annotations array cannot be null"); + } return new HttpUtils.JsonRequestBody(Serialisation.gson.toJson(annotations)); }
55-57: 🛠️ Refactor suggestionAdd null validation for channelOptions parameter
The method should validate that the channelOptions parameter is not null before passing it to the AnnotationBodyHandler constructor.
public static HttpCore.BodyHandler<Annotation> getAnnotationResponseHandler(ChannelOptions channelOptions) { + if (channelOptions == null) { + throw new IllegalArgumentException("ChannelOptions cannot be null"); + } return new AnnotationBodyHandler(channelOptions); }
59-66: 🛠️ Refactor suggestionAdd null validation for packed parameter
The method should validate that the packed byte array is not null before processing to provide clearer error messages.
public static Annotation[] readMsgpack(byte[] packed) throws AblyException { + if (packed == null) { + throw new IllegalArgumentException("Packed byte array cannot be null"); + } try { MessageUnpacker unpacker = Serialisation.msgpackUnpackerConfig.newUnpacker(packed); return readMsgpackArray(unpacker); } catch (IOException ioe) { throw AblyException.fromThrowable(ioe); } }
68-70:⚠️ Potential issueAdd null validation and specify charset
The method needs null validation for the packed parameter and should specify charset when creating String from bytes to avoid encoding issues.
public static Annotation[] readMessagesFromJson(byte[] packed) throws MessageDecodeException { + if (packed == null) { + throw new IllegalArgumentException("Packed byte array cannot be null"); + } - return Serialisation.gson.fromJson(new String(packed), Annotation[].class); + return Serialisation.gson.fromJson(new String(packed, StandardCharsets.UTF_8), Annotation[].class); }You'll need to import
java.nio.charset.StandardCharsets.
76-78: 🛠️ Refactor suggestionAdd null validation in constructor
The constructor should validate that channelOptions is not null to prevent potential issues in the handleResponseBody method.
AnnotationBodyHandler(ChannelOptions channelOptions) { + if (channelOptions == null) { + throw new IllegalArgumentException("ChannelOptions cannot be null"); + } this.channelOptions = channelOptions; }
81-101:⚠️ Potential issueAdd parameter validation and improve decode error handling
The method lacks validation for contentType and body parameters. Additionally, the current decode error handling logs errors but continues processing, potentially returning partial data without the caller knowing some annotations failed to decode.
@Override public Annotation[] handleResponseBody(String contentType, byte[] body) throws AblyException { + if (contentType == null) { + throw new IllegalArgumentException("Content type cannot be null"); + } + if (body == null) { + throw new IllegalArgumentException("Response body cannot be null"); + } try { Annotation[] annotations = null; if ("application/json".equals(contentType)) annotations = readMessagesFromJson(body); else if ("application/x-msgpack".equals(contentType)) annotations = readMsgpack(body); if (annotations != null) { + List<MessageDecodeException> decodeErrors = new ArrayList<>(); for (Annotation annotation : annotations) { try { if (annotation.data != null) annotation.decode(channelOptions); } catch (MessageDecodeException e) { Log.e(TAG, e.errorInfo.message); + decodeErrors.add(e); } } + if (!decodeErrors.isEmpty()) { + throw new AblyException("Failed to decode " + decodeErrors.size() + " annotations", 400, 40000); + } } return annotations; } catch (MessageDecodeException e) { throw AblyException.fromThrowable(e); } }
🧹 Nitpick comments (1)
lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotations.java (1)
34-42: Consider using ConcurrentHashMap for better thread safetyWhile all access to
typeListenersappears to be within synchronized methods, usingConcurrentHashMapwould provide additional safety and potentially better performance under concurrent access.private final ChannelBase channel; private final RestAnnotations restAnnotations; private final AnnotationMulticaster listeners = new AnnotationMulticaster(); -private final Map<String, AnnotationMulticaster> typeListeners = new HashMap<>(); +private final Map<String, AnnotationMulticaster> typeListeners = new ConcurrentHashMap<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitmodules(0 hunks)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(6 hunks)lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotations.java(1 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(2 hunks)lib/src/main/java/io/ably/lib/rest/RestAnnotations.java(1 hunks)lib/src/main/java/io/ably/lib/types/Annotation.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationAction.java(1 hunks)lib/src/main/java/io/ably/lib/types/AnnotationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/Message.java(7 hunks)lib/src/main/java/io/ably/lib/types/ProtocolMessage.java(7 hunks)lib/src/main/java/io/ably/lib/types/Summary.java(1 hunks)lib/src/main/java/io/ably/lib/util/Serialisation.java(2 hunks)lib/src/test/java/io/ably/lib/test/common/Setup.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java(1 hunks)lib/src/test/resources/ably-common(0 hunks)lib/src/test/resources/local/testAppSpec.json(1 hunks)
💤 Files with no reviewable changes (2)
- lib/src/test/resources/ably-common
- .gitmodules
🚧 Files skipped from review as they are similar to previous changes (12)
- lib/src/test/java/io/ably/lib/test/common/Setup.java
- lib/src/test/resources/local/testAppSpec.json
- lib/src/main/java/io/ably/lib/util/Serialisation.java
- lib/src/main/java/io/ably/lib/rest/ChannelBase.java
- lib/src/main/java/io/ably/lib/types/Message.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java
- lib/src/main/java/io/ably/lib/types/AnnotationAction.java
- lib/src/main/java/io/ably/lib/types/ProtocolMessage.java
- lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
- lib/src/main/java/io/ably/lib/types/Annotation.java
- lib/src/main/java/io/ably/lib/types/Summary.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: check-rest-okhttp
- GitHub Check: build
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check
ttypic
force-pushed
the
ECO-5065/annotation-support
branch
from
a8191ed to
93d9b31
Compare
lib/src/test/java/io/ably/lib/test/realtime/RealtimeAnnotationsTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
lib/src/test/java/io/ably/lib/test/util/AblyCommonsReader.java (1)
12-14: Consider making the base URL configurable and using a stable reference.The hard-coded URL points to the main branch which could be unstable. Consider:
- Making the base URL configurable via system properties
- Using a stable tag/release instead of main branch
- Adding validation for the URL format
public class AblyCommonsReader { - private static final String BASE_URL = "https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/"; + private static final String BASE_URL = System.getProperty("ably.commons.base.url", + "https://raw.githubusercontent.com/ably/ably-common/refs/tags/v1.0.0/"); private static Gson gson = new Gson();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java(5 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java(3 hunks)lib/src/test/java/io/ably/lib/test/util/AblyCommonsReader.java(1 hunks)lib/src/test/java/io/ably/lib/util/CryptoMessageTest.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/src/test/java/io/ably/lib/util/CryptoMessageTest.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java (1)
lib/src/test/java/io/ably/lib/test/util/AblyCommonsReader.java (1)
AblyCommonsReader(12-52)
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (1)
lib/src/test/java/io/ably/lib/test/util/AblyCommonsReader.java (1)
AblyCommonsReader(12-52)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check-liveobjects
- GitHub Check: build
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check
🔇 Additional comments (3)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java (1)
63-63: File path constant updated correctly.The path constant properly reflects the new location without the "ably-common/" prefix.
lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java (2)
45-45: LGTM - Import addition is correct.The import for
AblyCommonsReaderis properly added and aligns with the refactoring to use remote JSON fixture loading.
78-78:Details
✅ Verification successful
Verify the file path change is consistent.
The file path has been updated from
"ably-common/test-resources/presence-messages-encoding.json"to"test-resources/presence-messages-encoding.json", removing the"ably-common/"prefix. This change aligns with fetching directly from the Ably Commons repository.Verify that this path change is consistent across all test files that use similar fixtures:
🏁 Script executed:
#!/bin/bash # Search for other test files that might reference the old path pattern rg "ably-common/test-resources.*\.json" --type java rg "test-resources.*encoding\.json" --type javaLength of output: 451
All test fixture paths updated consistently
- Both
RealtimePresenceTest.javaandRealtimeMessageTest.javanow reference JSON fixtures undertest-resources/...without theably-common/prefix.- No remaining occurrences of
ably-common/test-resourceswere found in any Java test files.
ttypic
force-pushed
the
ECO-5065/annotation-support
branch
from
594e97a to
49978fa
Compare
lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotations.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Apart from review comments, it seems publish, delete and get annotation methods have first param as Message overloads. This is true for both RestAnnotations and RealtimeAnnotations. Seems same exists in case of ably-js and allows to extract serial from message.
Since a new feature with public API has been added, you can check to update the corresponding README. |
ttypic
force-pushed
the
ECO-5065/annotation-support
branch
from
49978fa to
680d474
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
lib/src/main/java/io/ably/lib/types/Summary.java (3)
63-63: Use TypeToken for proper generic type handlingUsing
Map.classloses generic type information. This could lead to unchecked cast warnings or runtime ClassCastException.
74-74: Use TypeToken for proper generic type handlingUsing
List.classloses generic type information.
117-119: Add null check before using MessagePackerThe method should validate that the packer parameter is not null to prevent potential NPE.
lib/src/main/java/io/ably/lib/rest/RestAnnotations.java (3)
44-49: Add null validation in constructorThe constructor should validate input parameters to prevent NullPointerException later in the code.
173-176: Remove throws declaration from async methodAsync methods typically don't throw checked exceptions. Any exceptions should be passed to the callback instead.
241-244: Avoid mutating input parametersModifying the annotation parameter is a side effect that can surprise callers. Consider creating a copy or using a builder pattern.
lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotations.java (2)
135-139: Avoid mutating input parametersSimilar to RestAnnotations, modifying the annotation parameter is a side effect that can surprise callers.
364-372: Fix inconsistent null handling between subscribe and unsubscribeThe
subscribeImplmethod converts null type to empty string, butunsubscribeImpldoesn't apply the same conversion, causing a mismatch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
lib/src/main/java/io/ably/lib/realtime/RealtimeAnnotations.java(1 hunks)lib/src/main/java/io/ably/lib/rest/RestAnnotations.java(1 hunks)lib/src/main/java/io/ably/lib/types/Summary.java(1 hunks)lib/src/main/java/io/ably/lib/types/SummaryClientIdCounts.java(1 hunks)lib/src/main/java/io/ably/lib/types/SummaryClientIdList.java(1 hunks)lib/src/main/java/io/ably/lib/types/SummaryTotal.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java(5 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java(3 hunks)lib/src/test/java/io/ably/lib/test/util/AblyCommonsReader.java(1 hunks)lib/src/test/java/io/ably/lib/types/SummaryTest.java(1 hunks)lib/src/test/java/io/ably/lib/util/CryptoMessageTest.java(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- lib/src/main/java/io/ably/lib/types/SummaryTotal.java
- lib/src/main/java/io/ably/lib/types/SummaryClientIdList.java
- lib/src/main/java/io/ably/lib/types/SummaryClientIdCounts.java
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/src/test/java/io/ably/lib/util/CryptoMessageTest.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeMessageTest.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimePresenceTest.java
- lib/src/test/java/io/ably/lib/test/util/AblyCommonsReader.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/src/test/java/io/ably/lib/types/SummaryTest.java (1)
lib/src/main/java/io/ably/lib/types/Summary.java (1)
Summary(30-142)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check (21)
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: build
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Lgtm.
You may optionally check whether the README needs updating—for example, by adding a snippet that demonstrates the use of annotations
Add support for managing message annotations via Rest and Realtime APIs
Introduced new classes and functionality to enable creating, retrieving, and deleting annotations on messages, both synchronously and asynchronously. Extended protocol support to handle annotation operations and added serialization/deserialization logic for annotations and summaries.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores