feat(apps): validate +file-list --page-size against server (0, 200] range#2007
Open
chenxingyang1019 wants to merge 1 commit into
Open
feat(apps): validate +file-list --page-size against server (0, 200] range#2007chenxingyang1019 wants to merge 1 commit into
chenxingyang1019 wants to merge 1 commit into
Conversation
…ange
paas_storage AppFileListForOpenAPI rejects page_size > 200 at the inner
checkMaxKeys guard with ErrInvalidRequest("maxKeys not in range (0, 200]").
Previously the CLI forwarded any --page-size straight to the API, so
--page-size 500 produced an opaque server error round-trip.
Add a client-side Validate check bounding --page-size to [1, 200] (aligned
with the existing validateAppsPageSize precedent in the observability
commands): out-of-range values now fail fast with a typed validation error
and never hit the network. The server tolerates page_size <= 0 by defaulting
to 20, but the CLI default is already 20 and an explicit < 1 is a user error,
so we reject it for a clearer message, consistent with other list commands.
Update the flag description and the lark-apps-file skill reference to
document the 1..200 range, and cover the boundaries in unit tests.
github-actions
Bot
added
the
size/M
📝 WalkthroughWalkthroughThe ChangesFile list page-size validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 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 `@shortcuts/apps/apps_file_list_test.go`:
- Around line 101-111: The TestAppsFileList_PageSizeBoundaryOK test only
verifies successful execution and does not confirm the boundary value is
forwarded. Inspect the dry-run payload or built parameters returned by
runAppsShortcut and assert that page_size matches the current ps value for both
"1" and "200", so the test detects regressions in parameter propagation.
🪄 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
Run ID: ee62d3b9-f667-4ec6-a653-68327530e2b5
📒 Files selected for processing (3)
shortcuts/apps/apps_file_list.goshortcuts/apps/apps_file_list_test.goskills/lark-apps/references/lark-apps-file.md
Comment on lines
+101
to
+111
| // TestAppsFileList_PageSizeBoundaryOK 验证边界值 1 与 200 通过校验（dry-run 不报错并把 page_size 下发）。 | ||
| func TestAppsFileList_PageSizeBoundaryOK(t *testing.T) { | ||
| for _, ps := range []string{"1", "200"} { | ||
| factory, stdout, _ := newAppsExecuteFactory(t) | ||
| if err := runAppsShortcut(t, AppsFileList, | ||
| []string{"+file-list", "--app-id", "app_x", "--page-size", ps, "--dry-run", "--as", "user"}, | ||
| factory, stdout); err != nil { | ||
| t.Fatalf("page-size=%s: dry-run err=%v", ps, err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the forwarded page_size value.
Lines 102-108 only verify that dry-run succeeds; the pre-change implementation would also accept 1 and 200. Inspect the dry-run payload or built parameters and assert that page_size equals each boundary value so reverting the implementation makes this test fail.
🤖 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 `@shortcuts/apps/apps_file_list_test.go` around lines 101 - 111, The
TestAppsFileList_PageSizeBoundaryOK test only verifies successful execution and
does not confirm the boundary value is forwarded. Inspect the dry-run payload or
built parameters returned by runAppsShortcut and assert that page_size matches
the current ps value for both "1" and "200", so the test detects regressions in
parameter propagation.
Source: Coding guidelines
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@570211de2ac8f8737ab1cb2fa3232fac35ae002b🧩 Skill updatenpx skills add larksuite/cli#feat/apps-file-list-page-size -y -g |
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.
What
apps +file-list --page-sizenow validates against the server's page-size contract before sending the request.Why
The
paas_storageAppFileListForOpenAPIchain rejectspage_size > 200at the innercheckMaxKeysguard:Previously the CLI forwarded any
--page-sizestraight through, so--page-size 500produced an opaque server-error round-trip instead of a clear local error.What changed
shortcuts/apps/apps_file_list.go— add aValidatecheck bounding--page-sizeto[1, 200], mirroring the existingvalidateAppsPageSizeprecedent in the observability commands. Out-of-range values fail fast with a typed validation error (exit 2) and never hit the network. Flag description updated topage size (1..200).shortcuts/apps/apps_file_list_test.go— unit tests for out-of-range (0/201/500) and boundary (1/200) values.skills/lark-apps/references/lark-apps-file.md— document the1..200range.Note on the lower bound
The server tolerates
page_size <= 0by defaulting to 20. The CLI default is already 20 and an explicit< 1is a user error, so we reject it for a clearer message — consistent with the other list commands. Happy to relax to> 200-only if reviewers prefer strict server-parity.Testing
go build ./...clean;go test ./shortcuts/apps/green.--page-sizevalidation cases (out-of-range → exit 2 no round-trip;200boundary → passes).Summary by CodeRabbit
Bug Fixes
Documentation