From d08d97bf43f9c0dbeed3eb8fbaf501b9e4adf780 Mon Sep 17 00:00:00 2001 From: Tamo Date: Mon, 14 Nov 2022 23:18:04 +0100 Subject: [PATCH 1/3] fix the error messages and add tests --- index-scheduler/src/error.rs | 14 +-- meilisearch-http/tests/common/index.rs | 10 +- meilisearch-http/tests/common/server.rs | 8 +- meilisearch-http/tests/tasks/mod.rs | 131 +++++++++++++++++++++++- 4 files changed, 146 insertions(+), 17 deletions(-) diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index 1cde58905..cfbf7a25e 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -60,9 +60,9 @@ pub enum Error { InvalidIndexUid { index_uid: String }, #[error("Task `{0}` not found.")] TaskNotFound(TaskId), - #[error("Query parameters to filter the tasks to delete are missing. Available query parameters are: `uid`, `indexUid`, `status`, `type`.")] + #[error("Query parameters to filter the tasks to delete are missing. Available query parameters are: `uids`, `indexUids`, `statuses`, `types`, `beforeEnqueuedAt`, `afterEnqueuedAt`, `beforeStartedAt`, `afterStartedAt`, `beforeFinishedAt`, `afterFinishedAt`.")] TaskDeletionWithEmptyQuery, - #[error("Query parameters to filter the tasks to cancel are missing. Available query parameters are: `uid`, `indexUid`, `status`, `type`.")] + #[error("Query parameters to filter the tasks to cancel are missing. Available query parameters are: `uids`, `indexUids`, `statuses`, `types`, `beforeEnqueuedAt`, `afterEnqueuedAt`, `beforeStartedAt`, `afterStartedAt`, `beforeFinishedAt`, `afterFinishedAt`.")] TaskCancelationWithEmptyQuery, #[error(transparent)] @@ -102,11 +102,11 @@ impl ErrorCode for Error { Error::IndexAlreadyExists(_) => Code::IndexAlreadyExists, Error::SwapDuplicateIndexesFound(_) => Code::DuplicateIndexFound, Error::SwapDuplicateIndexFound(_) => Code::DuplicateIndexFound, - Error::InvalidTaskDate { .. } => Code::InvalidTaskDate, - Error::InvalidTaskUids { .. } => Code::InvalidTaskUids, - Error::InvalidTaskStatuses { .. } => Code::InvalidTaskStatuses, - Error::InvalidTaskTypes { .. } => Code::InvalidTaskTypes, - Error::InvalidTaskCanceledBy { .. } => Code::InvalidTaskCanceledBy, + Error::InvalidTaskDate { .. } => Code::InvalidTaskDateFilter, + Error::InvalidTaskUids { .. } => Code::InvalidTaskUidsFilter, + Error::InvalidTaskStatuses { .. } => Code::InvalidTaskStatusesFilter, + Error::InvalidTaskTypes { .. } => Code::InvalidTaskTypesFilter, + Error::InvalidTaskCanceledBy { .. } => Code::InvalidTaskCanceledByFilter, Error::InvalidIndexUid { .. } => Code::InvalidIndexUid, Error::TaskNotFound(_) => Code::TaskNotFound, Error::TaskDeletionWithEmptyQuery => Code::TaskDeletionWithEmptyQuery, diff --git a/meilisearch-http/tests/common/index.rs b/meilisearch-http/tests/common/index.rs index 6a488d8aa..dac3653f7 100644 --- a/meilisearch-http/tests/common/index.rs +++ b/meilisearch-http/tests/common/index.rs @@ -110,13 +110,13 @@ impl Index<'_> { self.service.get(url).await } - pub async fn filtered_tasks(&self, type_: &[&str], status: &[&str]) -> (Value, StatusCode) { + pub async fn filtered_tasks(&self, types: &[&str], statuses: &[&str]) -> (Value, StatusCode) { let mut url = format!("/tasks?indexUids={}", self.uid); - if !type_.is_empty() { - let _ = write!(url, "&types={}", type_.join(",")); + if !types.is_empty() { + let _ = write!(url, "&types={}", types.join(",")); } - if !status.is_empty() { - let _ = write!(url, "&statuses={}", status.join(",")); + if !statuses.is_empty() { + let _ = write!(url, "&statuses={}", statuses.join(",")); } self.service.get(url).await } diff --git a/meilisearch-http/tests/common/server.rs b/meilisearch-http/tests/common/server.rs index b7ddc772c..3f72248c5 100644 --- a/meilisearch-http/tests/common/server.rs +++ b/meilisearch-http/tests/common/server.rs @@ -132,6 +132,10 @@ impl Server { self.service.get("/tasks").await } + pub async fn tasks_filter(&self, filter: Value) -> (Value, StatusCode) { + self.service.get(format!("/tasks?{}", yaup::to_string(&filter).unwrap())).await + } + pub async fn get_dump_status(&self, uid: &str) -> (Value, StatusCode) { self.service.get(format!("/dumps/{}/status", uid)).await } @@ -144,13 +148,13 @@ impl Server { self.service.post("/swap-indexes", value).await } - pub async fn cancel_task(&self, value: Value) -> (Value, StatusCode) { + pub async fn cancel_tasks(&self, value: Value) -> (Value, StatusCode) { self.service .post(format!("/tasks/cancel?{}", yaup::to_string(&value).unwrap()), json!(null)) .await } - pub async fn delete_task(&self, value: Value) -> (Value, StatusCode) { + pub async fn delete_tasks(&self, value: Value) -> (Value, StatusCode) { self.service.delete(format!("/tasks?{}", yaup::to_string(&value).unwrap())).await } diff --git a/meilisearch-http/tests/tasks/mod.rs b/meilisearch-http/tests/tasks/mod.rs index 145bb1b01..222906f6e 100644 --- a/meilisearch-http/tests/tasks/mod.rs +++ b/meilisearch-http/tests/tasks/mod.rs @@ -1,4 +1,4 @@ -use meili_snap::insta::assert_json_snapshot; +use meili_snap::insta::{self, assert_json_snapshot}; use serde_json::json; use time::format_description::well_known::Rfc3339; use time::OffsetDateTime; @@ -173,6 +173,131 @@ async fn list_tasks_status_and_type_filtered() { assert_eq!(response["results"].as_array().unwrap().len(), 2); } +#[actix_rt::test] +async fn get_task_filter_error() { + let server = Server::new().await; + + let (response, code) = server.tasks_filter(json!( { "lol": "pied" })).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Query deserialize error: unknown field `lol`", + "code": "bad_request", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#bad_request" + } + "###); + + let (response, code) = server.tasks_filter(json!( { "uids": "pied" })).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Task uid `pied` is invalid. It should only contain numeric characters.", + "code": "invalid_task_uids_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_task_uids_filter" + } + "###); + + let (response, code) = server.tasks_filter(json!( { "from": "pied" })).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Query deserialize error: invalid digit found in string", + "code": "bad_request", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#bad_request" + } + "###); + + let (response, code) = server.tasks_filter(json!( { "beforeStartedAt": "pied" })).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Task `beforeStartedAt` `pied` is invalid. It should follow the YYYY-MM-DD or RFC 3339 date-time format.", + "code": "invalid_task_date_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_task_date_filter" + } + "###); +} + +#[actix_rt::test] +async fn delete_task_filter_error() { + let server = Server::new().await; + + let (response, code) = server.delete_tasks(json!(null)).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Query parameters to filter the tasks to delete are missing. Available query parameters are: `uids`, `indexUids`, `statuses`, `types`, `beforeEnqueuedAt`, `afterEnqueuedAt`, `beforeStartedAt`, `afterStartedAt`, `beforeFinishedAt`, `afterFinishedAt`.", + "code": "missing_task_filters", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#missing_task_filters" + } + "###); + + let (response, code) = server.delete_tasks(json!({ "lol": "pied" })).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Query deserialize error: unknown field `lol`", + "code": "bad_request", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#bad_request" + } + "###); + + let (response, code) = server.delete_tasks(json!({ "uids": "pied" })).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Task uid `pied` is invalid. It should only contain numeric characters.", + "code": "invalid_task_uids_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_task_uids_filter" + } + "###); +} + +#[actix_rt::test] +async fn cancel_task_filter_error() { + let server = Server::new().await; + + let (response, code) = server.cancel_tasks(json!(null)).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Query parameters to filter the tasks to cancel are missing. Available query parameters are: `uids`, `indexUids`, `statuses`, `types`, `beforeEnqueuedAt`, `afterEnqueuedAt`, `beforeStartedAt`, `afterStartedAt`, `beforeFinishedAt`, `afterFinishedAt`.", + "code": "missing_task_filters", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#missing_task_filters" + } + "###); + + let (response, code) = server.cancel_tasks(json!({ "lol": "pied" })).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Query deserialize error: unknown field `lol`", + "code": "bad_request", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#bad_request" + } + "###); + + let (response, code) = server.cancel_tasks(json!({ "uids": "pied" })).await; + assert_eq!(code, 400, "{}", response); + insta::assert_json_snapshot!(response, @r###" + { + "message": "Task uid `pied` is invalid. It should only contain numeric characters.", + "code": "invalid_task_uids_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_task_uids_filter" + } + "###); +} + macro_rules! assert_valid_summarized_task { ($response:expr, $task_type:literal, $index:literal) => {{ assert_eq!($response.as_object().unwrap().len(), 5); @@ -789,7 +914,7 @@ async fn test_summarized_task_cancelation() { // to avoid being flaky we're only going to cancel an already finished task :( index.create(None).await; index.wait_task(0).await; - server.cancel_task(json!({ "uids": [0] })).await; + server.cancel_tasks(json!({ "uids": [0] })).await; index.wait_task(1).await; let (task, _) = index.get_task(1).await; assert_json_snapshot!(task, @@ -822,7 +947,7 @@ async fn test_summarized_task_deletion() { // to avoid being flaky we're only going to delete an already finished task :( index.create(None).await; index.wait_task(0).await; - server.delete_task(json!({ "uids": [0] })).await; + server.delete_tasks(json!({ "uids": [0] })).await; index.wait_task(1).await; let (task, _) = index.get_task(1).await; assert_json_snapshot!(task, From b4434dcad20c218aa4305414aed9b3113d9749b4 Mon Sep 17 00:00:00 2001 From: Tamo Date: Mon, 14 Nov 2022 23:26:50 +0100 Subject: [PATCH 2/3] rename the error codes for the sake of consistency --- meilisearch-types/src/error.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/meilisearch-types/src/error.rs b/meilisearch-types/src/error.rs index c81241741..5c0e1d9b8 100644 --- a/meilisearch-types/src/error.rs +++ b/meilisearch-types/src/error.rs @@ -147,11 +147,11 @@ pub enum Code { MissingMasterKey, NoSpaceLeftOnDevice, DumpNotFound, - InvalidTaskDate, - InvalidTaskStatuses, - InvalidTaskTypes, - InvalidTaskCanceledBy, - InvalidTaskUids, + InvalidTaskDateFilter, + InvalidTaskStatusesFilter, + InvalidTaskTypesFilter, + InvalidTaskCanceledByFilter, + InvalidTaskUidsFilter, TaskNotFound, TaskDeletionWithEmptyQuery, TaskCancelationWithEmptyQuery, @@ -243,19 +243,19 @@ impl Code { MissingMasterKey => { ErrCode::authentication("missing_master_key", StatusCode::UNAUTHORIZED) } - InvalidTaskDate => { + InvalidTaskDateFilter => { ErrCode::invalid("invalid_task_date_filter", StatusCode::BAD_REQUEST) } - InvalidTaskUids => { + InvalidTaskUidsFilter => { ErrCode::invalid("invalid_task_uids_filter", StatusCode::BAD_REQUEST) } - InvalidTaskStatuses => { + InvalidTaskStatusesFilter => { ErrCode::invalid("invalid_task_statuses_filter", StatusCode::BAD_REQUEST) } - InvalidTaskTypes => { + InvalidTaskTypesFilter => { ErrCode::invalid("invalid_task_types_filter", StatusCode::BAD_REQUEST) } - InvalidTaskCanceledBy => { + InvalidTaskCanceledByFilter => { ErrCode::invalid("invalid_task_canceled_by_filter", StatusCode::BAD_REQUEST) } TaskNotFound => ErrCode::invalid("task_not_found", StatusCode::NOT_FOUND), From 684b90066d5a9d2369964fddccf000c3da665253 Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 16 Nov 2022 11:34:39 +0100 Subject: [PATCH 3/3] update the analytics on the search route --- .../src/analytics/segment_analytics.rs | 42 ++++++++++++++++--- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/meilisearch-http/src/analytics/segment_analytics.rs b/meilisearch-http/src/analytics/segment_analytics.rs index 8028aee36..4a9d7aee7 100644 --- a/meilisearch-http/src/analytics/segment_analytics.rs +++ b/meilisearch-http/src/analytics/segment_analytics.rs @@ -467,11 +467,18 @@ pub struct SearchAggregator { finite_pagination: usize, // formatting + max_attributes_to_retrieve: usize, + max_attributes_to_highlight: usize, highlight_pre_tag: bool, highlight_post_tag: bool, + max_attributes_to_crop: usize, crop_marker: bool, show_matches_position: bool, crop_length: bool, + + // facets + facets_sum_of_terms: usize, + facets_total_number_of_facets: usize, } impl SearchAggregator { @@ -552,16 +559,19 @@ impl SearchAggregator { for user_agent in other.user_agents.into_iter() { self.user_agents.insert(user_agent); } + // request self.total_received = self.total_received.saturating_add(other.total_received); self.total_succeeded = self.total_succeeded.saturating_add(other.total_succeeded); self.time_spent.append(&mut other.time_spent); + // sort self.sort_with_geo_point |= other.sort_with_geo_point; self.sort_sum_of_criteria_terms = self.sort_sum_of_criteria_terms.saturating_add(other.sort_sum_of_criteria_terms); self.sort_total_number_of_criteria = self.sort_total_number_of_criteria.saturating_add(other.sort_total_number_of_criteria); + // filter self.filter_with_geo_radius |= other.filter_with_geo_radius; self.filter_sum_of_criteria_terms = @@ -576,20 +586,34 @@ impl SearchAggregator { // q self.max_terms_number = self.max_terms_number.max(other.max_terms_number); - for (key, value) in other.matching_strategy.into_iter() { - let matching_strategy = self.matching_strategy.entry(key).or_insert(0); - *matching_strategy = matching_strategy.saturating_add(value); - } // pagination self.max_limit = self.max_limit.max(other.max_limit); self.max_offset = self.max_offset.max(other.max_offset); self.finite_pagination += other.finite_pagination; + // formatting + self.max_attributes_to_retrieve = + self.max_attributes_to_retrieve.max(other.max_attributes_to_retrieve); + self.max_attributes_to_highlight = + self.max_attributes_to_highlight.max(other.max_attributes_to_highlight); self.highlight_pre_tag |= other.highlight_pre_tag; self.highlight_post_tag |= other.highlight_post_tag; + self.max_attributes_to_crop = self.max_attributes_to_crop.max(other.max_attributes_to_crop); self.crop_marker |= other.crop_marker; self.show_matches_position |= other.show_matches_position; self.crop_length |= other.crop_length; + + // facets + self.facets_sum_of_terms = + self.facets_sum_of_terms.saturating_add(other.facets_sum_of_terms); + self.facets_total_number_of_facets = + self.facets_total_number_of_facets.saturating_add(other.facets_total_number_of_facets); + + // matching strategy + for (key, value) in other.matching_strategy.into_iter() { + let matching_strategy = self.matching_strategy.entry(key).or_insert(0); + *matching_strategy = matching_strategy.saturating_add(value); + } } pub fn into_event(self, user: &User, event_name: &str) -> Option { @@ -622,7 +646,6 @@ impl SearchAggregator { }, "q": { "max_terms_number": self.max_terms_number, - "most_used_matching_strategy": self.matching_strategy.iter().max_by_key(|(_, v)| *v).map(|(k, _)| json!(k)).unwrap_or_else(|| json!(null)), }, "pagination": { "max_limit": self.max_limit, @@ -630,12 +653,21 @@ impl SearchAggregator { "finite_pagination": self.finite_pagination > self.total_received / 2, }, "formatting": { + "max_attributes_to_retrieve": self.max_attributes_to_retrieve, + "max_attributes_to_highlight": self.max_attributes_to_highlight, "highlight_pre_tag": self.highlight_pre_tag, "highlight_post_tag": self.highlight_post_tag, + "max_attributes_to_crop": self.max_attributes_to_crop, "crop_marker": self.crop_marker, "show_matches_position": self.show_matches_position, "crop_length": self.crop_length, }, + "facets": { + "avg_facets_number": format!("{:.2}", self.facets_sum_of_terms as f64 / self.facets_total_number_of_facets as f64), + }, + "matching_strategy": { + "most_used_strategy": self.matching_strategy.iter().max_by_key(|(_, v)| *v).map(|(k, _)| json!(k)).unwrap_or_else(|| json!(null)), + } }); Some(Track {