From e28e8739c534da0a8e86a04708c40b6594dae0cd Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Tue, 22 Sep 2026 17:23:22 -0700 Subject: [PATCH] Support bool/int/string_list attribute values in the target graph mapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Intent: - Tango's OptimizedTarget.Attributes only ever carried string-valued Bazel rule attributes: mapper.go filtered ingestion to Attribute_STRING and dropped every other type, so a target whose only real change was e.g. a boolean or integer attribute flip was still flagged as changed (via the target hash) but got misclassified during finer-grained diffing since the attribute-equality check never saw the change. Changes: - Replaced the STRING-only filter in mapper.ResultToTargetGraph with attributeValue, which stringifies STRING, BOOLEAN, INTEGER, and STRING_LIST typed attributes into the existing string value intern table. - STRING_LIST values are sorted before encoding, matching the existing precedent in core/targethasher/sourcehasher.go's encodeAttribute (which sorts string_list_value for the same reason: Bazel query does not guarantee stable list ordering across runs when nothing semantically changed). Values are JSON-encoded rather than delimiter-joined so elements containing arbitrary characters can't collide. - Other collection/dict/label attribute types (STRING_DICT, LABEL_LIST, INTEGER_LIST, etc.) are still dropped rather than stringified, since a naive string join isn't safe for them without a canonical encoder. --- Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace --- mapper/BUILD.bazel | 2 + mapper/mapper.go | 57 ++++++++++++++++++++++++- mapper/mapper_test.go | 98 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 2 deletions(-) diff --git a/mapper/BUILD.bazel b/mapper/BUILD.bazel index 0d04e656..ca992035 100644 --- a/mapper/BUILD.bazel +++ b/mapper/BUILD.bazel @@ -20,6 +20,8 @@ go_test( embed = [":mapper"], deps = [ "//core/targethasher", + "//entity", + "@com_github_bazelbuild_buildtools//build_proto", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", ], diff --git a/mapper/mapper.go b/mapper/mapper.go index 2468ce10..743d6dbc 100644 --- a/mapper/mapper.go +++ b/mapper/mapper.go @@ -3,6 +3,9 @@ package mapper import ( "context" "encoding/hex" + "encoding/json" + "slices" + "strconv" buildpb "github.com/bazelbuild/buildtools/build_proto" "github.com/uber/tango/core/targethasher" @@ -69,8 +72,11 @@ func ResultToTargetGraph(ctx context.Context, result targethasher.Result) ([]ent if len(t.Attributes) > 0 { attrs := make(map[int32]int32, len(t.Attributes)) for _, attr := range t.Attributes { - if attr.GetType() == buildpb.Attribute_STRING && attr.Name != nil && attr.StringValue != nil { - attrs[attrNameMapper.ID(*attr.Name)] = attrStrValMapper.ID(*attr.StringValue) + if attr.Name == nil { + continue + } + if val, ok := attributeValue(attr); ok { + attrs[attrNameMapper.ID(*attr.Name)] = attrStrValMapper.ID(val) } } if len(attrs) > 0 { @@ -143,3 +149,50 @@ func ResultToGraphChunks(ctx context.Context, result targethasher.Result, maxByt return chunks, nil } + +// attributeValue returns the canonical string form of a Bazel attribute's +// value for STRING, BOOLEAN, INTEGER, and STRING_LIST typed attributes. +// Other collection/dict/label types are not yet supported and are rejected +// rather than stringified, since a naive string join is not safe for them +// (ambiguous delimiters, unstable ordering). +// +// STRING_LIST values are sorted before encoding: Bazel query does not +// guarantee list ordering is stable across runs when nothing semantically +// changed, so treating order as significant would produce spurious "changed" +// results. This matches core/targethasher/sourcehasher.go's encodeAttribute, +// which sorts string_list_value for the same reason. The sorted list is +// JSON-encoded (not delimiter-joined) so that elements containing arbitrary +// characters can't collide with a different list's encoding. +func attributeValue(attr *buildpb.Attribute) (string, bool) { + switch attr.GetType() { + case buildpb.Attribute_STRING: + if attr.StringValue == nil { + return "", false + } + return *attr.StringValue, true + case buildpb.Attribute_BOOLEAN: + if attr.BooleanValue == nil { + return "", false + } + return strconv.FormatBool(*attr.BooleanValue), true + case buildpb.Attribute_INTEGER: + if attr.IntValue == nil { + return "", false + } + return strconv.FormatInt(int64(*attr.IntValue), 10), true + case buildpb.Attribute_STRING_LIST: + vals := attr.GetStringListValue() + if len(vals) == 0 { + return "", false + } + sorted := slices.Clone(vals) + slices.Sort(sorted) + encoded, err := json.Marshal(sorted) + if err != nil { + return "", false + } + return string(encoded), true + default: + return "", false + } +} diff --git a/mapper/mapper_test.go b/mapper/mapper_test.go index 8e9bb8e7..1b7a546b 100644 --- a/mapper/mapper_test.go +++ b/mapper/mapper_test.go @@ -4,12 +4,20 @@ import ( "context" "testing" + buildpb "github.com/bazelbuild/buildtools/build_proto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/tango/core/targethasher" + "github.com/uber/tango/entity" ) +func attrPtr(s string) *string { return &s } +func boolPtr(b bool) *bool { return &b } +func int32Ptr(i int32) *int32 { return &i } + +func discPtr(d buildpb.Attribute_Discriminator) *buildpb.Attribute_Discriminator { return &d } + func TestResultToTargetGraph_EmptyResult(t *testing.T) { t.Parallel() @@ -114,3 +122,93 @@ func TestResultToGraphChunks(t *testing.T) { assert.Equal(t, hashes, got) }) } + +func TestResultToTargetGraph_ScalarAttributes(t *testing.T) { + t.Parallel() + + result := targethasher.Result{ + TargetNames: []string{"//a:a"}, + Targets: map[string]*targethasher.Target{ + "//a:a": { + Attributes: []*buildpb.Attribute{ + {Name: attrPtr("srcs"), Type: discPtr(buildpb.Attribute_STRING), StringValue: attrPtr("main.go")}, + {Name: attrPtr("testonly"), Type: discPtr(buildpb.Attribute_BOOLEAN), BooleanValue: boolPtr(true)}, + {Name: attrPtr("size"), Type: discPtr(buildpb.Attribute_INTEGER), IntValue: int32Ptr(42)}, + {Name: attrPtr("tags"), Type: discPtr(buildpb.Attribute_STRING_LIST), StringListValue: []string{"b", "a"}}, + // Unsupported type: dropped, not stringified (even though it + // happens to reuse the StringListValue field, its type is LABEL_LIST). + {Name: attrPtr("deps"), Type: discPtr(buildpb.Attribute_LABEL_LIST), StringListValue: []string{"//x:x"}}, + }, + }, + }, + } + + targets, meta, err := ResultToTargetGraph(t.Context(), result) + require.NoError(t, err) + require.Len(t, targets, 1) + + attrs := targets[0].Attributes + assert.Len(t, attrs, 4, "unsupported LABEL_LIST attribute should be dropped") + + got := make(map[string]string, len(attrs)) + for nameID, valID := range attrs { + got[meta.AttributeNameMapping[nameID]] = meta.AttributeStringValueMapping[valID] + } + assert.Equal(t, map[string]string{ + "srcs": "main.go", + "testonly": "true", + "size": "42", + "tags": `["a","b"]`, + }, got) +} + +func TestResultToTargetGraph_StringListAttributeOrderInsensitive(t *testing.T) { + t.Parallel() + + // Bazel query does not guarantee stable list ordering across runs when + // nothing semantically changed, so two snapshots differing only in + // order must intern to the same value (no spurious "changed" attribute). + before := targethasher.Result{ + TargetNames: []string{"//a:a"}, + Targets: map[string]*targethasher.Target{ + "//a:a": { + Attributes: []*buildpb.Attribute{ + {Name: attrPtr("tags"), Type: discPtr(buildpb.Attribute_STRING_LIST), StringListValue: []string{"a", "b", "c"}}, + }, + }, + }, + } + after := targethasher.Result{ + TargetNames: []string{"//a:a"}, + Targets: map[string]*targethasher.Target{ + "//a:a": { + Attributes: []*buildpb.Attribute{ + {Name: attrPtr("tags"), Type: discPtr(buildpb.Attribute_STRING_LIST), StringListValue: []string{"c", "a", "b"}}, + }, + }, + }, + } + + beforeTargets, beforeMeta, err := ResultToTargetGraph(t.Context(), before) + require.NoError(t, err) + afterTargets, afterMeta, err := ResultToTargetGraph(t.Context(), after) + require.NoError(t, err) + + beforeAttrs := attributeValuesByName(beforeTargets[0].Attributes, beforeMeta) + afterAttrs := attributeValuesByName(afterTargets[0].Attributes, afterMeta) + require.Contains(t, beforeAttrs, "tags") + require.Contains(t, afterAttrs, "tags") + + beforeVal := beforeMeta.AttributeStringValueMapping[beforeAttrs["tags"]] + afterVal := afterMeta.AttributeStringValueMapping[afterAttrs["tags"]] + assert.Equal(t, beforeVal, afterVal) +} + +// attributeValuesByName maps attribute name to its interned value ID. +func attributeValuesByName(attrs map[int32]int32, meta *entity.Metadata) map[string]int32 { + byName := make(map[string]int32, len(attrs)) + for nameID, valID := range attrs { + byName[meta.AttributeNameMapping[nameID]] = valID + } + return byName +}