diff --git a/dbkit/postgres/CHANGELOG.md b/dbkit/postgres/CHANGELOG.md index 8c6354d..7f95e50 100644 --- a/dbkit/postgres/CHANGELOG.md +++ b/dbkit/postgres/CHANGELOG.md @@ -10,6 +10,19 @@ Releases of this module are tagged `dbkit/postgres/vX.Y.Z`. ## [Unreleased] +### Added + +- `pgtest.CheckAddress`, which returns every refusal of `pgtest.New`, those of `postgres.CheckAddress` included, marked `dbkit.ErrAddress` and never echoing the address. + +### Changed + +- `pgtest.New` and `pgtest.NewSwept` refuse a `pool_` key with no dot in the server address or its service file. + +### Fixed + +- `pgtest.NewSwept` checks the address before it sweeps, so a refused address never connects to the server. +- `Classify` classes 23001 as `dbkit.ErrForeignKey`, the code PostgreSQL 18 raises when an `ON DELETE RESTRICT` or `ON UPDATE RESTRICT` key stops a change. + ## [0.2.0] - 2026-10-10 ### Added diff --git a/dbkit/postgres/classify.go b/dbkit/postgres/classify.go index 7067db3..25dd3ef 100644 --- a/dbkit/postgres/classify.go +++ b/dbkit/postgres/classify.go @@ -15,6 +15,7 @@ import ( var codeClasses = map[string]error{ "23505": dbkit.ErrUnique, "23503": dbkit.ErrForeignKey, + "23001": dbkit.ErrForeignKey, "23502": dbkit.ErrNotNull, "23514": dbkit.ErrCheck, "55P03": dbkit.ErrBusy, diff --git a/dbkit/postgres/classify_test.go b/dbkit/postgres/classify_test.go index 6a58ed4..defa066 100644 --- a/dbkit/postgres/classify_test.go +++ b/dbkit/postgres/classify_test.go @@ -24,9 +24,13 @@ CREATE TABLE classify_rows ( amount integer CHECK (amount >= 0) ); CREATE TABLE classify_locks (id integer PRIMARY KEY, n integer NOT NULL); +CREATE TABLE classify_kept (id integer PRIMARY KEY); +CREATE TABLE classify_keepers (kept integer REFERENCES classify_kept (id) ON DELETE RESTRICT ON UPDATE RESTRICT); INSERT INTO classify_parents VALUES (1); INSERT INTO classify_rows VALUES (1, 'taken', 1, 0); INSERT INTO classify_locks VALUES (1, 0), (2, 0); +INSERT INTO classify_kept VALUES (1); +INSERT INTO classify_keepers VALUES (1); ` // classifyClasses lists every dbkit error class. @@ -146,15 +150,47 @@ func classifyMustCode(t *testing.T, err error, code string) { } } +// classifyCase is one server error a classify test raises and the class it wants. +type classifyCase struct { + // name names the case. + name string + // run raises the server error on a fresh handle. + run func(*testing.T, *sql.DB) error + // code is the code the server answers with. + code string + // class is the dbkit class the code carries, nil for none. + class error +} + +// classifyCheck fails t unless Classify gives the error of c its class alone and keeps its code and message. +func classifyCheck(t *testing.T, c classifyCase) { + t.Helper() + err := c.run(t, classifyOpen(t)) + classifyMustCode(t, err, c.code) + + got := postgres.Classify(err) + + classifyMustCode(t, got, c.code) + for _, class := range classifyClasses { + if want := class == c.class; errors.Is(got, class) != want { + t.Errorf("errors.Is(Classify(%v), %v) = %v, want %v", err, class, !want, want) + } + } + if c.class == nil && got != err { + t.Errorf("Classify(%v) = %v, want the error unchanged", err, got) + } + if c.class != nil && got.Error() != c.class.Error()+": "+err.Error() { + t.Errorf("Classify().Error() = %q, want the class and the server message", got.Error()) + } + if again := postgres.Classify(got); again != got { + t.Errorf("Classify(Classify()) = %v, want the classified error unchanged", again) + } +} + func TestClassify(t *testing.T) { t.Parallel() - cases := []struct { - name string - run func(*testing.T, *sql.DB) error - code string - class error - }{ + cases := []classifyCase{ {"unique", classifyExec("INSERT INTO classify_rows (id, code) VALUES (2, 'taken')"), "23505", dbkit.ErrUnique}, {"primary key", classifyExec("INSERT INTO classify_rows (id, code) VALUES (1, 'free')"), "23505", dbkit.ErrUnique}, @@ -162,6 +198,9 @@ func TestClassify(t *testing.T) { dbkit.ErrForeignKey}, {"a delete a foreign key stops", classifyExec("DELETE FROM classify_parents WHERE id = 1"), "23503", dbkit.ErrForeignKey}, + {"a restricted delete", classifyExec("DELETE FROM classify_kept WHERE id = 1"), "23001", dbkit.ErrForeignKey}, + {"a restricted update", classifyExec("UPDATE classify_kept SET id = 2 WHERE id = 1"), "23001", + dbkit.ErrForeignKey}, {"not null", classifyExec("INSERT INTO classify_rows (id, code) VALUES (4, NULL)"), "23502", dbkit.ErrNotNull}, {"check", classifyExec("INSERT INTO classify_rows VALUES (5, 'negative', 1, -1)"), "23514", dbkit.ErrCheck}, {"a row lock asked for with NOWAIT", classifyNoWait, "55P03", dbkit.ErrBusy}, @@ -175,26 +214,8 @@ func TestClassify(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { t.Parallel() - err := c.run(t, classifyOpen(t)) - classifyMustCode(t, err, c.code) - - got := postgres.Classify(err) - - classifyMustCode(t, got, c.code) - for _, class := range classifyClasses { - if want := class == c.class; errors.Is(got, class) != want { - t.Errorf("errors.Is(Classify(%v), %v) = %v, want %v", err, class, !want, want) - } - } - if c.class == nil && got != err { - t.Errorf("Classify(%v) = %v, want the error unchanged", err, got) - } - if c.class != nil && got.Error() != c.class.Error()+": "+err.Error() { - t.Errorf("Classify().Error() = %q, want the class and the server message", got.Error()) - } - if again := postgres.Classify(got); again != got { - t.Errorf("Classify(Classify()) = %v, want the classified error unchanged", again) - } + + classifyCheck(t, c) }) } } diff --git a/dbkit/postgres/pgtest/address.go b/dbkit/postgres/pgtest/address.go new file mode 100644 index 0000000..0bb3b3f --- /dev/null +++ b/dbkit/postgres/pgtest/address.go @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pgtest + +import ( + "fmt" + "strings" + + "github.com/gopherium/framework/dbkit" +) + +// poolPrefix starts every key pgxpool reads from an address. +const poolPrefix = "pool_" + +// errPoolKey refuses an address that sets a pool_ key, which pgtestdb would send to the server as a setting. +var errPoolKey = fmt.Errorf("%w: the address sets a pool_ key, which pgtestdb would send to the server, "+ + "remove it and set pool keys on the address New returns", dbkit.ErrAddress) + +// CheckAddress returns the error New refuses address with, or nil when New takes it. +func CheckAddress(address string) error { + _, err := configOf(address) + return err +} + +// checkPoolKeys returns errPoolKey when params holds a key with no dot that starts with poolPrefix. +func checkPoolKeys(params map[string]string) error { + for key := range params { + if strings.HasPrefix(key, poolPrefix) && !strings.Contains(key, ".") { + return errPoolKey + } + } + return nil +} diff --git a/dbkit/postgres/pgtest/address_test.go b/dbkit/postgres/pgtest/address_test.go new file mode 100644 index 0000000..124cde1 --- /dev/null +++ b/dbkit/postgres/pgtest/address_test.go @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pgtest_test + +import ( + "errors" + "net/url" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gopherium/framework/dbkit" + "github.com/gopherium/framework/dbkit/postgres/pgtest" +) + +const ( + // checkEmpty is the message of an empty or blank address. + checkEmpty = "dbkit: refused database address: the address is empty or blank, " + + "and pgx would read every setting from the PG* environment variables" + // checkServer is a server address with the secret parts of the address tests, ready for its query. + checkServer = "postgres://" + newSecretUser + "@" + newSecretHost + ":1/" + newSecretDatabase + "?" + // checkPoolKey is the message of an address that sets a pool_ key. + checkPoolKey = "dbkit: refused database address: the address sets a pool_ key, " + + "which pgtestdb would send to the server, remove it and set pool keys on the address New returns" +) + +// checkCase is one address a check test passes and the message it wants. +type checkCase struct { + // name names the case. + name string + // address is the address the case passes. + address string + // want is the message the case wants. + want string +} + +// checkPooledService returns the secret server address with a service whose file sets a pool_ key. +func checkPooledService(t *testing.T) string { + t.Helper() + file := filepath.Join(t.TempDir(), "pg_service.conf") + if err := os.WriteFile(file, []byte("[pooled]\npool_max_conns=4\n"), 0o600); err != nil { + t.Fatalf("write the service file: %v", err) + } + query := url.Values{"sslmode": {"disable"}, "service": {"pooled"}, "servicefile": {file}} + return checkServer + strings.ReplaceAll(query.Encode(), "+", "%20") +} + +// checkQueryKey returns the message of an address whose query sets the moving key. +func checkQueryKey(key string) string { + return "dbkit: refused database address: the query sets " + key + ", give it before the ? instead" +} + +// checkNoSecret fails t when message holds a part of an address the tests mark secret. +func checkNoSecret(t *testing.T, message string) { + t.Helper() + for _, secret := range []string{newSecret, newSecretUser, newSecretHost, newSecretDatabase} { + if strings.Contains(message, secret) { + t.Errorf("the message %q holds a secret part of the address, want none", message) + } + } +} + +// checkRefused fails t unless CheckAddress refuses the address of c with its want, the message New stops a test with. +func checkRefused(t *testing.T, c checkCase) { + t.Helper() + err := pgtest.CheckAddress(c.address) + if err == nil { + t.Fatalf("CheckAddress() = nil, want %q", c.want) + } + if !errors.Is(err, dbkit.ErrAddress) || err.Error() != c.want { + t.Errorf("CheckAddress() = %v, want %q marked dbkit.ErrAddress", err, c.want) + } + checkNoSecret(t, err.Error()) + if stopped := newRun(t, c.address).message; stopped != err.Error() { + t.Errorf("New() stopped the test with %q, want %q as CheckAddress answers", stopped, err.Error()) + } +} + +func TestCheckAddressRefusesEveryAddressNewRefuses(t *testing.T) { + t.Parallel() + + cases := []checkCase{ + {"an empty address", "", checkEmpty}, + {"a blank address", " \t", checkEmpty}, + {"a bare at", "postgres://u:" + newSecret + "@ss@localhost:5434/postgres", newBareAt}, + {"a keyword and value address", "host=localhost user=u password=" + newSecret, newNotURL}, + {"a keyword and value address with a stray %", "host=localhost%zz user=u password=" + newSecret, newNotURL}, + {"an address with another scheme", "mysql://u:" + newSecret + "@localhost/postgres", newUnparsable}, + {"a URL that does not parse", "postgres://u:" + newSecret + "@localhost:port/postgres", newUnparsable}, + {"an IPv6 literal host", "postgres://u:" + newSecret + "@[::1]:5434/postgres", newColonHost}, + {"a list of hosts with ports", "postgres://u:" + newSecret + "@localhost:5434,localhost:5434/postgres", + newColonHost}, + {"a query password", checkServer + "sslmode=disable&password=" + newSecret, newQueryPassword}, + {"a query password with an escaped key", checkServer + "sslmode=disable&pass%77ord=" + newSecret, + newQueryPassword}, + {"a query sslpassword", checkServer + "sslmode=disable&sslpassword=" + newSecret, newQuerySSLPassword}, + {"a query password and a bad sslmode", checkServer + "password=" + newSecret + "&sslmode=disabled", + newUnparsable}, + {"a query user", checkServer + "sslmode=disable&user=moved_user", checkQueryKey("user")}, + {"a query host", checkServer + "sslmode=disable&host=moved.example.com", checkQueryKey("host")}, + {"a query port", checkServer + "sslmode=disable&port=2", checkQueryKey("port")}, + {"a query dbname", checkServer + "sslmode=disable&dbname=moved_database", checkQueryKey("dbname")}, + {"a query database", checkServer + "sslmode=disable&database=moved_database", checkQueryKey("database")}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + + checkRefused(t, c) + }) + } +} + +func TestCheckAddressRefusesEveryPoolKey(t *testing.T) { + t.Parallel() + + cases := []checkCase{ + {"an escaped pool key", checkServer + "sslmode=disable&pool%5Fmax_conns=4", checkPoolKey}, + {"a pool key beside a query user", checkServer + "sslmode=disable&pool_max_conns=4&user=moved_user", + checkQueryKey("user")}, + } + for _, key := range []string{ + "pool_max_conns", "pool_min_conns", "pool_min_idle_conns", "pool_max_conn_lifetime", "pool_max_conn_idle_time", + "pool_health_check_period", "pool_max_conn_lifetime_jitter", "pool_ping_timeout", "pool_size", + } { + cases = append(cases, checkCase{"a query " + key, checkServer + "sslmode=disable&" + key + "=4", checkPoolKey}) + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + + checkRefused(t, c) + }) + } +} + +func TestCheckAddressRefusesAPoolKeyFromAServiceFile(t *testing.T) { + t.Parallel() + + checkRefused(t, checkCase{"a pool key from a service file", checkPooledService(t), checkPoolKey}) +} + +func TestCheckAddressAcceptsAnAddressNewTakes(t *testing.T) { + t.Parallel() + + for _, address := range []string{ + "postgres://u:p@localhost:5434/postgres?sslmode=disable&application_name=kept_option", + "postgresql://u:p@localhost:5434/postgres?sslmode=disable&application_name=kept_option", + "postgres://u:p@localhost:5434/postgres?sslmode=disable&application_name=pool_tests", + "postgres://u:p@localhost:5434/postgres?sslmode=disable&pool_app.tenant=1", + } { + if err := pgtest.CheckAddress(address); err != nil { + t.Errorf("CheckAddress() = %v, want nil for an address New takes", err) + } + } +} diff --git a/dbkit/postgres/pgtest/new.go b/dbkit/postgres/pgtest/new.go index 4ded9aa..3cab587 100644 --- a/dbkit/postgres/pgtest/new.go +++ b/dbkit/postgres/pgtest/new.go @@ -64,6 +64,9 @@ func configOf(address string) (pgtestdb.Config, error) { if err != nil { return pgtestdb.Config{}, err } + if err := checkPoolKeys(resolved.RuntimeParams); err != nil { + return pgtestdb.Config{}, err + } user, password, _ := strings.Cut(url.UserPassword(server.User.Username(), resolved.Password).String(), ":") return pgtestdb.Config{ DriverName: driverName, @@ -78,21 +81,43 @@ func configOf(address string) (pgtestdb.Config, error) { // serverURL returns the URL of address, or the error for a URL New refuses. func serverURL(address string) (*url.URL, error) { + server, err := parsedURL(address) + if err != nil { + return nil, err + } + if strings.Contains(server.Hostname(), ":") { + return nil, errColonHost + } + if err := checkQuery(server.Query()); err != nil { + return nil, err + } + return server, nil +} + +// parsedURL returns address read as a URL, or errNotURL for an address that is no postgres:// or postgresql:// URL. +func parsedURL(address string) (*url.URL, error) { server, err := url.Parse(address) - switch { - case err != nil || (server.Scheme != "postgres" && server.Scheme != "postgresql"): + if err != nil { return nil, errNotURL - case strings.Contains(server.Hostname(), ":"): - return nil, errColonHost - case server.Query().Has("password"): - return nil, errQueryPassword - case server.Query().Has("sslpassword"): - return nil, errQuerySSLPassword + } + if server.Scheme != "postgres" && server.Scheme != "postgresql" { + return nil, errNotURL + } + return server, nil +} + +// checkQuery returns the error for a query that sets a password or a key that would move the test databases. +func checkQuery(query url.Values) error { + if query.Has("password") { + return errQueryPassword + } + if query.Has("sslpassword") { + return errQuerySSLPassword } for _, key := range movingKeys { - if server.Query().Has(key) { - return nil, errQueryKey(key) + if query.Has(key) { + return errQueryKey(key) } } - return server, nil + return nil } diff --git a/dbkit/postgres/pgtest/swept.go b/dbkit/postgres/pgtest/swept.go index cbda015..cdb960f 100644 --- a/dbkit/postgres/pgtest/swept.go +++ b/dbkit/postgres/pgtest/swept.go @@ -21,7 +21,7 @@ type sweepKey struct { // sweeps holds the sweep of each address and scope that this test process ran. var sweeps sync.Map -// NewSwept sweeps the stale test databases of the server at address once per test process, then answers New. +// NewSwept checks address, sweeps the stale test databases of its server once per test process, then answers New. func NewSwept(t testing.TB, address string, olderThan time.Duration, migrator pgtestdb.Migrator) string { t.Helper() return newSwept(t, address, olderThan, migrator, "") @@ -33,6 +33,9 @@ func newSwept(t testing.TB, address string, olderThan time.Duration, migrator pg if olderThan <= 0 { t.Fatalf("dbkit: NewSwept needs olderThan above zero, got %v", olderThan) } + if err := CheckAddress(address); err != nil { + t.Fatal(err) + } once, _ := sweeps.LoadOrStore(sweepKey{address: address, scope: scope}, new(sync.Once)) once.(*sync.Once).Do(func() { if _, err := sweep(t.Context(), address, olderThan, scope); err != nil { diff --git a/dbkit/postgres/pgtest/swept_test.go b/dbkit/postgres/pgtest/swept_test.go index bb0fc7d..49e3be9 100644 --- a/dbkit/postgres/pgtest/swept_test.go +++ b/dbkit/postgres/pgtest/swept_test.go @@ -3,7 +3,9 @@ package pgtest_test import ( + "fmt" "net/url" + "slices" "strings" "testing" "time" @@ -38,9 +40,12 @@ func TestNewSweptSweepsEachAddressOnceThenHandsOutADatabase(t *testing.T) { keptBySameAddress := sweepExists(t, stale) third := pgtest.NewSweptWithin(t, other, sweepAnyAge, migrator, scope) - if !sweptFirst || !keptBySameAddress || sweepExists(t, stale) { - t.Errorf("the stale instances were swept by the first call %t, kept by the second %t, swept by another "+ - "address %t, want true, true and true", sweptFirst, keptBySameAddress, !sweepExists(t, stale)) + if !sweptFirst || !keptBySameAddress { + t.Errorf("the stale instances were swept by the first call %t and kept by the second %t, want true and true", + sweptFirst, keptBySameAddress) + } + if sweepExists(t, stale) { + t.Error("the stale instance outlived the call on another address, want that call to sweep it") } for _, address := range []string{first, second, third} { if got := newRows(t, address); got != 1 { @@ -105,29 +110,59 @@ func TestNewSweptLogsAFailedSweepAndHandsOutADatabase(t *testing.T) { logged := strings.Join(recorder.output, "\n") want := "dbkit: the sweep of stale test databases failed, the tests go on: dbkit: Sweep needs a role that may run " + "pg_stat_file" - if recorder.message != "" || address == "" || !strings.Contains(logged, want) { - t.Errorf("NewSwept() failed the test with %q, returned %q and logged %q, want no failure, a database and %q", - recorder.message, address, logged, want) + if recorder.message != "" || address == "" { + t.Errorf("NewSwept() failed the test with %q and returned %q, want no failure and a database", + recorder.message, address) + } + if !strings.Contains(logged, want) { + t.Errorf("NewSwept() logged %q, want %q", logged, want) } } +// sweptRun returns the recorder that run printed through once it ended. +func sweptRun(t *testing.T, run func(tb testing.TB)) *newFatal { + t.Helper() + recorder := &newFatal{TB: t} + done := make(chan struct{}) + go func() { + defer close(done) + run(recorder) + }() + <-done + return recorder +} + func TestNewSweptRefusesAnAgeAtOrBelowZero(t *testing.T) { t.Parallel() + refused := sweepUnreachable + "&user=moved_user" for age, want := range map[time.Duration]string{ 0: "dbkit: NewSwept needs olderThan above zero, got 0s", -time.Nanosecond: "dbkit: NewSwept needs olderThan above zero, got -1ns", } { - recorder := &newFatal{TB: t} - done := make(chan struct{}) - go func() { - defer close(done) - pgtest.NewSwept(recorder, serverAddress(), age, newMigrator()) - }() - <-done + recorder := sweptRun(t, func(tb testing.TB) { pgtest.NewSwept(tb, refused, age, newMigrator()) }) if recorder.message != want { t.Errorf("NewSwept() with the age %v failed the test with %q, want %q", age, recorder.message, want) } } } + +func TestNewSweptRefusesAnAddressBeforeItSweeps(t *testing.T) { + t.Parallel() + + for _, address := range []string{ + sweepUnreachable + "&user=moved_user", + sweepUnreachable + "&pool_max_conn_idle_time=1m", + "host=127.0.0.1 port=1 user=u password=p dbname=x sslmode=disable", + } { + recorder := sweptRun(t, func(tb testing.TB) { + pgtest.NewSweptWithin(tb, address, sweepAnyAge, newMigrator(), serverName("refused-")) + }) + + want := fmt.Sprint(pgtest.CheckAddress(address)) + if recorder.message != want || !slices.Equal(recorder.output, []string{want}) { + t.Errorf("NewSwept() printed %q, want only the refusal %q of CheckAddress", recorder.output, want) + } + } +}