Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make v2_ mutation prefix configurable #636

Merged
merged 10 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

- Change mutation pre/post checks to be optional

- Added `mutationPrefix` to configuration allowing mutation names to be
customised.

### Fixed

## [v1.1.2] - 2024-10-07
Expand Down
1 change: 1 addition & 0 deletions crates/configuration/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ pub struct Configuration {
pub connection_uri: String,
pub isolation_level: IsolationLevel,
pub mutations_version: Option<metadata::mutations::MutationsVersion>,
pub mutations_prefix: Option<String>,
}
pub async fn introspect(
input: ParsedConfiguration,
Expand Down
1 change: 1 addition & 0 deletions crates/configuration/src/version3/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,7 @@ pub fn make_runtime_configuration(
isolation_level: configuration.connection_settings.isolation_level,
mutations_version: convert_mutations_version(configuration.mutations_version),
configuration_version_tag: VersionTag::Version3,
mutations_prefix: None,
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub fn make_runtime_configuration(
isolation_level: parsed_config.connection_settings.isolation_level,
mutations_version: convert_mutations_version(parsed_config.mutations_version),
configuration_version_tag: VersionTag::Version4,
mutations_prefix: None,
})
}

Expand Down
5 changes: 5 additions & 0 deletions crates/configuration/src/version5/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ pub struct ParsedConfiguration {
/// Which version of the generated mutation procedures to include in the schema response
#[serde(default)]
pub mutations_version: Option<metadata::mutations::MutationsVersion>,
/// Provide a custom prefix for generated mutation names. Defaults to mutations version.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new config. serde(default) means old config that does not include this will get None, so we make sure that keeps old behaviour intact.

#[serde(default)]
pub mutations_prefix: Option<String>,
}

#[derive(Clone, PartialEq, Eq, Debug, Deserialize, Serialize, JsonSchema)]
Expand All @@ -68,6 +71,7 @@ impl ParsedConfiguration {
metadata: metadata::Metadata::default(),
introspection_options: options::IntrospectionOptions::default(),
mutations_version: Some(metadata::mutations::MutationsVersion::V2),
mutations_prefix: Some(String::new()),
}
}

Expand Down Expand Up @@ -192,6 +196,7 @@ pub async fn introspect(
},
introspection_options: args.introspection_options,
mutations_version: args.mutations_version,
mutations_prefix: args.mutations_prefix,
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub fn make_runtime_configuration(
isolation_level: parsed_config.connection_settings.isolation_level,
mutations_version: convert_mutations_version(parsed_config.mutations_version),
configuration_version_tag: VersionTag::Version4,
mutations_prefix: parsed_config.mutations_prefix,
})
}

Expand Down
1 change: 1 addition & 0 deletions crates/configuration/src/version5/upgrade_from_v4.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub fn upgrade_from_v4(v: version4::ParsedConfiguration) -> super::ParsedConfigu
introspection_options: ugrade_introspection_options(introspection_options),
metadata: upgrade_metadata(metadata),
mutations_version: mutations_version.map(upgrade_mutations_version),
mutations_prefix: Some(String::new()), // default to no prefixes
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/connectors/ndc-postgres/src/mutation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ fn plan_mutation(
operation,
request.collection_relationships.clone(),
configuration.mutations_version,
configuration.mutations_prefix.clone(),
)
})
.collect::<Result<Vec<_>, _>>()?;
Expand Down
8 changes: 7 additions & 1 deletion crates/connectors/ndc-postgres/src/schema/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,13 @@ pub fn get_schema(
.collect();

let mut more_object_types = BTreeMap::new();
let env = Env::new(metadata, BTreeMap::new(), config.mutations_version, None);
let env = Env::new(
metadata,
BTreeMap::new(),
config.mutations_version,
config.mutations_prefix.clone(),
None,
);
let generated_procedures: Vec<models::ProcedureInfo> =
query_engine_translation::translation::mutation::generate::generate(&env)
.iter()
Expand Down
4 changes: 4 additions & 0 deletions crates/query-engine/translation/src/translation/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub struct Env<'request> {
pub(crate) metadata: &'request metadata::Metadata,
relationships: BTreeMap<models::RelationshipName, models::Relationship>,
pub(crate) mutations_version: Option<metadata::mutations::MutationsVersion>,
pub(crate) mutations_prefix: Option<String>,
variables_table: Option<sql::ast::TableReference>,
}

