Introduce obfuscate module with tests #266
Merged
Conversation
This file contains 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
igrigorik
reviewed
Dec 5, 2021
|
|
||
| h.each_value do |v| | ||
| self.email(v) if v.is_a? Hash | ||
| v.each {|e| self.email(e)} if v.is_a? Array |
There was a problem hiding this comment.
There is an unhandled edge case here that recently took down the crawler. I believe the issue was that we had an array of strings and that crashed this function.
There was a problem hiding this comment.
Yeah I believe this is captured in the test - see test_no_obfuscation_for_other_fields which has an "array-string" value.
This bubbled up in local testing and was before I found the fix in prod.
|
Sorry I think I gave you a bunch of debug code in this PR. Will tidy up ASAP. |
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.
I hit an issue when running this locally and narrowed it down to the obfuscate method.
One additional layer here is to add a rescue exception clause which it looks like is being done in production, but it's unclear if that's necessary with this approach, but given the nature of this tool, it might be the best option to keep on going.