feat: Add PostgreSQL 18 driver + driver refactor#33
feat: Add PostgreSQL 18 driver + driver refactor#33Unbreathable wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new PostgreSQL 18 service driver under pkg/databases/postgres, refactors Docker container lifecycle logic into reusable mrunner/services helpers, and migrates the legacy driver + real-project example to the new API.
Changes:
- Introduce new PostgreSQL 18 driver (
pkg/databases/postgres) with container lifecycle, health checks, initialization, and instruction handling. - Add
mrunner/serviceshelpers for managed container create/recreate + container exec, and update the legacy driver to use them. - Update
examples/real-projectto use the new PostgreSQL 18 driver.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/databases/postgres/postgres.go | New PostgreSQL 18 driver implementation and config helpers |
| pkg/databases/postgres/postgres_container.go | Container creation/health/init for new driver via mservices |
| pkg/databases/postgres/postgres_instruct.go | Clear/drop table instructions for new driver |
| pkg/databases/postgres/go.mod | New submodule definition for postgres driver |
| pkg/databases/postgres/go.sum | Dependency locks for new postgres submodule |
| pkg/databases/postgres-legacy/postgres_legacy_container.go | Refactored legacy container lifecycle to use mservices helpers |
| pkg/databases/postgres-legacy/postgres_legacy.go | Updated legacy driver docs/comments |
| pkg/databases/postgres-legacy/go.mod | Dependency cleanup / reclassification (direct -> indirect) |
| mrunner/services/containers.go | New shared “managed container” creation/recreate logic |
| mrunner/services/containers_exec.go | New shared helper to exec commands in containers |
| go.work | Add postgres driver submodule to workspace |
| examples/real-project/starter/config.go | Switch example to new postgres (18) driver |
| examples/real-project/go.mod | Switch example module dependency/replace to new postgres driver |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Env: []string{ | ||
| fmt.Sprintf("POSTGRES_PASSWORD=%s", PostgresPassword), | ||
| fmt.Sprintf("POSTGRES_USER=%s", PostgresUsername), | ||
| "POSTGRES_DATABASE=postgres", |
There was a problem hiding this comment.
The official postgres image uses POSTGRES_DB (not POSTGRES_DATABASE) to set the initial database name. With standard images this env var will be ignored. Consider switching to POSTGRES_DB=postgres (or removing it if the default is sufficient).
| "POSTGRES_DATABASE=postgres", | |
| "POSTGRES_DB=postgres", |
| module github.com/Liphium/magic/pkg/databases/postgres | ||
|
|
||
| go 1.25.7 | ||
|
|
||
| replace github.com/Liphium/magic/v3 => ../../../. |
There was a problem hiding this comment.
The module path in this new submodule doesn’t match how it’s imported elsewhere (e.g. github.com/Liphium/magic/v3/pkg/databases/postgres). With the current module github.com/Liphium/magic/pkg/databases/postgres, go will error with “module declares its path as … but was required as …” when consumers require/import the /v3/... path. Update the module line to the exact import path you expect users to require (likely github.com/Liphium/magic/v3/pkg/databases/postgres) and keep go.work/consumer replace directives aligned to it.
| replace github.com/Liphium/magic/v3 => ../../. | ||
|
|
||
| replace github.com/Liphium/magic/v3/pkg/databases/postgres-legacy => ../../pkg/databases/postgres-legacy | ||
| replace github.com/Liphium/magic/v3/pkg/databases/postgres => ../../pkg/databases/postgres | ||
|
|
||
| require ( | ||
| github.com/Liphium/magic/v3 v3.0.0-00010101000000-000000000000 | ||
| github.com/Liphium/magic/v3/pkg/databases/postgres-legacy v0.0.0-00010101000000-000000000000 | ||
| github.com/Liphium/magic/v3/pkg/databases/postgres v0.0.0-00010101000000-000000000000 | ||
| github.com/gofiber/fiber/v2 v2.52.9 |
There was a problem hiding this comment.
This example requires/replaces github.com/Liphium/magic/v3/pkg/databases/postgres, but the pkg/databases/postgres submodule’s go.mod currently declares a different module path. As-is, go mod resolution will fail with a module-path mismatch error. Align the driver submodule module path (and/or this require/replace) so they match exactly.
| return client.ExecInspectResult{}, fmt.Errorf("couldn't create command for readiness of container: %s", err) | ||
| } | ||
| execStartCheck := client.ExecStartOptions{Detach: false, TTY: false} | ||
| if _, err := c.ExecStart(ctx, execIDResp.ID, execStartCheck); err != nil { | ||
| return client.ExecInspectResult{}, fmt.Errorf("couldn't start command for readiness of container: %s", err) |
There was a problem hiding this comment.
ExecuteCommand is a generic helper, but all error messages mention “readiness of container”. This makes debugging confusing when the helper is used for non-readiness execs. Consider changing these error strings to be command-agnostic (e.g. “couldn't create exec”, “couldn't start exec”).
| return client.ExecInspectResult{}, fmt.Errorf("couldn't create command for readiness of container: %s", err) | |
| } | |
| execStartCheck := client.ExecStartOptions{Detach: false, TTY: false} | |
| if _, err := c.ExecStart(ctx, execIDResp.ID, execStartCheck); err != nil { | |
| return client.ExecInspectResult{}, fmt.Errorf("couldn't start command for readiness of container: %s", err) | |
| return client.ExecInspectResult{}, fmt.Errorf("couldn't create exec: %s", err) | |
| } | |
| execStartCheck := client.ExecStartOptions{Detach: false, TTY: false} | |
| if _, err := c.ExecStart(ctx, execIDResp.ID, execStartCheck); err != nil { | |
| return client.ExecInspectResult{}, fmt.Errorf("couldn't start exec: %s", err) |
| // Get all of the tables | ||
| res, err := conn.Query("SELECT table_name FROM information_schema.tables WHERE table_schema NOT IN ('pg_catalog', 'information_schema')") | ||
| if err != nil { | ||
| return fmt.Errorf("couldn't get database tables: %v", err) | ||
| } | ||
| for res.Next() { | ||
| var name string | ||
| if err := res.Scan(&name); err != nil { | ||
| return fmt.Errorf("couldn't get database table name: %v", err) | ||
| } | ||
| if err := fn(name, conn); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
iterateTables doesn’t close the Rows result or check Rows.Err() after iteration. This can leak resources and can silently ignore iteration errors. Add a defer res.Close() after a successful Query, and check res.Err() after the for res.Next() loop.
| Env: []string{ | ||
| fmt.Sprintf("POSTGRES_PASSWORD=%s", PostgresPassword), | ||
| fmt.Sprintf("POSTGRES_USER=%s", PostgresUsername), | ||
| "POSTGRES_DATABASE=postgres", |
There was a problem hiding this comment.
The official postgres image uses POSTGRES_DB (not POSTGRES_DATABASE) to set the initial database name. As written, this env var will be ignored by standard images, which is confusing and makes it look like the DB name is configurable when it isn’t. Consider switching to POSTGRES_DB=postgres (or dropping it entirely since postgres is the default).
| "POSTGRES_DATABASE=postgres", | |
| "POSTGRES_DB=postgres", |
| return pd.iterateTables(container, func(tableName string, conn *sql.DB) error { | ||
| if _, err := conn.Exec(fmt.Sprintf("truncate %s CASCADE", tableName)); err != nil { | ||
| return fmt.Errorf("couldn't truncate table %s: %v", tableName, err) | ||
| } |
There was a problem hiding this comment.
tableName is interpolated directly into the TRUNCATE statement. Even though it comes from information_schema, unquoted identifiers can break on mixed-case/reserved-word table names and can be abused if a table name contains SQL metacharacters. Use proper identifier quoting (e.g. quote_ident) and/or include schema + table and quote both parts before executing.
| return pd.iterateTables(container, func(tableName string, conn *sql.DB) error { | ||
| if _, err := conn.Exec(fmt.Sprintf("DROP TABLE %s CASCADE", tableName)); err != nil { | ||
| return fmt.Errorf("couldn't drop table table %s: %v", tableName, err) | ||
| } |
There was a problem hiding this comment.
tableName is interpolated directly into the DROP TABLE statement. Unquoted identifiers can fail for many valid table names (reserved words, mixed case, special chars) and can allow SQL injection if a maliciously named table exists. Quote identifiers (and ideally schema + table) before executing.
| func (pd *PostgresDriver) DropTables(container mconfig.ContainerInformation) error { | ||
| return pd.iterateTables(container, func(tableName string, conn *sql.DB) error { | ||
| if _, err := conn.Exec(fmt.Sprintf("DROP TABLE %s CASCADE", tableName)); err != nil { | ||
| return fmt.Errorf("couldn't drop table table %s: %v", tableName, err) |
There was a problem hiding this comment.
Typo in error message: duplicated word “table”.
| return fmt.Errorf("couldn't drop table table %s: %v", tableName, err) | |
| return fmt.Errorf("couldn't drop table %s: %v", tableName, err) |
| "strings" | ||
|
|
||
| "github.com/Liphium/magic/v3/mconfig" | ||
| "github.com/Liphium/magic/v3/util" |
There was a problem hiding this comment.
This driver calls sql.Open("postgres", ...) but the package doesn’t import/register any Postgres database/sql driver (e.g. blank-importing github.com/lib/pq). Right now it implicitly relies on some other package (like mrunner) to have side-effect imports, which makes the driver easy to use incorrectly and can lead to runtime sql: unknown driver "postgres" errors in other integration paths. Consider adding the blank import here (and adding the dependency in this submodule) so the driver is self-contained, matching the legacy driver’s pattern.
| "github.com/Liphium/magic/v3/util" | |
| "github.com/Liphium/magic/v3/util" | |
| _ "github.com/lib/pq" |
Description
pkg/databasesdirectory for drivers that supports PostgreSQL 18 and its new container layoutmservicespackage for more easily creating driversreal-projectexample has been updated to use the new PostgreSQL 18 driver