refactor(refid)!: stop registering database/sql drivers in store subpackages - #186
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe database stores no longer register drivers. Documentation now requires callers to import drivers and pass opened ChangesDatabase driver registration
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to Database driver registration is now explicitly owned by callers, avoiding duplicate SQLite registration while allowing callers to choose their database driver. The updated documentation and tests align with this contract, with no remaining merge-readiness risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ackages
store/sqlite blank-imported modernc.org/sqlite and store/postgres
blank-imported pgx/v5/stdlib, purely as a convenience so callers could
sql.Open without a driver import of their own. Neither store needs it:
New and Migrate both receive an already-open *sql.DB and only ever issue
SQL against it.
The sqlite one actively breaks consumers. modernc.org/sqlite registers
the driver name "sqlite" in init(), as does github.com/glebarez/go-sqlite
(a fork of it, used by the GORM sqlite driver). Neither registration is
guarded, so any binary linking both panics at startup with
sql: Register called twice for driver sqlite
which made store/sqlite unusable in OpenNSW/agency. store/postgres does
not panic today — pgx guards its "pgx" registration, and only
pgx/v5/stdlib claims "pgx/v5" — but it is wrong for the same reason: a
store that receives a *sql.DB should not decide which driver gets linked
into its consumer's binary. Fixing both keeps the contract uniform
rather than an asymmetry the docs have to explain.
BREAKING CHANGE: callers must now import a driver themselves. A caller
relying on the old convenience will see `sql: unknown driver` from
sql.Open until it adds, e.g., _ "modernc.org/sqlite" or
_ "github.com/jackc/pgx/v5/stdlib". Nothing outside this module's own
tests imported either subpackage at the time of this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sthanikan2000
force-pushed
the
refactor/refid-store-driver-imports
branch
from
September 4, 2026 15:47
4ce9806 to
726cfd6
Compare
8 tasks
sthanikan2000
added a commit
to OpenNSW/agency
that referenced
this pull request
Sep 6, 2026
Drops the temporary replace directive now that OpenNSW/core#186 (the driver-registration fix refid/store/sqlite needs here) has merged, and repoints the require at that commit. Verified against the published module rather than the local checkout: build, vet and all tests pass, and the server boots without the "sql: Register called twice for driver sqlite" panic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sthanikan2000
added a commit
to OpenNSW/agency
that referenced
this pull request
Sep 6, 2026
Drops the temporary replace directive now that OpenNSW/core#186 (the driver-registration fix refid/store/sqlite needs here) has merged, and repoints the require at that commit. Verified against the published module rather than the local checkout: build, vet and all tests pass, and the server boots without the "sql: Register called twice for driver sqlite" panic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sthanikan2000
added a commit
to OpenNSW/agency
that referenced
this pull request
Sep 7, 2026
Drops the temporary replace directive now that OpenNSW/core#186 (the driver-registration fix refid/store/sqlite needs here) has merged, and repoints the require at that commit. Verified against the published module rather than the local checkout: build, vet and all tests pass, and the server boots without the "sql: Register called twice for driver sqlite" panic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ginaxu1
pushed a commit
to OpenNSW/agency
that referenced
this pull request
Sep 8, 2026
Drops the temporary replace directive now that OpenNSW/core#186 (the driver-registration fix refid/store/sqlite needs here) has merged, and repoints the require at that commit. Verified against the published module rather than the local checkout: build, vet and all tests pass, and the server boots without the "sql: Register called twice for driver sqlite" panic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sthanikan2000
added a commit
to OpenNSW/agency
that referenced
this pull request
Sep 8, 2026
* feat(refid): generate agency reference IDs on application inject An agency had no way to issue its own reference number for an application — the only identifier was the opaque NSW task ID. Where a number was needed it was typed by hand into the review form, with nothing guaranteeing it unique, sequential, correctly formatted, or scoped to the issuing office. Adopts github.com/OpenNSW/core/refid, split across two config layers so that what an agency can issue is a deployment decision while which tasks get one is a task decision: - refIDGen in config.yaml declares the formats (issuers, segments, lists). Optional — omit it and no task can generate a reference ID. - A new optional refid block in a task config names an (issuer, idType) from there, the JSON Pointer to store the result at, and params mapped to JSON Pointers into the injected data. Sourcing params from the data is what lets one task config serve every office rather than needing one config per office. Generated once, on first inject only; a re-inject keeps the number it already has. This required carrying ReviewerResponse forward in CreateApplication, since CreateOrUpdate does a full-row Save that would otherwise NULL the column and destroy an issued ID — the same reason ClaimedBy/ClaimedAt are already carried over. Generation failure fails the inject, so an application never exists without its reference ID. An unresolvable param maps to 400; an unconfigured issuer/idType, counter overflow or a database error to 500. Counters live in a new refid_sequences table (migration 000010) rather than refid's own Migrate helpers, keeping the .sql file the single source of truth for schema and getting down/status with it. The store reuses the existing GORM pool: a second sql.Open would be a different database for sqlite :memory: and a second competing writer for a file. internal/refidstore is tested against this module's real SQLite driver (glebarez), not modernc — refid's queries use RETURNING and ?N ordinal placeholders, which upstream only exercises against modernc. Requires the driver-registration fix in OpenNSW/core refid/store/*; the go.mod replace directive is temporary and must be dropped, and the require repointed at the merged ref, before this merges. Closes #306 * chore(deps): point refid at the merged core module Drops the temporary replace directive now that OpenNSW/core#186 (the driver-registration fix refid/store/sqlite needs here) has merged, and repoints the require at that commit. Verified against the published module rather than the local checkout: build, vet and all tests pass, and the server boots without the "sql: Register called twice for driver sqlite" panic. * fix(refid): address review feedback on reference ID generation Honour the documented refid.params contract. Three places said params may be declared generously because refid ignores keys a format doesn't consume, but generateRefID resolved every declared param and rejected the inject if any pointer missed. Resolve what's present and let refid decide what it needs: it returns ErrInvalidParam for a param a segment requires and for a scope key left with an unresolved placeholder, and that already maps to a 400. Generate before creating the consignment, so a generation failure leaves nothing behind. CreateConsignment fetches NSW extras and inserts a row, which previously survived a later generation failure as an orphan. The cost is a slightly wider window in which a crash strands the counter value just claimed, which refid tolerates by design. Skip building the counter store and registry when no refIDGen section is configured, rather than building an empty registry and taking a database handle for a feature that is off. refidstore.Disabled fills the gap: a Registry whose Generate always fails, so a task declaring refid on such a deployment is still a loud misconfiguration rather than a silent no-op, and application.NewService keeps its non-nil-dependency invariant. Its error wraps ErrUnknownIssuer so the HTTP mapping is unchanged, but names the real cause instead of reading like a task-config typo. The startup log now says "not configured" rather than "configured issuers=0". Pick the counter-table DDL by dialect in the end-to-end test. newTestStore runs against PostgreSQL when AGENCY_DB_DRIVER=postgres, which has no datetime('now'), so the test failed during setup on that path. Drop the migration number from the docs and refidstore's comment — it goes stale if migrations are ever collapsed. * refactor(refid): trim redundant tests and commentary Self-review pass over the PR, no behaviour change. Drop three redundant tests. TestRegistry_GeneratesFullID duplicated the application end-to-end test, which covers strictly more — same real registry and store, plus persistence, and both dialects rather than SQLite only. The orphan-consignment test merged into the unconfigured-deployment one, which shares its setup and trigger. The missing-required-param test folded into the end-to-end test, which already had the registry built and an adjacent rejection case. Cut commentary that states what isn't done rather than what the code does: the task-config doc no longer carries a note about review-payload validation being future work, keeping only the caveat a form author acts on. generateRefID's doc comment was longer than the function; the counter-burn trade-off in CreateApplication belongs in a commit message, not beside the code. Stop naming the migration by number in refidstore's test comment, for the same reason it was dropped elsewhere — it goes stale if migrations are ever collapsed. Both regression checks still catch what they were written for: the old generation ordering still leaves an orphan consignment, and removing the ReviewerResponse carry-forward still loses the ID on re-inject. * refactor(refid): generateRefID returns the ID, not a document generateRefID built a fresh JSONB and returned it for the caller to assign, which made two failures possible the moment anything changed: a caller running it against an existing reviewer response would silently discard that document, and the error path returned a nil map that nulls the field if the error is ever mishandled. It now returns a string, and CreateApplication owns the write. Fold the three consecutive `existing` checks into one if/else while here. They were mutually exclusive already, which is the only reason the reference ID write could not clobber a carried-forward reviewer response — as a single branch that safety is structural rather than incidental, and the new-application branch provably starts with no reviewer response, so no defensive nil check is needed. * refactor(refid): extract initRefIDs, drop HTTP codes from service docs Move the reference ID wiring out of main() into initRefIDs, which returns an error rather than calling log.Fatalf so it is testable. Three tests cover it, including that a deployment with no refIDGen section never reaches the database — the nil *gorm.DB they pass is the assertion. generateRefID's doc comment described its errors as a 400 and a 500. It isn't an HTTP handler and has no business naming status codes; it now says which sentinel it wraps and leaves the mapping to the handler.
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
2 participants
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
A store handed a
*sql.DBshouldn't decide which driver gets linked into its consumer's binary. Both subpackages did:store/sqliteblank-importedmodernc.org/sqliteandstore/postgresblank-importedpgx/v5/stdlib, purely as a convenience so callers couldsql.Openwithout an import of their own. Neither needs it —NewandMigrateboth receive an already-open connection and only ever issue SQL against it.For sqlite that convenience is actively breaking.
modernc.org/sqliteregisters the driver name"sqlite"ininit(), and so doesgithub.com/glebarez/go-sqlite— a fork of it, used by the GORM sqlite driver. Neither registration is guarded, so any binary linking both dies at startup:That made
store/sqliteunusable in OpenNSW/agency, which already links glebarez through its GORM setup.store/postgresdoes not panic today: pgx guards its"pgx"registration, and onlypgx/v5/stdlibclaims"pgx/v5". It changes anyway, because the rule above applies to it just as much — a consumer usinglib/pqstill compiles all of pgx for nothing and inherits its version constraints into their module graph. Fixing both leaves one contract rather than an asymmetry the docs have to keep explaining.Type of Change
Changes Made
store_test.gofiles, which had been relying on the package under test to register a driver for their ownsql.Open.Testing
go build ./... && go vet ./... && go test ./...all pass.Because
sql.Openresolves the driver by name at runtime, a missing import is not a compile error — so two extra checks:POSTGRES_TEST_DSN-gated test to run against an unreachable host fails withconnection refused, notunknown driver, confirming the test's own import registers pgx.go list -depson both stores now reports zero driver packages, wherestore/sqlitepreviously pulled in all of modernc (libc, memory, mathutil).Related Issues
Required by the reference ID integration in OpenNSW/agency (agency#306).
Additional Notes
Breaking: callers must now import a driver themselves (e.g.
_ "modernc.org/sqlite"), orsql.Openreturnssql: unknown driver. Nothing outside this module's own tests imports either subpackage today, so nothing breaks in practice — which is what makes now the cheap moment to do this.Summary by CodeRabbit
Documentation
database/sql.Bug Fixes