topdown/strings: make strings.replace_n use builtins.ObjectOperand #2825
Conversation
| - data: {} | ||
| modules: | ||
| - | | ||
| package generated |
There was a problem hiding this comment.
💭 What's the standard for test cases? "generated" isn't true anymore here...
There was a problem hiding this comment.
I've been using package test in most places. We might be able to just migrate all of the existing test cases to use that. Here's the hello-world example that I've pointed people at: https://github.com/open-policy-agent/opa/blob/master/test/cases/testdata/helloworld/test-helloworld-1.yaml
b514dea to
9d7d793
Compare
| - data: {} | ||
| modules: | ||
| - | | ||
| package generated |
There was a problem hiding this comment.
I've been using package test in most places. We might be able to just migrate all of the existing test cases to use that. Here's the hello-world example that I've pointed people at: https://github.com/open-policy-agent/opa/blob/master/test/cases/testdata/helloworld/test-helloworld-1.yaml
topdown/strings.go
Outdated
|
|
||
| s, err := builtins.StringOperand(b, 2) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var oldnewArr []string | ||
| for k, v := range oldnewObj { | ||
| strVal, ok := v.(string) | ||
| err = oldnewObj.Iter(func(k *ast.Term, v *ast.Term) error { |
There was a problem hiding this comment.
The object implementation iterates in insertion order so there could still be different outputs with this change. What about getting the keys from the object, sorting them, and then iterating over the sorted key set?
There was a problem hiding this comment.
🤔 As in strings.replace_n(o1, s) could differ from strings.replace_n(o2, s) although o1 and o2 are the same in terms of key-val pairs? Yup, and that is unfortunate. I had only considered multiple evals of strings.replace_n(o1, s) which should now no longer have different outcomes.
Sorting them would guarantee stable output, of course, but it would also mean that there's no way for the user to control the replacement order. I think this comes down to the interface of strings.replace_n being unfortunate, it's using something unordered, so you cannot control the ordering, end of story.
Since nobody has ever been complaining about this, it's probably not a big deal, the common "foo -> bar, baz -> quz" usecase isn't affected.
tl;dr: I'll update this, sorting the keys, tomorrow. Controlling the replacement ordering is just not going to be supported. (Or stay unsupported, rather.)
There was a problem hiding this comment.
Technically, we could go back to using ast.JSON, and then sort the keys; but I think I like builtins.ObjectOperand better. WDYT?
There was a problem hiding this comment.
tl;dr: I'll update this, sorting the keys, tomorrow. Controlling the replacement ordering is just not going to be supported. (Or stay unsupported, rather.)
Sounds good. We can add that in the future if someone requests it.
Technically, we could go back to using ast.JSON, and then sort the keys; but I think I like builtins.ObjectOperand better. WDYT?
I'd probably just use the ast types. I'd call Keys() and then sort the result.
8c957d5 to
b83154d
Compare
|
@srenatus can you squash the commits? Once that's done we can merge. |
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>
40c344f to
a7a8db2
Compare
|
@tsandall sure! ✔️ |
Fixes #2822.
A quick check with the old and new implementation shows:
So the non-determinism should be dealt with.
Still planning to add test cases to pin down the current behaviour.✔️