Change some error messages

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,
                 )
             }

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. \
+                    "`{}` is a reserved keyword and thus can't be used as a ranking rule. \
-{} can only be used for sorting at search time",
+`{}` 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. \
+                    "`{}` is a reserved keyword and thus can't be used as a ranking rule. \
-{} can only be used for filtering at search time",
+`{}` can only be used for filtering at search time",
                     name, name
                 )
             }

milli/src/error.rs

@@ -59,23 +59,28 @@ pub enum UserError {
     DocumentLimitReached,
     InvalidDocumentId { document_id: Value },
     InvalidFacetsDistribution { invalid_facets_name: HashSet<String> },
-    InvalidFilter(pest::error::Error<ParserRule>),
+    InvalidFilter(FilterError),
-    InvalidFilterAttribute(pest::error::Error<ParserRule>),
     InvalidGeoField { document_id: Value, object: Value },
     InvalidSortableAttribute { field: String, valid_fields: HashSet<String> },
     SortRankingRuleMissing,
     InvalidStoreFile,
     MaxDatabaseSizeReached,
-    MissingDocumentId { document: Object },
+    MissingDocumentId { primary_key: String, document: Object },
     MissingPrimaryKey,
     NoSpaceLeftOnDevice,
-    PrimaryKeyCannotBeChanged,
+    PrimaryKeyCannotBeChanged(String),
-    PrimaryKeyCannotBeReset,
     SerdeJson(serde_json::Error),
     SortError(SortError),
     UnknownInternalDocumentId { document_id: DocumentId },
 }
 
+#[derive(Debug)]
+pub enum FilterError {
+    InvalidAttribute { field: String, valid_fields: HashSet<String> },
+    ReservedKeyword { field: String, context: Option<String> },
+    Syntax(pest::error::Error<ParserRule>),
+}
+
 impl From<io::Error> for Error {
     fn from(error: io::Error) -> Error {
         // TODO must be improved and more precise
@@ -160,6 +165,12 @@ impl From<UserError> for Error {
     }
 }
 
+impl From<FilterError> for Error {
+    fn from(error: FilterError) -> Error {
+        Error::UserError(UserError::InvalidFilter(error))
+    }
+}
+
 impl From<SerializationError> for Error {
     fn from(error: SerializationError) -> Error {
         Error::InternalError(InternalError::Serialization(error))
@@ -169,7 +180,7 @@ impl From<SerializationError> 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::<Vec<_>>().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 {}, \
+                    "Document identifier `{}` is invalid. \
-a document id can be of type integer or string \
+A document identifier can be of type integer or string, \
-only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and underscores (_)",
+only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and underscores (_).",
-                    json
+                    document_id
                 )
             }
-            Self::InvalidFilterAttribute(error) => error.fmt(f),
             Self::InvalidSortableAttribute { field, valid_fields } => {
                 let valid_names =
                     valid_fields.iter().map(AsRef::as_ref).collect::<Vec<_>>().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 \
+                "The sort ranking rule must be specified in the \
-rankingRules setting to use the sort parameter at search time",
+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::MissingPrimaryKey => f.write_str("Missing primary key."),
-            Self::MaxDatabaseSizeReached => f.write_str("maximum database size reached"),
+            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::NoSpaceLeftOnDevice => f.write_str("No space left on device."),
-            Self::InvalidStoreFile => f.write_str("store file is not a valid database file"),
+            Self::InvalidStoreFile => f.write_str("Store file is not a valid database file."),
-            Self::PrimaryKeyCannotBeChanged => {
+            Self::PrimaryKeyCannotBeChanged(primary_key) => {
-                f.write_str("primary key cannot be changed if the database contains documents")
+                write!(f, "Index already has a primary key: `{}`.", primary_key)
-            }
-            Self::PrimaryKeyCannotBeReset => {
-                f.write_str("primary key cannot be reset if the database contains documents")
             }
             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)
             }
         }
     }

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<FilterCondition> {
         let fields_ids_map = index.fields_ids_map(rtxn)?;
         let filterable_fields = index.filterable_fields(rtxn)?;
-        let lexed =
+        let lexed = FilterParser::parse(Rule::prgm, expression).map_err(FilterError::Syntax)?;
-            FilterParser::parse(Rule::prgm, expression).map_err(UserError::InvalidFilter)?;
         FilterCondition::from_pairs(&fields_ids_map, &filterable_fields, lexed)
     }
 
