From 4b92f1b2692f7214656a886afa4321c1cd15d455 Mon Sep 17 00:00:00 2001 From: Tamo Date: Thu, 27 Apr 2023 13:51:02 +0200 Subject: [PATCH] wip --- meilisearch/src/routes/indexes/documents.rs | 99 ++++++++++--------- meilisearch/tests/common/index.rs | 2 +- .../tests/documents/delete_documents.rs | 11 +-- meilisearch/tests/documents/errors.rs | 6 +- 4 files changed, 61 insertions(+), 57 deletions(-) diff --git a/meilisearch/src/routes/indexes/documents.rs b/meilisearch/src/routes/indexes/documents.rs index 8c8229429..0ca6cf066 100644 --- a/meilisearch/src/routes/indexes/documents.rs +++ b/meilisearch/src/routes/indexes/documents.rs @@ -4,13 +4,13 @@ use actix_web::http::header::CONTENT_TYPE; use actix_web::web::Data; use actix_web::{web, HttpMessage, HttpRequest, HttpResponse}; use bstr::ByteSlice; -use deserr::actix_web::AwebQueryParameter; +use deserr::actix_web::{AwebJson, AwebQueryParameter}; use deserr::Deserr; use futures::StreamExt; use index_scheduler::IndexScheduler; use log::debug; use meilisearch_types::deserr::query_params::Param; -use meilisearch_types::deserr::DeserrQueryParamError; +use meilisearch_types::deserr::{DeserrJsonError, DeserrQueryParamError}; use meilisearch_types::document_formats::{read_csv, read_json, read_ndjson, PayloadType}; use meilisearch_types::error::deserr_codes::*; use meilisearch_types::error::{Code, ResponseError}; @@ -71,8 +71,11 @@ pub fn configure(cfg: &mut web::ServiceConfig) { .route(web::put().to(SeqHandler(update_documents))) .route(web::delete().to(SeqHandler(clear_all_documents))), ) - // this route needs to be before the /documents/{document_id} to match properly - .service(web::resource("/delete-batch").route(web::post().to(SeqHandler(delete_documents)))) + // these routes needs to be before the /documents/{document_id} to match properly + .service( + web::resource("/delete-batch").route(web::post().to(SeqHandler(delete_documents_batch))), + ) + .service(web::resource("/delete").route(web::post().to(SeqHandler(delete_documents_by_filter)))) .service( web::resource("/{document_id}") .route(web::get().to(SeqHandler(get_document))) @@ -373,59 +376,22 @@ async fn document_addition( Ok(task.into()) } -#[derive(Debug, Deserialize)] -#[serde(untagged)] -pub enum DocumentDeletionQuery { - Ids(Vec), - Object { filter: Option }, -} - -pub async fn delete_documents( +pub async fn delete_documents_batch( index_scheduler: GuardedData, Data>, index_uid: web::Path, - body: web::Json, + body: web::Json>, req: HttpRequest, analytics: web::Data, ) -> Result { debug!("called with params: {:?}", body); let index_uid = IndexUid::try_from(index_uid.into_inner())?; - let index_uid = index_uid.into_inner(); analytics.delete_documents(DocumentDeletionKind::PerBatch, &req); - let ids = match body.into_inner() { - DocumentDeletionQuery::Ids(body) => body - .iter() - .map(|v| v.as_str().map(String::from).unwrap_or_else(|| v.to_string())) - .collect(), - DocumentDeletionQuery::Object { filter } => { - debug!("filter: {:?}", filter); - - // FIXME: spawn_blocking - if let Some(mut filter) = filter { - if let Some(facets) = crate::search::parse_filter(&filter)? { - debug!("facets: {:?}", facets); - - let task = KindWithContent::DocumentDeletionByFilter { - index_uid: index_uid.to_string(), - filter_expr: filter.take(), - }; - - let task: SummarizedTaskView = - tokio::task::spawn_blocking(move || index_scheduler.register(task)) - .await?? - .into(); - - debug!("returns: {:?}", task); - return Ok(HttpResponse::Accepted().json(task)); - } else { - vec![] - } - } else { - vec![] - } - } - }; + let ids = body + .iter() + .map(|v| v.as_str().map(String::from).unwrap_or_else(|| v.to_string())) + .collect(); let task = KindWithContent::DocumentDeletion { index_uid: index_uid.to_string(), documents_ids: ids }; @@ -436,6 +402,45 @@ pub async fn delete_documents( Ok(HttpResponse::Accepted().json(task)) } +#[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)] + filter: Value, +} + +pub async fn delete_documents_by_filter( + index_scheduler: GuardedData, Data>, + index_uid: web::Path, + body: AwebJson, + 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"); + + 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)); +} + pub async fn clear_all_documents( index_scheduler: GuardedData, Data>, index_uid: web::Path, diff --git a/meilisearch/tests/common/index.rs b/meilisearch/tests/common/index.rs index 5d38b84af..38b43e910 100644 --- a/meilisearch/tests/common/index.rs +++ b/meilisearch/tests/common/index.rs @@ -226,7 +226,7 @@ impl Index<'_> { } pub async fn delete_document_by_filter(&self, body: Value) -> (Value, StatusCode) { - let url = format!("/indexes/{}/documents/delete-batch", urlencode(self.uid.as_ref())); + let url = format!("/indexes/{}/documents/delete", urlencode(self.uid.as_ref())); self.service.post_encoded(url, body, self.encoder).await } diff --git a/meilisearch/tests/documents/delete_documents.rs b/meilisearch/tests/documents/delete_documents.rs index 4624f9d60..ef7d3e7a5 100644 --- a/meilisearch/tests/documents/delete_documents.rs +++ b/meilisearch/tests/documents/delete_documents.rs @@ -156,14 +156,13 @@ 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, - "indexUid": "doggo", - "status": "enqueued", - "type": "documentDeletion", - "enqueuedAt": "[date]" + "message": "Missing fied `filter`", + "code": "bad_request", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#bad_request" } "###); diff --git a/meilisearch/tests/documents/errors.rs b/meilisearch/tests/documents/errors.rs index 2452a0ebd..267e4ccbb 100644 --- a/meilisearch/tests/documents/errors.rs +++ b/meilisearch/tests/documents/errors.rs @@ -429,10 +429,10 @@ async fn delete_document_by_filter() { snapshot!(code, @"400 Bad Request"); snapshot!(json_string!(response), @r###" { - "message": "Json deserialize error: data did not match any variant of untagged enum DocumentDeletionQuery", - "code": "bad_request", + "message": "Invalid syntax for the filter parameter: `expected String, Array, found: null`.", + "code": "invalid_search_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#bad_request" + "link": "https://docs.meilisearch.com/errors#invalid_search_filter" } "###);