-
Notifications
You must be signed in to change notification settings - Fork 57
fix: add check for arrays in hashAttributes #1147
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
base: development
Are you sure you want to change the base?
fix: add check for arrays in hashAttributes #1147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the hashAttributes function to properly handle array values by skipping the hashing process for them, addressing an issue where arrays were being incorrectly processed.
Changes:
- Added array type check in
hashAttributesto skip hashing for array values - Updated comment to clarify the new behavior
- Added comprehensive test coverage for array handling (both empty and non-empty arrays)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/roktManager.ts | Added Array.isArray() check to skip hashing array values and return null for hashedValue; updated inline comment |
| test/jest/roktManager.spec.ts | Added new test case verifying that array values (both empty and non-empty) are preserved without creating hashed versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fix typo Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
remove type assertion Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const attributeValue = attributes[key]; | ||
| if (Array.isArray(attributeValue)) { | ||
| return { key, attributeValue, hashedValue: null }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaissica12
what happens when you call hashAttributes on a value that's an arry using the Rokt SDK? we want to make sure the behavior is the same. something to flag to Matt if it's doing it improperly there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmi22186 In Rokt SDK when an array is passed for hashing, it is converted to a string using JavaScript String() coercion, which joins array elements with commas and then hashes them.
Example:
- Input: { favorite_colors: ['blue', 'green'] }
- String(['blue', 'green']) → 'blue,green'
- Calls hashString('blue,green') → SHA256 hash
- Output: { favorite_colorssha256: '' }
@mattbodle So as of now, both the Core SDK and the Rokt SDK allow hashing of arrays by implicitly converting them to strings. I’ve created this PR in core sdk to skip hashing when the value is an array.
Do we want to officially support hashing arrays, given that it currently just converts the array to a comma-separated string before hashing?



Background
What Has Changed
Screenshots/Video
Checklist
Additional Notes
Reference Issue (For employees only. Ignore if you are an outside contributor)