From 7da57058d4c26c3b1f0d785f5137d6cdc1d3647e Mon Sep 17 00:00:00 2001
From: Michael McGuinness <mmcguinness@aarete.com>
Date: Tue, 14 Jan 2025 17:28:26 +0000
Subject: [PATCH] Merged in bugfix/flakydb (pull request #21)

Bugfix/flakydb

* init

* fixenvvar

* fixtest
---
 README.md                                     | 10 +++-
 internal/api/listener_test.go                 |  6 ++-
 internal/database/config_test.go              | 32 ++++--------
 internal/database/connection.go               | 29 ++++++++---
 internal/database/connection_test.go          |  4 +-
 internal/database/migrations.go               | 50 ++++++-------------
 internal/database/migrations_test.go          |  2 -
 internal/database/migrationsprivate_test.go   |  4 +-
 .../database/repository/collector_test.go     |  4 +-
 internal/database/repository/query_test.go    |  4 +-
 internal/database/repository/result_test.go   |  4 +-
 internal/queue/listener_test.go               |  4 +-
 internal/server/server_test.go                |  4 +-
 internal/test/api_test.go                     |  4 +-
 internal/test/container_test.go               |  4 +-
 internal/test/database.go                     |  3 +-
 internal/test/database_test.go                | 17 +++++++
 17 files changed, 103 insertions(+), 82 deletions(-)

diff --git a/README.md b/README.md
index e55df6e8..67fc44a3 100644
--- a/README.md
+++ b/README.md
@@ -19,4 +19,12 @@ Using the following project as a baseline: https://github.com/golang-standards/p
 
 To find new packages: `https://search.nixos.org/packages`
 
-For regular usage, please use `task` scripts in order run the most appropriate configuration.
\ No newline at end of file
+For regular usage, please use `task` scripts in order run the most appropriate configuration.
+
+# Environment Variables
+
+When using environment variables, there is a hiearchy that will be respected. (First will override the next)
+
+1. Within the devbox.json file, a variable added to "env"
+2. Within the devbox.json file, a variable exported in "init_hook"
+3. A variable added to .env
\ No newline at end of file
diff --git a/internal/api/listener_test.go b/internal/api/listener_test.go
index 29177ffa..804a495a 100644
--- a/internal/api/listener_test.go
+++ b/internal/api/listener_test.go
@@ -2,6 +2,8 @@ package api
 
 import (
 	"context"
+	"os"
+	"path"
 	"queryorchestration/internal/database"
 	"queryorchestration/internal/server"
 	"queryorchestration/internal/test"
@@ -16,7 +18,7 @@ func TestNew(t *testing.T) {
 
 	_, cleanup := test.CreateDB(t, ctx, &test.CreateDatabaseConfig{
 		Migrations: &database.MigrationConfig{
-			BasePath: "../..",
+			BasePath: path.Join(os.Getenv("PWD"), "../.."),
 		},
 	})
 	defer cleanup()
@@ -26,7 +28,7 @@ func TestNew(t *testing.T) {
 	}
 	cfg := &Config{
 		GetControllers: getControllers,
-		BasePath:       "../..",
+		BasePath:       path.Join(os.Getenv("PWD"), "../.."),
 	}
 
 	server := New(ctx, cfg)
diff --git a/internal/database/config_test.go b/internal/database/config_test.go
index 512e0b9d..8af2673c 100644
--- a/internal/database/config_test.go
+++ b/internal/database/config_test.go
@@ -6,37 +6,36 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestMustGetConnectionString(t *testing.T) {
+func TestMustGetURIInvalidPort(t *testing.T) {
 	t.Setenv("DB_USER", "user")
 	t.Setenv("DB_PASS", "pass")
 	t.Setenv("DB_HOST", "host")
 	t.Setenv("DB_PORT", "port")
 	t.Setenv("DB_NAME", "name")
 
-	connStr := mustGetConnectionString()
-	assert.Equal(t, "postgres://user:pass@host:port/name?", connStr)
+	assert.Panics(t, func() { mustGetURI() })
 }
 
-func TestMustGetConnectionStringNoSSL(t *testing.T) {
+func TestMustGetURINoSSL(t *testing.T) {
 	t.Setenv("DB_USER", "user")
 	t.Setenv("DB_PASS", "pass")
 	t.Setenv("DB_HOST", "host")
-	t.Setenv("DB_PORT", "port")
+	t.Setenv("DB_PORT", "5432")
 	t.Setenv("DB_NAME", "name")
 	t.Setenv("DB_NOSSL", "1")
 
-	connStr := mustGetConnectionString()
-	assert.Equal(t, "postgres://user:pass@host:port/name?sslmode=disable", connStr)
+	connStr := mustGetURI()
+	assert.Equal(t, "postgres://user:pass@host:5432/name?sslmode=disable", connStr.String())
 }
 
-func TestMustGetConnectionStringNoEnv(t *testing.T) {
+func TestMustGetURINoEnv(t *testing.T) {
 	t.Setenv("DB_USER", "")
 	t.Setenv("DB_PASS", "")
 	t.Setenv("DB_HOST", "")
 	t.Setenv("DB_PORT", "")
 	t.Setenv("DB_NAME", "")
 
-	assert.Panics(t, func() { mustGetConnectionString() })
+	assert.Panics(t, func() { mustGetURI() })
 }
 
 func TestGetPoolConfig(t *testing.T) {
@@ -48,18 +47,7 @@ func TestGetPoolConfig(t *testing.T) {
 
 	config, err := getPoolConfig()
 	assert.Nil(t, err)
-	assert.Equal(t, config.ConnString(), mustGetConnectionString())
-}
-
-func TestGetPoolConfigInvalidPort(t *testing.T) {
-	t.Setenv("DB_USER", "user")
-	t.Setenv("DB_PASS", "pass")
-	t.Setenv("DB_HOST", "host")
-	t.Setenv("DB_PORT", "invalid_port")
-	t.Setenv("DB_NAME", "name")
-
-	_, err := getPoolConfig()
-	assert.EqualError(t, err, "cannot parse `postgres://user:xxxxxx@host:invalid_port/name?`: failed to parse as URL (invalid port \":invalid_port\" after host)")
+	assert.Equal(t, config.ConnString(), mustGetURI().String())
 }
 
 func TestMustGetPoolConfig(t *testing.T) {
@@ -70,7 +58,7 @@ func TestMustGetPoolConfig(t *testing.T) {
 	t.Setenv("DB_NAME", "name")
 
 	config := mustGetPoolConfig()
-	assert.Equal(t, config.ConnString(), mustGetConnectionString())
+	assert.Equal(t, config.ConnString(), mustGetURI().String())
 }
 
 func TestMustGetPoolConfigInvalidPort(t *testing.T) {
diff --git a/internal/database/connection.go b/internal/database/connection.go
index 835a6ad2..bb66095d 100644
--- a/internal/database/connection.go
+++ b/internal/database/connection.go
@@ -4,8 +4,10 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"net/url"
 	"queryorchestration/internal/env"
 
+	"github.com/docker/go-connections/nat"
 	"github.com/jackc/pgx/v5"
 	"github.com/jackc/pgx/v5/pgxpool"
 )
@@ -14,7 +16,7 @@ type config struct {
 	user       string
 	password   string
 	host       string
-	port       string
+	port       nat.Port
 	name       string
 	disableSSL bool
 }
@@ -24,6 +26,10 @@ func mustGetConfig() *config {
 	pass := env.GetPanic("DB_PASS")
 	host := env.GetPanic("DB_HOST")
 	port := env.GetPanic("DB_PORT")
+	natPort, err := nat.NewPort("tcp", port)
+	if err != nil {
+		log.Panicf("Failed to create port: %v", err)
+	}
 	name := env.GetPanic("DB_NAME")
 
 	disableSSL, _ := env.Get("DB_NOSSL")
@@ -32,13 +38,13 @@ func mustGetConfig() *config {
 		user:       user,
 		password:   pass,
 		host:       host,
-		port:       port,
+		port:       natPort,
 		name:       name,
 		disableSSL: disableSSL == "1",
 	}
 }
 
-func mustGetConnectionString() string {
+func mustGetURI() *url.URL {
 	driver := "postgres"
 	conf := mustGetConfig()
 	opts := ""
@@ -47,13 +53,20 @@ func mustGetConnectionString() string {
 		opts += "sslmode=disable"
 	}
 
-	return fmt.Sprintf("%s://%s:%s@%s:%s/%s?%s", driver, conf.user, conf.password, conf.host, conf.port, conf.name, opts)
+	connStr := fmt.Sprintf("%s://%s:%s@%s:%d/%s?%s", driver, conf.user, conf.password, conf.host, conf.port.Int(), conf.name, opts)
+
+	uri, err := url.Parse(connStr)
+	if err != nil {
+		log.Panicf("Unable to parse URI: %s", err)
+	}
+
+	return uri
 }
 
 func getPoolConfig() (*pgxpool.Config, error) {
-	connStr := mustGetConnectionString()
+	connStr := mustGetURI()
 
-	config, err := pgxpool.ParseConfig(connStr)
+	config, err := pgxpool.ParseConfig(connStr.String())
 	if err != nil {
 		return nil, err
 	}
@@ -82,9 +95,9 @@ func GetDBPool(ctx context.Context) *pgxpool.Pool {
 }
 
 func GetDBConn(ctx context.Context) *pgx.Conn {
-	config := mustGetConnectionString()
+	config := mustGetURI()
 
-	conn, err := pgx.Connect(ctx, config)
+	conn, err := pgx.Connect(ctx, config.String())
 	if err != nil {
 		log.Panicf("Unable to connect to database: %v\n", err)
 	}
diff --git a/internal/database/connection_test.go b/internal/database/connection_test.go
index 8a242073..780b7da6 100644
--- a/internal/database/connection_test.go
+++ b/internal/database/connection_test.go
@@ -2,6 +2,8 @@ package database_test
 
 import (
 	"context"
+	"os"
+	"path"
 	"queryorchestration/internal/database"
 	"queryorchestration/internal/test"
 	"testing"
@@ -13,7 +15,7 @@ func TestDBConn(t *testing.T) {
 	ctx := context.Background()
 	_, cleanup := test.CreateDB(t, ctx, &test.CreateDatabaseConfig{
 		Migrations: &database.MigrationConfig{
-			BasePath: "../..",
+			BasePath: path.Join(os.Getenv("PWD"), "../.."),
 		},
 	})
 	defer cleanup()
diff --git a/internal/database/migrations.go b/internal/database/migrations.go
index 8db50d1c..90021813 100644
--- a/internal/database/migrations.go
+++ b/internal/database/migrations.go
@@ -9,31 +9,33 @@ import (
 	"path"
 
 	"github.com/golang-migrate/migrate/v4"
-	"github.com/golang-migrate/migrate/v4/database/postgres"
+	_ "github.com/golang-migrate/migrate/v4/database/postgres"
 	_ "github.com/golang-migrate/migrate/v4/source/file"
-	"github.com/jackc/pgx/v5"
+	_ "github.com/lib/pq"
 )
 
-func createDB(ctx context.Context) {
+func createDB() {
 	driver := "postgres"
-	opts := ""
 	conf := mustGetConfig()
+
+	opts := ""
 	if conf.disableSSL {
 		opts += "sslmode=disable"
 	}
-	connStr := fmt.Sprintf("%s://%s:%s@%s:%s?%s", driver, conf.user, conf.password, conf.host, conf.port, opts)
 
-	conn, err := pgx.Connect(ctx, connStr)
+	connStr := fmt.Sprintf("%s://%s:%s@%s:%d/?%s", driver, conf.user, conf.password, conf.host, conf.port.Int(), opts)
+
+	db, err := sql.Open("postgres", connStr)
 	if err != nil {
 		log.Panicf("Error opening database: %v", err)
 	}
 
-	err = conn.Ping(ctx)
+	err = db.Ping()
 	if err != nil {
 		log.Panicf("Error pinging database: %v", err)
 	}
 
-	rs, err := conn.Query(ctx, fmt.Sprintf("SELECT 'CREATE DATABASE %s' WHERE NOT EXISTS (SELECT FROM pg_database WHERE datname = '%s')", conf.name, conf.name))
+	rs, err := db.Query(fmt.Sprintf("SELECT 'CREATE DATABASE %s' WHERE NOT EXISTS (SELECT FROM pg_database WHERE datname = '%s')", conf.name, conf.name))
 	if err != nil {
 		log.Panicf("Error creating database: %v", err)
 	}
@@ -46,45 +48,23 @@ func createDB(ctx context.Context) {
 }
 
 type MigrationConfig struct {
-	BasePath         string
-	ConnectionString string
+	BasePath string
 }
 
 func RunMigrations(ctx context.Context, config *MigrationConfig) {
-	// createDB(ctx) - Add back only if necessary
+	createDB()
 
-	connStr := config.ConnectionString
-	if connStr == "" {
-		connStr = mustGetConnectionString()
-	}
+	connStr := mustGetURI()
 
 	basePath := config.BasePath
 	if basePath == "" {
 		basePath = os.Getenv("PWD")
 	}
 
-	driver := "postgres"
-
-	db, err := sql.Open(driver, connStr)
-	if err != nil {
-		log.Panicf("failed to open database: %v", err)
-	}
-	defer db.Close()
-
-	if err = db.Ping(); err != nil {
-		log.Panic("failed to connect to database:", err)
-	}
-
-	conn, err := postgres.WithInstance(db, &postgres.Config{})
-	if err != nil {
-		log.Panic("failed to create migrations: ", err)
-	}
-
 	migPath := "file://" + path.Join(basePath, "database/migrations")
-	m, err := migrate.NewWithDatabaseInstance(
+	m, err := migrate.New(
 		migPath,
-		driver,
-		conn,
+		connStr.String(),
 	)
 	if err != nil {
 		log.Panicf("failed to create migrate instance: %v", err)
diff --git a/internal/database/migrations_test.go b/internal/database/migrations_test.go
index 216c4498..57e77fc8 100644
--- a/internal/database/migrations_test.go
+++ b/internal/database/migrations_test.go
@@ -19,8 +19,6 @@ func TestRunMigrations(t *testing.T) {
 	})
 	defer cleanup()
 
-	t.Setenv("DB_NOSSL", "1")
-
 	database.RunMigrations(ctx, &database.MigrationConfig{
 		BasePath: path.Join(os.Getenv("PWD"), "../.."),
 	})
diff --git a/internal/database/migrationsprivate_test.go b/internal/database/migrationsprivate_test.go
index 684199f8..ed30f131 100644
--- a/internal/database/migrationsprivate_test.go
+++ b/internal/database/migrationsprivate_test.go
@@ -1,13 +1,11 @@
 package database
 
 import (
-	"context"
 	"testing"
 )
 
 func TestCreateDB(t *testing.T) {
 	t.Skip("Must test appropriately")
-	ctx := context.Background()
 
-	createDB(ctx)
+	createDB()
 }
diff --git a/internal/database/repository/collector_test.go b/internal/database/repository/collector_test.go
index e25edef6..43072b61 100644
--- a/internal/database/repository/collector_test.go
+++ b/internal/database/repository/collector_test.go
@@ -2,6 +2,8 @@ package repository_test
 
 import (
 	"context"
+	"os"
+	"path"
 	"queryorchestration/internal/database"
 	"queryorchestration/internal/database/repository"
 	"queryorchestration/internal/test"
@@ -15,7 +17,7 @@ func TestCollector(t *testing.T) {
 	ctx := context.Background()
 	db, cleanup := test.CreateDB(t, ctx, &test.CreateDatabaseConfig{
 		Migrations: &database.MigrationConfig{
-			BasePath: "../../..",
+			BasePath: path.Join(os.Getenv("PWD"), "../../.."),
 		}})
 	defer cleanup()
 
diff --git a/internal/database/repository/query_test.go b/internal/database/repository/query_test.go
index 66018af8..659fdb6e 100644
--- a/internal/database/repository/query_test.go
+++ b/internal/database/repository/query_test.go
@@ -3,6 +3,8 @@ package repository_test
 import (
 	"context"
 	"log"
+	"os"
+	"path"
 	"queryorchestration/internal/database"
 	"queryorchestration/internal/database/repository"
 	"queryorchestration/internal/test"
@@ -17,7 +19,7 @@ func TestQueries(t *testing.T) {
 
 	db, cleanup := test.CreateDB(t, ctx, &test.CreateDatabaseConfig{
 		Migrations: &database.MigrationConfig{
-			BasePath: "../../..",
+			BasePath: path.Join(os.Getenv("PWD"), "../../.."),
 		}})
 	defer cleanup()
 
diff --git a/internal/database/repository/result_test.go b/internal/database/repository/result_test.go
index d965df7c..152e3d7a 100644
--- a/internal/database/repository/result_test.go
+++ b/internal/database/repository/result_test.go
@@ -2,6 +2,8 @@ package repository_test
 
 import (
 	"context"
+	"os"
+	"path"
 	"queryorchestration/internal/database"
 	"queryorchestration/internal/database/repository"
 	"queryorchestration/internal/test"
@@ -17,7 +19,7 @@ func TestResults(t *testing.T) {
 
 	db, cleanup := test.CreateDB(t, ctx, &test.CreateDatabaseConfig{
 		Migrations: &database.MigrationConfig{
-			BasePath: "../../..",
+			BasePath: path.Join(os.Getenv("PWD"), "../../.."),
 		}})
 	defer cleanup()
 
diff --git a/internal/queue/listener_test.go b/internal/queue/listener_test.go
index b3abb8d1..41450625 100644
--- a/internal/queue/listener_test.go
+++ b/internal/queue/listener_test.go
@@ -2,6 +2,8 @@ package queue_test
 
 import (
 	"context"
+	"os"
+	"path"
 	"queryorchestration/internal/queue"
 	"queryorchestration/internal/server"
 	"queryorchestration/internal/test"
@@ -29,7 +31,7 @@ func TestNew(t *testing.T) {
 
 	queue.NewServer(ctx, &queue.ListenerConfig{
 		Controller: controller,
-		BasePath:   "../..",
+		BasePath:   path.Join(os.Getenv("PWD"), "../.."),
 	})
 }
 
diff --git a/internal/server/server_test.go b/internal/server/server_test.go
index 0dc9252c..cb416db4 100644
--- a/internal/server/server_test.go
+++ b/internal/server/server_test.go
@@ -2,6 +2,8 @@ package server_test
 
 import (
 	"context"
+	"os"
+	"path"
 	"queryorchestration/internal/server"
 	"queryorchestration/internal/test"
 	"testing"
@@ -18,7 +20,7 @@ func TestNew(t *testing.T) {
 	defer cleanup()
 
 	newCfg := &server.NewConfig{
-		BasePath: "../..",
+		BasePath: path.Join(os.Getenv("PWD"), "../.."),
 	}
 
 	cfg := server.New(ctx, newCfg)
diff --git a/internal/test/api_test.go b/internal/test/api_test.go
index 29fa0541..d08f808f 100644
--- a/internal/test/api_test.go
+++ b/internal/test/api_test.go
@@ -2,6 +2,8 @@ package test_test
 
 import (
 	"context"
+	"os"
+	"path"
 	"queryorchestration/internal/database"
 	"queryorchestration/internal/test"
 	"testing"
@@ -17,7 +19,7 @@ func TestCreateAPIContainer(t *testing.T) {
 	dbcfg, dbcleanup := test.CreateDB(t, ctx, &test.CreateDatabaseConfig{
 		Network: ncfg,
 		Migrations: &database.MigrationConfig{
-			BasePath: "../..",
+			BasePath: path.Join(os.Getenv("PWD"), "../.."),
 		},
 	})
 	defer dbcleanup()
diff --git a/internal/test/container_test.go b/internal/test/container_test.go
index 5573dc6b..a8ef33df 100644
--- a/internal/test/container_test.go
+++ b/internal/test/container_test.go
@@ -2,6 +2,8 @@ package test
 
 import (
 	"context"
+	"os"
+	"path"
 	"queryorchestration/internal/database"
 	"testing"
 
@@ -16,7 +18,7 @@ func TestCreateContainer(t *testing.T) {
 	dbcfg, dbcleanup := CreateDB(t, ctx, &CreateDatabaseConfig{
 		Network: ncfg,
 		Migrations: &database.MigrationConfig{
-			BasePath: "../..",
+			BasePath: path.Join(os.Getenv("PWD"), "../.."),
 		},
 	})
 	defer dbcleanup()
diff --git a/internal/test/database.go b/internal/test/database.go
index 91fa6fef..77f292b4 100644
--- a/internal/test/database.go
+++ b/internal/test/database.go
@@ -52,7 +52,8 @@ func CreateDB(t *testing.T, ctx context.Context, cfg *CreateDatabaseConfig) (*Da
 			"POSTGRES_PASSWORD": pass,
 		},
 		ExposedPorts: []string{port.Port()},
-		WaitingFor:   wait.ForListeningPort(port),
+		// WaitingFor:   wait.ForLog("database system is ready to accept connections"),
+		WaitingFor: wait.ForExposedPort(),
 	}
 
 	if cfg.Network != nil {
diff --git a/internal/test/database_test.go b/internal/test/database_test.go
index fcc0c5e7..7cff56af 100644
--- a/internal/test/database_test.go
+++ b/internal/test/database_test.go
@@ -2,6 +2,9 @@ package test_test
 
 import (
 	"context"
+	"os"
+	"path"
+	"queryorchestration/internal/database"
 	"queryorchestration/internal/test"
 	"testing"
 
@@ -19,3 +22,17 @@ func TestCreateDB(t *testing.T) {
 
 	cleanup()
 }
+
+func TestCreateDBWithMigrations(t *testing.T) {
+	ctx := context.Background()
+
+	dbcfg, cleanup := test.CreateDB(t, ctx, &test.CreateDatabaseConfig{
+		Migrations: &database.MigrationConfig{
+			BasePath: path.Join(os.Getenv("PWD"), "../.."),
+		},
+	})
+	assert.NotNil(t, dbcfg)
+	assert.NotNil(t, cleanup)
+
+	cleanup()
+}