Merge #1001

1001: Fix settings bug r=MarinPostma a=MarinPostma

fix #942, see https://github.com/meilisearch/MeiliSearch/issues/942#issuecomment-706266440

Co-authored-by: mpostma <postma.marin@protonmail.com>
Co-authored-by: many <maxime@meilisearch.com>

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(),

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<FieldId>>,
+    pub displayed: bool,
 }
 
 impl<'de, 'a, 'b> de::Deserializer<'de> for &'b mut Deserializer<'a> {
@@ -93,7 +94,9 @@ 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)) {
+                // 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());
                         let ioread = SerdeJsonIoRead::new(cursor);

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::<T>::deserialize(&mut deserializer)?)

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::<HashMap<String, Value>>::deserialize(&mut deserializer)?;
@@ -228,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,

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"]
+        "searchableAttributes": ["genre", "type"]
     });
     server.update_all_settings(settings).await;
     let update1 = json!([

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;
 
@@ -486,15 +486,7 @@ async fn test_displayed_attributes_field() {
         ],
         "distinctAttribute": "id",
         "searchableAttributes": [
-            "id",
+            "*"
-            "name",
-            "color",
-            "gender",
-            "email",
-            "phone",
-            "address",
-            "registered",
-            "about"
         ],
         "displayedAttributes": [
             "age",
@@ -521,3 +513,71 @@ async fn test_displayed_attributes_field() {
 
     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);
+}

meilisearch-schema/src/schema.rs

@@ -16,14 +16,6 @@ impl<T> OptionAll<T> {
         std::mem::replace(self, OptionAll::None)
     }
 
-    fn map<U, F: FnOnce(T) -> U>(self, f: F) -> OptionAll<U> {
-        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)
     }
@@ -43,8 +35,8 @@ pub struct Schema {
     ranked: HashSet<FieldId>,
     displayed: OptionAll<HashSet<FieldId>>,
 
-    indexed: OptionAll<Vec<FieldId>>,
+    indexed: OptionAll<HashSet<FieldId>>,
-    indexed_map: HashMap<FieldId, IndexedPos>,
+    fields_position: HashMap<FieldId, IndexedPos>,
 }
 
 impl Schema {
@@ -68,7 +60,7 @@ impl Schema {
             ranked: HashSet::new(),
             displayed: OptionAll::All,
             indexed: OptionAll::All,
-            indexed_map,
+            fields_position: indexed_map,
         }
     }
 
@@ -109,14 +101,16 @@ impl Schema {
         self.fields_map.insert(name)
     }
 
-    pub fn insert_and_index(&mut self, name: &str) -> SResult<FieldId> {
+    pub fn register_field(&mut self, name: &str) -> SResult<FieldId> {
         match self.fields_map.id(name) {
             Some(id) => {
                 Ok(id)
             }
             None => {
-                self.set_indexed(name)?;
+                let id = self.fields_map.insert(name)?;
-                self.set_displayed(name)
+                let pos = self.fields_position.len() as u16;
+                self.fields_position.insert(id, pos.into());
+                Ok(id)
             }
         }
     }
@@ -156,18 +150,18 @@ impl Schema {
         }
     }
 
-    pub fn indexed(&self) -> Cow<[FieldId]> {
+    pub fn indexed(&self) -> Vec<FieldId> {
         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
+                    .fields_position
-                    .iter()
+                    .keys()
-                    .map(|(_, &f)| f)
+                    .cloned()
                     .collect();
-                Cow::Owned(fields)
+                fields
             },
-            OptionAll::None => Cow::Owned(Vec::new())
+            OptionAll::None => Vec::new()
         }
     }
 
@@ -201,15 +195,15 @@ impl Schema {
     pub fn set_indexed(&mut self, name: &str) -> SResult<(FieldId, IndexedPos)> {
         let id = self.fields_map.insert(name)?;
 
-        if let Some(indexed_pos) = self.indexed_map.get(&id) {
+        if let OptionAll::Some(ref mut v) = self.indexed {
+            v.insert(id);
+        }
+
+        if let Some(indexed_pos) = self.fields_position.get(&id) {
             return Ok((id, *indexed_pos))
         };
-        let pos = self.indexed_map.len() as u16;
+        let pos = self.fields_position.len() as u16;
-        self.indexed_map.insert(id, pos.into());
+        self.fields_position.insert(id, pos.into());
-        self.indexed = self.indexed.take().map(|mut v| {
-            v.push(id);
-            v
-        });
         Ok((id, pos.into()))
     }
 
@@ -223,50 +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::<HashSet<_>>();
-                    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()
     }
@@ -280,7 +230,17 @@ impl Schema {
     }
 
     pub fn is_indexed(&self, id: FieldId) -> Option<&IndexedPos> {
-        self.indexed_map.get(&id)
+        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
+        }
     }
 
     pub fn is_indexed_all(&self) -> bool {
@@ -290,7 +250,7 @@ impl Schema {
     pub fn indexed_pos_to_field_id<I: Into<IndexedPos>>(&self, pos: I) -> Option<FieldId> {
         let indexed_pos = pos.into().0;
         self
-            .indexed_map
+            .fields_position
             .iter()
             .find(|(_, &v)| v.0 == indexed_pos)
             .map(|(&k, _)| k)
@@ -324,9 +284,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())?;
         }
@@ -335,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;
+            let pos = self.fields_position.len() as u16;
-            self.indexed_map.insert(*id, pos.into());
+            self.fields_position.insert(*id, pos.into());
         }
     }