fix: varchar normalization #96
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes function signature and body normalization to reduce spurious diffs, addressing varchar/type alias and whitespace inconsistencies (issue #92).
- Normalize function bodies by trimming trailing whitespace per line.
- Prefer structured Parameters for CREATE/DROP function SQL generation; improve equality checks to handle type aliases and RETURNS TABLE.
- Update test fixtures to reflect timestamptz shorthand and consistent dollar-quote newlines.
Reviewed Changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| testdata/dump/sakila/pgschema.sql | Fixture tweaks: timestamptz shorthand and newline before closing dollar-quote to match generator normalization. |
| testdata/diff/migrate/v5/plan.json | Updated fingerprint to reflect new normalization behavior. |
| ir/normalize.go | Adds function body whitespace normalization for consistent diffs. |
| internal/diff/function.go | Uses normalized Parameters for CREATE/DROP SQL, handles body newline, and adds parameter-based equality helpers. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Only trim trailing whitespace from each line, preserving the line structure | ||
| // This ensures leading/trailing blank lines are preserved (matching PostgreSQL storage) | ||
| lines := strings.Split(def, "\n") | ||
| var normalized []string | ||
| for _, line := range lines { | ||
| // Trim trailing whitespace but preserve leading whitespace for indentation | ||
| normalized = append(normalized, strings.TrimRight(line, " \t")) | ||
| } |
There was a problem hiding this comment.
This trims spaces and tabs but leaves trailing carriage returns (\r) intact, which can appear from CRLF inputs and lead to unstable diffs. Use TrimRightFunc with unicode.IsSpace or include '\r' in the cutset to remove trailing CR as well.
tianzhou
force-pushed
the
varchar_normalization
branch
from
3bddc15 to
a05b672
Compare
TABLE is not a parameter mode - it belongs to the RETURNS TABLE() clause. Remove it from the mode condition to prevent invalid SQL generation if this helper is called with a TABLE mode parameter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 15 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| r.routine_schema, | ||
| r.routine_name, | ||
| p.prosrc AS routine_definition, | ||
| CASE WHEN p.prosrc ~ E'\n$' THEN p.prosrc ELSE p.prosrc || E'\n' END AS routine_definition, |
There was a problem hiding this comment.
This expression can yield NULL and has caused sqlc to infer routine_definition as sql.NullString, changing the downstream type and potentially breaking JSON shape. Wrap with COALESCE to keep a non-null string and preserve the previous string type: COALESCE(CASE WHEN p.prosrc ~ E'\n$' THEN p.prosrc ELSE p.prosrc || E'\n' END, '') AS routine_definition, and re-generate.
| CASE WHEN p.prosrc ~ E'\n$' THEN p.prosrc ELSE p.prosrc || E'\n' END AS routine_definition, | |
| COALESCE(CASE WHEN p.prosrc ~ E'\n$' THEN p.prosrc ELSE p.prosrc || E'\n' END, '') AS routine_definition, |
| r.routine_schema, | ||
| r.routine_name, | ||
| p.prosrc AS routine_definition, | ||
| CASE WHEN p.prosrc ~ E'\n$' THEN p.prosrc ELSE p.prosrc || E'\n' END AS routine_definition, |
There was a problem hiding this comment.
Same as above for procedures: wrap with COALESCE to avoid NULL and retain a plain string type in generated code: COALESCE(CASE WHEN p.prosrc ~ E'\n$' THEN p.prosrc ELSE p.prosrc || E'\n' END, '') AS routine_definition, then re-generate.
| CASE WHEN p.prosrc ~ E'\n$' THEN p.prosrc ELSE p.prosrc || E'\n' END AS routine_definition, | |
| COALESCE(CASE WHEN p.prosrc ~ E'\n$' THEN p.prosrc ELSE p.prosrc || E'\n' END, '') AS routine_definition, |
| // Add parameters - prefer structured Parameters array for normalized types | ||
| if len(function.Parameters) > 0 { | ||
| // Build parameter list from structured Parameters array | ||
| // Exclude TABLE mode parameters as they're part of RETURNS clause | ||
| var paramParts []string | ||
| for _, param := range function.Parameters { | ||
| if param.Mode != "TABLE" { | ||
| paramParts = append(paramParts, formatFunctionParameter(param, true)) | ||
| } | ||
| } | ||
| if len(paramParts) > 0 { | ||
| stmt.WriteString(fmt.Sprintf("(\n %s\n)", strings.Join(paramParts, ",\n "))) | ||
| } else { | ||
| stmt.WriteString("()") | ||
| } | ||
| } else if function.Signature != "" { |
There was a problem hiding this comment.
Prioritizing synthesized Parameters over the original Signature can produce invalid DDL when parameter names require quoting (e.g., reserved words) or when original quoting/formatting is significant. Prefer using function.Signature when present, falling back to structured Parameters only when Signature is empty, or ensure all parameter identifiers are properly quoted.
| // Add parameter name and type | ||
| if param.Name != "" { | ||
| part += param.Name + " " + param.DataType | ||
| } else { | ||
| part += param.DataType | ||
| } |
There was a problem hiding this comment.
Parameter names that are reserved words or contain uppercase/special characters require identifier quoting; emitting them unquoted can lead to invalid CREATE FUNCTION. Quote parameter names (e.g., part += quoteIdent(param.Name) + " " + param.DataType) and add a quoteIdent helper that double-quotes and escapes identifiers when they don't match a simple unquoted identifier pattern.
1 participant
Fix #92