]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
Revert AppenderQuery modifications from ORM
authorMike Bayer <mike_mp@zzzcomputing.com>
Thu, 25 Feb 2021 22:07:06 +0000 (17:07 -0500)
committerMike Bayer <mike_mp@zzzcomputing.com>
Thu, 25 Feb 2021 22:19:19 +0000 (17:19 -0500)
We are unfortunately stuck with this class completely
until we get rid of "dynamic" altogether.    The usage
contract includes the "query_class" mixin feature where
users add their own methods, and this use case  very
well in line with 2.0's contract.  As Query is not going away
in any case this has to stay in "legacy" style, there's no
point trying to change it as the new version was still fully
dependent on Query.

Fixes: #5981
Change-Id: I1bc623b17d976b4bb417ab623248d4ac227db74d

doc/build/changelog/unreleased_14/5981.rst [new file with mode: 0644]
doc/build/orm/collections.rst
lib/sqlalchemy/orm/dynamic.py
lib/sqlalchemy/orm/relationships.py
lib/sqlalchemy/testing/assertions.py
test/orm/test_dynamic.py

diff --git a/doc/build/changelog/unreleased_14/5981.rst b/doc/build/changelog/unreleased_14/5981.rst
new file mode 100644 (file)
index 0000000..cc856b0
--- /dev/null
@@ -0,0 +1,10 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 5981
+
+    Fixed regression where the :paramref:`_orm.relationship.query_class`
+    parameter stopped being functional for "dynamic" relationships.  The
+    ``AppenderQuery`` remains dependent on the legacy :class:`_orm.Query`
+    class; users are encouraged to migrate from the use of "dynamic"
+    relationships to using :func:`_orm.with_parent` instead.
+
index e37d85566c9a5447166ecb9d55e6cd1b0e29cf93..e803fc7843c9f35e981bdab25acf1b8db0b0ce83 100644 (file)
@@ -32,10 +32,15 @@ loading of child items both at load time as well as deletion time.
 Dynamic Relationship Loaders
 ----------------------------
 
-A key feature to enable management of a large collection is the so-called
-"dynamic" relationship.  This is an optional form of
-:func:`_orm.relationship` which returns a
-:class:`_orm.AppenderQuery` object in place of a collection
+.. note:: This is a legacy feature.  Using the :func:`_orm.with_parent`
+   filter in conjunction with :func:`_sql.select` is the :term:`2.0 style`
+   method of use.  For relationships that shouldn't load, set
+   :paramref:`_orm.relationship.lazy` to ``noload``.
+
+A :func:`_orm.relationship` which corresponds to a large collection can be
+configured so that it returns a legacy :class:`_orm.Query` object when
+accessed, which allows filtering of the relationship on criteria. The class is
+a special class :class:`_orm.AppenderQuery` returned in place of a collection
 when accessed. Filtering criterion may be applied as well as limits and
 offsets, either explicitly or via array slices::
 
index 32eb23199ffb590d26ba8d50855bb0c7ffa0fb63..bf8fc0e337227103aab88d94506eea9c29484e54 100644 (file)
@@ -23,13 +23,7 @@ from . import util as orm_util
 from .query import Query
 from .. import exc
 from .. import log
-from .. import sql
 from .. import util
-from ..engine import result as _result
-from ..sql import selectable
-from ..sql import util as sql_util
-from ..sql.base import _generative
-from ..sql.base import Generative
 
 
 @log.class_logger
@@ -80,6 +74,7 @@ class DynamicAttributeImpl(attributes.AttributeImpl):
         dispatch,
         target_mapper,
         order_by,
+        query_class=None,
         **kw
     ):
         super(DynamicAttributeImpl, self).__init__(
@@ -87,7 +82,12 @@ class DynamicAttributeImpl(attributes.AttributeImpl):
         )
         self.target_mapper = target_mapper
         self.order_by = order_by
-        self.query_class = AppenderQuery
+        if not query_class:
+            self.query_class = AppenderQuery
+        elif AppenderMixin in query_class.mro():
+            self.query_class = query_class
+        else:
+            self.query_class = mixin_user_query(query_class)
 
     def get(self, state, dict_, passive=attributes.PASSIVE_OFF):
         if not passive & attributes.SQL_OK:
@@ -259,26 +259,15 @@ class DynamicAttributeImpl(attributes.AttributeImpl):
         self.remove(state, dict_, value, initiator, passive=passive)
 
 
-class AppenderQuery(Generative):
-    """A dynamic query that supports basic collection storage operations."""
+class AppenderMixin(object):
+    query_class = None
 
     def __init__(self, attr, state):
-
-        # this can be select() except for aliased=True flag on join()
-        # and corresponding behaviors on select().
-        self._is_core = False
-        self._statement = Query([attr.target_mapper], None)
-
-        # self._is_core = True
-        # self._statement = sql.select(attr.target_mapper)._set_label_style(
-        #    selectable.LABEL_STYLE_TABLENAME_PLUS_COL
-        # )
-
-        self._autoflush = True
+        super(AppenderMixin, self).__init__(attr.target_mapper, None)
         self.instance = instance = state.obj()
         self.attr = attr
 
-        self.mapper = mapper = object_mapper(instance)
+        mapper = object_mapper(instance)
         prop = mapper._props[self.attr.key]
 
         if prop.secondary is not None:
@@ -288,154 +277,20 @@ class AppenderQuery(Generative):
             # is in the FROM.  So we purposely put the mapper selectable
             # in _from_obj[0] to ensure a user-defined join() later on
             # doesn't fail, and secondary is then in _from_obj[1].
-            self._statement = self._statement.select_from(
-                prop.mapper.selectable, prop.secondary
-            )
+            self._from_obj = (prop.mapper.selectable, prop.secondary)
 
-        self._statement = self._statement.where(
+        self._where_criteria = (
             prop._with_parent(instance, alias_secondary=False),
         )
 
         if self.attr.order_by:
-
-            self._statement = self._statement.order_by(*self.attr.order_by)
-
-    @_generative
-    def autoflush(self, setting):
-        """Set autoflush to a specific setting.
-
-        Note that a Session with autoflush=False will
-        not autoflush, even if this flag is set to True at the
-        Query level.  Therefore this flag is usually used only
-        to disable autoflush for a specific Query.
-
-        """
-        self._autoflush = setting
-
-    @property
-    def statement(self):
-        """Return the Core statement represented by this
-        :class:`.AppenderQuery`.
-
-        """
-        if self._is_core:
-            return self._statement._set_label_style(
-                selectable.LABEL_STYLE_DISAMBIGUATE_ONLY
-            )
-
-        else:
-            return self._statement.statement
-
-    def filter(self, *criteria):
-        """A synonym for the :meth:`_orm.AppenderQuery.where` method."""
-
-        return self.where(*criteria)
-
-    @_generative
-    def where(self, *criteria):
-        r"""Apply the given WHERE criterion, using SQL expressions.
-
-        Equivalent to :meth:`.Select.where`.
-
-        """
-        self._statement = self._statement.where(*criteria)
-
-    @_generative
-    def order_by(self, *criteria):
-        r"""Apply the given ORDER BY criterion, using SQL expressions.
-
-        Equivalent to :meth:`.Select.order_by`.
-
-        """
-        self._statement = self._statement.order_by(*criteria)
-
-    @_generative
-    def filter_by(self, **kwargs):
-        r"""Apply the given filtering criterion using keyword expressions.
-
-        Equivalent to :meth:`.Select.filter_by`.
-
-        """
-        self._statement = self._statement.filter_by(**kwargs)
-
-    @_generative
-    def join(self, target, *props, **kwargs):
-        r"""Create a SQL JOIN against this
-        object's criterion.
-
-        Equivalent to :meth:`.Select.join`.
-        """
-
-        self._statement = self._statement.join(target, *props, **kwargs)
-
-    @_generative
-    def outerjoin(self, target, *props, **kwargs):
-        r"""Create a SQL LEFT OUTER JOIN against this
-        object's criterion.
-
-        Equivalent to :meth:`.Select.outerjoin`.
-
-        """
-
-        self._statement = self._statement.outerjoin(target, *props, **kwargs)
-
-    def scalar(self):
-        """Return the first element of the first result or None
-        if no rows present.  If multiple rows are returned,
-        raises MultipleResultsFound.
-
-        Equivalent to :meth:`_query.Query.scalar`.
-
-        .. versionadded:: 1.1.6
-
-        """
-        return self._iter().scalar()
-
-    def first(self):
-        """Return the first row.
-
-        Equivalent to :meth:`_query.Query.first`.
-
-        """
-
-        # replicates limit(1) behavior
-        if self._statement is not None:
-            return self._iter().first()
-        else:
-            return self.limit(1)._iter().first()
-
-    def one(self):
-        """Return exactly one result or raise an exception.
-
-        Equivalent to :meth:`_query.Query.one`.
-
-        """
-        return self._iter().one()
-
-    def one_or_none(self):
-        """Return one or zero results, or raise an exception for multiple
-        rows.
-
-        Equivalent to :meth:`_query.Query.one_or_none`.
-
-        .. versionadded:: 1.0.9
-
-        """
-        return self._iter().one_or_none()
-
-    def all(self):
-        """Return all rows.
-
-        Equivalent to :meth:`_query.Query.all`.
-
-        """
-        return self._iter().all()
+            self._order_by_clauses = self.attr.order_by
 
     def session(self):
         sess = object_session(self.instance)
         if (
             sess is not None
-            and self._autoflush
+            and self.autoflush
             and sess.autoflush
             and self.instance in sess
         ):
@@ -447,63 +302,17 @@ class AppenderQuery(Generative):
 
     session = property(session, lambda s, x: None)
 
-    def _execute(self, sess=None):
-        # note we're returning an entirely new Query class instance
-        # here without any assignment capabilities; the class of this
-        # query is determined by the session.
-        instance = self.instance
-        if sess is None:
-            sess = object_session(instance)
-            if sess is None:
-                raise orm_exc.DetachedInstanceError(
-                    "Parent instance %s is not bound to a Session, and no "
-                    "contextual session is established; lazy load operation "
-                    "of attribute '%s' cannot proceed"
-                    % (orm_util.instance_str(instance), self.attr.key)
-                )
-
-        result = sess.execute(self._statement)
-        result = result.scalars()
-
-        if result._attributes.get("filtered", False):
-            result = result.unique()
-
-        return result
-
-    def _iter(self):
+    def __iter__(self):
         sess = self.session
         if sess is None:
-            instance = self.instance
-            state = attributes.instance_state(instance)
-
-            if state.detached:
-                raise orm_exc.DetachedInstanceError(
-                    "Parent instance %s is not bound to a Session, and no "
-                    "contextual session is established; lazy load operation "
-                    "of attribute '%s' cannot proceed"
-                    % (orm_util.instance_str(instance), self.attr.key)
-                )
-            else:
-                iterator = (
-                    (item,)
-                    for item in self.attr._get_collection_history(
-                        state,
-                        attributes.PASSIVE_NO_INITIALIZE,
-                    ).added_items
-                )
-
-                row_metadata = _result.SimpleResultMetaData(
-                    (self.mapper.class_.__name__,),
-                    [],
-                    _unique_filters=[id],
-                )
-
-                return _result.IteratorResult(row_metadata, iterator).scalars()
+            return iter(
+                self.attr._get_collection_history(
+                    attributes.instance_state(self.instance),
+                    attributes.PASSIVE_NO_INITIALIZE,
+                ).added_items
+            )
         else:
-            return self._execute(sess)
-
-    def __iter__(self):
-        return iter(self._iter())
+            return iter(self._generate(sess))
 
     def __getitem__(self, index):
         sess = self.session
@@ -513,44 +322,9 @@ class AppenderQuery(Generative):
                 attributes.PASSIVE_NO_INITIALIZE,
             ).indexed(index)
         else:
-            return orm_util._getitem(
-                self, index, allow_negative=not self.session.future
-            )
-
-    @_generative
-    def limit(self, limit):
-        self._statement = self._statement.limit(limit)
-
-    @_generative
-    def offset(self, offset):
-        self._statement = self._statement.offset(offset)
-
-    @_generative
-    def slice(self, start, stop):
-        """Computes the "slice" represented by
-        the given indices and apply as LIMIT/OFFSET.
-
-
-        """
-        limit_clause, offset_clause = sql_util._make_slice(
-            self._statement._limit_clause,
-            self._statement._offset_clause,
-            start,
-            stop,
-        )
-
-        self._statement = self._statement.limit(limit_clause).offset(
-            offset_clause
-        )
+            return self._generate(sess).__getitem__(index)
 
     def count(self):
-        """return the 'count'.
-
-        Equivalent to :meth:`_query.Query.count`.
-
-
-        """
-
         sess = self.session
         if sess is None:
             return len(
@@ -560,10 +334,33 @@ class AppenderQuery(Generative):
                 ).added_items
             )
         else:
-            col = sql.func.count(sql.literal_column("*"))
+            return self._generate(sess).count()
+
+    def _generate(self, sess=None):
+        # note we're returning an entirely new Query class instance
+        # here without any assignment capabilities; the class of this
+        # query is determined by the session.
+        instance = self.instance
+        if sess is None:
+            sess = object_session(instance)
+            if sess is None:
+                raise orm_exc.DetachedInstanceError(
+                    "Parent instance %s is not bound to a Session, and no "
+                    "contextual session is established; lazy load operation "
+                    "of attribute '%s' cannot proceed"
+                    % (orm_util.instance_str(instance), self.attr.key)
+                )
+
+        if self.query_class:
+            query = self.query_class(self.attr.target_mapper, session=sess)
+        else:
+            query = sess.query(self.attr.target_mapper)
+
+        query._where_criteria = self._where_criteria
+        query._from_obj = self._from_obj
+        query._order_by_clauses = self._order_by_clauses
 
