Fix mail rule reorder request IDs - #2166
Conversation
Fetch the current mail rule list before reorder and expand partial rule_ids into the complete order expected by the API. Reject duplicate or unknown rule IDs before submitting the write request. Test: go test ./cmd/service Co-authored-by: TRAE CLI <noreply@bytedance.com>
github-actions
Bot
added
the
size/L
📝 WalkthroughWalkthroughChangesMailbox rule reorder
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MailRulesReorderHook
participant mail_rules_list
participant mail_user_mailbox_rules_reorder
MailRulesReorderHook->>MailRulesReorderHook: resolve mailbox ID and validate rule IDs
MailRulesReorderHook->>mail_rules_list: fetch current rule IDs
mail_rules_list-->>MailRulesReorderHook: return paginated rule data
MailRulesReorderHook->>mail_user_mailbox_rules_reorder: submit completed rule ordering
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b47bee3bc03e8c8b8af411e8df7d368cb91da340🧩 Skill updatenpx skills add yangr-happy/cli#feat/960cc5c -y -g |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/service/service.go (1)
405-444: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--dry-runpreviews an incompleterule_idslist for mail rules reorder.The
mail.user_mailbox.rules.reordercompletion hook runs at Line 439, after theopts.DryRunearly return at Line 405. For this method,--dry-runprints the CLI-suppliedrule_idssubset, not the completed ordered list the API call would actually send. This defeats the purpose of--dry-runas a preview for a write operation, and directly undercuts this PR's goal of sending the complete ordered list.Move the schema-specific completion call before the
opts.DryRuncheck, so the previewed request matches what is actually sent:🐛 Proposed fix to complete the request before dry-run/confirmation
+ if opts.SchemaPath == "mail.user_mailbox.rules.reorder" { + if err := completeMailRulesReorderRequest(opts.Ctx, ac, opts, &request); err != nil { + return err + } + } + if opts.DryRun { if fileMeta != nil { return cmdutil.PrintDryRunWithFile(f.IOStreams.Out, request, config, opts.Format, fileMeta.FieldName, fileMeta.FilePath, fileMeta.FormFields) } return serviceDryRun(f, request, config, opts.Format) } if opts.Method.Risk == cmdutil.RiskHighRiskWrite { if yes, _ := opts.Cmd.Flags().GetBool("yes"); !yes { return cmdutil.RequireConfirmation(opts.SchemaPath) } } ac, err := f.NewAPIClientWithConfig(config) if err != nil { return err } out := f.IOStreams.Out format, formatOK := output.ParseFormat(opts.Format) if !formatOK { fmt.Fprintf(f.IOStreams.ErrOut, "warning: unknown format %q, falling back to json\n", opts.Format) } checkErr := ac.CheckResponse if opts.PageAll { return servicePaginate(opts.Ctx, ac, request, format, opts.JqExpr, out, f.IOStreams.ErrOut, opts.Cmd.CommandPath(), client.PaginationOptions{PageLimit: opts.PageLimit, PageDelay: opts.PageDelay}, checkErr) } - if opts.SchemaPath == "mail.user_mailbox.rules.reorder" { - if err := completeMailRulesReorderRequest(opts.Ctx, ac, opts, &request); err != nil { - return err - } - } - resp, err := ac.DoAPI(opts.Ctx, request)Note this requires moving the
acconstruction earlier too, sincecompleteMailRulesReorderRequestneedsacto fetch current rules. Confirm that issuing this read-onlyGETcall during--dry-runis acceptable for this command, since it is a real network call rather than a pure local preview.🤖 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 `@cmd/service/service.go` around lines 405 - 444, Move API client creation and the mail-specific completion call for completeMailRulesReorderRequest before the opts.DryRun early return in the service command flow. This ensures --dry-run fetches and previews the completed rule_ids list, matching the request sent during execution; preserve existing error handling and confirm the required read-only GET is performed during dry-run.
🤖 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 `@cmd/service/service_test.go`:
- Around line 482-504: Strengthen
TestMailRulesReorderListFailureDoesNotCallReorder by registering a POST stub for
the reorder endpoint with OnMatch setting a reorderCalled flag, then assert the
flag remains false after Execute returns an error. This must verify the reorder
request is skipped rather than only confirming that some error occurred.
In `@cmd/service/service.go`:
- Around line 808-826: The stringSliceField helper validates trimmed string
entries but stores their raw values, causing rule ID comparisons and duplicate
detection to disagree with normalized IDs. In stringSliceField, trim each
non-empty string and append the trimmed value to out, while preserving the
existing validation and error behavior.
- Around line 692-699: Update completeMailRulesReorderRequest to explicitly
check that body contains the rule_ids key before calling stringSliceField.
Return the existing typed validation error for a missing field, while preserving
the current handling of an explicitly provided empty array.
---
Outside diff comments:
In `@cmd/service/service.go`:
- Around line 405-444: Move API client creation and the mail-specific completion
call for completeMailRulesReorderRequest before the opts.DryRun early return in
the service command flow. This ensures --dry-run fetches and previews the
completed rule_ids list, matching the request sent during execution; preserve
existing error handling and confirm the required read-only GET is performed
during dry-run.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ddf950e-3b0c-4dc7-b959-bd25f40bdf2b
📒 Files selected for processing (2)
cmd/service/service.gocmd/service/service_test.go
| func TestMailRulesReorderListFailureDoesNotCallReorder(t *testing.T) { | ||
| f, _, _, reg := cmdutil.TestFactory(t, testConfig) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/mail/v1/user_mailboxes/me/rules", | ||
| Body: map[string]interface{}{ | ||
| "code": 999, | ||
| "msg": "list failed", | ||
| }, | ||
| }) | ||
|
|
||
| cmd := newMailRulesReorderCommand(f) | ||
| cmd.SetArgs([]string{ | ||
| "--as", "bot", | ||
| "--params", `{"user_mailbox_id":"me"}`, | ||
| "--data", `{"rule_ids":["rule_1"]}`, | ||
| }) | ||
|
|
||
| if err := cmd.Execute(); err == nil { | ||
| t.Fatal("expected list error") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Strengthen TestMailRulesReorderListFailureDoesNotCallReorder to actually prove the reorder call is skipped.
The test only asserts err != nil. If the ac.CheckResponse guard in fetchAllMailRuleIDs (service.go Line 757-759) were removed, extractMailRuleIDs would return nil for this malformed body (no data key), and completeMailRuleIDs would still return an "unknown rule_ids" validation error for the non-empty inputIDs. The test would still pass with that regression, so it doesn't verify the behavior its name claims.
Register a POST stub with OnMatch setting a reorderCalled flag (as in TestMailRulesReorderCompletesPartialRuleIDs), and assert !reorderCalled after Execute(), or assert the error message contains "list failed" to pin the failure to the list call specifically.
🧪 Proposed strengthening of the assertion
func TestMailRulesReorderListFailureDoesNotCallReorder(t *testing.T) {
f, _, _, reg := cmdutil.TestFactory(t, testConfig)
reg.Register(&httpmock.Stub{
Method: "GET",
URL: "/open-apis/mail/v1/user_mailboxes/me/rules",
Body: map[string]interface{}{
"code": 999,
"msg": "list failed",
},
})
+ reorderCalled := false
+ reg.Register(&httpmock.Stub{
+ Method: "POST",
+ URL: "/open-apis/mail/v1/user_mailboxes/me/rules/reorder",
+ OnMatch: func(req *http.Request) { reorderCalled = true },
+ Body: map[string]interface{}{
+ "code": 0,
+ "msg": "ok",
+ "data": map[string]interface{}{"ok": true},
+ },
+ })
cmd := newMailRulesReorderCommand(f)
cmd.SetArgs([]string{
"--as", "bot",
"--params", `{"user_mailbox_id":"me"}`,
"--data", `{"rule_ids":["rule_1"]}`,
})
if err := cmd.Execute(); err == nil {
t.Fatal("expected list error")
}
+ if reorderCalled {
+ t.Fatal("expected reorder API not to be called after list failure")
+ }
}📝 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.
| func TestMailRulesReorderListFailureDoesNotCallReorder(t *testing.T) { | |
| f, _, _, reg := cmdutil.TestFactory(t, testConfig) | |
| reg.Register(&httpmock.Stub{ | |
| Method: "GET", | |
| URL: "/open-apis/mail/v1/user_mailboxes/me/rules", | |
| Body: map[string]interface{}{ | |
| "code": 999, | |
| "msg": "list failed", | |
| }, | |
| }) | |
| cmd := newMailRulesReorderCommand(f) | |
| cmd.SetArgs([]string{ | |
| "--as", "bot", | |
| "--params", `{"user_mailbox_id":"me"}`, | |
| "--data", `{"rule_ids":["rule_1"]}`, | |
| }) | |
| if err := cmd.Execute(); err == nil { | |
| t.Fatal("expected list error") | |
| } | |
| } | |
| func TestMailRulesReorderListFailureDoesNotCallReorder(t *testing.T) { | |
| f, _, _, reg := cmdutil.TestFactory(t, testConfig) | |
| reg.Register(&httpmock.Stub{ | |
| Method: "GET", | |
| URL: "/open-apis/mail/v1/user_mailboxes/me/rules", | |
| Body: map[string]interface{}{ | |
| "code": 999, | |
| "msg": "list failed", | |
| }, | |
| }) | |
| reorderCalled := false | |
| reg.Register(&httpmock.Stub{ | |
| Method: "POST", | |
| URL: "/open-apis/mail/v1/user_mailboxes/me/rules/reorder", | |
| OnMatch: func(req *http.Request) { reorderCalled = true }, | |
| Body: map[string]interface{}{ | |
| "code": 0, | |
| "msg": "ok", | |
| "data": map[string]interface{}{"ok": true}, | |
| }, | |
| }) | |
| cmd := newMailRulesReorderCommand(f) | |
| cmd.SetArgs([]string{ | |
| "--as", "bot", | |
| "--params", `{"user_mailbox_id":"me"}`, | |
| "--data", `{"rule_ids":["rule_1"]}`, | |
| }) | |
| if err := cmd.Execute(); err == nil { | |
| t.Fatal("expected list error") | |
| } | |
| if reorderCalled { | |
| t.Fatal("expected reorder API not to be called after list failure") | |
| } | |
| } |
🤖 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 `@cmd/service/service_test.go` around lines 482 - 504, Strengthen
TestMailRulesReorderListFailureDoesNotCallReorder by registering a POST stub for
the reorder endpoint with OnMatch setting a reorderCalled flag, then assert the
flag remains false after Execute returns an error. This must verify the reorder
request is skipped rather than only confirming that some error occurred.
Source: Coding guidelines
| body, ok := request.Data.(map[string]interface{}) | ||
| if !ok { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--data must be a JSON object for mail rules reorder").WithParam("--data") | ||
| } | ||
| inputIDs, err := stringSliceField(body, "rule_ids") | ||
| if err != nil { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "rule_ids must be an array of non-empty strings").WithParam("rule_ids").WithCause(err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A missing rule_ids key silently becomes a no-op reorder instead of a validation error.
stringSliceField returns (nil, nil) when the rule_ids key is absent from body (Line 810-812). Nothing in completeMailRulesReorderRequest distinguishes "key missing" from "key present as empty array". Downstream, completeMailRuleIDs treats an empty inputIDs against a non-empty currentIDs as "keep the current order" (Lines 867-877), silently returning success instead of surfacing that the required rule_ids field was not supplied.
The method's own metadata declares rule_ids as "required": true in the request body. Rejecting an entirely missing rule_ids field with a typed validation error (distinct from an explicit empty array, if that is meant to be a valid "keep current order" request) makes the behavior consistent with the metadata contract and avoids a request silently doing nothing without any error to the caller.
🐛 Proposed fix to reject a missing `rule_ids` key
inputIDs, err := stringSliceField(body, "rule_ids")
if err != nil {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "rule_ids must be an array of non-empty strings").WithParam("rule_ids").WithCause(err)
}
+ if _, present := body["rule_ids"]; !present {
+ return errs.NewValidationError(errs.SubtypeInvalidArgument, "rule_ids is required").WithParam("rule_ids")
+ }
if duplicates := duplicateStrings(inputIDs); len(duplicates) > 0 {📝 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.
| body, ok := request.Data.(map[string]interface{}) | |
| if !ok { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--data must be a JSON object for mail rules reorder").WithParam("--data") | |
| } | |
| inputIDs, err := stringSliceField(body, "rule_ids") | |
| if err != nil { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "rule_ids must be an array of non-empty strings").WithParam("rule_ids").WithCause(err) | |
| } | |
| body, ok := request.Data.(map[string]interface{}) | |
| if !ok { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--data must be a JSON object for mail rules reorder").WithParam("--data") | |
| } | |
| inputIDs, err := stringSliceField(body, "rule_ids") | |
| if err != nil { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "rule_ids must be an array of non-empty strings").WithParam("rule_ids").WithCause(err) | |
| } | |
| if _, present := body["rule_ids"]; !present { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "rule_ids is required").WithParam("rule_ids") | |
| } |
🤖 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 `@cmd/service/service.go` around lines 692 - 699, Update
completeMailRulesReorderRequest to explicitly check that body contains the
rule_ids key before calling stringSliceField. Return the existing typed
validation error for a missing field, while preserving the current handling of
an explicitly provided empty array.
Source: Coding guidelines
| func stringSliceField(values map[string]interface{}, key string) ([]string, error) { | ||
| raw, ok := values[key] | ||
| if !ok || raw == nil { | ||
| return nil, nil | ||
| } | ||
| items, ok := raw.([]interface{}) | ||
| if !ok { | ||
| return nil, fmt.Errorf("%s is %T", key, raw) | ||
| } | ||
| out := make([]string, 0, len(items)) | ||
| for _, item := range items { | ||
| s, ok := item.(string) | ||
| if !ok || strings.TrimSpace(s) == "" { | ||
| return nil, fmt.Errorf("%s contains %T", key, item) | ||
| } | ||
| out = append(out, s) | ||
| } | ||
| return out, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validated rule_ids entries are stored untrimmed, causing mismatches with the trimmed currentIDs.
stringSliceField validates each entry using strings.TrimSpace(s) == "" (Line 820) but appends the raw s at Line 823. Meanwhile, firstString (used to build currentIDs from the API response) trims each value at Line 800-801. A user-supplied ID with surrounding whitespace, such as " rule_1", passes validation as non-empty but is then compared against the trimmed "rule_1" in completeMailRuleIDs, producing an incorrect "unknown rule_ids" error even though the ID logically matches. The same untrimmed value also defeats duplicateStrings, since "rule_1" and " rule_1" are treated as distinct.
🐛 Proposed fix to store the trimmed value
for _, item := range items {
s, ok := item.(string)
- if !ok || strings.TrimSpace(s) == "" {
+ trimmed := strings.TrimSpace(s)
+ if !ok || trimmed == "" {
return nil, fmt.Errorf("%s contains %T", key, item)
}
- out = append(out, s)
+ out = append(out, trimmed)
}📝 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.
| func stringSliceField(values map[string]interface{}, key string) ([]string, error) { | |
| raw, ok := values[key] | |
| if !ok || raw == nil { | |
| return nil, nil | |
| } | |
| items, ok := raw.([]interface{}) | |
| if !ok { | |
| return nil, fmt.Errorf("%s is %T", key, raw) | |
| } | |
| out := make([]string, 0, len(items)) | |
| for _, item := range items { | |
| s, ok := item.(string) | |
| if !ok || strings.TrimSpace(s) == "" { | |
| return nil, fmt.Errorf("%s contains %T", key, item) | |
| } | |
| out = append(out, s) | |
| } | |
| return out, nil | |
| } | |
| func stringSliceField(values map[string]interface{}, key string) ([]string, error) { | |
| raw, ok := values[key] | |
| if !ok || raw == nil { | |
| return nil, nil | |
| } | |
| items, ok := raw.([]interface{}) | |
| if !ok { | |
| return nil, fmt.Errorf("%s is %T", key, raw) | |
| } | |
| out := make([]string, 0, len(items)) | |
| for _, item := range items { | |
| s, ok := item.(string) | |
| trimmed := strings.TrimSpace(s) | |
| if !ok || trimmed == "" { | |
| return nil, fmt.Errorf("%s contains %T", key, item) | |
| } | |
| out = append(out, trimmed) | |
| } | |
| return out, 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 `@cmd/service/service.go` around lines 808 - 826, The stringSliceField helper
validates trimmed string entries but stores their raw values, causing rule ID
comparisons and duplicate detection to disagree with normalized IDs. In
stringSliceField, trim each non-empty string and append the trimmed value to
out, while preserving the existing validation and error behavior.
1 participant
Fixes mail rule reordering so the CLI sends the full ordered rule ID list expected by the mail API.
Summary by CodeRabbit
New Features
Bug Fixes