Skip to content

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.

Sign up for GitHub

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

Closed
jaissica12 wants to merge 4 commits into from
Closed

fix: add check for arrays in hashAttributes #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62948d4
fix: add check for arrays in hashAttributes
jaissica12 Jan 21, 2026
689fc9f
add comment in hashAttributes
jaissica12 Jan 21, 2026
68f6d88
Update src/roktManager.ts
jaissica12 Jan 21, 2026
dc65b12
Update src/roktManager.ts
jaissica12 Jan 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/roktManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,12 @@ export default class RoktManager {
return {};
}

// Hash all attributes in parallel
// Hash all attributes in parallel except arrays
const hashPromises = keys.map(async (key) => {
const attributeValue = attributes[key] as RoktAttributeValueType;
const attributeValue = attributes[key];
if (Array.isArray(attributeValue)) {
return { key, attributeValue, hashedValue: null };
}
Comment on lines +280 to +283
Copy link
Copy Markdown
Member

@rmi22186 rmi22186 Jan 21, 2026

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

Copy link
Copy Markdown
Contributor Author

@jaissica12 jaissica12 Jan 21, 2026

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?

Copy link
Copy Markdown
Member

@rmi22186 rmi22186 Jan 22, 2026

Choose a reason for hiding this comment

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

@jaissica12 IMO, while I see why you are doing this, the more important thing isn't having an array that ends up being hashed - the end result is that the final hashed string is consistent. whether it's hashing ['a', 'b', 'c'], or 'a,b,c' doesn't matter in the use case.

If we change this now, it would change behavior of the wSDK's customers to have a different final hashed string fro before and after the change, so I'd personally reject this PR.

Copy link
Copy Markdown
Contributor Author

@jaissica12 jaissica12 Jan 22, 2026

Choose a reason for hiding this comment

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

I understand what you’re saying. My thinking was around the value of hashing an array, but I agree that the priority here is keeping the final hashed output consistent for existing wSDK customers. Since changing this would alter behavior, I’m aligned with rejecting the PR and will go ahead and close it.

const hashedValue = await this.hashSha256(attributeValue);
Comment thread
jaissica12 marked this conversation as resolved.
return { key, attributeValue, hashedValue };
});
Expand Down
19 changes: 19 additions & 0 deletions test/jest/roktManager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,25 @@ describe('RoktManager', () => {
expect.stringContaining('Failed to hashAttributes, returning an empty object: Hashing failed')
);
});

it('should skip hashing for array values', async () => {
const attributes = {
email: 'test@example.com',
tags: ['vip', 'premium'],
emptyList: [],
};

const result = await roktManager.hashAttributes(attributes);

expect(result).toEqual({
email: 'test@example.com',
emailsha256: nodeCrypto.createHash('sha256').update('test@example.com').digest('hex'),
tags: ['vip', 'premium'],
emptyList: [],
});
expect(result['tagssha256']).toBeUndefined();
expect(result['emptyListsha256']).toBeUndefined();
});
});

describe('#hashSha256', () => {
Expand Down
Loading