Skip to content

fix: strip quotes from type names in view dependency detection#337

Open
FFX3 wants to merge 1 commit intopgplex:mainfrom
FFX3:fix/quoted-type-name-view-dependency
Open

fix: strip quotes from type names in view dependency detection#337
FFX3 wants to merge 1 commit intopgplex:mainfrom
FFX3:fix/quoted-type-name-view-dependency

Conversation

@FFX3
Copy link

@FFX3 FFX3 commented Mar 4, 2026

When a function returns a quoted type like SETOF public."ViewName", the extractBaseTypeName function was not stripping the double quotes. This caused the lookup in functionReferencesNewView to fail because the view lookup map stores unquoted names (e.g., public.viewname).

As a result, functions with quoted return types were not detected as having view dependencies, causing them to be created before the views they depend on, leading to "type does not exist" errors.

When a function returns a quoted type like SETOF public."ViewName",
the extractBaseTypeName function was not stripping the double quotes.
This caused the lookup in functionReferencesNewView to fail because
the view lookup map stores unquoted names (e.g., public.viewname).

As a result, functions with quoted return types were not detected as
having view dependencies, causing them to be created before the views
they depend on, leading to "type does not exist" errors.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@greptile-apps
Copy link

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR fixes a bug in extractBaseTypeName where double-quoted PostgreSQL identifiers (e.g. SETOF public."ViewName") were not having their quotes stripped before view-dependency lookup, causing functions that return quoted view types to be ordered before their dependent views — leading to type does not exist errors at runtime.

Key changes:

  • Added strings.ReplaceAll(t, "\"", "") in extractBaseTypeName to strip SQL delimited-identifier quotes, so the unquoted name can be matched against the lowercase view lookup map built by buildSchemaNameLookup.
  • Updated the function's doc comment to document the new stripping behavior.

Notes:

  • The fix is minimal and correct for all realistic PostgreSQL type expressions (quoted schema, quoted type name, quoted SETOF types, quoted array types).
  • No unit tests were added for the quoted-identifier path in extractBaseTypeName or functionReferencesNewView, leaving the fix without a regression guard.

Confidence Score: 4/5

  • Safe to merge — the one-line fix is correct and non-breaking, but lacks regression tests for the new behavior.
  • The change is minimal (a single strings.ReplaceAll call added after existing stripping steps), targets a well-understood code path, and introduces no new dependencies. The logic is correct: stripping SQL delimiter quotes before case-folded lookup is the right approach, and the existing downstream strings.ToLower in typeMatchesLookup handles the rest. Score is 4 rather than 5 only because the fix has no accompanying test coverage, leaving the quoted-identifier path un-guarded against future regressions.
  • No files require special attention, but consider adding unit tests for extractBaseTypeName in diff_test.go.

Important Files Changed

Filename Overview
internal/diff/diff.go Adds strings.ReplaceAll to strip double quotes from SQL identifiers in extractBaseTypeName; fix is correct and low-risk but no unit tests were added for the new behavior.

Sequence Diagram

sequenceDiagram
    participant Diff as diff.go (orderDDLStatements)
    participant FRV as functionReferencesNewView
    participant EBT as extractBaseTypeName
    participant TML as typeMatchesLookup
    participant VL as newViewLookup (map)

    Diff->>FRV: fn, newViewLookup
    FRV->>EBT: fn.ReturnType (e.g. SETOF public."ViewName")
    EBT-->>EBT: strip SETOF → public."ViewName"
    EBT-->>EBT: strip [] → public."ViewName"
    EBT-->>EBT: strip quotes → public.ViewName  [NEW]
    EBT-->>FRV: "public.ViewName"
    FRV->>TML: typeName="public.ViewName", schema, newViewLookup
    TML-->>TML: ToLower → "public.viewname"
    TML->>VL: lookup["public.viewname"]
    VL-->>TML: found ✓
    TML-->>FRV: true
    FRV-->>Diff: true → function depends on view, order accordingly
Loading

Last reviewed commit: b1f3b10

t = t[:len(t)-2]
}
// Strip double quotes from identifiers (e.g., public."ViewName" -> public.ViewName)
t = strings.ReplaceAll(t, "\"", "")
Copy link

Choose a reason for hiding this comment

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

No tests added for the new quoted-identifier behavior

The fix is correct, but there are no unit tests in the diff covering the new quoted-identifier stripping logic. Without tests, future refactors could silently regress this case. Consider adding a test for extractBaseTypeName (or an integration-style test for functionReferencesNewView) that covers at minimum:

  • SETOF public."ViewName"public.ViewName
  • "Schema"."View"Schema.View
  • "MyType"[]MyType
  • public.unquotedpublic.unquoted (no-op, regression guard)

Example:

func TestExtractBaseTypeName_QuotedIdentifiers(t *testing.T) {
    cases := []struct {
        input, want string
    }{
        {`SETOF public."ViewName"`, `public.ViewName`},
        {`"Schema"."View"`, `Schema.View`},
        {`"MyType"[]`, `MyType`},
        {`public.unquoted`, `public.unquoted`},
    }
    for _, tc := range cases {
        got := extractBaseTypeName(tc.input)
        if got != tc.want {
            t.Errorf("extractBaseTypeName(%q) = %q, want %q", tc.input, got, tc.want)
        }
    }
}

t = t[:len(t)-2]
}
// Strip double quotes from identifiers (e.g., public."ViewName" -> public.ViewName)
t = strings.ReplaceAll(t, "\"", "")
Copy link

Choose a reason for hiding this comment

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

Quote-stripping happens before case-folding, but after array stripping — works correctly, but edge case with dot-containing quoted schema names

The approach of strings.ReplaceAll(t, "\"", "") correctly handles the primary cases (public."ViewName"public.ViewName). However, it will silently mangle a (rare but valid) PostgreSQL identifier where the schema name itself is quoted and contains a dot — e.g., "my.schema"."view" would become my.schema.view, which typeMatchesLookup would treat as schema my + name schema.view instead of schema my.schema + name view.

In practice, schema names with dots are extremely uncommon and buildSchemaNameLookup would store the view under my.schema + view, so the lookup key would be my.schema.view — which happens to match the mangled string by coincidence in this case. However if the lookup was keyed differently this could fail.

This is a very low-probability edge case, but it's worth being aware of. The simpler alternative — stripping only quotes that wrap a single token (e.g., via a small regex like strings.Trim per identifier segment) — would be more precise, but for now the current approach covers all realistic PostgreSQL type expressions.

No code change required unless dot-containing schema names are a supported use case.

Copy link
Contributor

@tianzhou tianzhou left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. please add an unit test

@FFX3
Copy link
Author

FFX3 commented Mar 4, 2026

I'll take a look at adding the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants