From: Mike Bayer Date: Wed, 17 Aug 2022 21:47:19 +0000 (-0400) Subject: improve abstractconcretebase X-Git-Tag: rel_2_0_0b1~104^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d0abdbe247dc82db51aa0af3d31b051042a8ba87;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git improve abstractconcretebase try to limit the attributes on the base and set up wpoly etc so that things still work the way we want. It seems like I've tried this in the past before so not sure if this is actually working or if there are problems. it needs a little more strictness on how you set up your base since attributes are no longer implicit. So, it seems like perhaps the new behavior should be on a flag or something like "strict_attributes=True", something like that, so that nothing breaks for existing users and we can slowly deal with the new way being a little bit less worse than the old way. Fixes: #8403 Change-Id: Ic9652d9a0b024d649807aaf3505e67173e7dc3b9 --- diff --git a/doc/build/orm/inheritance.rst b/doc/build/orm/inheritance.rst index f006b0ac74..913311b23d 100644 --- a/doc/build/orm/inheritance.rst +++ b/doc/build/orm/inheritance.rst @@ -826,7 +826,9 @@ class called :class:`.AbstractConcreteBase` which achieves this automatically:: class Employee(AbstractConcreteBase, Base): - pass + strict_attrs = True + + name = mapped_column(String(50)) class Manager(Employee): @@ -864,6 +866,46 @@ With a mapping like the above, only instances of ``Manager`` and ``Engineer`` may be persisted; querying against the ``Employee`` class will always produce ``Manager`` and ``Engineer`` objects. +Using the above mapping, queries can be produced in terms of the ``Employee`` +class and any attributes that are locally declared upon it, such as the +``Employee.name``:: + + >>> stmt = select(Employee).where(Employee.name == 'n1') + >>> print(stmt) + SELECT pjoin.id, pjoin.name, pjoin.type, pjoin.manager_data, pjoin.engineer_info + FROM ( + SELECT engineer.id AS id, engineer.name AS name, engineer.engineer_info AS engineer_info, + CAST(NULL AS VARCHAR(40)) AS manager_data, 'engineer' AS type + FROM engineer + UNION ALL + SELECT manager.id AS id, manager.name AS name, CAST(NULL AS VARCHAR(40)) AS engineer_info, + manager.manager_data AS manager_data, 'manager' AS type + FROM manager + ) AS pjoin + WHERE pjoin.name = :name_1 + +The :paramref:`.AbstractConcreteBase.strict_attrs` parameter indicates that the +``Employee`` class should directly map only those attributes which are local to +the ``Employee`` class, in this case the ``Employee.name`` attribute. Other +attributes such as ``Manager.manager_data`` and ``Engineer.engineer_info`` are +present only on their corresponding subclass. +When :paramref:`.AbstractConcreteBase.strict_attrs` +is not set, then all subclass attributes such as ``Manager.manager_data`` and +``Engineer.engineer_info`` get mapped onto the base ``Employee`` class. This +is a legacy mode of use which may be more convenient for querying but has the +effect that all subclasses share the +full set of attributes for the whole hierarchy; in the above example, not +using :paramref:`.AbstractConcreteBase.strict_attrs` would have the effect +of generating non-useful ``Engineer.manager_name`` and ``Manager.engineer_info`` +attributes. + +.. versionadded:: 2.0 Added :paramref:`.AbstractConcreteBase.strict_attrs` + parameter to :class:`.AbstractConcreteBase` which produces a cleaner + mapping; the default is False to allow legacy mappings to continue working + as they did in 1.x versions. + + + .. seealso:: :class:`.AbstractConcreteBase` diff --git a/lib/sqlalchemy/ext/declarative/extensions.py b/lib/sqlalchemy/ext/declarative/extensions.py index 29eb76e82a..596379bacb 100644 --- a/lib/sqlalchemy/ext/declarative/extensions.py +++ b/lib/sqlalchemy/ext/declarative/extensions.py @@ -123,14 +123,18 @@ class AbstractConcreteBase(ConcreteBase): :class:`.AbstractConcreteBase` will use the :func:`.polymorphic_union` function automatically, against all tables mapped as a subclass to this class. The function is called via the - ``__declare_last__()`` function, which is essentially - a hook for the :meth:`.after_configured` event. - - :class:`.AbstractConcreteBase` does produce a mapped class - for the base class, however it is not persisted to any table; it - is instead mapped directly to the "polymorphic" selectable directly - and is only used for selecting. Compare to :class:`.ConcreteBase`, - which does create a persisted table for the base class. + ``__declare_first__()`` function, which is essentially + a hook for the :meth:`.before_configured` event. + + :class:`.AbstractConcreteBase` applies :class:`_orm.Mapper` for its + immediately inheriting class, as would occur for any other + declarative mapped class. However, the :class:`_orm.Mapper` is not + mapped to any particular :class:`.Table` object. Instead, it's + mapped directly to the "polymorphic" selectable produced by + :func:`.polymorphic_union`, and performs no persistence operations on its + own. Compare to :class:`.ConcreteBase`, which maps its + immediately inheriting class to an actual + :class:`.Table` that stores rows directly. .. note:: @@ -182,17 +186,21 @@ class AbstractConcreteBase(ConcreteBase): or an ``__abstract__`` base class. Once classes are configured and mappings are produced, it then gets mapped itself, but after all of its descendants. This is a very unique system of mapping - not found in any other SQLAlchemy system. + not found in any other SQLAlchemy API feature. Using this approach, we can specify columns and properties that will take place on mapped subclasses, in the way that we normally do as in :ref:`declarative_mixins`:: + from sqlalchemy.ext.declarative import AbstractConcreteBase + class Company(Base): __tablename__ = 'company' id = Column(Integer, primary_key=True) class Employee(AbstractConcreteBase, Base): + strict_attrs = True + employee_id = Column(Integer, primary_key=True) @declared_attr @@ -223,6 +231,13 @@ class AbstractConcreteBase(ConcreteBase): select(Employee).filter(Employee.company.has(id=5)) ) + :param strict_attrs: when specified on the base class, "strict" attribute + mode is enabled which attempts to limit ORM mapped attributes on the + base class to only those that are immediately present, while still + preserving "polymorphic" loading behavior. + + .. versionadded:: 2.0 + .. seealso:: :class:`.ConcreteBase` @@ -271,27 +286,46 @@ class AbstractConcreteBase(ConcreteBase): # In that case, ensure we update the properties entry # to the correct column from the pjoin target table. declared_cols = set(to_map.declared_columns) + declared_col_keys = {c.key for c in declared_cols} for k, v in list(to_map.properties.items()): if v in declared_cols: to_map.properties[k] = pjoin.c[v.key] + declared_col_keys.remove(v.key) to_map.local_table = pjoin + strict_attrs = cls.__dict__.get("strict_attrs", False) + m_args = to_map.mapper_args_fn or dict def mapper_args(): args = m_args() args["polymorphic_on"] = pjoin.c[discriminator_name] + if strict_attrs: + args["include_properties"] = ( + set(pjoin.primary_key) + | declared_col_keys + | {discriminator_name} + ) + args["with_polymorphic"] = ("*", pjoin) return args to_map.mapper_args_fn = mapper_args - m = to_map.map() + to_map.map() - for scls in cls.__subclasses__(): + stack = [cls] + while stack: + scls = stack.pop(0) + stack.extend(scls.__subclasses__()) sm = _mapper_or_none(scls) - if sm and sm.concrete and cls in scls.__bases__: - sm._set_concrete_base(m) + if sm and sm.concrete and sm.inherits is None: + for sup_ in scls.__mro__[1:]: + sup_sm = _mapper_or_none(sup_) + if sup_sm: + + sm._set_concrete_base(sup_sm) + break @classmethod def _sa_raise_deferred_config(cls): diff --git a/test/ext/declarative/test_inheritance.py b/test/ext/declarative/test_inheritance.py index a695aadba4..d22090086b 100644 --- a/test/ext/declarative/test_inheritance.py +++ b/test/ext/declarative/test_inheritance.py @@ -218,7 +218,7 @@ class ConcreteInhTest( def test_abstract_concrete_base_didnt_configure(self): class Employee(AbstractConcreteBase, Base, fixtures.ComparableEntity): - pass + strict_attrs = True assert_raises_message( orm_exc.UnmappedClassError, @@ -266,16 +266,17 @@ class ConcreteInhTest( self.assert_compile( Session().query(Employee), - "SELECT pjoin.employee_id AS pjoin_employee_id, " - "pjoin.name AS pjoin_name, pjoin.golf_swing AS pjoin_golf_swing, " - "pjoin.type AS pjoin_type FROM (SELECT manager.employee_id " + "SELECT pjoin.employee_id AS pjoin_employee_id, pjoin.type AS " + "pjoin_type, pjoin.name AS pjoin_name, " + "pjoin.golf_swing AS pjoin_golf_swing " + "FROM (SELECT manager.employee_id " "AS employee_id, manager.name AS name, manager.golf_swing AS " "golf_swing, 'manager' AS type FROM manager) AS pjoin", ) def test_abstract_concrete_extension(self): class Employee(AbstractConcreteBase, Base, fixtures.ComparableEntity): - pass + name = Column(String(50)) class Manager(Employee): __tablename__ = "manager" @@ -315,8 +316,13 @@ class ConcreteInhTest( self._roundtrip(Employee, Manager, Engineer, Boss) - def test_abstract_concrete_extension_descriptor_refresh(self): + @testing.combinations(True, False) + def test_abstract_concrete_extension_descriptor_refresh( + self, use_strict_attrs + ): class Employee(AbstractConcreteBase, Base, fixtures.ComparableEntity): + strict_attrs = use_strict_attrs + @declared_attr def name(cls): return Column(String(50)) @@ -352,10 +358,10 @@ class ConcreteInhTest( sess.add(Engineer(name="d")) sess.commit() - # paperwork is excluded because there's a descritor; so it is - # not in the Engineers mapped properties at all, though is inside the - # class manager. Maybe it shouldn't be in the class manager either. - assert "paperwork" in Engineer.__mapper__.class_manager + if use_strict_attrs: + assert "paperwork" not in Engineer.__mapper__.class_manager + else: + assert "paperwork" in Engineer.__mapper__.class_manager assert "paperwork" not in Engineer.__mapper__.attrs.keys() # type currently does get mapped, as a @@ -493,6 +499,67 @@ class ConcreteInhTest( "'manager' AS _type FROM manager) AS pjoin", ) + def test_abs_clean_dir(self): + """test #8402""" + + class Employee(AbstractConcreteBase, Base): + strict_attrs = True + + name = Column(String(50)) + + class Manager(Employee): + __tablename__ = "manager" + id = Column(Integer, primary_key=True) + name = Column(String(50)) + manager_data = Column(String(40)) + + __mapper_args__ = { + "polymorphic_identity": "manager", + "concrete": True, + } + + class Engineer(Employee): + __tablename__ = "engineer" + id = Column(Integer, primary_key=True) + name = Column(String(50)) + engineer_info = Column(String(40)) + + __mapper_args__ = { + "polymorphic_identity": "engineer", + "concrete": True, + } + + configure_mappers() + + eq_( + {n for n in dir(Employee) if not n.startswith("_")}, + {"name", "strict_attrs", "registry", "id", "type", "metadata"}, + ) + eq_( + {n for n in dir(Manager) if not n.startswith("_")}, + { + "type", + "strict_attrs", + "metadata", + "name", + "id", + "registry", + "manager_data", + }, + ) + eq_( + {n for n in dir(Engineer) if not n.startswith("_")}, + { + "name", + "strict_attrs", + "registry", + "id", + "type", + "metadata", + "engineer_info", + }, + ) + def test_abs_concrete_extension_warn_for_overlap(self): class Employee(AbstractConcreteBase, Base, fixtures.ComparableEntity): name = Column(String(50)) @@ -523,8 +590,12 @@ class ConcreteInhTest( ): configure_mappers() - def test_abs_concrete_extension_warn_concrete_disc_resolves_overlap(self): + @testing.combinations(True, False) + def test_abs_concrete_extension_warn_concrete_disc_resolves_overlap( + self, use_strict_attrs + ): class Employee(AbstractConcreteBase, Base, fixtures.ComparableEntity): + strict_attrs = use_strict_attrs _concrete_discriminator_name = "_type" name = Column(String(50)) @@ -547,8 +618,14 @@ class ConcreteInhTest( configure_mappers() self.assert_compile( select(Employee), - "SELECT pjoin.employee_id, pjoin.type, pjoin.name, pjoin._type " - "FROM (SELECT manager.employee_id AS employee_id, " + ( + "SELECT pjoin.employee_id, pjoin.name, pjoin._type, " + "pjoin.type " + if use_strict_attrs + else "SELECT pjoin.employee_id, pjoin.type, pjoin.name, " + "pjoin._type " + ) + + "FROM (SELECT manager.employee_id AS employee_id, " "manager.type AS type, manager.name AS name, 'manager' AS _type " "FROM manager) AS pjoin", ) @@ -594,7 +671,7 @@ class ConcreteInhTest( def test_ok_to_override_type_from_abstract(self): class Employee(AbstractConcreteBase, Base, fixtures.ComparableEntity): - pass + name = Column(String(50)) class Manager(Employee): __tablename__ = "manager" @@ -667,7 +744,7 @@ class ConcreteExtensionConfigTest( ) class BC(AbstractConcreteBase, Base, fixtures.ComparableEntity): - pass + a_id = Column(Integer, ForeignKey("a.id")) class B(BC): __tablename__ = "b" @@ -675,7 +752,6 @@ class ConcreteExtensionConfigTest( Integer, primary_key=True, test_needs_autoincrement=True ) - a_id = Column(Integer, ForeignKey("a.id")) data = Column(String(50)) b_data = Column(String(50)) __mapper_args__ = {"polymorphic_identity": "b", "concrete": True} @@ -837,8 +913,10 @@ class ConcreteExtensionConfigTest( "something.id = :id_1)", ) - def test_abstract_in_hierarchy(self): + @testing.combinations(True, False) + def test_abstract_in_hierarchy(self, use_strict_attrs): class Document(Base, AbstractConcreteBase): + strict_attrs = use_strict_attrs doctype = Column(String) class ContactDocument(Document): @@ -857,21 +935,34 @@ class ConcreteExtensionConfigTest( configure_mappers() session = Session() + self.assert_compile( session.query(Document), - "SELECT pjoin.id AS pjoin_id, pjoin.send_method AS " + "SELECT pjoin.id AS pjoin_id, pjoin.doctype AS pjoin_doctype, " + "pjoin.type AS pjoin_type, pjoin.send_method AS pjoin_send_method " + "FROM " + "(SELECT actual_documents.id AS id, " + "actual_documents.send_method AS send_method, " + "actual_documents.doctype AS doctype, " + "'actual' AS type FROM actual_documents) AS pjoin" + if use_strict_attrs + else "SELECT pjoin.id AS pjoin_id, pjoin.send_method AS " "pjoin_send_method, pjoin.doctype AS pjoin_doctype, " - "pjoin.type AS pjoin_type FROM " + "pjoin.type AS pjoin_type " + "FROM " "(SELECT actual_documents.id AS id, " "actual_documents.send_method AS send_method, " "actual_documents.doctype AS doctype, " "'actual' AS type FROM actual_documents) AS pjoin", ) - def test_column_attr_names(self): + @testing.combinations(True, False) + def test_column_attr_names(self, use_strict_attrs): """test #3480""" class Document(Base, AbstractConcreteBase): + strict_attrs = use_strict_attrs + documentType = Column("documenttype", String) class Offer(Document): diff --git a/test/orm/inheritance/test_concrete.py b/test/orm/inheritance/test_concrete.py index 78b503873f..1c5540115e 100644 --- a/test/orm/inheritance/test_concrete.py +++ b/test/orm/inheritance/test_concrete.py @@ -1490,6 +1490,8 @@ class AdaptOnNamesTest(_RemoveListeners, fixtures.DeclarativeMappedTest): """ + strict_attrs = True + @declared_attr def id(cls): return Column(Integer, primary_key=True) @@ -1665,9 +1667,10 @@ class AdaptOnNamesTest(_RemoveListeners, fixtures.DeclarativeMappedTest): ) asserter.assert_( CompiledSQL( - "SELECT anon_1.id, anon_1.metadata_id, anon_1.thing1, " - "anon_1.x1, anon_1.y1, anon_1.thing2, anon_1.x2, anon_1.y2, " - "anon_1.type, anon_1.id_1, anon_1.some_data FROM " + "SELECT anon_1.id, anon_1.metadata_id, anon_1.type, " + "anon_1.id_1, anon_1.some_data, anon_1.thing1, " + "anon_1.x1, anon_1.y1, anon_1.thing2, anon_1.x2, anon_1.y2 " + "FROM " "(SELECT a.id AS id, a.metadata_id AS metadata_id, " "a.thing1 AS thing1, a.x1 AS x1, a.y1 AS y1, " "NULL AS thing2, NULL AS x2, NULL AS y2, :param_1 AS type, "