Provide a means to deal with malformed facet text representation for …

…the query parser (#1056)

* Provide a means to deal with malformed facet text representation for the query parser.
* Specific error enum for the facet parse error.

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);

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,24 +563,24 @@ 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()?;
         let searcher = reader.searcher();
         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()

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();

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<ParseIntError> for QueryParserError {
@@ -88,6 +91,12 @@ impl From<chrono::ParseError> for QueryParserError {
     }
 }
 
+impl From<FacetParseError> 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();

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<T>(path: &T) -> Facet
+    pub fn from_text<T>(path: &T) -> Result<Facet, FacetParseError>
     where
         T: ?Sized + AsRef<str>,
     {
-        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<str> for Facet {
 
 impl<'a, T: ?Sized + AsRef<str>> 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")
+        );
+    }
 }

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;