fix(base): reject wrapped JSON payloads in +record-upsert#305
fix(base): reject wrapped JSON payloads in +record-upsert#305zfaustk wants to merge 3 commits intolarksuite:mainfrom
Conversation
Reject the common --json payload mistake where users pass a top-level fields wrapper to base +record-upsert. The CLI now surfaces a direct hint before the request reaches the Base API.
Update base record-upsert validation tests to accept direct record JSON, reject the top-level fields wrapper, and preserve invalid-JSON pass-through behavior.
Adjust base +record-upsert execution tests to use direct record JSON for create/update and add a regression case for the top-level fields wrapper misuse.
📝 WalkthroughWalkthroughThe changes fix JSON payload validation for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related issues
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)
Comment |
github-actions
bot
added
domain/base
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/base_execute_test.go`:
- Line 718: The test data contains a mojibake string for the field name where
the map entry with id "fld_status" uses corrupted bytes; replace the corrupted
value ("ç�¶æ��") with the correct UTF-8 Chinese string "状态" in the map literal,
and ensure the source file is saved/committed as UTF-8 to prevent recurrence;
scan for other occurrences of "ç�¶æ" in the test file to correct any additional
corrupted field names.
- Line 913: The test contains UTF-8 corrupted Chinese characters in the
options/keyword map literal (the map literal with "options":
[]interface{}{map[string]interface{}{"id": "opt_1", "name": "..."} } ) — replace
the corrupted sequences like "已" and "已��" with the correct Chinese
characters "已" and "已完成" respectively in that map (and the other identical
occurrences of the same option/keyword entries in the same test file) so the
option name and keyword strings use the original Chinese text.
🪄 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: 583d3c87-9f45-4dc5-b9c3-4062d1670d42
📒 Files selected for processing (3)
shortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/record_ops.go
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"id": "fld_status", "name": "状态", "type": "text"}, | ||
| "data": map[string]interface{}{"id": "fld_status", "name": "ç¶æ", "type": "text"}, |
There was a problem hiding this comment.
Same UTF-8 encoding corruption — 状态 became ç¶æ.
The field name 状态 (meaning "status") shows the same mojibake corruption pattern as the attachment field name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/base/base_execute_test.go` at line 718, The test data contains a
mojibake string for the field name where the map entry with id "fld_status" uses
corrupted bytes; replace the corrupted value ("��") with the correct UTF-8
Chinese string "状态" in the map literal, and ensure the source file is
saved/committed as UTF-8 to prevent recurrence; scan for other occurrences of
"ç�¶æ" in the test file to correct any additional corrupted field names.
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"options": []interface{}{map[string]interface{}{"id": "opt_1", "name": "已完成"}}, "total": 1}, | ||
| "data": map[string]interface{}{"options": []interface{}{map[string]interface{}{"id": "opt_1", "name": "å·²å®æ"}}, "total": 1}, |
There was a problem hiding this comment.
Same UTF-8 encoding corruption in search options test.
The keyword and option names have similar corruption:
已→å·²已完成→å·²å®æ
These should be restored to the original Chinese characters unless this is intentional test data for encoding handling.
Also applies to: 916-916, 919-919
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/base/base_execute_test.go` at line 913, The test contains UTF-8
corrupted Chinese characters in the options/keyword map literal (the map literal
with "options": []interface{}{map[string]interface{}{"id": "opt_1", "name":
"..."} } ) — replace the corrupted sequences like "å·²" and "å·²å®�æ��" with the
correct Chinese characters "已" and "已完成" respectively in that map (and the other
identical occurrences of the same option/keyword entries in the same test file)
so the option name and keyword strings use the original Chinese text.
Greptile SummaryThis PR adds early validation in
Confidence Score: 4/5Not safe to merge until encoding corruption in unrelated test strings is reverted. The validation logic in record_ops.go is correct and the new test coverage is solid, but the PR unintentionally corrupts Chinese-character test strings with Latin-1 mojibake (P1) that must be fixed before merging. shortcuts/base/base_execute_test.go — encoding corruption in unrelated test strings at lines 601–685 and 913–919. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI: +record-upsert --json value"] --> B["validateRecordJSON()"]
B --> C{"parseJSONObject()"}
C -->|"parse error"| D["return nil: defer to execution path"]
C -->|"ok"| E{"len(body) == 1?"}
E -->|"No"| F["return nil: pass through"]
E -->|"Yes"| G{"body[\"fields\"] is non-empty map?"}
G -->|"No"| F
G -->|"Yes"| H["FlagErrorf: must be direct record object"]
F --> I["executeRecordUpsert()"]
D --> I
I --> J{"record-id set?"}
J -->|"Yes"| K["PATCH /records/:id"]
J -->|"No"| L["POST /records"]
Reviews (1): Last reviewed commit: "test(base): reject wrapped upsert payloa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -598,7 +609,7 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) { | |||
| "data": map[string]interface{}{ | |||
| "record_id": "rec_x", | |||
| "fields": map[string]interface{}{ | |||
| "附件": []interface{}{ | |||
| "é件": []interface{}{ | |||
| map[string]interface{}{ | |||
| "file_token": "existing_tok", | |||
| "name": "existing.pdf", | |||
| @@ -629,7 +640,7 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) { | |||
| "data": map[string]interface{}{ | |||
| "record_id": "rec_x", | |||
| "fields": map[string]interface{}{ | |||
| "附件": []interface{}{ | |||
| "é件": []interface{}{ | |||
| map[string]interface{}{ | |||
| "file_token": "existing_tok", | |||
| "name": "existing.pdf", | |||
| @@ -671,7 +682,7 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) { | |||
| } | |||
|
|
|||
| updateBody := string(updateStub.CapturedBody) | |||
| if !strings.Contains(updateBody, `"附件"`) || | |||
| if !strings.Contains(updateBody, `"é件"`) || | |||
There was a problem hiding this comment.
The PR replaces valid UTF-8 Chinese characters with Latin-1 mojibake across multiple unrelated stubs and assertions: "附件" (attachment) → "é件" at lines 601, 612, 643, and 685; "状态" (status) → "ç¶æ" at line 718; "已完成" → "å·²å®æ" and keyword "已" → "å·²" at lines 913–919. This appears to be an editor encoding bug where the UTF-8 source was re-saved as Latin-1. The tests may still pass because the corruption is symmetric between stubs and assertions, but the data no longer represents realistic API responses and will mislead future maintainers. These lines were not part of the intended change — please revert to the original Chinese characters.
| return nil | ||
| } | ||
| if len(body) == 1 { | ||
| if fields, ok := body["fields"].(map[string]interface{}); ok && len(fields) > 0 { |
There was a problem hiding this comment.
{"fields":{}} wrapper silently bypasses the check
The len(fields) > 0 guard means a payload of {"fields": {}} is not rejected, even though it is still the wrong shape. Consider dropping the length check so the empty-wrapper case is also caught early:
| if fields, ok := body["fields"].(map[string]interface{}); ok && len(fields) > 0 { | |
| if _, ok := body["fields"].(map[string]interface{}); ok { |
1 participant
Summary
Fixes #272.
The current
base +record-upsert --jsonpath accepts a common wrong payload shape,{"fields": {...}}, and only fails later in the Base API layer. This change surfaces the contract earlier in the CLI by requiring a direct record object.Changes
fieldswrapper invalidateRecordJSON()and show a concrete example of the expected shapebase_shortcuts_test.goto accept direct record JSON and cover the wrapped-payload validation errorbase_execute_test.goto use the direct record shape for create/update and add a regression case for the wrapped payloadValidation
/tmp/larkcli-triage-VvPLOV/repogit diff --checkgois not installed in the containerSummary by CodeRabbit
Bug Fixes
Breaking Changes