From ced6cc0e23832724a4b40019fcef2f5a1d867810 Mon Sep 17 00:00:00 2001 From: mpostma Date: Mon, 22 Jun 2020 15:16:18 +0200 Subject: [PATCH 1/7] fix bad error report when primary key exists --- meilisearch-http/src/routes/index.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/meilisearch-http/src/routes/index.rs b/meilisearch-http/src/routes/index.rs index ba42dcc5d..a46e70621 100644 --- a/meilisearch-http/src/routes/index.rs +++ b/meilisearch-http/src/routes/index.rs @@ -253,17 +253,8 @@ async fn update_index( if let Some(id) = body.primary_key.clone() { if let Some(mut schema) = index.main.schema(writer)? { - match schema.primary_key() { - Some(_) => { - return Err(Error::bad_request( - "The primary key cannot be updated", - ).into()); - } - None => { - schema.set_primary_key(&id)?; - index.main.put_schema(writer, &schema)?; - } - } + schema.set_primary_key(&id)?; + index.main.put_schema(writer, &schema)?; } } index.main.put_updated_at(writer)?; From 6445eea94690af838144f5570ae442cd92c30e5d Mon Sep 17 00:00:00 2001 From: mpostma Date: Mon, 22 Jun 2020 16:09:57 +0200 Subject: [PATCH 2/7] update error types to be more accurate --- meilisearch-error/src/lib.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/meilisearch-error/src/lib.rs b/meilisearch-error/src/lib.rs index a6b4cbb3a..12cdc5930 100644 --- a/meilisearch-error/src/lib.rs +++ b/meilisearch-error/src/lib.rs @@ -86,26 +86,32 @@ impl Code { match self { // index related errors + // create index is thrown on internal error while creating an index. CreateIndex => ErrCode::invalid("index_creation_failed", StatusCode::BAD_REQUEST), IndexAlreadyExists => ErrCode::invalid("index_already_exists", StatusCode::BAD_REQUEST), + // thrown when requesting an unexisting index IndexNotFound => ErrCode::invalid("index_not_found", StatusCode::NOT_FOUND), InvalidIndexUid => ErrCode::invalid("invalid_index_uid", StatusCode::BAD_REQUEST), OpenIndex => ErrCode::internal("index_not_accessible", StatusCode::INTERNAL_SERVER_ERROR), // invalid state error InvalidState => ErrCode::internal("invalid_state", StatusCode::INTERNAL_SERVER_ERROR), + // thrown when no primary key has been set MissingPrimaryKey => ErrCode::internal("missing_primary_key", StatusCode::INTERNAL_SERVER_ERROR), - PrimaryKeyAlreadyPresent => ErrCode::internal("primary_key_already_present", StatusCode::INTERNAL_SERVER_ERROR), + // error thrown when trying to set an already existing primary key + PrimaryKeyAlreadyPresent => ErrCode::invalid("primary_key_already_present", StatusCode::BAD_REQUEST), // invalid document MaxFieldsLimitExceeded => ErrCode::invalid("max_field_limit_exceeded", StatusCode::BAD_REQUEST), MissingDocumentId => ErrCode::invalid("missing_document_id", StatusCode::BAD_REQUEST), + // error related to facets Facet => ErrCode::invalid("invalid_facet", StatusCode::BAD_REQUEST), + // error related to filters Filter => ErrCode::invalid("invalid_filter", StatusCode::BAD_REQUEST), BadParameter => ErrCode::invalid("bad_parameter", StatusCode::BAD_REQUEST), BadRequest => ErrCode::invalid("bad_request", StatusCode::BAD_REQUEST), - DocumentNotFound => ErrCode::internal("document_not_found", StatusCode::NOT_FOUND), + DocumentNotFound => ErrCode::invalid("document_not_found", StatusCode::NOT_FOUND), Internal => ErrCode::internal("internal", StatusCode::INTERNAL_SERVER_ERROR), InvalidToken => ErrCode::authentication("invalid_token", StatusCode::FORBIDDEN), Maintenance => ErrCode::internal("maintenance", StatusCode::SERVICE_UNAVAILABLE), From 7d3e93713452569cea2ebc597eb73a021cc8a527 Mon Sep 17 00:00:00 2001 From: mpostma Date: Mon, 22 Jun 2020 16:10:39 +0200 Subject: [PATCH 3/7] add tests for error codes --- meilisearch-http/tests/errors.rs | 126 +++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 meilisearch-http/tests/errors.rs diff --git a/meilisearch-http/tests/errors.rs b/meilisearch-http/tests/errors.rs new file mode 100644 index 000000000..5a7f2a708 --- /dev/null +++ b/meilisearch-http/tests/errors.rs @@ -0,0 +1,126 @@ +mod common; + +use serde_json::json; +use actix_http::http::StatusCode; + +macro_rules! assert_error { + ($code:literal, $type:literal, $status:path, $req:expr) => { + let (response, status_code) = $req; + assert_eq!(status_code, $status); + assert_eq!(response["errorCode"].as_str().unwrap(), $code); + assert_eq!(response["errorType"].as_str().unwrap(), $type); + }; +} + +#[actix_rt::test] +async fn index_already_exists_error() { + let mut server = common::Server::with_uid("test"); + let body = json!({ + "uid": "test" + }); + let (response, status_code) = server.create_index(body.clone()).await; + println!("{}", response); + assert_eq!(status_code, StatusCode::CREATED); + assert_error!( + "index_already_exists", + "invalid_request_error", + StatusCode::BAD_REQUEST, + server.create_index(body).await); +} + +#[actix_rt::test] +async fn index_not_found_error() { + let mut server = common::Server::with_uid("test"); + assert_error!( + "index_not_found", + "invalid_request_error", + StatusCode::NOT_FOUND, + server.get_index().await); +} + +#[actix_rt::test] +async fn primary_key_already_present_error() { + let mut server = common::Server::with_uid("test"); + let body = json!({ + "uid": "test", + "primaryKey": "test" + }); + server.create_index(body.clone()).await; + let body = json!({ + "primaryKey": "t" + }); + assert_error!( + "primary_key_already_present", + "invalid_request_error", + StatusCode::BAD_REQUEST, + server.update_index(body).await); +} + +#[actix_rt::test] +#[ignore] +async fn max_field_limit_exceeded_error() { + todo!("error reported in update") +} + +#[actix_rt::test] +async fn facet_error() { + let mut server = common::Server::test_server().await; + let search = json!({ + "q": "foo", + "facetFilters": ["test:hello"] + }); + assert_error!( + "invalid_facet", + "invalid_request_error", + StatusCode::BAD_REQUEST, + server.search_post(search).await); +} + +#[actix_rt::test] +async fn filters_error() { + let mut server = common::Server::test_server().await; + let search = json!({ + "q": "foo", + "filters": "fo:12" + }); + assert_error!( + "invalid_filter", + "invalid_request_error", + StatusCode::BAD_REQUEST, + server.search_post(search).await); +} + +#[actix_rt::test] +async fn bad_request_error() { + let mut server = common::Server::with_uid("test"); + let body = json!({ + "foo": "bar", + }); + assert_error!( + "bad_request", + "invalid_request_error", + StatusCode::BAD_REQUEST, + server.search_post(body).await); +} + +#[actix_rt::test] +async fn document_not_found_error() { + let mut server = common::Server::with_uid("test"); + server.create_index(json!({"uid": "test"})).await; + assert_error!( + "document_not_found", + "invalid_request_error", + StatusCode::NOT_FOUND, + server.get_document(100).await); +} + +#[actix_rt::test] +async fn payload_too_large_error() { + let mut server = common::Server::with_uid("test"); + let bigvec = vec![0u64; 10_000_000]; // 80mb + assert_error!( + "payload_too_large", + "invalid_request_error", + StatusCode::PAYLOAD_TOO_LARGE, + server.create_index(json!(bigvec)).await); +} From 3c51e9f5eded955ab1f77bdde5d12e0a857571b1 Mon Sep 17 00:00:00 2001 From: mpostma Date: Mon, 22 Jun 2020 18:37:04 +0200 Subject: [PATCH 4/7] Enable error code reporting for update errors --- meilisearch-core/src/update/mod.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/meilisearch-core/src/update/mod.rs b/meilisearch-core/src/update/mod.rs index d76649367..2443790c1 100644 --- a/meilisearch-core/src/update/mod.rs +++ b/meilisearch-core/src/update/mod.rs @@ -24,6 +24,8 @@ use sdset::Set; use serde::{Deserialize, Serialize}; use serde_json::Value; +use meilisearch_error::ErrorCode; + use crate::{store, MResult}; use crate::database::{MainT, UpdateT}; use crate::settings::SettingsUpdate; @@ -128,6 +130,12 @@ pub struct ProcessedUpdateResult { pub update_type: UpdateType, #[serde(skip_serializing_if = "Option::is_none")] pub error: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub error_type: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub error_code: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub error_link: Option, pub duration: f64, // in seconds pub enqueued_at: DateTime, pub processed_at: DateTime, @@ -288,7 +296,10 @@ pub fn update_task<'a, 'b>( let status = ProcessedUpdateResult { update_id, update_type, - error: result.map_err(|e| e.to_string()).err(), + error: result.as_ref().map_err(|e| e.to_string()).err(), + error_code: result.as_ref().map_err(|e| e.error_name()).err(), + error_type: result.as_ref().map_err(|e| e.error_type()).err(), + error_link: result.as_ref().map_err(|e| e.error_url()).err(), duration: duration.as_secs_f64(), enqueued_at, processed_at: Utc::now(), From 41707e32458223c3f5ff6e1f0c1c62e85ef3af3c Mon Sep 17 00:00:00 2001 From: mpostma Date: Mon, 22 Jun 2020 18:54:32 +0200 Subject: [PATCH 5/7] fix error on missing document id in document --- meilisearch-core/src/error.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/meilisearch-core/src/error.rs b/meilisearch-core/src/error.rs index bbb33526a..0540ca8d0 100644 --- a/meilisearch-core/src/error.rs +++ b/meilisearch-core/src/error.rs @@ -124,7 +124,10 @@ impl From for Error { impl From for Error { fn from(error: SerializerError) -> Error { - Error::Serializer(error) + match error { + SerializerError::DocumentIdNotFound => Error::MissingDocumentId, + e => Error::Serializer(e), + } } } From d7b49fa6714ffeca71127a711f819fa4285c6a94 Mon Sep 17 00:00:00 2001 From: mpostma Date: Mon, 22 Jun 2020 19:53:56 +0200 Subject: [PATCH 6/7] fix potential infinite loop --- meilisearch-http/tests/common.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/meilisearch-http/tests/common.rs b/meilisearch-http/tests/common.rs index 89724f379..b4e3b35f5 100644 --- a/meilisearch-http/tests/common.rs +++ b/meilisearch-http/tests/common.rs @@ -111,17 +111,19 @@ impl Server { pub async fn wait_update_id(&mut self, update_id: u64) { - loop { + // try 10 times to get status, or panic to not wait forever + for _ in 1..10 { let (response, status_code) = self.get_update_status(update_id).await; assert_eq!(status_code, 200); - if response["status"] == "processed" || response["status"] == "error" { + if response["status"] == "processed" || response["status"] == "failed" { eprintln!("{:#?}", response); return; } delay_for(Duration::from_secs(1)).await; } + panic!("Timeout waiting for update id"); } // Global Http request GET/POST/DELETE async or sync From e92f544fd1df42652f429a3bc0c0980f6147fc60 Mon Sep 17 00:00:00 2001 From: mpostma Date: Mon, 22 Jun 2020 20:51:14 +0200 Subject: [PATCH 7/7] add test for update errors --- meilisearch-http/tests/common.rs | 2 +- meilisearch-http/tests/errors.rs | 62 ++++++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/meilisearch-http/tests/common.rs b/meilisearch-http/tests/common.rs index b4e3b35f5..5985ce64f 100644 --- a/meilisearch-http/tests/common.rs +++ b/meilisearch-http/tests/common.rs @@ -112,7 +112,7 @@ impl Server { pub async fn wait_update_id(&mut self, update_id: u64) { // try 10 times to get status, or panic to not wait forever - for _ in 1..10 { + for _ in 0..10 { let (response, status_code) = self.get_update_status(update_id).await; assert_eq!(status_code, 200); diff --git a/meilisearch-http/tests/errors.rs b/meilisearch-http/tests/errors.rs index 5a7f2a708..ff0fb2c73 100644 --- a/meilisearch-http/tests/errors.rs +++ b/meilisearch-http/tests/errors.rs @@ -1,7 +1,10 @@ mod common; -use serde_json::json; +use std::thread; +use std::time::Duration; + use actix_http::http::StatusCode; +use serde_json::{json, Map, Value}; macro_rules! assert_error { ($code:literal, $type:literal, $status:path, $req:expr) => { @@ -12,6 +15,25 @@ macro_rules! assert_error { }; } +macro_rules! assert_error_async { + ($code:literal, $type:literal, $server:expr, $req:expr) => { + let (response, _) = $req; + let update_id = response["updateId"].as_u64().unwrap(); + for _ in 1..10 { + let (response, status_code) = $server.get_update_status(update_id).await; + assert_eq!(status_code, StatusCode::OK); + if response["status"] == "processed" || response["status"] == "failed" { + println!("response: {}", response); + assert_eq!(response["status"], "failed"); + assert_eq!(response["errorCode"], $code); + assert_eq!(response["errorType"], $type); + return + } + thread::sleep(Duration::from_secs(1)); + } + }; +} + #[actix_rt::test] async fn index_already_exists_error() { let mut server = common::Server::with_uid("test"); @@ -57,9 +79,43 @@ async fn primary_key_already_present_error() { } #[actix_rt::test] -#[ignore] async fn max_field_limit_exceeded_error() { - todo!("error reported in update") + let mut server = common::Server::test_server().await; + let body = json!({ + "uid": "test", + }); + server.create_index(body).await; + let mut doc = Map::with_capacity(70_000); + doc.insert("id".into(), Value::String("foo".into())); + for i in 0..69_999 { + doc.insert(format!("field{}", i), Value::String("foo".into())); + } + let docs = json!([doc]); + assert_error_async!( + "max_field_limit_exceeded", + "invalid_request_error", + server, + server.add_or_replace_multiple_documents_sync(docs).await); +} + +#[actix_rt::test] +async fn missing_document_id() { + let mut server = common::Server::test_server().await; + let body = json!({ + "uid": "test", + "primaryKey": "test" + }); + server.create_index(body).await; + let docs = json!([ + { + "foo": "bar", + } + ]); + assert_error_async!( + "missing_document_id", + "invalid_request_error", + server, + server.add_or_replace_multiple_documents_sync(docs).await); } #[actix_rt::test]