plugins/logs: Fixes unintended mutation of result #2808
Conversation
There was a problem hiding this comment.
@gshively11 thanks for submitting this! Left a few comments on the implementation.
|
Let me know if that |
| // DeepCopy performs a recursive deep copy for nested slices/maps and | ||
| // returns the copied object. Supports []interface{} | ||
| // and map[string]interface{} only | ||
| func DeepCopy(val interface{}) interface{} { |
There was a problem hiding this comment.
nit: I'd add some test coverage here now that this is exposed for re-use within the rest of OPA.
| @@ -254,20 +261,20 @@ func (r maskRule) mkdirp(node map[string]interface{}, path []string, value inter | |||
| return nil | |||
| } | |||
|
|
|||
| func resultValueToMaskRules(rv interface{}) ([]*maskRule, error) { | |||
|
|
|||
| func newMaskRuleSet(rv interface{}, onRuleError func(*maskRule, error)) (*maskRuleSet, error) { | |||
There was a problem hiding this comment.
onRuleError looks fine to me; definitely better than creating a cyclic dependency or adding logging into the mask.go file.
|
I spent a while trying to make something that could recursively compare the reference of the deep copy target/result, but I just don't have enough golang experience to pull that off, so I just went with this more simplistic mutation test, hope that's ok. |
There was a problem hiding this comment.
LGTM. @gshively11 can you squash your commits? Once that's done we can merge this.
When mask rules targeted /result, it was modifying both the result in the decision logs (intended) and the result in the API response (unintended). Added a step to deep copy the result only once, if there is at least one mask rule targeting the result. Fixes #2752 Signed-off-by: Grant Shively <[email protected]>
When mask rules targeted /result, it was modifying both the result
in the decision logs (intended) and the result in the API
response (unintended). Added a step to deep copy the result only once, if
there is at least one mask rule targeting the result.
Fixes #2752
Signed-off-by: Grant Shively [email protected]