[ECO-5642] feat: add message editing support to channels #1179
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds REST and realtime message edit/delete/version APIs: new MessageEditsMixin, MessageOperation model and serializers, single-message response handling, BaseMessage encode refactor, and end-to-end REST tests for edit/delete/version flows. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant CB as ChannelBase
participant MEM as MessageEditsMixin
participant HTTP as Http Layer
participant Server as Ably Server
rect rgb(245,250,255)
Note left of App: Retrieve latest message by serial (sync)
App->>CB: getMessage(serial)
CB->>MEM: getMessage(http, serial)
MEM->>HTTP: GET /channels/{chan}/messages/{serial}
HTTP->>Server: HTTP GET
Server-->>HTTP: 200 + payload (JSON/MsgPack)
HTTP-->>MEM: parsed Message
MEM-->>CB: Message
CB-->>App: Message
end
rect rgb(245,250,255)
Note left of App: Update message (PATCH)
App->>CB: updateMessage(message, operation?)
CB->>MEM: updateMessage(http, message, operation)
MEM->>HTTP: PATCH /channels/{chan}/messages/{serial} with body (JSON/MsgPack)
HTTP->>Server: HTTP PATCH
Server-->>HTTP: 200
HTTP-->>MEM: success
MEM-->>CB: void
CB-->>App: void
end
rect rgb(245,250,255)
Note left of App: Delete message (soft/hard)
App->>CB: deleteMessage(message, operation?)
CB->>MEM: deleteMessage(http, message, operation)
MEM->>HTTP: POST /channels/{chan}/messages/{serial}/delete (body)
HTTP->>Server: HTTP POST
Server-->>HTTP: 200
HTTP-->>MEM: success
MEM-->>CB: void
CB-->>App: void
end
rect rgb(245,250,255)
Note left of App: Get versions (paginated)
App->>CB: getMessageVersions(serial, params)
CB->>MEM: getMessageVersions(http, serial, params)
MEM->>HTTP: GET /channels/{chan}/messages/{serial}/versions?...
HTTP->>Server: HTTP GET
Server-->>HTTP: pages of versions
HTTP-->>MEM: PaginatedResult<Message>
MEM-->>CB: PaginatedResult
CB-->>App: PaginatedResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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/1179/features
github-actions
bot
temporarily deployed
to
staging/pull/1179/javadoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
52-52: REST message edit delegates look good; tweak version docs and async signatureThe new methods are thin wrappers over
messageEditsMixinand fit the existing RESTChannelBasepattern.Two follow‑ups to consider:
- Javadoc for
getMessageVersionssays “ordered chronologically”, but tests assert the newest version comes first. It would be clearer to document the actual default order (e.g. “reverse chronological (newest first), matching channel history defaults”) and mention how to change it via params if supported.getMessageVersionsAsyncdeclaresthrows AblyException, unlike other async methods here. Because the only synchronous failure right now is validation in the mixin (null/empty serial), it would be more consistent to catch that in the async wrapper and surface it viacallback.onError(...), then dropthrows AblyExceptionfrom the async signature.Also applies to: 320-429, 436-444
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
239-252: AsyncgetMessageVersions*methods unexpectedly throw; keep validation inside the async flow
getMessageVersionsImplvalidatesserialand throws anAblyExceptionwhen it’s null/empty. Because bothgetMessageVersions(...)andgetMessageVersionsAsync(...)call this helper, the async variants end up declaringthrows AblyExceptionand can fail synchronously, unlike the other async edit methods which always report failures via callbacks/listeners.For API consistency and to match existing patterns:
- Either move the
serialvalidation into theHttp.requestlambda (likeupdateMessageImpl/deleteMessageImpl), so even async calls report errors via the callback, or- Wrap the
getMessageVersionsImplcall in the async method with atry/catchthat convertsAblyExceptiontocallback.onError(e.errorInfo)and removethrows AblyExceptionfrom the async signatures in this mixin and both ChannelBase classes.This keeps all
*AsyncAPIs behaving uniformly and avoids surprising callers with synchronous exceptions from an async method.Also applies to: 254-262
📜 Review details
Configuration used: CodeRabbit 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 (4)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(4 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java(1 hunks)lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (4)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (3)
Helpers(74-1229)AsyncWaiter(936-962)CompletionSet(862-912)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (4)
lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageDecodeException.java (1)
MessageDecodeException(6-22)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (12)
lib/src/main/java/io/ably/lib/http/BasePaginatedQuery.java (1)
BasePaginatedQuery(25-345)lib/src/main/java/io/ably/lib/http/Http.java (1)
Http(10-99)lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/http/HttpScheduler.java (1)
HttpScheduler(23-457)lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)lib/src/main/java/io/ably/lib/types/ClientOptions.java (1)
ClientOptions(19-392)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageDecodeException.java (1)
MessageDecodeException(6-22)lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
MessageSerializer(23-189)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)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/rest/MessageEditsMixin.java (1)
MessageEditsMixin(24-264)
⏰ 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 (19)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check-rest-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-rest
- GitHub Check: build
- GitHub Check: check-realtime
- GitHub Check: check-realtime-okhttp
- GitHub Check: check
🔇 Additional comments (1)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
48-513: Solid coverage of REST message edit, delete, and version flowsThe test suite exercises sync/async get/update/delete plus version history and error paths in a realistic end‑to‑end fashion, which aligns well with the new API surface and should catch most regressions around message edits.
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds message editing support to channels, enabling retrieval, update, and deletion of messages by serial identifier, along with message version history tracking. The implementation introduces a new MessageEditsMixin class that encapsulates all message editing operations and is integrated into both REST and Realtime channel classes.
- Introduced
MessageEditsMixinto handle message editing operations (get, update, delete, and version history) - Added message editing APIs to both REST and Realtime
ChannelBaseclasses - Implemented comprehensive test coverage for all message editing operations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
MessageEditsMixin.java |
Core implementation of message editing operations with HTTP request handling for get, update, delete, and version retrieval |
ChannelBase.java (REST) |
Integration of MessageEditsMixin and public API methods for REST channels with added imports |
ChannelBase.java (Realtime) |
Integration of MessageEditsMixin and public API methods for Realtime channels |
RestChannelMessageEditTest.java |
Comprehensive test suite covering all message editing operations including error handling and workflow tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
|
Overall PR looks good. Couple of comments added
|
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
ECO-5642/edit-deletes-api
branch
from
a46f753 to
548ea8a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
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.
LGTM : )
PS. You can check/resolve review comments and check for failing CI.
ttypic
force-pushed
the
ECO-5642/edit-deletes-api
branch
from
548ea8a to
8448c8c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1179/features
github-actions
bot
temporarily deployed
to
staging/pull/1179/javadoc
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
1324-1351: Consider adding convenience overloads withoutparamsparameter.The tests and common usage patterns suggest that callers often don't need query parameters. Adding convenience overloads would improve the API ergonomics:
public PaginatedResult<Message> getMessageVersions(String serial) throws AblyException { return getMessageVersions(serial, null); } public void getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { getMessageVersionsAsync(serial, null, callback); }
105-106:MessageEditsMixinmay use staleChannelOptionsaftersetOptions()is called.The
messageEditsMixinis initialized once in the constructor with the initialoptionsreference. However,setOptions()at line 1458 can replacethis.optionswith a newChannelOptionsinstance (e.g., when changing encryption), but the mixin continues to use the original options it captured at construction.This can cause inconsistent behavior where publish/subscribe/history use updated options while edit/delete/version calls still use the old ones (notably for encryption/decoding).
Consider updating the mixin when options change:
public void setOptions(ChannelOptions options, CompletionListener listener) throws AblyException { this.options = options; + this.messageEditsMixin.setChannelOptions(options); if(this.shouldReattachToSetOptions(options)) { this.attach(true, listener); } else { callCompletionListenerSuccess(listener); } }This requires adding a
setChannelOptions(ChannelOptions)method toMessageEditsMixin.Also applies to: 1535-1535
1265-1276: Incomplete Javadoc for@param message.The documentation for the
messageparameter at line 1273 is truncated: "A {@link Message} object that may contain."Consider completing it:
- * @param message A {@link Message} object that may contain. + * @param message A {@link Message} object that may contain operation metadata such as + * clientId, description, or metadata in the version field.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (3)
117-140: Input message is mutated by settingactionfield.Line 123 directly modifies the caller's
message.actionfield, which may be unexpected. Callers may not anticipate their Message object being modified.Consider documenting this side effect in the Javadoc, or creating a defensive copy:
private Http.Request<Void> updateMessageImpl(Http http, String serial, Message message, MessageOperation operation) { return http.request((scheduler, callback) -> { if (serial == null || serial.isEmpty()) { throw AblyException.fromErrorInfo(new ErrorInfo("Message serial cannot be empty", 400, 40003)); } - /* Set action to MESSAGE_UPDATE */ - message.action = MessageAction.MESSAGE_UPDATE; + /* Create working copy with action set to MESSAGE_UPDATE */ + Message workingMessage = new Message(); + workingMessage.name = message.name; + workingMessage.data = message.data; + workingMessage.extras = message.extras; + workingMessage.action = MessageAction.MESSAGE_UPDATE; /* RTL6g3 */ - auth.checkClientId(message, true, false); + auth.checkClientId(workingMessage, true, false);
171-194: Same input mutation issue indeleteMessageImpl.Line 177 mutates
message.actiontoMESSAGE_DELETE. Apply the same fix as suggested forupdateMessageImpl.
127-129: Request body construction is duplicated.The pattern for creating
HttpCore.RequestBodybased onuseBinaryProtocolis repeated in bothupdateMessageImplanddeleteMessageImpl. Consider extracting a helper method inMessageOperationSerializer:public static HttpCore.RequestBody asRequest(Message message, MessageOperation operation, ChannelOptions channelOptions, boolean useBinaryProtocol) throws AblyException { return useBinaryProtocol ? asMsgPackRequest(message, operation, channelOptions) : asJsonRequest(message, operation, channelOptions); }Then both methods can simply call:
HttpCore.RequestBody requestBody = MessageOperationSerializer.asRequest( message, operation, channelOptions, clientOptions.useBinaryProtocol);Also applies to: 181-183
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
70-78: Incorrect helper method for retrieving a newly published message.
waitForUpdatedMessageAppearwaits for a message withaction == MESSAGE_UPDATE, but a freshly published message hasaction == MESSAGE_CREATE(or null). This test will timeout waiting for an update that never happened.Use
channel.getMessage(serial)directly since no update is expected:// Retrieve the message by serial - Message retrievedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial); + Message retrievedMessage = channel.getMessage(publishedMessage.serial);Or create a separate helper that doesn't filter by action.
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
191-224: Potential null element in returned array when content-type is unrecognized.If
contentTypeis neither"application/json"nor"application/x-msgpack",messageremainsnulland the method returnsnew Message[] { null }. Callers that iterate the array or accessmessages[0]will receivenull, which may not be expected behavior.Consider throwing an exception for unsupported content types (consistent with
Serialisation.HttpBodyHandler) or returning an empty array:+ if (message == null) { + throw AblyException.fromThrowable(new Exception("unknown content type " + contentType)); + } + if (message != null) { try { message.decode(opts);Additionally, Line 206 creates a throwaway
new Message()instance just to callreadMsgpack. IfreadMsgpackreturns a newMessage, consider usingMessage.fromMsgpack(unpacker)directly (if available) to avoid the unnecessary allocation.lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
434-442: Operator precedence is correct.The condition at line 438 uses proper parentheses to group the predicate check with the array length check before the timeout
||condition:if (history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout)However, for clarity and to match the pattern in other helper methods, consider adding explicit parentheses:
- if (history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout) + if ((history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout)
📜 Review details
Configuration used: CodeRabbit 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 (8)
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(1 hunks)lib/src/main/java/io/ably/lib/types/BaseMessage.java(2 hunks)lib/src/main/java/io/ably/lib/types/MessageOperation.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageSerializer.java(2 hunks)lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/main/java/io/ably/lib/types/BaseMessage.java
- lib/src/main/java/io/ably/lib/types/MessageOperation.java
- lib/src/main/java/io/ably/lib/rest/ChannelBase.java
🧰 Additional context used
🧬 Code graph analysis (4)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (11)
lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (1)
keys(71-78)liveobjects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (1)
testParams(18-97)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt (1)
channels(3-9)pubsub-adapter/src/test/kotlin/com/ably/Utils.kt (1)
waitFor(11-20)
lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (4)
lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)lib/src/main/java/io/ably/lib/util/Base64Coder.java (1)
Base64Coder(30-239)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
MessageEditsMixin(25-234)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (9)
lib/src/main/java/io/ably/lib/http/BasePaginatedQuery.java (1)
BasePaginatedQuery(25-345)lib/src/main/java/io/ably/lib/http/Http.java (1)
Http(10-99)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
MessageOperationSerializer(18-172)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)
⏰ 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-liveobjects
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (19)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check (21)
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check
- GitHub Check: check (29)
- GitHub Check: build
🔇 Additional comments (5)
lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
150-168: JSON serialization correctly handles binary data encoding.The
asJsonObjectmethod properly base64-encodes binary data and appends/base64to the encoding string. This aligns with Ably's encoding conventions for JSON payloads.lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
453-471: Helper methods correctly handle operator precedence.The
waitForUpdatedMessageAppearandwaitForDeletedMessageAppearmethods now have proper parentheses around the compound conditions before the||timeout check. This ensures correct evaluation order.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1179-1206: Message retrieval APIs look good.The
getMessageandgetMessageAsyncmethods properly delegate to the mixin with appropriate parameters. The Javadoc clearly documents the behavior and threading model for async callbacks.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (2)
68-87: getMessage implementation correctly uses the new single-message handler.The implementation properly validates the serial, uses the new
getSingleMessageResponseHandler, and throws appropriate errors for missing messages. Good use of the HTTP request pattern.
196-232: Version history retrieval is well-implemented.The
getMessageVersionsmethods properly useBasePaginatedQueryfor pagination support, validate the serial upfront, and correctly pass through query parameters. The Javadoc could clarify the ordering direction (chronological vs reverse chronological) as noted in past reviews.
ttypic
force-pushed
the
ECO-5642/edit-deletes-api
branch
from
8448c8c to
b5476b1
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1179/features
github-actions
bot
temporarily deployed
to
staging/pull/1179/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
1337-1350: Missing convenience overloads forgetMessageVersionsandgetMessageVersionsAsync.This was noted in previous reviews. Consider adding overloads without the
paramsparameter for consistency with other API methods likehistory():/** * Retrieves all historical versions of a specific message. * * @param serial The unique serial identifier of the message. * @return A {@link PaginatedResult} containing all versions of the message. * @throws AblyException If the versions cannot be retrieved. */ public PaginatedResult<Message> getMessageVersions(String serial) throws AblyException { return getMessageVersions(serial, null); } /** * Asynchronously retrieves all historical versions of a specific message. * * @param serial The unique serial identifier of the message. * @param callback A callback to handle the result asynchronously. */ public void getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { getMessageVersionsAsync(serial, null, callback); }
105-106:MessageEditsMixinwill use staleChannelOptionsaftersetOptions()is called.This issue was raised in a previous review. The
messageEditsMixinis initialized once in the constructor with the initialoptionsreference. WhensetOptions(...)replacesthis.options, the mixin continues using the originalChannelOptions, leading to inconsistent behavior (e.g., different encryption settings for edit/delete vs. publish/history).Add a setter on
MessageEditsMixinand call it fromsetOptions():public void setOptions(ChannelOptions options, CompletionListener listener) throws AblyException { this.options = options; + this.messageEditsMixin.setChannelOptions(options); if(this.shouldReattachToSetOptions(options)) { this.attach(true, listener); } else { callCompletionListenerSuccess(listener); } }Also applies to: 1535-1535
1265-1276: Incomplete javadoc formessageparameter.The
@param messagedocumentation is cut off mid-sentence at line 1273.- * @param message A {@link Message} object that may contain. + * @param message A {@link Message} object that may contain operation metadata such as + * clientId, description, or metadata in the version field.lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
55-66: Returningnullon IOException will cause NPE downstream.This issue was flagged in a previous review. If
writeMsgpackencounters anIOException, it logs the error and returnsnull. Thisnullis then passed toByteArrayRequestBodyat line 46, which will cause aNullPointerException.Propagate the exception instead:
- private static byte[] writeMsgpack(UpdateDeleteRequest request) { + private static byte[] writeMsgpack(UpdateDeleteRequest request) throws AblyException { try { ByteArrayOutputStream out = new ByteArrayOutputStream(); MessagePacker packer = Serialisation.msgpackPackerConfig.newPacker(out); request.writeMsgpack(packer); packer.flush(); return out.toByteArray(); } catch (IOException e) { - Log.e(TAG, "Failed to write msgpack", e); - return null; + throw AblyException.fromThrowable(e); } }lib/src/main/java/io/ably/lib/rest/ChannelBase.java (2)
403-417: Complete thedeleteMessageJavadoc for themessageparameterThe first
deleteMessage(String serial, Message message, MessageOperation operation)overload still has an incomplete@param messagedescription ("may contain."), while the second overload has the full, intended wording. Please align the first overload with the completed text.- * @param message A {@link Message} object that may contain. + * @param message A {@link Message} object that may contain operation metadata such as + * clientId, description, or metadata in the version field.
462-488: Clarify version ordering and add convenience overloads forgetMessageVersions(sync & async)Two things here:
- Document the actual ordering of versions
Tests and expected behavior indicate versions are returned newest-first, but the Javadoc currently says “ordered chronologically”, which is ambiguous and typically implies oldest-first. Please make this explicit:
- * This method returns a paginated result containing all versions of the message, - * ordered chronologically. Each version includes metadata about when and by whom + * This method returns a paginated result containing all versions of the message, + * ordered in reverse chronological order (newest first). Each version includes metadata about when and by whom
- Provide no-params convenience overloads (mirroring usage in tests)
There are calls like
channel.getMessageVersions(publishedMessage.serial)andchannel.getMessageVersionsAsync(publishedMessage.serial, waiter), but the implementation only exposes theParam[]variants. Adding convenience overloads keeps API parity with other methods (e.g.,history()) and fixes the compile-time gap./** * Retrieves all historical versions of a specific message. @@ * @throws AblyException If the versions cannot be retrieved. */ public PaginatedResult<Message> getMessageVersions(String serial, Param[] params) throws AblyException { return messageEditsMixin.getMessageVersions(ably.http, serial, params); } + + /** + * Convenience overload for retrieving all historical versions of a specific message + * without specifying query parameters. + * + * @param serial The unique serial identifier of the message. + * @return A {@link PaginatedResult} containing an array of {@link Message} objects + * representing all versions of the message. + * @throws AblyException If the versions cannot be retrieved. + */ + public PaginatedResult<Message> getMessageVersions(String serial) throws AblyException { + return getMessageVersions(serial, null); + } @@ * @param params Query parameters for filtering or pagination. * @param callback A callback to handle the result asynchronously. */ public void getMessageVersionsAsync(String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { messageEditsMixin.getMessageVersionsAsync(ably.http, serial, params, callback); } + + /** + * Convenience overload for asynchronously retrieving all historical versions of a specific message + * without specifying query parameters. + * + * @param serial The unique serial identifier of the message. + * @param callback A callback to handle the result asynchronously. + * @throws AblyException If the versions cannot be retrieved. + */ + public void getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { + getMessageVersionsAsync(serial, null, callback); + }
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (2)
480-488: Align asyncgetMessageVersionsAsynccontract (Javadoc vsthrows)
getMessageVersionsAsyncdeclaresthrows AblyExceptionbut the Javadoc doesn’t document any synchronous exception behavior, unlike other async methods in this class that generally don’t declare checked exceptions at all.Consider either:
- documenting when a synchronous
AblyExceptioncan be thrown (e.g., invalid serial/params pre-validation), or- adjusting the design to keep async methods exception-free at the signature level and route all failures via the callback, for consistency with
historyAsyncand presence async methods.
497-505: ConfirmMessageEditsMixinstays in sync with runtimeChannelOptionschanges
ChannelBase.optionsis mutable (non-final), butMessageEditsMixinreceives theoptionsreference only at construction:this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth);If caller code can replace
ChannelOptionswith a new instance after channel creation (rather than mutating the existing object),messageEditsMixinwill continue using the original options, potentially missing updated encryption or other per-channel configuration.You may want to:
- verify how
ChannelOptionsare updated at runtime for REST channels, and- if replacement is possible, either:
- pass a provider (or
ChannelBaseitself) intoMessageEditsMixinso it always reads the current options, or- expose a small
setOptions(ChannelOptions options)hook onMessageEditsMixinand call it from whereverChannelBase.optionsis changed.
📜 Review details
Configuration used: CodeRabbit 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 (8)
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(1 hunks)lib/src/main/java/io/ably/lib/types/BaseMessage.java(2 hunks)lib/src/main/java/io/ably/lib/types/MessageOperation.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageSerializer.java(2 hunks)lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java
- lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java
🧰 Additional context used
🧬 Code graph analysis (3)
lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (3)
lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)lib/src/main/java/io/ably/lib/util/Base64Coder.java (1)
Base64Coder(30-239)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (2)
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/MessageOperation.java (1)
MessageOperation(14-100)
⏰ 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-liveobjects
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check (29)
- GitHub Check: build
🔇 Additional comments (8)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
157-159: LGTM!The factory method follows the established pattern for body handlers in this class.
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
1-100: LGTM!The
MessageOperationclass follows existing conventions in the codebase with public fields and package-private serialization methods. The implementation correctly handles optional fields by checking for null before serialization in both JSON and MessagePack formats.lib/src/main/java/io/ably/lib/types/BaseMessage.java (2)
182-215: LGTM!The refactoring cleanly separates the encoding logic from the mutation of instance fields. The new
encodeData()method enablesMessageOperationSerializerto encode message data without modifying the originalMessageobject. The control flow correctly handles all data type cases (JsonElement, String, byte[]).
383-391: LGTM!The
EncodedMessageDatainner class is a clean, immutable data holder appropriate for returning encoded results.lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
72-169: LGTM!The
UpdateDeleteRequestclass correctly handles encoding of message data and serialization to both JSON and MessagePack formats. The base64 encoding for binary data in JSON output follows the established pattern fromBaseMessage.toJsonObject.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
1179-1236: LGTM!The
getMessage,updateMessage, and their async variants are implemented correctly with proper delegation tomessageEditsMixin. The overloads withoutMessageOperationparameter provide good API ergonomics.
1277-1322: LGTM!The
deleteMessagemethods and their async variants follow the same consistent delegation pattern and provide appropriate overloads.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
317-401: Overall message edits/deletes API shape looks goodThe new edit/delete APIs (sync + async) with
MessageOperationmetadata and overloads without theMessageOperationparameter follow existing ChannelBase patterns and give a clear, ergonomic surface for the new feature.
ttypic
force-pushed
the
ECO-5642/edit-deletes-api
branch
from
b5476b1 to
29d663b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1179/features
github-actions
bot
temporarily deployed
to
staging/pull/1179/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (2)
434-442: Operator precedence makes condition confusing.Line 438's condition relies on implicit operator precedence (
&&before||) which makes the logic harder to follow. While functionally correct, explicit parentheses improve readability.- if (history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout) + if ((history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout)
70-78: Incorrect helper method: test waits for MESSAGE_UPDATE on a freshly published message.At line 72, the test calls
waitForUpdatedMessageAppear()which waits for a message withaction == MESSAGE_UPDATE. However, at this point the message was only published, not updated, so it should haveaction == MESSAGE_CREATEor similar. This will cause the test to timeout and return an unexpected state.Use
getMessagedirectly since no update was performed:// Retrieve the message by serial - Message retrievedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial); + Message retrievedMessage = channel.getMessage(publishedMessage.serial);lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
55-66: Returningnullon IOException will cause NPE downstream.This issue was flagged in a previous review. When
writeMsgpackencounters anIOException, returningnullcauses aNullPointerExceptionwhenByteArrayRequestBodyis constructed at line 46.Apply this diff to propagate the exception:
private static byte[] writeMsgpack(UpdateDeleteRequest request) { try { ByteArrayOutputStream out = new ByteArrayOutputStream(); MessagePacker packer = Serialisation.msgpackPackerConfig.newPacker(out); request.writeMsgpack(packer); packer.flush(); return out.toByteArray(); } catch (IOException e) { - Log.e(TAG, "Failed to write msgpack", e); - return null; + throw new RuntimeException("Failed to write msgpack", e); } }lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
197-223: Inconsistent behavior: returns array with null element for unsupported content types.This was flagged in a previous review. Two issues:
- Line 202:
new String(body)uses platform default charset instead of UTF-8- Line 219: Returns
new Message[] { message }even whenmessageisnull(unsupported content type), which differs fromMessageBodyHandlerthat returnsnullfor the arrayApply this diff to align behavior and fix charset:
@Override public Message[] handleResponseBody(String contentType, byte[] body) throws AblyException { try { Message message = null; if ("application/json".equals(contentType)) { - message = Serialisation.gson.fromJson(new String(body), Message.class); + message = Serialisation.gson.fromJson(new String(body, "UTF-8"), Message.class); } else if ("application/x-msgpack".equals(contentType)) { MessageUnpacker unpacker = Serialisation.msgpackUnpackerConfig.newUnpacker(body); try { message = new Message().readMsgpack(unpacker); } catch (IOException ioe) { throw AblyException.fromThrowable(ioe); } } if (message != null) { try { message.decode(opts); } catch (MessageDecodeException e) { Log.e(TAG, e.errorInfo.message); } + return new Message[] { message }; } - return new Message[] { message }; + return null; } catch (MessageDecodeException e) { throw AblyException.fromThrowable(e); } }lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
194-230: Clarify version ordering and consider aligning async error handling withhistoryAsync
- The Javadoc for
getMessageVersionssays versions are “ordered chronologically”, which is ambiguous and (per previous feedback/tests) likely incorrect. If the service returns newest‑first, update the text to something like:* This method returns a paginated result containing all versions of the message, * ordered in reverse chronological order (newest first). Each version includes * metadata about when and by whom the message was modified.and mirror that wording on the realtime
ChannelBasewrapper.
- For
getMessageVersionsAsync, the serial validation happens ingetMessageVersionsImpl(which throwsAblyException), so calling the async method can fail synchronously. Other async flows likehistoryAsyncinstead capture parameter errors into a failedResultRequestand surface them via the callback. If you want consistency, consider moving the serial check inside theBasePaginatedQuery(similar tohistoryImpl) and droppingthrows AblyExceptionfrom the async signature, so all errors are delivered through the callback.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1179-1352: Tighten message edit/delete/version API ergonomics and docsThe new realtime APIs nicely mirror the REST mixin, but a few polish items remain:
deleteMessage(String serial, Message message, MessageOperation operation)Javadoc has a truncated@param message(“may contain.”). Please complete this description (e.g. “may contain operation metadata such as clientId, description, or metadata in the version field”), consistent with the overload below.
getMessageVersionsJavadoc says versions are “ordered chronologically”, which is ambiguous and (per existing feedback and tests) should likely be “ordered in reverse chronological order (newest first)”. Please align the wording here with the actual behaviour and with the REST mixin Javadoc.Both update/delete overload sets assume a non‑null
Message messageand directly delegate toMessageEditsMixin, wheremessage.actionis written andauth.checkClientId(message, …)is called. If a caller accidentally passesnull, this will result in aNullPointerExceptionrather than anAblyException. Once you address this at the mixin level (see comment inMessageEditsMixin.updateMessageImpl/deleteMessageImpl), these wrappers will be safe too; no additional changes needed here, but it’s worth being aware of the dependency.For symmetry with
history(), consider adding convenience overloads:
public PaginatedResult<Message> getMessageVersions(String serial)→ delegates togetMessageVersions(serial, null)public void getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult<Message>> callback)→ delegates togetMessageVersionsAsync(serial, null, callback)
This matches prior feedback and aligns with the usage patterns in tests.
🧹 Nitpick comments (1)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1458-1466: Avoid double initialization ofmessageEditsMixin
setOptions(...)now assigns:this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth);and the constructor calls
this.setOptions(options);and then reassigns:this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth);The second assignment in the constructor is redundant and risks future divergence if
setOptions(...)logic changes.You can simplify by relying solely on
setOptions(options)to initialise the mixin and remove the explicit assignment in the constructor:ChannelBase(AblyRealtime ably, String name, ChannelOptions options, @Nullable LiveObjectsPlugin liveObjectsPlugin) throws AblyException { ... this.basePath = "/channels/" + HttpUtils.encodeURIComponent(name); this.setOptions(options); ... - this.annotations = new RealtimeAnnotations( + this.annotations = new RealtimeAnnotations( this, new RestAnnotations(name, ably.http, ably.options, options) ); - this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth); }This keeps all mixin construction in one place and removes unnecessary work.
Also applies to: 1518-1537
📜 Review details
Configuration used: CodeRabbit 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 (8)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(6 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java(1 hunks)lib/src/main/java/io/ably/lib/types/BaseMessage.java(2 hunks)lib/src/main/java/io/ably/lib/types/MessageOperation.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageSerializer.java(2 hunks)lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/main/java/io/ably/lib/rest/ChannelBase.java
🧰 Additional context used
🧬 Code graph analysis (4)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (5)
lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)
lib/src/main/java/io/ably/lib/types/BaseMessage.java (3)
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/util/Crypto.java (1)
Crypto(25-410)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
MessageEditsMixin(23-232)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (3)
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/util/Log.java (1)
Log(21-157)
⏰ 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: build
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-realtime-httpurlconnection
🔇 Additional comments (14)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
1-100: LGTM!The
MessageOperationclass is well-structured with clean serialization logic for both MessagePack and JSON formats. The null-handling for optional fields is correct, and the class follows the existing patterns in the codebase.lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
72-168: LGTM!The
UpdateDeleteRequestinner class correctly handles message encoding and serialization for both JSON and MessagePack formats. The base64 encoding logic for binary data in JSON output properly appends the encoding suffix.lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
157-159: LGTM!The factory method follows the existing pattern in the codebase.
lib/src/main/java/io/ably/lib/types/BaseMessage.java (2)
182-215: LGTM!The refactoring cleanly separates the encoding logic into
encodeData()which returns anEncodedMessageDataobject, whileencode()simply delegates and updates fields. This enables reuse byMessageOperationSerializerwithout duplicating encoding logic. The encryption path correctly operates on the localdecodedDatavariable.
383-391: LGTM!The
EncodedMessageDataclass is a clean, immutable data transfer object with appropriate package-private visibility for internal use.lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (7)
36-48: LGTM!Test class setup is well-structured with appropriate timeout configuration and standard initialization pattern.
84-117: LGTM!The
updateMessage_updateDatatest correctly verifies the update workflow: publish → retrieve → update → verify updated data andMESSAGE_UPDATEaction.
119-156: LGTM!Good coverage of encrypted channel message updates with
ChannelOptions.withCipherKey.
195-261: LGTM!The delete tests (
deleteMessage_softDeleteanddeleteMessage_async) correctly verify soft-delete behavior andMESSAGE_DELETEaction.
263-332: LGTM!The
getMessageVersions_retrieveHistorytest comprehensively verifies version history including metadata fromMessageOperation.
337-387: LGTM!Error handling tests appropriately verify exceptions for invalid, null, and empty serials.
389-432: LGTM!The
completeWorkflow_publishUpdateVersionsDeletetest provides excellent end-to-end coverage of the full message lifecycle.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
19-37: MessageEditsMixin wiring into ChannelBase looks correctImporting
MessageEditsMixin/MessageOperationand storing avolatile MessageEditsMixinfield, then delegating all new APIs through it, is a reasonable way to keep the realtime surface thin and reuse the REST implementation. Assuming the mixin stays in sync withChannelOptions(see comment on ctor/setOptions), there are no functional issues here.Also applies to: 105-105
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
25-38: Constructor andgetMessageimplementation look consistent with existing REST patternsCapturing
basePath,ClientOptions,ChannelOptions, andAuthin the constructor and then usingMessageSerializer.getSingleMessageResponseHandler(channelOptions)plus request IDs ingetMessage*closely mirrors the existing history/REST flows and addresses earlier concerns about content‑type/charset handling. No issues spotted here.Also applies to: 40-85
ttypic
force-pushed
the
ECO-5642/edit-deletes-api
branch
from
29d663b to
51d3719
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1179/features
github-actions
bot
temporarily deployed
to
staging/pull/1179/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
54-79: Use a non‑update helper (or directgetMessage) ingetMessage_retrieveBySerial
getMessage_retrieveBySerialcallswaitForUpdatedMessageAppear, which polls until it seesMESSAGE_UPDATE, but this test never updates the message. The helper will just spin until the timeout elapses and then return whatevergetMessagelast returned, adding latency and hiding the fact that no update is expected here.Consider simplifying this test to call
channel.getMessage(publishedMessage.serial)directly (or introduce a helper that just waits for any non‑null message by serial) instead of reusing the “updated” helper.Also applies to: 464-472
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
191-223: Avoid returning an array containingnulland use explicit UTF‑8 decoding
SingleMessageBodyHandler.handleResponseBodycurrently:
- Returns
new Message[] { message }even whenmessageisnull, so callers likegetMessageImplwill treat “no message parsed” as a successful lookup and returnnullinstead of throwing “Message not found”.- Uses
new String(body)which depends on the platform default charset rather than UTF‑8.Consider aligning behavior with
MessageBodyHandler:
- When no message can be parsed (unsupported content type or parse failure), return
null(or an empty array) instead of a single‑element array containingnull, somessages != null && messages.length > 0only passes when a real message exists.- Decode JSON using UTF‑8 explicitly, e.g. via
new String(body, StandardCharsets.UTF_8)and add the corresponding import.Example sketch:
- if ("application/json".equals(contentType)) { - message = Serialisation.gson.fromJson(new String(body), Message.class); + if ("application/json".equals(contentType)) { + message = Serialisation.gson.fromJson( + new String(body, java.nio.charset.StandardCharsets.UTF_8), + Message.class + ); } @@ - if (message != null) { + if (message != null) { try { message.decode(opts); } catch (MessageDecodeException e) { Log.e(TAG, e.errorInfo.message); } - } - return new Message[] { message }; + return new Message[] { message }; + } + return null;This avoids surprising
nullelements and keeps charset handling deterministic.lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
55-66: Don’t returnnullfromwriteMsgpack– propagate the failure insteadIf
writeMsgpackhits anIOException, it logs the error and returnsnull, which is then passed intonew HttpUtils.ByteArrayRequestBody(packed, ...). That will likely cause aNullPointerExceptionat request encoding time instead of a clear, Ably‑level exception.Given
asMsgPackRequestalready declaresthrows AblyException, it’s safer to propagate:- private static byte[] writeMsgpack(UpdateDeleteRequest request) { + private static byte[] writeMsgpack(UpdateDeleteRequest request) throws AblyException { try { ByteArrayOutputStream out = new ByteArrayOutputStream(); MessagePacker packer = Serialisation.msgpackPackerConfig.newPacker(out); request.writeMsgpack(packer); packer.flush(); return out.toByteArray(); } catch (IOException e) { - Log.e(TAG, "Failed to write msgpack", e); - return null; + Log.e(TAG, "Failed to write msgpack", e); + throw AblyException.fromThrowable(e); } }and keep
asMsgPackRequestunchanged except for handling the newthrows AblyExceptiononwriteMsgpack. This avoids surprising NPEs and surfaces a meaningful error to callers.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
97-137: Guard againstnullmessages in update/delete operationsBoth
updateMessageImplanddeleteMessageImplassumemessageis non‑null and immediately dereferencemessage.serial. If a caller accidentally passesnull, this will surface as aNullPointerExceptionrather than a clearAblyExceptiondescribing invalid input.Consider adding an explicit guard near the top of each method:
private Http.Request<Void> updateMessageImpl(Http http, Message message, MessageOperation operation) { 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)); } @@ private Http.Request<Void> deleteMessageImpl(Http http, Message message, MessageOperation operation) { 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)); }This yields a predictable, Ably‑style error instead of an opaque NPE and aligns with the existing serial validation.
Also applies to: 148-187
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
1449-1452: Avoid redundantmessageEditsMixininitialisation
setOptionsinitialisesmessageEditsMixin(Line 1451), and the constructor both callssetOptions(options)and then reassignsmessageEditsMixinagain at Line 1527. The second assignment is redundant.You can simplify by relying on
setOptionsfor initialisation and dropping the explicit assignment in the constructor:- this.setOptions(options); + this.setOptions(options); @@ - this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth);This keeps the lifecycle in one place and avoids duplication.
Also applies to: 1527-1528
1181-1341: Consider documenting that realtime message edit APIs use REST under the hoodAll new realtime
getMessage/updateMessage/deleteMessage/getMessageVersionsmethods delegate tomessageEditsMixinwithably.http, so they execute over REST even when called on a realtime channel.This is a reasonable implementation given current server capabilities, but it may surprise users expecting websocket‑based semantics and ultra‑low latency. Consider adding a brief note to the class or method Javadocs clarifying that these operations are performed via REST calls so expectations around latency and failure modes are clear.
📜 Review details
Configuration used: CodeRabbit 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 (8)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(6 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java(1 hunks)lib/src/main/java/io/ably/lib/types/BaseMessage.java(2 hunks)lib/src/main/java/io/ably/lib/types/MessageOperation.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageSerializer.java(2 hunks)lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (2)
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/types/MessageOperationSerializer.java (5)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)lib/src/main/java/io/ably/lib/util/Base64Coder.java (1)
Base64Coder(30-239)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (7)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (2)
Helpers(74-1229)CompletionSet(862-912)lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
MessageEditsMixin(23-227)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)
⏰ 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: build
- GitHub Check: check (29)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-liveobjects
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check (24)
- GitHub Check: check (29)
🔇 Additional comments (3)
lib/src/main/java/io/ably/lib/types/BaseMessage.java (1)
182-215: Encode refactor viaencodeDatalooks consistent with previous behaviorThe new
encodeData(ChannelOptions)+EncodedMessageDatasplit preserves the prior encoding rules (JSON element → JSON string, String → UTF‑8 bytes when encrypted, then cipher application) while making the transformation reusable for other serializers. No correctness issues stand out.Also applies to: 383-391
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
14-98:MessageOperationstructure and JSON/MsgPack serialization look soundField selection and dynamic field counts are correct, and both MsgPack and JSON paths only include non‑null fields. This fits the existing patterns used for similar types in the library.
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
49-50: REST message edit/delete/version wiring viaMessageEditsMixinlooks correctThe REST
ChannelBasenow composes aMessageEditsMixinand cleanly delegates all new message edit/delete/version operations to it. Constructor initialisation passes the expectedbasePath,ClientOptions,ChannelOptions, andAuth, and the public API surface onChannelBasematches the mixin’s capabilities. No issues spotted in this wiring.Also applies to: 317-481, 488-496
ttypic
force-pushed
the
ECO-5642/edit-deletes-api
branch
from
51d3719 to
aa67dc6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1179/features
github-actions
bot
temporarily deployed
to
staging/pull/1179/javadoc
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
197-223: Address charset and null-handling issues from prior review.The past review flagged two issues that still appear present:
- Line 202:
new String(body)uses platform default charset instead of UTF-8- Line 219: Returns
new Message[] { message }even whenmessageis null (unsupported content type), which differs fromMessageBodyHandlerbehavior that returns the full array or nullApply the suggested fix from the prior review:
if ("application/json".equals(contentType)) { - message = Serialisation.gson.fromJson(new String(body), Message.class); + message = Serialisation.gson.fromJson(new String(body, "UTF-8"), Message.class); } ... if (message != null) { try { message.decode(opts); } catch (MessageDecodeException e) { Log.e(TAG, e.errorInfo.message); } + return new Message[] { message }; } -return new Message[] { message }; +return null;lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (3)
72-72: Incorrect helper method for initial message retrieval.
waitForUpdatedMessageAppear()waits forMESSAGE_UPDATEaction, but at this point the message was only published and should haveMESSAGE_CREATEaction. This test may pass only due to timeout fallback returning the message regardless of action.- Message retrievedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial); + Message retrievedMessage = channel.getMessage(publishedMessage.serial);
449-449: Add parentheses for operator precedence clarity.The condition lacks parentheses around the main success check, making the logic harder to read. While the current behavior may work due to short-circuit evaluation, explicit grouping improves clarity.
- if (history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout) + if ((history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout)
323-339: Incomplete async test - does not exercise async API.This test publishes a message and retrieves history but never calls
channel.getMessageVersionsAsync(...)or performs any assertions on version history. It doesn't exercise the async API it claims to test.Either complete the test to actually call
getMessageVersionsAsyncwith assertions, or remove it to avoid misleading test coverage.lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
55-66: Returningnullon IOException will cause NPE downstream.When
writeMsgpackencounters an IOException, it returnsnull, which is then passed toByteArrayRequestBodyat line 46. This will cause aNullPointerExceptionwhengetEncoded()is called on the request body.Propagate the exception instead:
private static byte[] writeMsgpack(UpdateDeleteRequest request) { try { ByteArrayOutputStream out = new ByteArrayOutputStream(); MessagePacker packer = Serialisation.msgpackPackerConfig.newPacker(out); request.writeMsgpack(packer); packer.flush(); return out.toByteArray(); } catch (IOException e) { - Log.e(TAG, "Failed to write msgpack", e); - return null; + throw new RuntimeException("Failed to write msgpack", e); } }lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1328-1341: Consider adding convenience overloads forgetMessageVersions.Tests and common usage patterns would benefit from convenience overloads that don't require the
paramsargument:public PaginatedResult<Message> getMessageVersions(String serial) throws AblyException { return getMessageVersions(serial, null); } public void getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { getMessageVersionsAsync(serial, null, callback); }lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
113-134: Add null guard formessageparameter.If a caller passes
nullformessage, aNullPointerExceptionwill occur when accessingmessage.serialat line 115. Add an explicit check for clearer error messaging.private Http.Request<Void> updateMessageImpl(Http http, Message message, MessageOperation operation) { 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)); }Apply the same guard to
deleteMessageImplat line 162.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
454-478: Fix version ordering docs, add convenience overloads, and align async signature
The Javadoc says versions are “ordered chronologically”, but tests and expected behaviour are reverse‑chronological (newest first). Please update the wording accordingly.
Suggested tweak:
- This method returns a paginated result containing all versions of the message,
- ordered chronologically. Each version includes metadata about when and by whom
- This method returns a paginated result containing all versions of the message,
- ordered in reverse chronological order (newest first). Each version includes metadata about when and by whom
There’s no convenience overloads without
Param[]:
getMessageVersions(String serial)delegating togetMessageVersions(serial, null).getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult<Message>> callback)delegating to the 3‑arg version.These match existing
history()patterns and simplify common usage.
- /**
* Convenience overload for retrieving all historical versions of a specific message* without query parameters.*/- public PaginatedResult getMessageVersions(String serial) throws AblyException {
return getMessageVersions(serial, null);- }
- /**
* Convenience overload for asynchronously retrieving all historical versions of a specific message* without query parameters.*/- public void getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult> callback) {
getMessageVersionsAsync(serial, null, callback);- }
3. `getMessageVersionsAsync` declares `throws AblyException`, unlike other async APIs here (e.g. `historyAsync`, `getAsync`). Unless this method can synchronously throw for programmer errors, it would be more consistent to drop `throws AblyException` from the signature and surface errors exclusively via the callback. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)</summary><blockquote> `1527-1527`: **Redundant initialization of `messageEditsMixin`.** This line is redundant because `this.setOptions(options)` at line 1514 already initializes `messageEditsMixin`. The mixin will be created twice during construction. Remove this redundant initialization: ```diff this.annotations = new RealtimeAnnotations( this, new RestAnnotations(name, ably.http, ably.options, options) ); - this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth); }lib/src/main/java/io/ably/lib/rest/ChannelBase.java (3)
49-50: MessageEditsMixin lifecycle vsChannelOptionsupdates
messageEditsMixinis constructed once with theoptionsinstance passed into the constructor. If channel options can change at runtime (e.g. encryption or other per‑channel settings), the mixin may keep using stale options.Consider either:
- passing
ChannelOptionsinto each mixin call instead of capturing it, or- ensuring any
setOptions‑style logic also updates the mixin’s view of the options.Also applies to: 488-496
345-353: Clarify operation metadata wording and align Javadoc between sync/async methodsThe
operationJavadoc currently says “metadata in the version field”, butMessageOperationexposesclientId,description, and ametadatamap; there is no explicit “version field” on that type. Also,deleteMessageAsync’smessageparameter Javadoc mentions “operation metadata”, while the sync variant just mentions the serial.To reduce confusion:
- Reword the
operationparam to something like “additional operation metadata such as clientId, description, or arbitrary key–value metadata”.- Make the
messageparameter description for delete/update sync and async variants consistent (likely just “containing the serial identifier” and, for updates, the fields to patch).Also applies to: 374-379, 399-408, 428-445
52-60: Minor Javadoc polish forpublish/publishAsyncThe
@param datadescriptions end with a semicolon ("the message payload;"), which reads oddly compared with the rest of the file’s Javadocs. Consider changing to a period for consistency:- * @param data the message payload; + * @param data the message payload.Also applies to: 68-79
📜 Review details
Configuration used: CodeRabbit 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 (8)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(6 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java(1 hunks)lib/src/main/java/io/ably/lib/types/BaseMessage.java(2 hunks)lib/src/main/java/io/ably/lib/types/MessageOperation.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageSerializer.java(2 hunks)lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (9)
lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt (1)
keys(71-78)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt (1)
channels(3-9)liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
data(162-172)liveobjects/src/main/kotlin/io/ably/lib/objects/ObjectMessage.kt (1)
action(164-220)pubsub-adapter/src/test/kotlin/com/ably/Utils.kt (1)
waitFor(11-20)
lib/src/main/java/io/ably/lib/types/MessageSerializer.java (3)
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/util/Log.java (1)
Log(21-157)
lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (5)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)lib/src/main/java/io/ably/lib/util/Base64Coder.java (1)
Base64Coder(30-239)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (2)
lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
MessageSerializer(23-228)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
MessageEditsMixin(23-223)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)
lib/src/main/java/io/ably/lib/types/BaseMessage.java (2)
lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)
⏰ 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 (24)
- GitHub Check: check (29)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: build
- GitHub Check: check (29)
- GitHub Check: check
🔇 Additional comments (7)
lib/src/main/java/io/ably/lib/types/BaseMessage.java (2)
182-215: Clean refactoring to separate encoding logic.The decomposition of
encode()intoencodeData()that returns anEncodedMessageDataresult is a good design choice. This allowsMessageOperationSerializerto obtain encoded data without mutating the original message object, which is important for the new message edit operations.
383-391: LGTM!The
EncodedMessageDataclass is a clean, immutable data holder with appropriate package-private visibility for internal use.lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
14-100: Well-structured metadata class.The
MessageOperationclass follows established patterns in the codebase with consistent JSON and MsgPack serialization. Good reuse ofMessageSerializer.write()for the metadata map.lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
93-101: Good use of the newencodeData()method.The constructor correctly uses
message.encodeData(channelOptions)to obtain encoded data without mutating the original message. This is the intended use of theBaseMessagerefactoring.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1179-1314: Message edit/delete API surface looks well-designed.The new public API methods for
getMessage,updateMessage,deleteMessage, and their async variants follow consistent patterns with the existing channel methods. The delegation tomessageEditsMixinkeeps the implementation clean.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (2)
66-85: Well-structured implementation using new single-message response handler.The
getMessageImplmethod correctly uses the newgetSingleMessageResponseHandlerand handles the response appropriately, throwing a 404 error when the message is not found.
213-221: LGTM!The
getMessageVersionsImplproperly validates the serial parameter and usesBasePaginatedQueryconsistently with other paginated endpoints in the codebase.
- Introduced APIs to retrieve, update, and delete messages by serial identifier. - Added support for fetching historical versions of messages. - Implemented `MessageEditsMixin` to handle message editing operations. - Added tests for message editing and retrieval functionality.
ttypic
force-pushed
the
ECO-5642/edit-deletes-api
branch
from
aa67dc6 to
c6cf81d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (2)
28-37: Add a setter forchannelOptionsto support runtime option changes.The
channelOptionsfield isfinal, but the realtimeChannelBasecan change its options viasetOptions(). Without a setter, the mixin will use stale options after the channel's options are updated at runtime.- private final ChannelOptions channelOptions; + private ChannelOptions channelOptions;Add a setter method:
public void setChannelOptions(ChannelOptions channelOptions) { this.channelOptions = channelOptions; }Then call this setter from
ChannelBase.setOptions()to keep the mixin in sync.#!/bin/bash # Check if ChannelBase.setOptions updates the mixin's channelOptions rg -n -A10 "setOptions.*ChannelOptions" lib/src/main/java/io/ably/lib/rest/ChannelBase.java lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
112-133: Guard against nullmessageparameter.Both
updateMessageImplanddeleteMessageImplassume a non-nullMessage. If a caller passesnull, they'll get aNullPointerExceptionfrommessage.serialrather than a clearAblyException.Add a null check at the start of the method:
private Http.Request<Void> updateMessageImpl(Http http, Message message, MessageOperation operation) { 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()) {Apply the same pattern to
deleteMessageImplat line 161.lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
55-66: Returningnullon IOException will cause NPE downstream.If
writeMsgpackencounters anIOException, it logs the error and returnsnull. Thisnullis passed toByteArrayRequestBodyat line 46, which will cause aNullPointerExceptionwhen the request body is used.Propagate the exception instead:
- private static byte[] writeMsgpack(UpdateDeleteRequest request) { + private static byte[] writeMsgpack(UpdateDeleteRequest request) throws AblyException { try { ByteArrayOutputStream out = new ByteArrayOutputStream(); MessagePacker packer = Serialisation.msgpackPackerConfig.newPacker(out); request.writeMsgpack(packer); packer.flush(); return out.toByteArray(); } catch (IOException e) { - Log.e(TAG, "Failed to write msgpack", e); - return null; + throw AblyException.fromThrowable(e); } }Since
asMsgPackRequestalready declaresthrows AblyException, this change is compatible.lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (3)
70-78: Incorrect wait helper for initial message retrieval.
waitForUpdatedMessageAppear()waits for a message withaction == MESSAGE_UPDATE, but a newly published message will haveMESSAGE_CREATEaction (or null), notMESSAGE_UPDATE. This test may time out or return incorrect results.Use
getMessage()directly since there's no update to wait for:- Message retrievedMessage = waitForUpdatedMessageAppear(channel, publishedMessage.serial); + Message retrievedMessage = channel.getMessage(publishedMessage.serial);
322-339: Incomplete async test - no async call or assertions.
getMessageVersions_asynconly publishes a message and fetches history synchronously; it never invokesgetMessageVersionsAsync()or makes any assertions on version history. This gives a misleading sense of coverage.Either complete the test to actually exercise the async API:
// After getting publishedMessage... Helpers.AsyncWaiter<AsyncPaginatedResult<Message>> waiter = new Helpers.AsyncWaiter<>(); channel.getMessageVersionsAsync(publishedMessage.serial, null, waiter); AsyncPaginatedResult<Message> versions = waiter.waitFor(); assertNotNull("Expected non-null versions", versions);Or remove the test to avoid false coverage.
447-452: Operator precedence issue in condition.The condition
history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeoutwill evaluate incorrectly due to&&having higher precedence than||. The timeout check should be grouped with the entire condition.- if (history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout) + if ((history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1315-1341: Tighten versions API docs and add convenience overloads before releaseTwo points here:
Version ordering Javadoc is ambiguous (also raised previously):
- The comment says “ordered chronologically” (Line 1318), while tests and API design typically expect reverse chronological (newest first), similar to
history.- Consider clarifying to “ordered in reverse chronological order (newest first)” to avoid confusion.
Missing convenience overloads likely cause test/compile friction (also raised previously):
- Tests and callers often expect
getMessageVersions(serial)andgetMessageVersionsAsync(serial, callback)overloads, analogous tohistory(...).- Currently only the
(String serial, Param[] params)and(String serial, Param[] params, Callback<...>)methods exist.You can address both with something like:
- * This method returns a paginated result containing all versions of the message, - * ordered chronologically. Each version includes metadata about when and by whom + * This method returns a paginated result containing all versions of the message, + * ordered in reverse chronological order (newest first). Each version includes metadata about when and by whom * the message was modified. @@ public PaginatedResult<Message> getMessageVersions(String serial, Param[] params) throws AblyException { return messageEditsMixin.getMessageVersions(ably.http, serial, params); } + + /** + * Convenience overload that retrieves all historical versions of a specific message + * without specifying query parameters. + * + * @param serial The unique serial identifier of the message. + * @return A {@link PaginatedResult} containing an array of {@link Message} objects + * representing all versions of the message. + * @throws AblyException If the versions cannot be retrieved. + */ + public PaginatedResult<Message> getMessageVersions(String serial) throws AblyException { + return getMessageVersions(serial, null); + } @@ - public void getMessageVersionsAsync(String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { + public void getMessageVersionsAsync(String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { messageEditsMixin.getMessageVersionsAsync(ably.http, serial, params, callback); } + + /** + * Convenience overload for asynchronously retrieving all historical versions of a specific + * message without specifying query parameters. + * + * @param serial The unique serial identifier of the message. + * @param callback A callback to handle the result asynchronously. + * @throws AblyException If the versions cannot be retrieved. + */ + public void getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { + getMessageVersionsAsync(serial, null, callback); + }Additionally,
getMessageVersionsAsync(...)is the only async channel method here that declaresthrows AblyException. For API consistency withhistoryAsync(...)and other async methods, consider moving any synchronous validation into the mixin and surfacing failures via the callback instead of a checked exception on this public async API.lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
454-479: Align versions docs/overloads/async signature with expectationsSame concerns as on the realtime channel:
Clarify version ordering in Javadoc (previously raised):
- “ordered chronologically” (Line 457) is ambiguous; tests and history APIs typically expect newest-first behaviour.
- Recommend: “ordered in reverse chronological order (newest first)”.
Add convenience overloads for callers/tests (previously raised):
- Only
(String serial, Param[] params)and(String serial, Param[] params, Callback<...>)exist.- Tests and client code often use
getMessageVersions(serial)andgetMessageVersionsAsync(serial, callback).Suggested changes:
- * This method returns a paginated result containing all versions of the message, - * ordered chronologically. Each version includes metadata about when and by whom + * This method returns a paginated result containing all versions of the message, + * ordered in reverse chronological order (newest first). Each version includes metadata about when and by whom * the message was modified. @@ public PaginatedResult<Message> getMessageVersions(String serial, Param[] params) throws AblyException { return messageEditsMixin.getMessageVersions(ably.http, serial, params); } + + /** + * Convenience overload that retrieves all historical versions of a specific message + * without specifying query parameters. + * + * @param serial The unique serial identifier of the message. + * @return A {@link PaginatedResult} containing an array of {@link Message} objects + * representing all versions of the message. + * @throws AblyException If the versions cannot be retrieved. + */ + public PaginatedResult<Message> getMessageVersions(String serial) throws AblyException { + return getMessageVersions(serial, null); + } @@ - public void getMessageVersionsAsync(String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { + public void getMessageVersionsAsync(String serial, Param[] params, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { messageEditsMixin.getMessageVersionsAsync(ably.http, serial, params, callback); } + + /** + * Convenience overload for asynchronously retrieving all historical versions of a specific + * message without specifying query parameters. + * + * @param serial The unique serial identifier of the message. + * @param callback A callback to handle the result asynchronously. + * @throws AblyException If the versions cannot be retrieved. + */ + public void getMessageVersionsAsync(String serial, Callback<AsyncPaginatedResult<Message>> callback) throws AblyException { + getMessageVersionsAsync(serial, null, callback); + }As with the realtime class, consider whether
getMessageVersionsAsync(...)should exposethrows AblyExceptionat all, given other async methods on this type do not. It may be cleaner to keep synchronous validation in the mixin and surface failures via thecallbackrather than a checked exception on the public API.
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
184-196: Clarify version ordering in documentation.The documentation states versions are "ordered chronologically" which is ambiguous. Based on test expectations (line 313-316 in the test file checking the last item for latest data), versions appear to be in chronological order (oldest first). Consider making this explicit.
- * ordered chronologically. Each version includes metadata about when and by whom + * ordered in chronological order (oldest first). Each version includes metadata about when and by whomlib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
105-105: Avoid double-initialisingmessageEditsMixin
messageEditsMixinis initialised both insetOptions(...)(Line 1451) and again in the constructor (Line 1527). The second assignment is redundant and risks future divergence if the initialisation logic changes in one place but not the other.You can rely on
setOptions(...)for initialisation and drop the explicit constructor assignment:public void setOptions(ChannelOptions options, CompletionListener listener) throws AblyException { this.options = options; this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth); @@ this.annotations = new RealtimeAnnotations( this, new RestAnnotations(name, ably.http, ably.options, options) ); - this.messageEditsMixin = new MessageEditsMixin(basePath, ably.options, options, ably.auth); }Also applies to: 1450-1452, 1527-1528
📜 Review details
Configuration used: CodeRabbit 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 (8)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(6 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(5 hunks)lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java(1 hunks)lib/src/main/java/io/ably/lib/types/BaseMessage.java(2 hunks)lib/src/main/java/io/ably/lib/types/MessageOperation.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java(1 hunks)lib/src/main/java/io/ably/lib/types/MessageSerializer.java(2 hunks)lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/main/java/io/ably/lib/types/MessageSerializer.java
🧰 Additional context used
🧬 Code graph analysis (3)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
MessageEditsMixin(22-222)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)
lib/src/main/java/io/ably/lib/types/BaseMessage.java (4)
lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)
lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (12)
lib/src/main/java/io/ably/lib/http/BasePaginatedQuery.java (1)
BasePaginatedQuery(25-345)lib/src/main/java/io/ably/lib/http/Http.java (1)
Http(10-99)lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (1)
MessageOperationSerializer(18-172)lib/src/main/java/io/ably/lib/types/MessageSerializer.java (1)
MessageSerializer(23-228)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)
⏰ 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-realtime-httpurlconnection
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check-liveobjects
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: build
🔇 Additional comments (17)
lib/src/main/java/io/ably/lib/types/MessageOperation.java (3)
14-48: LGTM!The class is well-structured with clear documentation, appropriate visibility modifiers, and consistent serialization patterns matching other types in the codebase.
56-76: LGTM!The MessagePack serialization correctly handles optional fields by counting non-null fields before packing the map header, and properly delegates metadata serialization to
MessageSerializer.write().
84-98: LGTM!The JSON serialization mirrors the MessagePack approach, properly handling optional fields and using
Serialisation.gson.toJsonTree()for the metadata map.lib/src/main/java/io/ably/lib/types/BaseMessage.java (2)
182-215: Clean refactor enabling immutable encoding.The extraction of
encodeData()allows serializers to obtain encoded data without mutating the original message, addressing the mutation concerns raised in past reviews. The logic correctly handles JSON elements, UTF-8 conversion for encryption, and cipher application.
383-391: LGTM!The
EncodedMessageDataclass is a simple, immutable value holder appropriate for returning the encoded result. The package-private visibility is correct since it's only used internally byMessageOperationSerializer.lib/src/main/java/io/ably/lib/rest/MessageEditsMixin.java (1)
65-84: LGTM! Properly uses the shared single-message response handler.The implementation correctly uses
MessageSerializer.getSingleMessageResponseHandler()as suggested in past reviews, ensuring consistent content-type handling and charset decoding.lib/src/main/java/io/ably/lib/types/MessageOperationSerializer.java (3)
93-101: Good use ofencodeData()to avoid double encoding.Using
message.encodeData(channelOptions)instead ofmessage.encode()correctly obtains the encoded data without mutating the input message, addressing the double encoding concerns raised in past reviews.
109-145: LGTM!The MessagePack serialization correctly handles all field types including binary data and properly delegates to nested object serializers.
150-168: LGTM!The JSON serialization correctly applies base64 encoding for byte[] data and updates the encoding string accordingly, matching the pattern in
BaseMessage.toJsonObject().lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (5)
464-472: LGTM - operator precedence is correct.The condition now has proper parentheses:
(message != null && message.action == MessageAction.MESSAGE_UPDATE) || ...
474-482: LGTM - operator precedence is correct.The condition now has proper parentheses:
(message != null && message.action == MessageAction.MESSAGE_DELETE) || ...
36-48: LGTM!Good test setup with appropriate timeout rule and proper initialization of the Ably REST client.
268-317: Comprehensive version history test with MessageOperation metadata verification.The test properly verifies that
MessageOperationmetadata (description, metadata map) is preserved and retrievable in version history. Good coverage of the operation metadata feature.
398-443: Excellent end-to-end workflow test.This comprehensive test covers the complete message lifecycle (publish → update → verify → delete → verify versions), providing good integration coverage for the new message editing feature.
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
19-20: Realtime message edit/delete API wiring looks solidThe new imports (
MessageEditsMixin,MessageOperation) and the Message Edits & Deletes region are wired cleanly:
- All methods delegate to
messageEditsMixinusingably.http, mirroring the existing REST-stylehistory(...)behaviour.- Sync/async overloads and
MessageOperationusage are symmetric across update/delete.- Overloads that omit
MessageOperationcorrectly delegate withnull.No functional issues spotted here; just rely on the mixin tests to keep behaviour aligned with the REST side.
Also applies to: 36-37, 1179-1331
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (2)
14-14:MessageEditsMixinwiring on REST channels looks correctThe REST
ChannelBasewiring for edits support looks coherent:
MessageOperationimport (Line 14) is used only in the new edit/delete APIs.messageEditsMixinisfinaland initialised once in the constructor (Line 495) withbasePath,ably.options,options, andably.auth, matching the realtime wiring.- Using a dedicated mixin keeps REST and Realtime implementations aligned while centralising HTTP/serialization logic.
No issues here.
Also applies to: 49-50, 488-496
317-452: REST message edit/delete API surface is consistent and well factoredThe new Message Edits and Deletes region mirrors the realtime API:
- Symmetric sync/async methods for get/update/delete.
- Overloads that omit
MessageOperationdelegate withnull.- All operations delegate cleanly to
messageEditsMixinusingably.http, consistent with existinghistory(...)patterns.This symmetry between REST and Realtime surfaces is good for usability.
MessageEditsMixinto handle message editing operations.Summary by CodeRabbit
New Features
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.