From 941a87e8a9e42db340769df24dc679d23497f689 Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Tue, 21 Jul 2026 11:27:20 +0300 Subject: [PATCH] selector: fail safe instead of silently corrupting on ambiguous dotted keys An unquoted query segment like "Foo.Bar.Baz" is ambiguous whenever a pset is literally named "Foo.Bar" (see #6937, ifccsv spreadsheet export/import). get_element_value already returned None for the simple case, but if a shorter, coincidentally-named pset/property also existed (e.g. pset "Foo" with property "Bar"), it silently returned that unrelated value instead. set_element_value had the same problem in reverse: it silently wrote into the wrong property/pset rather than reporting the extra unresolved key. Both now fail safely (None / a raised SetElementValueException) so bad data is never produced. The documented quoted/regex query forms already round-trip correctly and are unaffected. ifccsv now catches that exception per-column so one unresolved query doesn't abort the whole import. Generated with the assistance of an AI coding tool. --- src/ifccsv/ifccsv.py | 5 ++- .../ifcopenshell/util/selector.py | 9 ++++ .../test/util/test_selector.py | 41 +++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/ifccsv/ifccsv.py b/src/ifccsv/ifccsv.py index f6d3157f81..e3009475b2 100755 --- a/src/ifccsv/ifccsv.py +++ b/src/ifccsv/ifccsv.py @@ -511,7 +511,10 @@ class IfcCsv: if any(pattern in key.lower() for pattern in SKIP_PATTERNS): continue - ifcopenshell.util.selector.set_element_value(ifc_file, element, key, value, concat=concat) + try: + ifcopenshell.util.selector.set_element_value(ifc_file, element, key, value, concat=concat) + except ifcopenshell.util.selector.SetElementValueException as e: + print("Skipping column '{}' for element {}: {}".format(key, row[0], e)) if __name__ == "__main__": diff --git a/src/ifcopenshell-python/ifcopenshell/util/selector.py b/src/ifcopenshell-python/ifcopenshell/util/selector.py index f83d3cd3d5..5d0d12a7ca 100644 --- a/src/ifcopenshell-python/ifcopenshell/util/selector.py +++ b/src/ifcopenshell-python/ifcopenshell/util/selector.py @@ -557,6 +557,9 @@ def _get_element_value(element: ifcopenshell.entity_instance, keys: list[str]) - else: results.append(subvalue) value = results + else: + # No more keys can be applied to a terminal value. + value = None return value @@ -803,6 +806,12 @@ def set_element_value( element = result elif isinstance(element, dict): # Such as from the result of a prior get_pset + if len(keys) != i + 1: + raise SetElementValueException( + f"Failed to set value '{value}' for element '{original_element}' with query '{query}': " + f"'{key}' is not the last key. If a name contains a literal '.', quote it, " + 'e.g. "Pset.Name".Property or /Pset\\.Name/.Property.' + ) pset = ifc_file.by_id(element["id"]) if isinstance(key, re.Pattern): for prop, prop_value in element.items(): diff --git a/src/ifcopenshell-python/test/util/test_selector.py b/src/ifcopenshell-python/test/util/test_selector.py index 5ee2267269..6d6c83207b 100644 --- a/src/ifcopenshell-python/test/util/test_selector.py +++ b/src/ifcopenshell-python/test/util/test_selector.py @@ -195,6 +195,27 @@ class TestGetElementValue(test.bootstrap.IFC4): assert subject.get_element_value(element, "/Pset_.*Common/.Status") == ["New"] assert subject.get_element_value(element, "/Pset_.*Common/.Status.0") == "New" + def test_selecting_a_pset_with_a_dot_in_its_name_requires_quoting(self): + # See #6937: a pset literally named "Foo.Bar" is ambiguous with an + # unquoted "Foo.Bar.Baz" query, since "." also separates keys. + element = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcWall") + pset = ifcopenshell.api.pset.add_pset(self.file, product=element, name="Foo.Bar") + ifcopenshell.api.pset.edit_pset(self.file, pset=pset, properties={"Baz": "correct"}) + + # Quoting resolves the ambiguity unambiguously. + assert subject.get_element_value(element, '"Foo.Bar".Baz') == "correct" + assert subject.get_element_value(element, r"/Foo\.Bar/.Baz") == "correct" + + # An unquoted query fails silently rather than misresolving. + assert subject.get_element_value(element, "Foo.Bar.Baz") is None + + # If a *different*, coincidentally-named pset also exists (e.g. "Foo" + # with a property "Bar"), the unquoted query must not silently return + # that unrelated value instead. + other_pset = ifcopenshell.api.pset.add_pset(self.file, product=element, name="Foo") + ifcopenshell.api.pset.edit_pset(self.file, pset=other_pset, properties={"Bar": "unrelated"}) + assert subject.get_element_value(element, "Foo.Bar.Baz") is None + class TestFilterElements(test.bootstrap.IFC4): def test_selecting_by_globalid(self): @@ -440,6 +461,26 @@ class TestSetElementValue(test.bootstrap.IFC4): subject.set_element_value(self.file, layer, "Material.Name", "Foo") assert material.Name == "Foo" + def test_setting_a_pset_with_a_dot_in_its_name_requires_quoting(self): + # See #6937: an unquoted query must not silently write into an + # unrelated, coincidentally-named pset/property. + element = ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcWall") + pset = ifcopenshell.api.pset.add_pset(self.file, product=element, name="Foo.Bar") + ifcopenshell.api.pset.edit_pset(self.file, pset=pset, properties={"Baz": "original"}) + other_pset = ifcopenshell.api.pset.add_pset(self.file, product=element, name="Foo") + ifcopenshell.api.pset.edit_pset(self.file, pset=other_pset, properties={"Bar": "original_unrelated"}) + + with pytest.raises(subject.SetElementValueException): + subject.set_element_value(self.file, element, "Foo.Bar.Baz", "new_value") + + # Neither pset should have been touched. + assert ifcopenshell.util.element.get_pset(element, "Foo.Bar")["Baz"] == "original" + assert ifcopenshell.util.element.get_pset(element, "Foo")["Bar"] == "original_unrelated" + + # The quoted form is unambiguous and works. + subject.set_element_value(self.file, element, '"Foo.Bar".Baz', "new_value") + assert ifcopenshell.util.element.get_pset(element, "Foo.Bar")["Baz"] == "new_value" + class TestSetElementValuePredefinedType(test.bootstrap.IFC4): def test_setting_an_element_predefined_type(self):