Fix second batch of code-review findings: build, loader, deploy, pull, ddl

src/constants.rs

@@ -136,22 +136,31 @@ impl ObjectType {
     /// (constants.OBJ_KEYS)
     pub fn from_plural_key(key: &str) -> Option<Self> {
         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,
+            "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,
         })

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<String> {
         .map(|n| {
             n.find_all("RoleSpec")
                 .iter()
-                .map(|r| unquote(r.text(src)))
+                .map(|r| unquote_role(r.text(src)))
                 .collect()
         })
         .unwrap_or_default()

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<Statement, String> {
     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")

src/ddl/mod.rs

@@ -382,6 +382,56 @@ pub(crate) fn unquote(value: &str) -> String {
     if value.len() >= 2 && value.starts_with('"') && value.ends_with('"') {
         value[1..value.len() - 1].replace("\"\"", "\"")
     } else {
-        value.to_string()
+        // PostgreSQL folds unquoted identifiers to lowercase; quoted
+        // ones (handled above) keep their case as written
+        value.to_lowercase()
+    }
+}
+
+/// 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;
+
+    #[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_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");
     }
 }

src/ddl/object.rs

@@ -315,23 +315,56 @@ pub(crate) fn comment(node: &Node, src: &str) -> Result<Statement, String> {
     // the object type is the keyword sequence between ON and the name
     let mut object_type = Vec::new();
     let mut target: Option<QualifiedName> = 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<Statement, String> {
                     .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(),
@@ -486,6 +522,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;")

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<String> = 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() {