fix(#6): narrow lastInsertId to string or null - #17
Merged
Conversation
A `string | number` union invites servers to emit 64-bit rowids as JSON numbers, which silently lose precision in clients whose numbers are IEEE-754 doubles. Narrow the field to string-or-null, with integer ids encoded as decimal strings the way section 5 already encodes `bigint`. The D1 worker example stringifies; the conformance nice-to-have entry states the string form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lastInsertIdwas typed asstring | number | null, and thenumberarm is a precision trap: a 64-bit auto-increment rowid emitted as a JSON number silently loses fidelity in any client whose numbers are IEEE-754 doubles, which is most of them. This narrows the field to string-or-null across the three places it is specified or produced — the normative bullet in SPEC.md section 6.1, the D1 worker reference server, and the conformance suite's "nice to have" entry — with integer ids encoded as decimal strings exactly the way section 5 already encodesbigint. Nothing in the repo relied on thenumberarm, so the change is free today and would not be once third-party servers ship.Acceptance criteria mapping
1. SPEC.md narrows
lastInsertIdto string-or-nullThe section 6.1 field bullet now reads "OPTIONAL, string or null" and states explicitly that the value is never a JSON number, giving the reason (64-bit ids vs. IEEE-754 doubles) and pinning the integer encoding to a decimal string "consistent with the
bigintencoding in section 5". The decimal rule is deliberately scoped to the integer case: the spec's own examples and the conformance fixture use a TEXT primary key, so a blanket "always a decimal integer" rule would have falsified them. Text ids are stated to pass through as-is. The surrounding JSON examples needed no edit — SPEC.md:113 already showsnulland SPEC.md:131-132 already show"1"/"2".Evidence: SPEC.md:120, the
lastInsertIdbullet under section 6.1;git grep -n lastInsertId SPEC.mdreturns no remaining occurrence of thenumberarm.2. The D1 worker example stringifies
projectD1Resultin the Cloudflare Worker-to-D1 reference server now converts D1's numericmeta.last_row_idto a string, and its localStatementResultinterface narrows tostring | nullso the compiler enforces it. The conversion is written as an explicit null check beforeString(...)rather thanString(x ?? null), which would have produced the literal string"null"whenever D1 reported no rowid.Evidence:
examples/cloudflare-worker-to-d1/src/index.ts:24(interface field) and thelastInsertId: lastRowId === undefined || lastRowId === null ? null : String(lastRowId)line insideprojectD1Result.3. Conformance "nice to have" entry expects the string form
The optional-behaviour bullet for
lastInsertIdnow states the wire form as well as the population rule: a JSON string (integer ids as decimal strings) ornull, never a JSON number. It stays in the optional section rather than becoming a numbered mandatory case, since populating the field at all remains optional; only its encoding-when-present is now pinned.Evidence: the
lastInsertIdbullet under the "## Optional / "nice to have"" heading inconformance/README.md.Not done
Three things left out on purpose. First, D1 sets
meta.last_row_idto0on SELECT/UPDATE/DELETE, not only on INSERT, so the worker now emits"0"where it previously emitted0— the same over-reporting, differently typed. Mapping that tonullis a behaviour change beyond "stringifies" and would deserve its own issue and its own spec sentence about when the field is meaningful. Second, the remainingstring | number | nulldeclarations inexamples/cloudflare-durable-object/src/index.ts:69,examples/reference-server.ts:16, andexamples/reference-client.ts:11are untouched: they are type declarations only, no runtime path in them emits a number (the Durable Object hardcodeslastInsertId: null, verified at line 118), and the issue scoped the change to the D1 worker. Third, no typecheck was run: the Worker example has apackage.jsonwith onlydev/deployscripts and no installed dependencies, sotscwould fail on missing@cloudflare/workers-typesrather than on this diff; the two edited lines were reviewed by hand instead.Closes #6