Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/shiny-donkeys-repeat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@tanstack/db': patch
---

Fix: `materialize(q…findOne())` resolved to `null` instead of `undefined` when the correlation key itself was null. A null correlation key never reached the includes materialization step, so the compiler's `null` select placeholder leaked into the result, violating the typed `T | undefined` contract. Null correlation keys now resolve to the empty materialized value (`undefined` for `findOne()`, `[]` for arrays, `` for `concat`).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use a valid-looking query expression.

materialize(q…findOne()) is unclear and reads like invalid API syntax. Use materialize(q.from(...).findOne()) or prose instead.

Proposed fix
-Fix: `materialize(q…findOne())` resolved to `null` instead of `undefined`
+Fix: `materialize(q.from(...).findOne())` resolved to `null` instead of `undefined`
📝 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.

Suggested change
Fix: `materialize(qfindOne())` resolved to `null` instead of `undefined` when the correlation key itself was null. A null correlation key never reached the includes materialization step, so the compiler's `null` select placeholder leaked into the result, violating the typed `T | undefined` contract. Null correlation keys now resolve to the empty materialized value (`undefined` for `findOne()`, `[]` for arrays, `` for `concat`).
Fix: `materialize(q.from(...).findOne())` resolved to `null` instead of `undefined` when the correlation key itself was null. A null correlation key never reached the includes materialization step, so the compiler's `null` select placeholder leaked into the result, violating the typed `T | undefined` contract. Null correlation keys now resolve to the empty materialized value (`undefined` for `findOne()`, `[]` for arrays, `` for `concat`).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.changeset/shiny-donkeys-repeat.md at line 5, Update the changeset
description to replace the invalid-looking `materialize(q…findOne())` expression
with valid API syntax such as `materialize(q.from(...).findOne())`, or describe
the behavior in prose while preserving the existing null-correlation-key
details.

