Test/postgresql #73

noe.trevino · 2026-02-14T22:56:01-06:00

noe.trevino commented

2026-02-14 22:56:01 -06:00

No description provided.

github-actions[bot] commented

2026-02-14 22:57:40 -06:00

(Migrated from github.com)

PR Summary (auto-generated)

Added PostgreSQL support to CI/CD pipeline by configuring a postgres:17 service container in GitHub Actions with health checks and environment variables
Refactored all service layer tests (assignee, column, label, project, task, taskevent) to run against both SQLite and PostgreSQL databases using parameterized test helpers
Enhanced test fixture infrastructure to create isolated PostgreSQL databases with random names (paso_test_) enabling parallel test execution without conflicts
Eliminated duplicate DatabaseType enum by consolidating to internal/database.DatabaseType and removing fixtures.DatabaseType
Modified RunDatabaseTests and RunDatabaseTestsWithSetup helpers to accept and pass database.DatabaseType parameter to test functions for database-specific logic (e.g., connection pooling for SQLite)
Updated PostgreSQL test database setup to use goose migrations for schema creation and added proper cleanup with automatic database dropping via t.Cleanup()

**PR Summary** _(auto-generated)_ - Added PostgreSQL support to CI/CD pipeline by configuring a postgres:17 service container in GitHub Actions with health checks and environment variables - Refactored all service layer tests (assignee, column, label, project, task, taskevent) to run against both SQLite and PostgreSQL databases using parameterized test helpers - Enhanced test fixture infrastructure to create isolated PostgreSQL databases with random names (paso_test_<random>) enabling parallel test execution without conflicts - Eliminated duplicate DatabaseType enum by consolidating to internal/database.DatabaseType and removing fixtures.DatabaseType - Modified RunDatabaseTests and RunDatabaseTestsWithSetup helpers to accept and pass database.DatabaseType parameter to test functions for database-specific logic (e.g., connection pooling for SQLite) - Updated PostgreSQL test database setup to use goose migrations for schema creation and added proper cleanup with automatic database dropping via t.Cleanup()

claude[bot] (Migrated from github.com) reviewed

2026-02-14 22:59:06 -06:00

