From: Mike Bayer Date: Thu, 28 May 2026 14:25:39 +0000 (-0400) Subject: allow backref named 'metadata' to not break _metadata_for_cls X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=96f98a9119d79b4f6e6026249a4afcf3b4b98c8e;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git allow backref named 'metadata' to not break _metadata_for_cls Fixed regression caused by :ticket:`8068` where a ``backref`` named ``'metadata'`` on a mapped class would cause an ``AssertionError`` when the class also used string-based relationship references (e.g. ``secondary="some_table"``). The ``_metadata_for_cls()`` helper now checks ``isinstance(meta, MetaData)`` as a condition rather than asserting, falling back to ``registry.metadata`` when the class attribute has been overwritten by a backref. A warning is now emitted when a Declarative attribute name is named ``metadata`` or ``registry``. Previously, no warning was emitted for ``registry``, and using the name ``metadata`` would raise an InvalidRequestError. Since these names can be used for attributes that are mapped as backrefs or using imperative mappings, usage under Declarative has been relaxed for ``metadata`` but also warns for both names as they may have unintended interactions with the Declarative reserved names. References: https://bugs.launchpad.net/nova/+bug/2154165 References: https://github.com/sqlalchemy/sqlalchemy/discussions/8619 Fixes: #13333 Change-Id: I0f3173bce9c8c8881bd6b8ea75102bb8ec92be8b --- diff --git a/doc/build/changelog/unreleased_21/13333.rst b/doc/build/changelog/unreleased_21/13333.rst new file mode 100644 index 0000000000..53cf3c9996 --- /dev/null +++ b/doc/build/changelog/unreleased_21/13333.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, declarative + :tickets: 13333 + + A warning is now emitted when a Declarative attribute name is named + ``metadata`` or ``registry``. Previously, no warning was emitted for + ``registry``, and using the name ``metadata`` would raise an + InvalidRequestError. Since these names can be used for attributes + that are mapped as backrefs or using imperative mappings, usage + under Declarative has been relaxed for ``metadata`` but also warns + for both names as they may have unintended interactions with the + Declarative reserved names. diff --git a/lib/sqlalchemy/orm/decl_base.py b/lib/sqlalchemy/orm/decl_base.py index cf81513653..46d7786aec 100644 --- a/lib/sqlalchemy/orm/decl_base.py +++ b/lib/sqlalchemy/orm/decl_base.py @@ -1523,6 +1523,16 @@ class _DeclarativeMapperConfig(_MapperConfig, _ClassScanAbstractConfig): value = SynonymProperty(value.key) setattr(cls, k, value) + if k in ("metadata", "registry"): + noun = { + "metadata": "MetaData instance", + "registry": "Declarative class registry", + } + util.warn( + f"Attribute name '{k}' should be left reserved for the " + f"{noun[k]} when using a Declarative class." + ) + if ( isinstance(value, tuple) and len(value) == 1 @@ -1550,16 +1560,6 @@ class _DeclarativeMapperConfig(_MapperConfig, _ClassScanAbstractConfig): if not late_mapped: setattr(cls, k, value) continue - # we expect to see the name 'metadata' in some valid cases; - # however at this point we see it's assigned to something trying - # to be mapped, so raise for that. - # TODO: should "registry" here be also? might be too late - # to change that now (2.0 betas) - elif k in ("metadata",): - raise exc.InvalidRequestError( - f"Attribute name '{k}' is reserved when using the " - "Declarative API." - ) elif isinstance(value, Column): _undefer_column_name( k, self.column_copies.get(value, value) # type: ignore diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index e99e55fe7d..f333802622 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -256,8 +256,7 @@ class CascadeOptions(FrozenSet[str]): def _metadata_for_cls(cls: Type[Any], registry: RegistryType) -> MetaData: meta = getattr(cls, "metadata", None) - if meta is not None: - assert isinstance(meta, MetaData) + if meta is not None and isinstance(meta, MetaData): return meta return registry.metadata diff --git a/test/orm/declarative/test_basic.py b/test/orm/declarative/test_basic.py index 60cd3f14ae..b1164d24ee 100644 --- a/test/orm/declarative/test_basic.py +++ b/test/orm/declarative/test_basic.py @@ -659,12 +659,16 @@ class DeclarativeBaseSetupsTest(fixtures.TestBase): Base = registry.generate_base() - class A(Base): - __tablename__ = "a" + with assertions.expect_warnings( + "Attribute name 'registry' should be left reserved", + ): - registry = {"foo": "bar"} - id = Column(Integer, primary_key=True) - data = Column(String) + class A(Base): + __tablename__ = "a" + + registry = {"foo": "bar"} + id = Column(Integer, primary_key=True) + data = Column(String) class SubA(A): pass @@ -1775,14 +1779,9 @@ class DeclarativeMultiBaseTest( configure_mappers, ) - # currently "registry" is allowed, "metadata" is not. - @testing.combinations( - ("metadata", True), ("registry", False), argnames="name, expect_raise" - ) + @testing.combinations(("metadata",), ("registry",), argnames="name") @testing.variation("attrtype", ["column", "relationship"]) - def test_reserved_identifiers( - self, decl_base, name, expect_raise, attrtype - ): + def test_reserved_identifiers(self, decl_base, name, attrtype): if attrtype.column: clsdict = { "__tablename__": "user", @@ -1804,16 +1803,12 @@ class DeclarativeMultiBaseTest( else: assert False - if expect_raise: - with expect_raises_message( - exc.InvalidRequestError, - f"Attribute name '{name}' is reserved " - "when using the Declarative API.", - ): - type("User", (decl_base,), clsdict) - else: + with assertions.expect_warnings( + f"Attribute name '{name}' should be left reserved", + ): User = type("User", (decl_base,), clsdict) - assert getattr(User, name).property + + assert getattr(User, name).property def test_recompile_on_othermapper(self): """declarative version of the same test in mappers.py""" diff --git a/test/orm/declarative/test_clsregistry.py b/test/orm/declarative/test_clsregistry.py index 13e5dbc262..6469017fa1 100644 --- a/test/orm/declarative/test_clsregistry.py +++ b/test/orm/declarative/test_clsregistry.py @@ -587,3 +587,150 @@ class ClsRegistryTest(fixtures.TestBase): configure_mappers() assert class_mapper(A).get_property("bs").secondary is a_to_b + + @testing.variation( + "mapping_style", ["declarative_base", "registry_mapped"] + ) + @testing.variation("relationship_style", ["backref", "back_populates"]) + def test_backref_named_metadata(self, mapping_style, relationship_style): + """test that a backref or back_populates which overwrites + cls.metadata with a relationship collection does not break + _metadata_for_cls() when used in combination with string-based + relationship references on the same class. + + Reproduces the pattern used by OpenStack Nova where a child model + uses backref='metadata' on a relationship to a parent class, + thereby shadowing the declarative 'metadata' attribute with an + InstrumentedList. When the parent class also has a string-based + secondary table reference, the class registry resolver calls + _metadata_for_cls() after the backref has already overwritten + cls.metadata. + + See https://bugs.launchpad.net/nova/+bug/2154165, + https://github.com/sqlalchemy/sqlalchemy/discussions/8619 + """ + + use_backref = bool(relationship_style.backref) + + if mapping_style.declarative_base: + Base = declarative_base() + + class InstanceMetadata(Base): + __tablename__ = "instance_metadata" + id = Column(Integer, primary_key=True) + instance_id = Column(Integer, ForeignKey("instance.id")) + if use_backref: + instance = relationship("Instance", backref="metadata") + else: + instance = relationship( + "Instance", + back_populates="metadata", + ) + + class Tag(Base): + __tablename__ = "tag" + id = Column(Integer, primary_key=True) + + instance_tag = Table( + "instance_tag", + Base.metadata, + Column( + "instance_id", + Integer, + ForeignKey("instance.id"), + ), + Column( + "tag_id", + Integer, + ForeignKey("tag.id"), + ), + ) + + ctx = ( + expect_warnings( + r"Attribute name 'metadata' should be left reserved" + ) + if not use_backref + else nullcontext() + ) + with ctx: + + class Instance(Base): + __tablename__ = "instance" + id = Column(Integer, primary_key=True) + tags = relationship("Tag", secondary="instance_tag") + if not use_backref: + metadata = relationship( + "InstanceMetadata", + back_populates="instance", + ) + + elif mapping_style.registry_mapped: + reg = registry() + + @reg.mapped + class InstanceMetadata: + __tablename__ = "instance_metadata" + id = Column(Integer, primary_key=True) + instance_id = Column(Integer, ForeignKey("instance.id")) + if use_backref: + instance = relationship("Instance", backref="metadata") + else: + instance = relationship( + "Instance", + back_populates="metadata", + ) + + @reg.mapped + class Tag: + __tablename__ = "tag" + id = Column(Integer, primary_key=True) + + instance_tag = Table( + "instance_tag", + reg.metadata, + Column( + "instance_id", + Integer, + ForeignKey("instance.id"), + ), + Column( + "tag_id", + Integer, + ForeignKey("tag.id"), + ), + ) + + ctx = ( + expect_warnings( + r"Attribute name 'metadata' should be left reserved" + ) + if not use_backref + else nullcontext() + ) + with ctx: + + @reg.mapped + class Instance: + __tablename__ = "instance" + id = Column(Integer, primary_key=True) + tags = relationship("Tag", secondary="instance_tag") + if not use_backref: + metadata = relationship( + "InstanceMetadata", + back_populates="instance", + ) + + else: + mapping_style.fail() + + configure_mappers() + + eq_( + class_mapper(Instance).get_property("metadata").mapper.class_, + InstanceMetadata, + ) + is_( + class_mapper(Instance).get_property("tags").secondary, + instance_tag, + )