From 769576fd9465ff4c19aa04d04d6f01bd76f237f0 Mon Sep 17 00:00:00 2001 From: Tamo Date: Mon, 13 Feb 2023 19:34:47 +0100 Subject: [PATCH] get rids of the whole error_message module since it has been integrated into the last version of deserr --- .../src/deserr/error_messages.rs | 330 ------------------ meilisearch-types/src/deserr/mod.rs | 49 ++- meilisearch-types/src/keys.rs | 3 +- meilisearch/src/routes/indexes/mod.rs | 3 +- meilisearch/tests/auth/errors.rs | 4 +- 5 files changed, 48 insertions(+), 341 deletions(-) delete mode 100644 meilisearch-types/src/deserr/error_messages.rs diff --git a/meilisearch-types/src/deserr/error_messages.rs b/meilisearch-types/src/deserr/error_messages.rs deleted file mode 100644 index f17263813..000000000 --- a/meilisearch-types/src/deserr/error_messages.rs +++ /dev/null @@ -1,330 +0,0 @@ -/*! -This module implements the error messages of deserialization errors. - -We try to: -1. Give a human-readable description of where the error originated. -2. Use the correct terms depending on the format of the request (json/query param) -3. Categorise the type of the error (e.g. missing field, wrong value type, unexpected error, etc.) - */ -use std::ops::ControlFlow; - -use deserr::{ErrorKind, IntoValue, ValueKind, ValuePointerRef}; - -use super::{DeserrJsonError, DeserrQueryParamError}; -use crate::error::{Code, ErrorCode}; - -/// Return a description of the given location in a Json, preceded by the given article. -/// e.g. `at .key1[8].key2`. If the location is the origin, the given article will not be -/// included in the description. -pub fn location_json_description(location: ValuePointerRef, article: &str) -> String { - fn rec(location: ValuePointerRef) -> String { - match location { - ValuePointerRef::Origin => String::new(), - ValuePointerRef::Key { key, prev } => rec(*prev) + "." + key, - ValuePointerRef::Index { index, prev } => format!("{}[{index}]", rec(*prev)), - } - } - match location { - ValuePointerRef::Origin => String::new(), - _ => { - format!("{article} `{}`", rec(location)) - } - } -} - -/// Return a description of the list of value kinds for a Json payload. -fn value_kinds_description_json(kinds: &[ValueKind]) -> String { - // Rank each value kind so that they can be sorted (and deduplicated) - // Having a predictable order helps with pattern matching - fn order(kind: &ValueKind) -> u8 { - match kind { - ValueKind::Null => 0, - ValueKind::Boolean => 1, - ValueKind::Integer => 2, - ValueKind::NegativeInteger => 3, - ValueKind::Float => 4, - ValueKind::String => 5, - ValueKind::Sequence => 6, - ValueKind::Map => 7, - } - } - // Return a description of a single value kind, preceded by an article - fn single_description(kind: &ValueKind) -> &'static str { - match kind { - ValueKind::Null => "null", - ValueKind::Boolean => "a boolean", - ValueKind::Integer => "a positive integer", - ValueKind::NegativeInteger => "a negative integer", - ValueKind::Float => "a number", - ValueKind::String => "a string", - ValueKind::Sequence => "an array", - ValueKind::Map => "an object", - } - } - - fn description_rec(kinds: &[ValueKind], count_items: &mut usize, message: &mut String) { - let (msg_part, rest): (_, &[ValueKind]) = match kinds { - [] => (String::new(), &[]), - [ValueKind::Integer | ValueKind::NegativeInteger, ValueKind::Float, rest @ ..] => { - ("a number".to_owned(), rest) - } - [ValueKind::Integer, ValueKind::NegativeInteger, ValueKind::Float, rest @ ..] => { - ("a number".to_owned(), rest) - } - [ValueKind::Integer, ValueKind::NegativeInteger, rest @ ..] => { - ("an integer".to_owned(), rest) - } - [a] => (single_description(a).to_owned(), &[]), - [a, rest @ ..] => (single_description(a).to_owned(), rest), - }; - - if rest.is_empty() { - if *count_items == 0 { - message.push_str(&msg_part); - } else if *count_items == 1 { - message.push_str(&format!(" or {msg_part}")); - } else { - message.push_str(&format!(", or {msg_part}")); - } - } else { - if *count_items == 0 { - message.push_str(&msg_part); - } else { - message.push_str(&format!(", {msg_part}")); - } - - *count_items += 1; - description_rec(rest, count_items, message); - } - } - - let mut kinds = kinds.to_owned(); - kinds.sort_by_key(order); - kinds.dedup(); - - if kinds.is_empty() { - // Should not happen ideally - "a different value".to_owned() - } else { - let mut message = String::new(); - description_rec(kinds.as_slice(), &mut 0, &mut message); - message - } -} - -/// Return the JSON string of the value preceded by a description of its kind -fn value_description_with_kind_json(v: &serde_json::Value) -> String { - match v.kind() { - ValueKind::Null => "null".to_owned(), - kind => { - format!( - "{}: `{}`", - value_kinds_description_json(&[kind]), - serde_json::to_string(v).unwrap() - ) - } - } -} - -impl deserr::DeserializeError for DeserrJsonError { - fn error( - _self_: Option, - error: deserr::ErrorKind, - location: ValuePointerRef, - ) -> ControlFlow { - let mut message = String::new(); - - message.push_str(&match error { - ErrorKind::IncorrectValueKind { actual, accepted } => { - let expected = value_kinds_description_json(accepted); - let received = value_description_with_kind_json(&serde_json::Value::from(actual)); - - let location = location_json_description(location, " at"); - - format!("Invalid value type{location}: expected {expected}, but found {received}") - } - ErrorKind::MissingField { field } => { - let location = location_json_description(location, " inside"); - format!("Missing field `{field}`{location}") - } - ErrorKind::UnknownKey { key, accepted } => { - let location = location_json_description(location, " inside"); - format!( - "Unknown field `{}`{location}: expected one of {}", - key, - accepted - .iter() - .map(|accepted| format!("`{}`", accepted)) - .collect::>() - .join(", ") - ) - } - ErrorKind::UnknownValue { value, accepted } => { - let location = location_json_description(location, " at"); - format!( - "Unknown value `{}`{location}: expected one of {}", - value, - accepted - .iter() - .map(|accepted| format!("`{}`", accepted)) - .collect::>() - .join(", "), - ) - } - ErrorKind::Unexpected { msg } => { - let location = location_json_description(location, " at"); - format!("Invalid value{location}: {msg}") - } - }); - - ControlFlow::Break(DeserrJsonError::new(message, C::default().error_code())) - } -} - -pub fn immutable_field_error(field: &str, accepted: &[&str], code: Code) -> DeserrJsonError { - let msg = format!( - "Immutable field `{field}`: expected one of {}", - accepted - .iter() - .map(|accepted| format!("`{}`", accepted)) - .collect::>() - .join(", ") - ); - - DeserrJsonError::new(msg, code) -} - -/// Return a description of the given location in query parameters, preceded by the -/// given article. e.g. `at key5[2]`. If the location is the origin, the given article -/// will not be included in the description. -pub fn location_query_param_description(location: ValuePointerRef, article: &str) -> String { - fn rec(location: ValuePointerRef) -> String { - match location { - ValuePointerRef::Origin => String::new(), - ValuePointerRef::Key { key, prev } => { - if matches!(prev, ValuePointerRef::Origin) { - key.to_owned() - } else { - rec(*prev) + "." + key - } - } - ValuePointerRef::Index { index, prev } => format!("{}[{index}]", rec(*prev)), - } - } - match location { - ValuePointerRef::Origin => String::new(), - _ => { - format!("{article} `{}`", rec(location)) - } - } -} - -impl deserr::DeserializeError for DeserrQueryParamError { - fn error( - _self_: Option, - error: deserr::ErrorKind, - location: ValuePointerRef, - ) -> ControlFlow { - let mut message = String::new(); - - message.push_str(&match error { - ErrorKind::IncorrectValueKind { actual, accepted } => { - let expected = value_kinds_description_query_param(accepted); - let received = value_description_with_kind_query_param(actual); - - let location = location_query_param_description(location, " for parameter"); - - format!("Invalid value type{location}: expected {expected}, but found {received}") - } - ErrorKind::MissingField { field } => { - let location = location_query_param_description(location, " inside"); - format!("Missing parameter `{field}`{location}") - } - ErrorKind::UnknownKey { key, accepted } => { - let location = location_query_param_description(location, " inside"); - format!( - "Unknown parameter `{}`{location}: expected one of {}", - key, - accepted - .iter() - .map(|accepted| format!("`{}`", accepted)) - .collect::>() - .join(", ") - ) - } - ErrorKind::UnknownValue { value, accepted } => { - let location = location_query_param_description(location, " for parameter"); - format!( - "Unknown value `{}`{location}: expected one of {}", - value, - accepted - .iter() - .map(|accepted| format!("`{}`", accepted)) - .collect::>() - .join(", "), - ) - } - ErrorKind::Unexpected { msg } => { - let location = location_query_param_description(location, " in parameter"); - format!("Invalid value{location}: {msg}") - } - }); - - ControlFlow::Break(DeserrQueryParamError::new(message, C::default().error_code())) - } -} - -/// Return a description of the list of value kinds for query parameters -/// Since query parameters are always treated as strings, we always return -/// "a string" for now. -fn value_kinds_description_query_param(_accepted: &[ValueKind]) -> String { - "a string".to_owned() -} - -fn value_description_with_kind_query_param(actual: deserr::Value) -> String { - match actual { - deserr::Value::Null => "null".to_owned(), - deserr::Value::Boolean(x) => format!("a boolean: `{x}`"), - deserr::Value::Integer(x) => format!("an integer: `{x}`"), - deserr::Value::NegativeInteger(x) => { - format!("an integer: `{x}`") - } - deserr::Value::Float(x) => { - format!("a number: `{x}`") - } - deserr::Value::String(x) => { - format!("a string: `{x}`") - } - deserr::Value::Sequence(_) => "multiple values".to_owned(), - deserr::Value::Map(_) => "multiple parameters".to_owned(), - } -} - -#[cfg(test)] -mod tests { - use deserr::ValueKind; - - use crate::deserr::error_messages::value_kinds_description_json; - - #[test] - fn test_value_kinds_description_json() { - insta::assert_display_snapshot!(value_kinds_description_json(&[]), @"a different value"); - - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Boolean]), @"a boolean"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Integer]), @"a positive integer"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::NegativeInteger]), @"a negative integer"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Integer]), @"a positive integer"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::String]), @"a string"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Sequence]), @"an array"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Map]), @"an object"); - - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Integer, ValueKind::Boolean]), @"a boolean or a positive integer"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Null, ValueKind::Integer]), @"null or a positive integer"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Sequence, ValueKind::NegativeInteger]), @"a negative integer or an array"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Integer, ValueKind::Float]), @"a number"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Integer, ValueKind::Float, ValueKind::NegativeInteger]), @"a number"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Integer, ValueKind::Float, ValueKind::NegativeInteger, ValueKind::Null]), @"null or a number"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Boolean, ValueKind::Integer, ValueKind::Float, ValueKind::NegativeInteger, ValueKind::Null]), @"null, a boolean, or a number"); - insta::assert_display_snapshot!(value_kinds_description_json(&[ValueKind::Null, ValueKind::Boolean, ValueKind::Integer, ValueKind::Float, ValueKind::NegativeInteger, ValueKind::Null]), @"null, a boolean, or a number"); - } -} diff --git a/meilisearch-types/src/deserr/mod.rs b/meilisearch-types/src/deserr/mod.rs index 49474a9e6..33213c953 100644 --- a/meilisearch-types/src/deserr/mod.rs +++ b/meilisearch-types/src/deserr/mod.rs @@ -3,9 +3,10 @@ use std::fmt; use std::marker::PhantomData; use std::ops::ControlFlow; -use deserr::{DeserializeError, MergeWithError, ValuePointerRef}; +use deserr::errors::{JsonError, QueryParamError}; +use deserr::{take_cf_content, DeserializeError, IntoValue, MergeWithError, ValuePointerRef}; -use crate::error::deserr_codes::{self, *}; +use crate::error::deserr_codes::*; use crate::error::{ unwrap_any, Code, DeserrParseBoolError, DeserrParseIntError, ErrorCode, InvalidTaskDateError, ParseOffsetDateTimeError, @@ -13,7 +14,6 @@ use crate::error::{ use crate::index_uid::IndexUidFormatError; use crate::tasks::{ParseTaskKindError, ParseTaskStatusError}; -pub mod error_messages; pub mod query_params; /// Marker type for the Json format @@ -21,8 +21,8 @@ pub struct DeserrJson; /// Marker type for the Query Parameter format pub struct DeserrQueryParam; -pub type DeserrJsonError = DeserrError; -pub type DeserrQueryParamError = DeserrError; +pub type DeserrJsonError = DeserrError; +pub type DeserrQueryParamError = DeserrError; /// A request deserialization error. /// @@ -80,6 +80,45 @@ impl MergeWithError for DeserrError< } } +impl DeserializeError for DeserrJsonError { + fn error( + _self_: Option, + error: deserr::ErrorKind, + location: ValuePointerRef, + ) -> ControlFlow { + ControlFlow::Break(DeserrJsonError::new( + take_cf_content(JsonError::error(None, error, location)).to_string(), + C::default().error_code(), + )) + } +} + +impl DeserializeError for DeserrQueryParamError { + fn error( + _self_: Option, + error: deserr::ErrorKind, + location: ValuePointerRef, + ) -> ControlFlow { + ControlFlow::Break(DeserrQueryParamError::new( + take_cf_content(QueryParamError::error(None, error, location)).to_string(), + C::default().error_code(), + )) + } +} + +pub fn immutable_field_error(field: &str, accepted: &[&str], code: Code) -> DeserrJsonError { + let msg = format!( + "Immutable field `{field}`: expected one of {}", + accepted + .iter() + .map(|accepted| format!("`{}`", accepted)) + .collect::>() + .join(", ") + ); + + DeserrJsonError::new(msg, code) +} + // Implement a convenience function to build a `missing_field` error macro_rules! make_missing_field_convenience_builder { ($err_code:ident, $fn_name:ident) => { diff --git a/meilisearch-types/src/keys.rs b/meilisearch-types/src/keys.rs index 7478391ba..87ed543d6 100644 --- a/meilisearch-types/src/keys.rs +++ b/meilisearch-types/src/keys.rs @@ -11,8 +11,7 @@ use time::macros::{format_description, time}; use time::{Date, OffsetDateTime, PrimitiveDateTime}; use uuid::Uuid; -use crate::deserr::error_messages::immutable_field_error; -use crate::deserr::{DeserrError, DeserrJsonError}; +use crate::deserr::{immutable_field_error, DeserrError, DeserrJsonError}; use crate::error::deserr_codes::*; use crate::error::{unwrap_any, Code, ErrorCode, ParseOffsetDateTimeError}; use crate::index_uid_pattern::{IndexUidPattern, IndexUidPatternFormatError}; diff --git a/meilisearch/src/routes/indexes/mod.rs b/meilisearch/src/routes/indexes/mod.rs index c087fe202..16b9faa24 100644 --- a/meilisearch/src/routes/indexes/mod.rs +++ b/meilisearch/src/routes/indexes/mod.rs @@ -5,9 +5,8 @@ use actix_web::{web, HttpRequest, HttpResponse}; use deserr::{DeserializeError, Deserr, ValuePointerRef}; use index_scheduler::IndexScheduler; use log::debug; -use meilisearch_types::deserr::error_messages::immutable_field_error; use meilisearch_types::deserr::query_params::Param; -use meilisearch_types::deserr::{DeserrJsonError, DeserrQueryParamError}; +use meilisearch_types::deserr::{immutable_field_error, DeserrJsonError, DeserrQueryParamError}; use meilisearch_types::error::deserr_codes::*; use meilisearch_types::error::{unwrap_any, Code, ResponseError}; use meilisearch_types::index_uid::IndexUid; diff --git a/meilisearch/tests/auth/errors.rs b/meilisearch/tests/auth/errors.rs index 0bfef878b..904bb182d 100644 --- a/meilisearch/tests/auth/errors.rs +++ b/meilisearch/tests/auth/errors.rs @@ -138,7 +138,7 @@ async fn create_api_key_bad_expires_at() { snapshot!(code, @"400 Bad Request"); snapshot!(json_string!(response), @r###" { - "message": "Unknown field `expires_at`: expected one of `description`, `name`, `uid`, `actions`, `indexes`, `expiresAt`", + "message": "Unknown field `expires_at`: did you mean `expiresAt`? expected one of `description`, `name`, `uid`, `actions`, `indexes`, `expiresAt`", "code": "bad_request", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#bad_request" @@ -150,7 +150,7 @@ async fn create_api_key_bad_expires_at() { snapshot!(code, @"400 Bad Request"); snapshot!(json_string!(response), @r###" { - "message": "Unknown field `expires_at`: expected one of `description`, `name`, `uid`, `actions`, `indexes`, `expiresAt`", + "message": "Unknown field `expires_at`: did you mean `expiresAt`? expected one of `description`, `name`, `uid`, `actions`, `indexes`, `expiresAt`", "code": "bad_request", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#bad_request"