diff --git a/.dockerignore b/.dockerignore index 88c9c59f34..01e528b5cc 100644 --- a/.dockerignore +++ b/.dockerignore @@ -10,6 +10,7 @@ cmd/buf/buf cmd/buf/internal/command/alpha/protoc/internal/protoc-gen-insertion-point-receiver/protoc-gen-insertion-point-receiver cmd/buf/internal/command/alpha/protoc/internal/protoc-gen-insertion-point-writer/protoc-gen-insertion-point-writer cmd/buf/internal/command/alpha/protoc/test.txt +cmd/buf/internal/command/generate/internal/protoc-gen-files-to-generate-yaml/protoc-gen-files-to-generate-yaml cmd/buf/internal/command/generate/internal/protoc-gen-top-level-type-names-yaml/protoc-gen-top-level-type-names-yaml cmd/buf/testdata/imports/cache/v3/modulelocks/ cmd/buf/testdata/imports/corrupted_cache_dep/v3/modulelocks/ diff --git a/.gitignore b/.gitignore index e0a92183f1..ae39208642 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,7 @@ /cmd/buf/internal/command/alpha/protoc/internal/protoc-gen-insertion-point-receiver/protoc-gen-insertion-point-receiver /cmd/buf/internal/command/alpha/protoc/internal/protoc-gen-insertion-point-writer/protoc-gen-insertion-point-writer /cmd/buf/internal/command/alpha/protoc/test.txt +/cmd/buf/internal/command/generate/internal/protoc-gen-files-to-generate-yaml/protoc-gen-files-to-generate-yaml /cmd/buf/internal/command/generate/internal/protoc-gen-top-level-type-names-yaml/protoc-gen-top-level-type-names-yaml /cmd/buf/testdata/imports/cache/v3/modulelocks/ /cmd/buf/testdata/imports/corrupted_cache_dep/v3/modulelocks/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 76c945721b..b957dd0662 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ - Add `--stdin-filepath` flag to `buf format`, which reads a single `.proto` file from stdin and writes the formatted result to stdout. The path is not read from disk, and is only used to report parse errors and diffs. +- Fix `buf generate` with `strategy: directory` and `include_imports: true` adding imports + to the `CodeGeneratorRequest` of the directory that imports them. Imports are now split + by directory into their own requests, the same as non-imports. ## [v1.73.0] - 2026-09-11 diff --git a/cmd/buf/internal/command/generate/generate_test.go b/cmd/buf/internal/command/generate/generate_test.go index 4a88e931c9..007d2d3a2a 100644 --- a/cmd/buf/internal/command/generate/generate_test.go +++ b/cmd/buf/internal/command/generate/generate_test.go @@ -343,6 +343,103 @@ inputs: ) } +func TestGenerateV2LocalPluginStrategyDirectoryIncludeImports(t *testing.T) { + t.Parallel() + testRunTemplate := func(t *testing.T, expect map[string][]byte, template string, extraArgs ...string) { + t.Helper() + tempDirPath := t.TempDir() + testRunSuccess( + t, + append( + []string{ + "--output", + tempDirPath, + "--template", + template, + // Only target keyvalue, so that common is an import. + "--path", + filepath.Join("testdata", "v2", "include_imports", "keyvalue"), + filepath.Join("testdata", "v2", "include_imports"), + }, + extraArgs..., + )..., + ) + expected, err := storagemem.NewReadBucket(expect) + require.NoError(t, err) + actual, err := storageos.NewProvider().NewReadWriteBucket(tempDirPath) + require.NoError(t, err) + diff, err := storage.DiffBytes(t.Context(), expected, actual) + require.NoError(t, err) + require.Empty(t, string(diff)) + } + commonFilesToGenerate := []byte(`files: + - common/v1/value.proto +`) + keyValueFilesToGenerate := []byte(`files: + - keyvalue/v1/service.proto +`) + timestampFilesToGenerate := []byte(`files: + - google/protobuf/timestamp.proto +`) + // Without include_imports, only the directory with non-imports is generated. + testRunTemplate( + t, + map[string][]byte{ + filepath.Join("gen", "keyvalue", "v1", "files-to-generate.yaml"): keyValueFilesToGenerate, + }, + `version: v2 +plugins: + - local: protoc-gen-files-to-generate-yaml + out: gen + strategy: directory`, + ) + // With include_imports, imports are generated in a separate request for their directory. + // The plugin fails if a request has files to generate from more than one directory. + testRunTemplate( + t, + map[string][]byte{ + filepath.Join("gen", "common", "v1", "files-to-generate.yaml"): commonFilesToGenerate, + filepath.Join("gen", "keyvalue", "v1", "files-to-generate.yaml"): keyValueFilesToGenerate, + }, + `version: v2 +plugins: + - local: protoc-gen-files-to-generate-yaml + out: gen + strategy: directory + include_imports: true`, + ) + // With include_wkt as well, well-known types are generated in their own request. + testRunTemplate( + t, + map[string][]byte{ + filepath.Join("gen", "common", "v1", "files-to-generate.yaml"): commonFilesToGenerate, + filepath.Join("gen", "google", "protobuf", "files-to-generate.yaml"): timestampFilesToGenerate, + filepath.Join("gen", "keyvalue", "v1", "files-to-generate.yaml"): keyValueFilesToGenerate, + }, + `version: v2 +plugins: + - local: protoc-gen-files-to-generate-yaml + out: gen + strategy: directory + include_imports: true + include_wkt: true`, + ) + // --include-imports on the command line overrides the template. + testRunTemplate( + t, + map[string][]byte{ + filepath.Join("gen", "common", "v1", "files-to-generate.yaml"): commonFilesToGenerate, + filepath.Join("gen", "keyvalue", "v1", "files-to-generate.yaml"): keyValueFilesToGenerate, + }, + `version: v2 +plugins: + - local: protoc-gen-files-to-generate-yaml + out: gen + strategy: directory`, + "--include-imports", + ) +} + func TestOutputFlag(t *testing.T) { t.Parallel() for _, paths := range []struct { diff --git a/cmd/buf/internal/command/generate/internal/protoc-gen-files-to-generate-yaml/main.go b/cmd/buf/internal/command/generate/internal/protoc-gen-files-to-generate-yaml/main.go new file mode 100644 index 0000000000..7472b05cb9 --- /dev/null +++ b/cmd/buf/internal/command/generate/internal/protoc-gen-files-to-generate-yaml/main.go @@ -0,0 +1,72 @@ +// Copyright 2020-2026 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. + +// protoc-gen-files-to-generate-yaml writes one file per CodeGeneratorRequest +// listing the files to generate in that request. +// +// All files to generate in a request must be in the same directory, and the +// output is written to that directory. This is used to test that +// strategy: directory splits requests by directory, including when +// include_imports is set. +package main + +import ( + "context" + "errors" + "fmt" + "path" + "slices" + "strings" + + "github.com/bufbuild/protoplugin" + "gopkg.in/yaml.v3" +) + +const fileName = "files-to-generate.yaml" + +func main() { + protoplugin.Main(protoplugin.HandlerFunc(handle)) +} + +func handle( + _ context.Context, + _ protoplugin.PluginEnv, + responseWriter protoplugin.ResponseWriter, + request protoplugin.Request, +) error { + filesToGenerate := request.CodeGeneratorRequest().GetFileToGenerate() + if len(filesToGenerate) == 0 { + return errors.New("no files to generate") + } + var dirs []string + for _, fileToGenerate := range filesToGenerate { + dir := path.Dir(fileToGenerate) + if !slices.Contains(dirs, dir) { + dirs = append(dirs, dir) + } + } + if len(dirs) > 1 { + return fmt.Errorf("files to generate span multiple directories: %s", strings.Join(dirs, ", ")) + } + data, err := yaml.Marshal(&externalFile{Files: filesToGenerate}) + if err != nil { + return err + } + responseWriter.AddFile(path.Join(dirs[0], fileName), string(data)) + return nil +} + +type externalFile struct { + Files []string `json:"files,omitempty" yaml:"files,omitempty"` +} diff --git a/cmd/buf/internal/command/generate/testdata/v2/include_imports/buf.yaml b/cmd/buf/internal/command/generate/testdata/v2/include_imports/buf.yaml new file mode 100644 index 0000000000..b4d4e478b7 --- /dev/null +++ b/cmd/buf/internal/command/generate/testdata/v2/include_imports/buf.yaml @@ -0,0 +1 @@ +version: v2 diff --git a/cmd/buf/internal/command/generate/testdata/v2/include_imports/common/v1/value.proto b/cmd/buf/internal/command/generate/testdata/v2/include_imports/common/v1/value.proto new file mode 100644 index 0000000000..5c4501f401 --- /dev/null +++ b/cmd/buf/internal/command/generate/testdata/v2/include_imports/common/v1/value.proto @@ -0,0 +1,7 @@ +syntax = "proto3"; + +package common.v1; + +message Value { + string value = 1; +} diff --git a/cmd/buf/internal/command/generate/testdata/v2/include_imports/keyvalue/v1/service.proto b/cmd/buf/internal/command/generate/testdata/v2/include_imports/keyvalue/v1/service.proto new file mode 100644 index 0000000000..904773fb03 --- /dev/null +++ b/cmd/buf/internal/command/generate/testdata/v2/include_imports/keyvalue/v1/service.proto @@ -0,0 +1,11 @@ +syntax = "proto3"; + +package keyvalue.v1; + +import "common/v1/value.proto"; +import "google/protobuf/timestamp.proto"; + +message Entry { + common.v1.Value value = 1; + google.protobuf.Timestamp updated_at = 2; +} diff --git a/etc/windows/test.bash b/etc/windows/test.bash index a73145c136..b799f5cc68 100644 --- a/etc/windows/test.bash +++ b/etc/windows/test.bash @@ -33,6 +33,7 @@ go install connectrpc.com/connect/cmd/protoc-gen-connect-go@${CONNECT_VERSION} go install ./cmd/buf \ ./cmd/buf/internal/command/alpha/protoc/internal/protoc-gen-insertion-point-writer \ ./cmd/buf/internal/command/alpha/protoc/internal/protoc-gen-insertion-point-receiver \ + ./cmd/buf/internal/command/generate/internal/protoc-gen-files-to-generate-yaml \ ./cmd/buf/internal/command/generate/internal/protoc-gen-top-level-type-names-yaml \ ./private/bufpkg/bufcheck/internal/cmd/buf-plugin-panic \ ./private/bufpkg/bufcheck/internal/cmd/buf-plugin-suffix \ diff --git a/make/buf/all.mk b/make/buf/all.mk index ad99cc3560..c695bb8f6e 100644 --- a/make/buf/all.mk +++ b/make/buf/all.mk @@ -23,6 +23,7 @@ GO_BINS := $(GO_BINS) \ GO_TEST_BINS := $(GO_TEST_BINS) \ cmd/buf/internal/command/alpha/protoc/internal/protoc-gen-insertion-point-receiver \ cmd/buf/internal/command/alpha/protoc/internal/protoc-gen-insertion-point-writer \ + cmd/buf/internal/command/generate/internal/protoc-gen-files-to-generate-yaml \ cmd/buf/internal/command/generate/internal/protoc-gen-top-level-type-names-yaml \ private/bufpkg/bufcheck/internal/cmd/buf-plugin-panic \ private/bufpkg/bufcheck/internal/cmd/buf-plugin-suffix \ diff --git a/private/buf/bufgen/generator.go b/private/buf/bufgen/generator.go index 283f85da5f..789cd6f8a9 100644 --- a/private/buf/bufgen/generator.go +++ b/private/buf/bufgen/generator.go @@ -232,19 +232,6 @@ func (g *generator) execPlugins( } // Local plugins. - var images []bufimage.Image - switch Strategy(pluginConfigForKey.Strategy()) { - case StrategyAll: - images = []bufimage.Image{image} - case StrategyDirectory: - var err error - images, err = bufimage.ImageByDir(image) - if err != nil { - return nil, err - } - default: - return nil, fmt.Errorf("unknown strategy: %v", pluginConfigForKey.Strategy()) - } for _, indexedPluginConfig := range indexedPluginConfigs { jobs = append(jobs, func(ctx context.Context) error { includeImports := indexedPluginConfig.Value.IncludeImports() @@ -255,6 +242,15 @@ func (g *generator) execPlugins( if includeWellKnownTypesOverride != nil { includeWellKnownTypes = *includeWellKnownTypesOverride } + images, err := imagesForStrategy( + image, + Strategy(indexedPluginConfig.Value.Strategy()), + includeImports, + includeWellKnownTypes, + ) + if err != nil { + return err + } response, err := g.execLocalPlugin( ctx, container, @@ -331,6 +327,31 @@ func (g *generator) execLocalPlugin( return response, nil } +// imagesForStrategy returns the Images to use for a local plugin with the given Strategy. +func imagesForStrategy( + image bufimage.Image, + strategy Strategy, + includeImports bool, + includeWellKnownTypes bool, +) ([]bufimage.Image, error) { + switch strategy { + case StrategyAll: + return []bufimage.Image{image}, nil + // Split imports by directory similar to non-imports. + case StrategyDirectory: + var imageByDirOptions []bufimage.ImageByDirOption + if includeImports { + imageByDirOptions = append(imageByDirOptions, bufimage.ImageByDirWithIncludeImports()) + if includeWellKnownTypes { + imageByDirOptions = append(imageByDirOptions, bufimage.ImageByDirWithIncludeWellKnownTypes()) + } + } + return bufimage.ImageByDir(image, imageByDirOptions...) + default: + return nil, fmt.Errorf("unknown strategy: %v", strategy) + } +} + func (g *generator) execRemotePluginsV2( ctx context.Context, container app.EnvStdioContainer, diff --git a/private/bufpkg/bufimage/bufimage.go b/private/bufpkg/bufimage/bufimage.go index 3b1e411615..8ab2b2ab80 100644 --- a/private/bufpkg/bufimage/bufimage.go +++ b/private/bufpkg/bufimage/bufimage.go @@ -517,15 +517,32 @@ func ImageWithOnlyPathsAllowNotExist( // by directory. // // That is, each Image will only contain a single directory's files -// as it's non-imports, along with all required imports for the +// as its non-imports, along with all required imports for the // files in that directory. -func ImageByDir(image Image) ([]Image, error) { +// +// If ImageByDirWithIncludeImports is set, imports are split by directory as well. +// Each non-well-known-type import is a non-import in the Image for its directory, +// and remains an import in every other Image that requires it. If +// ImageByDirWithIncludeWellKnownTypes is also set, well-known-type imports are split +// by directory too. ImageByDirWithIncludeWellKnownTypes has no effect if +// ImageByDirWithIncludeImports is not set. +func ImageByDir(image Image, options ...ImageByDirOption) ([]Image, error) { + imageByDirOptions := newImageByDirOptions() + for _, option := range options { + option(imageByDirOptions) + } imageFiles := image.Files() paths := make([]string, 0, len(imageFiles)) for _, imageFile := range imageFiles { - if !imageFile.IsImport() { - paths = append(paths, imageFile.Path()) + if imageFile.IsImport() { + if !imageByDirOptions.includeImports { + continue + } + if !imageByDirOptions.includeWellKnownTypes && datawkt.Exists(imageFile.Path()) { + continue + } } + paths = append(paths, imageFile.Path()) } dirToPaths := normalpath.ByDir(paths...) // we need this to produce a deterministic order of the returned Images @@ -541,6 +558,8 @@ func ImageByDir(image Image) ([]Image, error) { // this should never happen return nil, fmt.Errorf("no dir for %q in dirToPaths", dir) } + // When includeImports is set, `paths` includes imports, and this call effectively + // promotes them to non-imports in their own image for generation. newImage, err := ImageWithOnlyPaths(image, paths, nil) if err != nil { return nil, err @@ -550,6 +569,27 @@ func ImageByDir(image Image) ([]Image, error) { return newImages, nil } +// ImageByDirOption is an option for ImageByDir. +type ImageByDirOption func(*imageByDirOptions) + +// ImageByDirWithIncludeImports returns a new ImageByDirOption that splits +// non-well-known-type imports by directory alongside non-imports. +func ImageByDirWithIncludeImports() ImageByDirOption { + return func(imageByDirOptions *imageByDirOptions) { + imageByDirOptions.includeImports = true + } +} + +// ImageByDirWithIncludeWellKnownTypes returns a new ImageByDirOption that also +// splits well-known-type imports by directory. +// +// This has no effect if ImageByDirWithIncludeImports is not set. +func ImageByDirWithIncludeWellKnownTypes() ImageByDirOption { + return func(imageByDirOptions *imageByDirOptions) { + imageByDirOptions.includeWellKnownTypes = true + } +} + // ImageToProtoImage returns a new ProtoImage for the Image. func ImageToProtoImage(image Image) (*imagev1.Image, error) { imageFiles := image.Files() @@ -669,6 +709,15 @@ type newImageForProtoOptions struct { computeUnusedImports bool } +type imageByDirOptions struct { + includeImports bool + includeWellKnownTypes bool +} + +func newImageByDirOptions() *imageByDirOptions { + return &imageByDirOptions{} +} + func reparseImageProto(protoImage *imagev1.Image, resolver protoencoding.Resolver, computeUnusedImports bool) error { if err := protoencoding.ReparseExtensions(resolver, protoImage.ProtoReflect()); err != nil { return fmt.Errorf("could not reparse image: %v", err) diff --git a/private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go b/private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go index 5c01fcb909..9355f42790 100644 --- a/private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go +++ b/private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go @@ -770,6 +770,88 @@ func TestBasic(t *testing.T) { diff = cmp.Diff(codeGeneratorRequestsIncludeImports[i], requestsFromImages[i], protocmp.Transform()) require.Empty(t, diff) } + + // ImageByDirWithIncludeWellKnownTypes has no effect without ImageByDirWithIncludeImports. + imagesByDirWellKnownTypesOnly, err := bufimage.ImageByDir(image, bufimage.ImageByDirWithIncludeWellKnownTypes()) + require.NoError(t, err) + require.Equal(t, len(imagesByDir), len(imagesByDirWellKnownTypesOnly)) + for i := range imagesByDir { + AssertImageFilesEqual(t, imagesByDir[i].Files(), imagesByDirWellKnownTypesOnly[i].Files()) + } + + // With ImageByDirWithIncludeImports, the non-well-known-type import gets its own Image + // for its directory, and the other Images are unchanged. + imagesByDirIncludeImports, err := bufimage.ImageByDir(image, bufimage.ImageByDirWithIncludeImports()) + require.NoError(t, err) + require.Equal(t, 4, len(imagesByDirIncludeImports)) + AssertImageFilesEqual( + t, + []bufimage.ImageFile{ + NewImageFile(t, protoImageFileImport, nil, uuid.Nil, "some/import/import.proto", "some/import/import.proto", false, false, nil), + }, + imagesByDirIncludeImports[0].Files(), + ) + for i := range imagesByDir { + AssertImageFilesEqual(t, imagesByDir[i].Files(), imagesByDirIncludeImports[i+1].Files()) + } + // Each file is generated in exactly one request, and each request only generates + // files from a single directory, even with includeImports set. + codeGeneratorRequestsByDirIncludeImports := []*pluginpb.CodeGeneratorRequest{ + { + ProtoFile: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileImport), + }, + Parameter: new("foo"), + FileToGenerate: []string{ + "import.proto", + }, + SourceFileDescriptors: []*descriptorpb.FileDescriptorProto{ + testProtoImageFileToFileDescriptorProto(protoImageFileImport), + }, + }, + codeGeneratorRequests[0], + codeGeneratorRequests[1], + codeGeneratorRequests[2], + } + requestsFromImages, err = bufimage.ImagesToCodeGeneratorRequests(imagesByDirIncludeImports, "foo", nil, true, false) + require.NoError(t, err) + require.Equal(t, len(codeGeneratorRequestsByDirIncludeImports), len(requestsFromImages)) + for i := range codeGeneratorRequestsByDirIncludeImports { + diff = cmp.Diff(codeGeneratorRequestsByDirIncludeImports[i], requestsFromImages[i], protocmp.Transform()) + require.Empty(t, diff) + } + + // With ImageByDirWithIncludeWellKnownTypes as well, the well-known type also gets its own Image. + imagesByDirIncludeImportsAndWellKnownTypes, err := bufimage.ImageByDir( + image, + bufimage.ImageByDirWithIncludeImports(), + bufimage.ImageByDirWithIncludeWellKnownTypes(), + ) + require.NoError(t, err) + require.Equal(t, 5, len(imagesByDirIncludeImportsAndWellKnownTypes)) + for i := range imagesByDirIncludeImports { + AssertImageFilesEqual(t, imagesByDirIncludeImports[i].Files(), imagesByDirIncludeImportsAndWellKnownTypes[i].Files()) + } + AssertImageFilesEqual( + t, + []bufimage.ImageFile{ + NewImageFile(t, protoImageFileWellKnownTypeImport, nil, uuid.Nil, "google/protobuf/timestamp.proto", "", false, false, nil), + }, + imagesByDirIncludeImportsAndWellKnownTypes[4].Files(), + ) + requestsFromImages, err = bufimage.ImagesToCodeGeneratorRequests(imagesByDirIncludeImportsAndWellKnownTypes, "foo", nil, true, true) + require.NoError(t, err) + require.Equal( + t, + [][]string{ + {"import.proto"}, + {"a/a.proto", "a/b.proto"}, + {"b/a.proto", "b/b.proto"}, + {"d/d.proto/d.proto"}, + {"google/protobuf/timestamp.proto"}, + }, + xslices.Map(requestsFromImages, (*pluginpb.CodeGeneratorRequest).GetFileToGenerate), + ) } func TestImageFileInfosWithOnlyTargetsAndTargetImports(t *testing.T) {