modules/core/domain/src/main/java/org/rhq/core/domain/criteria/Criteria.java
| 47 +++++++++-
modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftChangeSetCriteria.java
| 9 -
modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftCriteria.java
| 9 -
modules/core/domain/src/main/java/org/rhq/core/domain/criteria/MeasurementDataTraitCriteria.java
| 12 +-
modules/core/domain/src/main/java/org/rhq/core/domain/util/PageControl.java
| 34 ++++++-
modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/jaas/DelegatingLoginModule.java
| 10 +-
modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQuery.java
| 15 ---
modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQueryGenerator.java
| 10 ++
modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/util/CriteriaQueryTest.java
| 22 ++++
9 files changed, 126 insertions(+), 42 deletions(-)
New commits:
commit 471b8a20f984f79d448acf58c01ec8c38cee8e67
Author: Jay Shaughnessy <jshaughn(a)redhat.com>
Date: Fri May 24 19:45:41 2013 -0400
Bug 966665
- Criteria queries with paging and sorting on non unique fields may return same item
on different pages
All paged criteria queries now apply ID as the least significant
ordering field (see exceptions below). Since ID is unique, this ensures
that queries for pages always return the same ordering of rows.
ID will not be automatically appended as an ordering field if:
- the paging is unlimited (i.e. no pagesize, all rows returned)
- the Criteria class does not support an ID sort
- the caller has set the Criteria class instance to not support the ID sort
- ID is already specified as an ordering field
- the max number of ordering fields has already been set (3)
Note that CriteriaQuery has had its support for implicit ID sorting removed,
as the new mechanism, which moves support to the CriteriaQueryGenerator,
replaces it.
diff --git a/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/Criteria.java
b/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/Criteria.java
index 2dde3a4..d8552de 100644
--- a/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/Criteria.java
+++ b/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/Criteria.java
@@ -109,6 +109,7 @@ public abstract class Criteria implements Serializable, BaseCriteria
{
private List<Permission> requiredPermissions;
private boolean strict;
private Restriction restriction = null;
+ private boolean supportsAddSortId = true;
protected Map<String, String> filterOverrides;
protected Map<String, String> sortOverrides;
@@ -173,16 +174,54 @@ public abstract class Criteria implements Serializable, BaseCriteria
{
}
public void addSortId(PageOrdering sortId) {
- addSortField("id");
- this.sortId = sortId;
+ if (isSupportsAddSortId()) {
+ addSortField("id");
+ this.sortId = sortId;
+
+ } else {
+ throw new UnsupportedOperationException("ID sort is not supported by
" + this.getClass().getSimpleName());
+ }
}
public void addFilterId(Integer filterId) {
- this.filterId = filterId;
+ if (isSupportsAddFilterId()) {
+ this.filterId = filterId;
+
+ } else {
+ throw new UnsupportedOperationException("ID filter is not supported by
" + this.getClass().getSimpleName());
+ }
}
public void addFilterIds(Integer... filterIds) {
- this.filterIds = CriteriaUtils.getListIgnoringNulls(filterIds);
+ if (isSupportsAddFilterIds()) {
+ this.filterIds = CriteriaUtils.getListIgnoringNulls(filterIds);
+
+ } else {
+ throw new UnsupportedOperationException("IDS filter is not supported by
" + this.getClass().getSimpleName());
+ }
+ }
+
+ /**
+ * By default all Criteria support sort on ID. And this sort is applied implicitly
to criteria
+ * queries involving paging, to ensure consistent ordering of query results. If for
some unlikely reason
+ * the caller needs to disable the implicit ID sort then call this, setting the value
to false.
+ *
+ * @param supportsAddSortId
+ */
+ public void setSupportsAddSortId(boolean supportsAddSortId) {
+ this.supportsAddSortId = supportsAddSortId;
+ }
+
+ public boolean isSupportsAddSortId() {
+ return supportsAddSortId;
+ }
+
+ public boolean isSupportsAddFilterId() {
+ return true;
+ }
+
+ public boolean isSupportsAddFilterIds() {
+ return true;
}
protected void addSortField(String fieldName) {
diff --git
a/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftChangeSetCriteria.java
b/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftChangeSetCriteria.java
index af888ea..2b03228 100644
---
a/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftChangeSetCriteria.java
+++
b/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftChangeSetCriteria.java
@@ -292,13 +292,12 @@ public class JPADriftChangeSetCriteria extends Criteria implements
DriftChangeSe
}
@Override
- public void addSortId(PageOrdering pageOrdering) {
- throw new UnsupportedOperationException("The sort is not supported by
DriftChangeSetCriteria.");
+ public boolean isSupportsAddSortId() {
+ return false;
}
@Override
- public void addFilterIds(Integer... filterIds) {
- throw new UnsupportedOperationException("The filter is not supported by
DriftChangeSetCriteria.");
+ public boolean isSupportsAddFilterIds() {
+ return false;
}
-
}
diff --git
a/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftCriteria.java
b/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftCriteria.java
index 56f0ddf..b13ca90 100644
---
a/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftCriteria.java
+++
b/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/JPADriftCriteria.java
@@ -245,13 +245,12 @@ public class JPADriftCriteria extends Criteria implements
DriftCriteria {
}
@Override
- public void addFilterIds(Integer... filterIds) {
- throw new UnsupportedOperationException("The filter is not supported by
DriftCriteria.");
+ public boolean isSupportsAddSortId() {
+ return false;
}
@Override
- public void addSortId(PageOrdering pageOrdering) {
- throw new UnsupportedOperationException("The sort is not supported by
DriftCriteria.");
+ public boolean isSupportsAddFilterIds() {
+ return false;
}
-
}
diff --git
a/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/MeasurementDataTraitCriteria.java
b/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/MeasurementDataTraitCriteria.java
index a27e58d..3a2a2c1 100644
---
a/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/MeasurementDataTraitCriteria.java
+++
b/modules/core/domain/src/main/java/org/rhq/core/domain/criteria/MeasurementDataTraitCriteria.java
@@ -135,17 +135,17 @@ public class MeasurementDataTraitCriteria extends Criteria {
}
@Override
- public void addSortId(PageOrdering sortId) {
- throw new UnsupportedOperationException("The id column is not defined on
this entity.");
+ public boolean isSupportsAddSortId() {
+ return false;
}
@Override
- public void addFilterId(Integer filterId) {
- throw new UnsupportedOperationException("The id column is not defined on
this entity.");
+ public boolean isSupportsAddFilterId() {
+ return false;
}
@Override
- public void addFilterIds(Integer... filterIds) {
- throw new UnsupportedOperationException("The id column is not defined on
this entity.");
+ public boolean isSupportsAddFilterIds() {
+ return false;
}
}
diff --git a/modules/core/domain/src/main/java/org/rhq/core/domain/util/PageControl.java
b/modules/core/domain/src/main/java/org/rhq/core/domain/util/PageControl.java
index f87a814..888c821 100644
--- a/modules/core/domain/src/main/java/org/rhq/core/domain/util/PageControl.java
+++ b/modules/core/domain/src/main/java/org/rhq/core/domain/util/PageControl.java
@@ -82,14 +82,22 @@ public class PageControl implements Serializable, Cloneable {
return new PageControl(0, 1);
}
+ /**
+ * Equivalent to initDefaultOrderingField(defaultField, PageOrdering.ASC).
+ *
+ * @param defaultField
+ * @see #initDefaultOrderingField(String, PageOrdering)
+ */
public void initDefaultOrderingField(String defaultField) {
initDefaultOrderingField(defaultField, PageOrdering.ASC);
}
- public void addDefaultOrderingField(String defaultField) {
- addDefaultOrderingField(defaultField, PageOrdering.ASC);
- }
-
+ /**
+ * Sets initial sort. If sorting is already defined this call will have no effect.
+ *
+ * @param defaultField
+ * @param defaultPageOrdering
+ */
public void initDefaultOrderingField(String defaultField, PageOrdering
defaultPageOrdering) {
if (orderingFields.size() > 0) {
return;
@@ -98,6 +106,24 @@ public class PageControl implements Serializable, Cloneable {
addDefaultOrderingField(defaultField, defaultPageOrdering);
}
+ /**
+ * Equivalent to addDefaultOrderingField(defaultField, PageOrdering.ASC).
+ *
+ * @param defaultField
+ * @see #addDefaultOrderingField(String, PageOrdering)
+ */
+ public void addDefaultOrderingField(String defaultField) {
+ addDefaultOrderingField(defaultField, PageOrdering.ASC);
+ }
+
+ /**
+ * Add a default ordering field. If the maximum number of sort fields (currently 3)
are already
+ * defined this call will have no effect. If the field is already a sort field this
call will have no
+ * effect. Otherwise, the ordering field will be appended to the existing ordering
fields.
+ *
+ * @param defaultField
+ * @param defaultPageOrdering
+ */
public void addDefaultOrderingField(String defaultField, PageOrdering
defaultPageOrdering) {
if (orderingFields.size() >= MAX_ORDERING_FIELD_COUNT) {
return; // only need to add defaults if there are less than 3 sort orders
diff --git
a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQuery.java
b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQuery.java
index 2d40388..a2bbfee 100644
---
a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQuery.java
+++
b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQuery.java
@@ -27,7 +27,6 @@ import java.util.NoSuchElementException;
import org.rhq.core.domain.criteria.BaseCriteria;
import org.rhq.core.domain.util.PageControl;
import org.rhq.core.domain.util.PageList;
-import org.rhq.core.domain.util.PageOrdering;
/** This class provides a way to make PageList results easily iterable with 'for
each','while',etc. loops
* and importantly automatically handles iteration through all PageControl results.
This
@@ -62,17 +61,6 @@ public class CriteriaQuery<T, C extends BaseCriteria> implements
Iterable<T> {
public CriteriaQuery(C criteria, CriteriaQueryExecutor<T, C> queryExecutor) {
this.criteria = criteria;
this.queryExecutor = queryExecutor;
-
- // make sure we have at least a default sort, otherwise chunking doesn't
work
- PageControl pageControlOverrides = this.criteria.getPageControlOverrides();
- if (null != pageControlOverrides) {
- if (pageControlOverrides.getOrderingFields().isEmpty()) {
- pageControlOverrides.addDefaultOrderingField("id");
- }
-
- } else if (this.criteria.getOrderingFieldNames().isEmpty()) {
- this.criteria.addSortId(PageOrdering.ASC);
- }
}
/** Returns iterator for a single page of results as defined by
@@ -110,7 +98,8 @@ public class CriteriaQuery<T, C extends BaseCriteria> implements
Iterable<T> {
public QueryResultsIterator(PageList<T> firstPage) {
currentPage = firstPage;
iterator = currentPage.iterator();
- count = firstPage == null || firstPage.getPageControl() == null ? 0 :
firstPage.getPageControl().getStartRow();
+ count = firstPage == null || firstPage.getPageControl() == null ? 0 :
firstPage.getPageControl()
+ .getStartRow();
}
@Override
diff --git
a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQueryGenerator.java
b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQueryGenerator.java
index d1d2f73..574fe73 100644
---
a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQueryGenerator.java
+++
b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/util/CriteriaQueryGenerator.java
@@ -951,6 +951,7 @@ public final class CriteriaQueryGenerator {
} else {
pc = new PageControl(criteria.getPageNumber(), criteria.getPageSize());
}
+
for (String fieldName : criteria.getOrderingFieldNames()) {
for (Field sortField : getFields(criteria, Criteria.Type.SORT)) {
if (sortField.getName().equals(fieldName) == false) {
@@ -969,6 +970,15 @@ public final class CriteriaQueryGenerator {
}
}
}
+
+ // Unless paging is unlimited or it's not supported, add a sort on ID. This
ensures that when paging
+ // we always have a consistent ordering. In other words, if the data set is
unchanged between pages, there
+ // will no overlap/repetition of rows. Note that this applies even if other sort
fields have been
+ // set, because they may still not have unique values. See
https://bugzilla.redhat.com/show_bug.cgi?id=966665.
+ if (!pc.isUnlimited() && criteria.isSupportsAddSortId()) {
+ pc.addDefaultOrderingField("id");
+ }
+
return pc;
}
diff --git
a/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/util/CriteriaQueryTest.java
b/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/util/CriteriaQueryTest.java
index 2e1c897..b1f5272 100644
---
a/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/util/CriteriaQueryTest.java
+++
b/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/util/CriteriaQueryTest.java
@@ -24,11 +24,15 @@ import static org.testng.Assert.assertEquals;
import java.util.ArrayList;
import java.util.List;
+import org.testng.AssertJUnit;
import org.testng.annotations.Test;
import org.rhq.core.domain.criteria.Criteria;
+import org.rhq.core.domain.criteria.JPADriftCriteria;
+import org.rhq.core.domain.criteria.ResourceCriteria;
import org.rhq.core.domain.util.PageControl;
import org.rhq.core.domain.util.PageList;
+import org.rhq.core.domain.util.PageOrdering;
public class CriteriaQueryTest {
@@ -229,4 +233,22 @@ public class CriteriaQueryTest {
assert e.getMessage().contains("NonUniqueResultException");
}
}
+
+ @Test
+ public void testAddSort() {
+ try {
+ JPADriftCriteria c = new JPADriftCriteria();
+ c.addSortId(PageOrdering.ASC);
+ AssertJUnit.fail("Should have thrown exception");
+ } catch (UnsupportedOperationException e) {
+ //expected
+ }
+
+ ResourceCriteria c = new ResourceCriteria();
+ c.addSortName(PageOrdering.ASC);
+
+ assertEquals(2,
CriteriaQueryGenerator.getPageControl(c).getOrderingFields().size());
+ assertEquals("name",
CriteriaQueryGenerator.getPageControl(c).getOrderingFields().get(0).getField());
+ assertEquals("id",
CriteriaQueryGenerator.getPageControl(c).getOrderingFields().get(1).getField());
+ }
}
commit 6613ba3db6f3c57c2d2ecac4823e368269a07152
Author: Jay Shaughnessy <jshaughn(a)redhat.com>
Date: Fri May 24 15:40:51 2013 -0400
Use only apache logger and not the jboss logger from the base class.
diff --git
a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/jaas/DelegatingLoginModule.java
b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/jaas/DelegatingLoginModule.java
index d89c04b..b83a037 100644
---
a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/jaas/DelegatingLoginModule.java
+++
b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/jaas/DelegatingLoginModule.java
@@ -84,7 +84,7 @@ public class DelegatingLoginModule extends UsernamePasswordLoginModule
{
public void initialize(Subject subject, CallbackHandler callbackHandler,
Map<String, ?> sharedState,
Map<String, ?> options) {
- debugEnabled = log.isDebugEnabled();
+ debugEnabled = LOG.isDebugEnabled();
super.initialize(subject, callbackHandler, sharedState, options);
@@ -101,14 +101,14 @@ public class DelegatingLoginModule extends
UsernamePasswordLoginModule {
}
if (debugEnabled) {
- log.debug("Delegating to " + delegateTo + " with roles "
+ additionalRoles);
+ LOG.debug("Delegating to " + delegateTo + " with roles "
+ additionalRoles);
}
// Now create the context for later use
try {
loginContext = new LoginContext(delegateTo, new DelegateCallbackHandler());
} catch (LoginException e) {
- log.warn("Initialize failed : " + e.getMessage());
+ LOG.warn("Initialize failed : " + e.getMessage());
}
}
@@ -123,7 +123,7 @@ public class DelegatingLoginModule extends UsernamePasswordLoginModule
{
@Override
public boolean login() throws LoginException {
try {
- // Get the username / password the user entred and save if for later use
+ // Get the username / password the user entered and save if for later use
usernamePassword = super.getUsernameAndPassword();
// Try to log in via the delegate
@@ -141,7 +141,7 @@ public class DelegatingLoginModule extends UsernamePasswordLoginModule
{
loginOk = true;
if (debugEnabled) {
- log.debug("Login ok for " + usernamePassword[0]);
+ LOG.debug("Login ok for " + usernamePassword[0]);
}
return true;