fix(nuxt): fallback on failed key decoding in cache driver #33505
Conversation
|
|
WalkthroughIn packages/nitro-server/src/runtime/utils/cache-driver.js the key-normalisation implementation was replaced: an explicit Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nitro-server/src/runtime/utils/cache-driver.js (1)
10-18: Consider catching specificallyURIErrorand including error details in the warning.The implementation correctly handles the decoding failure scenario. However, the catch block is generic and the warning doesn't include error details, which could aid debugging.
Apply this diff to improve error handling:
function normalizeFsKey(item) { const normalized = item.replaceAll(':', '_') try { return decodeURIComponent(normalized) - } catch (error) { - console.warn(`Failed to decode key "${item}", using "${normalized}" instead.`) + } catch (error) { + if (error instanceof URIError) { + console.warn(`Failed to decode key "${item}" (${error.message}), using "${normalized}" instead.`) + } else { + throw error + } return normalized } }This ensures only expected URI decoding errors are caught whilst re-throwing unexpected errors, and provides the error message for better diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nitro-server/src/runtime/utils/cache-driver.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #33505 will not alter performanceComparing Summary
|
| */ | ||
| const normalizeFsKey = item => decodeURIComponent(item.replaceAll(':', '_')) | ||
| function normalizeFsKey(item) { | ||
| const normalized = item.replaceAll(':', '_') |
There was a problem hiding this comment.
can we choose a better normalisation method instead? maybe just use something like base64 encoding?
| * @param {string} item | ||
| */ | ||
| const normalizeFsKey = item => decodeURIComponent(item.replaceAll(':', '_')) | ||
| function normalizeFsKey(item) { |
There was a problem hiding this comment.
maybe just:
import { createHash } from 'node:crypto'
const normalizeFsKey = item => createHash('sha256').update(item).digest('hex')There was a problem hiding this comment.
Now use hashes, limit length, and preserve readability. defineURIComponent is not used because of the need for additional error handling and concerns about file system support for special characters.
There was a problem hiding this comment.
is there a reason you're using sha1 rather than sha256?
and one more question: a pure hash would be 64 chars, and this is 41 chars, which isn't that much shorter but increases the risk of collisions
There was a problem hiding this comment.
You're right. I initially thought SHA-1 would be faster, but after running some benchmarks, SHA-256 actually turned out to be faster on my machine. I've switched to using the full hash now.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nitro-server/src/runtime/utils/cache-driver.js (1)
11-16: Excellent solution – cleaner than the try/catch approach.The hybrid prefix-hash format elegantly solves the original issue with literal
%characters whilst maintaining filesystem safety and readability. The regex correctly sanitises problematic characters, and the full SHA-256 hash (64 chars) provides negligible collision risk, addressing concerns raised in previous reviews.Optionally, consider adding JSDoc to document the normalisation format for future maintainers:
/** * @param {string} item + * Normalizes cache keys for filesystem storage by combining a sanitized + * 20-character prefix with a SHA-256 hash for uniqueness. + * Format: "prefix-hash" (e.g., "posts_-abc123...") + * @returns {string} Normalized key safe for filesystem use */ function normalizeFsKey (item) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nitro-server/src/runtime/utils/cache-driver.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (2)
packages/nitro-server/src/runtime/utils/cache-driver.js (2)
3-3: LGTM: Crypto import is correct.The import is necessary for SHA-256 hashing in the normalisation function.
29-29: Correct usage of normalisation for filesystem operations—no issues found.Code inspection confirms the implementation correctly applies
normalizeFsKeyonly to filesystem operations whilst preserving the original key for the in-memory LRU cache. ThenormalizeFsKeyfunction replaces non-alphanumeric characters (including%) with underscores and appends a SHA256 hash of the original key, ensuring filesystem compatibility without losing information. This approach resolves issues with keys containing special characters like%.The usage pattern across setItem, hasItem, and getItem methods is consistent: LRU operations use the unmodified key, whilst filesystem operations use the normalised key.
🔗 Linked issue
introduced by #30973
📚 Description
When using
useAsyncData('posts/%', ...)in multiple pages, some pages occasionally receive an empty payload.After investigation, the issue was traced to
normalizeFsKey, wheredecodeURIComponentthrows a "URI malformed" error if the key contains a literal%(not part of valid URI encoding). This breaks cache key normalization and causes cache misses.🧪 Reproduction
Use the same key on multiple pages:
Pre-render or visit both pages
One of them may get an empty payload
🛠 Fix
Add a
try/catchfallback arounddecodeURIComponentinsidenormalizeFsKey. If decoding fails, fall back to the normalized key. This ensures keys likeposts/%work correctly without breaking cache consistency.