Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions internal/postgres/desired_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@ import (
"context"
"crypto/rand"
"encoding/hex"
"errors"
"fmt"
"regexp"
"strings"
"sync"
"time"

"github.com/jackc/pgx/v5/pgconn"
)

// DesiredStateProvider is an interface that abstracts the desired state database provider.
Expand Down Expand Up @@ -440,3 +443,46 @@ func replaceSchemaInDefaultPrivileges(sql string, targetSchema, tempSchema strin

return result
}

// enhanceApplyError extracts the surrounding SQL context from a PostgreSQL error's
// position field to help the user locate the problematic statement in their schema files.
func enhanceApplyError(err error, sql string) error {
var pgErr *pgconn.PgError
if !errors.As(err, &pgErr) || pgErr.Position == 0 {
return err
}

// PostgreSQL Position is 1-based character (not byte) offset
runes := []rune(sql)
pos := int(pgErr.Position) - 1
if pos < 0 || pos >= len(runes) {
return err
}

line := 1
lineStart := 0
for i := 0; i < pos; i++ {
if runes[i] == '\n' {
line++
lineStart = i + 1
}
}
col := pos - lineStart + 1

const contextLines = 3
lines := strings.Split(sql, "\n")

startLine := max(line-contextLines, 1)
endLine := min(line+contextLines, len(lines))
Comment on lines +472 to +476
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strings.Split(sql, "\n") will leave \r characters intact when the input SQL uses CRLF line endings. Those \r characters will be embedded into the rendered context snippet and can cause messy/overwritten terminal output on Windows or when schema files are checked out with CRLF. Consider normalizing line endings (e.g., replace \r\n with \n) or trimming a trailing \r from each displayed line before writing the snippet.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SQL here comes from PostgreSQL's wire protocol or pgschema's own dump output — neither produces CRLF. Even if a user's schema file has CRLF line endings, Go reads and sends it as-is, and the Position from PostgreSQL will be consistent with whatever was sent. Adding CRLF normalization for a diagnostic error path that won't encounter it in practice isn't worth the complexity.


var snippet strings.Builder
for i := startLine; i <= endLine; i++ {
prefix := " "
if i == line {
prefix = "> "
}
snippet.WriteString(fmt.Sprintf("%s%5d | %s\n", prefix, i, lines[i-1]))
}

return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, snippet.String())
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The snippet builder always appends a trailing newline (and snippet.String() is then appended verbatim). This makes the final error string end with an extra newline, which can produce an extra blank line in CLI output and makes error strings harder to compare in tests/logging. Consider avoiding the final \n (e.g., only add newlines between lines, or strings.TrimRight(snippet.String(), "\n") before formatting).

Suggested change
return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, snippet.String())
return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, strings.TrimRight(snippet.String(), "\n"))

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trailing newline is intentional — it ensures the snippet block ends cleanly before the shell prompt or any subsequent output. This is CLI diagnostic output, not a string used in test comparisons.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Trailing newline in error message

snippet.String() already ends with \n (every line in the loop appends one), so the returned error message ends with a bare newline. This can produce double-blank-lines or extra whitespace when callers wrap the error with fmt.Errorf("...: %w", enhancedErr). A simple strings.TrimRight(snippet.String(), "\n") keeps the snippet clean.

Suggested change
return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, snippet.String())
return fmt.Errorf("%w\n\nError location (line %d, column %d):\n%s", err, line, col, strings.TrimRight(snippet.String(), "\n"))

}
71 changes: 71 additions & 0 deletions internal/postgres/desired_state_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package postgres

import (
"fmt"
"reflect"
"strings"
"testing"

"github.com/jackc/pgx/v5/pgconn"
)

func TestSplitDollarQuotedSegments(t *testing.T) {
Expand Down Expand Up @@ -291,3 +295,70 @@ func TestStripSchemaQualifications_PreservesStringLiterals(t *testing.T) {
})
}
}

