diff --git a/internal/diff/diff.go b/internal/diff/diff.go index f54f49de..ec1e7c42 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -250,24 +250,24 @@ type Diff struct { } type ddlDiff struct { - addedSchemas []*ir.Schema - droppedSchemas []*ir.Schema - modifiedSchemas []*schemaDiff - addedTables []*ir.Table - droppedTables []*ir.Table - modifiedTables []*tableDiff - addedViews []*ir.View - droppedViews []*ir.View - modifiedViews []*viewDiff - addedFunctions []*ir.Function - droppedFunctions []*ir.Function - modifiedFunctions []*functionDiff - addedProcedures []*ir.Procedure - droppedProcedures []*ir.Procedure - modifiedProcedures []*procedureDiff - addedTypes []*ir.Type - droppedTypes []*ir.Type - modifiedTypes []*typeDiff + addedSchemas []*ir.Schema + droppedSchemas []*ir.Schema + modifiedSchemas []*schemaDiff + addedTables []*ir.Table + droppedTables []*ir.Table + modifiedTables []*tableDiff + addedViews []*ir.View + droppedViews []*ir.View + modifiedViews []*viewDiff + addedFunctions []*ir.Function + droppedFunctions []*ir.Function + modifiedFunctions []*functionDiff + addedProcedures []*ir.Procedure + droppedProcedures []*ir.Procedure + modifiedProcedures []*procedureDiff + addedTypes []*ir.Type + droppedTypes []*ir.Type + modifiedTypes []*typeDiff addedSequences []*ir.Sequence droppedSequences []*ir.Sequence modifiedSequences []*sequenceDiff @@ -412,38 +412,38 @@ type rlsChange struct { // GenerateMigration compares two IR schemas and returns the SQL differences func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { diff := &ddlDiff{ - addedSchemas: []*ir.Schema{}, - droppedSchemas: []*ir.Schema{}, - modifiedSchemas: []*schemaDiff{}, - addedTables: []*ir.Table{}, - droppedTables: []*ir.Table{}, - modifiedTables: []*tableDiff{}, - addedViews: []*ir.View{}, - droppedViews: []*ir.View{}, - modifiedViews: []*viewDiff{}, - addedFunctions: []*ir.Function{}, - droppedFunctions: []*ir.Function{}, - modifiedFunctions: []*functionDiff{}, - addedProcedures: []*ir.Procedure{}, - droppedProcedures: []*ir.Procedure{}, - modifiedProcedures: []*procedureDiff{}, - addedTypes: []*ir.Type{}, - droppedTypes: []*ir.Type{}, - modifiedTypes: []*typeDiff{}, - addedSequences: []*ir.Sequence{}, - droppedSequences: []*ir.Sequence{}, - modifiedSequences: []*sequenceDiff{}, - addedDefaultPrivileges: []*ir.DefaultPrivilege{}, - droppedDefaultPrivileges: []*ir.DefaultPrivilege{}, - modifiedDefaultPrivileges: []*defaultPrivilegeDiff{}, - addedPrivileges: []*ir.Privilege{}, - droppedPrivileges: []*ir.Privilege{}, - modifiedPrivileges: []*privilegeDiff{}, - addedRevokedDefaultPrivs: []*ir.RevokedDefaultPrivilege{}, - droppedRevokedDefaultPrivs: []*ir.RevokedDefaultPrivilege{}, - addedColumnPrivileges: []*ir.ColumnPrivilege{}, - droppedColumnPrivileges: []*ir.ColumnPrivilege{}, - modifiedColumnPrivileges: []*columnPrivilegeDiff{}, + addedSchemas: []*ir.Schema{}, + droppedSchemas: []*ir.Schema{}, + modifiedSchemas: []*schemaDiff{}, + addedTables: []*ir.Table{}, + droppedTables: []*ir.Table{}, + modifiedTables: []*tableDiff{}, + addedViews: []*ir.View{}, + droppedViews: []*ir.View{}, + modifiedViews: []*viewDiff{}, + addedFunctions: []*ir.Function{}, + droppedFunctions: []*ir.Function{}, + modifiedFunctions: []*functionDiff{}, + addedProcedures: []*ir.Procedure{}, + droppedProcedures: []*ir.Procedure{}, + modifiedProcedures: []*procedureDiff{}, + addedTypes: []*ir.Type{}, + droppedTypes: []*ir.Type{}, + modifiedTypes: []*typeDiff{}, + addedSequences: []*ir.Sequence{}, + droppedSequences: []*ir.Sequence{}, + modifiedSequences: []*sequenceDiff{}, + addedDefaultPrivileges: []*ir.DefaultPrivilege{}, + droppedDefaultPrivileges: []*ir.DefaultPrivilege{}, + modifiedDefaultPrivileges: []*defaultPrivilegeDiff{}, + addedPrivileges: []*ir.Privilege{}, + droppedPrivileges: []*ir.Privilege{}, + modifiedPrivileges: []*privilegeDiff{}, + addedRevokedDefaultPrivs: []*ir.RevokedDefaultPrivilege{}, + droppedRevokedDefaultPrivs: []*ir.RevokedDefaultPrivilege{}, + addedColumnPrivileges: []*ir.ColumnPrivilege{}, + droppedColumnPrivileges: []*ir.ColumnPrivilege{}, + modifiedColumnPrivileges: []*columnPrivilegeDiff{}, } // Compare schemas first in deterministic order diff --git a/internal/diff/function.go b/internal/diff/function.go index 3d34ef84..27ec76cb 100644 --- a/internal/diff/function.go +++ b/internal/diff/function.go @@ -9,6 +9,9 @@ import ( // generateCreateFunctionsSQL generates CREATE FUNCTION statements func generateCreateFunctionsSQL(functions []*ir.Function, targetSchema string, collector *diffCollector) { + // Build dependencies from function bodies (supplements pg_depend, which doesn't track SQL function body references) + buildFunctionBodyDependencies(functions) + // Sort functions by dependency order (topological sort) sortedFunctions := topologicallySortFunctions(functions) diff --git a/internal/diff/topological.go b/internal/diff/topological.go index cfbc13d4..7fc1bfe5 100644 --- a/internal/diff/topological.go +++ b/internal/diff/topological.go @@ -2,6 +2,7 @@ package diff import ( "sort" + "strings" "github.com/pgschema/pgschema/ir" ) @@ -653,3 +654,81 @@ func constraintMatchesFKReference(uniqueConstraint, fkConstraint *ir.Constraint) return true } + +// buildFunctionBodyDependencies scans function bodies for function calls and populates +// the Dependencies field. This supplements dependencies from pg_depend, which doesn't +// track references inside SQL function bodies. +func buildFunctionBodyDependencies(functions []*ir.Function) { + if len(functions) <= 1 { + return + } + + // Build lookup maps by function name (both qualified and unqualified) + // Map to the full key format used by Dependencies: schema.name(args) + type funcInfo struct { + fn *ir.Function + key string + } + functionLookup := make(map[string]funcInfo) + + for _, fn := range functions { + key := fn.Schema + "." + fn.Name + "(" + fn.GetArguments() + ")" + name := strings.ToLower(fn.Name) + + // Store under unqualified name + functionLookup[name] = funcInfo{fn: fn, key: key} + + // Store under qualified name + if fn.Schema != "" { + qualified := strings.ToLower(fn.Schema) + "." + name + functionLookup[qualified] = funcInfo{fn: fn, key: key} + } + } + + // For each function, scan its body for function calls + for _, fn := range functions { + if fn.Definition == "" { + continue + } + + fnKey := fn.Schema + "." + fn.Name + "(" + fn.GetArguments() + ")" + + matches := functionCallRegex.FindAllStringSubmatch(fn.Definition, -1) + for _, match := range matches { + if len(match) < 2 { + continue + } + identifier := strings.ToLower(match[1]) + if identifier == "" { + continue + } + + // Try to find the referenced function + var info funcInfo + var found bool + + if info, found = functionLookup[identifier]; !found { + // Try with schema prefix if identifier is unqualified + if !strings.Contains(identifier, ".") && fn.Schema != "" { + qualified := strings.ToLower(fn.Schema) + "." + identifier + info, found = functionLookup[qualified] + } + } + + // If found and not self-reference, add dependency + if found && info.key != fnKey { + // Check if dependency already exists + alreadyExists := false + for _, existing := range fn.Dependencies { + if existing == info.key { + alreadyExists = true + break + } + } + if !alreadyExists { + fn.Dependencies = append(fn.Dependencies, info.key) + } + } + } + } +} diff --git a/internal/diff/topological_test.go b/internal/diff/topological_test.go index ecf0ba5d..dd4a300e 100644 --- a/internal/diff/topological_test.go +++ b/internal/diff/topological_test.go @@ -222,3 +222,220 @@ func newTestDomainType(name, baseType string) *ir.Type { BaseType: baseType, } } + +func TestBuildFunctionBodyDependencies(t *testing.T) { + tests := []struct { + name string + funcs []*ir.Function + expected map[string][]string // function name -> expected dependency names + }{ + { + name: "simple dependency: wrapper calls helper", + funcs: []*ir.Function{ + { + Schema: "public", + Name: "wrapper", + Definition: "SELECT helper()", + Language: "sql", + }, + { + Schema: "public", + Name: "helper", + Definition: "SELECT 1", + Language: "sql", + }, + }, + expected: map[string][]string{ + "wrapper": {"public.helper()"}, + "helper": nil, + }, + }, + { + name: "qualified function call", + funcs: []*ir.Function{ + { + Schema: "public", + Name: "caller", + Definition: "SELECT public.callee()", + Language: "sql", + }, + { + Schema: "public", + Name: "callee", + Definition: "SELECT 1", + Language: "sql", + }, + }, + expected: map[string][]string{ + "caller": {"public.callee()"}, + "callee": nil, + }, + }, + { + name: "chain: a calls b calls c", + funcs: []*ir.Function{ + { + Schema: "public", + Name: "func_a", + Definition: "SELECT func_b()", + Language: "sql", + }, + { + Schema: "public", + Name: "func_b", + Definition: "SELECT func_c()", + Language: "sql", + }, + { + Schema: "public", + Name: "func_c", + Definition: "SELECT 1", + Language: "sql", + }, + }, + expected: map[string][]string{ + "func_a": {"public.func_b()"}, + "func_b": {"public.func_c()"}, + "func_c": nil, + }, + }, + { + name: "no self-dependency", + funcs: []*ir.Function{ + { + Schema: "public", + Name: "recursive", + Definition: "SELECT recursive()", // calls itself + Language: "sql", + }, + }, + expected: map[string][]string{ + "recursive": nil, // should not add self as dependency + }, + }, + { + name: "multiple calls in body", + funcs: []*ir.Function{ + { + Schema: "public", + Name: "orchestrator", + Definition: "SELECT step_one() + step_two() + step_three()", + Language: "sql", + }, + { + Schema: "public", + Name: "step_one", + Definition: "SELECT 1", + Language: "sql", + }, + { + Schema: "public", + Name: "step_two", + Definition: "SELECT 2", + Language: "sql", + }, + { + Schema: "public", + Name: "step_three", + Definition: "SELECT 3", + Language: "sql", + }, + }, + expected: map[string][]string{ + "orchestrator": {"public.step_one()", "public.step_two()", "public.step_three()"}, + "step_one": nil, + "step_two": nil, + "step_three": nil, + }, + }, + { + name: "external function not tracked", + funcs: []*ir.Function{ + { + Schema: "public", + Name: "my_func", + Definition: "SELECT pg_catalog.now() + external_func()", + Language: "sql", + }, + }, + expected: map[string][]string{ + "my_func": nil, // external functions not in our list + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Clear any existing dependencies + for _, fn := range tt.funcs { + fn.Dependencies = nil + } + + buildFunctionBodyDependencies(tt.funcs) + + for _, fn := range tt.funcs { + expectedDeps := tt.expected[fn.Name] + + if len(fn.Dependencies) != len(expectedDeps) { + t.Errorf("function %s: expected %d dependencies, got %d: %v", + fn.Name, len(expectedDeps), len(fn.Dependencies), fn.Dependencies) + continue + } + + // Check each expected dependency exists + for _, exp := range expectedDeps { + found := false + for _, dep := range fn.Dependencies { + if dep == exp { + found = true + break + } + } + if !found { + t.Errorf("function %s: expected dependency %s not found in %v", + fn.Name, exp, fn.Dependencies) + } + } + } + }) + } +} + +func TestBuildFunctionBodyDependenciesWithTopologicalSort(t *testing.T) { + // Integration test: build dependencies then sort + functions := []*ir.Function{ + { + Schema: "public", + Name: "z_wrapper", // alphabetically last, but should come last after sort + Definition: "SELECT a_helper()", + Language: "sql", + }, + { + Schema: "public", + Name: "a_helper", // alphabetically first + Definition: "SELECT 1", + Language: "sql", + }, + } + + // Build dependencies from function bodies + buildFunctionBodyDependencies(functions) + + // Verify dependency was detected + if len(functions[0].Dependencies) != 1 { + t.Fatalf("expected z_wrapper to have 1 dependency, got %d", len(functions[0].Dependencies)) + } + + // Now sort + sorted := topologicallySortFunctions(functions) + + // a_helper should come before z_wrapper + order := make(map[string]int) + for i, fn := range sorted { + order[fn.Name] = i + } + + if order["a_helper"] >= order["z_wrapper"] { + t.Errorf("expected a_helper before z_wrapper, got order: %v", order) + } +} diff --git a/testdata/diff/dependency/sql_function_body_reference/diff.sql b/testdata/diff/dependency/sql_function_body_reference/diff.sql new file mode 100644 index 00000000..3ecde26c --- /dev/null +++ b/testdata/diff/dependency/sql_function_body_reference/diff.sql @@ -0,0 +1,16 @@ +CREATE OR REPLACE FUNCTION z_helper( + input text +) +RETURNS text +LANGUAGE sql +IMMUTABLE +AS $$ SELECT upper(input) +$$; +CREATE OR REPLACE FUNCTION a_wrapper( + input text +) +RETURNS text +LANGUAGE sql +IMMUTABLE +AS $$ SELECT z_helper(input) +$$; diff --git a/testdata/diff/dependency/sql_function_body_reference/new.sql b/testdata/diff/dependency/sql_function_body_reference/new.sql new file mode 100644 index 00000000..65a36218 --- /dev/null +++ b/testdata/diff/dependency/sql_function_body_reference/new.sql @@ -0,0 +1,14 @@ +-- z_helper must be created first because a_wrapper calls it +-- Without body dependency scanning, alphabetical order would create a_wrapper first and fail + +CREATE FUNCTION z_helper(input text) +RETURNS text +LANGUAGE sql +IMMUTABLE +AS $$ SELECT upper(input) $$; + +CREATE FUNCTION a_wrapper(input text) +RETURNS text +LANGUAGE sql +IMMUTABLE +AS $$ SELECT z_helper(input) $$; diff --git a/testdata/diff/dependency/sql_function_body_reference/old.sql b/testdata/diff/dependency/sql_function_body_reference/old.sql new file mode 100644 index 00000000..03d7e9ae --- /dev/null +++ b/testdata/diff/dependency/sql_function_body_reference/old.sql @@ -0,0 +1 @@ +-- Empty schema diff --git a/testdata/diff/dependency/sql_function_body_reference/plan.json b/testdata/diff/dependency/sql_function_body_reference/plan.json new file mode 100644 index 00000000..b1acb63c --- /dev/null +++ b/testdata/diff/dependency/sql_function_body_reference/plan.json @@ -0,0 +1,26 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.6.1", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "965b1131737c955e24c7f827c55bd78e4cb49a75adfd04229e0ba297376f5085" + }, + "groups": [ + { + "steps": [ + { + "sql": "CREATE OR REPLACE FUNCTION z_helper(\n input text\n)\nRETURNS text\nLANGUAGE sql\nIMMUTABLE\nAS $$ SELECT upper(input)\n$$;", + "type": "function", + "operation": "create", + "path": "public.z_helper" + }, + { + "sql": "CREATE OR REPLACE FUNCTION a_wrapper(\n input text\n)\nRETURNS text\nLANGUAGE sql\nIMMUTABLE\nAS $$ SELECT z_helper(input)\n$$;", + "type": "function", + "operation": "create", + "path": "public.a_wrapper" + } + ] + } + ] +} diff --git a/testdata/diff/dependency/sql_function_body_reference/plan.sql b/testdata/diff/dependency/sql_function_body_reference/plan.sql new file mode 100644 index 00000000..42e3cbda --- /dev/null +++ b/testdata/diff/dependency/sql_function_body_reference/plan.sql @@ -0,0 +1,17 @@ +CREATE OR REPLACE FUNCTION z_helper( + input text +) +RETURNS text +LANGUAGE sql +IMMUTABLE +AS $$ SELECT upper(input) +$$; + +CREATE OR REPLACE FUNCTION a_wrapper( + input text +) +RETURNS text +LANGUAGE sql +IMMUTABLE +AS $$ SELECT z_helper(input) +$$; diff --git a/testdata/diff/dependency/sql_function_body_reference/plan.txt b/testdata/diff/dependency/sql_function_body_reference/plan.txt new file mode 100644 index 00000000..b2197982 --- /dev/null +++ b/testdata/diff/dependency/sql_function_body_reference/plan.txt @@ -0,0 +1,29 @@ +Plan: 2 to add. + +Summary by type: + functions: 2 to add + +Functions: + + a_wrapper + + z_helper + +DDL to be executed: +-------------------------------------------------- + +CREATE OR REPLACE FUNCTION z_helper( + input text +) +RETURNS text +LANGUAGE sql +IMMUTABLE +AS $$ SELECT upper(input) +$$; + +CREATE OR REPLACE FUNCTION a_wrapper( + input text +) +RETURNS text +LANGUAGE sql +IMMUTABLE +AS $$ SELECT z_helper(input) +$$; diff --git a/testdata/dump/sakila/pgschema.sql b/testdata/dump/sakila/pgschema.sql index 74beae12..37254f59 100644 --- a/testdata/dump/sakila/pgschema.sql +++ b/testdata/dump/sakila/pgschema.sql @@ -2,8 +2,8 @@ -- pgschema database dump -- --- Dumped from database version PostgreSQL 17.5 --- Dumped by pgschema version 1.5.1 +-- Dumped from database version PostgreSQL 18.0 +-- Dumped by pgschema version 1.6.1 -- @@ -610,46 +610,6 @@ SELECT CASE END $_$; --- --- Name: film_in_stock(integer, integer); Type: FUNCTION; Schema: -; Owner: - --- - -CREATE OR REPLACE FUNCTION film_in_stock( - p_film_id integer, - p_store_id integer, - OUT p_film_count integer -) -RETURNS SETOF integer -LANGUAGE sql -VOLATILE -AS $_$ - SELECT inventory_id - FROM inventory - WHERE film_id = $1 - AND store_id = $2 - AND inventory_in_stock(inventory_id); -$_$; - --- --- Name: film_not_in_stock(integer, integer); Type: FUNCTION; Schema: -; Owner: - --- - -CREATE OR REPLACE FUNCTION film_not_in_stock( - p_film_id integer, - p_store_id integer, - OUT p_film_count integer -) -RETURNS SETOF integer -LANGUAGE sql -VOLATILE -AS $_$ - SELECT inventory_id - FROM inventory - WHERE film_id = $1 - AND store_id = $2 - AND NOT inventory_in_stock(inventory_id); -$_$; - -- -- Name: get_customer_balance(integer, timestamptz); Type: FUNCTION; Schema: -; Owner: - -- @@ -760,6 +720,46 @@ BEGIN END $$; +-- +-- Name: film_in_stock(integer, integer); Type: FUNCTION; Schema: -; Owner: - +-- + +CREATE OR REPLACE FUNCTION film_in_stock( + p_film_id integer, + p_store_id integer, + OUT p_film_count integer +) +RETURNS SETOF integer +LANGUAGE sql +VOLATILE +AS $_$ + SELECT inventory_id + FROM inventory + WHERE film_id = $1 + AND store_id = $2 + AND inventory_in_stock(inventory_id); +$_$; + +-- +-- Name: film_not_in_stock(integer, integer); Type: FUNCTION; Schema: -; Owner: - +-- + +CREATE OR REPLACE FUNCTION film_not_in_stock( + p_film_id integer, + p_store_id integer, + OUT p_film_count integer +) +RETURNS SETOF integer +LANGUAGE sql +VOLATILE +AS $_$ + SELECT inventory_id + FROM inventory + WHERE film_id = $1 + AND store_id = $2 + AND NOT inventory_in_stock(inventory_id); +$_$; + -- -- Name: last_day(with time zone); Type: FUNCTION; Schema: -; Owner: - --