Fix storage OS bucket not returning ErrNotExist correctly (#3188)
This PR fixes a bug of the storage bucket where it does not return an
`fs.ErrNotExist` error when it should.
It happens when both conditions met:
* The bucket is created with an absolute path, i.e.
`provider.NewReadWriteBucket("/absPath")`
* `bucket.Get(ctx, "existing.file/foo")` is called, where
`existing.file` exists as a regular file.
Fixes https://github.com/bufbuild/buf/issues/3185diff --git a/private/buf/bufworkspace/workspace_targeting.go b/private/buf/bufworkspace/workspace_targeting.go
index 9695684..db2abf4 100644
--- a/private/buf/bufworkspace/workspace_targeting.go
+++ b/private/buf/bufworkspace/workspace_targeting.go
@@ -625,14 +625,6 @@
ignoreWorkspaceCheck bool,
) (buftarget.ControllingWorkspace, error) {
path = normalpath.Normalize(path)
- // We attempt to check that the provided target path is not a file by checking the extension.
- // Any valid proto file provided as a target would have the .proto extension, so we treat
- // any path given without as a directory.
- // This could be a file without an extension, in which case an error would be returned
- // to the user when we attempt to check for a controlling workspace.
- if normalpath.Ext(path) != "" {
- path = normalpath.Dir(path)
- }
// Keep track of any v1 module found along the way. If we find a v1 or v2 workspace, we
// return that over the v1 module, but we return this as the fallback.
var fallbackV1Module buftarget.ControllingWorkspace
diff --git a/private/pkg/storage/storageos/bucket.go b/private/pkg/storage/storageos/bucket.go
index 19d712b..5639ef8 100644
--- a/private/pkg/storage/storageos/bucket.go
+++ b/private/pkg/storage/storageos/bucket.go
@@ -20,7 +20,6 @@
"io/fs"
"os"
"path/filepath"
- "strings"
"github.com/bufbuild/buf/private/pkg/filepathext"
"github.com/bufbuild/buf/private/pkg/normalpath"
@@ -284,14 +283,10 @@
// It's important that we detect these cases so that
// multi buckets don't unnecessarily fail when one of
// its delegates actually defines the path.
- elements := strings.Split(normalpath.Normalize(externalPath), "/")
- if len(elements) == 1 {
- // The path is a single element, so there aren't
- // any other files to check.
- return err
- }
- for i := len(elements) - 1; i >= 0; i-- {
- parentFileInfo, err := os.Stat(filepath.Join(elements[:i]...))
+ lastParentPath := externalPath
+ parentPath := filepath.Dir(externalPath)
+ for ; parentPath != lastParentPath; lastParentPath, parentPath = parentPath, filepath.Dir(parentPath) {
+ parentFileInfo, err := os.Stat(parentPath)
if err != nil {
continue
}
diff --git a/private/pkg/storage/storageos/storageos_test.go b/private/pkg/storage/storageos/storageos_test.go
index f65a486..17b2cb1 100644
--- a/private/pkg/storage/storageos/storageos_test.go
+++ b/private/pkg/storage/storageos/storageos_test.go
@@ -15,6 +15,9 @@
package storageos_test
import (
+ "context"
+ "io/fs"
+ "os"
"path/filepath"
"testing"
@@ -37,6 +40,72 @@
testWriteBucketToReadBucket,
true,
)
+
+ t.Run("get_non_existent_file", func(t *testing.T) {
+ t.Parallel()
+ ctx := context.Background()
+ // Create a bucket at an absolute path.
+ tempDir := t.TempDir()
+ tempDir, err := filepath.Abs(tempDir)
+ require.NoError(t, err)
+ bucket, err := storageos.NewProvider().NewReadWriteBucket(tempDir)
+ require.NoError(t, err)
+
+ // Write a file to it.
+ writeObjectCloser, err := bucket.Put(ctx, "foo.txt")
+ require.NoError(t, err)
+ written, err := writeObjectCloser.Write([]byte(nil))
+ require.NoError(t, err)
+ require.Zero(t, written)
+ require.NoError(t, writeObjectCloser.Close())
+
+ // Try reading a file as if foo.txt is a directory.
+ _, err = bucket.Get(ctx, "foo.txt/bar.txt")
+ require.ErrorIs(t, err, fs.ErrNotExist)
+ _, err = bucket.Get(ctx, "foo.txt/bar.txt/baz.txt")
+ require.ErrorIs(t, err, fs.ErrNotExist)
+
+ // Read a file that does not exist at all.
+ _, err = bucket.Get(ctx, "baz.txt")
+ require.ErrorIs(t, err, fs.ErrNotExist)
+ })
+
+ t.Run("get_non_existent_file_symlink", func(t *testing.T) {
+ t.Parallel()
+ ctx := context.Background()
+ // Create a bucket at an absolute path.
+ actualTempDir := t.TempDir()
+ actualTempDir, err := filepath.Abs(actualTempDir)
+ require.NoError(t, err)
+ _, err = os.Create(filepath.Join(actualTempDir, "foo.txt"))
+ require.NoError(t, err)
+ tempDir := t.TempDir()
+ tempDir, err = filepath.Abs(tempDir)
+ require.NoError(t, err)
+ tempDir = filepath.Join(tempDir, "sym")
+ require.NoError(t, os.Symlink(actualTempDir, tempDir))
+ t.Cleanup(func() {
+ if err := os.Remove(tempDir); err != nil {
+ t.Error(err)
+ }
+ })
+ provider := storageos.NewProvider(storageos.ProviderWithSymlinks())
+ bucket, err := provider.NewReadWriteBucket(tempDir, storageos.ReadWriteBucketWithSymlinksIfSupported())
+ require.NoError(t, err)
+
+ _, err = bucket.Get(ctx, "foo.txt")
+ require.NoError(t, err)
+
+ // Try reading a file as if foo.txt is a directory.
+ _, err = bucket.Get(ctx, "foo.txt/bar.txt")
+ require.ErrorIs(t, err, fs.ErrNotExist)
+ _, err = bucket.Get(ctx, "foo.txt/bar.txt/baz.txt")
+ require.ErrorIs(t, err, fs.ErrNotExist)
+
+ // Read a file that does not exist at all.
+ _, err = bucket.Get(ctx, "baz.txt")
+ require.ErrorIs(t, err, fs.ErrNotExist)
+ })
}
func testNewReadBucket(t *testing.T, dirPath string, storageosProvider storageos.Provider) (storage.ReadBucket, storagetesting.GetExternalPathFunc) {