From: jonathan vanasco <jonathan@2xlp.com>
Date: Mon, 20 Apr 2020 17:56:49 +0000 (-0400)
Subject: More descriptive error for non-mapped string prop name
X-Git-Tag: rel_1_3_20~36^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5e691f91d2fa704ffe2de58f2b7581eb5ae837d3;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

More descriptive error for non-mapped string prop name

Fixed issue where using a loader option against a string attribute name
that is not actually a mapped attribute, such as a plain Python descriptor,
would raise an uninformative AttributeError;  a descriptive error is now
raised.

Fixes: #4589
Closes: #4594
Pull-request: https://github.com/sqlalchemy/sqlalchemy/pull/4594
Pull-request-sha: 2b7ed5240f49be90f9390e3d041c9cb957083465

Change-Id: I66b9937991eb7cdbe074a92f490af1c80d16449e
(cherry picked from commit 7700005a1f3504e26633a55db5cddeac114cb78f)
---

diff --git a/doc/build/changelog/unreleased_13/4589.rst b/doc/build/changelog/unreleased_13/4589.rst
new file mode 100644
index 0000000000..8f75bab232
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4589.rst
@@ -0,0 +1,10 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4589
+
+    Fixed issue where using a loader option against a string attribute name
+    that is not actually a mapped attribute, such as a plain Python descriptor,
+    would raise an uninformative AttributeError;  a descriptive error is now
+    raised.
+
+
diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py
index 1d9b92294b..cba0e7e80f 100644
--- a/lib/sqlalchemy/orm/strategy_options.py
+++ b/lib/sqlalchemy/orm/strategy_options.py
@@ -15,6 +15,7 @@ from .base import _is_aliased_class
 from .base import _is_mapped_class
 from .base import InspectionAttr
 from .interfaces import MapperOption
+from .interfaces import MapperProperty
 from .interfaces import PropComparator
 from .path_registry import _DEFAULT_TOKEN
 from .path_registry import _WILDCARD_TOKEN
@@ -205,6 +206,7 @@ class Load(Generative, MapperOption):
 
         if isinstance(attr, util.string_types):
             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
@@ -242,7 +244,21 @@ class Load(Generative, MapperOption):
                 else:
                     return None
             else:
-                attr = found_property = attr.property
+                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))
+                            ),
+                            replace_context=ae,
+                        )
+                    else:
+                        raise
 
             path = path[attr]
         elif _is_mapped_class(attr):
diff --git a/test/orm/test_options.py b/test/orm/test_options.py
index 84c8e5c0d2..183e7ba241 100644
--- a/test/orm/test_options.py
+++ b/test/orm/test_options.py
@@ -59,6 +59,13 @@ class QueryTest(_fixtures.FixtureTest):
             },
         )
 
+        class OrderWProp(cls.classes.Order):
+            @property
+            def some_attr(self):
+                return "hi"
+
+        mapper(OrderWProp, None, inherits=cls.classes.Order)
+
 
 class PathTest(object):
     def _make_path(self, path):
@@ -191,6 +198,21 @@ class LoadTest(PathTest, QueryTest):
             "relationship",
         )
 
+    def test_gen_path_attr_str_not_mapped(self):
+        OrderWProp = self.classes.OrderWProp
+
+        sess = Session()
+        q = sess.query(OrderWProp)
+
+        assert_raises_message(
+            sa.exc.ArgumentError,
+            r"Expected attribute \"some_attr\" on mapped class "
+            "OrderWProp->orders to be a mapped attribute; instead "
+            "got .*property.* object.",
+            q.options,
+            defer("some_attr"),
+        )
+
     def test_gen_path_attr_entity_invalid_noraiseerr(self):
         User = self.classes.User
         Order = self.classes.Order
@@ -1139,7 +1161,7 @@ class OptionsNoPropTest(_fixtures.FixtureTest):
             'column property "Keyword.keywords"',
         )
 
-    def test_wrong_type_in_option(self):
+    def test_wrong_type_in_option_cls(self):
         Item = self.classes.Item
         Keyword = self.classes.Keyword
         self._assert_eager_with_entity_exception(
@@ -1148,6 +1170,15 @@ class OptionsNoPropTest(_fixtures.FixtureTest):
             r"mapper option expects string key or list of attributes",
         )
 
+    def test_wrong_type_in_option_descriptor(self):
+        OrderWProp = self.classes.OrderWProp
+
+        self._assert_eager_with_entity_exception(
+            [OrderWProp],
+            (joinedload(OrderWProp.some_attr),),
+            r"mapper option expects string key or list of attributes",
+        )
+
     def test_non_contiguous_all_option(self):
         User = self.classes.User
         self._assert_eager_with_entity_exception(
@@ -1213,6 +1244,13 @@ class OptionsNoPropTest(_fixtures.FixtureTest):
             ),
         )
 
+        class OrderWProp(cls.classes.Order):
+            @property
+            def some_attr(self):
+                return "hi"
+
+        mapper(OrderWProp, None, inherits=cls.classes.Order)
+
     def _assert_option(self, entity_list, option):
         Item = self.classes.Item