From 361beb0bc8392c92403ffc1999eb2a9847e945c7 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Thu, 16 Feb 2023 21:52:18 +0100 Subject: [PATCH] Allow custom sorting of column in the ORM. To accommodate a change in column ordering used by ORM Declarative in SQLAlchemy 2.0, a new parameter :paramref:`_orm.mapped_column.sort_order` has been added that can be used to control the order of the columns defined in the table by the ORM, for common use cases such as mixins with primary key columns that should appear first in tables. The change notes at :ref:`change_9297` illustrate the default change in ordering behavior (which is part of all SQLAlchemy 2.0 releases) as well as use of the :paramref:`_orm.mapped_column.sort_order` to control column ordering when using mixins and multiple classes (new in 2.0.4). Fixes: #9297 Change-Id: Ic7163d64efdc0eccb53d6ae0dd89ec83427fb675 --- doc/build/changelog/unreleased_20/9297.rst | 17 ++++++ doc/build/changelog/whatsnew_20.rst | 62 ++++++++-------------- lib/sqlalchemy/orm/_orm_constructors.py | 22 ++++++-- lib/sqlalchemy/orm/decl_base.py | 22 ++++++-- lib/sqlalchemy/orm/descriptor_props.py | 4 +- lib/sqlalchemy/orm/interfaces.py | 2 +- lib/sqlalchemy/orm/properties.py | 12 +++-- lib/sqlalchemy/sql/_typing.py | 2 + lib/sqlalchemy/sql/schema.py | 7 +-- test/orm/declarative/test_basic.py | 42 +++++++++++++++ test/sql/test_metadata.py | 3 ++ 11 files changed, 136 insertions(+), 59 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/9297.rst diff --git a/doc/build/changelog/unreleased_20/9297.rst b/doc/build/changelog/unreleased_20/9297.rst new file mode 100644 index 0000000000..832eeda2fc --- /dev/null +++ b/doc/build/changelog/unreleased_20/9297.rst @@ -0,0 +1,17 @@ +.. change:: + :tags: orm, use_case + :tickets: 9297 + + To accommodate a change in column ordering used by ORM Declarative in + SQLAlchemy 2.0, a new parameter :paramref:`_orm.mapped_column.sort_order` + has been added that can be used to control the order of the columns defined + in the table by the ORM, for common use cases such as mixins with primary + key columns that should appear first in tables. The change notes at + :ref:`change_9297` illustrate the default change in ordering behavior + (which is part of all SQLAlchemy 2.0 releases) as well as use of the + :paramref:`_orm.mapped_column.sort_order` to control column ordering when + using mixins and multiple classes (new in 2.0.4). + + .. seealso:: + + :ref:`change_9297` diff --git a/doc/build/changelog/whatsnew_20.rst b/doc/build/changelog/whatsnew_20.rst index 3698a64f02..63fadc8a55 100644 --- a/doc/build/changelog/whatsnew_20.rst +++ b/doc/build/changelog/whatsnew_20.rst @@ -1822,9 +1822,10 @@ operations. :ticket:`8925` +.. _change_9297: -ORM Declarative Applies Column Orders Differently; Control behavior using ``__table_cls__`` -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +ORM Declarative Applies Column Orders Differently; Control behavior using ``sort_order`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Declarative has changed the system by which mapped columns that originate from mixin or abstract base classes are sorted along with the columns that are on the @@ -1833,19 +1834,19 @@ by mixin columns. The following mapping:: class Foo: - col1 = Column(Integer) - col3 = Column(Integer) + col1 = mapped_column(Integer) + col3 = mapped_column(Integer) class Bar: - col2 = Column(Integer) - col4 = Column(Integer) + col2 = mapped_column(Integer) + col4 = mapped_column(Integer) class Model(Base, Foo, Bar): - id = Column(Integer, primary_key=True) + id = mapped_column(Integer, primary_key=True) __tablename__ = "model" Produces a CREATE TABLE as follows on 1.4: @@ -1881,15 +1882,15 @@ around, as:: class Foo: - id = Column(Integer, primary_key=True) - col1 = Column(Integer) - col3 = Column(Integer) + id = mapped_column(Integer, primary_key=True) + col1 = mapped_column(Integer) + col3 = mapped_column(Integer) class Model(Foo, Base): - col2 = Column(Integer) - col4 = Column(Integer) + col2 = mapped_column(Integer) + col4 = mapped_column(Integer) __tablename__ = "model" This now produces CREATE TABLE output as: @@ -1905,39 +1906,22 @@ This now produces CREATE TABLE output as: PRIMARY KEY (id) ) -It seems clear that Declarative may benefit from a simple rule such as -"place primary key columns first, no matter what", or the availability of a -public "sort order" attribute on columns. Many users have become familiar with -a private attribute known as ``_creation_order``, however this attribute was -never sufficient at controlling ordering in mixin inheritance scenarios, and -is **no longer used** for column ordering in Declarative. - -In the interim, both SQLAlchemy 1.4 and 2.0 have a hook which can be used -to apply such "sort ordering" right now, which is the -:ref:`declarative_table_cls` hook. The above model can be given a deterministic -"primary key first" scheme that is cross-compatible with 1.4 / 2.0 right now, -using this hook in conjunction with the :paramref:`_schema.Column.info` -dictionary to apply custom parameters, as in the example below:: - - from sqlalchemy import Table - +To solve this issue, SQLAlchemy 2.0.4 introduces a new parameter on +:func:`_orm.mapped_column` called :paramref:`_orm.mapped_column.sort_order`, +which is an integer value, defaulting to ``0``, +that can be set to a positive or negative value so that columns are placed +before or after other columns, as in the example below:: class Foo: - @classmethod - def __table_cls__(cls, name, metadata_obj, *arg, **kw): - arg = sorted(arg, key=lambda obj: obj.info.get("column_order", 0)) - - return Table(name, metadata_obj, *arg, **kw) - - id = Column(Integer, primary_key=True, info={"column_order": -10}) - col1 = Column(Integer, info={"column_order": -1}) - col3 = Column(Integer) + id = mapped_column(Integer, primary_key=True, sort_order=-10) + col1 = mapped_column(Integer, sort_order=-1) + col3 = mapped_column(Integer) class Model(Foo, Base): - col2 = Column(Integer) - col4 = Column(Integer) + col2 = mapped_column(Integer) + col4 = mapped_column(Integer) __tablename__ = "model" The above model places "id" before all others and "col1" after "id": diff --git a/lib/sqlalchemy/orm/_orm_constructors.py b/lib/sqlalchemy/orm/_orm_constructors.py index 424385e1b7..3bd1db79d8 100644 --- a/lib/sqlalchemy/orm/_orm_constructors.py +++ b/lib/sqlalchemy/orm/_orm_constructors.py @@ -62,6 +62,7 @@ if TYPE_CHECKING: from .relationships import _RelationshipJoinConditionArgument from .relationships import ORMBackrefArgument from .session import _SessionBind + from ..sql._typing import _AutoIncrementType from ..sql._typing import _ColumnExpressionArgument from ..sql._typing import _FromClauseArgument from ..sql._typing import _InfoType @@ -116,7 +117,7 @@ def mapped_column( use_existing_column: bool = False, name: Optional[str] = None, type_: Optional[_TypeEngineArgument[Any]] = None, - autoincrement: Union[bool, Literal["auto", "ignore_fk"]] = "auto", + autoincrement: _AutoIncrementType = "auto", doc: Optional[str] = None, key: Optional[str] = None, index: Optional[bool] = None, @@ -129,7 +130,8 @@ def mapped_column( quote: Optional[bool] = None, system: bool = False, comment: Optional[str] = None, - **dialect_kwargs: Any, + sort_order: int = 0, + **kw: Any, ) -> MappedColumn[Any]: r"""declare a new ORM-mapped :class:`_schema.Column` construct for use within :ref:`Declarative Table ` @@ -248,6 +250,15 @@ def mapped_column( :paramref:`_orm.mapped_column.default` will always apply to the constructor default for a dataclasses mapping. + :param sort_order: An integer that indicates how this mapped column + should be sorted compared to the others when the ORM is creating a + :class:`_schema.Table`. Among mapped columns that have the same + value the default ordering is used, placing first the mapped columns + defined in the main class, then the ones in the super classes. + Defaults to 0. The sort is ascending. + + .. versionadded:: 2.0.4 + :param init: Specific to :ref:`orm_declarative_native_dataclasses`, specifies if the mapped attribute should be part of the ``__init__()`` method as generated by the dataclass process. @@ -270,7 +281,7 @@ def mapped_column( :ref:`orm_declarative_native_dataclasses`, indicates if this field should be marked as keyword-only when generating the ``__init__()``. - :param \**kw: All remaining keyword argments are passed through to the + :param \**kw: All remaining keyword arguments are passed through to the constructor for the :class:`_schema.Column`. """ @@ -303,7 +314,8 @@ def mapped_column( deferred=deferred, deferred_group=deferred_group, deferred_raiseload=deferred_raiseload, - **dialect_kwargs, + sort_order=sort_order, + **kw, ) @@ -2300,7 +2312,7 @@ def join( :func:`_orm.join` is an extension to the core join interface provided by :func:`_expression.join()`, where the - left and right selectables may be not only core selectable + left and right selectable may be not only core selectable objects such as :class:`_schema.Table`, but also mapped classes or :class:`.AliasedClass` instances. The "on" clause can be a SQL expression or an ORM mapped attribute diff --git a/lib/sqlalchemy/orm/decl_base.py b/lib/sqlalchemy/orm/decl_base.py index f0be55b892..29d7485961 100644 --- a/lib/sqlalchemy/orm/decl_base.py +++ b/lib/sqlalchemy/orm/decl_base.py @@ -211,13 +211,13 @@ def _get_immediate_cls_attr( return getattr(cls, attrname) for base in cls.__mro__[1:]: - _is_classicial_inherits = _dive_for_cls_manager(base) is not None + _is_classical_inherits = _dive_for_cls_manager(base) is not None if attrname in base.__dict__ and ( base is cls or ( (base in cls.__bases__ if strict else True) - and not _is_classicial_inherits + and not _is_classical_inherits ) ): return getattr(base, attrname) @@ -451,6 +451,7 @@ class _ClassScanMapperConfig(_MapperConfig): "local_table", "persist_selectable", "declared_columns", + "column_ordering", "column_copies", "table_args", "tablename", @@ -471,6 +472,7 @@ class _ClassScanMapperConfig(_MapperConfig): local_table: Optional[FromClause] persist_selectable: Optional[FromClause] declared_columns: util.OrderedSet[Column[Any]] + column_ordering: Dict[Column[Any], int] column_copies: Dict[ Union[MappedColumn[Any], Column[Any]], Union[MappedColumn[Any], Column[Any]], @@ -522,6 +524,7 @@ class _ClassScanMapperConfig(_MapperConfig): self.collected_attributes = {} self.collected_annotations = {} self.declared_columns = util.OrderedSet() + self.column_ordering = {} self.column_copies = {} self.dataclass_setup_arguments = dca = getattr( @@ -1557,6 +1560,7 @@ class _ClassScanMapperConfig(_MapperConfig): # extract columns from the class dict declared_columns = self.declared_columns + column_ordering = self.column_ordering name_to_prop_key = collections.defaultdict(set) for key, c in list(our_stuff.items()): @@ -1570,10 +1574,12 @@ class _ClassScanMapperConfig(_MapperConfig): # this is a MappedColumn that will produce a Column for us del our_stuff[key] - for col in c.columns_to_assign: + for col, sort_order in c.columns_to_assign: if not isinstance(c, CompositeProperty): name_to_prop_key[col.name].add(key) declared_columns.add(col) + assert col not in column_ordering + column_ordering[col] = sort_order # if this is a MappedColumn and the attribute key we # have is not what the column has for its key, map the @@ -1613,6 +1619,7 @@ class _ClassScanMapperConfig(_MapperConfig): table_args = self.table_args clsdict_view = self.clsdict_view declared_columns = self.declared_columns + column_ordering = self.column_ordering manager = attributes.manager_of_class(cls) @@ -1647,12 +1654,17 @@ class _ClassScanMapperConfig(_MapperConfig): if autoload: table_kw["autoload"] = True + sorted_columns = sorted( + declared_columns, + key=lambda c: column_ordering.get(c, 0), + ) table = self.set_cls_attribute( "__table__", table_cls( tablename, self._metadata_for_cls(manager), - *(tuple(declared_columns) + tuple(args)), + *sorted_columns, + *args, **table_kw, ), ) @@ -1998,7 +2010,7 @@ def _add_attribute( mapped_cls.__mapper__.add_property(key, value) elif isinstance(value, _MapsColumns): mp = value.mapper_property_to_assign - for col in value.columns_to_assign: + for col, _ in value.columns_to_assign: _undefer_column_name(key, col) _table_or_raise(mapped_cls).append_column( col, replace_existing=True diff --git a/lib/sqlalchemy/orm/descriptor_props.py b/lib/sqlalchemy/orm/descriptor_props.py index 3bab78123d..b65171c9d4 100644 --- a/lib/sqlalchemy/orm/descriptor_props.py +++ b/lib/sqlalchemy/orm/descriptor_props.py @@ -502,8 +502,8 @@ class CompositeProperty( return self @property - def columns_to_assign(self) -> List[schema.Column[Any]]: - return [c for c in self.columns if c.table is None] + def columns_to_assign(self) -> List[Tuple[schema.Column[Any], int]]: + return [(c, 0) for c in self.columns if c.table is None] @util.preload_module("orm.properties") def _setup_arguments_on_columns(self) -> None: diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py index 4a8b724415..8667491398 100644 --- a/lib/sqlalchemy/orm/interfaces.py +++ b/lib/sqlalchemy/orm/interfaces.py @@ -306,7 +306,7 @@ class _MapsColumns(_DCAttributeOptions, _MappedAttribute[_T]): raise NotImplementedError() @property - def columns_to_assign(self) -> List[Column[_T]]: + def columns_to_assign(self) -> List[Tuple[Column[_T], int]]: """A list of Column objects that should be declaratively added to the new Table object. diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index 60b1611aca..e736e4fd2d 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -21,6 +21,7 @@ from typing import List from typing import Optional from typing import Sequence from typing import Set +from typing import Tuple from typing import Type from typing import TYPE_CHECKING from typing import TypeVar @@ -214,10 +215,10 @@ class ColumnProperty( return self @property - def columns_to_assign(self) -> List[Column[Any]]: + def columns_to_assign(self) -> List[Tuple[Column[Any], int]]: # mypy doesn't care about the isinstance here return [ - c # type: ignore + (c, 0) # type: ignore for c in self.columns if isinstance(c, Column) and c.table is None ] @@ -524,6 +525,7 @@ class MappedColumn( __slots__ = ( "column", "_creation_order", + "_sort_order", "foreign_keys", "_has_nullable", "_has_insert_default", @@ -578,6 +580,7 @@ class MappedColumn( self.deferred_group or self.deferred_raiseload ) + self._sort_order = kw.pop("sort_order", 0) self.column = cast("Column[_T]", Column(*arg, **kw)) self.foreign_keys = self.column.foreign_keys self._has_nullable = "nullable" in kw and kw.get("nullable") not in ( @@ -597,6 +600,7 @@ class MappedColumn( new._has_insert_default = self._has_insert_default new._has_dataclass_arguments = self._has_dataclass_arguments new._use_existing_column = self._use_existing_column + new._sort_order = self._sort_order util.set_creation_order(new) return new @@ -618,8 +622,8 @@ class MappedColumn( return None @property - def columns_to_assign(self) -> List[Column[Any]]: - return [self.column] + def columns_to_assign(self) -> List[Tuple[Column[Any], int]]: + return [(self.column, self._sort_order)] def __clause_element__(self) -> Column[_T]: return self.column diff --git a/lib/sqlalchemy/sql/_typing.py b/lib/sqlalchemy/sql/_typing.py index ab124103fd..6bf9a5a1f4 100644 --- a/lib/sqlalchemy/sql/_typing.py +++ b/lib/sqlalchemy/sql/_typing.py @@ -264,6 +264,8 @@ _EquivalentColumnMap = Dict["ColumnElement[Any]", Set["ColumnElement[Any]"]] _LimitOffsetType = Union[int, _ColumnExpressionArgument[int], None] +_AutoIncrementType = Union[bool, Literal["auto", "ignore_fk"]] + if TYPE_CHECKING: def is_sql_compiler(c: Compiled) -> TypeGuard[SQLCompiler]: diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py index 2a713fea63..20c0341adb 100644 --- a/lib/sqlalchemy/sql/schema.py +++ b/lib/sqlalchemy/sql/schema.py @@ -84,6 +84,7 @@ from ..util.typing import Self from ..util.typing import TypeGuard if typing.TYPE_CHECKING: + from ._typing import _AutoIncrementType from ._typing import _DDLColumnArgument from ._typing import _InfoType from ._typing import _TextCoercedExpressionArgument @@ -1375,7 +1376,7 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause[_T]): *args: SchemaEventTarget, name: Optional[str] = None, type_: Optional[_TypeEngineArgument[_T]] = None, - autoincrement: Union[bool, Literal["auto", "ignore_fk"]] = "auto", + autoincrement: _AutoIncrementType = "auto", default: Optional[Any] = None, doc: Optional[str] = None, key: Optional[str] = None, @@ -1949,7 +1950,7 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause[_T]): self.system = system self.doc = doc - self.autoincrement = autoincrement + self.autoincrement: _AutoIncrementType = autoincrement self.constraints = set() self.foreign_keys = set() self.comment = comment @@ -2278,7 +2279,7 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause[_T]): def _copy(self, **kw: Any) -> Column[Any]: """Create a copy of this ``Column``, uninitialized. - This is used in :meth:`_schema.Table.to_metadata`. + This is used in :meth:`_schema.Table.to_metadata` and by the ORM. """ diff --git a/test/orm/declarative/test_basic.py b/test/orm/declarative/test_basic.py index d4890a7815..7e8cd49f38 100644 --- a/test/orm/declarative/test_basic.py +++ b/test/orm/declarative/test_basic.py @@ -993,6 +993,48 @@ class DeclarativeBaseSetupsTest(fixtures.TestBase): ): Foo.y = mapped_column(sa.Text) + def test_default_column_order(self, decl_base): + class M1: + a: Mapped[int] + b: Mapped[int] = mapped_column(primary_key=True) + + class M2(decl_base): + __abstract__ = True + c: Mapped[int] + d: Mapped[int] + + class M(M1, M2, decl_base): + e: Mapped[int] + f: Mapped[int] + g: Mapped[int] + + __tablename__ = "m" + + actual = list(M.__table__.c.keys()) + expected = ["e", "f", "g", "a", "b", "c", "d"] + eq_(actual, expected) + + def test_custom_column_sort_order(self, decl_base): + class M1: + a: Mapped[int] = mapped_column(sort_order=-42) + b: Mapped[int] = mapped_column(primary_key=True) + + class M2(decl_base): + __abstract__ = True + c: Mapped[int] = mapped_column(sort_order=-1) + d: Mapped[int] + + class M(M1, M2, decl_base): + e: Mapped[int] + f: Mapped[int] = mapped_column(sort_order=10) + g: Mapped[int] = mapped_column(sort_order=-10) + + __tablename__ = "m" + + actual = list(M.__table__.c.keys()) + expected = ["a", "g", "c", "e", "b", "d", "f"] + eq_(actual, expected) + @testing.combinations( ("declarative_base_nometa_superclass",), diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py index 684ad3a075..04a464bbb2 100644 --- a/test/sql/test_metadata.py +++ b/test/sql/test_metadata.py @@ -92,6 +92,7 @@ class MetaDataTest(fixtures.TestBase, ComparesTables): Sequence("foo_seq"), primary_key=True, key="bar", + autoincrement="ignore_fk", ), Column(Integer(), ForeignKey("bat.blah"), doc="this is a col"), Column( @@ -99,6 +100,7 @@ class MetaDataTest(fixtures.TestBase, ComparesTables): Integer(), ForeignKey("bat.blah"), primary_key=True, + comment="this is a comment", key="bar", ), Column("bar", Integer(), info={"foo": "bar"}), @@ -113,6 +115,7 @@ class MetaDataTest(fixtures.TestBase, ComparesTables): "unique", "info", "doc", + "autoincrement", ): eq_(getattr(col, attr), getattr(c2, attr)) eq_(len(col.foreign_keys), len(c2.foreign_keys)) -- 2.47.3