Merge #1623

1623: Use Setting enum r=Kerollmops a=shekhirin

Resolves https://github.com/meilisearch/MeiliSearch/issues/1620

Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>

meilisearch-http/src/index/mod.rs

@@ -6,16 +6,16 @@ use std::path::Path;
 use std::sync::Arc;
 
 use heed::{EnvOpenOptions, RoTxn};
+use milli::update::Setting;
 use milli::{obkv_to_json, FieldId};
-use serde::{de::Deserializer, Deserialize};
 use serde_json::{Map, Value};
 
-use crate::helpers::EnvSizer;
 use error::Result;
-
 pub use search::{default_crop_length, SearchQuery, SearchResult, DEFAULT_SEARCH_LIMIT};
 pub use updates::{Checked, Facets, Settings, Unchecked};
 
+use crate::helpers::EnvSizer;
+
 use self::error::IndexError;
 
 pub mod error;
@@ -38,14 +38,6 @@ impl Deref for Index {
     }
 }
 
-pub fn deserialize_some<'de, T, D>(deserializer: D) -> std::result::Result<Option<T>, D::Error>
-where
-    T: Deserialize<'de>,
-    D: Deserializer<'de>,
-{
-    Deserialize::deserialize(deserializer).map(Some)
-}
-
 impl Index {
     pub fn open(path: impl AsRef<Path>, size: usize) -> Result<Self> {
         create_dir_all(&path)?;
@@ -100,13 +92,22 @@ impl Index {
             .collect();
 
         Ok(Settings {
-            displayed_attributes: Some(displayed_attributes),
-            searchable_attributes: Some(searchable_attributes),
-            filterable_attributes: Some(Some(filterable_attributes)),
-            ranking_rules: Some(Some(criteria)),
-            stop_words: Some(Some(stop_words)),
-            distinct_attribute: Some(distinct_field),
-            synonyms: Some(Some(synonyms)),
+            displayed_attributes: match displayed_attributes {
+                Some(attrs) => Setting::Set(attrs),
+                None => Setting::Reset,
+            },
+            searchable_attributes: match searchable_attributes {
+                Some(attrs) => Setting::Set(attrs),
+                None => Setting::Reset,
+            },
+            filterable_attributes: Setting::Set(filterable_attributes),
+            ranking_rules: Setting::Set(criteria),
+            stop_words: Setting::Set(stop_words),
+            distinct_attribute: match distinct_field {
+                Some(field) => Setting::Set(field),
+                None => Setting::Reset,
+            },
+            synonyms: Setting::Set(synonyms),
             _kind: PhantomData,
         })
     }

meilisearch-http/src/index/updates.rs

@@ -5,27 +5,33 @@ use std::num::NonZeroUsize;
 
 use flate2::read::GzDecoder;
 use log::{debug, info, trace};
-use milli::update::{IndexDocumentsMethod, UpdateBuilder, UpdateFormat};
+use milli::update::{IndexDocumentsMethod, Setting, UpdateBuilder, UpdateFormat};
 use serde::{Deserialize, Serialize, Serializer};
 
 use crate::index_controller::UpdateResult;
 
 use super::error::Result;
-use super::{deserialize_some, Index};
+use super::Index;
 
 fn serialize_with_wildcard<S>(
-    field: &Option<Option<Vec<String>>>,
+    field: &Setting<Vec<String>>,
     s: S,
 ) -> std::result::Result<S::Ok, S::Error>
 where
     S: Serializer,
 {
     let wildcard = vec!["*".to_string()];
-    s.serialize_some(&field.as_ref().map(|o| o.as_ref().unwrap_or(&wildcard)))
+    match field {
+        Setting::Set(value) => Some(value),
+        Setting::Reset => Some(&wildcard),
+        Setting::NotSet => None,
+    }
+    .serialize(s)
 }
 
 #[derive(Clone, Default, Debug, Serialize)]
 pub struct Checked;
+
 #[derive(Clone, Default, Debug, Serialize, Deserialize)]
 pub struct Unchecked;
 
@@ -36,51 +42,28 @@ pub struct Unchecked;
 pub struct Settings<T> {
     #[serde(
         default,
-        deserialize_with = "deserialize_some",
         serialize_with = "serialize_with_wildcard",
-        skip_serializing_if = "Option::is_none"
+        skip_serializing_if = "Setting::is_not_set"
     )]
-    pub displayed_attributes: Option<Option<Vec<String>>>,
+    pub displayed_attributes: Setting<Vec<String>>,
 
     #[serde(
         default,
-        deserialize_with = "deserialize_some",
         serialize_with = "serialize_with_wildcard",
-        skip_serializing_if = "Option::is_none"
+        skip_serializing_if = "Setting::is_not_set"
     )]
