[messages] cancellation support#241
Draft
capcom6 wants to merge 1 commit into
Draft
Conversation
📝 WalkthroughWalkthroughAdds end-to-end message cancellation: new ChangesMessage Cancellation Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant ThirdPartyController
participant Service
participant Repository
participant Cache
participant Events
Client->>ThirdPartyController: DELETE /3rdparty/v1/messages/:id
ThirdPartyController->>Service: CancelMessage(userID, id)
Service->>Repository: GetMessage(userID, id)
Repository-->>Service: message + device info
Service->>Repository: CancelMessage(userID, id)
Repository-->>Service: ok (Pending→Cancelling) / ErrMessageNotPending
Service->>Cache: Delete(userID, id)
Cache-->>Service: nil / wrapped error
Service--)Events: Notify(device, NewMessageCancelledEvent) [async]
Service->>Repository: GetState(userID, id)
Repository-->>Service: MessageState
Service-->>ThirdPartyController: *MessageState, error
ThirdPartyController-->>Client: 200 GetMessageResponse / 400 BadRequest / 404
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
🤖 Pull request artifacts
|
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sms-gateway/handlers/messages/3rdparty.go`:
- Around line 269-271: The error handling in the cancel endpoint currently
returns a 400 BadRequest status for both ErrMessageNotPending and
ErrMessageNotFound errors, but ErrMessageNotFound should return a 404 NotFound
status to match the endpoint's contract and the controller's general not-found
mapping. Modify the conditional logic to handle ErrMessageNotFound separately by
checking for it first with a dedicated return statement that uses
fiber.StatusNotFound, then keep the existing check for ErrMessageNotPending that
returns fiber.StatusBadRequest.
In `@internal/sms-gateway/handlers/messages/params.go`:
- Around line 48-50: The State filter validation is rejecting cancellation
states (Cancelling and Cancelled) even though the filter now supports slices and
these states have been added to the models. Locate the validation logic that
validates the p.State parameter in the message filtering params (likely in a
validation function or validator tags for the params struct) and update it to
allow the cancellation states (Cancelling and Cancelled) in addition to any
existing allowed states. Ensure the validation permits these new states so
clients can properly filter messages by cancellation status.
In
`@internal/sms-gateway/models/migrations/mysql/20260617000000_add_cancelled_message_state.sql`:
- Around line 39-69: The Down migration removes `Cancelling` and `Cancelled`
enum values from the messages, message_recipients, and message_states tables
without first handling any existing rows that contain these values. Before each
MODIFY COLUMN statement for the messages table, message_recipients table, and
message_states table, add UPDATE statements to remap any rows with state values
of `Cancelling` or `Cancelled` to a valid enum value such as `Failed` or
`Pending`. This ensures the ALTER TABLE commands will not fail due to constraint
violations when attempting to shrink the enums during rollback.
In `@internal/sms-gateway/modules/messages/models.go`:
- Line 39: The enum ordering in the gorm tag for the State field with
ProcessingState type is inconsistent with the SQL migration. Update the enum
values in the gorm tag on lines 39, 202, and 231 to place 'Processed' before
'Sent' to match the migration definition. Change the order from
'Pending','Cancelling','Cancelled','Sent','Processed','Delivered','Failed' to
'Pending','Cancelling','Cancelled','Processed','Sent','Delivered','Failed' in
all three gorm type enum declarations.
In `@internal/sms-gateway/modules/messages/service.go`:
- Around line 218-220: After the successful CancelMessage call in the block at
lines 218-220, the cache needs to be invalidated or bypassed before the
subsequent GetState call at line 234. The issue is that GetState uses a
cache-first approach, so without invalidating the cache after the state
transition, it will return stale cached data showing the message as Pending
instead of the new Cancelling state. Add cache invalidation logic immediately
after the successful CancelMessage call, using the same userID and id parameters
to ensure the cached entry for this message is cleared so that GetState
retrieves fresh data.
🪄 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: a5cace6d-1c94-44de-8e1a-56080e336ba1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
go.modinternal/sms-gateway/handlers/converters/messages.gointernal/sms-gateway/handlers/messages/3rdparty.gointernal/sms-gateway/handlers/messages/params.gointernal/sms-gateway/handlers/messages/permissions.gointernal/sms-gateway/models/migrations/mysql/20260617000000_add_cancelled_message_state.sqlinternal/sms-gateway/modules/events/events.gointernal/sms-gateway/modules/messages/converters.gointernal/sms-gateway/modules/messages/domain.gointernal/sms-gateway/modules/messages/errors.gointernal/sms-gateway/modules/messages/models.gointernal/sms-gateway/modules/messages/repository.gointernal/sms-gateway/modules/messages/repository_filter.gointernal/sms-gateway/modules/messages/service.gointernal/sms-gateway/openapi/docs.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sms-gateway/modules/messages/cache.go`:
- Around line 75-82: The Delete method in stateCache lacks a timeout wrapper
around the context passed to c.storage.Delete, unlike the Set and Get methods
which apparently have this protection. This allows slow or stuck cache backends
to block indefinitely. Add a timeout context wrapper using cacheTimeout around
the ctx parameter before passing it to c.storage.Delete, similar to how it's
implemented in the Set and Get methods, to prevent request-path hangs when the
cache backend experiences delays.
🪄 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: 7213ea16-22d4-4915-be3a-a765e1461d26
📒 Files selected for processing (6)
internal/sms-gateway/handlers/messages/3rdparty.gointernal/sms-gateway/handlers/messages/params.gointernal/sms-gateway/models/migrations/mysql/20260617000000_add_cancelled_message_state.sqlinternal/sms-gateway/modules/messages/cache.gointernal/sms-gateway/modules/messages/models.gointernal/sms-gateway/modules/messages/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sms-gateway/modules/messages/service.go
- internal/sms-gateway/modules/messages/models.go
capcom6
force-pushed
the
messages/cancelation-support
branch
from
52d368f to
0e934d3
Compare
capcom6
force-pushed
the
messages/cancelation-support
branch
from
067b38f to
9610209
Compare
capcom6
force-pushed
the
messages/cancelation-support
branch
from
bc41ee1 to
3a692dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sms-gateway/modules/messages/repository.go`:
- Around line 196-208: The CancelMessage method uses a JOIN clause with an
UPDATE statement, but GORM v1.31.1 does not apply JOINs to UPDATE operations,
only to WHERE conditions. This causes the query to fail because devices.user_id
cannot be resolved. Replace the Joins("JOIN devices ON messages.device_id =
devices.id") with a subquery that selects device IDs where the user matches the
provided userID, then use this subquery in the WHERE clause instead of directly
referencing the devices table. This ensures the device ownership check works
correctly without relying on the unsupported JOIN syntax in UPDATE statements.
🪄 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: b6fb9b22-f158-4603-ae84-cda6ab16f125
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
api/local.httpapi/requests.httpgo.modinternal/sms-gateway/handlers/converters/messages.gointernal/sms-gateway/handlers/messages/3rdparty.gointernal/sms-gateway/handlers/messages/params.gointernal/sms-gateway/handlers/messages/permissions.gointernal/sms-gateway/models/migrations/mysql/20260617000000_add_cancelled_message_state.sqlinternal/sms-gateway/modules/events/events.gointernal/sms-gateway/modules/messages/cache.gointernal/sms-gateway/modules/messages/converters.gointernal/sms-gateway/modules/messages/domain.gointernal/sms-gateway/modules/messages/errors.gointernal/sms-gateway/modules/messages/models.gointernal/sms-gateway/modules/messages/repository.gointernal/sms-gateway/modules/messages/repository_filter.gointernal/sms-gateway/modules/messages/service.gointernal/sms-gateway/openapi/docs.go
✅ Files skipped from review due to trivial changes (5)
- go.mod
- internal/sms-gateway/handlers/converters/messages.go
- internal/sms-gateway/modules/messages/errors.go
- internal/sms-gateway/handlers/messages/permissions.go
- internal/sms-gateway/openapi/docs.go
🚧 Files skipped from review as they are similar to previous changes (12)
- api/requests.http
- internal/sms-gateway/handlers/messages/params.go
- internal/sms-gateway/modules/messages/converters.go
- internal/sms-gateway/modules/messages/cache.go
- internal/sms-gateway/modules/messages/repository_filter.go
- internal/sms-gateway/modules/messages/domain.go
- internal/sms-gateway/modules/events/events.go
- internal/sms-gateway/modules/messages/service.go
- internal/sms-gateway/handlers/messages/3rdparty.go
- api/local.http
- internal/sms-gateway/modules/messages/models.go
- internal/sms-gateway/models/migrations/mysql/20260617000000_add_cancelled_message_state.sql
Comment on lines
+196
to
+208
| func (r *Repository) CancelMessage(userID string, id string) error { | ||
| res := r.db.Model((*messageModel)(nil)). | ||
| Joins("JOIN devices ON messages.device_id = devices.id"). | ||
| Where("messages.ext_id = ? AND devices.user_id = ? AND messages.state = ?", id, userID, ProcessingStatePending). | ||
| Update("messages.state", ProcessingStateCancelling) | ||
| if res.Error != nil { | ||
| return fmt.Errorf("failed to cancel message: %w", res.Error) | ||
| } | ||
| if res.RowsAffected == 0 { | ||
| return ErrMessageNotPending | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🌐 Web query:
Does gorm.io/gorm v1.31 include the Joins clause when building an UPDATE statement, or is the JOIN omitted from the generated SQL?
💡 Result:
In GORM v1.31, the standard Joins clause (e.g., db.Joins("...").Updates(...)) does not automatically include a JOIN in the generated SQL for UPDATE statements [1]. When building an UPDATE query, GORM by default restricts the statement structure to UPDATE, SET, and WHERE clauses [2]. Consequently, any Joins specified through the standard API are typically omitted from the final UPDATE SQL, which often leads to errors if the WHERE clause relies on tables that were intended to be joined [1]. To perform an UPDATE with a JOIN, you must manually override the default clauses of the GORM statement. This is typically achieved by defining the required clauses (e.g., UPDATE, SET, FROM, WHERE) and using the db.Clauses API to inject them [3][2]. For example, to execute an UPDATE with a FROM clause (which is how many databases support joining tables during an update), you can use the following pattern [3][2]: tx:= db.Model(&YourModel{}).Clauses(clause.From{Tables: []clause.Table{{Name: "other_table"}}}) tx.Statement.BuildClauses = []string{"UPDATE", "SET", "FROM", "WHERE"} tx.Where("...").Updates(values) This approach ensures the generated SQL includes the necessary JOIN or FROM components that GORM omits by default during standard update operations [2].
Citations:
- 1: 在更新数据时join表不生效？ go-gorm/gorm#6100
- 2: How to change clauses of a given statement (UPDATE's ["UPDATE", "SET", "WHERE"] to ["UPDATE", "SET", "FROM", "WHERE"]) go-gorm/gorm#4445
- 3: Use FROM clause on update (PostgreSQL) go-gorm/gorm#4248
🏁 Script executed:
# First, check repository structure
git ls-files | grep -E "(go.mod|repository\.go)" | head -20Repository: android-sms-gateway/server
Length of output: 474
🏁 Script executed:
# Find the specific file mentioned in the review
fd -t f "repository.go" | grep messagesRepository: android-sms-gateway/server
Length of output: 216
🏁 Script executed:
# Check GORM version in go.mod
grep -E "gorm\.io/gorm" go.modRepository: android-sms-gateway/server
Length of output: 186
🏁 Script executed:
# Read the CancelMessage function
sed -n '196,208p' internal/sms-gateway/modules/messages/repository.goRepository: android-sms-gateway/server
Length of output: 668
CancelMessage will fail at runtime — GORM v1.31.1 omits the JOIN from UPDATE statements.
GORM does not include Joins(...) in the generated SQL for UPDATE operations; it only applies the join logic to WHERE conditions. The generated SQL is UPDATE messages SET state=? WHERE messages.ext_id=? AND devices.user_id=? AND messages.state=? with no JOIN devices, causing MySQL to fail with Unknown column 'devices.user_id' in 'where clause'. This is documented behavior in GORM v1.31.1 and makes the cancel path unreachable.
Replace the join with a subquery:
🐛 Proposed fix using a subquery for device ownership
func (r *Repository) CancelMessage(userID string, id string) error {
res := r.db.Model((*messageModel)(nil)).
- Joins("JOIN devices ON messages.device_id = devices.id").
- Where("messages.ext_id = ? AND devices.user_id = ? AND messages.state = ?", id, userID, ProcessingStatePending).
- Update("messages.state", ProcessingStateCancelling)
+ Where("ext_id = ? AND state = ?", id, ProcessingStatePending).
+ Where("device_id IN (?)", r.db.Table("devices").Select("id").Where("user_id = ?", userID)).
+ Update("state", ProcessingStateCancelling)
if res.Error != nil {
return fmt.Errorf("failed to cancel message: %w", res.Error)
}
if res.RowsAffected == 0 {
return ErrMessageNotPending
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (r *Repository) CancelMessage(userID string, id string) error { | |
| res := r.db.Model((*messageModel)(nil)). | |
| Joins("JOIN devices ON messages.device_id = devices.id"). | |
| Where("messages.ext_id = ? AND devices.user_id = ? AND messages.state = ?", id, userID, ProcessingStatePending). | |
| Update("messages.state", ProcessingStateCancelling) | |
| if res.Error != nil { | |
| return fmt.Errorf("failed to cancel message: %w", res.Error) | |
| } | |
| if res.RowsAffected == 0 { | |
| return ErrMessageNotPending | |
| } | |
| return nil | |
| } | |
| func (r *Repository) CancelMessage(userID string, id string) error { | |
| res := r.db.Model((*messageModel)(nil)). | |
| Where("ext_id = ? AND state = ?", id, ProcessingStatePending). | |
| Where("device_id IN (?)", r.db.Table("devices").Select("id").Where("user_id = ?", userID)). | |
| Update("state", ProcessingStateCancelling) | |
| if res.Error != nil { | |
| return fmt.Errorf("failed to cancel message: %w", res.Error) | |
| } | |
| if res.RowsAffected == 0 { | |
| return ErrMessageNotPending | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/sms-gateway/modules/messages/repository.go` around lines 196 - 208,
The CancelMessage method uses a JOIN clause with an UPDATE statement, but GORM
v1.31.1 does not apply JOINs to UPDATE operations, only to WHERE conditions.
This causes the query to fail because devices.user_id cannot be resolved.
Replace the Joins("JOIN devices ON messages.device_id = devices.id") with a
subquery that selects device IDs where the user matches the provided userID,
then use this subquery in the WHERE clause instead of directly referencing the
devices table. This ensures the device ownership check works correctly without
relying on the unsupported JOIN syntax in UPDATE statements.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
DELETE /3rdparty/v1/messages/{id}(requiresmessages:cancel), returning the updated message state.CancellingandCancelled.sms:cancelled.Improvements
Cancelling/Cancelledand support filtering across multiple states.