From 023446ecf3ceda4b80e4aa6fc49bca6bc97c28ed Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 22 Sep 2021 16:02:07 +0200 Subject: [PATCH] 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, };