From 1fa5eebf0cfe213de6b68101b79736da096497a3 Mon Sep 17 00:00:00 2001 From: Andrej730 Date: Wed, 2 Sep 2026 18:30:41 +0500 Subject: [PATCH] sql, stream: raise for accessing derived attributes Raising `AttributeError` is definitely wasn't correct here, since it might push the code to assume it's a wrong entity type. Returning some stub value like `None` also could suggest incorrect derived attribute value, leading to unexpected behaviour. So adding an error, so it would propagate and code would need to be adjusted not to rely on derived attributes, if it actually interacts with sql/stream. `test_unit` was asserting that derived attr will return `None`, though it was actually raising `AttributeError`. --- src/ifcopenshell-python/ifcopenshell/sql.py | 6 +++--- src/ifcopenshell-python/ifcopenshell/stream.py | 6 +++++- src/ifcopenshell-python/test/test_sql.py | 7 +++++++ src/ifcopenshell-python/test/test_stream.py | 9 +++++++++ src/ifcopenshell-python/test/util/test_unit.py | 9 +++------ 5 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/ifcopenshell-python/ifcopenshell/sql.py b/src/ifcopenshell-python/ifcopenshell/sql.py index 0867a9838e..29c08f3a44 100644 --- a/src/ifcopenshell-python/ifcopenshell/sql.py +++ b/src/ifcopenshell-python/ifcopenshell/sql.py @@ -380,9 +380,9 @@ class sqlite_entity: INVALID, FORWARD, INVERSE, DERIVED = range(4) attr_cat = self.wrapped_data.get_attribute_category(name) if attr_cat == DERIVED: - # Derived attributes aren't stored or computed for SQLite-linked - # files, so callers must treat None as "not available" here. - return None + raise RuntimeError( + f"Derived attributes (e.g. '{name}') are not supported for {type(self).__name__} entities." + ) if attr_cat == FORWARD: if self.sqlite_wrapper.attribute_cache: return self.sqlite_wrapper.attribute_cache[name] diff --git a/src/ifcopenshell-python/ifcopenshell/stream.py b/src/ifcopenshell-python/ifcopenshell/stream.py index 2898f28c8a..8eb6afbcac 100644 --- a/src/ifcopenshell-python/ifcopenshell/stream.py +++ b/src/ifcopenshell-python/ifcopenshell/stream.py @@ -360,8 +360,12 @@ try: assert False, "Not supported during streaming." def __getattr__(self, name: str) -> Any: - INVALID, FORWARD, INVERSE = range(3) + INVALID, FORWARD, INVERSE, DERIVED = range(4) attr_cat = self.wrapped_data.get_attribute_category(name) + if attr_cat == DERIVED: + raise RuntimeError( + f"Derived attributes (e.g. '{name}') are not supported for {type(self).__name__} entities." + ) if attr_cat == FORWARD: if self.stream_wrapper.attribute_cache: return self.stream_wrapper.attribute_cache[name] diff --git a/src/ifcopenshell-python/test/test_sql.py b/src/ifcopenshell-python/test/test_sql.py index 39cf4c2583..3f3f39aa8f 100644 --- a/src/ifcopenshell-python/test/test_sql.py +++ b/src/ifcopenshell-python/test/test_sql.py @@ -19,6 +19,7 @@ import tempfile from pathlib import Path +import pytest from ifcpatch.recipes import Ifc2Sql import ifcopenshell @@ -63,6 +64,12 @@ class TestEntity: assert (element := ifc_sqlite.by_id(1)) assert element.Name == "My Project" + def test_getattr_derive(self): + ifc_sqlite = get_ifc_sqlite() + assert (units := ifc_sqlite.by_type("IfcSIUnit")) + with pytest.raises(RuntimeError): + units[0].Dimensions + def test_setattr(self): ifc_sqlite = get_ifc_sqlite() assert (element := ifc_sqlite.by_id(1)) diff --git a/src/ifcopenshell-python/test/test_stream.py b/src/ifcopenshell-python/test/test_stream.py index 754fcc4527..b630054556 100644 --- a/src/ifcopenshell-python/test/test_stream.py +++ b/src/ifcopenshell-python/test/test_stream.py @@ -18,6 +18,8 @@ from pathlib import Path +import pytest + import ifcopenshell TEST_FILE = Path(__file__).parent / "files" / "basic.ifc" @@ -48,3 +50,10 @@ class TestEntity: stream_file = ifcopenshell.open(TEST_FILE, should_stream=True) assert (element := stream_file.by_id(1)) assert element.Name == "My Project" + + def test_getattr_derive(self): + stream_file: ifcopenshell.stream + stream_file = ifcopenshell.open(TEST_FILE, should_stream=True) + assert (units := stream_file.by_type("IfcSIUnit")) + with pytest.raises(RuntimeError): + units[0].Dimensions diff --git a/src/ifcopenshell-python/test/util/test_unit.py b/src/ifcopenshell-python/test/util/test_unit.py index c1e902583f..aaca7eb600 100644 --- a/src/ifcopenshell-python/test/util/test_unit.py +++ b/src/ifcopenshell-python/test/util/test_unit.py @@ -329,11 +329,9 @@ class TestCalculateUnitScaleOnLinkedFile(test.bootstrap.IFC4): def test_run(self): # Regression test: IfcSIUnit.Dimensions is a schema-*derived* # attribute that isn't computed for SQLite-linked files (used for - # Bonsai's "linked project" large-model workflow), so it returns None - # there instead of an IfcDimensionalExponents entity. calculate_unit_scale() - # used to access unit.Dimensions.LengthExponent unconditionally for - # every IfcSIUnit, which crashed project loading for any linked file. - # See the PR discussion for a standalone reproduction script. + # Bonsai's "linked project" large-model workflow) and raises there + # instead of returning an IfcDimensionalExponents entity. + # Test ensures `calculate_unit_scale` doesn't rely on derived attribute computation. ifcopenshell.api.root.create_entity(self.file, ifc_class="IfcProject") length = ifcopenshell.api.unit.add_si_unit(self.file, unit_type="LENGTHUNIT") ifcopenshell.api.unit.assign_unit(self.file, units=[length]) @@ -345,7 +343,6 @@ class TestCalculateUnitScaleOnLinkedFile(test.bootstrap.IFC4): try: linked_file = ifcopenshell.open(str(tmp_file)) - assert linked_file.by_type("IfcSIUnit")[0].Dimensions is None assert subject.calculate_unit_scale(linked_file, "LENGTHUNIT") == 1.0 finally: if isinstance(linked_file, ifcopenshell.sqlite):