Skip to content

Commit

Permalink
fix: regression on 1-1 nested connect in create (#3514)
Browse files Browse the repository at this point in the history
  • Loading branch information
Weakky committed Dec 20, 2022
1 parent ba103d5 commit d6e67a8
Show file tree
Hide file tree
Showing 10 changed files with 205 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,9 @@ mod connect_inside_create {
)?;

assert_error!(
runner,
format!(
r#"mutation {{
runner,
format!(
r#"mutation {{
createOneParent(data:{{
p: "p2"
p_1: "p2_1"
Expand All @@ -155,11 +155,11 @@ mod connect_inside_create {
}}
}}
}}"#,
child = child
),
2025,
"An operation failed because it depends on one or more records that were required but not found. No 'Child' record to connect was found was found for a nested connect on one-to-one relation 'ChildToParent'."
);
child = child
),
2025,
"An operation failed because it depends on one or more records that were required but not found."
);

Ok(())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ mod disconnect_inside_update {
}

// "a P1 to C1 relation " should "be disconnectable through a nested mutation by id"
#[relation_link_test(on_parent = "ToOneOpt", on_child = "ToOneOpt")]
// TODO: MongoDB doesn't support joins on top-level updates. It should be un-excluded once we fix that.
#[relation_link_test(on_parent = "ToOneOpt", on_child = "ToOneOpt", exclude(MongoDb))]
async fn p1_c1_by_fails_if_filters_no_match(runner: &Runner, t: &DatamodelWithParams) -> TestResult<()> {
let parent = t.parent().parse(
run_query_json!(
Expand Down Expand Up @@ -132,7 +133,7 @@ mod disconnect_inside_update {
where: {parent}
data:{{
p: {{ set: "p2" }}
childOpt: {{disconnect: {{ non_unique: "1" }} }}
childOpt: {{ disconnect: {{ non_unique: "1" }} }}
}}){{
childOpt {{
c
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ mod disconnect_inside_upsert {
}

// "a P1 to C1 relation " should "be disconnectable through a nested mutation by id"
#[relation_link_test(on_parent = "ToOneOpt", on_child = "ToOneOpt")]
// TODO: MongoDB doesn't support joins on top-level updates. It should be un-excluded once we fix that.
#[relation_link_test(on_parent = "ToOneOpt", on_child = "ToOneOpt", exclude(MongoDb))]
async fn p1_c1_by_fails_if_filter_no_match(runner: &Runner, t: &DatamodelWithParams) -> TestResult<()> {
let res = run_query_json!(
runner,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ fn run_relation_link_test_impl(

run_with_tokio(
async move {
tracing::debug!("Used datamodel:\n {}", datamodel.clone().yellow());
println!("Used datamodel:\n {}", datamodel.clone().yellow());
setup_project(&datamodel, Default::default()).await.unwrap();

let runner = Runner::load(config.runner(), datamodel.clone(), connector, metrics, log_capture)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::{ConnectorTag, RunnerInterface, TestResult, TxResult};
use colored::Colorize;
use query_core::{executor, schema::QuerySchemaRef, schema_builder, QueryExecutor, TxId};
use query_engine_metrics::MetricRegistry;
use request_handlers::{GraphQlBody, GraphQlHandler, MultiQuery};
Expand Down Expand Up @@ -36,6 +37,8 @@ impl RunnerInterface for DirectRunner {
}

async fn query(&self, query: String) -> TestResult<crate::QueryResult> {
println!("{}", query.bright_green());

let handler = GraphQlHandler::new(&*self.executor, &self.query_schema);
let query = GraphQlBody::Single(query.into());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,9 @@ pub fn simple_child_references<'a>(
) -> Vec<RelationReference<'a>> {
match *parent_id {
_ if on_child.is_list() && !on_parent.is_list() => vec![(RelationReference::NoRef)],
Identifier::Simple if on_child.is_to_one_opt() && on_parent.is_to_one_opt() => {
vec![RelationReference::SimpleParentId(on_child), RelationReference::NoRef]
}
Identifier::Simple => vec![RelationReference::SimpleParentId(on_child)],
Identifier::Compound => vec![RelationReference::CompoundParentId(on_child)],
Identifier::None => vec![RelationReference::ParentReference(on_child)],
Expand All @@ -150,6 +153,10 @@ pub fn full_child_references<'a>(
if !is_m2m {
match *parent_id {
_ if on_child.is_list() && !on_parent.is_list() => vec![RelationReference::NoRef],
Identifier::Simple if on_child.is_to_one_opt() && on_parent.is_to_one_opt() => {
vec![RelationReference::SimpleParentId(on_child), RelationReference::NoRef]
.clone_append(&mut common_parent_references(on_child))
}
Identifier::Simple => {
vec![RelationReference::SimpleParentId(on_child)].clone_append(&mut common_parent_references(on_child))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ impl RelationField {
}
}

pub fn is_to_one_opt(&self) -> bool {
matches!(self, Self::ToOneOpt { .. })
}

pub fn field_name(&self) -> String {
match self {
RelationField::ToOneOpt { child } => match child {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ fn render_relation_fields(

(rendered_parent, rendered_child)
} else {
let rendered_parent = format!(
let mut rendered_parent = format!(
"{} {} {}",
parent.field_name(),
parent.type_name(),
Expand All @@ -244,7 +244,20 @@ fn render_relation_fields(
);

if !child.is_list() && !parent.is_list() {
let unique_attribute = match child_ref_to_parent {
let child_unique = match child_ref_to_parent {
RelationReference::SimpleChildId(_) => r#"@@unique([childId])"#,
RelationReference::SimpleParentId(_) => r#"@@unique([parentId])"#,
RelationReference::CompoundParentId(_) => r#"@@unique([parent_id_1, parent_id_2])"#,
RelationReference::CompoundChildId(_) => r#"@@unique([child_id_1, child_id_2])"#,
RelationReference::ParentReference(_) => r#"@@unique([parentRef])"#,
RelationReference::CompoundParentReference(_) => r#"@@unique([parent_p_1, parent_p_2])"#,
RelationReference::ChildReference(_) => r#"@@unique([parent_c])"#,
RelationReference::CompoundChildReference(_) => r#"@@unique([child_c_1, child_c_2])"#,
RelationReference::IdReference => "",
RelationReference::NoRef => "",
};

let parent_unique = match parent_ref_to_child {
RelationReference::SimpleChildId(_) => r#"@@unique([childId])"#,
RelationReference::SimpleParentId(_) => r#"@@unique([parentId])"#,
RelationReference::CompoundParentId(_) => r#"@@unique([parent_id_1, parent_id_2])"#,
Expand All @@ -258,7 +271,10 @@ fn render_relation_fields(
};

rendered_child.push('\n');
rendered_child.push_str(unique_attribute);
rendered_child.push_str(child_unique);

rendered_parent.push('\n');
rendered_parent.push_str(parent_unique);
}

(rendered_parent, rendered_child)
Expand Down
184 changes: 147 additions & 37 deletions query-engine/core/src/query_graph_builder/write/nested/connect_nested.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,15 +342,10 @@ fn handle_one_to_one(
filter: Filter,
child_model: &ModelRef,
) -> QueryGraphBuilderResult<()> {
let parent_linking_fields = parent_relation_field.linking_fields();
let child_linking_fields = parent_relation_field.related_field().linking_fields();

let parent_is_create = utils::node_is_create(graph, &parent_node);
let child_relation_field = parent_relation_field.related_field();
let parent_side_required = parent_relation_field.is_required();
let child_side_required = child_relation_field.is_required();
let relation_inlined_parent = parent_relation_field.relation_is_inlined_in_parent();
let relation_inlined_child = !relation_inlined_parent;

// Build-time check
if parent_side_required && child_side_required {
Expand All @@ -361,6 +356,28 @@ fn handle_one_to_one(
));
}

if parent_is_create {
handle_one_to_one_parent_create(graph, parent_node, parent_relation_field, filter, child_model)
} else {
handle_one_to_one_parent_update(graph, parent_node, parent_relation_field, filter, child_model)
}
}

fn handle_one_to_one_parent_update(
graph: &mut QueryGraph,
parent_node: NodeRef,
parent_relation_field: &RelationFieldRef,
filter: Filter,
child_model: &ModelRef,
) -> QueryGraphBuilderResult<()> {
let child_linking_fields = parent_relation_field.related_field().linking_fields();

let child_relation_field = parent_relation_field.related_field();
let parent_side_required = parent_relation_field.is_required();
let child_side_required = child_relation_field.is_required();
let relation_inlined_parent = parent_relation_field.relation_is_inlined_in_parent();
let relation_inlined_child = !relation_inlined_parent;

let read_query = utils::read_ids_infallible(child_model.clone(), child_linking_fields.clone(), filter);
let read_new_child_node = graph.create_node(read_query);

Expand All @@ -381,42 +398,13 @@ fn handle_one_to_one(
graph.create_edge(&idempotent_check_node, &node, QueryGraphDependency::ExecutionOrder)?;
}

let relation_name = parent_relation_field.relation().name.clone();
let parent_model_name = parent_relation_field.model().name.clone();
let child_model_name = child_model.name.clone();

graph.create_edge(
&parent_node,
&read_new_child_node,
QueryGraphDependency::ProjectedDataDependency(
child_linking_fields.clone(),
Box::new(move |mut read_new_child_node, mut child_links| {
// This takes care of cases where the relation is inlined, CREATE ONLY. See doc comment for explanation.
if relation_inlined_parent && parent_is_create {
let child_link = match child_links.pop() {
Some(link) => Ok(link),
None => Err(QueryGraphBuilderError::RecordNotFound(format!(
"No '{}' record (needed to inline connect on create for '{}' record) was found for a nested connect on one-to-one relation '{}'.",
child_model_name, parent_model_name, relation_name
))),
}?;


if let Node::Query(Query::Write(ref mut wq)) = read_new_child_node {
wq.inject_result_into_args(parent_linking_fields.assimilate(child_link)?);
}
}

Ok(read_new_child_node)
}),
),
)?;
graph.create_edge(&parent_node, &read_new_child_node, QueryGraphDependency::ExecutionOrder)?;

// Finally, insert the check for (and possible disconnect of) an existing child record.
// Those checks are performed on the parent node model.
// We only need to do those checks if the parent operation is not a create, the reason being that
// if the parent is a create, it can't have an existing child already.
if !parent_is_create && (child_side_required || !relation_inlined_parent) {
if child_side_required || !relation_inlined_parent {
let node = utils::insert_existing_1to1_related_model_checks(graph, &parent_node, parent_relation_field)?;

// We do those checks only if the old & new child are different.
Expand Down Expand Up @@ -488,7 +476,7 @@ fn handle_one_to_one(
Ok(update_children_node)
})),
)?;
} else if relation_inlined_parent && !parent_is_create {
} else if relation_inlined_parent {
// Relation is inlined on the parent and a non-create.
// Create an update node for parent record to set the connection to the child.
let parent_model = parent_relation_field.model();
Expand Down Expand Up @@ -556,3 +544,125 @@ fn handle_one_to_one(

Ok(())
}

fn handle_one_to_one_parent_create(
graph: &mut QueryGraph,
parent_node: NodeRef,
parent_relation_field: &RelationFieldRef,
filter: Filter,
child_model: &ModelRef,
) -> QueryGraphBuilderResult<()> {
let parent_linking_fields = parent_relation_field.linking_fields();
let child_linking_fields = parent_relation_field.related_field().linking_fields();

let child_relation_field = parent_relation_field.related_field();
let parent_side_required = parent_relation_field.is_required();
let relation_inlined_parent = parent_relation_field.relation_is_inlined_in_parent();
let relation_inlined_child = !relation_inlined_parent;

let read_query = utils::read_ids_infallible(child_model.clone(), child_linking_fields.clone(), filter);
let read_new_child_node = graph.create_node(read_query);

// We always start with the read node in a nested connect 1:1 scenario.
graph.mark_nodes(&parent_node, &read_new_child_node);

// Next is the check for (and possible disconnect of) an existing parent.
// Those checks are performed on the new child node, hence we use the child relation field side ("backrelation").
if parent_side_required || relation_inlined_parent {
utils::insert_existing_1to1_related_model_checks(graph, &read_new_child_node, &child_relation_field)?;
}

let relation_name = parent_relation_field.relation().name.clone();
let parent_model_name = parent_relation_field.model().name.clone();
let child_model_name = child_model.name.clone();

graph.create_edge(
&parent_node,
&read_new_child_node,
QueryGraphDependency::ProjectedDataDependency(
child_linking_fields.clone(),
Box::new(move |mut read_new_child_node, mut child_links| {
// This takes care of cases where the relation is inlined, CREATE ONLY. See doc comment for explanation.
if relation_inlined_parent {
let child_link = match child_links.pop() {
Some(link) => Ok(link),
None => Err(QueryGraphBuilderError::RecordNotFound(format!(
"No '{}' record (needed to inline connect on create for '{}' record) was found for a nested connect on one-to-one relation '{}'.",
child_model_name, parent_model_name, relation_name
))),
}?;


if let Node::Query(Query::Write(ref mut wq)) = read_new_child_node {
wq.inject_result_into_args(parent_linking_fields.assimilate(child_link)?);
}
}

Ok(read_new_child_node)
}),
),
)?;

// If the relation is inlined on the child, we also need to update the child to connect it to the parent.
if relation_inlined_child {
let update_children_node =
utils::update_records_node_placeholder(graph, Filter::empty(), Arc::clone(child_model));

let parent_linking_fields = parent_relation_field.linking_fields();
let child_linking_fields = parent_relation_field.related_field().linking_fields();
let child_model_identifier = parent_relation_field.related_field().model().primary_identifier();
let relation_name = parent_relation_field.relation().name.clone();
let child_model_name = child_model.name.clone();

graph.create_edge(
&read_new_child_node,
&update_children_node,
QueryGraphDependency::ProjectedDataDependency(
child_model_identifier,
Box::new(move |mut update_children_node, mut child_ids| {
let child_id = match child_ids.pop() {
Some(pid) => Ok(pid),
None => Err(QueryGraphBuilderError::RecordNotFound(format!(
"No '{}' record to connect was found was found for a nested connect on one-to-one relation '{}'.",
child_model_name, relation_name
))),
}?;

if let Node::Query(Query::Write(ref mut wq)) = update_children_node {
wq.add_filter(child_id.filter());
}

Ok(update_children_node)
}),
),
)?;

let relation_name = parent_relation_field.relation().name.clone();
let parent_model_name = parent_relation_field.model().name.clone();
let child_model_name = child_model.name.clone();

graph.create_edge(
&parent_node,
&update_children_node,
QueryGraphDependency::ProjectedDataDependency(parent_linking_fields, Box::new(move |mut update_children_node, mut parent_links| {
let parent_link = match parent_links.pop() {
Some(link) => Ok(link),
None => Err(QueryGraphBuilderError::RecordNotFound(format!(
"No '{}' record (needed to update inlined relation on '{}') was found for a nested connect on one-to-one relation '{}'.",
parent_model_name,
child_model_name,
relation_name
))),
}?;

if let Node::Query(Query::Write(ref mut wq)) = update_children_node {
wq.inject_result_into_args(child_linking_fields.assimilate(parent_link)?);
}

Ok(update_children_node)
})),
)?;
}

Ok(())
}

0 comments on commit d6e67a8

Please sign in to comment.