From 3599df77f05f4e89a28c0160411e95a840e0b227 Mon Sep 17 00:00:00 2001 From: many Date: Tue, 26 Oct 2021 17:49:35 +0200 Subject: [PATCH 1/4] Change some error messages --- milli/src/asc_desc.rs | 12 +- milli/src/criterion.rs | 14 ++- milli/src/error.rs | 112 ++++++++++++------ milli/src/search/facet/filter_condition.rs | 104 ++++++---------- milli/src/update/index_documents/transform.rs | 6 +- milli/src/update/settings.rs | 8 +- 6 files changed, 138 insertions(+), 118 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index 00f65a459..c0a277c0c 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -28,12 +28,12 @@ impl fmt::Display for AscDescError { write!(f, "Longitude must be contained between -180 and 180 degrees.",) } Self::InvalidSyntax { name } => { - write!(f, "invalid asc/desc syntax for {}.", 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.", + "`{}` is a reserved keyword and thus can't be used as a asc/desc rule.", name ) } @@ -192,18 +192,18 @@ impl fmt::Display for SortError { Self::BadGeoPointUsage { name } => { write!( f, - "invalid syntax for the `_geoPoint` parameter: `{}`. \ + "Invalid syntax for the `_geoPoint` parameter: `{}`. \ Usage: `_geoPoint(latitude, longitude):asc`.", name ) } Self::InvalidName { name } => { - write!(f, "invalid syntax for the sort parameter `{}`.", name) + write!(f, "Invalid syntax for the sort parameter: expected expression ending by `:asc` or `:desc`, found `{}`.", name) } Self::ReservedName { name } => { write!( f, - "{} is a reserved keyword and thus can't be used as a sort expression.", + "`{}` is a reserved keyword and thus can't be used as a sort expression.", name ) } @@ -211,7 +211,7 @@ impl fmt::Display for SortError { write!( f, "`{}` is a reserved keyword and thus can't be used as a sort expression. \ - Use the `_geoPoint(latitude, longitude)` built-in rule to sort on `_geo` field coordinates.", + Use the _geoPoint(latitude, longitude) built-in rule to sort on _geo field coordinates.", name, ) } diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index aff7fcf68..0586fcc0f 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -19,21 +19,25 @@ impl fmt::Display for CriterionError { 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) + write!( + f, + "`{}` is a reserved keyword and thus can't be used as a ranking rule", + name + ) } Self::ReservedNameForSort { name } => { write!( f, - "{} is a reserved keyword and thus can't be used as a ranking rule. \ -{} can only be used for sorting at search time", + "`{}` 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, - "{} is a reserved keyword and thus can't be used as a ranking rule. \ -{} can only be used for filtering at search time", + "`{}` 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 1f1cc5264..a4125d117 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -59,23 +59,28 @@ pub enum UserError { DocumentLimitReached, InvalidDocumentId { document_id: Value }, InvalidFacetsDistribution { invalid_facets_name: HashSet }, - InvalidFilter(pest::error::Error), - InvalidFilterAttribute(pest::error::Error), + InvalidFilter(FilterError), InvalidGeoField { document_id: Value, object: Value }, InvalidSortableAttribute { field: String, valid_fields: HashSet }, SortRankingRuleMissing, InvalidStoreFile, MaxDatabaseSizeReached, - MissingDocumentId { document: Object }, + MissingDocumentId { primary_key: String, document: Object }, MissingPrimaryKey, NoSpaceLeftOnDevice, - PrimaryKeyCannotBeChanged, - PrimaryKeyCannotBeReset, + PrimaryKeyCannotBeChanged(String), SerdeJson(serde_json::Error), SortError(SortError), UnknownInternalDocumentId { document_id: DocumentId }, } +#[derive(Debug)] +pub enum FilterError { + InvalidAttribute { field: String, valid_fields: HashSet }, + ReservedKeyword { field: String, context: Option }, + Syntax(pest::error::Error), +} + impl From for Error { fn from(error: io::Error) -> Error { // TODO must be improved and more precise @@ -160,6 +165,12 @@ impl From for Error { } } +impl From for Error { + fn from(error: FilterError) -> Error { + Error::UserError(UserError::InvalidFilter(error)) + } +} + impl From for Error { fn from(error: SerializationError) -> Error { Error::InternalError(InternalError::Serialization(error)) @@ -169,7 +180,7 @@ impl From for Error { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::InternalError(error) => write!(f, "internal: {}", error), + Self::InternalError(error) => write!(f, "internal: {}.", error), Self::IoError(error) => error.fmt(f), Self::UserError(error) => error.fmt(f), } @@ -182,15 +193,15 @@ impl fmt::Display for InternalError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { Self::DatabaseMissingEntry { db_name, key } => { - write!(f, "missing {} in the {} database", key.unwrap_or("key"), db_name) + write!(f, "Missing {} in the {} database.", key.unwrap_or("key"), db_name) } Self::FieldIdMapMissingEntry(error) => error.fmt(f), Self::Fst(error) => error.fmt(f), Self::GrenadInvalidCompressionType => { - f.write_str("invalid compression type have been specified to grenad") + f.write_str("Invalid compression type have been specified to grenad.") } Self::IndexingMergingKeys { process } => { - write!(f, "invalid merge while processing {}", process) + write!(f, "Invalid merge while processing {}.", process) } Self::Serialization(error) => error.fmt(f), Self::InvalidDatabaseTyping => HeedError::InvalidDatabaseTyping.fmt(f), @@ -208,67 +219,100 @@ impl StdError for InternalError {} 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::AttributeLimitReached => f.write_str("Maximum number of attributes reached."), Self::CriterionError(error) => write!(f, "{}", error), - Self::DocumentLimitReached => f.write_str("maximum number of documents reached"), + Self::DocumentLimitReached => f.write_str("Maximum number of documents reached."), Self::InvalidFacetsDistribution { invalid_facets_name } => { let name_list = invalid_facets_name.iter().map(AsRef::as_ref).collect::>().join(", "); write!( f, - "invalid facet distribution, the fields {} are not set as filterable", + "Invalid facet distribution, the fields `{}` are not set as filterable.", name_list ) } Self::InvalidFilter(error) => error.fmt(f), Self::InvalidGeoField { document_id, object } => write!( f, - "the document with the id: {} contains an invalid _geo field: {}", + "The document with the id: `{}` contains an invalid _geo field: `{}`.", document_id, object ), Self::InvalidDocumentId { document_id } => { - let json = serde_json::to_string(document_id).unwrap(); + let document_id = match document_id { + Value::String(id) => id.clone(), + _ => document_id.to_string(), + }; write!( f, - "document identifier is invalid {}, \ -a document id can be of type integer or string \ -only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and underscores (_)", - json + "Document identifier `{}` is invalid. \ +A document identifier can be of type integer or string, \ +only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and underscores (_).", + document_id ) } - Self::InvalidFilterAttribute(error) => error.fmt(f), Self::InvalidSortableAttribute { field, valid_fields } => { let valid_names = valid_fields.iter().map(AsRef::as_ref).collect::>().join(", "); write!( f, - "Attribute {} is not sortable, available sortable attributes are: {}", + "Attribute `{}` is not sortable. Available sortable attributes are: `{}`.", field, valid_names ) } Self::SortRankingRuleMissing => f.write_str( - "You must specify where \"sort\" is listed in the \ -rankingRules setting to use the sort parameter at search time", + "The sort ranking rule must be specified in the \ +ranking rules settings to use the sort parameter at search time.", ), - Self::MissingDocumentId { document } => { + Self::MissingDocumentId { primary_key, document } => { let json = serde_json::to_string(document).unwrap(); - write!(f, "document doesn't have an identifier {}", json) + write!(f, "Document doesn't have a `{}` attribute: `{}`.", primary_key, json) } - Self::MissingPrimaryKey => f.write_str("missing primary key"), - Self::MaxDatabaseSizeReached => f.write_str("maximum database size reached"), + Self::MissingPrimaryKey => f.write_str("Missing primary key."), + Self::MaxDatabaseSizeReached => f.write_str("Maximum database size reached."), // TODO where can we find it instead of writing the text ourselves? - Self::NoSpaceLeftOnDevice => f.write_str("no space left on device"), - Self::InvalidStoreFile => f.write_str("store file is not a valid database file"), - Self::PrimaryKeyCannotBeChanged => { - f.write_str("primary key cannot be changed if the database contains documents") - } - Self::PrimaryKeyCannotBeReset => { - f.write_str("primary key cannot be reset if the database contains documents") + Self::NoSpaceLeftOnDevice => f.write_str("No space left on device."), + Self::InvalidStoreFile => f.write_str("Store file is not a valid database file."), + Self::PrimaryKeyCannotBeChanged(primary_key) => { + write!(f, "Index already has a primary key: `{}`.", primary_key) } Self::SerdeJson(error) => error.fmt(f), Self::SortError(error) => write!(f, "{}", error), Self::UnknownInternalDocumentId { document_id } => { - write!(f, "an unknown internal document id have been used ({})", document_id) + write!(f, "An unknown internal document id have been used: `{}`.", document_id) + } + } + } +} + +impl fmt::Display for FilterError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::InvalidAttribute { field, valid_fields } => write!( + f, + "Attribute `{}` is not filterable. Available filterable attributes are: `{}`.", + field, + valid_fields + .clone() + .into_iter() + .reduce(|left, right| left + "`, `" + &right) + .unwrap_or_default() + ), + Self::ReservedKeyword { field, context: Some(context) } => { + write!( + f, + "`{}` is a reserved keyword and thus can't be used as a filter expression. {}", + field, context + ) + } + Self::ReservedKeyword { field, context: None } => { + write!( + f, + "`{}` is a reserved keyword and thus can't be used as a filter expression.", + field + ) + } + Self::Syntax(syntax_helper) => { + write!(f, "Invalid syntax for the filter parameter: `{}`.", syntax_helper) } } } diff --git a/milli/src/search/facet/filter_condition.rs b/milli/src/search/facet/filter_condition.rs index f1055b2f8..3378054d4 100644 --- a/milli/src/search/facet/filter_condition.rs +++ b/milli/src/search/facet/filter_condition.rs @@ -6,7 +6,6 @@ use std::str::FromStr; use either::Either; use heed::types::DecodeIgnore; -use itertools::Itertools; use log::debug; use pest::error::{Error as PestError, ErrorVariant}; use pest::iterators::{Pair, Pairs}; @@ -17,7 +16,7 @@ use self::FilterCondition::*; use self::Operator::*; use super::parser::{FilterParser, Rule, PREC_CLIMBER}; use super::FacetNumberRange; -use crate::error::UserError; +use crate::error::FilterError; use crate::heed_codec::facet::{ FacetLevelValueF64Codec, FacetStringLevelZeroCodec, FacetStringLevelZeroValueCodec, }; @@ -117,8 +116,7 @@ impl FilterCondition { ) -> Result { let fields_ids_map = index.fields_ids_map(rtxn)?; let filterable_fields = index.filterable_fields(rtxn)?; - let lexed = - FilterParser::parse(Rule::prgm, expression).map_err(UserError::InvalidFilter)?; + let lexed = FilterParser::parse(Rule::prgm, expression).map_err(FilterError::Syntax)?; FilterCondition::from_pairs(&fields_ids_map, &filterable_fields, lexed) } @@ -169,15 +167,11 @@ impl FilterCondition { item: Pair, ) -> Result { if !filterable_fields.contains("_geo") { - return Err(UserError::InvalidFilterAttribute(PestError::new_from_span( - ErrorVariant::CustomError { - message: format!( - "attribute `_geo` is not filterable, available filterable attributes are: {}", - filterable_fields.iter().join(", "), - ), - }, - item.as_span(), - )))?; + return Err(FilterError::InvalidAttribute { + field: "_geo".to_string(), + valid_fields: filterable_fields.clone(), + } + .into()); } let mut items = item.into_inner(); let fid = match fields_ids_map.id("_geo") { @@ -194,27 +188,27 @@ impl FilterCondition { .map(|param| (param.clone(), param.as_span())) .map(|(param, span)| pest_parse(param).0.map(|arg| (arg, span))) .collect::, _>>() - .map_err(UserError::InvalidFilter)?; + .map_err(FilterError::Syntax)?; if parameters.len() != 3 { - return Err(UserError::InvalidFilter(PestError::new_from_span( + return Err(FilterError::Syntax(PestError::new_from_span( ErrorVariant::CustomError { message: format!("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`"), }, // we want to point to the last parameters and if there was no parameters we // point to the parenthesis parameters.last().map(|param| param.1.clone()).unwrap_or(param_span), - )))?; + )).into()); } let (lat, lng, distance) = (¶meters[0], ¶meters[1], parameters[2].0); if !(-90.0..=90.0).contains(&lat.0) { - return Err(UserError::InvalidFilter(PestError::new_from_span( + return Err(FilterError::Syntax(PestError::new_from_span( ErrorVariant::CustomError { message: format!("Latitude must be contained between -90 and 90 degrees."), }, lat.1.clone(), )))?; } else if !(-180.0..=180.0).contains(&lng.0) { - return Err(UserError::InvalidFilter(PestError::new_from_span( + return Err(FilterError::Syntax(PestError::new_from_span( ErrorVariant::CustomError { message: format!("Longitude must be contained between -180 and 180 degrees."), }, @@ -230,9 +224,7 @@ impl FilterCondition { item: Pair, ) -> Result { let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { + let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? { Some(fid) => fid, None => return Ok(Empty), }; @@ -240,8 +232,8 @@ impl FilterCondition { let (lresult, _) = pest_parse(items.next().unwrap()); let (rresult, _) = pest_parse(items.next().unwrap()); - let lvalue = lresult.map_err(UserError::InvalidFilter)?; - let rvalue = rresult.map_err(UserError::InvalidFilter)?; + let lvalue = lresult.map_err(FilterError::Syntax)?; + let rvalue = rresult.map_err(FilterError::Syntax)?; Ok(Operator(fid, Between(lvalue, rvalue))) } @@ -252,9 +244,7 @@ impl FilterCondition { item: Pair, ) -> Result { let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { + let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? { Some(fid) => fid, None => return Ok(Empty), }; @@ -272,16 +262,14 @@ impl FilterCondition { item: Pair, ) -> Result { let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { + let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? { Some(fid) => fid, None => return Ok(Empty), }; let value = items.next().unwrap(); let (result, _svalue) = pest_parse(value); - let value = result.map_err(UserError::InvalidFilter)?; + let value = result.map_err(FilterError::Syntax)?; Ok(Operator(fid, GreaterThan(value))) } @@ -292,16 +280,14 @@ impl FilterCondition { item: Pair, ) -> Result { let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { + let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? { Some(fid) => fid, None => return Ok(Empty), }; let value = items.next().unwrap(); let (result, _svalue) = pest_parse(value); - let value = result.map_err(UserError::InvalidFilter)?; + let value = result.map_err(FilterError::Syntax)?; Ok(Operator(fid, GreaterThanOrEqual(value))) } @@ -312,16 +298,14 @@ impl FilterCondition { item: Pair, ) -> Result { let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { + let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? { Some(fid) => fid, None => return Ok(Empty), }; let value = items.next().unwrap(); let (result, _svalue) = pest_parse(value); - let value = result.map_err(UserError::InvalidFilter)?; + let value = result.map_err(FilterError::Syntax)?; Ok(Operator(fid, LowerThan(value))) } @@ -332,16 +316,14 @@ impl FilterCondition { item: Pair, ) -> Result { let mut items = item.into_inner(); - let fid = match field_id(fields_ids_map, filterable_fields, &mut items) - .map_err(UserError::InvalidFilterAttribute)? - { + let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? { Some(fid) => fid, None => return Ok(Empty), }; let value = items.next().unwrap(); let (result, _svalue) = pest_parse(value); - let value = result.map_err(UserError::InvalidFilter)?; + let value = result.map_err(FilterError::Syntax)?; Ok(Operator(fid, LowerThanOrEqual(value))) } @@ -598,43 +580,27 @@ fn field_id( fields_ids_map: &FieldsIdsMap, filterable_fields: &HashSet, items: &mut Pairs, -) -> StdResult, PestError> { +) -> StdResult, FilterError> { // lexing ensures that we at least have a key let key = items.next().unwrap(); if key.as_rule() == Rule::reserved { - let message = match key.as_str() { + return match key.as_str() { key if key.starts_with("_geoPoint") => { - format!( - "`_geoPoint` is a reserved keyword and thus can't be used as a filter expression. \ - Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates.", - ) + Err(FilterError::ReservedKeyword { field: "_geoPoint".to_string(), context: Some("Use the _geoRadius(latitude, longitude, distance) built-in rule to filter on _geo field coordinates.".to_string()) }) } - key @ "_geo" => { - format!( - "`{}` is a reserved keyword and thus can't be used as a filter expression. \ - Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates.", - key - ) + "_geo" => { + Err(FilterError::ReservedKeyword { field: "_geo".to_string(), context: Some("Use the _geoRadius(latitude, longitude, distance) built-in rule to filter on _geo field coordinates.".to_string()) }) } - key => format!( - "`{}` is a reserved keyword and thus can't be used as a filter expression.", - key - ), + key => + Err(FilterError::ReservedKeyword { field: key.to_string(), context: None }), }; - return Err(PestError::new_from_span(ErrorVariant::CustomError { message }, key.as_span())); } if !filterable_fields.contains(key.as_str()) { - return Err(PestError::new_from_span( - ErrorVariant::CustomError { - message: format!( - "attribute `{}` is not filterable, available filterable attributes are: {}.", - key.as_str(), - filterable_fields.iter().join(", "), - ), - }, - key.as_span(), - )); + return Err(FilterError::InvalidAttribute { + field: key.as_str().to_string(), + valid_fields: filterable_fields.clone(), + }); } Ok(fields_ids_map.id(key.as_str())) diff --git a/milli/src/update/index_documents/transform.rs b/milli/src/update/index_documents/transform.rs index 08aa72d35..855fb8db9 100644 --- a/milli/src/update/index_documents/transform.rs +++ b/milli/src/update/index_documents/transform.rs @@ -187,7 +187,11 @@ impl Transform<'_, '_> { } } - return Err(UserError::MissingDocumentId { document: json }.into()); + return Err(UserError::MissingDocumentId { + primary_key: primary_key_name, + document: json, + } + .into()); } let uuid = diff --git a/milli/src/update/settings.rs b/milli/src/update/settings.rs index dee63c726..94875a079 100644 --- a/milli/src/update/settings.rs +++ b/milli/src/update/settings.rs @@ -465,7 +465,8 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { self.index.put_primary_key(self.wtxn, primary_key)?; Ok(()) } else { - Err(UserError::PrimaryKeyCannotBeChanged.into()) + let primary_key = self.index.primary_key(self.wtxn)?.unwrap(); + Err(UserError::PrimaryKeyCannotBeChanged(primary_key.to_string()).into()) } } Setting::Reset => { @@ -473,7 +474,8 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { self.index.delete_primary_key(self.wtxn)?; Ok(()) } else { - Err(UserError::PrimaryKeyCannotBeReset.into()) + let primary_key = self.index.primary_key(self.wtxn)?.unwrap(); + Err(UserError::PrimaryKeyCannotBeChanged(primary_key.to_string()).into()) } } Setting::NotSet => Ok(()), @@ -1105,7 +1107,7 @@ mod tests { builder.reset_primary_key(); let err = builder.execute(|_, _| ()).unwrap_err(); - assert!(matches!(err, Error::UserError(UserError::PrimaryKeyCannotBeReset))); + assert!(matches!(err, Error::UserError(UserError::PrimaryKeyCannotBeChanged(_)))); wtxn.abort().unwrap(); // But if we clear the database... From 2be755ce75d8b1e64c11bf1801a1f3474beb3362 Mon Sep 17 00:00:00 2001 From: many Date: Wed, 27 Oct 2021 19:50:41 +0200 Subject: [PATCH 2/4] Lower error check, already check in meilisearch --- milli/src/asc_desc.rs | 46 ---------------------- milli/src/search/facet/filter_condition.rs | 43 ++------------------ 2 files changed, 4 insertions(+), 85 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index c0a277c0c..8d4973c2f 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -318,50 +318,4 @@ mod tests { ); } } - - #[test] - fn sort_error_message() { - let errors = [ - ( - AscDescError::InvalidSyntax { name: S("truc:machin") }, - S("invalid syntax for the sort parameter `truc:machin`."), - ), - ( - AscDescError::InvalidSyntax { name: S("hello:world") }, - S("invalid syntax for the sort parameter `hello:world`."), - ), - ( - AscDescError::ReservedKeyword { name: S("_geo") }, - S("`_geo` is a reserved keyword and thus can't be used as a sort expression. Use the `_geoPoint(latitude, longitude)` built-in rule to sort on `_geo` field coordinates."), - ), - ( - AscDescError::ReservedKeyword { name: S("_geoDistance") }, - S("_geoDistance is a reserved keyword and thus can't be used as a sort expression.") - ), - ( - AscDescError::ReservedKeyword { name: S("_geoRadius(12, 13)") }, - S("`_geoRadius` is a reserved keyword and thus can't be used as a sort expression. Use the `_geoPoint(latitude, longitude)` built-in rule to sort on `_geo` field coordinates."), - ), - ( - AscDescError::InvalidLatitude, - S("Latitude must be contained between -90 and 90 degrees."), - ), - ( - AscDescError::InvalidLongitude, - S("Longitude must be contained between -180 and 180 degrees."), - ), - ]; - - for (asc_desc_error, expected_message) in errors { - let sort_error = SortError::from(asc_desc_error); - assert_eq!( - sort_error.to_string(), - expected_message, - "was expecting {} for the error {:?} but instead got {}", - expected_message, - sort_error, - sort_error.to_string() - ); - } - } } diff --git a/milli/src/search/facet/filter_condition.rs b/milli/src/search/facet/filter_condition.rs index 3378054d4..f8d40aefb 100644 --- a/milli/src/search/facet/filter_condition.rs +++ b/milli/src/search/facet/filter_condition.rs @@ -755,39 +755,13 @@ mod tests { let index = Index::new(options, &path).unwrap(); let rtxn = index.read_txn().unwrap(); - let error = FilterCondition::from_str(&rtxn, &index, "_geo = 12").unwrap_err(); - assert!(error - .to_string() - .contains("`_geo` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates."), - "{}", - error.to_string() - ); + assert!(FilterCondition::from_str(&rtxn, &index, "_geo = 12").is_err()); - let error = - FilterCondition::from_str(&rtxn, &index, r#"_geoDistance <= 1000"#).unwrap_err(); - assert!(error - .to_string() - .contains("`_geoDistance` is a reserved keyword and thus can't be used as a filter expression."), - "{}", - error.to_string() - ); + assert!(FilterCondition::from_str(&rtxn, &index, r#"_geoDistance <= 1000"#).is_err()); - let error = FilterCondition::from_str(&rtxn, &index, r#"_geoPoint > 5"#).unwrap_err(); - assert!(error - .to_string() - .contains("`_geoPoint` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates."), - "{}", - error.to_string() - ); + assert!(FilterCondition::from_str(&rtxn, &index, r#"_geoPoint > 5"#).is_err()); - let error = - FilterCondition::from_str(&rtxn, &index, r#"_geoPoint(12, 16) > 5"#).unwrap_err(); - assert!(error - .to_string() - .contains("`_geoPoint` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates."), - "{}", - error.to_string() - ); + assert!(FilterCondition::from_str(&rtxn, &index, r#"_geoPoint(12, 16) > 5"#).is_err()); } #[test] @@ -804,15 +778,6 @@ mod tests { builder.execute(|_, _| ()).unwrap(); wtxn.commit().unwrap(); - let rtxn = index.read_txn().unwrap(); - // _geo is not filterable - let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(12, 12, 10)"); - assert!(result.is_err()); - let error = result.unwrap_err(); - assert!(error - .to_string() - .contains("attribute `_geo` is not filterable, available filterable attributes are:"),); - let mut wtxn = index.write_txn().unwrap(); let mut builder = Settings::new(&mut wtxn, &index, 0); builder.set_filterable_fields(hashset! { S("_geo"), S("price") }); From ed6db196810f78632758fc386f8a7f5f6cd6f357 Mon Sep 17 00:00:00 2001 From: many Date: Thu, 28 Oct 2021 11:18:32 +0200 Subject: [PATCH 3/4] Fix PR comments --- milli/src/error.rs | 8 ++++---- milli/src/search/facet/filter_condition.rs | 22 +++++++++++++++------- milli/src/search/mod.rs | 4 ++-- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/milli/src/error.rs b/milli/src/error.rs index a4125d117..be8458ce6 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::BTreeSet; use std::convert::Infallible; use std::error::Error as StdError; use std::{fmt, io, str}; @@ -58,10 +58,10 @@ pub enum UserError { CriterionError(CriterionError), DocumentLimitReached, InvalidDocumentId { document_id: Value }, - InvalidFacetsDistribution { invalid_facets_name: HashSet }, + InvalidFacetsDistribution { invalid_facets_name: BTreeSet }, InvalidFilter(FilterError), InvalidGeoField { document_id: Value, object: Value }, - InvalidSortableAttribute { field: String, valid_fields: HashSet }, + InvalidSortableAttribute { field: String, valid_fields: BTreeSet }, SortRankingRuleMissing, InvalidStoreFile, MaxDatabaseSizeReached, @@ -76,7 +76,7 @@ pub enum UserError { #[derive(Debug)] pub enum FilterError { - InvalidAttribute { field: String, valid_fields: HashSet }, + InvalidAttribute { field: String, valid_fields: BTreeSet }, ReservedKeyword { field: String, context: Option }, Syntax(pest::error::Error), } diff --git a/milli/src/search/facet/filter_condition.rs b/milli/src/search/facet/filter_condition.rs index f8d40aefb..cd7bcdc4f 100644 --- a/milli/src/search/facet/filter_condition.rs +++ b/milli/src/search/facet/filter_condition.rs @@ -169,7 +169,7 @@ impl FilterCondition { if !filterable_fields.contains("_geo") { return Err(FilterError::InvalidAttribute { field: "_geo".to_string(), - valid_fields: filterable_fields.clone(), + valid_fields: filterable_fields.into_iter().cloned().collect(), } .into()); } @@ -192,7 +192,7 @@ impl FilterCondition { if parameters.len() != 3 { return Err(FilterError::Syntax(PestError::new_from_span( ErrorVariant::CustomError { - message: format!("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`"), + message: format!("The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)"), }, // we want to point to the last parameters and if there was no parameters we // point to the parenthesis @@ -599,7 +599,7 @@ fn field_id( if !filterable_fields.contains(key.as_str()) { return Err(FilterError::InvalidAttribute { field: key.as_str().to_string(), - valid_fields: filterable_fields.clone(), + valid_fields: filterable_fields.into_iter().cloned().collect(), }); } @@ -829,26 +829,34 @@ mod tests { let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius"); assert!(result.is_err()); let error = result.unwrap_err(); - assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); + assert!(error.to_string().contains( + "The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)" + )); // georadius don't have any parameters let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius()"); assert!(result.is_err()); let error = result.unwrap_err(); - assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); + assert!(error.to_string().contains( + "The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)" + )); // georadius don't have enough parameters let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(1, 2)"); assert!(result.is_err()); let error = result.unwrap_err(); - assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); + assert!(error.to_string().contains( + "The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)" + )); // georadius have too many parameters let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(1, 2, 3, 4, 5, 6, 7, 8, 9, 10)"); assert!(result.is_err()); let error = result.unwrap_err(); - assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); + assert!(error.to_string().contains( + "The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)" + )); // georadius have a bad latitude let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-100, 150, 10)"); diff --git a/milli/src/search/mod.rs b/milli/src/search/mod.rs index bec059d46..aa2544091 100644 --- a/milli/src/search/mod.rs +++ b/milli/src/search/mod.rs @@ -151,13 +151,13 @@ impl<'a> Search<'a> { Member::Field(ref field) if !sortable_fields.contains(field) => { return Err(UserError::InvalidSortableAttribute { field: field.to_string(), - valid_fields: sortable_fields, + valid_fields: sortable_fields.into_iter().collect(), })? } Member::Geo(_) if !sortable_fields.contains("_geo") => { return Err(UserError::InvalidSortableAttribute { field: "_geo".to_string(), - valid_fields: sortable_fields, + valid_fields: sortable_fields.into_iter().collect(), })? } _ => (), From 9f1e0d2a49447f106277b8a07e0bba65370b47c8 Mon Sep 17 00:00:00 2001 From: many Date: Thu, 28 Oct 2021 14:47:17 +0200 Subject: [PATCH 4/4] Refine asc/desc error messages --- milli/src/asc_desc.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index 8d4973c2f..f07e1ded8 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -28,7 +28,7 @@ impl fmt::Display for AscDescError { write!(f, "Longitude must be contained between -180 and 180 degrees.",) } Self::InvalidSyntax { name } => { - write!(f, "invalid asc/desc syntax for `{}`.", name) + write!(f, "Invalid syntax for the asc/desc parameter: expected expression ending by `:asc` or `:desc`, found `{}`.", name) } Self::ReservedKeyword { name } => { write!( @@ -192,8 +192,8 @@ impl fmt::Display for SortError { Self::BadGeoPointUsage { name } => { write!( f, - "Invalid syntax for the `_geoPoint` parameter: `{}`. \ - Usage: `_geoPoint(latitude, longitude):asc`.", + "Invalid syntax for the geo parameter: expected expression formated like \ + `_geoPoint(latitude, longitude)` and ending by `:asc` or `:desc`, found `{}`.", name ) }