From 7d190d807853455a76d1d39b42b9f69e3174e712 Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 17 Jan 2024 15:51:52 +0100 Subject: [PATCH 1/2] add a bunch of tests and fix the error message when adding the geosearch as filterable/sortable while there is malformed documents in the DB --- meilisearch/tests/common/mod.rs | 2 +- meilisearch/tests/documents/add_documents.rs | 175 ++++++++++++++++++ .../extract/extract_geo_points.rs | 6 +- 3 files changed, 180 insertions(+), 3 deletions(-) diff --git a/meilisearch/tests/common/mod.rs b/meilisearch/tests/common/mod.rs index d7888b7db..2b9e5e1d7 100644 --- a/meilisearch/tests/common/mod.rs +++ b/meilisearch/tests/common/mod.rs @@ -64,7 +64,7 @@ impl Display for Value { write!( f, "{}", - json_string!(self, { ".enqueuedAt" => "[date]", ".processedAt" => "[date]", ".finishedAt" => "[date]", ".duration" => "[duration]" }) + json_string!(self, { ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]", ".duration" => "[duration]" }) ) } } diff --git a/meilisearch/tests/documents/add_documents.rs b/meilisearch/tests/documents/add_documents.rs index b2904691f..9733f7741 100644 --- a/meilisearch/tests/documents/add_documents.rs +++ b/meilisearch/tests/documents/add_documents.rs @@ -1760,6 +1760,181 @@ async fn add_documents_invalid_geo_field() { "finishedAt": "[date]" } "###); + + // The three next tests are related to #4333 + + // _geo has a lat and lng but set to `null` + let documents = json!([ + { + "id": "12", + "_geo": { "lng": null, "lat": 67} + } + ]); + + let (response, code) = index.add_documents(documents, None).await; + snapshot!(code, @"202 Accepted"); + let response = index.wait_task(response.uid()).await; + snapshot!(json_string!(response, { ".duration" => "[duration]", ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]" }), + @r###" + { + "uid": 14, + "indexUid": "test", + "status": "failed", + "type": "documentAdditionOrUpdate", + "canceledBy": null, + "details": { + "receivedDocuments": 1, + "indexedDocuments": 0 + }, + "error": { + "message": "Could not parse longitude in the document with the id: `12`. Was expecting a finite number but instead got `null`.", + "code": "invalid_document_geo_field", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_geo_field" + }, + "duration": "[duration]", + "enqueuedAt": "[date]", + "startedAt": "[date]", + "finishedAt": "[date]" + } + "###); + + // _geo has a lat and lng but set to `null` + let documents = json!([ + { + "id": "12", + "_geo": { "lng": 35, "lat": null } + } + ]); + + let (response, code) = index.add_documents(documents, None).await; + snapshot!(code, @"202 Accepted"); + let response = index.wait_task(response.uid()).await; + snapshot!(json_string!(response, { ".duration" => "[duration]", ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]" }), + @r###" + { + "uid": 15, + "indexUid": "test", + "status": "failed", + "type": "documentAdditionOrUpdate", + "canceledBy": null, + "details": { + "receivedDocuments": 1, + "indexedDocuments": 0 + }, + "error": { + "message": "Could not parse latitude in the document with the id: `12`. Was expecting a finite number but instead got `null`.", + "code": "invalid_document_geo_field", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_geo_field" + }, + "duration": "[duration]", + "enqueuedAt": "[date]", + "startedAt": "[date]", + "finishedAt": "[date]" + } + "###); + + // _geo has a lat and lng but set to `null` + let documents = json!([ + { + "id": "13", + "_geo": { "lng": null, "lat": null } + } + ]); + + let (response, code) = index.add_documents(documents, None).await; + snapshot!(code, @"202 Accepted"); + let response = index.wait_task(response.uid()).await; + snapshot!(json_string!(response, { ".duration" => "[duration]", ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]" }), + @r###" + { + "uid": 16, + "indexUid": "test", + "status": "failed", + "type": "documentAdditionOrUpdate", + "canceledBy": null, + "details": { + "receivedDocuments": 1, + "indexedDocuments": 0 + }, + "error": { + "message": "Could not parse latitude nor longitude in the document with the id: `13`. Was expecting finite numbers but instead got `null` and `null`.", + "code": "invalid_document_geo_field", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_geo_field" + }, + "duration": "[duration]", + "enqueuedAt": "[date]", + "startedAt": "[date]", + "finishedAt": "[date]" + } + "###); +} + +// Related to #4333 +#[actix_rt::test] +async fn add_invalid_geo_and_then_settings() { + let server = Server::new().await; + let index = server.index("test"); + index.create(Some("id")).await; + + // _geo is not an object + let documents = json!([ + { + "id": "11", + "_geo": { "lat": null, "lng": null }, + } + ]); + let (ret, code) = index.add_documents(documents, None).await; + snapshot!(code, @"202 Accepted"); + let ret = index.wait_task(ret.uid()).await; + snapshot!(ret, @r###" + { + "uid": 1, + "indexUid": "test", + "status": "succeeded", + "type": "documentAdditionOrUpdate", + "canceledBy": null, + "details": { + "receivedDocuments": 1, + "indexedDocuments": 1 + }, + "error": null, + "duration": "[duration]", + "enqueuedAt": "[date]", + "startedAt": "[date]", + "finishedAt": "[date]" + } + "###); + + let (ret, code) = index.update_settings(json!({"sortableAttributes": ["_geo"]})).await; + snapshot!(code, @"202 Accepted"); + let ret = index.wait_task(ret.uid()).await; + snapshot!(ret, @r###" + { + "uid": 2, + "indexUid": "test", + "status": "failed", + "type": "settingsUpdate", + "canceledBy": null, + "details": { + "sortableAttributes": [ + "_geo" + ] + }, + "error": { + "message": "Could not parse latitude in the document with the id: `\"11\"`. Was expecting a finite number but instead got `null`.", + "code": "invalid_document_geo_field", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_document_geo_field" + }, + "duration": "[duration]", + "enqueuedAt": "[date]", + "startedAt": "[date]", + "finishedAt": "[date]" + } + "###); } #[actix_rt::test] diff --git a/milli/src/update/index_documents/extract/extract_geo_points.rs b/milli/src/update/index_documents/extract/extract_geo_points.rs index 5ee7967d2..d46b7ac37 100644 --- a/milli/src/update/index_documents/extract/extract_geo_points.rs +++ b/milli/src/update/index_documents/extract/extract_geo_points.rs @@ -34,8 +34,10 @@ pub fn extract_geo_points( // since we only need the primary key when we throw an error // we create this getter to lazily get it when needed let document_id = || -> Value { - let document_id = obkv.get(primary_key_id).unwrap(); - serde_json::from_slice(document_id).unwrap() + let reader = KvReaderDelAdd::new(obkv.get(primary_key_id).unwrap()); + let document_id = + reader.get(DelAdd::Deletion).or(reader.get(DelAdd::Addition)).unwrap(); + serde_json::from_slice(&document_id).unwrap() }; // first we get the two fields From 0887186ecfe39d8db946dd39969095772dba7f54 Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 17 Jan 2024 16:07:10 +0100 Subject: [PATCH 2/2] make clippy happy --- milli/src/update/index_documents/extract/extract_geo_points.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/milli/src/update/index_documents/extract/extract_geo_points.rs b/milli/src/update/index_documents/extract/extract_geo_points.rs index d46b7ac37..b3600e3bc 100644 --- a/milli/src/update/index_documents/extract/extract_geo_points.rs +++ b/milli/src/update/index_documents/extract/extract_geo_points.rs @@ -37,7 +37,7 @@ pub fn extract_geo_points( let reader = KvReaderDelAdd::new(obkv.get(primary_key_id).unwrap()); let document_id = reader.get(DelAdd::Deletion).or(reader.get(DelAdd::Addition)).unwrap(); - serde_json::from_slice(&document_id).unwrap() + serde_json::from_slice(document_id).unwrap() }; // first we get the two fields