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.

Sign up for GitHub

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

feat(deleteMany): add limit option to deleteMany #5105

Merged
merged 6 commits into from
Jan 8, 2025
Merged

feat(deleteMany): add limit option to deleteMany #5105

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9c9bf6a
feat(deleteMany): add limit option to deleteMany
FGoessler Jan 7, 2025
d1f69e1
format: fix linting issues
FGoessler Jan 7, 2025
90b30b7
chore(tests): update DMMF snapshots
FGoessler Jan 7, 2025
abfa139
chore(tests): update DMMF snapshots
FGoessler Jan 7, 2025
5e3670a
refactor: remove unwrap
FGoessler Jan 8, 2025
4917ea7
refactor: move statements according to review
FGoessler Jan 8, 2025
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
24 changes: 24 additions & 0 deletions prisma-fmt/src/get_dmmf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5592,6 +5592,18 @@ mod tests {
"isList": false
}
]
},
{
"name": "limit",
"isRequired": false,
"isNullable": false,
"inputTypes": [
{
"type": "Int",
"location": "scalar",
"isList": false
}
]
}
],
"isNullable": false,
Expand Down Expand Up @@ -5954,6 +5966,18 @@ mod tests {
"isList": false
}
]
},
{
"name": "limit",
"isRequired": false,
"isNullable": false,
"inputTypes": [
{
"type": "Int",
"location": "scalar",
"isList": false
}
]
}
],
"isNullable": false,
Expand Down
26 changes: 26 additions & 0 deletions .../connector-test-kit-rs/query-engine-tests/tests/writes/top_level_mutations/delete_many.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,32 @@ mod delete_many {
Ok(())
}

