Address review comments

quickwit/quickwit-cli/src/index.rs

@@ -81,7 +81,7 @@ pub fn build_index_command() -> Command {
             Command::new("update")
             .display_order(1)
             .about("Updates an index using an index config file.")
-            .long_about("This command follows PUT semantics, which means that all the fields of the current configuration are replaced by the values specified in this request or the associated defaults. In particular if the field is optional (e.g `retention_policy`), omitting it will delete the associated configuration. If the new configuration file contains updates that cannot be applied, the request fails and none of the updates are applied.")
+            .long_about("This command follows PUT semantics, which means that all the fields of the current configuration are replaced by the values specified in this request or the associated defaults. In particular, if the field is optional (e.g. `retention_policy`), omitting it will delete the associated configuration. If the new configuration file contains updates that cannot be applied, the request fails, and none of the updates are applied.")
             .args(&[
                 arg!(--index <INDEX> "ID of the target index")
                     .display_order(1)
@@ -541,8 +541,13 @@ pub async fn update_index_cli(args: UpdateIndexArgs) -> anyhow::Result<()> {
     println!("❯ Updating index...");
     let storage_resolver = StorageResolver::unconfigured();
     let file_content = load_file(&storage_resolver, &args.index_config_uri).await?;
-    let index_config_str: String = std::str::from_utf8(&file_content)
-        .with_context(|| format!("Invalid utf8: `{}`", args.index_config_uri))?
+    let index_config_str = std::str::from_utf8(&file_content)
+        .with_context(|| {
+            format!(
+                "index config file `{}` contains some invalid UTF-8 characters",
+                args.index_config_uri
+            )
+        })?
         .to_string();
     let config_format = ConfigFormat::sniff_from_uri(&args.index_config_uri)?;
     let qw_client = args.client_args.client();

quickwit/quickwit-cli/tests/helpers.rs

@@ -82,11 +82,9 @@ const DEFAULT_INDEX_CONFIG: &str = r#"
 "#;
 
 const RETENTION_CONFIG: &str = r#"
-
     retention:
       period: 1 week
       schedule: daily
-
 "#;
 
 const DEFAULT_QUICKWIT_CONFIG: &str = r#"

quickwit/quickwit-config/src/index_config/mod.rs

@@ -37,7 +37,7 @@ use quickwit_doc_mapper::{
 };
 use quickwit_proto::types::IndexId;
 use serde::{Deserialize, Serialize};
-pub use serialize::load_index_config_from_user_config;
+pub use serialize::{load_index_config_from_user_config, load_index_config_update};
 use tracing::warn;
 
 use crate::index_config::serialize::VersionedIndexConfig;

quickwit/quickwit-config/src/index_config/serialize.rs

@@ -17,7 +17,7 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with this program. If not, see <http://www.gnu.org/licenses/>.
 
-use anyhow::Context;
+use anyhow::{bail, Context};
 use quickwit_common::uri::Uri;
 use quickwit_proto::types::IndexId;
 use serde::{Deserialize, Serialize};
@@ -64,6 +64,50 @@ pub fn load_index_config_from_user_config(
     index_config_for_serialization.build_and_validate(Some(default_index_root_uri))
 }
 
+/// Parses and validates an [`IndexConfig`] update. Ensures that the new
+/// configuration is valid in itself and compared to the current index config.
+/// If the new configuration omits some fields, the default values will be used,
+/// not those of the current index config. The only exception is the index_uri
+/// because it cannot be updated.
+pub fn load_index_config_update(
+    config_format: ConfigFormat,
+    index_config_bytes: &[u8],
+    current_index_config: &IndexConfig,
+) -> anyhow::Result<IndexConfig> {
+    let current_index_parent_dir = &current_index_config
+        .index_uri
+        .parent()
+        .context("Unexpected index_uri format on current configuration")?;
+
+    let new_index_config = load_index_config_from_user_config(
+        config_format,
+        &index_config_bytes,
+        current_index_parent_dir,
+    )?;
+
+    if current_index_config.index_id != new_index_config.index_id {
+        bail!(
+            "`index_id` in config file {} does not match updated index_id {}",
+            current_index_config.index_id,
+            new_index_config.index_id
+        );
+    }
+
+    if current_index_config.index_uri != new_index_config.index_uri {
+        bail!(
+            "`index_uri` cannot be updated, current value {}, new expected value {}",
+            current_index_config.index_uri,
+            new_index_config.index_uri
+        );
+    }
+
+    if current_index_config.doc_mapping != new_index_config.doc_mapping {
+        bail!("`doc_mapping` cannot be updated");
+    }
+
+    Ok(new_index_config)
+}
+
 impl IndexConfigForSerialization {
     fn index_uri_or_fallback_to_default(
         &self,
@@ -249,4 +293,155 @@ mod test {
             assert_eq!(index_config.index_uri.as_str(), "s3://mybucket/hdfs-logs");
         }
     }
+
+    #[test]
+    fn test_update_index_root_uri() {
+        let original_config_yaml = r#"
+            version: 0.8
+            index_id: hdfs-logs
+            doc_mapping: {}
+        "#;
+        let original_config: IndexConfig = load_index_config_from_user_config(
+            ConfigFormat::Yaml,
+            original_config_yaml.as_bytes(),
+            &Uri::for_test("s3://mybucket"),
+        )
+        .unwrap();
+        {
+            // use default in update
+            let updated_config_yaml = r#"
+                version: 0.8
+                index_id: hdfs-logs
+                doc_mapping: {}
+            "#;
+            let updated_config = load_index_config_update(
+                ConfigFormat::Yaml,
+                updated_config_yaml.as_bytes(),
+                &original_config,
+            )
+            .unwrap();
+            assert_eq!(updated_config.index_uri.as_str(), "s3://mybucket/hdfs-logs");
+        }
+        {
+            // use the current index_uri explicitely
+            let updated_config_yaml = r#"
+                version: 0.8
+                index_id: hdfs-logs
+                index_uri: s3://mybucket/hdfs-logs
+                doc_mapping: {}
+            "#;
+            let updated_config = load_index_config_update(
+                ConfigFormat::Yaml,
+                updated_config_yaml.as_bytes(),
+                &original_config,
+            )
+            .unwrap();
+            assert_eq!(updated_config.index_uri.as_str(), "s3://mybucket/hdfs-logs");
+        }
+        {
+            // try using a different index_uri
+            let updated_config_yaml = r#"
+                version: 0.8
+                index_id: hdfs-logs
+                index_uri: s3://mybucket/new-directory/
+                doc_mapping: {}
+            "#;
+            let load_error = load_index_config_update(
+                ConfigFormat::Yaml,
+                updated_config_yaml.as_bytes(),
+                &original_config,
+            )
+            .unwrap_err();
+            assert!(format!("{:?}", load_error).contains("`index_uri` cannot be updated"));
+        }
+    }
+
+    #[test]
+    fn test_update_reset_defaults() {
+        let original_config_yaml = r#"
+            version: 0.8
+            index_id: hdfs-logs
+            doc_mapping:
+                field_mappings:
+                    - name: timestamp
+                      type: datetime
+                      fast: true
+                timestamp_field: timestamp
+
+            search_settings:
+                default_search_fields: [body]
+
+            indexing_settings:
+                commit_timeout_secs: 10
+
+            retention:
+                period: 90 days
+                schedule: daily
+        "#;
+        let original_config: IndexConfig = load_index_config_from_user_config(
+            ConfigFormat::Yaml,
+            original_config_yaml.as_bytes(),
+            &Uri::for_test("s3://mybucket"),
+        )
+        .unwrap();
+
+        let updated_config_yaml = r#"
+            version: 0.8
+            index_id: hdfs-logs
+            doc_mapping:
+                field_mappings:
+                    - name: timestamp
+                      type: datetime
+                      fast: true
+                timestamp_field: timestamp
+        "#;
+        let updated_config = load_index_config_update(
+            ConfigFormat::Yaml,
+            updated_config_yaml.as_bytes(),
+            &original_config,
+        )
+        .unwrap();
+        assert_eq!(
+            updated_config.search_settings.default_search_fields,
+            Vec::<String>::default(),
+        );
+        assert_eq!(
+            updated_config.indexing_settings.commit_timeout_secs,
+            IndexingSettings::default_commit_timeout_secs()
+        );
+        assert_eq!(updated_config.retention_policy_opt, None);
+    }
+
+    #[test]
+    fn test_update_doc_mappings() {
+        let original_config_yaml = r#"
+            version: 0.8
+            index_id: hdfs-logs
+            doc_mapping: {}
+        "#;
+        let original_config: IndexConfig = load_index_config_from_user_config(
+            ConfigFormat::Yaml,
+            original_config_yaml.as_bytes(),
+            &Uri::for_test("s3://mybucket"),
+        )
+        .unwrap();
+
+        let updated_config_yaml = r#"
+            version: 0.8
+            index_id: hdfs-logs
+            doc_mapping:
+                field_mappings:
+                    - name: body
+                      type: text
+                      tokenizer: default
+                      record: position
+        "#;
+        let load_error = load_index_config_update(
+            ConfigFormat::Yaml,
+            updated_config_yaml.as_bytes(),
+            &original_config,
+        )
+        .unwrap_err();
+        assert!(format!("{:?}", load_error).contains("`doc_mapping` cannot be updated"));
+    }
 }

quickwit/quickwit-config/src/lib.rs

@@ -48,8 +48,8 @@ pub use cluster_config::ClusterConfig;
 // See #2048
 use index_config::serialize::{IndexConfigV0_8, VersionedIndexConfig};
 pub use index_config::{
-    build_doc_mapper, load_index_config_from_user_config, DocMapping, IndexConfig,
-    IndexingResources, IndexingSettings, RetentionPolicy, SearchSettings,
+    build_doc_mapper, load_index_config_from_user_config, load_index_config_update, DocMapping,
+    IndexConfig, IndexingResources, IndexingSettings, RetentionPolicy, SearchSettings,
 };
 use serde::de::DeserializeOwned;
 use serde::Serialize;

quickwit/quickwit-control-plane/src/model/mod.rs

@@ -194,7 +194,7 @@ impl ControlPlaneModel {
         self.update_metrics();
     }
 
-    /// Update the configuration of the specified index, returning an error if
+    /// Updates the configuration of the specified index, returning an error if
     /// the index didn't exist.
     pub(crate) fn update_index_config(
         &mut self,

quickwit/quickwit-metastore/src/tests/index.rs

@@ -159,11 +159,11 @@ pub async fn test_metastore_update_search_settings<
         setup_metastore_for_update::<MetastoreToTest>().await;
 
     for loop_search_settings in [
-        vec![],
-        vec!["body".to_owned()],
-        vec!["body".to_owned()],
-        vec!["body".to_owned(), "owner".to_owned()],
-        vec![],
+        Vec::new(),
+        vec!["body".to_string()],
+        vec!["body".to_string()],
+        vec!["body".to_string(), "owner".to_string()],
+        Vec::new(),
     ] {
         let index_update = UpdateIndexRequest::try_from_updates(
             index_uid.clone(),

quickwit/quickwit-rest-client/src/rest_client.rs

@@ -331,12 +331,12 @@ impl<'a> IndexClient<'a> {
 
     pub async fn create(
         &self,
-        index_config: impl ToString,
+        index_config: impl AsRef<[u8]>,
         config_format: ConfigFormat,
         overwrite: bool,
     ) -> Result<IndexMetadata, Error> {
         let header_map = header_from_config_format(config_format);
-        let body = Bytes::from(index_config.to_string());
+        let body = Bytes::copy_from_slice(index_config.as_ref());
         let response = self
             .transport
             .send(
@@ -355,11 +355,11 @@ impl<'a> IndexClient<'a> {
     pub async fn update(
         &self,
         index_id: &str,
-        index_config: impl ToString,
+        index_config: impl AsRef<[u8]>,
         config_format: ConfigFormat,
     ) -> Result<IndexMetadata, Error> {
         let header_map = header_from_config_format(config_format);
-        let body = Bytes::from(index_config.to_string());
+        let body = Bytes::copy_from_slice(index_config.as_ref());
         let path = format!("indexes/{index_id}");
         let response = self
             .transport
@@ -497,11 +497,11 @@ impl<'a> SourceClient<'a> {
 
     pub async fn create(
         &self,
-        source_config_input: impl ToString,
+        source_config_input: impl AsRef<[u8]>,
         config_format: ConfigFormat,
     ) -> Result<SourceConfig, Error> {
         let header_map = header_from_config_format(config_format);
-        let source_config_bytes: Bytes = Bytes::from(source_config_input.to_string());
+        let source_config_bytes = Bytes::copy_from_slice(source_config_input.as_ref());
         let response = self
             .transport
             .send::<()>(