From 54e34beac6b7824f1b706fdb164a2abb9dbc65d3 Mon Sep 17 00:00:00 2001
From: James Hiew <james@hiew.net>
Date: Sat, 7 Dec 2024 15:08:38 +0000
Subject: [PATCH] Check attributes are filterable before evaluating search
 query

---
 crates/filter-parser/src/lib.rs         | 57 +++++++++++++++++++++++++
 crates/milli/src/search/facet/filter.rs | 27 ++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/crates/filter-parser/src/lib.rs b/crates/filter-parser/src/lib.rs
index cfe009acb..dc5e776ae 100644
--- a/crates/filter-parser/src/lib.rs
+++ b/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";
diff --git a/crates/milli/src/search/facet/filter.rs b/crates/milli/src/search/facet/filter.rs
index c059d2d27..aa3849ffc 100644
--- a/crates/milli/src/search/facet/filter.rs
+++ b/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]