// "The delete many Mutation" should "delete max the number of items specified in the limit"
#[connector_test]
async fn should_delete_max_limit_items(runner: Runner) -> TestResult<()> {
create_row(&runner, r#"{ id: 1, title: "title1" }"#).await?;
create_row(&runner, r#"{ id: 2, title: "title2" }"#).await?;
create_row(&runner, r#"{ id: 3, title: "title3" }"#).await?;
create_row(&runner, r#"{ id: 4, title: "title4" }"#).await?;

assert_todo_count(&runner, 4).await?;

insta::assert_snapshot!(
run_query!(&runner, r#"mutation {
deleteManyTodo(
limit: 3
){
count
}
}"#),
@r###"{"data":{"deleteManyTodo":{"count":3}}}"###
);

assert_todo_count(&runner, 1).await?;

Ok(())
}

fn nested_del_many() -> String {
let schema = indoc! {
r#"model ZChild{
Expand Down
2 changes: 2 additions & 0 deletions query-engine/connectors/mongodb-query-connector/src/interface/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,15 @@ impl WriteOperations for MongoDbConnection {
&mut self,
model: &Model,
record_filter: connector_interface::RecordFilter,
limit: Option<i64>,
_traceparent: Option<TraceParent>,
) -> connector_interface::Result<usize> {
catch(write::delete_records(
&self.database,
&mut self.session,
model,
record_filter,
limit,
))
.await
}
Expand Down
2 changes: 2 additions & 0 deletions query-engine/connectors/mongodb-query-connector/src/interface/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,13 +191,15 @@ impl WriteOperations for MongoDbTransaction<'_> {
&mut self,
model: &Model,
record_filter: connector_interface::RecordFilter,
limit: Option<i64>,
_traceparent: Option<TraceParent>,
) -> connector_interface::Result<usize> {
catch(write::delete_records(
&self.connection.database,
&mut self.connection.session,
model,
record_filter,
limit,
))
.await
}
Expand Down
9 changes: 7 additions & 2 deletions query-engine/connectors/mongodb-query-connector/src/root_queries/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ pub async fn update_records<'conn>(
.collect::<crate::Result<Vec<_>>>()?
} else {
let filter = MongoFilterVisitor::new(FilterPrefix::default(), false).visit(record_filter.filter)?;
find_ids(coll.clone(), session, model, filter).await?
find_ids(coll.clone(), session, model, filter, None).await?
};

if ids.is_empty() {
Expand Down Expand Up @@ -228,13 +228,15 @@ pub async fn delete_records<'conn>(
session: &mut ClientSession,
model: &Model,
record_filter: RecordFilter,
limit: Option<i64>,
) -> crate::Result<usize> {
let coll = database.collection::<Document>(model.db_name());
let id_field = pick_singular_id(model);

let ids = if let Some(selectors) = record_filter.selectors {
selectors
.into_iter()
.take(limit.unwrap_or(i64::MAX) as usize)
.map(|p| {
(&id_field, p.values().next().unwrap())
.into_bson()
Expand All @@ -243,7 +245,7 @@ pub async fn delete_records<'conn>(
.collect::<crate::Result<Vec<_>>>()?
} else {
let filter = MongoFilterVisitor::new(FilterPrefix::default(), false).visit(record_filter.filter)?;
find_ids(coll.clone(), session, model, filter).await?
find_ids(coll.clone(), session, model, filter, limit).await?
};

if ids.is_empty() {
Expand Down Expand Up @@ -303,6 +305,7 @@ async fn find_ids(
session: &mut ClientSession,
model: &Model,
filter: MongoFilter,
limit: Option<i64>,
) -> crate::Result<Vec<Bson>> {
let id_field = model.primary_identifier();
let mut builder = MongoReadQueryBuilder::new(model.clone());
Expand All @@ -316,6 +319,8 @@ async fn find_ids(
builder.query = Some(filter);
};

builder.limit = limit;

let builder = builder.with_model_projection(id_field)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to the first builder once that variable is aliased in line 324? Is the .limit field preserved? If that's the case, shouldn't these two lines be swapped to make the resulting code semantics more intuitive and apparent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is preserved. I am not sure how moving the limit statement down improves the semantics. Esp. considering the builder.joins etc assignments in the lines above.
Technically line 324 can also be simply builder = builder.with_model_projection(id_field)?; (no let).

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how moving the limit statement down improves the semantics.
It doesn't, but it makes it clearer to the human reader that the .limit field is preserved for the second builder shadowed variable

Technically line 324 can also be simply builder = builder.with_model_projection(id_field)?; (no let).
I'm sure there are reasons for that declaration that rescopes the second builder. Let's keep it that way unless we have a compelling argument to not do so :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Moved it but also had to make the second builder var now mutable.

Copy link
Member

@aqrln aqrln Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Late to the party here but I'd personally prefer to shadow the previous binding rather than make it mutable in cases like these. In this specific case a new immutable binding to replace the old mutable one looks intentional to limit the scope of mut only to the few lines where we need to assign some fields.

let query = builder.build()?;
let docs = query.execute(collection, session).await?;
Expand Down
1 change: 1 addition & 0 deletions query-engine/connectors/query-connector/src/interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ pub trait WriteOperations {
&mut self,
model: &Model,
record_filter: RecordFilter,
limit: Option<i64>,
traceparent: Option<TraceParent>,
) -> crate::Result<usize>;

Expand Down
3 changes: 2 additions & 1 deletion query-engine/connectors/sql-query-connector/src/database/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,12 +272,13 @@ where
&mut self,
model: &Model,
record_filter: RecordFilter,
limit: Option<i64>,
traceparent: Option<TraceParent>,
) -> connector::Result<usize> {
let ctx = Context::new(&self.connection_info, traceparent);
catch(
&self.connection_info,
write::delete_records(&self.inner, model, record_filter, &ctx),
write::delete_records(&self.inner, model, record_filter, limit, &ctx),
)
.await
}
Expand Down
16 changes: 13 additions & 3 deletions query-engine/connectors/sql-query-connector/src/database/operations/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ pub(crate) async fn delete_records(
conn: &dyn Queryable,
model: &Model,
record_filter: RecordFilter,
limit: Option<i64>,
ctx: &Context<'_>,
) -> crate::Result<usize> {
let filter_condition = FilterBuilder::without_top_level_joins().visit_filter(record_filter.clone().filter, ctx);
Expand All @@ -453,14 +454,23 @@ pub(crate) async fn delete_records(
let row_count = if record_filter.has_selectors() {
let ids: Vec<_> = record_filter.selectors.as_ref().unwrap().iter().collect();
let mut row_count = 0;
let mut remaining_limit = limit;

for delete in write::delete_many_from_ids_and_filter(model, ids.as_slice(), filter_condition, ctx) {
for delete in
write::delete_many_from_ids_and_filter(model, ids.as_slice(), filter_condition, remaining_limit, ctx)
{
row_count += conn.execute(delete).await?;
if let Some(old_remaining_limit) = remaining_limit {
let new_remaining_limit = old_remaining_limit - row_count as i64;
if new_remaining_limit <= 0 {
break;
}
remaining_limit = Some(new_remaining_limit);
}
}

row_count
} else {
conn.execute(write::delete_many_from_filter(model, filter_condition, ctx))
conn.execute(write::delete_many_from_filter(model, filter_condition, limit, ctx))
.await?
};

Expand Down
3 changes: 2 additions & 1 deletion query-engine/connectors/sql-query-connector/src/database/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,12 @@ impl WriteOperations for SqlConnectorTransaction<'_> {
&mut self,
model: &Model,
record_filter: RecordFilter,
limit: Option<i64>,
traceparent: Option<TraceParent>,
) -> connector::Result<usize> {
catch(&self.connection_info, async {
let ctx = Context::new(&self.connection_info, traceparent);
write::delete_records(self.inner.as_queryable(), model, record_filter, &ctx).await
write::delete_records(self.inner.as_queryable(), model, record_filter, limit, &ctx).await
})
.await
}
Expand Down
27 changes: 25 additions & 2 deletions query-engine/connectors/sql-query-connector/src/query_builder/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,32 @@ pub(crate) fn delete_returning(
pub(crate) fn delete_many_from_filter(
model: &Model,
filter_condition: ConditionTree<'static>,
limit: Option<i64>,
ctx: &Context<'_>,
) -> Query<'static> {
let condition = if let Some(limit) = limit {
let columns = model
.primary_identifier()
.as_scalar_fields()
.expect("primary identifier must contain scalar fields")
.into_iter()
.map(|f| f.as_column(ctx))
.collect::<Vec<_>>();

ConditionTree::from(
Row::from(columns.clone()).in_selection(
Select::from_table(model.as_table(ctx))
.columns(columns)
.so_that(filter_condition)
.limit(limit as usize),
),
)
} else {
filter_condition
};

Delete::from_table(model.as_table(ctx))
.so_that(filter_condition)
.so_that(condition)
.add_traceparent(ctx.traceparent)
.into()
}
Expand All @@ -238,14 +260,15 @@ pub(crate) fn delete_many_from_ids_and_filter(
model: &Model,
ids: &[&SelectionResult],
filter_condition: ConditionTree<'static>,
limit: Option<i64>,
ctx: &Context<'_>,
) -> Vec<Query<'static>> {
let columns: Vec<_> = ModelProjection::from(model.primary_identifier())
.as_columns(ctx)
.collect();

super::chunked_conditions(&columns, ids, ctx, |conditions| {
delete_many_from_filter(model, conditions.and(filter_condition.clone()), ctx)
delete_many_from_filter(model, conditions.and(filter_condition.clone()), limit, ctx)
})
}

Expand Down
6 changes: 4 additions & 2 deletions query-engine/core/src/interpreter/query_interpreters/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ async fn delete_one(

Ok(QueryResult::RecordSelection(Some(Box::new(selection))))
} else {
let result = tx.delete_records(&q.model, filter, traceparent).await?;
let result = tx.delete_records(&q.model, filter, None, traceparent).await?;
Ok(QueryResult::Count(result))
}
}
Expand Down Expand Up @@ -337,7 +337,9 @@ async fn delete_many(
q: DeleteManyRecords,
traceparent: Option<TraceParent>,
) -> InterpretationResult<QueryResult> {
let res = tx.delete_records(&q.model, q.record_filter, traceparent).await?;
let res = tx
.delete_records(&q.model, q.record_filter, q.limit, traceparent)
.await?;

Ok(QueryResult::Count(res))
}
Expand Down
1 change: 1 addition & 0 deletions query-engine/core/src/query_ast/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ pub struct DeleteRecordFields {
pub struct DeleteManyRecords {
pub model: Model,
pub record_filter: RecordFilter,
pub limit: Option<i64>,
}

#[derive(Debug, Clone)]
Expand Down
11 changes: 10 additions & 1 deletion query-engine/core/src/query_graph_builder/write/delete.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use super::*;
use crate::query_document::ParsedInputValue;
use crate::{
query_ast::*,
query_graph::{Node, QueryGraph, QueryGraphDependency},
ArgumentListLookup, FilteredQuery, ParsedField,
};
use psl::datamodel_connector::ConnectorCapability;
use query_structure::{Filter, Model};
use query_structure::{Filter, Model, PrismaValue};
use schema::{constants::args, QuerySchema};
use std::convert::TryInto;

Expand Down Expand Up @@ -110,12 +111,20 @@ pub fn delete_many_records(
Some(where_arg) => extract_filter(where_arg.value.try_into()?, &model)?,
None => Filter::empty(),
};
let limit = field
.arguments
.lookup(args::LIMIT)
.and_then(|limit_arg| match limit_arg.value {
ParsedInputValue::Single(PrismaValue::Int(i)) => Some(i),
_ => None,
});

let model_id = model.primary_identifier();
let record_filter = filter.clone().into();
let delete_many = WriteQuery::DeleteManyRecords(DeleteManyRecords {
model: model.clone(),
record_filter,
limit,
});

let delete_many_node = graph.create_node(Query::Write(delete_many));
Expand Down
2 changes: 2 additions & 0 deletions query-engine/core/src/query_graph_builder/write/nested/delete_nested.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ pub fn nested_delete(
let delete_many = WriteQuery::DeleteManyRecords(DeleteManyRecords {
model: child_model.clone(),
record_filter: or_filter.clone().into(),
limit: None,
});

let delete_many_node = graph.create_node(Query::Write(delete_many));
Expand Down Expand Up @@ -157,6 +158,7 @@ pub fn nested_delete_many(
let delete_many = WriteQuery::DeleteManyRecords(DeleteManyRecords {
model: child_model.clone(),
record_filter: RecordFilter::empty(),
limit: None,
});

let delete_many_node = graph.create_node(Query::Write(delete_many));
Expand Down
1 change: 1 addition & 0 deletions query-engine/core/src/query_graph_builder/write/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,7 @@ pub fn emulate_on_delete_cascade(
let delete_query = WriteQuery::DeleteManyRecords(DeleteManyRecords {
model: dependent_model.clone(),
record_filter: RecordFilter::empty(),
limit: None,
});

let delete_dependents_node = graph.create_node(Query::Write(delete_query));
Expand Down
Binary file modified query-engine/dmmf/src/tests/test-schemas/snapshots/odoo.snapshot.json.gz
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example update of a snapshot:

7006327 |             {
7006328 |               "name": "deleteManywebsite_redirect",
7006329 |               "args": [
7006330 |                 {
7006331 |                   "name": "where",
7006332 |                   "isRequired": false,
7006333 |                   "isNullable": false,
7006334 |                   "inputTypes": [
7006335 |                     {
7006336 |                       "type": "website_redirectWhereInput",
7006337 |                       "namespace": "prisma",
7006338 |                       "location": "inputObjectTypes",
7006339 |                       "isList": false
7006340 |                     }
7006341 |                   ]
7006342 |+                },
7006343 |+                {
7006344 |+                  "name": "limit",
7006345 |+                  "isRequired": false,
7006346 |+                  "isNullable": false,
7006347 |+                  "inputTypes": [
7006348 |+                    {
7006349 |+                      "type": "Int",
7006350 |+                      "location": "scalar",
7006351 |+                      "isList": false
7006352 |+                    }
7006353 |+                  ]
7006354 |                 }
7006355 |               ],
7006356 |               "isNullable": false,
7006357 |               "outputType": {
7006358 |                 "type": "AffectedRowsOutput",
7006359 |                 "namespace": "prisma",
7006360 |                 "location": "outputObjectTypes",
7006361 |                 "isList": false
7006362 |               }
7006363 |             },

Binary file not shown.
5 changes: 4 additions & 1 deletion query-engine/schema/src/build/input_types/fields/arguments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,10 @@ pub(crate) fn update_many_arguments(ctx: &QuerySchema, model: Model) -> Vec<Inpu
pub(crate) fn delete_many_arguments(ctx: &QuerySchema, model: Model) -> Vec<InputField<'_>> {
let where_arg = where_argument(ctx, &model);

vec![where_arg]
vec![
where_arg,
input_field(args::LIMIT, vec![InputType::int()], None).optional(),
]
}

/// Builds "many records where" arguments based on the given model and field.
Expand Down
3 changes: 3 additions & 0 deletions query-engine/schema/src/constants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ pub mod args {

// createMany-specific args
pub const SKIP_DUPLICATES: &str = "skipDuplicates";

// deleteMany-specific args
pub const LIMIT: &str = "limit";
}

pub mod operations {
Expand Down
Loading