topdown/strings: make strings.replace_n sort its pattern object keys

In the existing implementation, looping over the map was indeterministic,
and hence the result of subsequent evaluations of a string.replacen_n call
with overlapping patterns would be, too.

Now, the builtin will sort its keys before building the strings.Replacer.

Before, the ordering of overlapping patterns wasn't under the user's control,
and after, it still isn't. But it's deterministic.

Signed-off-by: Stephan Renatus <srenatus@chef.io>

test/cases/testdata/replacen/test-replacen-0374.yaml

@@ -14,3 +14,27 @@ cases:
   want_result:
   - x:
     - This is <b>HTML</b>!
+- data: {}
+  modules:
+  - |
+    package test
+
+    p = strings.replace_n({"f": "x", "foo": "xxx"}, "foobar")
+  note: replacen/replace multiple patterns/overlapping
+  query: data.test.p = x
+  want_result:
+  - x: xoobar
+- data: {}
+  modules:
+  - |
+    package test
+
+    p = x {
+      x := strings.replace_n({ k: v | k := ["f", "foo"][i]; v := ["x", "xxx"][i]}, "foo")
+      y := strings.replace_n({ k: v | k := ["foo", "f"][i]; v := ["xxx", "x"][i]}, "foo")
+      x == y
+    }
+  note: replacen/replace multiple patterns/overlapping/insertion order does not matter
+  query: data.test.p = x
+  want_result:
+  - x: xoo

test/cases/testdata/replacen/test-replacen-bad-operands.yaml

@@ -0,0 +1,34 @@
+cases:
+- data: {}
+  modules:
+  - |
+    package test
+
+    p = strings.replace_n({2: "x" | true}, "foo")
+  note: replacen/bad pattern object operand/non-string key
+  query: data.test.p = x
+  want_error: "strings.replace_n: operand 1 non-string key found in pattern object"
+  want_error_code: eval_type_error
+- data:
+    pattern:
+      f: 100
+  modules:
+  - |
+    package test
+
+    p = strings.replace_n(data.pattern, "foo")
+  note: replacen/bad pattern object operand/non-string value
+  query: data.test.p = x
+  want_error: "strings.replace_n: operand 1 non-string value found in pattern object"
+  want_error_code: eval_type_error
+- data:
+    string: 100
+  modules:
+  - |
+    package test
+
+    p = strings.replace_n({"foo":"baz"}, data.string)
+  note: replacen/bad pattern object operand/non-string value
+  query: data.test.p = x
+  want_error: "strings.replace_n: operand 2 must be string but got number"
+  want_error_code: eval_type_error

topdown/strings.go

@@ -5,8 +5,8 @@
 package topdown
 
 import (
-	"errors"
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/open-policy-agent/opa/ast"
@@ -234,27 +234,33 @@ func builtinReplace(a, b, c ast.Value) (ast.Value, error) {
 }
 
 func builtinReplaceN(a, b ast.Value) (ast.Value, error) {
-	asJSON, err := ast.JSON(a)
+	patterns, err := builtins.ObjectOperand(a, 1)
 	if err != nil {
 		return nil, err
 	}
-	oldnewObj, ok := asJSON.(map[string]interface{})
-	if !ok {
-		return nil, builtins.NewOperandTypeErr(1, a, "object")
-	}
+	keys := patterns.Keys()
+	sort.Slice(keys, func(i, j int) bool { return ast.Compare(keys[i].Value, keys[j].Value) < 0 })
 
 	s, err := builtins.StringOperand(b, 2)
 	if err != nil {
 		return nil, err
 	}
 
 	var oldnewArr []string
-	for k, v := range oldnewObj {
-		strVal, ok := v.(string)
+	for _, k := range keys {
+		keyVal, ok := k.Value.(ast.String)
+		if !ok {
+			return nil, builtins.NewOperandErr(1, "non-string key found in pattern object")
+		}
+		val := patterns.Get(k) // cannot be nil
+		strVal, ok := val.Value.(ast.String)
 		if !ok {
-			return nil, errors.New("non-string value found in pattern object")
+			return nil, builtins.NewOperandErr(1, "non-string value found in pattern object")
 		}
-		oldnewArr = append(oldnewArr, k, strVal)
+		oldnewArr = append(oldnewArr, string(keyVal), string(strVal))
+	}
+	if err != nil {
+		return nil, err
 	}
 
 	r := strings.NewReplacer(oldnewArr...)