From c25641ff2d5cda4f8b015a1033e6e92275f7765b Mon Sep 17 00:00:00 2001 From: qdequele Date: Wed, 11 Mar 2020 12:00:40 +0100 Subject: [PATCH 1/4] fix that AcceptNewFields does not take into account the primary-key; fix #518 --- .../tests/settings_accept_new_fields.rs | 59 +++++++++++++++++++ meilisearch-schema/src/schema.rs | 6 +- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/meilisearch-http/tests/settings_accept_new_fields.rs b/meilisearch-http/tests/settings_accept_new_fields.rs index 5d11b3fe9..807aa1cd2 100644 --- a/meilisearch-http/tests/settings_accept_new_fields.rs +++ b/meilisearch-http/tests/settings_accept_new_fields.rs @@ -288,3 +288,62 @@ fn index_new_fields_false_then_true() { assert_eq!(status_code, 200); assert_json_eq!(response, expected); } + + +// Fix issue https://github.com/meilisearch/MeiliSearch/issues/518 +#[test] +fn accept_new_fields_does_not_take_into_account_the_primary_key () { + let mut server = common::Server::with_uid("movies"); + + // 1 - Create an index with no primary-key + + let body = json!({ + "uid": "movies", + }); + let (response, status_code) = server.create_index(body); + assert_eq!(status_code, 201); + assert_eq!(response["primaryKey"], json!(null)); + + // 2 - Add searchable and displayed attributes as: ["title"] & Set acceptNewFields to false + + let body = json!({ + "searchableAttributes": ["title"], + "displayedAttributes": ["title"], + "acceptNewFields": false, + }); + + server.update_all_settings(body); + + // 4 - Add a document + + let body = json!([{ + "id": 1, + "title": "Test", + "comment": "comment test" + }]); + + server.add_or_replace_multiple_documents(body); + + // 5 - Get settings, they should not changed + + let (response, _status_code) = server.get_all_settings(); + + let expected = json!({ + "rankingRules": [ + "typo", + "words", + "proximity", + "attribute", + "wordsPosition", + "exactness", + ], + "distinctAttribute": null, + "searchableAttributes": ["title"], + "displayedAttributes": ["title"], + "stopWords": [], + "synonyms": {}, + "acceptNewFields": false, + }); + + assert_json_eq!(response, expected, ordered: false); +} diff --git a/meilisearch-schema/src/schema.rs b/meilisearch-schema/src/schema.rs index b1b8bc66b..64f622501 100644 --- a/meilisearch-schema/src/schema.rs +++ b/meilisearch-schema/src/schema.rs @@ -63,8 +63,10 @@ impl Schema { let id = self.insert(name)?; self.primary_key = Some(id); - self.set_indexed(name)?; - self.set_displayed(name)?; + if self.accept_new_fields { + self.set_indexed(name)?; + self.set_displayed(name)?; + } Ok(id) } From 4ccf1d10bd05fcce7821fec16cae4d2ac086403a Mon Sep 17 00:00:00 2001 From: qdequele Date: Wed, 11 Mar 2020 12:27:42 +0100 Subject: [PATCH 2/4] error message when impossible to infer the primary-key; fix #517 --- meilisearch-http/src/routes/document.rs | 2 +- meilisearch-http/tests/common.rs | 5 ++++ meilisearch-http/tests/index.rs | 33 +++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/meilisearch-http/src/routes/document.rs b/meilisearch-http/src/routes/document.rs index a3e59ca05..3669e3899 100644 --- a/meilisearch-http/src/routes/document.rs +++ b/meilisearch-http/src/routes/document.rs @@ -145,7 +145,7 @@ async fn update_multiple_documents(mut ctx: Request, is_partial: bool) -> Some(id) => id, None => match data.first().and_then(|docs| find_primary_key(docs)) { Some(id) => id, - None => return Err(ResponseError::bad_request("Could not infer a schema")), + None => return Err(ResponseError::bad_request("Could not infer a primary key")), }, }; diff --git a/meilisearch-http/tests/common.rs b/meilisearch-http/tests/common.rs index eee0fc283..3814db6de 100644 --- a/meilisearch-http/tests/common.rs +++ b/meilisearch-http/tests/common.rs @@ -205,6 +205,11 @@ impl Server { self.post_request_async(&url, body); } + pub fn add_or_replace_multiple_documents_sync(&mut self, body: Value) -> (Value, StatusCode) { + let url = format!("/indexes/{}/documents", self.uid); + self.post_request(&url, body) + } + pub fn add_or_update_multiple_documents(&mut self, body: Value) { let url = format!("/indexes/{}/documents", self.uid); self.put_request_async(&url, body); diff --git a/meilisearch-http/tests/index.rs b/meilisearch-http/tests/index.rs index 849cc425d..a16c10575 100644 --- a/meilisearch-http/tests/index.rs +++ b/meilisearch-http/tests/index.rs @@ -625,3 +625,36 @@ fn create_index_without_primary_key_and_search() { assert_eq!(status_code, 200); assert_eq!(response["hits"].as_array().unwrap().len(), 0); } + +// Test the error message when we push an document update and impossibility to find primary key +// Test issue https://github.com/meilisearch/MeiliSearch/issues/517 +#[test] +fn check_add_documents_without_primary_key() { + let mut server = common::Server::with_uid("movies"); + + // 1 - Create the index with no primary_key + + let body = json!({ + "uid": "movies", + }); + let (response, status_code) = server.create_index(body); + assert_eq!(status_code, 201); + assert_eq!(response["primaryKey"], json!(null)); + + // 2- Add document + + let body = json!([{ + "title": "Test", + "comment": "comment test" + }]); + + let (response, status_code) = server.add_or_replace_multiple_documents_sync(body); + + let expected = json!({ + "message": "Could not infer a primary key" + }); + + assert_eq!(status_code, 400); + assert_json_eq!(response, expected, ordered: false); +} + From ce0e8415d5c68fef1026b12d06d984c922e9bfc3 Mon Sep 17 00:00:00 2001 From: qdequele Date: Wed, 11 Mar 2020 14:12:38 +0100 Subject: [PATCH 3/4] adding primary-key when adding documents does not work; fix #519 --- meilisearch-http/src/routes/document.rs | 6 ++-- meilisearch-http/tests/common.rs | 33 +++++++++------------ meilisearch-http/tests/documents_add.rs | 38 +++++++++++++++++++++++++ meilisearch-http/tests/index.rs | 1 - 4 files changed, 54 insertions(+), 24 deletions(-) create mode 100644 meilisearch-http/tests/documents_add.rs diff --git a/meilisearch-http/src/routes/document.rs b/meilisearch-http/src/routes/document.rs index 3669e3899..4babe530f 100644 --- a/meilisearch-http/src/routes/document.rs +++ b/meilisearch-http/src/routes/document.rs @@ -118,9 +118,9 @@ fn find_primary_key(document: &IndexMap) -> Option { } #[derive(Default, Deserialize)] -#[serde(deny_unknown_fields)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] struct UpdateDocumentsQuery { - document_id: Option, + primary_key: Option, } async fn update_multiple_documents(mut ctx: Request, is_partial: bool) -> SResult { @@ -141,7 +141,7 @@ async fn update_multiple_documents(mut ctx: Request, is_partial: bool) -> .ok_or(ResponseError::internal("schema not found"))?; if schema.primary_key().is_none() { - let id = match query.document_id { + let id = match query.primary_key { Some(id) => id, None => match data.first().and_then(|docs| find_primary_key(docs)) { Some(id) => id, diff --git a/meilisearch-http/tests/common.rs b/meilisearch-http/tests/common.rs index 3814db6de..42a2ce556 100644 --- a/meilisearch-http/tests/common.rs +++ b/meilisearch-http/tests/common.rs @@ -44,30 +44,23 @@ impl Server { } } - fn wait_update_id(&mut self, update_id: u64) { + pub fn wait_update_id(&mut self, update_id: u64) { loop { - let req = http::Request::get(format!("/indexes/{}/updates/{}", self.uid, update_id)) - .body(Body::empty()) - .unwrap(); - - let res = self.mock.simulate(req).unwrap(); - assert_eq!(res.status(), 200); - - let mut buf = Vec::new(); - block_on(res.into_body().read_to_end(&mut buf)).unwrap(); - let response: Value = serde_json::from_slice(&buf).unwrap(); + let (response, status_code) = self.get_update_status(update_id); + assert_eq!(status_code, 200); if response["status"] == "processed" || response["status"] == "error" { eprintln!("{:#?}", response); return; } + block_on(sleep(Duration::from_secs(1))); } } - // // Global Http request GET/POST/DELETE async or sync + // Global Http request GET/POST/DELETE async or sync - fn get_request(&mut self, url: &str) -> (Value, StatusCode) { + pub fn get_request(&mut self, url: &str) -> (Value, StatusCode) { eprintln!("get_request: {}", url); let req = http::Request::get(url).body(Body::empty()).unwrap(); let res = self.mock.simulate(req).unwrap(); @@ -79,7 +72,7 @@ impl Server { (response, status_code) } - fn post_request(&mut self, url: &str, body: Value) -> (Value, StatusCode) { + pub fn post_request(&mut self, url: &str, body: Value) -> (Value, StatusCode) { eprintln!("post_request: {}", url); let body_bytes = body.to_string().into_bytes(); @@ -95,7 +88,7 @@ impl Server { (response, status_code) } - fn post_request_async(&mut self, url: &str, body: Value) -> (Value, StatusCode) { + pub fn post_request_async(&mut self, url: &str, body: Value) -> (Value, StatusCode) { eprintln!("post_request_async: {}", url); let (response, status_code) = self.post_request(url, body); assert_eq!(status_code, 202); @@ -104,7 +97,7 @@ impl Server { (response, status_code) } - fn put_request(&mut self, url: &str, body: Value) -> (Value, StatusCode) { + pub fn put_request(&mut self, url: &str, body: Value) -> (Value, StatusCode) { eprintln!("put_request: {}", url); let body_bytes = body.to_string().into_bytes(); @@ -120,7 +113,7 @@ impl Server { (response, status_code) } - fn put_request_async(&mut self, url: &str, body: Value) -> (Value, StatusCode) { + pub fn put_request_async(&mut self, url: &str, body: Value) -> (Value, StatusCode) { eprintln!("put_request_async: {}", url); let (response, status_code) = self.put_request(url, body); assert!(response["updateId"].as_u64().is_some()); @@ -129,7 +122,7 @@ impl Server { (response, status_code) } - fn delete_request(&mut self, url: &str) -> (Value, StatusCode) { + pub fn delete_request(&mut self, url: &str) -> (Value, StatusCode) { eprintln!("delete_request: {}", url); let req = http::Request::delete(url).body(Body::empty()).unwrap(); let res = self.mock.simulate(req).unwrap(); @@ -141,7 +134,7 @@ impl Server { (response, status_code) } - fn delete_request_async(&mut self, url: &str) -> (Value, StatusCode) { + pub fn delete_request_async(&mut self, url: &str) -> (Value, StatusCode) { eprintln!("delete_request_async: {}", url); let (response, status_code) = self.delete_request(url); assert!(response["updateId"].as_u64().is_some()); @@ -150,7 +143,7 @@ impl Server { (response, status_code) } - // // All Routes + // All Routes pub fn list_indexes(&mut self) -> (Value, StatusCode) { self.get_request("/indexes") diff --git a/meilisearch-http/tests/documents_add.rs b/meilisearch-http/tests/documents_add.rs new file mode 100644 index 000000000..7ba726810 --- /dev/null +++ b/meilisearch-http/tests/documents_add.rs @@ -0,0 +1,38 @@ +use serde_json::json; + +mod common; + +// Test issue https://github.com/meilisearch/MeiliSearch/issues/519 +#[test] +fn check_add_documents_with_primary_key_param() { + let mut server = common::Server::with_uid("movies"); + + // 1 - Create the index with no primary_key + + let body = json!({ + "uid": "movies", + }); + let (response, status_code) = server.create_index(body); + assert_eq!(status_code, 201); + assert_eq!(response["primaryKey"], json!(null)); + + // 2 - Add documents + + let body = json!([{ + "title": "Test", + "comment": "comment test" + }]); + + let url = "/indexes/movies/documents?primaryKey=title"; + let (response, status_code) = server.post_request(&url, body); + eprintln!("{:#?}", response); + assert_eq!(status_code, 202); + let update_id = response["updateId"].as_u64().unwrap(); + server.wait_update_id(update_id); + + // 3 - Check update sucess + + let (response, status_code) = server.get_update_status(update_id); + assert_eq!(status_code, 200); + assert_eq!(response["status"], "processed"); +} diff --git a/meilisearch-http/tests/index.rs b/meilisearch-http/tests/index.rs index a16c10575..ab10cf35b 100644 --- a/meilisearch-http/tests/index.rs +++ b/meilisearch-http/tests/index.rs @@ -657,4 +657,3 @@ fn check_add_documents_without_primary_key() { assert_eq!(status_code, 400); assert_json_eq!(response, expected, ordered: false); } - From 7be376721c0950205fe8bd45a81fd042272fe226 Mon Sep 17 00:00:00 2001 From: qdequele Date: Wed, 11 Mar 2020 14:42:58 +0100 Subject: [PATCH 4/4] global settings update make partial update; fix #516 --- meilisearch-http/src/routes/setting.rs | 26 +----- meilisearch-http/tests/search.rs | 7 -- meilisearch-http/tests/settings.rs | 112 ++++++++++++++++++++++++- 3 files changed, 111 insertions(+), 34 deletions(-) diff --git a/meilisearch-http/src/routes/setting.rs b/meilisearch-http/src/routes/setting.rs index 703607c32..9cdbcb7a3 100644 --- a/meilisearch-http/src/routes/setting.rs +++ b/meilisearch-http/src/routes/setting.rs @@ -1,5 +1,4 @@ use meilisearch_core::settings::{Settings, SettingsUpdate, UpdateState, DEFAULT_RANKING_RULES}; -use serde::Deserialize; use std::collections::{BTreeMap, BTreeSet, HashSet}; use tide::{Request, Response}; @@ -73,36 +72,13 @@ pub async fn get_all(ctx: Request) -> SResult { Ok(tide::Response::new(200).body_json(&settings).unwrap()) } -#[derive(Default, Clone, Deserialize)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct UpdateSettings { - pub ranking_rules: Option>, - pub distinct_attribute: Option, - pub primary_key: Option, - pub searchable_attributes: Option>, - pub displayed_attributes: Option>, - pub stop_words: Option>, - pub synonyms: Option>>, - pub accept_new_fields: Option, -} - pub async fn update_all(mut ctx: Request) -> SResult { ctx.is_allowed(Private)?; let index = ctx.index()?; - let settings_update: UpdateSettings = + let settings: Settings = ctx.body_json().await.map_err(ResponseError::bad_request)?; let db = &ctx.state().db; - let settings = Settings { - ranking_rules: Some(settings_update.ranking_rules), - distinct_attribute: Some(settings_update.distinct_attribute), - searchable_attributes: Some(settings_update.searchable_attributes), - displayed_attributes: Some(settings_update.displayed_attributes), - stop_words: Some(settings_update.stop_words), - synonyms: Some(settings_update.synonyms), - accept_new_fields: Some(settings_update.accept_new_fields), - }; - let mut writer = db.update_write_txn()?; let settings = settings.into_update().map_err(ResponseError::bad_request)?; let update_id = index.settings_update(&mut writer, settings)?; diff --git a/meilisearch-http/tests/search.rs b/meilisearch-http/tests/search.rs index a53aecf89..d044d3531 100644 --- a/meilisearch-http/tests/search.rs +++ b/meilisearch-http/tests/search.rs @@ -644,7 +644,6 @@ fn search_with_settings_basic() { "desc(vote_average)" ], "distinctAttribute": null, - "primaryKey": "id", "searchableAttributes": [ "title", "tagline", @@ -751,7 +750,6 @@ fn search_with_settings_stop_words() { "desc(vote_average)" ], "distinctAttribute": null, - "primaryKey": "id", "searchableAttributes": [ "title", "tagline", @@ -858,7 +856,6 @@ fn search_with_settings_synonyms() { "desc(vote_average)" ], "distinctAttribute": null, - "primaryKey": "id", "searchableAttributes": [ "title", "tagline", @@ -970,7 +967,6 @@ fn search_with_settings_ranking_rules() { "desc(popularity)" ], "distinctAttribute": null, - "primaryKey": "id", "searchableAttributes": [ "title", "tagline", @@ -1077,7 +1073,6 @@ fn search_with_settings_searchable_attributes() { "desc(vote_average)" ], "distinctAttribute": null, - "primaryKey": "id", "searchableAttributes": [ "tagline", "overview", @@ -1183,7 +1178,6 @@ fn search_with_settings_displayed_attributes() { "desc(vote_average)" ], "distinctAttribute": null, - "primaryKey": "id", "searchableAttributes": [ "title", "tagline", @@ -1254,7 +1248,6 @@ fn search_with_settings_searchable_attributes_2() { "desc(vote_average)" ], "distinctAttribute": null, - "primaryKey": "id", "searchableAttributes": [ "tagline", "overview", diff --git a/meilisearch-http/tests/settings.rs b/meilisearch-http/tests/settings.rs index c692e4104..7e2b1275d 100644 --- a/meilisearch-http/tests/settings.rs +++ b/meilisearch-http/tests/settings.rs @@ -192,6 +192,7 @@ fn write_all_and_update() { "exactness", "desc(release_date)", ], + "distinctAttribute": null, "searchableAttributes": [ "title", "description", @@ -204,8 +205,7 @@ fn write_all_and_update() { "rank", "poster", ], - "stopWords": [ - ], + "stopWords": [], "synonyms": { "wolverine": ["xmen", "logan"], "logan": ["wolverine", "xmen"], @@ -321,3 +321,111 @@ fn test_default_settings_2() { assert_json_eq!(body, response, ordered: false); } + +// Test issue https://github.com/meilisearch/MeiliSearch/issues/516 +#[test] +fn write_setting_and_update_partial() { + let mut server = common::Server::with_uid("movies"); + let body = json!({ + "uid": "movies", + }); + server.create_index(body); + + // 2 - Send the settings + + let body = json!({ + "searchableAttributes": [ + "uid", + "movie_id", + "title", + "description", + "poster", + "release_date", + "rank", + ], + "displayedAttributes": [ + "title", + "description", + "poster", + "release_date", + "rank", + ] + }); + + server.update_all_settings(body.clone()); + + // 2 - Send the settings + + let body = json!({ + "rankingRules": [ + "typo", + "words", + "proximity", + "attribute", + "wordsPosition", + "exactness", + "desc(release_date)", + "desc(rank)", + ], + "distinctAttribute": "movie_id", + "stopWords": [ + "the", + "a", + "an", + ], + "synonyms": { + "wolverine": ["xmen", "logan"], + "logan": ["wolverine"], + }, + "acceptNewFields": false, + }); + + server.update_all_settings(body.clone()); + + // 2 - Send the settings + + let expected = json!({ + "rankingRules": [ + "typo", + "words", + "proximity", + "attribute", + "wordsPosition", + "exactness", + "desc(release_date)", + "desc(rank)", + ], + "distinctAttribute": "movie_id", + "searchableAttributes": [ + "uid", + "movie_id", + "title", + "description", + "poster", + "release_date", + "rank", + ], + "displayedAttributes": [ + "title", + "description", + "poster", + "release_date", + "rank", + ], + "stopWords": [ + "the", + "a", + "an", + ], + "synonyms": { + "wolverine": ["xmen", "logan"], + "logan": ["wolverine"], + }, + "acceptNewFields": false, + }); + + let (response, _status_code) = server.get_all_settings(); + + assert_json_eq!(expected, response, ordered: false); + +}