diff --git a/base/src/expressions/utils/mod.rs b/base/src/expressions/utils/mod.rs index 3fee1fb..94d8aaa 100644 --- a/base/src/expressions/utils/mod.rs +++ b/base/src/expressions/utils/mod.rs @@ -211,15 +211,19 @@ pub fn parse_reference_a1(r: &str) -> Option { pub fn is_valid_identifier(name: &str) -> bool { // https://support.microsoft.com/en-us/office/names-in-formulas-fc2935f9-115d-4bef-a370-3aa8bb4c91f1 // https://github.com/MartinTrummer/excel-names/ - // NOTE: We are being much more restrictive than Excel. - // In particular we do not support non ascii characters. let upper = name.to_ascii_uppercase(); - let bytes = upper.as_bytes(); - let len = bytes.len(); + // length of chars + let len = upper.chars().count(); + + let mut chars = upper.chars(); + if len > 255 || len == 0 { return false; } - let first = bytes[0] as char; + let first = match chars.next() { + Some(ch) => ch, + None => return false, + }; // The first character of a name must be a letter, an underscore character (_), or a backslash (\). if !(first.is_ascii_alphabetic() || first == '_' || first == '\\') { return false; @@ -237,20 +241,10 @@ pub fn is_valid_identifier(name: &str) -> bool { if parse_reference_r1c1(name).is_some() { return false; } - let mut i = 1; - while i < len { - let ch = bytes[i] as char; - match ch { - 'a'..='z' => {} - 'A'..='Z' => {} - '0'..='9' => {} - '_' => {} - '.' => {} - _ => { - return false; - } + for ch in chars { + if !(ch.is_alphanumeric() || ch == '_' || ch == '.') { + return false; } - i += 1; } true diff --git a/base/src/expressions/utils/test.rs b/base/src/expressions/utils/test.rs index d53bc4e..962eb39 100644 --- a/base/src/expressions/utils/test.rs +++ b/base/src/expressions/utils/test.rs @@ -196,6 +196,7 @@ fn test_names() { assert!(is_valid_identifier("_.")); assert!(is_valid_identifier("_1")); assert!(is_valid_identifier("\\.")); + assert!(is_valid_identifier("truñe")); // invalid assert!(!is_valid_identifier("true")); @@ -209,7 +210,6 @@ fn test_names() { assert!(!is_valid_identifier("1true")); assert!(!is_valid_identifier("test€")); - assert!(!is_valid_identifier("truñe")); assert!(!is_valid_identifier("tr&ue")); assert!(!is_valid_identifier("LOG10")); diff --git a/base/src/model.rs b/base/src/model.rs index d55e95a..88c2410 100644 --- a/base/src/model.rs +++ b/base/src/model.rs @@ -2068,21 +2068,7 @@ impl Model { scope: Option, formula: &str, ) -> Result<(), String> { - if !is_valid_identifier(name) { - return Err("Invalid defined name".to_string()); - }; - let name_upper = name.to_uppercase(); - let defined_names = &self.workbook.defined_names; - let sheet_id = match scope { - Some(index) => Some(self.workbook.worksheet(index)?.sheet_id), - None => None, - }; - // if the defined name already exist return error - for df in defined_names { - if df.name.to_uppercase() == name_upper && df.sheet_id == sheet_id { - return Err("Defined name already exists".to_string()); - } - } + let sheet_id = self.is_valid_defined_name(name, scope, formula)?; self.workbook.defined_names.push(DefinedName { name: name.to_string(), formula: formula.to_string(), @@ -2093,6 +2079,48 @@ impl Model { Ok(()) } + /// Validates if a defined name can be created + pub fn is_valid_defined_name( + &self, + name: &str, + scope: Option, + formula: &str, + ) -> Result, String> { + if !is_valid_identifier(name) { + return Err("Name: Invalid defined name".to_string()); + } + let name_upper = name.to_uppercase(); + let defined_names = &self.workbook.defined_names; + let sheet_id = match scope { + Some(index) => match self.workbook.worksheet(index) { + Ok(ws) => Some(ws.sheet_id), + Err(_) => return Err("Scope: Invalid sheet index".to_string()), + }, + None => None, + }; + // if the defined name already exist return error + for df in defined_names { + if df.name.to_uppercase() == name_upper && df.sheet_id == sheet_id { + return Err("Name: Defined name already exists".to_string()); + } + } + + // Make sure the formula is valid + match common::ParsedReference::parse_reference_formula( + None, + formula, + &self.locale, + |name| self.get_sheet_index_by_name(name), + ) { + Ok(_) => {} + Err(_) => { + return Err("Formula: Invalid defined name formula".to_string()); + } + }; + + Ok(sheet_id) + } + /// Delete defined name of name and scope pub fn delete_defined_name(&mut self, name: &str, scope: Option) -> Result<(), String> { let name_upper = name.to_uppercase(); @@ -2126,7 +2154,7 @@ impl Model { new_formula: &str, ) -> Result<(), String> { if !is_valid_identifier(new_name) { - return Err("Invalid defined name".to_string()); + return Err("Name: Invalid defined name".to_string()); }; let name_upper = name.to_uppercase(); let new_name_upper = new_name.to_uppercase(); @@ -2134,18 +2162,28 @@ impl Model { if name_upper != new_name_upper || scope != new_scope { for key in self.parsed_defined_names.keys() { if key.1.to_uppercase() == new_name_upper && key.0 == new_scope { - return Err("Defined name already exists".to_string()); + return Err("Name: Defined name already exists".to_string()); } } } let defined_names = &self.workbook.defined_names; let sheet_id = match scope { - Some(index) => Some(self.workbook.worksheet(index)?.sheet_id), + Some(index) => Some( + self.workbook + .worksheet(index) + .map_err(|_| "Scope: Invalid sheet index")? + .sheet_id, + ), None => None, }; let new_sheet_id = match new_scope { - Some(index) => Some(self.workbook.worksheet(index)?.sheet_id), + Some(index) => Some( + self.workbook + .worksheet(index) + .map_err(|_| "Scope: Invalid sheet index")? + .sheet_id, + ), None => None, }; diff --git a/base/src/test/user_model/test_defined_names.rs b/base/src/test/user_model/test_defined_names.rs index 60619ec..542137c 100644 --- a/base/src/test/user_model/test_defined_names.rs +++ b/base/src/test/user_model/test_defined_names.rs @@ -254,19 +254,19 @@ fn invalid_names() { // spaces assert_eq!( model.new_defined_name("A real", None, "Sheet1!$A$1"), - Err("Invalid defined name".to_string()) + Err("Name: Invalid defined name".to_string()) ); // Starts with number assert_eq!( model.new_defined_name("2real", None, "Sheet1!$A$1"), - Err("Invalid defined name".to_string()) + Err("Name: Invalid defined name".to_string()) ); // Updating also fails assert_eq!( model.update_defined_name("MyName", None, "My Name", None, "Sheet1!$A$1"), - Err("Invalid defined name".to_string()) + Err("Name: Invalid defined name".to_string()) ); } @@ -284,13 +284,13 @@ fn already_existing() { // Can't create a new name with the same name assert_eq!( model.new_defined_name("MyName", None, "Sheet1!$A$2"), - Err("Defined name already exists".to_string()) + Err("Name: Defined name already exists".to_string()) ); // Can't update one into an existing assert_eq!( model.update_defined_name("Another", None, "MyName", None, "Sheet1!$A$1"), - Err("Defined name already exists".to_string()) + Err("Name: Defined name already exists".to_string()) ); } @@ -304,17 +304,17 @@ fn invalid_sheet() { assert_eq!( model.new_defined_name("Mything", Some(2), "Sheet1!$A$1"), - Err("Invalid sheet index".to_string()) + Err("Scope: Invalid sheet index".to_string()) ); assert_eq!( model.update_defined_name("MyName", None, "MyName", Some(2), "Sheet1!$A$1"), - Err("Invalid sheet index".to_string()) + Err("Scope: Invalid sheet index".to_string()) ); assert_eq!( model.update_defined_name("MyName", Some(9), "YourName", None, "Sheet1!$A$1"), - Err("Invalid sheet index".to_string()) + Err("General: Failed to get old name".to_string()) ); } @@ -322,7 +322,7 @@ fn invalid_sheet() { fn invalid_formula() { let mut model = UserModel::new_empty("model", "en", "UTC").unwrap(); model.set_user_input(0, 1, 1, "Hello").unwrap(); - model.new_defined_name("MyName", None, "A1").unwrap(); + assert!(model.new_defined_name("MyName", None, "A1").is_err()); model.set_user_input(0, 1, 2, "=MyName").unwrap(); diff --git a/base/src/user_model/common.rs b/base/src/user_model/common.rs index b6520f6..444a9a5 100644 --- a/base/src/user_model/common.rs +++ b/base/src/user_model/common.rs @@ -2001,7 +2001,10 @@ impl UserModel { new_scope: Option, new_formula: &str, ) -> Result<(), String> { - let old_formula = self.model.get_defined_name_formula(name, scope)?; + let old_formula = self + .model + .get_defined_name_formula(name, scope) + .map_err(|_| "General: Failed to get old name")?; let diff_list = vec![Diff::UpdateDefinedName { name: name.to_string(), scope, @@ -2017,6 +2020,16 @@ impl UserModel { Ok(()) } + /// validates a new defined name + pub fn is_valid_defined_name( + &self, + name: &str, + scope: Option, + formula: &str, + ) -> Result, String> { + self.model.is_valid_defined_name(name, scope, formula) + } + // **** Private methods ****** // pub(crate) fn push_diff_list(&mut self, diff_list: DiffList) { diff --git a/bindings/wasm/src/lib.rs b/bindings/wasm/src/lib.rs index d0481b9..3d61aae 100644 --- a/bindings/wasm/src/lib.rs +++ b/bindings/wasm/src/lib.rs @@ -775,4 +775,17 @@ impl Model { .get_first_non_empty_in_row_after_column(sheet, row, column) .map_err(to_js_error) } + + #[wasm_bindgen(js_name = "isValidDefinedName")] + pub fn is_valid_defined_name( + &self, + name: &str, + scope: Option, + formula: &str, + ) -> Result<(), JsError> { + match self.model.is_valid_defined_name(name, scope, formula) { + Ok(_) => Ok(()), + Err(e) => Err(to_js_error(e.to_string())), + } + } } diff --git a/webapp/IronCalc/src/components/RightDrawer/NamedRanges/EditNamedRange.tsx b/webapp/IronCalc/src/components/RightDrawer/NamedRanges/EditNamedRange.tsx index 6af5f0b..4e74777 100644 --- a/webapp/IronCalc/src/components/RightDrawer/NamedRanges/EditNamedRange.tsx +++ b/webapp/IronCalc/src/components/RightDrawer/NamedRanges/EditNamedRange.tsx @@ -83,7 +83,7 @@ function EditNamedRange({ } else { // Check for duplicates only if format is valid const scopeIndex = worksheets.findIndex((s) => s.name === scope); - const newScope = scopeIndex >= 0 ? scopeIndex : null; + const newScope = scopeIndex >= 0 ? scopeIndex : undefined; const existing = definedNameList.find( (dn) => dn.name === trimmed && @@ -99,6 +99,7 @@ function EditNamedRange({ } setNameError(error); + setFormulaError(""); }, [name, scope, definedNameList, editingDefinedName, worksheets]); const hasAnyError = nameError !== "" || formulaError !== "";