fix: make core migration PostgreSQL-safe

2026-07-20 16:18:05 +08:00
parent bb87ae3658
commit 8e72d0dad1
4 changed files with 190 additions and 8 deletions
@@ -1,4 +1,4 @@
-CREATE TABLE session_mappings (
+CREATE TABLE IF NOT EXISTS session_mappings (
    session_key TEXT PRIMARY KEY,
    tenant_key TEXT NOT NULL,
    channel TEXT NOT NULL,
@@ -10,10 +10,10 @@ CREATE TABLE session_mappings (
    updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
 );

-CREATE INDEX idx_session_mappings_tenant_channel
+CREATE INDEX IF NOT EXISTS idx_session_mappings_tenant_channel
    ON session_mappings (tenant_key, channel, external_subject_id);

-CREATE TABLE turns (
+CREATE TABLE IF NOT EXISTS turns (
    id TEXT PRIMARY KEY,
    tenant_key TEXT NOT NULL,
    channel TEXT NOT NULL,
@@ -23,7 +23,7 @@ CREATE TABLE turns (
    weknora_request_id TEXT NOT NULL DEFAULT '',
    question TEXT NOT NULL,
    answer TEXT NOT NULL DEFAULT '',
-    references JSONB NOT NULL DEFAULT '[]'::JSONB,
+    references_json JSONB NOT NULL DEFAULT '[]'::JSONB,
    status TEXT NOT NULL CHECK (status IN ('processing','succeeded','failed','cancelled')),
    error_message TEXT NOT NULL DEFAULT '',
    created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
@@ -31,5 +31,5 @@ CREATE TABLE turns (
    UNIQUE (tenant_key, channel, external_message_id)
 );

-CREATE INDEX idx_turns_tenant_created ON turns (tenant_key, created_at DESC);
-CREATE INDEX idx_turns_session_created ON turns (session_key, created_at DESC);
+CREATE INDEX IF NOT EXISTS idx_turns_tenant_created ON turns (tenant_key, created_at DESC);
+CREATE INDEX IF NOT EXISTS idx_turns_session_created ON turns (session_key, created_at DESC);
@@ -0,0 +1,91 @@
+// Package migrations_test contains source-level contract tests for the SQL
+// migration files. They guard against the PostgreSQL 16 reserved-word
+// failure (`references JSONB`) and guarantee the migration can be safely
+// re-run from a half-applied state (session_mappings + its index created,
+// turns not created). No real database connection is used.
+package migrations_test
+
+import (
+	"os"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"testing"
+)
+
+func readUpMigration(t *testing.T) string {
+	t.Helper()
+	data, err := os.ReadFile(filepath.Join(".", "000001_core.up.sql"))
+	if err != nil {
+		t.Fatalf("read up migration: %v", err)
+	}
+	return string(data)
+}
+
+func readDownMigration(t *testing.T) string {
+	t.Helper()
+	data, err := os.ReadFile(filepath.Join(".", "000001_core.down.sql"))
+	if err != nil {
+		t.Fatalf("read down migration: %v", err)
+	}
+	return string(data)
+}
+
+// TestUpMigrationUsesIfNotExists enforces idempotency: every CREATE TABLE and
+// CREATE INDEX must use IF NOT EXISTS so a half-applied migration (e.g.
+// session_mappings already created, turns missing) can be re-run cleanly.
+func TestUpMigrationUsesIfNotExists(t *testing.T) {
+	up := readUpMigration(t)
+
+	for _, stmt := range splitStatements(up) {
+		upper := strings.ToUpper(stmt)
+		if strings.Contains(upper, "CREATE TABLE") && !strings.Contains(upper, "CREATE TABLE IF NOT EXISTS") {
+			t.Errorf("CREATE TABLE without IF NOT EXISTS:\n%s", strings.TrimSpace(stmt))
+		}
+		if strings.Contains(upper, "CREATE INDEX") && !strings.Contains(upper, "CREATE INDEX IF NOT EXISTS") {
+			t.Errorf("CREATE INDEX without IF NOT EXISTS:\n%s", strings.TrimSpace(stmt))
+		}
+	}
+
+	if got := countMatches(up, `(?i)CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS`); got != 2 {
+		t.Errorf("expected 2 CREATE TABLE IF NOT EXISTS statements, got %d", got)
+	}
+	if got := countMatches(up, `(?i)CREATE\s+INDEX\s+IF\s+NOT\s+EXISTS`); got != 3 {
+		t.Errorf("expected 3 CREATE INDEX IF NOT EXISTS statements, got %d", got)
+	}
+}
+
+// TestUpMigrationAvoidsReservedWordReferences ensures the turns table does
+// not declare a bare `references JSONB` column. REFERENCES is a reserved word
+// in PostgreSQL 16 and fails with "syntax error at or near references"
+// unless quoted. The column must be named references_json.
+func TestUpMigrationAvoidsReservedWordReferences(t *testing.T) {
+	up := readUpMigration(t)
+
+	if regexp.MustCompile(`(?im)^\s*references\s+JSONB`).MatchString(up) {
+		t.Error("up migration declares reserved-word column `references JSONB`; rename it to references_json")
+	}
+	if !regexp.MustCompile(`(?im)^\s*references_json\s+JSONB\s+NOT\s+NULL\s+DEFAULT\s+'\[\]'::JSONB`).MatchString(up) {
+		t.Error("up migration must declare column `references_json JSONB NOT NULL DEFAULT '[]'::JSONB`")
+	}
+}
+
+// TestDownMigrationDropsBothTables guards the rollback path stays droppable.
+func TestDownMigrationDropsBothTables(t *testing.T) {
+	down := readDownMigration(t)
+	upper := strings.ToUpper(down)
+	if !strings.Contains(upper, "DROP TABLE IF EXISTS TURNS") {
+		t.Error("down migration must drop turns")
+	}
+	if !strings.Contains(upper, "DROP TABLE IF EXISTS SESSION_MAPPINGS") {
+		t.Error("down migration must drop session_mappings")
+	}
+}
+
+func splitStatements(sql string) []string {
+	return strings.Split(sql, ";")
+}
+
+func countMatches(haystack, pattern string) int {
+	return len(regexp.MustCompile(pattern).FindAllString(haystack, -1))
+}
@@ -84,14 +84,14 @@ func (s *Store) FinishTurn(ctx context.Context, turn core.Turn) error {
 	if err != nil {
 		return err
 	}
-	_, err = s.pool.Exec(ctx, `UPDATE turns SET weknora_session_id=$2,weknora_request_id=$3,answer=$4,references=$5,status=$6,error_message=$7,updated_at=NOW() WHERE id=$1`, turn.ID, turn.WeKnoraSessionID, turn.WeKnoraRequestID, turn.Answer, references, turn.Status, turn.ErrorMessage)
+	_, err = s.pool.Exec(ctx, `UPDATE turns SET weknora_session_id=$2,weknora_request_id=$3,answer=$4,references_json=$5,status=$6,error_message=$7,updated_at=NOW() WHERE id=$1`, turn.ID, turn.WeKnoraSessionID, turn.WeKnoraRequestID, turn.Answer, references, turn.Status, turn.ErrorMessage)
 	return err
 }

 func (s *Store) loadTurn(ctx context.Context, tenantKey, channel, messageID string) (core.Turn, error) {
 	var turn core.Turn
 	var references []byte
-	err := s.pool.QueryRow(ctx, `SELECT id,tenant_key,channel,external_message_id,session_key,weknora_session_id,weknora_request_id,question,answer,references,status,error_message FROM turns WHERE tenant_key=$1 AND channel=$2 AND external_message_id=$3`, tenantKey, channel, messageID).Scan(&turn.ID, &turn.TenantKey, &turn.Channel, &turn.ExternalMessageID, &turn.SessionKey, &turn.WeKnoraSessionID, &turn.WeKnoraRequestID, &turn.Question, &turn.Answer, &references, &turn.Status, &turn.ErrorMessage)
+	err := s.pool.QueryRow(ctx, `SELECT id,tenant_key,channel,external_message_id,session_key,weknora_session_id,weknora_request_id,question,answer,references_json,status,error_message FROM turns WHERE tenant_key=$1 AND channel=$2 AND external_message_id=$3`, tenantKey, channel, messageID).Scan(&turn.ID, &turn.TenantKey, &turn.Channel, &turn.ExternalMessageID, &turn.SessionKey, &turn.WeKnoraSessionID, &turn.WeKnoraRequestID, &turn.Question, &turn.Answer, &references, &turn.Status, &turn.ErrorMessage)
 	if err != nil {
 		return core.Turn{}, err
 	}
@@ -0,0 +1,91 @@
+// Source-level contract tests: the store's SQL must use the renamed
+// references_json column (the bare `references` identifier is a reserved
+// word in PostgreSQL 16), while the Go domain field stays turn.References.
+// No database connection is required.
+package postgres
+
+import (
+	"os"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"testing"
+)
+
+func readStoreSource(t *testing.T) string {
+	t.Helper()
+	data, err := os.ReadFile(filepath.Join(".", "store.go"))
+	if err != nil {
+		t.Fatalf("read store.go: %v", err)
+	}
+	return string(data)
+}
+
+func extractSQLLiterals(src string) []string {
+	var sqls []string
+	re := regexp.MustCompile("`([^`]*)`")
+	for _, m := range re.FindAllStringSubmatch(src, -1) {
+		upper := strings.ToUpper(m[1])
+		if strings.Contains(upper, "SELECT") || strings.Contains(upper, "UPDATE") ||
+			strings.Contains(upper, "INSERT") || strings.Contains(upper, "DELETE") {
+			sqls = append(sqls, m[1])
+		}
+	}
+	return sqls
+}
+
+func TestStoreSQLUsesReferencesJSONColumn(t *testing.T) {
+	src := readStoreSource(t)
+	sqls := extractSQLLiterals(src)
+	if len(sqls) == 0 {
+		t.Fatal("no SQL literals found in store.go")
+	}
+
+	var update, selectTurns string
+	for _, sql := range sqls {
+		upper := strings.ToUpper(sql)
+		if strings.HasPrefix(strings.TrimSpace(upper), "UPDATE TURNS") {
+			update = sql
+		}
+		if strings.Contains(upper, "SELECT") && strings.Contains(upper, "FROM TURNS") {
+			selectTurns = sql
+		}
+	}
+	if update == "" {
+		t.Fatal("FinishTurn UPDATE turns statement not found")
+	}
+	if selectTurns == "" {
+		t.Fatal("loadTurn SELECT FROM turns statement not found")
+	}
+	if !strings.Contains(update, "references_json=$5") {
+		t.Error("FinishTurn UPDATE must write to references_json column")
+	}
+	if !strings.Contains(selectTurns, "answer,references_json,status") {
+		t.Error("loadTurn SELECT must read references_json column")
+	}
+}
+
+func TestStoreSQLAvoidsBareReferencesIdentifier(t *testing.T) {
+	src := readStoreSource(t)
+
+	// Inside the actual SQL literals, the bare word `references` is a
+	// reserved word in PostgreSQL 16 and must not appear as a column
+	// identifier. The Go variable/field names are irrelevant here because
+	// we only inspect SQL string literals.
+	bare := regexp.MustCompile(`(?i)\breferences\b`)
+	for _, sql := range extractSQLLiterals(src) {
+		// references_json is fine; strip it before checking for the bare word.
+		scrubbed := regexp.MustCompile(`(?i)\breferences_json\b`).ReplaceAllString(sql, "")
+		if bare.MatchString(scrubbed) {
+			t.Errorf("SQL literal uses reserved word `references` as a column name; use references_json:\n%s", sql)
+		}
+	}
+}
+
+func TestStoreKeepsDomainFieldReferences(t *testing.T) {
+	src := readStoreSource(t)
+
+	if !regexp.MustCompile(`turn\.References`).MatchString(src) {
+		t.Error("store must keep using the domain field turn.References (API unchanged)")
+	}
+}