mirror of
https://github.com/larksuite/cli.git
synced 2026-07-08 18:13:01 +08:00
fix(sheets): harden batch type-checking and +workbook-create edge cases
From the branch code-review doc (3 findings): - +batch-update sub-ops: `operations` is skipped by parse-time schema validation and mapFlagView coerces a type-mismatched scalar to its zero value, so "index":"abc" or "multiple":"true" silently became 0 / false and wrote to the wrong place. translateBatchOp now runs validateRawTypes, which checks each sub-op scalar against its flag-defs type and rejects mismatches. - +workbook-create with empty arrays: buildInitialFillInput returned (nil,nil) for empty rows while the caller wrote fill["excel_id"] unconditionally, so --values '[]' panicked on a nil map and --headers '[]' produced an illegal "A1:1" range. It now also returns nil when no cells survive (maxCols==0 guard) and Execute/DryRun skip the fill when fill==nil. - +workbook-create partial failure: after the spreadsheet was created, a first-sheet lookup or fill failure returned a bare fmt.Errorf, losing the new token. It now returns a structured partial_success error carrying spreadsheet_token in the detail so callers can retry or clean up. Tests added for each path; sheets suite green.
This commit is contained in:
@@ -467,6 +467,57 @@ func TestBatchOp_ErrorEquivalence(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestBatchOp_RejectsWrongScalarType locks the type-check that closes the
|
||||
// silent-coercion gap: `operations` skips parse-time schema validation, and
|
||||
// mapFlagView coerces a mismatched scalar to its zero value, so a sub-op field
|
||||
// whose JSON type contradicts its flag-defs type must be rejected up front
|
||||
// rather than landing as 0 / false in the wrong place.
|
||||
func TestBatchOp_RejectsWrongScalarType(t *testing.T) {
|
||||
t.Parallel()
|
||||
cases := []struct {
|
||||
name string
|
||||
subShortcut string
|
||||
subInput string
|
||||
wantContains string
|
||||
}{
|
||||
{
|
||||
name: "int flag given a string",
|
||||
subShortcut: "+sheet-move",
|
||||
subInput: `{"sheet-id":"sh1","source-index":2,"index":"abc"}`,
|
||||
wantContains: "--index must be a number",
|
||||
},
|
||||
{
|
||||
name: "int flag given a boolean",
|
||||
subShortcut: "+sheet-move",
|
||||
subInput: `{"sheet-id":"sh1","source-index":true,"index":0}`,
|
||||
wantContains: "--source-index must be a number",
|
||||
},
|
||||
{
|
||||
name: "bool flag given a string",
|
||||
subShortcut: "+cells-set",
|
||||
subInput: `{"sheet-id":"sh1","range":"A1","cells":[[{"value":1}]],"allow-overwrite":"true"}`,
|
||||
wantContains: "--allow-overwrite must be a boolean",
|
||||
},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
var subInput map[string]interface{}
|
||||
if err := json.Unmarshal([]byte(tc.subInput), &subInput); err != nil {
|
||||
t.Fatalf("bad subInput JSON: %v", err)
|
||||
}
|
||||
rawOp := map[string]interface{}{"shortcut": tc.subShortcut, "input": subInput}
|
||||
_, err := translateBatchOp(rawOp, testToken, 0)
|
||||
if err == nil {
|
||||
t.Fatalf("translateBatchOp accepted wrong-typed field; want error containing %q", tc.wantContains)
|
||||
}
|
||||
if !strings.Contains(err.Error(), tc.wantContains) {
|
||||
t.Errorf("error = %q, want substring %q", err.Error(), tc.wantContains)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestBatchOp_GuardsBeyondCobra locks the two batch sub-ops whose standalone
|
||||
// required-flag enforcement lives OUTSIDE the shared *Input builder — so it is
|
||||
// invisible to TestBatchOp_ErrorEquivalence and was missed by the refactor:
|
||||
|
||||
@@ -306,6 +306,12 @@ func translateBatchOp(raw interface{}, token string, index int) (map[string]inte
|
||||
}
|
||||
}
|
||||
fv := newMapFlagViewForCommand(sc, input)
|
||||
// operations is skipped by parse-time schema validation, so type-check the
|
||||
// sub-op's scalar fields here before the translator reads them via
|
||||
// Int/Bool/Float64 (which would otherwise coerce a wrong type to zero).
|
||||
if err := fv.validateRawTypes(); err != nil {
|
||||
return nil, common.FlagErrorf("operations[%d] (%s): %v", index, sc, err)
|
||||
}
|
||||
sheetIDFlag, sheetNameFlag := sheetSelectorFlagsForSubOp(sc)
|
||||
sheetID := strings.TrimSpace(fv.Str(sheetIDFlag))
|
||||
sheetName := strings.TrimSpace(fv.Str(sheetNameFlag))
|
||||
|
||||
@@ -9,6 +9,7 @@ import (
|
||||
"testing"
|
||||
|
||||
"github.com/larksuite/cli/internal/httpmock"
|
||||
"github.com/larksuite/cli/internal/output"
|
||||
)
|
||||
|
||||
// TestExecute_WorkbookInfo_Happy stubs the invoke_read endpoint and
|
||||
@@ -362,6 +363,82 @@ func TestExecute_WorkbookCreate(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestExecute_WorkbookCreate_EmptyArraysSkipFill locks the fix for the nil-map
|
||||
// panic / illegal-range bug: --values '[]' or --headers '[]' must short-circuit
|
||||
// the initial fill (no structure/fill calls fire) and finish with the
|
||||
// spreadsheet created but no initial_fill — never panic on a nil fill map.
|
||||
func TestExecute_WorkbookCreate_EmptyArraysSkipFill(t *testing.T) {
|
||||
t.Parallel()
|
||||
for _, tc := range []struct{ name, flag, val string }{
|
||||
{"empty values", "--values", "[]"},
|
||||
{"empty headers", "--headers", "[]"},
|
||||
} {
|
||||
tc := tc
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
create := &httpmock.Stub{
|
||||
Method: "POST",
|
||||
URL: "/open-apis/sheets/v3/spreadsheets",
|
||||
Body: map[string]interface{}{
|
||||
"code": 0, "msg": "success",
|
||||
"data": map[string]interface{}{
|
||||
"spreadsheet": map[string]interface{}{"spreadsheet_token": "shtNEW", "title": "X"},
|
||||
},
|
||||
},
|
||||
}
|
||||
// Only the create stub is provided: an empty array must skip the fill
|
||||
// entirely, so no structure/fill call fires (and no nil-map panic).
|
||||
out, err := runShortcutWithStubs(t, WorkbookCreate, []string{"--title", "X", tc.flag, tc.val}, create)
|
||||
if err != nil {
|
||||
t.Fatalf("execute failed: %v\nout=%s", err, out)
|
||||
}
|
||||
data := decodeEnvelopeData(t, out)
|
||||
if data["initial_fill"] != nil {
|
||||
t.Errorf("initial_fill should be absent for %s %s; got %#v", tc.flag, tc.val, data["initial_fill"])
|
||||
}
|
||||
if ss, _ := data["spreadsheet"].(map[string]interface{}); ss["spreadsheet_token"] != "shtNEW" {
|
||||
t.Errorf("spreadsheet_token = %v, want shtNEW", ss["spreadsheet_token"])
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestExecute_WorkbookCreate_FillFailureKeepsToken locks the partial-success
|
||||
// contract: when the spreadsheet is created but the follow-up fill can't resolve
|
||||
// its first sheet, the error must be structured and retain spreadsheet_token so
|
||||
// the caller can recover instead of orphaning the new workbook.
|
||||
func TestExecute_WorkbookCreate_FillFailureKeepsToken(t *testing.T) {
|
||||
t.Parallel()
|
||||
create := &httpmock.Stub{
|
||||
Method: "POST",
|
||||
URL: "/open-apis/sheets/v3/spreadsheets",
|
||||
Body: map[string]interface{}{
|
||||
"code": 0, "msg": "success",
|
||||
"data": map[string]interface{}{
|
||||
"spreadsheet": map[string]interface{}{"spreadsheet_token": "shtNEW", "title": "X"},
|
||||
},
|
||||
},
|
||||
}
|
||||
// Structure comes back with no sheets, so lookupFirstSheetID fails AFTER the
|
||||
// spreadsheet already exists — exercising the partial-success path.
|
||||
structure := toolOutputStub("shtNEW", "read", `{"sheets":[]}`)
|
||||
out, err := runShortcutWithStubs(t, WorkbookCreate, []string{"--title", "X", "--values", `[["a"]]`}, create, structure)
|
||||
if err == nil {
|
||||
t.Fatalf("expected a partial-success error; got nil\nout=%s", out)
|
||||
}
|
||||
exitErr, ok := err.(*output.ExitError)
|
||||
if !ok {
|
||||
t.Fatalf("error type = %T, want *output.ExitError (structured)", err)
|
||||
}
|
||||
if exitErr.Detail == nil {
|
||||
t.Fatal("ExitError.Detail is nil; want structured detail carrying the token")
|
||||
}
|
||||
detail, _ := exitErr.Detail.Detail.(map[string]interface{})
|
||||
if detail["spreadsheet_token"] != "shtNEW" {
|
||||
t.Errorf("detail.spreadsheet_token = %v, want shtNEW (must survive the fill failure)", detail["spreadsheet_token"])
|
||||
}
|
||||
}
|
||||
|
||||
// TestExecute_DimMove covers the native v3 move_dimension call. CLI's
|
||||
// --source-range "1:3" (1-based inclusive) is parsed into v3's
|
||||
// source.{start_index=0,end_index=2} (0-based inclusive); --target "11" is
|
||||
|
||||
@@ -254,3 +254,76 @@ func (m mapFlagView) Changed(name string) bool {
|
||||
_, ok := m.lookupRaw(name)
|
||||
return ok
|
||||
}
|
||||
|
||||
// validateRawTypes rejects sub-op input fields whose JSON type contradicts the
|
||||
// flag's declared type in flag-defs. +batch-update skips parse-time schema
|
||||
// validation for `operations`, and Int/Int64/Float64/Bool silently fall back to
|
||||
// the zero value on a type mismatch — so without this guard a wrong-typed scalar
|
||||
// (e.g. "index":"abc" or "multiple":"true") would land as 0 / false instead of
|
||||
// erroring, writing to the wrong place. Only numeric and boolean flags are
|
||||
// checked; string and composite (array/object) flags stay permissive because
|
||||
// Str() intentionally coerces them and the translator/schema validates shape.
|
||||
//
|
||||
// Returns a bare error; the +batch-update translator wraps it with the
|
||||
// operations[i] (<shortcut>) context.
|
||||
func (m mapFlagView) validateRawTypes() error {
|
||||
if len(m.raw) == 0 {
|
||||
return nil
|
||||
}
|
||||
defs, err := loadFlagDefs()
|
||||
if err != nil {
|
||||
return nil
|
||||
}
|
||||
spec, ok := defs[m.command]
|
||||
if !ok {
|
||||
return nil
|
||||
}
|
||||
declaredType := make(map[string]string, len(spec.Flags))
|
||||
for _, df := range spec.Flags {
|
||||
declaredType[df.Name] = df.Type
|
||||
}
|
||||
for rawKey, val := range m.raw {
|
||||
name := rawKey
|
||||
typ, ok := declaredType[name]
|
||||
if !ok {
|
||||
// flag-defs use hyphen names; tolerate the underscore form users send.
|
||||
name = strings.ReplaceAll(rawKey, "_", "-")
|
||||
typ, ok = declaredType[name]
|
||||
}
|
||||
if !ok {
|
||||
continue // unknown key — leave it for the translator / schema layer
|
||||
}
|
||||
switch typ {
|
||||
case "int", "int64", "float64":
|
||||
if _, isNum := val.(float64); !isNum {
|
||||
return fmt.Errorf("--%s must be a number, got %s", name, jsonTypeName(val))
|
||||
}
|
||||
case "bool":
|
||||
if _, isBool := val.(bool); !isBool {
|
||||
return fmt.Errorf("--%s must be a boolean, got %s", name, jsonTypeName(val))
|
||||
}
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// jsonTypeName names the JSON kind of a value decoded by encoding/json, for
|
||||
// type-mismatch error messages.
|
||||
func jsonTypeName(v interface{}) string {
|
||||
switch v.(type) {
|
||||
case nil:
|
||||
return "null"
|
||||
case bool:
|
||||
return "boolean"
|
||||
case float64:
|
||||
return "number"
|
||||
case string:
|
||||
return "string"
|
||||
case []interface{}:
|
||||
return "array"
|
||||
case map[string]interface{}:
|
||||
return "object"
|
||||
default:
|
||||
return fmt.Sprintf("%T", v)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -598,8 +598,7 @@ var WorkbookCreate = common.Shortcut{
|
||||
POST("/open-apis/sheets/v3/spreadsheets").
|
||||
Desc("create spreadsheet").
|
||||
Body(body)
|
||||
if runtime.Str("headers") != "" || runtime.Str("values") != "" {
|
||||
fill, _ := buildInitialFillInput(runtime)
|
||||
if fill, _ := buildInitialFillInput(runtime); fill != nil {
|
||||
fill["excel_id"] = "<new-token>"
|
||||
fill["sheet_id"] = "<first-sheet-id>" // resolved from the workbook at execute time
|
||||
wireBody, _ := buildToolBody("set_cell_range", fill)
|
||||
@@ -629,24 +628,27 @@ var WorkbookCreate = common.Shortcut{
|
||||
|
||||
result := map[string]interface{}{"spreadsheet": ss}
|
||||
|
||||
if runtime.Str("headers") != "" || runtime.Str("values") != "" {
|
||||
fill, err := buildInitialFillInput(runtime)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
// --headers / --values are optional. buildInitialFillInput returns
|
||||
// (nil, nil) when both are absent or empty, in which case we skip the
|
||||
// fill entirely rather than dereferencing a nil map.
|
||||
fill, err := buildInitialFillInput(runtime)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if fill != nil {
|
||||
fill["excel_id"] = token
|
||||
// set_cell_range needs a concrete sheet selector; the create
|
||||
// response doesn't echo the default sheet's id, so read it back.
|
||||
firstSheetID, err := lookupFirstSheetID(ctx, runtime, token)
|
||||
if err != nil {
|
||||
return fmt.Errorf("spreadsheet %s created but resolving its first sheet for initial fill failed: %w", token, err)
|
||||
return workbookCreatedButFillFailed(token, ss,
|
||||
fmt.Sprintf("resolving its first sheet for initial fill failed: %v", err))
|
||||
}
|
||||
fill["sheet_id"] = firstSheetID
|
||||
fillOut, err := callTool(ctx, runtime, token, ToolKindWrite, "set_cell_range", fill)
|
||||
if err != nil {
|
||||
// Spreadsheet exists; surface the fill failure but keep the new
|
||||
// token in the envelope so the caller can recover or retry.
|
||||
return fmt.Errorf("spreadsheet %s created but initial fill failed: %w", token, err)
|
||||
return workbookCreatedButFillFailed(token, ss,
|
||||
fmt.Sprintf("initial fill failed: %v", err))
|
||||
}
|
||||
result["initial_fill"] = fillOut
|
||||
}
|
||||
@@ -658,6 +660,26 @@ var WorkbookCreate = common.Shortcut{
|
||||
},
|
||||
}
|
||||
|
||||
// workbookCreatedButFillFailed builds a structured partial-success error for the
|
||||
// window where the spreadsheet POST succeeded but the follow-up initial fill did
|
||||
// not. The new spreadsheet_token is surfaced in the error detail so callers can
|
||||
// retry the fill (+cells-set / +csv-put) or delete the orphan, instead of only
|
||||
// finding the token interpolated into a bare error string.
|
||||
func workbookCreatedButFillFailed(token string, spreadsheet interface{}, reason string) error {
|
||||
return &output.ExitError{
|
||||
Code: output.ExitAPI,
|
||||
Detail: &output.ErrDetail{
|
||||
Type: "partial_success",
|
||||
Message: fmt.Sprintf("spreadsheet %s created but %s", token, reason),
|
||||
Hint: "the spreadsheet exists; retry the fill with the returned spreadsheet_token, or delete it",
|
||||
Detail: map[string]interface{}{
|
||||
"spreadsheet_token": token,
|
||||
"spreadsheet": spreadsheet,
|
||||
},
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
// buildInitialFillInput zips --headers + --values into a single set_cell_range
|
||||
// payload writing to the first sheet starting at A1.
|
||||
func buildInitialFillInput(runtime *common.RuntimeContext) (map[string]interface{}, error) {
|
||||
@@ -692,6 +714,13 @@ func buildInitialFillInput(runtime *common.RuntimeContext) (map[string]interface
|
||||
maxCols = len(r)
|
||||
}
|
||||
}
|
||||
if maxCols == 0 {
|
||||
// --headers '[]' / --values '[]' parse to rows that carry no cells.
|
||||
// There is nothing to write and a 0-width range ("A1:1") would be
|
||||
// illegal, so treat it as "no initial fill" — same contract as the
|
||||
// len(rows)==0 case above — and let the caller skip the write.
|
||||
return nil, nil
|
||||
}
|
||||
// Normalize rows to the same length so cells matrix is rectangular.
|
||||
for i := range rows {
|
||||
for len(rows[i]) < maxCols {
|
||||
|
||||
Reference in New Issue
Block a user