Merge #5135

5135: Check all search filter attributes are filterable upfront r=curquiza a=jameshiew

# Pull Request

## Related issue
Fixes #5069

## What does this PR do?
- checks all `fid`s in the `Filter` tree are filterable before evaluating search query
- returns AttributeNotFilterable error if any are not

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Co-authored-by: James Hiew <james@hiew.net>

crates/filter-parser/src/lib.rs

@@ -179,6 +179,26 @@ impl<'a> FilterCondition<'a> {
         }
     }
 
+    pub fn fids(&self, depth: usize) -> Box<dyn Iterator<Item = &Token> + '_> {
+        if depth == 0 {
+            return Box::new(std::iter::empty());
+        }
+        match self {
+            FilterCondition::Condition { fid, .. } | FilterCondition::In { fid, .. } => {
+                Box::new(std::iter::once(fid))
+            }
+            FilterCondition::Not(filter) => {
+                let depth = depth.saturating_sub(1);
+                filter.fids(depth)
+            }
+            FilterCondition::And(subfilters) | FilterCondition::Or(subfilters) => {
+                let depth = depth.saturating_sub(1);
+                Box::new(subfilters.iter().flat_map(move |f| f.fids(depth)))
+            }
+            _ => Box::new(std::iter::empty()),
+        }
+    }
+
     /// Returns the first token found at the specified depth, `None` if no token at this depth.
     pub fn token_at_depth(&self, depth: usize) -> Option<&Token> {
         match self {
@@ -978,6 +998,43 @@ pub mod tests {
         assert!(filter.token_at_depth(3).is_none());
     }
 
+    #[test]
+    fn fids() {
+        let filter = Fc::parse("field = value").unwrap().unwrap();
+        let fids: Vec<_> = filter.fids(MAX_FILTER_DEPTH).collect();
+        assert_eq!(fids.len(), 1);
+        assert_eq!(fids[0].value(), "field");
+
+        let filter = Fc::parse("field IN [1, 2, 3]").unwrap().unwrap();
+        let fids: Vec<_> = filter.fids(MAX_FILTER_DEPTH).collect();
+        assert_eq!(fids.len(), 1);
+        assert_eq!(fids[0].value(), "field");
+
+        let filter = Fc::parse("field != value").unwrap().unwrap();
+        let fids: Vec<_> = filter.fids(MAX_FILTER_DEPTH).collect();
+        assert_eq!(fids.len(), 1);
+        assert_eq!(fids[0].value(), "field");
+
+        let filter = Fc::parse("field1 = value1 AND field2 = value2").unwrap().unwrap();
+        let fids: Vec<_> = filter.fids(MAX_FILTER_DEPTH).collect();
+        assert_eq!(fids.len(), 2);
+        assert!(fids[0].value() == "field1");
+        assert!(fids[1].value() == "field2");
+
+        let filter = Fc::parse("field1 = value1 OR field2 = value2").unwrap().unwrap();
+        let fids: Vec<_> = filter.fids(MAX_FILTER_DEPTH).collect();
+        assert_eq!(fids.len(), 2);
+        assert!(fids[0].value() == "field1");
+        assert!(fids[1].value() == "field2");
+
+        let depth = 2;
+        let filter =
+            Fc::parse("field1 = value1 AND (field2 = value2 OR field3 = value3)").unwrap().unwrap();
+        let fids: Vec<_> = filter.fids(depth).collect();
+        assert_eq!(fids.len(), 1);
+        assert_eq!(fids[0].value(), "field1");
+    }
+
     #[test]
     fn token_from_str() {
         let s = "test string that should not be parsed";

crates/milli/src/search/facet/filter.rs

@@ -230,6 +230,15 @@ impl<'a> Filter<'a> {
     pub fn evaluate(&self, rtxn: &heed::RoTxn<'_>, index: &Index) -> Result<RoaringBitmap> {
         // to avoid doing this for each recursive call we're going to do it ONCE ahead of time
         let filterable_fields = index.filterable_fields(rtxn)?;
+        for fid in self.condition.fids(MAX_FILTER_DEPTH) {
+            let attribute = fid.value();
+            if !crate::is_faceted(attribute, &filterable_fields) {
+                return Err(fid.as_external_error(FilterError::AttributeNotFilterable {
+                    attribute,
+                    filterable_fields,
+                }))?;
+            }
+        }
         self.inner_evaluate(rtxn, index, &filterable_fields, None)
     }
 
@@ -816,6 +825,24 @@ mod tests {
         assert!(error.to_string().starts_with(
             "Attribute `name` is not filterable. Available filterable attributes are: `title`."
         ));
+
+        let filter = Filter::from_str("title = \"test\" AND name = 12").unwrap().unwrap();
+        let error = filter.evaluate(&rtxn, &index).unwrap_err();
+        assert!(error.to_string().starts_with(
+            "Attribute `name` is not filterable. Available filterable attributes are: `title`."
+        ));
+
+        let filter = Filter::from_str("title = \"test\" AND name IN [12]").unwrap().unwrap();
+        let error = filter.evaluate(&rtxn, &index).unwrap_err();
+        assert!(error.to_string().starts_with(
+            "Attribute `name` is not filterable. Available filterable attributes are: `title`."
+        ));
+
+        let filter = Filter::from_str("title = \"test\" AND name != 12").unwrap().unwrap();
+        let error = filter.evaluate(&rtxn, &index).unwrap_err();
+        assert!(error.to_string().starts_with(
+            "Attribute `name` is not filterable. Available filterable attributes are: `title`."
+        ));
     }
 
     #[test]