-
Notifications
You must be signed in to change notification settings - Fork 56
feat: Add hashSha256 method for SHA-256 hashing #1116
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?
feat: Add hashSha256 method for SHA-256 hashing #1116
Conversation
src/roktManager.ts
Outdated
| */ | ||
| public async hashSha256(attribute: string | number | boolean | undefined | null): Promise<string> { | ||
| if (attribute === undefined || attribute === null) { | ||
| throw new Error('Value cannot be null or undefined'); |
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.
- i worry this will cause more harm than good in crashing the current loop/thread. our typical pattern would be to log an error or warning.
- I would also make the argument that there's nothing wrong with passing a null or undefined into this particular method. The method is able to act on the value passed in - it should just respond with the attribute (which is either null or undefined).
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.
Thanks for the feedback about logging pattern. Updated the code:
- For
null/undefinedinputs: Returns the value as-is with a logger.warning - For hash failures: Returns undefined with a logger.error
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.
I know this was updated already, but as an fyi - returning the null or undefined is fine because the wSDK will automatically sanitize these out when you pass a false-y value to it. I just tested
src/roktManager.ts
Outdated
| const hashedAttributes: IRoktPartnerAttributes = {}; | ||
|
|
||
| for (const key in attributes) { | ||
| const attributeValue = attributes[key]; | ||
|
|
||
| hashedAttributes[key] = attributeValue; | ||
|
|
||
| const hashedValue = await this.hashSha256(attributeValue); | ||
|
|
||
| if (hashedValue) { | ||
| hashedAttributes[`${key}sha256`] = hashedValue; | ||
| } | ||
| } |
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.
This currently hashes the attributes sequentially, but the Rokt wSDK hashes in parallel, which is faster.
| const hashedAttributes: IRoktPartnerAttributes = {}; | |
| for (const key in attributes) { | |
| const attributeValue = attributes[key]; | |
| hashedAttributes[key] = attributeValue; | |
| const hashedValue = await this.hashSha256(attributeValue); | |
| if (hashedValue) { | |
| hashedAttributes[`${key}sha256`] = hashedValue; | |
| } | |
| } | |
| // Get own property keys only | |
| const keys = Object.keys(attributes); | |
| if (keys.length === 0) { | |
| return {}; | |
| } | |
| // Hash all attributes in parallel | |
| const hashPromises = keys.map(async (key) => { | |
| const attributeValue = attributes[key]; | |
| const hashedValue = await this.hashSha256(attributeValue); | |
| return { key, attributeValue, hashedValue }; | |
| }); | |
| const results = await Promise.all(hashPromises); | |
| // Build the result object | |
| const hashedAttributes: IRoktPartnerAttributes = {}; | |
| for (const { key, attributeValue, hashedValue } of results) { | |
| hashedAttributes[key] = attributeValue; | |
| if (hashedValue) { | |
| hashedAttributes[`${key}sha256`] = hashedValue; | |
| } | |
| } |
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.
Thanks for the feedback, updated!
src/roktManager.ts
Outdated
|
|
||
| } catch (error) { | ||
| return Promise.reject(error instanceof Error ? error : new Error('Unknown error occurred')); | ||
| return Promise.reject(error instanceof Error ? error : new Error('Failed to hash attributes')); |
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.
This will throw an error, similar to what you did with hashSha256 before. Can you instead log the error here?
src/roktManager.ts
Outdated
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| this.logger.error(`Failed to hash "${attribute}" and returning undefined, selectPlacements will continue: ${errorMessage}`); | ||
| this.logger.error(`Failed to hash "${attribute}" and returning undefined: ${errorMessage}`); |
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.
@mattbodle - What are your thoughts on removing ${attribute} here? One positive is that it would reduce the total distinct logs, but we wouldn't be able to target specific attributes that the partner is using which would help us pinpoint the exact problem. What do you think?
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.
@gtamanaha also^^
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.
actually since this would likely be email based on the test below, I say we remove it since it'd be pii
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.
Should just be Failed to hashSha256, returning undefined: ${errorMessage}
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.
Updated error and warning message to exclude pii
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.
I think having the attribute will be helpful and you can use wildcard to filter DD queries. Example
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.
@gtamanaha - The issue here is that there is potential for this value being email (PII).
src/roktManager.ts
Outdated
| } catch (error) { | ||
| return Promise.reject(error instanceof Error ? error : new Error('Unknown error occurred')); | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| this.logger.error(`Failed to hash "${attributes}", selectPlacements will continue: ${errorMessage}`); |
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.
Should just be Failed to hashAttributes, returning undefined: ${errorMessage}
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.
Updated!
|



Background
hashSha256method and related helpers to provide SHA-256 hashing functionality for advertisers using the mParticle Web SDK without requiring the Rokt Kit dependency.What Has Changed
hashSha256methodScreenshots/Video
Checklist
Additional Notes
Reference Issue (For employees only. Ignore if you are an outside contributor)