Fix problem with input dir not detecting controlling workspace in certain cases (#3233)

Author: oliversun9

CHANGELOG.md

@@ -3,6 +3,8 @@
 ## [Unreleased]
 
 - Add `--http3` flag to `buf curl` which forces `buf curl` to use HTTP/3 as the transport.
+- Fix issue with directory inputs for v2 workspaces where the specified directory was not itself
+  a path to a module, but contained directories with modules, and the modules would not build.
 - Stop creating empty `buf.lock` files when `buf dep update` does not find new dependencies
   to update and there is no existing `buf.lock`.
 

private/buf/buftarget/terminate.go

@@ -33,8 +33,8 @@ type TerminateFunc func(
 	originalInputPath string,
 ) (ControllingWorkspace, error)
 
-// TerminateAtControllingWorkspace implements a TerminateFunc and returns controlling workspace
-// if one is found at the given prefix.
+// TerminateAtControllingWorkspace implements a TerminateFunc and returns the workspace controlling
+// the input, if one is found at the given prefix.
 func TerminateAtControllingWorkspace(
 	ctx context.Context,
 	bucket storage.ReadBucket,
@@ -77,28 +77,51 @@ func terminateAtControllingWorkspace(
 		// This isn't actually the external directory path, but we do the best we can here for now.
 		return nil, fmt.Errorf("cannot have a buf.work.yaml and buf.yaml in the same directory %q", prefix)
 	}
-	relDirPath, err := normalpath.Rel(prefix, originalInputPath)
+	relInputPath, err := normalpath.Rel(prefix, originalInputPath)
 	if err != nil {
 		return nil, err
 	}
 	if bufYAMLExists && bufYAMLFile.FileVersion() == bufconfig.FileVersionV2 {
+		// A input directory with a v2 buf.yaml is the controlling workspace for itself.
 		if prefix == originalInputPath {
 			return newControllingWorkspace(prefix, nil, bufYAMLFile), nil
 		}
 		for _, moduleConfig := range bufYAMLFile.ModuleConfigs() {
-			if normalpath.EqualsOrContainsPath(moduleConfig.DirPath(), relDirPath, normalpath.Relative) {
+			// For a prefix/buf.yaml with:
+			//  version: v2
+			//  modules:
+			//    - path: foo
+			//    - path: dir/bar
+			//    - path: dir/baz
+			//  ...
+			// - If the input is a module path (one of prefix/foo, prefix/dir/bar or prefix/dir/baz),
+			//   then the input is a module controlled by the workspace at prefix.
+			// - If the input is inside one of the module DirPaths (e.g. prefix/foo/suffix or prefix/dir/bar/suffix)
+			//   we still consider prefix to be the workspace that controls the input. It is then up
+			//   to the caller to decide what to do with this information. For example, the caller could
+			//   say this is equivalent to input being prefix/foo with --path=prefix/foo/suffix specified,
+			//   or it could say this is invalid, or the caller is not be concerned with validity.
+			if normalpath.EqualsOrContainsPath(moduleConfig.DirPath(), relInputPath, normalpath.Relative) {
+				return newControllingWorkspace(prefix, nil, bufYAMLFile), nil
+			}
+			// Only in v2: if the input is not any of the module paths but contains a module path,
+			// e.g. prefix/dir, we also consider prefix to be the controlling workspace, because in v2
+			// an input is allowed to be a subset of a workspace's modules. In this example, input prefix/dir
+			// is two modules, one at prefix/dir/bar and the other at prefix/dir/baz.
+			if normalpath.EqualsOrContainsPath(relInputPath, moduleConfig.DirPath(), normalpath.Relative) {
 				return newControllingWorkspace(prefix, nil, bufYAMLFile), nil
 			}
 		}
 	}
 	if bufWorkYAMLExists {
-		// For v1 workspaces, we ensure that the module path list actually contains the original
-		// input paths.
+		// A input directory with a buf.work.yaml is the controlling workspace for itself.
 		if prefix == originalInputPath {
 			return newControllingWorkspace(prefix, bufWorkYAMLFile, nil), nil
 		}
 		for _, dirPath := range bufWorkYAMLFile.DirPaths() {
-			if normalpath.EqualsOrContainsPath(dirPath, relDirPath, normalpath.Relative) {
+			// Unlike v2 workspaces, we only check whether the input is a module path or is contained
+			// in a module path.
+			if normalpath.EqualsOrContainsPath(dirPath, relInputPath, normalpath.Relative) {
 				return newControllingWorkspace(prefix, bufWorkYAMLFile, nil), nil
 			}
 		}

private/buf/cmd/buf/testdata/workspace/success/shared_parent_dir/buf.yaml

@@ -0,0 +1,6 @@
+version: v2
+modules:
+  - path: standalone
+  - path: parent/foo
+  - path: parent/nextlayer/bar
+  - path: parent/nextlayer/baz

private/buf/cmd/buf/testdata/workspace/success/shared_parent_dir/parent/foo/foo.proto

@@ -0,0 +1,7 @@
+package foo;
+
+import "imported.proto";
+
+message Foo {
+  optional standalone.Imported i = 1;
+}

private/buf/cmd/buf/testdata/workspace/success/shared_parent_dir/parent/nextlayer/bar/bar.proto

@@ -0,0 +1,7 @@
+package bar;
+
+import "foo.proto";
+
+message Bar {
+  optional foo.Foo foo = 1;
+}

private/buf/cmd/buf/testdata/workspace/success/shared_parent_dir/parent/nextlayer/baz/baz.proto

@@ -0,0 +1,7 @@
+package baz;
+
+import "bar.proto";
+
+message Baz {
+  optional bar.Bar bar = 1;
+}

private/buf/cmd/buf/testdata/workspace/success/shared_parent_dir/standalone/imported.proto

@@ -0,0 +1,3 @@
+package standalone;
+
+message Imported {}

private/buf/cmd/buf/testdata/workspace/success/shared_parent_dir/standalone/standalone.proto

@@ -0,0 +1,3 @@
+package standalone;
+
+message Standalone {}

private/buf/cmd/buf/workspace_test.go

@@ -24,11 +24,17 @@ import (
 	"testing"
 
 	"github.com/bufbuild/buf/private/buf/bufctl"
+	"github.com/bufbuild/buf/private/buf/cmd/buf/internal/internaltesting"
 	"github.com/bufbuild/buf/private/bufpkg/bufmodule"
+	imagev1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/image/v1"
+	"github.com/bufbuild/buf/private/pkg/app/appcmd"
+	"github.com/bufbuild/buf/private/pkg/app/appcmd/appcmdtesting"
 	"github.com/bufbuild/buf/private/pkg/osext"
+	"github.com/bufbuild/buf/private/pkg/slicesext"
 	"github.com/bufbuild/buf/private/pkg/storage/storagearchive"
 	"github.com/bufbuild/buf/private/pkg/storage/storageos"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
 )
 
 func TestWorkspaceDir(t *testing.T) {
@@ -1427,6 +1433,58 @@ func TestWorkspaceWithInvalidArchiveAbsolutePathFail(t *testing.T) {
 	)
 }
 
+func TestWorkspaceWithTargettingModuleCommonParentDir(t *testing.T) {
+	workspaceDir := filepath.Join("testdata", "workspace", "success", "shared_parent_dir")
+	requireBuildOutputFilePaths(
+		t,
+		map[string]expectedFileInfo{
+			"foo.proto":        {},
+			"bar.proto":        {},
+			"baz.proto":        {},
+			"imported.proto":   {},
+			"standalone.proto": {},
+		},
+		workspaceDir,
+	)
+	requireBuildOutputFilePaths(
+		t,
+		map[string]expectedFileInfo{
+			"imported.proto":   {},
+			"standalone.proto": {},
+		},
+		filepath.Join(workspaceDir, "standalone"),
+	)
+	requireBuildOutputFilePaths(
+		t,
+		map[string]expectedFileInfo{
+			"foo.proto":      {},
+			"bar.proto":      {},
+			"baz.proto":      {},
+			"imported.proto": {isImport: true},
+		},
+		filepath.Join(workspaceDir, "parent"),
+	)
+	requireBuildOutputFilePaths(
+		t,
+		map[string]expectedFileInfo{
+			"foo.proto":      {isImport: true},
+			"bar.proto":      {},
+			"baz.proto":      {},
+			"imported.proto": {isImport: true},
+		},
+		filepath.Join(workspaceDir, "parent/nextlayer"),
+	)
+	requireBuildOutputFilePaths(
+		t,
+		map[string]expectedFileInfo{
+			"foo.proto":      {isImport: true},
+			"bar.proto":      {},
+			"imported.proto": {isImport: true},
+		},
+		filepath.Join(workspaceDir, "parent/nextlayer/bar"),
+	)
+}
+
 func createZipFromDir(t *testing.T, rootPath string, archiveName string) string {
 	zipDir := filepath.Join(t.TempDir(), rootPath)
 	require.NoError(t, os.MkdirAll(zipDir, 0755))
@@ -1466,3 +1524,41 @@ func createZipFromDir(t *testing.T, rootPath string, archiveName string) string
 	require.NoError(t, err)
 	return zipDir
 }
+
+type expectedFileInfo struct {
+	isImport bool
+}
+
+func requireBuildOutputFilePaths(t *testing.T, expectedFilePathToInfo map[string]expectedFileInfo, buildArgs ...string) {
+	stdout := bytes.NewBuffer(nil)
+	stderr := bytes.NewBuffer(nil)
+	appcmdtesting.RunCommandExitCode(
+		t,
+		func(use string) *appcmd.Command { return NewRootCommand(use) },
+		0,
+		internaltesting.NewEnvFunc(t),
+		nil,
+		stdout,
+		stderr,
+		append(
+			[]string{
+				"build",
+				"-o=-#format=binpb",
+			},
+			buildArgs...,
+		)...,
+	)
+	outputImage := &imagev1.Image{}
+	require.NoError(t, proto.Unmarshal(stdout.Bytes(), outputImage))
+
+	filesToCheck := slicesext.ToStructMap(slicesext.MapKeysToSlice(expectedFilePathToInfo))
+
+	for _, imageFile := range outputImage.File {
+		filePath := imageFile.GetName()
+		expectedFileInfo, ok := expectedFilePathToInfo[filePath]
+		require.Truef(t, ok, "unexpected file in the image built: %s", filePath)
+		require.Equal(t, expectedFileInfo.isImport, imageFile.BufExtension.GetIsImport())
+		delete(filesToCheck, filePath)
+	}
+	require.Zerof(t, len(filesToCheck), "expected files missing from image built: %v", slicesext.MapKeysToSortedSlice(filesToCheck))
+}