fix: Optimize cookie value comparison using FNV-1a hash #1568
Conversation
WalkthroughAdds an FNV-1a string hash utility and a private Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Cookie as Cookie.value setter
participant Hash as hashString (FNV-1a)
participant JSON as JSON.stringify
Caller->>Cookie: set value
activate Cookie
alt strict equality
Cookie->>Cookie: short-circuit return (no jar write)
else object value
Cookie->>JSON: JSON.stringify(new) => valueStr
JSON-->>Cookie: valueStr
Cookie->>Hash: hashString(valueStr) => newHash
Hash-->>Cookie: newHash
alt valueHash defined & newHash != valueHash
Cookie->>Cookie: update valueHash\nproceed to set jar
else valueHash defined & newHash == valueHash
Cookie->>JSON: JSON.stringify(current) => currentStr
JSON-->>Cookie: currentStr (or throw)
alt currentStr == valueStr
Cookie->>Cookie: update valueHash (if needed)\nreturn early (no jar write)
else
Cookie->>Cookie: update valueHash\nproceed to set jar
end
else valueHash undefined
Cookie->>Cookie: set valueHash\nproceed to set jar
end
end
deactivate Cookie
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/cookie/unchanged.test.ts (1)
110-131: New tests don’t meaningfully exercise the hash-based optimizationBoth tests assert that repeated assignments of the same cookie (
data) yield exactly oneset-cookieheader. Given thatserializeCookieiterates over a map of cookie names, there will only ever be one header per cookie key per response, regardless of how many times.valueis assigned in a single handler.So these tests will still pass even if the hash-based early-return logic is completely removed or broken, as long as we set
dataat least once.If your goal is to guard the “unchanged object = no Set-Cookie” behavior, consider adding a test that:
- Sends an incoming
datacookie with a JSON object value, and- Reassigns an object with the same serialized shape, and
- Asserts that the response has zero
set-cookieheaders.The existing tests are fine as smoke tests, but they don’t protect the new optimization path.
src/cookies.ts (1)
11-26: Hash-based equality is correct but gives limited benefit and introduces extraJSON.stringifycallsFrom a correctness standpoint the new logic looks sound:
- You only early-return when
JSON.stringify(current) === JSON.stringify(value), so you won’t skip setting a changed cookie.- Hash collisions are handled by doing a stringify comparison when hashes match; in the collision case you’ll still treat the value as changed.
However, there are a couple of non-trivial performance/design issues:
Extra serialization in common object cases
In the object branch you always call:
hashObject(value)→ internally doesJSON.stringify(value), and- In the
elseblock,JSON.stringify(current)andJSON.stringify(value)again.So:
- When
valueHashisundefined(first object comparison) you now serializevaluetwice andcurrentonce (3 stringifications) vs 2 before.- When
valueHashis defined andnewHash === valueHash(unchanged object by hash), you again serializevaluetwice andcurrentonce (3 stringifications), even though this is exactly the “unchanged” path you are trying to optimize.The only case that benefits is when
valueHashis defined andnewHash !== valueHash(changed value): you pay 1 stringify (forvalue) instead of 2. That’s good, but narrower than intended and partially offset by the regressions above.You can keep the collision-safety while avoiding the redundant work with something like:
if ( typeof current === 'object' && current !== null && typeof value === 'object' && value !== null ) { try { const newHash = hashObject(value) // Only fall back to deep compare when the hash matches a previous one if (this.valueHash !== undefined && this.valueHash === newHash) { if (JSON.stringify(current) === JSON.stringify(value)) { // unchanged return } } this.valueHash = newHash } catch {} }This still:
- Uses the hash as a quick “probably changed” filter.
- Only pays the JSON.stringify(current/value) deep-equality cost when hashes match.
- Avoids the redundant
this.valueHashassignments and the unnecessary stringify path whenvalueHashis undefined.Hash cache is per
Cookieinstance, not per cookie keyBecause
createCookieJarreturns a newCookieinstance on each property access,valueHashonly persists for the lifetime of that instance (essentially within a single handler invocation for that destructured cookie). There’s no cross-request caching; each request reconstructs a newCookiewith a freshvalueHash.That means the optimization only helps when you assign to the same cookie’s
.valuemultiple times in the same request. If the intent was to speed up “incoming cookie object is unchanged this request” checks across many requests, you likely need to store the hash alongside the cookie value injar/initialinstead of on the transientCookieinstance.These aren’t correctness bugs, but they do undercut the performance win you’re aiming for. If you’re comfortable with the narrower optimization scope, I’d at least recommend simplifying the branch as above to remove the extra
JSON.stringifycalls.Also applies to: 145-145, 177-209
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/cookies.ts (2)
184-207: Optimize: JSON.stringify called twice on the same value when hashes match.When hashes match (line 195 condition false → line 200),
JSON.stringify(value)is invoked twice:
- Inside
hashObject(value)on line 192- In the comparison on line 200
This redundant serialization reduces the optimization benefit, especially for large objects.
Refactor to cache the stringified value:
- // For objects, use hash-based comparison for performance if ( typeof current === 'object' && current !== null && typeof value === 'object' && value !== null ) { try { - const newHash = hashObject(value) + const valueStr = JSON.stringify(value) + const newHash = hashString(valueStr) // If hash differs from cached hash, value definitely changed if (this.valueHash !== undefined && this.valueHash !== newHash) { this.valueHash = newHash } // First set (valueHash undefined) OR hashes match: do deep comparison else { - if (JSON.stringify(current) === JSON.stringify(value)) { + if (JSON.stringify(current) === valueStr) { this.valueHash = newHash return // Values are identical, skip update } this.valueHash = newHash } } catch {} }Where
hashStringis a refactored helper:const hashString = (str: string): number => { const FNV_OFFSET_BASIS = 2166136261 const FNV_PRIME = 16777619 let hash = FNV_OFFSET_BASIS const len = str.length for (let i = 0; i < len; i++) { hash ^= str.charCodeAt(i) hash = Math.imul(hash, FNV_PRIME) } return hash >>> 0 }
206-206: Consider logging errors in the catch block.The empty
catchblock silently suppresses errors fromhashObject()orJSON.stringify(). While acceptable as a fallback (allowing the setter to proceed with the update), it may hide issues during development.Consider adding minimal logging:
- } catch {} + } catch (err) { + // Fallback to always updating on hashing/comparison errors + // (e.g., circular references, BigInt values) + if (process.env.NODE_ENV === 'development') { + console.warn('Cookie value comparison error:', err) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cookies.ts(3 hunks)test/cookie/unchanged.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cookie/unchanged.test.ts
🔇 Additional comments (1)
src/cookies.ts (1)
145-145: LGTM!The private
valueHashfield appropriately caches the hash for comparison on subsequent sets.
src/cookies.ts
Outdated
| // FNV-1a hash for fast object comparison | ||
| const hashObject = (obj: unknown): number => { | ||
| const FNV_OFFSET_BASIS = 2166136261 | ||
| const FNV_PRIME = 16777619 | ||
|
|
||
| let hash = FNV_OFFSET_BASIS | ||
| const str = JSON.stringify(obj) | ||
| const len = str.length | ||
|
|
||
| for (let i = 0; i < len; i++) { | ||
| hash ^= str.charCodeAt(i) | ||
| hash = Math.imul(hash, FNV_PRIME) | ||
| } | ||
|
|
||
| return hash >>> 0 | ||
| } |
There was a problem hiding this comment.
JSON.stringify key ordering may defeat optimization for semantically equal objects.
The hashObject function uses JSON.stringify to serialize objects before hashing. Since JSON.stringify preserves property insertion order (not sorted), two semantically identical objects with different key orders will produce different hashes. For example:
{a: 1, b: 2}→ hash X{b: 2, a: 1}→ hash Y (different)
This causes unnecessary Set-Cookie headers even when objects are semantically equal, defeating the stated PR objective of reducing redundant headers.
Consider one of these solutions:
Solution 1: Sort keys before stringifying
-const hashObject = (obj: unknown): number => {
+const hashObject = (obj: unknown): number => {
const FNV_OFFSET_BASIS = 2166136261
const FNV_PRIME = 16777619
let hash = FNV_OFFSET_BASIS
- const str = JSON.stringify(obj)
+ const str = JSON.stringify(obj, Object.keys(obj).sort())
const len = str.length
for (let i = 0; i < len; i++) {
hash ^= str.charCodeAt(i)
hash = Math.imul(hash, FNV_PRIME)
}
return hash >>> 0
}Solution 2: Use a stable serialization library
Alternatively, consider using a library like fast-json-stable-stringify for deterministic serialization regardless of key order.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/cookies.ts around lines 11 to 26, the hashObject function uses
JSON.stringify which preserves property insertion order so semantically equal
objects with different key orders produce different hashes; change to a
deterministic serialization before hashing—either (preferable) import a stable
serializer like fast-json-stable-stringify and use it instead of JSON.stringify,
or implement a small utility that recursively sorts object keys (arrays
preserved) and then JSON.stringify the sorted result; ensure the chosen method
handles primitives, arrays, nested objects, and null, and then pass that stable
string into the existing FNV-1a hashing loop.
- Add FNV-1a hash function for fast object comparison - Use hash-based comparison to avoid unnecessary cookie updates - Cache JSON.stringify result to avoid duplicate serialization - Improve code comments explaining hash comparison logic - Add comprehensive tests for repeated identical values and large objects - Rename hashObject to hashString for clarity
YasserGomma
force-pushed
the
fix/cookie-hash-optimization
branch
from
25448c0 to
5eb7a8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cookies.ts (1)
183-185: Key ordering makes semantically equal objects appear different.As noted in previous reviews,
JSON.stringifypreserves property insertion order, so{a: 1, b: 2}and{b: 2, a: 1}produce different hashes and are treated as different values. This causes unnecessarySet-Cookieheaders for semantically equal objects, which contradicts the PR's optimization goal.Consider using a stable serialization approach (sorting keys before stringify or using a library like
fast-json-stable-stringify) as suggested in the previous review.
🧹 Nitpick comments (1)
src/cookies.ts (1)
198-208: Consider restructuring for clarity (optional).The current if-else structure correctly handles hash comparison and collision detection, but the logic flow could be more explicit about the three distinct cases: (1) hash differs → value changed, (2) no cached hash → first set, (3) hashes match → potential collision.
Example restructuring for clarity:
try { // Cache stringified value to avoid duplicate stringify calls const valueStr = JSON.stringify(value) const newHash = hashString(valueStr) - // If hash differs from cached hash, value definitely changed - if (this.valueHash !== undefined && this.valueHash !== newHash) { - this.valueHash = newHash - } - // First set (valueHash undefined) OR hashes match: do deep comparison - else { - if (JSON.stringify(current) === valueStr) { - this.valueHash = newHash - return // Values are identical, skip update - } - this.valueHash = newHash - } + // Fast path: hash differs, value definitely changed + if (this.valueHash !== undefined && this.valueHash !== newHash) { + this.valueHash = newHash + // Fall through to update + } + // Slow path: first set or potential hash collision, do deep comparison + else if (JSON.stringify(current) === valueStr) { + this.valueHash = newHash + return // Values are identical, skip update + } else { + this.valueHash = newHash + // Fall through to update + } } catch {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cookies.ts(3 hunks)test/cookie/unchanged.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cookie/unchanged.test.ts
🔇 Additional comments (1)
src/cookies.ts (1)
11-25: LGTM: FNV-1a implementation is correct.The hash function correctly implements the FNV-1a algorithm with proper constants, uses
Math.imulfor 32-bit multiplication, and converts to unsigned 32-bit integer.
| type Updater<T> = T | ((value: T) => T) | ||
|
|
||
| export class Cookie<T> implements ElysiaCookie { | ||
| private valueHash?: number |
There was a problem hiding this comment.
Stale valueHash when jar is updated via set(), update(), or remove() methods.
The valueHash cache can become stale when the jar is updated directly through the cookie setter (bypassing the value setter). This happens when calling set(), update(), or remove() methods, leading to incorrect change detection.
Example:
cookie.value = {a: 1} // valueHash = hash({a: 1})
cookie.set({value: {b: 2}}) // jar updated, but valueHash still = hash({a: 1})
cookie.value = {b: 2} // Compares against stale hash, treats as changedApply this diff to invalidate the cache when the jar is updated directly:
set cookie(jar: ElysiaCookie) {
if (!(this.name in this.jar)) this.jar[this.name] = this.initial
this.jar[this.name] = jar
+ this.valueHash = undefined // Invalidate cache when jar is updated
}Also applies to: 156-160, 168-170
🤖 Prompt for AI Agents
In src/cookies.ts around lines 144 (and also 156-160, 168-170), the cached
private field valueHash can become stale when the internal jar is modified
directly (via set(), update(), remove(), or when the cookie setter updates the
jar), causing incorrect change detection; fix by invalidating the cache whenever
the jar is mutated — i.e., after any direct jar update inside set(), update(),
remove(), and in the cookie setter that assigns into the jar, assign valueHash =
undefined (or otherwise clear the cache) so subsequent reads compute a fresh
hash.
- Invalidate hash cache when cookie jar is modified directly - Add tests for cache invalidation with update() and set() methods - Ensures hash cache stays consistent when value is changed through alternative methods
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cookies.ts (1)
185-211: Key ordering in JSON.stringify defeats optimization for semantically equal objects.The hash-based comparison uses
JSON.stringifywhich preserves property insertion order. Two semantically identical objects with different key orders will produce different hashes and be treated as different values, causing unnecessarySet-Cookieheaders:{a: 1, b: 2} // hash X {b: 2, a: 1} // hash Y (different)While the comments on lines 186-187 acknowledge this limitation, it contradicts the PR objective of reducing redundant headers.
Consider implementing stable serialization by sorting keys before hashing. For a lightweight solution, recursively sort object keys before calling
JSON.stringify:const stableStringify = (obj: unknown): string => { if (obj === null || typeof obj !== 'object') return JSON.stringify(obj) if (Array.isArray(obj)) return JSON.stringify(obj.map(stableStringify)) const sorted = Object.keys(obj).sort().reduce((acc, key) => { acc[key] = (obj as any)[key] return acc }, {} as any) return JSON.stringify(sorted) }Alternatively, use a library like
fast-json-stable-stringifyfor production-ready stable serialization.
🧹 Nitpick comments (1)
src/cookies.ts (1)
211-211: Add comment explaining empty catch block.The empty catch block silently swallows
JSON.stringifyerrors (e.g., circular references). While the fallback behavior (updating the jar) is safe, a brief comment would clarify the intent.- } catch {} + } catch { + // Stringify failed (e.g., circular ref); fall through to update jar + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cookies.ts(4 hunks)test/cookie/unchanged.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cookie/unchanged.test.ts
🔇 Additional comments (3)
src/cookies.ts (3)
11-25: LGTM - FNV-1a implementation looks correct.The hash function follows the standard FNV-1a algorithm with correct constants and operations. The use of
Math.imulensures proper 32-bit integer multiplication, and the unsigned right shift ensures a positive 32-bit result.
144-144: LGTM - cache field properly declared.The private optional field is appropriate for caching the hash value.
160-161: LGTM - cache invalidation properly implemented.The hash cache is correctly invalidated when the jar is modified directly. Since
set(),update(), andremove()all eventually call this setter (via thesetCookiesetter), the cache is properly invalidated for all mutation paths.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cookies.ts (1)
186-193: Key ordering defeats optimization for semantically equal objects.The comment on lines 186-187 acknowledges that
JSON.stringifypreserves insertion order, so{a: 1, b: 2}and{b: 2, a: 1}are treated as different values even though they're semantically identical. This defeats the stated PR objective of reducing redundant Set-Cookie headers when values are "unchanged."As noted in the previous review, consider either:
- Sorting keys before stringifying, or
- Using a stable serialization library like
fast-json-stable-stringifyHowever, if the current behavior is intentional to keep the implementation simple and performant, the existing comment adequately documents the limitation.
🧹 Nitpick comments (1)
src/cookies.ts (1)
211-211: Consider logging or documenting the empty catch block.The empty catch at line 211 silently ignores
JSON.stringifyerrors (e.g., circular references, BigInt without custom serializer). When serialization fails, the code falls through to update the jar anyway, potentially sending unnecessary Set-Cookie headers for non-serializable objects.Consider adding a comment explaining this behavior, or optionally log the error for debugging:
- } catch {} + } catch (err) { + // JSON.stringify failed (circular ref, BigInt, etc.) + // Fall through to update jar anyway + }Alternatively, if you want to log for debugging:
- } catch {} + } catch { + // Serialization failed; treat as changed value + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cookies.ts(4 hunks)test/cookie/unchanged.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cookie/unchanged.test.ts
🔇 Additional comments (4)
src/cookies.ts (4)
11-25: LGTM! Correct FNV-1a implementation.The hash function correctly implements the FNV-1a algorithm with proper constants and bit operations. The use of
Math.imulensures 32-bit integer multiplication, and the unsigned conversion via>>> 0is appropriate.
144-144: LGTM! Appropriate cache field.The private optional field correctly supports the hash caching strategy, with undefined indicating no cached hash.
156-162: LGTM! Cache invalidation correctly implemented.The cache is properly invalidated when the jar is modified directly. Since
set(),update(), andremove()all delegate through thesetCookiesetter (which calls thiscookiesetter), the invalidation applies consistently across all mutation paths.
178-217: Overall optimization approach is sound.The hash-based comparison correctly implements the fast-path optimization:
- When hashes differ, skips the expensive
JSON.stringify(current)call- When hashes match or this is the first set, performs full comparison to guard against collisions
- Caches the stringified new value (
valueStr) to avoid duplicate serialization- Correctly handles primitives, null, objects, and arrays
- Only updates the jar when values actually differ
The performance benefit for large objects (100+ properties) is legitimate, and the collision handling ensures correctness.
3 participants
Summary
This PR optimizes cookie value comparison by implementing FNV-1a hashing to avoid unnecessary cookie updates when object values haven't changed. This reduces redundant
Set-Cookieheaders and improves performance for applications that frequently set cookie values.Problem
Previously, when setting cookie values (especially objects), the implementation would perform expensive
JSON.stringify()comparisons on every set operation, even when the value hadn't changed. This could lead to:Set-Cookieheaders being sentSolution
Implemented FNV-1a (Fowler–Noll–Vo) hash-based comparison for object values:
JSON.stringify(value)result to avoid duplicate serialization callsChanges
hashString()function implementing FNV-1a algorithmvalueHashprivate property toCookieclass for cachingvaluesetter to use hash-based comparison for objectsBenefits
Testing
Added comprehensive test cases:
should not send set-cookie header when setting same object value as incoming cookie: Verifies that setting the same object value as an incoming cookie doesn't trigger a Set-Cookie headershould not send set-cookie header for large unchanged object values: Tests performance with large objects (100+ items) to ensure hash comparison works correctlyshould optimize multiple assignments of same object in single request: Ensures multiple assignments of the same value in one request only produce one Set-Cookie headerAll existing tests continue to pass, ensuring backward compatibility.
Technical Details
The FNV-1a hash algorithm was chosen for its:
Hash collisions are handled by falling back to full JSON string comparison when hashes match, ensuring correctness while maintaining performance benefits in the common case.
The implementation includes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.