From 37d59b58bf5a0d60680fc3b27e18a82068d87784 Mon Sep 17 00:00:00 2001
From: Xiaoyang Tan <tanx@uber.com>
Date: Fri, 29 May 2026 08:44:45 -0700
Subject: [PATCH] Reserve id 0 in NameIDMapper for proto3 unset

NameIDMapper handed out 0 as a legitimate id, colliding with proto3
semantics where 0 is the default/unset value for int32. Consumers using
encoding/json (which honors omitempty) silently dropped any target,
rule type, tag, or attribute whose id happened to be 0, making the
entry invisible by name in the response.

Start ids at 1 in both NameIDMapper and the topologically-ordered
targetNamesMapping in ResultToGetTargetGraphResponse, keeping 0 reserved.
Add a regression test asserting 0 is never assigned.
---
 core/common/nameidmapper.go      |  7 +++++--
 core/common/nameidmapper_test.go | 31 +++++++++++++++++++++----------
 core/common/utils.go             |  4 +++-
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/core/common/nameidmapper.go b/core/common/nameidmapper.go
index a2c8dd0..9577b00 100644
--- a/core/common/nameidmapper.go
+++ b/core/common/nameidmapper.go
@@ -15,7 +15,10 @@
 package common
 
 // NameIDMapper assigns stable int32 IDs to string names on demand.
-// IDs are assigned sequentially starting from 0.
+// IDs are assigned sequentially starting from 1. Zero is reserved as the
+// proto3 "unset" sentinel so consumers using encoding/json (which honors
+// `omitempty` on int32 fields) or any client that treats GetId() == 0 as
+// missing never silently lose real entries.
 type NameIDMapper struct {
 	nameToID map[string]int32
 	nextID   int32
@@ -25,7 +28,7 @@ type NameIDMapper struct {
 func NewNameIDMapper() *NameIDMapper {
 	return &NameIDMapper{
 		nameToID: make(map[string]int32),
-		nextID:   0,
+		nextID:   1,
 	}
 }
 
diff --git a/core/common/nameidmapper_test.go b/core/common/nameidmapper_test.go
index 90f0ac6..008e2fc 100644
--- a/core/common/nameidmapper_test.go
+++ b/core/common/nameidmapper_test.go
@@ -15,6 +15,7 @@
 package common
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -24,15 +25,15 @@ func TestNameIDMapper_AssignsSequentialAndStableIDs(t *testing.T) {
 	mapper := NewNameIDMapper()
 
 	idA := mapper.ID("a")
-	assert.Equal(t, int32(0), idA)
+	assert.Equal(t, int32(1), idA, "first id must be 1; 0 is reserved as proto3 unspecified")
 	idB := mapper.ID("b")
-	assert.Equal(t, int32(1), idB)
+	assert.Equal(t, int32(2), idB)
 	// Re-requesting 'a' should return the same id
 	idA2 := mapper.ID("a")
 	assert.Equal(t, idA, idA2)
 	// A new, third name should get the next sequential id
 	idC := mapper.ID("c")
-	assert.Equal(t, int32(2), idC)
+	assert.Equal(t, int32(3), idC)
 }
 
 func TestNameIDMapper_MappingAndInvert(t *testing.T) {
@@ -44,18 +45,28 @@ func TestNameIDMapper_MappingAndInvert(t *testing.T) {
 
 	mapping := mapper.Mapping()
 	assert.Equal(t, 3, len(mapping))
-	assert.Equal(t, int32(0), mapping["x"])
-	assert.Equal(t, int32(1), mapping["y"])
-	assert.Equal(t, int32(2), mapping["z"])
+	assert.Equal(t, int32(1), mapping["x"])
+	assert.Equal(t, int32(2), mapping["y"])
+	assert.Equal(t, int32(3), mapping["z"])
 
 	inv := mapper.Invert()
 	assert.Equal(t, 3, len(inv))
-	assert.Equal(t, "x", inv[0])
-	assert.Equal(t, "y", inv[1])
-	assert.Equal(t, "z", inv[2])
+	assert.Equal(t, "x", inv[1])
+	assert.Equal(t, "y", inv[2])
+	assert.Equal(t, "z", inv[3])
+	_, hasZero := inv[0]
+	assert.False(t, hasZero, "id 0 must remain reserved")
 
 	// Mutating the inverted map should not affect the original mapping
-	inv[3] = "extra"
+	inv[4] = "extra"
 	_, ok := mapping["extra"]
 	assert.False(t, ok)
 }
+
+func TestNameIDMapper_NeverAssignsZero(t *testing.T) {
+	mapper := NewNameIDMapper()
+	for i := 0; i < 1000; i++ {
+		id := mapper.ID(fmt.Sprintf("name-%d", i))
+		assert.NotEqual(t, int32(0), id)
+	}
+}
diff --git a/core/common/utils.go b/core/common/utils.go
index 4dd3b7a..91a45ac 100644
--- a/core/common/utils.go
+++ b/core/common/utils.go
@@ -147,9 +147,11 @@ func HashRequestOptions(opts *tangopb.RequestOptions) string {
 // ResultToGetTargetGraphResponse converts a Result to a GetTargetGraphResponse
 func ResultToGetTargetGraphResponse(result targethasher.Result) ([]*tangopb.GetTargetGraphResponse, error) {
 	// Map target names to ids. This list is topologically sorted, so the ids are stable.
+	// IDs start at 1 — 0 is reserved as the proto3 "unset" sentinel so consumers using
+	// encoding/json (which honors `omitempty` on int32 fields) never silently lose a target.
 	targetNamesMapping := make(map[string]int32, len(result.TargetNames))
 	for i, name := range result.TargetNames {
-		targetNamesMapping[name] = int32(i)
+		targetNamesMapping[name] = int32(i + 1)
 	}
 
 	ruleTypeMapper := NewNameIDMapper()