From a44de2b940e491854934c972701c08a49dbf4618 Mon Sep 17 00:00:00 2001 From: BK Date: Thu, 19 Jun 2025 18:05:45 -0700 Subject: [PATCH] fix bindetector and tests --- Makefile | 4 +- bindetector/BUILD.bazel | 8 ++- bindetector/bindetector.go | 18 +++---- bindetector/bindetector_unix.go | 29 +++++++---- bindetector/bindetector_unix_test.go | 75 ++++++++++++++++++++++++++-- bindetector/bindetector_windows.go | 22 ++------ exec/exec.go | 24 ++++++++- exec/exec_test.go | 28 +++++++++++ 8 files changed, 156 insertions(+), 52 deletions(-) diff --git a/Makefile b/Makefile index fe785df7..5fd89d02 100644 --- a/Makefile +++ b/Makefile @@ -30,11 +30,9 @@ lint: docker build --tag cavoritelint -f _ci/lint/Dockerfile . docker run cavoritelint -test: gazelle +tests: gazelle bazel test //... -tests: gazelle test - minio: docker run -p 9000:9000 -p 9001:9001 quay.io/minio/minio server /data --console-address ":9001" diff --git a/bindetector/BUILD.bazel b/bindetector/BUILD.bazel index a751e3f6..b75dd26d 100644 --- a/bindetector/BUILD.bazel +++ b/bindetector/BUILD.bazel @@ -9,12 +9,16 @@ go_library( ], importpath = "github.com/discentem/cavorite/bindetector", visibility = ["//:__subpackages__"], - deps = ["@com_github_google_logger//:logger"], + deps = ["//exec"], ) go_test( name = "bindetector_test", srcs = ["bindetector_unix_test.go"], embed = [":bindetector"], - deps = ["@com_github_stretchr_testify//require"], + deps = [ + "//exec", + "@com_github_hashicorp_go_multierror//:go-multierror", + "@com_github_stretchr_testify//assert", + ], ) diff --git a/bindetector/bindetector.go b/bindetector/bindetector.go index 82d828d7..7ee1c098 100644 --- a/bindetector/bindetector.go +++ b/bindetector/bindetector.go @@ -1,23 +1,17 @@ package bindetector import ( - "regexp" - - "github.com/google/logger" + shell "github.com/discentem/cavorite/exec" ) -func IsBinary(filepath string) bool { +func IsBinary(filepath string) (bool, error) { // for now, we must shell out to /usr/bin/file or respective windows exe called file.exe provided by git // someday, binary determination could be implemented in-house without the need for external tools but until then // shelling out is a necessary evil - return isBinary(execFile(filepath)) -} - -func isBinary(bytes []byte) bool { - matched, err := regexp.Match(`binary`, bytes) + e := shell.NewRealExecutor() + binary, err := fileIsABinary(e, filepath) if err != nil { - logger.Errorf("isBinary regex matching error: %v", err) + return false, err } - - return matched + return binary, nil } diff --git a/bindetector/bindetector_unix.go b/bindetector/bindetector_unix.go index d9a81461..54589ccb 100644 --- a/bindetector/bindetector_unix.go +++ b/bindetector/bindetector_unix.go @@ -5,28 +5,35 @@ package bindetector import ( "bytes" - "os/exec" + "errors" - "github.com/google/logger" + shell "github.com/discentem/cavorite/exec" ) -func execFile(filepath string) []byte { - var stdoutBuf, stderrBuf bytes.Buffer - cmd := exec.Command( +func fileIsABinary(executor shell.Executor, filepath string) (bool, error) { + buf := new(bytes.Buffer) + + executor.Command( "/usr/bin/file", "--mime-encoding", "-b", filepath, ) - cmd.Stdout = &stdoutBuf - cmd.Stderr = &stderrBuf + if err := executor.Stream(&nopWriteCloser{buf}); err != nil { + return false, err + } - err := cmd.Run() - if err != nil { - logger.Errorf("isBinary cmd run error: %v", err) + output := bytes.TrimSpace(buf.Bytes()) + if len(output) == 0 { + return false, errors.New("fileIsABinary: no output from file command") } - return stdoutBuf.Bytes() + return bytes.Contains(output, []byte("binary")), nil +} +type nopWriteCloser struct { + *bytes.Buffer } + +func (n *nopWriteCloser) Close() error { return nil } diff --git a/bindetector/bindetector_unix_test.go b/bindetector/bindetector_unix_test.go index ea81ae7e..4a45c08b 100644 --- a/bindetector/bindetector_unix_test.go +++ b/bindetector/bindetector_unix_test.go @@ -1,13 +1,80 @@ package bindetector import ( + "errors" + "io" "testing" - "github.com/stretchr/testify/require" + shell "github.com/discentem/cavorite/exec" + "github.com/hashicorp/go-multierror" + multierr "github.com/hashicorp/go-multierror" + "github.com/stretchr/testify/assert" ) -func TestIsBinary(t *testing.T) { - require.True(t, isBinary([]byte(`binary`))) - require.False(t, isBinary([]byte(`blah`))) +type fakeExecutor struct { + output string +} + +func (f fakeExecutor) Command(name string, arg ...string) {} + +func (f fakeExecutor) Stream(posters ...io.WriteCloser) error { + var result *multierror.Error + for _, p := range posters { + _, err := p.Write([]byte(f.output)) + result = multierr.Append(result, err) + } + return result.ErrorOrNil() +} +type brokenExecutor struct{} + +func (b brokenExecutor) Command(name string, arg ...string) {} + +func (b brokenExecutor) Stream(posters ...io.WriteCloser) error { + return multierr.Append(nil, errors.New("broken executor")) +} +func TestFileIsABinary(t *testing.T) { + tests := []struct { + name string + executor shell.Executor + filepath string + expected bool + expectedErr bool + }{ + { + name: "file is a binary", + executor: fakeExecutor{output: "application/x-executable; charset=binary\n"}, + filepath: "whatever", + expected: true, + expectedErr: false, + }, + { + name: "file is not a binary", + executor: fakeExecutor{output: "text/plain; charset=us-ascii\n"}, + filepath: "whatever", + expected: false, + expectedErr: false, + }, + { + name: "no output from binary check command", + executor: fakeExecutor{output: ""}, + filepath: "whatever", + expected: false, // no output means we can't determine if the file is a binary + expectedErr: true, // we expect an error because the output is empty + }, + { + name: "broken executor", + executor: brokenExecutor{}, + filepath: "whatever", + expected: false, // we can't determine if the file is a binary because the executor + expectedErr: true, // we expect an error because the executor is broken + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual, err := fileIsABinary(test.executor, test.filepath) + assert.Equal(t, test.expected, actual, "expected %v, got %v", test.expected, actual) + assert.Equal(t, test.expectedErr, err != nil, "expected error: %v", test.expectedErr) + }) + } } diff --git a/bindetector/bindetector_windows.go b/bindetector/bindetector_windows.go index e1e230bc..04dc495e 100644 --- a/bindetector/bindetector_windows.go +++ b/bindetector/bindetector_windows.go @@ -3,25 +3,9 @@ package bindetector -func execFile(filepath string) []byte { - var stdoutBuf bytes.Buffer - // cmd := exec.Command( - // "/usr/bin/file", - // "--mime-encoding", - // "-b", - // filepath, - // ) +import "errors" - // cmd.Stdout = &stdoutBuf - // cmd.Stderr = &stderrBuf - - // err := cmd.Run() - // if err != nil { - // logger.Errorf("isBinary cmd run error: %v", err) - // } - - // return stdoutBuf.Bytes() - - return stdoutBuf.Bytes() +func fileIsABinary(filepath string) (*bool, error) { + return nil, errors.New("fileIsABinary not implemented on Windows") } diff --git a/exec/exec.go b/exec/exec.go index a7aead0f..20c2131c 100644 --- a/exec/exec.go +++ b/exec/exec.go @@ -18,6 +18,23 @@ var ( type RealExecutor struct { *exec.Cmd + posters []io.WriteCloser +} + +type RealExecutorOption func(e *RealExecutor) + +func WithPosters(posters ...io.WriteCloser) RealExecutorOption { + return func(e *RealExecutor) { + e.posters = posters + } +} + +func NewRealExecutor(opts ...RealExecutorOption) *RealExecutor { + re := &RealExecutor{} + for _, opt := range opts { + opt(re) + } + return re } func (e *RealExecutor) Command(bin string, args ...string) { @@ -45,7 +62,12 @@ func (e *RealExecutor) Stream(posters ...io.WriteCloser) error { } if posters == nil { - posters = []io.WriteCloser{os.Stdout} + if e.posters == nil { + posters = []io.WriteCloser{os.Stdout} + } else { + posters = e.posters + } + } for _, pipe := range inputPipes { diff --git a/exec/exec_test.go b/exec/exec_test.go index ca5f9c85..5385c05e 100644 --- a/exec/exec_test.go +++ b/exec/exec_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) type NopBufferCloser struct { @@ -115,3 +116,30 @@ func TestWriteOutput(t *testing.T) { assert.Equal(t, test.expected, out.String()) } } + +type mockWriteCloser struct { + io.Writer +} + +func (m *mockWriteCloser) Close() error { + return nil +} + +func TestWithPosters_AssignsExactInstances(t *testing.T) { + var buf1, buf2 bytes.Buffer + w1 := &mockWriteCloser{Writer: &buf1} + w2 := &mockWriteCloser{Writer: &buf2} + + exec := RealExecutor{} + WithPosters(w1, w2)(&exec) + + require.Len(t, exec.posters, 2) + + // Confirm the WriteClosers themselves are the same (not just equal) + require.Same(t, w1, exec.posters[0]) + require.Same(t, w2, exec.posters[1]) + + // Also confirm that the internal buffers are the same instances + require.Same(t, &buf1, exec.posters[0].(*mockWriteCloser).Writer) + require.Same(t, &buf2, exec.posters[1].(*mockWriteCloser).Writer) +}