From ef9520e432183fd25ea0ba9c9c2a8d8b9d36f8f6 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 4 Mar 2024 09:12:34 -0500 Subject: [PATCH] accommodate False conditions for unique / index merge Fixed issue in ORM annotated declarative where using :func:`_orm.mapped_column()` with an :paramref:`_orm.mapped_column.index` or :paramref:`_orm.mapped_column.unique` setting of False would be overridden by an incoming ``Annotated`` element that featured that parameter set to ``True``, even though the immediate :func:`_orm.mapped_column()` element is more specific and should take precedence. The logic to reconcile the booleans has been enhanced to accommodate a local value of ``False`` as still taking precedence over an incoming ``True`` value from the annotated element. Fixes: #11091 Change-Id: I15cda4a0a07a289015c0a09bbe3ca2849956604e (cherry picked from commit e4c4bd03abae2d3948f894d38992d51c9be2a8c0) --- doc/build/changelog/unreleased_20/11091.rst | 13 +++++++++ lib/sqlalchemy/sql/schema.py | 10 +++---- .../test_tm_future_annotations_sync.py | 28 +++++++++++++++++++ test/orm/declarative/test_typed_mapping.py | 28 +++++++++++++++++++ test/sql/test_metadata.py | 22 +++++++++++++++ 5 files changed, 96 insertions(+), 5 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/11091.rst diff --git a/doc/build/changelog/unreleased_20/11091.rst b/doc/build/changelog/unreleased_20/11091.rst new file mode 100644 index 00000000000..30f2fbcd355 --- /dev/null +++ b/doc/build/changelog/unreleased_20/11091.rst @@ -0,0 +1,13 @@ +.. change:: + :tags: bug, orm + :tickets: 11091 + + Fixed issue in ORM annotated declarative where using + :func:`_orm.mapped_column()` with an :paramref:`_orm.mapped_column.index` + or :paramref:`_orm.mapped_column.unique` setting of False would be + overridden by an incoming ``Annotated`` element that featured that + parameter set to ``True``, even though the immediate + :func:`_orm.mapped_column()` element is more specific and should take + precedence. The logic to reconcile the booleans has been enhanced to + accommodate a local value of ``False`` as still taking precedence over an + incoming ``True`` value from the annotated element. diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py index 2932fffad47..6d5f941786a 100644 --- a/lib/sqlalchemy/sql/schema.py +++ b/lib/sqlalchemy/sql/schema.py @@ -2569,8 +2569,11 @@ def _merge(self, other: Column[Any]) -> None: new_onupdate = self.onupdate._copy() new_onupdate._set_parent(other) - if self.index and not other.index: - other.index = True + if self.index in (True, False) and other.index is None: + other.index = self.index + + if self.unique in (True, False) and other.unique is None: + other.unique = self.unique if self.doc and other.doc is None: other.doc = self.doc @@ -2578,9 +2581,6 @@ def _merge(self, other: Column[Any]) -> None: if self.comment and other.comment is None: other.comment = self.comment - if self.unique and not other.unique: - other.unique = True - for const in self.constraints: if not const._type_bound: new_const = const._copy() diff --git a/test/orm/declarative/test_tm_future_annotations_sync.py b/test/orm/declarative/test_tm_future_annotations_sync.py index 9b55827d499..6b118139178 100644 --- a/test/orm/declarative/test_tm_future_annotations_sync.py +++ b/test/orm/declarative/test_tm_future_annotations_sync.py @@ -904,7 +904,9 @@ def test_we_got_all_attrs_test_annotated(self): ), ("index", True, lambda column: column.index is True), ("index", _NoArg.NO_ARG, lambda column: column.index is None), + ("index", False, lambda column: column.index is False), ("unique", True, lambda column: column.unique is True), + ("unique", False, lambda column: column.unique is False), ("autoincrement", True, lambda column: column.autoincrement is True), ("system", True, lambda column: column.system is True), ("primary_key", True, lambda column: column.primary_key is True), @@ -1062,6 +1064,32 @@ class User(Base): argument, ) + @testing.combinations(("index",), ("unique",), argnames="paramname") + @testing.combinations((True,), (False,), (None,), argnames="orig") + @testing.combinations((True,), (False,), (None,), argnames="merging") + def test_index_unique_combinations( + self, paramname, orig, merging, decl_base + ): + """test #11091""" + + global myint + + amc = mapped_column(**{paramname: merging}) + myint = Annotated[int, amc] + + mc = mapped_column(**{paramname: orig}) + + class User(decl_base): + __tablename__ = "user" + id: Mapped[int] = mapped_column(primary_key=True) + myname: Mapped[myint] = mc + + result = getattr(User.__table__.c.myname, paramname) + if orig is None: + is_(result, merging) + else: + is_(result, orig) + def test_pep484_newtypes_as_typemap_keys( self, decl_base: Type[DeclarativeBase] ): diff --git a/test/orm/declarative/test_typed_mapping.py b/test/orm/declarative/test_typed_mapping.py index ba8ab455ca1..0b9f4c1acbd 100644 --- a/test/orm/declarative/test_typed_mapping.py +++ b/test/orm/declarative/test_typed_mapping.py @@ -895,7 +895,9 @@ def test_we_got_all_attrs_test_annotated(self): ), ("index", True, lambda column: column.index is True), ("index", _NoArg.NO_ARG, lambda column: column.index is None), + ("index", False, lambda column: column.index is False), ("unique", True, lambda column: column.unique is True), + ("unique", False, lambda column: column.unique is False), ("autoincrement", True, lambda column: column.autoincrement is True), ("system", True, lambda column: column.system is True), ("primary_key", True, lambda column: column.primary_key is True), @@ -1053,6 +1055,32 @@ class User(Base): argument, ) + @testing.combinations(("index",), ("unique",), argnames="paramname") + @testing.combinations((True,), (False,), (None,), argnames="orig") + @testing.combinations((True,), (False,), (None,), argnames="merging") + def test_index_unique_combinations( + self, paramname, orig, merging, decl_base + ): + """test #11091""" + + # anno only: global myint + + amc = mapped_column(**{paramname: merging}) + myint = Annotated[int, amc] + + mc = mapped_column(**{paramname: orig}) + + class User(decl_base): + __tablename__ = "user" + id: Mapped[int] = mapped_column(primary_key=True) + myname: Mapped[myint] = mc + + result = getattr(User.__table__.c.myname, paramname) + if orig is None: + is_(result, merging) + else: + is_(result, orig) + def test_pep484_newtypes_as_typemap_keys( self, decl_base: Type[DeclarativeBase] ): diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py index 8b43b0f98ac..a54a5fcc8d5 100644 --- a/test/sql/test_metadata.py +++ b/test/sql/test_metadata.py @@ -4376,6 +4376,28 @@ def compile_(element, compiler, **kw): deregister(schema.CreateColumn) + @testing.combinations(("index",), ("unique",), argnames="paramname") + @testing.combinations((True,), (False,), (None,), argnames="orig") + @testing.combinations((True,), (False,), (None,), argnames="merging") + def test_merge_index_unique(self, paramname, orig, merging): + """test #11091""" + source = Column(**{paramname: merging}) + + target = Column(**{paramname: orig}) + + source._merge(target) + + target_copy = target._copy() + for col in ( + target, + target_copy, + ): + result = getattr(col, paramname) + if orig is None: + is_(result, merging) + else: + is_(result, orig) + @testing.combinations( ("default", lambda ctx: 10), ("default", func.foo()),