From 113a061bee3f40134fab7a01f80af24ee4ed349b Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 22 Sep 2021 15:09:07 +0200 Subject: [PATCH 1/7] fix the error handling on the criterion side --- milli/src/criterion.rs | 181 +++++++++++++++++++++++++++++++++++------ milli/src/error.rs | 36 +++++++- 2 files changed, 189 insertions(+), 28 deletions(-) diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index c526a7e32..33112508c 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -53,10 +53,31 @@ impl FromStr for Criterion { Ok(AscDesc::Asc(Member::Field(field))) => Ok(Criterion::Asc(field)), Ok(AscDesc::Desc(Member::Field(field))) => Ok(Criterion::Desc(field)), Ok(AscDesc::Asc(Member::Geo(_))) | Ok(AscDesc::Desc(Member::Geo(_))) => { - Err(UserError::InvalidRankingRuleName { name: text.to_string() })? + Err(UserError::InvalidReservedRankingRuleNameSort { + name: "_geoPoint".to_string(), + })? } Err(UserError::InvalidAscDescSyntax { name }) => { - Err(UserError::InvalidRankingRuleName { name }.into()) + Err(UserError::InvalidRankingRuleName { name })? + } + Err(UserError::InvalidReservedAscDescSyntax { name }) + if name.starts_with("_geoPoint") => + { + Err(UserError::InvalidReservedRankingRuleNameSort { + name: "_geoPoint".to_string(), + } + .into()) + } + Err(UserError::InvalidReservedAscDescSyntax { name }) + if name.starts_with("_geoRadius") => + { + Err(UserError::InvalidReservedRankingRuleNameFilter { + name: "_geoRadius".to_string(), + } + .into()) + } + Err(UserError::InvalidReservedAscDescSyntax { name }) => { + Err(UserError::InvalidReservedRankingRuleName { name }.into()) } Err(error) => { Err(UserError::InvalidRankingRuleName { name: error.to_string() }.into()) @@ -80,20 +101,22 @@ impl FromStr for Member { Some(point) => { let (lat, long) = point .split_once(',') - .ok_or_else(|| UserError::InvalidRankingRuleName { name: text.to_string() }) + .ok_or_else(|| UserError::InvalidReservedAscDescSyntax { + name: text.to_string(), + }) .and_then(|(lat, long)| { lat.trim() .parse() .and_then(|lat| long.trim().parse().map(|long| (lat, long))) - .map_err(|_| UserError::InvalidRankingRuleName { + .map_err(|_| UserError::InvalidReservedAscDescSyntax { name: text.to_string(), }) })?; Ok(Member::Geo([lat, long])) } None => { - if is_reserved_keyword(text) { - return Err(UserError::InvalidReservedRankingRuleName { + if is_reserved_keyword(text) || text.starts_with("_geoRadius(") { + return Err(UserError::InvalidReservedAscDescSyntax { name: text.to_string(), })?; } @@ -191,14 +214,15 @@ impl fmt::Display for Criterion { #[cfg(test)] mod tests { + use big_s::S; + use AscDesc::*; + use Member::*; + use UserError::*; + use super::*; #[test] fn parse_asc_desc() { - use big_s::S; - use AscDesc::*; - use Member::*; - let valid_req = [ ("truc:asc", Asc(Field(S("truc")))), ("bidule:desc", Desc(Field(S("bidule")))), @@ -216,28 +240,52 @@ mod tests { ]; for (req, expected) in valid_req { - let res = req.parse(); - assert!(res.is_ok(), "Failed to parse `{}`, was expecting `{:?}`", req, expected); - assert_eq!(expected, res.unwrap()); + let res = req.parse::(); + assert!( + res.is_ok(), + "Failed to parse `{}`, was expecting `{:?}` but instead got `{:?}`", + req, + expected, + res + ); + assert_eq!(res.unwrap(), expected); } let invalid_req = [ - "truc:machin", - "truc:deesc", - "truc:asc:deesc", - "42desc", - "_geoPoint:asc", - "_geoDistance:asc", - "_geoPoint(42.12 , 59.598)", - "_geoPoint(42.12 , 59.598):deesc", - "_geoPoint(42.12 , 59.598):machin", - "_geoPoint(42.12 , 59.598):asc:aasc", - "_geoPoint(42,12 , 59,598):desc", - "_geoPoint(35, 85, 75):asc", - "_geoPoint(18):asc", + ("truc:machin", InvalidAscDescSyntax { name: S("truc:machin") }), + ("truc:deesc", InvalidAscDescSyntax { name: S("truc:deesc") }), + ("truc:asc:deesc", InvalidAscDescSyntax { name: S("truc:asc:deesc") }), + ("42desc", InvalidAscDescSyntax { name: S("42desc") }), + ("_geoPoint:asc", InvalidReservedAscDescSyntax { name: S("_geoPoint") }), + ("_geoDistance:asc", InvalidReservedAscDescSyntax { name: S("_geoDistance") }), + ( + "_geoPoint(42.12 , 59.598)", + InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598)") }, + ), + ( + "_geoPoint(42.12 , 59.598):deesc", + InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):deesc") }, + ), + ( + "_geoPoint(42.12 , 59.598):machin", + InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):machin") }, + ), + ( + "_geoPoint(42.12 , 59.598):asc:aasc", + InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):asc:aasc") }, + ), + ( + "_geoPoint(42,12 , 59,598):desc", + InvalidReservedAscDescSyntax { name: S("_geoPoint(42,12 , 59,598)") }, + ), + ( + "_geoPoint(35, 85, 75):asc", + InvalidReservedAscDescSyntax { name: S("_geoPoint(35, 85, 75)") }, + ), + ("_geoPoint(18):asc", InvalidReservedAscDescSyntax { name: S("_geoPoint(18)") }), ]; - for req in invalid_req { + for (req, expected_error) in invalid_req { let res = req.parse::(); assert!( res.is_err(), @@ -245,6 +293,85 @@ mod tests { req, res, ); + let res = res.unwrap_err(); + assert_eq!( + res.to_string(), + expected_error.to_string(), + "Bad error for input {}: got `{:?}` instead of `{:?}`", + req, + res, + expected_error + ); + } + } + + #[test] + fn parse_criterion() { + let valid_criteria = [ + ("words", Criterion::Words), + ("typo", Criterion::Typo), + ("proximity", Criterion::Proximity), + ("attribute", Criterion::Attribute), + ("sort", Criterion::Sort), + ("exactness", Criterion::Exactness), + ("price:asc", Criterion::Asc(S("price"))), + ("price:desc", Criterion::Desc(S("price"))), + ("price:asc:desc", Criterion::Desc(S("price:asc"))), + ("truc:machin:desc", Criterion::Desc(S("truc:machin"))), + ("hello-world!:desc", Criterion::Desc(S("hello-world!"))), + ("it's spacy over there:asc", Criterion::Asc(S("it's spacy over there"))), + ]; + + for (input, expected) in valid_criteria { + let res = input.parse::(); + assert!( + res.is_ok(), + "Failed to parse `{}`, was expecting `{:?}` but instead got `{:?}`", + input, + expected, + res + ); + assert_eq!(res.unwrap(), expected); + } + + let invalid_criteria = [ + ("words suffix", InvalidRankingRuleName { name: S("words suffix") }), + ("prefix typo", InvalidRankingRuleName { name: S("prefix typo") }), + ("proximity attribute", InvalidRankingRuleName { name: S("proximity attribute") }), + ("price", InvalidRankingRuleName { name: S("price") }), + ("asc:price", InvalidRankingRuleName { name: S("asc:price") }), + ("price:deesc", InvalidRankingRuleName { name: S("price:deesc") }), + ("price:aasc", InvalidRankingRuleName { name: S("price:aasc") }), + ("price:asc and desc", InvalidRankingRuleName { name: S("price:asc and desc") }), + ("price:asc:truc", InvalidRankingRuleName { name: S("price:asc:truc") }), + ("_geo:asc", InvalidReservedRankingRuleName { name: S("_geo") }), + ("_geoDistance:asc", InvalidReservedRankingRuleName { name: S("_geoDistance") }), + ("_geoPoint:asc", InvalidReservedRankingRuleNameSort { name: S("_geoPoint") }), + ("_geoPoint(42, 75):asc", InvalidReservedRankingRuleNameSort { name: S("_geoPoint") }), + ("_geoRadius:asc", InvalidReservedRankingRuleNameFilter { name: S("_geoRadius") }), + ( + "_geoRadius(42, 75, 59):asc", + InvalidReservedRankingRuleNameFilter { name: S("_geoRadius") }, + ), + ]; + + for (input, expected) in invalid_criteria { + let res = input.parse::(); + assert!( + res.is_err(), + "Should no be able to parse `{}`, was expecting an error but instead got: `{:?}`", + input, + res + ); + let res = res.unwrap_err(); + assert_eq!( + res.to_string(), + expected.to_string(), + "Bad error for input {}: got `{:?}` instead of `{:?}`", + input, + res, + expected + ); } } } diff --git a/milli/src/error.rs b/milli/src/error.rs index fe0ac2cf7..2e2d3088e 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -57,14 +57,18 @@ pub enum UserError { AttributeLimitReached, DocumentLimitReached, InvalidAscDescSyntax { name: String }, + InvalidReservedAscDescSyntax { name: String }, InvalidDocumentId { document_id: Value }, InvalidFacetsDistribution { invalid_facets_name: HashSet }, InvalidFilter(pest::error::Error), InvalidFilterAttribute(pest::error::Error), InvalidSortName { name: String }, + InvalidReservedSortName { name: String }, InvalidGeoField { document_id: Value, object: Value }, InvalidRankingRuleName { name: String }, InvalidReservedRankingRuleName { name: String }, + InvalidReservedRankingRuleNameSort { name: String }, + InvalidReservedRankingRuleNameFilter { name: String }, InvalidSortableAttribute { field: String, valid_fields: HashSet }, SortRankingRuleMissing, InvalidStoreFile, @@ -230,10 +234,40 @@ impl fmt::Display for UserError { "the document with the id: {} contains an invalid _geo field: {}", document_id, object ), - Self::InvalidRankingRuleName { name } => write!(f, "invalid criterion {}", name), + Self::InvalidRankingRuleName { name } => write!(f, "invalid ranking rule {}", name), + Self::InvalidReservedAscDescSyntax { name } => { + write!( + f, + "{} is a reserved keyword and thus can't be used as a asc/desc rule", + name + ) + } Self::InvalidReservedRankingRuleName { name } => { write!(f, "{} is a reserved keyword and thus can't be used as a ranking rule", name) } + Self::InvalidReservedRankingRuleNameSort { name } => { + write!( + f, + "{0} is a reserved keyword and thus can't be used as a ranking rule. \ +{0} can only be used for sorting at search time", + name + ) + } + Self::InvalidReservedRankingRuleNameFilter { name } => { + write!( + f, + "{0} is a reserved keyword and thus can't be used as a ranking rule. \ +{0} can only be used for filtering at search time", + name + ) + } + Self::InvalidReservedSortName { name } => { + write!( + f, + "{} is a reserved keyword and thus can't be used as a sort expression", + name + ) + } Self::InvalidDocumentId { document_id } => { let json = serde_json::to_string(document_id).unwrap(); write!( From 257e621d4011857d998d9c2b51996a60edba4c2c Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 22 Sep 2021 15:18:39 +0200 Subject: [PATCH 2/7] create an asc_desc module --- milli/src/asc_desc.rs | 228 +++++++++++++++++++++++++++++++ milli/src/criterion.rs | 186 +------------------------ milli/src/lib.rs | 4 +- milli/src/search/criteria/mod.rs | 3 +- milli/src/search/mod.rs | 3 +- 5 files changed, 235 insertions(+), 189 deletions(-) create mode 100644 milli/src/asc_desc.rs diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs new file mode 100644 index 000000000..d68d5b6a2 --- /dev/null +++ b/milli/src/asc_desc.rs @@ -0,0 +1,228 @@ +//! This module provide the `AscDesc` type and define a all the errors related to this type + +use std::fmt; +use std::str::FromStr; + +use serde::{Deserialize, Serialize}; + +use crate::error::is_reserved_keyword; +use crate::CriterionError; + +/// This error type is never supposed to be shown to the end user. +/// You must always cast it to a sort error or a criterion error. +#[derive(Debug)] +pub enum AscDescError { + InvalidSyntax { name: String }, + ReservedKeyword { name: String }, +} + +impl fmt::Display for AscDescError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::InvalidSyntax { name } => { + write!(f, "invalid asc/desc syntax for {}", name) + } + Self::ReservedKeyword { name } => { + write!( + f, + "{} is a reserved keyword and thus can't be used as a asc/desc rule", + name + ) + } + } + } +} + +impl From for CriterionError { + fn from(error: AscDescError) -> Self { + match error { + AscDescError::InvalidSyntax { name } => CriterionError::InvalidName { name }, + AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => { + CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() } + } + AscDescError::ReservedKeyword { name } if name.starts_with("_geoRadius") => { + CriterionError::ReservedNameForFilter { name: "_geoRadius".to_string() } + } + AscDescError::ReservedKeyword { name } => (CriterionError::ReservedName { name }), + } + } +} + +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] +pub enum Member { + Field(String), + Geo([f64; 2]), +} + +impl FromStr for Member { + type Err = AscDescError; + + fn from_str(text: &str) -> Result { + match text.strip_prefix("_geoPoint(").and_then(|text| text.strip_suffix(")")) { + Some(point) => { + let (lat, long) = point + .split_once(',') + .ok_or_else(|| AscDescError::ReservedKeyword { name: text.to_string() }) + .and_then(|(lat, long)| { + lat.trim() + .parse() + .and_then(|lat| long.trim().parse().map(|long| (lat, long))) + .map_err(|_| AscDescError::ReservedKeyword { name: text.to_string() }) + })?; + Ok(Member::Geo([lat, long])) + } + None => { + if is_reserved_keyword(text) || text.starts_with("_geoRadius(") { + return Err(AscDescError::ReservedKeyword { name: text.to_string() })?; + } + Ok(Member::Field(text.to_string())) + } + } + } +} + +impl fmt::Display for Member { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Member::Field(name) => f.write_str(name), + Member::Geo([lat, lng]) => write!(f, "_geoPoint({}, {})", lat, lng), + } + } +} + +impl Member { + pub fn field(&self) -> Option<&str> { + match self { + Member::Field(field) => Some(field), + Member::Geo(_) => None, + } + } + + pub fn geo_point(&self) -> Option<&[f64; 2]> { + match self { + Member::Geo(point) => Some(point), + Member::Field(_) => None, + } + } +} + +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] +pub enum AscDesc { + Asc(Member), + Desc(Member), +} + +impl AscDesc { + pub fn member(&self) -> &Member { + match self { + AscDesc::Asc(member) => member, + AscDesc::Desc(member) => member, + } + } + + pub fn field(&self) -> Option<&str> { + self.member().field() + } +} + +impl FromStr for AscDesc { + type Err = AscDescError; + + /// Since we don't know if this was deserialized for a criterion or a sort we just return a + /// string and let the caller create his own error + fn from_str(text: &str) -> Result { + match text.rsplit_once(':') { + Some((left, "asc")) => Ok(AscDesc::Asc(left.parse()?)), + Some((left, "desc")) => Ok(AscDesc::Desc(left.parse()?)), + _ => Err(AscDescError::InvalidSyntax { name: text.to_string() }), + } + } +} + +#[cfg(test)] +mod tests { + use big_s::S; + use AscDesc::*; + use AscDescError::*; + use Member::*; + + use super::*; + + #[test] + fn parse_asc_desc() { + let valid_req = [ + ("truc:asc", Asc(Field(S("truc")))), + ("bidule:desc", Desc(Field(S("bidule")))), + ("a-b:desc", Desc(Field(S("a-b")))), + ("a:b:desc", Desc(Field(S("a:b")))), + ("a12:asc", Asc(Field(S("a12")))), + ("42:asc", Asc(Field(S("42")))), + ("_geoPoint(42, 59):asc", Asc(Geo([42., 59.]))), + ("_geoPoint(42.459, 59):desc", Desc(Geo([42.459, 59.]))), + ("_geoPoint(42, 59.895):desc", Desc(Geo([42., 59.895]))), + ("_geoPoint(42, 59.895):desc", Desc(Geo([42., 59.895]))), + ("_geoPoint(42.0002, 59.895):desc", Desc(Geo([42.0002, 59.895]))), + ("_geoPoint(42., 59.):desc", Desc(Geo([42., 59.]))), + ("truc(12, 13):desc", Desc(Field(S("truc(12, 13)")))), + ]; + + for (req, expected) in valid_req { + let res = req.parse::(); + assert!( + res.is_ok(), + "Failed to parse `{}`, was expecting `{:?}` but instead got `{:?}`", + req, + expected, + res + ); + assert_eq!(res.unwrap(), expected); + } + + let invalid_req = [ + ("truc:machin", InvalidSyntax { name: S("truc:machin") }), + ("truc:deesc", InvalidSyntax { name: S("truc:deesc") }), + ("truc:asc:deesc", InvalidSyntax { name: S("truc:asc:deesc") }), + ("42desc", InvalidSyntax { name: S("42desc") }), + ("_geoPoint:asc", ReservedKeyword { name: S("_geoPoint") }), + ("_geoDistance:asc", ReservedKeyword { name: S("_geoDistance") }), + ("_geoPoint(42.12 , 59.598)", InvalidSyntax { name: S("_geoPoint(42.12 , 59.598)") }), + ( + "_geoPoint(42.12 , 59.598):deesc", + InvalidSyntax { name: S("_geoPoint(42.12 , 59.598):deesc") }, + ), + ( + "_geoPoint(42.12 , 59.598):machin", + InvalidSyntax { name: S("_geoPoint(42.12 , 59.598):machin") }, + ), + ( + "_geoPoint(42.12 , 59.598):asc:aasc", + InvalidSyntax { name: S("_geoPoint(42.12 , 59.598):asc:aasc") }, + ), + ( + "_geoPoint(42,12 , 59,598):desc", + ReservedKeyword { name: S("_geoPoint(42,12 , 59,598)") }, + ), + ("_geoPoint(35, 85, 75):asc", ReservedKeyword { name: S("_geoPoint(35, 85, 75)") }), + ("_geoPoint(18):asc", ReservedKeyword { name: S("_geoPoint(18)") }), + ]; + + for (req, expected_error) in invalid_req { + let res = req.parse::(); + assert!( + res.is_err(), + "Should no be able to parse `{}`, was expecting an error but instead got: `{:?}`", + req, + res, + ); + let res = res.unwrap_err(); + assert_eq!( + res.to_string(), + expected_error.to_string(), + "Bad error for input {}: got `{:?}` instead of `{:?}`", + req, + res, + expected_error + ); + } + } +} diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index 33112508c..acc0148bb 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -3,7 +3,8 @@ use std::str::FromStr; use serde::{Deserialize, Serialize}; -use crate::error::{is_reserved_keyword, Error, UserError}; +use crate::error::{Error, UserError}; +use crate::{AscDesc, Member}; #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] pub enum Criterion { @@ -87,103 +88,6 @@ impl FromStr for Criterion { } } -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] -pub enum Member { - Field(String), - Geo([f64; 2]), -} - -impl FromStr for Member { - type Err = UserError; - - fn from_str(text: &str) -> Result { - match text.strip_prefix("_geoPoint(").and_then(|text| text.strip_suffix(")")) { - Some(point) => { - let (lat, long) = point - .split_once(',') - .ok_or_else(|| UserError::InvalidReservedAscDescSyntax { - name: text.to_string(), - }) - .and_then(|(lat, long)| { - lat.trim() - .parse() - .and_then(|lat| long.trim().parse().map(|long| (lat, long))) - .map_err(|_| UserError::InvalidReservedAscDescSyntax { - name: text.to_string(), - }) - })?; - Ok(Member::Geo([lat, long])) - } - None => { - if is_reserved_keyword(text) || text.starts_with("_geoRadius(") { - return Err(UserError::InvalidReservedAscDescSyntax { - name: text.to_string(), - })?; - } - Ok(Member::Field(text.to_string())) - } - } - } -} - -impl fmt::Display for Member { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Member::Field(name) => f.write_str(name), - Member::Geo([lat, lng]) => write!(f, "_geoPoint({}, {})", lat, lng), - } - } -} - -impl Member { - pub fn field(&self) -> Option<&str> { - match self { - Member::Field(field) => Some(field), - Member::Geo(_) => None, - } - } - - pub fn geo_point(&self) -> Option<&[f64; 2]> { - match self { - Member::Geo(point) => Some(point), - Member::Field(_) => None, - } - } -} - -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] -pub enum AscDesc { - Asc(Member), - Desc(Member), -} - -impl AscDesc { - pub fn member(&self) -> &Member { - match self { - AscDesc::Asc(member) => member, - AscDesc::Desc(member) => member, - } - } - - pub fn field(&self) -> Option<&str> { - self.member().field() - } -} - -impl FromStr for AscDesc { - type Err = UserError; - - /// Since we don't know if this was deserialized for a criterion or a sort we just return a - /// string and let the caller create his own error - fn from_str(text: &str) -> Result { - match text.rsplit_once(':') { - Some((left, "asc")) => Ok(AscDesc::Asc(left.parse()?)), - Some((left, "desc")) => Ok(AscDesc::Desc(left.parse()?)), - _ => Err(UserError::InvalidAscDescSyntax { name: text.to_string() }), - } - } -} - pub fn default_criteria() -> Vec { vec![ Criterion::Words, @@ -215,96 +119,10 @@ impl fmt::Display for Criterion { #[cfg(test)] mod tests { use big_s::S; - use AscDesc::*; - use Member::*; use UserError::*; use super::*; - #[test] - fn parse_asc_desc() { - let valid_req = [ - ("truc:asc", Asc(Field(S("truc")))), - ("bidule:desc", Desc(Field(S("bidule")))), - ("a-b:desc", Desc(Field(S("a-b")))), - ("a:b:desc", Desc(Field(S("a:b")))), - ("a12:asc", Asc(Field(S("a12")))), - ("42:asc", Asc(Field(S("42")))), - ("_geoPoint(42, 59):asc", Asc(Geo([42., 59.]))), - ("_geoPoint(42.459, 59):desc", Desc(Geo([42.459, 59.]))), - ("_geoPoint(42, 59.895):desc", Desc(Geo([42., 59.895]))), - ("_geoPoint(42, 59.895):desc", Desc(Geo([42., 59.895]))), - ("_geoPoint(42.0002, 59.895):desc", Desc(Geo([42.0002, 59.895]))), - ("_geoPoint(42., 59.):desc", Desc(Geo([42., 59.]))), - ("truc(12, 13):desc", Desc(Field(S("truc(12, 13)")))), - ]; - - for (req, expected) in valid_req { - let res = req.parse::(); - assert!( - res.is_ok(), - "Failed to parse `{}`, was expecting `{:?}` but instead got `{:?}`", - req, - expected, - res - ); - assert_eq!(res.unwrap(), expected); - } - - let invalid_req = [ - ("truc:machin", InvalidAscDescSyntax { name: S("truc:machin") }), - ("truc:deesc", InvalidAscDescSyntax { name: S("truc:deesc") }), - ("truc:asc:deesc", InvalidAscDescSyntax { name: S("truc:asc:deesc") }), - ("42desc", InvalidAscDescSyntax { name: S("42desc") }), - ("_geoPoint:asc", InvalidReservedAscDescSyntax { name: S("_geoPoint") }), - ("_geoDistance:asc", InvalidReservedAscDescSyntax { name: S("_geoDistance") }), - ( - "_geoPoint(42.12 , 59.598)", - InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598)") }, - ), - ( - "_geoPoint(42.12 , 59.598):deesc", - InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):deesc") }, - ), - ( - "_geoPoint(42.12 , 59.598):machin", - InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):machin") }, - ), - ( - "_geoPoint(42.12 , 59.598):asc:aasc", - InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):asc:aasc") }, - ), - ( - "_geoPoint(42,12 , 59,598):desc", - InvalidReservedAscDescSyntax { name: S("_geoPoint(42,12 , 59,598)") }, - ), - ( - "_geoPoint(35, 85, 75):asc", - InvalidReservedAscDescSyntax { name: S("_geoPoint(35, 85, 75)") }, - ), - ("_geoPoint(18):asc", InvalidReservedAscDescSyntax { name: S("_geoPoint(18)") }), - ]; - - for (req, expected_error) in invalid_req { - let res = req.parse::(); - assert!( - res.is_err(), - "Should no be able to parse `{}`, was expecting an error but instead got: `{:?}`", - req, - res, - ); - let res = res.unwrap_err(); - assert_eq!( - res.to_string(), - expected_error.to_string(), - "Bad error for input {}: got `{:?}` instead of `{:?}`", - req, - res, - expected_error - ); - } - } - #[test] fn parse_criterion() { let valid_criteria = [ diff --git a/milli/src/lib.rs b/milli/src/lib.rs index 550e7f13d..f36de8437 100644 --- a/milli/src/lib.rs +++ b/milli/src/lib.rs @@ -4,6 +4,7 @@ extern crate pest_derive; #[macro_use] pub mod documents; +mod asc_desc; mod criterion; mod error; mod external_documents_ids; @@ -24,7 +25,8 @@ use fxhash::{FxHasher32, FxHasher64}; pub use grenad::CompressionType; use serde_json::{Map, Value}; -pub use self::criterion::{default_criteria, AscDesc, Criterion, Member}; +pub use self::asc_desc::{AscDesc, Member}; +pub use self::criterion::{default_criteria, Criterion}; pub use self::error::{ Error, FieldIdMapMissingEntry, InternalError, SerializationError, UserError, }; diff --git a/milli/src/search/criteria/mod.rs b/milli/src/search/criteria/mod.rs index c2de55de5..a23e5acf9 100644 --- a/milli/src/search/criteria/mod.rs +++ b/milli/src/search/criteria/mod.rs @@ -12,10 +12,9 @@ use self::r#final::Final; use self::typo::Typo; use self::words::Words; use super::query_tree::{Operation, PrimitiveQueryPart, Query, QueryKind}; -use crate::criterion::{AscDesc as AscDescName, Member}; use crate::search::criteria::geo::Geo; use crate::search::{word_derivations, WordDerivationsCache}; -use crate::{DocumentId, FieldId, Index, Result, TreeLevel}; +use crate::{AscDesc as AscDescName, DocumentId, FieldId, Index, Member, Result, TreeLevel}; mod asc_desc; mod attribute; diff --git a/milli/src/search/mod.rs b/milli/src/search/mod.rs index f752f5822..3984ed130 100644 --- a/milli/src/search/mod.rs +++ b/milli/src/search/mod.rs @@ -18,10 +18,9 @@ pub(crate) use self::facet::ParserRule; pub use self::facet::{FacetDistribution, FacetNumberIter, FilterCondition, Operator}; pub use self::matching_words::MatchingWords; use self::query_tree::QueryTreeBuilder; -use crate::criterion::{AscDesc, Criterion}; use crate::error::UserError; use crate::search::criteria::r#final::{Final, FinalResult}; -use crate::{DocumentId, Index, Result}; +use crate::{AscDesc, Criterion, DocumentId, Index, Result}; // Building these factories is not free. static LEVDIST0: Lazy = Lazy::new(|| LevBuilder::new(0, true)); From 86e272856ad03071c1d67aea8d8aefa2dc532f27 Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 22 Sep 2021 15:33:32 +0200 Subject: [PATCH 3/7] create an asc_desc error type that is never supposed to be returned to the end user --- milli/src/criterion.rs | 17 +++++------------ milli/src/error.rs | 12 ------------ milli/src/lib.rs | 2 +- 3 files changed, 6 insertions(+), 25 deletions(-) diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index acc0148bb..3afc1b1f8 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -4,7 +4,7 @@ use std::str::FromStr; use serde::{Deserialize, Serialize}; use crate::error::{Error, UserError}; -use crate::{AscDesc, Member}; +use crate::{AscDesc, AscDescError, Member}; #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] pub enum Criterion { @@ -58,31 +58,24 @@ impl FromStr for Criterion { name: "_geoPoint".to_string(), })? } - Err(UserError::InvalidAscDescSyntax { name }) => { + Err(AscDescError::InvalidSyntax { name }) => { Err(UserError::InvalidRankingRuleName { name })? } - Err(UserError::InvalidReservedAscDescSyntax { name }) - if name.starts_with("_geoPoint") => - { + Err(AscDescError::ReservedKeyword { name }) if name.starts_with("_geoPoint") => { Err(UserError::InvalidReservedRankingRuleNameSort { name: "_geoPoint".to_string(), } .into()) } - Err(UserError::InvalidReservedAscDescSyntax { name }) - if name.starts_with("_geoRadius") => - { + Err(AscDescError::ReservedKeyword { name }) if name.starts_with("_geoRadius") => { Err(UserError::InvalidReservedRankingRuleNameFilter { name: "_geoRadius".to_string(), } .into()) } - Err(UserError::InvalidReservedAscDescSyntax { name }) => { + Err(AscDescError::ReservedKeyword { name }) => { Err(UserError::InvalidReservedRankingRuleName { name }.into()) } - Err(error) => { - Err(UserError::InvalidRankingRuleName { name: error.to_string() }.into()) - } }, } } diff --git a/milli/src/error.rs b/milli/src/error.rs index 2e2d3088e..1c0125c70 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -56,8 +56,6 @@ pub enum FieldIdMapMissingEntry { pub enum UserError { AttributeLimitReached, DocumentLimitReached, - InvalidAscDescSyntax { name: String }, - InvalidReservedAscDescSyntax { name: String }, InvalidDocumentId { document_id: Value }, InvalidFacetsDistribution { invalid_facets_name: HashSet }, InvalidFilter(pest::error::Error), @@ -226,22 +224,12 @@ impl fmt::Display for UserError { ) } Self::InvalidFilter(error) => error.fmt(f), - Self::InvalidAscDescSyntax { name } => { - write!(f, "invalid asc/desc syntax for {}", name) - } Self::InvalidGeoField { document_id, object } => write!( f, "the document with the id: {} contains an invalid _geo field: {}", document_id, object ), Self::InvalidRankingRuleName { name } => write!(f, "invalid ranking rule {}", name), - Self::InvalidReservedAscDescSyntax { name } => { - write!( - f, - "{} is a reserved keyword and thus can't be used as a asc/desc rule", - name - ) - } Self::InvalidReservedRankingRuleName { name } => { write!(f, "{} is a reserved keyword and thus can't be used as a ranking rule", name) } diff --git a/milli/src/lib.rs b/milli/src/lib.rs index f36de8437..d61e7d6e3 100644 --- a/milli/src/lib.rs +++ b/milli/src/lib.rs @@ -25,7 +25,7 @@ use fxhash::{FxHasher32, FxHasher64}; pub use grenad::CompressionType; use serde_json::{Map, Value}; -pub use self::asc_desc::{AscDesc, Member}; +pub use self::asc_desc::{AscDesc, AscDescError, Member}; pub use self::criterion::{default_criteria, Criterion}; pub use self::error::{ Error, FieldIdMapMissingEntry, InternalError, SerializationError, UserError, From 023446ecf3ceda4b80e4aa6fc49bca6bc97c28ed Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 22 Sep 2021 16:02:07 +0200 Subject: [PATCH 4/7] create a smaller and easier to maintain CriterionError type --- milli/src/criterion.rs | 100 ++++++++++++++++++++++++++--------------- milli/src/error.rs | 28 ++---------- milli/src/lib.rs | 2 +- 3 files changed, 69 insertions(+), 61 deletions(-) diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index 3afc1b1f8..8abfb5a30 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -3,8 +3,43 @@ use std::str::FromStr; use serde::{Deserialize, Serialize}; -use crate::error::{Error, UserError}; -use crate::{AscDesc, AscDescError, Member}; +use crate::error::Error; +use crate::{AscDesc, AscDescError, Member, UserError}; + +#[derive(Debug)] +pub enum CriterionError { + InvalidName { name: String }, + ReservedName { name: String }, + ReservedNameForSort { name: String }, + ReservedNameForFilter { name: String }, +} + +impl fmt::Display for CriterionError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::InvalidName { name } => write!(f, "invalid ranking rule {}", name), + Self::ReservedName { name } => { + write!(f, "{} is a reserved keyword and thus can't be used as a ranking rule", name) + } + Self::ReservedNameForSort { name } => { + write!( + f, + "{0} is a reserved keyword and thus can't be used as a ranking rule. \ +{0} can only be used for sorting at search time", + name + ) + } + Self::ReservedNameForFilter { name } => { + write!( + f, + "{0} is a reserved keyword and thus can't be used as a ranking rule. \ +{0} can only be used for filtering at search time", + name + ) + } + } + } +} #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] pub enum Criterion { @@ -40,7 +75,7 @@ impl Criterion { } impl FromStr for Criterion { - type Err = Error; + type Err = CriterionError; fn from_str(text: &str) -> Result { match text { @@ -54,33 +89,31 @@ impl FromStr for Criterion { Ok(AscDesc::Asc(Member::Field(field))) => Ok(Criterion::Asc(field)), Ok(AscDesc::Desc(Member::Field(field))) => Ok(Criterion::Desc(field)), Ok(AscDesc::Asc(Member::Geo(_))) | Ok(AscDesc::Desc(Member::Geo(_))) => { - Err(UserError::InvalidReservedRankingRuleNameSort { - name: "_geoPoint".to_string(), - })? + Err(CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() })? } Err(AscDescError::InvalidSyntax { name }) => { - Err(UserError::InvalidRankingRuleName { name })? + Err(CriterionError::InvalidName { name })? } Err(AscDescError::ReservedKeyword { name }) if name.starts_with("_geoPoint") => { - Err(UserError::InvalidReservedRankingRuleNameSort { - name: "_geoPoint".to_string(), - } - .into()) + Err(CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() })? } Err(AscDescError::ReservedKeyword { name }) if name.starts_with("_geoRadius") => { - Err(UserError::InvalidReservedRankingRuleNameFilter { - name: "_geoRadius".to_string(), - } - .into()) + Err(CriterionError::ReservedNameForFilter { name: "_geoRadius".to_string() })? } Err(AscDescError::ReservedKeyword { name }) => { - Err(UserError::InvalidReservedRankingRuleName { name }.into()) + Err(CriterionError::ReservedName { name })? } }, } } } +impl From for Error { + fn from(error: CriterionError) -> Self { + Self::UserError(UserError::CriterionError(error)) + } +} + pub fn default_criteria() -> Vec { vec![ Criterion::Words, @@ -112,7 +145,7 @@ impl fmt::Display for Criterion { #[cfg(test)] mod tests { use big_s::S; - use UserError::*; + use CriterionError::*; use super::*; @@ -146,24 +179,21 @@ mod tests { } let invalid_criteria = [ - ("words suffix", InvalidRankingRuleName { name: S("words suffix") }), - ("prefix typo", InvalidRankingRuleName { name: S("prefix typo") }), - ("proximity attribute", InvalidRankingRuleName { name: S("proximity attribute") }), - ("price", InvalidRankingRuleName { name: S("price") }), - ("asc:price", InvalidRankingRuleName { name: S("asc:price") }), - ("price:deesc", InvalidRankingRuleName { name: S("price:deesc") }), - ("price:aasc", InvalidRankingRuleName { name: S("price:aasc") }), - ("price:asc and desc", InvalidRankingRuleName { name: S("price:asc and desc") }), - ("price:asc:truc", InvalidRankingRuleName { name: S("price:asc:truc") }), - ("_geo:asc", InvalidReservedRankingRuleName { name: S("_geo") }), - ("_geoDistance:asc", InvalidReservedRankingRuleName { name: S("_geoDistance") }), - ("_geoPoint:asc", InvalidReservedRankingRuleNameSort { name: S("_geoPoint") }), - ("_geoPoint(42, 75):asc", InvalidReservedRankingRuleNameSort { name: S("_geoPoint") }), - ("_geoRadius:asc", InvalidReservedRankingRuleNameFilter { name: S("_geoRadius") }), - ( - "_geoRadius(42, 75, 59):asc", - InvalidReservedRankingRuleNameFilter { name: S("_geoRadius") }, - ), + ("words suffix", InvalidName { name: S("words suffix") }), + ("prefix typo", InvalidName { name: S("prefix typo") }), + ("proximity attribute", InvalidName { name: S("proximity attribute") }), + ("price", InvalidName { name: S("price") }), + ("asc:price", InvalidName { name: S("asc:price") }), + ("price:deesc", InvalidName { name: S("price:deesc") }), + ("price:aasc", InvalidName { name: S("price:aasc") }), + ("price:asc and desc", InvalidName { name: S("price:asc and desc") }), + ("price:asc:truc", InvalidName { name: S("price:asc:truc") }), + ("_geo:asc", ReservedName { name: S("_geo") }), + ("_geoDistance:asc", ReservedName { name: S("_geoDistance") }), + ("_geoPoint:asc", ReservedNameForSort { name: S("_geoPoint") }), + ("_geoPoint(42, 75):asc", ReservedNameForSort { name: S("_geoPoint") }), + ("_geoRadius:asc", ReservedNameForFilter { name: S("_geoRadius") }), + ("_geoRadius(42, 75, 59):asc", ReservedNameForFilter { name: S("_geoRadius") }), ]; for (input, expected) in invalid_criteria { diff --git a/milli/src/error.rs b/milli/src/error.rs index 1c0125c70..519de8516 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -8,7 +8,7 @@ use rayon::ThreadPoolBuildError; use serde_json::{Map, Value}; use crate::search::ParserRule; -use crate::{DocumentId, FieldId}; +use crate::{CriterionError, DocumentId, FieldId}; pub type Object = Map; @@ -55,6 +55,7 @@ pub enum FieldIdMapMissingEntry { #[derive(Debug)] pub enum UserError { AttributeLimitReached, + CriterionError(CriterionError), DocumentLimitReached, InvalidDocumentId { document_id: Value }, InvalidFacetsDistribution { invalid_facets_name: HashSet }, @@ -63,10 +64,6 @@ pub enum UserError { InvalidSortName { name: String }, InvalidReservedSortName { name: String }, InvalidGeoField { document_id: Value, object: Value }, - InvalidRankingRuleName { name: String }, - InvalidReservedRankingRuleName { name: String }, - InvalidReservedRankingRuleNameSort { name: String }, - InvalidReservedRankingRuleNameFilter { name: String }, InvalidSortableAttribute { field: String, valid_fields: HashSet }, SortRankingRuleMissing, InvalidStoreFile, @@ -213,6 +210,7 @@ impl fmt::Display for UserError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { Self::AttributeLimitReached => f.write_str("maximum number of attributes reached"), + Self::CriterionError(error) => f.write_str(&error.to_string()), Self::DocumentLimitReached => f.write_str("maximum number of documents reached"), Self::InvalidFacetsDistribution { invalid_facets_name } => { let name_list = @@ -229,26 +227,6 @@ impl fmt::Display for UserError { "the document with the id: {} contains an invalid _geo field: {}", document_id, object ), - Self::InvalidRankingRuleName { name } => write!(f, "invalid ranking rule {}", name), - Self::InvalidReservedRankingRuleName { name } => { - write!(f, "{} is a reserved keyword and thus can't be used as a ranking rule", name) - } - Self::InvalidReservedRankingRuleNameSort { name } => { - write!( - f, - "{0} is a reserved keyword and thus can't be used as a ranking rule. \ -{0} can only be used for sorting at search time", - name - ) - } - Self::InvalidReservedRankingRuleNameFilter { name } => { - write!( - f, - "{0} is a reserved keyword and thus can't be used as a ranking rule. \ -{0} can only be used for filtering at search time", - name - ) - } Self::InvalidReservedSortName { name } => { write!( f, diff --git a/milli/src/lib.rs b/milli/src/lib.rs index d61e7d6e3..8a54bbbdf 100644 --- a/milli/src/lib.rs +++ b/milli/src/lib.rs @@ -26,7 +26,7 @@ pub use grenad::CompressionType; use serde_json::{Map, Value}; pub use self::asc_desc::{AscDesc, AscDescError, Member}; -pub use self::criterion::{default_criteria, Criterion}; +pub use self::criterion::{default_criteria, Criterion, CriterionError}; pub use self::error::{ Error, FieldIdMapMissingEntry, InternalError, SerializationError, UserError, }; From 1e5e3d57e256410931df2ccfe54cd21b5eb0dc41 Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 22 Sep 2021 16:09:08 +0200 Subject: [PATCH 5/7] auto convert AscDescError into CriterionError --- milli/src/criterion.rs | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index 8abfb5a30..4299e4974 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -4,7 +4,7 @@ use std::str::FromStr; use serde::{Deserialize, Serialize}; use crate::error::Error; -use crate::{AscDesc, AscDescError, Member, UserError}; +use crate::{AscDesc, Member, UserError}; #[derive(Debug)] pub enum CriterionError { @@ -41,6 +41,12 @@ impl fmt::Display for CriterionError { } } +impl From for Error { + fn from(error: CriterionError) -> Self { + Self::UserError(UserError::CriterionError(error)) + } +} + #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] pub enum Criterion { /// Sorted by decreasing number of matched query terms. @@ -85,35 +91,17 @@ impl FromStr for Criterion { "attribute" => Ok(Criterion::Attribute), "sort" => Ok(Criterion::Sort), "exactness" => Ok(Criterion::Exactness), - text => match AscDesc::from_str(text) { - Ok(AscDesc::Asc(Member::Field(field))) => Ok(Criterion::Asc(field)), - Ok(AscDesc::Desc(Member::Field(field))) => Ok(Criterion::Desc(field)), - Ok(AscDesc::Asc(Member::Geo(_))) | Ok(AscDesc::Desc(Member::Geo(_))) => { + text => match AscDesc::from_str(text)? { + AscDesc::Asc(Member::Field(field)) => Ok(Criterion::Asc(field)), + AscDesc::Desc(Member::Field(field)) => Ok(Criterion::Desc(field)), + AscDesc::Asc(Member::Geo(_)) | AscDesc::Desc(Member::Geo(_)) => { Err(CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() })? } - Err(AscDescError::InvalidSyntax { name }) => { - Err(CriterionError::InvalidName { name })? - } - Err(AscDescError::ReservedKeyword { name }) if name.starts_with("_geoPoint") => { - Err(CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() })? - } - Err(AscDescError::ReservedKeyword { name }) if name.starts_with("_geoRadius") => { - Err(CriterionError::ReservedNameForFilter { name: "_geoRadius".to_string() })? - } - Err(AscDescError::ReservedKeyword { name }) => { - Err(CriterionError::ReservedName { name })? - } }, } } } -impl From for Error { - fn from(error: CriterionError) -> Self { - Self::UserError(UserError::CriterionError(error)) - } -} - pub fn default_criteria() -> Vec { vec![ Criterion::Words, From 47ee93b0bd7e81ee042aa5f891a01c00c88f9454 Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 22 Sep 2021 16:29:11 +0200 Subject: [PATCH 6/7] return an error when _geoPoint is used but _geo is not sortable --- milli/src/search/mod.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/milli/src/search/mod.rs b/milli/src/search/mod.rs index 3984ed130..bec059d46 100644 --- a/milli/src/search/mod.rs +++ b/milli/src/search/mod.rs @@ -20,7 +20,7 @@ pub use self::matching_words::MatchingWords; use self::query_tree::QueryTreeBuilder; use crate::error::UserError; use crate::search::criteria::r#final::{Final, FinalResult}; -use crate::{AscDesc, Criterion, DocumentId, Index, Result}; +use crate::{AscDesc, Criterion, DocumentId, Index, Member, Result}; // Building these factories is not free. static LEVDIST0: Lazy = Lazy::new(|| LevBuilder::new(0, true)); @@ -147,15 +147,20 @@ impl<'a> Search<'a> { if let Some(sort_criteria) = &self.sort_criteria { let sortable_fields = self.index.sortable_fields(self.rtxn)?; for asc_desc in sort_criteria { - // we are not supposed to find any geoPoint in the criterion - if let Some(field) = asc_desc.field() { - if !sortable_fields.contains(field) { + match asc_desc.member() { + Member::Field(ref field) if !sortable_fields.contains(field) => { return Err(UserError::InvalidSortableAttribute { field: field.to_string(), valid_fields: sortable_fields, - } - .into()); + })? } + Member::Geo(_) if !sortable_fields.contains("_geo") => { + return Err(UserError::InvalidSortableAttribute { + field: "_geo".to_string(), + valid_fields: sortable_fields, + })? + } + _ => (), } } } From 218f0a666163cbfb6980eb8fd23535d60b0dc2ad Mon Sep 17 00:00:00 2001 From: Irevoire Date: Wed, 22 Sep 2021 16:59:23 +0200 Subject: [PATCH 7/7] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Clément Renault --- milli/src/asc_desc.rs | 6 +++--- milli/src/criterion.rs | 12 ++++++------ milli/src/error.rs | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index d68d5b6a2..9a3bda934 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -1,4 +1,4 @@ -//! This module provide the `AscDesc` type and define a all the errors related to this type +//! This module provides the `AscDesc` type and defines all the errors related to this type. use std::fmt; use std::str::FromStr; @@ -43,7 +43,7 @@ impl From for CriterionError { AscDescError::ReservedKeyword { name } if name.starts_with("_geoRadius") => { CriterionError::ReservedNameForFilter { name: "_geoRadius".to_string() } } - AscDescError::ReservedKeyword { name } => (CriterionError::ReservedName { name }), + AscDescError::ReservedKeyword { name } => CriterionError::ReservedName { name }, } } } @@ -129,7 +129,7 @@ impl FromStr for AscDesc { type Err = AscDescError; /// Since we don't know if this was deserialized for a criterion or a sort we just return a - /// string and let the caller create his own error + /// string and let the caller create his own error. fn from_str(text: &str) -> Result { match text.rsplit_once(':') { Some((left, "asc")) => Ok(AscDesc::Asc(left.parse()?)), diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index 4299e4974..aff7fcf68 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -24,17 +24,17 @@ impl fmt::Display for CriterionError { Self::ReservedNameForSort { name } => { write!( f, - "{0} is a reserved keyword and thus can't be used as a ranking rule. \ -{0} can only be used for sorting at search time", - name + "{} is a reserved keyword and thus can't be used as a ranking rule. \ +{} can only be used for sorting at search time", + name, name ) } Self::ReservedNameForFilter { name } => { write!( f, - "{0} is a reserved keyword and thus can't be used as a ranking rule. \ -{0} can only be used for filtering at search time", - name + "{} is a reserved keyword and thus can't be used as a ranking rule. \ +{} can only be used for filtering at search time", + name, name ) } } diff --git a/milli/src/error.rs b/milli/src/error.rs index 519de8516..bd4f02b99 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -210,7 +210,7 @@ impl fmt::Display for UserError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { Self::AttributeLimitReached => f.write_str("maximum number of attributes reached"), - Self::CriterionError(error) => f.write_str(&error.to_string()), + Self::CriterionError(error) => write!(f, "{}", error), Self::DocumentLimitReached => f.write_str("maximum number of documents reached"), Self::InvalidFacetsDistribution { invalid_facets_name } => { let name_list =