From f9fa1533a834d98926ab48696fd7987ceeef57aa Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Mon, 4 Dec 2023 10:36:16 +0100 Subject: [PATCH] fix(qe): make `UpdateRecord::WithSelection` nodes return `RecordSelection` (#4508) Query graph nodes that are not result nodes and fulfill the requirements of other nodes don't inherently need names because their results aren't serialized. However, we still need some name to be able to build RecordSelection because it's required there. Previously the update node with selection that has no name would return a QueryResult::Id instead of QueryResult::RecordSelection to work around that, but it caused problems as the query interpreter relied on QueryResult::Id being the primary identifier and not arbitrary fields. This PR makes the name required for UpdateRecord::WithSelection and provides a stub name when the node doesn't directly correspond to the client's query. This is consistent with other intermediate nodes that need to build RecordSelection. Some of them use constant placeholder names (like find_children_by_parent for RelatedRecordsQuery), some of them use empty strings (e.g. CreateRecord does this). I chose an empty string because it needs to be owned here and an empty String::new() doesn't allocate memory, and the specific value doesn't matter anyway because there's likely not even a way to observe this name (it's not used in Display and ToGraphviz impls for UpdateRecord). Fixes: https://github.com/prisma/prisma/issues/21182 --- nix/shell.nix | 1 + .../tests/new/regressions/mod.rs | 1 + .../tests/new/regressions/prisma_21182.rs | 98 +++++++++++++++++++ .../interpreter/query_interpreters/write.rs | 11 +-- query-engine/core/src/query_ast/write.rs | 9 +- .../src/query_graph_builder/write/update.rs | 4 +- 6 files changed, 110 insertions(+), 14 deletions(-) create mode 100644 query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_21182.rs diff --git a/nix/shell.nix b/nix/shell.nix index 8cde4e9c4a7..792f9a6540f 100644 --- a/nix/shell.nix +++ b/nix/shell.nix @@ -15,6 +15,7 @@ in nodejs.pkgs.pnpm jq + graphviz wasm-bindgen-cli wasm-pack ]; diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs index 252ba620b5f..2cd2938f916 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs @@ -19,6 +19,7 @@ mod prisma_16760; mod prisma_17103; mod prisma_18517; mod prisma_20799; +mod prisma_21182; mod prisma_21369; mod prisma_21901; mod prisma_5952; diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_21182.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_21182.rs new file mode 100644 index 00000000000..c89b2ecc84c --- /dev/null +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_21182.rs @@ -0,0 +1,98 @@ +use query_engine_tests::*; + +/// Regression test for https://github.com/prisma/prisma/issues/21182 +#[test_suite(schema(schema))] +mod regression { + use indoc::indoc; + + fn schema() -> String { + indoc! {r#" + model User { + #id(id, Int, @id) + email String @unique + name String? + roleId Int + role Role @relation(fields: [roleId], references: [id]) + } + + model Role { + #id(id, Int, @id) + name String + users User[] + + tagId Int? + tag Tag? @relation(fields: [tagId], references: [id]) + } + + model Tag { + #id(id, Int, @id) + name String + roles Role[] + } + "#} + .to_owned() + } + + #[connector_test] + async fn query_with_normalized_dependencies(runner: Runner) -> TestResult<()> { + run_query!( + runner, + indoc! {r#" + mutation { + createOneUser ( + data: { + id: 1, + email: "user@prisma.io", + role: { + create: { + id: 1, + name: "ADMIN", + tag: { + create: { + id: 1, + name: "cs" + } + } + } + } + } + ) { + id, + email, + roleId + } + } + "#} + ); + + run_query!( + runner, + indoc! {r#" + mutation { + updateOneUser( + where: { + email: "user@prisma.io", + }, + data: { + role: { + update: { + data: { + tag: { + disconnect: true + } + } + } + } + } + ) { + id, + email, + roleId + } + } + "#} + ); + + Ok(()) + } +} diff --git a/query-engine/core/src/interpreter/query_interpreters/write.rs b/query-engine/core/src/interpreter/query_interpreters/write.rs index 093c528298c..1e0a7d34844 100644 --- a/query-engine/core/src/interpreter/query_interpreters/write.rs +++ b/query-engine/core/src/interpreter/query_interpreters/write.rs @@ -81,10 +81,10 @@ async fn update_one( .await?; match q { - UpdateRecord::WithSelection(q) if q.name.is_some() => { + UpdateRecord::WithSelection(q) => { let res = res .map(|res| RecordSelection { - name: q.name.unwrap(), + name: q.name, fields: q.selection_order, scalars: res.into(), nested: vec![], @@ -95,13 +95,6 @@ async fn update_one( Ok(QueryResult::RecordSelection(res)) } - UpdateRecord::WithSelection(q) => { - let res = res - .map(|record| record.extract_selection_result(&q.selected_fields)) - .transpose()?; - - Ok(QueryResult::Id(res)) - } UpdateRecord::WithoutSelection(_) => { let res = res .map(|record| record.extract_selection_result(&q.model().primary_identifier())) diff --git a/query-engine/core/src/query_ast/write.rs b/query-engine/core/src/query_ast/write.rs index ee51830e796..fa9bc27376d 100644 --- a/query-engine/core/src/query_ast/write.rs +++ b/query-engine/core/src/query_ast/write.rs @@ -206,7 +206,11 @@ impl ToGraphviz for WriteQuery { match self { Self::CreateRecord(q) => format!("CreateRecord(model: {}, args: {:?})", q.model.name(), q.args), Self::CreateManyRecords(q) => format!("CreateManyRecord(model: {})", q.model.name()), - Self::UpdateRecord(q) => format!("UpdateRecord(model: {})", q.model().name(),), + Self::UpdateRecord(q) => format!( + "UpdateRecord(model: {}, selection: {:?})", + q.model().name(), + q.selected_fields() + ), Self::DeleteRecord(q) => format!("DeleteRecord: {}, {:?}", q.model.name(), q.record_filter), Self::UpdateManyRecords(q) => format!("UpdateManyRecords(model: {}, args: {:?})", q.model.name(), q.args), Self::DeleteManyRecords(q) => format!("DeleteManyRecords: {}", q.model.name()), @@ -303,8 +307,7 @@ impl UpdateRecord { #[derive(Debug, Clone)] pub struct UpdateRecordWithSelection { - // Used for serialization. When `None`, the result will only be used to fulfill other nodes requirement. - pub name: Option, + pub name: String, pub model: Model, pub record_filter: RecordFilter, pub args: WriteArgs, diff --git a/query-engine/core/src/query_graph_builder/write/update.rs b/query-engine/core/src/query_graph_builder/write/update.rs index 001e2b48a96..63cf88a3d29 100644 --- a/query-engine/core/src/query_graph_builder/write/update.rs +++ b/query-engine/core/src/query_graph_builder/write/update.rs @@ -200,7 +200,7 @@ where Query::Write(WriteQuery::UpdateRecord(UpdateRecord::WithSelection( UpdateRecordWithSelection { - name: Some(field.name.to_owned()), + name: field.name, model: model.clone(), record_filter: filter.into(), args, @@ -215,7 +215,7 @@ where Query::Write(WriteQuery::UpdateRecord(UpdateRecord::WithSelection( UpdateRecordWithSelection { - name: None, + name: String::new(), // This node will not be serialized so we don't need a name. model: model.clone(), record_filter: filter.into(), args,