Perform Go map key sorting inside a scope (#522)

This allows defining local variables without them conflicting with other local variables for the same parent message. This patch also adds a test case to prevent regressions in the future. Signed-off-by: Alex Konradi <[email protected]>
bufbuild · Sep 30, 2021 · 8c0f637 · 8c0f637
1 parent 8b7ce13
commit 8c0f637
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 20 deletions.
diff --git a/templates/goshared/map.go b/templates/goshared/map.go
@@ -37,28 +37,30 @@ const mapTpl = `
 
 	{{ if or $r.GetNoSparse (ne (.Elem "" "").Typ "none") (ne (.Key "" "").Typ "none") }}
 		{{- /* Sort the keys to make the iteration order (and therefore failure output) deterministic. */ -}}
-		sorted_keys := make([]{{ (typ .Field).Key }}, len({{ accessor . }}))
-		i := 0
-		for key := range {{ accessor . }} {
-			sorted_keys[i] = key
-			i++
-		}
-		sort.Slice(sorted_keys, func (i, j int) bool { return sorted_keys[i] < sorted_keys[j] })
-		for _, key := range sorted_keys {
-			val := {{ accessor .}}[key]
-			_ = val
+		{
+			sorted_keys := make([]{{ (typ .Field).Key }}, len({{ accessor . }}))
+			i := 0
+			for key := range {{ accessor . }} {
+				sorted_keys[i] = key
+				i++
+			}
+			sort.Slice(sorted_keys, func (i, j int) bool { return sorted_keys[i] < sorted_keys[j] })
+			for _, key := range sorted_keys {
+				val := {{ accessor .}}[key]
+				_ = val
 
-			{{ if $r.GetNoSparse }}
-				if val == nil {
-					err := {{ errIdx . "key" "value cannot be sparse, all pairs must be non-nil" }}
-					if !all { return err }
-					errors = append(errors, err)
-				}
-			{{ end }}
+				{{ if $r.GetNoSparse }}
+					if val == nil {
+						err := {{ errIdx . "key" "value cannot be sparse, all pairs must be non-nil" }}
+						if !all { return err }
+						errors = append(errors, err)
+					}
+				{{ end }}
 
-			{{ render (.Key "key" "key") }}
+				{{ render (.Key "key" "key") }}
 
-			{{ render (.Elem "val" "key") }}
+				{{ render (.Elem "val" "key") }}
+			}
 		}
 	{{ end }}
 

diff --git a/tests/harness/cases/maps.proto b/tests/harness/cases/maps.proto
@@ -30,3 +30,9 @@ message MapRecursive {
 }
 
 message MapExactIgnore  { map<uint64,string> val = 1 [(validate.rules).map = {min_pairs: 3, max_pairs: 3, ignore_empty: true}]; }
+
+message MultipleMaps {
+  map <uint32, string> first = 1 [(validate.rules).map.keys.uint32.gt = 0];
+  map <int32, bool> second = 2 [(validate.rules).map.keys.int32.lt = 0];
+  map <int32, bool> third = 3 [(validate.rules).map.keys.int32.gt = 0];
+}
diff --git a/tests/harness/executor/cases.go b/tests/harness/executor/cases.go
@@ -1172,6 +1172,7 @@ var mapCases = []TestCase{
 	{"map - recursive - valid", &cases.MapRecursive{Val: map[uint32]*cases.MapRecursive_Msg{1: {Val: "abc"}}}, 0},
 	{"map - recursive - invalid", &cases.MapRecursive{Val: map[uint32]*cases.MapRecursive_Msg{1: {}}}, 1},
 	{"map - exact - valid (ignore_empty)", &cases.MapExactIgnore{Val: nil}, 0},
+	{"map - multiple - valid", &cases.MultipleMaps{First: map[uint32]string{1: "a", 2: "b"}, Second: map[int32]bool{-1: true, -2: false}}, 0},
 }
 
 var oneofCases = []TestCase{
@@ -1421,7 +1422,7 @@ var kitchenSink = []TestCase{
 	{"kitchensink - many - all non-message fields invalid", &cases.KitchenSinkMessage{Val: &cases.ComplexTestMsg{BoolConst: true, FloatVal: &wrapperspb.FloatValue{}, TsVal: &timestamppb.Timestamp{}, FloatConst: 8, AnyVal: &anypb.Any{TypeUrl: "asdf"}, RepTsVal: []*timestamppb.Timestamp{{Nanos: 1}}}}, 13},
 }
 
-var nestedCases = []TestCase {
+var nestedCases = []TestCase{
 	{"nested wkt uuid - field - valid", &cases.WktLevelOne{Two: &cases.WktLevelOne_WktLevelTwo{Three: &cases.WktLevelOne_WktLevelTwo_WktLevelThree{Uuid: "f81d16ef-40e2-40c6-bebc-89aaf5292f9a"}}}, 0},
 	{"nested wkt uuid - field - invalid", &cases.WktLevelOne{Two: &cases.WktLevelOne_WktLevelTwo{Three: &cases.WktLevelOne_WktLevelTwo_WktLevelThree{Uuid: "not-a-valid-uuid"}}}, 1},
 }