You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ro...@apache.org on 2019/07/08 11:35:57 UTC
[cloudstack] branch master updated: framework/db: Fix bug in
counting items for search query (#3457)
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 <an...@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 <ro...@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();