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 87ca1cb310a6c0720958e08a3c771c97d83ec89c..7a41731bff0eac7d97b9dad99c5b8744abba6d11 100644 (file)
--- 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 e2aaec4ecfe9610d80d22f65d96f636bfe508dcf..0ad2d983b5fcb9d2380ec25b2e96a9b7306affa1 100644 (file)
--- 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 673085cf77b2934426c81aad6d8b42bf94ab1fa7..dd3178eb5fa4f44c3cf9fe95e2ddd889750ca372 100644 (file)
--- 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 ed014af3e27cedecd7d586d311273617db017aec..d3c2325d773ff78ff18759545370d8fa94b6823d 100644 (file)
--- 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 281fa95ba6e7bbb1b971711983fa66d784f4ced8..ca21b462914b377b1dbbe6f28cdb87ed89b9cc7c 100644 (file)
--- 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):