From c88540d5a5476a0c261c1b9e7c2b1d13fd82a9a1 Mon Sep 17 00:00:00 2001 From: Anurag Awasthi Date: Mon, 8 Jul 2019 17:05:47 +0530 Subject: [PATCH] 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 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 extends Compone } } - List 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 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 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 --- .../com/cloud/utils/db/GenericDaoBase.java | 40 +++++++++---------- .../main/java/com/cloud/utils/db/GroupBy.java | 12 +++--- .../java/com/cloud/utils/db/SqlGenerator.java | 8 ++++ 3 files changed, 34 insertions(+), 26 deletions(-) diff --git a/framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java b/framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java index 442d3cc181d..fb923c6f068 100644 --- a/framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java +++ b/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 extends Compone } } - // we have to disable group by in getting count, since count for groupBy clause will be different. - //List groupByValues = addGroupBy(str, sc); final TransactionLegacy txn = TransactionLegacy.currentTxn(); final String sql = str.toString(); @@ -2033,14 +2032,6 @@ public abstract class GenericDaoBase 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 extends Compone @DB() protected StringBuilder createCountSelect(SearchCriteria sc, final boolean whereClause) { StringBuilder sql = new StringBuilder(_count); + if (sc != null) { + Pair, List> 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()); diff --git a/framework/db/src/main/java/com/cloud/utils/db/GroupBy.java b/framework/db/src/main/java/com/cloud/utils/db/GroupBy.java index 60b59baef96..e87b1cc6675 100644 --- a/framework/db/src/main/java/com/cloud/utils/db/GroupBy.java +++ b/framework/db/src/main/java/com/cloud/utils/db/GroupBy.java @@ -63,6 +63,13 @@ public class GroupBy, 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 groupBy : _groupBys) { if (groupBy.first() != null) { String func = groupBy.first().toString(); @@ -71,14 +78,9 @@ public class GroupBy, 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 { diff --git a/framework/db/src/main/java/com/cloud/utils/db/SqlGenerator.java b/framework/db/src/main/java/com/cloud/utils/db/SqlGenerator.java index 516849650f1..e65fd8a5796 100644 --- a/framework/db/src/main/java/com/cloud/utils/db/SqlGenerator.java +++ b/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();