Merge "More descriptive error for non-mapped string prop name"

3 files changed, 65 insertions, 2 deletions

diff --git a/doc/build/changelog/unreleased_13/4589.rst b/doc/build/changelog/unreleased_13/4589.rst
new file mode 100644
index 000000000..8f75bab23
--- /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 b3913ec5b..6c6f0307d 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 b5a6e3b29..cec8865d9 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