internal/testing/fixtures/db_postgres.go

					
				@ -126,1 +134,3 @@

				func cleanupPostgresTestData(db *sql.DB) {

				// dropTestDatabase connects to the admin database and drops the specified test database.

				func dropTestDatabase(config PostgresTestConfig, dbName string) {

					adminDB, err := sql.Open("postgres", config.ConnectionString())

SQL Injection Risk: Database identifiers should be properly quoted. Use pq.QuoteIdentifier(testDBName) instead of direct string interpolation.

	if _, err := adminDB.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE %s", pq.QuoteIdentifier(testDBName))); err != nil {

While testDBName is internally generated, this establishes an unsafe pattern that could be copied elsewhere.

**SQL Injection Risk**: Database identifiers should be properly quoted. Use `pq.QuoteIdentifier(testDBName)` instead of direct string interpolation. ```suggestion if _, err := adminDB.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE %s", pq.QuoteIdentifier(testDBName))); err != nil { ``` While `testDBName` is internally generated, this establishes an unsafe pattern that could be copied elsewhere.

claude[bot] (Migrated from github.com) reviewed

2026-02-14 22:59:08 -06:00

internal/testing/fixtures/db_postgres.go

CRITICAL - SQL Injection Vulnerability: Direct string formatting into SQL with unescaped single quotes. If dbName ever comes from untrusted input, this is exploitable.

Use a parameterized query instead:

	terminateQuery := "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = $1 AND pid <> pg_backend_pid()"
	_, _ = adminDB.ExecContext(ctx, terminateQuery, dbName)

**CRITICAL - SQL Injection Vulnerability**: Direct string formatting into SQL with unescaped single quotes. If `dbName` ever comes from untrusted input, this is exploitable. Use a parameterized query instead: ```suggestion terminateQuery := "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = $1 AND pid <> pg_backend_pid()" _, _ = adminDB.ExecContext(ctx, terminateQuery, dbName) ```

claude[bot] (Migrated from github.com) reviewed

2026-02-14 22:59:10 -06:00

internal/testing/fixtures/db_postgres.go

SQL Injection Risk: Same issue as line 95. Database identifiers must be properly quoted.

	_, _ = adminDB.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS %s", pq.QuoteIdentifier(dbName)))

**SQL Injection Risk**: Same issue as line 95. Database identifiers must be properly quoted. ```suggestion _, _ = adminDB.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS %s", pq.QuoteIdentifier(dbName))) ```

claude[bot] (Migrated from github.com) reviewed

2026-02-14 22:59:14 -06:00

internal/testing/fixtures/db_postgres.go

					
				@ -122,1 +124,3 @@

					_, _ = db.ExecContext(ctx, drops)

				// uniqueTestDBName generates a unique database name like "paso_test_a1b2c3d4".

				func uniqueTestDBName() string {

					b := make([]byte, 4)

Guard Pattern Flattening: The manual adminDB.Close() on line 84 is redundant. Use early defer instead:

adminDB, err := sql.Open("postgres", config.ConnectionString())
if err != nil {
	tb.Skipf("PostgreSQL not available: %v", err)
	return nil
}
defer adminDB.Close()

ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

if err := adminDB.PingContext(ctx); err != nil {
	tb.Skipf("PostgreSQL connection failed: %v. Ensure PostgreSQL is running at %s:%s with database=%s",
		err, config.Host, config.Port, config.Database)
	return nil
}

**Guard Pattern Flattening**: The manual `adminDB.Close()` on line 84 is redundant. Use early defer instead: ```go adminDB, err := sql.Open("postgres", config.ConnectionString()) if err != nil { tb.Skipf("PostgreSQL not available: %v", err) return nil } defer adminDB.Close() ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() if err := adminDB.PingContext(ctx); err != nil { tb.Skipf("PostgreSQL connection failed: %v. Ensure PostgreSQL is running at %s:%s with database=%s", err, config.Host, config.Port, config.Database) return nil } ```

claude[bot] commented

2026-02-14 22:59:53 -06:00

(Migrated from github.com)

Code Review Summary

Critical Security Issues

SQL Injection Vulnerabilities - 3 locations in db_postgres.go:

Line 95: CREATE DATABASE with unescaped identifier
Lines 146-149: String formatting in pg_terminate_backend query - CRITICAL
Line 152: DROP DATABASE with unescaped identifier

All PostgreSQL identifiers must use pq.QuoteIdentifier() and query parameters must use $1 placeholders instead of string formatting.

Code Quality Issues

DRY Violations:

testEnv struct and setupTestEnv function duplicated across column, label, project, and task service test files
Table-driven validation test structures repeated in every service test file
RunDatabaseTests wrapper pattern repeated 80+ times
Consider creating generic test helpers in internal/testing/fixtures using Go generics

Guard Pattern Flattening (db_postgres.go:74-89):

Manual adminDB.Close() on line 84 should use early defer instead

Error Handling:

Line 150: Connection termination errors silently ignored - should at least log
Multiple Close() errors ignored without logging throughout cleanup paths

Minor Issues:

Lines 128-131: Timestamp fallback in uniqueTestDBName() could collide in parallel tests - add PID or additional entropy

Priority

Issue	Severity	Priority
SQL Injection	Critical	P0 - Block merge
DRY Violations	Medium	P1 - Refactor soon
Error Handling	Medium	P1 - Improve
Guard Patterns	Low	P2 - Nice to have

See inline comments for specific code suggestions.

## Code Review Summary ### Critical Security Issues **SQL Injection Vulnerabilities** - 3 locations in `db_postgres.go`: 1. Line 95: `CREATE DATABASE` with unescaped identifier 2. Lines 146-149: String formatting in `pg_terminate_backend` query - **CRITICAL** 3. Line 152: `DROP DATABASE` with unescaped identifier All PostgreSQL identifiers must use `pq.QuoteIdentifier()` and query parameters must use `$1` placeholders instead of string formatting. ### Code Quality Issues **DRY Violations**: - `testEnv` struct and `setupTestEnv` function duplicated across column, label, project, and task service test files - Table-driven validation test structures repeated in every service test file - `RunDatabaseTests` wrapper pattern repeated 80+ times - Consider creating generic test helpers in `internal/testing/fixtures` using Go generics **Guard Pattern Flattening** (db_postgres.go:74-89): - Manual `adminDB.Close()` on line 84 should use early defer instead **Error Handling**: - Line 150: Connection termination errors silently ignored - should at least log - Multiple `Close()` errors ignored without logging throughout cleanup paths **Minor Issues**: - Lines 128-131: Timestamp fallback in `uniqueTestDBName()` could collide in parallel tests - add PID or additional entropy ### Priority | Issue | Severity | Priority | |-------|----------|----------| | SQL Injection | Critical | **P0 - Block merge** | | DRY Violations | Medium | P1 - Refactor soon | | Error Handling | Medium | P1 - Improve | | Guard Patterns | Low | P2 - Nice to have | See inline comments for specific code suggestions.

Sign in to join this conversation.

No reviewers