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_4_0b1~150^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=aff9d06e00019ee171ea98fbe9ae1687c53a3e25;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
---

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 b3913ec5bf..6c6f0307d8 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 LoaderOption
+from .interfaces import MapperProperty
 from .interfaces import PropComparator
 from .path_registry import _DEFAULT_TOKEN
 from .path_registry import _WILDCARD_TOKEN
@@ -170,6 +171,7 @@ class Load(Generative, LoaderOption):
 
         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
@@ -207,7 +209,21 @@ class Load(Generative, LoaderOption):
                 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 b5a6e3b291..cec8865d9c 100644
--- a/test/orm/test_options.py
+++ b/test/orm/test_options.py
@@ -58,6 +58,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):
@@ -193,6 +200,20 @@ class LoadTest(PathTest, QueryTest):
             "relationship",
         )
 
+    def test_gen_path_attr_str_not_mapped(self):
+        OrderWProp = self.classes.OrderWProp
+
+        sess = Session()
+        q = sess.query(OrderWProp).options(defer("some_attr"))
+
+        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._compile_state,
+        )
+
     def test_gen_path_attr_entity_invalid_noraiseerr(self):
         User = self.classes.User
         Order = self.classes.Order
@@ -1141,7 +1162,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(
@@ -1150,6 +1171,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(
@@ -1215,6 +1245,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