From 4aa9c6f5c341b9772c700560b2b1c88c849170c5 Mon Sep 17 00:00:00 2001 From: yushan Date: Tue, 22 Sep 2026 19:08:32 +0000 Subject: [PATCH 1/3] fix(itg): only propagate string-valued attributes through the optimized graph OptimizedGraph.AddTarget and upsertTarget previously recorded every target attribute via GetName()/GetStringValue(), regardless of its underlying type. For non-string attributes (bool, int, label lists, etc.) this silently coerced values through the zero-value string getters, corrupting the stored attribute map. Only record attributes whose Type is buildpb.Attribute_STRING and whose Name/StringValue are non-nil. OptimizedTargetToTarget now also sets Type explicitly to buildpb.Attribute_STRING when reconstructing attributes, since every attribute stored in the optimized graph is now guaranteed to be string-valued and downstream consumers that check GetType() should not see an ambiguous zero-value default. Co-Authored-By: Claude Sonnet 5 --- core/itg/graph/graph.go | 9 +++- core/itg/graph/graph_test.go | 89 +++++++++++++++++++++++++++++++ core/itg/graph/invalidate.go | 8 ++- core/itg/graph/invalidate_test.go | 45 ++++++++++++++++ 4 files changed, 147 insertions(+), 4 deletions(-) diff --git a/core/itg/graph/graph.go b/core/itg/graph/graph.go index 89b85eb1..d9c0cbad 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.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..9f5663d7 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,69 @@ 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) + }) +} + // --- OptimizedTarget.Copy --- func TestOptimizedTargetCopy(t *testing.T) { @@ -288,4 +356,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..2190b07b 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.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..7e61188f 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,48 @@ 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("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") + }) } From 0a6c44f99daafad2c9c4047fbb371dc2fe95ca12 Mon Sep 17 00:00:00 2001 From: yushan Date: Tue, 22 Sep 2026 22:25:36 +0000 Subject: [PATCH 2/3] fix(itg): guard nil attribute elements in optimized-graph filtering Make the STRING-type attribute filter in AddTarget/upsertTarget explicitly skip a nil *buildpb.Attribute element instead of relying on GetType()'s nil-safe default (Attribute_INTEGER) short-circuiting the `||` before the nil-unsafe Name/StringValue field reads. Behavior is unchanged, but the safety is now explicit rather than incidental. Co-Authored-By: Claude Sonnet 5 --- core/itg/graph/graph.go | 2 +- core/itg/graph/graph_test.go | 18 ++++++++++++++++++ core/itg/graph/invalidate.go | 2 +- core/itg/graph/invalidate_test.go | 17 +++++++++++++++++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/core/itg/graph/graph.go b/core/itg/graph/graph.go index d9c0cbad..d92a215c 100644 --- a/core/itg/graph/graph.go +++ b/core/itg/graph/graph.go @@ -219,7 +219,7 @@ func (g *OptimizedGraph) AddTarget(target *targethasher.Target) { attributes := make(map[int]int, len(target.Attributes)) for _, attr := range target.Attributes { - if attr.GetType() != buildpb.Attribute_STRING || attr.Name == nil || attr.StringValue == nil { + if attr == nil || attr.GetType() != buildpb.Attribute_STRING || attr.Name == nil || attr.StringValue == nil { continue } attrNameID := getOrGenerateRecordReverse(*attr.Name, g.AttrNameToID, g.AttrNameIDToString) diff --git a/core/itg/graph/graph_test.go b/core/itg/graph/graph_test.go index 9f5663d7..f444d0d0 100644 --- a/core/itg/graph/graph_test.go +++ b/core/itg/graph/graph_test.go @@ -250,6 +250,24 @@ func TestAddTargetAttributes(t *testing.T) { 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 --- diff --git a/core/itg/graph/invalidate.go b/core/itg/graph/invalidate.go index 2190b07b..e0c22350 100644 --- a/core/itg/graph/invalidate.go +++ b/core/itg/graph/invalidate.go @@ -165,7 +165,7 @@ func (g *OptimizedGraph) upsertTarget(target *targethasher.Target, invalidated I attributes := make(map[int]int, len(target.Attributes)) for _, attr := range target.Attributes { - if attr.GetType() != buildpb.Attribute_STRING || attr.Name == nil || attr.StringValue == nil { + if attr == nil || attr.GetType() != buildpb.Attribute_STRING || attr.Name == nil || attr.StringValue == nil { continue } attrNameID := getOrGenerateRecordReverse(*attr.Name, g.AttrNameToID, g.AttrNameIDToString) diff --git a/core/itg/graph/invalidate_test.go b/core/itg/graph/invalidate_test.go index 7e61188f..c67d5bfc 100644 --- a/core/itg/graph/invalidate_test.go +++ b/core/itg/graph/invalidate_test.go @@ -267,6 +267,23 @@ func TestUpsertTarget(t *testing.T) { 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{ From fd527e9d8e12363dbdb0f35cc0d60f7658bb0b38 Mon Sep 17 00:00:00 2001 From: yushan Date: Tue, 22 Sep 2026 23:09:32 +0000 Subject: [PATCH 3/3] test(itg): verify tags order does not affect computeAvailableHashes Directly answers the PR review question on this branch about whether StringListValue order (e.g. tags = ["a","b"] vs ["b","a"]) affects the target hash. It doesn't: HashRuleCommon's canonical encoder sorts string-list attribute values before hashing, so itg's computeAvailableHashes produces an identical HashWithoutDeps for both orderings. Verified this assertion actually exercises the sort by temporarily removing it and confirming the new test fails. Co-Authored-By: Claude Sonnet 5 --- core/itg/graph/update_test.go | 41 +++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) 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}