searchengines-web/MeiliSearch
searchengines-web/MeiliSearch
1
0
Fork 0
mirror of https://github.com/meilisearch/MeiliSearch synced 2024-11-23 13:24:27 +01:00
Code Issues Releases Wiki Activity

Merge #405

Browse Source 
405: Change some error messages r=ManyTheFish a=ManyTheFish



Co-authored-by: many <maxime@meilisearch.com>
This commit is contained in:
bors[bot] 2021-10-28 13:35:55 +00:00 committed by GitHub
commit 08ae47e475
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 161 additions and 214 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
milli/src/asc_desc.rs
View File

@ -28,12 +28,12 @@ impl fmt::Display for AscDescError {
write!(f, "Longitude must be contained between -180 and 180 degrees.",) write!(f, "Longitude must be contained between -180 and 180 degrees.",)
} }
Self::InvalidSyntax { name } => { Self::InvalidSyntax { name } => {
write!(f, "invalid asc/desc syntax for {}.", name) write!(f, "Invalid syntax for the asc/desc parameter: expected expression ending by `:asc` or `:desc`, found `{}`.", name)
} }
Self::ReservedKeyword { name } => { Self::ReservedKeyword { name } => {
write!( write!(
f, f,
"{} is a reserved keyword and thus can't be used as a asc/desc rule.", "`{}` is a reserved keyword and thus can't be used as a asc/desc rule.",
name name
) )
} }
@ -192,18 +192,18 @@ impl fmt::Display for SortError {
Self::BadGeoPointUsage { name } => { Self::BadGeoPointUsage { name } => {
write!( write!(
f, f,
"invalid syntax for the `_geoPoint` parameter: `{}`. \ "Invalid syntax for the geo parameter: expected expression formated like \
Usage: `_geoPoint(latitude, longitude):asc`.", `_geoPoint(latitude, longitude)` and ending by `:asc` or `:desc`, found `{}`.",
name name
) )
} }
Self::InvalidName { name } => { Self::InvalidName { name } => {
write!(f, "invalid syntax for the sort parameter `{}`.", name) write!(f, "Invalid syntax for the sort parameter: expected expression ending by `:asc` or `:desc`, found `{}`.", name)
} }
Self::ReservedName { name } => { Self::ReservedName { name } => {
write!( write!(
f, f,
"{} is a reserved keyword and thus can't be used as a sort expression.", "`{}` is a reserved keyword and thus can't be used as a sort expression.",
name name
) )
} }
@ -211,7 +211,7 @@ impl fmt::Display for SortError {
write!( write!(
f, f,
"`{}` is a reserved keyword and thus can't be used as a sort expression. \ "`{}` is a reserved keyword and thus can't be used as a sort expression. \
Use the `_geoPoint(latitude, longitude)` built-in rule to sort on `_geo` field coordinates.", Use the _geoPoint(latitude, longitude) built-in rule to sort on _geo field coordinates.",
name, name,
) )
} }
@ -318,50 +318,4 @@ mod tests {
); );
} }
} }
#[test]
fn sort_error_message() {
let errors = [
(
AscDescError::InvalidSyntax { name: S("truc:machin") },
S("invalid syntax for the sort parameter `truc:machin`."),
),
(
AscDescError::InvalidSyntax { name: S("hello:world") },
S("invalid syntax for the sort parameter `hello:world`."),
),
(
AscDescError::ReservedKeyword { name: S("_geo") },
S("`_geo` is a reserved keyword and thus can't be used as a sort expression. Use the `_geoPoint(latitude, longitude)` built-in rule to sort on `_geo` field coordinates."),
),
(
AscDescError::ReservedKeyword { name: S("_geoDistance") },
S("_geoDistance is a reserved keyword and thus can't be used as a sort expression.")
),
(
AscDescError::ReservedKeyword { name: S("_geoRadius(12, 13)") },
S("`_geoRadius` is a reserved keyword and thus can't be used as a sort expression. Use the `_geoPoint(latitude, longitude)` built-in rule to sort on `_geo` field coordinates."),
),
(
AscDescError::InvalidLatitude,
S("Latitude must be contained between -90 and 90 degrees."),
),
(
AscDescError::InvalidLongitude,
S("Longitude must be contained between -180 and 180 degrees."),
),
];
for (asc_desc_error, expected_message) in errors {
let sort_error = SortError::from(asc_desc_error);
assert_eq!(
sort_error.to_string(),
expected_message,
"was expecting {} for the error {:?} but instead got {}",
expected_message,
sort_error,
sort_error.to_string()
);
}
}
} }

14
milli/src/criterion.rs
View File

