Skip to content

Commit bff8718

Browse files
committed
Driver tests for full lib/pq connection + necessary fixes
So it turns out that while bearing some relation to `lib/pq` through various pieces that sqlc pulls in, the `riverdatabasesql` driver didn't actually work when used with `lib/pq`. When testing it, we'd open a `sql.DB` pool _via_ Pgx, so Pgx was doing all the heavy lifting. Here, add an alternate driver test strategy to test `riverdatabasesql` with a `lib/pq` pool, thereby ensuring that the full range of operations works even with no Pgx in the mix at all. Things were broken, so we get those fixed up. There we two main problems: * As described in #545, `lib/pq` doesn't handle intervals correctly, and reads them in as ints, which is a big problem because they're in nanoseconds. We work around this by injecting intervals (like leader TTLs) an second floats instead, which really doesn't feel that much worse. * The custom bits class for `unique_states` didn't work. To fix this I move to an alternate strategy of inserts `unique_states` as an integer, a technique that I've been using in SQLite because there's no `bits` type. I could make a strong argument that the use of an integer over `bits` would've been a better design from the get go anyway, because it makes all the bit-wise arithmetic much simpler as you use C-style conventions like `states & (1 << 3)` that are very widespread and well understood. Either way though, things work reasonably well if we insert as an integer, then transition to storage in bits. Fixes #545 and #566. Supersedes #882.
1 parent 5114d52 commit bff8718

16 files changed

Lines changed: 166 additions & 106 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1111

