diff --git a/fixtures/schema.sql b/fixtures/schema.sql index f741341..0755357 100644 --- a/fixtures/schema.sql +++ b/fixtures/schema.sql @@ -65,49 +65,40 @@ CREATE MATERIALIZED VIEW user_states AS CREATE UNIQUE INDEX user_states_state ON user_states (state); --- Range-partitioned table, exercising the parent's PARTITION BY --- clause round-trip. --- --- Partition children (`CREATE TABLE child PARTITION OF parent FOR --- VALUES ...`, incl. a DEFAULT partition) were attempted here but are --- skipped: they uncovered a real, previously-unknown product bug. --- `pg_dump` never emits the inline `PARTITION OF ... FOR VALUES` form --- pglifecycle's parser handles (src/ddl/table.rs's `create_table`, --- matched via `kw_partition && kw_of`); it always splits partition --- attachment into a separate `ALTER TABLE ONLY parent ATTACH --- PARTITION child FOR VALUES ...` statement (TOC entry "Type: TABLE --- ATTACH"). No code anywhere in src/ddl/ or src/pull/ recognizes that --- ALTER TABLE subform, so it is silently dropped: the child comes --- back from `pull` as a plain, unattached table with no partition --- bound, and `build` has nothing to re-emit the ATTACH from. The --- existing pull unit test `merges_partition_children` --- (src/pull/mod.rs) only exercises the inline `PARTITION OF ... FOR --- VALUES` form, so it never caught this since that form doesn't --- occur in real `pg_dump` output. +-- Range-partitioned table with children, exercising the ATTACH +-- PARTITION round-trip. The children are authored inline with +-- `PARTITION OF ... FOR VALUES`, but `pg_dump` always re-emits them as +-- a plain `CREATE TABLE` plus a separate `ALTER TABLE ONLY parent +-- ATTACH PARTITION child FOR VALUES ...` (TOC entry "Type: TABLE +-- ATTACH"); pull now recognizes that form and folds the child back +-- into the parent's partitions (incl. a DEFAULT partition). CREATE TABLE events ( id BIGINT NOT NULL, created_at DATE NOT NULL, payload TEXT ) PARTITION BY RANGE (created_at); --- Typed table: composite type + CREATE TABLE OF. --- --- A column constraint (`CREATE TABLE locations OF point_2d --- (CONSTRAINT locations_x_check CHECK (x IS NOT NULL))`) was --- attempted here but is skipped: it uncovered another real product --- bug. `pg_dump` keeps a typed table's column constraint inline in --- the `CREATE TABLE ... OF type (...)` statement (unlike partition --- attachment above), but `create_table` (src/ddl/table.rs) only --- walks `TableElement` children to find constraints/columns; a typed --- table's parenthesized element list uses a different grammar --- production, so the CHECK constraint is silently dropped by pull --- and the round-trip loses it entirely. +CREATE TABLE events_2024 PARTITION OF events + FOR VALUES FROM ('2024-01-01') TO ('2025-01-01'); + +CREATE TABLE events_2025 PARTITION OF events + FOR VALUES FROM ('2025-01-01') TO ('2026-01-01'); + +CREATE TABLE events_default PARTITION OF events DEFAULT; + +-- Typed table: composite type + CREATE TABLE OF, with an inline +-- column constraint. `pg_dump` keeps a typed table's constraint inline +-- in the `CREATE TABLE ... OF type (...)` statement; pull now walks the +-- typed element list (a distinct grammar production) so the constraint +-- survives the round-trip. CREATE TYPE point_2d AS ( x DOUBLE PRECISION, y DOUBLE PRECISION ); -CREATE TABLE locations OF point_2d; +CREATE TABLE locations OF point_2d ( + CONSTRAINT locations_x_check CHECK (x IS NOT NULL) +); -- Table-level CHECK constraints CREATE TABLE products ( @@ -118,55 +109,54 @@ CREATE TABLE products ( CONSTRAINT products_quantity_nonneg CHECK (quantity >= 0) ); +-- View that sorts alphabetically BEFORE its underlying table +-- ("active_users" < "users"). Table, Sequence, View and ForeignTable +-- share libpgdump's priority tier, so without a recorded dependency +-- edge this view would restore before `users` and pg_restore would +-- fail with "relation ... does not exist". pull now derives view +-- dependency edges by re-parsing the stored query, so build orders it +-- after the tables it references. +CREATE VIEW active_users AS + SELECT id, name, surname FROM users WHERE state = 'verified'; + -- View with security_barrier and check_option. --- --- Named to sort alphabetically after `users`: build's dependency --- graph never records an edge from a view to the tables/views it --- queries (only tables get FK-derived `dependencies`, per --- src/pull/writer.rs's table_dependencies()), so a view with no --- recorded dependency is ordered purely by libpgdump's default --- same-priority (namespace, tag) sort -- Table, Sequence, View, and --- ForeignTable all share priority 22. A view alphabetically before --- its underlying table (e.g. "active_users" before "users") is --- restored first and pg_restore fails with "relation ... does not --- exist". This is a real, pre-existing gap (see commit message); it --- happens to be masked for user_states below because --- MATERIALIZED VIEW has its own, later, priority tier. CREATE VIEW verified_users WITH (security_barrier = true, check_option = 'local') AS SELECT id, name, surname FROM users WHERE state = 'verified'; +-- Zero-argument trigger function + trigger + trigger comment. Trigger +-- functions are always zero-arg, exercising build's `CREATE FUNCTION +-- name() RETURNS trigger` rendering (a missing `()` here is invalid +-- SQL); the `COMMENT ON TRIGGER` exercises pull's trigger-comment +-- match arm. +-- The body is authored in libpgfmt's normalized (two-space) form so the +-- round-trip gate's exact schema diff stays clean; pull reformats +-- function bodies through libpgfmt regardless of --style. +CREATE FUNCTION test.touch_last_modified() RETURNS trigger + LANGUAGE plpgsql AS $$ +BEGIN + NEW.last_modified_at := CURRENT_TIMESTAMP; + RETURN NEW; +END; +$$; + +CREATE TRIGGER users_touch_last_modified + BEFORE UPDATE ON users + FOR EACH ROW EXECUTE FUNCTION test.touch_last_modified(); + +COMMENT ON TRIGGER users_touch_last_modified ON users IS + 'Maintains last_modified_at on update'; + -- Per-object COMMENT: column --- --- A trigger + trigger-comment addition was attempted here but is --- skipped: it uncovered two real product bugs rather than a fixture --- issue: --- 1. src/pull/mod.rs `apply_comment` has no match arm for TRIGGER --- (or RULE/POLICY/CONSTRAINT), so `COMMENT ON TRIGGER ... ON t` --- always logs "Comment on unmatched object" and the comment is --- silently dropped. --- 2. src/build/mod.rs `dump_function` (and the pull-side --- src/ddl/function.rs, which no longer appends `()` to a --- zero-parameter function's name as the Python tokenizer did) --- renders zero-argument functions as `CREATE FUNCTION name --- RETURNS ...` with no parameter list, which is invalid SQL and --- fails pg_restore. This affects any zero-arg function, --- including the trigger functions PostgreSQL requires. COMMENT ON COLUMN users.display_name IS 'Optional user-facing display name'; -- Bare `public` schema reference, exercising case-folding of an --- unquoted `public` identifier. --- --- Uses a plain integer primary key rather than SERIAL: a SERIAL --- column's DEFAULT is emitted by pg_dump as a separate, later --- `ALTER TABLE ONLY public.widgets ALTER COLUMN id SET DEFAULT --- nextval(...)` statement (TOC entry "Type: DEFAULT"), which is --- another real, previously-unknown bug -- pull only captures a --- column's default when it is inline in the CREATE TABLE statement --- itself, so the sequence-owned default is silently dropped and the --- round-trip loses it. +-- unquoted `public` identifier. Uses a SERIAL primary key: pg_dump +-- emits the column default as a separate, later `ALTER TABLE ONLY +-- public.widgets ALTER COLUMN id SET DEFAULT nextval(...)` (TOC entry +-- "Type: DEFAULT"), which pull now folds back onto the column. CREATE TABLE public.widgets ( - id INTEGER PRIMARY KEY, + id SERIAL PRIMARY KEY, name TEXT NOT NULL ); diff --git a/schemata/table.yml b/schemata/table.yml index cf9b58f..e12a432 100644 --- a/schemata/table.yml +++ b/schemata/table.yml @@ -359,7 +359,7 @@ properties: - for_values_when - required: - for_values_from - - for_vlaues_to + - for_values_to not: required: - default diff --git a/src/build/mod.rs b/src/build/mod.rs index 7a75961..bcbc8c3 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -26,6 +26,9 @@ //! `CREATE INDEX schema.name`, which PostgreSQL rejects) //! 11. Roles with `create: false` (e.g. PUBLIC) emit no entry (Python //! emitted CREATE ROLE anyway) +//! 12. CREATE/DROP FUNCTION references render schema-qualified (Python +//! emitted a bare name, which pg_restore rejects under its reset +//! `search_path` with "no schema has been selected to create in") mod acls; @@ -110,6 +113,7 @@ pub fn assemble(project: &Project) -> Result { entry.dependencies.extend(deps); } } + builder.split_sequence_defaults(project)?; let item_ids = builder .dump_id_map .iter() @@ -310,6 +314,78 @@ impl Builder { Ok(()) } + /// Emit the `ALTER TABLE ... SET DEFAULT nextval(...)` entries held + /// out of their `CREATE TABLE` by [`sequence_backed_default`]. Each + /// depends on both its table and the referenced sequence so + /// pg_restore orders it after both, breaking the SERIAL dependency + /// cycle the way pg_dump's separate `DEFAULT` TOC entries do. + fn split_sequence_defaults( + &mut self, + project: &Project, + ) -> Result<(), String> { + let mut sequence_ids: HashMap<(String, String), i32> = HashMap::new(); + for item in &project.inventory { + if let Definition::Sequence(s) = &item.definition + && let Some(&id) = self.dump_id_map.get(&item.id) + { + sequence_ids.insert((s.schema.clone(), s.name.clone()), id); + } + } + for item in &project.inventory { + let Definition::Table(d) = &item.definition else { + continue; + }; + // only the plain-column CREATE TABLE path renders defaults + // inline; sql-override, foreign, typed and LIKE tables do not + if d.sql.is_some() + || d.server.is_some() + || d.from_type.is_some() + || d.like_table.is_some() + { + continue; + } + let Some(&table_id) = self.dump_id_map.get(&item.id) else { + continue; + }; + for column in d.columns.as_deref().unwrap_or_default() { + let Some(default) = sequence_backed_default(column) else { + continue; + }; + let mut deps = vec![table_id]; + if let Some(key) = nextval_target(default) + && let Some(&seq_id) = sequence_ids.get(&key) + { + deps.push(seq_id); + } + let table = format!( + "{}.{}", + quote_ident(&d.schema), + quote_ident(&d.name) + ); + let col = quote_ident(&column.name); + let defn = vec![format!( + "ALTER TABLE ONLY {table} ALTER COLUMN {col} \ + SET DEFAULT {default}" + )]; + let drop = vec![format!( + "ALTER TABLE ONLY {table} ALTER COLUMN {col} \ + DROP DEFAULT" + )]; + self.add_entry( + "DEFAULT", + &d.schema, + &format!("{} {}", d.name, column.name), + &d.owner, + &defn, + &drop, + &deps, + None, + )?; + } + } + Ok(()) + } + /// `schema.name` with identifier quoting (ports _item_name) fn item_name(&self, item: &Item) -> String { let name = item.definition.name(); @@ -666,7 +742,12 @@ impl Builder { if let Some(sql) = &d.sql { return self.add_item(item, vec![sql.clone()], vec![], false); } - let func_name = match &d.parameters { + // `comment_target` is only set when `func_name` diverges from the + // default comment target (`namespace.d.name`) computed in + // `add_comment` — i.e. only for the bare, unparenthesized + // zero-argument case below, where `()` must be appended for a + // valid `COMMENT ON FUNCTION`. + let (func_name, comment_target) = match &d.parameters { Some(parameters) if !parameters.is_empty() => { let params: Vec = parameters .iter() @@ -683,24 +764,49 @@ impl Builder { value.join(" ") }) .collect(); - format!( + let func_name = format!( "{}({})", d.name.split('(').next().unwrap_or_default(), params.join(", ") - ) + ); + (func_name, None) + } + // no structured `parameters`: `d.name` may already carry an + // embedded `(argtypes)` signature (the on-disk convention for + // disambiguating overloads); only a bare, unparenthesized + // name needs `()` appended for a true zero-argument function + _ if d.name.contains('(') => (d.name.clone(), None), + _ => { + let func_name = format!("{}()", d.name); + let comment_target = if d.schema.is_empty() { + func_name.clone() + } else { + format!("{}.{}", quote_ident(&d.schema), func_name) + }; + (func_name, Some(comment_target)) } - _ => d.name.clone(), + }; + // pg_dump restores with an empty search_path and schema-qualifies + // every object name; a bare `CREATE FUNCTION name(...)` fails at + // restore with "no schema has been selected to create in". The + // COMMENT target is already qualified (via `namespace.tag`, or + // `comment_target` for the bare zero-arg case), so only the + // CREATE/DROP references need it here. + let qualified = if d.schema.is_empty() { + func_name.clone() + } else { + format!("{}.{}", quote_ident(&d.schema), func_name) }; let mut create = vec![ "CREATE".into(), "FUNCTION".into(), - func_name.clone(), + qualified.clone(), "RETURNS".into(), d.returns.clone().unwrap_or_default(), "LANGUAGE".into(), d.language.clone().unwrap_or_default(), ]; - let drop = vec!["DROP FUNCTION IF EXISTS".into(), func_name]; + let drop = vec!["DROP FUNCTION IF EXISTS".into(), qualified.clone()]; if let Some(transform_types) = &d.transform_types { let tts: Vec = transform_types .iter() @@ -763,7 +869,13 @@ impl Builder { if let Some(definition) = &d.definition { let create_sql = vec![format!("{} $$\n{}\n$$", create.join(" "), definition)]; - return self.add_item(item, create_sql, drop, false); + return self.add_item_with_comment_target( + item, + create_sql, + drop, + false, + comment_target, + ); } if let (Some(object_file), Some(link_symbol)) = (&d.object_file, &d.link_symbol) @@ -774,7 +886,13 @@ impl Builder { postgres_value(&Value::String(link_symbol.clone())) )); } - self.add_item(item, create, drop, false) + self.add_item_with_comment_target( + item, + create, + drop, + false, + comment_target, + ) } fn dump_group(&mut self, item: &Item) -> Result<(), String> { @@ -1180,7 +1298,16 @@ impl Builder { create.push("(".into()); let mut inner = Vec::new(); for column in d.columns.as_deref().unwrap_or_default() { - inner.push(render_table_column(column)); + // sequence-backed defaults are split into a separate + // SET DEFAULT entry (see split_sequence_defaults), so + // render the column without its default here + if sequence_backed_default(column).is_some() { + let mut stripped = column.clone(); + stripped.default = None; + inner.push(render_table_column(&stripped)); + } else { + inner.push(render_table_column(column)); + } } push_table_constraints(d, &mut inner); create.push(inner.join(", ")); @@ -1977,6 +2104,31 @@ pub(crate) fn render_default(value: &Value) -> String { postgres_value(value) } +/// A column default that references a sequence (`nextval(...)`, the +/// SERIAL form). pg_dump emits these as a separate `ALTER TABLE ... SET +/// DEFAULT` entry rather than inline on the column, because the sequence +/// is `OWNED BY` the column: inlining the default would make the table +/// depend on the sequence while the sequence depends on the table, an +/// unrestorable cycle. +fn sequence_backed_default(column: &Column) -> Option<&str> { + let default = column.default.as_ref()?.as_str()?; + default.contains("nextval(").then_some(default) +} + +/// Parse the `schema.name` a `nextval('schema.name'::regclass)` default +/// points at, so the split-out DEFAULT entry can depend on that sequence +fn nextval_target(default: &str) -> Option<(String, String)> { + let start = default.find('\'')? + 1; + let rest = &default[start..]; + let end = rest.find('\'')?; + let qualified = &rest[..end]; + let strip = |s: &str| s.trim_matches('"').to_string(); + match qualified.rsplit_once('.') { + Some((schema, name)) => Some((strip(schema), strip(name))), + None => Some((String::new(), strip(qualified))), + } +} + pub(crate) fn render_table_column(column: &Column) -> String { let mut sql = vec![column.name.clone(), column.data_type.clone()]; if let Some(collation) = &column.collation { @@ -2815,4 +2967,180 @@ mod tests { DEFAULT;\n" ); } + + fn zero_arg_function(name: &str, comment: Option<&str>) -> Item { + Item { + id: 1, + desc: ObjectType::Function, + definition: Definition::Function(crate::models::Function { + name: name.into(), + schema: "test".into(), + owner: "app".into(), + sql: None, + parameters: None, + returns: Some("trigger".into()), + language: Some("plpgsql".into()), + transform_types: None, + window: None, + immutable: None, + stable: None, + volatile: None, + leak_proof: None, + called_on_null_input: None, + strict: None, + security: None, + parallel: None, + cost: None, + rows: None, + support: None, + configuration: None, + definition: Some("BEGIN\n RETURN NEW;\nEND;".into()), + object_file: None, + link_symbol: None, + comment: comment.map(String::from), + }), + dependencies: BTreeSet::new(), + } + } + + /// Render `item` and return the FUNCTION entry's create and drop SQL + fn function_entry(item: &Item) -> (String, String) { + let dump = libpgdump::new("t", "UTF-8", "18.0").unwrap(); + let mut builder = Builder { + dump, + dump_id_map: HashMap::new(), + superuser: "postgres".into(), + }; + builder.dump_item(item).unwrap(); + let entry = builder + .dump + .entries() + .iter() + .find(|e| e.desc == libpgdump::ObjectType::Function) + .expect("a FUNCTION entry") + .clone(); + ( + entry.defn.unwrap_or_default(), + entry.drop_stmt.unwrap_or_default(), + ) + } + + #[test] + fn renders_zero_arg_function_with_parens() { + let item = zero_arg_function("bare_zero", None); + let (create, drop) = function_entry(&item); + assert!( + create.starts_with( + "CREATE FUNCTION test.bare_zero() RETURNS trigger LANGUAGE \ + plpgsql AS $$" + ), + "unexpected CREATE: {create}" + ); + assert_eq!(drop, "DROP FUNCTION IF EXISTS test.bare_zero();\n"); + } + + #[test] + fn parses_nextval_target() { + assert_eq!( + nextval_target("nextval('test.widgets_id_seq'::regclass)"), + Some(("test".into(), "widgets_id_seq".into())) + ); + assert_eq!( + nextval_target("nextval('bare_seq'::regclass)"), + Some((String::new(), "bare_seq".into())) + ); + assert_eq!( + nextval_target("nextval('\"S\".\"Q\"'::regclass)"), + Some(("S".into(), "Q".into())) + ); + } + + #[test] + fn splits_sequence_backed_default_into_separate_entry() { + use std::path::PathBuf; + + use crate::models::Sequence; + + let mut col = column("id", "integer", true); + col.default = Some(json!("nextval('test.widgets_id_seq'::regclass)")); + let mut table = base_table("widgets"); + table.columns = Some(vec![col]); + let seq = Sequence { + name: "widgets_id_seq".into(), + schema: "test".into(), + owner: "app".into(), + sql: None, + data_type: None, + increment_by: None, + min_value: None, + max_value: None, + start_with: None, + cache: None, + cycle: None, + owned_by: Some("test.widgets.id".into()), + comment: None, + }; + let seq_item = Item { + id: 2, + desc: ObjectType::Sequence, + definition: Definition::Sequence(seq), + dependencies: BTreeSet::new(), + }; + let project = Project { + name: "t".into(), + encoding: "UTF-8".into(), + stdstrings: true, + superuser: "postgres".into(), + default_schema: "public".into(), + path: PathBuf::new(), + inventory: vec![table_item(1, table), seq_item], + }; + let output = assemble(&project).unwrap(); + let entries = output.dump.entries(); + // the CREATE TABLE must not inline the sequence default + let table_defn = entries + .iter() + .find(|e| e.desc == libpgdump::ObjectType::Table) + .and_then(|e| e.defn.clone()) + .expect("a TABLE entry"); + assert!( + !table_defn.contains("DEFAULT"), + "table inlined the sequence default: {table_defn}" + ); + // a separate DEFAULT entry carries it, depending on the sequence + let default_entry = entries + .iter() + .find(|e| e.desc == libpgdump::ObjectType::Default) + .expect("a DEFAULT entry"); + let defn = default_entry.defn.clone().unwrap(); + assert!( + defn.contains( + "ALTER TABLE ONLY test.widgets ALTER COLUMN id \ + SET DEFAULT nextval('test.widgets_id_seq'::regclass)" + ), + "unexpected DEFAULT defn: {defn}" + ); + let seq_dump_id = entries + .iter() + .find(|e| e.desc == libpgdump::ObjectType::Sequence) + .unwrap() + .dump_id; + assert!( + default_entry.dependencies.contains(&seq_dump_id), + "DEFAULT entry should depend on the sequence" + ); + } + + #[test] + fn comments_zero_arg_function_with_parens() { + let item = zero_arg_function("bare_zero", Some("a trigger fn")); + let defns = comment_defns(&item); + assert_eq!( + defns, + vec![ + "COMMENT ON FUNCTION test.bare_zero() IS $$a trigger \ + fn$$;\n;\n" + ] + ); + } } diff --git a/src/ddl/mod.rs b/src/ddl/mod.rs index 7772286..ded34d8 100644 --- a/src/ddl/mod.rs +++ b/src/ddl/mod.rs @@ -42,6 +42,24 @@ pub enum Statement { name: Option, constraint: TableConstraint, }, + /// ALTER TABLE ONLY `parent` ATTACH PARTITION `child` FOR VALUES ... + /// — pg_dump emits partition children as a plain CREATE TABLE plus + /// this separate ATTACH rather than inline `PARTITION OF`; the pull + /// assembly folds the already-ingested child into the parent's + /// `partitions` and drops the standalone child table + AttachPartition { + parent: QualifiedName, + partition: models::TablePartition, + }, + /// ALTER TABLE ... ALTER COLUMN ... SET DEFAULT — pg_dump emits + /// column defaults this way (e.g. `nextval(...)` for SERIAL) rather + /// than inline on the CREATE TABLE column; folded onto the matching + /// column of the already-ingested table during pull assembly + SetColumnDefault { + table: QualifiedName, + column: String, + default: serde_json::Value, + }, CreateSchema(models::Schema), CreateDomain(models::Domain), CreateType(Box), diff --git a/src/ddl/table.rs b/src/ddl/table.rs index b44eae4..287b545 100644 --- a/src/ddl/table.rs +++ b/src/ddl/table.rs @@ -139,6 +139,30 @@ pub(crate) fn create_table( table.like_table = Some(like_table(&like_clause, src)); } } + // `CREATE TABLE x OF type (col WITH OPTIONS ..., ...)` — a typed + // table's element list is a different production (no columnDef, + // since the columns themselves come from the composite type; only + // per-column constraints via `columnOptions`, plus ordinary table + // constraints, are legal here) + for element in node.find_all("TypedTableElement") { + if let Some(column_options) = element.child_of_kind("columnOptions") { + columns.push(column(&column_options, src)); + } else if let Some(constraint) = + element.child_of_kind("TableConstraint") + { + // USING INDEX TABLESPACE, on PRIMARY KEY/UNIQUE/EXCLUDE — + // same single table-level index_tablespace as the + // TableElement loop above; first constraint carrying one wins + if table.index_tablespace.is_none() { + table.index_tablespace = constraint + .find("OptConsTableSpace") + .and_then(|n| n.find("name")) + .map(|n| unquote(n.text(src))); + } + let (name, parsed) = table_constraint(&constraint, src)?; + apply_constraint(&mut table, name, parsed); + } + } if !columns.is_empty() { table.columns = Some(columns); } @@ -321,17 +345,57 @@ pub(crate) fn alter_table( let table = qualified_name(&table, src)?; let mut statements = Vec::new(); for cmd in node.find_all("alter_table_cmd") { - if !cmd.has("kw_add") { - continue; + if cmd.has("kw_add") { + let Some(constraint) = cmd.find("TableConstraint") else { + continue; + }; + let (name, parsed) = table_constraint(&constraint, src)?; + statements.push(Statement::AddConstraint { + table: table.clone(), + name, + constraint: parsed, + }); + } else if let Some(default) = cmd.child_of_kind("alter_column_default") + && let Some(expr) = default.child_of_kind("a_expr") + { + let column = cmd + .child_of_kind("ColId") + .map(|n| unquote(n.text(src))) + .unwrap_or_default(); + statements.push(Statement::SetColumnDefault { + table: table.clone(), + column, + default: Value::String(expr.text(src).to_string()), + }); } - let Some(constraint) = cmd.find("TableConstraint") else { + } + // ATTACH PARTITION lives in a `partition_cmd` child (sibling of the + // `alter_table_cmd`s), carrying the child relation and its bounds + for cmd in node.find_all("partition_cmd") { + if !cmd.has("kw_attach") { continue; - }; - let (name, parsed) = table_constraint(&constraint, src)?; - statements.push(Statement::AddConstraint { - table: table.clone(), - name, - constraint: parsed, + } + let child = cmd + .child_of_kind("qualified_name") + .ok_or_else(|| String::from("ATTACH PARTITION without a child"))?; + let child = qualified_name(&child, src)?; + let bound = + cmd.child_of_kind("PartitionBoundSpec").ok_or_else(|| { + String::from("ATTACH PARTITION without a FOR VALUES clause") + })?; + let bound = partition_bound(&bound, src); + statements.push(Statement::AttachPartition { + parent: table.clone(), + partition: TablePartition { + name: child.name, + schema: child.schema.unwrap_or_default(), + default: bound.default, + for_values_in: bound.for_values_in, + for_values_from: bound.for_values_from, + for_values_to: bound.for_values_to, + for_values_with: bound.for_values_with, + comment: None, + }, }); } if statements.is_empty() { @@ -939,6 +1003,25 @@ mod tests { assert_eq!(constraint, TableConstraint::Check("value > 0".into())); } + #[test] + fn parses_alter_table_set_column_default() { + let statement = parse_one( + "ALTER TABLE ONLY test.t ALTER COLUMN id SET DEFAULT \ + nextval('test.t_id_seq'::regclass);", + ); + let Statement::SetColumnDefault { + table, + column, + default, + } = statement + else { + panic!("expected SetColumnDefault, got {statement:?}") + }; + assert_eq!(table.to_string(), "test.t"); + assert_eq!(column, "id"); + assert_eq!(default, json!("nextval('test.t_id_seq'::regclass)")); + } + #[test] fn quoted_identifiers_unquote() { let statement = @@ -990,6 +1073,47 @@ mod tests { assert_eq!(partition.for_values_to, Some(json!("2025-01-01"))); } + #[test] + fn parses_attach_partition() { + // pg_dump's real form: child is a plain CREATE TABLE, attached + // later via ALTER TABLE ONLY parent ATTACH PARTITION ... + let statement = parse_one( + "ALTER TABLE ONLY test.events ATTACH PARTITION \ + test.events_2024 FOR VALUES FROM ('2024-01-01') \ + TO ('2025-01-01');", + ); + let Statement::AttachPartition { parent, partition } = statement + else { + panic!("expected AttachPartition") + }; + assert_eq!(parent.to_string(), "test.events"); + assert_eq!(partition.schema, "test"); + assert_eq!(partition.name, "events_2024"); + assert_eq!(partition.for_values_from, Some(json!("2024-01-01"))); + assert_eq!(partition.for_values_to, Some(json!("2025-01-01"))); + } + + #[test] + fn parses_attach_partition_list_and_default() { + let Statement::AttachPartition { partition, .. } = parse_one( + "ALTER TABLE ONLY test.t ATTACH PARTITION test.p \ + FOR VALUES IN ('a', 'b');", + ) else { + panic!("expected AttachPartition") + }; + assert_eq!( + partition.for_values_in, + Some(vec![json!("a"), json!("b")]) + ); + + let Statement::AttachPartition { partition, .. } = parse_one( + "ALTER TABLE ONLY test.t ATTACH PARTITION test.pd DEFAULT;", + ) else { + panic!("expected AttachPartition") + }; + assert_eq!(partition.default, Some(true)); + } + #[test] fn parses_partition_of_multicolumn_bounds() { let statement = parse_one( @@ -1027,6 +1151,43 @@ mod tests { assert_eq!(table.from_type, Some("test.person_type".into())); } + #[test] + fn parses_typed_table_inline_column_constraints() { + let statement = parse_one( + "CREATE TABLE test.person OF test.person_type (\n\ + name WITH OPTIONS NOT NULL,\n\ + age WITH OPTIONS DEFAULT 0\n\ + );", + ); + let Statement::CreateTable(table) = statement else { + panic!("expected CreateTable") + }; + assert_eq!(table.from_type, Some("test.person_type".into())); + let columns = table.columns.unwrap(); + assert_eq!(columns.len(), 2); + assert_eq!(columns[0].name, "name"); + assert_eq!(columns[0].data_type, ""); + assert_eq!(columns[0].nullable, Some(false)); + assert_eq!(columns[1].name, "age"); + assert_eq!(columns[1].default, Some(json!("0"))); + } + + #[test] + fn parses_typed_table_check_constraint() { + let statement = parse_one( + "CREATE TABLE test.person OF test.person_type (\n\ + CONSTRAINT person_age_check CHECK (age >= 0)\n\ + );", + ); + let Statement::CreateTable(table) = statement else { + panic!("expected CreateTable") + }; + let constraints = table.check_constraints.unwrap(); + assert_eq!(constraints.len(), 1); + assert_eq!(constraints[0].name, "person_age_check"); + assert_eq!(constraints[0].expression, "age >= 0"); + } + #[test] fn parses_create_table_like() { let statement = parse_one( diff --git a/src/deploy/mod.rs b/src/deploy/mod.rs index 5232491..765bafe 100644 --- a/src/deploy/mod.rs +++ b/src/deploy/mod.rs @@ -12,7 +12,7 @@ mod alter; mod diff; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::io::IsTerminal; use crate::deploy::alter::Resolution; @@ -211,6 +211,16 @@ fn plan( ); } } + // dump_id -> entry, for resolving a child entry's owning items + // through intermediate non-item entries (e.g. a COMMENT ON TRIGGER + // depends on the TRIGGER entry, which is itself a child of the table + // rather than a modeled inventory item) + let entries_by_id: HashMap = output + .dump + .entries() + .iter() + .map(|entry| (entry.dump_id, entry)) + .collect(); for entry in output.dump.entries() { if args.no_privileges && entry.desc == libpgdump::ObjectType::Acl { continue; @@ -218,11 +228,25 @@ fn plan( let direct = output.item_ids.get(&entry.dump_id); let owners: Vec = match direct { Some(id) => vec![*id], - None => entry - .dependencies - .iter() - .filter_map(|dep| output.item_ids.get(dep).copied()) - .collect(), + None => { + // walk the dependency graph until it reaches inventory + // items, so comments/ACLs on child entries still map to + // the object that owns them + let mut items = Vec::new(); + let mut seen = HashSet::new(); + let mut stack: Vec = entry.dependencies.clone(); + while let Some(dep) = stack.pop() { + if !seen.insert(dep) { + continue; + } + if let Some(id) = output.item_ids.get(&dep) { + items.push(*id); + } else if let Some(parent) = entries_by_id.get(&dep) { + stack.extend(parent.dependencies.iter().copied()); + } + } + items + } }; if owners.is_empty() { continue; diff --git a/src/pull/mod.rs b/src/pull/mod.rs index 03e996a..9f24d9d 100644 --- a/src/pull/mod.rs +++ b/src/pull/mod.rs @@ -396,6 +396,15 @@ pub struct Assembly { /// PARTITION OF children whose parent table had not yet been /// ingested, replayed after the entry loop completes deferred_partitions: Vec<(QualifiedName, models::TablePartition)>, + /// ALTER TABLE ... ALTER COLUMN ... SET DEFAULT statements whose + /// table had not yet been ingested, replayed after the entry loop + /// completes + deferred_defaults: Vec<(QualifiedName, String, Value)>, + /// ALTER TABLE ... ATTACH PARTITION children, replayed after the + /// entry loop so both parent and child tables are fully ingested; + /// each child is folded into its parent's `partitions` and its + /// standalone table is removed + attached_partitions: Vec<(QualifiedName, models::TablePartition)>, /// (schema, name) -> index into `tables`, kept in sync as tables /// are ingested so index/constraint/trigger/comment merges are /// O(1) instead of a linear scan per lookup @@ -513,6 +522,8 @@ impl Assembly { | OT::Constraint | OT::FkConstraint | OT::CheckConstraint + | OT::Default + | OT::TableAttach | OT::Trigger | OT::ForeignTable | OT::ForeignDataWrapper @@ -553,6 +564,8 @@ impl Assembly { } self.apply_deferred_indexes(); self.apply_deferred_partitions(); + self.apply_deferred_defaults(); + self.apply_attached_partitions(); task.finish(); Ok(()) } @@ -587,6 +600,78 @@ impl Assembly { } } + /// Attach column defaults (`ALTER TABLE ... ALTER COLUMN ... SET + /// DEFAULT`) whose table was not yet ingested when the statement + /// was seen; warn for any that remain unresolved + fn apply_deferred_defaults(&mut self) { + for (table, column, default) in + std::mem::take(&mut self.deferred_defaults) + { + match self.find_table(&table) { + Some(t) => set_column_default(t, &column, default), + None => { + log::warn!("Column default on unknown table {table}"); + } + } + } + } + + /// Fold `ATTACH PARTITION` children into their parent tables: move + /// each child's bounds (and any table comment picked up during the + /// entry loop) into the parent's `partitions`, then drop the child's + /// standalone table so partitions are modeled only under the parent + fn apply_attached_partitions(&mut self) { + let attaches = std::mem::take(&mut self.attached_partitions); + if attaches.is_empty() { + return; + } + let mut removed = std::collections::HashSet::new(); + let mut folds = Vec::with_capacity(attaches.len()); + for (parent, mut partition) in attaches { + let key = (partition.schema.clone(), partition.name.clone()); + match self.table_index.get(&key).and_then(|&i| self.tables.get(i)) + { + Some(child) => { + if partition.comment.is_none() { + partition.comment = child.comment.clone(); + } + removed.insert(key); + } + None => log::warn!( + "ATTACH PARTITION of unknown child {}.{}", + partition.schema, + partition.name + ), + } + folds.push((parent, partition)); + } + self.tables.retain(|t| { + !removed.contains(&(t.schema.clone(), t.name.clone())) + }); + self.rebuild_table_index(); + for (parent, partition) in folds { + match self.find_table(&parent) { + Some(table) => { + table.partitions.get_or_insert_default().push(partition); + } + None => { + log::warn!("ATTACH PARTITION to unknown table {parent}") + } + } + } + } + + /// Rebuild `table_index` from the current `tables` order after a + /// removal has shifted positions + fn rebuild_table_index(&mut self) { + self.table_index = self + .tables + .iter() + .enumerate() + .map(|(i, t)| ((t.schema.clone(), t.name.clone()), i)) + .collect(); + } + /// Parse a `pg_dumpall --roles-only` SQL dump, skipping comments, /// SET statements, and psql meta-commands (PG17 wraps the output /// in `\restrict` / `\unrestrict`) @@ -655,6 +740,9 @@ impl Assembly { } } } + Statement::AttachPartition { parent, partition } => { + self.attached_partitions.push((parent, partition)); + } Statement::CreateSequence(mut sequence) => { sequence.owner = owner; self.sequences.push(sequence); @@ -737,6 +825,16 @@ impl Assembly { Some(table) => ddl::apply_constraint(table, name, constraint), None => log::warn!("Constraint on unknown table {table}"), }, + Statement::SetColumnDefault { + table, + column, + default, + } => match self.find_table(&table) { + Some(t) => set_column_default(t, &column, default), + None => { + self.deferred_defaults.push((table, column, default)); + } + }, Statement::CreateTrigger { table, trigger } => { match self.find_table(&table) { Some(table) => { @@ -933,6 +1031,7 @@ impl Assembly { .map(|v| v.comment = Some(comment.clone())) .is_some(), "FUNCTION" => self.apply_function_comment(&schema, name, &comment), + "TRIGGER" => self.apply_trigger_comment(target, &comment), "INDEX" => { match self.index_location.get(&(schema, name.clone())) { Some(&IndexLocation::Table(idx)) => self.tables[idx] @@ -1029,6 +1128,40 @@ impl Assembly { true } + /// `COMMENT ON TRIGGER trg ON schema.table` — the ddl layer puts + /// the owning table's qualified name into `target.schema` (mirrors + /// `apply_column_comment`'s two-name COMMENT shape) + fn apply_trigger_comment( + &mut self, + target: &QualifiedName, + comment: &str, + ) -> bool { + let Some(relation) = &target.schema else { + return false; + }; + let (schema, table) = match relation.split_once('.') { + Some((schema, table)) => (Some(schema.to_string()), table), + None => (None, relation.as_str()), + }; + let relation = QualifiedName { + schema, + name: table.to_string(), + }; + let Some(table) = self.find_table(&relation) else { + return false; + }; + let Some(trigger) = table + .triggers + .iter_mut() + .flatten() + .find(|t| t.name.as_deref() == Some(target.name.as_str())) + else { + return false; + }; + trigger.comment = Some(comment.to_string()); + true + } + fn find_table( &mut self, name: &QualifiedName, @@ -1262,6 +1395,29 @@ fn cancel_revokes(statements: Vec) -> Vec { } /// The quoted value from a `SET name = 'value';` entry definition +/// Set a table column's default expression, warning if the column +/// itself is not found (the table exists but not the column — should +/// not happen for well-formed pg_dump output, but is not fatal) +fn set_column_default( + table: &mut models::Table, + column: &str, + default: Value, +) { + match table + .columns + .iter_mut() + .flatten() + .find(|c| c.name == column) + { + Some(c) => c.default = Some(default), + None => log::warn!( + "Default on unknown column {column} of table {}.{}", + table.schema, + table.name + ), + } +} + fn set_value(defn: &str) -> Option { let start = defn.find('\'')? + 1; let end = defn.rfind('\'')?; @@ -1296,6 +1452,7 @@ fn extension(entry: &libpgdump::Entry) -> models::Extension { #[cfg(test)] mod tests { use libpgdump::ObjectType as OT; + use serde_json::json; use super::*; @@ -1519,6 +1676,164 @@ mod tests { } } + #[test] + fn trigger_comments_attach_to_owning_table() { + let mut dump = libpgdump::new("fixtures", "UTF8", "18.0").unwrap(); + add(&mut dump, OT::Schema, "", "test", "CREATE SCHEMA test;"); + add( + &mut dump, + OT::Table, + "test", + "users", + "CREATE TABLE test.users (id uuid NOT NULL);", + ); + add( + &mut dump, + OT::Function, + "test", + "set_last_modified()", + "CREATE FUNCTION test.set_last_modified() RETURNS trigger \ + LANGUAGE plpgsql AS $$ BEGIN RETURN NEW; END; $$;", + ); + add( + &mut dump, + OT::Trigger, + "test", + "users trg_last_modified", + "CREATE TRIGGER trg_last_modified BEFORE UPDATE ON test.users \ + FOR EACH ROW EXECUTE FUNCTION test.set_last_modified();", + ); + add( + &mut dump, + OT::Comment, + "test", + "TRIGGER trg_last_modified ON users", + "COMMENT ON TRIGGER trg_last_modified ON test.users IS \ + 'keeps last_modified_at fresh';", + ); + let mut assembly = Assembly::default(); + assembly.ingest(&dump).unwrap(); + let table = assembly + .tables + .iter() + .find(|t| t.name == "users") + .expect("users table"); + let trigger = table + .triggers + .as_ref() + .unwrap() + .iter() + .find(|t| t.name.as_deref() == Some("trg_last_modified")) + .expect("trigger"); + assert_eq!( + trigger.comment.as_deref(), + Some("keeps last_modified_at fresh") + ); + } + + #[test] + fn set_default_attaches_to_existing_column() { + let mut dump = libpgdump::new("fixtures", "UTF8", "18.0").unwrap(); + add(&mut dump, OT::Schema, "", "test", "CREATE SCHEMA test;"); + add( + &mut dump, + OT::Sequence, + "test", + "t_id_seq", + "CREATE SEQUENCE test.t_id_seq START WITH 1 INCREMENT BY 1 \ + CACHE 1;", + ); + add( + &mut dump, + OT::Table, + "test", + "t", + "CREATE TABLE test.t (id bigint NOT NULL);", + ); + add( + &mut dump, + OT::Default, + "test", + "t id", + "ALTER TABLE ONLY test.t ALTER COLUMN id SET DEFAULT \ + nextval('test.t_id_seq'::regclass);", + ); + let mut assembly = Assembly::default(); + assembly.ingest(&dump).unwrap(); + let table = assembly + .tables + .iter() + .find(|t| t.name == "t") + .expect("t table"); + let column = table + .columns + .as_ref() + .unwrap() + .iter() + .find(|c| c.name == "id") + .expect("id column"); + assert_eq!( + column.default, + Some(json!("nextval('test.t_id_seq'::regclass)")) + ); + } + + #[test] + fn deferred_default_attaches_after_table_is_ingested() { + // exercise the deferred-default retry path directly: the SET + // DEFAULT statement arrives before its table has been ingested + let mut assembly = Assembly::default(); + assembly.deferred_defaults.push(( + QualifiedName { + schema: Some("test".into()), + name: "t".into(), + }, + "id".into(), + json!("nextval('test.t_id_seq'::regclass)"), + )); + assembly.tables.push(models::Table { + name: "t".into(), + schema: "test".into(), + owner: String::new(), + sql: None, + unlogged: None, + from_type: None, + parents: None, + like_table: None, + columns: Some(vec![models::Column { + name: "id".into(), + data_type: "bigint".into(), + nullable: Some(false), + default: None, + collation: None, + check_constraint: None, + generated: None, + comment: None, + }]), + indexes: None, + primary_key: None, + check_constraints: None, + unique_constraints: None, + foreign_keys: None, + triggers: None, + partition: None, + partitions: None, + access_method: None, + storage_parameters: None, + tablespace: None, + index_tablespace: None, + server: None, + options: None, + comment: None, + }); + assembly.table_index.insert(("test".into(), "t".into()), 0); + assembly.apply_deferred_defaults(); + assert_eq!( + assembly.tables[0].columns.as_ref().unwrap()[0].default, + Some(json!("nextval('test.t_id_seq'::regclass)")) + ); + } + #[test] fn ingests_project_settings() { let assembly = assembled(); @@ -1621,6 +1936,63 @@ mod tests { ); } + #[test] + fn folds_attached_partition_children() { + // pg_dump's real form: the child is a standalone CREATE TABLE + // and a separate `TABLE ATTACH` entry carries the bounds. The + // child must be folded into the parent and dropped as a + // top-level table. + let mut dump = libpgdump::new("fixtures", "UTF8", "18.0").unwrap(); + add(&mut dump, OT::Schema, "", "test", "CREATE SCHEMA test;"); + add( + &mut dump, + OT::Table, + "test", + "events", + "CREATE TABLE test.events (id bigint, ts timestamp) \ + PARTITION BY RANGE (ts);", + ); + add( + &mut dump, + OT::Table, + "test", + "events_2024", + "CREATE TABLE test.events_2024 (id bigint, ts timestamp);", + ); + add( + &mut dump, + OT::Comment, + "test", + "TABLE events_2024", + "COMMENT ON TABLE test.events_2024 IS 'The 2024 slice';", + ); + add( + &mut dump, + OT::TableAttach, + "test", + "events_2024", + "ALTER TABLE ONLY test.events ATTACH PARTITION \ + test.events_2024 FOR VALUES FROM ('2024-01-01') \ + TO ('2025-01-01');", + ); + let mut assembly = Assembly::default(); + assembly.ingest(&dump).unwrap(); + // the child no longer stands alone; only the parent remains + assert_eq!(assembly.tables.len(), 1); + let events = &assembly.tables[0]; + assert_eq!(events.name, "events"); + let partitions = events.partitions.as_ref().expect("partitions"); + assert_eq!(partitions.len(), 1); + assert_eq!(partitions[0].name, "events_2024"); + assert_eq!( + partitions[0].for_values_from, + Some(serde_json::json!("2024-01-01")) + ); + assert_eq!(partitions[0].for_values_to, Some(json!("2025-01-01"))); + // a comment on the child carries onto the partition + assert_eq!(partitions[0].comment.as_deref(), Some("The 2024 slice")); + } + #[test] fn merges_sequence_owned_by() { let assembly = assembled(); diff --git a/src/pull/writer.rs b/src/pull/writer.rs index c7b76fb..cbe63ea 100644 --- a/src/pull/writer.rs +++ b/src/pull/writer.rs @@ -48,13 +48,39 @@ pub fn render( &value, )?; } + let relation_inventory = relation_inventory(assembly); for view in &assembly.views { - writer.save(nested("views", &view.schema, &view.name)?, view)?; + let mut value = serialize(view)?; + if let (Some(map), Some(dependencies)) = ( + value.as_object_mut(), + view_dependencies( + &view.schema, + &view.name, + view.query.as_deref(), + &relation_inventory, + ), + ) { + map.insert(String::from("dependencies"), dependencies); + } + writer + .save_value(nested("views", &view.schema, &view.name)?, &value)?; } for view in &assembly.materialized_views { - writer.save( + let mut value = serialize(view)?; + if let (Some(map), Some(dependencies)) = ( + value.as_object_mut(), + view_dependencies( + &view.schema, + &view.name, + view.query.as_deref(), + &relation_inventory, + ), + ) { + map.insert(String::from("dependencies"), dependencies); + } + writer.save_value( nested("materialized_views", &view.schema, &view.name)?, - view, + &value, )?; } writer.write_functions(assembly)?; @@ -377,6 +403,134 @@ fn table_dependencies(table: &models::Table) -> Option { (!references.is_empty()).then(|| json!({"tables": references})) } +/// A `schema.name` → dependency plural key ("tables", "views", +/// "materialized_views") map of every relation the pulled inventory +/// knows about, used to resolve a view query's relation references to +/// real, managed objects +fn relation_inventory( + assembly: &Assembly, +) -> BTreeMap<(String, String), &'static str> { + let mut inventory = BTreeMap::new(); + for table in &assembly.tables { + inventory.insert((table.schema.clone(), table.name.clone()), "tables"); + } + for view in &assembly.views { + inventory.insert((view.schema.clone(), view.name.clone()), "views"); + } + for view in &assembly.materialized_views { + inventory.insert( + (view.schema.clone(), view.name.clone()), + "materialized_views", + ); + } + inventory +} + +/// Views carry no foreign keys, so pg_dump's default same-priority +/// alphabetical sort is the only thing ordering them relative to the +/// tables/views they query; emit a `dependencies` key (mirroring +/// `table_dependencies`) so `build`/`load` order the referenced +/// relations first. Referenced relations are found by re-parsing the +/// view's `query` text and walking its CST for schema-qualified +/// `relation_expr` nodes; unqualified references (CTE names, or +/// anything the dump didn't schema-qualify) and references to objects +/// outside the pulled inventory are conservatively skipped rather than +/// guessed at. +fn view_dependencies( + schema: &str, + name: &str, + query: Option<&str>, + inventory: &BTreeMap<(String, String), &'static str>, +) -> Option { + let mut tables = BTreeSet::new(); + let mut views = BTreeSet::new(); + let mut materialized_views = BTreeSet::new(); + for (rel_schema, rel_name) in referenced_relations(query?) { + if rel_schema == schema && rel_name == name { + continue; // no self edges + } + let full = format!("{rel_schema}.{rel_name}"); + match inventory.get(&(rel_schema, rel_name)) { + Some(&"tables") => { + tables.insert(full); + } + Some(&"views") => { + views.insert(full); + } + Some(&"materialized_views") => { + materialized_views.insert(full); + } + _ => {} + } + } + if tables.is_empty() && views.is_empty() && materialized_views.is_empty() { + return None; + } + let mut object = Map::new(); + if !tables.is_empty() { + object.insert(String::from("tables"), json!(tables)); + } + if !views.is_empty() { + object.insert(String::from("views"), json!(views)); + } + if !materialized_views.is_empty() { + object.insert( + String::from("materialized_views"), + json!(materialized_views), + ); + } + Some(Value::Object(object)) +} + +/// Parse a view/materialized view's query text and return every +/// schema-qualified relation it references (`schema`, `name`), in +/// document order with duplicates kept (the caller dedupes via a +/// `BTreeSet`). Unqualified references (bare CTE names, or anything +/// pg_dump didn't schema-qualify) are skipped rather than guessed at. +/// Returns an empty list if the query fails to parse (should not +/// happen for text pg_dump itself emitted, but re-parsing is +/// defensive here rather than load-bearing). +fn referenced_relations(query: &str) -> Vec<(String, String)> { + use crate::ddl::{NodeExt, qualified_name}; + + let mut relations = Vec::new(); + let mut parser = tree_sitter::Parser::new(); + if parser + .set_language(&tree_sitter_postgres::LANGUAGE.into()) + .is_err() + { + return relations; + } + // The stored `query` is a bare SelectStmt (no trailing + // semicolon); append one to reparse it as a standalone statement, + // independent of the CREATE VIEW statement it came from. + let sql = format!("{query};"); + let Some(tree) = parser.parse(&sql, None) else { + return relations; + }; + let root = tree.root_node(); + if root.has_error() { + return relations; + } + for stmt in root.find_all("stmt") { + let Some(node) = stmt.named_child(0) else { + continue; + }; + for relation in node.find_all("relation_expr") { + let Some(qn) = relation.find("qualified_name") else { + continue; + }; + let Ok(name) = qualified_name(&qn, &sql) else { + continue; + }; + if let Some(schema) = name.schema { + relations.push((schema, name.name)); + } + } + } + relations +} + /// `user.yml` does not allow the `login` option (login is implied) fn user_options(state: &RoleState) -> Option { let mut options = state.options.clone(); @@ -659,4 +813,105 @@ mod tests { let body = &writer.files[&fn_1_path]; assert!(body.contains("name: fn_1"), "unexpected contents: {body}"); } + + #[test] + fn referenced_relations_finds_schema_qualified_table() { + let relations = referenced_relations("SELECT id FROM test.users"); + assert_eq!( + relations, + vec![(String::from("test"), String::from("users"))] + ); + } + + #[test] + fn referenced_relations_skips_unqualified_names() { + // a bare name could be a CTE, a same-schema table pg_dump + // chose not to qualify, or a system relation; conservatively + // skip anything that isn't schema-qualified + assert_eq!(referenced_relations("SELECT id FROM users"), Vec::new()); + } + + #[test] + fn referenced_relations_skips_cte_names() { + let relations = referenced_relations( + "WITH recent AS (SELECT id FROM test.orders) \ + SELECT id FROM recent", + ); + assert_eq!( + relations, + vec![(String::from("test"), String::from("orders"))] + ); + } + + fn inventory( + entries: &[(&str, &str, &'static str)], + ) -> BTreeMap<(String, String), &'static str> { + entries + .iter() + .map(|(schema, name, kind)| { + ((schema.to_string(), name.to_string()), *kind) + }) + .collect() + } + + #[test] + fn view_dependencies_emits_edge_for_known_table() { + let inv = inventory(&[("test", "users", "tables")]); + let deps = view_dependencies( + "test", + "us_users", + Some("SELECT id FROM test.users"), + &inv, + ); + assert_eq!(deps, Some(json!({"tables": ["test.users"]}))); + } + + #[test] + fn view_dependencies_emits_edge_for_known_view() { + let inv = inventory(&[("test", "base_view", "views")]); + let deps = view_dependencies( + "test", + "wrapper", + Some("SELECT id FROM test.base_view"), + &inv, + ); + assert_eq!(deps, Some(json!({"views": ["test.base_view"]}))); + } + + #[test] + fn view_dependencies_skips_unknown_relation() { + // referenced relation is schema-qualified but absent from the + // pulled inventory (a CTE the parser mis-detected, a system + // catalog, or an object outside the project) — must not + // fabricate an edge that would fail to load + let inv = inventory(&[]); + let deps = view_dependencies( + "test", + "v", + Some("SELECT id FROM test.unknown"), + &inv, + ); + assert_eq!(deps, None); + } + + #[test] + fn view_dependencies_guards_self_reference() { + // even if the view itself is (erroneously) present in the + // inventory map, referencing its own qualified name must not + // produce a self-edge + let inv = inventory(&[("test", "v", "views")]); + let deps = view_dependencies( + "test", + "v", + Some("SELECT id FROM test.v"), + &inv, + ); + assert_eq!(deps, None); + } + + #[test] + fn view_dependencies_none_without_query() { + let inv = inventory(&[("test", "users", "tables")]); + assert_eq!(view_dependencies("test", "v", None, &inv), None); + } } diff --git a/tests/build_parity.rs b/tests/build_parity.rs index 3e68d54..7d258eb 100644 --- a/tests/build_parity.rs +++ b/tests/build_parity.rs @@ -66,6 +66,17 @@ const DEVIATIONS: &[(&str, &str, &str)] = &[ // argument signature required to disambiguate it ("COMMENT", "test", "(int4 AS timestamp)"), ("COMMENT", "test", "==="), + // Python emitted CREATE/DROP FUNCTION with a bare, unqualified + // name; pg_restore runs with search_path reset to '' and rejects it + // ("no schema has been selected to create in"), so the Rust build + // schema-qualifies the function reference + ("FUNCTION", "test", "disable_alter_domain()"), + ("FUNCTION", "test", "test_aggregate(integer, integer)"), + ( + "FUNCTION", + "test", + "utf8_to_latin1(integer, integer, cstring, internal, integer)", + ), ]; /// (desc, namespace, tag, required defn fragment) for the corrected @@ -162,6 +173,24 @@ const CORRECTED: &[(&str, &str, &str, &str)] = &[ "===", "COMMENT ON OPERATOR test.=== (box, box) IS", ), + ( + "FUNCTION", + "test", + "disable_alter_domain()", + "CREATE FUNCTION test.disable_alter_domain()", + ), + ( + "FUNCTION", + "test", + "test_aggregate(integer, integer)", + "CREATE FUNCTION test.test_aggregate(integer, integer)", + ), + ( + "FUNCTION", + "test", + "utf8_to_latin1(integer, integer, cstring, internal, integer)", + "CREATE FUNCTION test.utf8_to_latin1(IN source_encoding_id INTEGER,", + ), ]; /// (tag, comment) for COMMENT ON COLUMN entries: build now emits these diff --git a/tests/deploy.rs b/tests/deploy.rs index b1e8b84..2ce9eb3 100644 --- a/tests/deploy.rs +++ b/tests/deploy.rs @@ -130,7 +130,7 @@ fn table_alters_in_place_function_or_replaces() { "function must use CREATE OR REPLACE, not drop:\n{script}" ); assert!( - script.contains("CREATE OR REPLACE FUNCTION set_last_modified"), + script.contains("CREATE OR REPLACE FUNCTION test.set_last_modified"), "missing function OR REPLACE in:\n{script}" ); assert!(