Expand Down Expand Up @@ -208,6 +209,7 @@ impl<'request> Env<'request> {
metadata: &temp_metadata,
relationships: BTreeMap::new(),
mutations_version: None,
mutations_prefix: None,
variables_table: None,
};
f(temp_env)
Expand All @@ -218,12 +220,14 @@ impl<'request> Env<'request> {
metadata: &'request metadata::Metadata,
relationships: BTreeMap<models::RelationshipName, models::Relationship>,
mutations_version: Option<metadata::mutations::MutationsVersion>,
mutations_prefix: Option<String>,
variables_table: Option<sql::ast::TableReference>,
) -> Self {
Env {
metadata,
relationships,
mutations_version,
mutations_prefix,
variables_table,
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ pub fn generate(env: &Env) -> BTreeMap<models::ProcedureName, Mutation> {
.into_iter()
.map(|(name, mutation)| (name, Mutation::V1(mutation)))
.collect(),
Some(mutations::MutationsVersion::V2) => v2::generate(&env.metadata.tables)
.into_iter()
.map(|(name, mutation)| (name, Mutation::V2(mutation)))
.collect(),
Some(mutations::MutationsVersion::V2) => {
v2::generate(&env.metadata.tables, &env.mutations_prefix)
.into_iter()
.map(|(name, mutation)| (name, Mutation::V2(mutation)))
.collect()
}
None => BTreeMap::new(),
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,15 @@ pub fn translate(
operation: models::MutationOperation,
collection_relationships: BTreeMap<models::RelationshipName, models::Relationship>,
mutations_version: Option<metadata::mutations::MutationsVersion>,
mutations_prefix: Option<String>,
) -> Result<sql::execution_plan::Mutation, Error> {
let env = Env::new(metadata, collection_relationships, mutations_version, None);
let env = Env::new(
metadata,
collection_relationships,
mutations_version,
mutations_prefix,
None,
);

match operation {
models::MutationOperation::Procedure {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,25 @@ pub struct CheckArgument {
pub fn default_constraint() -> serde_json::Value {
serde_json::json!({"type": "and", "expressions": []})
}

// the old default was to prefix generated mutations with `v2_` or `v1_`
// but now we are able to override this
pub fn get_version_prefix(mutations_prefix: &Option<String>) -> String {
match mutations_prefix {
None => format!("{}_", super::VERSION),
Some(str) => match str.as_str() {
"" => String::new(),
_ => format!("{str}_"),
},
}
}

#[test]
fn test_version_prefix() {
assert_eq!(get_version_prefix(&None), "v2_".to_string());
assert_eq!(
get_version_prefix(&Some("horse".into())),
"horse_".to_string()
);
assert_eq!(get_version_prefix(&Some(String::new())), String::new());
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub struct DeleteByKey {
pub fn generate_delete_by_unique(
collection_name: &models::CollectionName,
table_info: &database::TableInfo,
mutations_prefix: &Option<String>,
) -> Vec<(models::ProcedureName, DeleteMutation)> {
table_info
.uniqueness_constraints
Expand All @@ -53,8 +54,8 @@ pub fn generate_delete_by_unique(
)?;

let name = format!(
"{}_delete_{collection_name}_by_{constraint_name}",
super::VERSION
"{}delete_{collection_name}_by_{constraint_name}",
common::get_version_prefix(mutations_prefix)
)
.into();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,27 @@ pub enum Mutation {
}

/// Given our introspection data, work out all the mutations we can generate
pub fn generate(tables_info: &database::TablesInfo) -> BTreeMap<models::ProcedureName, Mutation> {
pub fn generate(
tables_info: &database::TablesInfo,
mutations_prefix: &Option<String>,
) -> BTreeMap<models::ProcedureName, Mutation> {
let mut mutations = BTreeMap::new();
for (collection_name, table_info) in &tables_info.0 {
// Delete mutations.
let delete_mutations = generate_delete_by_unique(collection_name, table_info);
let delete_mutations =
generate_delete_by_unique(collection_name, table_info, mutations_prefix);
for (name, delete_mutation) in delete_mutations {
mutations.insert(name, Mutation::DeleteMutation(delete_mutation));
}

// Insert mutations.
let (name, insert_mutation) = insert::generate(collection_name, table_info);
let (name, insert_mutation) =
insert::generate(collection_name, table_info, mutations_prefix);
mutations.insert(name, Mutation::InsertMutation(insert_mutation));

// Update mutations.
let update_mutations = generate_update_by_unique(collection_name, table_info);
let update_mutations =
generate_update_by_unique(collection_name, table_info, mutations_prefix);
for (name, update_mutation) in update_mutations {
mutations.insert(name, Mutation::UpdateMutation(update_mutation));
}
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::{default_constraint, CheckArgument};
use super::common::{self, default_constraint, CheckArgument};

/// A representation of an auto-generated insert mutation.
///
Expand All @@ -31,8 +31,13 @@ pub struct InsertMutation {
pub fn generate(
collection_name: &models::CollectionName,
table_info: &database::TableInfo,
mutations_prefix: &Option<String>,
) -> (models::ProcedureName, InsertMutation) {
let name = format!("{}_insert_{collection_name}", super::VERSION).into();
let name = format!(
"{}insert_{collection_name}",
common::get_version_prefix(mutations_prefix)
)
.into();

let description = format!("Insert into the {collection_name} table");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
//!
//! * A single insert procedure is generated per table of the form:
//!
//! > v2_insert_<table>(
//! > insert_<table>(
//! > objects: [<object>],
//! > post_check: <boolexpr>
//! > )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub fn translate(
),
Error,
> {
let mutation = lookup_generated_mutation(env, procedure_name)?;
let mutation = lookup_generated_mutation(env, procedure_name, &env.mutations_prefix)?;

Ok(match mutation {
super::generate::Mutation::DeleteMutation(delete) => {
Expand Down Expand Up @@ -79,11 +79,12 @@ pub fn translate(
fn lookup_generated_mutation(
env: &Env<'_>,
procedure_name: &models::ProcedureName,
mutations_prefix: &Option<String>,
) -> Result<super::generate::Mutation, Error> {
// this means we generate them on every mutation request
// i don't think this is optimal but I'd like to get this working before working out
// where best to store these
let generated = super::generate::generate(&env.metadata.tables);
let generated = super::generate::generate(&env.metadata.tables, mutations_prefix);

generated
.get(procedure_name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub struct UpdateByKey {
pub fn generate_update_by_unique(
collection_name: &models::CollectionName,
table_info: &database::TableInfo,
mutations_prefix: &Option<String>,
) -> Vec<(models::ProcedureName, UpdateMutation)> {
table_info
.uniqueness_constraints
Expand All @@ -57,8 +58,8 @@ pub fn generate_update_by_unique(
)?;

let name = format!(
"{}_update_{collection_name}_by_{constraint_name}",
super::VERSION
"{}update_{collection_name}_by_{constraint_name}",
common::get_version_prefix(mutations_prefix)
)
.into();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub fn translate(
metadata,
query_request.collection_relationships,
None,
None,
variables_table_ref,
);

Expand Down
1 change: 1 addition & 0 deletions crates/query-engine/translation/tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ pub async fn test_mutation_translation(
operation,
request.collection_relationships.clone(),
Some(query_engine_metadata::metadata::mutations::MutationsVersion::V2),
configuration.mutations_prefix.clone(),
)
})
.collect::<Result<Vec<_>, translation::error::Error>>()?;
Expand Down
12 changes: 6 additions & 6 deletions crates/tests/databases-tests/src/postgres/explain_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,12 @@ mod mutation {
&["Insert", "Aggregate"],
result
.details
.get("0 v2_insert_custom_dog Execution Plan")
.get("0 insert_custom_dog Execution Plan")
.unwrap(),
);
insta::assert_snapshot!(result
.details
.get("0 v2_insert_custom_dog SQL Mutation")
.get("0 insert_custom_dog SQL Mutation")
.unwrap());
}

Expand All @@ -185,12 +185,12 @@ mod mutation {
],
result
.details
.get("0 v2_delete_InvoiceLine_by_InvoiceLineId Execution Plan")
.get("0 delete_InvoiceLine_by_InvoiceLineId Execution Plan")
.unwrap(),
);
insta::assert_snapshot!(result
.details
.get("0 v2_delete_InvoiceLine_by_InvoiceLineId SQL Mutation")
.get("0 delete_InvoiceLine_by_InvoiceLineId SQL Mutation")
.unwrap());
}

Expand All @@ -202,12 +202,12 @@ mod mutation {
&["Update", "Aggregate"],
result
.details
.get("1 v2_update_custom_dog_by_id Execution Plan")
.get("1 update_custom_dog_by_id Execution Plan")
.unwrap(),
);
insta::assert_snapshot!(result
.details
.get("1 v2_update_custom_dog_by_id SQL Mutation")
.get("1 update_custom_dog_by_id SQL Mutation")
.unwrap());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3123,5 +3123,6 @@ expression: default_configuration
"varchar": "string"
}
},
"mutationsVersion": "v2"
"mutationsVersion": "v2",
"mutationsPrefix": ""
}
Loading