Fix rename module when no module identity (#2360)

Missing nil-check catched when syncing `bufbuild/buf`.

private/buf/bufsync/rebuild_module_test.go

@@ -0,0 +1,135 @@
+// Copyright 2020-2023 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 bufsync
+
+import (
+	"context"
+	"testing"
+
+	"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
+	"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleconfig"
+	"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
+	"github.com/bufbuild/buf/private/pkg/storage/storagemem"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestRebuildModule(t *testing.T) {
+	t.Parallel()
+	identity1, err := bufmoduleref.NewModuleIdentity("buf.first", "foo", "bar")
+	require.NoError(t, err)
+	identity2, err := bufmoduleref.NewModuleIdentity("buf.second", "baz", "qux")
+	require.NoError(t, err)
+	moduleBucket, err := storagemem.NewReadBucket(map[string][]byte{
+		"buf.yaml":         []byte("version: v1\n"),
+		"foo/v1/foo.proto": []byte(`syntax = "proto3";\nmessage Test {}\n`),
+	})
+	require.NoError(t, err)
+	emptyConfig, err := bufmoduleconfig.NewConfigV1(bufmoduleconfig.ExternalConfigV1{})
+	require.NoError(t, err)
+	type testCase struct {
+		name                   string
+		originalModuleIdentity bufmoduleref.ModuleIdentity
+	}
+	testCases := []testCase{
+		{
+			name: "when_no_module_identity",
+		},
+		{
+			name:                   "when_some_module_identity",
+			originalModuleIdentity: identity1,
+		},
+	}
+	for _, tc := range testCases {
+		func(tc testCase) {
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+				originalModule, err := bufmodulebuild.NewModuleBucketBuilder().BuildForBucket(
+					context.Background(),
+					moduleBucket,
+					emptyConfig,
+					bufmodulebuild.WithModuleIdentity(tc.originalModuleIdentity),
+				)
+				require.NoError(t, err)
+				renamedModule, err := renameModule(
+					context.Background(),
+					originalModule,
+					identity2,
+				)
+				require.NoError(t, err)
+				require.NotNil(t, renamedModule.ModuleIdentity())
+				assert.Equal(t, identity2.IdentityString(), renamedModule.ModuleIdentity().IdentityString())
+			})
+		}(tc)
+	}
+}
+
+func TestRebuildModuleFailures(t *testing.T) {
+	t.Parallel()
+	moduleBucket, err := storagemem.NewReadBucket(map[string][]byte{
+		"buf.yaml":         []byte("version: v1\n"),
+		"foo/v1/foo.proto": []byte(`syntax = "proto3";\nmessage Test {}\n`),
+	})
+	require.NoError(t, err)
+	emptyConfig, err := bufmoduleconfig.NewConfigV1(bufmoduleconfig.ExternalConfigV1{})
+	require.NoError(t, err)
+	validModule, err := bufmodulebuild.NewModuleBucketBuilder().BuildForBucket(
+		context.Background(),
+		moduleBucket,
+		emptyConfig,
+	)
+	require.NoError(t, err)
+	validIdentity, err := bufmoduleref.NewModuleIdentity("buf.test", "acme", "foo")
+	require.NoError(t, err)
+	type testCase struct {
+		name                    string
+		missingOriginalModule   bool
+		missingIdentityOverride bool
+	}
+	testCases := []testCase{
+		{
+			name:                  "when_no_original_module",
+			missingOriginalModule: true,
+		},
+		{
+			name:                    "when_no_new_identity",
+			missingIdentityOverride: true,
+		},
+	}
+	for _, tc := range testCases {
+		func(tc testCase) {
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+				var (
+					originalModule *bufmodulebuild.BuiltModule
+					newIdentity    bufmoduleref.ModuleIdentity
+				)
+				if !tc.missingOriginalModule {
+					originalModule = validModule
+				}
+				if !tc.missingIdentityOverride {
+					newIdentity = validIdentity
+				}
+				renamedModule, err := renameModule(
+					context.Background(),
+					originalModule,
+					newIdentity,
+				)
+				require.Nil(t, renamedModule)
+				require.Error(t, err)
+			})
+		}(tc)
+	}
+}

private/buf/bufsync/syncer.go

@@ -437,12 +437,12 @@ func (s *syncer) branchSyncableCommits(ctx context.Context, branch string) ([]*s
 					if builtModule == nil {
 						return fmt.Errorf("cannot override commit, no built module: %w", readErr)
 					}
-					// rebuild the module with the target identity, and add it to the queue
-					rebuiltModule, err := rebuildModule(ctx, builtModule, pendingModule.targetModuleIdentity)
+					// rename the module to the target identity, and add it to the queue
+					renamedModule, err := renameModule(ctx, builtModule, pendingModule.targetModuleIdentity)
 					if err != nil {
-						return fmt.Errorf("override module in commit: %s, rebuild module: %w", readErr.Error(), err)
+						return fmt.Errorf("override module in commit: %s, rename module: %w", readErr.Error(), err)
 					}
-					syncModules[moduleDir] = rebuiltModule
+					syncModules[moduleDir] = renamedModule
 				default:
 					return fmt.Errorf("unexpected decision code %d for read module error %w", decision, readErr)
 				}
@@ -684,34 +684,35 @@ type moduleTarget struct {
 	expectedSyncPoint    string
 }
 
-// rebuildModule takes a built module, and rebuilds it with a new module identity.
-func rebuildModule(
+// renameModule takes a module, and rebuilds it with a new module identity.
+func renameModule(
 	ctx context.Context,
-	builtModule *bufmodulebuild.BuiltModule,
-	identityOverride bufmoduleref.ModuleIdentity,
+	baseModule *bufmodulebuild.BuiltModule,
+	newIdentity bufmoduleref.ModuleIdentity,
 ) (*bufmodulebuild.BuiltModule, error) {
-	if builtModule == nil {
-		return nil, errors.New("no built module to rebuild")
+	if baseModule == nil {
+		return nil, errors.New("no base module to rebuild")
 	}
-	if identityOverride == nil {
+	if newIdentity == nil {
 		return nil, errors.New("no new identity to apply")
 	}
-	if builtModule.ModuleIdentity().IdentityString() == identityOverride.IdentityString() {
-		// same identity, no need to rebuild anything
-		return builtModule, nil
+	if baseModule.ModuleIdentity() != nil &&
+		baseModule.ModuleIdentity().IdentityString() == newIdentity.IdentityString() {
+		// same identity, no need to rename anything
+		return baseModule, nil
 	}
-	sourceConfig, err := bufconfig.GetConfigForBucket(ctx, builtModule.Bucket)
+	sourceConfig, err := bufconfig.GetConfigForBucket(ctx, baseModule.Bucket)
 	if err != nil {
 		return nil, fmt.Errorf("invalid module config: %w", err)
 	}
-	rebuiltModule, err := bufmodulebuild.NewModuleBucketBuilder().BuildForBucket(
+	renamedModule, err := bufmodulebuild.NewModuleBucketBuilder().BuildForBucket(
 		ctx,
-		builtModule.Bucket,
+		baseModule.Bucket,
 		sourceConfig.Build,
-		bufmodulebuild.WithModuleIdentity(identityOverride),
+		bufmodulebuild.WithModuleIdentity(newIdentity),
 	)
 	if err != nil {
-		return nil, fmt.Errorf("rebuild module: %w", err)
+		return nil, fmt.Errorf("rebuild module with new identity: %w", err)
 	}
-	return rebuiltModule, nil
+	return renamedModule, nil
 }