[AIT-99] feat: protocol v5 + message appends #1182
Conversation
WalkthroughUpdate/delete APIs now return UpdateDeleteResult and async methods use Callback; new appendMessage APIs and getMessageVersions (REST) added; MessageOperationSerializer removed; MessageSerializer gained single-message helpers; UpdateDeleteResult and Listeners adapter added; protocol version bumped to "5". Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client App
participant Channel as ChannelBase
participant EditMixin as MessageEditsMixin
participant Http as HttpCore
participant Server as Ably REST
App->>Channel: updateMessage(msg[, op])
Channel->>EditMixin: updateMessage(http, msg, op)
EditMixin->>Http: PATCH /channels/:name/messages (single-msg body)
Http->>Server: HTTP PATCH (JSON / MsgPack)
Server-->>Http: 200 + body (content-type + bytes)
Http->>EditMixin: response bytes + content-type
EditMixin->>EditMixin: parse UpdateDeleteResult via BodyHandler
EditMixin-->>Channel: return UpdateDeleteResult
Channel-->>App: return UpdateDeleteResult
alt async
App->>Channel: updateMessageAsync(msg, Callback)
Channel->>EditMixin: updateMessageAsync(..., Callback)
EditMixin->>Http: async PATCH...
Server-->>Http: response
Http->>EditMixin: response
EditMixin->>Callback: onSuccess(UpdateDeleteResult)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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/1182/features
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
9153b20 to
44c71e5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1182/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: 2
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
77-85: Silent null return on IOException.The
write(Message)method returnsnullonIOException, which follows the existing pattern inwriteMsgpackArray. However,asSingleMsgpackRequest(line 51) passes this directly toByteArrayRequestBody, which could cause issues downstream if serialization fails.Consider throwing
AblyExceptioninstead for consistency with other methods that usethrows AblyException:🔎 Suggested improvement:
-public static byte[] write(Message message) { +public static byte[] write(Message message) throws AblyException { try { ByteArrayOutputStream out = new ByteArrayOutputStream(); MessagePacker packer = Serialisation.msgpackPackerConfig.newPacker(out); message.writeMsgpack(packer); packer.flush(); return out.toByteArray(); - } catch(IOException e) { return null; } + } catch(IOException e) { + throw AblyException.fromThrowable(e); + } }lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
20-22: Specify UTF-8 charset explicitly.
new String(packed)uses platform default charset. JSON should be decoded as UTF-8 for consistency and portability.🔎 Suggested fix:
+import java.nio.charset.StandardCharsets; + public static UpdateDeleteResult readFromJson(byte[] packed) throws MessageDecodeException { - return Serialisation.gson.fromJson(new String(packed), UpdateDeleteResult.class); + return Serialisation.gson.fromJson(new String(packed, StandardCharsets.UTF_8), UpdateDeleteResult.class); }lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
143-165: Minor Javadoc clarification suggestion.The Javadoc states "Appends message text to the end of the message" but the
Message.datafield can contain other types (byte arrays, JSON objects, etc.). Consider updating to "Appends data to the message" for accuracy, or clarify if only text appending is supported.🔎 Suggested Javadoc update:
/** - * Appends message text to the end of the message. + * Appends data to the end of the message. * * @param message A {@link Message} object containing the serial identifier and data to append.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java(3 hunks)lib/src/main/java/io/ably/lib/transport/Defaults.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageAction.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java(0 hunks)lib/src/main/java/io/ably/lib/types/MessageSerializer.java(3 hunks)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java
🧰 Additional context used
🧬 Code graph analysis (5)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java (1)
liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt (1)
id(272-345)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (6)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
MessageSerializer(23-242)lib/src/main/java/io/ably/lib/types/MessageVersion.java (1)
MessageVersion(24-232)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(10-71)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(10-71)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(10-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-rest-okhttp
- GitHub Check: check (21)
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-realtime-okhttp
- GitHub Check: build
🔇 Additional comments (15)
lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java (1)
84-84: LGTM!The test expectation correctly updated to match the protocol version bump to "5".
lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java (1)
83-84: LGTM!The WebSocket version parameter test expectation correctly updated to match protocol version "5".
lib/src/main/java/io/ably/lib/types/MessageAction.java (1)
8-9: New message action types added.The new
MESSAGE_SUMMARY(ordinal 4) andMESSAGE_APPEND(ordinal 5) actions are correctly added. Note that ordinal values are significant for protocol serialization—ensure these match the server-side protocol specification for version 5.lib/src/main/java/io/ably/lib/types/MessageSerializer.java (2)
50-52: LGTM!The
asSingleMsgpackRequestmethod correctly serializes a single message for the new message operations API.
151-153: LGTM!The
asSingleJsonRequestmethod correctly provides JSON serialization for single messages, complementing the existing array-basedasJsonRequest.lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (2)
10-18: LGTM!The class structure with immutable
versionSerialfield is clean and appropriate for representing update/delete/append operation results.
51-70: LGTM!The body handler implementation follows the established pattern in the codebase for handling both JSON and MessagePack content types.
lib/src/main/java/io/ably/lib/transport/Defaults.java (1)
15-15: Verify that Ably service supports protocol version 5 before deployment.This change declares support for protocol version 5, but current Ably SDKs (ably-java and ably-js) only show support up to protocol version 4. Confirm with the Ably team that version 5 is available on their service and document the deployment coordination required.
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (2)
167-205: LGTM! Well-structured shared implementation for update/delete/append operations.The consolidation of update, delete, and append into a single
updateMessageImplmethod with an action parameter is a clean design. The method properly:
- Validates the serial identifier
- Constructs the request with action and version metadata
- Handles both binary and JSON protocols
- Returns the
UpdateDeleteResultwith proper error handling
175-183: LGTM! Proper construction of the augmented message with version metadata.The
MessageVersionis correctly initialized with operation metadata only. Theserialandtimestampfields remain unset as they are populated by the server in the response.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
1218-1262: LGTM! Consistent API surface for update operations.The update methods properly delegate to
messageEditsMixinand maintain consistent overload patterns for both sync and async variants.
1274-1318: LGTM! Delete operations follow the same consistent pattern.
1320-1366: LGTM! Append operations correctly mirror update/delete patterns.The new append methods are consistent with the existing update and delete API surface, properly delegating to
messageEditsMixin.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (2)
356-504: LGTM! REST ChannelBase maintains API consistency with Realtime ChannelBase.All update, delete, and append operations are properly implemented with:
- Correct return types (
UpdateDeleteResult)- Consistent async callback patterns (
Callback<UpdateDeleteResult>)- Proper delegation to
MessageEditsMixin- Matching overload patterns for optional
MessageOperationparameter
519-532: LGTM! Message versioning API properly exposes paginated history.The
getMessageVersionsandgetMessageVersionsAsyncmethods correctly delegate to the mixin for retrieving historical message versions.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
staging/pull/1182/javadoc
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
33-48: Missing skip for unknown fields corrupts MessagePack stream.This issue was flagged in a previous review but doesn't appear to be fixed. When an unknown field is encountered, the code does not consume (skip) its value before continuing to the next field. This will cause incorrect parsing of subsequent fields or an
IOException.🔎 Apply this diff to fix the issue:
public static UpdateDeleteResult readMsgpack(MessageUnpacker unpacker) throws IOException { int fieldCount = unpacker.unpackMapHeader(); String versionSerial = null; for(int i = 0; i < fieldCount; i++) { String fieldName = unpacker.unpackString().intern(); MessageFormat fieldFormat = unpacker.getNextFormat(); if(fieldFormat.equals(MessageFormat.NIL)) { unpacker.unpackNil(); continue; } if(fieldName.equals(VERSION_SERIAL)) { versionSerial = unpacker.unpackString(); + } else { + unpacker.skipValue(); } } return new UpdateDeleteResult(versionSerial); }
🧹 Nitpick comments (5)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
50-52: Potential null propagation if serialization fails.The
asSingleMsgpackRequestmethod passes the result ofwrite(message)directly toByteArrayRequestBody. Ifwrite()fails due to anIOException, it returnsnull, which would create a request body with a null byte array.This follows the existing pattern in
writeMsgpackArray(line 65), but consider whether this silent failure is intentional or should propagate the exception.🔎 Consider propagating the exception:
- public static HttpCore.RequestBody asSingleMsgpackRequest(Message message) throws AblyException { - return new HttpUtils.ByteArrayRequestBody(write(message), "application/x-msgpack"); + public static HttpCore.RequestBody asSingleMsgpackRequest(Message message) throws AblyException { + byte[] packed = write(message); + if (packed == null) { + throw AblyException.fromThrowable(new IOException("Failed to serialize message")); + } + return new HttpUtils.ByteArrayRequestBody(packed, "application/x-msgpack"); }lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
55-70: Consider handling unknown content types explicitly.If the
contentTypeis neitherapplication/jsonnorapplication/x-msgpack, the method returns[null]. This matches the pattern inMessageBodyHandler, but callers likeMessageEditsMixin.updateMessageImplwill then throw due to missingversionSerial. Consider adding an explicit check for clarity.🔎 Optional improvement for explicit handling:
public UpdateDeleteResult[] handleResponseBody(String contentType, byte[] body) throws AblyException { try { UpdateDeleteResult updateDeleteResult = null; if("application/json".equals(contentType)) updateDeleteResult = readFromJson(body); else if("application/x-msgpack".equals(contentType)) updateDeleteResult = readMsgpack(body); + else + throw AblyException.fromThrowable(new Exception("Unknown content type: " + contentType)); return new UpdateDeleteResult[] { updateDeleteResult }; } catch (MessageDecodeException e) { throw AblyException.fromThrowable(e); } }lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
458-504: Consider clarifying the Javadoc for data type flexibility.The Javadoc states "Appends message text to the end of the message" but the
Messageobject can contain various data types, not just text. Consider updating the documentation to be more accurate about what data types are supported for append operations.🔎 Suggested documentation update:
/** - * Appends message text to the end of the message. + * Appends data to the end of the message. * * @param message A {@link Message} object containing the serial identifier and data to append.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
1320-1366: Same documentation nit as REST ChannelBase.The Javadoc describes "Appends message text" but the operation supports various data types. Consider updating for consistency with the REST channel.
🔎 Suggested documentation update:
/** - * Appends message text to the end of the message. + * Appends data to the end of the message. * * @param message A {@link Message} object containing the serial identifier and data to append.
1580-1581: Redundant initialization ofmessageEditsMixin.The constructor calls
this.setOptions(options)at line 1567, which already initializesmessageEditsMixinat line 1504. The assignment at line 1580 is therefore redundant and can be removed.🔎 Suggested fix:
this.annotations = new RealtimeAnnotations( this, new RestAnnotations(name, ably.http, ably.options, options) ); - this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java(3 hunks)lib/src/main/java/io/ably/lib/transport/Defaults.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageAction.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java(0 hunks)lib/src/main/java/io/ably/lib/types/MessageSerializer.java(3 hunks)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java(1 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java(1 hunks)lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/main/java/io/ably/lib/types/MessageAction.java
- lib/src/main/java/io/ably/lib/transport/Defaults.java
🧰 Additional context used
🧬 Code graph analysis (5)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (3)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(10-71)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (6)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
MessageSerializer(23-242)lib/src/main/java/io/ably/lib/types/MessageVersion.java (1)
MessageVersion(24-232)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(10-71)lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(10-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check-liveobjects
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check
🔇 Additional comments (16)
lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java (1)
84-84: LGTM!The test assertion correctly updated to expect protocol version "5", aligning with the protocol version bump in
Defaults.java.lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java (1)
83-84: LGTM!The WebSocket version parameter assertion correctly updated to "5", consistent with the protocol version bump.
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (2)
77-85: LGTM with note on existing pattern.The
write(Message)method correctly mirrors the existingwriteMsgpackArraypattern. The silentnullreturn onIOExceptionis consistent with the codebase style, though it's a known trade-off.
151-153: LGTM!Clean single-message JSON serialization using Gson, consistent with the existing
asJsonRequest(Message[])pattern.lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
10-18: LGTM!Clean immutable result type with public final field for
versionSerial.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (4)
143-165: LGTM! New append functionality correctly implemented.The
appendMessageandappendMessageAsyncmethods are cleanly implemented, reusing the sharedupdateMessageImplwithMessageAction.MESSAGE_APPEND. The Javadoc is clear about the operation's purpose.
175-183: LGTM! Message augmentation logic is sound.The implementation correctly:
- Creates a new
Messagewith copied fields to avoid mutating the input- Sets the appropriate
actionfor the operation type- Populates
MessageVersionwith operation metadata (server will setserial/timestamp)- Encodes the message before serialization
196-201: LGTM! Error handling is appropriate.The response handler correctly validates the result and throws a descriptive error if
versionSerialis missing from the response.
99-141: LGTM! API changes are consistent and well-documented.The transition from
void/CompletionListenertoUpdateDeleteResult/Callback<UpdateDeleteResult>is applied consistently acrossupdateMessage,deleteMessage, and their async variants. The Javadoc correctly documents the new return types.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (3)
20-20: LGTM!The import for
UpdateDeleteResultis correctly added to support the new return types for update/delete/append operations.
356-400: LGTM!The
updateMessagemethods are correctly updated to returnUpdateDeleteResultand useCallback<UpdateDeleteResult>for async variants. The delegation tomessageEditsMixinis consistent, and the Javadoc accurately reflects the new return type.
412-456: LGTM!The
deleteMessagemethods follow the same pattern asupdateMessage, correctly returningUpdateDeleteResultand using callbacks for async variants.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (4)
46-46: LGTM!The import for
UpdateDeleteResultis correctly added.
106-106: LGTM!The
volatilemodifier ensures thread-safe visibility whenmessageEditsMixinis reassigned insetOptions(). Since the mixin is treated as immutable after construction and only replaced atomically, this is appropriate.
1218-1262: LGTM!The
updateMessagemethods are correctly updated, consistent with the RESTChannelBaseimplementation.
1274-1318: LGTM!The
deleteMessagemethods follow the same pattern and are correctly implemented.
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
44c71e5 to
95f332a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1182/features
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
95f332a to
9c5ac4a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1182/features
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java:
- Around line 28-30: The bytes-to-string conversion in
UpdateDeleteResult.readFromJson currently uses the platform default charset;
change the String construction to explicitly use UTF-8 (e.g., new String(packed,
StandardCharsets.UTF_8)) before calling Serialisation.gson.fromJson, and add the
StandardCharsets import if missing so JSON deserialization is deterministic
across platforms.
- Around line 68-79: The method handleResponseBody in UpdateDeleteResult
currently returns an array containing null when contentType is neither
"application/json" nor "application/x-msgpack"; change it to explicitly treat
unknown content types as an error by throwing an AblyException (or
AblyException.fromThrowable with a suitable cause) instead of returning null,
include the unexpected contentType in the error message for diagnostics, and
ensure the method still returns a non-null UpdateDeleteResult[] on success
(calling readFromJson/readMsgpack) so callers never receive an array with a null
element.
🧹 Nitpick comments (4)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (2)
196-201: Error message should reflect the actual operation type.The error message at line 200 says "update message response" but this method handles update, delete, and append operations. Consider making the error message more generic or including the action type.
🔎 Suggested fix
- throw AblyException.fromErrorInfo(new ErrorInfo("No versionSerial in the update message response", 500, 50000)); + throw AblyException.fromErrorInfo(new ErrorInfo("No versionSerial in the " + action.name() + " response", 500, 50000));
143-152: Minor Javadoc inaccuracy: "text" should be "data".The Javadoc states "Appends message text to the end of the message" but
Message.datacan contain any object type, not just text. Consider updating to "Appends data to the end of the message" for accuracy.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1320-1341: Minor Javadoc inaccuracy: "text" should be "data".Same as in
MessageEditsMixin, the Javadoc states "Appends message text" but should say "Appends data" sinceMessage.datacan contain any object type.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
458-479: Minor Javadoc inaccuracy: "text" should be "data".Same documentation issue as in the other files - "Appends message text" should be "Appends data" since
Message.datais not limited to text.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/rest/ChannelBase.javalib/src/main/java/io/ably/lib/rest/MessageEditsMixin.javalib/src/main/java/io/ably/lib/transport/Defaults.javalib/src/main/java/io/ably/lib/types/MessageAction.javalib/src/main/java/io/ably/lib/types/MessageOperationSerializer.javalib/src/main/java/io/ably/lib/types/MessageSerializer.javalib/src/main/java/io/ably/lib/types/UpdateDeleteResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.javalib/src/test/java/io/ably/lib/transport/DefaultsTest.java
💤 Files with no reviewable changes (1)
- lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
- lib/src/main/java/io/ably/lib/transport/Defaults.java
- lib/src/main/java/io/ably/lib/util/Listeners.java
- lib/src/main/java/io/ably/lib/types/MessageSerializer.java
🧰 Additional context used
🧬 Code graph analysis (5)
lib/src/test/java/io/ably/lib/transport/DefaultsTest.java (1)
lib/src/main/java/io/ably/lib/transport/Defaults.java (1)
Defaults(6-80)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (3)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (4)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
MessageSerializer(23-242)lib/src/main/java/io/ably/lib/types/MessageVersion.java (1)
MessageVersion(24-232)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (29)
🔇 Additional comments (13)
lib/src/test/java/io/ably/lib/transport/DefaultsTest.java (1)
12-12: LGTM! Test correctly updated for protocol v5.The assertion now correctly expects protocol version "5" to match the updated
ABLY_PROTOCOL_VERSIONconstant.lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java (1)
84-84: LGTM! Header expectation correctly updated.The test now expects the
x-ably-versionheader to be "5", consistent with the protocol version upgrade.lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java (1)
84-84: LGTM! WebSocket parameter test correctly updated.The version parameter expectation now matches protocol version "5".
lib/src/main/java/io/ably/lib/types/MessageAction.java (1)
8-9: LGTM! MESSAGE_APPEND constant added correctly.The new enum constant is properly positioned with ordinal 5, and the existing
tryFindByOrdinalmethod will handle it correctly.lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
41-59: LGTM! MessagePack deserialization correctly handles unknown fields.The implementation properly:
- Handles NIL values (lines 47-50)
- Extracts the
versionSerialfield (lines 52-53)- Skips unknown fields to prevent stream corruption (lines 54-56)
The past review concern about missing
skipValue()has been properly addressed.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (2)
175-183: LGTM - Clean construction of the updated message with version metadata.The approach of creating a new
Messagewith the action and populating version fields from the optionalMessageOperationis well-structured. The null-check onoperationproperly handles the case when no operation metadata is provided.
99-141: LGTM - Well-structured API consolidation.The refactoring to return
UpdateDeleteResultand consolidating update/delete into a shared implementation with theMessageActionparameter is clean and reduces code duplication. The async variants correctly useCallback<UpdateDeleteResult>consistently.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
1220-1237: LGTM - Consistent delegation pattern for updateMessage.The sync and overloaded methods correctly delegate to
messageEditsMixinand follow the established pattern of the null-operation overload delegating to the full-signature method.
1276-1318: LGTM - Consistent deleteMessage API updates.The delete operations mirror the update operations pattern correctly, returning
UpdateDeleteResultand usingCallback<UpdateDeleteResult>for async variants.
1352-1366: LGTM - Async append methods are consistent.The async variants follow the established pattern of delegating to
messageEditsMixinand providing an overload without the operation parameter.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (3)
358-400: LGTM - UpdateMessage API changes are consistent with realtime ChannelBase.The REST channel's
updateMessagemethods correctly mirror the realtime implementation, delegating tomessageEditsMixinand returningUpdateDeleteResult.
414-456: LGTM - DeleteMessage API changes are consistent.
519-532: LGTM - getMessageVersions methods correctly delegate to mixin.
github-actions
bot
temporarily deployed
to
staging/pull/1182/javadoc
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
9c5ac4a to
d4cf2d1
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1182/features
github-actions
bot
temporarily deployed
to
staging/pull/1182/javadoc
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java:
- Around line 67-79: The handleResponseBody method in UpdateDeleteResult
currently returns an array containing a null when contentType is unrecognized;
change UpdateDeleteResult.handleResponseBody to explicitly throw an exception
for unknown content types (following the pattern in
Serialisation.HttpBodyHandler) instead of returning new
UpdateDeleteResult[]{null}; add an else branch that throws a
MessageDecodeException (or AblyException via
AblyException.fromThrowable/fromErrorInfo) with a clear message including the
unsupported contentType (and optionally body length) so callers don't receive a
null result silently.
In @lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java:
- Around line 233-235: The Javadoc for the test in RestChannelMessageEditTest is
incorrect (it says "Test deleteMessage: Soft delete a message")—update the
comment to correctly describe the test for appendMessage (e.g., "Test
appendMessage: Append data to an existing message" or similar) so the Javadoc
matches the test method name/behavior (appendMessage).
- Around line 252-257: The inline comment above the append operation is
incorrect; replace the misleading "Delete the message" comment with an accurate
description such as "Append a message" or "Create and append a message" to
reflect the code creating Message appendMessage and calling
channel.appendMessage(appendMessage) in RestChannelMessageEditTest (use the
appendMessage variable and channel.appendMessage call as the anchors to find the
change).
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
151-153: API signature inconsistency.
asSingleJsonRequest(Message)doesn't throwAblyException, but the existingasJsonRequest(Message)at lines 147-149 does throw. Similarly,asSingleMsgpackRequest(Message)throwsAblyException. Consider aligning the exception signatures for consistency.lib/src/main/java/io/ably/lib/util/Listeners.java (1)
9-11: Consider adding Javadoc for public API.A brief Javadoc comment explaining that this adapter bridges legacy
CompletionListenercallbacks to the newerCallback<T>interface (discarding the result) would help future maintainers understand the intended use case.lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
28-30: Specify charset encoding when converting bytes to String.The
new String(packed)constructor uses the platform's default charset, which may vary across environments. For consistency and correctness, explicitly specify UTF-8.Suggested fix
- public static UpdateDeleteResult readFromJson(byte[] packed) throws MessageDecodeException { - return Serialisation.gson.fromJson(new String(packed), UpdateDeleteResult.class); + public static UpdateDeleteResult readFromJson(byte[] packed) throws MessageDecodeException { + try { + return Serialisation.gson.fromJson(new String(packed, "UTF-8"), UpdateDeleteResult.class); + } catch (java.io.UnsupportedEncodingException e) { + throw new MessageDecodeException("UTF-8 encoding not supported", e); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/rest/ChannelBase.javalib/src/main/java/io/ably/lib/rest/MessageEditsMixin.javalib/src/main/java/io/ably/lib/transport/Defaults.javalib/src/main/java/io/ably/lib/types/MessageAction.javalib/src/main/java/io/ably/lib/types/MessageOperationSerializer.javalib/src/main/java/io/ably/lib/types/MessageSerializer.javalib/src/main/java/io/ably/lib/types/UpdateDeleteResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.javalib/src/test/java/io/ably/lib/transport/DefaultsTest.java
💤 Files with no reviewable changes (1)
- lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java
✅ Files skipped from review due to trivial changes (1)
- lib/src/test/java/io/ably/lib/transport/DefaultsTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java
🧰 Additional context used
🧬 Code graph analysis (4)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(7-34)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (3)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (19)
🔇 Additional comments (12)
lib/src/main/java/io/ably/lib/transport/Defaults.java (1)
15-15: LGTM!Protocol version bump to "5" aligns with the PR objectives for message appends support.
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
205-238: LGTM!The
SingleMessageBodyHandlercorrectly handles JSON and Msgpack responses with appropriate null checks before decoding. The pattern aligns with the existingMessageBodyHandler.lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (2)
264-265: Verify expected concatenation format.The assertion expects
"Initial messageMessage append"(no separator between original and appended content). If the server-side append behavior concatenates without a delimiter, this is correct. However, if there should be a space or other separator, this assertion will fail.Please confirm this matches the expected server-side message append behavior for protocol v5.
188-188: LGTM!The
Listeners.fromCompletionListenerwrapper correctly adapts the existingCompletionSetpattern to work with the newCallback<UpdateDeleteResult>API.lib/src/main/java/io/ably/lib/types/MessageAction.java (1)
8-9: LGTM!The
MESSAGE_APPENDaction is correctly added with ordinal 5, preserving existing ordinal values for backward compatibility.lib/src/main/java/io/ably/lib/util/Listeners.java (1)
7-11: LGTM!Clean adapter implementation that bridges
CompletionListenertoCallback<T>. The null-safety check is appropriate, and the generic type parameter allows flexible reuse across different callback result types.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
1218-1262: LGTM! Update and delete methods correctly return UpdateDeleteResult.The synchronous methods return
UpdateDeleteResultand async variants useCallback<UpdateDeleteResult>, which is consistent with the new API design. The delegation tomessageEditsMixinis correct.
1320-1366: LGTM! New append message APIs are well-structured.The new
appendMessageandappendMessageAsyncmethods follow the same pattern as the update/delete methods, with proper overloads for optionalMessageOperationparameter and consistent Javadoc.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (2)
167-205: LGTM! Unified implementation for update/delete/append operations.The
updateMessageImplmethod cleanly handles all three message actions through a single code path. The creation ofupdatedMessagecorrectly excludes theserial(used in URL path) and properly populates theactionandversionfields from the operation metadata.
143-165: LGTM! New append message API follows established patterns.The
appendMessageandappendMessageAsyncmethods are consistent with the update/delete counterparts and correctly delegate toupdateMessageImplwithMessageAction.MESSAGE_APPEND.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (2)
358-400: LGTM! REST channel update methods correctly return UpdateDeleteResult.The method signatures and implementation are consistent with the realtime channel counterpart, maintaining API consistency across REST and realtime interfaces.
458-504: LGTM! New append message APIs mirror realtime channel.The
appendMessageandappendMessageAsyncmethods follow the same pattern as their realtime counterparts, ensuring consistent API behavior between REST and realtime channels.
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Will take a look once free, meanwhile will be great if I can get link to relevant spec
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
d4cf2d1 to
a0402bc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1182/features
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
a0402bc to
b852631
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1182/features
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/types/MessageSerializer.java:
- Around line 77-85: The write(Message) method currently swallows IOException
and returns null, which breaks the asSingleMsgpackRequest contract and allows
ByteArrayRequestBody to receive null; change write(Message) to propagate
failures by catching IOException and throwing an AblyException (wrapping the
IOException as the cause) instead of returning null so asSingleMsgpackRequest
can surface serialization errors; update method signature (or overload) if
necessary so callers (e.g., asSingleMsgpackRequest) receive and propagate the
AblyException and ensure no null byte[] is passed into ByteArrayRequestBody.
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (2)
28-30: Consider specifying explicit charset for JSON parsing.
new String(packed)uses the platform default charset, which may not be UTF-8 on all JVMs. Consider usingnew String(packed, StandardCharsets.UTF_8)for consistent behavior.Suggested fix
+import java.nio.charset.StandardCharsets; + public static UpdateDeleteResult readFromJson(byte[] packed) throws MessageDecodeException { - return Serialisation.gson.fromJson(new String(packed), UpdateDeleteResult.class); + return Serialisation.gson.fromJson(new String(packed, StandardCharsets.UTF_8), UpdateDeleteResult.class); }
65-80: Unknown content type silently returns null in array.If
contentTypeis neitherapplication/jsonnorapplication/x-msgpack, the handler returnsnew UpdateDeleteResult[]{null}. This may cause unexpectedNullPointerExceptionin callers expecting a valid result.Consider throwing an
AblyExceptionfor unsupported content types, similar to how other handlers in the codebase might behave.Suggested fix
public UpdateDeleteResult[] handleResponseBody(String contentType, byte[] body) throws AblyException { try { UpdateDeleteResult updateDeleteResult = null; if ("application/json".equals(contentType)) updateDeleteResult = readFromJson(body); else if ("application/x-msgpack".equals(contentType)) updateDeleteResult = readMsgpack(body); + else if (contentType != null) + throw AblyException.fromErrorInfo(new ErrorInfo("Unsupported content type: " + contentType, 400, 40000)); return new UpdateDeleteResult[]{updateDeleteResult}; } catch (MessageDecodeException e) { throw AblyException.fromThrowable(e); } }lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
167-205: Clarify error message for missing response body.The error message on line 200 says "No versionSerial in the update message response" but the condition checks if
resultsis null or empty, not ifversionSerialis null. According toUpdateDeleteResultjavadoc,versionSerialcan legitimately be null when "the message was superseded by a subsequent update before it could be published."The current logic correctly returns the result when the response body exists (even if
versionSerialis null), but the error message is misleading. Consider updating the message to reflect what's actually being checked:✏️ Suggested improvement
- throw AblyException.fromErrorInfo(new ErrorInfo("No versionSerial in the update message response", 500, 50000)); + throw AblyException.fromErrorInfo(new ErrorInfo("Empty response body from update message request", 500, 50000));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/rest/ChannelBase.javalib/src/main/java/io/ably/lib/rest/MessageEditsMixin.javalib/src/main/java/io/ably/lib/transport/Defaults.javalib/src/main/java/io/ably/lib/types/MessageAction.javalib/src/main/java/io/ably/lib/types/MessageOperationSerializer.javalib/src/main/java/io/ably/lib/types/MessageSerializer.javalib/src/main/java/io/ably/lib/types/UpdateDeleteResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.javalib/src/test/java/io/ably/lib/transport/DefaultsTest.java
💤 Files with no reviewable changes (1)
- lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java
- lib/src/test/java/io/ably/lib/transport/DefaultsTest.java
- lib/src/main/java/io/ably/lib/transport/Defaults.java
🧰 Additional context used
🧬 Code graph analysis (3)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (24)
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: build
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check (21)
🔇 Additional comments (18)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (3)
50-52: LGTM - Method renamed appropriately for single-message variant.The rename from
asMsgpackRequest(Message)toasSingleMsgpackRequest(Message)clearly distinguishes the single-message path from the array variant, improving API clarity.
151-153: LGTM - Clean JSON serialization for single message.The implementation correctly wraps a single
Messageas JSON (not an array) usingJsonRequestBody.
205-238: LGTM - SingleMessageBodyHandler implementation is correct.The handler properly:
- Dispatches based on content type
- Deserializes single message (not array) from JSON/msgpack
- Applies decoding with channel options
- Wraps result in array to conform to
BodyHandler<Message>interface- Handles exceptions appropriately
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
7-34: LGTM - Clean adapter pattern for bridging listener types.This utility correctly bridges
CompletionListener(fire-and-forget success) toCallback<T>(result-bearing). The null-safety guards and the intentional discarding of the resultTinonSuccessare appropriate sinceCompletionListener.onSuccess()has no parameters.lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
41-59: LGTM - Msgpack deserialization follows established patterns.The implementation correctly handles:
- Map header unpacking
- Nil value skipping
- Unknown field skipping
- Field name interning for memory efficiency
lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java (1)
84-84: LGTM - Test updated to reflect protocol version 5.The assertion change correctly aligns with the protocol version bump from 4 to 5 in
Defaults.java.lib/src/main/java/io/ably/lib/types/MessageAction.java (1)
8-9: LGTM - New MESSAGE_APPEND action added correctly.The new enum constant is properly positioned with ordinal 5, and the existing
tryFindByOrdinalmethod will correctly resolve it.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (4)
46-46: LGTM!Import for
UpdateDeleteResultis correctly added to support the new return types.
1218-1262: LGTM!The
updateMessagemethods are correctly updated to returnUpdateDeleteResultand useCallback<UpdateDeleteResult>for async versions. The delegation pattern tomessageEditsMixinis consistent.
1274-1318: LGTM!The
deleteMessagemethods follow the same pattern asupdateMessage, returningUpdateDeleteResultand usingCallback<UpdateDeleteResult>for async versions.
1320-1366: LGTM!New
appendMessagemethods are well-structured and follow the same pattern asupdateMessageanddeleteMessage. The sync/async variants are consistent, and delegation tomessageEditsMixinis correct.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (3)
14-20: LGTM!Imports for
MessageAction,MessageVersion, andUpdateDeleteResultare correctly added.
96-165: LGTM!The public methods for
updateMessage,deleteMessage, andappendMessage(both sync and async) are well-structured. They all delegate to a unifiedupdateMessageImplwith the appropriateMessageAction, reducing code duplication.
175-183: LGTM!The message construction correctly creates a new
Messageobject with only the fields needed for the patch operation (name,data,extras,action,version), avoiding mutation of the original message. The serial is properly used in the URL path rather than the body.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (4)
20-20: LGTM!Import for
UpdateDeleteResultis correctly added.
356-400: LGTM!The
updateMessagemethods are correctly updated withUpdateDeleteResultreturn types andCallback<UpdateDeleteResult>for async versions. The API is consistent with the realtimeChannelBase.
412-456: LGTM!The
deleteMessagemethods follow the same pattern, returningUpdateDeleteResultand using callbacks for async versions.
458-504: LGTM!New
appendMessagemethods are correctly implemented with consistent patterns matchingupdateMessageanddeleteMessage. The sync/async variants and delegation tomessageEditsMixinare properly structured.
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
b852631 to
58541a7
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1182/features
github-actions
bot
temporarily deployed
to
staging/pull/1182/javadoc
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
530-532: Remove unnecessarythrows AblyExceptionfrom async method signature.The
getMessageVersionsAsyncmethod declaresthrows AblyExceptionin its signature, but async methods typically should not throw checked exceptions synchronously since errors are delivered via the callback. This is inconsistent with other async methods in this class (e.g.,updateMessageAsync,deleteMessageAsync,appendMessageAsync) which do not declarethrows AblyException.Suggested fix
- public void getMessageVersionsAsync(String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { + public void getMessageVersionsAsync(String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) { messageEditsMixin.getMessageVersionsAsync(ably.http, serial, params, callback); }lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
231-233: Remove unnecessarythrows AblyExceptionfrom async method signature.Similar to the issue in
ChannelBase.java, thegetMessageVersionsAsyncmethod declaresthrows AblyException, but async methods typically deliver errors via the callback. Note thatgetMessageVersionsImplis called synchronously to build the query, but this can be refactored for consistency.Suggested fix
- public void getMessageVersionsAsync(Http http, String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { + public void getMessageVersionsAsync(Http http, String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) { + try { getMessageVersionsImpl(http, serial, params).async(callback); + } catch (AblyException e) { + callback.onError(e.errorInfo); + } }
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java:
- Around line 67-79: In UpdateDeleteResult.handleResponseBody, the content-type
equality check fails for values with charset parameters and the method
incorrectly returns an array containing null for unknown types; update the
content-type handling to canonicalise or strip parameters (e.g., take the media
type portion before any ';' and compare case-insensitively to "application/json"
and "application/x-msgpack") and, when neither matches, throw the same exception
used elsewhere (e.g., a MessageDecodeException or AblyException) instead of
returning new UpdateDeleteResult[]{null}; ensure you still call
readFromJson(...) and readMsgpack(...) for the matched types.
In @lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java:
- Around line 188-197: Before accessing updateWaiter.result.versionSerial, check
for async errors and null result: after updateWaiter.waitFor() assert that
updateWaiter.error is null (or fail the test with updateWaiter.error) and that
updateWaiter.result is not null, then proceed to compare
updateWaiter.result.versionSerial with updatedMessage.version.serial; reference
Helpers.AsyncWaiter, updateWaiter, updateMessageAsync and
updateWaiter.result.versionSerial when making these checks.
- Around line 295-304: The test accesses deleteWaiter.result.versionSerial
without checking for errors; update the code around deleteWaiter (the
AsyncWaiter instance used with channel.deleteMessageAsync and
deleteWaiter.waitFor()) to assert that deleteWaiter.error is null (or fail the
test if non-null) before reading deleteWaiter.result, then proceed to compare
deleteWaiter.result.versionSerial with deletedMessage.version.serial; this
mirrors the fix used in updateMessage_async by validating deleteWaiter.error
after waitFor() and before accessing deleteWaiter.result.
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
77-85: Silent error handling returns null; consistent with existing code but risky.Returning
nullonIOExceptionmatches the pattern used elsewhere in this file (e.g.,writeMsgpackArray), but callers may not expect or handlenull. Consider whether a more explicit error propagation would be safer for the new single-message APIs.Also, the method name
write(Message)is quite generic given the overloadwrite(Map, MessagePacker)exists at line 87. A more specific name likewriteMsgpack(Message)could improve clarity.lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
28-30: Specify charset when converting bytes to String.The
new String(packed)constructor uses the platform default charset, which may differ across environments. Consider usingStandardCharsets.UTF_8for consistency.♻️ Suggested fix
+import java.nio.charset.StandardCharsets; + public static UpdateDeleteResult readFromJson(byte[] packed) throws MessageDecodeException { - return Serialisation.gson.fromJson(new String(packed), UpdateDeleteResult.class); + return Serialisation.gson.fromJson(new String(packed, StandardCharsets.UTF_8), UpdateDeleteResult.class); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/rest/ChannelBase.javalib/src/main/java/io/ably/lib/rest/MessageEditsMixin.javalib/src/main/java/io/ably/lib/transport/Defaults.javalib/src/main/java/io/ably/lib/types/MessageAction.javalib/src/main/java/io/ably/lib/types/MessageOperationSerializer.javalib/src/main/java/io/ably/lib/types/MessageSerializer.javalib/src/main/java/io/ably/lib/types/UpdateDeleteResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/rest/RestSuite.javalib/src/test/java/io/ably/lib/transport/DefaultsTest.java
💤 Files with no reviewable changes (1)
- lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java
- lib/src/main/java/io/ably/lib/util/Listeners.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java
- lib/src/main/java/io/ably/lib/types/MessageAction.java
- lib/src/test/java/io/ably/lib/transport/DefaultsTest.java
🧰 Additional context used
🧬 Code graph analysis (4)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (2)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (1)
Helpers(74-1229)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (6)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
MessageSerializer(23-242)lib/src/main/java/io/ably/lib/types/MessageVersion.java (1)
MessageVersion(24-232)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check (29)
🔇 Additional comments (17)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
151-153: LGTM!Clean single-message JSON serialization that mirrors the pattern of
asJsonRequest(Message[]). The lack of checked exception is appropriate since Gson'stoJsondoesn't throw checked exceptions.lib/src/test/java/io/ably/lib/test/rest/RestSuite.java (1)
31-31: LGTM!Test class properly added to the REST test suite.
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
41-58: LGTM!The msgpack parsing correctly handles unknown fields by skipping them, which provides forward compatibility.
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
264-268: Verify expected concatenation behavior for append operation.The assertion expects
"Initial messageMessage append"(no space/separator between original and appended data). Confirm this is the intended server-side append behavior, as it may surprise users expecting a delimiter.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (4)
1218-1237: LGTM!The
updateMessagemethods correctly returnUpdateDeleteResultand follow consistent delegation patterns. Documentation is clear.
1248-1262: LGTM!Async update methods properly use
Callback<UpdateDeleteResult>and delegate to the mixin implementation.
1274-1318: LGTM!Delete methods follow the same consistent pattern as update methods with proper return types and callback signatures.
1320-1366: LGTM!New
appendMessageandappendMessageAsyncmethods are well-documented and follow the established patterns for update/delete operations. The API is consistent with the rest of the message editing functionality.lib/src/main/java/io/ably/lib/transport/Defaults.java (1)
15-15: Protocol v5 implementation is complete with proper test coverage and feature integration.The protocol version bump to v5 is part of a complete feature release (AIT-99) that includes message append/edit/delete operations. Test coverage exists in DefaultsTest.java, and dependent features are fully implemented (MessageEditsMixin, UpdateDeleteResult, MessageVersion). The constant is properly integrated across HTTP and realtime transports with spec references (G4, RSC7e).
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (3)
356-400: LGTM! Update message API changes are consistent.The signature changes to return
UpdateDeleteResultand useCallback<UpdateDeleteResult>for async variants are correctly implemented. The delegation tomessageEditsMixinis properly maintained, and the overloaded methods correctly passnullfor the optionaloperationparameter.
412-456: LGTM! Delete message API changes follow the same consistent pattern.
458-504: LGTM! New append message API is well-structured.The new
appendMessageandappendMessageAsyncmethods follow the established pattern used byupdateMessageanddeleteMessage. The Javadoc clearly describes the operation's purpose, and delegation to the mixin is consistent with other message operations.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (5)
99-113: LGTM! Update message implementation correctly returns UpdateDeleteResult.The method properly delegates to
updateMessageImplwith theMESSAGE_UPDATEaction.
127-141: LGTM! Delete message implementation follows the same pattern.
143-165: LGTM! Append message methods are correctly implemented.The new
appendMessageandappendMessageAsyncmethods properly useMESSAGE_APPENDaction and follow the established patterns.
196-201: LGTM! Response handling with appropriate error for missing versionSerial.The error handling correctly throws an exception when the response doesn't contain the expected
UpdateDeleteResult, using an appropriate 500 error code to indicate a server-side issue.
175-183: No action needed—this code is correct as written.The comment misinterprets the semantics of message updates. The
checkClientId(message, true, false)call validates the original message'sclientIdfor security (rejecting wildcard IDs), but does not require copying it to the update request.Message updates intentionally maintain a distinction:
message.clientId: The client ID of the original publisher (historical, immutable)version.clientId: The client ID of who performed THIS version update (set fromoperation.clientId)When updating a message identified by its
serial, the server already knows the original state. The update request should specify only the new content and who is making the change (version.clientId = operation.clientId), not resend the original publisher's ID. Copyingmessage.clientIdtoupdatedMessage.clientIdwould incorrectly attribute the update to the original publisher rather than the actual updater.Likely an incorrect or invalid review comment.
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
58541a7 to
31969a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
108-119: Guard against nullableversionSerial/versionin assertions (avoid NPE + spec mismatch).Given
UpdateDeleteResult.versionSerialis documented as nullable, these tests should either assert it’s non-null (if that’s an invariant here) or handle null explicitly.Possible test hardening
UpdateDeleteResult result = channel.updateMessage(updateMessage); + assertNotNull("Expected UpdateDeleteResult", result); + assertNotNull("Expected versionSerial to be set", result.versionSerial); Message updatedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial); + assertNotNull("Expected message version to be present", updatedMessage.version); ... assertEquals(result.versionSerial, updatedMessage.version.serial);UpdateDeleteResult result = channel.deleteMessage(deleteMessage); + assertNotNull("Expected UpdateDeleteResult", result); + assertNotNull("Expected versionSerial to be set", result.versionSerial); Message deletedMessage = waitForDeletedMessageAppear(channel, publishedMessage.serial); + assertNotNull("Expected message version to be present", deletedMessage.version); assertEquals(result.versionSerial, deletedMessage.version.serial);Also applies to: 149-160, 226-235
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
167-205: Handle null UpdateDeleteResult from unhandled content-types and add defensive validations.The
UpdateDeleteResult.getBodyHandler()can return an array with a null element for unexpected content-types, causing the code to accept and returnnullas a successful result. Additionally, add defensive null checks for the message parameter and the result element itself.Optional: Validate
operation.clientId(if provided) similar to howmessage.clientIdis validated, and consider explicitly settingmessage.serialin the outbound message body for completeness.Suggested improvements
private Http.Request<UpdateDeleteResult> updateMessageImpl(Http http, Message message, MessageOperation operation, MessageAction action) { return http.request((scheduler, callback) -> { + if (message == null) { + throw AblyException.fromErrorInfo(new ErrorInfo("Message cannot be null", 400, 40003)); + } if (message.serial == null || message.serial.isEmpty()) { throw AblyException.fromErrorInfo(new ErrorInfo("Message serial cannot be empty", 400, 40003)); } /* RTL6g3 */ auth.checkClientId(message, true, false); + if (operation != null && operation.clientId != null) { + Message clientIdCheck = new Message(); + clientIdCheck.clientId = operation.clientId; + auth.checkClientId(clientIdCheck, true, false); + } Message updatedMessage = new Message(message.name, message.data, message.extras); + updatedMessage.serial = message.serial; updatedMessage.action = action; updatedMessage.version = new MessageVersion(); if (operation != null) { updatedMessage.version.clientId = operation.clientId; updatedMessage.version.description = operation.description; updatedMessage.version.metadata = operation.metadata; } updatedMessage.encode(channelOptions); HttpCore.RequestBody requestBody = clientOptions.useBinaryProtocol ? MessageSerializer.asSingleMsgpackRequest(updatedMessage) : MessageSerializer.asSingleJsonRequest(updatedMessage); final Param[] params = clientOptions.addRequestIds ? Param.array(Crypto.generateRandomRequestId()) : null; HttpCore.BodyHandler<UpdateDeleteResult> bodyHandler = UpdateDeleteResult.getBodyHandler(); scheduler.patch(basePath + "/messages/" + HttpUtils.encodeURIComponent(message.serial), HttpUtils.defaultAcceptHeaders(clientOptions.useBinaryProtocol), params, requestBody, (response, error) -> { if (error != null) throw AblyException.fromErrorInfo(error); UpdateDeleteResult[] results = bodyHandler.handleResponseBody(response.contentType, response.body); - if (results != null && results.length > 0) return results[0]; - throw AblyException.fromErrorInfo(new ErrorInfo("No versionSerial in the update message response", 500, 50000)); + if (results != null && results.length > 0 && results[0] != null) return results[0]; + throw AblyException.fromErrorInfo(new ErrorInfo("Failed to parse update/delete/append response", 500, 50000)); }, true, callback); }); }
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java:
- Around line 14-21: Update the Javadoc for async methods in
MessageEditsMixin.java to refer to the parameter type as a "callback"
(Callback<UpdateDeleteResult>) instead of "listener" for the relevant methods
(the async update/delete methods that accept Callback<UpdateDeleteResult>). Also
validate operation.clientId by calling auth.checkClientId(operation.clientId)
before writing it into the payload/version metadata (the same check already used
for message.clientId) to prevent unvalidated clientId values from being
serialized.
In @lib/src/main/java/io/ably/lib/types/MessageSerializer.java:
- Around line 50-52: The write(...) helper currently swallows IOException and
returns null, which lets asSingleMsgpackRequest (and the similar
asBatchMsgpackRequest) produce a null RequestBody or NPE; change write(Message)
and write(Message[]) to not return null on IO errors but instead throw an
AblyException (wrap the IOException with an appropriate AblyException message),
and update asSingleMsgpackRequest / asBatchMsgpackRequest to call write(...)
knowing it will either return a valid byte[] or throw AblyException (no null
checks needed), ensuring the HttpUtils.ByteArrayRequestBody is always
constructed with a non-null byte[].
In @lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java:
- Around line 188-198: The async test dereferences the AsyncWaiter.result (e.g.,
updateWaiter.result.versionSerial) without first asserting the operation
succeeded, which can cause NPEs and mask failures; update the test to check the
waiter completed successfully (assertTrue or assertNotNull/ assertFalse on an
error flag exposed by Helpers.AsyncWaiter) immediately after
updateWaiter.waitFor() and before accessing updateWaiter.result, and apply the
same guard for the other occurrences (the AsyncWaiter instances around lines
301-311) so you always assert success/failure explicitly before using
result/versionSerial.
- Around line 237-273: The test asserts the wrong action for an append: change
the expected action from MessageAction.MESSAGE_UPDATE to
MessageAction.MESSAGE_APPEND for the appendMessage flow (in
appendMessage_checkUpdatedData), and ensure the waiter used
(waitForUpdatedMessageAppear) is either updated to accept MESSAGE_APPEND or
replaced with a helper that waits for a MESSAGE_APPEND for the given serial;
alternatively, if you intended to test update semantics, replace
channel.appendMessage(appendMessage) with channel.updateMessage(...) and leave
the existing MESSAGE_UPDATE assertion.
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
7-11: Consider adding a private constructor and handling null listener at factory level.As a utility class,
Listenersshould prevent instantiation. Additionally, handlingnulllistener in the factory method avoids unnecessary object allocation.♻️ Suggested improvement
public class Listeners { + private Listeners() { + // Utility class + } + public static <T> Callback<T> fromCompletionListener(CompletionListener listener) { + if (listener == null) { + return new Callback<T>() { + @Override + public void onSuccess(T result) { } + + @Override + public void onError(ErrorInfo reason) { } + }; + } return new CompletionListenerWrapper<T>(listener); }lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1218-1366: API change looks consistent (return UpdateDeleteResult + Callback), but consider clarifying channel-state expectations.
All new/changed methods delegate tomessageEditsMixinoverably.http; it’s not obvious from the Javadoc whether the channel must beATTACHED(or whether this works whileDETACHED/FAILED, etc.). If no attach is required (likely), consider stating “This is a REST operation and does not require the channel to be attached.”
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/rest/ChannelBase.javalib/src/main/java/io/ably/lib/rest/MessageEditsMixin.javalib/src/main/java/io/ably/lib/transport/Defaults.javalib/src/main/java/io/ably/lib/types/MessageAction.javalib/src/main/java/io/ably/lib/types/MessageOperationSerializer.javalib/src/main/java/io/ably/lib/types/MessageSerializer.javalib/src/main/java/io/ably/lib/types/UpdateDeleteResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.javalib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/rest/RestSuite.javalib/src/test/java/io/ably/lib/transport/DefaultsTest.java
💤 Files with no reviewable changes (1)
- lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java
- lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java
- lib/src/test/java/io/ably/lib/test/rest/RestSuite.java
- lib/src/test/java/io/ably/lib/transport/DefaultsTest.java
🧰 Additional context used
🧬 Code graph analysis (5)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (7)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
MessageSerializer(23-242)lib/src/main/java/io/ably/lib/types/MessageVersion.java (1)
MessageVersion(24-232)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (2)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (1)
Helpers(74-1229)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (29)
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check (29)
- GitHub Check: build
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check (24)
🔇 Additional comments (9)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
13-33: LGTM!The
CompletionListenerWrappercorrectly implements the adapter pattern, bridgingCompletionListenerto the genericCallback<T>interface. The null-safe delegation in bothonSuccessandonErroris appropriate, and the intentional discarding of the result parameter inonSuccessis the expected behavior sinceCompletionListener.onSuccess()is parameterless.lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
151-153:asSingleJsonRequest(Message)looks good as a dedicated single-message entry point.Nice separation from the bulk
Message[]serializers.lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java (1)
83-85: Test expectation update to protocol v5 is consistent.lib/src/main/java/io/ably/lib/transport/Defaults.java (1)
15-15: Protocol version bump is fine, but please confirm rollout/compat guarantees for v5.lib/src/main/java/io/ably/lib/types/MessageAction.java (1)
8-13: AddingMESSAGE_APPENDat the end preserves existing ordinals (good for wire compatibility).lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
7-17: Imports forHelpersandUpdateDeleteResultare consistent with the API changes.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
46-46: Import addition is fine; make sure nullability/behavior is defined for UpdateDeleteResult.
Since these methods now returnUpdateDeleteResult, please ensure downstream callers never see an unexpectednullresult (e.g., from response parsing) or explicitly annotate/document ifnullcan happen.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (2)
20-20: LGTM (type import aligns with new return types).
356-504: Delegation is clean; ensure behavior is consistent across REST + Realtime surfaces.
Given bothio.ably.lib.rest.ChannelBaseandio.ably.lib.realtime.ChannelBasenow expose the same edit APIs, please ensure they have consistent semantics for: required fields (e.g., serial), allowedoperation.clientId, and what “append” supports (expected data type).
|
@sacOO7 added link to the spec |
There was a problem hiding this comment.
Pull request overview
This pull request implements protocol version 5 upgrade and adds message append functionality to the Ably Java SDK, aligned with the specification changes in https://github.com/ably/specification/pull/406/files.
Changes:
- Upgraded protocol version from v4 to v5 across the codebase
- Added message append API with new
MESSAGE_APPENDaction - Refactored message edit operations (update/delete) to return version information via
UpdateDeleteResult - Changed async operation callbacks from
CompletionListenertoCallback<UpdateDeleteResult>for typed responses
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/main/java/io/ably/lib/transport/Defaults.java | Updates protocol version constant from "4" to "5" |
| lib/src/main/java/io/ably/lib/types/MessageAction.java | Adds MESSAGE_APPEND enum value for append operations |
| lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java | New type to encapsulate version serial returned from update/delete/append operations |
| lib/src/main/java/io/ably/lib/types/MessageSerializer.java | Adds single-message serialization methods for individual message operations |
| lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java | Refactors update/delete to use unified implementation and adds appendMessage methods |
| lib/src/main/java/io/ably/lib/rest/ChannelBase.java | Adds appendMessage API and updates signatures to return UpdateDeleteResult |
| lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | Mirrors REST channel changes for realtime channels |
| lib/src/main/java/io/ably/lib/util/Listeners.java | Utility class for converting CompletionListener to Callback (currently unused) |
| lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java | Removed as operation serialization is now handled via Message object with action field |
| lib/src/test/java/io/ably/lib/transport/DefaultsTest.java | Updates test assertion for protocol version |
| lib/src/test/java/io/ably/lib/test/rest/HttpHeaderTest.java | Updates expected x-ably-version header to "5" |
| lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java | Updates expected version parameter to "5" |
| lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java | Adds appendMessage tests and updates existing tests to verify version serials |
| lib/src/test/java/io/ably/lib/test/rest/RestSuite.java | Registers RestChannelMessageEditTest in test suite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| @Test | ||
| public void deleteMessage_softDelete() throws Exception { | ||
| if (engineType == EngineType.DEFAULT) return; |
There was a problem hiding this comment.
The test is conditionally skipped when engineType == EngineType.DEFAULT, but there's no explanation or assertion message indicating why this test requires a non-default engine. Consider adding a comment or using JUnit's Assume.assumeFalse to make the test skip more explicit and maintainable.
| */ | ||
| @Test | ||
| public void appendMessage_checkUpdatedData() throws Exception { | ||
| if (engineType == EngineType.DEFAULT) return; |
There was a problem hiding this comment.
The test is conditionally skipped when engineType == EngineType.DEFAULT, but there's no explanation or assertion message indicating why this test requires a non-default engine. Consider adding a comment or using JUnit's Assume.assumeFalse to make the test skip more explicit and maintainable.
| */ | ||
| @Test | ||
| public void deleteMessage_async() throws Exception { | ||
| if (engineType == EngineType.DEFAULT) return; |
There was a problem hiding this comment.
The test is conditionally skipped when engineType == EngineType.DEFAULT, but there's no explanation or assertion message indicating why this test requires a non-default engine. Consider adding a comment or using JUnit's Assume.assumeFalse to make the test skip more explicit and maintainable.
| import io.ably.lib.realtime.CompletionListener; | ||
| import io.ably.lib.types.Callback; | ||
| import io.ably.lib.types.ErrorInfo; | ||
|
|
||
| public class Listeners { | ||
|
|
||
| public static <T> Callback<T> fromCompletionListener(CompletionListener listener) { | ||
| return new CompletionListenerWrapper<T>(listener); | ||
| } | ||
|
|
||
| private static class CompletionListenerWrapper<T> implements Callback<T> { | ||
| private final CompletionListener listener; | ||
|
|
||
| private CompletionListenerWrapper(CompletionListener listener) { | ||
| this.listener = listener; | ||
| } | ||
|
|
||
| @Override | ||
| public void onSuccess(T result) { | ||
| if (listener != null) { | ||
| listener.onSuccess(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onError(ErrorInfo reason) { | ||
| if (listener != null) { | ||
| listener.onError(reason); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The Listeners utility class is introduced but appears to be unused in the codebase. The fromCompletionListener method provides a wrapper to convert CompletionListener to Callback, but this conversion is already handled by CompletionListener.ToCallback. Consider either using this utility class in the implementation or removing it if it's not needed.
| import io.ably.lib.realtime.CompletionListener; | |
| import io.ably.lib.types.Callback; | |
| import io.ably.lib.types.ErrorInfo; | |
| public class Listeners { | |
| public static <T> Callback<T> fromCompletionListener(CompletionListener listener) { | |
| return new CompletionListenerWrapper<T>(listener); | |
| } | |
| private static class CompletionListenerWrapper<T> implements Callback<T> { | |
| private final CompletionListener listener; | |
| private CompletionListenerWrapper(CompletionListener listener) { | |
| this.listener = listener; | |
| } | |
| @Override | |
| public void onSuccess(T result) { | |
| if (listener != null) { | |
| listener.onSuccess(); | |
| } | |
| } | |
| @Override | |
| public void onError(ErrorInfo reason) { | |
| if (listener != null) { | |
| listener.onError(reason); | |
| } | |
| } | |
| } | |
| } | |
| /** | |
| * This file previously contained the Listeners utility class, which has been | |
| * removed because CompletionListener.ToCallback already provides the same | |
| * functionality for converting CompletionListener to Callback. | |
| */ |
| if (error != null) throw AblyException.fromErrorInfo(error); | ||
| UpdateDeleteResult[] results = bodyHandler.handleResponseBody(response.contentType, response.body); | ||
| if (results != null && results.length > 0) return results[0]; | ||
| throw AblyException.fromErrorInfo(new ErrorInfo("No versionSerial in the update message response", 500, 50000)); |
There was a problem hiding this comment.
The error message says "No versionSerial in the update message response" but this method is used for update, delete, and append operations. Consider making the error message more generic, such as "No versionSerial in the response" to accurately reflect all use cases.
| throw AblyException.fromErrorInfo(new ErrorInfo("No versionSerial in the update message response", 500, 50000)); | |
| throw AblyException.fromErrorInfo(new ErrorInfo("No versionSerial in the response", 500, 50000)); |
Adds message appends and protocol v5 upgrade, Spec PR: https://github.com/ably/specification/pull/406/files
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.