From 0b38f211ac9290cc9e9dc0315ff631dff5eced89 Mon Sep 17 00:00:00 2001 From: Tamo Date: Tue, 16 May 2023 12:07:44 +0200 Subject: [PATCH 1/2] test the new introduced route --- meilisearch/tests/auth/authorization.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/meilisearch/tests/auth/authorization.rs b/meilisearch/tests/auth/authorization.rs index ef4a7eaa1..58fba4481 100644 --- a/meilisearch/tests/auth/authorization.rs +++ b/meilisearch/tests/auth/authorization.rs @@ -16,8 +16,11 @@ pub static AUTHORIZATIONS: Lazy hashset!{"search", "*"}, ("POST", "/indexes/products/documents") => hashset!{"documents.add", "documents.*", "*"}, ("GET", "/indexes/products/documents") => hashset!{"documents.get", "documents.*", "*"}, + ("POST", "/indexes/products/documents/fetch") => hashset!{"documents.get", "documents.*", "*"}, ("GET", "/indexes/products/documents/0") => hashset!{"documents.get", "documents.*", "*"}, ("DELETE", "/indexes/products/documents/0") => hashset!{"documents.delete", "documents.*", "*"}, + ("POST", "/indexes/products/documents/delete-batch") => hashset!{"documents.delete", "documents.*", "*"}, + ("POST", "/indexes/products/documents/delete") => hashset!{"documents.delete", "documents.*", "*"}, ("GET", "/tasks") => hashset!{"tasks.get", "tasks.*", "*"}, ("DELETE", "/tasks") => hashset!{"tasks.delete", "tasks.*", "*"}, ("GET", "/tasks?indexUid=products") => hashset!{"tasks.get", "tasks.*", "*"}, From 96da5130a4074fab1150f35dfb7c0c5f605f5515 Mon Sep 17 00:00:00 2001 From: Tamo Date: Tue, 16 May 2023 13:56:18 +0200 Subject: [PATCH 2/2] fix the error code in case of not filterable attributes on the get / delete documents by filter routes --- index-scheduler/src/batch.rs | 8 ++++- index-scheduler/src/error.rs | 8 +++++ meilisearch/src/routes/indexes/documents.rs | 7 ++++- meilisearch/tests/documents/errors.rs | 35 +++++++++++++++++---- 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/index-scheduler/src/batch.rs b/index-scheduler/src/batch.rs index c88234809..67f70d367 100644 --- a/index-scheduler/src/batch.rs +++ b/index-scheduler/src/batch.rs @@ -24,6 +24,7 @@ use std::io::BufWriter; use dump::IndexMetadata; use log::{debug, error, info}; +use meilisearch_types::error::Code; use meilisearch_types::heed::{RoTxn, RwTxn}; use meilisearch_types::milli::documents::{obkv_to_object, DocumentsBatchReader}; use meilisearch_types::milli::heed::CompactionOption; @@ -1491,7 +1492,12 @@ fn delete_document_by_filter(filter: &serde_json::Value, index: Index) -> Result Ok(if let Some(filter) = filter { let mut wtxn = index.write_txn()?; - let candidates = filter.evaluate(&wtxn, &index)?; + let candidates = filter.evaluate(&wtxn, &index).map_err(|err| match err { + milli::Error::UserError(milli::UserError::InvalidFilter(_)) => { + Error::from(err).with_custom_error_code(Code::InvalidDocumentFilter) + } + e => e.into(), + })?; let mut delete_operation = DeleteDocuments::new(&mut wtxn, &index)?; delete_operation.delete_documents(&candidates); let deleted_documents = diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index 3a19ed4d2..acab850d1 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -46,6 +46,8 @@ impl From for Code { #[allow(clippy::large_enum_variant)] #[derive(Error, Debug)] pub enum Error { + #[error("{1}")] + WithCustomErrorCode(Code, Box), #[error("Index `{0}` not found.")] IndexNotFound(String), #[error("Index `{0}` already exists.")] @@ -144,6 +146,7 @@ impl Error { pub fn is_recoverable(&self) -> bool { match self { Error::IndexNotFound(_) + | Error::WithCustomErrorCode(_, _) | Error::IndexAlreadyExists(_) | Error::SwapDuplicateIndexFound(_) | Error::SwapDuplicateIndexesFound(_) @@ -176,11 +179,16 @@ impl Error { Error::PlannedFailure => false, } } + + pub fn with_custom_error_code(self, code: Code) -> Self { + Self::WithCustomErrorCode(code, Box::new(self)) + } } impl ErrorCode for Error { fn error_code(&self) -> Code { match self { + Error::WithCustomErrorCode(code, _) => *code, Error::IndexNotFound(_) => Code::IndexNotFound, Error::IndexAlreadyExists(_) => Code::IndexAlreadyExists, Error::SwapDuplicateIndexesFound(_) => Code::InvalidSwapDuplicateIndexFound, diff --git a/meilisearch/src/routes/indexes/documents.rs b/meilisearch/src/routes/indexes/documents.rs index eb0f5a59e..096f5737f 100644 --- a/meilisearch/src/routes/indexes/documents.rs +++ b/meilisearch/src/routes/indexes/documents.rs @@ -540,7 +540,12 @@ fn retrieve_documents>( }; let candidates = if let Some(filter) = filter { - filter.evaluate(&rtxn, index)? + filter.evaluate(&rtxn, index).map_err(|err| match err { + milli::Error::UserError(milli::UserError::InvalidFilter(_)) => { + ResponseError::from_msg(err.to_string(), Code::InvalidDocumentFilter) + } + e => e.into(), + })? } else { index.documents_ids(&rtxn)? }; diff --git a/meilisearch/tests/documents/errors.rs b/meilisearch/tests/documents/errors.rs index b72dc40f3..0210d1bb2 100644 --- a/meilisearch/tests/documents/errors.rs +++ b/meilisearch/tests/documents/errors.rs @@ -180,9 +180,9 @@ async fn get_all_documents_bad_filter() { snapshot!(json_string!(response), @r###" { "message": "Attribute `doggo` is not filterable. This index does not have configured filterable attributes.\n1:6 doggo=bernese", - "code": "invalid_search_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" } "###); } @@ -630,9 +630,9 @@ async fn delete_document_by_filter() { }, "error": { "message": "Attribute `doggo` is not filterable. This index does not have configured filterable attributes.\n1:6 doggo = bernese", - "code": "invalid_search_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" }, "duration": "[duration]", "enqueuedAt": "[date]", @@ -664,9 +664,9 @@ async fn delete_document_by_filter() { }, "error": { "message": "Attribute `catto` is not filterable. Available filterable attributes are: `doggo`.\n1:6 catto = jorts", - "code": "invalid_search_filter", + "code": "invalid_document_filter", "type": "invalid_request", - "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" }, "duration": "[duration]", "enqueuedAt": "[date]", @@ -748,4 +748,27 @@ async fn fetch_document_by_filter() { "link": "https://docs.meilisearch.com/errors#invalid_document_filter" } "###); + + let (response, code) = index.get_document_by_filter(json!({ "filter": "cool doggo" })).await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `IN`, `NOT IN`, `TO`, `EXISTS`, `NOT EXISTS`, `IS NULL`, `IS NOT NULL`, `IS EMPTY`, `IS NOT EMPTY`, `_geoRadius`, or `_geoBoundingBox` at `cool doggo`.\n1:11 cool doggo", + "code": "invalid_document_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" + } + "###); + + let (response, code) = + index.get_document_by_filter(json!({ "filter": "doggo = bernese" })).await; + snapshot!(code, @"400 Bad Request"); + snapshot!(json_string!(response), @r###" + { + "message": "Attribute `doggo` is not filterable. Available filterable attributes are: `color`.\n1:6 doggo = bernese", + "code": "invalid_document_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_filter" + } + "###); }