-    pub searchable_attributes: Option<Option<Vec<String>>>,
+    pub searchable_attributes: Setting<Vec<String>>,
 
-    #[serde(
-        default,
-        deserialize_with = "deserialize_some",
-        skip_serializing_if = "Option::is_none"
-    )]
-    pub filterable_attributes: Option<Option<HashSet<String>>>,
-
-    #[serde(
-        default,
-        deserialize_with = "deserialize_some",
-        skip_serializing_if = "Option::is_none"
-    )]
-    pub ranking_rules: Option<Option<Vec<String>>>,
-    #[serde(
-        default,
-        deserialize_with = "deserialize_some",
-        skip_serializing_if = "Option::is_none"
-    )]
-    pub stop_words: Option<Option<BTreeSet<String>>>,
-    #[serde(
-        default,
-        deserialize_with = "deserialize_some",
-        skip_serializing_if = "Option::is_none"
-    )]
-    pub synonyms: Option<Option<BTreeMap<String, Vec<String>>>>,
-    #[serde(
-        default,
-        deserialize_with = "deserialize_some",
-        skip_serializing_if = "Option::is_none"
-    )]
-    pub distinct_attribute: Option<Option<String>>,
+    #[serde(default, skip_serializing_if = "Setting::is_not_set")]
+    pub filterable_attributes: Setting<HashSet<String>>,
+    #[serde(default, skip_serializing_if = "Setting::is_not_set")]
+    pub ranking_rules: Setting<Vec<String>>,
+    #[serde(default, skip_serializing_if = "Setting::is_not_set")]
+    pub stop_words: Setting<BTreeSet<String>>,
+    #[serde(default, skip_serializing_if = "Setting::is_not_set")]
+    pub synonyms: Setting<BTreeMap<String, Vec<String>>>,
+    #[serde(default, skip_serializing_if = "Setting::is_not_set")]
+    pub distinct_attribute: Setting<String>,
 
     #[serde(skip)]
     pub _kind: PhantomData<T>,
