framework/db: Fix bug in counting items for search query (#3457)

The implementation of GenericBaseDao#searchAndCount() can result in
incorrect count. This happens because the implementation of the
GenericBaseDao#getCount method excludes the groupBy components of the
search queries. This means the count returned will always be larger than
the actual count when not considering pagination.

The change was brought in b0ce8fd which also fails to explain the rationale.

Further investigation of the getCount usage reveal they are always
accompanied by search queries which include groupBy components via
GenericBaseDao#searchIncludingRemoved method.

```
Current code diff between search and count methods is as follows -
diff --git a/framework/db/src/main/java/com/cloud/uddtils/db/GenericDaoBase.java b/framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
@@ -371,7 +371,7 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone
             clause = null;
         }

-        final StringBuilder str = createPartialSelectSql(sc, clause != null, enableQueryCache);
+        final StringBuilder str = createCountSelect(sc, clause != null);
@@ -384,19 +384,12 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone
             }
         }

-        List<Object> groupByValues = addGroupBy(str, sc);
-        addFilter(str, filter);
-
+        // we have to disable group by in getting count, since count for groupBy clause will be different.
+        //List<Object> groupByValues = addGroupBy(str, sc);
         final TransactionLegacy txn = TransactionLegacy.currentTxn();
-        if (lock != null) {
-            assert (txn.dbTxnStarted() == true) : "As nice as I can here now....how do you lock when there's no DB transaction?  Review your db 101 course from college.";
-            str.append(lock ? FOR_UPDATE_CLAUSE : SHARE_MODE_CLAUSE);
-        }
-
@@ -410,20 +403,19 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone

+            /*
             if (groupByValues != null) {
                 for (Object value : groupByValues) {
                     pstmt.setObject(i++, value);
                 }
             }
+             */

-            if (s_logger.isDebugEnabled() && lock != null) {
-                txn.registerLock(pstmt.toString());
-            }
             final ResultSet rs = pstmt.executeQuery();
             while (rs.next()) {
-                result.add(toEntityBean(rs, cache));
+                return rs.getInt(1);
             }
-            return result;
+            return 0;
--
2.17.1
```

The fix is to update the way we setup query for counting with group by
params.

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java

@@ -44,7 +44,7 @@ import java.util.Map;
 import java.util.TimeZone;
 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
-import com.google.common.base.Strings;
+
 import javax.naming.ConfigurationException;
 import javax.persistence.AttributeOverride;
 import javax.persistence.Column;
@@ -55,16 +55,6 @@ import javax.persistence.Enumerated;
 import javax.persistence.Table;
 import javax.persistence.TableGenerator;
 
-import net.sf.cglib.proxy.Callback;
-import net.sf.cglib.proxy.CallbackFilter;
-import net.sf.cglib.proxy.Enhancer;
-import net.sf.cglib.proxy.Factory;
-import net.sf.cglib.proxy.MethodInterceptor;
-import net.sf.cglib.proxy.NoOp;
-import net.sf.ehcache.Cache;
-import net.sf.ehcache.CacheManager;
-import net.sf.ehcache.Element;
-
 import org.apache.log4j.Logger;
 
 import com.cloud.utils.DateUtil;
@@ -79,6 +69,17 @@ import com.cloud.utils.db.SearchCriteria.SelectType;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.utils.net.Ip;
 import com.cloud.utils.net.NetUtils;
+import com.google.common.base.Strings;
+
+import net.sf.cglib.proxy.Callback;
+import net.sf.cglib.proxy.CallbackFilter;
+import net.sf.cglib.proxy.Enhancer;
+import net.sf.cglib.proxy.Factory;
+import net.sf.cglib.proxy.MethodInterceptor;
+import net.sf.cglib.proxy.NoOp;
+import net.sf.ehcache.Cache;
+import net.sf.ehcache.CacheManager;
+import net.sf.ehcache.Element;
 
 /**
  *  GenericDaoBase is a simple way to implement DAOs.  It DOES NOT
@@ -2014,8 +2015,6 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone
             }
         }
 
-        // we have to disable group by in getting count, since count for groupBy clause will be different.
-        //List<Object> groupByValues = addGroupBy(str, sc);
         final TransactionLegacy txn = TransactionLegacy.currentTxn();
         final String sql = str.toString();
 
@@ -2033,14 +2032,6 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone
                 i = addJoinAttributes(i, pstmt, joins);
             }
 
-            /*
-            if (groupByValues != null) {
-                for (Object value : groupByValues) {
-                    pstmt.setObject(i++, value);
-                }
-            }
-             */
-
             final ResultSet rs = pstmt.executeQuery();
             while (rs.next()) {
                 return rs.getInt(1);
@@ -2056,6 +2047,13 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone
     @DB()
     protected StringBuilder createCountSelect(SearchCriteria<?> sc, final boolean whereClause) {
         StringBuilder sql = new StringBuilder(_count);
+        if (sc != null) {
+            Pair<GroupBy<?, ?, ?>, List<Object>> groupBys = sc.getGroupBy();
+            if (groupBys != null) {
+                final SqlGenerator generator = new SqlGenerator(_entityBeanType);
+                sql = new StringBuilder(generator.buildCountSqlWithGroupBy(groupBys.first()));
+            }
+        }
 
         if (!whereClause) {
             sql.delete(sql.length() - (_discriminatorClause == null ? 6 : 4), sql.length());

framework/db/src/main/java/com/cloud/utils/db/GroupBy.java

@@ -63,6 +63,13 @@ public class GroupBy<J extends SearchBase<?, T, R>, T, R> {
 
     public void toSql(final StringBuilder builder) {
         builder.append(" GROUP BY ");
+        appendGroupByAttributes(builder);
+        if (_having != null) {
+            _having.toSql(builder);
+        }
+    }
+
+    public void appendGroupByAttributes(final StringBuilder builder) {
         for (final Pair<Func, Attribute> groupBy : _groupBys) {
             if (groupBy.first() != null) {
                 String func = groupBy.first().toString();
@@ -71,14 +78,9 @@ public class GroupBy<J extends SearchBase<?, T, R>, T, R> {
             } else {
                 builder.append(groupBy.second().table + "." + groupBy.second().columnName);
             }
-
             builder.append(", ");
         }
-
         builder.delete(builder.length() - 2, builder.length());
-        if (_having != null) {
-            _having.toSql(builder);
-        }
     }
 
     protected class Having {

framework/db/src/main/java/com/cloud/utils/db/SqlGenerator.java

@@ -675,6 +675,14 @@ public class SqlGenerator {
         return sql.append("SELECT COUNT(*) FROM ").append(buildTableReferences()).append(" WHERE ").append(buildDiscriminatorClause().first()).toString();
     }
 
+    public String buildCountSqlWithGroupBy(final GroupBy groupBy) {
+        StringBuilder sql = new StringBuilder();
+        sql.append("SELECT COUNT(DISTINCT ");
+        groupBy.appendGroupByAttributes(sql);
+        sql.append(") FROM ").append(buildTableReferences()).append(" WHERE ").append(buildDiscriminatorClause().first());
+        return sql.toString();
+    }
+
     public String buildDistinctIdSql() {
         StringBuilder sql = new StringBuilder();