diff --git a/internal/apiform/encoder.go b/internal/apiform/encoder.go index fc963b2..adb5e78 100644 --- a/internal/apiform/encoder.go +++ b/internal/apiform/encoder.go @@ -49,6 +49,19 @@ func (e *encoder) encodeValue(key string, val reflect.Value, writer *multipart.W return writer.WriteField(key, "") } + // Unwrap interfaces before detecting io.Reader. A non-nil interface can + // contain a typed nil pointer, which must retain the encoder's empty-field + // semantics instead of being passed to io.Copy. + for val.Kind() == reflect.Interface { + if val.IsNil() { + return writer.WriteField(key, "") + } + val = val.Elem() + } + if val.Kind() == reflect.Pointer && val.IsNil() { + return writer.WriteField(key, "") + } + t := val.Type() if t.Implements(reflect.TypeOf((*io.Reader)(nil)).Elem()) { @@ -57,9 +70,6 @@ func (e *encoder) encodeValue(key string, val reflect.Value, writer *multipart.W switch t.Kind() { case reflect.Pointer: - if val.IsNil() || !val.IsValid() { - return writer.WriteField(key, "") - } return e.encodeValue(key, val.Elem(), writer) case reflect.Slice, reflect.Array: @@ -68,12 +78,6 @@ func (e *encoder) encodeValue(key string, val reflect.Value, writer *multipart.W case reflect.Map: return e.encodeMap(key, val, writer) - case reflect.Interface: - if val.IsNil() { - return writer.WriteField(key, "") - } - return e.encodeValue(key, val.Elem(), writer) - case reflect.String: return writer.WriteField(key, val.String()) diff --git a/internal/apiform/typed_nil_reader_test.go b/internal/apiform/typed_nil_reader_test.go new file mode 100644 index 0000000..df243ad --- /dev/null +++ b/internal/apiform/typed_nil_reader_test.go @@ -0,0 +1,48 @@ +package apiform + +import ( + "bytes" + "io" + "mime/multipart" + "testing" + + "github.com/stretchr/testify/require" +) + +type panicOnRead struct{} + +func (*panicOnRead) Read([]byte) (int, error) { + panic("Read called on typed nil receiver") +} + +func TestMarshalTreatsTypedNilReaderAsEmptyField(t *testing.T) { + t.Parallel() + + var concrete *panicOnRead + var reader io.Reader = concrete + tests := map[string]any{ + "concrete pointer in any map": map[string]any{"file": concrete}, + "pointer in reader map": map[string]io.Reader{"file": reader}, + "nil reader interface": map[string]io.Reader{"file": nil}, + } + + for name, value := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + writer := multipart.NewWriter(&buf) + require.NoError(t, writer.SetBoundary("xxx")) + + require.NotPanics(t, func() { + require.NoError(t, Marshal(value, writer)) + require.NoError(t, writer.Close()) + }) + + require.Equal(t, + "--xxx\r\nContent-Disposition: form-data; name=\"file\"\r\n\r\n\r\n--xxx--\r\n", + buf.String(), + ) + }) + } +} diff --git a/pkg/cmd/multipartbody.go b/pkg/cmd/multipartbody.go index a9983e6..72c1b2d 100644 --- a/pkg/cmd/multipartbody.go +++ b/pkg/cmd/multipartbody.go @@ -219,6 +219,14 @@ type multipartBodyInfo struct { knownLength bool } +func isTypedNilReader(value any) bool { + if _, ok := value.(io.Reader); !ok { + return false + } + reflected := reflect.ValueOf(value) + return reflected.Kind() == reflect.Pointer && reflected.IsNil() +} + func inspectMultipartBody(value any) multipartBodyInfo { switch value := value.(type) { case map[string]any: @@ -240,6 +248,9 @@ func inspectMultipartBody(value any) multipartBodyInfo { case fileUpload: return multipartBodyInfo{hasUpload: true, knownLength: value.hasKnownSize()} default: + if isTypedNilReader(value) { + return multipartBodyInfo{knownLength: true} + } _, isReader := value.(io.Reader) return multipartBodyInfo{hasUpload: isReader, knownLength: !isReader} } @@ -311,6 +322,9 @@ func transformFileUploads( } return result, nil default: + if isTypedNilReader(value) { + return value, nil + } if _, isReader := value.(io.Reader); isReader { return nil, errors.New("multipart body contains an unknown-size reader") } diff --git a/pkg/cmd/multipartbody_typed_nil_test.go b/pkg/cmd/multipartbody_typed_nil_test.go new file mode 100644 index 0000000..46df970 --- /dev/null +++ b/pkg/cmd/multipartbody_typed_nil_test.go @@ -0,0 +1,146 @@ +package cmd + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "github.com/openai/openai-cli/internal/apiform" + "github.com/openai/openai-go/v3" + "github.com/openai/openai-go/v3/option" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func typedNilMultipartReader() io.Reader { + var reader *strings.Reader + return reader +} + +func TestInspectMultipartBodyTreatsTypedNilReaderAsScalar(t *testing.T) { + info := inspectMultipartBody(map[string]any{"file": typedNilMultipartReader()}) + + require.False(t, info.hasUpload) + require.True(t, info.knownLength) +} + +func TestMultipartRequestOptionsRetryTypedNilReaderAsScalar(t *testing.T) { + var requestCount atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount.Add(1) + if !assert.NoError(t, r.ParseMultipartForm(1<<20)) { + http.Error(w, "invalid multipart form", http.StatusBadRequest) + return + } + assert.Equal(t, []string{""}, r.MultipartForm.Value["file"]) + assert.Empty(t, r.MultipartForm.File["file"]) + assert.Positive(t, r.ContentLength) + + w.Header().Set("Content-Type", "application/json") + if requestCount.Load() == 1 { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = io.WriteString(w, `{"error":{"message":"retry me","type":"rate_limit_error"}}`) + return + } + _, _ = io.WriteString(w, `{"id":"video_123","object":"video","status":"queued"}`) + })) + t.Cleanup(server.Close) + + options, err := multipartRequestOptions(map[string]any{ + "file": typedNilMultipartReader(), + "prompt": "hello", + }, apiform.FormatBrackets) + require.NoError(t, err) + client := openai.NewClient( + option.WithAPIKey("test-key"), + option.WithBaseURL(server.URL+"/"), + ) + + _, err = client.Videos.New(context.Background(), openai.VideoNewParams{}, options...) + require.NoError(t, err) + require.Equal(t, int32(2), requestCount.Load()) +} + +func TestMultipartRequestOptionsReplayTypedNilReaderAcrossRedirects(t *testing.T) { + for _, status := range []int{http.StatusTemporaryRedirect, http.StatusPermanentRedirect} { + t.Run(http.StatusText(status), func(t *testing.T) { + var requestCount atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount.Add(1) + if r.URL.Path != "/redirected" { + http.Redirect(w, r, "/redirected", status) + return + } + + if !assert.NoError(t, r.ParseMultipartForm(1<<20)) { + http.Error(w, "invalid multipart form", http.StatusBadRequest) + return + } + assert.Equal(t, []string{""}, r.MultipartForm.Value["file"]) + assert.Empty(t, r.MultipartForm.File["file"]) + assert.Positive(t, r.ContentLength) + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"id":"video_123","object":"video","status":"queued"}`) + })) + t.Cleanup(server.Close) + + options, err := multipartRequestOptions(map[string]any{ + "file": typedNilMultipartReader(), + "prompt": "hello", + }, apiform.FormatBrackets) + require.NoError(t, err) + client := openai.NewClient( + option.WithAPIKey("test-key"), + option.WithBaseURL(server.URL+"/"), + ) + + _, err = client.Videos.New(context.Background(), openai.VideoNewParams{}, options...) + require.NoError(t, err) + require.Equal(t, int32(2), requestCount.Load()) + }) + } +} + +func TestMultipartRequestOptionsKnownUploadAllowsTypedNilReaderField(t *testing.T) { + path := filepath.Join(t.TempDir(), "payload.txt") + require.NoError(t, os.WriteFile(path, []byte("payload"), 0o600)) + upload, err := openFileUpload(path) + require.NoError(t, err) + + var requestCount atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount.Add(1) + assert.Positive(t, r.ContentLength) + if !assert.NoError(t, r.ParseMultipartForm(1<<20)) { + http.Error(w, "invalid multipart form", http.StatusBadRequest) + return + } + assert.Equal(t, []string{""}, r.MultipartForm.Value["optional"]) + assert.Len(t, r.MultipartForm.File["file"], 1) + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"id":"file_123","object":"file","bytes":7,"filename":"payload.txt","purpose":"assistants"}`) + })) + t.Cleanup(server.Close) + + options, err := multipartRequestOptions(map[string]any{ + "file": upload, + "optional": typedNilMultipartReader(), + "purpose": "assistants", + }, apiform.FormatBrackets) + require.NoError(t, err) + client := openai.NewClient( + option.WithAPIKey("test-key"), + option.WithBaseURL(server.URL+"/"), + ) + + _, err = client.Files.New(context.Background(), openai.FileNewParams{}, options...) + require.NoError(t, err) + require.Equal(t, int32(1), requestCount.Load()) +}