1212
- Job kinds must comply to a format of `\A[\w][\w\-\[\]<>\/.·:+]+\z`, mainly in an attempt to eliminate commas and spaces to make format more predictable for an upcoming search UI. This check can be disabled for now using `Config.SkipJobKindValidation`, but this option will likely be removed in a future version of River. [PR #879](https://github.com/riverqueue/river/pull/879).
1313

14+
### Fixed
15+
16+
- The `riverdatabasesql` now fully supports raw connections through [`lib/pq`](https://github.com/lib/pq) rather than just `database/sql` through Pgx. We don't recommend the use of `lib/pq` as it's an unmaintained project, but this change should help with compatibility for older projects. [PR #883](https://github.com/riverqueue/river/pull/883).
17+
1418
## [0.21.0] - 2025-05-02
1519

1620
⚠️ Internal APIs used for communication between River and River Pro have changed. If using River Pro, make sure to update River and River Pro to latest at the same time to get compatible versions. River v0.21.0 is compatible with River Pro v0.13.0.

driver_test.go

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/jackc/pgx/v5"
1212
"github.com/jackc/pgx/v5/stdlib"
13+
"github.com/lib/pq"
1314
"github.com/stretchr/testify/require"
1415

1516
"github.com/riverqueue/river/internal/rivercommon"
@@ -19,10 +20,41 @@ import (
1920
"github.com/riverqueue/river/riverdriver/riverdatabasesql"
2021
"github.com/riverqueue/river/riverdriver/riverpgxv5"
2122
"github.com/riverqueue/river/rivershared/riversharedtest"
23+
"github.com/riverqueue/river/rivershared/util/urlutil"
2224
"github.com/riverqueue/river/rivertype"
2325
)
2426

25-
func TestDriverDatabaseSQL(t *testing.T) {
27+
func TestDriverDatabaseSQLLibPQ(t *testing.T) {
28+
t.Parallel()
29+
30+
ctx := context.Background()
31+
32+
connector, err := pq.NewConnector(urlutil.DatabaseSQLCompatibleURL(riversharedtest.TestDatabaseURL()))
33+
require.NoError(t, err)
34+
35+
stdPool := sql.OpenDB(connector)
36+
t.Cleanup(func() { require.NoError(t, stdPool.Close()) })
37+
38+
driver := riverdatabasesql.New(stdPool)
39+
40+
riverdrivertest.Exercise(ctx, t,
41+
func(ctx context.Context, t *testing.T) (riverdriver.Driver[*sql.Tx], string) {
42+
t.Helper()
43+
44+
return driver, riverdbtest.TestSchema(ctx, t, driver, nil)
45+
},
46+
func(ctx context.Context, t *testing.T) riverdriver.Executor {
47+
t.Helper()
48+
49+
tx := riverdbtest.TestTx(ctx, t, driver, nil)
50+
51+
// TODO(brandur): Set `search_path` path here when SQLite changes come in.
52+
53+
return riverdatabasesql.New(nil).UnwrapExecutor(tx)
54+
})
55+
}
56+
57+
func TestDriverDatabaseSQLPgx(t *testing.T) {
2658
t.Parallel()
2759

2860
var (

internal/riverinternaltest/riverdrivertest/riverdrivertest.go

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package riverdrivertest
33
import (
44
"context"
55
"encoding/json"
6+
"errors"
67
"fmt"
78
"slices"
89
"sort"
@@ -2708,7 +2709,8 @@ func Exercise[TTx any](ctx context.Context, t *testing.T,
27082709
defer close(goroutineDone)
27092710

27102711
_, err := otherExec.PGAdvisoryXactLock(ctx, 123456)
2711-
require.ErrorIs(t, err, context.Canceled)
2712+
// pgx will produce a context.Canceled error, but pg swallows it to emit its own
2713+
require.Regexp(t, "(context canceled|pq: canceling statement due to user request)", err.Error())
27122714
}()
27132715

27142716
select {
@@ -3387,22 +3389,30 @@ func exerciseListener[TTx any](ctx context.Context, t *testing.T, driverWithPool
33873389
func requireEqualTime(t *testing.T, expected, actual time.Time) {
33883390
t.Helper()
33893391

3390-
// Leaving off the nanosecond portion has the effect of truncating it rather
3391-
// than rounding to the nearest microsecond, which functionally matches
3392-
// pgx's behavior while persisting.
33933392
const rfc3339Micro = "2006-01-02T15:04:05.999999Z07:00"
33943393

3395-
require.Equal(t,
3396-
expected.Format(rfc3339Micro),
3397-
actual.Format(rfc3339Micro),
3394+
// This is a bit unfortunate, but while Pgx truncates to the nearest
3395+
// microsecond, lib/pq will round, thereby producing off-by-one microsecond
3396+
// problems in tests without intervention. Here, allow either the truncated
3397+
// or rounded version. It's a bit gnarly, but the upcoming SQLite change
3398+
// brings in a better way to accomplish this, so it should be short-lived.
3399+
var (
3400+
actualFormatted = actual.Format(rfc3339Micro)
3401+
expectedRoundedFormatted = expected.Round(1 * time.Microsecond).Format(rfc3339Micro)
3402+
expectedTruncatedFormatted = expected.Format(rfc3339Micro)
33983403
)
3404+
require.True(t, expectedRoundedFormatted == actualFormatted || expectedTruncatedFormatted == actualFormatted,
3405+
"Expected time %s to be equal to either %s (rounded, for lib/pq) or %s (truncated, for Pgx)", actualFormatted, expectedRoundedFormatted, expectedTruncatedFormatted)
33993406
}
34003407

34013408
func requireMissingRelation(t *testing.T, err error, missingRelation string) {
34023409
t.Helper()
34033410

34043411
var pgErr *pgconn.PgError
3405-
require.ErrorAs(t, err, &pgErr)
3406-
require.Equal(t, pgerrcode.UndefinedTable, pgErr.Code)
3407-
require.Equal(t, fmt.Sprintf(`relation "%s" does not exist`, missingRelation), pgErr.Message)
3412+
if errors.As(err, &pgErr) {
3413+
require.Equal(t, pgerrcode.UndefinedTable, pgErr.Code)
3414+
require.Equal(t, fmt.Sprintf(`relation "%s" does not exist`, missingRelation), pgErr.Message)
3415+
} else {
3416+
require.ErrorContains(t, err, fmt.Sprintf("pq: relation %q does not exist", missingRelation))
3417+
}
34083418
}

riverdbtest/riverdbtest.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,11 @@ func TestTx[TTx any](ctx context.Context, tb testing.TB, driver riverdriver.Driv
399399
return
400400
}
401401

402+
// Cancelled context again, but this one from libpq.
403+
if err.Error() == "driver: bad connection" {
404+
return
405+
}
406+
402407
// Similar to the above, but a newly appeared error that wraps the
403408
// above. As far as I can tell, no error variables are available to use
404409
// with `errors.Is`.

riverdriver/riverdatabasesql/internal/dbsqlc/models.go

Lines changed: 1 addition & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go

Lines changed: 6 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

riverdriver/riverdatabasesql/internal/dbsqlc/river_leader.sql.go

Lines changed: 8 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

riverdriver/riverdatabasesql/internal/dbsqlc/sqlc.yaml

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,15 @@ sql:
4949
- db_type: "jsonb"
5050
go_type: "string"
5151

52+
- db_type: "pg_catalog.bit"
53+
go_type: int
54+
55+
- db_type: "pg_catalog.bit"
56+
go_type:
57+
type: int
58+
pointer: true
59+
nullable: true
60+
5261
- db_type: "pg_catalog.interval"
5362
go_type: "time.Duration"
5463

@@ -60,23 +69,3 @@ sql:
6069
type: "time.Time"
6170
pointer: true
6271
nullable: true
63-
64-
# There doesn't appear to be a good type that's suitable for database/sql other
65-
# than the ones in pgtype. It's not great to make the database/sql driver take
66-
# a dependency on pgx, but the reality is most users will (or should) be using
67-
# pgx anyway.
68-
#
69-
# Unfortunately due to some sqlc limitations, you can't just use the
70-
# pgtype package directly (it tries to use the non-v5 import path and
71-
# you end up with duplicate pgtype imports). So there's an alias
72-
# package that exposes it indirectly.
73-
- db_type: "pg_catalog.bit"
74-
go_type:
75-
import: "github.com/riverqueue/river/riverdriver/riverdatabasesql/internal/pgtypealias"
76-
type: "Bits"
77-
78-
- db_type: "pg_catalog.bit"
79-
go_type:
80-
import: "github.com/riverqueue/river/riverdriver/riverdatabasesql/internal/pgtypealias"
81-
type: "Bits"
82-
nullable: true

riverdriver/riverdatabasesql/internal/pgtypealias/pgtype_alias.go

Lines changed: 0 additions & 9 deletions
This file was deleted.

0 commit comments

Comments
 (0)