Conversation
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
…ass this means just the instance variables more or less. since not all the values in the instance variables are primitives, do a deep clone so as to ensure everythign is a copy and not a shared reference to an object Signed-off-by: Amndeep Singh Mann <amann@mitre.org>
| }); | ||
|
|
||
| return new Control(unflatten(flattened)); | ||
| return new Control(this); |
There was a problem hiding this comment.
Is there a reason not to remove this function entirely?
There was a problem hiding this comment.
@em-c-rod I agree, there isn't any references to control.toUnformattedObject() only the Profile.toUnformattedObject() is used.
Therefore, I don't know what issue this was causing to warrant being changed during testing!
There was a problem hiding this comment.
@em-c-rod it seems like it's useful as a pseudo-factory function to create a clone of the original clone for whatever reason it was needed.
in any case, i want to make minimal changes here so reduce scope creep of something that's already larger than i want it to be
| this.tags = {}; | ||
| if (data) { | ||
| Object.entries(data).forEach(([key, value]) => { | ||
| Object.entries(_.cloneDeep(data)).forEach(([key, value]) => { |
There was a problem hiding this comment.
Would the original purpose of the usage of the flat library have been to ensure a deep clone of the control object?
There was a problem hiding this comment.
@DMedina6 The _.cloneDeep is creating a copy (copying by value, recursively, including nested objects) of data and assigning to this. If data gets changes this is not changed. We do not make use (not in the SAF CLI) of the Control contractor with data.
I'm ok with this change.
There was a problem hiding this comment.
@DMedina6 that's what it seems like might be happening to me but idk what the point of the loop was with the string only values
|
Cloned and ran the tests and everything passed on my end. |
georgedias
left a comment
There was a problem hiding this comment.
I'm ok with proposed changes.
| this.tags = {}; | ||
| if (data) { | ||
| Object.entries(data).forEach(([key, value]) => { | ||
| Object.entries(_.cloneDeep(data)).forEach(([key, value]) => { |
There was a problem hiding this comment.
@DMedina6 The _.cloneDeep is creating a copy (copying by value, recursively, including nested objects) of data and assigning to this. If data gets changes this is not changed. We do not make use (not in the SAF CLI) of the Control contractor with data.
I'm ok with this change.
| }); | ||
|
|
||
| return new Control(unflatten(flattened)); | ||
| return new Control(this); |
There was a problem hiding this comment.
@em-c-rod I agree, there isn't any references to control.toUnformattedObject() only the Profile.toUnformattedObject() is used.
Therefore, I don't know what issue this was causing to warrant being changed during testing!
It's causing issues downstream with the esm/cjs stuff esp in jest, also I don't think it is necessary at all.