diff --git a/core/itg/graph/graph.go b/core/itg/graph/graph.go index 89b85eb1..d92a215c 100644 --- a/core/itg/graph/graph.go +++ b/core/itg/graph/graph.go @@ -219,8 +219,11 @@ func (g *OptimizedGraph) AddTarget(target *targethasher.Target) { attributes := make(map[int]int, len(target.Attributes)) for _, attr := range target.Attributes { - attrNameID := getOrGenerateRecordReverse(attr.GetName(), g.AttrNameToID, g.AttrNameIDToString) - attrValueID := getOrGenerateRecordReverse(attr.GetStringValue(), g.AttrValueToID, g.AttrValueIDToString) + if attr == nil || attr.GetType() != buildpb.Attribute_STRING || attr.Name == nil || attr.StringValue == nil { + continue + } + attrNameID := getOrGenerateRecordReverse(*attr.Name, g.AttrNameToID, g.AttrNameIDToString) + attrValueID := getOrGenerateRecordReverse(*attr.StringValue, g.AttrValueToID, g.AttrValueIDToString) attributes[attrNameID] = attrValueID } optimizedTarget.HashWithoutDeps = target.HashWithoutDeps @@ -271,9 +274,11 @@ func (g *OptimizedGraph) OptimizedTargetToTarget(targetID int) targethasher.Targ for nameID, valID := range optimizedTarget.Attributes { n := g.AttrNameIDToString[nameID] v := g.AttrValueIDToString[valID] + t := buildpb.Attribute_STRING target.Attributes = append(target.Attributes, &buildpb.Attribute{ Name: &n, StringValue: &v, + Type: &t, }) } diff --git a/core/itg/graph/graph_test.go b/core/itg/graph/graph_test.go index a56c718b..f444d0d0 100644 --- a/core/itg/graph/graph_test.go +++ b/core/itg/graph/graph_test.go @@ -18,11 +18,16 @@ import ( "sort" "testing" + buildpb "github.com/bazelbuild/buildtools/build_proto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/tango/core/targethasher" ) +func strPtr(s string) *string { return &s } + +func attrTypePtr(t buildpb.Attribute_Discriminator) *buildpb.Attribute_Discriminator { return &t } + // --- IntSet --- func TestIntSet(t *testing.T) { @@ -184,6 +189,87 @@ func TestOptimizeGraph(t *testing.T) { }) } +// --- AddTarget attribute filtering --- + +func TestAddTargetAttributes(t *testing.T) { + t.Parallel() + + t.Run("only string-typed attributes are recorded", func(t *testing.T) { + t.Parallel() + targets := map[string]*targethasher.Target{ + "//pkg:a": { + Name: "//pkg:a", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + {Name: strPtr("importpath"), StringValue: strPtr("example.com/a"), Type: attrTypePtr(buildpb.Attribute_STRING)}, + {Name: strPtr("deps"), StringValue: strPtr("ignored"), Type: attrTypePtr(buildpb.Attribute_LABEL_LIST)}, + }, + }, + } + g := OptimizeGraph(targets) + aID := g.TargetNameToID["//pkg:a"] + + assert.Len(t, g.OptimizedTargets[aID].Attributes, 1) + _, ok := g.AttrNameToID["importpath"] + assert.True(t, ok, "string attribute should be recorded") + _, ok = g.AttrNameToID["deps"] + assert.False(t, ok, "non-string attribute should be skipped") + }) + + t.Run("attribute with nil type is skipped", func(t *testing.T) { + t.Parallel() + targets := map[string]*targethasher.Target{ + "//pkg:a": { + Name: "//pkg:a", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + {Name: strPtr("untyped"), StringValue: strPtr("value")}, + }, + }, + } + g := OptimizeGraph(targets) + aID := g.TargetNameToID["//pkg:a"] + + assert.Empty(t, g.OptimizedTargets[aID].Attributes) + }) + + t.Run("string attribute with nil name or value is skipped", func(t *testing.T) { + t.Parallel() + targets := map[string]*targethasher.Target{ + "//pkg:a": { + Name: "//pkg:a", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + {Name: nil, StringValue: strPtr("value"), Type: attrTypePtr(buildpb.Attribute_STRING)}, + {Name: strPtr("name"), StringValue: nil, Type: attrTypePtr(buildpb.Attribute_STRING)}, + }, + }, + } + g := OptimizeGraph(targets) + aID := g.TargetNameToID["//pkg:a"] + + assert.Empty(t, g.OptimizedTargets[aID].Attributes) + }) + + t.Run("nil attribute element does not panic", func(t *testing.T) { + t.Parallel() + targets := map[string]*targethasher.Target{ + "//pkg:a": { + Name: "//pkg:a", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + nil, + {Name: strPtr("importpath"), StringValue: strPtr("example.com/a"), Type: attrTypePtr(buildpb.Attribute_STRING)}, + }, + }, + } + g := OptimizeGraph(targets) + aID := g.TargetNameToID["//pkg:a"] + + assert.Len(t, g.OptimizedTargets[aID].Attributes, 1) + }) +} + // --- OptimizedTarget.Copy --- func TestOptimizedTargetCopy(t *testing.T) { @@ -288,4 +374,25 @@ func TestOptimizedTargetToTarget(t *testing.T) { result := g.OptimizedTargetToTarget(libID) assert.Contains(t, result.Deps, "//pkg:dep") }) + + t.Run("reconstructed attributes are marked as string type", func(t *testing.T) { + t.Parallel() + targets := map[string]*targethasher.Target{ + "//pkg:a": { + Name: "//pkg:a", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + {Name: strPtr("importpath"), StringValue: strPtr("example.com/a"), Type: attrTypePtr(buildpb.Attribute_STRING)}, + }, + }, + } + g := OptimizeGraph(targets) + aID := g.TargetNameToID["//pkg:a"] + + result := g.OptimizedTargetToTarget(aID) + require.Len(t, result.Attributes, 1) + assert.Equal(t, "importpath", result.Attributes[0].GetName()) + assert.Equal(t, "example.com/a", result.Attributes[0].GetStringValue()) + assert.Equal(t, buildpb.Attribute_STRING, result.Attributes[0].GetType()) + }) } diff --git a/core/itg/graph/invalidate.go b/core/itg/graph/invalidate.go index f4f3224d..e0c22350 100644 --- a/core/itg/graph/invalidate.go +++ b/core/itg/graph/invalidate.go @@ -19,6 +19,7 @@ import ( "fmt" "strings" + buildpb "github.com/bazelbuild/buildtools/build_proto" "github.com/uber/tango/core/targethasher" ) @@ -164,8 +165,11 @@ func (g *OptimizedGraph) upsertTarget(target *targethasher.Target, invalidated I attributes := make(map[int]int, len(target.Attributes)) for _, attr := range target.Attributes { - attrNameID := getOrGenerateRecordReverse(attr.GetName(), g.AttrNameToID, g.AttrNameIDToString) - attrValueID := getOrGenerateRecordReverse(attr.GetStringValue(), g.AttrValueToID, g.AttrValueIDToString) + if attr == nil || attr.GetType() != buildpb.Attribute_STRING || attr.Name == nil || attr.StringValue == nil { + continue + } + attrNameID := getOrGenerateRecordReverse(*attr.Name, g.AttrNameToID, g.AttrNameIDToString) + attrValueID := getOrGenerateRecordReverse(*attr.StringValue, g.AttrValueToID, g.AttrValueIDToString) attributes[attrNameID] = attrValueID } diff --git a/core/itg/graph/invalidate_test.go b/core/itg/graph/invalidate_test.go index fffdd025..c67d5bfc 100644 --- a/core/itg/graph/invalidate_test.go +++ b/core/itg/graph/invalidate_test.go @@ -17,6 +17,7 @@ package graph import ( "testing" + buildpb "github.com/bazelbuild/buildtools/build_proto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/tango/core/targethasher" @@ -246,4 +247,65 @@ func TestUpsertTarget(t *testing.T) { libID := g.TargetNameToID["//pkg:lib"] assert.True(t, invalidated.Contains(libID), "target with nil hash should be in invalidated set") }) + + t.Run("only string-typed attributes are recorded on upsert", func(t *testing.T) { + t.Parallel() + g := OptimizeGraph(nil) + newTarget := &targethasher.Target{ + Name: "//pkg:lib", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + {Name: strPtr("importpath"), StringValue: strPtr("example.com/lib"), Type: attrTypePtr(buildpb.Attribute_STRING)}, + {Name: strPtr("deps"), StringValue: strPtr("ignored"), Type: attrTypePtr(buildpb.Attribute_LABEL_LIST)}, + }, + } + require.NoError(t, g.upsertTarget(newTarget, NewIntSet())) + + libID := g.TargetNameToID["//pkg:lib"] + assert.Len(t, g.OptimizedTargets[libID].Attributes, 1) + _, ok := g.AttrNameToID["deps"] + assert.False(t, ok, "non-string attribute should be skipped") + }) + + t.Run("nil attribute element does not panic on upsert", func(t *testing.T) { + t.Parallel() + g := OptimizeGraph(nil) + newTarget := &targethasher.Target{ + Name: "//pkg:lib", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + nil, + {Name: strPtr("importpath"), StringValue: strPtr("example.com/lib"), Type: attrTypePtr(buildpb.Attribute_STRING)}, + }, + } + require.NoError(t, g.upsertTarget(newTarget, NewIntSet())) + + libID := g.TargetNameToID["//pkg:lib"] + assert.Len(t, g.OptimizedTargets[libID].Attributes, 1) + }) + + t.Run("attribute updates replace prior attributes on upsert", func(t *testing.T) { + t.Parallel() + g := OptimizeGraph(map[string]*targethasher.Target{ + "//pkg:lib": { + Name: "//pkg:lib", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + {Name: strPtr("importpath"), StringValue: strPtr("example.com/old"), Type: attrTypePtr(buildpb.Attribute_STRING)}, + }, + }, + }) + libID := g.TargetNameToID["//pkg:lib"] + require.Len(t, g.OptimizedTargets[libID].Attributes, 1) + + updated := &targethasher.Target{ + Name: "//pkg:lib", + RuleType: "go_library", + Attributes: []*buildpb.Attribute{ + {Name: strPtr("nonstring"), StringValue: strPtr("value"), Type: attrTypePtr(buildpb.Attribute_INTEGER)}, + }, + } + require.NoError(t, g.upsertTarget(updated, NewIntSet())) + assert.Empty(t, g.OptimizedTargets[libID].Attributes, "non-string attribute should not replace prior one") + }) } diff --git a/core/itg/graph/update_test.go b/core/itg/graph/update_test.go index da4d63ee..8b400866 100644 --- a/core/itg/graph/update_test.go +++ b/core/itg/graph/update_test.go @@ -116,6 +116,47 @@ func TestComputeAvailableHashes(t *testing.T) { assert.Nil(t, targets[name].Hash, "full hash is not computed here — deps are needed") }) + t.Run("tags order does not affect rule HashWithoutDeps", func(t *testing.T) { + t.Parallel() + name := "//pkg:lib" + ruleName := name + ruleClass := "go_library" + tagsAttr := func(tags []string) *buildpb.Attribute { + typ := buildpb.Attribute_STRING_LIST + n := "tags" + return &buildpb.Attribute{Name: &n, Type: &typ, StringListValue: tags} + } + + forward := map[string]*targethasher.Target{ + name: { + Name: name, + RuleType: "go_library", + Rule: &buildpb.Rule{ + Name: &ruleName, + RuleClass: &ruleClass, + Attribute: []*buildpb.Attribute{tagsAttr([]string{"a", "b"})}, + }, + }, + } + reversed := map[string]*targethasher.Target{ + name: { + Name: name, + RuleType: "go_library", + Rule: &buildpb.Rule{ + Name: &ruleName, + RuleClass: &ruleClass, + Attribute: []*buildpb.Attribute{tagsAttr([]string{"b", "a"})}, + }, + }, + } + + require.NoError(t, computeAvailableHashes(context.Background(), &fakeSourceHasher{}, forward)) + require.NoError(t, computeAvailableHashes(context.Background(), &fakeSourceHasher{}, reversed)) + + assert.Equal(t, forward[name].HashWithoutDeps, reversed[name].HashWithoutDeps, + "tags=[a,b] and tags=[b,a] should hash identically") + }) + t.Run("source hasher error is propagated", func(t *testing.T) { t.Parallel() hasher := &fakeSourceHasher{err: assert.AnError}