Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions internal/output/csv.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"encoding/csv"
"fmt"
"io"
"os"
)

// FormatAsCSV formats data as CSV (with header) and writes it to w.
Expand Down Expand Up @@ -67,10 +66,10 @@ func writeCSVRows(w io.Writer, rows []map[string]string, cols []string, writeHea
flushCSV(cw)
}

// flushCSV flushes the csv.Writer and reports any write error to stderr.
// flushCSV flushes the csv.Writer and reports any write error to ErrorSink.
func flushCSV(cw *csv.Writer) {
cw.Flush()
if err := cw.Error(); err != nil {
fmt.Fprintf(os.Stderr, "csv write error: %v\n", err)
writeInternalError("csv write error: %v\n", err)
}
}
24 changes: 24 additions & 0 deletions internal/output/csv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package output

import (
"bytes"
"errors"
"strings"
"testing"
)
Expand Down Expand Up @@ -150,3 +151,26 @@ func TestFormatAsCSV_SingleObject(t *testing.T) {
t.Errorf("output should contain 'Alice', got:\n%s", out)
}
}

type failingCSVWriter struct{}

func (failingCSVWriter) Write(p []byte) (int, error) {
return 0, errors.New("boom")
}

func TestFormatAsCSV_WriteError_UsesErrorSink(t *testing.T) {
origSink := ErrorSink
var errBuf bytes.Buffer
ErrorSink = &errBuf
defer func() { ErrorSink = origSink }()

data := []interface{}{
map[string]interface{}{"name": "Alice"},
}

FormatAsCSV(failingCSVWriter{}, data)

if got := errBuf.String(); got == "" || !strings.Contains(got, "csv write error: boom") {
t.Fatalf("expected csv write error in ErrorSink, got: %q", got)
}
}
12 changes: 10 additions & 2 deletions internal/output/print.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,20 @@ import (
"github.com/larksuite/cli/internal/validate"
)

// ErrorSink receives best-effort internal output formatting errors.
// Tests and embedders may override it to capture these messages.
var ErrorSink io.Writer = os.Stderr

func writeInternalError(format string, args ...interface{}) {
fmt.Fprintf(ErrorSink, format, args...)
}
Comment on lines +17 to +21
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 ErrorSink not safe for concurrent access

The exported ErrorSink variable is an interface value read inside writeInternalError (which can be called from any goroutine processing output) and written by callers or test setup. Concurrent reads and writes of an interface variable are a data race in Go's memory model. The current tests avoid this by not calling t.Parallel(), but the exported variable invites future callers to mutate it without synchronization. Consider using a sync/atomic.Pointer[io.Writer] or a small mutex-protected accessor to make this safe for concurrent use.

Comment on lines +19 to +21
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silently ignored ErrorSink write error

fmt.Fprintf returns (int, error) and the helper discards it. Given the "best-effort" framing this is intentional, but a brief //nolint or comment like // best-effort; ignore write errors would make it clear the discard is deliberate rather than accidental.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


// PrintJson prints data as formatted JSON to w.
func PrintJson(w io.Writer, data interface{}) {
injectNotice(data)
b, err := json.MarshalIndent(data, "", " ")
if err != nil {
fmt.Fprintf(os.Stderr, "json marshal error: %v\n", err)
writeInternalError("json marshal error: %v\n", err)
return
}
fmt.Fprintln(w, string(b))
Expand Down Expand Up @@ -53,7 +61,7 @@ func PrintNdjson(w io.Writer, data interface{}) {
emit := func(item interface{}) {
b, err := json.Marshal(item)
if err != nil {
fmt.Fprintf(os.Stderr, "ndjson marshal error: %v\n", err)
writeInternalError("ndjson marshal error: %v\n", err)
return
}
fmt.Fprintln(w, string(b))
Expand Down
34 changes: 34 additions & 0 deletions internal/output/print_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,37 @@ func TestPrintJson_NoNotice(t *testing.T) {
t.Error("expected no _notice when PendingNotice is nil")
}
}

func TestPrintJson_MarshalError_UsesErrorSink(t *testing.T) {
origSink := ErrorSink
var errBuf bytes.Buffer
ErrorSink = &errBuf
defer func() { ErrorSink = origSink }()

var out bytes.Buffer
PrintJson(&out, map[string]interface{}{"bad": make(chan int)})

if out.Len() != 0 {
t.Fatalf("expected no stdout output on marshal error, got: %q", out.String())
}
if got := errBuf.String(); got == "" || !bytes.Contains(errBuf.Bytes(), []byte("json marshal error:")) {
t.Fatalf("expected json marshal error in ErrorSink, got: %q", got)
}
}

func TestPrintNdjson_MarshalError_UsesErrorSink(t *testing.T) {
origSink := ErrorSink
var errBuf bytes.Buffer
ErrorSink = &errBuf
defer func() { ErrorSink = origSink }()

var out bytes.Buffer
PrintNdjson(&out, map[string]interface{}{"bad": make(chan int)})

if out.Len() != 0 {
t.Fatalf("expected no stdout output on marshal error, got: %q", out.String())
}
if got := errBuf.String(); got == "" || !bytes.Contains(errBuf.Bytes(), []byte("ndjson marshal error:")) {
t.Fatalf("expected ndjson marshal error in ErrorSink, got: %q", got)
}
}
Loading