Skip to content

Commit

Permalink
Make mutation pre/post checks optional (#635)
Browse files Browse the repository at this point in the history
<!-- The PR description should answer 2 (maybe 3) important questions:
-->

### What
-
[ENG-1205](https://linear.app/hasura/issue/ENG-1205/make-mutation-precheck-and-postcheck-arguments-optional)
- 60fc380 - Change mutation pre/post
checks to be optional

<!-- What is this PR trying to accomplish (and why, if it's not
obvious)? -->

<!-- Consider: do we need to add a changelog entry? -->

---------

Co-authored-by: Daniel Harvey <[email protected]>
  • Loading branch information
codedmart and danieljharvey authored Oct 25, 2024
1 parent 352a5fa commit dc8ef3e
Show file tree
Hide file tree
Showing 8 changed files with 512 additions and 274 deletions.
2 changes: 2 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

### Changed

- Change mutation pre/post checks to be optional

### Fixed

## [v1.1.2] - 2024-10-07
Expand Down
95 changes: 47 additions & 48 deletions crates/connectors/ndc-postgres/src/schema/mutation/v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,50 +16,42 @@ pub fn delete_to_procedure(
object_types: &mut BTreeMap<models::ObjectTypeName, models::ObjectType>,
scalar_types: &mut BTreeMap<models::ScalarTypeName, models::ScalarType>,
) -> models::ProcedureInfo {
match delete {
mutation::v2::delete::DeleteMutation::DeleteByKey {
by_columns,
pre_check,
description,
collection_name,
columns_prefix,
table_name: _,
schema_name: _,
} => {
let mut arguments = BTreeMap::new();

for column in by_columns {
arguments.insert(
format!("{}{}", columns_prefix, column.name).into(),
models::ArgumentInfo {
argument_type: column_to_type(column),
description: column.description.clone(),
},
);
}

arguments.insert(
pre_check.argument_name.clone(),
models::ArgumentInfo {
argument_type: models::Type::Predicate {
object_type_name: collection_name.as_str().into(),
},
description: Some(pre_check.description.clone()),
},
);
let mutation::v2::delete::DeleteMutation::DeleteByKey(delete_by_key) = delete;

make_procedure_type(
name.clone(),
Some(description.to_string()),
arguments,
models::Type::Named {
name: collection_name.as_str().into(),
},
object_types,
scalar_types,
)
}
let mut arguments = BTreeMap::new();

for column in &delete_by_key.by_columns {
arguments.insert(
format!("{}{}", delete_by_key.columns_prefix, column.name).into(),
models::ArgumentInfo {
argument_type: column_to_type(column),
description: column.description.clone(),
},
);
}

arguments.insert(
delete_by_key.pre_check.argument_name.clone(),
models::ArgumentInfo {
argument_type: models::Type::Nullable {
underlying_type: Box::new(models::Type::Predicate {
object_type_name: delete_by_key.collection_name.as_str().into(),
}),
},
description: Some(delete_by_key.pre_check.description.clone()),
},
);

make_procedure_type(
name.clone(),
Some(delete_by_key.description.to_string()),
arguments,
models::Type::Named {
name: delete_by_key.collection_name.as_str().into(),
},
object_types,
scalar_types,
)
}

/// Given an v2 `UpdateMutation`, turn it into a `ProcedureInfo` to be output in the schema.
Expand Down Expand Up @@ -88,8 +80,10 @@ pub fn update_to_procedure(
arguments.insert(
update_by_key.pre_check.argument_name.clone(),
models::ArgumentInfo {
argument_type: models::Type::Predicate {
object_type_name: update_by_key.collection_name.as_str().into(),
argument_type: models::Type::Nullable {
underlying_type: Box::new(models::Type::Predicate {
object_type_name: update_by_key.collection_name.as_str().into(),
}),
},
description: Some(update_by_key.pre_check.description.clone()),
},
Expand All @@ -99,8 +93,10 @@ pub fn update_to_procedure(
arguments.insert(
update_by_key.post_check.argument_name.clone(),
models::ArgumentInfo {
argument_type: models::Type::Predicate {
object_type_name: update_by_key.collection_name.as_str().into(),
argument_type: models::Type::Nullable {
underlying_type: Box::new(models::Type::Predicate {
object_type_name: update_by_key.collection_name.as_str().into(),
}),
},
description: Some(update_by_key.post_check.description.clone()),
},
Expand Down Expand Up @@ -209,11 +205,14 @@ pub fn insert_to_procedure(
description: None,
},
);

arguments.insert(
insert.post_check.argument_name.clone(),
models::ArgumentInfo {
argument_type: models::Type::Predicate {
object_type_name: insert.collection_name.as_str().into(),
argument_type: models::Type::Nullable {
underlying_type: Box::new(models::Type::Predicate {
object_type_name: insert.collection_name.as_str().into(),
}),
},
description: Some(insert.post_check.description.clone()),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,8 @@ pub struct CheckArgument {
pub argument_name: models::ArgumentName,
pub description: String,
}

// Default check argument/constraint
pub fn default_constraint() -> serde_json::Value {
serde_json::json!({"type": "and", "expressions": []})
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,26 @@ use query_engine_metadata::metadata::database;
use query_engine_sql::sql;
use std::collections::BTreeMap;

use super::common::{self, CheckArgument};
use super::common::{self, default_constraint, CheckArgument};

/// A representation of an auto-generated delete mutation.
///
/// This can get us `DELETE FROM <table> WHERE column = <column_name_arg>, ...`.
#[derive(Debug, Clone)]
pub enum DeleteMutation {
DeleteByKey {
description: String,
collection_name: models::CollectionName,
schema_name: sql::ast::SchemaName,
table_name: sql::ast::TableName,
by_columns: NonEmpty<metadata::database::ColumnInfo>,
columns_prefix: String,
pre_check: CheckArgument,
},
DeleteByKey(DeleteByKey),
}

/// A representation of an auto-generated delete mutation by a unique key.
#[derive(Debug, Clone)]
pub struct DeleteByKey {
pub description: String,
pub collection_name: models::CollectionName,
pub schema_name: sql::ast::SchemaName,
pub table_name: sql::ast::TableName,
pub by_columns: NonEmpty<metadata::database::ColumnInfo>,
pub columns_prefix: String,
pub pre_check: CheckArgument,
}

/// generate a delete for each simple unique constraint on this table
Expand Down Expand Up @@ -59,7 +63,7 @@ pub fn generate_delete_by_unique(
common::description_keys(&keys.0.values().collect())
);

let delete_mutation = DeleteMutation::DeleteByKey {
let delete_mutation = DeleteMutation::DeleteByKey(DeleteByKey {
schema_name: sql::ast::SchemaName(table_info.schema_name.clone()),
table_name: sql::ast::TableName(table_info.table_name.clone()),
collection_name: collection_name.clone(),
Expand All @@ -72,7 +76,7 @@ pub fn generate_delete_by_unique(
),
},
description,
};
});

Some((name, delete_mutation))
})
Expand All @@ -83,29 +87,21 @@ pub fn generate_delete_by_unique(
pub fn translate(
env: &crate::translation::helpers::Env,
state: &mut crate::translation::helpers::State,
delete: &DeleteMutation,
mutation: &DeleteMutation,
arguments: &BTreeMap<models::ArgumentName, serde_json::Value>,
) -> Result<(sql::ast::Delete, sql::ast::ColumnAlias), Error> {
match delete {
DeleteMutation::DeleteByKey {
collection_name,
schema_name,
table_name,
by_columns,
columns_prefix,
pre_check,
description: _,
} => {
match mutation {
DeleteMutation::DeleteByKey(mutation) => {
// The root table we are going to be deleting from.
let table = sql::ast::TableReference::DBTable {
schema: schema_name.clone(),
table: table_name.clone(),
schema: mutation.schema_name.clone(),
table: mutation.table_name.clone(),
};

let table_alias = state.make_table_alias(table_name.0.clone());
let table_alias = state.make_table_alias(mutation.table_name.0.clone());

let table_name_and_reference = TableSourceAndReference {
source: helpers::TableSource::Collection(collection_name.clone()),
source: helpers::TableSource::Collection(mutation.collection_name.clone()),
reference: sql::ast::TableReference::AliasedTable(table_alias.clone()),
};

Expand All @@ -115,10 +111,12 @@ pub fn translate(
};

// Build the `UNIQUE_KEY = <value>, ...` boolean expression.
let unique_expressions = by_columns
let unique_expressions = mutation
.by_columns
.iter()
.map(|by_column| {
let argument_name = format!("{}{}", columns_prefix, by_column.name).into();
let argument_name =
format!("{}{}", mutation.columns_prefix, by_column.name).into();
let unique_key = arguments
.get(&argument_name)
.ok_or(Error::ArgumentNotFound(argument_name))?;
Expand All @@ -141,15 +139,16 @@ pub fn translate(
.collect::<Result<Vec<sql::ast::Expression>, Error>>()?;

// Build the `pre_check` argument boolean expression.
let default_constraint = default_constraint();
let predicate_json = arguments
.get(&pre_check.argument_name)
.ok_or(Error::ArgumentNotFound(pre_check.argument_name.clone()))?;
.get(&mutation.pre_check.argument_name)
.unwrap_or(&default_constraint);

let predicate: models::Expression = serde_json::from_value(predicate_json.clone())
.map_err(|_| {
Error::UnexpectedStructure(format!(
"Argument '{}' should have an ndc-spec Expression structure",
pre_check.argument_name.clone()
mutation.pre_check.argument_name.clone()
))
})?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use query_engine_metadata::metadata::database;
use query_engine_sql::sql;
use std::collections::{BTreeMap, BTreeSet};

use super::common::CheckArgument;
use super::common::{default_constraint, CheckArgument};

/// A representation of an auto-generated insert mutation.
///
Expand Down Expand Up @@ -224,12 +224,10 @@ pub fn translate(
};

// Build the `post_check` argument boolean expression.
let predicate_json =
arguments
.get(&mutation.post_check.argument_name)
.ok_or(Error::ArgumentNotFound(
mutation.post_check.argument_name.clone(),
))?;
let default_constraint = default_constraint();
let predicate_json = arguments
.get(&mutation.post_check.argument_name)
.unwrap_or(&default_constraint);

let predicate: models::Expression =
serde_json::from_value(predicate_json.clone()).map_err(|_| {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use crate::translation::helpers::{Env, State};
use ndc_models as models;
use query_engine_sql::sql;

use super::delete::DeleteByKey;

/// Translate a built-in delete mutation into an ExecutionPlan (SQL) to be run against the database.
/// This part is specialized for this mutations versions.
/// To be invoke from the main mutations translate function.
Expand All @@ -28,10 +30,10 @@ pub fn translate(
Ok(match mutation {
super::generate::Mutation::DeleteMutation(delete) => {
let return_collection = match delete {
super::delete::DeleteMutation::DeleteByKey {
super::delete::DeleteMutation::DeleteByKey(DeleteByKey {
ref collection_name,
..
} => collection_name.clone(),
}) => collection_name.clone(),
};

let (delete_cte, check_constraint_alias) =
Expand Down
Loading

0 comments on commit dc8ef3e

Please sign in to comment.