@ -19,21 +19,25 @@ impl fmt::Display for CriterionError {
match self { match self {
Self::InvalidName { name } => write!(f, "invalid ranking rule {}", name), Self::InvalidName { name } => write!(f, "invalid ranking rule {}", name),
Self::ReservedName { name } => { Self::ReservedName { name } => {
write!(f, "{} is a reserved keyword and thus can't be used as a ranking rule", name) write!(
f,
"`{}` is a reserved keyword and thus can't be used as a ranking rule",
name
)
} }
Self::ReservedNameForSort { name } => { Self::ReservedNameForSort { name } => {
write!( write!(
f, f,
"{} is a reserved keyword and thus can't be used as a ranking rule. \ "`{}` is a reserved keyword and thus can't be used as a ranking rule. \
{} can only be used for sorting at search time", `{}` can only be used for sorting at search time",
name, name name, name
) )
} }
Self::ReservedNameForFilter { name } => { Self::ReservedNameForFilter { name } => {
write!( write!(
f, f,
"{} is a reserved keyword and thus can't be used as a ranking rule. \ "`{}` is a reserved keyword and thus can't be used as a ranking rule. \
{} can only be used for filtering at search time", `{}` can only be used for filtering at search time",
name, name name, name
) )
} }

118
milli/src/error.rs
View File

@ -1,4 +1,4 @@
use std::collections::HashSet; use std::collections::BTreeSet;
use std::convert::Infallible; use std::convert::Infallible;
use std::error::Error as StdError; use std::error::Error as StdError;
use std::{fmt, io, str}; use std::{fmt, io, str};
@ -58,24 +58,29 @@ pub enum UserError {
CriterionError(CriterionError), CriterionError(CriterionError),
DocumentLimitReached, DocumentLimitReached,
InvalidDocumentId { document_id: Value }, InvalidDocumentId { document_id: Value },
InvalidFacetsDistribution { invalid_facets_name: HashSet<String> }, InvalidFacetsDistribution { invalid_facets_name: BTreeSet<String> },
InvalidFilter(pest::error::Error<ParserRule>), InvalidFilter(FilterError),
InvalidFilterAttribute(pest::error::Error<ParserRule>),
InvalidGeoField { document_id: Value, object: Value }, InvalidGeoField { document_id: Value, object: Value },
InvalidSortableAttribute { field: String, valid_fields: HashSet<String> }, InvalidSortableAttribute { field: String, valid_fields: BTreeSet<String> },
SortRankingRuleMissing, SortRankingRuleMissing,
InvalidStoreFile, InvalidStoreFile,
MaxDatabaseSizeReached, MaxDatabaseSizeReached,
MissingDocumentId { document: Object }, MissingDocumentId { primary_key: String, document: Object },
MissingPrimaryKey, MissingPrimaryKey,
NoSpaceLeftOnDevice, NoSpaceLeftOnDevice,
PrimaryKeyCannotBeChanged, PrimaryKeyCannotBeChanged(String),
PrimaryKeyCannotBeReset,
SerdeJson(serde_json::Error), SerdeJson(serde_json::Error),
SortError(SortError), SortError(SortError),
UnknownInternalDocumentId { document_id: DocumentId }, UnknownInternalDocumentId { document_id: DocumentId },
} }
#[derive(Debug)]
pub enum FilterError {
InvalidAttribute { field: String, valid_fields: BTreeSet<String> },
ReservedKeyword { field: String, context: Option<String> },
Syntax(pest::error::Error<ParserRule>),
}
impl From<io::Error> for Error { impl From<io::Error> for Error {
fn from(error: io::Error) -> Error { fn from(error: io::Error) -> Error {
// TODO must be improved and more precise // TODO must be improved and more precise
@ -160,6 +165,12 @@ impl From<UserError> for Error {
} }
} }
impl From<FilterError> for Error {
fn from(error: FilterError) -> Error {
Error::UserError(UserError::InvalidFilter(error))
}
}
impl From<SerializationError> for Error { impl From<SerializationError> for Error {
fn from(error: SerializationError) -> Error { fn from(error: SerializationError) -> Error {
Error::InternalError(InternalError::Serialization(error)) Error::InternalError(InternalError::Serialization(error))
@ -169,7 +180,7 @@ impl From<SerializationError> for Error {
impl fmt::Display for Error { impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self { match self {
Self::InternalError(error) => write!(f, "internal: {}", error), Self::InternalError(error) => write!(f, "internal: {}.", error),
Self::IoError(error) => error.fmt(f), Self::IoError(error) => error.fmt(f),
Self::UserError(error) => error.fmt(f), Self::UserError(error) => error.fmt(f),
} }
@ -182,15 +193,15 @@ impl fmt::Display for InternalError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self { match self {
Self::DatabaseMissingEntry { db_name, key } => { Self::DatabaseMissingEntry { db_name, key } => {
write!(f, "missing {} in the {} database", key.unwrap_or("key"), db_name) write!(f, "Missing {} in the {} database.", key.unwrap_or("key"), db_name)
} }
Self::FieldIdMapMissingEntry(error) => error.fmt(f), Self::FieldIdMapMissingEntry(error) => error.fmt(f),
Self::Fst(error) => error.fmt(f), Self::Fst(error) => error.fmt(f),
Self::GrenadInvalidCompressionType => { Self::GrenadInvalidCompressionType => {
f.write_str("invalid compression type have been specified to grenad") f.write_str("Invalid compression type have been specified to grenad.")
} }
Self::IndexingMergingKeys { process } => { Self::IndexingMergingKeys { process } => {
write!(f, "invalid merge while processing {}", process) write!(f, "Invalid merge while processing {}.", process)
} }
Self::Serialization(error) => error.fmt(f), Self::Serialization(error) => error.fmt(f),
Self::InvalidDatabaseTyping => HeedError::InvalidDatabaseTyping.fmt(f), Self::InvalidDatabaseTyping => HeedError::InvalidDatabaseTyping.fmt(f),
@ -208,67 +219,100 @@ impl StdError for InternalError {}
impl fmt::Display for UserError { impl fmt::Display for UserError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self { match self {
Self::AttributeLimitReached => f.write_str("maximum number of attributes reached"), Self::AttributeLimitReached => f.write_str("Maximum number of attributes reached."),
Self::CriterionError(error) => write!(f, "{}", error), Self::CriterionError(error) => write!(f, "{}", error),
Self::DocumentLimitReached => f.write_str("maximum number of documents reached"), Self::DocumentLimitReached => f.write_str("Maximum number of documents reached."),
Self::InvalidFacetsDistribution { invalid_facets_name } => { Self::InvalidFacetsDistribution { invalid_facets_name } => {
let name_list = let name_list =
invalid_facets_name.iter().map(AsRef::as_ref).collect::<Vec<_>>().join(", "); invalid_facets_name.iter().map(AsRef::as_ref).collect::<Vec<_>>().join(", ");
write!( write!(
f, f,
"invalid facet distribution, the fields {} are not set as filterable", "Invalid facet distribution, the fields `{}` are not set as filterable.",
name_list name_list
) )
} }
Self::InvalidFilter(error) => error.fmt(f), Self::InvalidFilter(error) => error.fmt(f),
Self::InvalidGeoField { document_id, object } => write!( Self::InvalidGeoField { document_id, object } => write!(
f, f,
"the document with the id: {} contains an invalid _geo field: {}", "The document with the id: `{}` contains an invalid _geo field: `{}`.",
document_id, object document_id, object
), ),
Self::InvalidDocumentId { document_id } => { Self::InvalidDocumentId { document_id } => {
let json = serde_json::to_string(document_id).unwrap(); let document_id = match document_id {
Value::String(id) => id.clone(),
_ => document_id.to_string(),
};
write!( write!(
f, f,
"document identifier is invalid {}, \ "Document identifier `{}` is invalid. \
a document id can be of type integer or string \ A document identifier can be of type integer or string, \
only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and underscores (_)", only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and underscores (_).",
json document_id
) )
} }
Self::InvalidFilterAttribute(error) => error.fmt(f),
Self::InvalidSortableAttribute { field, valid_fields } => { Self::InvalidSortableAttribute { field, valid_fields } => {
let valid_names = let valid_names =
valid_fields.iter().map(AsRef::as_ref).collect::<Vec<_>>().join(", "); valid_fields.iter().map(AsRef::as_ref).collect::<Vec<_>>().join(", ");
write!( write!(
f, f,
"Attribute {} is not sortable, available sortable attributes are: {}", "Attribute `{}` is not sortable. Available sortable attributes are: `{}`.",
field, valid_names field, valid_names
) )
} }
Self::SortRankingRuleMissing => f.write_str( Self::SortRankingRuleMissing => f.write_str(
"You must specify where \"sort\" is listed in the \ "The sort ranking rule must be specified in the \
rankingRules setting to use the sort parameter at search time", ranking rules settings to use the sort parameter at search time.",
), ),
Self::MissingDocumentId { document } => { Self::MissingDocumentId { primary_key, document } => {
let json = serde_json::to_string(document).unwrap(); let json = serde_json::to_string(document).unwrap();
write!(f, "document doesn't have an identifier {}", json) write!(f, "Document doesn't have a `{}` attribute: `{}`.", primary_key, json)
} }
Self::MissingPrimaryKey => f.write_str("missing primary key"), Self::MissingPrimaryKey => f.write_str("Missing primary key."),
Self::MaxDatabaseSizeReached => f.write_str("maximum database size reached"), Self::MaxDatabaseSizeReached => f.write_str("Maximum database size reached."),
// TODO where can we find it instead of writing the text ourselves? // TODO where can we find it instead of writing the text ourselves?
Self::NoSpaceLeftOnDevice => f.write_str("no space left on device"), Self::NoSpaceLeftOnDevice => f.write_str("No space left on device."),
Self::InvalidStoreFile => f.write_str("store file is not a valid database file"), Self::InvalidStoreFile => f.write_str("Store file is not a valid database file."),
Self::PrimaryKeyCannotBeChanged => { Self::PrimaryKeyCannotBeChanged(primary_key) => {
f.write_str("primary key cannot be changed if the database contains documents") write!(f, "Index already has a primary key: `{}`.", primary_key)
}
Self::PrimaryKeyCannotBeReset => {
f.write_str("primary key cannot be reset if the database contains documents")
} }
Self::SerdeJson(error) => error.fmt(f), Self::SerdeJson(error) => error.fmt(f),
Self::SortError(error) => write!(f, "{}", error), Self::SortError(error) => write!(f, "{}", error),
Self::UnknownInternalDocumentId { document_id } => { Self::UnknownInternalDocumentId { document_id } => {
write!(f, "an unknown internal document id have been used ({})", document_id) write!(f, "An unknown internal document id have been used: `{}`.", document_id)
}
}
}
}
impl fmt::Display for FilterError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Self::InvalidAttribute { field, valid_fields } => write!(
f,
"Attribute `{}` is not filterable. Available filterable attributes are: `{}`.",
field,
valid_fields
.clone()
.into_iter()
.reduce(|left, right| left + "`, `" + &right)
.unwrap_or_default()
),
Self::ReservedKeyword { field, context: Some(context) } => {
write!(
f,
"`{}` is a reserved keyword and thus can't be used as a filter expression. {}",
field, context
)
}
Self::ReservedKeyword { field, context: None } => {
write!(
f,
"`{}` is a reserved keyword and thus can't be used as a filter expression.",
field
)
}
Self::Syntax(syntax_helper) => {
write!(f, "Invalid syntax for the filter parameter: `{}`.", syntax_helper)
} }
} }
} }

165
milli/src/search/facet/filter_condition.rs
View File

@ -6,7 +6,6 @@ use std::str::FromStr;
use either::Either; use either::Either;
use heed::types::DecodeIgnore; use heed::types::DecodeIgnore;
use itertools::Itertools;
use log::debug; use log::debug;
use pest::error::{Error as PestError, ErrorVariant}; use pest::error::{Error as PestError, ErrorVariant};
use pest::iterators::{Pair, Pairs}; use pest::iterators::{Pair, Pairs};
@ -17,7 +16,7 @@ use self::FilterCondition::*;
use self::Operator::*; use self::Operator::*;
use super::parser::{FilterParser, Rule, PREC_CLIMBER}; use super::parser::{FilterParser, Rule, PREC_CLIMBER};
use super::FacetNumberRange; use super::FacetNumberRange;
use crate::error::UserError; use crate::error::FilterError;
use crate::heed_codec::facet::{ use crate::heed_codec::facet::{
FacetLevelValueF64Codec, FacetStringLevelZeroCodec, FacetStringLevelZeroValueCodec, FacetLevelValueF64Codec, FacetStringLevelZeroCodec, FacetStringLevelZeroValueCodec,
}; };
@ -117,8 +116,7 @@ impl FilterCondition {
) -> Result<FilterCondition> { ) -> Result<FilterCondition> {
let fields_ids_map = index.fields_ids_map(rtxn)?; let fields_ids_map = index.fields_ids_map(rtxn)?;
let filterable_fields = index.filterable_fields(rtxn)?; let filterable_fields = index.filterable_fields(rtxn)?;
let lexed = let lexed = FilterParser::parse(Rule::prgm, expression).map_err(FilterError::Syntax)?;
FilterParser::parse(Rule::prgm, expression).map_err(UserError::InvalidFilter)?;
FilterCondition::from_pairs(&fields_ids_map, &filterable_fields, lexed) FilterCondition::from_pairs(&fields_ids_map, &filterable_fields, lexed)
} }
@ -169,15 +167,11 @@ impl FilterCondition {
item: Pair<Rule>, item: Pair<Rule>,
) -> Result<FilterCondition> { ) -> Result<FilterCondition> {
if !filterable_fields.contains("_geo") { if !filterable_fields.contains("_geo") {
return Err(UserError::InvalidFilterAttribute(PestError::new_from_span( return Err(FilterError::InvalidAttribute {
ErrorVariant::CustomError { field: "_geo".to_string(),
message: format!( valid_fields: filterable_fields.into_iter().cloned().collect(),
"attribute `_geo` is not filterable, available filterable attributes are: {}", }
filterable_fields.iter().join(", "), .into());
),
},
item.as_span(),
)))?;
} }
let mut items = item.into_inner(); let mut items = item.into_inner();
let fid = match fields_ids_map.id("_geo") { let fid = match fields_ids_map.id("_geo") {
@ -194,27 +188,27 @@ impl FilterCondition {
.map(|param| (param.clone(), param.as_span())) .map(|param| (param.clone(), param.as_span()))
.map(|(param, span)| pest_parse(param).0.map(|arg| (arg, span))) .map(|(param, span)| pest_parse(param).0.map(|arg| (arg, span)))
.collect::<StdResult<Vec<(f64, _)>, _>>() .collect::<StdResult<Vec<(f64, _)>, _>>()
.map_err(UserError::InvalidFilter)?; .map_err(FilterError::Syntax)?;
if parameters.len() != 3 { if parameters.len() != 3 {
return Err(UserError::InvalidFilter(PestError::new_from_span( return Err(FilterError::Syntax(PestError::new_from_span(
ErrorVariant::CustomError { ErrorVariant::CustomError {
message: format!("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`"), message: format!("The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)"),
}, },
// we want to point to the last parameters and if there was no parameters we // we want to point to the last parameters and if there was no parameters we
// point to the parenthesis // point to the parenthesis
parameters.last().map(|param| param.1.clone()).unwrap_or(param_span), parameters.last().map(|param| param.1.clone()).unwrap_or(param_span),
)))?; )).into());
} }
let (lat, lng, distance) = (&parameters[0], &parameters[1], parameters[2].0); let (lat, lng, distance) = (&parameters[0], &parameters[1], parameters[2].0);
if !(-90.0..=90.0).contains(&lat.0) { if !(-90.0..=90.0).contains(&lat.0) {
return Err(UserError::InvalidFilter(PestError::new_from_span( return Err(FilterError::Syntax(PestError::new_from_span(
ErrorVariant::CustomError { ErrorVariant::CustomError {
message: format!("Latitude must be contained between -90 and 90 degrees."), message: format!("Latitude must be contained between -90 and 90 degrees."),
}, },
lat.1.clone(), lat.1.clone(),
)))?; )))?;
} else if !(-180.0..=180.0).contains(&lng.0) { } else if !(-180.0..=180.0).contains(&lng.0) {
return Err(UserError::InvalidFilter(PestError::new_from_span( return Err(FilterError::Syntax(PestError::new_from_span(
ErrorVariant::CustomError { ErrorVariant::CustomError {
message: format!("Longitude must be contained between -180 and 180 degrees."), message: format!("Longitude must be contained between -180 and 180 degrees."),
}, },
@ -230,9 +224,7 @@ impl FilterCondition {
item: Pair<Rule>, item: Pair<Rule>,
) -> Result<FilterCondition> { ) -> Result<FilterCondition> {
let mut items = item.into_inner(); let mut items = item.into_inner();
let fid = match field_id(fields_ids_map, filterable_fields, &mut items) let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
.map_err(UserError::InvalidFilterAttribute)?
{
Some(fid) => fid, Some(fid) => fid,
None => return Ok(Empty), None => return Ok(Empty),
}; };
@ -240,8 +232,8 @@ impl FilterCondition {
let (lresult, _) = pest_parse(items.next().unwrap()); let (lresult, _) = pest_parse(items.next().unwrap());
let (rresult, _) = pest_parse(items.next().unwrap()); let (rresult, _) = pest_parse(items.next().unwrap());
let lvalue = lresult.map_err(UserError::InvalidFilter)?; let lvalue = lresult.map_err(FilterError::Syntax)?;
let rvalue = rresult.map_err(UserError::InvalidFilter)?; let rvalue = rresult.map_err(FilterError::Syntax)?;
Ok(Operator(fid, Between(lvalue, rvalue))) Ok(Operator(fid, Between(lvalue, rvalue)))
} }
@ -252,9 +244,7 @@ impl FilterCondition {
item: Pair<Rule>, item: Pair<Rule>,
) -> Result<FilterCondition> { ) -> Result<FilterCondition> {
let mut items = item.into_inner(); let mut items = item.into_inner();
let fid = match field_id(fields_ids_map, filterable_fields, &mut items) let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
.map_err(UserError::InvalidFilterAttribute)?
{
Some(fid) => fid, Some(fid) => fid,
None => return Ok(Empty), None => return Ok(Empty),
}; };
@ -272,16 +262,14 @@ impl FilterCondition {
item: Pair<Rule>, item: Pair<Rule>,
) -> Result<FilterCondition> { ) -> Result<FilterCondition> {
let mut items = item.into_inner(); let mut items = item.into_inner();
let fid = match field_id(fields_ids_map, filterable_fields, &mut items) let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
.map_err(UserError::InvalidFilterAttribute)?
{
Some(fid) => fid, Some(fid) => fid,
None => return Ok(Empty), None => return Ok(Empty),
}; };
let value = items.next().unwrap(); let value = items.next().unwrap();
let (result, _svalue) = pest_parse(value); let (result, _svalue) = pest_parse(value);
let value = result.map_err(UserError::InvalidFilter)?; let value = result.map_err(FilterError::Syntax)?;
Ok(Operator(fid, GreaterThan(value))) Ok(Operator(fid, GreaterThan(value)))
} }
@ -292,16 +280,14 @@ impl FilterCondition {
item: Pair<Rule>, item: Pair<Rule>,
) -> Result<FilterCondition> { ) -> Result<FilterCondition> {
let mut items = item.into_inner(); let mut items = item.into_inner();
let fid = match field_id(fields_ids_map, filterable_fields, &mut items) let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
.map_err(UserError::InvalidFilterAttribute)?
{
Some(fid) => fid, Some(fid) => fid,
None => return Ok(Empty), None => return Ok(Empty),
}; };
let value = items.next().unwrap(); let value = items.next().unwrap();
let (result, _svalue) = pest_parse(value); let (result, _svalue) = pest_parse(value);
let value = result.map_err(UserError::InvalidFilter)?; let value = result.map_err(FilterError::Syntax)?;
Ok(Operator(fid, GreaterThanOrEqual(value))) Ok(Operator(fid, GreaterThanOrEqual(value)))
} }
@ -312,16 +298,14 @@ impl FilterCondition {
item: Pair<Rule>, item: Pair<Rule>,
) -> Result<FilterCondition> { ) -> Result<FilterCondition> {
let mut items = item.into_inner(); let mut items = item.into_inner();
let fid = match field_id(fields_ids_map, filterable_fields, &mut items) let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
.map_err(UserError::InvalidFilterAttribute)?
{
Some(fid) => fid, Some(fid) => fid,
None => return Ok(Empty), None => return Ok(Empty),
}; };
let value = items.next().unwrap(); let value = items.next().unwrap();
let (result, _svalue) = pest_parse(value); let (result, _svalue) = pest_parse(value);
let value = result.map_err(UserError::InvalidFilter)?; let value = result.map_err(FilterError::Syntax)?;
Ok(Operator(fid, LowerThan(value))) Ok(Operator(fid, LowerThan(value)))
} }
@ -332,16 +316,14 @@ impl FilterCondition {
item: Pair<Rule>, item: Pair<Rule>,
) -> Result<FilterCondition> { ) -> Result<FilterCondition> {
let mut items = item.into_inner(); let mut items = item.into_inner();
let fid = match field_id(fields_ids_map, filterable_fields, &mut items) let fid = match field_id(fields_ids_map, filterable_fields, &mut items)? {
.map_err(UserError::InvalidFilterAttribute)?
{
Some(fid) => fid, Some(fid) => fid,
None => return Ok(Empty), None => return Ok(Empty),
}; };
let value = items.next().unwrap(); let value = items.next().unwrap();
let (result, _svalue) = pest_parse(value); let (result, _svalue) = pest_parse(value);
let value = result.map_err(UserError::InvalidFilter)?; let value = result.map_err(FilterError::Syntax)?;
Ok(Operator(fid, LowerThanOrEqual(value))) Ok(Operator(fid, LowerThanOrEqual(value)))
} }
@ -598,43 +580,27 @@ fn field_id(
fields_ids_map: &FieldsIdsMap, fields_ids_map: &FieldsIdsMap,
filterable_fields: &HashSet<String>, filterable_fields: &HashSet<String>,
items: &mut Pairs<Rule>, items: &mut Pairs<Rule>,
) -> StdResult<Option<FieldId>, PestError<Rule>> { ) -> StdResult<Option<FieldId>, FilterError> {
// lexing ensures that we at least have a key // lexing ensures that we at least have a key
let key = items.next().unwrap(); let key = items.next().unwrap();
if key.as_rule() == Rule::reserved { if key.as_rule() == Rule::reserved {
let message = match key.as_str() { return match key.as_str() {
key if key.starts_with("_geoPoint") => { key if key.starts_with("_geoPoint") => {
format!( Err(FilterError::ReservedKeyword { field: "_geoPoint".to_string(), context: Some("Use the _geoRadius(latitude, longitude, distance) built-in rule to filter on _geo field coordinates.".to_string()) })
"`_geoPoint` is a reserved keyword and thus can't be used as a filter expression. \
Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates.",
)
} }
key @ "_geo" => { "_geo" => {
format!( Err(FilterError::ReservedKeyword { field: "_geo".to_string(), context: Some("Use the _geoRadius(latitude, longitude, distance) built-in rule to filter on _geo field coordinates.".to_string()) })
"`{}` is a reserved keyword and thus can't be used as a filter expression. \
Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates.",
key
)
} }
key => format!( key =>
"`{}` is a reserved keyword and thus can't be used as a filter expression.", Err(FilterError::ReservedKeyword { field: key.to_string(), context: None }),
key
),
}; };
return Err(PestError::new_from_span(ErrorVariant::CustomError { message }, key.as_span()));
} }
if !filterable_fields.contains(key.as_str()) { if !filterable_fields.contains(key.as_str()) {
return Err(PestError::new_from_span( return Err(FilterError::InvalidAttribute {
ErrorVariant::CustomError { field: key.as_str().to_string(),
message: format!( valid_fields: filterable_fields.into_iter().cloned().collect(),
"attribute `{}` is not filterable, available filterable attributes are: {}.", });
key.as_str(),
filterable_fields.iter().join(", "),
),
},
key.as_span(),
));
} }
Ok(fields_ids_map.id(key.as_str())) Ok(fields_ids_map.id(key.as_str()))
@ -789,39 +755,13 @@ mod tests {
let index = Index::new(options, &path).unwrap(); let index = Index::new(options, &path).unwrap();
let rtxn = index.read_txn().unwrap(); let rtxn = index.read_txn().unwrap();
let error = FilterCondition::from_str(&rtxn, &index, "_geo = 12").unwrap_err(); assert!(FilterCondition::from_str(&rtxn, &index, "_geo = 12").is_err());
assert!(error
.to_string()
.contains("`_geo` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates."),
"{}",
error.to_string()
);
let error = assert!(FilterCondition::from_str(&rtxn, &index, r#"_geoDistance <= 1000"#).is_err());
FilterCondition::from_str(&rtxn, &index, r#"_geoDistance <= 1000"#).unwrap_err();
assert!(error
.to_string()
.contains("`_geoDistance` is a reserved keyword and thus can't be used as a filter expression."),
"{}",
error.to_string()
);
let error = FilterCondition::from_str(&rtxn, &index, r#"_geoPoint > 5"#).unwrap_err(); assert!(FilterCondition::from_str(&rtxn, &index, r#"_geoPoint > 5"#).is_err());
assert!(error
.to_string()
.contains("`_geoPoint` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates."),
"{}",
error.to_string()
);
let error = assert!(FilterCondition::from_str(&rtxn, &index, r#"_geoPoint(12, 16) > 5"#).is_err());
FilterCondition::from_str(&rtxn, &index, r#"_geoPoint(12, 16) > 5"#).unwrap_err();
assert!(error
.to_string()
.contains("`_geoPoint` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` built-in rule to filter on `_geo` field coordinates."),
"{}",
error.to_string()
);
} }
#[test] #[test]
@ -838,15 +778,6 @@ mod tests {
builder.execute(|_, _| ()).unwrap(); builder.execute(|_, _| ()).unwrap();
wtxn.commit().unwrap(); wtxn.commit().unwrap();
let rtxn = index.read_txn().unwrap();
// _geo is not filterable
let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(12, 12, 10)");
assert!(result.is_err());
let error = result.unwrap_err();
assert!(error
.to_string()
.contains("attribute `_geo` is not filterable, available filterable attributes are:"),);
let mut wtxn = index.write_txn().unwrap(); let mut wtxn = index.write_txn().unwrap();
let mut builder = Settings::new(&mut wtxn, &index, 0); let mut builder = Settings::new(&mut wtxn, &index, 0);
builder.set_filterable_fields(hashset! { S("_geo"), S("price") }); builder.set_filterable_fields(hashset! { S("_geo"), S("price") });
@ -898,26 +829,34 @@ mod tests {
let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius"); let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius");
assert!(result.is_err()); assert!(result.is_err());
let error = result.unwrap_err(); let error = result.unwrap_err();
assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); assert!(error.to_string().contains(
"The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)"
));
// georadius don't have any parameters // georadius don't have any parameters
let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius()"); let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius()");
assert!(result.is_err()); assert!(result.is_err());
let error = result.unwrap_err(); let error = result.unwrap_err();
assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); assert!(error.to_string().contains(
"The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)"
));
// georadius don't have enough parameters // georadius don't have enough parameters
let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(1, 2)"); let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(1, 2)");
assert!(result.is_err()); assert!(result.is_err());
let error = result.unwrap_err(); let error = result.unwrap_err();
assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); assert!(error.to_string().contains(
"The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)"
));
// georadius have too many parameters // georadius have too many parameters
let result = let result =
FilterCondition::from_str(&rtxn, &index, "_geoRadius(1, 2, 3, 4, 5, 6, 7, 8, 9, 10)"); FilterCondition::from_str(&rtxn, &index, "_geoRadius(1, 2, 3, 4, 5, 6, 7, 8, 9, 10)");
assert!(result.is_err()); assert!(result.is_err());
let error = result.unwrap_err(); let error = result.unwrap_err();
assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); assert!(error.to_string().contains(
"The _geoRadius filter expect three arguments: _geoRadius(latitude, longitude, radius)"
));
// georadius have a bad latitude // georadius have a bad latitude
let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-100, 150, 10)"); let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-100, 150, 10)");

4
milli/src/search/mod.rs
View File

@ -151,13 +151,13 @@ impl<'a> Search<'a> {
Member::Field(ref field) if !sortable_fields.contains(field) => { Member::Field(ref field) if !sortable_fields.contains(field) => {
return Err(UserError::InvalidSortableAttribute { return Err(UserError::InvalidSortableAttribute {
field: field.to_string(), field: field.to_string(),
valid_fields: sortable_fields, valid_fields: sortable_fields.into_iter().collect(),
})? })?
} }
Member::Geo(_) if !sortable_fields.contains("_geo") => { Member::Geo(_) if !sortable_fields.contains("_geo") => {
return Err(UserError::InvalidSortableAttribute { return Err(UserError::InvalidSortableAttribute {
field: "_geo".to_string(), field: "_geo".to_string(),
valid_fields: sortable_fields, valid_fields: sortable_fields.into_iter().collect(),
})? })?
} }
_ => (), _ => (),

6
milli/src/update/index_documents/transform.rs
View File

@ -187,7 +187,11 @@ impl Transform<'_, '_> {
} }
} }
return Err(UserError::MissingDocumentId { document: json }.into()); return Err(UserError::MissingDocumentId {
primary_key: primary_key_name,
document: json,
}
.into());
} }
let uuid = let uuid =

8
milli/src/update/settings.rs
View File

@ -465,7 +465,8 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> {
self.index.put_primary_key(self.wtxn, primary_key)?; self.index.put_primary_key(self.wtxn, primary_key)?;
Ok(()) Ok(())
} else { } else {
Err(UserError::PrimaryKeyCannotBeChanged.into()) let primary_key = self.index.primary_key(self.wtxn)?.unwrap();
Err(UserError::PrimaryKeyCannotBeChanged(primary_key.to_string()).into())
} }
} }
Setting::Reset => { Setting::Reset => {
@ -473,7 +474,8 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> {
self.index.delete_primary_key(self.wtxn)?; self.index.delete_primary_key(self.wtxn)?;
Ok(()) Ok(())
} else { } else {
Err(UserError::PrimaryKeyCannotBeReset.into()) let primary_key = self.index.primary_key(self.wtxn)?.unwrap();
Err(UserError::PrimaryKeyCannotBeChanged(primary_key.to_string()).into())
} }
} }
Setting::NotSet => Ok(()), Setting::NotSet => Ok(()),
@ -1105,7 +1107,7 @@ mod tests {
builder.reset_primary_key(); builder.reset_primary_key();
let err = builder.execute(|_, _| ()).unwrap_err(); let err = builder.execute(|_, _| ()).unwrap_err();
assert!(matches!(err, Error::UserError(UserError::PrimaryKeyCannotBeReset))); assert!(matches!(err, Error::UserError(UserError::PrimaryKeyCannotBeChanged(_))));
wtxn.abort().unwrap(); wtxn.abort().unwrap();
// But if we clear the database... // But if we clear the database...