This is an automated email from the ASF dual-hosted git repository. rohit pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/master by this push: new c88540d framework/db: Fix bug in counting items for search query (#3457) c88540d is described below commit c88540d5a5476a0c261c1b9e7c2b1d13fd82a9a1 Author: Anurag Awasthi <anurag.awas...@shapeblue.com> AuthorDate: Mon Jul 8 17:05:47 2019 +0530 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.ya...@shapeblue.com> --- .../java/com/cloud/utils/db/GenericDaoBase.java | 40 ++++++++++------------ .../src/main/java/com/cloud/utils/db/GroupBy.java | 12 ++++--- .../main/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 442d3cc..fb923c6 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<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()); 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 60b59ba..e87b1cc 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<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 { 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 5168496..e65fd8a 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();