Skip to content
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

Add feature in JsonBodyFilters to replace value by custom function. #855

Merged
merged 3 commits into from
Oct 14, 2020

Conversation

dnandola-wyze
Copy link
Contributor

@dnandola-wyze dnandola-wyze commented Oct 8, 2020

Summary:
Currently Json Body filters obfusicate the property values with a replacement string. eg. "XXX". However, users may want to do a custom obfusication of these values for eg. Hash of the given value. In order to make it extensible, add a function in JSonBodyFilter to convert a primitive JSON using a custom function provided by the user.

Description:
The following changes are made:

  • Make PrimitiveJsonPropertyBodyFilter as abstract class, where child class will provide replacement function.
  • Change Pattern regular expression to add groupname "propertyValue" for grouping the value.
  • Add two subclass for string replacement and function replacement.

Motivation and Context

Adds extension to help users create a custom filter function.
Fixes #851
#755

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation. (Not sure about this)
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@whiskeysierra
Copy link
Collaborator

I'd prefer an easier change:

  • Leave the structure of PrimitiveJsonPropertyBodyFilter as it is
  • Change private final String replacement to private final BinaryOperator<String> replacement
  • Change all static methods that are calling new PrimitiveJsonPropertyBodyFilter(..., replacement) to new PrimitiveJsonPropertyBodyFilter(..., (name, value) -> replacement)

@whiskeysierra
Copy link
Collaborator

In other words, make the current default implementation (string replacement) a special case of the new default implementation (function-based replacement).

@dnandola-wyze
Copy link
Contributor Author

dnandola-wyze commented Oct 8, 2020

In other words, make the current default implementation (string replacement) a special case of the new default implementation (function-based replacement).

I actually had implemented it that way initially. The issue with that is it will break the BodyFilter merge functionality. Because it will become a bit complicated to compare replacement function instead of replacement string:

public boolean compatibleWith(final PrimitiveJsonPropertyBodyFilter that) {
return pattern.equals(that.pattern)
&& replacement.equals(that.replacement);
}

@whiskeysierra
Copy link
Collaborator

That works if you have a non-anonymous function. So instead of my suggestion from above ((name, value) -> replacement) you need a real class that overrides equals/hashCode and then it works:

@AllArgsConstructor
private static final class StaticReplacement implements BinaryOperator<String> {

    private final String replacement;

    // TODO apply
    // TODO equals/hashCode
}

@dnandola-wyze dnandola-wyze force-pushed the feature/JsonReplaceFunction branch from d80836c to e582f8b Compare October 12, 2020 01:35
@dnandola-wyze
Copy link
Contributor Author

Thanks for the feedback. I hadn't thought of that. I have updated the PR with the changes.

}

static BodyFilter replacePrimitive(
final Predicate<String> predicate, final String replacement) {
return create(PRIMITIVE, predicate, quote(replacement));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. I didn't realize that we always quoted here. Means there is no way to replace booleans and numbers because they will become strings.

@whiskeysierra
Copy link
Collaborator

Looks good. Let me fix the issue in the master first. Might take a couple of days.


@AllArgsConstructor
@EqualsAndHashCode
final class QuotedStringReplacementOperator<T> implements BinaryOperator<String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need for this class to be generic, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Not in current use cases, I guess. It can be removed if you want.


@AllArgsConstructor
@EqualsAndHashCode
final class StaticParameterReplacementOperator<T> implements BinaryOperator<String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. T is always String, no need to be generic.

Copy link
Contributor Author

@dnandola-wyze dnandola-wyze Oct 13, 2020

Choose a reason for hiding this comment

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

Not in this case. It could be number or boolean. Or I guess, it can be passed as string through constructor.

@whiskeysierra
Copy link
Collaborator

If you rebase onto main, then I can fix the final review comments myself before releasing.

@dnandola-wyze dnandola-wyze force-pushed the feature/JsonReplaceFunction branch from f4c4ced to e72cfba Compare October 13, 2020 23:53
@dnandola-wyze
Copy link
Contributor Author

I rebased it with main.

@whiskeysierra
Copy link
Collaborator

👍

@whiskeysierra whiskeysierra merged commit e378d2c into zalando:main Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide function in JSON Body filter to obfuscate values rather than replacing completely.
2 participants