From 3599df77f05f4e89a28c0160411e95a840e0b227 Mon Sep 17 00:00:00 2001 From: many Date: Tue, 26 Oct 2021 17:49:35 +0200 Subject: [PATCH] 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...