diff --git a/meilisearch-types/src/error.rs b/meilisearch-types/src/error.rs index c36d27529..e4ca39c8b 100644 --- a/meilisearch-types/src/error.rs +++ b/meilisearch-types/src/error.rs @@ -218,6 +218,7 @@ InvalidDocumentGeoField , InvalidRequest , BAD_REQUEST ; InvalidDocumentId , InvalidRequest , BAD_REQUEST ; InvalidDocumentLimit , InvalidRequest , BAD_REQUEST ; InvalidDocumentOffset , InvalidRequest , BAD_REQUEST ; +InvalidDocumentDeleteFilter , InvalidRequest , BAD_REQUEST ; InvalidIndexLimit , InvalidRequest , BAD_REQUEST ; InvalidIndexOffset , InvalidRequest , BAD_REQUEST ; InvalidIndexPrimaryKey , InvalidRequest , BAD_REQUEST ; diff --git a/meilisearch/src/analytics/mod.rs b/meilisearch/src/analytics/mod.rs index 4d295db57..6223b9db7 100644 --- a/meilisearch/src/analytics/mod.rs +++ b/meilisearch/src/analytics/mod.rs @@ -64,6 +64,7 @@ pub enum DocumentDeletionKind { PerDocumentId, ClearAll, PerBatch, + PerFilter, } pub trait Analytics: Sync + Send { diff --git a/meilisearch/src/error.rs b/meilisearch/src/error.rs index 850803a07..9d0a03c34 100644 --- a/meilisearch/src/error.rs +++ b/meilisearch/src/error.rs @@ -20,6 +20,8 @@ pub enum MeilisearchHttpError { InvalidContentType(String, Vec), #[error("Document `{0}` not found.")] DocumentNotFound(String), + #[error("Sending an empty filter is forbidden.")] + EmptyFilter, #[error("Invalid syntax for the filter parameter: `expected {}, found: {1}`.", .0.join(", "))] InvalidExpression(&'static [&'static str], Value), #[error("A {0} payload is missing.")] @@ -58,6 +60,7 @@ impl ErrorCode for MeilisearchHttpError { MeilisearchHttpError::MissingPayload(_) => Code::MissingPayload, MeilisearchHttpError::InvalidContentType(_, _) => Code::InvalidContentType, MeilisearchHttpError::DocumentNotFound(_) => Code::DocumentNotFound, + MeilisearchHttpError::EmptyFilter => Code::InvalidDocumentDeleteFilter, MeilisearchHttpError::InvalidExpression(_, _) => Code::InvalidSearchFilter, MeilisearchHttpError::PayloadTooLarge => Code::PayloadTooLarge, MeilisearchHttpError::SwapIndexPayloadWrongLength(_) => Code::InvalidSwapIndexes, diff --git a/meilisearch/src/routes/indexes/documents.rs b/meilisearch/src/routes/indexes/documents.rs index 0ca6cf066..5793b6fb4 100644 --- a/meilisearch/src/routes/indexes/documents.rs +++ b/meilisearch/src/routes/indexes/documents.rs @@ -405,8 +405,7 @@ pub async fn delete_documents_batch( #[derive(Debug, Deserr)] #[deserr(error = DeserrJsonError, rename_all = camelCase, deny_unknown_fields)] pub struct DocumentDeletionByFilter { - // TODO: Update the error code to something more appropriate - #[deserr(error = DeserrJsonError)] + #[deserr(error = DeserrJsonError)] filter: Value, } @@ -417,28 +416,26 @@ pub async fn delete_documents_by_filter( req: HttpRequest, analytics: web::Data, ) -> Result { - println!("here"); debug!("called with params: {:?}", body); let index_uid = IndexUid::try_from(index_uid.into_inner())?; let index_uid = index_uid.into_inner(); let filter = body.into_inner().filter; - analytics.delete_documents(DocumentDeletionKind::PerBatch, &req); - - debug!("filter: {:?}", filter); - - // FIXME: spawn_blocking => tamo: but why, it's making zero IO and almost no allocation? - // TODO: what should we do in case of an empty filter? Create a task that does nothing (then we should be able to store a None in the task queue) - // or refuse the payload with a cool™️ error message 😎 - let _ = crate::search::parse_filter(&filter)?.expect("You can't send an empty filter"); + analytics.delete_documents(DocumentDeletionKind::PerFilter, &req); + // we ensure the filter is well formed before enqueuing it + || -> Result<_, ResponseError> { + Ok(crate::search::parse_filter(&filter)?.ok_or(MeilisearchHttpError::EmptyFilter)?) + }() + // and whatever was the error, the error code should always be an InvalidDocumentDeleteFilter + .map_err(|err| ResponseError::from_msg(err.message, Code::InvalidDocumentDeleteFilter))?; let task = KindWithContent::DocumentDeletionByFilter { index_uid, filter_expr: filter }; let task: SummarizedTaskView = tokio::task::spawn_blocking(move || index_scheduler.register(task)).await??.into(); debug!("returns: {:?}", task); - return Ok(HttpResponse::Accepted().json(task)); + Ok(HttpResponse::Accepted().json(task)) } pub async fn clear_all_documents( diff --git a/meilisearch/tests/documents/delete_documents.rs b/meilisearch/tests/documents/delete_documents.rs index 267552069..a34bcfa95 100644 --- a/meilisearch/tests/documents/delete_documents.rs +++ b/meilisearch/tests/documents/delete_documents.rs @@ -156,7 +156,7 @@ async fn delete_document_by_filter() { index.wait_task(1).await; let (response, code) = index.delete_document_by_filter(json!({ "filter": "color = blue"})).await; - // snapshot!(code, @"202 Accepted"); + snapshot!(code, @"202 Accepted"); snapshot!(json_string!(response, { ".enqueuedAt" => "[date]" }), @r###" { "taskUid": 2, @@ -247,14 +247,11 @@ async fn delete_document_by_filter() { { "id": 0, "color": "red" - }, - { - "id": 3 } ], "offset": 0, "limit": 20, - "total": 2 + "total": 1 } "###); } diff --git a/meilisearch/tests/documents/errors.rs b/meilisearch/tests/documents/errors.rs index 5ba286c70..18c01623b 100644 --- a/meilisearch/tests/documents/errors.rs +++ b/meilisearch/tests/documents/errors.rs @@ -436,15 +436,39 @@ async fn delete_document_by_filter() { } "###); + // send bad payload type + let (response, code) = index.delete_document_by_filter(json!({ "filter": true })).await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Invalid syntax for the filter parameter: `expected String, Array, found: true`.", + "code": "invalid_document_delete_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_delete_filter" + } + "###); + // send bad filter let (response, code) = index.delete_document_by_filter(json!({ "filter": "hello"})).await; snapshot!(code, @"400 Bad Request"); snapshot!(json_string!(response), @r###" { "message": "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `IN`, `NOT IN`, `TO`, `EXISTS`, `NOT EXISTS`, `_geoRadius`, or `_geoBoundingBox` at `hello`.\n1:6 hello", - "code": "invalid_search_filter", + "code": "invalid_document_delete_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_delete_filter" + } + "###); + + // send empty filter + let (response, code) = index.delete_document_by_filter(json!({ "filter": ""})).await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Sending an empty filter is forbidden.", + "code": "invalid_document_delete_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_delete_filter" } "###);