func TestEnhanceApplyError(t *testing.T) {
sql := "CREATE TABLE foo (id int);\nCREATE TABLE bar (\n name text\n);\nSELECT 1;\nCREATE TABLE baz (id int);"

t.Run("pgError with position", func(t *testing.T) {
// Position points to "SELECT" on line 5
pos := int32(strings.Index(sql, "SELECT 1") + 1) // 1-based
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Byte offset used as character position

strings.Index returns a byte offset, but pgconn.PgError.Position is a character (rune) offset — the two happen to be equal here only because the test SQL is pure ASCII. Consider using utf8.RuneCountInString(sql[:strings.Index(sql, "SELECT 1")]) + 1 to make the intent explicit and future-proof against non-ASCII test data.

Suggested change
pos := int32(strings.Index(sql, "SELECT 1") + 1) // 1-based
pos := int32(utf8.RuneCountInString(sql[:strings.Index(sql, "SELECT 1")]) + 1) // 1-based character offset

pgErr := &pgconn.PgError{
Message: "syntax error at or near \"SELECT\"",
Code: "42601",
Position: pos,
}
enhanced := enhanceApplyError(pgErr, sql)
errMsg := enhanced.Error()

if !strings.Contains(errMsg, "line 5") {
t.Errorf("expected error to mention line 5, got: %s", errMsg)
}
if !strings.Contains(errMsg, "SELECT 1") {
t.Errorf("expected error to contain the offending line, got: %s", errMsg)
}
// Should still contain original error
if !strings.Contains(errMsg, "syntax error") {
t.Errorf("expected error to contain original message, got: %s", errMsg)
}
})

t.Run("multi-byte UTF-8 position", func(t *testing.T) {
// PostgreSQL Position counts characters, not bytes.
// "café" is 4 characters but 5 bytes (é is 2 bytes in UTF-8).
mbSQL := "-- café\nSELECT 1;"
// "SELECT" starts at character position 9 (1-based): "-- café\n" = 8 chars
pgErr := &pgconn.PgError{
Message: "syntax error",
Code: "42601",
Position: 9,
}
enhanced := enhanceApplyError(pgErr, mbSQL)
errMsg := enhanced.Error()

if !strings.Contains(errMsg, "line 2, column 1") {
t.Errorf("expected line 2, column 1 for multi-byte SQL, got: %s", errMsg)
}
if !strings.Contains(errMsg, "SELECT 1") {
t.Errorf("expected snippet to contain the error line, got: %s", errMsg)
}
})

t.Run("non-pg error passes through", func(t *testing.T) {
origErr := fmt.Errorf("some other error")
result := enhanceApplyError(origErr, sql)
if result != origErr {
t.Errorf("expected same error instance, got: %s", result.Error())
}
})

t.Run("pgError without position passes through", func(t *testing.T) {
pgErr := &pgconn.PgError{
Message: "some error",
Code: "42601",
}
result := enhanceApplyError(pgErr, sql)
if result != pgErr {
t.Errorf("expected same error instance, got: %s", result.Error())
}
})
}
2 changes: 1 addition & 1 deletion internal/postgres/embedded.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func (ep *EmbeddedPostgres) ApplySchema(ctx context.Context, schema string, sql
// Note: Desired state SQL should never contain operations like CREATE INDEX CONCURRENTLY
// that cannot run in transactions. Those are migration details, not state declarations.
if _, err := util.ExecContextWithLogging(ctx, conn, schemaAgnosticSQL, "apply desired state SQL to temporary schema"); err != nil {
return fmt.Errorf("failed to apply schema SQL to temporary schema %s: %w", ep.tempSchema, err)
return fmt.Errorf("failed to apply schema SQL to temporary schema %s: %w", ep.tempSchema, enhanceApplyError(err, schemaAgnosticSQL))
}

return nil
Expand Down
2 changes: 1 addition & 1 deletion internal/postgres/external.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func (ed *ExternalDatabase) ApplySchema(ctx context.Context, schema string, sql
// Note: Desired state SQL should never contain operations like CREATE INDEX CONCURRENTLY
// that cannot run in transactions. Those are migration details, not state declarations.
if _, err := util.ExecContextWithLogging(ctx, conn, schemaAgnosticSQL, "apply desired state SQL to temporary schema"); err != nil {
return fmt.Errorf("failed to apply schema SQL to temporary schema %s: %w", ed.tempSchema, err)
return fmt.Errorf("failed to apply schema SQL to temporary schema %s: %w", ed.tempSchema, enhanceApplyError(err, schemaAgnosticSQL))
}

return nil
Expand Down