From 4779c559d3b8359b57c733c022b4f9e535732df1 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 11:45:05 -0400 Subject: [PATCH 1/6] Capture COMMENT ON ROLE in pull instead of dropping it apply_role_statement had no arm for Statement::Comment, so COMMENT ON ROLE emitted by pg_dumpall fell through to the unexpected-statement branch, logged a spurious warning, and was lost. Add a comment field to RoleState, populate it from the matched COMMENT ON ROLE statement, and wire it through to both the Role and User models on write (they already had a comment field, always written None). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/pull/mod.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/pull/writer.rs | 4 ++-- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/pull/mod.rs b/src/pull/mod.rs index 1a44339..e841140 100644 --- a/src/pull/mod.rs +++ b/src/pull/mod.rs @@ -279,6 +279,7 @@ pub struct RoleState { /// Whether a CREATE ROLE statement was seen; grantee-only roles /// (e.g. PUBLIC) are written with `create: false` pub created: bool, + pub comment: Option, pub options: models::RoleOptions, pub password: Option, pub valid_until: Option, @@ -723,6 +724,13 @@ impl Assembly { } } } + Statement::Comment { + on, + target, + comment, + } if on == "ROLE" => { + self.role(&target.name).comment = Some(comment); + } other => { log::warn!("Unexpected statement in roles dump: {other:?}"); self.remaining.push(Remaining { @@ -1592,6 +1600,57 @@ mod tests { assert_eq!(readonly.options.login, Some(false)); } + /// `COMMENT ON ROLE` in the roles dump is captured on the role's + /// state (rather than dropped with an "Unexpected statement" + /// warning) and carried through to both the role and user models + /// on write + #[test] + fn comment_on_role_is_captured_and_written() { + use clap::Parser; + let mut assembly = Assembly::default(); + assembly + .ingest_roles( + "CREATE ROLE app;\n\ + ALTER ROLE app WITH LOGIN;\n\ + COMMENT ON ROLE app IS 'application role';\n\ + CREATE ROLE readonly;\n\ + ALTER ROLE readonly WITH NOLOGIN;\n\ + COMMENT ON ROLE readonly IS 'read-only role';\n", + ) + .unwrap(); + assert_eq!( + assembly.roles["app"].comment.as_deref(), + Some("application role") + ); + assert_eq!( + assembly.roles["readonly"].comment.as_deref(), + Some("read-only role") + ); + + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("project"); + let dump = dir.path().join("unused.dump"); + let args = match cli::Cli::try_parse_from([ + "pglifecycle", + "pull", + "--dump", + dump.to_str().unwrap(), + dest.to_str().unwrap(), + ]) + .unwrap() + .action + { + cli::Action::Pull(args) => args, + _ => unreachable!(), + }; + let files = writer::render(&assembly, &args).unwrap(); + + let user = files.get(Path::new("users/app.yaml")).unwrap(); + assert!(user.contains("comment: application role")); + let role = files.get(Path::new("roles/readonly.yaml")).unwrap(); + assert!(role.contains("comment: read-only role")); + } + /// LOGIN roles become users, NOLOGIN roles stay roles, and reserved /// pg_* roles are dropped from the project (the bootstrap superuser /// is kept) diff --git a/src/pull/writer.rs b/src/pull/writer.rs index 543b2d1..d1621ee 100644 --- a/src/pull/writer.rs +++ b/src/pull/writer.rs @@ -239,7 +239,7 @@ impl Writer { if kind == crate::pull::RoleKind::User { let user = models::User { name: name.clone(), - comment: None, + comment: state.comment.clone(), environments: None, password: state.password.clone(), valid_until: state.valid_until.clone(), @@ -255,7 +255,7 @@ impl Writer { } else { let role = models::Role { name: name.clone(), - comment: None, + comment: state.comment.clone(), create: (!state.created).then_some(false), environments: None, grants: state.grants.to_acls(), From f4a4445c6b5f4631803ea13afff6a0e6a4024074 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 11:48:56 -0400 Subject: [PATCH 2/6] Fix drop ordering for removed functions with named parameters The removed-object drop pass in deploy keyed archive TOC entries by their literal tag (entry_key), which pg_dump renders as parameter types only. ObjectKey's function identity (function_key_name) includes parameter names when present, so a removed function with a named parameter never matched an archive entry and fell into the unordered fallback loop instead of the dependency-ordered drop pass. Add diff::function_tag_name, a type-only signature in the same shape as the archive tag, and use it (via drop_match_key) only when matching removed objects against snapshot entries for ordering. function_key_name itself is untouched, so its other callers (identity/ACL matching) are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/deploy/diff.rs | 17 +++++ src/deploy/mod.rs | 153 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 167 insertions(+), 3 deletions(-) diff --git a/src/deploy/diff.rs b/src/deploy/diff.rs index 9b93f3c..2578dc8 100644 --- a/src/deploy/diff.rs +++ b/src/deploy/diff.rs @@ -64,6 +64,23 @@ fn function_key_name(function: &crate::models::Function) -> String { format!("{}({})", function.name, args.join(", ")) } +/// The function's parameter *types* only, in the format pg_dump's +/// archive TOC tag uses (no argument names or modes). `deploy`'s +/// drop-ordering pass (`entry_key` in `mod.rs`) keys snapshot entries +/// by their literal tag, which does not carry argument names, so it +/// cannot be compared against [`function_key_name`]'s identity +/// signature directly; this gives that pass a key in the same shape. +pub(crate) fn function_tag_name(function: &crate::models::Function) -> String { + let args: Vec = function + .parameters + .iter() + .flatten() + .filter(|p| p.mode != "OUT" && p.mode != "TABLE") + .map(|p| canonical_type(&p.data_type)) + .collect(); + format!("{}({})", function.name, args.join(", ")) +} + impl std::fmt::Display for ObjectKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { if self.schema.is_empty() { diff --git a/src/deploy/mod.rs b/src/deploy/mod.rs index 4cffee0..5232491 100644 --- a/src/deploy/mod.rs +++ b/src/deploy/mod.rs @@ -167,9 +167,16 @@ fn plan( } }; // pg_dump archives are stored in dependency order, so dropping in - // reverse entry order removes dependents before dependencies - let wanted: std::collections::BTreeSet<&ObjectKey> = - diff.removed.keys().collect(); + // reverse entry order removes dependents before dependencies. + // `entry_key` derives a function's name from the archive tag + // (types only, no argument names), while `ObjectKey`'s own name + // may include them, so match through `drop_match_key`'s + // tag-shaped form rather than the removed key directly + let wanted: std::collections::BTreeMap = diff + .removed + .iter() + .map(|(key, definition)| (drop_match_key(key, definition), key)) + .collect(); let mut emitted: std::collections::BTreeSet<&ObjectKey> = std::collections::BTreeSet::new(); let ordered: Vec<&ObjectKey> = snapshot @@ -400,6 +407,21 @@ fn drop_sql(key: &ObjectKey, definition: Option<&Definition>) -> String { format!("DROP {} IF EXISTS {qualified};\n", key.desc.as_str()) } +/// The key a removed object is looked up under when matching archive +/// entries for drop ordering: functions use the tag-shaped signature +/// ([`diff::function_tag_name`]) so they compare equal to +/// `entry_key`'s output; everything else uses its own key unchanged +fn drop_match_key(key: &ObjectKey, definition: &Definition) -> ObjectKey { + match definition { + Definition::Function(f) => ObjectKey { + desc: key.desc, + schema: key.schema.clone(), + name: diff::function_tag_name(f), + }, + _ => key.clone(), + } +} + /// Map a snapshot entry to the diff key space (modeled types only); /// the schema component mirrors [`ObjectKey::new`] — empty for /// schemaless types and extensions @@ -464,3 +486,128 @@ fn source_label(args: &cli::Deploy) -> String { ), } } + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use clap::Parser; + + use super::*; + use crate::models::Function; + + fn function(json: serde_json::Value) -> Function { + serde_json::from_value(json).expect("function deserializes") + } + + /// A removed function whose database-side name (with a named + /// parameter, per `function_key_name`) diverges from the archive + /// tag it was dropped from (types only) must still be matched by + /// the ordered drop pass, so it drops before the function it + /// depends on rather than falling into the unordered fallback + /// (finding L5) + #[test] + fn removed_function_with_named_parameter_drops_in_dependency_order() { + let dep_fn = function(serde_json::json!({ + "name": "dep", + "schema": "public", + "owner": "postgres", + "returns": "integer", + "language": "sql", + "parameters": [], + })); + let main_fn = function(serde_json::json!({ + "name": "main", + "schema": "public", + "owner": "postgres", + "returns": "integer", + "language": "sql", + "parameters": [ + {"mode": "IN", "data_type": "integer", "name": "x"}, + ], + })); + let dep_key = ObjectKey::new( + constants::ObjectType::Function, + &Definition::Function(dep_fn.clone()), + ); + let main_key = ObjectKey::new( + constants::ObjectType::Function, + &Definition::Function(main_fn.clone()), + ); + // the database-side identity signature includes the + // parameter name, unlike the archive tag it must match below + assert_eq!(main_key.name, "main(x integer)"); + + let mut diff = Diff { + items: BTreeMap::new(), + changed: BTreeMap::new(), + removed: BTreeMap::new(), + }; + diff.removed + .insert(dep_key.clone(), Definition::Function(dep_fn)); + diff.removed + .insert(main_key.clone(), Definition::Function(main_fn)); + + // pg_dump archives store entries in dependency order: `dep` + // before `main`, which depends on it; the TOC tag carries + // only parameter types, never argument names + let mut snapshot = + libpgdump::new("test", "UTF8", "18.0").expect("new dump"); + let dep_id = snapshot + .add_entry( + libpgdump::ObjectType::Function, + Some("public"), + Some("dep()"), + None, + None, + None, + None, + &[], + ) + .expect("add dep entry"); + snapshot + .add_entry( + libpgdump::ObjectType::Function, + Some("public"), + Some("main(integer)"), + None, + None, + None, + None, + &[dep_id], + ) + .expect("add main entry"); + + let output = build::BuildOutput { + dump: libpgdump::new("test", "UTF8", "18.0") + .expect("new output dump"), + item_ids: std::collections::HashMap::new(), + }; + let cli = cli::Cli::parse_from([ + "pglifecycle", + "deploy", + "--allow-drop", + "proj", + ]); + let args = match cli.action { + cli::Action::Deploy(deploy) => deploy, + _ => unreachable!("parsed the deploy subcommand"), + }; + + let plan = plan(&diff, &BTreeMap::new(), &output, &snapshot, &args) + .expect("plan succeeds"); + + assert!(plan.excluded.is_empty()); + assert_eq!( + plan.included + .iter() + .map(|s| s.label.clone()) + .collect::>(), + vec![main_key.to_string(), dep_key.to_string()], + "main (the dependent) must drop before dep (its \ + dependency), which requires the removed main() to be \ + matched by the ordered pass despite its named-parameter \ + key diverging from the archive tag" + ); + } +} From 30e393836efd206753cb0f5353b008316a9fe822 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 11:49:08 -0400 Subject: [PATCH 3/6] Fix four loader safety gaps: duplicate objects, cast dependency tags, missing-name aborts, and unsupported dependency types - add_definition now rejects a second object with the same (desc, schema, name) as an error naming both file paths, instead of silently keeping the duplicate and letting build emit it twice - cache_and_remove_dependencies computes a cast's identity tag the same way Definition::name() does, so a cast's dependencies: block resolves instead of being dropped with a "missing" warning - a definition missing name now increments the error count and skips just that entry instead of aborting the rest of the load via `?` - from_plural_key gains the plural keys for aggregates, collations, event_triggers, materialized_views, publications, servers, subscriptions, users, and user_mappings, so dependencies: blocks naming those types are recognized rather than failing the load Co-Authored-By: Claude Opus 4.8 (1M context) --- src/constants.rs | 9 ++ src/project/load.rs | 194 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 191 insertions(+), 12 deletions(-) diff --git a/src/constants.rs b/src/constants.rs index cf1e97e..5ef5b71 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -136,22 +136,31 @@ impl ObjectType { /// (constants.OBJ_KEYS) pub fn from_plural_key(key: &str) -> Option { Some(match key { + "aggregates" => Self::Aggregate, "casts" => Self::Cast, + "collations" => Self::Collation, "conversions" => Self::Conversion, "domains" => Self::Domain, + "event_triggers" => Self::EventTrigger, "extensions" => Self::Extension, "foreign data wrappers" => Self::ForeignDataWrapper, "functions" => Self::Function, "groups" => Self::Group, "languages" => Self::ProceduralLanguage, + "materialized_views" => Self::MaterializedView, "operators" => Self::Operator, + "publications" => Self::Publication, "roles" => Self::Role, "sequences" => Self::Sequence, "schemata" => Self::Schema, + "servers" => Self::Server, + "subscriptions" => Self::Subscription, "tables" => Self::Table, "tablespaces" => Self::Tablespace, "text_search" => Self::TextSearch, "types" => Self::Type, + "users" => Self::User, + "user_mappings" => Self::UserMapping, "views" => Self::View, _ => return None, }) diff --git a/src/project/load.rs b/src/project/load.rs index 5316e4e..d7281c1 100644 --- a/src/project/load.rs +++ b/src/project/load.rs @@ -24,6 +24,9 @@ struct CachedDependency { pub struct Loader { project: Project, cached_dependencies: Vec, + /// File path each inventory item was loaded from, parallel to + /// `project.inventory`, kept for duplicate-object diagnostics + paths: Vec>, errors: usize, } @@ -40,6 +43,7 @@ impl Loader { inventory: Vec::new(), }, cached_dependencies: Vec::new(), + paths: Vec::new(), errors: 0, } } @@ -87,14 +91,22 @@ impl Loader { self.project.encoding = encoding.to_string(); } for entry in array_field(&project, "extensions") { - self.add_definition(ObjectType::Extension, entry); + self.add_definition(ObjectType::Extension, entry, Some(&path)); } for mut entry in array_field(&project, "foreign_data_wrappers") { inject(&mut entry, "owner", &self.project.superuser); - self.add_definition(ObjectType::ForeignDataWrapper, entry); + self.add_definition( + ObjectType::ForeignDataWrapper, + entry, + Some(&path), + ); } for entry in array_field(&project, "languages") { - self.add_definition(ObjectType::ProceduralLanguage, entry); + self.add_definition( + ObjectType::ProceduralLanguage, + entry, + Some(&path), + ); } Ok(()) } @@ -102,14 +114,20 @@ impl Loader { /// Read regular one-object-per-file definitions fn read_object_files(&mut self, ot: ObjectType) -> Result<(), String> { log::debug!("Reading {} definitions", ot.as_str()); - for (mut defn, _) in self.iterate_files(ot)? { - let name = object_name(&defn)?; + for (mut defn, path) in self.iterate_files(ot)? { + let name = match object_name(&defn) { + Ok(name) => name, + Err(_) => { + self.errors += 1; + continue; + } + }; if !validate::validate_object(&ot.schema_file(), &name, &defn) { self.errors += 1; continue; } self.cache_and_remove_dependencies(ot, &mut defn); - self.add_definition(ot, defn); + self.add_definition(ot, defn, Some(&path)); } Ok(()) } @@ -119,7 +137,7 @@ impl Loader { fn read_container_files(&mut self, ot: ObjectType) -> Result<(), String> { log::debug!("Reading {} objects", ot.as_str()); let key = ot.plural_key(); - for (mut container, _) in self.iterate_files(ot)? { + for (mut container, path) in self.iterate_files(ot)? { let container_schema = container["schema"].as_str().unwrap_or_default().to_string(); if !validate::validate_object(key, &container_schema, &container) { @@ -127,7 +145,7 @@ impl Loader { continue; } if ot == ObjectType::TextSearch { - self.add_definition(ot, container); + self.add_definition(ot, container, Some(&path)); continue; } let owner = @@ -145,7 +163,13 @@ impl Loader { entry["target_type"].as_str().unwrap_or_default() ) } else { - object_name(&entry)? + match object_name(&entry) { + Ok(name) => name, + Err(_) => { + self.errors += 1; + continue; + } + } }; if !validate::validate_object(&ot.schema_file(), &name, &entry) { @@ -153,7 +177,7 @@ impl Loader { continue; } self.cache_and_remove_dependencies(ot, &mut entry); - self.add_definition(ot, entry); + self.add_definition(ot, entry, Some(&path)); } } Ok(()) @@ -229,7 +253,17 @@ impl Loader { defn: &mut Value, ) { let namespace = defn["schema"].as_str().map(str::to_string); - let tag = defn["name"].as_str().unwrap_or_default().to_string(); + let tag = if desc == ObjectType::Cast { + // casts have no `name` key; their identity is derived from + // source/target types (see Definition::name()) + format!( + "({} AS {})", + defn["source_type"].as_str().unwrap_or_default(), + defn["target_type"].as_str().unwrap_or_default() + ) + } else { + defn["name"].as_str().unwrap_or_default().to_string() + }; if let Value::Object(deps) = defn[DEPENDENCIES].take() { for (key, names) in &deps { let Some(parent_desc) = ObjectType::from_plural_key(key) @@ -303,7 +337,12 @@ impl Loader { /// Deserialize a definition into its model and add it to the /// inventory, verifying the model round-trips to the same value - fn add_definition(&mut self, ot: ObjectType, value: Value) { + fn add_definition( + &mut self, + ot: ObjectType, + value: Value, + path: Option<&Path>, + ) { let definition = match to_definition(ot, value.clone()) { Ok(definition) => definition, Err(error) => { @@ -327,12 +366,36 @@ impl Loader { self.errors += 1; return; } + if let Some(existing) = lookup_item( + &self.project.inventory, + ot, + definition.schema(), + &definition.name(), + ) { + let existing_path = self + .paths + .get(existing) + .and_then(Option::as_ref) + .map(|p| p.display().to_string()) + .unwrap_or_else(|| String::from("")); + log::error!( + "Duplicate {} {} defined in {} (already defined in {})", + ot.as_str(), + definition.name(), + path.map(|p| p.display().to_string()) + .unwrap_or_else(|| String::from("")), + existing_path, + ); + self.errors += 1; + return; + } self.project.inventory.push(Item { id: self.project.inventory.len(), desc: ot, definition, dependencies: BTreeSet::new(), }); + self.paths.push(path.map(Path::to_path_buf)); } } @@ -468,3 +531,110 @@ fn dir_name(path: &Path) -> String { .unwrap_or_default() .to_string() } + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + /// M6: a second object with the same (desc, schema, name) is + /// rejected as an error instead of silently duplicating the item + #[test] + fn duplicate_objects_are_flagged_as_errors() { + let dir = tempfile::tempdir().unwrap(); + let users = dir.path().join("users"); + std::fs::create_dir_all(&users).unwrap(); + std::fs::write(users.join("a.yaml"), "name: dup\n").unwrap(); + std::fs::write(users.join("b.yaml"), "name: dup\n").unwrap(); + + let mut loader = Loader::new(dir.path()); + loader.read_object_files(ObjectType::User).unwrap(); + + assert_eq!(loader.errors, 1); + assert_eq!(loader.project.inventory.len(), 1); + } + + /// L7: a cast's `dependencies` block is cached under the same tag + /// `Definition::name()` computes for it, so the edge resolves + /// instead of being dropped as "missing" + #[test] + fn cast_dependency_edge_resolves() { + let mut loader = Loader::new(Path::new(".")); + loader.project.inventory.push(Item { + id: 0, + desc: ObjectType::Extension, + definition: Definition::Extension(crate::models::Extension { + name: String::from("myext"), + schema: Some(String::from("public")), + version: None, + cascade: None, + comment: None, + }), + dependencies: BTreeSet::new(), + }); + + let mut entry = json!({ + "source_type": "int4", + "target_type": "text", + "schema": "test", + "owner": "postgres", + "function": "f", + "dependencies": {"extensions": ["public.myext"]}, + }); + loader.cache_and_remove_dependencies(ObjectType::Cast, &mut entry); + assert_eq!(entry.get("dependencies"), None); + loader.add_definition(ObjectType::Cast, entry, None); + loader.apply_cached_dependencies().unwrap(); + + assert_eq!(loader.project.inventory[1].dependencies, [0].into()); + } + + /// L10: a container entry missing `name` is skipped as an error + /// rather than aborting the read of the rest of the file + #[test] + fn missing_name_skips_entry_without_aborting() { + let dir = tempfile::tempdir().unwrap(); + let operators = dir.path().join("operators"); + std::fs::create_dir_all(&operators).unwrap(); + std::fs::write( + operators.join("t.yaml"), + "operators:\n \ + - function: f1\n \ + left_arg: int4\n \ + right_arg: int4\n \ + - name: valid_op\n \ + function: f2\n \ + left_arg: int4\n \ + right_arg: int4\n", + ) + .unwrap(); + + let mut loader = Loader::new(dir.path()); + loader.read_container_files(ObjectType::Operator).unwrap(); + + assert_eq!(loader.errors, 1); + assert_eq!(loader.project.inventory.len(), 1); + assert_eq!(loader.project.inventory[0].definition.name(), "valid_op"); + } + + /// L8: dependency keys for object types omitted from + /// `from_plural_key` (e.g. servers) are recognized instead of + /// being rejected as "Unknown dependency type" + #[test] + fn previously_unsupported_dependency_type_is_recognized() { + let mut loader = Loader::new(Path::new(".")); + let mut defn = json!({ + "name": "child", + "schema": "test", + "dependencies": {"servers": ["myserver"]}, + }); + loader.cache_and_remove_dependencies(ObjectType::Table, &mut defn); + + assert_eq!(loader.errors, 0); + assert_eq!(loader.cached_dependencies.len(), 1); + let dep = &loader.cached_dependencies[0]; + assert_eq!(dep.parent_desc, ObjectType::Server); + assert_eq!(dep.parent_tag, "myserver"); + } +} From 4506f3595593b0a398882a1cb44b901d8bbca4af Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 11:50:32 -0400 Subject: [PATCH 4/6] Render from_type, partitions, and index_tablespace in build's dump_table These three Table fields were declared but never rendered, so a hand-authored or pulled value for a typed table (CREATE TABLE OF type), a partitioned table's child partitions, or a PK/UNIQUE index tablespace silently vanished from the build archive, producing an unreconcilable deploy diff (deploy::alter::table already treats all three as rebuild-triggering, but the rebuilt DDL dropped them). - from_type renders CREATE TABLE ... OF type, with column entries reduced to their WITH OPTIONS constraints per the typed-table grammar (no explicit data type). - partitions renders each child as its own archive entry: CREATE TABLE child PARTITION OF parent FOR VALUES ... / DEFAULT, depending on the parent's dump id. - index_tablespace renders USING INDEX TABLESPACE on the PRIMARY KEY constraint (or every UNIQUE constraint when there is no primary key, since the model holds one table-wide value). Added inline unit tests for each of the three renders. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/build/mod.rs | 417 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 379 insertions(+), 38 deletions(-) diff --git a/src/build/mod.rs b/src/build/mod.rs index 2b05494..75ef69d 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -35,8 +35,8 @@ use std::path::Path; use serde_json::{Map, Value}; use crate::models::{ - Column, ConstraintColumns, Definition, Index, Item, RoleOptions, - TablePartitionColumn, Trigger, ViewColumn, + Column, ConstraintColumns, Definition, Index, Item, RoleOptions, Table, + TablePartition, TablePartitionColumn, Trigger, ViewColumn, }; use crate::progress; use crate::project::Project; @@ -1079,47 +1079,55 @@ impl Builder { } create.push("TABLE".into()); create.push(self.item_name(item)); - create.push("(".into()); - if let Some(like_table) = &d.like_table { - create.push("LIKE".into()); - create.push(like_table.name.clone()); - for (field, value) in [ - ("comments", like_table.include_comments), - ("constraints", like_table.include_constraints), - ("defaults", like_table.include_defaults), - ("generated", like_table.include_generated), - ("identity", like_table.include_identity), - ("indexes", like_table.include_indexes), - ("statistics", like_table.include_statistics), - ("storage", like_table.include_storage), - ("all", like_table.include_all), - ] { - if let Some(value) = value { - create.push( - if value { "INCLUDING" } else { "EXCLUDING" } - .into(), - ); - create.push(format!("include_{field}")); - } - } - } else { + if let Some(from_type) = &d.from_type { + // typed table: columns come from the composite type, so + // only per-column constraints (via WITH OPTIONS) and + // table constraints are legal here + create.push("OF".into()); + create.push(from_type.clone()); let mut inner = Vec::new(); for column in d.columns.as_deref().unwrap_or_default() { - inner.push(render_table_column(column)); - } - for constraint in - d.unique_constraints.as_deref().unwrap_or_default() - { - inner.push(render_constraint("UNIQUE", constraint)); + if let Some(sql) = render_typed_table_column(column) { + inner.push(sql); + } } - if let Some(primary_key) = &d.primary_key { - inner.push(render_constraint("PRIMARY KEY", primary_key)); + push_table_constraints(d, &mut inner); + if !inner.is_empty() { + create.push(format!("({})", inner.join(", "))); } - for fk in d.foreign_keys.as_deref().unwrap_or_default() { - inner.push(render_foreign_key(fk)); + } else { + create.push("(".into()); + if let Some(like_table) = &d.like_table { + create.push("LIKE".into()); + create.push(like_table.name.clone()); + for (field, value) in [ + ("comments", like_table.include_comments), + ("constraints", like_table.include_constraints), + ("defaults", like_table.include_defaults), + ("generated", like_table.include_generated), + ("identity", like_table.include_identity), + ("indexes", like_table.include_indexes), + ("statistics", like_table.include_statistics), + ("storage", like_table.include_storage), + ("all", like_table.include_all), + ] { + if let Some(value) = value { + create.push( + if value { "INCLUDING" } else { "EXCLUDING" } + .into(), + ); + create.push(format!("include_{field}")); + } + } + } else { + let mut inner = Vec::new(); + for column in d.columns.as_deref().unwrap_or_default() { + inner.push(render_table_column(column)); + } + push_table_constraints(d, &mut inner); + create.push(inner.join(", ")); + create.push(")".into()); } - create.push(inner.join(", ")); - create.push(")".into()); } if let Some(parents) = &d.parents { create.push("INHERITS".into()); @@ -1163,6 +1171,56 @@ impl Builder { for trigger in d.triggers.as_deref().unwrap_or_default() { self.dump_trigger(trigger, item, d)?; } + for partition in d.partitions.as_deref().unwrap_or_default() { + self.dump_partition(item, d, partition)?; + } + Ok(()) + } + + /// A child partition folded into its parent's `partitions` list — + /// the child has no columns of its own (they come from the + /// parent), so it is emitted as its own archive entry rather than + /// through `dump_table` + fn dump_partition( + &mut self, + parent: &Item, + table: &Table, + partition: &TablePartition, + ) -> Result<(), String> { + let qualified = format!( + "{}.{}", + quote_ident(&partition.schema), + quote_ident(&partition.name) + ); + let mut create = vec![ + "CREATE TABLE".into(), + qualified.clone(), + "PARTITION OF".into(), + self.item_name(parent), + ]; + create.push(render_partition_for_values(partition)); + let drop = vec!["DROP TABLE IF EXISTS".into(), qualified]; + let parent_dump_id = self.dump_id_map[&parent.id]; + let dump_id = self.add_entry( + "TABLE", + &partition.schema, + &partition.name, + &table.owner, + &create, + &drop, + &[parent_dump_id], + None, + )?; + if let Some(comment) = &partition.comment { + self.add_comment( + "TABLE", + &partition.schema, + &partition.name, + &table.owner, + dump_id, + comment, + )?; + } Ok(()) } @@ -2038,6 +2096,106 @@ pub(crate) fn render_constraint( sql.join(" ") } +/// UNIQUE / PRIMARY KEY / FOREIGN KEY constraints shared by the plain +/// and typed (`OF type`) forms of `CREATE TABLE`. `index_tablespace` +/// selects the tablespace for the index backing a UNIQUE or PRIMARY +/// KEY constraint; the model holds one table-wide value, so it is +/// attached to the primary key when there is one, else to every +/// unique constraint +fn push_table_constraints(table: &Table, inner: &mut Vec) { + for constraint in table.unique_constraints.as_deref().unwrap_or_default() { + let mut sql = render_constraint("UNIQUE", constraint); + if table.primary_key.is_none() + && let Some(tablespace) = &table.index_tablespace + { + sql.push_str(&format!(" USING INDEX TABLESPACE {tablespace}")); + } + inner.push(sql); + } + if let Some(primary_key) = &table.primary_key { + let mut sql = render_constraint("PRIMARY KEY", primary_key); + if let Some(tablespace) = &table.index_tablespace { + sql.push_str(&format!(" USING INDEX TABLESPACE {tablespace}")); + } + inner.push(sql); + } + for fk in table.foreign_keys.as_deref().unwrap_or_default() { + inner.push(render_foreign_key(fk)); + } +} + +/// A typed table (`CREATE TABLE ... OF type`) column: the data type +/// comes from the composite type, so only constraints render, gated +/// behind `WITH OPTIONS` per the grammar; `None` when the column adds +/// no constraint (nothing to render) +fn render_typed_table_column(column: &Column) -> Option { + let mut constraints = Vec::new(); + if let Some(collation) = &column.collation { + constraints.push("COLLATE".into()); + constraints.push(collation.clone()); + } + if column.nullable == Some(false) { + constraints.push("NOT NULL".into()); + } + if let Some(check_constraint) = &column.check_constraint { + constraints.push("CHECK".into()); + constraints.push(check_constraint.clone()); + } + if let Some(default) = &column.default { + constraints.push("DEFAULT".into()); + constraints.push(render_default(default)); + } + if constraints.is_empty() { + None + } else { + Some(format!( + "{} WITH OPTIONS {}", + column.name, + constraints.join(" ") + )) + } +} + +/// The `FOR VALUES ...` (or `DEFAULT`) clause attaching a child +/// partition to its parent +fn render_partition_for_values(partition: &TablePartition) -> String { + if partition.default == Some(true) { + return "DEFAULT".into(); + } + if let Some(values) = &partition.for_values_in { + let values: Vec = values.iter().map(postgres_value).collect(); + return format!("FOR VALUES IN ({})", values.join(", ")); + } + if partition.for_values_from.is_some() || partition.for_values_to.is_some() + { + let from = partition + .for_values_from + .as_ref() + .map(render_partition_bound) + .unwrap_or_else(|| "MINVALUE".into()); + let to = partition + .for_values_to + .as_ref() + .map(render_partition_bound) + .unwrap_or_else(|| "MAXVALUE".into()); + return format!("FOR VALUES FROM ({from}) TO ({to})"); + } + if let Some(with) = &partition.for_values_with { + return format!("FOR VALUES WITH ({with})"); + } + String::new() +} + +/// A single RANGE partition bound: `MINVALUE`/`MAXVALUE` render as +/// bare keywords, everything else through the normal value quoting +fn render_partition_bound(value: &Value) -> String { + match value.as_str() { + Some(s) if s.eq_ignore_ascii_case("MINVALUE") => "MINVALUE".into(), + Some(s) if s.eq_ignore_ascii_case("MAXVALUE") => "MAXVALUE".into(), + _ => postgres_value(value), + } +} + fn render_partition_column(column: &TablePartitionColumn) -> String { match column { TablePartitionColumn::Name(name) => name.clone(), @@ -2227,4 +2385,187 @@ mod tests { EXECUTE FUNCTION test.emit()" ); } + + fn base_table(name: &str) -> Table { + Table { + name: name.into(), + schema: "test".into(), + owner: "app".into(), + sql: None, + unlogged: None, + from_type: None, + parents: None, + like_table: None, + columns: 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, + } + } + + fn table_item(id: usize, table: Table) -> Item { + Item { + id, + desc: ObjectType::Table, + definition: Definition::Table(table), + dependencies: BTreeSet::new(), + } + } + + /// Render `item` through the full builder and return the definition + /// SQL of the entry tagged `tag` with the given `desc` + fn table_defn( + item: &Item, + desc: libpgdump::ObjectType, + tag: &str, + ) -> 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(); + builder + .dump + .entries() + .iter() + .find(|e| e.desc == desc && e.tag.as_deref() == Some(tag)) + .and_then(|e| e.defn.clone()) + .unwrap_or_else(|| panic!("no {desc:?} entry tagged {tag}")) + } + + #[test] + fn renders_typed_table() { + let mut table = base_table("events"); + table.from_type = Some("test.event_type".into()); + let item = table_item(1, table); + assert_eq!( + table_defn(&item, libpgdump::ObjectType::Table, "events"), + "CREATE TABLE test.events OF test.event_type;\n" + ); + } + + #[test] + fn renders_typed_table_with_column_constraint_and_primary_key() { + let mut table = base_table("events"); + table.from_type = Some("test.event_type".into()); + table.columns = Some(vec![column("id", "bigint", true)]); + table.primary_key = + Some(ConstraintColumns::Columns(vec!["id".into()])); + let item = table_item(1, table); + assert_eq!( + table_defn(&item, libpgdump::ObjectType::Table, "events"), + "CREATE TABLE test.events OF test.event_type (id WITH OPTIONS \ + NOT NULL, PRIMARY KEY (id));\n" + ); + } + + #[test] + fn renders_primary_key_index_tablespace() { + let mut table = base_table("widgets"); + table.columns = Some(vec![column("id", "bigint", true)]); + table.primary_key = + Some(ConstraintColumns::Columns(vec!["id".into()])); + table.index_tablespace = Some("fastdisk".into()); + let item = table_item(1, table); + assert_eq!( + table_defn(&item, libpgdump::ObjectType::Table, "widgets"), + "CREATE TABLE test.widgets ( id bigint NOT NULL, PRIMARY KEY \ + (id) USING INDEX TABLESPACE fastdisk );\n" + ); + } + + #[test] + fn renders_unique_constraint_index_tablespace_without_primary_key() { + let mut table = base_table("widgets"); + table.columns = Some(vec![column("code", "text", false)]); + table.unique_constraints = + Some(vec![ConstraintColumns::Columns(vec!["code".into()])]); + table.index_tablespace = Some("fastdisk".into()); + let item = table_item(1, table); + assert_eq!( + table_defn(&item, libpgdump::ObjectType::Table, "widgets"), + "CREATE TABLE test.widgets ( code text, UNIQUE (code) USING \ + INDEX TABLESPACE fastdisk );\n" + ); + } + + #[test] + fn renders_list_partition() { + let mut table = base_table("events"); + table.columns = Some(vec![column("region", "text", true)]); + table.partitions = Some(vec![TablePartition { + name: "events_us".into(), + schema: "test".into(), + default: None, + for_values_in: Some(vec![json!("us"), json!("ca")]), + for_values_from: None, + for_values_to: None, + for_values_with: None, + comment: None, + }]); + let item = table_item(1, table); + assert_eq!( + table_defn(&item, libpgdump::ObjectType::Table, "events_us"), + "CREATE TABLE test.events_us PARTITION OF test.events FOR \ + VALUES IN ('us', 'ca');\n" + ); + } + + #[test] + fn renders_range_partition_with_open_upper_bound() { + let mut table = base_table("metrics"); + table.columns = Some(vec![column("recorded_at", "timestamptz", true)]); + table.partitions = Some(vec![TablePartition { + name: "metrics_2024".into(), + schema: "test".into(), + default: None, + for_values_in: None, + for_values_from: Some(json!("2024-01-01")), + for_values_to: Some(json!("MAXVALUE")), + for_values_with: None, + comment: None, + }]); + let item = table_item(1, table); + assert_eq!( + table_defn(&item, libpgdump::ObjectType::Table, "metrics_2024"), + "CREATE TABLE test.metrics_2024 PARTITION OF test.metrics FOR \ + VALUES FROM ('2024-01-01') TO (MAXVALUE);\n" + ); + } + + #[test] + fn renders_default_partition() { + let mut table = base_table("events"); + table.columns = Some(vec![column("region", "text", true)]); + table.partitions = Some(vec![TablePartition { + name: "events_default".into(), + schema: "test".into(), + default: Some(true), + for_values_in: None, + for_values_from: None, + for_values_to: None, + for_values_with: None, + comment: None, + }]); + let item = table_item(1, table); + assert_eq!( + table_defn(&item, libpgdump::ObjectType::Table, "events_default"), + "CREATE TABLE test.events_default PARTITION OF test.events \ + DEFAULT;\n" + ); + } } From e705c95d791c27ee6c4131a239862f3db5388e1c Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 11:50:57 -0400 Subject: [PATCH 5/6] Fix COMMENT target extraction and case-fold unquoted identifiers - COMMENT ON forms with two names (TRIGGER/RULE/POLICY ... ON ..., CONSTRAINT ... ON [DOMAIN] ...) previously had the first name overwritten by the second; both are now preserved, using the same schema/name convention as COLUMN comments (schema holds the relation, name holds the trigger/rule/policy/constraint name). - Unmatched COMMENT targets (e.g. LARGE OBJECT) now log a warning instead of silently producing an empty target. - unquote() now folds unquoted identifiers to lowercase, matching PostgreSQL's identifier folding rules; quoted identifiers are unaffected. PUBLIC is special-cased to stay uppercase since it's a keyword denoting the pseudo-role rather than a folded identifier (mirrors utils::user_mapping_subject's existing handling). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ddl/mod.rs | 35 ++++++++++++- src/ddl/object.rs | 128 +++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 148 insertions(+), 15 deletions(-) diff --git a/src/ddl/mod.rs b/src/ddl/mod.rs index fe1401e..e1b9ab8 100644 --- a/src/ddl/mod.rs +++ b/src/ddl/mod.rs @@ -374,7 +374,40 @@ pub(crate) fn qualified_name( pub(crate) fn unquote(value: &str) -> String { if value.len() >= 2 && value.starts_with('"') && value.ends_with('"') { value[1..value.len() - 1].replace("\"\"", "\"") + } else if value.eq_ignore_ascii_case("PUBLIC") { + // PUBLIC is a keyword denoting the pseudo-role, not a folded + // identifier; the rest of the codebase treats it as the + // canonical uppercase literal (see `utils::user_mapping_subject`) + String::from("PUBLIC") } else { - value.to_string() + // PostgreSQL folds unquoted identifiers to lowercase; quoted + // ones (handled above) keep their case as written + value.to_lowercase() + } +} + +#[cfg(test)] +mod unquote_tests { + use super::unquote; + + #[test] + fn unquoted_identifier_is_case_folded() { + assert_eq!(unquote("MyTable"), "mytable"); + } + + #[test] + fn quoted_identifier_keeps_case() { + assert_eq!(unquote("\"MyTable\""), "MyTable"); + } + + #[test] + fn quoted_identifier_unescapes_doubled_quotes() { + assert_eq!(unquote("\"My\"\"Table\""), "My\"Table"); + } + + #[test] + fn unquoted_public_keyword_stays_uppercase() { + assert_eq!(unquote("PUBLIC"), "PUBLIC"); + assert_eq!(unquote("public"), "PUBLIC"); } } diff --git a/src/ddl/object.rs b/src/ddl/object.rs index d66c621..257be84 100644 --- a/src/ddl/object.rs +++ b/src/ddl/object.rs @@ -315,23 +315,56 @@ pub(crate) fn comment(node: &Node, src: &str) -> Result { // the object type is the keyword sequence between ON and the name let mut object_type = Vec::new(); let mut target: Option = None; + // two-name forms (`COMMENT ON TRIGGER trg ON tbl`, `... RULE r ON + // tbl`, `... POLICY p ON tbl`, `... CONSTRAINT c ON [DOMAIN] tbl`) + // give the first name (trg/r/p/c) before the second, so `target` is + // set from the leading `name`/`ColId` and this flag marks that it + // still needs qualifying by the node that follows rather than being + // overwritten by it + let mut pending_first_name = false; let mut cursor = node.walk(); let mut past_on = false; + // once a target-bearing node has been seen, later `kw_*` children + // (e.g. `DOMAIN` in `CONSTRAINT c ON DOMAIN d`) are part of the + // target's own syntax, not the object-type keyword sequence + let mut in_target = false; for child in node.children(&mut cursor) { - match child.kind() { + let kind = child.kind(); + match kind { "kw_on" => past_on = true, "kw_is" => break, - kind if kind.starts_with("kw_") && past_on => { + _ if kind.starts_with("kw_") && past_on && !in_target => { object_type .push(kind.trim_start_matches("kw_").to_uppercase()); } // most object types nest their keywords (e.g. - // `object_type_any_name (kw_table)`) - kind if kind.starts_with("object_type") && past_on => { + // `object_type_any_name (kw_table)`), including the + // two-name forms (`object_type_name_on_any_name (kw_trigger + // | kw_rule | kw_policy)`) + _ if kind.starts_with("object_type") && past_on => { collect_keywords(&child, &mut object_type); } - "any_name" => target = Some(any_name(&child, src)), - "function_with_argtypes" => { + "any_name" | "qualified_name" | "Typename" if past_on => { + in_target = true; + let qualifier = match kind { + "qualified_name" => { + crate::ddl::qualified_name(&child, src)? + } + "Typename" => split_dotted(child.text(src)), + _ => any_name(&child, src), + }; + target = Some(if pending_first_name { + QualifiedName { + schema: Some(qualifier.to_string()), + name: target.take().unwrap_or_default().name, + } + } else { + qualifier + }); + pending_first_name = false; + } + "function_with_argtypes" if past_on => { + in_target = true; let mut name = child .find("func_name") .map(|n| any_name(&n, src)) @@ -343,23 +376,26 @@ pub(crate) fn comment(node: &Node, src: &str) -> Result { .collect(); name.name = format!("{}({})", name.name, args.join(", ")); target = Some(name); + pending_first_name = false; } - "qualified_name" => { - target = Some(crate::ddl::qualified_name(&child, src)?); - } - "Typename" => { - let text = child.text(src); - target = Some(split_dotted(text)); - } - "name" | "ColId" => { + "name" | "ColId" if past_on => { + in_target = true; target = Some(QualifiedName { schema: None, name: unquote(child.text(src)), }); + pending_first_name = true; } _ => {} } } + if target.is_none() { + log::warn!( + "Unhandled COMMENT ON {} target: {}", + object_type.join(" "), + truncate(node.text(src), 80) + ); + } Ok(Statement::Comment { on: object_type.join(" "), target: target.unwrap_or_default(), @@ -444,6 +480,70 @@ mod tests { assert_eq!(comment, "x"); } + #[test] + fn unquoted_table_name_is_case_folded() { + let Statement::CreateTable(table) = + parse_one("CREATE TABLE MyTable (id int);") + else { + panic!("expected CreateTable") + }; + assert_eq!(table.name, "mytable"); + } + + #[test] + fn quoted_table_name_keeps_case() { + let Statement::CreateTable(table) = + parse_one("CREATE TABLE \"MyTable\" (id int);") + else { + panic!("expected CreateTable") + }; + assert_eq!(table.name, "MyTable"); + } + + #[test] + fn parses_trigger_comment_preserves_both_names() { + let Statement::Comment { + on, + target, + comment, + } = parse_one("COMMENT ON TRIGGER trg ON tbl IS 'x';") + else { + panic!("expected Comment") + }; + assert_eq!(on, "TRIGGER"); + assert_eq!(target.schema.as_deref(), Some("tbl")); + assert_eq!(target.name, "trg"); + assert_eq!(comment, "x"); + } + + #[test] + fn parses_policy_comment_preserves_both_names() { + let Statement::Comment { on, target, .. } = + parse_one("COMMENT ON POLICY pol ON tbl IS 'x';") + else { + panic!("expected Comment") + }; + assert_eq!(on, "POLICY"); + assert_eq!(target.schema.as_deref(), Some("tbl")); + assert_eq!(target.name, "pol"); + } + + #[test] + fn parses_unhandled_comment_target_without_dropping_statement() { + // LARGE OBJECT comments have no name/qualified-name/Typename + // node for their numeric id, so the target falls through + // unhandled; this should log a warning (see `object.rs`) but + // still return a Comment statement with an empty target rather + // than erroring or panicking + let Statement::Comment { on, target, .. } = + parse_one("COMMENT ON LARGE OBJECT 12345 IS 'x';") + else { + panic!("expected Comment") + }; + assert_eq!(on, "LARGE OBJECT"); + assert_eq!(target, QualifiedName::default()); + } + #[test] fn parses_create_schema() { let Statement::CreateSchema(schema) = parse_one("CREATE SCHEMA test;") From c0a51e2962ec81f53d95ce30fc49522d07a4376d Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 14:12:52 -0400 Subject: [PATCH 6/6] Address CodeRabbit review: unquote PUBLIC scope, FDW dep key, typed CHECK Three verified fixes from CodeRabbit's review of PR #54: - unquote() no longer special-cases PUBLIC, which was corrupting every unquoted "public" identifier (e.g. the public schema in "public.foo") to the pseudo-role literal "PUBLIC". Pseudo-role canonicalization now lives in a role-scoped unquote_role() applied only at grantee (ACL) and USER MAPPING subject parse sites, where PUBLIC is a keyword rather than a folded identifier. - constants::from_plural_key now maps "foreign_data_wrappers" (the underscore form used everywhere else, including schemata and the writer) instead of "foreign data wrappers", so FDW dependencies in project.yaml are recognized rather than logged as unknown and skipped. - Typed-table column CHECK constraints render as CHECK (expr) to match table-level constraint rendering and produce valid SQL, instead of the bare CHECK expr that PostgreSQL rejects. Skipped two CodeRabbit findings as invalid/redundant: - Partition-bound validation in render_partition_for_values is already enforced by schemata/table.yml's oneOf on the partition bound styles, which the loader validates before build. - A guard requiring a parent partition key before emitting child partitions would add renderer-side validation of malformed input (the renderer trusts schema-validated input) and would break the existing renders_list_partition / renders_range_partition / renders_default_partition unit tests, which intentionally render child partitions without a parent partition key. Co-Authored-By: Claude Opus 4.8 --- src/build/mod.rs | 18 ++++++++++++++++-- src/constants.rs | 2 +- src/ddl/acl.rs | 4 ++-- src/ddl/foreign.rs | 4 ++-- src/ddl/mod.rs | 33 +++++++++++++++++++++++++-------- 5 files changed, 46 insertions(+), 15 deletions(-) diff --git a/src/build/mod.rs b/src/build/mod.rs index ff0242e..621f1eb 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -2237,8 +2237,7 @@ fn render_typed_table_column(column: &Column) -> Option { constraints.push("NOT NULL".into()); } if let Some(check_constraint) = &column.check_constraint { - constraints.push("CHECK".into()); - constraints.push(check_constraint.clone()); + constraints.push(format!("CHECK ({check_constraint})")); } if let Some(default) = &column.default { constraints.push("DEFAULT".into()); @@ -2706,6 +2705,21 @@ mod tests { ); } + #[test] + fn renders_typed_table_column_check_wrapped_in_parens() { + let mut table = base_table("events"); + table.from_type = Some("test.event_type".into()); + let mut id = column("id", "bigint", false); + id.check_constraint = Some("id > 0".into()); + table.columns = Some(vec![id]); + let item = table_item(1, table); + assert_eq!( + table_defn(&item, libpgdump::ObjectType::Table, "events"), + "CREATE TABLE test.events OF test.event_type (id WITH OPTIONS \ + CHECK (id > 0));\n" + ); + } + #[test] fn renders_primary_key_index_tablespace() { let mut table = base_table("widgets"); diff --git a/src/constants.rs b/src/constants.rs index 5ef5b71..bc7d2f8 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -143,7 +143,7 @@ impl ObjectType { "domains" => Self::Domain, "event_triggers" => Self::EventTrigger, "extensions" => Self::Extension, - "foreign data wrappers" => Self::ForeignDataWrapper, + "foreign_data_wrappers" => Self::ForeignDataWrapper, "functions" => Self::Function, "groups" => Self::Group, "languages" => Self::ProceduralLanguage, diff --git a/src/ddl/acl.rs b/src/ddl/acl.rs index f08fb29..15009e6 100644 --- a/src/ddl/acl.rs +++ b/src/ddl/acl.rs @@ -6,7 +6,7 @@ use tree_sitter::Node; use crate::ddl::object::{string_value, unstring}; use crate::ddl::{ Acl, AclTarget, NodeExt, Privilege, RoleDef, Statement, any_name, - qualified_name, truncate, unquote, + qualified_name, truncate, unquote, unquote_role, }; use crate::models::RoleOptions; @@ -257,7 +257,7 @@ fn role_specs(node: &Node, src: &str) -> Vec { .map(|n| { n.find_all("RoleSpec") .iter() - .map(|r| unquote(r.text(src))) + .map(|r| unquote_role(r.text(src))) .collect() }) .unwrap_or_default() diff --git a/src/ddl/foreign.rs b/src/ddl/foreign.rs index 3eab5e9..48b5ddb 100644 --- a/src/ddl/foreign.rs +++ b/src/ddl/foreign.rs @@ -7,7 +7,7 @@ use tree_sitter::Node; use crate::ddl::object::string_value; use crate::ddl::table::column; -use crate::ddl::{NodeExt, Statement, qualified_name, unquote}; +use crate::ddl::{NodeExt, Statement, qualified_name, unquote, unquote_role}; use crate::models::{ ForeignDataWrapper, Server, Table, UserMapping, UserMappingServer, }; @@ -83,7 +83,7 @@ pub(crate) fn create_user_mapping( ) -> Result { let name = node .child_of_kind("auth_ident") - .map(|a| unquote(a.text(src))) + .map(|a| unquote_role(a.text(src))) .ok_or_else(|| String::from("CREATE USER MAPPING without a user"))?; let server = node .child_of_kind("name") diff --git a/src/ddl/mod.rs b/src/ddl/mod.rs index 68b4529..a067019 100644 --- a/src/ddl/mod.rs +++ b/src/ddl/mod.rs @@ -381,11 +381,6 @@ pub(crate) fn qualified_name( pub(crate) fn unquote(value: &str) -> String { if value.len() >= 2 && value.starts_with('"') && value.ends_with('"') { value[1..value.len() - 1].replace("\"\"", "\"") - } else if value.eq_ignore_ascii_case("PUBLIC") { - // PUBLIC is a keyword denoting the pseudo-role, not a folded - // identifier; the rest of the codebase treats it as the - // canonical uppercase literal (see `utils::user_mapping_subject`) - String::from("PUBLIC") } else { // PostgreSQL folds unquoted identifiers to lowercase; quoted // ones (handled above) keep their case as written @@ -393,6 +388,19 @@ pub(crate) fn unquote(value: &str) -> String { } } +/// Like [`unquote`], but preserves the pseudo-role keyword `PUBLIC` as +/// the canonical uppercase literal. Use only for role references +/// (grantees, USER MAPPING subjects) where `PUBLIC` is a keyword rather +/// than a folded identifier; the rest of the codebase keys the +/// pseudo-role on the uppercase form (see `utils::user_mapping_subject`). +pub(crate) fn unquote_role(value: &str) -> String { + if value.eq_ignore_ascii_case("PUBLIC") { + String::from("PUBLIC") + } else { + unquote(value) + } +} + #[cfg(test)] mod unquote_tests { use super::unquote; @@ -413,8 +421,17 @@ mod unquote_tests { } #[test] - fn unquoted_public_keyword_stays_uppercase() { - assert_eq!(unquote("PUBLIC"), "PUBLIC"); - assert_eq!(unquote("public"), "PUBLIC"); + fn unquoted_public_identifier_is_case_folded() { + assert_eq!(unquote("PUBLIC"), "public"); + assert_eq!(unquote("public"), "public"); + } + + #[test] + fn unquote_role_preserves_public_keyword() { + use super::unquote_role; + assert_eq!(unquote_role("PUBLIC"), "PUBLIC"); + assert_eq!(unquote_role("public"), "PUBLIC"); + assert_eq!(unquote_role("app_user"), "app_user"); + assert_eq!(unquote_role("\"MyRole\""), "MyRole"); } }