@@ -169,15 +167,11 @@ impl FilterCondition {
         item: Pair<Rule>,
     ) -> Result<FilterCondition> {
         if !filterable_fields.contains("_geo") {
-            return Err(UserError::InvalidFilterAttribute(PestError::new_from_span(
+            return Err(FilterError::InvalidAttribute {
-                ErrorVariant::CustomError {
+                field: "_geo".to_string(),
-                    message: format!(
+                valid_fields: filterable_fields.clone(),
-                    "attribute `_geo` is not filterable, available filterable attributes are: {}",
+            }
-                    filterable_fields.iter().join(", "),
+            .into());
-                ),
-                },
-                item.as_span(),
-            )))?;
         }
         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::<StdResult<Vec<(f64, _)>, _>>()
-            .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) = (&parameters[0], &parameters[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<Rule>,
     ) -> Result<FilterCondition> {
         let mut items = item.into_inner();
-        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)
+        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
-            .map_err(UserError::InvalidFilterAttribute)?
-        {
             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 lvalue = lresult.map_err(FilterError::Syntax)?;
-        let rvalue = rresult.map_err(UserError::InvalidFilter)?;
+        let rvalue = rresult.map_err(FilterError::Syntax)?;
 
         Ok(Operator(fid, Between(lvalue, rvalue)))
     }
@@ -252,9 +244,7 @@ impl FilterCondition {
         item: Pair<Rule>,
     ) -> Result<FilterCondition> {
         let mut items = item.into_inner();
-        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)
+        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
-            .map_err(UserError::InvalidFilterAttribute)?
-        {
             Some(fid) => fid,
             None => return Ok(Empty),
         };
@@ -272,16 +262,14 @@ impl FilterCondition {
         item: Pair<Rule>,
     ) -> Result<FilterCondition> {
         let mut items = item.into_inner();
-        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)
+        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
-            .map_err(UserError::InvalidFilterAttribute)?
-        {
             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<Rule>,
     ) -> Result<FilterCondition> {
         let mut items = item.into_inner();
-        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)
+        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
-            .map_err(UserError::InvalidFilterAttribute)?
-        {
             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<Rule>,
     ) -> Result<FilterCondition> {
         let mut items = item.into_inner();
-        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)
+        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
-            .map_err(UserError::InvalidFilterAttribute)?
-        {
             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<Rule>,
     ) -> Result<FilterCondition> {
         let mut items = item.into_inner();
-        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)
+        let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
-            .map_err(UserError::InvalidFilterAttribute)?
-        {
             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<String>,
     items: &mut Pairs<Rule>,
-) -> StdResult<Option<FieldId>, PestError<Rule>> {
+) -> StdResult<Option<FieldId>, 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!(
+                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()) })
-                    "`_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.",
-                )
             }
-            key @ "_geo" => {
+            "_geo" => {
-                format!(
+                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()) })
-                    "`{}` 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
-                )
             }
-            key => format!(
+            key =>
-                "`{}` is a reserved keyword and thus can't be used as a filter expression.",
+                Err(FilterError::ReservedKeyword { field: key.to_string(), context: None }),
-                key
-            ),
         };
-        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(
+        return Err(FilterError::InvalidAttribute {
-            ErrorVariant::CustomError {
+            field: key.as_str().to_string(),
-                message: format!(
+            valid_fields: filterable_fields.clone(),
-                    "attribute `{}` is not filterable, available filterable attributes are: {}.",
+        });
-                    key.as_str(),
-                    filterable_fields.iter().join(", "),
-                ),
-            },
-            key.as_span(),
-        ));
     }
 
     Ok(fields_ids_map.id(key.as_str()))

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 =

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...