Revise the mutations API (#501)

### What

We want to make the api for mutations pretty. We make the following
changes:
1. Rename arguments
2. prefix the argument names of unique constraint columns with `key_`

### How

#### General design

* We generate delete, insert and update procedures for each table.
* A single insert procedure is generated per _table_ of the form:
  ```graphql
  <mutation-version>_insert_<table>(
      objects: [<object>],
      post_check: <boolexpr>
  )
  ```
It lets us insert multiple objects and include a post check for
permissions.
* A delete procedure is generated per _table X unique constraint_ of the
form:
  ```graphql
  <mutation-version>_delete_<table>_by_<column_and_column_and...>(
      key_<column1>: <value>,
      key_<column2>: <value>,
      ...,
      pre_check: <boolexpr>
  )
  ```
It lets us delete a single row using the uniqueness constraint, and
contains a pre check for permissions.
* An update procedure is generated per _table X unique constraint_ of
the form:
  ```graphql
  <mutation-version>_update_<table>_by_<column_and_column_and...>(
      key_<column1>: <value>,
      key_<column2>: <value>,
      ...,
      update_columns: { <column>: { _set: <value> }, ... },
      pre_check: <boolexpr>,
      post_check: <boolexpr>
  )
  ```
It lets us update a single row using the uniqueness constraint by
updating the relevant columns, and contains a pre check and post check
for permissions.
Note that the `update_columns` structure is different than the v2
version where we had `_set`, `_inc`, and other fields.
* Mutations using uniqueness constraints use the naming schema
`by_column_and_column_and_column` instead of the db constraint name,
because the former is far more helpful.
* If generating a mutation encounters an internal error, we skip that
particular mutation instead of throwing an error so the connector can
start at any situation.
* Naming collisions between the unique constraints and the
update_columns / pre_check / post_check is avoided by prefixing argument
names of the columns of a unique constraint with `key_`.

---------

Co-authored-by: Samir Talwar <samir.talwar@hasura.io>

crates/connectors/ndc-postgres/src/schema.rs

@@ -426,17 +426,18 @@ fn experimental_delete_to_procedure(
     match delete {
         mutation::experimental::delete::DeleteMutation::DeleteByKey {
             by_columns,
-            filter,
+            pre_check,
             description,
             collection_name,
+            columns_prefix,
             table_name: _,
             schema_name: _,
         } => {
             let mut arguments = BTreeMap::new();
 
             for column in by_columns {
                 arguments.insert(
-                    column.name.clone(),
+                    format!("{}{}", columns_prefix, column.name),
                     models::ArgumentInfo {
                         argument_type: column_to_type(column),
                         description: column.description.clone(),
@@ -445,12 +446,12 @@ fn experimental_delete_to_procedure(
             }
 
             arguments.insert(
-                filter.argument_name.clone(),
+                pre_check.argument_name.clone(),
                 models::ArgumentInfo {
                     argument_type: models::Type::Predicate {
                         object_type_name: collection_name.clone(),
                     },
-                    description: Some(filter.description.clone()),
+                    description: Some(pre_check.description.clone()),
                 },
             );
 
@@ -547,7 +548,7 @@ fn experimental_insert_to_procedure(
     object_types.insert(object_name.clone(), object_type);
 
     arguments.insert(
-        "_objects".to_string(),
+        insert.objects_argument_name.clone(),
         models::ArgumentInfo {
             argument_type: models::Type::Array {
                 element_type: Box::new(models::Type::Named { name: object_name }),
@@ -556,12 +557,12 @@ fn experimental_insert_to_procedure(
         },
     );
     arguments.insert(
-        insert.constraint.argument_name.clone(),
+        insert.post_check.argument_name.clone(),
         models::ArgumentInfo {
             argument_type: models::Type::Predicate {
                 object_type_name: insert.collection_name.clone(),
             },
-            description: Some(insert.constraint.description.clone()),
+            description: Some(insert.post_check.description.clone()),
         },
     );
 
@@ -591,7 +592,7 @@ fn experimental_update_to_procedure(
     // by columns arguments.
     for by_column in &update_by_key.by_columns {
         arguments.insert(
-            by_column.name.clone(),
+            format!("{}{}", update_by_key.columns_prefix, by_column.name),
             models::ArgumentInfo {
                 argument_type: column_to_type(by_column),
                 description: by_column.description.clone(),

crates/query-engine/translation/src/translation/mutation/experimental/common.rs

@@ -94,3 +94,10 @@ pub fn get_unique_constraint_name_and_key_columns(
 
     Some((constraint_name, key_columns))
 }
+
+/// The name and description of a check input argument.
+#[derive(Debug, Clone)]
+pub struct CheckArgument {
+    pub argument_name: String,
+    pub description: String,
+}

crates/query-engine/translation/src/translation/mutation/experimental/delete.rs

@@ -11,7 +11,7 @@ use query_engine_metadata::metadata::database;
 use query_engine_sql::sql;
 use std::collections::BTreeMap;
 
-use super::common;
+use super::common::{self, CheckArgument};
 
 /// A representation of an auto-generated delete mutation.
 ///
@@ -24,17 +24,11 @@ pub enum DeleteMutation {
         schema_name: sql::ast::SchemaName,
         table_name: sql::ast::TableName,
         by_columns: NonEmpty<metadata::database::ColumnInfo>,
-        filter: Filter,
+        columns_prefix: String,
+        pre_check: CheckArgument,
     },
 }
 
-/// The name and description of the filter input argument.
-#[derive(Debug, Clone)]
-pub struct Filter {
-    pub argument_name: String,
-    pub description: String,
-}
-
 /// generate a delete for each simple unique constraint on this table
 pub fn generate_delete_by_unique(
     collection_name: &String,
@@ -66,8 +60,9 @@ pub fn generate_delete_by_unique(
                 table_name: sql::ast::TableName(table_info.table_name.clone()),
                 collection_name: collection_name.clone(),
                 by_columns: key_columns,
-                filter: Filter {
-                    argument_name: "%filter".to_string(),
+                columns_prefix: "key_".to_string(),
+                pre_check: CheckArgument {
+                    argument_name: "pre_check".to_string(),
                     description: format!(
                         "Delete permission predicate over the '{collection_name}' collection"
                     ),
@@ -93,7 +88,8 @@ pub fn translate(
             schema_name,
             table_name,
             by_columns,
-            filter,
+            columns_prefix,
+            pre_check,
             description: _,
         } => {
             // The root table we are going to be deleting from.
@@ -119,7 +115,7 @@ pub fn translate(
                 .iter()
                 .map(|by_column| {
                     let unique_key = arguments
-                        .get(&by_column.name)
+                        .get(&format!("{}{}", columns_prefix, by_column.name))
                         .ok_or(Error::ArgumentNotFound(by_column.name.clone()))?;
 
                     let key_value =
@@ -139,13 +135,13 @@ pub fn translate(
                 })
                 .collect::<Result<Vec<sql::ast::Expression>, Error>>()?;
 
-            // Build the `filter` argument boolean expression.
+            // Build the `pre_check` argument boolean expression.
             let predicate_json = arguments
-                .get(&filter.argument_name)
-                .ok_or(Error::ArgumentNotFound(filter.argument_name.clone()))?;
+                .get(&pre_check.argument_name)
+                .ok_or(Error::ArgumentNotFound(pre_check.argument_name.clone()))?;
 
             let predicate: models::Expression = serde_json::from_value(predicate_json.clone())
-                .map_err(|_| Error::ArgumentNotFound(filter.argument_name.clone()))?;
+                .map_err(|_| Error::ArgumentNotFound(pre_check.argument_name.clone()))?;
 
             let predicate_expression = filtering::translate_expression(
                 env,

crates/query-engine/translation/src/translation/mutation/experimental/insert.rs

@@ -11,6 +11,8 @@ use query_engine_metadata::metadata::database;
 use query_engine_sql::sql;
 use std::collections::{BTreeMap, BTreeSet};
 
+use super::common::CheckArgument;
+
 /// A representation of an auto-generated insert mutation.
 ///
 /// This can get us `INSERT INTO <table>(<columns>) VALUES (<values>)`.
@@ -20,15 +22,9 @@ pub struct InsertMutation {
     pub description: String,
     pub schema_name: sql::ast::SchemaName,
     pub table_name: sql::ast::TableName,
+    pub objects_argument_name: String,
     pub columns: BTreeMap<String, metadata::database::ColumnInfo>,
-    pub constraint: Constraint,
-}
-
-/// The name and description of the constraint input argument.
-#[derive(Debug, Clone)]
-pub struct Constraint {
-    pub argument_name: String,
-    pub description: String,
+    pub post_check: CheckArgument,
 }
 
 /// generate an insert mutation.
@@ -46,8 +42,9 @@ pub fn generate(
         schema_name: sql::ast::SchemaName(table_info.schema_name.clone()),
         table_name: sql::ast::TableName(table_info.table_name.clone()),
         columns: table_info.columns.clone(),
-        constraint: Constraint {
-            argument_name: "constraint".to_string(),
+        objects_argument_name: "objects".to_string(),
+        post_check: CheckArgument {
+            argument_name: "post_check".to_string(),
             description: format!(
                 "Insert permission predicate over the '{collection_name}' collection"
             ),
@@ -90,14 +87,14 @@ fn translate_object_into_columns_and_values(
             }
             Ok(())
         }
-        serde_json::Value::Array(_) => Err(Error::UnexpectedStructure(
-            "array of arrays structure in insert _objects argument. Expecting an array of objects."
-                .to_string(),
-        )),
-        _ => Err(Error::UnexpectedStructure(
-            "array of values structure in insert _objects argument. Expecting an array of objects."
-                .to_string(),
-        )),
+        serde_json::Value::Array(_) => Err(Error::UnexpectedStructure(format!(
+            "array of arrays structure in insert {} argument. Expecting an array of objects.",
+            mutation.objects_argument_name
+        ))),
+        _ => Err(Error::UnexpectedStructure(format!(
+            "array of values structure in insert {} argument. Expecting an array of objects.",
+            mutation.objects_argument_name
+        ))),
     }?;
     Ok(columns_to_values)
 }
@@ -115,7 +112,7 @@ fn translate_objects_to_columns_and_values(
             let mut all_columns_and_values: Vec<
                 BTreeMap<sql::ast::ColumnName, sql::ast::MutationValueExpression>,
             > = vec![];
-            // We fetch the column names and values for each user specified object in the _objects array.
+            // We fetch the column names and values for each user specified object in the objects array.
             for object in array {
                 all_columns_and_values.push(translate_object_into_columns_and_values(
                     env, state, mutation, object,
@@ -193,14 +190,14 @@ fn translate_objects_to_columns_and_values(
                 ))
             }
         }
-        serde_json::Value::Object(_) => Err(Error::UnexpectedStructure(
-            "object structure in insert _objects argument. Expecting an array of objects."
-                .to_string(),
-        )),
-        _ => Err(Error::UnexpectedStructure(
-            "value structure in insert _objects argument. Expecting an array of objects."
-                .to_string(),
-        )),
+        serde_json::Value::Object(_) => Err(Error::UnexpectedStructure(format!(
+            "object structure in insert {} argument. Expecting an array of objects.",
+            mutation.objects_argument_name
+        ))),
+        _ => Err(Error::UnexpectedStructure(format!(
+            "value structure in insert {} argument. Expecting an array of objects.",
+            mutation.objects_argument_name
+        ))),
     }
 }
 
@@ -213,8 +210,10 @@ pub fn translate(
     arguments: &BTreeMap<String, serde_json::Value>,
 ) -> Result<(sql::ast::Insert, sql::ast::ColumnAlias), Error> {
     let object = arguments
-        .get("_objects")
-        .ok_or(Error::ArgumentNotFound("_objects".to_string()))?;
+        .get(&mutation.objects_argument_name)
+        .ok_or(Error::ArgumentNotFound(
+            mutation.objects_argument_name.clone(),
+        ))?;
 
     let (columns, from) = translate_objects_to_columns_and_values(env, state, mutation, object)?;
 
@@ -226,16 +225,16 @@ pub fn translate(
         },
     };
 
-    // Build the `constraint` argument boolean expression.
+    // Build the `post_check` argument boolean expression.
     let predicate_json =
         arguments
-            .get(&mutation.constraint.argument_name)
+            .get(&mutation.post_check.argument_name)
             .ok_or(Error::ArgumentNotFound(
-                mutation.constraint.argument_name.clone(),
+                mutation.post_check.argument_name.clone(),
             ))?;
 
     let predicate: models::Expression = serde_json::from_value(predicate_json.clone())
-        .map_err(|_| Error::ArgumentNotFound(mutation.constraint.argument_name.clone()))?;
+        .map_err(|_| Error::ArgumentNotFound(mutation.post_check.argument_name.clone()))?;
 
     let predicate_expression = filtering::translate_expression(
         env,
@@ -247,7 +246,7 @@ pub fn translate(
         &predicate,
     )?;
 
-    let check_constraint_alias =
+    let post_check_alias =
         sql::helpers::make_column_alias(sql::helpers::CHECK_CONSTRAINT_FIELD.to_string());
 
     let insert = sql::ast::Insert {
@@ -258,11 +257,11 @@ pub fn translate(
         returning: sql::ast::Returning(sql::ast::SelectList::SelectListComposite(
             Box::new(sql::ast::SelectList::SelectStar),
             Box::new(sql::ast::SelectList::SelectList(vec![(
-                check_constraint_alias.clone(),
+                post_check_alias.clone(),
                 predicate_expression,
             )])),
         )),
     };
 
-    Ok((insert, check_constraint_alias))
+    Ok((insert, post_check_alias))
 }

crates/query-engine/translation/src/translation/mutation/experimental/mod.rs

@@ -8,23 +8,23 @@
 //! * A single insert procedure is generated per table of the form:
 //!   > <version>_insert_<table>(
 //!   >     objects: [<object>],
-//!   >     constraint: <boolexpr>
+//!   >     post_check: <boolexpr>
 //!   > )
 //!   It allows us to insert multiple objects and include a post check for permissions.
 //!
 //! * A delete procedure is generated per table X unique constraint of the form:
-//!   > <version>_delete_<table>_by_<column, ...>(
-//!   >     <column1>: <value>,
-//!   >     <column2>: <value>,
+//!   > <version>_delete_<table>_by_<column_and_...>(
+//!   >     key_<column1>: <value>,
+//!   >     key_<column2>: <value>,
 //!   >     ...,
-//!   >     filter: <boolexpr>
+//!   >     pre_check: <boolexpr>
 //!   > )
 //!   It allows us to delete a single row using the uniqueness constraint, and contains a boolexpr for permissions.
 //!
 //! * An update procedure is generated per table X unique constraint of the form:
-//!   > <version>_update_<table>_by_<column, ...>(
-//!   >     <column1>: <value>,
-//!   >     <column2>: <value>,
+//!   > <version>_update_<table>_by_<column_and_...>(
+//!   >     key_<column1>: <value>,
+//!   >     key_<column2>: <value>,
 //!   >     ...,
 //!   >     update_columns: { <column>: { _set: <value> }, ... },
 //!   >     pre_check: <boolexpr>,
@@ -33,10 +33,12 @@
 //!   It allows us to update a single row using the uniqueness constraint by updating the relevant columns,
 //!   and contains a pre check and post check for permissions.
 //!
-//! * Mutations using uniqueness constraints use the naming schema `by_column_and_column_and_column` instead of the db constraint name.
-//! * If generating a mutation encounters an internal error, we skip that particular mutation instead of throwing an error so the
-//!   connector can start at any situation.
-//! * Naming collisions between the unique constraints and the update_columns / pre_check / post_check could be a problem.
+//! * Mutations using uniqueness constraints use the naming schema `by_column_and_column_and_column` instead of the db constraint name,
+//!   because the former is far more helpful.
+//! * If generating a mutation encounters an internal error, we skip that particular mutation and trace a warning instead of throwing
+//!   an error so the connector can start at any situation.
+//! * Naming collisions between the unique constraints and the update_columns / pre_check / post_check is avoided by prefixing argument
+//!   names of the columns of a unique constraint with `key_`.
 
 pub mod common;
 pub mod delete;

crates/query-engine/translation/src/translation/mutation/experimental/update.rs

@@ -30,6 +30,7 @@ pub struct UpdateByKey {
     pub schema_name: sql::ast::SchemaName,
     pub table_name: sql::ast::TableName,
     pub by_columns: NonEmpty<metadata::database::ColumnInfo>,
+    pub columns_prefix: String,
     pub update_columns_argument_name: String,
     pub pre_check: Constraint,
     pub post_check: Constraint,
@@ -74,6 +75,7 @@ pub fn generate_update_by_unique(
                 table_name: sql::ast::TableName(table_info.table_name.clone()),
                 collection_name: collection_name.clone(),
                 by_columns: key_columns,
+                columns_prefix: "key_".to_string(),
                 update_columns_argument_name: "update_columns".to_string(),
                 pre_check: Constraint {
                     argument_name: "pre_check".to_string(),
@@ -129,7 +131,7 @@ pub fn translate(
                 .iter()
                 .map(|by_column| {
                     let unique_key = arguments
-                        .get(&by_column.name)
+                        .get(&format!("{}{}", mutation.columns_prefix, by_column.name))
                         .ok_or(Error::ArgumentNotFound(by_column.name.clone()))?;
 
                     let key_value =