fix(db-ivm): hash Temporal objects by value instead of identity#1368
fix(db-ivm): hash Temporal objects by value instead of identity#1368goatrenterguy wants to merge 1 commit intoTanStack:mainfrom
Conversation
Temporal objects (PlainDate, ZonedDateTime, etc.) have no enumerable own properties, so Object.keys() returns [] and all instances produce identical hashes. This causes the IVM join Index to treat old and new rows as equal, silently swallowing updates when only a Temporal field changed. Hash Temporal objects by their Symbol.toStringTag type and toString() representation to produce correct, value-based hashes. Fixes TanStack#1367 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5dd91ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughFixes hashing of Temporal objects in the IVM hash function. Temporal objects previously produced identical hashes due to lacking enumerable properties, causing live join queries to silently discard updates when only a Temporal field changed. New logic detects and hashes Temporal objects by type tag and string representation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/db/tests/query/join.test.ts (1)
2081-2100: Please verify this regression is not reading through a live row reference.Because the projection returns
taskitself and the assertion readstask.dueDateafter the Line 2093 update, this can still pass if live-query rows retain source references. Projecting a scalar snapshot likedueDateText: task.dueDate.toString()makes the test fail unless the join actually recomputes, and it also lets you get rid of theas anyon Line 2094.Based on learnings, "Add unit tests that reproduce bugs before fixing them to validate fixes and prevent regressions."Proposed tightening for the regression assertion
query: (q) => q .from({ task: taskCollection }) .where(({ task }) => inArray(task.id, [1])) .innerJoin({ project: projectCollection }, ({ task, project }) => eq(task.project_id, project.id), ) .select(({ task, project }) => ({ - task, project, + dueDateText: task.dueDate.toString(), })), }) await liveQuery.preload() expect(liveQuery.toArray).toHaveLength(1) - expect( - (liveQuery.toArray[0]!.task.dueDate as Temporal.PlainDate).toString(), - ).toBe(`2024-01-15`) + expect(liveQuery.toArray[0]!.dueDateText).toBe(`2024-01-15`) @@ - expect( - (liveQuery.toArray[0]!.task.dueDate as Temporal.PlainDate).toString(), - ).toBe(`2024-06-15`) + expect(liveQuery.toArray[0]!.dueDateText).toBe(`2024-06-15`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/tests/query/join.test.ts` around lines 2081 - 2100, The test is vulnerable to reading a live row reference because the projection returns the task object directly; change the projection in the liveQuery.select call to project a scalar snapshot (e.g., dueDateText: task.dueDate.toString() or clone the dueDate value) instead of returning task, update assertions to check that scalar field (dueDateText) after taskCollection.update and flushPromises, and remove the cast `(draft as any)` by assigning a string value in the update; ensure the liveQuery.preload and liveQuery.toArray checks use the new scalar property to verify the join recomputes rather than reading a live reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/db-ivm/src/hashing/hash.ts`:
- Around line 112-132: isTemporal currently only checks Symbol.toStringTag and
hashTemporal casts input to any then calls toString(), which can throw for
null-prototype objects; update both functions to use unknown instead of any,
keep temporalTypes and the tag check, and in hashTemporal (which uses
TEMPORAL_MARKER and MurmurHashStream) add a type guard to verify (input as
unknown) has a callable toString method before calling input.toString(); if
toString is not callable, fallback to a safe string representation (e.g.,
String(input) or include the tag only) to avoid throwing.
In `@packages/db-ivm/tests/utils.test.ts`:
- Around line 7-12: Add an explicit return type to the createTemporalLike helper
(so Object.create(...) doesn’t infer any) and update the PlainDate/PlainDateTime
cross-type test that calls hashTemporal() so both mocks use the identical string
value for toString() (e.g., same date/time string) while only differing in
[Symbol.toStringTag]; this ensures hashTemporal()’s use of both the tag and
string is actually validated. Update createTemporalLike’s signature and the test
that compares hashes for PlainDate vs PlainDateTime (and optionally add a
focused tag-only case) to ensure the test fails if tags are ignored.
---
Nitpick comments:
In `@packages/db/tests/query/join.test.ts`:
- Around line 2081-2100: The test is vulnerable to reading a live row reference
because the projection returns the task object directly; change the projection
in the liveQuery.select call to project a scalar snapshot (e.g., dueDateText:
task.dueDate.toString() or clone the dueDate value) instead of returning task,
update assertions to check that scalar field (dueDateText) after
taskCollection.update and flushPromises, and remove the cast `(draft as any)` by
assigning a string value in the update; ensure the liveQuery.preload and
liveQuery.toArray checks use the new scalar property to verify the join
recomputes rather than reading a live reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e8e5cf9-918a-4ee2-983e-a8e799ce572a
📒 Files selected for processing (4)
.changeset/fix-temporal-hash.mdpackages/db-ivm/src/hashing/hash.tspackages/db-ivm/tests/utils.test.tspackages/db/tests/query/join.test.ts
| const temporalTypes = [ | ||
| `Temporal.PlainDate`, | ||
| `Temporal.PlainTime`, | ||
| `Temporal.PlainDateTime`, | ||
| `Temporal.PlainYearMonth`, | ||
| `Temporal.PlainMonthDay`, | ||
| `Temporal.ZonedDateTime`, | ||
| `Temporal.Instant`, | ||
| `Temporal.Duration`, | ||
| ] | ||
|
|
||
| function isTemporal(input: object): boolean { | ||
| const tag = (input as any)[Symbol.toStringTag] | ||
| return typeof tag === `string` && temporalTypes.includes(tag) | ||
| } | ||
|
|
||
| function hashTemporal(input: object): number { | ||
| const hasher = new MurmurHashStream() | ||
| hasher.update(TEMPORAL_MARKER) | ||
| hasher.update((input as any)[Symbol.toStringTag]) | ||
| hasher.update(input.toString()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "hash.ts" --path "*/db-ivm/*"Repository: TanStack/db
Length of output: 288
🏁 Script executed:
cat -n packages/db-ivm/src/hashing/hash.ts | sed -n '100,150p'Repository: TanStack/db
Length of output: 1804
🏁 Script executed:
cat -n packages/db-ivm/src/hashing/hash.ts | sed -n '1,50p'Repository: TanStack/db
Length of output: 2053
🏁 Script executed:
rg "isTemporal|hashTemporal" packages/db-ivm/src/hashing/hash.ts -A 2 -B 2Repository: TanStack/db
Length of output: 806
🏁 Script executed:
wc -l packages/db-ivm/src/hashing/hash.tsRepository: TanStack/db
Length of output: 94
🏁 Script executed:
cat -n packages/db-ivm/src/hashing/hash.tsRepository: TanStack/db
Length of output: 7580
🏁 Script executed:
node <<'EOF'
// Test if null-prototype objects have toString
const nullProtoObj = Object.create(null);
nullProtoObj[Symbol.toStringTag] = "Temporal.PlainDate";
console.log("Has toString?", "toString" in nullProtoObj);
console.log("Type of toString:", typeof nullProtoObj.toString);
try {
const result = nullProtoObj.toString();
console.log("toString() succeeded:", result);
} catch (e) {
console.log("toString() threw:", e.message);
}
// Test with Object.prototype
const normalObj = {};
normalObj[Symbol.toStringTag] = "Temporal.PlainDate";
console.log("\n=== Normal object ===");
console.log("Has toString?", "toString" in normalObj);
console.log("Type of toString:", typeof normalObj.toString);
console.log("toString() result:", normalObj.toString());
EOFRepository: TanStack/db
Length of output: 276
🏁 Script executed:
rg "function hash|hash\(" packages/db-ivm/src/hashing/hash.ts -B 2 -A 5 | head -80Repository: TanStack/db
Length of output: 1276
🏁 Script executed:
cat -n packages/db-ivm/src/hashing/hash.ts | sed -n '65,112p'Repository: TanStack/db
Length of output: 1640
🏁 Script executed:
cat -n packages/db-ivm/src/hashing/hash.ts | sed -n '133,200p'Repository: TanStack/db
Length of output: 2221
🏁 Script executed:
rg "import.*hash|from.*hash" packages/db-ivm/src --type ts --type tsx | head -20Repository: TanStack/db
Length of output: 82
🏁 Script executed:
rg "\bhash\(" packages/db-ivm/src --type ts --type tsx -B 1 -A 1 | head -40Repository: TanStack/db
Length of output: 82
🏁 Script executed:
cd packages/db-ivm && grep -r "from.*hash" src --include="*.ts" --include="*.tsx"Repository: TanStack/db
Length of output: 428
🏁 Script executed:
cd packages/db-ivm && rg "hash\(" src --type ts -B 2 -A 2 | head -80Repository: TanStack/db
Length of output: 3975
🏁 Script executed:
node <<'EOF'
// Verify the proposed type guard works
type TemporalLike = {
[Symbol.toStringTag]: string
toString(): string
}
function isTemporal(input: object): boolean {
const temporalInput = input as {
[Symbol.toStringTag]?: unknown
toString?: unknown
}
const tag = temporalInput[Symbol.toStringTag]
return (
typeof tag === `string` &&
["Temporal.PlainDate", "Temporal.PlainTime"].includes(tag) &&
typeof temporalInput.toString === `function`
)
}
// Test 1: null-proto with spoofed tag - should fail
const nullProtoFake = Object.create(null);
nullProtoFake[Symbol.toStringTag] = "Temporal.PlainDate";
console.log("nullProtoFake passes isTemporal?", isTemporal(nullProtoFake));
// Test 2: Normal object with Temporal tag - should pass
const normalTemporal = { [Symbol.toStringTag]: "Temporal.PlainDate" };
console.log("normalTemporal passes isTemporal?", isTemporal(normalTemporal));
// Test 3: null-proto with fake toString
const nullProtoWithFunc = Object.create(null);
nullProtoWithFunc[Symbol.toStringTag] = "Temporal.PlainDate";
nullProtoWithFunc.toString = function() { return "test"; };
console.log("nullProtoWithFunc passes isTemporal?", isTemporal(nullProtoWithFunc));
if (isTemporal(nullProtoWithFunc)) {
console.log("nullProtoWithFunc.toString():", nullProtoWithFunc.toString());
}
EOFRepository: TanStack/db
Length of output: 209
🏁 Script executed:
grep -n "as any" packages/db-ivm/src/hashing/hash.tsRepository: TanStack/db
Length of output: 163
🏁 Script executed:
cat -n packages/db-ivm/src/hashing/hash.ts | sed -n '100,140p'Repository: TanStack/db
Length of output: 1514
🏁 Script executed:
rg "function hashTemporal|function hash\(" packages/db-ivm/src/hashing/hash.ts -A 8Repository: TanStack/db
Length of output: 619
Guard the Temporal branch before calling toString().
isTemporal() currently only checks Symbol.toStringTag. A null-prototype object that spoofs Temporal.PlainDate (e.g., via Object.create(null)) will pass the tag check but throw when hashTemporal() calls toString() on line 132, since null-prototype objects do not inherit toString. Replace the any casts with unknown and add a type guard to verify toString is callable before hashing:
Safer type-guarding for the Temporal path
+type TemporalLike = {
+ [Symbol.toStringTag]: string
+ toString(): string
+}
+
function isTemporal(input: object): boolean {
- const tag = (input as any)[Symbol.toStringTag]
- return typeof tag === `string` && temporalTypes.includes(tag)
+ const temporalInput = input as {
+ [Symbol.toStringTag]?: unknown
+ toString?: unknown
+ }
+ const tag = temporalInput[Symbol.toStringTag]
+ return (
+ typeof tag === `string` &&
+ temporalTypes.includes(tag) &&
+ typeof temporalInput.toString === `function`
+ )
}
-function hashTemporal(input: object): number {
+function hashTemporal(input: TemporalLike): number {
const hasher = new MurmurHashStream()
hasher.update(TEMPORAL_MARKER)
- hasher.update((input as any)[Symbol.toStringTag])
+ hasher.update(input[Symbol.toStringTag])
hasher.update(input.toString())
return hasher.digest()
}Per coding guidelines: "Avoid using any types; use unknown instead when type is truly unknown and provide type guards to narrow safely."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const temporalTypes = [ | |
| `Temporal.PlainDate`, | |
| `Temporal.PlainTime`, | |
| `Temporal.PlainDateTime`, | |
| `Temporal.PlainYearMonth`, | |
| `Temporal.PlainMonthDay`, | |
| `Temporal.ZonedDateTime`, | |
| `Temporal.Instant`, | |
| `Temporal.Duration`, | |
| ] | |
| function isTemporal(input: object): boolean { | |
| const tag = (input as any)[Symbol.toStringTag] | |
| return typeof tag === `string` && temporalTypes.includes(tag) | |
| } | |
| function hashTemporal(input: object): number { | |
| const hasher = new MurmurHashStream() | |
| hasher.update(TEMPORAL_MARKER) | |
| hasher.update((input as any)[Symbol.toStringTag]) | |
| hasher.update(input.toString()) | |
| type TemporalLike = { | |
| [Symbol.toStringTag]: string | |
| toString(): string | |
| } | |
| const temporalTypes = [ | |
| `Temporal.PlainDate`, | |
| `Temporal.PlainTime`, | |
| `Temporal.PlainDateTime`, | |
| `Temporal.PlainYearMonth`, | |
| `Temporal.PlainMonthDay`, | |
| `Temporal.ZonedDateTime`, | |
| `Temporal.Instant`, | |
| `Temporal.Duration`, | |
| ] | |
| function isTemporal(input: object): boolean { | |
| const temporalInput = input as { | |
| [Symbol.toStringTag]?: unknown | |
| toString?: unknown | |
| } | |
| const tag = temporalInput[Symbol.toStringTag] | |
| return ( | |
| typeof tag === `string` && | |
| temporalTypes.includes(tag) && | |
| typeof temporalInput.toString === `function` | |
| ) | |
| } | |
| function hashTemporal(input: TemporalLike): number { | |
| const hasher = new MurmurHashStream() | |
| hasher.update(TEMPORAL_MARKER) | |
| hasher.update(input[Symbol.toStringTag]) | |
| hasher.update(input.toString()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db-ivm/src/hashing/hash.ts` around lines 112 - 132, isTemporal
currently only checks Symbol.toStringTag and hashTemporal casts input to any
then calls toString(), which can throw for null-prototype objects; update both
functions to use unknown instead of any, keep temporalTypes and the tag check,
and in hashTemporal (which uses TEMPORAL_MARKER and MurmurHashStream) add a type
guard to verify (input as unknown) has a callable toString method before calling
input.toString(); if toString is not callable, fallback to a safe string
representation (e.g., String(input) or include the tag only) to avoid throwing.
| function createTemporalLike(tag: string, value: string) { | ||
| return Object.create(null, { | ||
| [Symbol.toStringTag]: { value: tag }, | ||
| toString: { value: () => value }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "utils.test.ts" | grep -E "db-ivm|packages"Repository: TanStack/db
Length of output: 126
🏁 Script executed:
cd packages/db-ivm && git ls-files | grep -E "test|utils"Repository: TanStack/db
Length of output: 1090
🏁 Script executed:
# Get the file and check its existence and size
wc -l packages/db-ivm/tests/utils.test.ts 2>/dev/null || echo "File not found, trying alternative paths"
find . -type f -name "utils.test.ts" -path "*db-ivm*"Repository: TanStack/db
Length of output: 132
🏁 Script executed:
find . -type f -name "*.test.ts" 2>/dev/null | head -20Repository: TanStack/db
Length of output: 1072
🏁 Script executed:
# Look for db-ivm directory structure
fd -t d db-ivm 2>/dev/nullRepository: TanStack/db
Length of output: 71
🏁 Script executed:
# List top-level packages
ls -la packages/ 2>/dev/null || echo "packages dir not found"Repository: TanStack/db
Length of output: 1120
🏁 Script executed:
head -30 packages/db-ivm/tests/utils.test.tsRepository: TanStack/db
Length of output: 975
🏁 Script executed:
sed -n '1,20p' packages/db-ivm/tests/utils.test.tsRepository: TanStack/db
Length of output: 729
🏁 Script executed:
sed -n '190,210p' packages/db-ivm/tests/utils.test.tsRepository: TanStack/db
Length of output: 984
🏁 Script executed:
# Verify what hashTemporal actually does - search for the hash function implementation
rg -A 10 "hashTemporal|Symbol.toStringTag" packages/db-ivm/src --type tsRepository: TanStack/db
Length of output: 1887
🏁 Script executed:
# Check if there are any other uses of createTemporalLike to understand the broader context
rg "createTemporalLike" packages/db-ivm/tests/utils.test.ts -B 2 -A 2Repository: TanStack/db
Length of output: 1619
Add explicit return type to helper and tighten cross-type test.
The createTemporalLike() helper should have an explicit return type since Object.create() returns any by default. Additionally, the PlainDate/PlainDateTime cross-type test uses different string values (2024-01-15 vs 2024-01-15T00:00:00), so it would pass even if hashTemporal() only hashes the string and ignores Symbol.toStringTag. Since both the tag and string are hashed in hashTemporal(), use the same string value for both mocks to ensure the test specifically validates that different tags produce different hashes.
Typed helper and isolated tag-checking test
+type TemporalLike = {
+ readonly [Symbol.toStringTag]: string
+ toString(): string
+}
+
-function createTemporalLike(tag: string, value: string) {
+function createTemporalLike(tag: string, value: string): TemporalLike {
return Object.create(null, {
[Symbol.toStringTag]: { value: tag },
toString: { value: () => value },
- })
+ }) as TemporalLike
}
@@
- const plainDate = createTemporalLike(`Temporal.PlainDate`, `2024-01-15`)
+ const sharedValue = `same-value`
+ const plainDate = createTemporalLike(`Temporal.PlainDate`, sharedValue)
const plainDateTime = createTemporalLike(
`Temporal.PlainDateTime`,
- `2024-01-15T00:00:00`,
+ sharedValue,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db-ivm/tests/utils.test.ts` around lines 7 - 12, Add an explicit
return type to the createTemporalLike helper (so Object.create(...) doesn’t
infer any) and update the PlainDate/PlainDateTime cross-type test that calls
hashTemporal() so both mocks use the identical string value for toString()
(e.g., same date/time string) while only differing in [Symbol.toStringTag]; this
ensures hashTemporal()’s use of both the tag and string is actually validated.
Update createTemporalLike’s signature and the test that compares hashes for
PlainDate vs PlainDateTime (and optionally add a focused tag-only case) to
ensure the test fails if tags are ignored.
Incorporates Ben's Temporal hash fix with tests: - Unit tests for Temporal hash correctness (mock-based) - Regression test for join live query with Temporal field updates - Removed duplicate changeset (ours covers both packages) - Resolved hash.ts conflict: kept Set-based temporalTypes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Heh we both wrote PRs 😆 I just pulled yours into mine at #1370 so I'll close this one. |
2 participants
Summary
Fixes #1367
Temporal objects (
PlainDate,ZonedDateTime,Instant, etc.) have no enumerable own properties, soObject.keys()returns[]. This caused the IVMhash()function to produce identical hashes for all Temporal instances, which meant the joinIndextreated old and new rows as equal and silently swallowed updates when only a Temporal field changed.This adds a Temporal-specific branch in
hashObject()that detects Temporal objects viaSymbol.toStringTag(the same detection approach already used inpackages/db/src/utils.ts) and hashes them by their type tag andtoString()representation.Changes
packages/db-ivm/src/hashing/hash.ts- AddedisTemporal()detection andhashTemporal()hashing before the plain object fallbackpackages/db-ivm/tests/utils.test.ts- Unit tests for Temporal hash correctness (same value, different value, cross-type distinction)packages/db/tests/query/join.test.ts- Regression test verifying Temporal field updates propagate through joined live queriesSummary by CodeRabbit