feat: replace mattn/go-sqlite3 with modernc.org/sqlite (pure Go, no CGO)#15
feat: replace mattn/go-sqlite3 with modernc.org/sqlite (pure Go, no CGO)#15
Conversation
Switches the SQLite driver from the CGO-based mattn/go-sqlite3 to the pure-Go modernc.org/sqlite. The modernc driver is registered under the "sqlite3" name so all existing DSN schemes (sqlite3://, sqlite://) and dialect configuration continue to work without any changes. This eliminates the CGO requirement for SQLite builds, enabling cross-compilation and simpler Docker/CI builds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR switches the SQLite backend used by pop from the CGO-based github.com/mattn/go-sqlite3 to the pure-Go modernc.org/sqlite, while preserving existing sqlite3/sqlite dialect and URL scheme behavior via explicit driver registration.
Changes:
- Replace the SQLite driver dependency with
modernc.org/sqliteand update transitive module dependencies. - Register the modernc SQLite driver under the
"sqlite3"driver name behind thesqlitebuild tag. - Update
golang.org/x/*dependency versions as part of the module graph changes.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
dialect_sqlite_tag.go |
Switches driver loading/registration from mattn’s init-based registration to an explicit sql.Register("sqlite3", ...) using modernc. |
go.mod |
Drops direct mattn/go-sqlite3 requirement, adds modernc.org/sqlite, and bumps several dependencies. |
go.sum |
Updates checksums to reflect the new dependency graph (modernc + related transitive modules). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
modernc.org/sqlite only recognizes _pragma=name(value) DSN params. Legacy mattn-style params (_fk, _journal_mode, _busy_timeout) are silently ignored. finalizerSQLite now: - translates _fk, _journal_mode, _busy_timeout to _pragma= equivalents - adds busy_timeout(5000) and foreign_keys(1) defaults via _pragma - sets _time_format=sqlite for correct time round-tripping The fix works by building url.Values (which supports duplicate keys) from RawOptions or the Options map, then re-encoding to RawOptions.
…ound-trip
The _time_format=sqlite option writes times with +00:00 timezone which
time.Parse creates as FixedZone("", 0) rather than time.UTC (nil).
This causes reflect.DeepEqual failures when comparing times written as
UTC to times read back from the database.
Without _time_format, modernc uses t.String() format which includes the
"UTC" timezone name. time.Parse correctly identifies this as time.UTC
(nil), making round-trips preserve the location for reflect.DeepEqual.
The scan errors originally attributed to missing _time_format were
caused by DEFAULT 'CURRENT_TIMESTAMP' (string literal) in migrations,
not by the format itself. That bug is fixed separately in the migrations.
|
WDYT about these findings:
I also found some issues:
Don't get me wrong, I would love for it to work. |
I find the conclusion hard to believe to be honest! SQLite is not a toy database |
Tests and client code check cd.Options for legacy keys (_fk, _busy_timeout) but finalizerSQLite only wrote translated pragmas to cd.RawOptions. Add two setOptionWithDefault calls to reflect applied defaults back into cd.Options. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pragma detection
- Remove sqlite build tag: modernc.org/sqlite is pure-Go, no build constraint needed
- Delete dialect_nosqlite_test.go: its !sqlite complement is obsolete
- urlParserSQLite3: set cd.RawOptions from raw query string so finalizerSQLite
sees all duplicate _pragma keys (map[string]string drops duplicates silently)
- sqlitePragmaSet: require '(' after pragma name to prevent false prefix match
(e.g. foreign_keys_per_table would have matched foreign_keys check)
- Expand legacy param translation from 3 to 23 entries covering the full mattn
DSN surface: _foreign_keys/_fk, _journal_mode/_journal, _busy_timeout/_timeout,
_synchronous/_sync, _auto_vacuum/_vacuum, _case_sensitive_like/_cslike,
_defer_foreign_keys/_defer_fk, _locking_mode/_locking, _recursive_triggers/_rt,
_cache_size, _ignore_check_constraints, _query_only, _secure_delete, _writable_schema
- Warn and drop _loc/tz/timezone (mattn-only, no modernc equivalent)
- Generic pragma echo-back: all applied _pragma values reflected into cd.Options
via sqliteOptionKey map (foreign_keys -> _fk for backward compat)
- Replace wrong-polarity _time_format=sqlite test with negative guard
- Add comprehensive DSN-level tests: RawOptions assertions, legacy param table,
programmatic path, direct _pragma round-trip
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, fix double TrimSpace - Extract sqliteInternalKeys to package-level var (avoid per-call allocation) - Add moderncSQLiteParams allowlist: warn and strip any DSN param not supported by modernc.org/sqlite (vfs, _pragma, _time_format, _txlock, _time_integer_format, _inttotime, _texttotime) - Remove spurious _loc warning for tz/timezone (neither was ever a mattn param) - Remove redundant outer TrimSpace in pragma value extraction - Update tests: unsupported pass-through params (foo=bar) are now stripped from both RawOptions and cd.Options Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| @@ -1,15 +0,0 @@ | |||
| //go:build !sqlite | |||
| // +build !sqlite | |||
| if err := genericSelectOne(c, model, query); err != nil { | ||
| return fmt.Errorf("sqlite select one: %w", err) | ||
| } | ||
| normalizeTimesToUTC(model.Value) |
There was a problem hiding this comment.
The problem is that modernc and mattn parse timezones differently, and to ensure we return UTC codes consistently, we normalize this here.
…race The parallel subtests both call finalizerSQLite which writes to the package-level log var, racing with setNewTestLogger in migration tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
LGTM, terrific |
Summary
Replaces
github.com/mattn/go-sqlite3(CGO-based) withmodernc.org/sqlite(pure Go). The modernc driver is registered under the"sqlite3"name, so all existing DSN schemes (sqlite3://,sqlite://) and dialect configuration continue to work without any further changes.Why this is safe
SQLite's on-disk file format — including WAL mode — is a public specification at https://sqlite.org/fileformat.html and is implementation-independent. Both drivers implement the same spec:
mattn/go-sqlite3wraps the official SQLite C amalgamation via CGOmodernc.org/sqliteis a mechanical C→Go translation of the same amalgamationThey are not different databases. They are different build-time approaches to the same library. Any conforming SQLite implementation reads any other's files — existing production databases written by mattn are fully readable/writable by modernc.
Additional confirmation:
.shmshared memory index file is transient and regenerated on open by both drivers — not a concern for migrationWhy this is needed
mattn/go-sqlite3 via CGO:
CGO_ENABLED=1explicitly)Switching to modernc enables pure-Go builds and is a prerequisite for removing CGO requirements across the Ory monorepo (Kratos, Hydra, Keto, Oathkeeper).
DSN compatibility
This change also resolves the open risk item for the monorepo migration: pop previously required
sqlite3://to select the mattn driver by name. By registering modernc under the"sqlite3"name, thesqlite3://scheme continues to work — no DSN changes are needed in consuming services.Test plan
go build -tags sqlite ./...— cleango test -tags sqlite -run TestSqlite ./...— all pass🤖 Generated with Claude Code