Merge "Removals: strings for join(), loader_options()." into main

5 files changed, 85 insertions, 183 deletions

diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py
index 4a3d5286b..8e3dc3134 100644
--- a/lib/sqlalchemy/orm/context.py
+++ b/lib/sqlalchemy/orm/context.py
@@ -1457,7 +1457,7 @@ class ORMSelectCompileState(ORMCompileState, SelectState):
             # legacy ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
             if (
-                isinstance(right, (interfaces.PropComparator, str))
+                isinstance(right, interfaces.PropComparator)
                 and onclause is None
             ):
                 onclause = right
@@ -1478,11 +1478,9 @@ class ORMSelectCompileState(ORMCompileState, SelectState):
                 of_type = None
 
             if isinstance(onclause, str):
-                # string given, e.g. query(Foo).join("bar").
-                # we look to the left entity or what we last joined
-                # towards
-                onclause = _entity_namespace_key(
-                    inspect(self._joinpoint_zero()), onclause
+                raise sa_exc.ArgumentError(
+                    "ORM mapped attributes as join targets must be "
+                    "stated the attribute itself, not string name"
                 )
 
             # legacy vvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
@@ -1495,7 +1493,7 @@ class ORMSelectCompileState(ORMCompileState, SelectState):
             # to work with the aliased=True flag, which is also something
             # that probably shouldn't exist on join() due to its high
             # complexity/usefulness ratio
-            elif from_joinpoint and isinstance(
+            if from_joinpoint and isinstance(
                 onclause, interfaces.PropComparator
             ):
                 jp0 = self._joinpoint_zero()
diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py
index bd80749d2..a2e247f14 100644
--- a/lib/sqlalchemy/orm/query.py
+++ b/lib/sqlalchemy/orm/query.py
@@ -2186,25 +2186,6 @@ class Query(
         in the 1.4 series for the following features:
 
 
-        * Joining on relationship names rather than attributes::
-
-            session.query(User).join("addresses")
-
-          **Why it's legacy**: the string name does not provide enough context
-          for :meth:`_query.Query.join` to always know what is desired,
-          notably in that there is no indication of what the left side
-          of the join should be.  This gives rise to flags like
-          ``from_joinpoint`` as well as the ability to place several
-          join clauses in a single :meth:`_query.Query.join` call
-          which don't solve the problem fully while also
-          adding new calling styles that are unnecessary and expensive to
-          accommodate internally.
-
-          **Modern calling pattern**:  Use the actual relationship,
-          e.g. ``User.addresses`` in the above case::
-
-              session.query(User).join(User.addresses)
-
         * Automatic aliasing with the ``aliased=True`` flag::
 
             session.query(Node).join(Node.children, aliased=True).\
@@ -2337,7 +2318,6 @@ class Query(
                 onclause,
                 (
                     elements.ColumnElement,
-                    str,
                     interfaces.PropComparator,
                     types.FunctionType,
                 ),
@@ -2361,7 +2341,7 @@ class Query(
 
                     # this checks for an extremely ancient calling form of
                     # reversed tuples.
-                    if isinstance(prop[0], (str, interfaces.PropComparator)):
+                    if isinstance(prop[0], interfaces.PropComparator):
                         prop = (prop[1], prop[0])
 
                     _props.append(prop)
@@ -2400,11 +2380,7 @@ class Query(
                     legacy=True,
                     apply_propagate_attrs=self,
                 ),
-                (
-                    coercions.expect(roles.OnClauseRole, prop[1], legacy=True)
-                    #                    if not isinstance(prop[1], str)
-                    #                    else prop[1]
-                )
+                (coercions.expect(roles.OnClauseRole, prop[1], legacy=True))
                 if len(prop) == 2
                 else None,
                 None,
diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py
index 363307bfd..a5c539773 100644
--- a/lib/sqlalchemy/orm/strategy_options.py
+++ b/lib/sqlalchemy/orm/strategy_options.py
@@ -15,7 +15,6 @@ from .base import _is_aliased_class
 from .base import _is_mapped_class
 from .base import InspectionAttr
 from .interfaces import LoaderOption
-from .interfaces import MapperProperty
 from .interfaces import PropComparator
 from .path_registry import _DEFAULT_TOKEN
 from .path_registry import _WILDCARD_TOKEN
@@ -304,7 +303,6 @@ class Load(Generative, LoaderOption):
         if isinstance(attr, str):
 
             default_token = attr.endswith(_DEFAULT_TOKEN)
-            attr_str_name = attr
             if attr.endswith(_WILDCARD_TOKEN) or default_token:
                 if default_token:
                     self.propagate_to_loaders = False
@@ -321,137 +319,97 @@ class Load(Generative, LoaderOption):
                 self.path = path
                 return path
 
-            if existing_of_type:
-                ent = inspect(existing_of_type)
-            else:
-                ent = path.entity
-
-            util.warn_deprecated_20(
-                "Using strings to indicate column or "
-                "relationship paths in loader options is deprecated "
-                "and will be removed in SQLAlchemy 2.0.  Please use "
-                "the class-bound attribute directly.",
+            raise sa_exc.ArgumentError(
+                "Strings are not accepted for attribute names in loader "
+                "options; please use class-bound attributes directly."
             )
-            try:
-                # use getattr on the class to work around
-                # synonyms, hybrids, etc.
-                attr = getattr(ent.class_, attr)
-            except AttributeError as err:
+
+        insp = inspect(attr)
+
+        if insp.is_mapper or insp.is_aliased_class:
+            # TODO: this does not appear to be a valid codepath.  "attr"
+            # would never be a mapper.  This block is present in 1.2
+            # as well however does not seem to be accessed in any tests.
+            if not orm_util._entity_corresponds_to_use_path_impl(
+                attr.parent, path[-1]
+            ):
                 if raiseerr:
-                    util.raise_(
-                        sa_exc.ArgumentError(
-                            'Can\'t find property named "%s" on '
-                            "%s in this Query." % (attr, ent)
-                        ),
-                        replace_context=err,
+                    raise sa_exc.ArgumentError(
+                        "Attribute '%s' does not "
+                        "link from element '%s'" % (attr, path.entity)
                     )
                 else:
                     return None
-            else:
-                try:
-                    attr = found_property = attr.property
-                except AttributeError as ae:
-                    if not isinstance(attr, MapperProperty):
-                        util.raise_(
-                            sa_exc.ArgumentError(
-                                'Expected attribute "%s" on %s to be a '
-                                "mapped attribute; "
-                                "instead got %s object."
-                                % (attr_str_name, ent, type(attr))
+        elif insp.is_property:
+            prop = found_property = attr
+            path = path[prop]
+        elif insp.is_attribute:
+            prop = found_property = attr.property
+
+            if not orm_util._entity_corresponds_to_use_path_impl(
+                attr.parent, path[-1]
+            ):
+                if raiseerr:
+                    raise sa_exc.ArgumentError(
+                        'Attribute "%s" does not '
+                        'link from element "%s".%s'
+                        % (
+                            attr,
+                            path.entity,
+                            (
+                                "  Did you mean to use "
+                                "%s.of_type(%s)?"
+                                % (path[-2], attr.class_.__name__)
+                                if len(path) > 1
+                                and path.entity.is_mapper
+                                and attr.parent.is_aliased_class
+                                else ""
                             ),
-                            replace_context=ae,
-                        )
-                    else:
-                        raise
-
-            path = path[attr]
-        else:
-            insp = inspect(attr)
-
-            if insp.is_mapper or insp.is_aliased_class:
-                # TODO: this does not appear to be a valid codepath.  "attr"
-                # would never be a mapper.  This block is present in 1.2
-                # as well however does not seem to be accessed in any tests.
-                if not orm_util._entity_corresponds_to_use_path_impl(
-                    attr.parent, path[-1]
-                ):
-                    if raiseerr:
-                        raise sa_exc.ArgumentError(
-                            "Attribute '%s' does not "
-                            "link from element '%s'" % (attr, path.entity)
                         )
-                    else:
-                        return None
-            elif insp.is_property:
-                prop = found_property = attr
-                path = path[prop]
-            elif insp.is_attribute:
-                prop = found_property = attr.property
+                    )
+                else:
+                    return None
 
-                if not orm_util._entity_corresponds_to_use_path_impl(
-                    attr.parent, path[-1]
-                ):
-                    if raiseerr:
-                        raise sa_exc.ArgumentError(
-                            'Attribute "%s" does not '
-                            'link from element "%s".%s'
-                            % (
-                                attr,
-                                path.entity,
-                                (
-                                    "  Did you mean to use "
-                                    "%s.of_type(%s)?"
-                                    % (path[-2], attr.class_.__name__)
-                                    if len(path) > 1
-                                    and path.entity.is_mapper
-                                    and attr.parent.is_aliased_class
-                                    else ""
-                                ),
-                            )
-                        )
-                    else:
-                        return None
+            if attr._extra_criteria and not self._extra_criteria:
+                # in most cases, the process that brings us here will have
+                # already established _extra_criteria.  however if not,
+                # and it's present on the attribute, then use that.
+                self._extra_criteria = attr._extra_criteria
 
-                if attr._extra_criteria and not self._extra_criteria:
-                    # in most cases, the process that brings us here will have
-                    # already established _extra_criteria.  however if not,
-                    # and it's present on the attribute, then use that.
-                    self._extra_criteria = attr._extra_criteria
+            if getattr(attr, "_of_type", None):
+                ac = attr._of_type
+                ext_info = of_type_info = inspect(ac)
 
-                if getattr(attr, "_of_type", None):
-                    ac = attr._of_type
-                    ext_info = of_type_info = inspect(ac)
+                if polymorphic_entity_context is None:
+                    polymorphic_entity_context = self.context
 
-                    if polymorphic_entity_context is None:
-                        polymorphic_entity_context = self.context
+                existing = path.entity_path[prop].get(
+                    polymorphic_entity_context, "path_with_polymorphic"
+                )
 
-                    existing = path.entity_path[prop].get(
-                        polymorphic_entity_context, "path_with_polymorphic"
+                if not ext_info.is_aliased_class:
+                    ac = orm_util.with_polymorphic(
+                        ext_info.mapper.base_mapper,
+                        ext_info.mapper,
+                        aliased=True,
+                        _use_mapper_path=True,
+                        _existing_alias=inspect(existing)
+                        if existing is not None
+                        else None,
                     )
 
-                    if not ext_info.is_aliased_class:
-                        ac = orm_util.with_polymorphic(
-                            ext_info.mapper.base_mapper,
-                            ext_info.mapper,
-                            aliased=True,
-                            _use_mapper_path=True,
-                            _existing_alias=inspect(existing)
-                            if existing is not None
-                            else None,
-                        )
-
-                        ext_info = inspect(ac)
+                    ext_info = inspect(ac)
 
-                    path.entity_path[prop].set(
-                        polymorphic_entity_context, "path_with_polymorphic", ac
-                    )
+                path.entity_path[prop].set(
+                    polymorphic_entity_context, "path_with_polymorphic", ac
+                )
 
-                    path = path[prop][ext_info]
+                path = path[prop][ext_info]
 
-                    self._of_type = of_type_info
+                self._of_type = of_type_info
 
-                else:
-                    path = path[prop]
+            else:
+                path = path[prop]
 
         if for_strategy is not None:
             found_property._get_strategy(for_strategy)
diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index e7a485d0b..140464b87 100644
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -1856,13 +1856,10 @@ def with_parent(instance, prop, from_entity=None):
       An instance which has some :func:`_orm.relationship`.
 
     :param property:
-      String property name, or class-bound attribute, which indicates
+      Class-bound attribute, which indicates
       what relationship from the instance should be used to reconcile the
       parent/child relationship.
 
-      .. deprecated:: 1.4 Using strings is deprecated and will be removed
-         in SQLAlchemy 2.0.  Please use the class-bound attribute directly.
-
     :param from_entity:
       Entity in which to consider as the left side.  This defaults to the
       "zero" entity of the :class:`_query.Query` itself.
@@ -1871,13 +1868,9 @@ def with_parent(instance, prop, from_entity=None):
 
     """
     if isinstance(prop, str):
-        util.warn_deprecated_20(
-            "Using strings to indicate relationship names in the ORM "
-            "with_parent() function is deprecated and will be removed "
-            "SQLAlchemy 2.0.  Please use the class-bound attribute directly."
+        raise sa_exc.ArgumentError(
+            "with_parent() accepts class-bound mapped attributes, not strings"
         )
-        mapper = object_mapper(instance)
-        prop = getattr(mapper.class_, prop).property
     elif isinstance(prop, attributes.QueryableAttribute):
         if prop._of_type:
             from_entity = prop._of_type
diff --git a/lib/sqlalchemy/sql/coercions.py b/lib/sqlalchemy/sql/coercions.py
index 07da49c4e..38eed4d2a 100644
--- a/lib/sqlalchemy/sql/coercions.py
+++ b/lib/sqlalchemy/sql/coercions.py
@@ -610,20 +610,6 @@ class OnClauseImpl(_CoerceLiterals, _ColumnCoercions, RoleImpl):
 
     _coerce_consts = True
 
-    def _implicit_coercions(
-        self, original_element, resolved, argname=None, legacy=False, **kw
-    ):
-        if legacy and isinstance(resolved, str):
-            return resolved
-        else:
-            return super(OnClauseImpl, self)._implicit_coercions(
-                original_element,
-                resolved,
-                argname=argname,
-                legacy=legacy,
-                **kw
-            )
-
     def _text_coercion(self, element, argname=None, legacy=False):
         if legacy and isinstance(element, str):
             util.warn_deprecated_20(
@@ -925,9 +911,8 @@ class JoinTargetImpl(RoleImpl):
 
     _skip_clauseelement_for_target_match = True
 
-    def _literal_coercion(self, element, legacy=False, **kw):
-        if isinstance(element, str):
-            return element
+    def _literal_coercion(self, element, argname=None, **kw):
+        self._raise_for_expected(element, argname)
 
     def _implicit_coercions(
         self, original_element, resolved, argname=None, legacy=False, **kw
@@ -937,14 +922,6 @@ class JoinTargetImpl(RoleImpl):
             # #6550, unless JoinTargetImpl._skip_clauseelement_for_target_match
             # were set to False.
             return original_element
-        elif legacy and isinstance(resolved, str):
-            util.warn_deprecated_20(
-                "Using strings to indicate relationship names in "
-                "Query.join() is deprecated and will be removed in "
-                "SQLAlchemy 2.0.  Please use the class-bound attribute "
-                "directly."
-            )
-            return resolved
         elif legacy and isinstance(resolved, roles.WhereHavingRole):
             return resolved
         elif legacy and resolved._is_select_statement: