Add more tests for buf.work.yaml and normalize and validate DirPaths

private/bufnew/bufconfig/buf_work_yaml_file.go

@@ -58,6 +58,8 @@ type BufWorkYAMLFile interface {
 	// - There are no duplicate paths - all values of DirPaths() are unique.
 	// - No path contains another path, i.e. "foo" and "foo/bar" will not be in DirPaths().
 	// - "." is not in DirPaths().
+	// - Each path is normalized and validated, because this is guaranteed at the
+	//   construction time of a BufWorkYAMLFile.
 	//
 	// Returned paths are sorted.
 	DirPaths() []string
@@ -67,14 +69,7 @@ type BufWorkYAMLFile interface {
 
 // NewBufWorkYAMLFile returns a new BufWorkYAMLFile.
 func NewBufWorkYAMLFile(fileVersion FileVersion, dirPaths []string) (BufWorkYAMLFile, error) {
-	bufWorkYAMLFile, err := newBufWorkYAMLFile(fileVersion, dirPaths)
-	if err != nil {
-		return nil, err
-	}
-	if err := checkV2SupportedYet(bufWorkYAMLFile); err != nil {
-		return nil, err
-	}
-	return bufWorkYAMLFile, nil
+	return newBufWorkYAMLFile(fileVersion, dirPaths)
 }
 
 // GetBufWorkYAMLFileForPrefix gets the buf.work.yaml file at the given bucket prefix.
@@ -129,13 +124,16 @@ type bufWorkYAMLFile struct {
 }
 
 func newBufWorkYAMLFile(fileVersion FileVersion, dirPaths []string) (*bufWorkYAMLFile, error) {
-	if err := validateBufWorkYAMLDirPaths(dirPaths); err != nil {
+	if fileVersion != FileVersionV1 {
+		return nil, newUnsupportedFileVersionError(fileVersion)
+	}
+	sortedNormalizedDirPaths, err := validateBufWorkYAMLDirPaths(dirPaths)
+	if err != nil {
 		return nil, err
 	}
-	sort.Strings(dirPaths)
 	return &bufWorkYAMLFile{
 		fileVersion: fileVersion,
-		dirPaths:    dirPaths,
+		dirPaths:    sortedNormalizedDirPaths,
 	}, nil
 }
 
@@ -200,52 +198,59 @@ func writeBufWorkYAMLFile(writer io.Writer, bufWorkYAMLFile BufWorkYAMLFile) err
 	}
 }
 
-func validateBufWorkYAMLDirPaths(dirPaths []string) error {
+// validateBufWorkYAMLDirPaths validates dirPaths and returns normalized and
+// sorted dirPaths.
+func validateBufWorkYAMLDirPaths(dirPaths []string) ([]string, error) {
 	if len(dirPaths) == 0 {
-		return fmt.Errorf(`directories is empty`)
+		return nil, fmt.Errorf(`directories is empty`)
 	}
-	dirPathMap := make(map[string]struct{}, len(dirPaths))
+	normalizedDirPathToDirPath := make(map[string]string, len(dirPaths))
 	for _, dirPath := range dirPaths {
-		dirPath, err := normalpath.NormalizeAndValidate(dirPath)
+		normalizedDirPath, err := normalpath.NormalizeAndValidate(dirPath)
 		if err != nil {
-			return fmt.Errorf(`directory %q is invalid: %w`, normalpath.Unnormalize(dirPath), err)
+			return nil, fmt.Errorf(`directory %q is invalid: %w`, dirPath, err)
 		}
-		if _, ok := dirPathMap[dirPath]; ok {
-			return fmt.Errorf(`directory %q is listed more than once`, dirPath)
+		if _, ok := normalizedDirPathToDirPath[normalizedDirPath]; ok {
+			return nil, fmt.Errorf(`directory %q is listed more than once`, dirPath)
 		}
-		if dirPath == "." {
-			return fmt.Errorf(`directory "." is listed, it is not valid to have "." as a workspace directory, as this is no different than not having a workspace at all, see https://buf.build/docs/reference/workspaces/#directories for more details`)
+		if normalizedDirPath == "." {
+			return nil, fmt.Errorf(`directory "." is listed, it is not valid to have "." as a workspace directory, as this is no different than not having a workspace at all, see https://buf.build/docs/reference/workspaces/#directories for more details`)
 		}
-		dirPathMap[dirPath] = struct{}{}
+		normalizedDirPathToDirPath[normalizedDirPath] = dirPath
 	}
 	// We already know the paths are unique due to above validation.
 	// We sort to print deterministic errors.
-	if err := validateDirPathsNoOverlap(slicesextended.MapToSortedSlice(dirPathMap)); err != nil {
-		return err
+	// TODO: use this line:
+	// sortedNormalizedDirPaths := slicesextended.MapKeysToSortedSlice(normalDirPathToDirPath)
+	sortedNormalizedDirPaths := make([]string, 0, len(normalizedDirPathToDirPath))
+	for normalizedDirPath := range normalizedDirPathToDirPath {
+		sortedNormalizedDirPaths = append(sortedNormalizedDirPaths, normalizedDirPath)
 	}
-	return nil
-}
-
-func validateDirPathsNoOverlap(dirPaths []string) error {
-	for i := 0; i < len(dirPaths); i++ {
-		for j := i + 1; j < len(dirPaths); j++ {
-			left := dirPaths[i]
-			right := dirPaths[j]
+	sort.Slice(
+		sortedNormalizedDirPaths,
+		func(i int, j int) bool {
+			return sortedNormalizedDirPaths[i] < sortedNormalizedDirPaths[j]
+		},
+	)
+	for i := 0; i < len(sortedNormalizedDirPaths); i++ {
+		for j := i + 1; j < len(sortedNormalizedDirPaths); j++ {
+			left := sortedNormalizedDirPaths[i]
+			right := sortedNormalizedDirPaths[j]
 			if normalpath.ContainsPath(left, right, normalpath.Relative) {
-				return fmt.Errorf(
+				return nil, fmt.Errorf(
 					`directory %q contains directory %q`,
-					normalpath.Unnormalize(left),
-					normalpath.Unnormalize(right),
+					normalizedDirPathToDirPath[left],
+					normalizedDirPathToDirPath[right],
 				)
 			}
 			if normalpath.ContainsPath(right, left, normalpath.Relative) {
-				return fmt.Errorf(
+				return nil, fmt.Errorf(
 					`directory %q contains directory %q`,
-					normalpath.Unnormalize(right),
-					normalpath.Unnormalize(left),
+					normalizedDirPathToDirPath[right],
+					normalizedDirPathToDirPath[left],
 				)
 			}
 		}
 	}
-	return nil
+	return sortedNormalizedDirPaths, nil
 }

private/bufnew/bufconfig/buf_work_yaml_file_test.go

@@ -0,0 +1,199 @@
+// Copyright 2020-2023 Buf Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package bufconfig
+
+import (
+	"context"
+	"path/filepath"
+	"testing"
+
+	"github.com/bufbuild/buf/private/pkg/storage/storagemem"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPutAndGetBufWorkYAMLFileForPrefix(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	bufWorkYAMLFile, err := NewBufWorkYAMLFile(FileVersionV1, []string{"foo", "bar"})
+	require.NoError(t, err)
+	readWriteBucket := storagemem.NewReadWriteBucket()
+	err = PutBufWorkYAMLFileForPrefix(ctx, readWriteBucket, "pre", bufWorkYAMLFile)
+	require.NoError(t, err)
+	_, err = GetBufWorkYAMLFileForPrefix(ctx, readWriteBucket, ".")
+	require.Error(t, err)
+	readBufWorkYAMLFile, err := GetBufWorkYAMLFileForPrefix(ctx, readWriteBucket, "pre")
+	require.NoError(t, err)
+	require.Equal(
+		t,
+		[]string{"bar", "foo"},
+		readBufWorkYAMLFile.DirPaths(),
+	)
+}
+
+func TestReadBufWorkYAMLFileValidateVersion(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	testcases := []struct {
+		description      string
+		prefix           string
+		content          string
+		isErr            bool
+		expectedDirPaths []string
+	}{
+		{
+			description: "current_directory",
+			prefix:      ".",
+			content: `version: v1
+directories:
+  - foo
+  - bar
+`,
+			expectedDirPaths: []string{"bar", "foo"},
+		},
+		{
+			description: "sub_directory",
+			prefix:      "path",
+			content: `version: v1
+directories:
+  - foo
+`,
+			expectedDirPaths: []string{"bar"},
+		},
+		{
+			description: "invalid_version",
+			prefix:      ".",
+			content: `version: 1
+directories:
+  - foo
+`,
+			isErr: true,
+		},
+	}
+	for _, testcase := range testcases {
+		testcase := testcase
+		t.Run(testcase.description, func(t *testing.T) {
+			t.Parallel()
+			readBucket, err := storagemem.NewReadBucket(
+				map[string][]byte{
+					filepath.Join(testcase.prefix, "buf.work.yaml"): []byte(testcase.content),
+				},
+			)
+			require.NoError(t, err)
+			bufWorkYAMLFile, err := GetBufWorkYAMLFileForPrefix(ctx, readBucket, testcase.prefix)
+			if testcase.isErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, testcase.expectedDirPaths, bufWorkYAMLFile.DirPaths())
+		})
+	}
+}
+
+func TestNewBufWorkYAMLFile(t *testing.T) {
+	t.Parallel()
+	testcases := []struct {
+		description      string
+		version          FileVersion
+		dirPaths         []string
+		expectedDirPaths []string
+	}{
+		{
+			description:      "one_dir_path",
+			version:          FileVersionV1,
+			dirPaths:         []string{"foo"},
+			expectedDirPaths: []string{"foo"},
+		},
+		{
+			description:      "sort",
+			version:          FileVersionV1,
+			dirPaths:         []string{"foo", "baz", "bat", "bar"},
+			expectedDirPaths: []string{"bar", "bat", "baz", "foo"},
+		},
+		{
+			description:      "sort_and_normalize",
+			version:          FileVersionV1,
+			dirPaths:         []string{"bat", "./baz", "./bar/../bar", "foo"},
+			expectedDirPaths: []string{"bar", "bat", "baz", "foo"},
+		},
+	}
+	for _, testcase := range testcases {
+		testcase := testcase
+		t.Run(testcase.description, func(t *testing.T) {
+			t.Parallel()
+			bufWorkYAMLFile, err := NewBufWorkYAMLFile(testcase.version, testcase.dirPaths)
+			require.NoError(t, err)
+			require.Equal(t, testcase.expectedDirPaths, bufWorkYAMLFile.DirPaths())
+		})
+	}
+}
+
+func TestNewWorkYAMLFileFail(t *testing.T) {
+	t.Parallel()
+	testcases := []struct {
+		description string
+		version     FileVersion
+		dirPaths    []string
+	}{
+		{
+			description: "empty_dirPaths",
+			version:     FileVersionV1,
+			dirPaths:    []string{},
+		},
+		{
+			description: "duplicate_dirPaths",
+			version:     FileVersionV1,
+			dirPaths:    []string{"foo", "bar", "foo"},
+		},
+		{
+			description: "duplicate_dirPaths_different_forms",
+			version:     FileVersionV1,
+			dirPaths:    []string{"foo", "./foo"},
+		},
+		{
+			description: "overlapping_dirPaths",
+			version:     FileVersionV1,
+			dirPaths:    []string{"foo", "bar", "foo/baz"},
+		},
+		{
+			description: "overlapping_dirPaths_with_dot",
+			version:     FileVersionV1,
+			dirPaths:    []string{"foo", "bar", "./foo/baz"},
+		},
+		{
+			description: "current_directory",
+			version:     FileVersionV1,
+			dirPaths:    []string{"foo", "."},
+		},
+		{
+			description: "invalid_version_v1beta1",
+			version:     FileVersionV1Beta1,
+			dirPaths:    []string{"foo", "bar"},
+		},
+		{
+			description: "invalid_version_v2",
+			version:     FileVersionV2,
+			dirPaths:    []string{"foo", "bar"},
+		},
+	}
+	for _, testcase := range testcases {
+		testcase := testcase
+		t.Run(testcase.description, func(t *testing.T) {
+			t.Parallel()
+			_, err := NewBufWorkYAMLFile(testcase.version, testcase.dirPaths)
+			require.Error(t, err)
+		})
+	}
+}