Provide a means to deal with malformed facet text representation for the query parser #1056
Conversation
…the query parser.
| /// The format for the facet field invalid. | ||
| #[error("The facet field is malformed")] | ||
| FacetFormatError(String), |
There was a problem hiding this comment.
I think this should be moved to TantivyError
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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),
There was a problem hiding this comment.
Having a specific error enum for the facet parser totally makes sense to me.
There was a problem hiding this comment.
I just added an enum to facet as I wouldn't think extending TantivyError was necessary.
|
As a sidenote, I intended to use Tantivy in a production and found that user-supplied queries including wrong facet strings can effectively cause DoS (the server itself is written in Python). That's why I made this fix. Anyway, thanks for offering such a great product for free with a generous license! |
| @@ -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")] | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Good catch :) I missed that. Thanks.
2a13c61 to
29aaf00
Compare
This patch requires API change.
As
Facet::from_textis not designed to return aResult, the query parser cannot handle invalid facet representations. This patch enables it to do so by modifyingFacet::from_text()to return aResult<Facet, TantivyError>instead of a bare Facet. Also, I added a new error kind toQueryParserError.BTW, I will agree to not insist any copyright on this patch.