Merge #371

371: Provide a sort error handler r=Kerollmops a=irevoire

This PR simplify the error handling of asc-desc rules for Meilisearch or any other wrapper by providing directly in milli a new error type called `SortError` that can be generated from an `AscDescError` and that can be automatically converted to a `UserError`.

Basically now, wherever you are in the code as a user or in milli you can parse an `AscDesc` syntax and depending on the context, cast it either as a `SortError` or a `CriterionError` in one line with improved error messages.

Co-authored-by: Tamo <tamo@meilisearch.com>

http-ui/src/main.rs

@@ -22,7 +22,9 @@ use meilisearch_tokenizer::{Analyzer, AnalyzerConfig};
 use milli::documents::DocumentBatchReader;
 use milli::update::UpdateIndexingStep::*;
 use milli::update::{IndexDocumentsMethod, Setting, UpdateBuilder};
-use milli::{obkv_to_json, CompressionType, FilterCondition, Index, MatchingWords, SearchResult};
+use milli::{
+    obkv_to_json, CompressionType, FilterCondition, Index, MatchingWords, SearchResult, SortError,
+};
 use once_cell::sync::OnceCell;
 use rayon::ThreadPool;
 use serde::{Deserialize, Serialize};
@@ -756,7 +758,7 @@ async fn main() -> anyhow::Result<()> {
             }
 
             if let Some(sort) = query.sort {
-                search.sort_criteria(vec![sort.parse().unwrap()]);
+                search.sort_criteria(vec![sort.parse().map_err(SortError::from).unwrap()]);
             }
 
             let SearchResult { matching_words, candidates, documents_ids } =

milli/src/asc_desc.rs

@@ -6,7 +6,7 @@ use std::str::FromStr;
 use serde::{Deserialize, Serialize};
 
 use crate::error::is_reserved_keyword;
-use crate::CriterionError;
+use crate::{CriterionError, Error, UserError};
 
 /// 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.
@@ -139,6 +139,68 @@ impl FromStr for AscDesc {
     }
 }
 
+#[derive(Debug)]
+pub enum SortError {
+    InvalidName { name: String },
+    ReservedName { name: String },
+    ReservedNameForSettings { name: String },
+    ReservedNameForFilter { name: String },
+}
+
+impl From<AscDescError> for SortError {
+    fn from(error: AscDescError) -> Self {
+        match error {
+            AscDescError::InvalidSyntax { name } => SortError::InvalidName { name },
+            AscDescError::ReservedKeyword { name } if &name == "_geo" => {
+                SortError::ReservedNameForSettings { name: "_geoPoint".to_string() }
+            }
+            AscDescError::ReservedKeyword { name } if name.starts_with("_geoRadius") => {
+                SortError::ReservedNameForFilter { name: "_geoRadius".to_string() }
+            }
+            AscDescError::ReservedKeyword { name } => SortError::ReservedName { name },
+        }
+    }
+}
+
+impl fmt::Display for SortError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Self::InvalidName { name } => {
+                write!(f, "invalid syntax for the sort parameter {}", name)
+            }
+            Self::ReservedName { name } => {
+                write!(
+                    f,
+                    "{} is a reserved keyword and thus can't be used as a sort expression",
+                    name
+                )
+            }
+            Self::ReservedNameForSettings { name } => {
+                write!(
+                    f,
+                    "{} is a reserved keyword and thus can't be used as a sort expression. \
+{} can only be used in the settings",
+                    name, name
+                )
+            }
+            Self::ReservedNameForFilter { name } => {
+                write!(
+                    f,
+                    "{} is a reserved keyword and thus can't be used as a sort expression. \
+{} can only be used for filtering at search time",
+                    name, name
+                )
+            }
+        }
+    }
+}
+
+impl From<SortError> for Error {
+    fn from(error: SortError) -> Self {
+        Self::UserError(UserError::SortError(error))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use big_s::S;

milli/src/error.rs

@@ -8,7 +8,7 @@ use rayon::ThreadPoolBuildError;
 use serde_json::{Map, Value};
 
 use crate::search::ParserRule;
-use crate::{CriterionError, DocumentId, FieldId};
+use crate::{CriterionError, DocumentId, FieldId, SortError};
 
 pub type Object = Map<String, Value>;
 
@@ -61,8 +61,6 @@ pub enum UserError {
     InvalidFacetsDistribution { invalid_facets_name: HashSet<String> },
     InvalidFilter(pest::error::Error<ParserRule>),
     InvalidFilterAttribute(pest::error::Error<ParserRule>),
-    InvalidSortName { name: String },
-    InvalidReservedSortName { name: String },
     InvalidGeoField { document_id: Value, object: Value },
     InvalidSortableAttribute { field: String, valid_fields: HashSet<String> },
     SortRankingRuleMissing,
@@ -74,6 +72,7 @@ pub enum UserError {
     PrimaryKeyCannotBeChanged,
     PrimaryKeyCannotBeReset,
     SerdeJson(serde_json::Error),
+    SortError(SortError),
     UnknownInternalDocumentId { document_id: DocumentId },
 }
 
@@ -227,13 +226,6 @@ impl fmt::Display for UserError {
                 "the document with the id: {} contains an invalid _geo field: {}",
                 document_id, object
             ),
-            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!(
@@ -245,9 +237,6 @@ only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and undersco
                 )
             }
             Self::InvalidFilterAttribute(error) => error.fmt(f),
-            Self::InvalidSortName { name } => {
-                write!(f, "Invalid syntax for the sort parameter: {}", name)
-            }
             Self::InvalidSortableAttribute { field, valid_fields } => {
                 let valid_names =
                     valid_fields.iter().map(AsRef::as_ref).collect::<Vec<_>>().join(", ");
@@ -277,6 +266,7 @@ only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and undersco
                 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)
             }

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, AscDescError, Member};
+pub use self::asc_desc::{AscDesc, AscDescError, Member, SortError};
 pub use self::criterion::{default_criteria, Criterion, CriterionError};
 pub use self::error::{
     Error, FieldIdMapMissingEntry, InternalError, SerializationError, UserError,