diff --git a/internal/openapi/generate.go b/internal/openapi/generate.go index 7df83c3..07bcfc9 100644 --- a/internal/openapi/generate.go +++ b/internal/openapi/generate.go @@ -417,7 +417,8 @@ func schemaRequested(cmd *cobra.Command, name string) bool { // requestBodyMediaType returns the media type and definition the CLI will use, // preferring application/json for backward compatibility and otherwise using -// the first declared media type. +// the first declared media type. Entries with no schema are skipped, so a +// schema-less application/json stub never shadows a real multipart definition. func requestBodyMediaType(rb *v3.RequestBody) (string, *v3.MediaType) { if rb == nil || rb.Content == nil { return "", nil @@ -426,7 +427,7 @@ func requestBodyMediaType(rb *v3.RequestBody) (string, *v3.MediaType) { var first *v3.MediaType for pair := rb.Content.First(); pair != nil; pair = pair.Next() { mt := pair.Value() - if mt == nil { + if mt == nil || mt.Schema == nil { continue } if pair.Key() == "application/json" { diff --git a/internal/openapi/multipart.go b/internal/openapi/multipart.go index f44191c..5ed77ee 100644 --- a/internal/openapi/multipart.go +++ b/internal/openapi/multipart.go @@ -106,12 +106,21 @@ func registerMultipartFlags(cmd *cobra.Command, fields []multipartFieldInfo) { "body": true, "json-body": true, "schema": true, "field": true, "depth": true, "profile": true, "token": true, "base-url": true, "compact": true, "format": true, "help": true, } + taken := func(name string) bool { + return reserved[name] || cmd.Flags().Lookup(name) != nil + } for i := range fields { field := &fields[i] flagName := field.FlagName - if reserved[flagName] || cmd.Flags().Lookup(flagName) != nil { + if taken(flagName) { flagName = "form-" + flagName } + // Two fields can collide on the same prefixed name. Registering a + // duplicate makes pflag panic, which would take down the whole CLI at + // startup, so keep suffixing until the name is free. + for suffix := 2; taken(flagName); suffix++ { + flagName = fmt.Sprintf("form-%s-%d", field.FlagName, suffix) + } field.FlagName = flagName description := field.Description @@ -136,6 +145,11 @@ func buildMultipartBody(cmd *cobra.Command, rawBody []byte, bodyProvided bool, f if err := decoder.Decode(&values); err != nil { return nil, "", fmt.Errorf("invalid multipart --body JSON: %w", err) } + // JSON "null" decodes into a nil map, which the flag merge below would + // panic on. + if values == nil { + return nil, "", fmt.Errorf("multipart --body must be a JSON object of field values") + } } for _, field := range fields { @@ -213,18 +227,41 @@ func parseMultipartFlagValue(field multipartFieldInfo, value string) (interface{ case "number": return strconv.ParseFloat(value, 64) case "array", "object": - var parsed interface{} - decoder := json.NewDecoder(strings.NewReader(value)) - decoder.UseNumber() - if err := decoder.Decode(&parsed); err != nil { - return nil, fmt.Errorf("expected JSON %s: %w", field.Type, err) - } - return parsed, nil + return decodeJSONFlagValue(field.Type, value) default: return value, nil } } +// decodeJSONFlagValue parses a JSON flag value against the field's declared +// type. Decoding into interface{} would accept an object where the schema says +// array and would silently ignore anything after the first value, so the type +// is pinned and the input must end there. +func decodeJSONFlagValue(fieldType, value string) (interface{}, error) { + decoder := json.NewDecoder(strings.NewReader(value)) + decoder.UseNumber() + + var parsed interface{} + if fieldType == "array" { + var typed []interface{} + if err := decoder.Decode(&typed); err != nil { + return nil, fmt.Errorf("expected JSON array: %w", err) + } + parsed = typed + } else { + var typed map[string]interface{} + if err := decoder.Decode(&typed); err != nil { + return nil, fmt.Errorf("expected JSON object: %w", err) + } + parsed = typed + } + + if err := decoder.Decode(new(json.RawMessage)); err != io.EOF { + return nil, fmt.Errorf("expected a single JSON %s with no trailing data", fieldType) + } + return parsed, nil +} + func writeMultipartValue(writer *multipart.Writer, field multipartFieldInfo, value interface{}) error { if values, ok := value.([]interface{}); ok && field.Explode { for _, item := range values { @@ -243,6 +280,8 @@ func writeMultipartSingleValue(writer *multipart.Writer, field multipartFieldInf if !ok { return fmt.Errorf("multipart file field %q must be a file path", field.Name) } + // Match --body @path: shells don't expand "~" inside a flag value. + path = expandHome(path) file, err := os.Open(path) if err != nil { return fmt.Errorf("opening multipart file %q for field %q: %w", path, field.Name, err) diff --git a/internal/openapi/multipart_test.go b/internal/openapi/multipart_test.go index c3caf36..697251f 100644 --- a/internal/openapi/multipart_test.go +++ b/internal/openapi/multipart_test.go @@ -10,6 +10,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/spf13/cobra" ) const multipartTestSpec = `{ @@ -289,3 +291,139 @@ func TestGenerateCommands_MultipartBodyPathHint(t *testing.T) { t.Fatal("executor called for a path-shaped --body") } } + +// An explicitly empty --body is a body the caller asked for, so it gets the +// "omit the flag" error rather than reaching buildMultipartBody as a bare EOF. +func TestGenerateCommands_MultipartEmptyBodyFlag(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "people.csv") + if err := os.WriteFile(filePath, []byte("name\nAda\n"), 0o600); err != nil { + t.Fatal(err) + } + + called := false + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + called = true + return nil + }) + if err != nil { + t.Fatal(err) + } + command := commands[0].Commands()[0] + for flag, value := range map[string]string{"file": filePath, "model-id": "model-123", "body": ""} { + if err := command.Flags().Set(flag, value); err != nil { + t.Fatalf("set --%s: %v", flag, err) + } + } + + err = command.RunE(command, nil) + if err == nil || !strings.Contains(err.Error(), "--body is empty") { + t.Fatalf("error = %v, want the empty --body message", err) + } + if called { + t.Fatal("executor called for an empty --body") + } +} + +// Binary field values expand "~" the same way --body @path does; shells leave +// it alone inside a flag value. +func TestGenerateCommands_MultipartExpandsHomeInFilePath(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if err := os.WriteFile(filepath.Join(home, "people.csv"), []byte("name\nAda\n"), 0o600); err != nil { + t.Fatal(err) + } + + var captured APIRequest + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + captured = request + return nil + }) + if err != nil { + t.Fatal(err) + } + command := commands[0].Commands()[0] + for flag, value := range map[string]string{"file": "~/people.csv", "model-id": "model-123"} { + if err := command.Flags().Set(flag, value); err != nil { + t.Fatalf("set --%s: %v", flag, err) + } + } + if err := command.RunE(command, nil); err != nil { + t.Fatalf("RunE: %v", err) + } + + parts := parseCapturedMultipart(t, captured) + if got := parts["file"]; got.fileName != "people.csv" || len(got.values) != 1 || got.values[0] != "name\nAda\n" { + t.Errorf("file part = %#v", got) + } +} + +// JSON "null" decodes into a nil map, which the flag merge would panic on. +func TestGenerateCommands_MultipartRejectsNullBody(t *testing.T) { + called := false + commands, err := GenerateCommands([]byte(multipartTestSpec), func(request APIRequest) error { + called = true + return nil + }) + if err != nil { + t.Fatal(err) + } + command := commands[0].Commands()[0] + for flag, value := range map[string]string{"body": "null", "model-id": "model-123"} { + if err := command.Flags().Set(flag, value); err != nil { + t.Fatalf("set --%s: %v", flag, err) + } + } + + err = command.RunE(command, nil) + if err == nil || !strings.Contains(err.Error(), "JSON object of field values") { + t.Fatalf("error = %v, want a non-object --body error", err) + } + if called { + t.Fatal("executor called for a null --body") + } +} + +func TestParseMultipartFlagValue_ChecksTypeAndTrailingData(t *testing.T) { + array := multipartFieldInfo{Name: "labels", Type: "array"} + object := multipartFieldInfo{Name: "meta", Type: "object"} + + if _, err := parseMultipartFlagValue(array, `{"x":1}`); err == nil { + t.Error("an object passed for an array field was accepted") + } + if _, err := parseMultipartFlagValue(object, `["a"]`); err == nil { + t.Error("an array passed for an object field was accepted") + } + if _, err := parseMultipartFlagValue(array, `["a"] trailing`); err == nil { + t.Error("trailing data after an array was accepted") + } + if _, err := parseMultipartFlagValue(array, `["a","b"]`); err != nil { + t.Errorf("valid array rejected: %v", err) + } +} + +// Two fields whose flag names collide must not register the same pflag twice — +// pflag panics on a redefinition, taking down the whole CLI at startup. +func TestRegisterMultipartFlags_ResolvesCollisions(t *testing.T) { + command := &cobra.Command{Use: "upload"} + fields := []multipartFieldInfo{ + {Name: "body", FlagName: "body"}, + {Name: "Body", FlagName: "body"}, + {Name: "body_", FlagName: "body"}, + } + + registerMultipartFlags(command, fields) + + seen := map[string]bool{} + for _, field := range fields { + if field.FlagName == "body" { + t.Errorf("field %q kept the reserved --body name", field.Name) + } + if seen[field.FlagName] { + t.Errorf("duplicate flag name %q", field.FlagName) + } + seen[field.FlagName] = true + if command.Flags().Lookup(field.FlagName) == nil { + t.Errorf("flag --%s was not registered", field.FlagName) + } + } +}