Limit field names to 255 characters

Author: guilload

quickwit-doc-mapper/src/default_doc_mapper/default_mapper.rs

@@ -162,7 +162,10 @@ impl DefaultDocMapperBuilder {
             for (field_path, field_type) in field_mapping.field_entries() {
                 let field_name = field_path.field_name();
                 if field_name == SOURCE_FIELD_NAME {
-                    bail!("`_source` is a reserved name, change your field name.");
+                    bail!(
+                        "`_source` is a reserved field name, please, use a different name for \
+                         this field."
+                    );
                 }
                 if self.tag_fields.contains(&field_name) {
                     match &field_type {
@@ -713,8 +716,10 @@ mod tests {
         }"#;
 
         let builder = serde_json::from_str::<DefaultDocMapperBuilder>(doc_mapper)?;
-        let expected_msg = "`_source` is a reserved name, change your field name.".to_string();
-        assert_eq!(builder.build().unwrap_err().to_string(), expected_msg);
+        assert_eq!(
+            builder.build().unwrap_err().to_string(),
+            "`_source` is a reserved field name, please, use a different name for this field."
+        );
         Ok(())
     }
 

quickwit-doc-mapper/src/default_doc_mapper/field_mapping_entry.rs

@@ -31,7 +31,7 @@ use tantivy::schema::{
 use thiserror::Error;
 
 use super::{default_as_true, FieldMappingType};
-use crate::default_doc_mapper::is_valid_field_mapping_name;
+use crate::default_doc_mapper::validate_field_mapping_name;
 
 /// A `FieldMappingEntry` defines how a field is indexed, stored,
 /// and mapped from a JSON document to the related index fields.
@@ -50,7 +50,7 @@ pub struct FieldMappingEntry {
 impl FieldMappingEntry {
     /// Creates a new [`FieldMappingEntry`].
     pub fn new(name: String, mapping_type: FieldMappingType) -> Self {
-        assert!(is_valid_field_mapping_name(&name));
+        assert!(validate_field_mapping_name(&name).is_ok());
         FieldMappingEntry { name, mapping_type }
     }
 
@@ -512,9 +512,7 @@ impl TryFrom<FieldMappingEntryForSerialization> for FieldMappingEntry {
                 type_str
             ),
         };
-        if !is_valid_field_mapping_name(&value.name) {
-            bail!("Invalid field name: `{}`.", value.name)
-        }
+        validate_field_mapping_name(&value.name)?;
         Ok(FieldMappingEntry::new(value.name, field_type))
     }
 }
@@ -894,17 +892,17 @@ mod tests {
 
     #[test]
     fn test_deserialize_i64_field_with_invalid_name() {
-        let result = serde_json::from_str::<FieldMappingEntry>(
+        assert!(serde_json::from_str::<FieldMappingEntry>(
             r#"
             {
                 "name": "this is not ok",
                 "type": "i64"
             }
             "#,
-        );
-        assert!(result.is_err());
-        let error = result.unwrap_err();
-        assert_eq!(error.to_string(), "Invalid field name: `this is not ok`.");
+        )
+        .unwrap_err()
+        .to_string()
+        .contains("illegal characters"));
     }
 
     #[test]
@@ -1064,35 +1062,20 @@ mod tests {
 
     #[test]
     fn test_deserialize_u64_field_with_wrong_options() {
-        let cases = vec![
-            (
+        assert_eq!(
+            serde_json::from_str::<FieldMappingEntry>(
                 r#"
             {
                 "name": "my_field_name",
                 "type": "u64",
                 "tokenizer": "basic"
-            }
-            "#,
-                "Error when parsing `my_field_name`: `record` and `tokenizer` parameters are for \
-                 text field only.",
-            ),
-            (
-                r#"
-            {
-                "name": "this is not ok",
-                "type": "i64"
-            }
-            "#,
-                "Invalid field name: `this is not ok`.",
-            ),
-        ];
-
-        for (json_str, err_str) in cases {
-            let result = serde_json::from_str::<FieldMappingEntry>(json_str);
-            assert!(result.is_err());
-            let error = result.unwrap_err();
-            assert_eq!(error.to_string(), err_str,)
-        }
+            }"#
+            )
+            .unwrap_err()
+            .to_string(),
+            "Error when parsing `my_field_name`: `record` and `tokenizer` parameters are for text \
+             field only."
+        );
     }
 
     #[test]

quickwit-doc-mapper/src/default_doc_mapper/mod.rs

@@ -21,6 +21,7 @@ mod default_mapper;
 mod field_mapping_entry;
 mod field_mapping_type;
 
+use anyhow::bail;
 use once_cell::sync::Lazy;
 use regex::Regex;
 
@@ -29,20 +30,74 @@ pub use self::field_mapping_entry::{DocParsingError, FieldMappingEntry};
 pub use self::field_mapping_type::FieldMappingType;
 
 /// Regular expression representing the restriction on a valid field name.
-pub const FIELD_MAPPING_NAME_PATTERN: &str = r#"^[_a-zA-Z][_\.\-a-zA-Z0-9]*$"#;
+pub const FIELD_MAPPING_NAME_PATTERN: &str = r#"^[_a-zA-Z][_\.\-a-zA-Z0-9]{0,254}$"#;
 
 /// Validator for a potential `field_mapping_name`.
 /// Returns true if the name can be use for a field mapping name.
 ///
 /// A field mapping name must start by a letter `[a-zA-Z]`.
 /// The other characters can be any alphanumic character `[a-ZA-Z0-9]` or `_`, `.`, `-`.
-pub fn is_valid_field_mapping_name(field_mapping_name: &str) -> bool {
+pub fn validate_field_mapping_name(field_mapping_name: &str) -> anyhow::Result<()> {
     static FIELD_MAPPING_NAME_PTN: Lazy<Regex> =
         Lazy::new(|| Regex::new(FIELD_MAPPING_NAME_PATTERN).unwrap());
-    FIELD_MAPPING_NAME_PTN.is_match(field_mapping_name)
+
+    if FIELD_MAPPING_NAME_PTN.is_match(field_mapping_name) {
+        return Ok(());
+    }
+    if field_mapping_name.is_empty() {
+        bail!("Field name is empty.");
+    }
+    if field_mapping_name.len() > 255 {
+        bail!(
+            "Field name `{}` is too long. Field names must not be longer than 255 characters.",
+            field_mapping_name
+        )
+    }
+    let first_char = field_mapping_name.chars().next().unwrap();
+    if !first_char.is_ascii_alphabetic() && first_char != '_' {
+        bail!(
+            "Field name `{}` is invalid. Field names must start with an uppercase or lowercase \
+             ASCII letter or an underscore `_`.",
+            field_mapping_name
+        )
+    }
+    bail!(
+        "Field name `{}` contains illegal characters. Field names must only contain uppercase and \
+         lowercase ASCII letters, digits, hyphens `-`, periods `.`, and underscores `_`.",
+        field_mapping_name
+    );
 }
 
 /// Function used with serde to initialize boolean value at true if there is no value in json.
 fn default_as_true() -> bool {
     true
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_validate_field_mapping_name() {
+        assert!(validate_field_mapping_name("")
+            .unwrap_err()
+            .to_string()
+            .contains("is empty"));
+        assert!(validate_field_mapping_name(&"a".repeat(256))
+            .unwrap_err()
+            .to_string()
+            .contains("is too long"));
+        assert!(validate_field_mapping_name("0")
+            .unwrap_err()
+            .to_string()
+            .contains("must start with"));
+        assert!(validate_field_mapping_name("_my-field!")
+            .unwrap_err()
+            .to_string()
+            .contains("illegal characters"));
+        assert!(validate_field_mapping_name("my-field").is_ok());
+        assert!(validate_field_mapping_name("my.field").is_ok());
+        assert!(validate_field_mapping_name("my_field").is_ok());
+        assert!(validate_field_mapping_name(&"a".repeat(255)).is_ok());
+    }
+}