From 57da80900dcbb444b3e5ab476dbae8be609d1faa Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 18 Jan 2023 14:16:00 +0100 Subject: [PATCH 1/5] make the swap indexes not found errors return an IndexNotFound error code --- index-scheduler/src/error.rs | 4 ++-- .../lib.rs/swap_indexes_errors/first_swap_failed.snap | 2 +- meilisearch/tests/tasks/mod.rs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index 013bcf595..95161fd5e 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -141,8 +141,8 @@ impl ErrorCode for Error { Error::IndexAlreadyExists(_) => Code::IndexAlreadyExists, Error::SwapDuplicateIndexesFound(_) => Code::InvalidSwapDuplicateIndexFound, Error::SwapDuplicateIndexFound(_) => Code::InvalidSwapDuplicateIndexFound, - Error::SwapIndexNotFound(_) => Code::InvalidSwapIndexes, - Error::SwapIndexesNotFound(_) => Code::InvalidSwapIndexes, + Error::SwapIndexNotFound(_) => Code::IndexNotFound, + Error::SwapIndexesNotFound(_) => Code::IndexNotFound, Error::InvalidTaskDate { field, .. } => (*field).into(), Error::InvalidTaskUids { .. } => Code::InvalidTaskUids, Error::InvalidTaskStatuses { .. } => Code::InvalidTaskStatuses, diff --git a/index-scheduler/src/snapshots/lib.rs/swap_indexes_errors/first_swap_failed.snap b/index-scheduler/src/snapshots/lib.rs/swap_indexes_errors/first_swap_failed.snap index 1a47f87fb..3e95c5b25 100644 --- a/index-scheduler/src/snapshots/lib.rs/swap_indexes_errors/first_swap_failed.snap +++ b/index-scheduler/src/snapshots/lib.rs/swap_indexes_errors/first_swap_failed.snap @@ -10,7 +10,7 @@ source: index-scheduler/src/lib.rs 1 {uid: 1, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "b", primary_key: Some("id") }} 2 {uid: 2, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "c", primary_key: Some("id") }} 3 {uid: 3, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "d", primary_key: Some("id") }} -4 {uid: 4, status: failed, error: ResponseError { code: 200, message: "Indexes `e`, `f` not found.", error_code: "invalid_swap_indexes", error_type: "invalid_request", error_link: "https://docs.meilisearch.com/errors#invalid-swap-indexes" }, details: { swaps: [IndexSwap { indexes: ("a", "b") }, IndexSwap { indexes: ("c", "e") }, IndexSwap { indexes: ("d", "f") }] }, kind: IndexSwap { swaps: [IndexSwap { indexes: ("a", "b") }, IndexSwap { indexes: ("c", "e") }, IndexSwap { indexes: ("d", "f") }] }} +4 {uid: 4, status: failed, error: ResponseError { code: 200, message: "Indexes `e`, `f` not found.", error_code: "index_not_found", error_type: "invalid_request", error_link: "https://docs.meilisearch.com/errors#index-not-found" }, details: { swaps: [IndexSwap { indexes: ("a", "b") }, IndexSwap { indexes: ("c", "e") }, IndexSwap { indexes: ("d", "f") }] }, kind: IndexSwap { swaps: [IndexSwap { indexes: ("a", "b") }, IndexSwap { indexes: ("c", "e") }, IndexSwap { indexes: ("d", "f") }] }} ---------------------------------------------------------------------- ### Status: enqueued [] diff --git a/meilisearch/tests/tasks/mod.rs b/meilisearch/tests/tasks/mod.rs index 7fadf0a10..b20b2dabb 100644 --- a/meilisearch/tests/tasks/mod.rs +++ b/meilisearch/tests/tasks/mod.rs @@ -844,9 +844,9 @@ async fn test_summarized_index_swap() { }, "error": { "message": "Indexes `cattos`, `doggos` not found.", - "code": "invalid_swap_indexes", + "code": "index_not_found", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid-swap-indexes" + "link": "https://docs.meilisearch.com/errors#index-not-found" }, "duration": "[duration]", "enqueuedAt": "[date]", From a4476c20f88bbba66eaa8ed782e80b829a99055b Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 18 Jan 2023 15:28:02 +0100 Subject: [PATCH 2/5] fix a wrong error code and add tests on the document resource --- meilisearch/src/routes/indexes/documents.rs | 4 +- meilisearch/tests/common/index.rs | 10 +++ meilisearch/tests/documents/errors.rs | 99 +++++++++++++++++++++ meilisearch/tests/documents/mod.rs | 1 + 4 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 meilisearch/tests/documents/errors.rs diff --git a/meilisearch/src/routes/indexes/documents.rs b/meilisearch/src/routes/indexes/documents.rs index 2b36ba834..0ec1057ae 100644 --- a/meilisearch/src/routes/indexes/documents.rs +++ b/meilisearch/src/routes/indexes/documents.rs @@ -128,11 +128,11 @@ pub async fn delete_document( #[derive(Debug, DeserializeFromValue)] #[deserr(error = DeserrQueryParamError, rename_all = camelCase, deny_unknown_fields)] pub struct BrowseQuery { - #[deserr(default, error = DeserrQueryParamError)] + #[deserr(default, error = DeserrQueryParamError)] offset: Param, #[deserr(default = Param(PAGINATION_DEFAULT_LIMIT), error = DeserrQueryParamError)] limit: Param, - #[deserr(default, error = DeserrQueryParamError)] + #[deserr(default, error = DeserrQueryParamError)] fields: OptionStarOrList, } diff --git a/meilisearch/tests/common/index.rs b/meilisearch/tests/common/index.rs index b98ed9827..454c84565 100644 --- a/meilisearch/tests/common/index.rs +++ b/meilisearch/tests/common/index.rs @@ -155,6 +155,11 @@ impl Index<'_> { self.service.get(url).await } + pub async fn get_all_documents_raw(&self, options: &str) -> (Value, StatusCode) { + let url = format!("/indexes/{}/documents{}", urlencode(self.uid.as_ref()), options); + self.service.get(url).await + } + pub async fn get_all_documents(&self, options: GetAllDocumentsOptions) -> (Value, StatusCode) { let mut url = format!("/indexes/{}/documents?", urlencode(self.uid.as_ref())); if let Some(limit) = options.limit { @@ -187,6 +192,11 @@ impl Index<'_> { self.service.post_encoded(url, serde_json::to_value(&ids).unwrap(), self.encoder).await } + pub async fn delete_batch_raw(&self, body: Value) -> (Value, StatusCode) { + let url = format!("/indexes/{}/documents/delete-batch", urlencode(self.uid.as_ref())); + self.service.post_encoded(url, body, self.encoder).await + } + pub async fn settings(&self) -> (Value, StatusCode) { let url = format!("/indexes/{}/settings", urlencode(self.uid.as_ref())); self.service.get(url).await diff --git a/meilisearch/tests/documents/errors.rs b/meilisearch/tests/documents/errors.rs new file mode 100644 index 000000000..4c50a8e02 --- /dev/null +++ b/meilisearch/tests/documents/errors.rs @@ -0,0 +1,99 @@ +use meili_snap::*; +use serde_json::json; + +use crate::common::Server; + +#[actix_rt::test] +async fn get_all_documents_bad_offset() { + let server = Server::new().await; + let index = server.index("test"); + + let (response, code) = index.get_all_documents_raw("?offset").await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Invalid value in parameter `offset`: could not parse `` as a positive integer", + "code": "invalid_document_offset", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid-document-offset" + } + "###); + + let (response, code) = index.get_all_documents_raw("?offset=doggo").await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Invalid value in parameter `offset`: could not parse `doggo` as a positive integer", + "code": "invalid_document_offset", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid-document-offset" + } + "###); + + let (response, code) = index.get_all_documents_raw("?offset=-1").await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Invalid value in parameter `offset`: could not parse `-1` as a positive integer", + "code": "invalid_document_offset", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid-document-offset" + } + "###); +} + +#[actix_rt::test] +async fn get_all_documents_bad_limit() { + let server = Server::new().await; + let index = server.index("test"); + + let (response, code) = index.get_all_documents_raw("?limit").await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Invalid value in parameter `limit`: could not parse `` as a positive integer", + "code": "invalid_document_limit", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid-document-limit" + } + "###); + + let (response, code) = index.get_all_documents_raw("?limit=doggo").await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Invalid value in parameter `limit`: could not parse `doggo` as a positive integer", + "code": "invalid_document_limit", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid-document-limit" + } + "###); + + let (response, code) = index.get_all_documents_raw("?limit=-1").await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Invalid value in parameter `limit`: could not parse `-1` as a positive integer", + "code": "invalid_document_limit", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid-document-limit" + } + "###); +} + +#[actix_rt::test] +async fn delete_documents_batch() { + let server = Server::new().await; + let index = server.index("test"); + + let (response, code) = index.delete_batch_raw(json!("doggo")).await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Json deserialize error: invalid type: string \"doggo\", expected a sequence at line 1 column 7", + "code": "bad_request", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#bad-request" + } + "###); +} diff --git a/meilisearch/tests/documents/mod.rs b/meilisearch/tests/documents/mod.rs index 794b57c3a..f6430b108 100644 --- a/meilisearch/tests/documents/mod.rs +++ b/meilisearch/tests/documents/mod.rs @@ -1,4 +1,5 @@ mod add_documents; mod delete_documents; +mod errors; mod get_documents; mod update_documents; From 182eea1f1703dc2df26b697cc40865f9d9d8742e Mon Sep 17 00:00:00 2001 From: Kerollmops Date: Wed, 18 Jan 2023 15:50:05 +0100 Subject: [PATCH 3/5] Introduce a canceledBy filter for the tests --- meilisearch/tests/common/index.rs | 10 +++++++++- meilisearch/tests/documents/add_documents.rs | 2 +- meilisearch/tests/tasks/mod.rs | 11 ++++++----- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/meilisearch/tests/common/index.rs b/meilisearch/tests/common/index.rs index b98ed9827..4902f221f 100644 --- a/meilisearch/tests/common/index.rs +++ b/meilisearch/tests/common/index.rs @@ -132,7 +132,12 @@ impl Index<'_> { self.service.get(url).await } - pub async fn filtered_tasks(&self, types: &[&str], statuses: &[&str]) -> (Value, StatusCode) { + pub async fn filtered_tasks( + &self, + types: &[&str], + statuses: &[&str], + canceled_by: &[&str], + ) -> (Value, StatusCode) { let mut url = format!("/tasks?indexUids={}", self.uid); if !types.is_empty() { let _ = write!(url, "&types={}", types.join(",")); @@ -140,6 +145,9 @@ impl Index<'_> { if !statuses.is_empty() { let _ = write!(url, "&statuses={}", statuses.join(",")); } + if !canceled_by.is_empty() { + let _ = write!(url, "&canceledBy={}", canceled_by.join(",")); + } self.service.get(url).await } diff --git a/meilisearch/tests/documents/add_documents.rs b/meilisearch/tests/documents/add_documents.rs index 4af365a7e..8452955fd 100644 --- a/meilisearch/tests/documents/add_documents.rs +++ b/meilisearch/tests/documents/add_documents.rs @@ -1077,7 +1077,7 @@ async fn batch_several_documents_addition() { futures::future::join_all(waiter).await; index.wait_task(9).await; - let (response, _code) = index.filtered_tasks(&[], &["failed"]).await; + let (response, _code) = index.filtered_tasks(&[], &["failed"], &[]).await; // Check if only the 6th task failed println!("{}", &response); diff --git a/meilisearch/tests/tasks/mod.rs b/meilisearch/tests/tasks/mod.rs index 7fadf0a10..d44381729 100644 --- a/meilisearch/tests/tasks/mod.rs +++ b/meilisearch/tests/tasks/mod.rs @@ -115,7 +115,7 @@ async fn list_tasks_status_filtered() { .add_documents(serde_json::from_str(include_str!("../assets/test_set.json")).unwrap(), None) .await; - let (response, code) = index.filtered_tasks(&[], &["succeeded"]).await; + let (response, code) = index.filtered_tasks(&[], &["succeeded"], &[]).await; assert_eq!(code, 200, "{}", response); assert_eq!(response["results"].as_array().unwrap().len(), 1); @@ -126,7 +126,7 @@ async fn list_tasks_status_filtered() { index.wait_task(1).await; - let (response, code) = index.filtered_tasks(&[], &["succeeded"]).await; + let (response, code) = index.filtered_tasks(&[], &["succeeded"], &[]).await; assert_eq!(code, 200, "{}", response); assert_eq!(response["results"].as_array().unwrap().len(), 2); } @@ -141,12 +141,12 @@ async fn list_tasks_type_filtered() { .add_documents(serde_json::from_str(include_str!("../assets/test_set.json")).unwrap(), None) .await; - let (response, code) = index.filtered_tasks(&["indexCreation"], &[]).await; + let (response, code) = index.filtered_tasks(&["indexCreation"], &[], &[]).await; assert_eq!(code, 200, "{}", response); assert_eq!(response["results"].as_array().unwrap().len(), 1); let (response, code) = - index.filtered_tasks(&["indexCreation", "documentAdditionOrUpdate"], &[]).await; + index.filtered_tasks(&["indexCreation", "documentAdditionOrUpdate"], &[], &[]).await; assert_eq!(code, 200, "{}", response); assert_eq!(response["results"].as_array().unwrap().len(), 2); } @@ -161,7 +161,7 @@ async fn list_tasks_status_and_type_filtered() { .add_documents(serde_json::from_str(include_str!("../assets/test_set.json")).unwrap(), None) .await; - let (response, code) = index.filtered_tasks(&["indexCreation"], &["failed"]).await; + let (response, code) = index.filtered_tasks(&["indexCreation"], &["failed"], &[]).await; assert_eq!(code, 200, "{}", response); assert_eq!(response["results"].as_array().unwrap().len(), 0); @@ -169,6 +169,7 @@ async fn list_tasks_status_and_type_filtered() { .filtered_tasks( &["indexCreation", "documentAdditionOrUpdate"], &["succeeded", "processing", "enqueued"], + &[], ) .await; assert_eq!(code, 200, "{}", response); From d3c796af380be64548c5957067d22584772ac9c2 Mon Sep 17 00:00:00 2001 From: Kerollmops Date: Wed, 18 Jan 2023 15:50:36 +0100 Subject: [PATCH 4/5] Add a new test to check that invalid canceledBy works correctly --- meilisearch/tests/tasks/mod.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/meilisearch/tests/tasks/mod.rs b/meilisearch/tests/tasks/mod.rs index d44381729..2b429f798 100644 --- a/meilisearch/tests/tasks/mod.rs +++ b/meilisearch/tests/tasks/mod.rs @@ -151,6 +151,21 @@ async fn list_tasks_type_filtered() { assert_eq!(response["results"].as_array().unwrap().len(), 2); } +#[actix_rt::test] +async fn list_tasks_invalid_canceled_by_filter() { + let server = Server::new().await; + let index = server.index("test"); + index.create(None).await; + index.wait_task(0).await; + index + .add_documents(serde_json::from_str(include_str!("../assets/test_set.json")).unwrap(), None) + .await; + + let (response, code) = index.filtered_tasks(&[], &[], &["0"]).await; + assert_eq!(code, 200, "{}", response); + assert_eq!(response["results"].as_array().unwrap().len(), 0); +} + #[actix_rt::test] async fn list_tasks_status_and_type_filtered() { let server = Server::new().await; From e89973f1bf01c8a687275116205abf9d869df104 Mon Sep 17 00:00:00 2001 From: Kerollmops Date: Wed, 18 Jan 2023 15:25:27 +0100 Subject: [PATCH 5/5] Do not delete all tasks when no canceled-by matches --- index-scheduler/src/lib.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs index 0b9e856d2..4374a0612 100644 --- a/index-scheduler/src/lib.rs +++ b/index-scheduler/src/lib.rs @@ -502,13 +502,22 @@ impl IndexScheduler { } if let Some(canceled_by) = &query.canceled_by { + let mut all_canceled_tasks = RoaringBitmap::new(); for cancel_task_uid in canceled_by { if let Some(canceled_by_uid) = self.canceled_by.get(rtxn, &BEU32::new(*cancel_task_uid))? { - tasks &= canceled_by_uid; + all_canceled_tasks |= canceled_by_uid; } } + + // if the canceled_by has been specified but no task + // matches then we prefer matching zero than all tasks. + if all_canceled_tasks.is_empty() { + return Ok(RoaringBitmap::new()); + } else { + tasks &= all_canceled_tasks; + } } if let Some(kind) = &query.types {