Skip to content

libsql: fix cursor step_end deserialization, honor last_insert_rowid - #2304

Open
freedom-winds wants to merge 1 commit into
tursodatabase:mainfrom
freedom-winds:fix-cursor-last-insert-rowid
Open

freedom-winds wants to merge 1 commit into
tursodatabase:mainfrom
freedom-winds:fix-cursor-last-insert-rowid

Conversation

@freedom-winds

Copy link
Copy Markdown

While debugging why Connection::last_insert_rowid() kept returning 0 after an INSERT executed through query() on a remote connection, I found that the hrana cursor path never receives the rowid at all.

The client's StepEndEntry declares a field named last_inserted_rowid, but both the Hrana 3 spec ("last_insert_rowid": string | null) and sqld's serializer use last_insert_rowid. Serde silently ignores the unknown key, so CursorStepState never sees a rowid: into_batch_result() always reports None, and HranaRows::next() — which already propagates affected_row_count back to the stream when a step ends — never propagates the rowid either, leaving Connection::last_insert_rowid() stale after query().

This renames the field to last_insert_rowid, deserializes it as Option<i64> accepting both the JSON number form sqld emits and the string form the spec defines, propagates the value to the stream like the non-cursor execute path already does, and adds a test covering both encodings.

The client's StepEndEntry expected a field named last_inserted_rowid,
but both the Hrana 3 spec and sqld's serializer use last_insert_rowid.
Serde silently ignored the unknown key, so the cursor never surfaced a
rowid: into_batch_result() always reported None and the query path never
updated the stream, leaving Connection::last_insert_rowid() stale after
query().

Rename the field to last_insert_rowid and deserialize it as Option<i64>,
accepting both the JSON number sqld emits and the string form the spec
defines. Propagate the value to the stream when the cursor step ends,
matching what the non-cursor execute path already does.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant