fix(query): NULL-safe columnValue for cached-type default branch - #12
Open
totalslacker wants to merge 1 commit into
Open
fix(query): NULL-safe columnValue for cached-type default branch#12totalslacker wants to merge 1 commit into
totalslacker wants to merge 1 commit into
Conversation
Database.query(_:) captures column types once from the first row and reuses them across the whole result set. A nullable TEXT column whose first row has a value and later row is NULL sends the NULL cell into columnValue's `default:` branch (as SQLITE_TEXT), which called `String(cString: UnsafePointer(sqlite3_column_text(stmt, index)))` — force-unwrapping a nil return, trapping with "Unexpectedly found nil while implicitly unwrapping an Optional value". Guard the pointer and return nil, matching the SQLITE_NULL case's semantics (row dictionary omits the key). Two regression tests added, one for the crashing shape and one documenting the pre-existing "NULL-first-row-loses-later-value" behavior so this fix doesn't regress it. Reported downstream at totalslacker/WebBrain#361 (Dashboard timeline crash) and totalslacker/SemanticHistory#173 (pin-bump for the fix).
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
1 participant
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
Database.columnValue'sdefault:branch: guard the return ofsqlite3_column_textinstead of force-unwrapping viaUnsafePointer(nil!).DatabaseTests.swift.Why
Database.query(_:)caches column types once from the first row and reuses them for every subsequent row. A nullable TEXT column whose first row has a value and later row is NULL dispatches the NULL cell intocolumnValue'sdefault:branch (asSQLITE_TEXT) rather thancase SQLITE_NULL:. That branch callsString(cString: UnsafePointer(sqlite3_column_text(stmt, index)))—sqlite3_column_textreturns nil for a genuinely-NULL cell, andUnsafePointer(nil)!traps withUnexpectedly found nil while implicitly unwrapping an Optional value.Reproducible any time a SELECT returns a value-then-NULL sequence in the same nullable TEXT column across multiple rows. Not a corner case — this is the shape of any typical
SELECT * FROM twhere a nullable TEXT column has mixed content.Fix shape
Return
nilfromcolumnValuewhensqlite3_column_textreturns nil — matches thecase SQLITE_NULL:branch's semantics (row dictionary omits the key). Alternative considered — re-readsqlite3_column_typeper-row inside the loop — would structurally prevent the same class of bug in the other type-switch branches; noted in the code comment but skipped here as a scope-widening change. Happy to include the per-row re-read if maintainer prefers.Test plan
swift test --filter DatabaseTests— 12/12 pass in 0.021s locally on Apple Silicon macOS 14testNullableTextColumnAcrossRowsDoesNotCrash— the fix is the delta between "trap" and "pass"testNullFirstRowLosesValueInSecondRow— locks in the pre-existing "NULL-first-row-loses-later-value" behaviour (distinct bug from what this PR targets; documented so a future per-row-type change knows what it needs to fix)Downstream context
Surfaced by a downstream consumer (SkippedPageStore in a private app) hitting the crash in production. Full crash log available on request. Fix has also been landed on a downstream fork (
totalslacker/SQLiteVec#1) so the affected app can ship before upstream review latency; this PR is the equivalent contribution back to the source of truth.