[AIT-84] feat: add support for returning serials on publish
#1184
Conversation
WalkthroughThe pull request introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/1184/features
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
8 times, most recently
from
c4ffcd4 to
99a2c94
Compare
- Introduced `publishWithResult` API to include message serials in the response. - Added callback-based `publishAsync` method for asynchronous publishing with serials. - Deprecated older `CompletionListener`-based publish methods in favor of `Callback<PublishResult>`.
ttypic
force-pushed
the
AIT-84/message-publish-result
branch
from
66c4061 to
e3a601c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java`:
- Around line 131-139: The test creates a Helpers.AsyncWaiter<PublishResult>
named publishResultAsyncWaiter but never passes it to channel.publish(), so
publishResultAsyncWaiter.waitFor() blocks; fix by calling channel.publish(...)
with publishResultAsyncWaiter as the callback parameter (i.e., pass
publishResultAsyncWaiter or its callback method into the channel.publish
invocation used to publish the "test_event" so PublishResult is delivered and
waitFor() can complete), then use publishResultAsyncWaiter.result to assert
serials.
🧹 Nitpick comments (2)
lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java (1)
62-63: Consider adding null safety forserialsarray.
PublishResult.serialsis annotated as@Nullable. AccessingpublishResult.serials[0]directly risks an NPE if the server returns a null serials array. Consider assertingserials != nullfirst:assertNotNull("Expected serials array", publishResult.serials); assertNotNull("Expected message to have a serial", publishResult.serials[0]);This pattern should be applied consistently throughout the test file.
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
447-455: Consider renaming for clarity.The method is named
callCompletionListenerErrorbut accepts aCallback<PublishResult>parameter. Consider renaming tocallCallbackErroror similar to avoid confusion with the existingCompletionListener-based overload at line 437.♻️ Suggested rename
- private static void callCompletionListenerError(Callback<PublishResult> listener, ErrorInfo err) { + private static void callCallbackError(Callback<PublishResult> listener, ErrorInfo err) {
| Helpers.AsyncWaiter<PublishResult> publishResultAsyncWaiter = new Helpers.AsyncWaiter<>(); | ||
|
|
||
| // Publish a message | ||
| channel.publish("test_event", "Original message data"); | ||
|
|
||
| // Get the message from history to obtain its serial | ||
| PaginatedResult<Message> history = waitForMessageAppearInHistory(channel); | ||
| assertNotNull("Expected non-null history", history); | ||
| assertEquals(1, history.items().length); | ||
|
|
||
| Message publishedMessage = history.items()[0]; | ||
| assertNotNull("Expected message to have a serial", publishedMessage.serial); | ||
| publishResultAsyncWaiter.waitFor(); | ||
| PublishResult publishResult = publishResultAsyncWaiter.result; | ||
| assertEquals("Expected to have one serial", 1, publishResult.serials.length); |
There was a problem hiding this comment.
Critical bug: Missing callback causes test timeout.
The publishResultAsyncWaiter is created but never passed to channel.publish(). This causes waitFor() on line 137 to hang indefinitely, explaining the 300-second timeout in the pipeline.
🐛 Proposed fix
- // Publish a message
- channel.publish("test_event", "Original message data");
+ // Publish a message
+ channel.publish("test_event", "Original message data", publishResultAsyncWaiter);🧰 Tools
🪛 GitHub Actions: Integration Test
[error] 137-137: Test timed out after 300 seconds while updating encoded data in updateMessage_updateEncodedData.
🤖 Prompt for AI Agents
In
`@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java`
around lines 131 - 139, The test creates a Helpers.AsyncWaiter<PublishResult>
named publishResultAsyncWaiter but never passes it to channel.publish(), so
publishResultAsyncWaiter.waitFor() blocks; fix by calling channel.publish(...)
with publishResultAsyncWaiter as the callback parameter (i.e., pass
publishResultAsyncWaiter or its callback method into the channel.publish
invocation used to publish the "test_event" so PublishResult is delivered and
waitFor() can complete), then use publishResultAsyncWaiter.result to assert
serials.
| * @param data the message payload | ||
| * @throws AblyException | ||
| */ | ||
| public void publish(String name, Object data) throws AblyException { |
There was a problem hiding this comment.
Similar NonBlocking annotations for all publish methods
| * @return A {@link PublishResult} containing the message serial(s) | ||
| * @throws AblyException | ||
| */ | ||
| public PublishResult publishWithResult(String name, Object data) throws AblyException { |
There was a problem hiding this comment.
Is this a blocking method?
Also do we have different naming for REST publish i.e. publishWithResult instead of plain publish
There was a problem hiding this comment.
Pull request overview
This pull request adds support for returning message serials from publish operations. It introduces new publishWithResult and publishAsync APIs that return PublishResult objects containing message serials, and deprecates older CompletionListener-based methods in favor of Callback<PublishResult>.
Changes:
- Introduced
publishWithResultsynchronous API that returnsPublishResultwith message serials - Added new
publishAsyncmethods acceptingCallback<PublishResult>for asynchronous publishing with typed results - Deprecated
CompletionListener-based publish methods in favor of callback-based variants with better type safety
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/main/java/io/ably/lib/rest/ChannelBase.java | Added publishWithResult and publishAsync(Callback<PublishResult>) methods; deprecated older CompletionListener variants |
| lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | Added new publish methods with Callback<PublishResult> parameter; deprecated CompletionListener variants |
| lib/src/main/java/io/ably/lib/realtime/CompletionListener.java | Made ToCallback generic to support different result types |
| lib/src/main/java/io/ably/lib/push/PushBase.java | Updated to use generic ToCallback<> with type parameter |
| android/src/main/java/io/ably/lib/push/PushChannel.java | Updated to use generic ToCallback<> with type parameter |
| lib/src/test/java/io/ably/lib/test/rest/RestChannelMessageEditTest.java | Updated tests to use publishWithResult and publishAsync with callback |
| lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java | Simplified publish calls by removing explicit null listeners |
| lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java | Updated tests to use async publish with PublishResult callback |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Helpers.AsyncWaiter<PublishResult> publishResultAsyncWaiter = new Helpers.AsyncWaiter<>(); | ||
|
|
||
| // Publish a message | ||
| channel.publish("test_event", "Original message data"); | ||
|
|
||
| // Get the message from history to obtain its serial | ||
| PaginatedResult<Message> history = waitForMessageAppearInHistory(channel); | ||
| assertNotNull("Expected non-null history", history); | ||
| assertEquals(1, history.items().length); | ||
|
|
||
| Message publishedMessage = history.items()[0]; | ||
| assertNotNull("Expected message to have a serial", publishedMessage.serial); | ||
| publishResultAsyncWaiter.waitFor(); | ||
| PublishResult publishResult = publishResultAsyncWaiter.result; | ||
| assertEquals("Expected to have one serial", 1, publishResult.serials.length); |
There was a problem hiding this comment.
The publishResultAsyncWaiter is created on line 131 but the publish call on line 134 does not pass it as an argument. This means waitFor() on line 137 will wait indefinitely and publishResult will be null. The publish call should pass publishResultAsyncWaiter as the third argument to enable the callback to populate the result.
| * Asynchronously publish an array of messages on this channel | ||
| * | ||
| * @param messages the message | ||
| * @param listener a listener to be notified of the outcome of this message. |
There was a problem hiding this comment.
The documentation parameter name is inconsistent. The parameter is named "listener" but should be "callback" to match the actual parameter name and type (Callback) in the method signature.
| * @param listener a listener to be notified of the outcome of this message. | |
| * @param callback a callback to be notified of the outcome of this message. |
| assertEquals(1, history.items().length); | ||
| // Verify version history | ||
| assertNotNull("Expected non-null versions", versions); | ||
| assertTrue("Expected at least 2 versions (original + 2 updates)", versions.items().length >= 2); |
There was a problem hiding this comment.
The test comment states "Expected at least 2 versions (original + 2 updates)" but only 1 update is performed in the test (updateMessage1). The comment should read "Expected at least 2 versions (original + 1 update)" to accurately reflect the test logic.
| assertTrue("Expected at least 2 versions (original + 2 updates)", versions.items().length >= 2); | |
| assertTrue("Expected at least 2 versions (original + 1 update)", versions.items().length >= 2); |
| * @param listener A listener may optionally be passed in to this call to be notified of success or failure of the operation. | ||
| * <p> | ||
| * This listener is invoked on a background thread. | ||
| * @throws AblyException |
There was a problem hiding this comment.
The javadoc for this deprecated method is missing the @deprecated tag. For consistency with other deprecated methods in this file (lines 1079 and 1117), this method's javadoc should include "@deprecated Use {@link #publish(Message[], Callback)} instead."
| * @throws AblyException | |
| * @throws AblyException | |
| * @deprecated Use {@link #publish(Message[], Callback)} instead. |
| try { | ||
| listener.onError(err); | ||
| } catch(Throwable t) { | ||
| Log.e(TAG, "Unexpected exception calling CompletionListener", t); |
There was a problem hiding this comment.
The log message refers to "CompletionListener" but this method handles Callback, not CompletionListener. The error message should be updated to "Unexpected exception calling Callback" for accuracy.
| Log.e(TAG, "Unexpected exception calling CompletionListener", t); | |
| Log.e(TAG, "Unexpected exception calling Callback", t); |
publishWithResultAPI to include message serials in the response.publishAsyncmethod for asynchronous publishing with serials.CompletionListener-based publish methods in favor ofCallback<PublishResult>.Summary by CodeRabbit
Release Notes
New Features
publishmethods returningPublishResultwith message metadata and serials.Deprecations
CompletionListener-based publish methods; use result-based variants instead.Improvements
✏️ Tip: You can customize this high-level summary in your review settings.