34 changes: 29 additions & 5 deletions packages/db/src/query/compiler/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,12 @@ export function compileQuery(
getRouting: (nsRow: any) => {
correlationKey: unknown
parentContext: Record<string, any> | null
/**
* Set when this row does not participate in the include at all
* (conditional-select guards failed or no routing info was found),
* as opposed to participating with a null correlation key.
*/
skipped?: boolean
}
}> = []
for (const { sourceAlias, include } of sourceIncludes) {
Expand Down Expand Up @@ -388,12 +394,13 @@ export function compileQuery(
fieldName,
getRouting: (nsRow: any) => {
if (!matchesConditionalSelectGuards(compiledGuards, nsRow)) {
return { correlationKey: null, parentContext: null }
return { correlationKey: null, parentContext: null, skipped: true }
}
return (
nsRow[sourceAlias]?.[INCLUDES_ROUTING]?.[include.fieldName] ?? {
correlationKey: null,
parentContext: null,
skipped: true,
}
)
},
Expand Down Expand Up @@ -427,12 +434,17 @@ export function compileQuery(
fieldName,
getRouting: (nsRow: any) => {
if (!matchesConditionalSelectGuards(compiledGuards, nsRow)) {
return { correlationKey: null, parentContext: null }
return {
correlationKey: null,
parentContext: null,
skipped: true,
}
}
return (
nsRow[INCLUDES_ROUTING]?.[include.fieldName] ?? {
correlationKey: null,
parentContext: null,
skipped: true,
}
)
},
Expand Down Expand Up @@ -641,7 +653,11 @@ export function compileQuery(
fieldName,
getRouting: (nsRow: any) => {
if (!matchesConditionalSelectGuards(compiledRoutingGuards, nsRow)) {
return { correlationKey: null, parentContext: null }
return {
correlationKey: null,
parentContext: null,
skipped: true,
}
}
const parentContext: Record<string, Record<string, any>> = {}
for (const proj of compiledProjs) {
Expand All @@ -667,7 +683,11 @@ export function compileQuery(
fieldName,
getRouting: (nsRow: any) => {
if (!matchesConditionalSelectGuards(compiledRoutingGuards, nsRow)) {
return { correlationKey: null, parentContext: null }
return {
correlationKey: null,
parentContext: null,
skipped: true,
}
}
return {
correlationKey: compiledCorrelation(nsRow),
Expand Down Expand Up @@ -760,7 +780,11 @@ export function compileQuery(
map(([key, namespacedRow]: any) => {
const routing: Record<
string,
{ correlationKey: unknown; parentContext: Record<string, any> | null }
{
correlationKey: unknown
parentContext: Record<string, any> | null
skipped?: boolean
}
> = {}
for (const { fieldName, getRouting } of includesRoutingFns) {
routing[fieldName] = getRouting(namespacedRow)
Expand Down
14 changes: 14 additions & 0 deletions packages/db/src/query/live/collection-config-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1912,6 +1912,20 @@ function flushIncludesState(

// Parent rows may already be materialized in the live collection by the
// time includes state is flushed, so update the stored row as well.
const storedParent = parentCollection.get(parentKey as any)
if (storedParent && storedParent !== parentResult) {
setIncludedValue(storedParent, state.resultPath, childValue)
}
} else if (routing && !routing.skipped && materializesInline(state)) {
// The correlation key itself is null/undefined, so no child row
// can ever match and no child pipeline output will arrive for
// this parent. Without this branch the compiler's `null` select
// placeholder leaks into the result (#1706). Resolve the field
// to its empty materialization instead: `undefined` for
// findOne(), `[]` for arrays, `` for concat.
const childValue = materializeIncludedValue(state, undefined)
setIncludedValue(parentResult, state.resultPath, childValue)

const storedParent = parentCollection.get(parentKey as any)
if (storedParent && storedParent !== parentResult) {
setIncludedValue(storedParent, state.resultPath, childValue)
Expand Down
110 changes: 110 additions & 0 deletions packages/db/tests/query/includes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6573,6 +6573,116 @@ describe(`includes subqueries`, () => {
expect(orphan.project).toBeUndefined()
})

// Regression tests for #1706: a null correlation key must resolve to the
// empty materialized value (`undefined` for findOne(), `[]` for arrays),
// not leak the compiler's `null` select placeholder into the result.
describe(`null correlation key (#1706)`, () => {
type Author = { id: number; name: string }
type Post = { id: number; title: string; authorId: number | null }

function createAuthorAndPostCollections(posts: Array<Post>) {
const authorCollection = createCollection(
localOnlyCollectionOptions<Author>({
getKey: (author) => author.id,
initialData: [{ id: 1, name: `Ada` }],
}),
)
const postCollection = createCollection(
localOnlyCollectionOptions<Post>({
getKey: (post) => post.id,
initialData: posts,
}),
)
return { authorCollection, postCollection }
}

it(`findOne() with a null correlation key yields undefined`, async () => {
const { authorCollection, postCollection } =
createAuthorAndPostCollections([
{ id: 1, title: `fk matches an author`, authorId: 1 },
{ id: 2, title: `fk set, but no author matches`, authorId: 999 },
{ id: 3, title: `fk is null`, authorId: null },
])

const postsWithAuthor = createLiveQueryCollection((q) =>
q.from({ post: postCollection }).select(({ post }) => ({
...post,
author: materialize(
q
.from({ author: authorCollection })
.where(({ author }) => eq(author.id, post.authorId))
.findOne(),
),
})),
)
await postsWithAuthor.preload()

expect(
stripVirtualPropsDeep((postsWithAuthor.get(1) as any).author),
).toEqual({ id: 1, name: `Ada` })
expect((postsWithAuthor.get(2) as any).author).toBeUndefined()
expect((postsWithAuthor.get(3) as any).author).toBeUndefined()
expect((postsWithAuthor.get(3) as any).author).not.toBeNull()
})

it(`array materialize with a null correlation key yields []`, async () => {
const { authorCollection, postCollection } =
createAuthorAndPostCollections([
{ id: 3, title: `fk is null`, authorId: null },
])

const postsWithAuthors = createLiveQueryCollection((q) =>
q.from({ post: postCollection }).select(({ post }) => ({
id: post.id,
authors: materialize(
q
.from({ author: authorCollection })
.where(({ author }) => eq(author.id, post.authorId))
.select(({ author }) => ({
id: author.id,
name: author.name,
})),
),
})),
)
await postsWithAuthors.preload()

expect((postsWithAuthors.get(3) as any).authors).toEqual([])
})

it(`updating the correlation key to null re-emits undefined`, async () => {
const { authorCollection, postCollection } =
createAuthorAndPostCollections([
{ id: 1, title: `starts with a matching fk`, authorId: 1 },
])

const postsWithAuthor = createLiveQueryCollection((q) =>
q.from({ post: postCollection }).select(({ post }) => ({
...post,
author: materialize(
q
.from({ author: authorCollection })
.where(({ author }) => eq(author.id, post.authorId))
.findOne(),
),
})),
)
await postsWithAuthor.preload()

expect(
stripVirtualPropsDeep((postsWithAuthor.get(1) as any).author),
).toEqual({ id: 1, name: `Ada` })

postCollection.update(1, (draft) => {
draft.authorId = null
})
await flushPromises()

expect((postsWithAuthor.get(1) as any).author).toBeUndefined()
expect((postsWithAuthor.get(1) as any).author).not.toBeNull()
})
})

it(`inserting the matching child re-emits parent with populated singleton`, async () => {
// Start with an issue whose project doesn't exist yet
issues.utils.begin()
Expand Down