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

Masking Strategies: Handle Null Values and Non-Strings #2377

Merged
merged 3 commits into from
Jan 26, 2023

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Jan 26, 2023

Closes: #2391

Code Changes

  • Hash, Hmac, and AES Encrypt masking strategies adjusted to better handle null values and non-strings (here we try to coerce to a string instead of failing)

Steps to Confirm

  • list any manual steps for reviewers to confirm the changes

Pre-Merge Checklist

Description Of Changes

Several of our masking strategies are set up to expect values to be strings. There are some early exits if the input value is None, but since most inputs are now a list of values, even a list of one value, so that's not likely to be True.

If an array of values are passed in, if the original index is None, do nothing to that index. Also try to coerce the original value to a string.

…an array or non string values are passed in.

If an array of values are passed in, if the original value is None, do nothing.  Also try to coerce the original value to a string.
@pattisdr pattisdr self-assigned this Jan 26, 2023
@pattisdr
Copy link
Contributor Author

@adamsachs wanted to get your thoughts on the strategy here

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

this looks good to me @pattisdr! nice quick fix.

the None handling is exactly what i'd expect, and that's the most important piece.

the non-string-type coercion is a tricky one, and i agree with the approach of trying -- essentially, why not? my only question is whether there's anything we can do to make a somewhat-expected type coercion error from the DB clearly communicated to the end user. maybe it's already going to be clear in how it's set up now, i'm not sure -- just wanted to raise point up to ensure it's considered.

@pattisdr pattisdr marked this pull request as ready for review January 26, 2023 17:12
@pattisdr pattisdr merged commit 84838b4 into main Jan 26, 2023
@pattisdr pattisdr deleted the masking_strategies_handle_non_strings branch January 26, 2023 21:56
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.

Masking Strategies Failing on Null/Non Strings
4 participants