-            stmt = sql.select(col).select_from(self._statement.subquery())
-            return self.session.execute(stmt).scalar()
+        return query
 
     def extend(self, iterator):
         for item in iterator:
@@ -591,6 +388,16 @@ class AppenderQuery(Generative):
         )
 
 
+class AppenderQuery(AppenderMixin, Query):
+    """A dynamic query that supports basic collection storage operations."""
+
+
+def mixin_user_query(cls):
+    """Return a new class with AppenderQuery functionality layered over."""
+    name = "Appender" + cls.__name__
+    return type(name, (AppenderMixin, cls), {"query_class": cls})
+
+
 class CollectionHistory(object):
     """Overrides AttributeHistory to receive append/remove events directly."""
 
index b0a2f7b088a47b78c859a8b07834d287cacf7990..63dca4c95a1a32f093a186a1b3bee597e9a90d06 100644 (file)
@@ -831,8 +831,8 @@ class RelationshipProperty(StrategizedProperty):
 
         :param query_class:
           A :class:`_query.Query`
-          subclass that will be used as the base of the
-          "appender query" returned by a "dynamic" relationship, that
+          subclass that will be used internally by the
+          ``AppenderQuery`` returned by a "dynamic" relationship, that
           is, a relationship that specifies ``lazy="dynamic"`` or was
           otherwise constructed using the :func:`_orm.dynamic_loader`
           function.
index 44d7405b12e7372f09437e180e6c91ce90aaeafd..81c74e7c161433dfd7242ae85c244f91b0d15d9f 100644 (file)
@@ -444,9 +444,6 @@ class AssertsCompiledSQL(object):
 
         from sqlalchemy import orm
 
-        if isinstance(clause, orm.dynamic.AppenderQuery):
-            clause = clause._statement
-
         if isinstance(clause, orm.Query):
             compile_state = clause._compile_state()
             compile_state.statement._label_style = (
index 942e8383a5a13cdc5dade5083fb51b138b55d4c9..37eed391c0b4e0603a9d6975d909a550a05391e4 100644 (file)
@@ -11,6 +11,7 @@ from sqlalchemy.orm import backref
 from sqlalchemy.orm import configure_mappers
 from sqlalchemy.orm import exc as orm_exc
 from sqlalchemy.orm import mapper
+from sqlalchemy.orm import Query
 from sqlalchemy.orm import relationship
 from sqlalchemy.testing import assert_raises
 from sqlalchemy.testing import assert_raises_message
@@ -205,15 +206,41 @@ class DynamicTest(_DynamicFixture, _fixtures.FixtureTest, AssertsCompiledSQL):
             use_default_dialect=True,
         )
 
+    def test_query_class_custom_method(self):
+        class MyClass(Query):
+            def my_filter(self, arg):
+                return self.filter(Address.email_address == arg)
+
+        User, Address = self._user_address_fixture(
+            addresses_args=dict(query_class=MyClass)
+        )
+
+        sess = fixture_session()
+        q = sess.query(User)
+
+        u = q.filter(User.id == 7).first()
+
+        assert isinstance(u.addresses, MyClass)
+
+        self.assert_compile(
+            u.addresses.my_filter("x").statement,
+            "SELECT addresses.id, addresses.user_id, addresses.email_address "
+            "FROM "
+            "addresses WHERE :param_1 = addresses.user_id AND "
+            "addresses.email_address = :email_address_1",
+            use_default_dialect=True,
+        )
+
     def test_detached_raise(self):
         User, Address = self._user_address_fixture()
         sess = fixture_session()
         u = sess.query(User).get(8)
         sess.expunge(u)
-
-        q = u.addresses.filter_by(email_address="e")
-
-        assert_raises(orm_exc.DetachedInstanceError, q.first)
+        assert_raises(
+            orm_exc.DetachedInstanceError,
+            u.addresses.filter_by,
+            email_address="e",
+        )
 
     def test_no_uselist_false(self):
         User, Address = self._user_address_fixture(