Warn when a plugin does not proto3 optional instead of error-ing (#3015)

This changes the behaviour of `buf generate` to warn when
a feature is not supported instead of erroring. And only error
in cases when edition is found.
This change is to keep backwards compatibility with pre-1.32.0
versions of the CLI, which would warn but ignore unsupported
features and continue to generate code.

CHANGELOG.md

@@ -2,7 +2,8 @@
 
 ## [Unreleased]
 
-- No changes yet.
+- Update `buf generate` to warn instead of error when proto3 optional is required but not
+  supported by a plugin.
 
 ## [v1.32.1] - 2024-05-21
 

private/buf/bufgen/features.go

@@ -21,7 +21,9 @@ import (
 
 	"github.com/bufbuild/buf/private/bufpkg/bufconfig"
 	"github.com/bufbuild/buf/private/bufpkg/bufimage"
+	"github.com/bufbuild/buf/private/pkg/slicesext"
 	"go.uber.org/multierr"
+	"go.uber.org/zap"
 	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/types/descriptorpb"
 	"google.golang.org/protobuf/types/pluginpb"
@@ -85,6 +87,7 @@ func computeRequiredFeatures(image bufimage.Image) *requiredFeatures {
 }
 
 func checkRequiredFeatures(
+	logger *zap.Logger,
 	required *requiredFeatures,
 	responses []*pluginpb.CodeGeneratorResponse,
 	configs []bufconfig.GeneratePluginConfig,
@@ -133,10 +136,28 @@ func checkRequiredFeatures(
 				return failedFeatures[i] < failedFeatures[j]
 			})
 			for _, feature := range failedFeatures {
-				for _, file := range failed.featureToFilenames[feature] {
-					errs = append(errs, fmt.Errorf("plugin %q does not support feature %q which is required by %q",
-						pluginName, featureName(feature), file))
+				// For CLI versions pre-1.32.0, we logged unsupported features. However, this is an
+				// unsafe behavior for editions. So, in keeping with pre-1.32.0 CLI versions, we
+				// warn for proto3 optional, but error if editions are required (BSR-3931).
+				if feature == pluginpb.CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL {
+					warningMessage := fmt.Sprintf("plugin %q does not support required features.\n", pluginName)
+
+					files := failed.featureToFilenames[feature]
+					warningMessage = fmt.Sprintln(
+						warningMessage,
+						fmt.Sprintf(" Feature %q is required by %d file(s):", featureName(feature), len(files)),
+					)
+					warningMessage = fmt.Sprintln(warningMessage, fmt.Sprintf("   %s", strings.Join(files, ",")))
+					logger.Warn(strings.TrimSpace(warningMessage))
+					continue
 				}
+				featureErrs := slicesext.Map(
+					failed.featureToFilenames[feature],
+					func(fileName string) error {
+						return fmt.Errorf("plugin %q does not support feature %q which is required by %q", pluginName, featureName(feature), fileName)
+					},
+				)
+				errs = append(errs, featureErrs...)
 			}
 		}
 		if len(failedEditions) > 0 {

private/buf/bufgen/features_test.go

@@ -23,6 +23,9 @@ import (
 	"github.com/gofrs/uuid/v5"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
+	"go.uber.org/zap/zaptest/observer"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/descriptorpb"
 	"google.golang.org/protobuf/types/pluginpb"
@@ -93,32 +96,58 @@ func TestCheckRequiredFeatures(t *testing.T) {
 	}
 
 	// Successful cases
-	testCheckRequiredFeatures(t, noRequiredFeatures, supportsNoFeatures, "")
-	testCheckRequiredFeatures(t, requiresProto3Optional, supportsBoth, "")
-	testCheckRequiredFeatures(t, requiresEditions, supportsBoth, "")
-	testCheckRequiredFeatures(t, requiresBoth, supportsBoth, "")
+	testCheckRequiredFeatures(t, noRequiredFeatures, supportsNoFeatures, "", "")
+	testCheckRequiredFeatures(t, requiresProto3Optional, supportsBoth, "", "")
+	testCheckRequiredFeatures(t, requiresEditions, supportsBoth, "", "")
+	testCheckRequiredFeatures(t, requiresBoth, supportsBoth, "", "")
 
 	// Error cases
-	testCheckRequiredFeatures(t, requiresProto3Optional, supportsNoFeatures,
-		`plugin "test" does not support feature "proto3 optional" which is required by "proto3_optional.proto"`)
-	testCheckRequiredFeatures(t, requiresEditions, supportsNoFeatures,
-		`plugin "test" does not support feature "supports editions" which is required by "editions.proto"`)
-	testCheckRequiredFeatures(t, requiresBoth, supportsNoFeatures,
-		`plugin "test" does not support feature "proto3 optional" which is required by "proto3_optional.proto"; `+
-			`plugin "test" does not support feature "supports editions" which is required by "editions.proto"`)
-	testCheckRequiredFeatures(t, requiresEditions, supportsEditionsButOutOfRange,
-		`plugin "test" does not support edition "2023" which is required by "editions.proto"`)
+	testCheckRequiredFeatures(
+		t,
+		requiresProto3Optional,
+		supportsNoFeatures,
+		`plugin "test" does not support required features.
+  Feature "proto3 optional" is required by 1 file(s):
+    proto3_optional.proto`,
+		"", // No error expected
+	)
+	testCheckRequiredFeatures(
+		t,
+		requiresEditions,
+		supportsNoFeatures,
+		"", // No stderr expected
+		`plugin "test" does not support feature "supports editions" which is required by "editions.proto"`,
+	)
+	testCheckRequiredFeatures(
+		t,
+		requiresBoth,
+		supportsNoFeatures,
+		`plugin "test" does not support required features.
+  Feature "proto3 optional" is required by 1 file(s):
+    proto3_optional.proto`,
+		`plugin "test" does not support feature "supports editions" which is required by "editions.proto"`,
+	)
+	testCheckRequiredFeatures(
+		t,
+		requiresEditions,
+		supportsEditionsButOutOfRange,
+		"", // No stderr expected
+		`plugin "test" does not support edition "2023" which is required by "editions.proto"`,
+	)
 }
 
 func testCheckRequiredFeatures(
 	t *testing.T,
 	image bufimage.Image,
 	codeGenResponse *pluginpb.CodeGeneratorResponse,
+	expectedStdErr string,
 	expectedErr string,
 ) {
 	t.Helper()
 	required := computeRequiredFeatures(image)
+	observed, logs := observer.New(zapcore.WarnLevel)
 	err := checkRequiredFeatures(
+		zap.New(observed),
 		required,
 		[]*pluginpb.CodeGeneratorResponse{
 			codeGenResponse,
@@ -134,6 +163,12 @@ func testCheckRequiredFeatures(
 			newMockPluginConfig("never_fails"),
 		},
 	)
+	if expectedStdErr != "" {
+		require.NotEmpty(t, logs.All())
+		require.Equal(t, expectedStdErr, logs.All()[0].Message)
+	} else {
+		require.Empty(t, logs.All())
+	}
 	if expectedErr != "" {
 		require.ErrorContains(t, err, expectedErr)
 	} else {

private/buf/bufgen/generator.go

@@ -274,7 +274,7 @@ func (g *generator) execPlugins(
 	if err := validateResponses(responses, pluginConfigs); err != nil {
 		return nil, err
 	}
-	if err := checkRequiredFeatures(requiredFeatures, responses, pluginConfigs); err != nil {
+	if err := checkRequiredFeatures(g.logger, requiredFeatures, responses, pluginConfigs); err != nil {
 		return nil, err
 	}
 	return responses, nil