From e74c5e2c728be42dd6594be32dd0504a3b8d484d Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Fri, 16 Jan 2026 18:24:38 +0100 Subject: [PATCH 1/5] Add buf source edit deprecate command Adds a new `buf source edit deprecate` command that edits Protobuf source files in-place. The command supports `--name` flags that adds `option deprecated = true;` to types matching the FQN prefix. --- cmd/buf/buf.go | 14 + .../sourceeditdeprecate.go | 271 ++++++++++++++++++ private/buf/bufformat/bufformat.go | 37 ++- private/buf/bufformat/deprecate.go | 161 +++++++++++ private/buf/bufformat/deprecate_test.go | 230 +++++++++++++++ private/buf/bufformat/formatter.go | 194 ++++++++++++- private/buf/bufformat/formatter_test.go | 61 ++++ .../deprecate/already_deprecated.golden | 113 ++++++++ .../deprecate/already_deprecated.proto | 108 +++++++ .../deprecate/enum_value_deprecation.golden | 31 ++ .../deprecate/enum_value_deprecation.proto | 25 ++ .../deprecate/field_deprecation.golden | 12 + .../deprecate/field_deprecation.proto | 9 + .../testdata/deprecate/nested_types.golden | 39 +++ .../testdata/deprecate/nested_types.proto | 30 ++ 15 files changed, 1319 insertions(+), 16 deletions(-) create mode 100644 cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go create mode 100644 private/buf/bufformat/deprecate.go create mode 100644 private/buf/bufformat/deprecate_test.go create mode 100644 private/buf/bufformat/testdata/deprecate/already_deprecated.golden create mode 100644 private/buf/bufformat/testdata/deprecate/already_deprecated.proto create mode 100644 private/buf/bufformat/testdata/deprecate/enum_value_deprecation.golden create mode 100644 private/buf/bufformat/testdata/deprecate/enum_value_deprecation.proto create mode 100644 private/buf/bufformat/testdata/deprecate/field_deprecation.golden create mode 100644 private/buf/bufformat/testdata/deprecate/field_deprecation.proto create mode 100644 private/buf/bufformat/testdata/deprecate/nested_types.golden create mode 100644 private/buf/bufformat/testdata/deprecate/nested_types.proto diff --git a/cmd/buf/buf.go b/cmd/buf/buf.go index ff197379a2..6d87cc3407 100644 --- a/cmd/buf/buf.go +++ b/cmd/buf/buf.go @@ -116,6 +116,7 @@ import ( "github.com/bufbuild/buf/cmd/buf/internal/command/registry/sdk/sdkinfo" "github.com/bufbuild/buf/cmd/buf/internal/command/registry/sdk/version" "github.com/bufbuild/buf/cmd/buf/internal/command/registry/whoami" + "github.com/bufbuild/buf/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate" "github.com/bufbuild/buf/cmd/buf/internal/command/stats" "github.com/bufbuild/buf/private/buf/bufcli" "github.com/bufbuild/buf/private/buf/bufctl" @@ -176,6 +177,19 @@ func newRootCommand(name string) *appcmd.Command { configlsmodules.NewCommand("ls-modules", builder), }, }, + { + Use: "source", + Short: "Work with Protobuf source files", + SubCommands: []*appcmd.Command{ + { + Use: "edit", + Short: "Edit Protobuf source files", + SubCommands: []*appcmd.Command{ + sourceeditdeprecate.NewCommand("deprecate", builder), + }, + }, + }, + }, { Use: "lsp", Short: "Work with Buf Language Server", diff --git a/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go b/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go new file mode 100644 index 0000000000..c873633d3b --- /dev/null +++ b/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go @@ -0,0 +1,271 @@ +// Copyright 2020-2025 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 sourceeditdeprecate + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + + "buf.build/go/app/appcmd" + "buf.build/go/app/appext" + "buf.build/go/standard/xslices" + "buf.build/go/standard/xstrings" + "github.com/bufbuild/buf/private/buf/bufcli" + "github.com/bufbuild/buf/private/buf/bufctl" + "github.com/bufbuild/buf/private/buf/buffetch" + "github.com/bufbuild/buf/private/buf/bufformat" + "github.com/bufbuild/buf/private/bufpkg/bufanalysis" + "github.com/bufbuild/buf/private/bufpkg/bufmodule" + "github.com/bufbuild/buf/private/pkg/storage" + "github.com/spf13/pflag" +) + +const ( + configFlagName = "config" + diffFlagName = "diff" + diffFlagShortName = "d" + disableSymlinksFlagName = "disable-symlinks" + errorFormatFlagName = "error-format" + excludePathsFlagName = "exclude-path" + pathsFlagName = "path" + nameFlagName = "name" +) + +// NewCommand returns a new Command. +func NewCommand( + name string, + builder appext.SubCommandBuilder, +) *appcmd.Command { + flags := newFlags() + return &appcmd.Command{ + Use: name + " ", + Short: "Deprecate Protobuf types", + Long: ` +Deprecate Protobuf types by adding the 'deprecated = true' option. + +The --name flag is required and specifies the fully-qualified name prefix of the +types to deprecate. All types whose fully-qualified name starts with this prefix +will have the 'deprecated = true' option added. For fields and enum values, only +exact matches are deprecated. + +By default, the source is the current directory and files are formatted and rewritten in-place. + +Examples: + +Deprecate all types under a package prefix: + + $ buf source edit deprecate --name foo.bar + +Deprecate a specific message and all nested types: + + $ buf source edit deprecate --name foo.bar.MyMessage + +Deprecate a specific field: + + $ buf source edit deprecate --name foo.bar.MyMessage.my_field + +Multiple --name flags can be specified: + + $ buf source edit deprecate --name foo.bar --name baz.qux + +Display a diff of the changes instead of rewriting files: + + $ buf source edit deprecate --name foo.bar -d +`, + Args: appcmd.MaximumNArgs(1), + Run: builder.NewRunFunc( + func(ctx context.Context, container appext.Container) error { + return run(ctx, container, flags) + }, + ), + BindFlags: flags.Bind, + } +} + +type flags struct { + Config string + Diff bool + DisableSymlinks bool + ErrorFormat string + ExcludePaths []string + Paths []string + Names []string + // special + InputHashtag string +} + +func newFlags() *flags { + return &flags{} +} + +func (f *flags) Bind(flagSet *pflag.FlagSet) { + bufcli.BindInputHashtag(flagSet, &f.InputHashtag) + bufcli.BindPaths(flagSet, &f.Paths, pathsFlagName) + bufcli.BindExcludePaths(flagSet, &f.ExcludePaths, excludePathsFlagName) + bufcli.BindDisableSymlinks(flagSet, &f.DisableSymlinks, disableSymlinksFlagName) + flagSet.BoolVarP( + &f.Diff, + diffFlagName, + diffFlagShortName, + false, + "Display diffs instead of rewriting files", + ) + flagSet.StringVar( + &f.ErrorFormat, + errorFormatFlagName, + "text", + fmt.Sprintf( + "The format for build errors printed to stderr. Must be one of %s", + xstrings.SliceToString(bufanalysis.AllFormatStrings), + ), + ) + flagSet.StringVar( + &f.Config, + configFlagName, + "", + `The buf.yaml file or data to use for configuration`, + ) + flagSet.StringSliceVar( + &f.Names, + nameFlagName, + nil, + `Required. The fully-qualified name prefix of the types to deprecate. May be specified multiple times.`, + ) + _ = appcmd.MarkFlagRequired(flagSet, nameFlagName) +} + +func run( + ctx context.Context, + container appext.Container, + flags *flags, +) (retErr error) { + source, err := bufcli.GetInputValue(container, flags.InputHashtag, ".") + if err != nil { + return err + } + // We use getDirOrProtoFileRef to see if we have a valid DirOrProtoFileRef. + // This is needed to write files in-place. + sourceDirOrProtoFileRef, sourceDirOrProtoFileRefErr := getDirOrProtoFileRef(ctx, container, source) + if sourceDirOrProtoFileRefErr != nil { + if errors.Is(sourceDirOrProtoFileRefErr, buffetch.ErrModuleFormatDetectedForDirOrProtoFileRef) { + return appcmd.NewInvalidArgumentErrorf("invalid input %q: must be a directory or proto file", source) + } + return appcmd.NewInvalidArgumentErrorf("invalid input %q: %v", source, sourceDirOrProtoFileRefErr) + } + if err := validateNoIncludePackageFiles(sourceDirOrProtoFileRef); err != nil { + return err + } + + controller, err := bufcli.NewController( + container, + bufctl.WithDisableSymlinks(flags.DisableSymlinks), + bufctl.WithFileAnnotationErrorFormat(flags.ErrorFormat), + ) + if err != nil { + return err + } + workspace, err := controller.GetWorkspace( + ctx, + source, + bufctl.WithTargetPaths(flags.Paths, flags.ExcludePaths), + bufctl.WithConfigOverride(flags.Config), + ) + if err != nil { + return err + } + moduleReadBucket := bufmodule.ModuleReadBucketWithOnlyTargetFiles( + bufmodule.ModuleSetToModuleReadBucketWithOnlyProtoFilesForTargetModules(workspace), + ) + originalReadBucket := bufmodule.ModuleReadBucketToStorageReadBucket(moduleReadBucket) + + // Build format options from all name prefixes + var formatOpts []bufformat.FormatOption + for _, name := range flags.Names { + formatOpts = append(formatOpts, bufformat.WithDeprecate(name)) + } + + formattedReadBucket, err := bufformat.FormatBucket(ctx, originalReadBucket, formatOpts...) + if err != nil { + return err + } + + diffBuffer := bytes.NewBuffer(nil) + changedPaths, err := storage.DiffWithFilenames( + ctx, + diffBuffer, + originalReadBucket, + formattedReadBucket, + storage.DiffWithExternalPaths(), + ) + if err != nil { + return err + } + diffExists := diffBuffer.Len() > 0 + + if flags.Diff { + if diffExists { + if _, err := io.Copy(container.Stdout(), diffBuffer); err != nil { + return err + } + } + return nil + } + + // Write files in-place (default behavior) + changedPathSet := xslices.ToStructMap(changedPaths) + return storage.WalkReadObjects( + ctx, + formattedReadBucket, + "", + func(readObject storage.ReadObject) error { + if _, ok := changedPathSet[readObject.Path()]; !ok { + // no change, nothing to re-write + return nil + } + file, err := os.OpenFile(readObject.ExternalPath(), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) + if err != nil { + return err + } + defer func() { + retErr = errors.Join(retErr, file.Close()) + }() + if _, err := file.ReadFrom(readObject); err != nil { + return err + } + return nil + }, + ) +} + +func getDirOrProtoFileRef( + ctx context.Context, + container appext.Container, + value string, +) (buffetch.DirOrProtoFileRef, error) { + return buffetch.NewDirOrProtoFileRefParser( + container.Logger(), + ).GetDirOrProtoFileRef(ctx, value) +} + +func validateNoIncludePackageFiles(dirOrProtoFileRef buffetch.DirOrProtoFileRef) error { + if protoFileRef, ok := dirOrProtoFileRef.(buffetch.ProtoFileRef); ok && protoFileRef.IncludePackageFiles() { + return appcmd.NewInvalidArgumentError("cannot specify include_package_files=true with source edit deprecate") + } + return nil +} diff --git a/private/buf/bufformat/bufformat.go b/private/buf/bufformat/bufformat.go index ec72155e03..c888816a5f 100644 --- a/private/buf/bufformat/bufformat.go +++ b/private/buf/bufformat/bufformat.go @@ -28,8 +28,25 @@ import ( "github.com/bufbuild/protocompile/reporter" ) +// FormatOption is an option for formatting. +type FormatOption func(*formatOptions) + +// formatOptions contains options for formatting. +type formatOptions struct { + deprecatePrefixes []string +} + +// WithDeprecate adds a deprecation prefix. All types whose fully-qualified name +// starts with this prefix will have the deprecated option added to them. +// For fields and enum values, only exact matches are deprecated. +func WithDeprecate(fqnPrefix string) FormatOption { + return func(opts *formatOptions) { + opts.deprecatePrefixes = append(opts.deprecatePrefixes, fqnPrefix) + } +} + // FormatModuleSet formats and writes the target files into a read bucket. -func FormatModuleSet(ctx context.Context, moduleSet bufmodule.ModuleSet) (_ storage.ReadBucket, retErr error) { +func FormatModuleSet(ctx context.Context, moduleSet bufmodule.ModuleSet, opts ...FormatOption) (_ storage.ReadBucket, retErr error) { return FormatBucket( ctx, bufmodule.ModuleReadBucketToStorageReadBucket( @@ -37,11 +54,16 @@ func FormatModuleSet(ctx context.Context, moduleSet bufmodule.ModuleSet) (_ stor bufmodule.ModuleSetToModuleReadBucketWithOnlyProtoFilesForTargetModules(moduleSet), ), ), + opts..., ) } // FormatBucket formats the .proto files in the bucket and returns a new bucket with the formatted files. -func FormatBucket(ctx context.Context, bucket storage.ReadBucket) (_ storage.ReadBucket, retErr error) { +func FormatBucket(ctx context.Context, bucket storage.ReadBucket, opts ...FormatOption) (_ storage.ReadBucket, retErr error) { + options := &formatOptions{} + for _, opt := range opts { + opt(options) + } readWriteBucket := storagemem.NewReadWriteBucket() paths, err := storage.AllPaths(ctx, storage.FilterReadBucket(bucket, storage.MatchPathExt(".proto")), "") if err != nil { @@ -68,7 +90,7 @@ func FormatBucket(ctx context.Context, bucket storage.ReadBucket) (_ storage.Rea defer func() { retErr = errors.Join(retErr, writeObjectCloser.Close()) }() - if err := FormatFileNode(writeObjectCloser, fileNode); err != nil { + if err := formatFileNode(writeObjectCloser, fileNode, options); err != nil { return err } return writeObjectCloser.SetExternalPath(readObjectCloser.ExternalPath()) @@ -80,14 +102,19 @@ func FormatBucket(ctx context.Context, bucket storage.ReadBucket) (_ storage.Rea return readWriteBucket, nil } -// FormatFileNode formats the given file node and writ the result to dest. +// FormatFileNode formats the given file node and writes the result to dest. func FormatFileNode(dest io.Writer, fileNode *ast.FileNode) error { + return formatFileNode(dest, fileNode, &formatOptions{}) +} + +// formatFileNode formats the given file node with options and writes the result to dest. +func formatFileNode(dest io.Writer, fileNode *ast.FileNode, options *formatOptions) error { // Construct the file descriptor to ensure the AST is valid. This will // capture unknown syntax like edition "2024" which at the current time is // not supported. if _, err := parser.ResultFromAST(fileNode, true, reporter.NewHandler(nil)); err != nil { return err } - formatter := newFormatter(dest, fileNode) + formatter := newFormatter(dest, fileNode, options) return formatter.Run() } diff --git a/private/buf/bufformat/deprecate.go b/private/buf/bufformat/deprecate.go new file mode 100644 index 0000000000..d2012a5c05 --- /dev/null +++ b/private/buf/bufformat/deprecate.go @@ -0,0 +1,161 @@ +// Copyright 2020-2025 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 bufformat + +import ( + "strings" + + "github.com/bufbuild/protocompile/ast" +) + +// deprecationChecker determines which types should have deprecated options added. +// It is used during formatting to inject deprecation options at appropriate locations. +type deprecationChecker struct { + prefixes [][]string // FQN prefixes split into components +} + +// newDeprecationChecker creates a new deprecationChecker for the given FQN prefixes. +// Each prefix is split by "." into components for matching. +func newDeprecationChecker(fqnPrefixes []string) *deprecationChecker { + prefixes := make([][]string, 0, len(fqnPrefixes)) + for _, prefix := range fqnPrefixes { + if prefix != "" { + prefixes = append(prefixes, strings.Split(prefix, ".")) + } + } + return &deprecationChecker{ + prefixes: prefixes, + } +} + +// isEmpty returns true if there are no deprecation prefixes configured. +func (d *deprecationChecker) isEmpty() bool { + return len(d.prefixes) == 0 +} + +// shouldDeprecate returns true if the given FQN should be deprecated using prefix matching. +// This is used for packages, messages, enums, services, and RPCs. +func (d *deprecationChecker) shouldDeprecate(fqn []string) bool { + for _, prefix := range d.prefixes { + if fqnMatchesPrefix(fqn, prefix) { + return true + } + } + return false +} + +// shouldDeprecateExact returns true if the given FQN matches exactly. +// This is used for fields and enum values which are only deprecated on exact match. +func (d *deprecationChecker) shouldDeprecateExact(fqn []string) bool { + for _, prefix := range d.prefixes { + if fqnMatchesExact(fqn, prefix) { + return true + } + } + return false +} + +// fqnMatchesPrefix returns true if fqn starts with prefix using component-based matching. +// For example, "foo.bar" matches "foo.bar.baz" but "foo.bar.b" does NOT match "foo.bar.baz". +func fqnMatchesPrefix(fqn, prefix []string) bool { + if len(prefix) > len(fqn) { + return false + } + for i, p := range prefix { + if fqn[i] != p { + return false + } + } + return true +} + +// fqnMatchesExact returns true if fqn exactly equals prefix. +func fqnMatchesExact(fqn, prefix []string) bool { + if len(fqn) != len(prefix) { + return false + } + for i, p := range prefix { + if fqn[i] != p { + return false + } + } + return true +} + +// hasDeprecatedOption checks if a slice of declarations contains a deprecated = true option. +func hasDeprecatedOption[T any](decls []T) bool { + for _, decl := range decls { + if opt, ok := any(decl).(*ast.OptionNode); ok && isDeprecatedOptionNode(opt) { + return true + } + } + return false +} + +// hasCompactDeprecatedOption checks if a CompactOptionsNode contains deprecated = true. +func hasCompactDeprecatedOption(opts *ast.CompactOptionsNode) bool { + if opts == nil { + return false + } + for _, opt := range opts.Options { + if isDeprecatedOptionNode(opt) { + return true + } + } + return false +} + +// isDeprecatedOptionNode checks if an option node is "deprecated = true". +func isDeprecatedOptionNode(opt *ast.OptionNode) bool { + if opt.Name == nil || len(opt.Name.Parts) != 1 { + return false + } + part := opt.Name.Parts[0] + if part.Name == nil { + return false + } + // Get the identifier value + var name string + switch n := part.Name.(type) { + case *ast.IdentNode: + name = n.Val + default: + return false + } + if name != "deprecated" { + return false + } + // Check value is "true" + if ident, ok := opt.Val.(*ast.IdentNode); ok { + return ident.Val == "true" + } + return false +} + +// packageNameToComponents extracts package name components from an identifier node. +func packageNameToComponents(name ast.IdentValueNode) []string { + switch n := name.(type) { + case *ast.IdentNode: + return []string{n.Val} + case *ast.CompoundIdentNode: + components := make([]string, len(n.Components)) + for i, comp := range n.Components { + components[i] = comp.Val + } + return components + default: + return nil + } +} diff --git a/private/buf/bufformat/deprecate_test.go b/private/buf/bufformat/deprecate_test.go new file mode 100644 index 0000000000..a63bc0f2ba --- /dev/null +++ b/private/buf/bufformat/deprecate_test.go @@ -0,0 +1,230 @@ +// Copyright 2020-2025 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 bufformat + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFQNMatchesPrefix(t *testing.T) { + t.Parallel() + tests := []struct { + name string + fqn []string + prefix []string + expected bool + }{ + { + name: "exact match", + fqn: []string{"foo", "bar", "baz"}, + prefix: []string{"foo", "bar", "baz"}, + expected: true, + }, + { + name: "prefix match", + fqn: []string{"foo", "bar", "baz"}, + prefix: []string{"foo", "bar"}, + expected: true, + }, + { + name: "single component prefix", + fqn: []string{"foo", "bar", "baz"}, + prefix: []string{"foo"}, + expected: true, + }, + { + name: "empty prefix matches all", + fqn: []string{"foo", "bar"}, + prefix: []string{}, + expected: true, + }, + { + name: "prefix longer than fqn", + fqn: []string{"foo", "bar"}, + prefix: []string{"foo", "bar", "baz"}, + expected: false, + }, + { + name: "partial component mismatch - foo.bar.b does not match foo.bar.baz", + fqn: []string{"foo", "bar", "baz"}, + prefix: []string{"foo", "bar", "b"}, + expected: false, + }, + { + name: "different path", + fqn: []string{"foo", "bar", "baz"}, + prefix: []string{"foo", "qux"}, + expected: false, + }, + { + name: "empty fqn", + fqn: []string{}, + prefix: []string{"foo"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := fqnMatchesPrefix(tt.fqn, tt.prefix) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestFQNMatchesExact(t *testing.T) { + t.Parallel() + tests := []struct { + name string + fqn []string + prefix []string + expected bool + }{ + { + name: "exact match", + fqn: []string{"foo", "bar", "baz"}, + prefix: []string{"foo", "bar", "baz"}, + expected: true, + }, + { + name: "prefix only - not exact", + fqn: []string{"foo", "bar", "baz"}, + prefix: []string{"foo", "bar"}, + expected: false, + }, + { + name: "longer prefix - not exact", + fqn: []string{"foo", "bar"}, + prefix: []string{"foo", "bar", "baz"}, + expected: false, + }, + { + name: "both empty", + fqn: []string{}, + prefix: []string{}, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := fqnMatchesExact(tt.fqn, tt.prefix) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestDeprecationCheckerShouldDeprecate(t *testing.T) { + t.Parallel() + checker := newDeprecationChecker([]string{"foo.bar", "baz.qux"}) + + tests := []struct { + name string + fqn []string + expected bool + }{ + { + name: "matches first prefix", + fqn: []string{"foo", "bar", "baz"}, + expected: true, + }, + { + name: "matches second prefix", + fqn: []string{"baz", "qux", "quux"}, + expected: true, + }, + { + name: "exact match on prefix", + fqn: []string{"foo", "bar"}, + expected: true, + }, + { + name: "no match", + fqn: []string{"other", "package"}, + expected: false, + }, + { + name: "partial component no match", + fqn: []string{"foo", "bart"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := checker.shouldDeprecate(tt.fqn) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestDeprecationCheckerShouldDeprecateExact(t *testing.T) { + t.Parallel() + checker := newDeprecationChecker([]string{"foo.bar.baz.SomeMessage.some_field"}) + + tests := []struct { + name string + fqn []string + expected bool + }{ + { + name: "exact match", + fqn: []string{"foo", "bar", "baz", "SomeMessage", "some_field"}, + expected: true, + }, + { + name: "prefix only - not exact", + fqn: []string{"foo", "bar", "baz", "SomeMessage"}, + expected: false, + }, + { + name: "longer than prefix", + fqn: []string{"foo", "bar", "baz", "SomeMessage", "some_field", "extra"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := checker.shouldDeprecateExact(tt.fqn) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestDeprecationCheckerEmptyPrefixes(t *testing.T) { + t.Parallel() + checker := newDeprecationChecker([]string{}) + + // Empty prefixes should not match anything + assert.True(t, checker.isEmpty()) + assert.False(t, checker.shouldDeprecate([]string{"foo", "bar"})) + assert.False(t, checker.shouldDeprecateExact([]string{"foo", "bar"})) +} + +func TestDeprecationCheckerEmptyStringsFiltered(t *testing.T) { + t.Parallel() + checker := newDeprecationChecker([]string{"", "foo.bar", ""}) + + // Empty strings should be filtered out + assert.False(t, checker.isEmpty()) + assert.True(t, checker.shouldDeprecate([]string{"foo", "bar", "baz"})) +} diff --git a/private/buf/bufformat/formatter.go b/private/buf/bufformat/formatter.go index 6711169c7a..758da04937 100644 --- a/private/buf/bufformat/formatter.go +++ b/private/buf/bufformat/formatter.go @@ -68,18 +68,31 @@ type formatter struct { // Records all errors that occur during the formatting process. Nearly any // non-nil error represents a bug in the implementation. err error + + // deprecation tracks which types should have deprecated options injected. + deprecation *deprecationChecker + // packageFQN holds the current package's fully-qualified name components. + packageFQN []string + // injectCompactDeprecation is set when the next compact options should have + // deprecated = true injected at the beginning. + injectCompactDeprecation bool } // newFormatter returns a new formatter for the given file. func newFormatter( writer io.Writer, fileNode *ast.FileNode, + options *formatOptions, ) *formatter { - return &formatter{ + f := &formatter{ writer: writer, fileNode: fileNode, overrideTrailingComments: map[ast.Node]ast.Comments{}, } + if options != nil && len(options.deprecatePrefixes) > 0 { + f.deprecation = newDeprecationChecker(options.deprecatePrefixes) + } + return f } // Run runs the formatter and writes the file's content to the formatter's writer. @@ -247,6 +260,10 @@ func (f *formatter) writeFileHeader() { continue } } + // Extract package FQN for deprecation tracking + if packageNode != nil { + f.packageFQN = packageNameToComponents(packageNode.Name) + } if f.fileNode.Syntax == nil && f.fileNode.Edition == nil && packageNode == nil && importNodes == nil && optionNodes == nil { // There aren't any header values, so we can return early. @@ -331,6 +348,13 @@ func (f *formatter) writeFileHeader() { f.writeFileOption(optionNode, i > 0, first) first = false } + // Inject file-level deprecated option if needed + if f.shouldInjectDeprecation(f.packageFQN) && !hasDeprecatedOption(optionNodes) { + if len(optionNodes) == 0 && f.previousNode != nil { + f.P("") + } + f.writeDeprecatedOption() + } } // writeFileTypes writes the types defined in a .proto file. This includes the messages, enums, @@ -577,13 +601,27 @@ func (f *formatter) writeOptionName(optionNameNode *ast.OptionNameNode) { // Baz baz = 2; // } func (f *formatter) writeMessage(messageNode *ast.MessageNode) { + // Track FQN for deprecation + popFQN := f.pushFQN(messageNode.Name.Val) + defer popFQN() + var elementWriterFunc func() if len(messageNode.Decls) != 0 { + // Check if we need to inject deprecation + needsDeprecation := f.shouldInjectDeprecation(f.currentFQN()) && !hasDeprecatedOption(messageNode.Decls) elementWriterFunc = func() { + if needsDeprecation { + f.writeDeprecatedOption() + } for _, decl := range messageNode.Decls { f.writeNode(decl) } } + } else if f.shouldInjectDeprecation(f.currentFQN()) { + // Empty message that needs deprecation + elementWriterFunc = func() { + f.writeDeprecatedOption() + } } f.writeStart(messageNode.Keyword, false) f.Space() @@ -848,13 +886,27 @@ func (f *formatter) writeMessageFieldPrefix(messageFieldNode *ast.MessageFieldNo // FOO_UNSPECIFIED = 0; // } func (f *formatter) writeEnum(enumNode *ast.EnumNode) { + // Track FQN for deprecation + popFQN := f.pushFQN(enumNode.Name.Val) + defer popFQN() + var elementWriterFunc func() if len(enumNode.Decls) > 0 { + // Check if we need to inject deprecation + needsDeprecation := f.shouldInjectDeprecation(f.currentFQN()) && !hasDeprecatedOption(enumNode.Decls) elementWriterFunc = func() { + if needsDeprecation { + f.writeDeprecatedOption() + } for _, decl := range enumNode.Decls { f.writeNode(decl) } } + } else if f.shouldInjectDeprecation(f.currentFQN()) { + // Empty enum that needs deprecation + elementWriterFunc = func() { + f.writeDeprecatedOption() + } } f.writeStart(enumNode.Keyword, false) f.Space() @@ -881,9 +933,22 @@ func (f *formatter) writeEnumValue(enumValueNode *ast.EnumValueNode) { f.writeInline(enumValueNode.Equals) f.Space() f.writeInline(enumValueNode.Number) + // Check if we need to inject deprecation for this enum value (exact match only) + // Enum values are scoped to their parent (message or package), NOT the enum itself. + // So we use the parent FQN (without the enum name) + the enum value name. + parentFQN := f.packageFQN[:len(f.packageFQN)-1] + enumValueFQN := append(parentFQN[:len(parentFQN):len(parentFQN)], enumValueNode.Name.Val) + needsDeprecation := f.shouldInjectDeprecationExact(enumValueFQN) && + !hasCompactDeprecatedOption(enumValueNode.Options) if enumValueNode.Options != nil { f.Space() + if needsDeprecation { + f.injectCompactDeprecation = true + } f.writeNode(enumValueNode.Options) + f.injectCompactDeprecation = false + } else if needsDeprecation { + f.writeCompactDeprecatedOption() } f.writeLineEnd(enumValueNode.Semicolon) } @@ -924,9 +989,19 @@ func (f *formatter) writeField(fieldNode *ast.FieldNode) { if fieldNode.Tag != nil { f.writeInline(fieldNode.Tag) } + // Check if we need to inject deprecation for this field (exact match only) + fieldFQN := append(f.currentFQN(), fieldNode.Name.Val) + needsDeprecation := f.shouldInjectDeprecationExact(fieldFQN) && + !hasCompactDeprecatedOption(fieldNode.Options) if fieldNode.Options != nil { f.Space() + if needsDeprecation { + f.injectCompactDeprecation = true + } f.writeNode(fieldNode.Options) + f.injectCompactDeprecation = false + } else if needsDeprecation { + f.writeCompactDeprecatedOption() } f.writeLineEnd(fieldNode.Semicolon) } @@ -1005,13 +1080,27 @@ func (f *formatter) writeExtend(extendNode *ast.ExtendNode) { // // rpc Foo(FooRequest) returns (FooResponse) {}; func (f *formatter) writeService(serviceNode *ast.ServiceNode) { + // Track FQN for deprecation + popFQN := f.pushFQN(serviceNode.Name.Val) + defer popFQN() + var elementWriterFunc func() if len(serviceNode.Decls) > 0 { + // Check if we need to inject deprecation + needsDeprecation := f.shouldInjectDeprecation(f.currentFQN()) && !hasDeprecatedOption(serviceNode.Decls) elementWriterFunc = func() { + if needsDeprecation { + f.writeDeprecatedOption() + } for _, decl := range serviceNode.Decls { f.writeNode(decl) } } + } else if f.shouldInjectDeprecation(f.currentFQN()) { + // Empty service that needs deprecation + elementWriterFunc = func() { + f.writeDeprecatedOption() + } } f.writeStart(serviceNode.Keyword, false) f.Space() @@ -1033,13 +1122,26 @@ func (f *formatter) writeService(serviceNode *ast.ServiceNode) { // option deprecated = true; // }; func (f *formatter) writeRPC(rpcNode *ast.RPCNode) { + // Track FQN for deprecation (RPCs are children of services) + popFQN := f.pushFQN(rpcNode.Name.Val) + defer popFQN() + + needsDeprecation := f.shouldInjectDeprecation(f.currentFQN()) && !hasDeprecatedOption(rpcNode.Decls) + var elementWriterFunc func() if len(rpcNode.Decls) > 0 { elementWriterFunc = func() { + if needsDeprecation { + f.writeDeprecatedOption() + } for _, decl := range rpcNode.Decls { f.writeNode(decl) } } + } else if needsDeprecation { + elementWriterFunc = func() { + f.writeDeprecatedOption() + } } f.writeStart(rpcNode.Keyword, false) f.Space() @@ -1049,21 +1151,37 @@ func (f *formatter) writeRPC(rpcNode *ast.RPCNode) { f.writeInline(rpcNode.Returns) f.Space() f.writeInline(rpcNode.Output) - if rpcNode.OpenBrace == nil { - // This RPC doesn't have any elements, so we prefer the - // ';' form. + if rpcNode.OpenBrace == nil && !needsDeprecation { + // This RPC doesn't have any elements and doesn't need deprecation, + // so we prefer the ';' form. // // rpc Ping(PingRequest) returns (PingResponse); // f.writeLineEnd(rpcNode.Semicolon) return } + // If we need to add deprecation to an RPC without a body, we need to + // create a body for it. f.Space() - f.writeCompositeTypeBody( - rpcNode.OpenBrace, - rpcNode.CloseBrace, - elementWriterFunc, - ) + if rpcNode.OpenBrace != nil { + f.writeCompositeTypeBody( + rpcNode.OpenBrace, + rpcNode.CloseBrace, + elementWriterFunc, + ) + } else { + // RPC had no body but needs deprecation - create synthetic body + f.WriteString("{") + f.P("") + f.In() + if elementWriterFunc != nil { + elementWriterFunc() + } + f.Out() + f.Indent(nil) + f.WriteString("}") + f.P("") + } } // writeRPCType writes the RPC type node (e.g. (stream foo.Bar)). @@ -1248,7 +1366,9 @@ func (f *formatter) writeCompactOptions(compactOptionsNode *ast.CompactOptionsNo defer func() { f.inCompactOptions = false }() - if len(compactOptionsNode.Options) == 1 && + // If we need to inject deprecation, we must use multiline format + injectDeprecation := f.injectCompactDeprecation + if len(compactOptionsNode.Options) == 1 && !injectDeprecation && !f.hasInteriorComments(compactOptionsNode.OpenBracket, compactOptionsNode.Options[0].Name) { // If there's only a single compact scalar option without comments, we can write it // in-line. For example: @@ -1282,8 +1402,16 @@ func (f *formatter) writeCompactOptions(compactOptionsNode *ast.CompactOptionsNo return } var elementWriterFunc func() - if len(compactOptionsNode.Options) > 0 { + if len(compactOptionsNode.Options) > 0 || injectDeprecation { elementWriterFunc = func() { + // If we need to inject deprecation, write it first + if injectDeprecation { + if len(compactOptionsNode.Options) > 0 { + f.P("deprecated = true,") + } else { + f.P("deprecated = true") + } + } for i, opt := range compactOptionsNode.Options { if i == len(compactOptionsNode.Options)-1 { // The last element won't have a trailing comma. @@ -2491,3 +2619,47 @@ func messageLiteralClose(msg *ast.MessageLiteralNode) *ast.RuneNode { // For consistent formatted output, change it to "}". return ast.NewRuneNode('}', node.Token()) } + +// writeDeprecatedOption writes "option deprecated = true;" on its own line. +// This is used to inject deprecation options for types that should be deprecated. +func (f *formatter) writeDeprecatedOption() { + f.Indent(nil) + f.WriteString("option deprecated = true;") + f.P("") +} + +// currentFQN returns the current fully-qualified name by combining package and type path. +func (f *formatter) currentFQN() []string { + return f.packageFQN +} + +// pushFQN appends a name component to the current FQN and returns a function to restore it. +func (f *formatter) pushFQN(name string) func() { + originalLen := len(f.packageFQN) + f.packageFQN = append(f.packageFQN, name) + return func() { + f.packageFQN = f.packageFQN[:originalLen] + } +} + +// shouldInjectDeprecation returns true if the given FQN should have a deprecated option injected. +func (f *formatter) shouldInjectDeprecation(fqn []string) bool { + if f.deprecation == nil || f.deprecation.isEmpty() { + return false + } + return f.deprecation.shouldDeprecate(fqn) +} + +// shouldInjectDeprecationExact returns true if the given FQN should have a deprecated option +// injected using exact matching (for fields and enum values). +func (f *formatter) shouldInjectDeprecationExact(fqn []string) bool { + if f.deprecation == nil || f.deprecation.isEmpty() { + return false + } + return f.deprecation.shouldDeprecateExact(fqn) +} + +// writeCompactDeprecatedOption writes " [deprecated = true]" for compact options. +func (f *formatter) writeCompactDeprecatedOption() { + f.WriteString(" [deprecated = true]") +} diff --git a/private/buf/bufformat/formatter_test.go b/private/buf/bufformat/formatter_test.go index 5b5c592774..e3cced876c 100644 --- a/private/buf/bufformat/formatter_test.go +++ b/private/buf/bufformat/formatter_test.go @@ -130,3 +130,64 @@ func testFormatError(t *testing.T, path string, errContains string) { require.ErrorContains(t, err, errContains) }) } + +func TestFormatterWithDeprecation(t *testing.T) { + t.Parallel() + // Test basic deprecation with prefix matching + testDeprecateNoDiff(t, "basic", "testdata/deprecate", []string{"test.deprecate"}, + []string{"already_deprecated.proto", "nested_types.proto"}) + // Test field deprecation with exact match + testDeprecateNoDiff(t, "field", "testdata/deprecate", []string{"test.deprecate", "test.deprecate.MyMessage.id"}, + []string{"field_deprecation.proto"}) + // Test enum value deprecation with exact match + testDeprecateNoDiff(t, "enum_value", "testdata/deprecate", []string{ + "test.deprecate", + "test.deprecate.STATUS_ACTIVE", + "test.deprecate.STATUS_INACTIVE", + "test.deprecate.OuterMessage.NESTED_STATUS_ACTIVE", + }, []string{"enum_value_deprecation.proto"}) +} + +func testDeprecateNoDiff(t *testing.T, name string, path string, deprecatePrefixes []string, files []string) { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + bucket, err := storageos.NewProvider().NewReadWriteBucket(path) + require.NoError(t, err) + var opts []FormatOption + for _, prefix := range deprecatePrefixes { + opts = append(opts, WithDeprecate(prefix)) + } + var matchers []storage.Matcher + for _, file := range files { + matchers = append(matchers, storage.MatchPathEqual(file)) + } + filteredBucket := storage.FilterReadBucket(bucket, storage.MatchOr(matchers...)) + assertGolden := func(formatBucket storage.ReadBucket) { + err := storage.WalkReadObjects( + ctx, + formatBucket, + "", + func(formattedFile storage.ReadObject) error { + formattedData, err := io.ReadAll(formattedFile) + require.NoError(t, err) + expectedPath := strings.Replace(formattedFile.Path(), ".proto", ".golden", 1) + expectedData, err := storage.ReadPath(ctx, bucket, expectedPath) + require.NoError(t, err) + fileDiff, err := diff.Diff(ctx, expectedData, formattedData, expectedPath, formattedFile.Path()+" (formatted)") + require.NoError(t, err) + require.Empty(t, string(fileDiff), "formatted output differs from golden file for %s", formattedFile.Path()) + return nil + }, + ) + require.NoError(t, err) + } + // First pass: format with deprecation options + formatBucket, err := FormatBucket(ctx, filteredBucket, opts...) + require.NoError(t, err) + assertGolden(formatBucket) + // Second pass: re-format the already-formatted output to verify stability + reformatBucket, err := FormatBucket(ctx, formatBucket, opts...) + require.NoError(t, err) + assertGolden(reformatBucket) + }) +} diff --git a/private/buf/bufformat/testdata/deprecate/already_deprecated.golden b/private/buf/bufformat/testdata/deprecate/already_deprecated.golden new file mode 100644 index 0000000000..59c1cc17da --- /dev/null +++ b/private/buf/bufformat/testdata/deprecate/already_deprecated.golden @@ -0,0 +1,113 @@ +syntax = "proto2"; + +package test.deprecate; + +option cc_enable_arenas = true; +// File already has deprecated option - should not add duplicate. +option deprecated = true; + +// Message already deprecated - should not add duplicate. +message AlreadyDeprecatedMessage { + option deprecated = true; + optional string name = 1; +} + +// Message with other options but not deprecated - should add deprecated. +message MessageWithOtherOptions { + option deprecated = true; + option no_standard_descriptor_accessor = true; + optional string name = 1; +} + +// Message with deprecated and other options - should not add duplicate. +message MessageWithDeprecatedAndOtherOptions { + option deprecated = true; + option no_standard_descriptor_accessor = true; + optional string name = 1; +} + +// Message to test field deprecation. +message MessageWithFields { + option deprecated = true; + // Field already deprecated - should not add duplicate. + optional string already_deprecated_field = 1 [deprecated = true]; + // Field with other options but not deprecated - stays as is (not in exact match list). + optional NestedMessage field_with_other_options = 2 [lazy = true]; + // Field with deprecated and other options - should not add duplicate. + optional NestedMessage field_with_deprecated_and_other_options = 3 [ + deprecated = true, + lazy = true + ]; + // Nested message for lazy fields. + message NestedMessage { + option deprecated = true; + optional string value = 1; + } +} + +// Enum already deprecated - should not add duplicate. +enum AlreadyDeprecatedEnum { + option deprecated = true; + ALREADY_DEPRECATED_ENUM_UNSPECIFIED = 0; + ALREADY_DEPRECATED_ENUM_VALUE = 1; +} + +// Enum with other options but not deprecated - should add deprecated. +enum EnumWithOtherOptions { + option deprecated = true; + option allow_alias = true; + ENUM_WITH_OTHER_OPTIONS_UNSPECIFIED = 0; + ENUM_WITH_OTHER_OPTIONS_VALUE = 1; + ENUM_WITH_OTHER_OPTIONS_ALIAS = 1; +} + +// Enum with deprecated and other options - should not add duplicate. +enum EnumWithDeprecatedAndOtherOptions { + option deprecated = true; + option allow_alias = true; + ENUM_WITH_DEPRECATED_AND_OTHER_OPTIONS_UNSPECIFIED = 0; + ENUM_WITH_DEPRECATED_AND_OTHER_OPTIONS_VALUE = 1; + ENUM_WITH_DEPRECATED_AND_OTHER_OPTIONS_ALIAS = 1; +} + +// Enum to test enum value deprecation. +enum EnumWithValues { + option deprecated = true; + ENUM_WITH_VALUES_UNSPECIFIED = 0; + // Enum value already deprecated - should not add duplicate. + ENUM_WITH_VALUES_ALREADY_DEPRECATED = 1 [deprecated = true]; + // Enum value with other options - stays as is (not in exact match list). + ENUM_WITH_VALUES_WITH_OTHER_OPTIONS = 2 [debug_redact = true]; + // Enum value with deprecated and other options - should not add duplicate. + ENUM_WITH_VALUES_WITH_DEPRECATED_AND_OTHER = 3 [ + deprecated = true, + debug_redact = true + ]; +} + +// Service already deprecated - should not add duplicate. +service AlreadyDeprecatedService { + option deprecated = true; + rpc GetData(AlreadyDeprecatedMessage) returns (AlreadyDeprecatedMessage) { + option deprecated = true; + } +} + +// Service with other methods to test method deprecation. +service ServiceWithMethods { + option deprecated = true; + // Method already deprecated - should not add duplicate. + rpc AlreadyDeprecatedMethod(AlreadyDeprecatedMessage) returns (AlreadyDeprecatedMessage) { + option deprecated = true; + } + // Method with other options but not deprecated - should add deprecated. + rpc MethodWithOtherOptions(AlreadyDeprecatedMessage) returns (AlreadyDeprecatedMessage) { + option deprecated = true; + option idempotency_level = IDEMPOTENT; + } + // Method with deprecated and other options - should not add duplicate. + rpc MethodWithDeprecatedAndOtherOptions(AlreadyDeprecatedMessage) returns (AlreadyDeprecatedMessage) { + option deprecated = true; + option idempotency_level = IDEMPOTENT; + } +} diff --git a/private/buf/bufformat/testdata/deprecate/already_deprecated.proto b/private/buf/bufformat/testdata/deprecate/already_deprecated.proto new file mode 100644 index 0000000000..786a70ae43 --- /dev/null +++ b/private/buf/bufformat/testdata/deprecate/already_deprecated.proto @@ -0,0 +1,108 @@ +syntax = "proto2"; + +package test.deprecate; + +// File already has deprecated option - should not add duplicate. +option deprecated = true; +option cc_enable_arenas = true; + +// Message already deprecated - should not add duplicate. +message AlreadyDeprecatedMessage { + option deprecated = true; + optional string name = 1; +} + +// Message with other options but not deprecated - should add deprecated. +message MessageWithOtherOptions { + option no_standard_descriptor_accessor = true; + optional string name = 1; +} + +// Message with deprecated and other options - should not add duplicate. +message MessageWithDeprecatedAndOtherOptions { + option deprecated = true; + option no_standard_descriptor_accessor = true; + optional string name = 1; +} + +// Message to test field deprecation. +message MessageWithFields { + option deprecated = true; + // Field already deprecated - should not add duplicate. + optional string already_deprecated_field = 1 [deprecated = true]; + // Field with other options but not deprecated - stays as is (not in exact match list). + optional NestedMessage field_with_other_options = 2 [lazy = true]; + // Field with deprecated and other options - should not add duplicate. + optional NestedMessage field_with_deprecated_and_other_options = 3 [ + deprecated = true, + lazy = true + ]; + // Nested message for lazy fields. + message NestedMessage { + option deprecated = true; + optional string value = 1; + } +} + +// Enum already deprecated - should not add duplicate. +enum AlreadyDeprecatedEnum { + option deprecated = true; + ALREADY_DEPRECATED_ENUM_UNSPECIFIED = 0; + ALREADY_DEPRECATED_ENUM_VALUE = 1; +} + +// Enum with other options but not deprecated - should add deprecated. +enum EnumWithOtherOptions { + option allow_alias = true; + ENUM_WITH_OTHER_OPTIONS_UNSPECIFIED = 0; + ENUM_WITH_OTHER_OPTIONS_VALUE = 1; + ENUM_WITH_OTHER_OPTIONS_ALIAS = 1; +} + +// Enum with deprecated and other options - should not add duplicate. +enum EnumWithDeprecatedAndOtherOptions { + option deprecated = true; + option allow_alias = true; + ENUM_WITH_DEPRECATED_AND_OTHER_OPTIONS_UNSPECIFIED = 0; + ENUM_WITH_DEPRECATED_AND_OTHER_OPTIONS_VALUE = 1; + ENUM_WITH_DEPRECATED_AND_OTHER_OPTIONS_ALIAS = 1; +} + +// Enum to test enum value deprecation. +enum EnumWithValues { + option deprecated = true; + ENUM_WITH_VALUES_UNSPECIFIED = 0; + // Enum value already deprecated - should not add duplicate. + ENUM_WITH_VALUES_ALREADY_DEPRECATED = 1 [deprecated = true]; + // Enum value with other options - stays as is (not in exact match list). + ENUM_WITH_VALUES_WITH_OTHER_OPTIONS = 2 [debug_redact = true]; + // Enum value with deprecated and other options - should not add duplicate. + ENUM_WITH_VALUES_WITH_DEPRECATED_AND_OTHER = 3 [ + deprecated = true, + debug_redact = true + ]; +} + +// Service already deprecated - should not add duplicate. +service AlreadyDeprecatedService { + option deprecated = true; + rpc GetData(AlreadyDeprecatedMessage) returns (AlreadyDeprecatedMessage); +} + +// Service with other methods to test method deprecation. +service ServiceWithMethods { + option deprecated = true; + // Method already deprecated - should not add duplicate. + rpc AlreadyDeprecatedMethod(AlreadyDeprecatedMessage) returns (AlreadyDeprecatedMessage) { + option deprecated = true; + } + // Method with other options but not deprecated - should add deprecated. + rpc MethodWithOtherOptions(AlreadyDeprecatedMessage) returns (AlreadyDeprecatedMessage) { + option idempotency_level = IDEMPOTENT; + } + // Method with deprecated and other options - should not add duplicate. + rpc MethodWithDeprecatedAndOtherOptions(AlreadyDeprecatedMessage) returns (AlreadyDeprecatedMessage) { + option deprecated = true; + option idempotency_level = IDEMPOTENT; + } +} diff --git a/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.golden b/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.golden new file mode 100644 index 0000000000..d9a393f991 --- /dev/null +++ b/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.golden @@ -0,0 +1,31 @@ +syntax = "proto2"; + +package test.deprecate; + +import "google/protobuf/descriptor.proto"; + +option deprecated = true; + +extend google.protobuf.EnumValueOptions { + optional string string_name = 123456789; +} + +enum Status { + option deprecated = true; + STATUS_UNSPECIFIED = 0; + STATUS_ACTIVE = 1 [deprecated = true]; + STATUS_INACTIVE = 2 [ + deprecated = true, + (string_name) = "display_value" + ]; +} + +// Message with nested enum to test nested enum value FQN. +message OuterMessage { + option deprecated = true; + enum NestedStatus { + option deprecated = true; + NESTED_STATUS_UNSPECIFIED = 0; + NESTED_STATUS_ACTIVE = 1 [deprecated = true]; + } +} diff --git a/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.proto b/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.proto new file mode 100644 index 0000000000..e9f1183ea3 --- /dev/null +++ b/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.proto @@ -0,0 +1,25 @@ +syntax = "proto2"; + +package test.deprecate; + +import "google/protobuf/descriptor.proto"; + +extend google.protobuf.EnumValueOptions { + optional string string_name = 123456789; +} + +enum Status { + STATUS_UNSPECIFIED = 0; + STATUS_ACTIVE = 1; + STATUS_INACTIVE = 2 [ + (string_name) = "display_value" + ]; +} + +// Message with nested enum to test nested enum value FQN. +message OuterMessage { + enum NestedStatus { + NESTED_STATUS_UNSPECIFIED = 0; + NESTED_STATUS_ACTIVE = 1; + } +} diff --git a/private/buf/bufformat/testdata/deprecate/field_deprecation.golden b/private/buf/bufformat/testdata/deprecate/field_deprecation.golden new file mode 100644 index 0000000000..590d86e84d --- /dev/null +++ b/private/buf/bufformat/testdata/deprecate/field_deprecation.golden @@ -0,0 +1,12 @@ +syntax = "proto3"; + +package test.deprecate; + +option deprecated = true; + +message MyMessage { + option deprecated = true; + string name = 1; + int32 id = 2 [deprecated = true]; + bool active = 3; +} diff --git a/private/buf/bufformat/testdata/deprecate/field_deprecation.proto b/private/buf/bufformat/testdata/deprecate/field_deprecation.proto new file mode 100644 index 0000000000..6397dd328b --- /dev/null +++ b/private/buf/bufformat/testdata/deprecate/field_deprecation.proto @@ -0,0 +1,9 @@ +syntax = "proto3"; + +package test.deprecate; + +message MyMessage { + string name = 1; + int32 id = 2; + bool active = 3; +} diff --git a/private/buf/bufformat/testdata/deprecate/nested_types.golden b/private/buf/bufformat/testdata/deprecate/nested_types.golden new file mode 100644 index 0000000000..500fc7c531 --- /dev/null +++ b/private/buf/bufformat/testdata/deprecate/nested_types.golden @@ -0,0 +1,39 @@ +syntax = "proto3"; + +package test.deprecate; + +option deprecated = true; + +// Outer message - should be deprecated. +message OuterMessage { + option deprecated = true; + string name = 1; + + // Nested message - should also be deprecated. + message NestedMessage { + option deprecated = true; + string value = 1; + } + + // Nested enum - should also be deprecated. + enum NestedEnum { + option deprecated = true; + NESTED_ENUM_UNSPECIFIED = 0; + NESTED_ENUM_VALUE = 1; + } +} + +// Top-level enum - should be deprecated. +enum TopLevelEnum { + option deprecated = true; + TOP_LEVEL_ENUM_UNSPECIFIED = 0; + TOP_LEVEL_ENUM_VALUE = 1; +} + +// Service - should be deprecated. +service TestService { + option deprecated = true; + rpc GetMessage(OuterMessage) returns (OuterMessage) { + option deprecated = true; + } +} diff --git a/private/buf/bufformat/testdata/deprecate/nested_types.proto b/private/buf/bufformat/testdata/deprecate/nested_types.proto new file mode 100644 index 0000000000..ce03c3e12f --- /dev/null +++ b/private/buf/bufformat/testdata/deprecate/nested_types.proto @@ -0,0 +1,30 @@ +syntax = "proto3"; + +package test.deprecate; + +// Outer message - should be deprecated. +message OuterMessage { + string name = 1; + + // Nested message - should also be deprecated. + message NestedMessage { + string value = 1; + } + + // Nested enum - should also be deprecated. + enum NestedEnum { + NESTED_ENUM_UNSPECIFIED = 0; + NESTED_ENUM_VALUE = 1; + } +} + +// Top-level enum - should be deprecated. +enum TopLevelEnum { + TOP_LEVEL_ENUM_UNSPECIFIED = 0; + TOP_LEVEL_ENUM_VALUE = 1; +} + +// Service - should be deprecated. +service TestService { + rpc GetMessage(OuterMessage) returns (OuterMessage); +} From d1449597901565b614922a29f62a5bd26d894aef Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Mon, 26 Jan 2026 23:21:35 +0100 Subject: [PATCH 2/5] CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1867398c2f..b2f51552a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## [Unreleased] - Add `buf registry policy {commit,create,delete,info,label,settings}` commands to manage BSR policies. +- Add `buf source edit deprecate` command to deprecate types by setting the `deprecate = true` option. ## [v1.64.0] - 2026-01-19 From 0c448dda2d2ee4794a9e6dbf075c60d351c2352b Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Tue, 27 Jan 2026 23:17:39 +0100 Subject: [PATCH 3/5] Cleanup matcher --- private/buf/bufformat/deprecate.go | 84 ++++++--------- private/buf/bufformat/deprecate_test.go | 129 +++++++----------------- private/buf/bufformat/formatter.go | 41 ++++---- 3 files changed, 89 insertions(+), 165 deletions(-) diff --git a/private/buf/bufformat/deprecate.go b/private/buf/bufformat/deprecate.go index d2012a5c05..b35981bee8 100644 --- a/private/buf/bufformat/deprecate.go +++ b/private/buf/bufformat/deprecate.go @@ -20,34 +20,18 @@ import ( "github.com/bufbuild/protocompile/ast" ) -// deprecationChecker determines which types should have deprecated options added. -// It is used during formatting to inject deprecation options at appropriate locations. -type deprecationChecker struct { - prefixes [][]string // FQN prefixes split into components +// fullNameMatcher determines which types should have deprecated options added. +type fullNameMatcher struct { + prefixes []string } -// newDeprecationChecker creates a new deprecationChecker for the given FQN prefixes. -// Each prefix is split by "." into components for matching. -func newDeprecationChecker(fqnPrefixes []string) *deprecationChecker { - prefixes := make([][]string, 0, len(fqnPrefixes)) - for _, prefix := range fqnPrefixes { - if prefix != "" { - prefixes = append(prefixes, strings.Split(prefix, ".")) - } - } - return &deprecationChecker{ - prefixes: prefixes, - } -} - -// isEmpty returns true if there are no deprecation prefixes configured. -func (d *deprecationChecker) isEmpty() bool { - return len(d.prefixes) == 0 +// newFullNameMatcher creates a new matcher for the given FQN prefixes. +func newFullNameMatcher(fqnPrefixes ...string) *fullNameMatcher { + return &fullNameMatcher{prefixes: fqnPrefixes} } -// shouldDeprecate returns true if the given FQN should be deprecated using prefix matching. -// This is used for packages, messages, enums, services, and RPCs. -func (d *deprecationChecker) shouldDeprecate(fqn []string) bool { +// matchesPrefix returns true if the given FQN matches using prefix matching. +func (d *fullNameMatcher) matchesPrefix(fqn string) bool { for _, prefix := range d.prefixes { if fqnMatchesPrefix(fqn, prefix) { return true @@ -56,11 +40,10 @@ func (d *deprecationChecker) shouldDeprecate(fqn []string) bool { return false } -// shouldDeprecateExact returns true if the given FQN matches exactly. -// This is used for fields and enum values which are only deprecated on exact match. -func (d *deprecationChecker) shouldDeprecateExact(fqn []string) bool { +// matchesExact returns true if the given FQN matches exactly. +func (d *fullNameMatcher) matchesExact(fqn string) bool { for _, prefix := range d.prefixes { - if fqnMatchesExact(fqn, prefix) { + if fqn == prefix { return true } } @@ -68,30 +51,14 @@ func (d *deprecationChecker) shouldDeprecateExact(fqn []string) bool { } // fqnMatchesPrefix returns true if fqn starts with prefix using component-based matching. -// For example, "foo.bar" matches "foo.bar.baz" but "foo.bar.b" does NOT match "foo.bar.baz". -func fqnMatchesPrefix(fqn, prefix []string) bool { +func fqnMatchesPrefix(fqn, prefix string) bool { if len(prefix) > len(fqn) { return false } - for i, p := range prefix { - if fqn[i] != p { - return false - } + if len(prefix) == len(fqn) { + return fqn == prefix } - return true -} - -// fqnMatchesExact returns true if fqn exactly equals prefix. -func fqnMatchesExact(fqn, prefix []string) bool { - if len(fqn) != len(prefix) { - return false - } - for i, p := range prefix { - if fqn[i] != p { - return false - } - } - return true + return prefix == "" || strings.HasPrefix(fqn, prefix+".") } // hasDeprecatedOption checks if a slice of declarations contains a deprecated = true option. @@ -126,7 +93,6 @@ func isDeprecatedOptionNode(opt *ast.OptionNode) bool { if part.Name == nil { return false } - // Get the identifier value var name string switch n := part.Name.(type) { case *ast.IdentNode: @@ -137,25 +103,33 @@ func isDeprecatedOptionNode(opt *ast.OptionNode) bool { if name != "deprecated" { return false } - // Check value is "true" if ident, ok := opt.Val.(*ast.IdentNode); ok { return ident.Val == "true" } return false } -// packageNameToComponents extracts package name components from an identifier node. -func packageNameToComponents(name ast.IdentValueNode) []string { +// packageNameToString extracts package name as a dot-separated string from an identifier node. +func packageNameToString(name ast.IdentValueNode) string { switch n := name.(type) { case *ast.IdentNode: - return []string{n.Val} + return n.Val case *ast.CompoundIdentNode: components := make([]string, len(n.Components)) for i, comp := range n.Components { components[i] = comp.Val } - return components + return strings.Join(components, ".") default: - return nil + return "" + } +} + +// parentFQN returns the parent FQN by removing the last component. +// For example, "foo.bar.baz" returns "foo.bar". +func parentFQN(fqn string) string { + if idx := strings.LastIndex(fqn, "."); idx >= 0 { + return fqn[:idx] } + return "" } diff --git a/private/buf/bufformat/deprecate_test.go b/private/buf/bufformat/deprecate_test.go index a63bc0f2ba..fb908d0c17 100644 --- a/private/buf/bufformat/deprecate_test.go +++ b/private/buf/bufformat/deprecate_test.go @@ -24,56 +24,56 @@ func TestFQNMatchesPrefix(t *testing.T) { t.Parallel() tests := []struct { name string - fqn []string - prefix []string + fqn string + prefix string expected bool }{ { name: "exact match", - fqn: []string{"foo", "bar", "baz"}, - prefix: []string{"foo", "bar", "baz"}, + fqn: "foo.bar.baz", + prefix: "foo.bar.baz", expected: true, }, { name: "prefix match", - fqn: []string{"foo", "bar", "baz"}, - prefix: []string{"foo", "bar"}, + fqn: "foo.bar.baz", + prefix: "foo.bar", expected: true, }, { name: "single component prefix", - fqn: []string{"foo", "bar", "baz"}, - prefix: []string{"foo"}, + fqn: "foo.bar.baz", + prefix: "foo", expected: true, }, { name: "empty prefix matches all", - fqn: []string{"foo", "bar"}, - prefix: []string{}, + fqn: "foo.bar", + prefix: "", expected: true, }, { name: "prefix longer than fqn", - fqn: []string{"foo", "bar"}, - prefix: []string{"foo", "bar", "baz"}, + fqn: "foo.bar", + prefix: "foo.bar.baz", expected: false, }, { name: "partial component mismatch - foo.bar.b does not match foo.bar.baz", - fqn: []string{"foo", "bar", "baz"}, - prefix: []string{"foo", "bar", "b"}, + fqn: "foo.bar.baz", + prefix: "foo.bar.b", expected: false, }, { name: "different path", - fqn: []string{"foo", "bar", "baz"}, - prefix: []string{"foo", "qux"}, + fqn: "foo.bar.baz", + prefix: "foo.qux", expected: false, }, { name: "empty fqn", - fqn: []string{}, - prefix: []string{"foo"}, + fqn: "", + prefix: "foo", expected: false, }, } @@ -87,81 +87,38 @@ func TestFQNMatchesPrefix(t *testing.T) { } } -func TestFQNMatchesExact(t *testing.T) { +func TestFullNameMatcherMatchesPrefix(t *testing.T) { t.Parallel() - tests := []struct { - name string - fqn []string - prefix []string - expected bool - }{ - { - name: "exact match", - fqn: []string{"foo", "bar", "baz"}, - prefix: []string{"foo", "bar", "baz"}, - expected: true, - }, - { - name: "prefix only - not exact", - fqn: []string{"foo", "bar", "baz"}, - prefix: []string{"foo", "bar"}, - expected: false, - }, - { - name: "longer prefix - not exact", - fqn: []string{"foo", "bar"}, - prefix: []string{"foo", "bar", "baz"}, - expected: false, - }, - { - name: "both empty", - fqn: []string{}, - prefix: []string{}, - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - result := fqnMatchesExact(tt.fqn, tt.prefix) - assert.Equal(t, tt.expected, result) - }) - } -} - -func TestDeprecationCheckerShouldDeprecate(t *testing.T) { - t.Parallel() - checker := newDeprecationChecker([]string{"foo.bar", "baz.qux"}) + matcher := newFullNameMatcher("foo.bar", "baz.qux") tests := []struct { name string - fqn []string + fqn string expected bool }{ { name: "matches first prefix", - fqn: []string{"foo", "bar", "baz"}, + fqn: "foo.bar.baz", expected: true, }, { name: "matches second prefix", - fqn: []string{"baz", "qux", "quux"}, + fqn: "baz.qux.quux", expected: true, }, { name: "exact match on prefix", - fqn: []string{"foo", "bar"}, + fqn: "foo.bar", expected: true, }, { name: "no match", - fqn: []string{"other", "package"}, + fqn: "other.package", expected: false, }, { name: "partial component no match", - fqn: []string{"foo", "bart"}, + fqn: "foo.bart", expected: false, }, } @@ -169,34 +126,34 @@ func TestDeprecationCheckerShouldDeprecate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - result := checker.shouldDeprecate(tt.fqn) + result := matcher.matchesPrefix(tt.fqn) assert.Equal(t, tt.expected, result) }) } } -func TestDeprecationCheckerShouldDeprecateExact(t *testing.T) { +func TestFullNameMatcherMatchesExact(t *testing.T) { t.Parallel() - checker := newDeprecationChecker([]string{"foo.bar.baz.SomeMessage.some_field"}) + matcher := newFullNameMatcher("foo.bar.baz.SomeMessage.some_field") tests := []struct { name string - fqn []string + fqn string expected bool }{ { name: "exact match", - fqn: []string{"foo", "bar", "baz", "SomeMessage", "some_field"}, + fqn: "foo.bar.baz.SomeMessage.some_field", expected: true, }, { name: "prefix only - not exact", - fqn: []string{"foo", "bar", "baz", "SomeMessage"}, + fqn: "foo.bar.baz.SomeMessage", expected: false, }, { name: "longer than prefix", - fqn: []string{"foo", "bar", "baz", "SomeMessage", "some_field", "extra"}, + fqn: "foo.bar.baz.SomeMessage.some_field.extra", expected: false, }, } @@ -204,27 +161,17 @@ func TestDeprecationCheckerShouldDeprecateExact(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - result := checker.shouldDeprecateExact(tt.fqn) + result := matcher.matchesExact(tt.fqn) assert.Equal(t, tt.expected, result) }) } } -func TestDeprecationCheckerEmptyPrefixes(t *testing.T) { +func TestFullNameMatcherEmptyPrefixes(t *testing.T) { t.Parallel() - checker := newDeprecationChecker([]string{}) + matcher := newFullNameMatcher() // Empty prefixes should not match anything - assert.True(t, checker.isEmpty()) - assert.False(t, checker.shouldDeprecate([]string{"foo", "bar"})) - assert.False(t, checker.shouldDeprecateExact([]string{"foo", "bar"})) -} - -func TestDeprecationCheckerEmptyStringsFiltered(t *testing.T) { - t.Parallel() - checker := newDeprecationChecker([]string{"", "foo.bar", ""}) - - // Empty strings should be filtered out - assert.False(t, checker.isEmpty()) - assert.True(t, checker.shouldDeprecate([]string{"foo", "bar", "baz"})) + assert.False(t, matcher.matchesPrefix("foo.bar")) + assert.False(t, matcher.matchesExact("foo.bar")) } diff --git a/private/buf/bufformat/formatter.go b/private/buf/bufformat/formatter.go index 758da04937..dc78408d94 100644 --- a/private/buf/bufformat/formatter.go +++ b/private/buf/bufformat/formatter.go @@ -70,9 +70,9 @@ type formatter struct { err error // deprecation tracks which types should have deprecated options injected. - deprecation *deprecationChecker - // packageFQN holds the current package's fully-qualified name components. - packageFQN []string + deprecation *fullNameMatcher + // packageFQN holds the current fully-qualified name. + packageFQN string // injectCompactDeprecation is set when the next compact options should have // deprecated = true injected at the beginning. injectCompactDeprecation bool @@ -90,7 +90,7 @@ func newFormatter( overrideTrailingComments: map[ast.Node]ast.Comments{}, } if options != nil && len(options.deprecatePrefixes) > 0 { - f.deprecation = newDeprecationChecker(options.deprecatePrefixes) + f.deprecation = newFullNameMatcher(options.deprecatePrefixes...) } return f } @@ -262,7 +262,7 @@ func (f *formatter) writeFileHeader() { } // Extract package FQN for deprecation tracking if packageNode != nil { - f.packageFQN = packageNameToComponents(packageNode.Name) + f.packageFQN = packageNameToString(packageNode.Name) } if f.fileNode.Syntax == nil && f.fileNode.Edition == nil && packageNode == nil && importNodes == nil && optionNodes == nil { @@ -936,8 +936,7 @@ func (f *formatter) writeEnumValue(enumValueNode *ast.EnumValueNode) { // Check if we need to inject deprecation for this enum value (exact match only) // Enum values are scoped to their parent (message or package), NOT the enum itself. // So we use the parent FQN (without the enum name) + the enum value name. - parentFQN := f.packageFQN[:len(f.packageFQN)-1] - enumValueFQN := append(parentFQN[:len(parentFQN):len(parentFQN)], enumValueNode.Name.Val) + enumValueFQN := parentFQN(f.packageFQN) + "." + enumValueNode.Name.Val needsDeprecation := f.shouldInjectDeprecationExact(enumValueFQN) && !hasCompactDeprecatedOption(enumValueNode.Options) if enumValueNode.Options != nil { @@ -990,7 +989,7 @@ func (f *formatter) writeField(fieldNode *ast.FieldNode) { f.writeInline(fieldNode.Tag) } // Check if we need to inject deprecation for this field (exact match only) - fieldFQN := append(f.currentFQN(), fieldNode.Name.Val) + fieldFQN := f.currentFQN() + "." + fieldNode.Name.Val needsDeprecation := f.shouldInjectDeprecationExact(fieldFQN) && !hasCompactDeprecatedOption(fieldNode.Options) if fieldNode.Options != nil { @@ -2628,35 +2627,39 @@ func (f *formatter) writeDeprecatedOption() { f.P("") } -// currentFQN returns the current fully-qualified name by combining package and type path. -func (f *formatter) currentFQN() []string { +// currentFQN returns the current fully-qualified name. +func (f *formatter) currentFQN() string { return f.packageFQN } // pushFQN appends a name component to the current FQN and returns a function to restore it. func (f *formatter) pushFQN(name string) func() { - originalLen := len(f.packageFQN) - f.packageFQN = append(f.packageFQN, name) + original := f.packageFQN + if f.packageFQN == "" { + f.packageFQN = name + } else { + f.packageFQN = f.packageFQN + "." + name + } return func() { - f.packageFQN = f.packageFQN[:originalLen] + f.packageFQN = original } } // shouldInjectDeprecation returns true if the given FQN should have a deprecated option injected. -func (f *formatter) shouldInjectDeprecation(fqn []string) bool { - if f.deprecation == nil || f.deprecation.isEmpty() { +func (f *formatter) shouldInjectDeprecation(fqn string) bool { + if f.deprecation == nil { return false } - return f.deprecation.shouldDeprecate(fqn) + return f.deprecation.matchesPrefix(fqn) } // shouldInjectDeprecationExact returns true if the given FQN should have a deprecated option // injected using exact matching (for fields and enum values). -func (f *formatter) shouldInjectDeprecationExact(fqn []string) bool { - if f.deprecation == nil || f.deprecation.isEmpty() { +func (f *formatter) shouldInjectDeprecationExact(fqn string) bool { + if f.deprecation == nil { return false } - return f.deprecation.shouldDeprecateExact(fqn) + return f.deprecation.matchesExact(fqn) } // writeCompactDeprecatedOption writes " [deprecated = true]" for compact options. From f79f9e386829a1379e39cd9fecf59a0fc312160f Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Wed, 28 Jan 2026 17:03:16 +0100 Subject: [PATCH 4/5] Error on no types match --- .../sourceeditdeprecate.go | 56 +++++++++++-------- private/buf/bufformat/bufformat.go | 31 ++++++++-- private/buf/bufformat/formatter.go | 17 +++++- private/buf/bufformat/formatter_test.go | 11 ++++ .../deprecate/already_deprecated.proto | 2 +- .../deprecate/enum_value_deprecation.proto | 4 +- 6 files changed, 88 insertions(+), 33 deletions(-) diff --git a/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go b/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go index c873633d3b..100f8c1032 100644 --- a/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go +++ b/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go @@ -44,7 +44,7 @@ const ( errorFormatFlagName = "error-format" excludePathsFlagName = "exclude-path" pathsFlagName = "path" - nameFlagName = "name" + prefixFlagName = "prefix" ) // NewCommand returns a new Command. @@ -59,34 +59,37 @@ func NewCommand( Long: ` Deprecate Protobuf types by adding the 'deprecated = true' option. -The --name flag is required and specifies the fully-qualified name prefix of the +The --prefix flag is required and specifies the fully-qualified name prefix of the types to deprecate. All types whose fully-qualified name starts with this prefix will have the 'deprecated = true' option added. For fields and enum values, only exact matches are deprecated. +Returns an error if no types match the specified prefixes. If matching types are +already deprecated, no changes are made and the command succeeds. + By default, the source is the current directory and files are formatted and rewritten in-place. Examples: Deprecate all types under a package prefix: - $ buf source edit deprecate --name foo.bar + $ buf source edit deprecate --prefix foo.bar Deprecate a specific message and all nested types: - $ buf source edit deprecate --name foo.bar.MyMessage + $ buf source edit deprecate --prefix foo.bar.MyMessage Deprecate a specific field: - $ buf source edit deprecate --name foo.bar.MyMessage.my_field + $ buf source edit deprecate --prefix foo.bar.MyMessage.my_field -Multiple --name flags can be specified: +Multiple --prefix flags can be specified: - $ buf source edit deprecate --name foo.bar --name baz.qux + $ buf source edit deprecate --prefix foo.bar --prefix baz.qux Display a diff of the changes instead of rewriting files: - $ buf source edit deprecate --name foo.bar -d + $ buf source edit deprecate --prefix foo.bar -d `, Args: appcmd.MaximumNArgs(1), Run: builder.NewRunFunc( @@ -105,7 +108,7 @@ type flags struct { ErrorFormat string ExcludePaths []string Paths []string - Names []string + Prefixes []string // special InputHashtag string } @@ -142,12 +145,12 @@ func (f *flags) Bind(flagSet *pflag.FlagSet) { `The buf.yaml file or data to use for configuration`, ) flagSet.StringSliceVar( - &f.Names, - nameFlagName, + &f.Prefixes, + prefixFlagName, nil, - `Required. The fully-qualified name prefix of the types to deprecate. May be specified multiple times.`, + `Required. The fully-qualified name prefix of types to deprecate. May be specified multiple times.`, ) - _ = appcmd.MarkFlagRequired(flagSet, nameFlagName) + _ = appcmd.MarkFlagRequired(flagSet, prefixFlagName) } func run( @@ -194,10 +197,10 @@ func run( ) originalReadBucket := bufmodule.ModuleReadBucketToStorageReadBucket(moduleReadBucket) - // Build format options from all name prefixes + // Build format options from all prefixes var formatOpts []bufformat.FormatOption - for _, name := range flags.Names { - formatOpts = append(formatOpts, bufformat.WithDeprecate(name)) + for _, prefix := range flags.Prefixes { + formatOpts = append(formatOpts, bufformat.WithDeprecate(prefix)) } formattedReadBucket, err := bufformat.FormatBucket(ctx, originalReadBucket, formatOpts...) @@ -205,10 +208,15 @@ func run( return err } - diffBuffer := bytes.NewBuffer(nil) + // Find changed files. Only generate diff text if displaying diff. + var diffBuffer bytes.Buffer + var diffWriter io.Writer = io.Discard + if flags.Diff { + diffWriter = &diffBuffer + } changedPaths, err := storage.DiffWithFilenames( ctx, - diffBuffer, + diffWriter, originalReadBucket, formattedReadBucket, storage.DiffWithExternalPaths(), @@ -216,13 +224,15 @@ func run( if err != nil { return err } - diffExists := diffBuffer.Len() > 0 + // If no files changed, the matched types were already deprecated. This is not an error. + // Note: if no types matched the prefix at all, FormatBucket already returned an error above. + if len(changedPaths) == 0 { + return nil + } if flags.Diff { - if diffExists { - if _, err := io.Copy(container.Stdout(), diffBuffer); err != nil { - return err - } + if _, err := io.Copy(container.Stdout(), &diffBuffer); err != nil { + return err } return nil } diff --git a/private/buf/bufformat/bufformat.go b/private/buf/bufformat/bufformat.go index c888816a5f..f7a74bd2cd 100644 --- a/private/buf/bufformat/bufformat.go +++ b/private/buf/bufformat/bufformat.go @@ -17,7 +17,9 @@ package bufformat import ( "context" "errors" + "fmt" "io" + "sync/atomic" "github.com/bufbuild/buf/private/bufpkg/bufmodule" "github.com/bufbuild/buf/private/pkg/storage" @@ -59,6 +61,7 @@ func FormatModuleSet(ctx context.Context, moduleSet bufmodule.ModuleSet, opts .. } // FormatBucket formats the .proto files in the bucket and returns a new bucket with the formatted files. +// If WithDeprecate options are provided but no types match the prefixes, an error is returned. func FormatBucket(ctx context.Context, bucket storage.ReadBucket, opts ...FormatOption) (_ storage.ReadBucket, retErr error) { options := &formatOptions{} for _, opt := range opts { @@ -69,6 +72,8 @@ func FormatBucket(ctx context.Context, bucket storage.ReadBucket, opts ...Format if err != nil { return nil, err } + // Track if any deprecation prefix matched across all files. + var deprecationMatched atomic.Bool jobs := make([]func(context.Context) error, len(paths)) for i, path := range paths { jobs[i] = func(ctx context.Context) (retErr error) { @@ -90,31 +95,49 @@ func FormatBucket(ctx context.Context, bucket storage.ReadBucket, opts ...Format defer func() { retErr = errors.Join(retErr, writeObjectCloser.Close()) }() - if err := formatFileNode(writeObjectCloser, fileNode, options); err != nil { + matched, err := formatFileNodeWithMatch(writeObjectCloser, fileNode, options) + if err != nil { return err } + if matched { + deprecationMatched.Store(true) + } return writeObjectCloser.SetExternalPath(readObjectCloser.ExternalPath()) } } if err := thread.Parallelize(ctx, jobs); err != nil { return nil, err } + // If deprecation was requested but nothing matched, return an error. + if len(options.deprecatePrefixes) > 0 && !deprecationMatched.Load() { + return nil, fmt.Errorf("no types matched the specified deprecation prefixes") + } return readWriteBucket, nil } // FormatFileNode formats the given file node and writes the result to dest. func FormatFileNode(dest io.Writer, fileNode *ast.FileNode) error { - return formatFileNode(dest, fileNode, &formatOptions{}) + _, err := formatFileNodeWithMatch(dest, fileNode, &formatOptions{}) + return err } // formatFileNode formats the given file node with options and writes the result to dest. func formatFileNode(dest io.Writer, fileNode *ast.FileNode, options *formatOptions) error { + _, err := formatFileNodeWithMatch(dest, fileNode, options) + return err +} + +// formatFileNodeWithMatch formats the given file node and returns whether any deprecation prefix matched. +func formatFileNodeWithMatch(dest io.Writer, fileNode *ast.FileNode, options *formatOptions) (bool, error) { // Construct the file descriptor to ensure the AST is valid. This will // capture unknown syntax like edition "2024" which at the current time is // not supported. if _, err := parser.ResultFromAST(fileNode, true, reporter.NewHandler(nil)); err != nil { - return err + return false, err } formatter := newFormatter(dest, fileNode, options) - return formatter.Run() + if err := formatter.Run(); err != nil { + return false, err + } + return formatter.deprecationMatched, nil } diff --git a/private/buf/bufformat/formatter.go b/private/buf/bufformat/formatter.go index dc78408d94..08f2425d01 100644 --- a/private/buf/bufformat/formatter.go +++ b/private/buf/bufformat/formatter.go @@ -76,6 +76,9 @@ type formatter struct { // injectCompactDeprecation is set when the next compact options should have // deprecated = true injected at the beginning. injectCompactDeprecation bool + // deprecationMatched is set to true when any type matches the deprecation prefix, + // regardless of whether it was already deprecated. + deprecationMatched bool } // newFormatter returns a new formatter for the given file. @@ -2646,20 +2649,30 @@ func (f *formatter) pushFQN(name string) func() { } // shouldInjectDeprecation returns true if the given FQN should have a deprecated option injected. +// It also sets deprecationMatched to true if the FQN matches the prefix. func (f *formatter) shouldInjectDeprecation(fqn string) bool { if f.deprecation == nil { return false } - return f.deprecation.matchesPrefix(fqn) + if f.deprecation.matchesPrefix(fqn) { + f.deprecationMatched = true + return true + } + return false } // shouldInjectDeprecationExact returns true if the given FQN should have a deprecated option // injected using exact matching (for fields and enum values). +// It also sets deprecationMatched to true if the FQN matches exactly. func (f *formatter) shouldInjectDeprecationExact(fqn string) bool { if f.deprecation == nil { return false } - return f.deprecation.matchesExact(fqn) + if f.deprecation.matchesExact(fqn) { + f.deprecationMatched = true + return true + } + return false } // writeCompactDeprecatedOption writes " [deprecated = true]" for compact options. diff --git a/private/buf/bufformat/formatter_test.go b/private/buf/bufformat/formatter_test.go index e3cced876c..f65ef860cb 100644 --- a/private/buf/bufformat/formatter_test.go +++ b/private/buf/bufformat/formatter_test.go @@ -191,3 +191,14 @@ func testDeprecateNoDiff(t *testing.T, name string, path string, deprecatePrefix assertGolden(reformatBucket) }) } + +func TestFormatBucketNoTypesMatchedError(t *testing.T) { + t.Parallel() + ctx := context.Background() + bucket, err := storageos.NewProvider().NewReadWriteBucket("testdata/deprecate") + require.NoError(t, err) + // Use a prefix that won't match anything in the deprecate testdata + _, err = FormatBucket(ctx, bucket, WithDeprecate("nonexistent.package")) + require.Error(t, err) + require.Contains(t, err.Error(), "no types matched") +} diff --git a/private/buf/bufformat/testdata/deprecate/already_deprecated.proto b/private/buf/bufformat/testdata/deprecate/already_deprecated.proto index 786a70ae43..c1d4b5e873 100644 --- a/private/buf/bufformat/testdata/deprecate/already_deprecated.proto +++ b/private/buf/bufformat/testdata/deprecate/already_deprecated.proto @@ -2,9 +2,9 @@ syntax = "proto2"; package test.deprecate; +option cc_enable_arenas = true; // File already has deprecated option - should not add duplicate. option deprecated = true; -option cc_enable_arenas = true; // Message already deprecated - should not add duplicate. message AlreadyDeprecatedMessage { diff --git a/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.proto b/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.proto index e9f1183ea3..741906ee80 100644 --- a/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.proto +++ b/private/buf/bufformat/testdata/deprecate/enum_value_deprecation.proto @@ -11,9 +11,7 @@ extend google.protobuf.EnumValueOptions { enum Status { STATUS_UNSPECIFIED = 0; STATUS_ACTIVE = 1; - STATUS_INACTIVE = 2 [ - (string_name) = "display_value" - ]; + STATUS_INACTIVE = 2 [(string_name) = "display_value"]; } // Message with nested enum to test nested enum value FQN. From 706b945e3d6d9e39176e4263d5f3be890e459573 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Wed, 28 Jan 2026 17:08:05 +0100 Subject: [PATCH 5/5] Fix lint --- .../sourceedit/sourceeditdeprecate/sourceeditdeprecate.go | 2 +- private/buf/bufformat/bufformat.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go b/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go index 100f8c1032..a4481cab9e 100644 --- a/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go +++ b/cmd/buf/internal/command/source/sourceedit/sourceeditdeprecate/sourceeditdeprecate.go @@ -210,7 +210,7 @@ func run( // Find changed files. Only generate diff text if displaying diff. var diffBuffer bytes.Buffer - var diffWriter io.Writer = io.Discard + diffWriter := io.Discard if flags.Diff { diffWriter = &diffBuffer } diff --git a/private/buf/bufformat/bufformat.go b/private/buf/bufformat/bufformat.go index f7a74bd2cd..0a1834aeda 100644 --- a/private/buf/bufformat/bufformat.go +++ b/private/buf/bufformat/bufformat.go @@ -117,8 +117,7 @@ func FormatBucket(ctx context.Context, bucket storage.ReadBucket, opts ...Format // FormatFileNode formats the given file node and writes the result to dest. func FormatFileNode(dest io.Writer, fileNode *ast.FileNode) error { - _, err := formatFileNodeWithMatch(dest, fileNode, &formatOptions{}) - return err + return formatFileNode(dest, fileNode, &formatOptions{}) } // formatFileNode formats the given file node with options and writes the result to dest.