3479: Unify "Bad latitude" & "Bad longitude" errors r=irevoire a=cymruu

# Pull Request

## Related issue
Fix part of #3006

## What does this PR do?
- Moved out `BadGeoLat`, `BadGeoLng`, `BadGeoBoundingBoxTopIsBelowBottom` from `FilterError` into newly introduced error type `ParseGeoError`. 
- Renamed `BadGeo` error  to `ReservedGeo`
- Used new `ParseGeoError` type in `FilterError` and `AscDescError`

Screenshot: 
![image](https://user-images.githubusercontent.com/2981598/217927231-fe23b6a3-2ea8-4145-98af-38eb61c4ff16.png)

I ran `cargo test --package milli -- --test-threads 1` and tests passed.
`--test-threads` was set to 1 because my OS complained about too many opened files.

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [ ] Have you made sure that the title is accurate and descriptive of the changes?


Co-authored-by: Filip Bachul <filipbachul@gmail.com>
Co-authored-by: filip <filipbachul@gmail.com>
This commit is contained in:
bors[bot] 2023-02-14 18:35:51 +00:00 committed by GitHub
commit d494c29768
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 81 additions and 67 deletions

View File

@ -7,45 +7,31 @@ use serde::{Deserialize, Serialize};
use thiserror::Error; use thiserror::Error;
use crate::error::is_reserved_keyword; use crate::error::is_reserved_keyword;
use crate::search::facet::BadGeoError;
use crate::{CriterionError, Error, UserError}; use crate::{CriterionError, Error, UserError};
/// This error type is never supposed to be shown to the end user. /// This error type is never supposed to be shown to the end user.
/// You must always cast it to a sort error or a criterion error. /// You must always cast it to a sort error or a criterion error.
#[derive(Debug)] #[derive(Error, Debug)]
pub enum AscDescError { pub enum AscDescError {
InvalidLatitude, #[error(transparent)]
InvalidLongitude, GeoError(BadGeoError),
#[error("Invalid syntax for the asc/desc parameter: expected expression ending by `:asc` or `:desc`, found `{name}`.")]
InvalidSyntax { name: String }, InvalidSyntax { name: String },
#[error("`{name}` is a reserved keyword and thus can't be used as a asc/desc rule.")]
ReservedKeyword { name: String }, ReservedKeyword { name: String },
} }
impl fmt::Display for AscDescError { impl From<BadGeoError> for AscDescError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn from(geo_error: BadGeoError) -> Self {
match self { AscDescError::GeoError(geo_error)
Self::InvalidLatitude => {
write!(f, "Latitude must be contained between -90 and 90 degrees.",)
}
Self::InvalidLongitude => {
write!(f, "Longitude must be contained between -180 and 180 degrees.",)
}
Self::InvalidSyntax { name } => {
write!(f, "Invalid syntax for the asc/desc parameter: expected expression ending by `:asc` or `:desc`, found `{}`.", name)
}
Self::ReservedKeyword { name } => {
write!(
f,
"`{}` is a reserved keyword and thus can't be used as a asc/desc rule.",
name
)
}
}
} }
} }
impl From<AscDescError> for CriterionError { impl From<AscDescError> for CriterionError {
fn from(error: AscDescError) -> Self { fn from(error: AscDescError) -> Self {
match error { match error {
AscDescError::InvalidLatitude | AscDescError::InvalidLongitude => { AscDescError::GeoError(_) => {
CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() } CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() }
} }
AscDescError::InvalidSyntax { name } => CriterionError::InvalidName { name }, AscDescError::InvalidSyntax { name } => CriterionError::InvalidName { name },
@ -85,9 +71,9 @@ impl FromStr for Member {
.map_err(|_| AscDescError::ReservedKeyword { name: text.to_string() }) .map_err(|_| AscDescError::ReservedKeyword { name: text.to_string() })
})?; })?;
if !(-90.0..=90.0).contains(&lat) { if !(-90.0..=90.0).contains(&lat) {
return Err(AscDescError::InvalidLatitude)?; return Err(BadGeoError::Lat(lat))?;
} else if !(-180.0..=180.0).contains(&lng) { } else if !(-180.0..=180.0).contains(&lng) {
return Err(AscDescError::InvalidLongitude)?; return Err(BadGeoError::Lng(lng))?;
} }
Ok(Member::Geo([lat, lng])) Ok(Member::Geo([lat, lng]))
} }
@ -162,10 +148,8 @@ impl FromStr for AscDesc {
#[derive(Error, Debug)] #[derive(Error, Debug)]
pub enum SortError { pub enum SortError {
#[error("{}", AscDescError::InvalidLatitude)] #[error(transparent)]
InvalidLatitude, ParseGeoError { error: BadGeoError },
#[error("{}", AscDescError::InvalidLongitude)]
InvalidLongitude,
#[error("Invalid syntax for the geo parameter: expected expression formated like \ #[error("Invalid syntax for the geo parameter: expected expression formated like \
`_geoPoint(latitude, longitude)` and ending by `:asc` or `:desc`, found `{name}`.")] `_geoPoint(latitude, longitude)` and ending by `:asc` or `:desc`, found `{name}`.")]
BadGeoPointUsage { name: String }, BadGeoPointUsage { name: String },
@ -184,8 +168,7 @@ pub enum SortError {
impl From<AscDescError> for SortError { impl From<AscDescError> for SortError {
fn from(error: AscDescError) -> Self { fn from(error: AscDescError) -> Self {
match error { match error {
AscDescError::InvalidLatitude => SortError::InvalidLatitude, AscDescError::GeoError(error) => SortError::ParseGeoError { error },
AscDescError::InvalidLongitude => SortError::InvalidLongitude,
AscDescError::InvalidSyntax { name } => SortError::InvalidName { name }, AscDescError::InvalidSyntax { name } => SortError::InvalidName { name },
AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => { AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => {
SortError::BadGeoPointUsage { name } SortError::BadGeoPointUsage { name }
@ -277,11 +260,11 @@ mod tests {
), ),
("_geoPoint(35, 85, 75):asc", ReservedKeyword { name: S("_geoPoint(35, 85, 75)") }), ("_geoPoint(35, 85, 75):asc", ReservedKeyword { name: S("_geoPoint(35, 85, 75)") }),
("_geoPoint(18):asc", ReservedKeyword { name: S("_geoPoint(18)") }), ("_geoPoint(18):asc", ReservedKeyword { name: S("_geoPoint(18)") }),
("_geoPoint(200, 200):asc", InvalidLatitude), ("_geoPoint(200, 200):asc", GeoError(BadGeoError::Lat(200.))),
("_geoPoint(90.000001, 0):asc", InvalidLatitude), ("_geoPoint(90.000001, 0):asc", GeoError(BadGeoError::Lat(90.000001))),
("_geoPoint(0, -180.000001):desc", InvalidLongitude), ("_geoPoint(0, -180.000001):desc", GeoError(BadGeoError::Lng(-180.000001))),
("_geoPoint(159.256, 130):asc", InvalidLatitude), ("_geoPoint(159.256, 130):asc", GeoError(BadGeoError::Lat(159.256))),
("_geoPoint(12, -2021):desc", InvalidLongitude), ("_geoPoint(12, -2021):desc", GeoError(BadGeoError::Lng(-2021.))),
]; ];
for (req, expected_error) in invalid_req { for (req, expected_error) in invalid_req {

View File

@ -21,18 +21,51 @@ pub struct Filter<'a> {
condition: FilterCondition<'a>, condition: FilterCondition<'a>,
} }
#[derive(Debug)]
pub enum BadGeoError {
Lat(f64),
Lng(f64),
BoundingBoxTopIsBelowBottom(f64, f64),
}
impl std::error::Error for BadGeoError {}
impl Display for BadGeoError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::BoundingBoxTopIsBelowBottom(top, bottom) => {
write!(f, "The top latitude `{top}` is below the bottom latitude `{bottom}`.")
}
Self::Lat(lat) => write!(
f,
"Bad latitude `{}`. Latitude must be contained between -90 and 90 degrees. ",
lat
),
Self::Lng(lng) => write!(
f,
"Bad longitude `{}`. Longitude must be contained between -180 and 180 degrees. ",
lng
),
}
}
}
#[derive(Debug)] #[derive(Debug)]
enum FilterError<'a> { enum FilterError<'a> {
AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet<String> }, AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet<String> },
BadGeo(&'a str), ParseGeoError(BadGeoError),
BadGeoLat(f64), ReservedGeo(&'a str),
BadGeoLng(f64),
BadGeoBoundingBoxTopIsBelowBottom(f64, f64),
Reserved(&'a str), Reserved(&'a str),
TooDeep, TooDeep,
} }
impl<'a> std::error::Error for FilterError<'a> {} impl<'a> std::error::Error for FilterError<'a> {}
impl<'a> From<BadGeoError> for FilterError<'a> {
fn from(geo_error: BadGeoError) -> Self {
FilterError::ParseGeoError(geo_error)
}
}
impl<'a> Display for FilterError<'a> { impl<'a> Display for FilterError<'a> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self { match self {
@ -44,7 +77,11 @@ impl<'a> Display for FilterError<'a> {
attribute, attribute,
) )
} else { } else {
let filterables_list = filterable_fields.iter().map(AsRef::as_ref).collect::<Vec<&str>>().join(" "); let filterables_list = filterable_fields
.iter()
.map(AsRef::as_ref)
.collect::<Vec<&str>>()
.join(" ");
write!( write!(
f, f,
@ -53,20 +90,19 @@ impl<'a> Display for FilterError<'a> {
filterables_list, filterables_list,
) )
} }
}, }
Self::TooDeep => write!(f, Self::TooDeep => write!(
f,
"Too many filter conditions, can't process more than {} filters.", "Too many filter conditions, can't process more than {} filters.",
MAX_FILTER_DEPTH MAX_FILTER_DEPTH
), ),
Self::ReservedGeo(keyword) => write!(f, "`{}` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` or `_geoBoundingBox([latitude, longitude], [latitude, longitude])` built-in rules to filter on `_geo` field coordinates.", keyword),
Self::Reserved(keyword) => write!( Self::Reserved(keyword) => write!(
f, f,
"`{}` is a reserved keyword and thus can't be used as a filter expression.", "`{}` is a reserved keyword and thus can't be used as a filter expression.",
keyword keyword
), ),
Self::BadGeo(keyword) => write!(f, "`{}` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` or `_geoBoundingBox([latitude, longitude], [latitude, longitude])` built-in rules to filter on `_geo` field coordinates.", keyword), Self::ParseGeoError(error) => write!(f, "{}", error),
Self::BadGeoBoundingBoxTopIsBelowBottom(top, bottom) => write!(f, "The top latitude `{top}` is below the bottom latitude `{bottom}`."),
Self::BadGeoLat(lat) => write!(f, "Bad latitude `{}`. Latitude must be contained between -90 and 90 degrees. ", lat),
Self::BadGeoLng(lng) => write!(f, "Bad longitude `{}`. Longitude must be contained between -180 and 180 degrees. ", lng),
} }
} }
} }
@ -298,10 +334,10 @@ impl<'a> Filter<'a> {
} else { } else {
match fid.value() { match fid.value() {
attribute @ "_geo" => { attribute @ "_geo" => {
Err(fid.as_external_error(FilterError::BadGeo(attribute)))? Err(fid.as_external_error(FilterError::ReservedGeo(attribute)))?
} }
attribute if attribute.starts_with("_geoPoint(") => { attribute if attribute.starts_with("_geoPoint(") => {
Err(fid.as_external_error(FilterError::BadGeo("_geoPoint")))? Err(fid.as_external_error(FilterError::ReservedGeo("_geoPoint")))?
} }
attribute @ "_geoDistance" => { attribute @ "_geoDistance" => {
Err(fid.as_external_error(FilterError::Reserved(attribute)))? Err(fid.as_external_error(FilterError::Reserved(attribute)))?
@ -353,14 +389,10 @@ impl<'a> Filter<'a> {
let base_point: [f64; 2] = let base_point: [f64; 2] =
[point[0].parse_finite_float()?, point[1].parse_finite_float()?]; [point[0].parse_finite_float()?, point[1].parse_finite_float()?];
if !(-90.0..=90.0).contains(&base_point[0]) { if !(-90.0..=90.0).contains(&base_point[0]) {
return Err( return Err(point[0].as_external_error(BadGeoError::Lat(base_point[0])))?;
point[0].as_external_error(FilterError::BadGeoLat(base_point[0]))
)?;
} }
if !(-180.0..=180.0).contains(&base_point[1]) { if !(-180.0..=180.0).contains(&base_point[1]) {
return Err( return Err(point[1].as_external_error(BadGeoError::Lng(base_point[1])))?;
point[1].as_external_error(FilterError::BadGeoLng(base_point[1]))
)?;
} }
let radius = radius.parse_finite_float()?; let radius = radius.parse_finite_float()?;
let rtree = match index.geo_rtree(rtxn)? { let rtree = match index.geo_rtree(rtxn)? {
@ -398,27 +430,26 @@ impl<'a> Filter<'a> {
bottom_right_point[1].parse_finite_float()?, bottom_right_point[1].parse_finite_float()?,
]; ];
if !(-90.0..=90.0).contains(&top_left[0]) { if !(-90.0..=90.0).contains(&top_left[0]) {
return Err(top_left_point[0] return Err(
.as_external_error(FilterError::BadGeoLat(top_left[0])))?; top_left_point[0].as_external_error(BadGeoError::Lat(top_left[0]))
)?;
} }
if !(-180.0..=180.0).contains(&top_left[1]) { if !(-180.0..=180.0).contains(&top_left[1]) {
return Err(top_left_point[1] return Err(
.as_external_error(FilterError::BadGeoLng(top_left[1])))?; top_left_point[1].as_external_error(BadGeoError::Lng(top_left[1]))
)?;
} }
if !(-90.0..=90.0).contains(&bottom_right[0]) { if !(-90.0..=90.0).contains(&bottom_right[0]) {
return Err(bottom_right_point[0] return Err(bottom_right_point[0]
.as_external_error(FilterError::BadGeoLat(bottom_right[0])))?; .as_external_error(BadGeoError::Lat(bottom_right[0])))?;
} }
if !(-180.0..=180.0).contains(&bottom_right[1]) { if !(-180.0..=180.0).contains(&bottom_right[1]) {
return Err(bottom_right_point[1] return Err(bottom_right_point[1]
.as_external_error(FilterError::BadGeoLng(bottom_right[1])))?; .as_external_error(BadGeoError::Lng(bottom_right[1])))?;
} }
if top_left[0] < bottom_right[0] { if top_left[0] < bottom_right[0] {
return Err(bottom_right_point[1].as_external_error( return Err(bottom_right_point[1].as_external_error(
FilterError::BadGeoBoundingBoxTopIsBelowBottom( BadGeoError::BoundingBoxTopIsBelowBottom(top_left[0], bottom_right[0]),
top_left[0],
bottom_right[0],
),
))?; ))?;
} }

View File

@ -4,7 +4,7 @@ use heed::types::{ByteSlice, DecodeIgnore};
use heed::{BytesDecode, RoTxn}; use heed::{BytesDecode, RoTxn};
pub use self::facet_distribution::{FacetDistribution, DEFAULT_VALUES_PER_FACET}; pub use self::facet_distribution::{FacetDistribution, DEFAULT_VALUES_PER_FACET};
pub use self::filter::Filter; pub use self::filter::{BadGeoError, Filter};
use crate::heed_codec::facet::{FacetGroupKeyCodec, FacetGroupValueCodec}; use crate::heed_codec::facet::{FacetGroupKeyCodec, FacetGroupValueCodec};
use crate::heed_codec::ByteSliceRefCodec; use crate::heed_codec::ByteSliceRefCodec;
mod facet_distribution; mod facet_distribution;