♻️ Custom sanitizer for Context Manager #3290
Conversation
Datadog ReportBranch report: ❌ 16 Failed (0 Known Flaky), 17071 Passed, 345 Skipped, 1m 38.34s Total Time ❌ Failed Tests (16)
|
nulrich
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3290 +/- ##
==========================================
- Coverage 93.68% 93.67% -0.02%
==========================================
Files 288 288
Lines 7617 7619 +2
Branches 1739 1743 +4
==========================================
+ Hits 7136 7137 +1
- Misses 481 482 +1 ☔ View full report in Codecov by Sentry. |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
| export type PropertiesConfig = { | ||
| [key: string]: { | ||
| required?: boolean | ||
| type?: 'string' | ||
| } | ||
| } | ||
|
|
||
| function enforceTypeProperties(context: Context, propertiesConfig: PropertiesConfig) { | ||
| const newContext = { ...context } | ||
| for (const [key, { type }] of Object.entries(propertiesConfig)) { | ||
| if (type === 'string' && key in newContext) { | ||
| /* eslint-disable @typescript-eslint/no-base-to-string */ | ||
| newContext[key] = String(newContext[key]) | ||
| } | ||
| } | ||
|
|
||
| return newContext | ||
| } | ||
|
|
||
| function checkRequiredProperties(context: Context, propertiesConfig: PropertiesConfig, name: string) { | ||
| for (const [key, { required }] of Object.entries(propertiesConfig)) { | ||
| if (required && !(key in context)) { | ||
| display.warn(`The property ${key} of ${name} context is required; context will not be sent to the intake.`) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 suggestion: I think it could be beneficial to include these as part part of the sanitize module.
sanitize(context, {
ensureProperty: {
id: ' { type: 'string', required: true },
name: { type: 'string' },
email: { type: 'string' },
}
});Also you could merge enforceTypeProperties and checkRequiredProperties into a single function. This would allow iterating over propertiesConfig only once
function ensureProperties(context, propertiesConfig) {
...
}There was a problem hiding this comment.
I merged the functions 👍
Concerning moving the code into sanitize module, I found that the type enforcement and requirement are specific to context management and do not help to sanitize the content.
The type enforcement is more for controlling the content for the intake than preventing illegal data, and property requirement is not really sanitization work.
Also sanitize is checking all properties recursively so we would need to pass the path of properties we want to check and not only supporting the first level.
Motivation
When using Context Manager in our Public API, we manually call the sanitazition functions at multiple places before setting the context.
This could be done automatically by the Context Manager itself and just write
Changes
Now we can pass the sanization configuration to
createContextManager. It will be applied whenever we update the internal context.Also the configuration can also warn the user if a required property is missing:
Testing
I have gone over the contributing documentation.