Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a means to deal with malformed facet text representation for the query parser #1056

Merged
merged 5 commits into from
May 27, 2021
Merged

Provide a means to deal with malformed facet text representation for the query parser #1056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9c7affa
Provide a means to deal with malformed facet text representation for …
moriyoshi May 19, 2021
08e0bd6
s/path_/path_ref/
moriyoshi May 21, 2021
e944f48
Specific error enum for the facet parse error.
moriyoshi May 21, 2021
2a1bdba
Add test.
moriyoshi May 21, 2021
29aaf00
Propagate the original error message.
moriyoshi May 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/faceted_search.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
18 changes: 9 additions & 9 deletions src/collector/facet_collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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()
Expand Down
10 changes: 5 additions & 5 deletions src/fastfield/facet_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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();
Expand Down
17 changes: 13 additions & 4 deletions src/query/query_parser/query_parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::schema::{Field, Schema};
use crate::schema::{FieldType, Term};
use crate::tokenizer::TokenizerManager;
use crate::Score;
use crate::TantivyError;
use std::borrow::Cow;
use std::collections::HashMap;
use std::num::{ParseFloatError, ParseIntError};
Expand Down Expand Up @@ -68,6 +69,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 invalid.
#[error("The facet field is malformed")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but I think this error should forward the original error message or the user would not see which part of the query failed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this #[error("{0}")]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch :) I missed that. Thanks.

FacetFormatError(String),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be moved to TantivyError

Copy link
Contributor Author

@moriyoshi moriyoshi May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While an additional entry in TantivyError for this will possibly be handy later, I prefer this to be a query parser error like others (e.g. date time format error) because one can think Facet string representations are coincidentally the same between Facet and QueryParser.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the view point of Facet::from_text(), it will just suffice, for now, to return with a single kind of error (meaning TantivyError::InvalidArgument) because the parser isn't made possible to report a detailed error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another look and think from_text should return a FacetParseError. This error would then be included in the tantivty and parse error sets. Currently there is a conversion to _ => Err(QueryParserError::SyntaxError),, but this is dead code as I understand it, which would be removed.

/// Error that may occur when parsing a facet
#[derive(Debug, Error)]
pub enum FacetParseError {
    /// The format for the facet field invalid.
    #[error("The facet field is malformed")]
    FacetParseError(String),
}

pub enum TantivyError {
    /// Failed to parse facet.
    #[error("{0}")]
    FacetParseError(#[from] FacetParseError),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a specific error enum for the facet parser totally makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added an enum to facet as I wouldn't think extending TantivyError was necessary.

}

impl From<ParseIntError> for QueryParserError {
Expand Down Expand Up @@ -358,10 +362,15 @@ 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) => match e {
TantivyError::InvalidArgument(path) => {
Err(QueryParserError::FacetFormatError(path))
}
_ => Err(QueryParserError::SyntaxError),
},
},
FieldType::Bytes(_) => {
let bytes = base64::decode(phrase).map_err(QueryParserError::ExpectedBase64)?;
let term = Term::from_field_bytes(field, &bytes);
Expand Down
75 changes: 40 additions & 35 deletions src/schema/facet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::common::BinarySerializable;
use crate::TantivyError;
use once_cell::sync::Lazy;
use regex::Regex;
use serde::{Deserialize, Deserializer, Serialize, Serializer};
Expand Down Expand Up @@ -75,11 +76,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, TantivyError>
where
T: ?Sized + AsRef<str>,
{
From::from(path)
#[derive(Copy, Clone)]
enum State {
Escaped,
Idle,
}
let path_ = path.as_ref();
moriyoshi marked this conversation as resolved.
Show resolved Hide resolved
if path_.is_empty() {
return Err(TantivyError::InvalidArgument(path_.to_string()));
}
if !path_.starts_with('/') {
return Err(TantivyError::InvalidArgument(path_.to_string()));
}
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..]);
Ok(Facet(facet_encoded))
}

/// Returns a `Facet` from an iterator over the different
Expand Down Expand Up @@ -137,39 +174,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)]
fulmicoton marked this conversation as resolved.
Show resolved Hide resolved
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()
}
}

Expand Down