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
--- /dev/null
+.. 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.
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
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
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
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
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",
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"""
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,
+ )
projects / thirdparty / sqlalchemy / sqlalchemy.git / commitdiff
