Honor hybrid property / method docstrings

The docstring specified on a hybrid property or method is now honored
at the class level, allowing it to work with tools like Sphinx
autodoc.  The mechanics here necessarily involve some wrapping of
expressions to occur for hybrid properties, which may cause them
to appear differently using introspection.

Fixes: #3653
Change-Id: I02549977fe8b2a051802eed7b00cc532fbc214e3
Pull-request: https://github.com/zzzeek/sqlalchemy/pull/239

5 files changed, 371 insertions, 16 deletions

diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst
index dc7d5105e..426b552e2 100644
--- a/doc/build/changelog/changelog_11.rst
+++ b/doc/build/changelog/changelog_11.rst
@@ -28,6 +28,20 @@
         to Christoph Zwerschke and Kaolin Imago Fire for their efforts.
 
     .. change::
+        :tags: bug, ext
+        :tickets: 3653
+
+        The docstring specified on a hybrid property or method is now honored
+        at the class level, allowing it to work with tools like Sphinx
+        autodoc.  The mechanics here necessarily involve some wrapping of
+        expressions to occur for hybrid properties, which may cause them
+        to appear differently using introspection.
+
+        .. seealso::
+
+            :ref:`change_3653`
+
+    .. change::
         :tags: bug, orm
         :tickets: 3488
 
diff --git a/doc/build/changelog/migration_11.rst b/doc/build/changelog/migration_11.rst
index ac6cf1dc7..be1609130 100644
--- a/doc/build/changelog/migration_11.rst
+++ b/doc/build/changelog/migration_11.rst
@@ -540,6 +540,50 @@ for an attribute being replaced.
 
 :ticket:`3630`
 
+.. _change_3653:
+
+Hybrid properties and methods now propagate the docstring
+---------------------------------------------------------
+
+A hybrid method or property will now reflect the ``__doc__`` value
+present in the original docstring::
+
+    class A(Base):
+        __tablename__ = 'a'
+        id = Column(Integer, primary_key=True)
+
+        name = Column(String)
+
+        @hybrid_property
+        def some_name(self):
+            """The name field"""
+            return self.name
+
+The above value of ``A.some_name.__doc__`` is now honored::
+
+    >>> A.some_name.__doc__
+    The name field
+
+However, to accomplish this, the mechanics of hybrid properties necessarily
+becomes more complex.  Previously, the class-level accessor for a hybrid
+would be a simple pass-thru, that is, this test would succeed::
+
+    >>> assert A.name is A.some_name
+
+With the change, the expression returned by ``A.some_name`` is wrapped inside
+of its own ``QueryableAttribute`` wrapper::
+
+    >>> A.some_name
+    <sqlalchemy.orm.attributes.hybrid_propertyProxy object at 0x7fde03888230>
+
+A lot of testing went into making sure this wrapper works correctly, including
+for elaborate schemes like that of the
+`Custom Value Object <http://techspot.zzzeek.org/2011/10/21/hybrids-and-value-agnostic-types/>`_
+recipe, however we'll be looking to see that no other regressions occur for
+users.
+
+:ticket:`3653`
+
 .. _change_3601:
 
 Session.merge resolves pending conflicts the same as persistent
diff --git a/lib/sqlalchemy/ext/hybrid.py b/lib/sqlalchemy/ext/hybrid.py
index bbf386742..18516eae3 100644
--- a/lib/sqlalchemy/ext/hybrid.py
+++ b/lib/sqlalchemy/ext/hybrid.py
@@ -687,7 +687,7 @@ class hybrid_method(interfaces.InspectionAttrInfo):
 
         """
         self.func = func
-        self.expr = expr or func
+        self.expression(expr or func)
 
     def __get__(self, instance, owner):
         if instance is None:
@@ -700,6 +700,8 @@ class hybrid_method(interfaces.InspectionAttrInfo):
         SQL-expression producing method."""
 
         self.expr = expr
+        if not self.expr.__doc__:
+            self.expr.__doc__ = self.func.__doc__
         return self
 
 
@@ -732,7 +734,7 @@ class hybrid_property(interfaces.InspectionAttrInfo):
         self.fget = fget
         self.fset = fset
         self.fdel = fdel
