Repository navigation
Conversation
Accept sqlite:path and sqlite://path consistently in Go, Java, JavaScript, Ruby, and Rust, including :memory: and offline SQL export. Normalize Java URLs before JDBC dispatch and select the Ruby adapter's native URL form while retaining sqlite3: and jdbc:sqlite: aliases. Cover relative and absolute files, in-memory databases, and export without opening a database. Document the common forms and native aliases.
5d524e0 to
e5466b3
Compare
bgentry
left a comment
There was a problem hiding this comment.
🤖 Agent review summary. Blake reviewed each of these comments before they were posted.
Issues that likely warrant fixing before merge:
- Active Record misreads relative paths: a regression that can target the wrong file, with a one-line fix and spec change.
benchfails onsqlite::memory:: with a one-line fix and regression test.- An empty path reports success on a throwaway database: with a Go fix and regression test. The other CLIs would need the same rule.
Non-blocking:
The merge conflict with master is only in CHANGELOG.md.
Separately, and already on master: PGDATABASE overrides --database-url in the Go CLI, because RunCommand checks pgEnvConfigured() first. With PGDATABASE set, migrate-up --database-url sqlite:f.db ignores the URL and goes to Postgres. That contradicts the CLI's help text, which says --database-url takes precedence, and could run migrations against the wrong database. It's outside this PR, but probably deserves its own fix.
| connection = if sqlite_path == ":memory:" | ||
| {adapter: "sqlite3", database: sqlite_path} | ||
| elsif sqlite_path | ||
| "sqlite3://#{sqlite_path}" |
There was a problem hiding this comment.
🤖 Regression for Active Record: it reads the first segment after // as a host, so this breaks relative paths and can target the wrong file. For example, sqlite3://storage/s.db resolves to host storage and database /s.db, and sqlite3://rel.db resolves to no database at all. That means sqlite3:rel.db now fails with "No database file specified", and sqlite3:storage/s.db targets /s.db, which would succeed for a process that can write to /. Both worked before this change. It also breaks the documented sqlite://storage/river.sqlite3.
Dropping the // fixes it. A subdirectory path, a plain filename, and an absolute path all created the right file through the real CLI:
| "sqlite3://#{sqlite_path}" | |
| "sqlite3:#{sqlite_path}" |
The specs pass by accident because the relative path climbs out through ../../../, and dropping the first .. as the "host" still lands on the right absolute file. Creating the temp directory under the working directory exposes this. In ruby/spec/migration_cli_shared_examples.rb:
- Dir.mktmpdir("river-cli-urls-") do |directory|
+ # Stay under the working directory so the relative path has no `../`
+ # prefix, which makes an adapter that misreads its first segment visible.
+ Dir.mktmpdir("river-cli-urls-", Dir.pwd) do |directory|With that spec change alone, all four prefixes fail on Active Record and pass on Sequel. With the fix as well, the spec file passes.
| if !ok { | ||
| var hasProtocol bool | ||
| if sqlitePath, sqlite := strings.CutPrefix(*databaseURL, "sqlite:"); sqlite { | ||
| protocol, urlWithoutProtocol, hasProtocol = "sqlite", strings.TrimPrefix(sqlitePath, "//"), true |
There was a problem hiding this comment.
🤖 Accepting the short form here exposes a gap in bench. Its in-memory migration check at river_cli.go:415 only looks at parsedDatabaseURL.Host, but url.Parse("sqlite::memory:") leaves Host empty and puts :memory: in Opaque. So river bench --database-url sqlite::memory: skips the migration and fails with SQL logic error: no such table: river_job (1), while sqlite://:memory: works.
One-line fix in cmd/river/rivercli/river_cli.go:
- if parsedDatabaseURL.Host == ":memory:" {
+ if parsedDatabaseURL.Host == ":memory:" || parsedDatabaseURL.Opaque == ":memory:" {Suggested regression test, as the first subtest in TestBaseCommandSetSQLiteURLs in river_cli_test.go. The sqlite::memory: case fails on this branch and passes with the fix:
t.Run("BenchMemory", func(t *testing.T) {
t.Parallel()
for _, databaseURL := range []string{"sqlite::memory:", "sqlite://:memory:"} {
t.Run(databaseURL, func(t *testing.T) {
t.Parallel()
runFunc(t, databaseURL, "bench", "--num-total-jobs", "10")
})
}
})| migration commands and application processes. | ||
|
|
||
| For `migrate-get`, `--database-url` only selects the SQL dialect. Both `sqlite:` | ||
| and `sqlite://` work without a path, and export never opens or creates a database. |
There was a problem hiding this comment.
🤖 Bare forms are documented only for migrate-get, but migration commands accept them too, and SQLite treats an empty path as a temporary database. So migrate-up --database-url sqlite: reports every migration as applied and exits 0 on Go, Sequel, Java, and Rust, with nothing saved. An unset variable in sqlite:$DB_PATH would hit this and look like a successful deploy step. Active Record raises a raw exception instead, and JavaScript rejects it with a usage error.
For Go, the bare sqlite: is new in this PR, while sqlite:// already behaved this way. Could every CLI reject an empty path except for migrate-get? A tested Go version, in RunCommand in command.go, though there may be a cleaner way to mark commands that don't need a database:
case "sqlite":
+ if _, isMigrateGet := any(command).(*migrateGet); urlWithoutProtocol == "" && !isMigrateGet {
+ return false, fmt.Errorf("SQLite database URL (`%s`) needs a path or `:memory:`", *databaseURL)
+ }
+
dbPool, err := openSQLitePool(protocol, urlWithoutProtocol)Suggested regression test, as a subtest between DialectHint and File in TestBaseCommandSetSQLiteURLs. It fails on this branch and passes with the change above:
t.Run("EmptyPathRejected", func(t *testing.T) {
t.Parallel()
for _, databaseURL := range []string{"sqlite:", "sqlite://"} {
t.Run(databaseURL, func(t *testing.T) {
t.Parallel()
cli := NewCLI(&Config{Name: "River"})
cli.SetOut(&bytes.Buffer{})
cmd := cli.BaseCommandSet()
cmd.SetArgs([]string{"migrate-up", "--database-url", databaseURL})
require.ErrorContains(t, cmd.Execute(), "needs a path")
})
}
})|
|
||
| ### Fixed | ||
|
|
||
| - Normalize `sqlite://` URLs before opening JDBC connections so relative paths remain relative and `sqlite://:memory:` opens an in-memory database. Preserve `sqlite:` and native `jdbc:sqlite:` URLs. [PR #1588](https://github.com/riverqueue/river/pull/1588). |
There was a problem hiding this comment.
🤖 Non-blocking, and probably low impact given how few Java users there are yet: sqlite://var/lib/x.db used to open the absolute /var/lib/x.db and now opens ./var/lib/x.db. Worth a brief mention here if you think anyone is relying on it.
| Java also accepts native `jdbc:sqlite:` URLs, passed unchanged to the JDBC driver. | ||
| These aliases are specific to those languages. Driver-specific URI options and | ||
| query parameters are not portable between implementations; for example, Rust's | ||
| `?mode=rwc` creates a missing database file. |
There was a problem hiding this comment.
🤖 Non-blocking docs suggestion. "Not portable" undersells this a little, because a URL can silently select a different file by language rather than just lose an option. For example:
sqlite:q.db?mode=rwcopensq.dbon Go and Ruby, but creates a file literally namedq.db?mode=rwcon Java and JavaScript.sqlite:a%20b.dbopensa b.dbon Ruby and Rust, buta%20b.dbon Go, Java, and JavaScript.sqlite::memory:?cache=sharedstays in memory on Go, but creates an on-disk file on Java and JavaScript.
Maybe add something like: "Query parameters and percent-encoding are handled differently by each language, and can select a different file. For portable URLs, avoid ? and % in paths, and don't add parameters to :memory:."
🤖 Align SQLite URLs across the Go, Java, JavaScript, Ruby, and Rust CLIs. Both
sqlite:pathandsqlite://pathselect the same database, includingsqlite::memory:andsqlite://:memory:. Offlinemigrate-getalso accepts the baresqlite:andsqlite://dialect hints without opening a database.Go and JavaScript now accept the short form. Java strips the optional
//before calling JDBC so relative paths stay relative and the long memory form works. Ruby translates either prefix for the selected Active Record or Sequel adapter. Rust's SQL export now accepts the same short forms as its migration commands. Ruby'ssqlite3:aliases and Java's nativejdbc:sqlite:URLs remain supported.Regression tests cover file paths, alternate forms reopening the same database, memory databases, and offline export. The shared URL reference documents portable forms and driver-specific aliases/options.
Validation:
GOMAXPROCS=4 GOFLAGS=-p=2 make test lint.make lint/ruston Rust 1.99.1.