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

fix: regression on 1-1 nested connect in create #3514

Merged
merged 8 commits into from
Dec 20, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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]
}
Comment on lines +137 to +139
Copy link
Member Author

@Weakky Weakky Dec 15, 2022

Choose a reason for hiding this comment

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

Makes sure we generate optional 1-1 relation inlined on both sides (child and parent).

It's a bit hard to follow what this does without the full context. In a nutshell, RelationReference::NoRef means some of the generated datamodels will not be inlined on the child side. Therefore, the logic for the parent side will inline the relation when it's not already inlined in the child.

This means we can expect generated datamodels of these (rough) shapes:

(inlined child-side)

model Parent {
  id Int @id
  childOpt Child?
}

model Child {
  id Int @id
  parentId Int? @unique
  parentOpt? @relation(fields: [parentId], references: [id])
}

(inlined parent-side)

model Parent {
  id Int @id

  childId Int? @unique
  childOpt Child? @relation(fields: [childId], references: [id])
}

model Child {
  id Int @id
  parentOpt?
}

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
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)
}
Comment on lines +359 to +363
Copy link
Member Author

Choose a reason for hiding this comment

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

The diff of the split is not super easy to read. This code mainly removed most of the is_parent_create checks and inlined everything in separate functions.

It made it easier to only do the idempotency check on nested connect inside update.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's easier to read than before, so 👍🏾

}

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(())
}