-
-
Notifications
You must be signed in to change notification settings - Fork 753
fix(masking): Various performance, functional, and readability fixes for JSON-based masking #2051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc: @AlexisSouquiere since you have familiarity reviewing this code beforehand :) A few things changed as explained in the PR. Makes it more reliable and functional and should improve performance (a bit) too |
|
hi @tchiotludo I was wondering if I could get a quick look on this please? It would be great :) |
|
@jamfor352 I'm starting to review the PR ! |
AlexisSouquiere
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add some comments to explain that you handle nested objects aa.bb but that it also applies to arrays ? For maintainability :)
| import com.google.gson.JsonElement; | ||
| import com.google.gson.JsonObject; | ||
| import com.google.gson.JsonParser; | ||
| import com.google.gson.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you avoid using * and import only the specific types ?
| this.topicToKeysMap = buildTopicKeysMap(dataMasking); | ||
| } | ||
|
|
||
| private Map<String, List<String>> buildTopicKeysMap(DataMasking dataMasking) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is duplicated in JsonMaskByDefaultMasker
| this.jsonMaskingFilters = dataMasking.getJsonFilters(); | ||
| this.jsonMaskReplacement = dataMasking.getJsonMaskReplacement(); | ||
| this.topicToKeysMap = buildTopicKeysMap(dataMasking); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move all the common variables / constructor upwards in Masker ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move all the common variables / constructor upwards in Masker ?
Masker is an interface, but I can make an intermediary abstract class, for sure
| return topicToKeysMap.getOrDefault(topic.toLowerCase(), Collections.emptyList()); | ||
| } | ||
|
|
||
| private Record createNonJsonRecord(Record record) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 simple functions have a single reference in maskRecord. I think you can remove them and do the job in maskRecord directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 simple functions have a single reference in maskRecord. I think you can remove them and do the job in maskRecord directly
while true, I wanted to make the intention behind them more readable as lots of detail in one method can be difficult to parse. But I also appreciate these bits may be overly simple and add too much to the stack, so I'll adjust it
| return record; | ||
| } | ||
|
|
||
| private Record maskJsonRecord(Record record) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge with maskJsonRecord as it's only used here
| if (node.has(keys[index])) { | ||
| node.addProperty(keys[index], jsonMaskReplacement); | ||
| } | ||
| private String[][] preProcessPaths(List<String> maskedKeys) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge with applyMasking
|
THanks for the review @AlexisSouquiere , I've addressed your comments :) |
|
Realised I missed the documentation request. I've added this, and also added more testing as well @AlexisSouquiere , so hopefully it should all be up to scratch now. |
|
Hi @tchiotludo @AlexisSouquiere is there any chance this could be reviewed soon? My team is itching to get the array issue fixed :) |
AlexisSouquiere
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you! |
Several fixes here:
this will now apply to both:
and now also to all the
firstLinevalues in an array type, too (this was not working correctly previously):Primitives will be handled as expected as well, where a filter like:
would now result that the following object:
will correctly look like:
instead of the current:
which is currently losing the format, as well as non-sensitive information such as array length.
Some readability improvements & fixes, also updating tests to be uniform, so the exact same test code is not repeated. Now the
JsonMaskerTestinterface defines the core tests the input and output, with theShowByDefaultandMaskByDefaulttests implementing the interface (with their own backingapplication.ymlwhich makes them fit the test criteria) and have their own specific tests written in the class onlySmall performance improvements, including defining the topic -> filtering keys in the constructor rather than constantly re-evaluating
Remove the phrase "Please contact akhq administrator" which is likely to cause confusion and bring people here, when the issue isn't AKHQ but that a record has not fulfilled the criteria for masking