diff --git a/examples/faceted_search.rs b/examples/faceted_search.rs index f6ed20ed7..427873581 100644 --- a/examples/faceted_search.rs +++ b/examples/faceted_search.rs @@ -92,7 +92,7 @@ fn main() -> tantivy::Result<()> { // Check the reference doc for different ways to create a `Facet` object. { - let facet = Facet::from_text("/Felidae/Pantherinae"); + let facet = Facet::from("/Felidae/Pantherinae"); let facet_term = Term::from_facet(classification, &facet); let facet_term_query = TermQuery::new(facet_term, IndexRecordOption::Basic); let mut facet_collector = FacetCollector::for_field(classification); diff --git a/src/collector/facet_collector.rs b/src/collector/facet_collector.rs index b2b35b69a..79b764400 100644 --- a/src/collector/facet_collector.rs +++ b/src/collector/facet_collector.rs @@ -539,10 +539,10 @@ mod tests { let index = Index::create_in_ram(schema); let mut index_writer = index.writer_for_tests().unwrap(); index_writer.add_document(doc!( - facet_field => Facet::from_text(&"/subjects/A/a"), - facet_field => Facet::from_text(&"/subjects/B/a"), - facet_field => Facet::from_text(&"/subjects/A/b"), - facet_field => Facet::from_text(&"/subjects/B/b"), + facet_field => Facet::from_text(&"/subjects/A/a").unwrap(), + facet_field => Facet::from_text(&"/subjects/B/a").unwrap(), + facet_field => Facet::from_text(&"/subjects/A/b").unwrap(), + facet_field => Facet::from_text(&"/subjects/B/b").unwrap(), )); index_writer.commit().unwrap(); let reader = index.reader().unwrap(); @@ -563,16 +563,16 @@ mod tests { let index = Index::create_in_ram(schema); let mut index_writer = index.writer_for_tests()?; index_writer.add_document(doc!( - facet_field => Facet::from_text(&"/A/A"), + facet_field => Facet::from_text(&"/A/A").unwrap(), )); index_writer.add_document(doc!( - facet_field => Facet::from_text(&"/A/B"), + facet_field => Facet::from_text(&"/A/B").unwrap(), )); index_writer.add_document(doc!( - facet_field => Facet::from_text(&"/A/C/A"), + facet_field => Facet::from_text(&"/A/C/A").unwrap(), )); index_writer.add_document(doc!( - facet_field => Facet::from_text(&"/D/C/A"), + facet_field => Facet::from_text(&"/D/C/A").unwrap(), )); index_writer.commit()?; let reader = index.reader()?; @@ -580,7 +580,7 @@ mod tests { assert_eq!(searcher.num_docs(), 4); let count_facet = |facet_str: &str| { - let term = Term::from_facet(facet_field, &Facet::from_text(facet_str)); + let term = Term::from_facet(facet_field, &Facet::from_text(facet_str).unwrap()); searcher .search(&TermQuery::new(term, IndexRecordOption::Basic), &Count) .unwrap() diff --git a/src/fastfield/facet_reader.rs b/src/fastfield/facet_reader.rs index 727e44424..ed27396b5 100644 --- a/src/fastfield/facet_reader.rs +++ b/src/fastfield/facet_reader.rs @@ -95,7 +95,7 @@ mod tests { let schema = schema_builder.build(); let index = Index::create_in_ram(schema); let mut index_writer = index.writer_for_tests()?; - index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b"))); + index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b").unwrap())); index_writer.commit()?; let searcher = index.reader()?.searcher(); let facet_reader = searcher @@ -118,7 +118,7 @@ mod tests { let schema = schema_builder.build(); let index = Index::create_in_ram(schema); let mut index_writer = index.writer_for_tests()?; - index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b"))); + index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b").unwrap())); index_writer.commit()?; let searcher = index.reader()?.searcher(); let facet_reader = searcher @@ -141,7 +141,7 @@ mod tests { let schema = schema_builder.build(); let index = Index::create_in_ram(schema); let mut index_writer = index.writer_for_tests()?; - index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b"))); + index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b").unwrap())); index_writer.commit()?; let searcher = index.reader()?.searcher(); let facet_reader = searcher @@ -164,7 +164,7 @@ mod tests { let schema = schema_builder.build(); let index = Index::create_in_ram(schema); let mut index_writer = index.writer_for_tests()?; - index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b"))); + index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b").unwrap())); index_writer.commit()?; let searcher = index.reader()?.searcher(); let facet_reader = searcher @@ -187,7 +187,7 @@ mod tests { let schema = schema_builder.build(); let index = Index::create_in_ram(schema); let mut index_writer = index.writer_for_tests()?; - index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b"))); + index_writer.add_document(doc!(facet_field=>Facet::from_text("/a/b").unwrap())); index_writer.add_document(Document::default()); index_writer.commit()?; let searcher = index.reader()?.searcher(); diff --git a/src/query/query_parser/query_parser.rs b/src/query/query_parser/query_parser.rs index 5ca96364c..650213359 100644 --- a/src/query/query_parser/query_parser.rs +++ b/src/query/query_parser/query_parser.rs @@ -8,7 +8,7 @@ use crate::query::Query; use crate::query::RangeQuery; use crate::query::TermQuery; use crate::query::{AllQuery, BoostQuery}; -use crate::schema::{Facet, IndexRecordOption}; +use crate::schema::{Facet, FacetParseError, IndexRecordOption}; use crate::schema::{Field, Schema}; use crate::schema::{FieldType, Term}; use crate::tokenizer::TokenizerManager; @@ -68,6 +68,9 @@ pub enum QueryParserError { /// The format for the date field is not RFC 3339 compliant. #[error("The date field has an invalid format")] DateFormatError(chrono::ParseError), + /// The format for the facet field is invalid. + #[error("The facet field is malformed: {0}")] + FacetFormatError(FacetParseError), } impl From for QueryParserError { @@ -88,6 +91,12 @@ impl From for QueryParserError { } } +impl From for QueryParserError { + fn from(err: FacetParseError) -> QueryParserError { + QueryParserError::FacetFormatError(err) + } +} + /// Recursively remove empty clause from the AST /// /// Returns `None` iff the `logical_ast` ended up being empty. @@ -358,10 +367,10 @@ impl QueryParser { )) } } - FieldType::HierarchicalFacet(_) => { - let facet = Facet::from_text(phrase); - Ok(vec![(0, Term::from_field_text(field, facet.encoded_str()))]) - } + FieldType::HierarchicalFacet(_) => match Facet::from_text(phrase) { + Ok(facet) => Ok(vec![(0, Term::from_field_text(field, facet.encoded_str()))]), + Err(e) => Err(QueryParserError::from(e)), + }, FieldType::Bytes(_) => { let bytes = base64::decode(phrase).map_err(QueryParserError::ExpectedBase64)?; let term = Term::from_field_bytes(field, &bytes); @@ -1027,6 +1036,19 @@ mod test { .is_ok()); } + #[test] + pub fn test_query_parser_expected_facet() { + let query_parser = make_query_parser(); + match query_parser.parse_query("facet:INVALID") { + Ok(_) => panic!("should never succeed"), + Err(e) => assert_eq!( + "The facet field is malformed: Failed to parse the facet string: 'INVALID'", + format!("{}", e) + ), + } + assert!(query_parser.parse_query("facet:\"/foo/bar\"").is_ok()); + } + #[test] pub fn test_query_parser_not_empty_but_no_tokens() { let query_parser = make_query_parser(); diff --git a/src/schema/facet.rs b/src/schema/facet.rs index 05243ce96..b98d67bc4 100644 --- a/src/schema/facet.rs +++ b/src/schema/facet.rs @@ -20,6 +20,14 @@ pub const FACET_SEP_BYTE: u8 = 0u8; /// representation of facets. (It is the null codepoint.) pub const FACET_SEP_CHAR: char = '\u{0}'; +/// An error enum for facet parser. +#[derive(Debug, PartialEq, Eq, Error)] +pub enum FacetParseError { + /// The facet text representation is unparsable. + #[error("Failed to parse the facet string: '{0}'")] + FacetParseError(String), +} + /// A Facet represent a point in a given hierarchy. /// /// They are typically represented similarly to a filepath. @@ -75,11 +83,47 @@ impl Facet { /// It is conceptually, if one of the steps of this path /// contains a `/` or a `\`, it should be escaped /// using an anti-slash `/`. - pub fn from_text(path: &T) -> Facet + pub fn from_text(path: &T) -> Result where T: ?Sized + AsRef, { - From::from(path) + #[derive(Copy, Clone)] + enum State { + Escaped, + Idle, + } + let path_ref = path.as_ref(); + if path_ref.is_empty() { + return Err(FacetParseError::FacetParseError(path_ref.to_string())); + } + if !path_ref.starts_with('/') { + return Err(FacetParseError::FacetParseError(path_ref.to_string())); + } + let mut facet_encoded = String::new(); + let mut state = State::Idle; + let path_bytes = path_ref.as_bytes(); + let mut last_offset = 1; + for i in 1..path_bytes.len() { + let c = path_bytes[i]; + match (state, c) { + (State::Idle, ESCAPE_BYTE) => { + facet_encoded.push_str(&path_ref[last_offset..i]); + last_offset = i + 1; + state = State::Escaped + } + (State::Idle, SLASH_BYTE) => { + facet_encoded.push_str(&path_ref[last_offset..i]); + facet_encoded.push(FACET_SEP_CHAR); + last_offset = i + 1; + } + (State::Escaped, _escaped_char) => { + state = State::Idle; + } + (State::Idle, _any_char) => {} + } + } + facet_encoded.push_str(&path_ref[last_offset..]); + Ok(Facet(facet_encoded)) } /// Returns a `Facet` from an iterator over the different @@ -137,39 +181,7 @@ impl Borrow for Facet { impl<'a, T: ?Sized + AsRef> From<&'a T> for Facet { fn from(path_asref: &'a T) -> Facet { - #[derive(Copy, Clone)] - enum State { - Escaped, - Idle, - } - let path: &str = path_asref.as_ref(); - assert!(!path.is_empty()); - assert!(path.starts_with('/')); - let mut facet_encoded = String::new(); - let mut state = State::Idle; - let path_bytes = path.as_bytes(); - let mut last_offset = 1; - for i in 1..path_bytes.len() { - let c = path_bytes[i]; - match (state, c) { - (State::Idle, ESCAPE_BYTE) => { - facet_encoded.push_str(&path[last_offset..i]); - last_offset = i + 1; - state = State::Escaped - } - (State::Idle, SLASH_BYTE) => { - facet_encoded.push_str(&path[last_offset..i]); - facet_encoded.push(FACET_SEP_CHAR); - last_offset = i + 1; - } - (State::Escaped, _escaped_char) => { - state = State::Idle; - } - (State::Idle, _any_char) => {} - } - } - facet_encoded.push_str(&path[last_offset..]); - Facet(facet_encoded) + Facet::from_text(path_asref).unwrap() } } @@ -226,7 +238,7 @@ impl Debug for Facet { #[cfg(test)] mod tests { - use super::Facet; + use super::{Facet, FacetParseError}; #[test] fn test_root() { @@ -288,4 +300,12 @@ mod tests { let facet = Facet::from_path(v.iter()); assert_eq!(facet.to_path_string(), "/"); } + + #[test] + fn test_from_text() { + assert_eq!( + Err(FacetParseError::FacetParseError("INVALID".to_string())), + Facet::from_text("INVALID") + ); + } } diff --git a/src/schema/mod.rs b/src/schema/mod.rs index 1c1a62171..b1f3b7d96 100644 --- a/src/schema/mod.rs +++ b/src/schema/mod.rs @@ -128,6 +128,7 @@ pub use self::schema::{Schema, SchemaBuilder}; pub use self::value::Value; pub use self::facet::Facet; +pub use self::facet::FacetParseError; pub(crate) use self::facet::FACET_SEP_BYTE; pub use self::facet_options::FacetOptions;