Fix constraints of paths on target images (#3235)

CHANGELOG.md

@@ -10,6 +10,8 @@
 - Update `buf push` to push the license file or doc file (e.g. `README.md`, `LICENSE`) in the
   same directory as `buf.yaml` if a module does not have a license file or doc file in the
   module's directory.
+- Fix constraints of `--path` flag for lint and breaking rules to avoid resolving all files
+  within a module. This change can result in a performance improvement for large workspaces.
 
 ## [v1.37.0] - 2024-08-16
 

private/buf/bufctl/controller.go

@@ -1010,6 +1010,8 @@ func (c *controller) buildImage(
 	return filterImage(image, functionOptions, true)
 }
 
+// buildTargetImageWithConfigs builds an image for each module in the workspace.
+// This is used to associate LintConfig and BreakingConfig on a per-module basis.
 func (c *controller) buildTargetImageWithConfigs(
 	ctx context.Context,
 	workspace bufworkspace.Workspace,
@@ -1029,14 +1031,12 @@ func (c *controller) buildTargetImageWithConfigs(
 		if err != nil {
 			return nil, err
 		}
-		module := moduleSet.GetModuleForOpaqueID(opaqueID)
-		if module == nil {
-			return nil, syserror.Newf("new ModuleSet from WithTargetOpaqueIDs did not have opaqueID %q", opaqueID)
-		}
-		moduleReadBucket, err := bufmodule.ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles(module)
-		if err != nil {
-			return nil, err
-		}
+		// The moduleReadBucket may include more modules than the target module
+		// and its dependencies. This is because the moduleSet is constructed from
+		// the workspace. Targeting the module does not remove non-related modules.
+		// Build image will use the target info to build the image for the specific
+		// module. Non-targeted modules will not be included in the image.
+		moduleReadBucket := bufmodule.ModuleSetToModuleReadBucketWithOnlyProtoFiles(moduleSet)
 		targetFileInfos, err := bufmodule.GetTargetFileInfos(ctx, moduleReadBucket)
 		if err != nil {
 			return nil, err

private/buf/cmd/buf/buf_test.go

@@ -294,10 +294,15 @@ func TestFail7(t *testing.T) {
 		t,
 		nil,
 		bufctl.ExitCodeFileAnnotation,
-		"", // stdout is empty
+		filepath.FromSlash(`testdata/fail/buf/buf.proto:3:1:Files with package "other" must be within a directory "other" relative to root but were in directory "fail/buf".
+testdata/fail/buf/buf.proto:6:9:Field name "oneTwo" should be lower_snake_case, such as "one_two".`),
 		// This is new behavior we introduced. When setting a config override, we no longer do
 		// a search for the controlling workspace. See bufctl/option.go for additional details.
-		filepath.FromSlash(`Failure: testdata/export/other/proto/unimported.proto: import "another.proto": file does not exist`),
+		// Only the paths specified by "--path" in the command are considered. This avoids build
+		// failures from other proto files under testdata. Command "build" succeeds with this path
+		// restriction, "lint" should be able to build the image and only fail on lint issue for
+		// the specified file.
+		"",
 		"lint",
 		"--path",
 		filepath.Join("testdata", "fail", "buf", "buf.proto"),

private/buf/cmd/buf/workspace_test.go

@@ -145,8 +145,8 @@ func TestWorkspaceDir(t *testing.T) {
 			t,
 			nil,
 			bufctl.ExitCodeFileAnnotation,
+			filepath.FromSlash(`testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto:5:8:import "request.proto": file does not exist`),
 			"",
-			filepath.FromSlash(`Failure: testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto: import "request.proto": file does not exist`),
 			"lint",
 			filepath.Join("testdata", "workspace", "success", baseDirPath),
 			"--config",
@@ -156,8 +156,8 @@ func TestWorkspaceDir(t *testing.T) {
 			t,
 			nil,
 			bufctl.ExitCodeFileAnnotation,
+			filepath.FromSlash(`testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto:5:8:import "request.proto": file does not exist`),
 			"",
-			filepath.FromSlash(`Failure: testdata/workspace/success/`+baseDirPath+`/proto/rpc.proto: import "request.proto": file does not exist`),
 			"lint",
 			filepath.Join("testdata", "workspace", "success", baseDirPath),
 			"--config",
@@ -392,8 +392,8 @@ func TestWorkspaceDetached(t *testing.T) {
 			t,
 			nil,
 			bufctl.ExitCodeFileAnnotation,
+			filepath.FromSlash(`testdata/workspace/success/`+dirPath+`/proto/rpc.proto:5:8:import "request.proto": file does not exist`),
 			``,
-			filepath.FromSlash(`Failure: testdata/workspace/success/`+dirPath+`/proto/rpc.proto: import "request.proto": file does not exist`),
 			"lint",
 			filepath.Join("testdata", "workspace", "success", dirPath, "proto"),
 		)

private/bufpkg/bufcheck/buflint/buflint_test.go

@@ -1243,10 +1243,7 @@ func testLintWithOptions(
 	// build the image for the specified module string (opaqueID)
 	moduleSet, err := workspace.WithTargetOpaqueIDs(opaqueID)
 	require.NoError(t, err)
-	module := moduleSet.GetModuleForOpaqueID(opaqueID)
-	require.NotNil(t, module)
-	moduleReadBucket, err := bufmodule.ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles(module)
-	require.NoError(t, err)
+	moduleReadBucket := bufmodule.ModuleSetToModuleReadBucketWithOnlyProtoFiles(moduleSet)
 	image, err := bufimage.BuildImage(
 		ctx,
 		tracing.NopTracer,

private/bufpkg/bufimage/bufimage.go

@@ -262,9 +262,11 @@ func NewImage(imageFiles []ImageFile) (Image, error) {
 // The given ModuleReadBucket must be self-contained.
 //
 // A ModuleReadBucket is self-contained if it was constructed from
-// ModuleSetToModuleReadBucketWithOnlyProtoFiles or
-// ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles. These are likely
-// the only two ways you should have a ModuleReadBucket that you pass to BuildImage.
+// ModuleSetToModuleReadBucketWithOnlyProtoFiles. This is likely the only way you
+// should have a ModuleReadBucket that you pass to BuildImage.
+//
+// The image will contain only files targeted by the input moduleReadBucket,
+// those returned by [bufmodule.GetTargetFileInfos].
 func BuildImage(
 	ctx context.Context,
 	tracer tracing.Tracer,

private/bufpkg/bufimage/build_image_test.go

@@ -318,6 +318,57 @@ func TestCompareSemicolons(t *testing.T) {
 	testCompare(t, runner, "semicolons")
 }
 
+func TestModuleTargetFiles(t *testing.T) {
+	t.Parallel()
+	moduleSet, err := bufmoduletesting.NewModuleSet(
+		bufmoduletesting.ModuleData{
+			Name: "buf.build/foo/a",
+			PathToData: map[string][]byte{
+				"a.proto": []byte(
+					`syntax = "proto3"; package a; import "b.proto";`,
+				),
+			},
+		},
+		bufmoduletesting.ModuleData{
+			Name: "buf.build/foo/b",
+			PathToData: map[string][]byte{
+				"b.proto": []byte(
+					`syntax = "proto3"; package b; import "c.proto";`,
+				),
+			},
+		},
+		bufmoduletesting.ModuleData{
+			Name: "buf.build/foo/c",
+			PathToData: map[string][]byte{
+				"c.proto": []byte(
+					`syntax = "proto3"; package c;`,
+				),
+			},
+		},
+	)
+	require.NoError(t, err)
+	testTagetImageFiles := func(t *testing.T, want []string, opaqueID ...string) {
+		targetModuleSet := moduleSet
+		if len(opaqueID) > 0 {
+			var err error
+			targetModuleSet, err = moduleSet.WithTargetOpaqueIDs(opaqueID...)
+			require.NoError(t, err)
+		}
+		image, err := bufimage.BuildImage(
+			context.Background(),
+			tracing.NopTracer,
+			bufmodule.ModuleSetToModuleReadBucketWithOnlyProtoFiles(targetModuleSet),
+		)
+		require.NoError(t, err)
+		assert.Equal(t, want, testGetImageFilePaths(image))
+	}
+	testTagetImageFiles(t, []string{"a.proto", "b.proto", "c.proto"})
+	testTagetImageFiles(t, []string{"a.proto", "b.proto", "c.proto"}, "buf.build/foo/a")
+	testTagetImageFiles(t, []string{"b.proto", "c.proto"}, "buf.build/foo/b")
+	testTagetImageFiles(t, []string{"c.proto"}, "buf.build/foo/c")
+	testTagetImageFiles(t, []string{"b.proto", "c.proto"}, "buf.build/foo/b", "buf.build/foo/c")
+}
+
 func testCompare(t *testing.T, runner command.Runner, relDirPath string) {
 	dirPath := filepath.Join("testdata", relDirPath)
 	image, fileAnnotations := testBuild(t, false, dirPath, false)

private/bufpkg/bufmodule/module.go

@@ -196,32 +196,6 @@ func ModuleToModuleKey(module Module, digestType DigestType) (ModuleKey, error)
 	)
 }
 
-// ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles converts the Module to a
-// ModuleReadBucket that contains all the .proto files of the Module and its dependencies.
-//
-// Targeting information will remain the same. Note that this means that the result ModuleReadBucket
-// may have no target files! This can occur when path filtering was applied, but the path filters did
-// not match any files in the Module, and none of the Module's files were targeted.
-// It can also happen if the Module nor any of its dependencies were targeted.
-//
-// *** THIS IS PROBABLY NOT THE FUNCTION YOU ARE LOOKING FOR. *** You probably want
-// ModuleSetToModuleReadBucketWithOnlyProtoFiles to convert a ModuleSet/Workspace to a
-// ModuleReadBucket. This function is used for cases where we want to create an Image
-// specifically for one Module, such as when we need to associate LintConfig and BreakingConfig
-// on a per-Module basis for buf lint and buf breaking. See bufctl.Controller, which is likely
-// the only place this should be used outside of testing.
-func ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles(module Module) (ModuleReadBucket, error) {
-	modules := []Module{module}
-	moduleDeps, err := module.ModuleDeps()
-	if err != nil {
-		return nil, err
-	}
-	for _, moduleDep := range moduleDeps {
-		modules = append(modules, moduleDep)
-	}
-	return newMultiProtoFileModuleReadBucket(modules, true), nil
-}
-
 // ModuleDirectModuleDeps is a convenience function that returns only the direct dependencies of the Module.
 func ModuleDirectModuleDeps(module Module) ([]ModuleDep, error) {
 	moduleDeps, err := module.ModuleDeps()

private/bufpkg/bufmodule/module_read_bucket.go

@@ -73,8 +73,7 @@ type ModuleReadBucket interface {
 	// being self-contained.
 	//
 	// A ModuleReadBucket is self-contained if it was constructed from
-	// ModuleSetToModuleReadBucketWithOnlyProtoFiles or
-	// ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles.
+	// ModuleSetToModuleReadBucketWithOnlyProtoFiles.
 	//
 	// A ModuleReadBucket as inherited from a Module is not self-contained.
 	//