Merge #186

186: settings fix r=MarinPostma a=MarinPostma

add type checked settigns validation. For now it only transform the settings accepting wildcard


Co-authored-by: Marin Postma <postma.marin@protonmail.com>

meilisearch-http/src/data/mod.rs

@@ -3,7 +3,7 @@ use std::sync::Arc;
 
 use sha2::Digest;
 
-use crate::index::Settings;
+use crate::index::{Checked, Settings};
 use crate::index_controller::{IndexController, IndexStats, Stats};
 use crate::index_controller::{IndexMetadata, IndexSettings};
 use crate::option::Opt;
@@ -74,7 +74,7 @@ impl Data {
         Ok(Data { inner })
     }
 
-    pub async fn settings(&self, uid: String) -> anyhow::Result<Settings> {
+    pub async fn settings(&self, uid: String) -> anyhow::Result<Settings<Checked>> {
         self.index_controller.settings(uid).await
     }
 

meilisearch-http/src/data/updates.rs

@@ -2,7 +2,7 @@ use actix_web::web::Payload;
 use milli::update::{IndexDocumentsMethod, UpdateFormat};
 
 use super::Data;
-use crate::index::Settings;
+use crate::index::{Checked, Settings};
 use crate::index_controller::{IndexMetadata, IndexSettings, UpdateStatus};
 
 impl Data {
@@ -24,7 +24,7 @@ impl Data {
     pub async fn update_settings(
         &self,
         index: String,
-        settings: Settings,
+        settings: Settings<Checked>,
         create: bool,
     ) -> anyhow::Result<UpdateStatus> {
         let update = self

meilisearch-http/src/index/mod.rs

@@ -1,4 +1,4 @@
-use std::collections::{BTreeSet, HashSet};
+use std::{collections::{BTreeSet, HashSet}, marker::PhantomData};
 use std::ops::Deref;
 use std::sync::Arc;
 
@@ -8,7 +8,7 @@ use serde_json::{Map, Value};
 
 use crate::helpers::EnvSizer;
 pub use search::{SearchQuery, SearchResult, DEFAULT_SEARCH_LIMIT};
-pub use updates::{Facets, Settings};
+pub use updates::{Facets, Settings, Checked, Unchecked};
 
 mod search;
 mod updates;
@@ -27,7 +27,7 @@ impl Deref for Index {
 }
 
 impl Index {
-    pub fn settings(&self) -> anyhow::Result<Settings> {
+    pub fn settings(&self) -> anyhow::Result<Settings<Checked>> {
         let txn = self.read_txn()?;
 
         let displayed_attributes = self
@@ -68,6 +68,7 @@ impl Index {
             ranking_rules: Some(Some(criteria)),
             stop_words: Some(Some(stop_words)),
             distinct_attribute: Some(distinct_attribute),
+            _kind: PhantomData,
         })
     }
 

meilisearch-http/src/index/updates.rs

@@ -1,6 +1,7 @@
 use std::collections::{BTreeSet, HashMap};
 use std::io;
 use std::num::NonZeroUsize;
+use std::marker::PhantomData;
 
 use flate2::read::GzDecoder;
 use log::info;
@@ -10,10 +11,15 @@ use serde::{de::Deserializer, Deserialize, Serialize};
 use super::Index;
 use crate::index_controller::UpdateResult;
 
+#[derive(Clone, Default, Debug)]
+pub struct Checked;
+#[derive(Clone, Default, Debug)]
+pub struct Unchecked;
+
 #[derive(Debug, Clone, Default, Serialize, Deserialize)]
 #[serde(deny_unknown_fields)]
 #[serde(rename_all = "camelCase")]
-pub struct Settings {
+pub struct Settings<T> {
     #[serde(
         default,
         deserialize_with = "deserialize_some",
@@ -49,17 +55,57 @@ pub struct Settings {
         skip_serializing_if = "Option::is_none"
     )]
     pub distinct_attribute: Option<Option<String>>,
+
+    #[serde(skip)]
+    pub _kind: PhantomData<T>,
 }
 
-impl Settings {
+impl Settings<Checked> {
-    pub fn cleared() -> Self {
+    pub fn cleared() -> Settings<Checked> {
-        Self {
+        Settings {
             displayed_attributes: Some(None),
             searchable_attributes: Some(None),
             attributes_for_faceting: Some(None),
             ranking_rules: Some(None),
             stop_words: Some(None),
             distinct_attribute: Some(None),
+            _kind: PhantomData,
+        }
+    }
+}
+
+impl Settings<Unchecked> {
+    pub fn check(mut self) -> Settings<Checked> {
+        let displayed_attributes = match self.displayed_attributes.take() {
+            Some(Some(fields)) => {
+                if fields.iter().any(|f| f == "*") {
+                    Some(None)
+                } else {
+                    Some(Some(fields))
+                }
+            }
+            otherwise => otherwise,
+        };
+
+        let searchable_attributes = match self.searchable_attributes.take() {
+            Some(Some(fields)) => {
+                if fields.iter().any(|f| f == "*") {
+                    Some(None)
+                } else {
+                    Some(Some(fields))
+                }
+            }
+            otherwise => otherwise,
+        };
+
+        Settings {
+            displayed_attributes,
+            searchable_attributes,
+            attributes_for_faceting: self.attributes_for_faceting,
+            ranking_rules: self.ranking_rules,
+            stop_words: self.stop_words,
+            distinct_attribute: self.distinct_attribute,
+            _kind: PhantomData,
         }
     }
 }
@@ -137,7 +183,7 @@ impl Index {
 
     pub fn update_settings(
         &self,
-        settings: &Settings,
+        settings: &Settings<Checked>,
         update_builder: UpdateBuilder,
     ) -> anyhow::Result<UpdateResult> {
         // We must use the write transaction of the update here.
@@ -222,3 +268,42 @@ impl Index {
         }
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[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")])),
+            attributes_for_faceting: None,
+            ranking_rules: None,
+            stop_words: None,
+            distinct_attribute: None,
+            _kind: PhantomData::<Unchecked>,
+        };
+
+        let checked = settings.clone().check();
+        assert_eq!(settings.displayed_attributes, checked.displayed_attributes);
+        assert_eq!(settings.searchable_attributes, checked.searchable_attributes);
+
+        // 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("*")])),
+            attributes_for_faceting: None,
+            ranking_rules: None,
+            stop_words: None,
+            distinct_attribute: None,
+            _kind: PhantomData::<Unchecked>,
+        };
+
+        let checked = settings.check();
+        assert_eq!(checked.displayed_attributes, Some(None));
+        assert_eq!(checked.searchable_attributes, Some(None));
+    }
+}

meilisearch-http/src/index_controller/index_actor/actor.rs

@@ -10,7 +10,7 @@ use tokio::sync::mpsc;
 use tokio::task::spawn_blocking;
 use uuid::Uuid;
 
-use crate::index::{Document, SearchQuery, SearchResult, Settings};
+use crate::index::{Checked, Document, SearchQuery, SearchResult, Settings};
 use crate::index_controller::{
     get_arc_ownership_blocking, update_handler::UpdateHandler, Failed, IndexStats, Processed,
     Processing,
@@ -164,7 +164,7 @@ impl<S: IndexStore + Sync + Send> IndexActor<S> {
             .map_err(|e| IndexError::Error(e.into()))
     }
 
-    async fn handle_settings(&self, uuid: Uuid) -> IndexResult<Settings> {
+    async fn handle_settings(&self, uuid: Uuid) -> IndexResult<Settings<Checked>> {
         let index = self
             .store
             .get(uuid)

meilisearch-http/src/index_controller/index_actor/handle_impl.rs

@@ -3,7 +3,7 @@ use std::path::{Path, PathBuf};
 use tokio::sync::{mpsc, oneshot};
 use uuid::Uuid;
 
-use crate::index_controller::{IndexSettings, IndexStats, Processing};
+use crate::{index::Checked, index_controller::{IndexSettings, IndexStats, Processing}};
 use crate::{
     index::{Document, SearchQuery, SearchResult, Settings},
     index_controller::{Failed, Processed},
@@ -57,7 +57,7 @@ impl IndexActorHandle for IndexActorHandleImpl {
         Ok(receiver.await.expect("IndexActor has been killed")?)
     }
 
-    async fn settings(&self, uuid: Uuid) -> IndexResult<Settings> {
+    async fn settings(&self, uuid: Uuid) -> IndexResult<Settings<Checked>> {
         let (ret, receiver) = oneshot::channel();
         let msg = IndexMsg::Settings { uuid, ret };
         let _ = self.sender.send(msg).await;

meilisearch-http/src/index_controller/index_actor/message.rs

@@ -3,7 +3,7 @@ use std::path::PathBuf;
 use tokio::sync::oneshot;
 use uuid::Uuid;
 
-use crate::index::{Document, SearchQuery, SearchResult, Settings};
+use crate::index::{Document, SearchQuery, SearchResult, Settings, Checked};
 use crate::index_controller::{Failed, IndexStats, Processed, Processing};
 
 use super::{IndexMeta, IndexResult, IndexSettings};
@@ -27,7 +27,7 @@ pub enum IndexMsg {
     },
     Settings {
         uuid: Uuid,
-        ret: oneshot::Sender<IndexResult<Settings>>,
+        ret: oneshot::Sender<IndexResult<Settings<Checked>>>,
     },
     Documents {
         uuid: Uuid,

meilisearch-http/src/index_controller/index_actor/mod.rs

@@ -14,7 +14,7 @@ pub use handle_impl::IndexActorHandleImpl;
 use message::IndexMsg;
 use store::{IndexStore, MapIndexStore};
 
-use crate::index::{Document, Index, SearchQuery, SearchResult, Settings};
+use crate::index::{Checked, Document, Index, SearchQuery, SearchResult, Settings};
 use crate::index_controller::{Failed, Processed, Processing, IndexStats};
 
 use super::IndexSettings;
@@ -74,7 +74,7 @@ pub trait IndexActorHandle {
         data: Option<File>,
     ) -> anyhow::Result<Result<Processed, Failed>>;
     async fn search(&self, uuid: Uuid, query: SearchQuery) -> IndexResult<SearchResult>;
-    async fn settings(&self, uuid: Uuid) -> IndexResult<Settings>;
+    async fn settings(&self, uuid: Uuid) -> IndexResult<Settings<Checked>>;
 
     async fn documents(
         &self,
@@ -130,7 +130,7 @@ mod test {
             self.as_ref().search(uuid, query).await
         }
 
-        async fn settings(&self, uuid: Uuid) -> IndexResult<Settings> {
+        async fn settings(&self, uuid: Uuid) -> IndexResult<Settings<Checked>> {
             self.as_ref().settings(uuid).await
         }
 

meilisearch-http/src/index_controller/mod.rs

@@ -20,7 +20,7 @@ use snapshot::{SnapshotService, load_snapshot};
 use update_actor::UpdateActorHandle;
 use uuid_resolver::{UuidError, UuidResolverHandle};
 
-use crate::index::{Settings, Document, SearchQuery, SearchResult};
+use crate::index::{Checked, Document, SearchQuery, SearchResult, Settings};
 use crate::option::Opt;
 
 mod index_actor;
@@ -202,7 +202,7 @@ impl IndexController {
     pub async fn update_settings(
         &self,
         uid: String,
-        settings: Settings,
+        settings: Settings<Checked>,
         create: bool,
     ) -> anyhow::Result<UpdateStatus> {
         let perform_udpate = |uuid| async move {
@@ -282,7 +282,7 @@ impl IndexController {
         Ok(ret)
     }
 
-    pub async fn settings(&self, uid: String) -> anyhow::Result<Settings> {
+    pub async fn settings(&self, uid: String) -> anyhow::Result<Settings<Checked>> {
         let uuid = self.uuid_resolver.get(uid.clone()).await?;
         let settings = self.index_handle.settings(uuid).await?;
         Ok(settings)

meilisearch-http/src/index_controller/updates.rs

@@ -4,7 +4,7 @@ use chrono::{DateTime, Utc};
 use milli::update::{DocumentAdditionResult, IndexDocumentsMethod, UpdateFormat};
 use serde::{Deserialize, Serialize};
 
-use crate::index::Settings;
+use crate::index::{Checked, Settings};
 
 pub type UpdateError = String;
 
@@ -25,7 +25,7 @@ pub enum UpdateMeta {
     },
     ClearDocuments,
     DeleteDocuments,
-    Settings(Settings),
+    Settings(Settings<Checked>),
 }
 
 #[derive(Debug, Serialize, Deserialize, Clone)]

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

@@ -1,6 +1,6 @@
 use actix_web::{delete, get, post, web, HttpResponse};
 
-use crate::error::ResponseError;
+use crate::{error::ResponseError, index::Unchecked};
 use crate::helpers::Authentication;
 use crate::index::Settings;
 use crate::Data;
@@ -138,10 +138,11 @@ create_services!(
 async fn update_all(
     data: web::Data<Data>,
     index_uid: web::Path<String>,
-    body: web::Json<Settings>,
+    body: web::Json<Settings<Unchecked>>,
 ) -> Result<HttpResponse, ResponseError> {
+    let settings = body.into_inner().check();
     match data
-        .update_settings(index_uid.into_inner(), body.into_inner(), true)
+        .update_settings(index_uid.into_inner(), settings, true)
         .await
     {
         Ok(update_result) => Ok(