From d9dd2a038b0dded7222b710f7fd90365720bd768 Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin Date: Tue, 24 Aug 2021 21:55:29 +0300 Subject: [PATCH] refactor(http): use Setting enum --- meilisearch-http/src/index/mod.rs | 37 ++-- meilisearch-http/src/index/updates.rs | 190 ++++++++---------- .../index_controller/dump_actor/loaders/v1.rs | 80 ++++++-- .../src/routes/indexes/settings.rs | 9 +- 4 files changed, 169 insertions(+), 147 deletions(-) diff --git a/meilisearch-http/src/index/mod.rs b/meilisearch-http/src/index/mod.rs index 37fd6fc39..827d30753 100644 --- a/meilisearch-http/src/index/mod.rs +++ b/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, D::Error> -where - T: Deserialize<'de>, - D: Deserializer<'de>, -{ - Deserialize::deserialize(deserializer).map(Some) -} - impl Index { pub fn open(path: impl AsRef, size: usize) -> Result { 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, }) } diff --git a/meilisearch-http/src/index/updates.rs b/meilisearch-http/src/index/updates.rs index 34fafd143..abfb55d1d 100644 --- a/meilisearch-http/src/index/updates.rs +++ b/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( - field: &Option>>, + field: &Setting>, s: S, ) -> std::result::Result 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 { #[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>>, + pub displayed_attributes: Setting>, #[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>>, + pub searchable_attributes: Setting>, - #[serde( - default, - deserialize_with = "deserialize_some", - skip_serializing_if = "Option::is_none" - )] - pub filterable_attributes: Option>>, - - #[serde( - default, - deserialize_with = "deserialize_some", - skip_serializing_if = "Option::is_none" - )] - pub ranking_rules: Option>>, - #[serde( - default, - deserialize_with = "deserialize_some", - skip_serializing_if = "Option::is_none" - )] - pub stop_words: Option>>, - #[serde( - default, - deserialize_with = "deserialize_some", - skip_serializing_if = "Option::is_none" - )] - pub synonyms: Option>>>, - #[serde( - default, - deserialize_with = "deserialize_some", - skip_serializing_if = "Option::is_none" - )] - pub distinct_attribute: Option>, + #[serde(default, skip_serializing_if = "Setting::is_not_set")] + pub filterable_attributes: Setting>, + #[serde(default, skip_serializing_if = "Setting::is_not_set")] + pub ranking_rules: Setting>, + #[serde(default, skip_serializing_if = "Setting::is_not_set")] + pub stop_words: Setting>, + #[serde(default, skip_serializing_if = "Setting::is_not_set")] + pub synonyms: Setting>>, + #[serde(default, skip_serializing_if = "Setting::is_not_set")] + pub distinct_attribute: Setting, #[serde(skip)] pub _kind: PhantomData, @@ -89,13 +72,13 @@ pub struct Settings { impl Settings { pub fn cleared() -> Settings { 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 { } impl Settings { - pub fn check(mut self) -> Settings { - let displayed_attributes = match self.displayed_attributes.take() { - Some(Some(fields)) => { + pub fn check(self) -> Settings { + 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::, }; @@ -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::, }; 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); } } diff --git a/meilisearch-http/src/index_controller/dump_actor/loaders/v1.rs b/meilisearch-http/src/index_controller/dump_actor/loaders/v1.rs index 30923584e..9bcc4dd7b 100644 --- a/meilisearch-http/src/index_controller/dump_actor/loaders/v1.rs +++ b/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, D::Error> +where + T: Deserialize<'de>, + D: Deserializer<'de>, +{ + Deserialize::deserialize(deserializer).map(Some) +} + // These are the settings used in legacy meilisearch ( for index_controller::Settings { 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` into a `BTreeSet` - 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` but now we have a `HashMap` // 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` into a `BTreeSet` - 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` into a `BTreeSet` - 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` into a `BTreeMap` - 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, } } diff --git a/meilisearch-http/src/routes/indexes/settings.rs b/meilisearch-http/src/routes/indexes/settings.rs index 47f18a68a..9cdf515aa 100644 --- a/meilisearch-http/src/routes/indexes/settings.rs +++ b/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 { 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>, ) -> std::result::Result { 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() };