diff --git a/internal/cli/check.go b/internal/cli/check.go index 4375d32..a0db793 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -298,7 +298,11 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { log.Infof("checking manifest %s with base %s", manifestPath, basePath) // Create checker - chk, err := mfer.NewChecker(manifestPath, basePath, mfa.Fs) + chk, err := mfer.NewChecker(&mfer.CheckerOptions{ + ManifestPath: manifestPath, + BasePath: basePath, + Fs: mfa.Fs, + }) if err != nil { return fmt.Errorf("failed to load manifest: %w", err) } diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 8169b8d..398ba3b 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -405,7 +405,10 @@ func TestGenerateExcludesDotfilesByDefault(t *testing.T) { assert.True(t, exists) // Verify manifest only has 1 file (the non-dotfile) - manifest, err := mfer.NewManifestFromFile(fs, testMF) + manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ + Path: testMF, + Fs: fs, + }) require.NoError(t, err) assert.Len(t, manifest.Files(), 1) assert.Equal(t, "file1.txt", manifest.Files()[0].GetPath()) @@ -429,7 +432,10 @@ func TestGenerateWithIncludeDotfiles(t *testing.T) { require.Equal(t, 0, exitCode) // Verify manifest has 2 files (including dotfile) - manifest, err := mfer.NewManifestFromFile(fs, testMF) + manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ + Path: testMF, + Fs: fs, + }) require.NoError(t, err) assert.Len(t, manifest.Files(), 2) } diff --git a/internal/cli/errmsg_test.go b/internal/cli/errmsg_test.go index 5683120..66a7a0f 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -61,7 +61,11 @@ func unsignedChecker(t *testing.T) *mfer.Checker { require.NoError(t, s.ToManifest(context.Background(), &buf, nil)) require.NoError(t, afero.WriteFile(fs, "/d/index.mf", buf.Bytes(), 0o644)) - chk, err := mfer.NewChecker("/d/index.mf", "/d", fs) + chk, err := mfer.NewChecker(&mfer.CheckerOptions{ + ManifestPath: "/d/index.mf", + BasePath: "/d", + Fs: fs, + }) require.NoError(t, err) require.False(t, chk.IsSigned()) @@ -167,7 +171,11 @@ func signedChecker(t *testing.T) *mfer.Checker { fs := afero.NewMemMapFs() require.NoError(t, afero.WriteFile(fs, "/index.mf", buf.Bytes(), 0o644)) - chk, err := mfer.NewChecker("/index.mf", "/", fs) + chk, err := mfer.NewChecker(&mfer.CheckerOptions{ + ManifestPath: "/index.mf", + BasePath: "/", + Fs: fs, + }) require.NoError(t, err) require.True(t, chk.IsSigned()) diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index 0640b15..170179f 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -445,7 +445,10 @@ func (mfa *CLIApp) loadExistingEntries( log.Infof("loading manifest from %s", manifestPath) // Load existing manifest - manifest, err := mfer.NewManifestFromFile(mfa.Fs, manifestPath) + manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ + Path: manifestPath, + Fs: mfa.Fs, + }) if err != nil { return nil, fmt.Errorf("failed to load manifest: %w", err) } diff --git a/internal/cli/freshen_test.go b/internal/cli/freshen_test.go index 0679729..84d69ba 100644 --- a/internal/cli/freshen_test.go +++ b/internal/cli/freshen_test.go @@ -58,7 +58,10 @@ func TestFreshenUnchanged(t *testing.T) { setupFreshenDir(t, fs) // Parse manifest to verify - manifest, err := mfer.NewManifestFromFile(fs, "/testdir/.index.mf") + manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ + Path: "/testdir/.index.mf", + Fs: fs, + }) require.NoError(t, err) assert.Len(t, manifest.Files(), 2) } @@ -70,7 +73,10 @@ func TestFreshenWithChanges(t *testing.T) { setupFreshenDir(t, fs) // Verify initial manifest has 2 files - manifest, err := mfer.NewManifestFromFile(fs, "/testdir/.index.mf") + manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ + Path: "/testdir/.index.mf", + Fs: fs, + }) require.NoError(t, err) assert.Len(t, manifest.Files(), 2) diff --git a/mfer/builder_test.go b/mfer/builder_test.go index 6a90e8a..4b720c4 100644 --- a/mfer/builder_test.go +++ b/mfer/builder_test.go @@ -5,10 +5,12 @@ import ( "bytes" "context" "fmt" + "path/filepath" "strings" "testing" "time" + "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -397,6 +399,29 @@ func TestNewManifestFromReaderTruncated(t *testing.T) { assert.Error(t, err) } +func TestNewManifestFromFileRequiresPath(t *testing.T) { + t.Parallel() + + _, err := NewManifestFromFile(nil) + require.ErrorIs(t, err, errManifestPathEmpty) + + _, err = NewManifestFromFile(&ManifestFromFileOptions{Fs: afero.NewMemMapFs()}) + require.ErrorIs(t, err, errManifestPathEmpty) +} + +func TestNewManifestFromFileNilFsUsesOsFs(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "index.mf") + createTestManifest(t, afero.NewOsFs(), path, map[string][]byte{ + testFileName: []byte("hello"), + }) + + m, err := NewManifestFromFile(&ManifestFromFileOptions{Path: path}) + require.NoError(t, err) + assert.Len(t, m.Files(), 1) +} + func TestManifestString(t *testing.T) { t.Parallel() diff --git a/mfer/checker.go b/mfer/checker.go index 1a97028..0b4fd37 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -14,7 +14,11 @@ import ( "github.com/spf13/afero" ) -var errNoSigningPubKey = errors.New("manifest has no signing public key") +var ( + errNoSigningPubKey = errors.New("manifest has no signing public key") + errManifestPathEmpty = errors.New("manifest path cannot be empty") + errBasePathEmpty = errors.New("base path cannot be empty") +) // Result represents the outcome of checking a single file. type Result struct { @@ -82,20 +86,42 @@ type Checker struct { signingPubKey []byte } -// NewChecker creates a new Checker for the given manifest, base path, and filesystem. -// The basePath is the directory relative to which manifest paths are resolved. -// If fs is nil, the real filesystem (OsFs) is used. -func NewChecker(manifestPath string, basePath string, fs afero.Fs) (*Checker, error) { +// CheckerOptions configures a Checker. +type CheckerOptions struct { + // ManifestPath is the manifest file to check against (required). + ManifestPath string + // BasePath is the directory relative to which manifest paths are + // resolved (required). + BasePath string + // Fs is the filesystem to use, defaults to OsFs if nil. + Fs afero.Fs +} + +// NewChecker creates a new Checker with the given options. It returns an +// error if opts is nil or either path is empty. +func NewChecker(opts *CheckerOptions) (*Checker, error) { + if opts == nil || opts.ManifestPath == "" { + return nil, errManifestPathEmpty + } + + if opts.BasePath == "" { + return nil, errBasePathEmpty + } + + fs := opts.Fs if fs == nil { fs = afero.NewOsFs() } - m, err := NewManifestFromFile(fs, manifestPath) + m, err := NewManifestFromFile(&ManifestFromFileOptions{ + Path: opts.ManifestPath, + Fs: fs, + }) if err != nil { return nil, err } - abs, err := filepath.Abs(basePath) + abs, err := filepath.Abs(opts.BasePath) if err != nil { return nil, err } @@ -108,7 +134,7 @@ func NewChecker(manifestPath string, basePath string, fs afero.Fs) (*Checker, er } // Compute manifest's relative path from basePath for exclusion in FindExtraFiles - absManifest, err := filepath.Abs(manifestPath) + absManifest, err := filepath.Abs(opts.ManifestPath) if err != nil { return nil, err } diff --git a/mfer/checker_test.go b/mfer/checker_test.go index febc428..0b2c692 100644 --- a/mfer/checker_test.go +++ b/mfer/checker_test.go @@ -5,6 +5,8 @@ import ( "bytes" "context" "fmt" + "os" + "path/filepath" "testing" "time" @@ -92,7 +94,11 @@ func TestNewChecker(t *testing.T) { } createTestManifest(t, fs, "/manifest.mf", files) - chk, err := NewChecker("/manifest.mf", "/", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/", + Fs: fs, + }) require.NoError(t, err) assert.NotNil(t, chk) assert.Equal(t, FileCount(2), chk.FileCount()) @@ -102,7 +108,11 @@ func TestNewChecker(t *testing.T) { t.Parallel() fs := afero.NewMemMapFs() - _, err := NewChecker("/nonexistent.mf", "/", fs) + _, err := NewChecker(&CheckerOptions{ + ManifestPath: "/nonexistent.mf", + BasePath: "/", + Fs: fs, + }) assert.Error(t, err) }) @@ -111,11 +121,73 @@ func TestNewChecker(t *testing.T) { fs := afero.NewMemMapFs() require.NoError(t, afero.WriteFile(fs, "/bad.mf", []byte("not a manifest"), 0o644)) - _, err := NewChecker("/bad.mf", "/", fs) + _, err := NewChecker(&CheckerOptions{ + ManifestPath: "/bad.mf", + BasePath: "/", + Fs: fs, + }) assert.Error(t, err) }) } +func TestNewCheckerRequiredPaths(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + opts *CheckerOptions + want string + is error + }{ + { + name: "nil options", + opts: nil, + want: "manifest path cannot be empty", + is: errManifestPathEmpty, + }, + { + name: "empty manifest path", + opts: &CheckerOptions{BasePath: "/data"}, + want: "manifest path cannot be empty", + is: errManifestPathEmpty, + }, + { + name: "empty base path", + opts: &CheckerOptions{ManifestPath: "/manifest.mf"}, + want: "base path cannot be empty", + is: errBasePathEmpty, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + chk, err := NewChecker(tc.opts) + require.ErrorIs(t, err, tc.is) + assert.EqualError(t, err, tc.want) + assert.Nil(t, chk) + }) + } +} + +func TestNewCheckerNilFsUsesOsFs(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + manifestPath := filepath.Join(dir, "index.mf") + content := []byte("hello") + createTestManifest(t, afero.NewOsFs(), manifestPath, map[string][]byte{ + testFile1: content, + }) + require.NoError(t, os.WriteFile(filepath.Join(dir, testFile1), content, 0o600)) + + chk, err := NewChecker(&CheckerOptions{ManifestPath: manifestPath, BasePath: dir}) + require.NoError(t, err) + + results := make(chan Result, 1) + require.NoError(t, chk.Check(context.Background(), results, nil)) + assert.Equal(t, StatusOK, (<-results).Status) +} + func TestCheckerFileCountAndTotalBytes(t *testing.T) { t.Parallel() @@ -127,7 +199,11 @@ func TestCheckerFileCountAndTotalBytes(t *testing.T) { } createTestManifest(t, fs, "/manifest.mf", files) - chk, err := NewChecker("/manifest.mf", "/", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/", + Fs: fs, + }) require.NoError(t, err) assert.Equal(t, FileCount(3), chk.FileCount()) @@ -145,7 +221,11 @@ func TestCheckAllFilesOK(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -178,7 +258,11 @@ func TestCheckMissingFile(t *testing.T) { testExistsFile: []byte("I exist"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -217,7 +301,11 @@ func TestCheckSizeMismatch(t *testing.T) { testFileName: []byte("short"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -245,7 +333,11 @@ func TestCheckHashMismatch(t *testing.T) { testFileName: differentContent, }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -268,7 +360,11 @@ func TestCheckWithProgress(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -308,7 +404,11 @@ func TestCheckContextCancellation(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) @@ -335,7 +435,11 @@ func TestFindExtraFiles(t *testing.T) { testFile2: []byte("extra file"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -371,7 +475,11 @@ func TestFindExtraFilesSkipsManifestAndDotfiles(t *testing.T) { require.NoError(t, fs.MkdirAll("/data", 0o755)) require.NoError(t, afero.WriteFile(fs, "/data/extra.txt", []byte("extra"), 0o644)) - chk, err := NewChecker("/data/.index.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/data/.index.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -403,7 +511,11 @@ func TestFindExtraFilesContextCancellation(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) @@ -422,7 +534,11 @@ func TestCheckNilChannels(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) // Should not panic with nil channels @@ -438,7 +554,11 @@ func TestFindExtraFilesNilChannel(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) // Should not panic with nil channel @@ -465,7 +585,11 @@ func TestCheckSubdirectories(t *testing.T) { require.NoError(t, afero.WriteFile(fs, fullPath, content, 0o644)) } - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -499,7 +623,11 @@ func TestCheckMissingFileDetectedWithoutFallback(t *testing.T) { testExistsFile: []byte("here"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -537,7 +665,11 @@ func TestFindExtraFilesSkipsDotfiles(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/data/.git/config", []byte("git config"), 0o644)) - chk, err := NewChecker("/data/.index.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/data/.index.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -566,7 +698,11 @@ func TestFindExtraFilesSkipsManifestFile(t *testing.T) { createTestManifest(t, fs, "/data/index.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/data/index.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/data/index.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 10) @@ -589,7 +725,11 @@ func TestCheckEmptyManifest(t *testing.T) { // Create manifest with no files createTestManifest(t, fs, "/manifest.mf", map[string][]byte{}) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) assert.Equal(t, FileCount(0), chk.FileCount()) @@ -624,7 +764,11 @@ func TestCheckProgressRateLimited(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: "/manifest.mf", + BasePath: "/data", + Fs: fs, + }) require.NoError(t, err) results := make(chan Result, 200) diff --git a/mfer/deserialize.go b/mfer/deserialize.go index 1fd842f..4c8a208 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -263,16 +263,29 @@ func NewManifestFromReader(input io.Reader) (*manifest, error) { return m, nil } -// NewManifestFromFile reads a manifest from a file path using the given filesystem. -// If fs is nil, the real filesystem (OsFs) is used. +// ManifestFromFileOptions configures NewManifestFromFile. +type ManifestFromFileOptions struct { + // Path is the manifest file to read (required). + Path string + // Fs is the filesystem to use, defaults to OsFs if nil. + Fs afero.Fs +} + +// NewManifestFromFile reads a manifest from a file. It returns an error if +// opts is nil or its path is empty. // //nolint:revive // unexported-return: exporting manifest is owner question 13 -func NewManifestFromFile(fs afero.Fs, path string) (*manifest, error) { +func NewManifestFromFile(opts *ManifestFromFileOptions) (*manifest, error) { + if opts == nil || opts.Path == "" { + return nil, errManifestPathEmpty + } + + fs := opts.Fs if fs == nil { fs = afero.NewOsFs() } - f, err := fs.Open(path) + f, err := fs.Open(opts.Path) if err != nil { return nil, err }