fix: stop keyset paging from dropping NULL sort rows

[grvijayan]

Fixes #766.

Paging a list ordered by a nullable column silently lost every row whose sort value was NULL, on all three dialects. Two defects combined: nullable bindings flattened nil to a zero value so the cursor never carried a NULL, and no dialect stated NULL ordering in ORDER BY, so the null-aware compare helper's assumption (ASC NULLS FIRST / DESC NULLS LAST) held only by accident on SQLite and Spanner and was contradicted by Postgres.

Changes:

• Session user_id and token_id bindings now bind SQL NULL instead of "", via a new database.NullableValue helper. token_id is NULL until the session token is created; "" is never stored.
• CoerceString gained a nil branch so a NULL cursor value survives the token round-trip, as timestamps already did.
• Postgres and SQLite state NULLS FIRST (ASC) / NULLS LAST (DESC) in ORDER BY for columns marked Nullable in the schema. Spanner rejects the clause but guarantees that ordering by default.
• A new Keyset flag on CompareGreater/CompareLess plus the Nullable schema flag route keyset compares through null-aware SQL even when the cursor value is non-NULL, so DESC paging keeps admitting the NULL block beyond the cursor.
• New stmttest pagination tests page sessions by (user_id, id) in both directions, including a NULL block spanning a page boundary. Compiler unit tests pin the emitted SQL per dialect.

The second commit marks every remaining nullable binding, so the fix holds for whatever becomes a sort column next. Verified against the DDL, that covers the issue's list (user.lifecycle_owner_team_id, the credential schemas' verification and check timestamps, json_schema.object_type, token.expires_at) plus bindings the sweep found beyond it: crypto keys activated_at/retired_at, tokens.user_id, the three token session id columns, and usertotp.verified_at. Two accessors (token.expires_at, crypto keys) returned typed nil pointers, which is worse than zero-flattening: a typed nil slips past the compiler's nil checks and binds NULL into an ordered compare. Sentinel-typed fields (token.user_id, usertotp.verified_at) map their never-stored sentinel to NULL, with the convention documented at the binding.

Every nullable binding now has a unit test asserting the Nullable flag is set, the absent state binds untyped nil (asserted with == nil, since assert.Nil passes for typed nils), and the present state binds the dereferenced value. Still open: nothing cross-checks binding flags against the live DDL, so a future nullable column added without the flag goes unnoticed until it becomes a sort column.

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
nextgen	Ready	Preview	Aug 7, 2026 3:46pm
nextgen-docs	Ready	Preview	Aug 7, 2026 3:46pm
nextgen-mock-zitadel	Ready	Preview	Aug 7, 2026 3:46pm

[github-actions]

⚠️ No Changeset found

Latest commit: c2c19fe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Copilot]

Pull request overview

This PR fixes a keyset-pagination correctness bug when sorting sessions by nullable columns (notably user_id and token_id), where rows with NULL sort values could be silently skipped depending on dialect and sort direction. It does so by preserving NULL through cursor encoding/decoding, explicitly stating NULLS FIRST/LAST where supported, and routing keyset predicates on nullable columns through null-aware SQL generation.

Changes:

• Introduces schema-level Nullable metadata (plus NullableValue) so bindings and compilers can reliably treat nullable columns as NULL instead of zero values.
• Updates dialect ORDER BY compilation (Postgres/SQLite) to explicitly emit NULLS FIRST / NULLS LAST for nullable columns; Spanner documents its fixed behavior.
• Adds integration pagination tests for sessions to ensure NULL blocks are paged correctly in both ASC and DESC directions, plus compiler unit tests pinning emitted SQL.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/storage/v2/stmttest/session_pagination_test.go	New cross-dialect integration tests to ensure paging over (user_id, id) doesn’t drop NULL user_id sessions.
internal/storage/v2/dialect/sqlite/session.go	Marks token_id and user_id as nullable and binds SQL NULL instead of "" for absent values.
internal/storage/v2/dialect/sqlite/compiler.go	Uses null-aware compare expansion when required; emits NULLS FIRST/LAST in ORDER BY for nullable columns.
internal/storage/v2/dialect/sqlite/compiler_test.go	Adds unit tests pinning ORDER BY NULLS behavior and nullable-keyset compare SQL.
internal/storage/v2/dialect/spanner/session.go	Marks token_id and user_id as nullable and binds SQL NULL instead of "" for absent values.
internal/storage/v2/dialect/spanner/compiler.go	Switches to new null-aware decision function; documents Spanner’s fixed NULL ordering (no clause emitted).
internal/storage/v2/dialect/spanner/compiler_test.go	Adds unit tests ensuring Spanner emits no NULLS clause and pins nullable-keyset compare SQL.
internal/storage/v2/dialect/postgres/session.go	Marks token_id and user_id as nullable and binds SQL NULL instead of "" for absent values.
internal/storage/v2/dialect/postgres/compiler.go	Uses null-aware compare expansion when required; emits NULLS FIRST/LAST in ORDER BY for nullable columns.
internal/storage/v2/dialect/postgres/compiler_test.go	Adds unit tests pinning ORDER BY NULLS behavior and nullable-keyset compare SQL.
internal/storage/v2/dialect/compare/null_aware.go	Adds NeedsNullAware logic for nullable keyset columns and refines null-safe ordered predicate generation.
internal/storage/v2/database/schema.go	Adds Nullable field to bindings, Schema.Nullable, and NullableValue helper for untyped-nil SQL NULL binding.
internal/storage/v2/database/filter.go	Adds Keyset flag to CompareFilter; CompareGreater/Less now mark keyset cursor predicates.
internal/storage/v2/database/coerce.go	Updates CoerceString to preserve JSON null as Go nil so nullable string cursors bind SQL NULL.
internal/storage/v2/database/coerce_test.go	Adds tests for CoerceString(nil) and NullableValue behavior.