-        self.expr = expr or fget
+        self.expression(expr or fget)
         util.update_wrapper(self, fget)
 
     def __get__(self, instance, owner):
@@ -768,8 +770,12 @@ class hybrid_property(interfaces.InspectionAttrInfo):
         """Provide a modifying decorator that defines a SQL-expression
         producing method."""
 
-        self.expr = expr
-        return self
+        def _expr(cls):
+            return ExprComparator(expr(cls))
+        util.update_wrapper(_expr, expr)
+
+        self.expr = _expr
+        return self.comparator(_expr)
 
     def comparator(self, comparator):
         """Provide a modifying decorator that defines a custom
@@ -784,7 +790,9 @@ class hybrid_property(interfaces.InspectionAttrInfo):
             create_proxied_attribute(self)
 
         def expr(owner):
-            return proxy_attr(owner, self.__name__, self, comparator(owner))
+            return proxy_attr(
+                owner, self.__name__, self, comparator(owner),
+                doc=comparator.__doc__ or self.__doc__)
         self.expr = expr
         return self
 
@@ -808,3 +816,15 @@ class Comparator(interfaces.PropComparator):
     def adapt_to_entity(self, adapt_to_entity):
         # interesting....
         return self
+
+
+class ExprComparator(Comparator):
+
+    def __getattr__(self, key):
+        return getattr(self.expression, key)
+
+    def operate(self, op, *other, **kwargs):
+        return op(self.expression, *other, **kwargs)
+
+    def reverse_operate(self, op, other, **kwargs):
+        return op(other, self.expression, **kwargs)
diff --git a/test/ext/test_hybrid.py b/test/ext/test_hybrid.py
index e36b8f7e9..dac65dbab 100644
--- a/test/ext/test_hybrid.py
+++ b/test/ext/test_hybrid.py
@@ -1,11 +1,13 @@
-from sqlalchemy import func, Integer, String, ForeignKey
+from sqlalchemy import func, Integer, Numeric, String, ForeignKey
 from sqlalchemy.orm import relationship, Session, aliased
 from sqlalchemy.testing.schema import Column
 from sqlalchemy.ext.declarative import declarative_base
 from sqlalchemy.ext import hybrid
-from sqlalchemy.testing import eq_, AssertsCompiledSQL, assert_raises_message
+from sqlalchemy.testing import eq_, is_, AssertsCompiledSQL, \
+    assert_raises_message
 from sqlalchemy.testing import fixtures
 from sqlalchemy import inspect
+from decimal import Decimal
 
 
 class PropertyComparatorTest(fixtures.TestBase, AssertsCompiledSQL):
@@ -30,6 +32,7 @@ class PropertyComparatorTest(fixtures.TestBase, AssertsCompiledSQL):
 
             @hybrid.hybrid_property
             def value(self):
+                "This is a docstring"
                 return self._value - 5
 
             @value.comparator
@@ -81,8 +84,14 @@ class PropertyComparatorTest(fixtures.TestBase, AssertsCompiledSQL):
             "FROM a AS a_1 WHERE upper(a_1.value) = upper(:upper_1)"
         )
 
+    def test_docstring(self):
+        A = self._fixture()
+        eq_(A.value.__doc__, "This is a docstring")
+
+
 class PropertyExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
     __dialect__ = 'default'
+
     def _fixture(self):
         Base = declarative_base()
 
@@ -93,10 +102,12 @@ class PropertyExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
 
             @hybrid.hybrid_property
             def value(self):
+                "This is an instance-level docstring"
                 return int(self._value) - 5
 
             @value.expression
             def value(cls):
+                "This is a class-level docstring"
                 return func.foo(cls._value) + cls.bar_value
 
             @value.setter
@@ -159,7 +170,7 @@ class PropertyExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
     def test_expression(self):
         A = self._fixture()
         self.assert_compile(
-            A.value,
+            A.value.__clause_element__(),
             "foo(a.value) + bar(a.value)"
         )
 
@@ -177,7 +188,7 @@ class PropertyExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
     def test_aliased_expression(self):
         A = self._fixture()
         self.assert_compile(
-            aliased(A).value,
+            aliased(A).value.__clause_element__(),
             "foo(a_1.value) + bar(a_1.value)"
         )
 
@@ -199,8 +210,18 @@ class PropertyExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
             "FROM a AS a_1 WHERE foo(a_1.value) + bar(a_1.value) = :param_1"
         )
 
+    def test_docstring(self):
+        A = self._fixture()
+        eq_(A.value.__doc__, "This is a class-level docstring")
+
+        # no docstring here since we get a literal
+        a1 = A(_value=10)
+        eq_(a1.value, 5)
+
+
 class PropertyValueTest(fixtures.TestBase, AssertsCompiledSQL):
     __dialect__ = 'default'
+
     def _fixture(self, assignable):
         Base = declarative_base()
 
@@ -238,15 +259,16 @@ class PropertyValueTest(fixtures.TestBase, AssertsCompiledSQL):
             delattr, a1, 'value'
         )
 
-
     def test_set_get(self):
         A = self._fixture(True)
         a1 = A(value=5)
         eq_(a1.value, 5)
         eq_(a1._value, 10)
 
+
 class MethodExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
     __dialect__ = 'default'
+
     def _fixture(self):
         Base = declarative_base()
 
@@ -257,10 +279,21 @@ class MethodExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
 
             @hybrid.hybrid_method
             def value(self, x):
+                "This is an instance-level docstring"
                 return int(self._value) + x
 
             @value.expression
             def value(cls, value):
+                "This is a class-level docstring"
+                return func.foo(cls._value, value) + value
+
+            @hybrid.hybrid_method
+            def other_value(self, x):
+                "This is an instance-level docstring"
+                return int(self._value) + x
+
+            @other_value.expression
+            def other_value(cls, value):
                 return func.foo(cls._value, value) + value
 
         return A
@@ -285,7 +318,6 @@ class MethodExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
             {"some key": "some value"}
         )
 
-
     def test_aliased_expression(self):
         A = self._fixture()
         self.assert_compile(
@@ -327,3 +359,234 @@ class MethodExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
             sess.query(aliased(A).value(5)),
             "SELECT foo(a_1.value, :foo_1) + :foo_2 AS anon_1 FROM a AS a_1"
         )
+
+    def test_docstring(self):
+        A = self._fixture()
+        eq_(A.value.__doc__, "This is a class-level docstring")
+        eq_(A.other_value.__doc__, "This is an instance-level docstring")
+        a1 = A(_value=10)
+
+        # a1.value is still a method, so it has a
+        # docstring
+        eq_(a1.value.__doc__, "This is an instance-level docstring")
+
+        eq_(a1.other_value.__doc__, "This is an instance-level docstring")
+
+
+class SpecialObjectTest(fixtures.TestBase, AssertsCompiledSQL):
+    """tests against hybrids that return a non-ClauseElement.
+
+    use cases derived from the example at
+    http://techspot.zzzeek.org/2011/10/21/hybrids-and-value-agnostic-types/
+
+    """
+    __dialect__ = 'default'
+
+    @classmethod
+    def setup_class(cls):
+        from sqlalchemy import literal
+
+        symbols = ('usd', 'gbp', 'cad', 'eur', 'aud')
+        currency_lookup = dict(
+            ((currency_from, currency_to), Decimal(str(rate)))
+            for currency_to, values in zip(
+                symbols,
+                [
+                    (1, 1.59009, 0.988611, 1.37979, 1.02962),
+                    (0.628895, 1, 0.621732, 0.867748, 0.647525),
+                    (1.01152, 1.6084, 1, 1.39569, 1.04148),
+                    (0.724743, 1.1524, 0.716489, 1, 0.746213),
+                    (0.971228, 1.54434, 0.960166, 1.34009, 1),
+                ])
+            for currency_from, rate in zip(symbols, values)
+        )
+
+        class Amount(object):
+            def __init__(self, amount, currency):
+                self.currency = currency
+                self.amount = amount
+
+            def __add__(self, other):
+                return Amount(
+                    self.amount +
+                    other.as_currency(self.currency).amount,
+                    self.currency
+                )
+
+            def __sub__(self, other):
+                return Amount(
+                    self.amount -
+                    other.as_currency(self.currency).amount,
+                    self.currency
+                )
+
+            def __lt__(self, other):
+                return self.amount < other.as_currency(self.currency).amount
+
+            def __gt__(self, other):
+                return self.amount > other.as_currency(self.currency).amount
+
+            def __eq__(self, other):
+                return self.amount == other.as_currency(self.currency).amount
+
+            def as_currency(self, other_currency):
+                return Amount(
+                    currency_lookup[(self.currency, other_currency)] *
+                    self.amount,
+                    other_currency
+                )
+
+            def __clause_element__(self):
+                # helper method for SQLAlchemy to interpret
+                # the Amount object as a SQL element
+                if isinstance(self.amount, (float, int, Decimal)):
+                    return literal(self.amount)
+                else:
+                    return self.amount
+
+            def __str__(self):
+                return "%2.4f %s" % (self.amount, self.currency)
+
+            def __repr__(self):
+                return "Amount(%r, %r)" % (self.amount, self.currency)
+
+        Base = declarative_base()
+
+        class BankAccount(Base):
+            __tablename__ = 'bank_account'
+            id = Column(Integer, primary_key=True)
+
+            _balance = Column('balance', Numeric)
+
+            @hybrid.hybrid_property
+            def balance(self):
+                """Return an Amount view of the current balance."""
+                return Amount(self._balance, "usd")
+
+            @balance.setter
+            def balance(self, value):
+                self._balance = value.as_currency("usd").amount
+
+        cls.Amount = Amount
+        cls.BankAccount = BankAccount
+
+    def test_instance_one(self):
+        BankAccount, Amount = self.BankAccount, self.Amount
+        account = BankAccount(balance=Amount(4000, "usd"))
+
+        # 3b. print balance in usd
+        eq_(account.balance.amount, 4000)
+
+    def test_instance_two(self):
+        BankAccount, Amount = self.BankAccount, self.Amount
+        account = BankAccount(balance=Amount(4000, "usd"))
+
+        # 3c. print balance in gbp
+        eq_(account.balance.as_currency("gbp").amount, Decimal('2515.58'))
+
+    def test_instance_three(self):
+        BankAccount, Amount = self.BankAccount, self.Amount
+        account = BankAccount(balance=Amount(4000, "usd"))
+
+        # 3d. perform currency-agnostic comparisons, math
+        is_(account.balance > Amount(500, "cad"), True)
+
+    def test_instance_four(self):
+        BankAccount, Amount = self.BankAccount, self.Amount
+        account = BankAccount(balance=Amount(4000, "usd"))
+        eq_(
+            account.balance + Amount(500, "cad") - Amount(50, "eur"),
+            Amount(Decimal("4425.316"), "usd")
+        )
+
+    def test_query_one(self):
+        BankAccount, Amount = self.BankAccount, self.Amount
+        session = Session()
+
+        query = session.query(BankAccount).\
+            filter(BankAccount.balance == Amount(10000, "cad"))
+
+        self.assert_compile(
+            query,
+            "SELECT bank_account.balance AS bank_account_balance, "
+            "bank_account.id AS bank_account_id FROM bank_account "
+            "WHERE bank_account.balance = :balance_1",
+            checkparams={'balance_1': Decimal('9886.110000')}
+        )
+
+    def test_query_two(self):
+        BankAccount, Amount = self.BankAccount, self.Amount
+        session = Session()
+
+        # alternatively we can do the calc on the DB side.
+        query = session.query(BankAccount).\
+            filter(
+                BankAccount.balance.as_currency("cad") > Amount(9999, "cad")).\
+            filter(
+                BankAccount.balance.as_currency("cad") < Amount(10001, "cad"))
+        self.assert_compile(
+            query,
+            "SELECT bank_account.balance AS bank_account_balance, "
+            "bank_account.id AS bank_account_id "
+            "FROM bank_account "
+            "WHERE :balance_1 * bank_account.balance > :param_1 "
+            "AND :balance_2 * bank_account.balance < :param_2",
+            checkparams={
+                'balance_1': Decimal('1.01152'),
+                'balance_2': Decimal('1.01152'),
+                'param_1': Decimal('9999'),
+                'param_2': Decimal('10001')}
+        )
+
+    def test_query_three(self):
+        BankAccount = self.BankAccount
+        session = Session()
+
+        query = session.query(BankAccount).\
+            filter(
+                BankAccount.balance.as_currency("cad") >
+                BankAccount.balance.as_currency("eur"))
+        self.assert_compile(
+            query,
+            "SELECT bank_account.balance AS bank_account_balance, "
+            "bank_account.id AS bank_account_id FROM bank_account "
+            "WHERE :balance_1 * bank_account.balance > "
+            ":param_1 * :balance_2 * bank_account.balance",
+            checkparams={
+                'balance_1': Decimal('1.01152'),
+                'balance_2': Decimal('0.724743'),
+                'param_1': Decimal('1.39569')}
+        )
+
+    def test_query_four(self):
+        BankAccount = self.BankAccount
+        session = Session()
+
+        # 4c. query all amounts, converting to "CAD" on the DB side
+        query = session.query(BankAccount.balance.as_currency("cad").amount)
+        self.assert_compile(
+            query,
+            "SELECT :balance_1 * bank_account.balance AS anon_1 "
+            "FROM bank_account",
+            checkparams={'balance_1': Decimal('1.01152')}
+        )
+
+    def test_query_five(self):
+        BankAccount = self.BankAccount
+        session = Session()
+
+        # 4d. average balance in EUR
+        query = session.query(func.avg(BankAccount.balance.as_currency("eur")))
+        self.assert_compile(
+            query,
+            "SELECT avg(:balance_1 * bank_account.balance) AS avg_1 "
+            "FROM bank_account",
+            checkparams={'balance_1': Decimal('0.724743')}
+        )
+
+    def test_docstring(self):
+        BankAccount = self.BankAccount
+        eq_(
+            BankAccount.balance.__doc__,
+            "Return an Amount view of the current balance.")
+
diff --git a/test/orm/test_utils.py b/test/orm/test_utils.py
index 168cee19c..3c783c03d 100644
--- a/test/orm/test_utils.py
+++ b/test/orm/test_utils.py
@@ -159,8 +159,10 @@ class AliasedClassTest(fixtures.TestBase, AssertsCompiledSQL):
         self._fixture(Point)
         alias = aliased(Point)
 
-        eq_(str(Point.double_x), "point.x * :x_1")
-        eq_(str(alias.double_x), "point_1.x * :x_1")
+        eq_(str(Point.double_x), "Point.double_x")
+        eq_(str(alias.double_x), "AliasedClass_Point.double_x")
+        eq_(str(Point.double_x.__clause_element__()), "point.x * :x_1")
+        eq_(str(alias.double_x.__clause_element__()), "point_1.x * :x_1")
 
         sess = Session()
 
@@ -183,10 +185,22 @@ class AliasedClassTest(fixtures.TestBase, AssertsCompiledSQL):
         self._fixture(Point)
         alias = aliased(Point)
 
-        eq_(str(Point.x_alone), "Point.x")
-        eq_(str(alias.x_alone), "AliasedClass_Point.x")
+        eq_(str(Point.x_alone), "Point.x_alone")
+        eq_(str(alias.x_alone), "AliasedClass_Point.x_alone")
 
-        assert Point.x_alone is Point.x
+        # from __clause_element__() perspective, Point.x_alone
+        # and Point.x return the same thing, so that's good
+        eq_(str(Point.x.__clause_element__()), "point.x")
+        eq_(str(Point.x_alone.__clause_element__()), "point.x")
+
+        # same for the alias
+        eq_(str(alias.x + 1), "point_1.x + :x_1")
+        eq_(str(alias.x_alone + 1), "point_1.x + :x_1")
+
+        is_(
+            Point.x_alone.__clause_element__(),
+            Point.x.__clause_element__()
+        )
 
         eq_(str(alias.x_alone == alias.x), "point_1.x = point_1.x")