-
Notifications
You must be signed in to change notification settings - Fork 40
fix: show error location with SQL context when plan fails #394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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. | ||||||||||
|
|
@@ -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)) | ||||||||||
|
|
||||||||||
| 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()) | ||||||||||
|
||||||||||
| 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")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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")) |
| 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) { | ||||||
|
|
@@ -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 | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| pgErr := &pgconn.PgError{ | ||||||
tianzhou marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| 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()) | ||||||
| } | ||||||
tianzhou marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| }) | ||||||
|
|
||||||
| 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()) | ||||||
| } | ||||||
tianzhou marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| }) | ||||||
| } | ||||||
There was a problem hiding this comment.
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\rcharacters intact when the input SQL uses CRLF line endings. Those\rcharacters 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\nwith\n) or trimming a trailing\rfrom each displayed line before writing the snippet.There was a problem hiding this comment.
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.