[Copilot]

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 5 comments.

Suppressed comments (5)

internal/storage/v2/dialect/sqlite/nullable_bindings_test.go:54

• new("user") is invalid Go (built-in new needs a type). Use a local variable and take its address for the optional ObjectType pointer.

	schema := &domain.JSONSchema{ObjectType: new("user")}
	assert.Equal(t, []any{"user"}, jsonSchemaSchema.ValuesFrom(schema, cols))

internal/storage/v2/dialect/postgres/nullable_bindings_test.go:54

• new("user") is invalid Go (built-in new needs a type). Use a local variable and take its address for the optional ObjectType pointer.

	schema := &domain.JSONSchema{ObjectType: new("user")}
	assert.Equal(t, []any{"user"}, jsonSchemaSchema.ValuesFrom(schema, cols))

internal/storage/v2/dialect/spanner/nullable_bindings_test.go:54

• new("user") is invalid Go (built-in new needs a type). Use a local variable and take its address for the optional ObjectType pointer.

	schema := &domain.JSONSchema{ObjectType: new("user")}
	assert.Equal(t, []any{"user"}, jsonSchemaSchema.ValuesFrom(schema, cols))

internal/storage/v2/database/coerce_test.go:92

• new("usr_1") is invalid Go (built-in new requires a type). Use a local string variable and pass its address into NullableValue.

	assert.Equal(t, "usr_1", database.NullableValue(new("usr_1")))

internal/storage/v2/database/schema.go:21

• The doc comment is inaccurate: the typed-nil issue happens when returning p (the pointer) as any, not when returning *p (which dereferences). Please adjust the comment to match the actual behavior.

// NullableValue flattens a nil pointer to untyped nil so it binds as SQL NULL.
// Returning *p directly would box a typed nil that fails == nil checks.
func NullableValue[V any](p *V) any {

[cursor]

Thermo-nuclear code quality review

Verdict: REQUEST CHANGES

Behavior looks correct (stmttest paging + compiler SQL pins are the right proof layers). I’m not approving on that alone.

The PR adds a hand-maintained Nullable schema flag, gates null-aware OpLess on it, and triples identical binding tests — while the author’s own writeup admits unmarked nullable columns stay silently wrong until they become sort keys. That is incidental complexity with a clear deletion path:

1. Keyset ⇒ always CompileNullAware. In that path, always (col < ? OR col IS NULL) for OpLess (NOT NULL columns are unchanged).
2. PG/SQLite ORDER BY ⇒ always NULLS FIRST/LAST. Harmless on NOT NULL columns; Spanner keeps the engine-default comment.
3. Delete FieldBinding.Nullable / Schema.Nullable / the annotation+test sweep. Keep accessor untyped-nil fixes + CoerceString(nil) — those are the real data-plane fix.
4. Collapse the three byte-identical nullable_bindings_test.go files (and put any remaining shared nil-binding checks behind one helper).
5. Centralize the ASC NULLS FIRST / DESC NULLS LAST policy in dialect/compare so compilers don’t re-encode the invariant that OpLess-only NULL admission depends on.

CompareGreater/Less always setting Keyset=true is fine given GreaterThan/LessThan for ranges — but only if Keyset remains the only mode bit. Do not keep Keyset×Nullable.

Happy to re-review once the flag/layer is deleted or there’s a strong justification for keeping a manually synced DDL shadow.

[cursor]

NeedsNullAware should not consult schema.Nullable. if filter.Keyset (or any term is nil), take the null-aware path. the per-column flag turns one concept into a mode product and recreates the silent-miss hazard this PR is trying to close.

[cursor]

this file is byte-identical to sqlite/ and spanner/ except the package line. that violates the stmttest/dialect test-layering rule and mostly exists to assert the Nullable flag. if we delete the flag, drop these clones; if anything remains, one shared helper — not ×3.

[cursor]

this is a simplicity regression. pre-PR, null-aware OpLess always emitted OR IS NULL — which is a no-op on NOT NULL columns. gating on schema.Nullable adds a branch that only exists to serve the flag. please restore the unconditional form inside this path.

[cursor]

Keyset-always on CompareGreater/Less is fine (GreaterThan/LessThan stay the range API). what is not fine is Keyset × Nullable as two orthogonal bits. keep Keyset as the single mode that changes SQL shape; don’t make callers reason about a second flag.

[cursor]

this Nullable flag is the structural problem. it has to be hand-kept against DDL/accessors, and the PR already admits unmarked columns fail closed only by accident. code judo: Keyset-driven always-null-aware compare + always NULLS FIRST/LAST on PG/SQLite makes this field unnecessary. can we delete it instead of sweeping Nullable: true across every binding?

[cursor]

NULLS FIRST/LAST here is load-bearing for the OpLess-only IS NULL admission in compare. that invariant is currently tribal knowledge across packages. either always emit NULLS on every ORDER BY column (and drop the Nullable check), or move a shared WriteNullsOrder helper into dialect/compare so both sides share one policy.