Skip to content

Conversation

@jamfor999
Copy link
Contributor

@jamfor999 jamfor999 commented Jan 25, 2025

Several fixes here:

  1. I realised after further usage and some extensive testing in more complex scenarios that JSON Arrays weren't handled correctly - they were always either masked at the whole field level (on Mask By Default mode), or left unmasked (on Show By Default mode). Now, the same settings that apply now also apply to nested objects inside arrays, or if the array contains primitives, the higher level array will apply. So for example, given:
address.firstLine

this will now apply to both:

{
  "address": {
      "firstLine": "value"
  }
}

and now also to all the firstLine values in an array type, too (this was not working correctly previously):

{
  "address": [
    {
      "firstLine": "value"
    },
    {
      "firstLine": "someOtherValue"
    }
  ]
}

Primitives will be handled as expected as well, where a filter like:

address.values

would now result that the following object:

{
    "address": {
      "values": [ "one", "two", "three" ]
    }
}

will correctly look like:

{
    "address": {
      "values": [ "xxxx", "xxxx", "xxxx" ]
    }
}

instead of the current:

{
    "address": {
      "values": "xxxx"
    }
}

which is currently losing the format, as well as non-sensitive information such as array length.

  1. Some readability improvements & fixes, also updating tests to be uniform, so the exact same test code is not repeated. Now the JsonMaskerTest interface defines the core tests the input and output, with the ShowByDefault and MaskByDefault tests implementing the interface (with their own backing application.yml which makes them fit the test criteria) and have their own specific tests written in the class only

  2. Small performance improvements, including defining the topic -> filtering keys in the constructor rather than constantly re-evaluating

  3. 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

@jamfor999 jamfor999 changed the title fix(masking): Various performance and readability fixes fix(masking): Various performance, functional, and readability fixes for JSON-based masking Jan 25, 2025
@jamfor999
Copy link
Contributor Author

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

@jamfor999
Copy link
Contributor Author

hi @tchiotludo I was wondering if I could get a quick look on this please? It would be great :)
Thank you

@AlexisSouquiere
Copy link
Collaborator

@jamfor352 I'm starting to review the PR !

Copy link
Collaborator

@AlexisSouquiere AlexisSouquiere left a 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.*;
Copy link
Collaborator

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) {
Copy link
Collaborator

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);
}
Copy link
Collaborator

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 ?

Copy link
Contributor Author

@jamfor999 jamfor999 Feb 28, 2025

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) {
Copy link
Collaborator

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

Copy link
Contributor Author

@jamfor999 jamfor999 Feb 28, 2025

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) {
Copy link
Collaborator

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge with applyMasking

@jamfor999
Copy link
Contributor Author

THanks for the review @AlexisSouquiere , I've addressed your comments :)

@jamfor999
Copy link
Contributor Author

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.

@jamfor999
Copy link
Contributor Author

jamfor999 commented Mar 14, 2025

Hi @tchiotludo @AlexisSouquiere is there any chance this could be reviewed soon? My team is itching to get the array issue fixed :)

Copy link
Collaborator

@AlexisSouquiere AlexisSouquiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jamfor999
Copy link
Contributor Author

jamfor999 commented Mar 14, 2025

LGTM

Thank you!
I think this is ready to merge now @tchiotludo , I won't be updating this PR any more as I'm happy with the state of it

@tchiotludo tchiotludo merged commit f4ae29d into tchiotludo:dev Mar 14, 2025
4 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in Backlog Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants