mirror of
				https://github.com/meilisearch/meilisearch.git
				synced 2025-10-31 16:06:31 +00:00 
			
		
		
		
	Merge #468
468: Add a new error message when the filterableAttributes are empty r=Kerollmops a=brunoocasali Fixes https://github.com/meilisearch/meilisearch/issues/2140 Is there a good way to reduce de duplication here? Maybe adding a shared function? I don't know the best and idiomatic way to do that, I appreciate any tip! Another doubt is related to the duplication of the calling: ```rs // filter.rs:373 FilterError::AttributeNotFilterable { attribute, filterable: filterable_fields.into_iter().collect::<Vec<_>>().join(" "), }, ``` and ```rs // filter.rs:424 return Err(point[0].as_external_error(FilterError::AttributeNotFilterable { attribute: "_geo", filterable: filterable_fields.into_iter().collect::<Vec<_>>().join(" "), }))?; ``` I think we could make the `filterable_fields.into_iter().collect::<Vec<_>>().join(" ")` directly into the error handling like the sortable error. I made it into the last commit, if this is something to avoid, let me know and I can remove it :) Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
This commit is contained in:
		| @@ -1,3 +1,4 @@ | |||||||
|  | use std::collections::HashSet; | ||||||
| use std::fmt::{Debug, Display}; | use std::fmt::{Debug, Display}; | ||||||
| use std::ops::Bound::{self, Excluded, Included}; | use std::ops::Bound::{self, Excluded, Included}; | ||||||
| use std::ops::Deref; | use std::ops::Deref; | ||||||
| @@ -27,7 +28,7 @@ pub struct Filter<'a> { | |||||||
|  |  | ||||||
| #[derive(Debug)] | #[derive(Debug)] | ||||||
| enum FilterError<'a> { | enum FilterError<'a> { | ||||||
|     AttributeNotFilterable { attribute: &'a str, filterable: String }, |     AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet<String> }, | ||||||
|     BadGeo(&'a str), |     BadGeo(&'a str), | ||||||
|     BadGeoLat(f64), |     BadGeoLat(f64), | ||||||
|     BadGeoLng(f64), |     BadGeoLng(f64), | ||||||
| @@ -39,12 +40,24 @@ impl<'a> std::error::Error for FilterError<'a> {} | |||||||
| impl<'a> Display for FilterError<'a> { | impl<'a> Display for FilterError<'a> { | ||||||
|     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { |     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||||||
|         match self { |         match self { | ||||||
|             Self::AttributeNotFilterable { attribute, filterable } => write!( |             Self::AttributeNotFilterable { attribute, filterable_fields } => { | ||||||
|  |                 if filterable_fields.is_empty() { | ||||||
|  |                     write!( | ||||||
|  |                         f, | ||||||
|  |                         "Attribute `{}` is not filterable. This index does not have configured filterable attributes.", | ||||||
|  |                         attribute, | ||||||
|  |                     ) | ||||||
|  |                 } else { | ||||||
|  |                     let filterables_list = filterable_fields.iter().map(AsRef::as_ref).collect::<Vec<_>>().join(" "); | ||||||
|  |  | ||||||
|  |                     write!( | ||||||
|                         f, |                         f, | ||||||
|                         "Attribute `{}` is not filterable. Available filterable attributes are: `{}`.", |                         "Attribute `{}` is not filterable. Available filterable attributes are: `{}`.", | ||||||
|                         attribute, |                         attribute, | ||||||
|                 filterable, |                         filterables_list, | ||||||
|             ), |                     ) | ||||||
|  |                 } | ||||||
|  |             }, | ||||||
|             Self::TooDeep => write!(f, |             Self::TooDeep => write!(f, | ||||||
|                 "Too many filter conditions, can't process more than {} filters.", |                 "Too many filter conditions, can't process more than {} filters.", | ||||||
|                 MAX_FILTER_DEPTH |                 MAX_FILTER_DEPTH | ||||||
| @@ -362,10 +375,7 @@ impl<'a> Filter<'a> { | |||||||
|                             return Err(fid.as_external_error( |                             return Err(fid.as_external_error( | ||||||
|                                 FilterError::AttributeNotFilterable { |                                 FilterError::AttributeNotFilterable { | ||||||
|                                     attribute, |                                     attribute, | ||||||
|                                     filterable: filterable_fields |                                     filterable_fields, | ||||||
|                                         .into_iter() |  | ||||||
|                                         .collect::<Vec<_>>() |  | ||||||
|                                         .join(" "), |  | ||||||
|                                 }, |                                 }, | ||||||
|                             ))?; |                             ))?; | ||||||
|                         } |                         } | ||||||
| @@ -416,7 +426,7 @@ impl<'a> Filter<'a> { | |||||||
|                 } else { |                 } else { | ||||||
|                     return Err(point[0].as_external_error(FilterError::AttributeNotFilterable { |                     return Err(point[0].as_external_error(FilterError::AttributeNotFilterable { | ||||||
|                         attribute: "_geo", |                         attribute: "_geo", | ||||||
|                         filterable: filterable_fields.into_iter().collect::<Vec<_>>().join(" "), |                         filterable_fields, | ||||||
|                     }))?; |                     }))?; | ||||||
|                 } |                 } | ||||||
|             } |             } | ||||||
| @@ -554,13 +564,13 @@ mod tests { | |||||||
|         let filter = Filter::from_str("_geoRadius(42, 150, 10)").unwrap().unwrap(); |         let filter = Filter::from_str("_geoRadius(42, 150, 10)").unwrap().unwrap(); | ||||||
|         let error = filter.evaluate(&rtxn, &index).unwrap_err(); |         let error = filter.evaluate(&rtxn, &index).unwrap_err(); | ||||||
|         assert!(error.to_string().starts_with( |         assert!(error.to_string().starts_with( | ||||||
|             "Attribute `_geo` is not filterable. Available filterable attributes are: ``." |             "Attribute `_geo` is not filterable. This index does not have configured filterable attributes." | ||||||
|         )); |         )); | ||||||
|  |  | ||||||
|         let filter = Filter::from_str("dog = \"bernese mountain\"").unwrap().unwrap(); |         let filter = Filter::from_str("dog = \"bernese mountain\"").unwrap().unwrap(); | ||||||
|         let error = filter.evaluate(&rtxn, &index).unwrap_err(); |         let error = filter.evaluate(&rtxn, &index).unwrap_err(); | ||||||
|         assert!(error.to_string().starts_with( |         assert!(error.to_string().starts_with( | ||||||
|             "Attribute `dog` is not filterable. Available filterable attributes are: ``." |             "Attribute `dog` is not filterable. This index does not have configured filterable attributes." | ||||||
|         )); |         )); | ||||||
|         drop(rtxn); |         drop(rtxn); | ||||||
|  |  | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user