From ac7226bb27d717ea72cdd54605723d762a8c9999 Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 13 Oct 2020 13:08:17 +0200 Subject: [PATCH 1/3] fix deserializer --- meilisearch-core/src/serde/deserializer.rs | 3 ++- meilisearch-core/src/store/mod.rs | 1 + meilisearch-core/src/update/documents_addition.rs | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/meilisearch-core/src/serde/deserializer.rs b/meilisearch-core/src/serde/deserializer.rs index e5e02a4d6..d4674016b 100644 --- a/meilisearch-core/src/serde/deserializer.rs +++ b/meilisearch-core/src/serde/deserializer.rs @@ -55,6 +55,7 @@ pub struct Deserializer<'a> { pub documents_fields: DocumentsFields, pub schema: &'a Schema, pub fields: Option<&'a HashSet>, + pub displayed: bool, } impl<'de, 'a, 'b> de::Deserializer<'de> for &'b mut Deserializer<'a> { @@ -93,7 +94,7 @@ impl<'de, 'a, 'b> de::Deserializer<'de> for &'b mut Deserializer<'a> { }; let is_displayed = self.schema.is_displayed(attr); - if is_displayed && self.fields.map_or(true, |f| f.contains(&attr)) { + if !self.displayed || (is_displayed && self.fields.map_or(true, |f| f.contains(&attr))) { if let Some(attribute_name) = self.schema.name(attr) { let cursor = Cursor::new(value.to_owned()); let ioread = SerdeJsonIoRead::new(cursor); diff --git a/meilisearch-core/src/store/mod.rs b/meilisearch-core/src/store/mod.rs index fa5baa831..4f8e1250a 100644 --- a/meilisearch-core/src/store/mod.rs +++ b/meilisearch-core/src/store/mod.rs @@ -243,6 +243,7 @@ impl Index { documents_fields: self.documents_fields, schema: &schema, fields: attributes.as_ref(), + displayed: true, }; Ok(Option::::deserialize(&mut deserializer)?) diff --git a/meilisearch-core/src/update/documents_addition.rs b/meilisearch-core/src/update/documents_addition.rs index 5603966b0..e6b9db90e 100644 --- a/meilisearch-core/src/update/documents_addition.rs +++ b/meilisearch-core/src/update/documents_addition.rs @@ -197,6 +197,7 @@ pub fn apply_addition<'a, 'b>( documents_fields: index.documents_fields, schema: &schema, fields: None, + displayed: false, }; let old_document = Option::>::deserialize(&mut deserializer)?; From 1639a7338d6b863958b965c16a9a5483bc835da4 Mon Sep 17 00:00:00 2001 From: many Date: Mon, 31 Aug 2020 18:15:07 +0200 Subject: [PATCH 2/3] add test to reproduce #891 bug report fix bug --- .../src/update/documents_addition.rs | 2 + meilisearch-core/src/update/helpers.rs | 1 + .../src/update/settings_update.rs | 2 + meilisearch-http/tests/search.rs | 6 +- meilisearch-http/tests/settings.rs | 72 ++++++++++++++++++- meilisearch-schema/src/schema.rs | 53 +++++++++----- 6 files changed, 115 insertions(+), 21 deletions(-) diff --git a/meilisearch-core/src/update/documents_addition.rs b/meilisearch-core/src/update/documents_addition.rs index e6b9db90e..7e7d069d3 100644 --- a/meilisearch-core/src/update/documents_addition.rs +++ b/meilisearch-core/src/update/documents_addition.rs @@ -201,12 +201,14 @@ pub fn apply_addition<'a, 'b>( }; let old_document = Option::>::deserialize(&mut deserializer)?; + println!("old document: {:#?}", old_document); if let Some(old_document) = old_document { for (key, value) in old_document { document.entry(key).or_insert(value); } } } + println!("new document: {:#?}", document); documents_additions.insert(internal_docid, document); } diff --git a/meilisearch-core/src/update/helpers.rs b/meilisearch-core/src/update/helpers.rs index 1aad1f505..77e2d5705 100644 --- a/meilisearch-core/src/update/helpers.rs +++ b/meilisearch-core/src/update/helpers.rs @@ -20,6 +20,7 @@ pub fn index_value( ) -> Option where A: AsRef<[u8]>, { + println!("indexing value: {}", value); match value { Value::Null => None, Value::Bool(boolean) => { diff --git a/meilisearch-core/src/update/settings_update.rs b/meilisearch-core/src/update/settings_update.rs index 94e337265..7d5bb1d3e 100644 --- a/meilisearch-core/src/update/settings_update.rs +++ b/meilisearch-core/src/update/settings_update.rs @@ -70,9 +70,11 @@ pub fn apply_settings_update( match settings.searchable_attributes.clone() { UpdateState::Update(v) => { + println!("updating indexed attributes"); if v.iter().any(|e| e == "*") || v.is_empty() { schema.set_all_fields_as_indexed(); } else { + println!("updating indexed"); schema.update_indexed(v)?; } must_reindex = true; diff --git a/meilisearch-http/tests/search.rs b/meilisearch-http/tests/search.rs index 170d9ffaf..6eb46e7f6 100644 --- a/meilisearch-http/tests/search.rs +++ b/meilisearch-http/tests/search.rs @@ -1779,8 +1779,8 @@ async fn update_documents_with_facet_distribution() { server.create_index(body).await; let settings = json!({ "attributesForFaceting": ["genre"], - "displayedAttributes": ["genre"], - "searchableAttributes": ["genre"] + "displayedAttributes": ["genre", "type"], + "searchableAttributes": ["genre", "type"] }); server.update_all_settings(settings).await; let update1 = json!([ @@ -1809,6 +1809,7 @@ async fn update_documents_with_facet_distribution() { "facetsDistribution": ["genre"] }); let (response1, _) = server.search_post(search.clone()).await; + println!("response1: {}", response1); let expected_facet_distribution = json!({ "genre": { "grunge": 1, @@ -1827,5 +1828,6 @@ async fn update_documents_with_facet_distribution() { ]); server.add_or_update_multiple_documents(update2).await; let (response2, _) = server.search_post(search).await; + println!("response2: {}", response2); assert_json_eq!(expected_facet_distribution, response2["facetsDistribution"].clone()); } diff --git a/meilisearch-http/tests/settings.rs b/meilisearch-http/tests/settings.rs index 6b125c13a..02e35b32d 100644 --- a/meilisearch-http/tests/settings.rs +++ b/meilisearch-http/tests/settings.rs @@ -1,5 +1,5 @@ use assert_json_diff::assert_json_eq; -use serde_json::json; +use serde_json::{json, Value}; use std::convert::Into; mod common; @@ -520,4 +520,72 @@ async fn test_displayed_attributes_field() { let (response, _status_code) = server.get_all_settings().await; assert_json_eq!(body, response, ordered: true); -} \ No newline at end of file +} + +#[actix_rt::test] +async fn push_documents_after_updating_settings_should_not_change_settings() { + let mut server = common::Server::with_uid("test"); + + let index_body = json!({ + "uid": "test", + "primaryKey": "id", + }); + + let settings_body = json!({ + "rankingRules": [ + "typo", + "words", + "proximity", + "attribute", + "wordsPosition", + "exactness", + "desc(registered)", + "desc(age)", + ], + "distinctAttribute": "id", + "searchableAttributes": [ + "id", + "name", + "color", + "gender", + "email", + "phone", + "address", + "registered", + "about" + ], + "displayedAttributes": [ + "name", + "gender", + "email", + "registered", + "age", + ], + "stopWords": [ + "ad", + "in", + "ut", + ], + "synonyms": { + "road": ["street", "avenue"], + "street": ["avenue"], + }, + "attributesForFaceting": ["name", "color"], + }); + + let dataset = include_bytes!("assets/test_set.json"); + + let documents_body: Value = serde_json::from_slice(dataset).unwrap(); + + server.create_index(index_body).await; + + server.update_all_settings(settings_body.clone()).await; + + server.add_or_replace_multiple_documents(documents_body).await; + + // === check if settings are the same ==== + + let (response, _status_code) = server.get_all_settings().await; + + assert_json_eq!(settings_body, response, ordered: false); +} diff --git a/meilisearch-schema/src/schema.rs b/meilisearch-schema/src/schema.rs index a1992080a..d35ae4f76 100644 --- a/meilisearch-schema/src/schema.rs +++ b/meilisearch-schema/src/schema.rs @@ -43,7 +43,7 @@ pub struct Schema { ranked: HashSet, displayed: OptionAll>, - indexed: OptionAll>, + indexed: OptionAll>, indexed_map: HashMap, } @@ -115,8 +115,10 @@ impl Schema { Ok(id) } None => { - self.set_indexed(name)?; - self.set_displayed(name) + let id = self.fields_map.insert(name)?; + let pos = self.indexed_map.len() as u16; + self.indexed_map.insert(id, pos.into()); + Ok(id) } } } @@ -156,18 +158,18 @@ impl Schema { } } - pub fn indexed(&self) -> Cow<[FieldId]> { + pub fn indexed(&self) -> Vec { match self.indexed { - OptionAll::Some(ref v) => Cow::Borrowed(v), + OptionAll::Some(ref v) => v.iter().cloned().collect(), OptionAll::All => { let fields = self - .fields_map + .indexed_map .iter() - .map(|(_, &f)| f) + .map(|(&f, _)| f) .collect(); - Cow::Owned(fields) + fields }, - OptionAll::None => Cow::Owned(Vec::new()) + OptionAll::None => Vec::new() } } @@ -200,16 +202,18 @@ impl Schema { pub fn set_indexed(&mut self, name: &str) -> SResult<(FieldId, IndexedPos)> { let id = self.fields_map.insert(name)?; + println!("setting {} indexed", name); + + self.indexed = self.indexed.take().map(|mut v| { + v.insert(id); + v + }); if let Some(indexed_pos) = self.indexed_map.get(&id) { return Ok((id, *indexed_pos)) }; let pos = self.indexed_map.len() as u16; self.indexed_map.insert(id, pos.into()); - self.indexed = self.indexed.take().map(|mut v| { - v.push(id); - v - }); Ok((id, pos.into())) } @@ -257,7 +261,13 @@ impl Schema { self.indexed = match self.indexed.take() { // valid because indexed is All and indexed() return the content of // indexed_map that is already updated - OptionAll::All => OptionAll::Some(self.indexed().into_owned()), + OptionAll::All => { + let indexed = self.indexed_map + .iter() + .filter_map(|(&i, _)| if i != id { Some(i) } else { None }) + .collect(); + OptionAll::Some(indexed) + }, OptionAll::Some(mut v) => { v.retain(|x| *x != id); OptionAll::Some(v) @@ -280,7 +290,17 @@ impl Schema { } pub fn is_indexed(&self, id: FieldId) -> Option<&IndexedPos> { - self.indexed_map.get(&id) + match self.indexed { + OptionAll::All => self.indexed_map.get(&id), + OptionAll::Some(ref v) => { + if v.contains(&id) { + self.indexed_map.get(&id) + } else { + None + } + } + OptionAll::None => None + } } pub fn is_indexed_all(&self) -> bool { @@ -324,9 +344,8 @@ impl Schema { v.clear(); OptionAll::Some(v) }, - _ => OptionAll::Some(Vec::new()), + _ => OptionAll::Some(HashSet::new()), }; - self.indexed_map.clear(); for name in data { self.set_indexed(name.as_ref())?; } From dc2e5ceed2672aeb1be52bd2cb21ac0950071d03 Mon Sep 17 00:00:00 2001 From: mpostma Date: Tue, 13 Oct 2020 13:50:42 +0200 Subject: [PATCH 3/3] fix bug --- meilisearch-core/src/facets.rs | 4 +- meilisearch-core/src/serde/deserializer.rs | 2 + .../src/update/documents_addition.rs | 4 +- meilisearch-core/src/update/helpers.rs | 1 - .../src/update/settings_update.rs | 2 - meilisearch-http/tests/search.rs | 4 +- meilisearch-http/tests/settings.rs | 10 +- meilisearch-schema/src/schema.rs | 98 ++++--------------- 8 files changed, 26 insertions(+), 99 deletions(-) diff --git a/meilisearch-core/src/facets.rs b/meilisearch-core/src/facets.rs index 11135c179..6f5c2e837 100644 --- a/meilisearch-core/src/facets.rs +++ b/meilisearch-core/src/facets.rs @@ -246,7 +246,7 @@ mod test { #[test] fn test_facet_key() { let mut schema = Schema::new(); - let id = schema.insert_and_index("hello").unwrap(); + let id = schema.register_field("hello").unwrap(); let facet_list = [schema.id("hello").unwrap()]; assert_eq!( FacetKey::from_str("hello:12", &schema, &facet_list).unwrap(), @@ -287,7 +287,7 @@ mod test { fn test_parse_facet_array() { use either::Either::{Left, Right}; let mut schema = Schema::new(); - let _id = schema.insert_and_index("hello").unwrap(); + let _id = schema.register_field("hello").unwrap(); let facet_list = [schema.id("hello").unwrap()]; assert_eq!( FacetFilter::from_str("[[\"hello:12\"]]", &schema, &facet_list).unwrap(), diff --git a/meilisearch-core/src/serde/deserializer.rs b/meilisearch-core/src/serde/deserializer.rs index d4674016b..2ef266565 100644 --- a/meilisearch-core/src/serde/deserializer.rs +++ b/meilisearch-core/src/serde/deserializer.rs @@ -94,6 +94,8 @@ impl<'de, 'a, 'b> de::Deserializer<'de> for &'b mut Deserializer<'a> { }; let is_displayed = self.schema.is_displayed(attr); + // Check if displayed fields were requested, if yes, return only displayed fields, + // else return all fields if !self.displayed || (is_displayed && self.fields.map_or(true, |f| f.contains(&attr))) { if let Some(attribute_name) = self.schema.name(attr) { let cursor = Cursor::new(value.to_owned()); diff --git a/meilisearch-core/src/update/documents_addition.rs b/meilisearch-core/src/update/documents_addition.rs index 7e7d069d3..2dc68c3ba 100644 --- a/meilisearch-core/src/update/documents_addition.rs +++ b/meilisearch-core/src/update/documents_addition.rs @@ -201,14 +201,12 @@ pub fn apply_addition<'a, 'b>( }; let old_document = Option::>::deserialize(&mut deserializer)?; - println!("old document: {:#?}", old_document); if let Some(old_document) = old_document { for (key, value) in old_document { document.entry(key).or_insert(value); } } } - println!("new document: {:#?}", document); documents_additions.insert(internal_docid, document); } @@ -231,7 +229,7 @@ pub fn apply_addition<'a, 'b>( for (document_id, document) in &documents_additions { // For each key-value pair in the document. for (attribute, value) in document { - let field_id = schema.insert_and_index(&attribute)?; + let field_id = schema.register_field(&attribute)?; index_document( writer, index.documents_fields, diff --git a/meilisearch-core/src/update/helpers.rs b/meilisearch-core/src/update/helpers.rs index 77e2d5705..1aad1f505 100644 --- a/meilisearch-core/src/update/helpers.rs +++ b/meilisearch-core/src/update/helpers.rs @@ -20,7 +20,6 @@ pub fn index_value( ) -> Option where A: AsRef<[u8]>, { - println!("indexing value: {}", value); match value { Value::Null => None, Value::Bool(boolean) => { diff --git a/meilisearch-core/src/update/settings_update.rs b/meilisearch-core/src/update/settings_update.rs index 7d5bb1d3e..94e337265 100644 --- a/meilisearch-core/src/update/settings_update.rs +++ b/meilisearch-core/src/update/settings_update.rs @@ -70,11 +70,9 @@ pub fn apply_settings_update( match settings.searchable_attributes.clone() { UpdateState::Update(v) => { - println!("updating indexed attributes"); if v.iter().any(|e| e == "*") || v.is_empty() { schema.set_all_fields_as_indexed(); } else { - println!("updating indexed"); schema.update_indexed(v)?; } must_reindex = true; diff --git a/meilisearch-http/tests/search.rs b/meilisearch-http/tests/search.rs index 6eb46e7f6..92879bb7b 100644 --- a/meilisearch-http/tests/search.rs +++ b/meilisearch-http/tests/search.rs @@ -1779,7 +1779,7 @@ async fn update_documents_with_facet_distribution() { server.create_index(body).await; let settings = json!({ "attributesForFaceting": ["genre"], - "displayedAttributes": ["genre", "type"], + "displayedAttributes": ["genre"], "searchableAttributes": ["genre", "type"] }); server.update_all_settings(settings).await; @@ -1809,7 +1809,6 @@ async fn update_documents_with_facet_distribution() { "facetsDistribution": ["genre"] }); let (response1, _) = server.search_post(search.clone()).await; - println!("response1: {}", response1); let expected_facet_distribution = json!({ "genre": { "grunge": 1, @@ -1828,6 +1827,5 @@ async fn update_documents_with_facet_distribution() { ]); server.add_or_update_multiple_documents(update2).await; let (response2, _) = server.search_post(search).await; - println!("response2: {}", response2); assert_json_eq!(expected_facet_distribution, response2["facetsDistribution"].clone()); } diff --git a/meilisearch-http/tests/settings.rs b/meilisearch-http/tests/settings.rs index 02e35b32d..4b7b591a4 100644 --- a/meilisearch-http/tests/settings.rs +++ b/meilisearch-http/tests/settings.rs @@ -486,15 +486,7 @@ async fn test_displayed_attributes_field() { ], "distinctAttribute": "id", "searchableAttributes": [ - "id", - "name", - "color", - "gender", - "email", - "phone", - "address", - "registered", - "about" + "*" ], "displayedAttributes": [ "age", diff --git a/meilisearch-schema/src/schema.rs b/meilisearch-schema/src/schema.rs index d35ae4f76..bc73d9ba7 100644 --- a/meilisearch-schema/src/schema.rs +++ b/meilisearch-schema/src/schema.rs @@ -16,14 +16,6 @@ impl OptionAll { std::mem::replace(self, OptionAll::None) } - fn map U>(self, f: F) -> OptionAll { - match self { - OptionAll::Some(x) => OptionAll::Some(f(x)), - OptionAll::All => OptionAll::All, - OptionAll::None => OptionAll::None, - } - } - pub fn is_all(&self) -> bool { matches!(self, OptionAll::All) } @@ -44,7 +36,7 @@ pub struct Schema { displayed: OptionAll>, indexed: OptionAll>, - indexed_map: HashMap, + fields_position: HashMap, } impl Schema { @@ -68,7 +60,7 @@ impl Schema { ranked: HashSet::new(), displayed: OptionAll::All, indexed: OptionAll::All, - indexed_map, + fields_position: indexed_map, } } @@ -109,15 +101,15 @@ impl Schema { self.fields_map.insert(name) } - pub fn insert_and_index(&mut self, name: &str) -> SResult { + pub fn register_field(&mut self, name: &str) -> SResult { match self.fields_map.id(name) { Some(id) => { Ok(id) } None => { let id = self.fields_map.insert(name)?; - let pos = self.indexed_map.len() as u16; - self.indexed_map.insert(id, pos.into()); + let pos = self.fields_position.len() as u16; + self.fields_position.insert(id, pos.into()); Ok(id) } } @@ -163,9 +155,9 @@ impl Schema { OptionAll::Some(ref v) => v.iter().cloned().collect(), OptionAll::All => { let fields = self - .indexed_map - .iter() - .map(|(&f, _)| f) + .fields_position + .keys() + .cloned() .collect(); fields }, @@ -202,18 +194,16 @@ impl Schema { pub fn set_indexed(&mut self, name: &str) -> SResult<(FieldId, IndexedPos)> { let id = self.fields_map.insert(name)?; - println!("setting {} indexed", name); - self.indexed = self.indexed.take().map(|mut v| { + if let OptionAll::Some(ref mut v) = self.indexed { v.insert(id); - v - }); + } - if let Some(indexed_pos) = self.indexed_map.get(&id) { + if let Some(indexed_pos) = self.fields_position.get(&id) { return Ok((id, *indexed_pos)) }; - let pos = self.indexed_map.len() as u16; - self.indexed_map.insert(id, pos.into()); + let pos = self.fields_position.len() as u16; + self.fields_position.insert(id, pos.into()); Ok((id, pos.into())) } @@ -227,56 +217,6 @@ impl Schema { } } - /// remove field from displayed attributes. If diplayed attributes is OptionAll::All, - /// dipslayed attributes is turned into OptionAll::Some(v) where v is all displayed attributes - /// except name. - pub fn remove_displayed(&mut self, name: &str) { - if let Some(id) = self.fields_map.id(name) { - self.displayed = match self.displayed.take() { - OptionAll::Some(mut v) => { - v.remove(&id); - OptionAll::Some(v) - } - OptionAll::All => { - let displayed = self.fields_map - .iter() - .filter_map(|(key, &value)| { - if key != name { - Some(value) - } else { - None - } - }) - .collect::>(); - OptionAll::Some(displayed) - } - OptionAll::None => OptionAll::None, - }; - } - } - - pub fn remove_indexed(&mut self, name: &str) { - if let Some(id) = self.fields_map.id(name) { - self.indexed_map.remove(&id); - self.indexed = match self.indexed.take() { - // valid because indexed is All and indexed() return the content of - // indexed_map that is already updated - OptionAll::All => { - let indexed = self.indexed_map - .iter() - .filter_map(|(&i, _)| if i != id { Some(i) } else { None }) - .collect(); - OptionAll::Some(indexed) - }, - OptionAll::Some(mut v) => { - v.retain(|x| *x != id); - OptionAll::Some(v) - } - OptionAll::None => OptionAll::None, - } - } - } - pub fn is_ranked(&self, id: FieldId) -> bool { self.ranked.get(&id).is_some() } @@ -291,10 +231,10 @@ impl Schema { pub fn is_indexed(&self, id: FieldId) -> Option<&IndexedPos> { match self.indexed { - OptionAll::All => self.indexed_map.get(&id), + OptionAll::All => self.fields_position.get(&id), OptionAll::Some(ref v) => { if v.contains(&id) { - self.indexed_map.get(&id) + self.fields_position.get(&id) } else { None } @@ -310,7 +250,7 @@ impl Schema { pub fn indexed_pos_to_field_id>(&self, pos: I) -> Option { let indexed_pos = pos.into().0; self - .indexed_map + .fields_position .iter() .find(|(_, &v)| v.0 == indexed_pos) .map(|(&k, _)| k) @@ -354,11 +294,11 @@ impl Schema { pub fn set_all_fields_as_indexed(&mut self) { self.indexed = OptionAll::All; - self.indexed_map.clear(); + self.fields_position.clear(); for (_name, id) in self.fields_map.iter() { - let pos = self.indexed_map.len() as u16; - self.indexed_map.insert(*id, pos.into()); + let pos = self.fields_position.len() as u16; + self.fields_position.insert(*id, pos.into()); } }