-
Notifications
You must be signed in to change notification settings - Fork 43
feat(cache): add backward compatibility for pako 1.x to 2.x migration #1383
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: master
Are you sure you want to change the base?
feat(cache): add backward compatibility for pako 1.x to 2.x migration #1383
Conversation
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
Signed-off-by: ElaineDeMattosSilvaB <elaine.de-mattos-silva-bezerra@deutschebahn.com>
| service: config.get('CACHING_REDIS_SERVICE'), | ||
| apiKey: config.get('CACHING_REDIS_API_KEY'), | ||
| port: Number(config.get('CACHING_REDIS_PORT')) || 6380 | ||
| port: Number(config.get('CACHING_REDIS_PORT')) |
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.
Was the removal of || 6380 here intentional? redis.js has it in the function signature default, and config.js has added the fallback.
| "pako": "^2.1.0", | ||
| "pako-1": "npm:pako@^1.0.8" |
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.
nit: sort alphabetically.
qtomlinson
left a comment
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 adding the migration and the unit tests!
| "testcontainers": "^11.2.1", | ||
| "typescript": "5.8.3" | ||
| "typescript": "5.8.3", | ||
| "pako": "^2.1.0", |
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.
duplicate? pako is already listed in dependencies.
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.
It looks like it's pako v1 and v2 for backwards compat.
|
|
||
| // Detect format: base64 (new) vs binary string (old) | ||
| // Base64 only contains A-Z, a-z, 0-9, +, /, and optional = padding | ||
| const isBase64 = /^[A-Za-z0-9+/]+=*$/.test(dataString) |
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.
Most of our cached values are objects and this works well for those cases. When a number is cached (e.g. statusService.requestCount), isBase64 becomes false positive. The number of such cases is small.
Hi,
as part of the Node.js and package updates effort (#1340), here is the follow-up PR that adds backward compatibility to handle Redis cache entries compressed with both pako 1.x (binary string) and pako 2.x (base64) formats, preventing cache invalidation during the upgrade.
Summary
Adds backward compatibility to handle Redis cache entries compressed with both pako 1.x (binary string) and pako 2.x (base64) formats, preventing cache invalidation during the upgrade.
Changes
redis.js: Automatically detects whether cached data is in old binary string format (pako 1.x) or new base64 format (pako 2.x)def_*(definitions) andhrv_*(harvest) key patternsTesting
pako@1.0.8andpako@2.1.0to prove cross-version compatibilityBenefits