From ed6db196810f78632758fc386f8a7f5f6cd6f357 Mon Sep 17 00:00:00 2001 From: many Date: Thu, 28 Oct 2021 11:18:32 +0200 Subject: [PATCH] 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(), })? } _ => (),