From a8ab15d65df44fe5e11a1052489b8969c1ebc3fc Mon Sep 17 00:00:00 2001 From: mpostma Date: Fri, 30 Oct 2020 11:30:18 +0100 Subject: [PATCH] Revert "Merge #1001" This reverts commit 690eab4a253bf25d6e457953182a86fb5ba8ee8b, reversing changes made to 086020e543d422237121949656d43358a7f93ff0. update changelog --- CHANGELOG.md | 1 - meilisearch-core/src/facets.rs | 4 +- meilisearch-core/src/serde/deserializer.rs | 5 +- meilisearch-core/src/store/mod.rs | 1 - .../src/update/documents_addition.rs | 3 +- meilisearch-http/tests/search.rs | 2 +- meilisearch-http/tests/settings.rs | 82 ++---------- meilisearch-schema/src/schema.rs | 117 ++++++++++++------ 8 files changed, 95 insertions(+), 120 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35672602a..27c27abfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,5 @@ ## v0.16.0 - - Fix settings bug that caused setting to be overwitten (#1001) - Automatically create index on document push if index doesn't exist (#914) - Sort displayedAttributes and facetDistribution (#946) diff --git a/meilisearch-core/src/facets.rs b/meilisearch-core/src/facets.rs index 6f5c2e837..11135c179 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.register_field("hello").unwrap(); + let id = schema.insert_and_index("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.register_field("hello").unwrap(); + let _id = schema.insert_and_index("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 2ef266565..e5e02a4d6 100644 --- a/meilisearch-core/src/serde/deserializer.rs +++ b/meilisearch-core/src/serde/deserializer.rs @@ -55,7 +55,6 @@ 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> { @@ -94,9 +93,7 @@ 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 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 4f8e1250a..fa5baa831 100644 --- a/meilisearch-core/src/store/mod.rs +++ b/meilisearch-core/src/store/mod.rs @@ -243,7 +243,6 @@ 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 2dc68c3ba..5603966b0 100644 --- a/meilisearch-core/src/update/documents_addition.rs +++ b/meilisearch-core/src/update/documents_addition.rs @@ -197,7 +197,6 @@ pub fn apply_addition<'a, 'b>( documents_fields: index.documents_fields, schema: &schema, fields: None, - displayed: false, }; let old_document = Option::>::deserialize(&mut deserializer)?; @@ -229,7 +228,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.register_field(&attribute)?; + let field_id = schema.insert_and_index(&attribute)?; index_document( writer, index.documents_fields, diff --git a/meilisearch-http/tests/search.rs b/meilisearch-http/tests/search.rs index 92879bb7b..170d9ffaf 100644 --- a/meilisearch-http/tests/search.rs +++ b/meilisearch-http/tests/search.rs @@ -1780,7 +1780,7 @@ async fn update_documents_with_facet_distribution() { let settings = json!({ "attributesForFaceting": ["genre"], "displayedAttributes": ["genre"], - "searchableAttributes": ["genre", "type"] + "searchableAttributes": ["genre"] }); server.update_all_settings(settings).await; let update1 = json!([ diff --git a/meilisearch-http/tests/settings.rs b/meilisearch-http/tests/settings.rs index 4b7b591a4..6b125c13a 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, Value}; +use serde_json::json; use std::convert::Into; mod common; @@ -486,7 +486,15 @@ async fn test_displayed_attributes_field() { ], "distinctAttribute": "id", "searchableAttributes": [ - "*" + "id", + "name", + "color", + "gender", + "email", + "phone", + "address", + "registered", + "about" ], "displayedAttributes": [ "age", @@ -512,72 +520,4 @@ async fn test_displayed_attributes_field() { let (response, _status_code) = server.get_all_settings().await; assert_json_eq!(body, response, ordered: true); -} - -#[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); -} +} \ No newline at end of file diff --git a/meilisearch-schema/src/schema.rs b/meilisearch-schema/src/schema.rs index bc73d9ba7..a1992080a 100644 --- a/meilisearch-schema/src/schema.rs +++ b/meilisearch-schema/src/schema.rs @@ -16,6 +16,14 @@ 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) } @@ -35,8 +43,8 @@ pub struct Schema { ranked: HashSet, displayed: OptionAll>, - indexed: OptionAll>, - fields_position: HashMap, + indexed: OptionAll>, + indexed_map: HashMap, } impl Schema { @@ -60,7 +68,7 @@ impl Schema { ranked: HashSet::new(), displayed: OptionAll::All, indexed: OptionAll::All, - fields_position: indexed_map, + indexed_map, } } @@ -101,16 +109,14 @@ impl Schema { self.fields_map.insert(name) } - pub fn register_field(&mut self, name: &str) -> SResult { + pub fn insert_and_index(&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.fields_position.len() as u16; - self.fields_position.insert(id, pos.into()); - Ok(id) + self.set_indexed(name)?; + self.set_displayed(name) } } } @@ -150,18 +156,18 @@ impl Schema { } } - pub fn indexed(&self) -> Vec { + pub fn indexed(&self) -> Cow<[FieldId]> { match self.indexed { - OptionAll::Some(ref v) => v.iter().cloned().collect(), + OptionAll::Some(ref v) => Cow::Borrowed(v), OptionAll::All => { let fields = self - .fields_position - .keys() - .cloned() + .fields_map + .iter() + .map(|(_, &f)| f) .collect(); - fields + Cow::Owned(fields) }, - OptionAll::None => Vec::new() + OptionAll::None => Cow::Owned(Vec::new()) } } @@ -195,15 +201,15 @@ impl Schema { pub fn set_indexed(&mut self, name: &str) -> SResult<(FieldId, IndexedPos)> { let id = self.fields_map.insert(name)?; - if let OptionAll::Some(ref mut v) = self.indexed { - v.insert(id); - } - - if let Some(indexed_pos) = self.fields_position.get(&id) { + if let Some(indexed_pos) = self.indexed_map.get(&id) { return Ok((id, *indexed_pos)) }; - let pos = self.fields_position.len() as u16; - self.fields_position.insert(id, pos.into()); + 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())) } @@ -217,6 +223,50 @@ 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 => OptionAll::Some(self.indexed().into_owned()), + 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() } @@ -230,17 +280,7 @@ impl Schema { } pub fn is_indexed(&self, id: FieldId) -> Option<&IndexedPos> { - match self.indexed { - OptionAll::All => self.fields_position.get(&id), - OptionAll::Some(ref v) => { - if v.contains(&id) { - self.fields_position.get(&id) - } else { - None - } - } - OptionAll::None => None - } + self.indexed_map.get(&id) } pub fn is_indexed_all(&self) -> bool { @@ -250,7 +290,7 @@ impl Schema { pub fn indexed_pos_to_field_id>(&self, pos: I) -> Option { let indexed_pos = pos.into().0; self - .fields_position + .indexed_map .iter() .find(|(_, &v)| v.0 == indexed_pos) .map(|(&k, _)| k) @@ -284,8 +324,9 @@ impl Schema { v.clear(); OptionAll::Some(v) }, - _ => OptionAll::Some(HashSet::new()), + _ => OptionAll::Some(Vec::new()), }; + self.indexed_map.clear(); for name in data { self.set_indexed(name.as_ref())?; } @@ -294,11 +335,11 @@ impl Schema { pub fn set_all_fields_as_indexed(&mut self) { self.indexed = OptionAll::All; - self.fields_position.clear(); + self.indexed_map.clear(); for (_name, id) in self.fields_map.iter() { - let pos = self.fields_position.len() as u16; - self.fields_position.insert(*id, pos.into()); + let pos = self.indexed_map.len() as u16; + self.indexed_map.insert(*id, pos.into()); } }