(Thrift Field Masker Pt2) Implement field masking for thrift - #6311
Merged
jrhee17 merged 11 commits intoJul 28, 2025
Conversation
Contributor
jrhee17
marked this pull request as ready for review
July 16, 2025 01:19
Contributor
Author
|
ready for review |
ikhoon
reviewed
Jul 16, 2025
|
|
||
| private static ContentSanitizer<String> commonContentSanitizer() { | ||
| return ContentSanitizer.builder() | ||
| .fieldMaskerSelector(ThriftFieldMaskerSelector.of(info -> { |
Contributor
There was a problem hiding this comment.
What do you think of adding a builder to create a ThriftFieldMaskerSelector in a declarative way for simple cases?
For example:
ThriftFieldMaskerSelector
.buider()
.onFieldAnnotation("secret", FieldMasker.nullify())
.onFieldAnnotation("grade", "red", new CustomFieldMasker())
...
.build();
Contributor
Author
There was a problem hiding this comment.
Added a variant of ThriftFieldMaskerSelector to thrift0.19> since that's when annotations were supported.
Note that this the variant still requires that the thrift idl is compiled with the annotations_as_metadata option
Contributor
There was a problem hiding this comment.
It looks nice. Should we add a integration test case using ThriftFieldMaskerSelectorBuilder as an example so that users can reference it?
minwoox
reviewed
Jul 23, 2025
minwoox
left a comment
minwoox
left a comment
Contributor
There was a problem hiding this comment.
Looks good all in all. 👍
minwoox
reviewed
Jul 23, 2025
minwoox
left a comment
minwoox
left a comment
Contributor
There was a problem hiding this comment.
Looks good all in all. 👍
ikhoon
approved these changes
Jul 24, 2025
| import com.linecorp.armeria.common.logging.FieldMasker; | ||
|
|
||
| /** | ||
| * Holds information about a thrift struct field. |
Contributor
There was a problem hiding this comment.
Suggested change
| * Holds information about a thrift struct field. | |
| * Holds information about a Thrift struct field. |
| @Override | ||
| public FieldMasker fieldMasker(ThriftFieldInfo info) { | ||
| for (AnnotationAndMasker rule : rules) { | ||
| if (rule.matches(info.fieldMetaData().getFieldAnnotations())) { |
Contributor
There was a problem hiding this comment.
Should we extract info.fieldMetaData().getFieldAnnotations() into a local variable since getFieldAnnotations() may create UnmodifiableMap for each invocation.
// Copied from the generated `SecretStruct`
new org.apache.thrift.meta_data.FieldMetaData("secret", org.apache.thrift.TFieldRequirementType.DEFAULT,
new org.apache.thrift.meta_data.FieldValueMetaData(org.apache.thrift.protocol.TType.STRING),
java.util.stream.Stream.<java.util.Map.Entry<java.lang.String, java.lang.String>>builder()
.add(new java.util.AbstractMap.SimpleImmutableEntry<>("grade", "red"))
.build().collect(java.util.stream.Collectors.toMap(java.util.Map.Entry::getKey, java.util.Map.Entry::getValue))));|
|
||
| private static ContentSanitizer<String> commonContentSanitizer() { | ||
| return ContentSanitizer.builder() | ||
| .fieldMaskerSelector(ThriftFieldMaskerSelector.of(info -> { |
Contributor
There was a problem hiding this comment.
It looks nice. Should we add a integration test case using ThriftFieldMaskerSelectorBuilder as an example so that users can reference it?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6311 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ikhoon
pushed a commit
that referenced
this pull request
Aug 4, 2025
Motivation: Following the implementation of #6311, this changeset attempts to implement deserialization of masked fields. Doing so would allow users to define a `FieldMasker` as follows: ``` FieldMasker.builder() .addMasker( NormalFooStruct.class, struct -> encryptToString(struct), str -> return decryptToStruct(str)); ``` Conceptually, this implementation is a corollary of `MaskingBeanDeserializerModifier`. There is no change in the public API as `FieldMaskerBuilder#addMasker(Class, Function, Function)` is already exposed. Modifications: - Added `UnMaskingTProtocol`, `TMaskingDeserializer`, `UnMaskingContexts` which helps with the implementation of unmasking - Deserialization requires instantiation of the default instance. To aid with this, `TBaseCache` has been introduced. Result: - The unmasking API functions correctly for thrift structs
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
3 participants
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.
Motivation:
Following field maskers for annotated services at #6232 , this PR attempts to introduce
FieldMaskersupport for thrift.To ensure that deserialization is also supported natively using
TProtocol, the design of thrift uses a variant ofTProtocolwhich keeps track of the field of a specified instance.Unmasking support is to be done in the ensuing PR for easier reviews.
Modifications:
RpcRequestSerializer,RpcResponseSerializerto serialize rpc requestsThriftFieldInfo,ThriftFieldMaskerSelector, andThriftFieldMaskerSelectorProviderare implemented to support theFieldMaskerAPI.TBaseSelectorCache,TBaseSerializer,MaskingTProtocolare implemented to actually apply field masking during serializationannotations_as_metadataoption to the gradle scriptsResult:
FieldMaskerAPI for thrift services