From 8c434b71673e57564dd4815b1708e309ff8c873f Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Mon, 20 Jul 2026 15:54:59 +0300 Subject: [PATCH] ifcopenshell.util.element: dedupe SET-typed attributes in replace_attribute replace_attribute() rewrites references inside aggregate attributes via element.walk(), but never checked whether the replacement value was already present elsewhere in the same aggregate. For an EXPRESS SET (e.g. IfcProject.RepresentationContexts, IfcRelAggregates.RelatedObjects) this can leave the same reference listed twice, which is invalid IFC. LIST and BAG aggregates legitimately allow duplicates, so a blanket dedup would be wrong; only SET-typed attributes are deduplicated, determined at runtime from the schema declaration (IfcOpenShell#8706 review comment). The SET/LIST/BAG check is cached per (schema, class, attribute index), and the dedup pass itself only runs when a cheap linear pre-check finds the replacement value already present in the aggregate, so the common case (no duplicate produced) pays only that pre-check, not a hash-set rebuild. Benchmarked against a 23MB (431k entities) and a 104MB (2.4M entities) IFC model against a large SET attribute: worst case adds well under 1ms per call; the realistic case (merging duplicate contexts, matching the PR #8706 scenario) shows no measurable regression. Fixes the root cause flagged in IfcOpenShell#8706 (Moult), obviating the need for MergeDuplicateContexts' own manual aggregate-dedup pass for that scenario. Generated with the assistance of an AI coding tool. --- .../ifcopenshell/util/element.py | 30 ++++++++++++++++++- .../test/util/test_element.py | 16 ++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/ifcopenshell-python/ifcopenshell/util/element.py b/src/ifcopenshell-python/ifcopenshell/util/element.py index cf6c52e37d..dc05059ec4 100644 --- a/src/ifcopenshell-python/ifcopenshell/util/element.py +++ b/src/ifcopenshell-python/ifcopenshell/util/element.py @@ -16,12 +16,14 @@ # You should have received a copy of the GNU Lesser General Public License # along with IfcOpenShell. If not, see . +import functools from collections import namedtuple from collections.abc import Callable, Generator, Sequence from typing import Any, Literal, Optional, Union, overload import ifcopenshell import ifcopenshell.guid +import ifcopenshell.ifcopenshell_wrapper import ifcopenshell.util.element import ifcopenshell.util.representation @@ -1590,10 +1592,36 @@ def replace_element(element: ifcopenshell.entity_instance, replacement: ifcopens replace_attribute(inverse, element, replacement) +@functools.cache +def _is_set_attribute(schema_identifier: str, ifc_class: str, index: int) -> bool: + """Whether attribute `index` of `ifc_class` is declared as an EXPRESS SET. + + SET aggregates may not contain duplicate members, whereas LIST and BAG + aggregates may, so only SET-typed attributes are safe to deduplicate. + """ + declaration = ifcopenshell.ifcopenshell_wrapper.schema_by_name(schema_identifier).declaration_by_name(ifc_class) + attribute = declaration.attribute_by_index(index) + aggregation = attribute.type_of_attribute().as_aggregation_type() + return aggregation is not None and aggregation.type_of_aggregation_string() == "set" + + def replace_attribute(element: ifcopenshell.entity_instance, old: Any, new: Any) -> None: for i, attribute_value in enumerate(element): if has_element_reference(attribute_value, old): - element[i] = element.walk(lambda v: v == old, lambda v: new, attribute_value) + new_value = element.walk(lambda v: v == old, lambda v: new, attribute_value) + if ( + isinstance(attribute_value, tuple) + and has_element_reference(attribute_value, new) + and _is_set_attribute(element.file.schema_identifier, element.is_a(), i) + ): + seen: set[Any] = set() + deduplicated = [] + for v in new_value: + if v not in seen: + seen.add(v) + deduplicated.append(v) + new_value = tuple(deduplicated) + element[i] = new_value def has_element_reference(value: Any, element: ifcopenshell.entity_instance) -> bool: diff --git a/src/ifcopenshell-python/test/util/test_element.py b/src/ifcopenshell-python/test/util/test_element.py index ffd5789f01..9ec355f7fe 100644 --- a/src/ifcopenshell-python/test/util/test_element.py +++ b/src/ifcopenshell-python/test/util/test_element.py @@ -1244,6 +1244,22 @@ class TestReplaceAttributeIFC4(test.bootstrap.IFC4): subject.replace_attribute(rel, old, new) assert rel.RelatedObjects == (new,) + def test_replacing_into_a_set_deduplicates_the_survivor(self): + old = self.file.createIfcWall() + new = self.file.createIfcWall() + rel = self.file.createIfcRelAggregates() + rel.RelatedObjects = [old, new] + subject.replace_attribute(rel, old, new) + assert rel.RelatedObjects == (new,) + + def test_replacing_into_a_list_keeps_legitimate_duplicates(self): + p1 = self.file.createIfcCartesianPoint((0.0, 0.0, 0.0)) + p2 = self.file.createIfcCartesianPoint((1.0, 0.0, 0.0)) + p3 = self.file.createIfcCartesianPoint((2.0, 0.0, 0.0)) + polyline = self.file.createIfcPolyline([p1, p2, p3, p1]) + subject.replace_attribute(polyline, p2, p3) + assert polyline.Points == (p1, p3, p3, p1) + class TestHasElementReferenceIFC4(test.bootstrap.IFC4): def test_if_a_element_attribute_references_another_element(self):