feat(base): support batch record get and delete#630
feat(base): support batch record get and delete#630zgz2048 wants to merge 4 commits intolarksuite:mainfrom
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:
📝 WalkthroughWalkthroughExtend record GET and DELETE shortcuts to support multi-record operations via repeatable Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI
participant Validator as validateRecordSelection
participant Router as Router/Executor
participant API as Lark API
User->>CLI: +record-get --record-id rec_1 rec_2 --field-id f1
CLI->>Validator: validateRecordSelection(runtime)
Validator-->>CLI: normalized record_id_list, select_fields
CLI->>Router: decide endpoint based on selection
Router->>API: POST /records/batch_get { record_id_list:[rec_1,rec_2], select_fields:[f1] }
alt 200 OK
API-->>Router: batch response (multiple records)
Router-->>CLI: format batch output
CLI-->>User: display records
else 404 Not Found
Router->>API: GET /records/{id} (per-record fallback)
API-->>Router: per-record responses
Router-->>CLI: aggregate/format per-record output
CLI-->>User: display records
end
sequenceDiagram
actor User
participant CLI as CLI
participant Validator as validateRecordSelection
participant Router as Router/Executor
participant API as Lark API
User->>CLI: +record-delete --record-id rec_1 rec_2 --yes
CLI->>Validator: validateRecordSelection(runtime)
Validator-->>CLI: normalized record_id_list
CLI->>Router: decide endpoint based on selection
alt multiple IDs or JSON
Router->>API: POST /records/batch_delete { record_id_list:[rec_1,rec_2] }
API-->>Router: batch delete response
Router-->>CLI: format batch delete output
CLI-->>User: display batch result
else single ID or batch 404
Router->>API: DELETE /records/{id}
API-->>Router: single delete response
Router-->>CLI: format single delete output
CLI-->>User: display result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #630 +/- ##
==========================================
+ Coverage 60.65% 60.75% +0.09%
==========================================
Files 416 416
Lines 43969 44292 +323
==========================================
+ Hits 26669 26908 +239
- Misses 15231 15296 +65
- Partials 2069 2088 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
shortcuts/common/runner.go (1)
856-890: Shorthand registration looks correct; consider a collision safeguard.The
*Pvariants are used appropriately whenfl.Shorthandis non-empty. One thing to keep in mind: Cobra will panic atMounttime if two flags within the same shortcut declare the same shorthand (or collide with the hard-coded-qfor--jqat line 912). That's an authoring-time error rather than a runtime bug, but it may be worth a lightweight sanity check inregisterShortcutFlagsWithContext(or a unit test enumerating all built-in shortcuts) to surface future regressions early — especially now that shortcut authors can freely pick single letters like-r/-f.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner.go` around lines 856 - 890, registerShortcutFlagsWithContext currently calls the Cobra *P flag helpers directly with fl.Shorthand which will panic on duplicate shorthands (or collision with the reserved -q/JQ flag); add a lightweight collision safeguard by tracking used shorthands per command before registering each flag (use fl.Shorthand and the target cmd.Flags()), and if a shorthand is already present or equals the reserved 'q', avoid calling the *P variant and either return an error or fall back to the non-shorthand variant and surface a clear error/log; update registerShortcutFlagsWithContext to consult this shorthand set before calling BoolP/IntP/StringP/StringArrayP/StringSliceP so duplicate single-letter shortcuts are detected and handled safely, and add a unit test that iterates built-in shortcuts to assert no shorthand collisions.skills/lark-base/references/lark-base-record-delete.md (1)
39-42: Nit: add language to the fenced code block (MD040).markdownlint flags the HTTP snippet as missing a language identifier.
💅 Proposed fix
-``` +```http DELETE /open-apis/base/v3/bases/:base_token/tables/:table_id/records/:record_id POST /open-apis/base/v3/bases/:base_token/tables/:table_id/records/batch_delete</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@skills/lark-base/references/lark-base-record-delete.mdaround lines 39 - 42,
The fenced HTTP snippet containing the two endpoints "DELETE
/open-apis/base/v3/bases/:base_token/tables/:table_id/records/:record_id" and
"POST
/open-apis/base/v3/bases/:base_token/tables/:table_id/records/batch_delete" is
missing a language identifier; add "http" right after the opening backticks
(```http) so markdownlint MD040 is satisfied and the block is correctly
highlighted.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@shortcuts/base/record_ops.go:
- Around line 503-514: Tests are missing for the HTTP 404 fallback paths: when
baseV3Raw for "batch_get" or "batch_delete" returns a 404 (detected via
isHTTPNotFoundError), the code should call baseV3Call per-record (as in the
"GET" fallback using baseV3Call and fallbackBatchGetRecords) or iterate deletes
and produce the correct output shape; add unit/integration tests that stub
baseV3Raw to return a 404 for batch_get and batch_delete and assert that
baseV3Call is invoked for individual record requests, that
fallbackBatchGetRecords (and the delete equivalent) is exercised, and that the
runtime.Out/output shape matches expected single-item and iterative responses.
Ensure tests cover both the single-record shortcut (len(selection.recordIDs)==1
&& !selection.fromJSON) and the multi-record fallback paths referenced by
baseV3Raw, isHTTPNotFoundError, baseV3Call, and fallbackBatchGetRecords.- Around line 156-181: coerceBatchGetSingleRecordData currently builds the
single-record envelope but drops the selected record ID; update it to preserve
the ID by reading data["record_id_list"] (or the field the batch returns with
selected IDs), extract the single ID when rows length == 1, and include it in
the returned result map (e.g. set result["record_id_list"] =
[]interface{}{selectedID} or result["record_id"]=selectedID) alongside the
"record" and optional "ignored_fields"; also update the corresponding get test
to assert that the returned envelope contains the record ID (matching the
original selected ID) so single-record callers retain the identifier.In
@skills/lark-base/references/lark-base-record-get.md:
- Around line 32-34: Update the flag descriptions to make it explicit that one
selector is required: change the-r, --record-id <id>text to indicate it is
mutually exclusive with--json(e.g., “与 --json 二选一;记录 ID,可重复使用;这是主推荐用法”) and
change the--json <object>text to indicate it is mutually exclusive with
--record-id(e.g., “与 --record-id 二选一;脚本/代理场景可传
{'record_id_list':['rec_xxx']};也可附带 'select_fields'”), and ensure the help text
for-f, --field-id <id_or_name>remains unchanged; reference the exact flag
names (-r, --record-id,--json,-f, --field-id) so reviewers can locate
and update the CLI docs accordingly.
Nitpick comments:
In@shortcuts/common/runner.go:
- Around line 856-890: registerShortcutFlagsWithContext currently calls the
Cobra *P flag helpers directly with fl.Shorthand which will panic on duplicate
shorthands (or collision with the reserved -q/JQ flag); add a lightweight
collision safeguard by tracking used shorthands per command before registering
each flag (use fl.Shorthand and the target cmd.Flags()), and if a shorthand is
already present or equals the reserved 'q', avoid calling the *P variant and
either return an error or fall back to the non-shorthand variant and surface a
clear error/log; update registerShortcutFlagsWithContext to consult this
shorthand set before calling BoolP/IntP/StringP/StringArrayP/StringSliceP so
duplicate single-letter shortcuts are detected and handled safely, and add a
unit test that iterates built-in shortcuts to assert no shorthand collisions.In
@skills/lark-base/references/lark-base-record-delete.md:
- Around line 39-42: The fenced HTTP snippet containing the two endpoints
"DELETE
/open-apis/base/v3/bases/:base_token/tables/:table_id/records/:record_id" and
"POST
/open-apis/base/v3/bases/:base_token/tables/:table_id/records/batch_delete" is
missing a language identifier; add "http" right after the opening backticks
(```http) so markdownlint MD040 is satisfied and the block is correctly
highlighted.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `84b82ecb-df97-44a9-83ed-385685dd0a03` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 81d22c6f34afd7c73122d169a745cc9e3bdf6e54 and b7d446a794c0ddab5e2fffafe62c40ae04c178ca. </details> <details> <summary>📒 Files selected for processing (13)</summary> * `shortcuts/base/base_dryrun_ops_test.go` * `shortcuts/base/base_execute_test.go` * `shortcuts/base/base_shortcuts_test.go` * `shortcuts/base/record_delete.go` * `shortcuts/base/record_get.go` * `shortcuts/base/record_ops.go` * `shortcuts/common/runner.go` * `shortcuts/common/runner_flag_completion_test.go` * `shortcuts/common/types.go` * `skills/lark-base/SKILL.md` * `skills/lark-base/references/lark-base-record-delete.md` * `skills/lark-base/references/lark-base-record-get.md` * `skills/lark-base/references/lark-base-record.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| result, err := baseV3Raw(runtime, "POST", baseV3Path("bases", runtime.Str("base-token"), "tables", baseTableID(runtime), "records", "batch_get"), nil, recordGetBatchBody(selection)) | ||
| if isHTTPNotFoundError(err) { | ||
| if len(selection.recordIDs) == 1 && !selection.fromJSON { | ||
| data, callErr := baseV3Call(runtime, "GET", baseV3Path("bases", runtime.Str("base-token"), "tables", baseTableID(runtime), "records", selection.recordIDs[0]), recordGetQueryParams(selection.selectFields), nil) | ||
| if callErr != nil { | ||
| return callErr | ||
| } | ||
| runtime.Out(data, nil) | ||
| return nil | ||
| } | ||
| return fallbackBatchGetRecords(runtime, selection.recordIDs, selection.selectFields) | ||
| } |
There was a problem hiding this comment.
Add direct tests for the HTTP 404 fallback paths.
The new compatibility behavior is important, but the provided execute tests only cover successful batch routes. Add tests where batch_get/batch_delete returns HTTP 404 and verify the single/iterative fallback requests and output shape. As per coding guidelines, “Every behavior change must have an accompanying test”.
Also applies to: 613-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/base/record_ops.go` around lines 503 - 514, Tests are missing for
the HTTP 404 fallback paths: when baseV3Raw for "batch_get" or "batch_delete"
returns a 404 (detected via isHTTPNotFoundError), the code should call
baseV3Call per-record (as in the "GET" fallback using baseV3Call and
fallbackBatchGetRecords) or iterate deletes and produce the correct output
shape; add unit/integration tests that stub baseV3Raw to return a 404 for
batch_get and batch_delete and assert that baseV3Call is invoked for individual
record requests, that fallbackBatchGetRecords (and the delete equivalent) is
exercised, and that the runtime.Out/output shape matches expected single-item
and iterative responses. Ensure tests cover both the single-record shortcut
(len(selection.recordIDs)==1 && !selection.fromJSON) and the multi-record
fallback paths referenced by baseV3Raw, isHTTPNotFoundError, baseV3Call, and
fallbackBatchGetRecords.
b7d446a to
d2dd8ed
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f37c8fa33dceb50f69c1b82ef62876eb3858cdfc🧩 Skill updatenpx skills add zgz2048/cli#codex/feat-batch-record-get-and-delete -y -g |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/base/base_execute_test.go (1)
1281-1300: Consider asserting the captured request body for the json-selector batch delete.The sibling
delete batch with repeated record-id flagssubtest (lines 1275–1278) verifies that the POST body contains"record_id_list":["rec_2","rec_1"], but this json-selector subtest only checks stdout. Adding a body assertion would make the test more precise in guarding the--json→record_id_listforwarding path (whereresolveRecordSelectionmust extract and re-emitrecord_id_list).🛠 Suggested tightening
if got := stdout.String(); !strings.Contains(got, `"record_id_list"`) || !strings.Contains(got, `"rec_3"`) { t.Fatalf("stdout=%s", got) } + body := string(batchStub.CapturedBody) + if !strings.Contains(body, `"record_id_list":["rec_3"]`) { + t.Fatalf("request body=%s", body) + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/base_execute_test.go` around lines 1281 - 1300, The test "delete batch with json selector" currently only asserts stdout; update it to also assert the HTTP request body sent to the batch delete endpoint contains the forwarded record_id_list from the JSON selector. Locate the subtest using newExecuteFactory, reg.Register(batchStub) and runShortcut(BaseRecordDelete, ... `"--json", '{"record_id_list":["rec_3"]}'`), capture the request received by the batchStub (the httpmock.Stub used for POST to /open-apis/base/v3/bases/app_x/tables/tbl_x/records/batch_delete) and add an assertion that the parsed request body includes "record_id_list":["rec_3"] (mirroring the sibling "delete batch with repeated record-id flags" test's body check) to ensure resolveRecordSelection forwards the list correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/base/record_get.go`:
- Around line 19-28: The flags advertised with shorthands (-r and -f) are not
wired up because common.Flag lacks a Shorthand field and the Cobra registration
in runner.go uses StringArray (no-P) calls; add a Shorthand string field to the
common.Flag struct (in shortcuts/common/types.go), set the shorthand values for
the "record-id" and "field-id" Flags in shortcuts/base/record_get.go (e.g. "r"
and "f"), and update the Cobra flag registration logic in
shortcuts/common/runner.go to use the P variants (StringArrayP) and pass
fl.Shorthand when present so the shorthands are actually registered;
alternatively, if you prefer not to add shorthands, remove the shorthand claims
from the flag definitions and docs so they match the current registration.
---
Nitpick comments:
In `@shortcuts/base/base_execute_test.go`:
- Around line 1281-1300: The test "delete batch with json selector" currently
only asserts stdout; update it to also assert the HTTP request body sent to the
batch delete endpoint contains the forwarded record_id_list from the JSON
selector. Locate the subtest using newExecuteFactory, reg.Register(batchStub)
and runShortcut(BaseRecordDelete, ... `"--json",
'{"record_id_list":["rec_3"]}'`), capture the request received by the batchStub
(the httpmock.Stub used for POST to
/open-apis/base/v3/bases/app_x/tables/tbl_x/records/batch_delete) and add an
assertion that the parsed request body includes "record_id_list":["rec_3"]
(mirroring the sibling "delete batch with repeated record-id flags" test's body
check) to ensure resolveRecordSelection forwards the list correctly.
🪄 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: 581c6104-2bba-4813-a116-283af33e9508
📒 Files selected for processing (11)
shortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/record_delete.goshortcuts/base/record_get.goshortcuts/base/record_ops.goshortcuts/common/types.goskills/lark-base/SKILL.mdskills/lark-base/references/lark-base-record-delete.mdskills/lark-base/references/lark-base-record-get.mdskills/lark-base/references/lark-base-record.md
✅ Files skipped from review due to trivial changes (1)
- shortcuts/common/types.go
🚧 Files skipped from review as they are similar to previous changes (5)
- shortcuts/base/record_delete.go
- skills/lark-base/references/lark-base-record-get.md
- skills/lark-base/references/lark-base-record.md
- shortcuts/base/record_ops.go
- shortcuts/base/base_dryrun_ops_test.go
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 `@skills/lark-base/SKILL.md`:
- Line 109: Update the `+record-delete` row in SKILL.md to document both input
forms: keep the existing CLI-style repeated flag form `--record-id` (allowing
multiple entries) and add the JSON multi-delete form using the `record_id_list`
field (e.g., an array of IDs) so routing matches the implemented shortcut
behavior; reference `+record-delete`, `--record-id`, and `record_id_list` in the
description and note that `--yes` can be used when the user has explicitly
confirmed the action.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…cord-get-and-delete # Conflicts: # skills/lark-base/SKILL.md # skills/lark-base/references/lark-base-record.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
skills/lark-base/SKILL.md (1)
109-109:⚠️ Potential issue | 🟡 MinorDocument JSON
record_id_listfor multi-delete.Line 109 still only mentions repeated
--record-id, but+record-deletealso supports--json {"record_id_list":[...]}. Include both input forms so the Skill routing guidance matches the shortcut behavior.Suggested doc tweak
-| `+record-delete` | 删除一条或多条记录 | [`lark-base-record-delete.md`](references/lark-base-record-delete.md) | 删除多条时重复传 `--record-id` 指定多个记录;用户已明确目标可直接执行并带 `--yes` | +| `+record-delete` | 删除一条或多条记录 | [`lark-base-record-delete.md`](references/lark-base-record-delete.md) | 删除多条可重复传 `--record-id`,或用 `--json {"record_id_list":[...]}`;用户已明确目标可直接执行并带 `--yes` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-base/SKILL.md` at line 109, Update the documentation for the `+record-delete` shortcut to document both ways to specify multiple records: repeated `--record-id` flags and the JSON payload form `--json {"record_id_list":[...]}`; mention that either form can be used for multi-delete and that `--yes` still bypasses confirmation when the user has explicitly indicated intent. Reference the `+record-delete` shortcut and the `--record-id` and `--json {"record_id_list":[...]}` input forms so routing guidance matches the actual shortcut behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@skills/lark-base/SKILL.md`:
- Line 109: Update the documentation for the `+record-delete` shortcut to
document both ways to specify multiple records: repeated `--record-id` flags and
the JSON payload form `--json {"record_id_list":[...]}`; mention that either
form can be used for multi-delete and that `--yes` still bypasses confirmation
when the user has explicitly indicated intent. Reference the `+record-delete`
shortcut and the `--record-id` and `--json {"record_id_list":[...]}` input forms
so routing guidance matches the actual shortcut behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd651ed4-ea86-4a56-b53c-a6e28dd3872f
📒 Files selected for processing (7)
shortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/helpers_test.goskills/lark-base/SKILL.mdskills/lark-base/references/lark-base-record-delete.mdskills/lark-base/references/lark-base-record-get.mdskills/lark-base/references/lark-base-record.md
✅ Files skipped from review due to trivial changes (1)
- shortcuts/base/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/lark-base/references/lark-base-record.md
- shortcuts/base/base_shortcuts_test.go
- skills/lark-base/references/lark-base-record-get.md
- skills/lark-base/references/lark-base-record-delete.md
Summary
This change upgrades
base +record-getandbase +record-deletefrom single-record shortcuts to multi-record operations, while keeping single-record usage backward compatible. It also adds field selection for+record-getso agents and users can reduce response size when reading records.Changes
+record-getto support repeatable--record-id, optional--json {"record_id_list": [...]}, and repeatable--field-idfor field selection.+record-deleteto support repeatable--record-idand optional--json {"record_id_list": [...]}.+record-getthroughbatch_getfor both single-record and multi-record reads, while preserving single-record output shape for single-record flag usage.+record-deleteon single DELETE for one record and switch tobatch_deletefor multiple records or JSON mode.batch_getorbatch_deletereturns HTTP 404 in environments where the batch routes are unavailable.--field-idsupport to+record-getand map it toselect_fields, so callers can reduce response size by returning only required fields.Test Plan
lark xxxcommand works as expectedManual verification:
make unit-testgo vet ./...gofmt -l .go mod tidy(nogo.mod/go.sumdiff)go test ./shortcuts/base ./shortcuts/commongo test ./shortcuts/...make buildpreenvironment:+record-getsingle record via--record-id+record-getmulti-record via repeated--record-id+record-getJSON mode via--json {"record_id_list": [...]}+record-getfield selection via--field-id+record-deletesingle record via--record-id+record-deletemulti-record via repeated--record-id+record-deleteJSON mode via--json {"record_id_list": [...]}not_foundRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
Documentation