Skip to content

Commit

Permalink
Check attributes are filterable before evaluating search query
Browse files Browse the repository at this point in the history
  • Loading branch information
jameshiew committed Dec 7, 2024
1 parent b21d7ae commit 54e34be
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 0 deletions.
57 changes: 57 additions & 0 deletions crates/filter-parser/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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";
Expand Down
27 changes: 27 additions & 0 deletions crates/milli/src/search/facet/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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]
Expand Down

0 comments on commit 54e34be

Please sign in to comment.