From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Mon, 13 Jun 2016 19:18:13 +0000 (-0400)
Subject: Deprecate FromClause.count() (pending for 1.1)
X-Git-Tag: rel_1_0_14~10
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e7ea2a4e198f3203b6f8e13649cd5bf5ea4ad172;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Deprecate FromClause.count() (pending for 1.1)

count() here is misleading in that it not only
counts from an arbitrary column in the table, it also
does not make accommodations for DISTINCT, JOIN, etc.
as the ORM-level function does.  Core should not be
attempting to provide a function like this.

Change-Id: I9916fc51ef744389a92c54660ab08e9695b8afc2
Fixes: #3724
---

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index 87ca1cb310..7a41731bff 100644
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -18,6 +18,14 @@
 .. changelog::
     :version: 1.0.14
 
+    .. change::
+        :tags: bug, sql
+        :tickets: 3724
+
+        :meth:`.FromClause.count` is pending deprecation for 1.1.  This function
+        makes use of an arbitrary column in the table and is not reliable;
+        for Core use, ``func.count()`` should be preferred.
+
     .. change::
         :tags: bug, sql
         :tickets: 3722
diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py
index e2aaec4ecf..0ad2d983b5 100644
--- a/lib/sqlalchemy/sql/selectable.py
+++ b/lib/sqlalchemy/sql/selectable.py
@@ -286,10 +286,35 @@ class FromClause(Selectable):
 
     _memoized_property = util.group_expirable_memoized_property(["_columns"])
 
+    @util.pending_deprecation(
+        '1.1',
+        message="``Table.count()`` is deprecated. Counting "
+        "rows requires that the correct column expression and "
+        "accommodations for joins, DISTINCT, etc. must be made, "
+        "otherwise results may not be what's expected. "
+        "Please use an appropriate ``func.count()`` expression "
+        "directly."
+    )
     @util.dependencies("sqlalchemy.sql.functions")
     def count(self, functions, whereclause=None, **params):
         """return a SELECT COUNT generated against this
-        :class:`.FromClause`."""
+        :class:`.FromClause`.
+
+        The function generates COUNT against the
+        first column in the primary key of the table, or against
+        the first column in the table overall.   Explicit use of
+        ``func.count()`` should be preferred::
+
+            row_count = conn.scalar(
+                select([func.count('*')]).select_from(table)
+            )
+
+
+        .. seealso::
+
+            :data:`.func`
+
+        """
 
         if self.primary_key:
             col = list(self.primary_key)[0]
@@ -1361,21 +1386,6 @@ class TableClause(Immutable, FromClause):
         else:
             return []
 
-    @util.dependencies("sqlalchemy.sql.functions")
-    def count(self, functions, whereclause=None, **params):
-        """return a SELECT COUNT generated against this
-        :class:`.TableClause`."""
-
-        if self.primary_key:
-            col = list(self.primary_key)[0]
-        else:
-            col = list(self.columns)[0]
-        return Select(
-            [functions.func.count(col).label('tbl_row_count')],
-            whereclause,
-            from_obj=[self],
-            **params)
-
     @util.dependencies("sqlalchemy.sql.dml")
     def insert(self, dml, values=None, inline=False, **kwargs):
         """Generate an :func:`.insert` construct against this
diff --git a/test/sql/test_defaults.py b/test/sql/test_defaults.py
index 673085cf77..dd3178eb5f 100644
--- a/test/sql/test_defaults.py
+++ b/test/sql/test_defaults.py
@@ -774,7 +774,8 @@ class AutoIncrementTest(fixtures.TablesTest):
 
         testing.db.execute(dataset_no_autoinc.insert())
         eq_(
-            testing.db.scalar(dataset_no_autoinc.count()), 1
+            testing.db.scalar(
+                select([func.count('*')]).select_from(dataset_no_autoinc)), 1
         )
 
 
diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py
index ed014af3e2..d3c2325d77 100644
--- a/test/sql/test_metadata.py
+++ b/test/sql/test_metadata.py
@@ -21,18 +21,6 @@ from sqlalchemy import util
 
 class MetaDataTest(fixtures.TestBase, ComparesTables):
 
-    def test_metadata_connect(self):
-        metadata = MetaData()
-        t1 = Table('table1', metadata,
-                   Column('col1', Integer, primary_key=True),
-                   Column('col2', String(20)))
-        metadata.bind = testing.db
-        metadata.create_all()
-        try:
-            assert t1.count().scalar() == 0
-        finally:
-            metadata.drop_all()
-
     def test_metadata_contains(self):
         metadata = MetaData()
         t1 = Table('t1', metadata, Column('x', Integer))
diff --git a/test/sql/test_types.py b/test/sql/test_types.py
index 281fa95ba6..ca21b46291 100644
--- a/test/sql/test_types.py
+++ b/test/sql/test_types.py
@@ -1247,8 +1247,8 @@ class BinaryTest(fixtures.TestBase, AssertsExecutionResults):
         data = os.urandom(32)
         binary_table.insert().execute(data=data)
         eq_(
-            binary_table.select().where(binary_table.c.data == data).alias().
-            count().scalar(), 1)
+            select([func.count('*')]).select_from(binary_table).
+            where(binary_table.c.data == data).scalar(), 1)
 
     @testing.requires.binary_literals
     def test_literal_roundtrip(self):