From 497187083b918b6cee3832fe7ddca5467e4bd69a Mon Sep 17 00:00:00 2001 From: Philipp Ahlner Date: Wed, 18 Jan 2023 13:24:26 +0100 Subject: [PATCH 1/3] Add test for bug #3007: Wrong error message Adds a test for #3007: Wrong error message when lat and lng are unparseable --- milli/src/index.rs | 36 +++++++++++++++++++ .../bug_3007/geo_faceted_documents_ids.snap | 4 +++ 2 files changed, 40 insertions(+) create mode 100644 milli/src/snapshots/index.rs/bug_3007/geo_faceted_documents_ids.snap diff --git a/milli/src/index.rs b/milli/src/index.rs index 46f8eb6a3..7ed9af424 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -2292,4 +2292,40 @@ pub(crate) mod tests { assert!(all_ids.insert(id)); } } + + #[test] + fn bug_3007() { + // https://github.com/meilisearch/meilisearch/issues/3007 + + use crate::error::{GeoError, UserError}; + let index = TempIndex::new(); + + // Given is an index with a geo field NOT contained in the sortable_fields of the settings + index + .update_settings(|settings| { + settings.set_primary_key("id".to_string()); + settings.set_filterable_fields(HashSet::from(["_geo".to_string()])); + }) + .unwrap(); + + // happy path + index.add_documents(documents!({ "id" : 5, "_geo": {"lat": 12.0, "lng": 11.0}})).unwrap(); + + db_snap!(index, geo_faceted_documents_ids); + + // both are unparseable, we expect GeoError::BadLatitudeAndLongitude + let err1 = index + .add_documents( + documents!({ "id" : 6, "_geo": {"lat": "unparseable", "lng": "unparseable"}}), + ) + .unwrap_err(); + assert!(matches!( + err1, + Error::UserError(UserError::InvalidGeoField( + GeoError::BadLatitudeAndLongitude { .. } + )) + )); + + db_snap!(index, geo_faceted_documents_ids); // ensure that no more document was inserted + } } diff --git a/milli/src/snapshots/index.rs/bug_3007/geo_faceted_documents_ids.snap b/milli/src/snapshots/index.rs/bug_3007/geo_faceted_documents_ids.snap new file mode 100644 index 000000000..f9ebc0c20 --- /dev/null +++ b/milli/src/snapshots/index.rs/bug_3007/geo_faceted_documents_ids.snap @@ -0,0 +1,4 @@ +--- +source: milli/src/index.rs +--- +[0, ] From a2cd7214f0fc31cd983a46fa19d2f9fed5ca3d32 Mon Sep 17 00:00:00 2001 From: Philipp Ahlner Date: Wed, 18 Jan 2023 13:24:46 +0100 Subject: [PATCH 2/3] Fixes error message when lat/lng are unparseable --- milli/src/index.rs | 4 +--- milli/src/update/index_documents/enrich.rs | 11 +++++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/milli/src/index.rs b/milli/src/index.rs index 7ed9af424..0ab596fa9 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -2321,9 +2321,7 @@ pub(crate) mod tests { .unwrap_err(); assert!(matches!( err1, - Error::UserError(UserError::InvalidGeoField( - GeoError::BadLatitudeAndLongitude { .. } - )) + Error::UserError(UserError::InvalidGeoField(GeoError::BadLatitudeAndLongitude { .. })) )); db_snap!(index, geo_faceted_documents_ids); // ensure that no more document was inserted diff --git a/milli/src/update/index_documents/enrich.rs b/milli/src/update/index_documents/enrich.rs index 3331497c9..4c735856d 100644 --- a/milli/src/update/index_documents/enrich.rs +++ b/milli/src/update/index_documents/enrich.rs @@ -98,7 +98,12 @@ pub fn enrich_documents_batch( // If the settings specifies that a _geo field must be used therefore we must check the // validity of it in all the documents of this batch and this is when we return `Some`. let geo_field_id = match documents_batch_index.id("_geo") { - Some(geo_field_id) if index.sortable_fields(rtxn)?.contains("_geo") => Some(geo_field_id), + Some(geo_field_id) + if index.sortable_fields(rtxn)?.contains("_geo") + || index.filterable_fields(rtxn)?.contains("_geo") => + { + Some(geo_field_id) + } _otherwise => None, }; @@ -367,7 +372,9 @@ pub fn extract_finite_float_from_value(value: Value) -> StdResult { pub fn validate_geo_from_json(id: &DocumentId, bytes: &[u8]) -> Result> { use GeoError::*; - let debug_id = || Value::from(id.debug()); + let debug_id = || { + serde_json::from_slice(id.value().as_bytes()).unwrap_or_else(|_| Value::from(id.debug())) + }; match serde_json::from_slice(bytes).map_err(InternalError::SerdeJson)? { Value::Object(mut object) => match (object.remove("lat"), object.remove("lng")) { (Some(lat), Some(lng)) => { From f5ca421227405ab312f57b0f13da48e6586ccb90 Mon Sep 17 00:00:00 2001 From: Philipp Ahlner Date: Thu, 19 Jan 2023 15:39:21 +0100 Subject: [PATCH 3/3] Superfluous test removed --- milli/src/update/index_documents/mod.rs | 28 ------------------------- 1 file changed, 28 deletions(-) diff --git a/milli/src/update/index_documents/mod.rs b/milli/src/update/index_documents/mod.rs index f912a756a..7e13afb1b 100644 --- a/milli/src/update/index_documents/mod.rs +++ b/milli/src/update/index_documents/mod.rs @@ -965,34 +965,6 @@ mod tests { .unwrap(); } - #[test] - fn index_all_flavour_of_geo() { - let mut index = TempIndex::new(); - index.index_documents_config.update_method = IndexDocumentsMethod::ReplaceDocuments; - - index - .update_settings(|settings| { - settings.set_filterable_fields(hashset!(S("_geo"))); - }) - .unwrap(); - - index - .add_documents(documents!([ - { "id": 0, "_geo": { "lat": 31, "lng": [42] } }, - { "id": 1, "_geo": { "lat": "31" }, "_geo.lng": 42 }, - { "id": 2, "_geo": { "lng": "42" }, "_geo.lat": "31" }, - { "id": 3, "_geo.lat": 31, "_geo.lng": "42" }, - ])) - .unwrap(); - - let rtxn = index.read_txn().unwrap(); - - let mut search = crate::Search::new(&rtxn, &index); - search.filter(crate::Filter::from_str("_geoRadius(31, 42, 0.000001)").unwrap().unwrap()); - let crate::SearchResult { documents_ids, .. } = search.execute().unwrap(); - assert_eq!(documents_ids, vec![0, 1, 2, 3]); - } - #[test] fn geo_error() { let mut index = TempIndex::new();