feat: Implement Message Updates, Deletes, and Appends#697
feat: Implement Message Updates, Deletes, and Appends#697
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds message versioning and actions (create/update/delete/append), publish-with-result APIs for REST and Realtime, serial-aware ACK delivery via an ackCallback wrapper, default protocol bumped to v5, per-message retrieval and version history, supporting decoders, and tests/examples for message lifecycle flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant REST_Channel as "REST Channel"
participant Server
Client->>REST_Channel: PublishWithResult(name, data)
REST_Channel->>Server: POST /messages (Action=CREATE)
Server-->>REST_Channel: {serials: ["s1"]}
REST_Channel-->>Client: PublishResult{Serial:"s1"}
Client->>REST_Channel: UpdateMessage(msg{Serial:"s1"}, opts)
REST_Channel->>REST_Channel: validate serial, build Version
REST_Channel->>Server: PATCH /messages/s1 (Action=UPDATE, Version)
Server-->>REST_Channel: {versionSerial: "v2"}
REST_Channel-->>Client: UpdateDeleteResult{VersionSerial:"v2"}
sequenceDiagram
participant Client
participant Realtime_Channel as "Realtime Channel"
participant Connection
participant Server
Client->>Realtime_Channel: PublishWithResultAsync(name, data, cb)
Realtime_Channel->>Realtime_Channel: create ackCallback wrapper
Realtime_Channel->>Connection: send(msg, callback)
Connection->>Server: send PUBLISH
Server-->>Connection: ACK {serials: ["s1"]}
Connection->>Realtime_Channel: callback.call([{serials:["s1"]}], nil)
Realtime_Channel-->>Client: cb(PublishResult{Serial:"s1"}, nil)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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/697/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ably/paginated_result.go (1)
189-194:⚠️ Potential issue | 🟠 MajorBug:
copyHeaderassigns the original slice instead of the copy.Line 192 creates a copy
dof the header values slice, but line 193 assigns the originalvtodest[k], making the copy dead code. The destination header will share the same backing array as the source.🐛 Proposed fix
func copyHeader(dest, src http.Header) { for k, v := range src { d := make([]string, len(v)) copy(d, v) - dest[k] = v + dest[k] = d } }ably/rest_channel.go (1)
112-119:⚠️ Potential issue | 🟡 Minor
publishMultiplemutates caller's message slice elements in place.The
*m, err = (*m).withEncodedData(cipher)dereferences and overwrites the pointed-toMessagevalue. Sincemessagesis[]*Message, this modifies the caller's original messages (e.g., overwritingDatawith encoded bytes and settingEncoding). This could surprise callers who reuse message objects. The same pattern exists in theUpdateMessage/DeleteMessage/AppendMessagemethods, but those copy first (updateMsg := *msg), which is safer.
🤖 Fix all issues with AI agents
In `@ably/message_updates_integration_test.go`:
- Around line 315-329: The async callback passed to channel.AppendMessageAsync
currently calls t.Logf from a goroutine (in the loop over tokens) which can
panic if the test ends; change the callback to never call testing.T methods and
instead send results over the existing completed channel (e.g., send an index or
an error value/struct) so the main test goroutine can observe failures; update
the completion loop that reads from completed to detect and fail the test via
require/ t.Fatalf when an error result is received, and remove the t.Logf call
inside the callback (refer to the loop variable tokens, the
channel.AppendMessageAsync callback, and the completed channel when making the
change).
In `@ably/state.go`:
- Line 138: The Ack method on pendingEmitter has an unused conn *Connection
parameter; remove the parameter from the function signature (func (q
*pendingEmitter) Ack(msg *protocolMessage, errInfo *ErrorInfo)) and update all
callers (e.g., the calls from realtime_conn.go that currently pass a Connection)
to stop passing conn. Ensure you update any interface definitions or
implementations that referenced pendingEmitter.Ack to match the new signature
and run build/tests to catch remaining references.
🧹 Nitpick comments (6)
ably/rest_client.go (1)
812-819: Header ordering: custom headers copied beforeAcceptis set.Custom headers from
r.headerare copied at line 813 beforeAcceptis unconditionally set at line 815. If a caller ever passes a customAcceptheader, it would be silently overwritten. Currently this isn't an issue (only protocol version is overridden), but the conditional guard applied toablyProtocolVersionHeader(line 817) is not applied toAccept.Consider applying the same "set only if absent" pattern to
Acceptfor consistency, or document that certain headers cannot be overridden.ably/proto_message.go (1)
34-106: DRY: action↔numeric mapping is duplicated four times.The same switch logic for converting between
MessageActionand its numeric wire representation is repeated inMarshalJSON,UnmarshalJSON,CodecEncodeSelf, andCodecDecodeSelf. Consider extracting helpers likemessageActionToNumandnumToMessageActionto centralize the mapping.Also, the
defaultbranch silently maps unknown actions toMessageActionCreate(0) during both marshaling and unmarshaling. This could mask bugs if an invalid action is accidentally used. Consider returning an error for unknown values instead.♻️ Proposed helper extraction
+var messageActionToNum = map[MessageAction]int{ + MessageActionCreate: 0, + MessageActionUpdate: 1, + MessageActionDelete: 2, + MessageActionAppend: 5, +} + +var numToMessageAction = map[int]MessageAction{ + 0: MessageActionCreate, + 1: MessageActionUpdate, + 2: MessageActionDelete, + 5: MessageActionAppend, +} + func (a MessageAction) MarshalJSON() ([]byte, error) { - var num int - switch a { - case MessageActionCreate: - num = 0 - case MessageActionUpdate: - num = 1 - case MessageActionDelete: - num = 2 - case MessageActionAppend: - num = 5 - default: - num = 0 + num, ok := messageActionToNum[a] + if !ok { + return nil, fmt.Errorf("unknown MessageAction: %q", a) } return json.Marshal(num) }Apply the same pattern to
UnmarshalJSON,CodecEncodeSelf, andCodecDecodeSelf.ably/realtime_channel.go (1)
852-1046: Significant DRY opportunity:UpdateMessageAsync,DeleteMessageAsync, andAppendMessageAsyncare nearly identical.The three async methods differ only in the
MessageActionconstant assigned. The blocking variants follow the same boilerplate pattern too. Consider extracting a shared helper to reduce ~180 lines of near-duplicate code.♻️ Sketch of a shared helper
+func (c *RealtimeChannel) messageOperationAsync(msg *Message, action MessageAction, onAck func(*UpdateResult, error), options ...UpdateOption) error { + if err := validateMessageSerial(msg); err != nil { + return err + } + var opts updateOptions + for _, o := range options { + o(&opts) + } + version := &MessageVersion{ + Description: opts.description, + ClientID: opts.clientID, + Metadata: opts.metadata, + } + opMsg := *msg + opMsg.Action = action + opMsg.Version = version + protoMsg := &protocolMessage{ + Action: actionMessage, + Channel: c.Name, + Messages: []*Message{&opMsg}, + } + return c.sendWithSerialCallback(protoMsg, func(serials []string, err error) { + if err != nil { + onAck(nil, err) + return + } + result := &UpdateResult{} + if len(serials) > 0 { + result.VersionSerial = serials[0] + } + onAck(result, nil) + }) +} + +func (c *RealtimeChannel) messageOperation(ctx context.Context, msg *Message, action MessageAction, options ...UpdateOption) (*UpdateResult, error) { + type resultOrError struct { + result *UpdateResult + err error + } + listen := make(chan resultOrError, 1) + if err := c.messageOperationAsync(msg, action, func(result *UpdateResult, err error) { + listen <- resultOrError{result, err} + }, options...); err != nil { + return nil, err + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case result := <-listen: + return result.result, result.err + } +}Then each public method becomes a one-liner:
func (c *RealtimeChannel) UpdateMessage(ctx context.Context, msg *Message, options ...UpdateOption) (*UpdateResult, error) { return c.messageOperation(ctx, msg, MessageActionUpdate, options...) }ably/message_updates_integration_test.go (1)
205-209: Assertion on version ordering may be fragile.The test asserts that
versions[0]isMessageActionCreateandversions[1]/versions[2]areMessageActionUpdate. This depends on the server returning versions in a specific order. If the API's default ordering changes, this test will break. Consider documenting or parameterizing the expected direction.ably/rest_channel.go (2)
208-209: Remove debug log from production code.This
Debugfline logs response details including serials. While it's at debug level, it appears to be a development artifact (// Debug: log response). Consider removing it or ensuring it's intentional.Proposed fix
- // Debug: log response - c.log().Debugf("PublishMultipleWithResult response: serials=%v, count=%d", response.Serials, len(response.Serials)) - // Build results from serials
221-363: Significant code duplication acrossUpdateMessage,DeleteMessage, andAppendMessage.These three methods are nearly identical, differing only in the
Actionfield (MessageActionUpdate,MessageActionDelete,MessageActionAppend). Consider extracting a shared helper.Proposed refactor
+func (c *RESTChannel) performMessageAction(ctx context.Context, action MessageAction, msg *Message, options ...UpdateOption) (*UpdateResult, error) { + if err := validateMessageSerial(msg); err != nil { + return nil, err + } + + var opts updateOptions + for _, o := range options { + o(&opts) + } + + version := &MessageVersion{ + Description: opts.description, + ClientID: opts.clientID, + Metadata: opts.metadata, + } + + actionMsg := *msg + actionMsg.Action = action + actionMsg.Version = version + + cipher, _ := c.options.GetCipher() + var err error + actionMsg, err = actionMsg.withEncodedData(cipher) + if err != nil { + return nil, fmt.Errorf("encoding data for message: %w", err) + } + + var response publishResponse + res, err := c.client.post(ctx, c.baseURL+"/messages", []*Message{&actionMsg}, &response) + if err != nil { + return nil, err + } + defer res.Body.Close() + + result := &UpdateResult{} + if len(response.Serials) > 0 { + result.VersionSerial = response.Serials[0] + } + return result, nil +} + func (c *RESTChannel) UpdateMessage(ctx context.Context, msg *Message, options ...UpdateOption) (*UpdateResult, error) { - // ... ~45 lines of duplicated code + return c.performMessageAction(ctx, MessageActionUpdate, msg, options...) } func (c *RESTChannel) DeleteMessage(ctx context.Context, msg *Message, options ...UpdateOption) (*UpdateResult, error) { - // ... ~45 lines of duplicated code + return c.performMessageAction(ctx, MessageActionDelete, msg, options...) } func (c *RESTChannel) AppendMessage(ctx context.Context, msg *Message, options ...UpdateOption) (*UpdateResult, error) { - // ... ~45 lines of duplicated code + return c.performMessageAction(ctx, MessageActionAppend, msg, options...) }
github-actions
bot
temporarily deployed
to
staging/pull/697/features
|
It would have been nice as an experiment to derive the tests from https://github.com/ably/uts/blob/uts-experiments/test/completion-status.md |
github-actions
bot
temporarily deployed
to
staging/pull/697/features
|
@paddybyers yes apologies I did start with that but got a bit bogged down so I thought I'd just get this implemented (because I want it for load testing), and then follow up with you on the UTS stuff (we can try it out for annotations/summaries, which I haven't added here). |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ably/state_test.go (3)
20-161: Tests cover the happy path well. Consider adding edge-case coverage.The three sub-tests correctly verify per-message serial routing for the straightforward ACK case. The test logic and assertions are sound.
A few additional scenarios would strengthen confidence in the
Ackimplementation:
- Implicit NACK (serialShift > 0): Enqueue messages starting at serial N, send an ACK starting at serial N+1 — verify the first message gets
errImplictNACKand a nil result, while the rest get their correct serials.- NACK with errInfo: Call
Ackwith a non-nilerrInfoand verify the error propagates to all callbacks.- Nil callback safety: Enqueue a message with a nil
*ackCallbackand verifyAckdoesn't panic.onAck(non-serial) path: Enqueue with anackCallbackthat only setsonAck, verify it receives the error correctly.
21-23: Optional: Extract repeated logger setup into a test helper.
testLogger := logger{l: &stdLogger{log.New(io.Discard, "", 0)}}is duplicated in all three sub-tests. A small helper or asetupat the top ofTestPendingEmitter_AckResultwould reduce repetition.♻️ Suggested refactor
func TestPendingEmitter_AckResult(t *testing.T) { + testLogger := logger{l: &stdLogger{log.New(io.Discard, "", 0)}} + t.Run("two messages with single serial each", func(t *testing.T) { - testLogger := logger{l: &stdLogger{log.New(io.Discard, "", 0)}} emitter := newPendingEmitter(testLogger)
34-48: Optional: Assert thaterris nil in callbacks for ACK (non-NACK) scenarios.The callbacks capture
serialsbut silently discarderr. Addingassert.NoError(t, err)inside each callback would catch unexpected error propagation.♻️ Example for callback1
callback1 := &ackCallback{ - onAckWithSerials: func(serials []string, err error) { + onAckWithSerials: func(serials []string, err error) { + assert.NoError(t, err) msg1Serials = serials }, }
There was a problem hiding this comment.
Pull request overview
This PR implements message updates, deletes, and appends functionality for the ably-go SDK, enabling full message lifecycle management including publishing with result tracking, updating, deleting, and appending messages. The implementation follows the design specified in issue #696 and updates the protocol version from 2 to 5 to support message serials and version information.
Changes:
- Adds message operations API (PublishWithResult, UpdateMessage, DeleteMessage, AppendMessage) for both REST and Realtime channels with sync and async variants
- Introduces new message-related types: MessageAction enum, MessageVersion, PublishResult, UpdateResult, and UpdateOption functional options
- Updates protocol version to v5 by default, with special handling to keep Stats on v2 for backward compatibility
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ablytest/sandbox.go | Adds MutableMessages flag to namespace configuration for testing message operations |
| ably/state_test.go | Adds comprehensive unit tests for ACK result distribution across multiple messages |
| ably/state.go | Refactors callback handling to support serial extraction via new ackCallback struct |
| ably/rest_client.go | Implements protocol version override for Stats requests to maintain v2 compatibility |
| ably/rest_channel.go | Adds REST message operation methods (PublishWithResult, UpdateMessage, DeleteMessage, AppendMessage, GetMessage, GetMessageVersions) |
| ably/realtime_presence.go | Updates presence code to use new ackCallback pattern |
| ably/realtime_experimental_objects_test.go | Updates experimental objects tests for new callback signature |
| ably/realtime_experimental_objects.go | Updates experimental objects to use new ackCallback pattern |
| ably/realtime_conn.go | Updates connection send() to use ackCallback with nil safety checks |
| ably/realtime_channel.go | Adds Realtime message operation methods with both blocking and async variants |
| ably/proto_protocol_message.go | Adds Res field to protocolMessage for ACK serial responses |
| ably/proto_message_operations_test.go | Adds unit tests for MessageAction encoding/decoding and validation |
| ably/proto_message.go | Adds MessageAction, MessageVersion, PublishResult, UpdateResult types and Message struct fields |
| ably/proto_http.go | Updates default protocol version to v5 with documentation |
| ably/paginated_result.go | Adds header field to paginatedRequest for custom HTTP headers |
| ably/message_updates_integration_test.go | Adds comprehensive integration tests for message operations |
| ably/export_test.go | Updates test helper to use new callback pattern |
| ably/example_message_updates_test.go | Adds usage examples for new message operation APIs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I reviewed current ably-go PR with respect to
Key Spec Points: RSL15 (REST update/delete/append), RTL32 (Realtime update/delete/append), TM5 (MessageAction enum), TM2s (version initialization), PBR2a (PublishResult), UDR2a (UpdateDeleteResult), MOP2 (MessageOperation), TR4s (ACK res field) 1. Critical Issues (Missing or Incorrect Spec Implementation)1.1 ❌ REST Update/Delete/Append Uses POST Instead of PATCH (RSL15b)Spec Requirement (
ably-go Implementation ( // UpdateMessage uses POST instead of PATCH, wrong URL, and sends array instead of single Message
res, err := c.client.post(ctx, c.baseURL+"/messages", []*Message{&updateMsg}, &response)ably-java Implementation ( scheduler.patch(basePath + "/messages/" + HttpUtils.encodeURIComponent(message.serial), ...)Issues:
Fix Required:
1.2 ❌ REST Response Parsing Uses Wrong Type (RSL15e)Spec Requirement (
ably-go Implementation ( var response publishResponse // Uses publishResponse (serials array) instead of UpdateDeleteResultably-java Implementation ( HttpCore.BodyHandler<UpdateDeleteResult> bodyHandler = UpdateDeleteResult.getBodyHandler();Issue: The ably-go REST update/delete/append methods parse the response as a Fix Required: Parse the response as an 1.3
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
ably/rest_channel.go (2)
208-210: Remove debug artifact logging raw serial arrays.This debug line looks temporary and can add noisy logs with internal identifiers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ably/rest_channel.go` around lines 208 - 210, Remove the temporary debug log that prints raw serial arrays in the PublishMultipleWithResult flow: delete the c.log().Debugf("PublishMultipleWithResult response: serials=%v, count=%d", response.Serials, len(response.Serials)) line in rest_channel.go (inside the PublishMultipleWithResult handling) or replace it with a non-sensitive summary (e.g., logging only the count or a masked identifier) to avoid leaking internal identifiers.
68-70: Preserve nullable serial semantics in publish responses.
[]stringcannot representnullserial entries, so you lose the distinction between “discarded/superseded” and empty string. Consider using nullable elements in the wire struct (and document the public ergonomic mapping if you keepstringoutwardly).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ably/rest_channel.go` around lines 68 - 70, The publishResponse struct currently declares Serials as []string which cannot represent null elements; change Serials to a nullable-element slice (e.g., []*string) in the wire struct publishResponse to preserve null vs empty-string semantics when marshaling/unmarshaling, and update any serializers/deserializers and tests that touch publishResponse to handle pointers; if the public API must remain []string, add a clear conversion layer (documented) that maps nil pointers to a sentinel or explicitly documents the loss of null information.ably/realtime_channel.go (1)
788-794: Extract repeated clientId compatibility validation into a helper.This same check already exists in
PublishMultipleAsync; consolidating it avoids drift between result/non-result publish paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ably/realtime_channel.go` around lines 788 - 794, Extract the repeated clientId compatibility check into a shared helper (e.g., validateClientIDCompatibility) that accepts the slice of messages and the Auth/client used to obtain clientIDForCheck; move the loop and the existing error construction into that helper (use client.Auth.clientIDForCheck() and preserve the exact error text), then replace the inlined loop in the current function and in PublishMultipleAsync with a call to validateClientIDCompatibility and return its error if non-nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ably/proto_message_operations_test.go`:
- Around line 43-44: The test currently asserts that an unknown action code
("999") maps to MessageActionCreate which codifies a fallback and hides
unsupported actions; update the assertions that reference "999" and
MessageActionCreate (and the duplicate occurrences later) so they no longer
expect Create — instead assert that parsing/validation returns an explicit error
or an Unknown/invalid action result (e.g., an error value or a dedicated
MessageActionUnknown) so unknown/new action codes are surfaced rather than
silently mapped to MessageActionCreate; update any helper parsing/assertion
logic used by the test to check for the error/unknown outcome.
In `@ably/realtime_channel.go`:
- Around line 859-881: The protocolMessage being built for realtime
update/delete/append never sets Params so operation-level params are dropped;
add a Params field to updateOptions (e.g., updateOptions.params) if it doesn't
exist, populate it via the provided option functions, and then set
protoMsg.Params = opts.params when constructing protocolMessage (the code that
creates version := &MessageVersion{...}, opMsg := *msg, and protoMsg :=
&protocolMessage{...}). Ensure option setters wire into updateOptions.params so
operation params flow into ProtocolMessage.params.
In `@ably/rest_channel.go`:
- Around line 254-267: The code is using the publish contract (posting an array
to "/messages" and reading publishResponse.Serials) instead of the
operation-specific request/response for update/delete/append; change the call in
the method that builds opMsg so it posts to the operation-specific path and body
(use the operation request contract expected by the server rather than
c.client.post(..., c.baseURL+"/messages", []*Message{&opMsg}, ...)), swap the
response type from publishResponse to the operation result schema (e.g.,
UpdateResponse/OperationResult type) and decode that response, then set
result.VersionSerial from the operation response's version field (e.g.,
response.VersionSerial or response.Version) instead of reading
publishResponse.Serials; keep using c.client.post and res.Body.Close but ensure
URL, request body shape, and response decoding match the operation contract.
---
Nitpick comments:
In `@ably/realtime_channel.go`:
- Around line 788-794: Extract the repeated clientId compatibility check into a
shared helper (e.g., validateClientIDCompatibility) that accepts the slice of
messages and the Auth/client used to obtain clientIDForCheck; move the loop and
the existing error construction into that helper (use
client.Auth.clientIDForCheck() and preserve the exact error text), then replace
the inlined loop in the current function and in PublishMultipleAsync with a call
to validateClientIDCompatibility and return its error if non-nil.
In `@ably/rest_channel.go`:
- Around line 208-210: Remove the temporary debug log that prints raw serial
arrays in the PublishMultipleWithResult flow: delete the
c.log().Debugf("PublishMultipleWithResult response: serials=%v, count=%d",
response.Serials, len(response.Serials)) line in rest_channel.go (inside the
PublishMultipleWithResult handling) or replace it with a non-sensitive summary
(e.g., logging only the count or a masked identifier) to avoid leaking internal
identifiers.
- Around line 68-70: The publishResponse struct currently declares Serials as
[]string which cannot represent null elements; change Serials to a
nullable-element slice (e.g., []*string) in the wire struct publishResponse to
preserve null vs empty-string semantics when marshaling/unmarshaling, and update
any serializers/deserializers and tests that touch publishResponse to handle
pointers; if the public API must remain []string, add a clear conversion layer
(documented) that maps nil pointers to a sentinel or explicitly documents the
loss of null information.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
ably/proto_message_operations_test.goably/realtime_channel.goably/realtime_conn.goably/rest_channel.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/realtime_conn.go
|
@sacOO7 thanks for taking the time to review this with the agents, I see you've pushed some changes but let's please discuss this first because quite a few of the issues the agents have identified here aren't accurate. For example the agents are encouraging the use of I think this highlights two issues with how the agents have reviewed this PR:
Let's discuss this as a team, because I think if we're using agents to check that SDK implementations are correct, I think we should be comparing them with the reference implementation (i.e. ably-js), rather than a spec + ably-java. |
|
@sacOO7 I realise I was incorrect, the points raised are valid. I've updated the design doc in #696, and pushed b3275a9 to address the issues raised:
PTAL. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ably/message_updates_integration_test.go`:
- Around line 175-221: The test assumes GetMessageVersions returns versions
oldest-first but the API defaults to newest-first; update the call to
channel.GetMessageVersions(*publishResult.Serial,
ably.HistoryWithDirection(ably.Forwards)) so versions are returned
chronologically, or alternatively invert the final assertions to expect
newest-first order (use ably.MessageActionUpdate for versions[0] and
ably.MessageActionCreate for the last) in the test function that calls
GetMessageVersions and asserts MessageActionCreate/MessageActionUpdate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fac6f24e-5447-498c-abb9-df4c6b7c7f36
📒 Files selected for processing (7)
ably/example_message_updates_test.goably/message_updates_integration_test.goably/proto_message.goably/proto_message_operations_test.goably/realtime_channel.goably/rest_channel.goably/rest_client.go
✅ Files skipped from review due to trivial changes (1)
- ably/example_message_updates_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- ably/proto_message_operations_test.go
- ably/proto_message.go
- ably/rest_channel.go
| t.Run("GetMessageVersions", func(t *testing.T) { | ||
| channel := client.Channels.Get("mutable:test_get_message_versions") | ||
|
|
||
| t.Run("retrieves all versions after updates", func(t *testing.T) { | ||
| // Publish initial message | ||
| publishResult, err := channel.PublishWithResult(ctx, "test-event", "version 1") | ||
| require.NoError(t, err) | ||
| require.NotNil(t, publishResult.Serial) | ||
|
|
||
| // Update the message twice | ||
| msg := &ably.Message{ | ||
| Serial: *publishResult.Serial, | ||
| Data: "version 2", | ||
| } | ||
| _, err = channel.UpdateMessage(ctx, msg, ably.UpdateWithDescription("First update")) | ||
| require.NoError(t, err) | ||
|
|
||
| msg.Data = "version 3" | ||
| _, err = channel.UpdateMessage(ctx, msg, ably.UpdateWithDescription("Second update")) | ||
| require.NoError(t, err) | ||
|
|
||
| // GetMessageVersions is eventually consistent - retry until all versions are available | ||
| var versions []*ably.Message | ||
| require.Eventually(t, func() bool { | ||
| page, err := channel.GetMessageVersions(*publishResult.Serial, nil).Pages(ctx) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Must call Next() to decode the response body into items | ||
| if !page.Next(ctx) { | ||
| return false | ||
| } | ||
|
|
||
| versions = page.Items() | ||
|
|
||
| // Should have exactly 3 versions: original publish + 2 updates | ||
| return len(versions) == 3 | ||
| }, 10*time.Second, 200*time.Millisecond, "All three message versions should be retrievable") | ||
|
|
||
| // Verify we have exactly 3 versions in the correct order | ||
| require.Equal(t, 3, len(versions)) | ||
| assert.Equal(t, ably.MessageActionCreate, versions[0].Action, "First version should be MESSAGE_CREATE") | ||
| assert.Equal(t, ably.MessageActionUpdate, versions[1].Action, "Second version should be MESSAGE_UPDATE") | ||
| assert.Equal(t, ably.MessageActionUpdate, versions[2].Action, "Third version should be MESSAGE_UPDATE") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or spec reference about version ordering
rg -n -i 'GetMessageVersions|version.*order|chronological' --type=go -C2Repository: ably/ably-go
Length of output: 2700
🏁 Script executed:
#!/bin/bash
# Check the HistoryRequest implementation and how it's used
rg -A10 'type HistoryRequest' --type=go
# Also look for any paginate or iteration logic that might indicate ordering
rg -n 'Messages.*serial|sort.*version' --type=go -iRepository: ably/ably-go
Length of output: 2936
🏁 Script executed:
#!/bin/bash
# Check if there are other tests with message versions to understand the expected order
rg -B5 -A10 'MessageActionCreate|MessageActionUpdate' ably/message_updates_integration_test.goRepository: ably/ably-go
Length of output: 805
🏁 Script executed:
#!/bin/bash
# Look for any API documentation or specification comments
find . -name "*.md" -o -name "*.txt" | xargs grep -l "GetMessageVersions\|message versions\|version.*order" 2>/dev/null | head -5Repository: ably/ably-go
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look at how the endpoint is called and if there are any query parameters
rg -B5 -A15 'func.*GetMessageVersions' --type=go ably/rest_channel.go
# Check if there are any sort order parameters
rg -n 'direction|sort|order' --type=go -i | grep -i 'version\|history' | head -10Repository: ably/ably-go
Length of output: 1554
🏁 Script executed:
#!/bin/bash
# Look for any API specification files or constants that define ordering
find . -type f \( -name "*.go" -o -name "*.md" -o -name "*.txt" -o -name "*.json" \) | xargs grep -l "message.*version.*order\|direction.*asc\|direction.*desc" 2>/dev/null | head -5Repository: ably/ably-go
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if MessagesPaginatedResult has any ordering logic
rg -B5 -A20 'type MessagesPaginatedResult' --type=goRepository: ably/ably-go
Length of output: 1456
🏁 Script executed:
#!/bin/bash
# Look at the Direction type and constants
rg -B5 -A10 'type Direction|const.*Direction' --type=go | head -50
# Check the spec test to see what ordering is expected
rg -B10 -A20 'TestHistory_Direction_RSL2b2' --type=go ably/rest_channel_spec_integration_test.goRepository: ably/ably-go
Length of output: 1381
🏁 Script executed:
#!/bin/bash
# Look for any documentation about default ordering in history/versions
rg -B3 -A3 'default.*direction|oldest|newest' --type=go -i | head -30Repository: ably/ably-go
Length of output: 1607
The test relies on undocumented ordering assumptions that may be incorrect.
GetMessageVersions defaults to backwards direction (newest-first per RSL2b2), but the assertions at lines 217-219 assume chronological order (oldest-first). Either pass ably.HistoryWithDirection(ably.Forwards) when calling GetMessageVersions(), or reverse the assertion order to match the default backwards ordering:
assert.Equal(t, ably.MessageActionUpdate, versions[0].Action, "First version should be MESSAGE_UPDATE (newest)")
assert.Equal(t, ably.MessageActionUpdate, versions[1].Action, "Second version should be MESSAGE_UPDATE")
assert.Equal(t, ably.MessageActionCreate, versions[2].Action, "Third version should be MESSAGE_CREATE (oldest)")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ably/message_updates_integration_test.go` around lines 175 - 221, The test
assumes GetMessageVersions returns versions oldest-first but the API defaults to
newest-first; update the call to
channel.GetMessageVersions(*publishResult.Serial,
ably.HistoryWithDirection(ably.Forwards)) so versions are returned
chronologically, or alternatively invert the final assertions to expect
newest-first order (use ably.MessageActionUpdate for versions[0] and
ably.MessageActionCreate for the last) in the test function that calls
GetMessageVersions and asserts MessageActionCreate/MessageActionUpdate.
There was a problem hiding this comment.
Whilst it is not documented, message versions are always returned in this order, there is no support for retrieving message versions in any other order.
There was a problem hiding this comment.
@lmars, thanks for the clarification! I apologize for the incorrect assumption — I mistakenly conflated GetMessageVersions with the general history API's default backwards direction. The test's assertions are correct as-is.
✏️ Learnings added
Learnt from: lmars
Repo: ably/ably-go PR: 697
File: ably/message_updates_integration_test.go:175-221
Timestamp: 2026-03-24T14:38:14.657Z
Learning: In ably/ably-go, `GetMessageVersions` (in `ably/rest_channel.go` and `ably/realtime_channel.go`) always returns message versions in chronological order (oldest-first). Unlike the history API which defaults to newest-first (backwards), `GetMessageVersions` has a fixed ordering with no support for changing direction. Test assertions should therefore expect MESSAGE_CREATE first, followed by subsequent MESSAGE_UPDATE entries in order.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Same issue here: *updateResult.VersionSerial is dereferenced without checking for nil, even though VersionSerial is documented as optional. Add a nil check (and consider documenting why it may be nil) so the example remains safe to copy/paste.
| // VersionSerial is optional and may be nil if the server did not return it. | |
| if updateResult.VersionSerial == nil { | |
| fmt.Println("Message updated, but version serial is not available") | |
| return | |
| } |
| Action: actionMessage, | ||
| Channel: c.Name, | ||
| Messages: []*Message{&opMsg}, | ||
| Params: opts.params, |
There was a problem hiding this comment.
opts.params is a map[string]string (see updateOptions), but protocolMessage.Params is of type channelParams (a distinct named type). The struct literal assignment Params: opts.params will not compile. Convert explicitly (e.g., Params: channelParams(opts.params)) or change updateOptions.params to channelParams and adapt UpdateWithParams accordingly.
| Params: opts.params, | |
| Params: channelParams(opts.params), |
There was a problem hiding this comment.
channelParams is a type alias for map[string]string so the two can be used interchangeably, and map[string]string makes it more obvious what these params are, so keeping as is.
| version *MessageVersion // unexported, built lazily from options | ||
| params map[string]string // URL query parameters (RSL15f) / ProtocolMessage params (RTL32e) |
There was a problem hiding this comment.
updateOptions.params is declared as map[string]string, but protocolMessage.Params expects channelParams (a named map[string]string type). This causes type-mismatch at call sites (e.g., protocolMessage{Params: opts.params}) unless every use does an explicit conversion. Consider changing params to type channelParams (and convert in UpdateWithParams) to keep types consistent.
| version *MessageVersion // unexported, built lazily from options | |
| params map[string]string // URL query parameters (RSL15f) / ProtocolMessage params (RTL32e) | |
| version *MessageVersion // unexported, built lazily from options | |
| params channelParams // URL query parameters (RSL15f) / ProtocolMessage params (RTL32e) |
There was a problem hiding this comment.
channelParams is a type alias for map[string]string so the two can be used interchangeably, and map[string]string makes it more obvious what these params are, so keeping as is.
ably/rest_channel.go
Outdated
| var err error | ||
| *t.dst, err = t.dst.withDecodedData(cipher) | ||
| if err != nil { | ||
| t.c.log().Errorf("Couldn't fully decode message data from channel %q: %w", t.c.Name, err) |
There was a problem hiding this comment.
%w is only valid with fmt.Errorf; in Errorf/Sprintf-style logging it will render as %!w(...) and lose the intended error formatting. Use %v (or %s if appropriate) for the error argument in this log line.
| t.c.log().Errorf("Couldn't fully decode message data from channel %q: %w", t.c.Name, err) | |
| t.c.log().Errorf("Couldn't fully decode message data from channel %q: %v", t.c.Name, err) |
ably/paginated_result.go
Outdated
| path string | ||
| rawPath string | ||
| params url.Values | ||
| header http.Header // Optional custom headers |
There was a problem hiding this comment.
paginatedRequest gained a header http.Header field but it isn't referenced anywhere (pagination uses the query func directly). If custom headers are no longer needed for this approach (since Stats overrides req.query), consider removing this field to avoid dead/unused API surface, or wire it into newPaginatedRequest/query so it has an effect.
| header http.Header // Optional custom headers |
| // Publish a message and get its serial | ||
| result, err := channel.PublishWithResult(context.Background(), "event-name", "message data") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| fmt.Printf("Message published with serial: %s\n", *result.Serial) | ||
| } |
There was a problem hiding this comment.
These examples dereference *result.Serial / *updateResult.VersionSerial directly, but both fields are documented as potentially nil (e.g., when a message is discarded by conflation or superseded). For safety (and to avoid encouraging panics in copied example code), add a nil check and/or print an alternate message when the pointer is nil.
Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
| } | ||
| } | ||
|
|
||
| type ackCallback struct { |
There was a problem hiding this comment.
| type ackCallback struct { | |
| type msgAckCallback struct { |
So, we can use ackCallback as a variable name for msgAckCallback interface
| msg *protocolMessage | ||
| onAck func(err error) | ||
| msg *protocolMessage | ||
| callback *ackCallback |
There was a problem hiding this comment.
| callback *ackCallback | |
| ackCallback *msgAckCallback |
| if enqueued := c.maybeEnqueue(msg, onAck); enqueued { | ||
| // performMessageOperationAsync is a shared helper for UpdateMessageAsync, DeleteMessageAsync, and AppendMessageAsync. | ||
| // It validates the message serial, applies update options, sets the action, and sends the protocol message. | ||
| func (c *RealtimeChannel) performMessageOperationAsync(msg *Message, action MessageAction, onAck func(*UpdateDeleteResult, error), options ...UpdateOption) error { |
There was a problem hiding this comment.
As per spec, we also need clientId validation RTL6g3
This implements message updates, deletes, and appends as proposed in #696.
Summary by CodeRabbit
New Features
Tests