Allow custom masking config without defining allowed keys#319
Open
andrebrait wants to merge 1 commit into
Open
Allow custom masking config without defining allowed keys#319andrebrait wants to merge 1 commit into
andrebrait wants to merge 1 commit into
Conversation
|
Note These results are affected by shared workloads on GitHub runners. Use the results only to detect possible regressions, but always rerun on a more stable machine before drawing conclusions! Benchmark resultsBaselineBenchmark
Full results — BaselineBenchmark
InstanceCreationBenchmarkNo significant changes (all within 3.0%). Full results — InstanceCreationBenchmark
JsonMaskerBenchmark
Full results — JsonMaskerBenchmark
LargeKeySetInstanceCreationBenchmark
Full results — LargeKeySetInstanceCreationBenchmark
StreamTypeBenchmark
Full results — StreamTypeBenchmark
ValueMaskerBenchmark
Full results — ValueMaskerBenchmark
Raw output (PR @ 4514245)Raw output (master @ 550a7d4) |
Author
|
I'll take a look and fix it. Weirdly enough, it all passed locally. |
Breus
reviewed
Apr 22, 2026
| private static final JsonPathParser JSON_PATH_PARSER = new JsonPathParser(); | ||
|
|
||
| private final Set<String> targetKeys = new HashSet<>(); | ||
| private final Set<String> allowedKeys = new HashSet<>(); |
Owner
There was a problem hiding this comment.
@andrebrait I don't think this can be called allowedKeys it is only allowed in allow mode, they are target keys and have a TargetKeyMode
Author
There was a problem hiding this comment.
Probably an accidental rename. I'll fix it.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #317 where JSONPath-specific masking configs were ignored in ALLOW mode when no allowed JSONPaths were configured, enabling “mask everything by default, but apply specific configs to selected keys/paths”.
Changes:
- Add coverage for ALLOW mode with empty
allowJsonPaths()while still applying key/JSONPath-specific configs. - Store JSONPath-specific masking configs separately from key configs and wire them into matcher initialization.
- Minor refactors in JSONPath parsing and ambiguity checking (including making
JsonPathcomparable).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/dev/blaauwendraad/masker/json/CustomKeyMaskingConfigTest.java | Adds a regression test for ALLOW mode with empty allowed JSONPaths while using specific configs. |
| src/main/java/dev/blaauwendraad/masker/json/path/JsonPathParser.java | Refactors segment parsing and simplifies ambiguity sorting via Comparable. |
| src/main/java/dev/blaauwendraad/masker/json/path/JsonPath.java | Implements Comparable<JsonPath> to support natural sorting. |
| src/main/java/dev/blaauwendraad/masker/json/config/JsonMaskingConfig.java | Introduces separate JSONPath config storage and exposes JSONPath config accessors. |
| src/main/java/dev/blaauwendraad/masker/json/KeyMatcher.java | Inserts JSONPath config keys into the trie and resolves configs from either key or JSONPath config maps. |
| src/main/java/dev/blaauwendraad/masker/json/KeyContainsMasker.java | Ensures JSONPath tracking is enabled when JSONPath configs exist even if allow-list JSONPaths are empty. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
329
to
333
| targetJsonPaths.add(parsed); | ||
| } | ||
| if (config != null) { | ||
| targetKeyConfigs.put(parsed.toString(), config); | ||
| targetJsonPathConfigs.put(parsed, config); | ||
| } |
There was a problem hiding this comment.
addMaskJsonPath now stores configs in targetJsonPathConfigs (instead of targetKeyConfigs), but the duplicate detection logic above still checks targetKeyConfigs.containsKey(parsed.toString()). In ALLOW mode (where the path isn’t added to targetJsonPaths), adding the same JSONPath config twice will no longer be rejected and will silently overwrite. Please update the duplicate check to consult targetJsonPathConfigs for this case.
Comment on lines
+90
to
+92
| if (symbol == '.' || (symbol == '[' && !segment.isEmpty())) { | ||
| segments.add(segment.toString()); | ||
| segment = new StringBuilder(); | ||
| segment.setLength(0); |
There was a problem hiding this comment.
parseSegments uses segment.isEmpty() on a StringBuilder. This method is not available on JDK 11 (the library’s minimum runtime), so this can compile against a newer JDK but fail at runtime with NoSuchMethodError on Java 11. Use segment.length() != 0 (or segment.length() > 0) instead.
Comment on lines
+105
to
106
| if (!segment.isEmpty() || literal.endsWith("[]")) { | ||
| segments.add(segment.toString()); |
There was a problem hiding this comment.
Same JDK 11 compatibility issue here: StringBuilder#isEmpty() is not available on Java 11. Please replace with a segment.length() check.
| * Returns the config for the given JSON path. If no specific config is available for the given path, returns {@code null}. | ||
| * | ||
| * @param path the JSON path to be masked | ||
| * @return the config for the given key |
There was a problem hiding this comment.
Javadoc mismatch: getJsonPathConfig documents @return the config for the given key, but the method is for a JSON path. Please update the return description to refer to the JSON path to avoid confusion in generated docs.
Suggested change
| * @return the config for the given key | |
| * @return the config for the given JSON path |
Comment on lines
+427
to
+432
| public Builder maskWith(KeyMaskingConfig maskingConfig) { | ||
| defaultConfigBuilder.maskStringsWith(maskingConfig.getStringValueMasker()) | ||
| .maskBooleansWith(maskingConfig.getBooleanValueMasker()) | ||
| .maskNumbersWith(maskingConfig.getNumberValueMasker()); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
maskWith should validate maskingConfig similarly to the other builder methods (e.g., Objects.requireNonNull(maskingConfig)), so callers get a clear exception at the API boundary instead of an indirect NPE from get*ValueMasker().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #317