[AIT-99] feat: protocol v5 + message appends #1182
Conversation
WalkthroughMessage update/delete APIs now return UpdateDeleteResult and use Callback for async calls; new appendMessage and message-versions REST APIs 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.
| // Delete the message | ||
| Message appendMessage = new Message(); | ||
| appendMessage.serial = publishedMessage.serial; | ||
| appendMessage.data = "Message append"; | ||
|
|
||
| channel.appendMessage(appendMessage); |
There was a problem hiding this comment.
Misleading inline comment.
The comment says "Delete the message" but the code is creating an append message. Update the comment to accurately describe the operation.
Suggested fix
- // Delete the message
+ // Append to the message
Message appendMessage = new Message();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Delete the message | |
| Message appendMessage = new Message(); | |
| appendMessage.serial = publishedMessage.serial; | |
| appendMessage.data = "Message append"; | |
| channel.appendMessage(appendMessage); | |
| // Append to the message | |
| Message appendMessage = new Message(); | |
| appendMessage.serial = publishedMessage.serial; | |
| appendMessage.data = "Message append"; | |
| channel.appendMessage(appendMessage); |
🤖 Prompt for AI Agents
In @lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
around lines 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).
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.
| public static byte[] write(Message message) { | ||
| try { | ||
| ByteArrayOutputStream out = new ByteArrayOutputStream(); | ||
| MessagePacker packer = Serialisation.msgpackPackerConfig.newPacker(out); | ||
| message.writeMsgpack(packer); | ||
| packer.flush(); | ||
| return out.toByteArray(); | ||
| } catch(IOException e) { return null; } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the MessageSerializer.java file
fd MessageSerializer.javaRepository: ably/ably-java
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Check the asSingleMsgpackRequest method and surrounding code
rg -n -A10 "asSingleMsgpackRequest" lib/src/main/java/io/ably/lib/types/MessageSerializer.javaRepository: ably/ably-java
Length of output: 629
🏁 Script executed:
#!/bin/bash
# Check the write(Message) method implementation
rg -n -B2 -A10 "public static byte\[\] write\(Message" lib/src/main/java/io/ably/lib/types/MessageSerializer.javaRepository: ably/ably-java
Length of output: 593
🏁 Script executed:
#!/bin/bash
# Find ByteArrayRequestBody class and check how it handles the body parameter
fd ByteArrayRequestBody.javaRepository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for ByteArrayRequestBody usage and initialization
rg -n "ByteArrayRequestBody" lib/src/main/java/io/ably/lib/types/MessageSerializer.javaRepository: ably/ably-java
Length of output: 369
🏁 Script executed:
#!/bin/bash
# Find HttpUtils class where ByteArrayRequestBody is defined
fd HttpUtils.javaRepository: ably/ably-java
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Search for ByteArrayRequestBody class definition
rg -n "class ByteArrayRequestBody" --type java -A15Repository: ably/ably-java
Length of output: 1538
🏁 Script executed:
#!/bin/bash
# Check where getEncoded() is called and how the result is used
rg -n "getEncoded\(\)" --type java -B2 -A2Repository: ably/ably-java
Length of output: 11492
🏁 Script executed:
#!/bin/bash
# Also check how asMsgpackRequest (line 54-56) is used, to see if the null pattern is truly consistent
rg -n "asMsgpackRequest\|asSingleMsgpackRequest" --type java -B2 -A2Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there are any null checks before using the RequestBody
rg -n "RequestBody" lib/src/main/java/io/ably/lib/types/MessageSerializer.java -B2 -A5Repository: ably/ably-java
Length of output: 1992
🏁 Script executed:
#!/bin/bash
# Check HttpBody class and how it handles null bytes
rg -n "class HttpBody" --type java -A15Repository: ably/ably-java
Length of output: 1961
🏁 Script executed:
#!/bin/bash
# Check if there are test cases covering IOException in write methods
rg -n "IOException" lib/src/test/java/io/ably/lib/test/rest/RestChannelPublishTest.java -B3 -A3Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the full HttpBody class implementation
rg -n "public class HttpBody" network-client-core/src/main/java/io/ably/lib/network/HttpBody.java -A20Repository: ably/ably-java
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check if there's any test coverage for null bytes scenario
rg -n "write\(Message\)" lib/src/test/java -r '$0' -A3 -B1 | head -50Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check how the request body is used in the actual HTTP request sending
rg -n "class HttpBody" network-client-core/src/main/java/io/ably/lib/network/HttpBody.java -A30Repository: ably/ably-java
Length of output: 167
Method contract violation: asSingleMsgpackRequest declares throws AblyException but silently returns null on IOException.
The write(Message) method catches IOException and returns null instead of propagating the error. This null value is then passed directly to ByteArrayRequestBody (line 51), creating a request body with null bytes. More critically, this violates the method's declared contract—asSingleMsgpackRequest signature declares throws AblyException, but IOException is silently swallowed and null is returned instead. Callers expecting to catch AblyException will not be notified of serialization failures.
This is inconsistent with the parallel asMsgpackRequest(Message[]) method (line 54-56), which uses the same null-returning pattern but correctly does NOT declare throws AblyException. Either align asSingleMsgpackRequest to not throw the exception, or make it actually throw AblyException on IOException.
🤖 Prompt for AI Agents
In @lib/src/main/java/io/ably/lib/types/MessageSerializer.java around lines 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.
b852631 to
58541a7
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/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.
| public static HttpCore.RequestBody asSingleMsgpackRequest(Message message) throws AblyException { | ||
| return new HttpUtils.ByteArrayRequestBody(write(message), "application/x-msgpack"); | ||
| } |
There was a problem hiding this comment.
Potential null body from write() and misleading exception signature.
The write(message) method returns null on IOException (line 84), but this method signature declares throws AblyException. If an I/O error occurs, a ByteArrayRequestBody with null bytes is created rather than throwing an exception. Either propagate the exception properly or remove the throws declaration.
Suggested fix: propagate exceptions from write()
- 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[] bytes = write(message);
+ if (bytes == null) {
+ throw AblyException.fromErrorInfo(new ErrorInfo("Failed to serialize message", 400, 40000));
+ }
+ return new HttpUtils.ByteArrayRequestBody(bytes, "application/x-msgpack");
}Alternatively, make write(Message) throw AblyException instead of returning null.
| @Override | ||
| 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); | ||
| return new UpdateDeleteResult[]{updateDeleteResult}; | ||
| } catch (MessageDecodeException e) { | ||
| throw AblyException.fromThrowable(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other BodyHandler implementations and their content-type handling
rg -n "handleResponseBody" --type=java -A 15 | head -200Repository: ably/ably-java
Length of output: 20028
🏁 Script executed:
#!/bin/bash
# Check for content-type utilities or helpers
rg -i "content.?type|charset" --type=java -n | grep -i "strip\|parse\|split\|substring" | head -20Repository: ably/ably-java
Length of output: 431
🏁 Script executed:
#!/bin/bash
# Look for any utility methods that might extract base content-type
rg -A 5 "public.*String.*content" --type=java | head -50Repository: ably/ably-java
Length of output: 5053
🏁 Script executed:
#!/bin/bash
# Check how HttpCore.Response populates contentType
rg -n "contentType\s*=" --type=java -B 2 -A 2 | head -80Repository: ably/ably-java
Length of output: 4073
🏁 Script executed:
#!/bin/bash
# Check OkHttp imports and usage
rg "okhttp3|MediaType" --type=java -n | grep import | head -20Repository: ably/ably-java
Length of output: 2044
🏁 Script executed:
#!/bin/bash
# Check if there's any test that shows actual content-type values
rg "charset" --type=java -n -B 2 -A 2 | grep -i "application/json\|application/x-msgpack" | head -30Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the actual HTTP response handling in OkHttpCall more closely
cat -n network-client-okhttp/src/main/java/io/ably/lib/network/OkHttpCall.java | sed -n '40,60p'Repository: ably/ably-java
Length of output: 560
🏁 Script executed:
#!/bin/bash
# Check if there's any parsing or stripping of content-type before being passed to bodyHandler
rg -B 5 "handleResponseBody" lib/src/main/java/io/ably/lib/http/HttpCore.java -A 3 | head -40Repository: ably/ably-java
Length of output: 328
🏁 Script executed:
#!/bin/bash
# Look at the Response class definition to see how contentType is set
rg -A 20 "public static class Response" lib/src/main/java/io/ably/lib/http/HttpCore.java | head -40Repository: ably/ably-java
Length of output: 863
🏁 Script executed:
#!/bin/bash
# Check if there are any tests verifying content-type handling with charset
find . -name "*Test*.java" -type f | xargs grep -l "charset\|application/json" | head -5Repository: ably/ably-java
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Check the RestRequestTest to see if charset is tested
rg "charset\|contentType" lib/src/test/java/io/ably/lib/test/rest/RestRequestTest.java -B 2 -A 2Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for any tests that verify handleResponseBody behavior
rg "handleResponseBody" --type=java -B 3 -A 10 lib/src/test/Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check what OkHttp's MediaType.toString() actually returns - look for examples
rg "toString\|MediaType" network-client-okhttp/src/main/java/io/ably/lib/network/OkHttpCall.java -B 2 -A 2Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check DefaultHttpCall to see how it handles content-type
cat -n network-client-default/src/main/java/io/ably/lib/network/DefaultHttpCall.java | sed -n '105,125p'Repository: ably/ably-java
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Look for any utility that might normalize or parse content-type
rg "split|substring|indexOf" lib/src/main/java/io/ably/lib/types/*.java | grep -i "content\|type"Repository: ably/ably-java
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Check if there are any discussions or comments about content-type with charset
rg -i "content.type.*charset|charset.*content" lib/src/main/java/ -B 2 -A 2Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check which OkHttp version is used
rg "okhttp" --type=gradle --type=xml --type=pomRepository: ably/ably-java
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Look for build files
find . -maxdepth 3 -name "build.gradle*" -o -name "pom.xml" | head -10Repository: ably/ably-java
Length of output: 369
🏁 Script executed:
#!/bin/bash
# Check the build gradle for okhttp dependency
cat build.gradle 2>/dev/null | grep -i okhttp -B 1 -A 1Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the okhttp network client build file
cat network-client-okhttp/build.gradle.kts | grep -i okhttp -B 1 -A 1Repository: ably/ably-java
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Let's verify the actual behavior by checking how contentType is used
# Look at a practical example - check MessageSerializer more carefully
cat -n lib/src/main/java/io/ably/lib/types/MessageSerializer.java | sed -n '180,200p'Repository: ably/ably-java
Length of output: 1124
🏁 Script executed:
#!/bin/bash
# Check if there are integration tests or examples showing actual HTTP responses
find . -name "*.java" -type f | xargs grep -l "application/json.*charset" 2>/dev/null | head -5Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check what the Ably server actually sends - look for API documentation or tests
rg "Content-Type\|Content-type" lib/src/test/ -B 2 -A 2 | head -50Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if Serialisation.java's approach (throwing exception) is the preferred pattern
cat -n lib/src/main/java/io/ably/lib/util/Serialisation.java | sed -n '153,170p'Repository: ably/ably-java
Length of output: 1090
🏁 Script executed:
#!/bin/bash
# Verify the exact return behavior - do other handlers return null or throw?
rg "return null" lib/src/main/java/io/ably/lib/types/*.java | grep -i "bodyhandler\|handleResponseBody" -A 5 -B 5Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check what Ably spec says - look at any documentation or comments
rg -i "ably.*spec|rfc.*content.type" lib/src/main/java/ | head -10Repository: ably/ably-java
Length of output: 1639
🏁 Script executed:
#!/bin/bash
# Let's see if there's any normalization of content-type happening at HttpCore level
cat -n lib/src/main/java/io/ably/lib/http/HttpCore.java | sed -n '470,490p'Repository: ably/ably-java
Length of output: 1033
🏁 Script executed:
#!/bin/bash
# Check what the actual problem is - does OkHttp include charset in toString()?
# Look for any OkHttp documentation or tests in the codebase
find . -name "*.java" -type f | xargs grep -l "MediaType\|contentType" network-client-okhttp/ | head -5Repository: ably/ably-java
Length of output: 435
🏁 Script executed:
#!/bin/bash
# Check how HttpBody is created and if it processes contentType
cat -n lib/src/main/java/io/ably/lib/http/HttpUtils.java | sed -n '200,240p'Repository: ably/ably-java
Length of output: 1714
🏁 Script executed:
#!/bin/bash
# Look at HttpBody class definition
rg -A 10 "class HttpBody" lib/src/main/java/ --type=javaRepository: ably/ably-java
Length of output: 1092
🏁 Script executed:
#!/bin/bash
# Final check: see if there's any stripping of charset happening anywhere
rg -n "substring|split|indexOf.*;" lib/src/main/java/io/ably/lib/http/ --type=java | grep -i "content\|type"Repository: ably/ably-java
Length of output: 182
Content-type comparison fails with charset parameters, and returning null in array is inconsistent with codebase patterns.
The equals() check won't match content types that include charset parameters (e.g., "application/json; charset=utf-8"). Both OkHttp's MediaType.toString() and URLConnection.getContentType() include charset in their output. Additionally, returning an array containing null instead of throwing an exception is inconsistent with the Serialisation.java pattern, which throws an exception for unknown content types (the better approach).
🤖 Prompt for AI Agents
In @lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java around lines 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.
| Helpers.AsyncWaiter<UpdateDeleteResult> updateWaiter = new Helpers.AsyncWaiter<>(); | ||
| channel.updateMessageAsync(updateMessage, updateWaiter); | ||
|
|
||
| ErrorInfo[] updateErrors = updateComplete.waitFor(); | ||
| assertEquals("Expected no errors from update", 0, updateErrors.length); | ||
| updateWaiter.waitFor(); | ||
|
|
||
| // Retrieve the updated message | ||
| Message updatedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial); | ||
| assertNotNull("Expected non-null updated message", updatedMessage); | ||
| assertEquals("Expected updated message data", "Updated message data async", updatedMessage.data); | ||
| assertEquals(updateWaiter.result.versionSerial, updatedMessage.version.serial); |
There was a problem hiding this comment.
Add null/error check before accessing async result.
The test accesses updateWaiter.result.versionSerial without checking if updateWaiter.error is set. If the async operation fails, result could be null, causing an NPE that masks the actual error.
💡 Suggested fix
updateWaiter.waitFor();
+assertNull("Expected no error", updateWaiter.error);
+assertNotNull("Expected non-null result", updateWaiter.result);
// Retrieve the updated message
Message updatedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Helpers.AsyncWaiter<UpdateDeleteResult> updateWaiter = new Helpers.AsyncWaiter<>(); | |
| channel.updateMessageAsync(updateMessage, updateWaiter); | |
| ErrorInfo[] updateErrors = updateComplete.waitFor(); | |
| assertEquals("Expected no errors from update", 0, updateErrors.length); | |
| updateWaiter.waitFor(); | |
| // Retrieve the updated message | |
| Message updatedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial); | |
| assertNotNull("Expected non-null updated message", updatedMessage); | |
| assertEquals("Expected updated message data", "Updated message data async", updatedMessage.data); | |
| assertEquals(updateWaiter.result.versionSerial, updatedMessage.version.serial); | |
| Helpers.AsyncWaiter<UpdateDeleteResult> updateWaiter = new Helpers.AsyncWaiter<>(); | |
| channel.updateMessageAsync(updateMessage, updateWaiter); | |
| updateWaiter.waitFor(); | |
| assertNull("Expected no error", updateWaiter.error); | |
| assertNotNull("Expected non-null result", updateWaiter.result); | |
| // Retrieve the updated message | |
| Message updatedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial); | |
| assertNotNull("Expected non-null updated message", updatedMessage); | |
| assertEquals("Expected updated message data", "Updated message data async", updatedMessage.data); | |
| assertEquals(updateWaiter.result.versionSerial, updatedMessage.version.serial); |
🤖 Prompt for AI Agents
In @lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
around lines 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.
| Helpers.AsyncWaiter<UpdateDeleteResult> deleteWaiter = new Helpers.AsyncWaiter<>(); | ||
| channel.deleteMessageAsync(deleteMessage, deleteWaiter); | ||
|
|
||
| ErrorInfo[] deleteErrors = deleteComplete.waitFor(); | ||
| assertEquals("Expected no errors from delete", 0, deleteErrors.length); | ||
| deleteWaiter.waitFor(); | ||
|
|
||
| // Retrieve the deleted message | ||
| Message deletedMessage = waitForDeletedMessageAppear(channel, publishedMessage.serial); | ||
| assertNotNull("Expected non-null deleted message", deletedMessage); | ||
| assertEquals("Expected action to be MESSAGE_DELETE", MessageAction.MESSAGE_DELETE, deletedMessage.action); | ||
| assertEquals(deleteWaiter.result.versionSerial, deletedMessage.version.serial); |
There was a problem hiding this comment.
Add null/error check before accessing async result.
Same issue as in updateMessage_async - the test accesses deleteWaiter.result.versionSerial without checking for errors first.
💡 Suggested fix
deleteWaiter.waitFor();
+assertNull("Expected no error", deleteWaiter.error);
+assertNotNull("Expected non-null result", deleteWaiter.result);
// Retrieve the deleted message
Message deletedMessage = waitForDeletedMessageAppear(channel, publishedMessage.serial);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Helpers.AsyncWaiter<UpdateDeleteResult> deleteWaiter = new Helpers.AsyncWaiter<>(); | |
| channel.deleteMessageAsync(deleteMessage, deleteWaiter); | |
| ErrorInfo[] deleteErrors = deleteComplete.waitFor(); | |
| assertEquals("Expected no errors from delete", 0, deleteErrors.length); | |
| deleteWaiter.waitFor(); | |
| // Retrieve the deleted message | |
| Message deletedMessage = waitForDeletedMessageAppear(channel, publishedMessage.serial); | |
| assertNotNull("Expected non-null deleted message", deletedMessage); | |
| assertEquals("Expected action to be MESSAGE_DELETE", MessageAction.MESSAGE_DELETE, deletedMessage.action); | |
| assertEquals(deleteWaiter.result.versionSerial, deletedMessage.version.serial); | |
| Helpers.AsyncWaiter<UpdateDeleteResult> deleteWaiter = new Helpers.AsyncWaiter<>(); | |
| channel.deleteMessageAsync(deleteMessage, deleteWaiter); | |
| deleteWaiter.waitFor(); | |
| assertNull("Expected no error", deleteWaiter.error); | |
| assertNotNull("Expected non-null result", deleteWaiter.result); | |
| // Retrieve the deleted message | |
| Message deletedMessage = waitForDeletedMessageAppear(channel, publishedMessage.serial); | |
| assertNotNull("Expected non-null deleted message", deletedMessage); | |
| assertEquals("Expected action to be MESSAGE_DELETE", MessageAction.MESSAGE_DELETE, deletedMessage.action); | |
| assertEquals(deleteWaiter.result.versionSerial, deletedMessage.version.serial); |
🤖 Prompt for AI Agents
In @lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
around lines 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.
Adds message appends and protocol v5 upgrade
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.