fix(api): support --body alias for raw requests#504
fix(api): support --body alias for raw requests#504hiSandog wants to merge 3 commits intolarksuite:mainfrom
Conversation
|
|
github-actions
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 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. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/api.go (1)
162-169:⚠️ Potential issue | 🟡 Minor
--bodyusers can still get--data-only validation messages.After alias resolution, this flow can be reached via
--body, but Line 168 still says--data, andParseOptionalBodyerrors (invoked at Line 163 and Line 189) also report--data. Please emit--data/--body(or pass a flag label) so guidance stays accurate for users and automation.Suggested direction
- return client.RawApiRequest{}, nil, output.ErrValidation("--data must be a JSON object when used with --file") + return client.RawApiRequest{}, nil, output.ErrValidation("--data/--body must be a JSON object when used with --file")And consider adding a
ParseOptionalBodyWithFlag(flagName, ...)variant so invalid-JSON/resolve-input errors can mention--data/--bodyin this command path.As per coding guidelines: "Error messages must be structured, actionable, and specific since they will be parsed by AI agents to determine next actions."
Also applies to: 189-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/api.go` around lines 162 - 169, The validation/error text wrongly references only "--data" even when the input came via the "--body" alias; update the code paths that call ParseOptionalBody (the dataFlag handling using variable dataFlag and the later call that also invokes ParseOptionalBody) to surface the correct flag name by either (a) passing a flag label into a new ParseOptionalBodyWithFlag(flagName, method, flagValue, stdin) and use that in error returns, or (b) build the error strings dynamically from dataFlag (or an explicit flagName variable) so the output.ErrValidation and any ParseOptionalBody error messages mention "--data/--body" (or the actual flag used) instead of always "--data"; ensure you update both call sites that currently call ParseOptionalBody to use the new variant or dynamic flag-aware error text and change the ErrValidation message reference (the return that currently uses output.ErrValidation("--data must be a JSON object when used with --file")) to include the correct flag label.
🧹 Nitpick comments (2)
cmd/api/api_test.go (2)
117-120: Add explicit config-dir isolation in these new tests.Please set
LARKSUITE_CLI_CONFIG_DIRviat.Setenvin each new test before creating the factory, to keep test config state isolated.Suggested patch
func TestBuildAPIRequest_BodyAlias(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, _, _, _ := cmdutil.TestFactory(t, &core.CliConfig{ AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu, }) @@ func TestBuildAPIRequest_DataAndBodyConflict(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, _, _, _ := cmdutil.TestFactory(t, &core.CliConfig{ AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu, })As per coding guidelines: "
**/*_test.go: Isolate config state in Go tests by usingt.Setenv(\"LARKSUITE_CLI_CONFIG_DIR\", t.TempDir())."Also applies to: 143-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/api_test.go` around lines 117 - 120, The new tests (e.g., TestBuildAPIRequest_BodyAlias) do not isolate config state; before calling cmdutil.TestFactory(t, ...) set the LARKSUITE_CLI_CONFIG_DIR environment to a temp dir using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so each test gets its own config directory; apply the same change to the other affected test(s) around the TestBuildAPIRequest_* functions (the places that call cmdutil.TestFactory) so factory creation happens after t.Setenv is called.
143-161: Consider adding a non-conflict parity test (--data==--body).You currently assert the differing-values conflict; a same-value case would lock the intended “allowed” behavior and prevent regressions in
resolveBodyFlag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/api_test.go` around lines 143 - 161, Add a new unit test (e.g., TestBuildAPIRequest_DataAndBodyParity) that mirrors TestBuildAPIRequest_DataAndBodyConflict but sets APIOptions.Data and APIOptions.Body to the identical JSON string and asserts buildAPIRequest returns no error and the resolved request body equals that string; locate the test helper/TestFactory usage and call buildAPIRequest (same symbol) and also validate resolveBodyFlag behavior indirectly by checking the request body or returned payload to ensure parity is allowed and preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/api/api.go`:
- Around line 162-169: The validation/error text wrongly references only
"--data" even when the input came via the "--body" alias; update the code paths
that call ParseOptionalBody (the dataFlag handling using variable dataFlag and
the later call that also invokes ParseOptionalBody) to surface the correct flag
name by either (a) passing a flag label into a new
ParseOptionalBodyWithFlag(flagName, method, flagValue, stdin) and use that in
error returns, or (b) build the error strings dynamically from dataFlag (or an
explicit flagName variable) so the output.ErrValidation and any
ParseOptionalBody error messages mention "--data/--body" (or the actual flag
used) instead of always "--data"; ensure you update both call sites that
currently call ParseOptionalBody to use the new variant or dynamic flag-aware
error text and change the ErrValidation message reference (the return that
currently uses output.ErrValidation("--data must be a JSON object when used with
--file")) to include the correct flag label.
---
Nitpick comments:
In `@cmd/api/api_test.go`:
- Around line 117-120: The new tests (e.g., TestBuildAPIRequest_BodyAlias) do
not isolate config state; before calling cmdutil.TestFactory(t, ...) set the
LARKSUITE_CLI_CONFIG_DIR environment to a temp dir using
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so each test gets its own
config directory; apply the same change to the other affected test(s) around the
TestBuildAPIRequest_* functions (the places that call cmdutil.TestFactory) so
factory creation happens after t.Setenv is called.
- Around line 143-161: Add a new unit test (e.g.,
TestBuildAPIRequest_DataAndBodyParity) that mirrors
TestBuildAPIRequest_DataAndBodyConflict but sets APIOptions.Data and
APIOptions.Body to the identical JSON string and asserts buildAPIRequest returns
no error and the resolved request body equals that string; locate the test
helper/TestFactory usage and call buildAPIRequest (same symbol) and also
validate resolveBodyFlag behavior indirectly by checking the request body or
returned payload to ensure parity is allowed and preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5f6c5c2-1a30-4691-8f5c-49d83aa64be9
📒 Files selected for processing (2)
cmd/api/api.gocmd/api/api_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/api.go (1)
168-170:⚠️ Potential issue | 🟡 MinorError message references
--dataonly, even when--bodywas supplied.When the user passed
--body(not--data), the message"--data must be a JSON object when used with --file"is misleading and harder for AI agents to map back to the offending flag. Consider mirroring the wording used in the stdin conflict on Line 139 (--data/--body).🔧 Proposed fix
- return client.RawApiRequest{}, nil, output.ErrValidation("--data must be a JSON object when used with --file") + return client.RawApiRequest{}, nil, output.ErrValidation("--data/--body must be a JSON object when used with --file")As per coding guidelines: "Make error messages structured, actionable, and specific for AI agent parsing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/api.go` around lines 168 - 170, Update the validation error message that checks dataFields (the map assertion around dataFields) so it references both flags `--data/--body` instead of only `--data`; modify the ErrValidation call in that branch (the block that returns client.RawApiRequest{}, nil, output.ErrValidation(...)) to use the same phrasing used for stdin conflicts (e.g., "--data/--body must be a JSON object when used with --file") so callers and agents can unambiguously map the error to either flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/api/api.go`:
- Around line 168-170: Update the validation error message that checks
dataFields (the map assertion around dataFields) so it references both flags
`--data/--body` instead of only `--data`; modify the ErrValidation call in that
branch (the block that returns client.RawApiRequest{}, nil,
output.ErrValidation(...)) to use the same phrasing used for stdin conflicts
(e.g., "--data/--body must be a JSON object when used with --file") so callers
and agents can unambiguously map the error to either flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb74d33d-a812-44ef-a507-1e36994f00b0
📒 Files selected for processing (2)
cmd/api/api.gocmd/api/api_test.go
2 participants
Summary
Validation
Context
README examples currently show --body, while the command only accepted --data. An open docs PR already fixes the examples; this change makes the CLI itself tolerant of the documented flag without overlapping that PR.
Summary by CodeRabbit
New Features
Bug Fixes
Tests