From c9e8bbeaa08d07e630461150dd530c1e83dc2ff0 Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Sun, 19 Jul 2026 22:20:18 +0300 Subject: [PATCH] Fix #414: propagate file ownership when assigning instance attributes Root cause: IfcUtil::IfcBaseClass::set_attribute_value stored the raw instance pointer without checking which file owns it, and serialization writes references as the instance's own id. An instance never added to the file has id 0 and serializes as the invalid reference #0, and an instance owned by another file serializes as its id in that other file, which silently aliases an unrelated instance in the target file. The inverse reference map was corrupted the same way through register_inverse. aothms reported the unowned case in #414 in 2018 as a regression against the pre 0.5 behaviour where ownership was propagated automatically on assignment. Ownership is now propagated through the idempotent IfcFile::addEntity before the value is stored, for scalar instance attributes and both aggregate forms. An unowned instance is registered in this file in place, together with its forward references, and receives a fresh id. An instance owned by another file of the same schema is recursively copied, the same behaviour file.add() gives. A schema mismatch throws inside addEntity instead of storing a dangling reference. Same file assignments are unchanged. This extends the approach of the #486 fix (PR #8406), which adopts foreign file instances on assignment but leaves unowned instances untouched. The test_facet.py change is the same owner history settings isolation fix PR #8406 carries, needed because a cross schema user leaking from the IFC2X3 test now raises in later IFC4 tests instead of being stored silently. Generated with the assistance of an AI coding tool. --- src/ifcopenshell-python/test/test_file.py | 48 +++++++++++++++++++++++ src/ifcparse/IfcParse.cpp | 37 ++++++++++++++++- src/ifctester/test/test_facet.py | 31 +++++++++------ 3 files changed, 103 insertions(+), 13 deletions(-) diff --git a/src/ifcopenshell-python/test/test_file.py b/src/ifcopenshell-python/test/test_file.py index c2c0aa54ea..0913ad2e6f 100644 --- a/src/ifcopenshell-python/test/test_file.py +++ b/src/ifcopenshell-python/test/test_file.py @@ -198,6 +198,54 @@ class TestFile(test.bootstrap.IFC4): result = self.file.add(element) assert result.is_a() == element.is_a() + def test_assigning_an_unowned_instance_adds_it_to_the_file(self): + user = ifcopenshell.create_entity("IfcPersonAndOrganization", schema="IFC4") + owner = self.file.createIfcOwnerHistory() + owner.OwningUser = user + assert user.id() != 0 + assert self.file.by_id(user.id()) == user + assert self.file.get_inverse(user) == {owner} + assert f"#{user.id()}" in self.file.to_string() + + def test_assigning_an_unowned_instance_adds_its_references_to_the_file(self): + person = ifcopenshell.create_entity("IfcPerson", schema="IFC4") + organization = ifcopenshell.create_entity("IfcOrganization", schema="IFC4", Name="o") + user = ifcopenshell.create_entity( + "IfcPersonAndOrganization", schema="IFC4", ThePerson=person, TheOrganization=organization + ) + owner = self.file.createIfcOwnerHistory() + owner.OwningUser = user + assert self.file.by_id(person.id()) == person + assert self.file.by_id(organization.id()) == organization + assert self.file.traverse(user) == [user, person, organization] + + def test_assigning_unowned_instances_in_an_aggregate_adds_them_to_the_file(self): + role = ifcopenshell.create_entity("IfcActorRole", schema="IFC4") + person = self.file.createIfcPerson() + person.Roles = [role] + assert role.id() != 0 + assert self.file.by_id(role.id()) == role + + def test_assigning_an_instance_from_another_file_copies_it_into_the_file(self): + g = ifcopenshell.file(schema="IFC4") + other_user = g.createIfcPersonAndOrganization() + owner = self.file.createIfcOwnerHistory() + owner.OwningUser = other_user + copied = owner.OwningUser + assert copied.wrapped_data.file_pointer() != other_user.wrapped_data.file_pointer() + assert self.file.by_id(copied.id()) == copied + assert g.by_id(other_user.id()) == other_user + + def test_assigning_an_instance_of_another_schema_raises(self): + g = ifcopenshell.file(schema="IFC2X3") + other_user = g.createIfcPersonAndOrganization() + owner = self.file.createIfcOwnerHistory() + with pytest.raises(Exception): + owner.OwningUser = other_user + unowned = ifcopenshell.create_entity("IfcPersonAndOrganization", schema="IFC2X3") + with pytest.raises(Exception): + owner.OwningUser = unowned + def test_getting_elements_by_type(self): wall = self.file.createIfcWall() slab = self.file.createIfcSlab() diff --git a/src/ifcparse/IfcParse.cpp b/src/ifcparse/IfcParse.cpp index bc6df697af..221d3d46b8 100644 --- a/src/ifcparse/IfcParse.cpp +++ b/src/ifcparse/IfcParse.cpp @@ -1180,12 +1180,45 @@ IfcUtil::IfcBaseClass::set_attribute_value(size_t i, const T& t) { } { void* const storage = file_ ? std::visit([](const auto& m) { return (void*)&m; }, file_->storage_) : nullptr; + // #414 #486 Propagate ownership of instances not yet owned by this file + // through the idempotent IfcFile::addEntity: unowned instances are + // registered in place, instances from another file are copied, and a + // schema mismatch throws instead of storing a dangling reference. + auto adopt_if_not_owned = [](IfcParse::IfcFile* file, IfcUtil::IfcBaseClass* instance) -> IfcUtil::IfcBaseClass* { + if (instance != nullptr && file != nullptr && instance->file_ != file) { + return file->addEntity(instance); + } + return instance; + }; if constexpr (std::is_pointer_v) { - if (t) { - data_.set_attribute_value(storage, &declaration(), id() ? id() : identity(), i, t); + IfcUtil::IfcBaseClass* to_write = adopt_if_not_owned(file_, t); + if (to_write) { + data_.set_attribute_value(storage, &declaration(), id() ? id() : identity(), i, to_write); } else { data_.set_attribute_value(storage, &declaration(), id() ? id() : identity(), i, Blank{}); } + } else if constexpr (std::is_same_v) { + aggregate_of_instance::ptr to_write(new aggregate_of_instance); + if (t) { + to_write->reserve(t->size()); + for (auto* instance : *t) { + to_write->push(adopt_if_not_owned(file_, instance)); + } + } + data_.set_attribute_value(storage, &declaration(), id() ? id() : identity(), i, to_write); + } else if constexpr (std::is_same_v) { + aggregate_of_aggregate_of_instance::ptr to_write(new aggregate_of_aggregate_of_instance); + if (t) { + for (auto outer = t->begin(); outer != t->end(); ++outer) { + std::vector inner; + inner.reserve(outer->size()); + for (auto* instance : *outer) { + inner.push_back(adopt_if_not_owned(file_, instance)); + } + to_write->push(inner); + } + } + data_.set_attribute_value(storage, &declaration(), id() ? id() : identity(), i, to_write); } else { data_.set_attribute_value(storage, &declaration(), id() ? id() : identity(),i, t); } diff --git a/src/ifctester/test/test_facet.py b/src/ifctester/test/test_facet.py index 3c5d91a6e4..f7a9c4c3e1 100644 --- a/src/ifctester/test/test_facet.py +++ b/src/ifctester/test/test_facet.py @@ -252,19 +252,28 @@ class TestEntity: ifc, identification="AWB", name="Architects Without Ballpens" ) user = ifcopenshell.api.owner.add_person_and_organisation(ifc, person=person, organisation=organisation) - ifcopenshell.api.owner.settings.get_user = lambda x: user - ifcopenshell.api.owner.settings.get_application = lambda x: application + # The owner-history settings are module-global. Save and restore them so the + # IFC2X3 user/application created here does not leak into later IFC4 tests, + # where assigning a cross-schema instance now raises (see #486). + original_get_user = ifcopenshell.api.owner.settings.get_user + original_get_application = ifcopenshell.api.owner.settings.get_application + try: + ifcopenshell.api.owner.settings.get_user = lambda x: user + ifcopenshell.api.owner.settings.get_application = lambda x: application - element = ifcopenshell.api.root.create_entity(ifc, "IfcFlowTerminal") - element_type = ifcopenshell.api.root.create_entity(ifc, "IfcAirTerminalType") - ifcopenshell.api.type.assign_type(ifc, related_objects=[element], relating_type=element_type) - facet = Entity(name="IFCAIRTERMINAL") - assert facet.filter(ifc) == [element] - run("In IFC2X3 the type class is checked instead 1/2", facet=facet, inst=element, expected=True) + element = ifcopenshell.api.root.create_entity(ifc, "IfcFlowTerminal") + element_type = ifcopenshell.api.root.create_entity(ifc, "IfcAirTerminalType") + ifcopenshell.api.type.assign_type(ifc, related_objects=[element], relating_type=element_type) + facet = Entity(name="IFCAIRTERMINAL") + assert facet.filter(ifc) == [element] + run("In IFC2X3 the type class is checked instead 1/2", facet=facet, inst=element, expected=True) - facet = Entity(name="IFCELECTRICAPPLIANCE") - assert facet.filter(ifc) == [] - run("In IFC2X3 the type class is checked instead 2/2", facet=facet, inst=element, expected=False) + facet = Entity(name="IFCELECTRICAPPLIANCE") + assert facet.filter(ifc) == [] + run("In IFC2X3 the type class is checked instead 2/2", facet=facet, inst=element, expected=False) + finally: + ifcopenshell.api.owner.settings.get_user = original_get_user + ifcopenshell.api.owner.settings.get_application = original_get_application def test_to_string_required_applicability(self): spec = ifctester.ids.Specification(name="Foo", minOccurs=1, maxOccurs="unbounded")