Cache node building in graph and add testing (#2245)

private/bufpkg/bufgraph/bufgraph_test.go

@@ -0,0 +1,122 @@
+// 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 bufgraph
+
+import (
+	"context"
+	"path/filepath"
+	"testing"
+
+	"github.com/bufbuild/buf/private/buf/bufwork"
+	"github.com/bufbuild/buf/private/bufpkg/bufconfig"
+	"github.com/bufbuild/buf/private/bufpkg/bufmodule"
+	"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmodulebuild"
+	"github.com/bufbuild/buf/private/pkg/storage"
+	"github.com/bufbuild/buf/private/pkg/storage/storageos"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+func TestBasic(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	workspace, err := testBuildWorkspace(ctx, filepath.Join("testdata", "basic"))
+	require.NoError(t, err)
+	builder := NewBuilder(
+		zap.NewNop(),
+		bufmodule.NewNopModuleResolver(),
+		bufmodule.NewNopModuleReader(),
+	)
+	graph, fileAnnotations, err := builder.Build(
+		ctx,
+		workspace.GetModules(),
+		BuildWithWorkspace(workspace),
+	)
+	require.NoError(t, err)
+	require.Empty(t, fileAnnotations)
+	dotString, err := graph.DOTString(func(key Node) string { return key.String() })
+	require.NoError(t, err)
+	require.Equal(
+		t,
+		`digraph {
+
+  1 [label="bsr.internal/foo/test-a"]
+  2 [label="bsr.internal/foo/test-b"]
+  3 [label="bsr.internal/foo/test-c"]
+  4 [label="bsr.internal/foo/test-d"]
+  5 [label="bsr.internal/foo/test-e"]
+  6 [label="bsr.internal/foo/test-f"]
+  7 [label="bsr.internal/foo/test-g"]
+
+  1 -> 2
+  2 -> 3
+  3 -> 4
+  1 -> 4
+  1 -> 5
+  5 -> 6
+  7
+
+}`,
+		dotString,
+	)
+}
+
+// TODO: This entire function is all you should need to do to build workspaces, and even
+// this is overly complicated because of the wonkiness of bufmodulebuild and NewWorkspace.
+// We should have this in a common place for at least testing.
+func testBuildWorkspace(ctx context.Context, workspacePath string) (bufmodule.Workspace, error) {
+	workspaceBucket, err := storageos.NewProvider().NewReadWriteBucket(workspacePath)
+	if err != nil {
+		return nil, err
+	}
+	workspaceConfig, err := bufwork.GetConfigForBucket(ctx, workspaceBucket, ".")
+	if err != nil {
+		return nil, err
+	}
+	moduleBucketBuilder := bufmodulebuild.NewModuleBucketBuilder()
+	namedModules := make(map[string]bufmodule.Module, len(workspaceConfig.Directories))
+	allModules := make([]bufmodule.Module, 0, len(workspaceConfig.Directories))
+	for _, directory := range workspaceConfig.Directories {
+		moduleBucket := storage.MapReadBucket(
+			workspaceBucket,
+			storage.MapOnPrefix(directory),
+		)
+		moduleConfig, err := bufconfig.GetConfigForBucket(ctx, moduleBucket)
+		if err != nil {
+			return nil, err
+		}
+		module, err := moduleBucketBuilder.BuildForBucket(
+			ctx,
+			moduleBucket,
+			moduleConfig.Build,
+			bufmodulebuild.WithModuleIdentity(
+				moduleConfig.ModuleIdentity,
+			),
+		)
+		if err != nil {
+			return nil, err
+		}
+		if moduleConfig.ModuleIdentity != nil {
+			namedModules[moduleConfig.ModuleIdentity.IdentityString()] = module
+		}
+		allModules = append(allModules, module)
+	}
+	return bufmodule.NewWorkspace(
+		ctx,
+		namedModules,
+		allModules,
+	)
+}

private/bufpkg/bufgraph/builder.go

@@ -72,13 +72,15 @@ func (b *builder) build(
 	workspace bufmodule.Workspace,
 ) (*dag.Graph[Node], []bufanalysis.FileAnnotation, error) {
 	graph := dag.NewGraph[Node]()
+	alreadyProcessedNodes := make(map[Node]struct{})
 	for _, module := range modules {
 		fileAnnotations, err := b.buildForModule(
 			ctx,
 			module,
 			newNodeForModule(module),
 			workspace,
 			graph,
+			alreadyProcessedNodes,
 		)
 		if err != nil {
 			return nil, nil, err
@@ -96,7 +98,15 @@ func (b *builder) buildForModule(
 	node Node,
 	workspace bufmodule.Workspace,
 	graph *dag.Graph[Node],
+	alreadyProcessedNodes map[Node]struct{},
 ) ([]bufanalysis.FileAnnotation, error) {
+	// We can't rely on the existence of a node in the graph for this, as when we add an edge
+	// to the graph, the node is added, and we still need to process the node as a potential
+	// source node.
+	if _, ok := alreadyProcessedNodes[node]; ok {
+		return nil, nil
+	}
+	alreadyProcessedNodes[node] = struct{}{}
 	graph.AddNode(node)
 	image, fileAnnotations, err := b.imageBuilder.Build(
 		ctx,
@@ -129,6 +139,7 @@ func (b *builder) buildForModule(
 			dependencyNode,
 			workspace,
 			graph,
+			alreadyProcessedNodes,
 		)
 		if err != nil {
 			return nil, err

private/bufpkg/bufgraph/testdata/basic/buf.work.yaml

@@ -0,0 +1,9 @@
+version: v1
+directories:
+  - test-a
+  - test-b
+  - test-c
+  - test-d
+  - test-e
+  - test-f
+  - test-g

private/bufpkg/bufgraph/testdata/basic/test-a/a/v1/a.proto

@@ -0,0 +1,13 @@
+syntax = "proto3";
+
+package a.v1;
+
+import "b/v1/b.proto";
+import "d/v1/d.proto";
+import "e/v1/e.proto";
+
+message A {
+  b.v1.B b = 1;
+  d.v1.D d = 2;
+  e.v1.E e = 3;
+}

private/bufpkg/bufgraph/testdata/basic/test-a/a2/v1/a2.proto

@@ -0,0 +1,9 @@
+syntax = "proto3";
+
+package a2.v1;
+
+import "e/v1/e.proto";
+
+message A2 {
+  e.v1.E e = 3;
+}

private/bufpkg/bufgraph/testdata/basic/test-a/buf.yaml

@@ -0,0 +1,6 @@
+version: v1
+name: bsr.internal/foo/test-a
+deps:
+  - bsr.internal/foo/test-b
+  - bsr.internal/foo/test-d
+  - bsr.internal/foo/test-e

private/bufpkg/bufgraph/testdata/basic/test-b/b/v1/b.proto

@@ -0,0 +1,9 @@
+syntax = "proto3";
+
+package b.v1;
+
+import "c/v1/c.proto";
+
+message B {
+  c.v1.C c = 1;
+}

private/bufpkg/bufgraph/testdata/basic/test-b/buf.yaml

@@ -0,0 +1,4 @@
+version: v1
+name: bsr.internal/foo/test-b
+deps:
+  - bsr.internal/foo/test-c

private/bufpkg/bufgraph/testdata/basic/test-c/buf.yaml

@@ -0,0 +1,4 @@
+version: v1
+name: bsr.internal/foo/test-c
+deps:
+  - bsr.internal/foo/test-d

private/bufpkg/bufgraph/testdata/basic/test-c/c/v1/c.proto

@@ -0,0 +1,9 @@
+syntax = "proto3";
+
+package c.v1;
+
+import "d/v1/d.proto";
+
+message C {
+  d.v1.D d = 1;
+}

private/bufpkg/bufgraph/testdata/basic/test-d/buf.yaml

@@ -0,0 +1,2 @@
+version: v1
+name: bsr.internal/foo/test-d

private/bufpkg/bufgraph/testdata/basic/test-d/d/v1/d.proto

@@ -0,0 +1,5 @@
+syntax = "proto3";
+
+package d.v1;
+
+message D {}

private/bufpkg/bufgraph/testdata/basic/test-e/buf.yaml

@@ -0,0 +1,4 @@
+version: v1
+name: bsr.internal/foo/test-e
+deps:
+  - bsr.internal/foo/test-f

private/bufpkg/bufgraph/testdata/basic/test-e/e/v1/e.proto

@@ -0,0 +1,9 @@
+syntax = "proto3";
+
+package e.v1;
+
+import "f/v1/f.proto";
+
+message E {
+  f.v1.F f = 1;
+}

private/bufpkg/bufgraph/testdata/basic/test-f/buf.yaml

@@ -0,0 +1,2 @@
+version: v1
+name: bsr.internal/foo/test-f

private/bufpkg/bufgraph/testdata/basic/test-f/f/v1/f.proto

@@ -0,0 +1,5 @@
+syntax = "proto3";
+
+package f.v1;
+
+message F {}

private/bufpkg/bufgraph/testdata/basic/test-g/buf.yaml

@@ -0,0 +1,2 @@
+version: v1
+name: bsr.internal/foo/test-g

private/bufpkg/bufgraph/testdata/basic/test-g/g/v1/g.proto

@@ -0,0 +1,5 @@
+syntax = "proto3";
+
+package g.v1;
+
+message G {}

private/bufpkg/bufmodule/bufmodulebuild/bufmodulebuild.go

@@ -68,6 +68,8 @@ type ModuleBucketBuilder interface {
 	// This means the module is built differently than described in storage, which
 	// may cause building to fail or succeed when it shouldn't. For your own
 	// sanity, you should pass a config value read from the provided bucket.
+	//
+	// TODO: why do we pass a config here?! This parameter should be removed.
 	BuildForBucket(
 		ctx context.Context,
 		readBucket storage.ReadBucket,