fix: match quoted identifiers in function dependency detection - #572
Conversation
functionCallRegex only matched unquoted identifiers, so a function created with a double-quoted, case-sensitive name (e.g. "MyFunc") was not recognized as a dependency of tables, policies, or domains that referenced it in defaults, generated columns, CHECK constraints, or expression indexes. This could place CREATE TABLE/ALTER TABLE before the function it depends on, failing at apply time. Extend functionCallRegex to also match double-quoted identifier segments, and normalize matched identifiers (stripping quotes, lowercasing each dot-separated segment) before looking them up. Fixes #571 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
Greptile SummaryThis PR expands function-reference detection to recognize quoted and mixed schema-qualified identifiers and adds a dependency-order regression fixture. The implementation does not yet cover escaped quotes and is not applied consistently to function-body dependency ordering.
Confidence Score: 3/5The PR should not merge until quoted identifiers with escaped quotes and quoted function-to-function dependencies are ordered correctly. Valid PostgreSQL quoted names containing escaped quotes remain undetectable, while quoted calls in function definitions are captured but compared against lookup keys without normalization; both paths can emit dependent DDL before its function. Files Needing Attention: internal/diff/diff.go and internal/diff/topological.go Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
E[Inspected SQL expression] --> R[functionCallRegex]
R --> N[Normalize identifier]
N --> L[New-function lookup]
L --> O[Dependency ordering]
O --> P[Executable migration plan]
Reviews (1): Last reviewed commit: "fix: match quoted identifiers in functio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes dependency-ordering for functions referenced via double-quoted identifiers by expanding function-call detection to recognize quoted segments and normalizing captured identifiers before lookup, plus adds a regression fixture for the reported case (Issue #571).
Changes:
- Expanded
functionCallRegexto match schema-qualified identifiers that may include double-quoted segments. - Added
normalizeFunctionIdentifier()and updatedreferencesNewFunction()to normalize matches before checking newly-added function lookups. - Added
testdata/diff/dependency/issue_571_quoted_function_dependency/regression fixture covering a quoted function used in a column default.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/diff/diff.go | Extends function-call matching and normalizes identifiers to detect quoted function dependencies for ordering. |
| testdata/diff/dependency/issue_571_quoted_function_dependency/new.sql | Defines a quoted function and a table referencing it via a default expression. |
| testdata/diff/dependency/issue_571_quoted_function_dependency/old.sql | Establishes an empty starting schema for the regression. |
| testdata/diff/dependency/issue_571_quoted_function_dependency/diff.sql | Expected DDL ordering output (function before table). |
| testdata/diff/dependency/issue_571_quoted_function_dependency/plan.sql | Expected plan SQL showing correct execution order. |
| testdata/diff/dependency/issue_571_quoted_function_dependency/plan.txt | Expected human-readable plan output validating ordering. |
| testdata/diff/dependency/issue_571_quoted_function_dependency/plan.json | Expected plan JSON output validating ordering and step paths. |
Suppressed comments (1)
internal/diff/diff.go:2637
- normalizeFunctionIdentifier removes surrounding quotes but doesn’t unescape doubled double-quotes (
""->"). Without unescaping, lookup keys won’t match for valid quoted identifiers containing embedded quotes.
if len(atom) >= 2 && atom[0] == '"' && atom[len(atom)-1] == '"' {
atom = atom[1 : len(atom)-1]
}
atoms[i] = strings.ToLower(atom)
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…escaped quotes Addresses Greptile review feedback on #572: - buildFunctionBodyDependencies (topological.go) reused the shared functionCallRegex but still lowercased the raw match instead of calling normalizeFunctionIdentifier, so a quoted function call inside another function's body (e.g. SELECT "Helper"(x)) was captured but never matched the unquoted lookup key, silently ordering the caller before its dependency. - functionCallRegex's quoted-identifier atom didn't account for embedded double quotes escaped as "" (valid Postgres identifier syntax), so a function name containing a literal quote wasn't parsed correctly. Both are covered by new tests: a fixture reproducing the quoted function-to-function body reference, and direct unit tests for normalizeFunctionIdentifier/referencesNewFunction covering escaped quotes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
|
Thanks for the review — both findings are valid and addressed in 6b640fb:
One thing I deliberately left out of scope: testing an escaped-quote function name through the full Generated by Claude Code |
Addresses Copilot review feedback on #572: lowercasing every matched identifier segment (quoted or not) made dependency detection effectively case-insensitive, even though PostgreSQL only folds *unquoted* identifiers to lowercase — a quoted "MyFunc"() and an unquoted myfunc() are distinct functions and must not be conflated. - Export ir.NeedsQuoting (was unexported needsQuoting) so the diff package can reuse the same quoting rule used for DDL emission. - Add functionLookupKeyPart: case-folds a schema/function name segment only when it doesn't require quoting, otherwise keeps it as-is. - buildFunctionLookup and the function-body dependency scan in topological.go now build/query keys with functionLookupKeyPart instead of a blanket strings.ToLower. - normalizeFunctionIdentifier now preserves case (after unescaping "") for quoted segments and only lowercases bare ones, matching the new lookup keys. Extended function_identifier_test.go with cases proving a quoted and an unquoted reference of the same letters no longer match each other. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
Merge issue_571_quoted_function_body_reference into issue_571_quoted_function_dependency so a single fixture covers both quoted-identifier dependency paths: a column default referencing a quoted function, and a function body calling another quoted function. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EZ62kuoP1LFrETHiNeM97j
There was a problem hiding this comment.
🟡 Changes recommended
Dotted quoted identifiers can collide in lookup keys, and embedded quotes remain invalid when emitted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
…dentifier Addresses Copilot review feedback on #572 (round 2): - normalizeFunctionIdentifier and buildFunctionLookup joined a schema-qualified function reference's normalized parts with a plain ".", so a quoted schema/name pair could flatten to the same string as a different split (e.g. schema "a.b" function "c" and schema "a" function "b.c" both became "a.b.c"), letting buildFunctionBodyDependencies attach a call to the wrong function. Introduced functionGraphKey(schema, name), which joins case-folded parts with a NUL byte (illegal in real Postgres identifiers) instead of ".", mirroring the existing typeGraphKey pattern in topological.go. normalizeFunctionIdentifier now splits on the last unquoted dot (findLastUnquotedDot/unquoteIdent, also reused from topological.go) instead of flattening every atom into one string. - ir.QuoteIdentifier wrapped an identifier in quotes without escaping embedded double quotes, so a catalog name containing a literal quote (e.g. the one added in the escaped-quote test) was re-emitted as invalid SQL. It now doubles embedded quotes per quote_ident's own rules. Extended function_identifier_test.go with cases proving the schema/name split no longer collides. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
|
The This happened in I re-ran the failed job ( Generated by Claude Code |
There was a problem hiding this comment.
🟡 Changes recommended
Normalization incorrectly preserves uppercase unquoted calls instead of applying PostgreSQL’s lowercase folding.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
normalizeFunctionIdentifier used ir.NeedsQuoting on already-unquoted text to decide whether to case-fold a segment, but that infers quote-ness from content instead of the actual source. An unquoted call like MYFUNC() in a function body was left as "MYFUNC" instead of folding to "myfunc" like PostgreSQL does, so it failed to match the lowercase function it was calling. Read the quote status directly off the raw captured segment before stripping it, and always fold unquoted segments regardless of spelling. Also added a missing quote_test.go case for embedded-quote escaping in QuoteIdentifier, per review feedback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
There was a problem hiding this comment.
🟡 Changes recommended
Qualified calls containing valid whitespace around the dot can lose their schema and resolve to the wrong dependency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
PostgreSQL permits whitespace around the `.` separator in a qualified name (e.g. `other . helper()`), but functionCallRegex required an exact `.` with no surrounding whitespace. That qualified form failed to match as one identifier, so the regex fell back to matching just the final segment unqualified - in a function body with two same-named functions in different schemas, this could attribute the dependency to the wrong one. Allow `\s*\.\s*` between segments and trim each split segment in normalizeIdentSegment before checking for surrounding quotes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
There was a problem hiding this comment.
🟡 Changes recommended
Index dependencies remain unhandled, and comments inside qualified calls can cause incorrect dependency resolution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
tableReferencesNewFunction only scanned column defaults, generated columns, and CHECK constraints. But generateCreateTablesSQL emits a table's indexes immediately alongside it, so a table whose only tie to a new function was an expression/functional index column or a partial index's WHERE predicate stayed in the "no dependencies" bucket and its CREATE INDEX ran before the function existed. Scan index.Where and, for IsExpression indexes, each index column's Name (which holds the expression text, not a bare column name) for new function references. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
Resolve conflict in tableReferencesNewFunction: main's #570 added the same expression/partial index dependency scan, so keep main's version. Consolidate the issue_571 fixtures into one: fold the index cases into issue_571_quoted_function_dependency (now that #570 strips the planning schema qualifier from expression index columns, the expression index case is stable too) and drop issue_571_index_function_dependency. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
There was a problem hiding this comment.
🟡 Changes recommended
Partition-key and exclusion-constraint function dependencies can still be emitted before their newly added functions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
…onstraints tableReferencesNewFunction still missed two create-time expressions that generateCreateTableSQL emits inline: the partition key (PARTITION BY ... (fn(col))) and EXCLUDE constraint definitions (whose index elements and WHERE predicate may call functions). A new table whose only tie to a new function was one of these stayed in tablesWithoutDeps and was created before the function. Scan table.PartitionKey and each exclusion constraint's ExclusionDefinition. Unit tests cover both; the consolidated issue_571 fixture now also carries an EXCLUDE constraint on "MyFunc"(code). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
There was a problem hiding this comment.
🟡 Changes recommended
Unqualified same-named functions across schemas can still resolve to the wrong dependency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
…ema first buildFunctionBodyDependencies keyed every function under its bare name in one shared slot (last writer wins) and consulted that slot before the caller-schema key. With same-named functions in two schemas, an unqualified call like "ZHelper"() inside other."AWrapper" could attach to public."ZHelper" instead of other."ZHelper", leaving the real callee ordered after its caller. Try functionGraphKey(caller.Schema, name) first for unqualified calls and fall back to the bare name only if that misses. Qualified calls are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E8WPjyrEffMgi1zqAaSmsF
There was a problem hiding this comment.
🟡 Changes recommended
Function graph keys remain ambiguous for quoted dotted identifiers and can omit a function from generated DDL.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The quoted-identifier behavior and affected dependency paths are implemented consistently with focused regression coverage.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…modify phases Address the second review round: - Aggregates whose support function is typed on a view being recreated (#480 path) now wait for the recreation in the modify phase, alongside those functions, instead of being created before the function exists. - Views deferred to the modify phase (#414 path) that call a deferred aggregate or function are now created after it. The create-phase and both modify-phase emission points share generateViewsAndDependentRoutinesSQL. - tableRefPattern accepts quoted identifiers, so a SQL body reading from "My View" is detected as a dependency; matches are normalized with the same unquoting helpers the function-call scanner uses (#572). - Aggregate identity args and signature strip the quote_ident form of the schema name as well as the raw form, so a schema that needs quoting round-trips without churn. Covered by a new inspector test. Scenarios are folded into the existing issue_414 and issue_480 fixtures and the issue_580 case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Summary
functionCallRegexininternal/diff/diff.goonly matched unquoted identifiers ([a-z_][a-z0-9_$]*), so a function created with a double-quoted, case-sensitive name (e.g.CREATE FUNCTION "MyFunc"(...)) was not recognized byreferencesNewFunction()when referenced in column defaults, generated columns, CHECK constraints, expression index columns, or partial index WHERE clauses.CREATE INDEX/ALTER TABLE/CREATE TABLEfail at apply time.functionCallRegexto also match double-quoted identifier segments (optionally schema-qualified, mixing quoted and unquoted parts, with""escapes), and addednormalizeFunctionIdentifier()to unescape quoted segments and case-fold only unquoted ones (viafunctionLookupKeyPart, reusing the newly-exportedir.NeedsQuoting), matching the keys built bybuildFunctionLookup. The same normalization is applied inbuildFunctionBodyDependenciesso a quoted call inside a function body is ordered correctly too.Test plan
testdata/diff/dependency/issue_571_quoted_function_dependency/covering both quoted-identifier dependency paths: a quoted function"MyFunc"referenced in a table's column default, and a quoted function"AWrapper"whose body calls another quoted function"ZHelper"(which alphabetical ordering would otherwise create last).internal/diff/function_identifier_test.gocover escaped quotes ("My""Func") and thatmyfunc()and"MyFunc"()are not conflated.go test ./internal/diff -run TestDiffFromFiles— full suite passes.go test ./cmd -run TestPlanAndApply(dependency category) — full suite passes.gofmt -l/go vetclean.Fixes #571
Generated by Claude Code