@@ -89,13 +72,13 @@ pub struct Settings<T> {
 impl Settings<Checked> {
     pub fn cleared() -> Settings<Checked> {
         Settings {
-            displayed_attributes: Some(None),
-            searchable_attributes: Some(None),
-            filterable_attributes: Some(None),
-            ranking_rules: Some(None),
-            stop_words: Some(None),
-            synonyms: Some(None),
-            distinct_attribute: Some(None),
+            displayed_attributes: Setting::Reset,
+            searchable_attributes: Setting::Reset,
+            filterable_attributes: Setting::Reset,
+            ranking_rules: Setting::Reset,
+            stop_words: Setting::Reset,
+            synonyms: Setting::Reset,
+            distinct_attribute: Setting::Reset,
             _kind: PhantomData,
         }
     }
@@ -126,24 +109,24 @@ impl Settings<Checked> {
 }
 
 impl Settings<Unchecked> {
-    pub fn check(mut self) -> Settings<Checked> {
-        let displayed_attributes = match self.displayed_attributes.take() {
-            Some(Some(fields)) => {
+    pub fn check(self) -> Settings<Checked> {
+        let displayed_attributes = match self.displayed_attributes {
+            Setting::Set(fields) => {
                 if fields.iter().any(|f| f == "*") {
-                    Some(None)
+                    Setting::Reset
                 } else {
-                    Some(Some(fields))
+                    Setting::Set(fields)
                 }
             }
             otherwise => otherwise,
         };
 
-        let searchable_attributes = match self.searchable_attributes.take() {
-            Some(Some(fields)) => {
+        let searchable_attributes = match self.searchable_attributes {
+            Setting::Set(fields) => {
                 if fields.iter().any(|f| f == "*") {
-                    Some(None)
+                    Setting::Reset
                 } else {
-                    Some(Some(fields))
+                    Setting::Set(fields)
                 }
             }
             otherwise => otherwise,
@@ -252,51 +235,48 @@ impl Index {
         // We must use the write transaction of the update here.
         let mut builder = update_builder.settings(txn, self);
 
-        if let Some(ref names) = settings.searchable_attributes {
-            match names {
-                Some(names) => builder.set_searchable_fields(names.clone()),
-                None => builder.reset_searchable_fields(),
-            }
+        match settings.searchable_attributes {
+            Setting::Set(ref names) => builder.set_searchable_fields(names.clone()),
+            Setting::Reset => builder.reset_searchable_fields(),
+            Setting::NotSet => (),
         }
 
-        if let Some(ref names) = settings.displayed_attributes {
-            match names {
-                Some(names) => builder.set_displayed_fields(names.clone()),
-                None => builder.reset_displayed_fields(),
-            }
+        match settings.displayed_attributes {
+            Setting::Set(ref names) => builder.set_displayed_fields(names.clone()),
+            Setting::Reset => builder.reset_displayed_fields(),
+            Setting::NotSet => (),
         }
 
-        if let Some(ref facet_types) = settings.filterable_attributes {
-            let facet_types = facet_types.clone().unwrap_or_else(HashSet::new);
-            builder.set_filterable_fields(facet_types);
+        match settings.filterable_attributes {
+            Setting::Set(ref facet_types) => builder.set_filterable_fields(facet_types.clone()),
+            Setting::Reset => builder.set_filterable_fields(HashSet::new()),
+            Setting::NotSet => (),
         }
 
-        if let Some(ref criteria) = settings.ranking_rules {
-            match criteria {
-                Some(criteria) => builder.set_criteria(criteria.clone()),
-                None => builder.reset_criteria(),
-            }
+        match settings.ranking_rules {
+            Setting::Set(ref criteria) => builder.set_criteria(criteria.clone()),
+            Setting::Reset => builder.reset_criteria(),
+            Setting::NotSet => (),
         }
 
-        if let Some(ref stop_words) = settings.stop_words {
-            match stop_words {
-                Some(stop_words) => builder.set_stop_words(stop_words.clone()),
-                None => builder.reset_stop_words(),
-            }
+        match settings.stop_words {
+            Setting::Set(ref stop_words) => builder.set_stop_words(stop_words.clone()),
+            Setting::Reset => builder.reset_stop_words(),
+            Setting::NotSet => (),
         }
 
-        if let Some(ref synonyms) = settings.synonyms {
-            match synonyms {
-                Some(synonyms) => builder.set_synonyms(synonyms.clone().into_iter().collect()),
-                None => builder.reset_synonyms(),
+        match settings.synonyms {
+            Setting::Set(ref synonyms) => {
+                builder.set_synonyms(synonyms.clone().into_iter().collect())
             }
+            Setting::Reset => builder.reset_synonyms(),
+            Setting::NotSet => (),
         }
 
-        if let Some(ref distinct_attribute) = settings.distinct_attribute {
-            match distinct_attribute {
-                Some(attr) => builder.set_distinct_field(attr.clone()),
-                None => builder.reset_distinct_field(),
-            }
+        match settings.distinct_attribute {
+            Setting::Set(ref attr) => builder.set_distinct_field(attr.clone()),
+            Setting::Reset => builder.reset_distinct_field(),
+            Setting::NotSet => (),
         }
 
         builder.execute(|indexing_step, update_id| {
@@ -345,13 +325,13 @@ mod test {
     fn test_setting_check() {
         // test no changes
         let settings = Settings {
-            displayed_attributes: Some(Some(vec![String::from("hello")])),
-            searchable_attributes: Some(Some(vec![String::from("hello")])),
-            filterable_attributes: None,
-            ranking_rules: None,
-            stop_words: None,
-            synonyms: None,
-            distinct_attribute: None,
+            displayed_attributes: Setting::Set(vec![String::from("hello")]),
+            searchable_attributes: Setting::Set(vec![String::from("hello")]),
+            filterable_attributes: Setting::NotSet,
+            ranking_rules: Setting::NotSet,
+            stop_words: Setting::NotSet,
+            synonyms: Setting::NotSet,
+            distinct_attribute: Setting::NotSet,
             _kind: PhantomData::<Unchecked>,
         };
 
@@ -365,18 +345,18 @@ mod test {
         // test wildcard
         // test no changes
         let settings = Settings {
-            displayed_attributes: Some(Some(vec![String::from("*")])),
-            searchable_attributes: Some(Some(vec![String::from("hello"), String::from("*")])),
-            filterable_attributes: None,
-            ranking_rules: None,
-            stop_words: None,
-            synonyms: None,
-            distinct_attribute: None,
+            displayed_attributes: Setting::Set(vec![String::from("*")]),
+            searchable_attributes: Setting::Set(vec![String::from("hello"), String::from("*")]),
+            filterable_attributes: Setting::NotSet,
+            ranking_rules: Setting::NotSet,
+            stop_words: Setting::NotSet,
+            synonyms: Setting::NotSet,
+            distinct_attribute: Setting::NotSet,
             _kind: PhantomData::<Unchecked>,
         };
 
         let checked = settings.check();
-        assert_eq!(checked.displayed_attributes, Some(None));
-        assert_eq!(checked.searchable_attributes, Some(None));
+        assert_eq!(checked.displayed_attributes, Setting::Reset);
+        assert_eq!(checked.searchable_attributes, Setting::Reset);
     }
 }

meilisearch-http/src/index_controller/dump_actor/loaders/v1.rs

@@ -7,13 +7,13 @@ use std::sync::Arc;
 
 use heed::EnvOpenOptions;
 use log::{error, info, warn};
-use milli::update::{IndexDocumentsMethod, UpdateFormat};
-use serde::{Deserialize, Serialize};
+use milli::update::{IndexDocumentsMethod, Setting, UpdateFormat};
+use serde::{Deserialize, Deserializer, Serialize};
 use uuid::Uuid;
 
 use crate::index_controller::{self, uuid_resolver::HeedUuidStore, IndexMetadata};
 use crate::{
-    index::{deserialize_some, update_handler::UpdateHandler, Index, Unchecked},
+    index::{update_handler::UpdateHandler, Index, Unchecked},
     option::IndexerOpts,
 };
 
@@ -56,6 +56,14 @@ impl MetadataV1 {
     }
 }
 
+pub fn deserialize_some<'de, T, D>(deserializer: D) -> std::result::Result<Option<T>, D::Error>
+where
+    T: Deserialize<'de>,
+    D: Deserializer<'de>,
+{
+    Deserialize::deserialize(deserializer).map(Some)
+}
+
 // These are the settings used in legacy meilisearch (<v0.21.0).
 #[derive(Default, Clone, Serialize, Deserialize, Debug)]
 #[serde(rename_all = "camelCase", deny_unknown_fields)]
@@ -134,34 +142,62 @@ fn load_index(
 impl From<Settings> for index_controller::Settings<Unchecked> {
     fn from(settings: Settings) -> Self {
         Self {
-            distinct_attribute: settings.distinct_attribute,
+            distinct_attribute: match settings.distinct_attribute {
+                Some(Some(attr)) => Setting::Set(attr),
+                Some(None) => Setting::Reset,
+                None => Setting::NotSet
+            },
             // we need to convert the old `Vec<String>` into a `BTreeSet<String>`
-            displayed_attributes: settings.displayed_attributes.map(|o| o.map(|vec| vec.into_iter().collect())),
-            searchable_attributes: settings.searchable_attributes,
+            displayed_attributes: match settings.displayed_attributes {
+                Some(Some(attrs)) => Setting::Set(attrs.into_iter().collect()),
+                Some(None) => Setting::Reset,
+                None => Setting::NotSet
+            },
+            searchable_attributes: match settings.searchable_attributes {
+                Some(Some(attrs)) => Setting::Set(attrs),
+                Some(None) => Setting::Reset,
+                None => Setting::NotSet
+            },
             // we previously had a `Vec<String>` but now we have a `HashMap<String, String>`
             // representing the name of the faceted field + the type of the field. Since the type
             // was not known in the V1 of the dump we are just going to assume everything is a
             // String
-            filterable_attributes: settings.attributes_for_faceting.map(|o| o.map(|vec| vec.into_iter().collect())),
+            filterable_attributes: match settings.attributes_for_faceting {
+                Some(Some(attrs)) => Setting::Set(attrs.into_iter().collect()),
+                Some(None) => Setting::Reset,
+                None => Setting::NotSet
+            },
             // we need to convert the old `Vec<String>` into a `BTreeSet<String>`
-            ranking_rules: settings.ranking_rules.map(|o| o.map(|vec| vec.into_iter().filter(|criterion| {
-                match criterion.as_str() {
-                    "words" | "typo" | "proximity" | "attribute" | "exactness" => true,
-                    s if s.starts_with("asc") || s.starts_with("desc") => true,
-                    "wordsPosition" => {
-                        warn!("The criteria `attribute` and `wordsPosition` have been merged into a single criterion `attribute` so `wordsPositon` will be ignored");
-                        false
+            ranking_rules: match settings.ranking_rules {
+                Some(Some(ranking_rules)) => Setting::Set(ranking_rules.into_iter().filter(|criterion| {
+                    match criterion.as_str() {
+                        "words" | "typo" | "proximity" | "attribute" | "exactness" => true,
+                        s if s.starts_with("asc") || s.starts_with("desc") => true,
+                        "wordsPosition" => {
+                            warn!("The criteria `attribute` and `wordsPosition` have been merged into a single criterion `attribute` so `wordsPositon` will be ignored");
+                            false
+                        }
+                        s => {
+                            error!("Unknown criterion found in the dump: `{}`, it will be ignored", s);
+                            false
+                        }
                     }
-                    s => {
-                        error!("Unknown criterion found in the dump: `{}`, it will be ignored", s);
-                        false
-                    }
-                    }
-                }).collect())),
+                }).collect()),
+                Some(None) => Setting::Reset,
+                None => Setting::NotSet
+            },
             // we need to convert the old `Vec<String>` into a `BTreeSet<String>`
-            stop_words: settings.stop_words.map(|o| o.map(|vec| vec.into_iter().collect())),
+            stop_words: match settings.stop_words {
+                Some(Some(stop_words)) => Setting::Set(stop_words.into_iter().collect()),
+                Some(None) => Setting::Reset,
+                None => Setting::NotSet
+            },
             // we need to convert the old `Vec<String>` into a `BTreeMap<String>`
-            synonyms: settings.synonyms.map(|o| o.map(|vec| vec.into_iter().collect())),
+            synonyms: match settings.synonyms {
+                Some(Some(synonyms)) => Setting::Set(synonyms.into_iter().collect()),
+                Some(None) => Setting::Reset,
+                None => Setting::NotSet
+            },
             _kind: PhantomData,
         }
     }

meilisearch-http/src/routes/indexes/settings.rs

@@ -13,6 +13,8 @@ macro_rules! make_setting_route {
             use log::debug;
             use actix_web::{web, HttpResponse, Resource};
 
+            use milli::update::Setting;
+
             use crate::data;
             use crate::error::ResponseError;
             use crate::index::Settings;
@@ -24,7 +26,7 @@ macro_rules! make_setting_route {
             ) -> Result<HttpResponse, ResponseError> {
                 use crate::index::Settings;
                 let settings = Settings {
-                    $attr: Some(None),
+                    $attr: Setting::Reset,
                     ..Default::default()
                 };
                 let update_status = data.update_settings(index_uid.into_inner(), settings, false).await?;
@@ -38,7 +40,10 @@ macro_rules! make_setting_route {
                 body: actix_web::web::Json<Option<$type>>,
             ) -> std::result::Result<HttpResponse, ResponseError> {
                 let settings = Settings {
-                    $attr: Some(body.into_inner()),
+                    $attr: match body.into_inner() {
+                        Some(inner_body) => Setting::Set(inner_body),
+                        None => Setting::Reset
+                    },
                     ..Default::default()
                 };