You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by bo...@apache.org on 2018/08/03 16:32:18 UTC
[geode] branch develop updated: GEODE-5246: Changed
DefaultQueryService to return an empty list when no indexes are defined
This is an automated email from the ASF dual-hosted git repository.
boglesby pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 8abaf05 GEODE-5246: Changed DefaultQueryService to return an empty list when no indexes are defined
8abaf05 is described below
commit 8abaf0521d4aeaa499805862720d967daf31bc35
Author: Juan José Ramos <ju...@users.noreply.github.com>
AuthorDate: Fri Aug 3 17:32:14 2018 +0100
GEODE-5246: Changed DefaultQueryService to return an empty list when no indexes are defined
---
.../cache/query/dunit/QueryIndexDUnitTest.java | 6 +-
.../CreateDefinedIndexesCommandDUnitTest.java | 7 +-
.../geode/cache/query/QueryServiceJUnitTest.java | 84 +++++++++++++++++-----
.../org/apache/geode/cache/query/QueryService.java | 9 ++-
.../cache/query/internal/DefaultQueryService.java | 23 +++---
.../internal/cache/xmlcache/CacheCreation.java | 3 +-
.../internal/cache/xmlcache/CacheXmlGenerator.java | 8 +--
.../cli/functions/DestroyIndexFunction.java | 15 ++--
8 files changed, 104 insertions(+), 51 deletions(-)
diff --git a/geode-core/src/distributedTest/java/org/apache/geode/cache/query/dunit/QueryIndexDUnitTest.java b/geode-core/src/distributedTest/java/org/apache/geode/cache/query/dunit/QueryIndexDUnitTest.java
index 23bc64b..b478fc3 100644
--- a/geode-core/src/distributedTest/java/org/apache/geode/cache/query/dunit/QueryIndexDUnitTest.java
+++ b/geode-core/src/distributedTest/java/org/apache/geode/cache/query/dunit/QueryIndexDUnitTest.java
@@ -1291,12 +1291,10 @@ public class QueryIndexDUnitTest extends JUnit4CacheTestCase {
if (qs != null) {
try {
Collection indexes = qs.getIndexes(region);
- if (indexes == null) {
+ if (indexes.isEmpty()) {
return; // no IndexManager defined
}
- if (indexes.size() == 0) {
- return; // no indexes defined
- }
+
Iterator iter = indexes.iterator();
if (iter.hasNext()) {
Index idx = (Index) (iter.next());
diff --git a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/CreateDefinedIndexesCommandDUnitTest.java b/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/CreateDefinedIndexesCommandDUnitTest.java
index bc7bf10..093a536 100644
--- a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/CreateDefinedIndexesCommandDUnitTest.java
+++ b/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/CreateDefinedIndexesCommandDUnitTest.java
@@ -278,8 +278,8 @@ public class CreateDefinedIndexesCommandDUnitTest {
QueryService queryService = cache.getQueryService();
Region region1 = cache.getRegion(region1Name);
- // Returns null instead of an empty collection if there are no indexes...
- assertThat(queryService.getIndexes(region1)).isNull();
+ assertThat(queryService.getIndexes(region1)).isNotNull();
+ assertThat(queryService.getIndexes(region1).isEmpty()).isTrue();
}, server1, server2);
VMProvider.invokeInEveryMember(() -> {
@@ -287,7 +287,8 @@ public class CreateDefinedIndexesCommandDUnitTest {
QueryService queryService = cache.getQueryService();
Region region2 = cache.getRegion(region2Name);
- assertThat(queryService.getIndexes(region2)).isNull();
+ assertThat(queryService.getIndexes(region2)).isNotNull();
+ assertThat(queryService.getIndexes(region2).isEmpty()).isTrue();
}, server3);
locator.invoke(() -> {
diff --git a/geode-core/src/integrationTest/java/org/apache/geode/cache/query/QueryServiceJUnitTest.java b/geode-core/src/integrationTest/java/org/apache/geode/cache/query/QueryServiceJUnitTest.java
index fd89796..441aa66 100644
--- a/geode-core/src/integrationTest/java/org/apache/geode/cache/query/QueryServiceJUnitTest.java
+++ b/geode-core/src/integrationTest/java/org/apache/geode/cache/query/QueryServiceJUnitTest.java
@@ -19,9 +19,11 @@
*/
package org.apache.geode.cache.query;
+import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.fail;
+import java.util.Arrays;
import java.util.Collection;
import org.junit.After;
@@ -31,6 +33,7 @@ import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.cache.query.data.Portfolio;
import org.apache.geode.test.junit.categories.OQLQueryTest;
@@ -54,7 +57,7 @@ public class QueryServiceJUnitTest {
// tests to make sure no exception is thrown when a valid query is created
// and an invalid query throws an exception
@Test
- public void testNewQuery() throws Exception {
+ public void testNewQuery() {
CacheUtils.log("testNewQuery");
QueryService qs = CacheUtils.getQueryService();
qs.newQuery("SELECT DISTINCT * FROM /root");
@@ -123,21 +126,23 @@ public class QueryServiceJUnitTest {
try {
index = qs.createIndex("statusIndex", IndexType.FUNCTIONAL, "status", "/Portfolios");
- if (index != null)
+ if (index != null) {
fail("QueryService.createIndex allows duplicate index names");
+ }
} catch (IndexNameConflictException e) {
}
try {
index = qs.createIndex("statusIndex1", IndexType.FUNCTIONAL, "status", "/Portfolios");
- if (index != null)
+ if (index != null) {
fail("QueryService.createIndex allows duplicate indexes");
+ }
} catch (IndexExistsException e) {
}
}
@Test
- public void testIndexDefinitions() throws Exception {
+ public void testIndexDefinitions() {
Object[][] testDataFromClauses = {{"status", "/Portfolios", Boolean.TRUE},
{"status", "/Portfolios.entries", Boolean.FALSE},
{"status", "/Portfolios.values", Boolean.TRUE},
@@ -162,7 +167,7 @@ public class QueryServiceJUnitTest {
runCreateIndexTests(testDataIndexExpr);
}
- private void runCreateIndexTests(Object testData[][]) throws Exception {
+ private void runCreateIndexTests(Object testData[][]) {
QueryService qs = CacheUtils.getQueryService();
qs.removeIndexes();
for (int i = 0; i < testData.length; i++) {
@@ -186,8 +191,9 @@ public class QueryServiceJUnitTest {
+ " from=" + testData[i][1]);
}
} finally {
- if (index != null)
+ if (index != null) {
qs.removeIndex(index);
+ }
}
}
}
@@ -197,14 +203,6 @@ public class QueryServiceJUnitTest {
public void testGetIndex() throws Exception {
CacheUtils.log("testGetIndex");
QueryService qs = CacheUtils.getQueryService();
- Object testData[][] =
- {{"status", "/Portfolios", Boolean.TRUE}, {"status", "/Portfolios.values", Boolean.FALSE},
- {"status", "/Portfolios p", Boolean.TRUE}, {"p.status", "/Portfolios p", Boolean.TRUE},
- {"status", "/Portfolios.values x", Boolean.FALSE},
- {"x.status", "/Portfolios.values x", Boolean.FALSE},
- {"status", "/Portfolio", Boolean.FALSE}, {"p.status", "/Portfolios", Boolean.FALSE},
- {"ID", "/Portfolios", Boolean.FALSE}, {"p.ID", "/Portfolios p", Boolean.FALSE},
- {"is_defined(status)", "/Portfolios", Boolean.FALSE},};
Region r = CacheUtils.getRegion("/Portfolios");
Index index = qs.createIndex("statusIndex", IndexType.FUNCTIONAL, "status", "/Portfolios");
@@ -228,8 +226,9 @@ public class QueryServiceJUnitTest {
Index index = qs.createIndex("statusIndex", IndexType.FUNCTIONAL, "p.status", "/Portfolios p");
qs.removeIndex(index);
index = qs.getIndex(CacheUtils.getRegion("/Portfolios"), "statusIndex");
- if (index != null)
+ if (index != null) {
fail("QueryService.removeIndex is not removing index");
+ }
}
@Test
@@ -242,8 +241,9 @@ public class QueryServiceJUnitTest {
qs.createIndex("statusIndex", IndexType.FUNCTIONAL, "status", "/Ptfs");
qs.removeIndexes();
Collection allIndexes = qs.getIndexes();
- if (allIndexes.size() != 0)
+ if (allIndexes.size() != 0) {
fail("QueryService.removeIndexes() does not removes all indexes");
+ }
}
@Test
@@ -255,8 +255,58 @@ public class QueryServiceJUnitTest {
qs.createIndex("statusIndex", IndexType.FUNCTIONAL, "status", "/Portfolios");
qs.createIndex("statusIndex", IndexType.FUNCTIONAL, "status", "/Ptfs");
Collection allIndexes = qs.getIndexes();
- if (allIndexes.size() != 2)
+ if (allIndexes.size() != 2) {
fail("QueryService.getIndexes() does not return correct indexes");
+ }
}
+ @Test
+ public void getIndexesShouldReturnEmptyCollectionWhenNoIndexesAreDefined() {
+ CacheUtils.log("getIndexesShouldReturnEmptyCollectionWhenNoIndexesAreDefined");
+ QueryService queryService = CacheUtils.getQueryService();
+ Region replicatedRegion = CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE)
+ .create("ReplicatedRegion");
+ Region partitionedRegion = CacheUtils.getCache().createRegionFactory(RegionShortcut.PARTITION)
+ .create("PartitionedRegion");
+
+ // Get All Indexes.
+ assertThat(queryService.getIndexes()).isNotNull();
+ assertThat(queryService.getIndexes().size()).isEqualTo(0);
+
+ // Get All Indexes For Replicated Region.
+ assertThat(queryService.getIndexes(replicatedRegion)).isNotNull();
+ assertThat(queryService.getIndexes(replicatedRegion).isEmpty()).isTrue();
+
+ // Get All Indexes For Partitioned Region.
+ assertThat(queryService.getIndexes(partitionedRegion)).isNotNull();
+ assertThat(queryService.getIndexes(partitionedRegion).isEmpty()).isTrue();
+ }
+
+ @Test
+ public void getIndexesShouldReturnEmptyCollectionWhenNoIndexesOfTheSpecifiedTypeAreDefined() {
+ CacheUtils
+ .log("getIndexesShouldReturnEmptyCollectionWhenNoIndexesOfTheSpecifiedTypeAreDefined");
+ IndexType[] indexTypes = IndexType.values();
+ QueryService queryService = CacheUtils.getQueryService();
+ Region replicatedRegion = CacheUtils.getCache().createRegionFactory(RegionShortcut.REPLICATE)
+ .create("ReplicatedRegion");
+ Region partitionedRegion = CacheUtils.getCache().createRegionFactory(RegionShortcut.PARTITION)
+ .create("PartitionedRegion");
+
+ // Get All Indexes.
+ assertThat(queryService.getIndexes()).isNotNull();
+ assertThat(queryService.getIndexes().size()).isEqualTo(0);
+
+ // Get All Indexes For Replicated Region.
+ Arrays.stream(indexTypes).forEach(indexType -> {
+ assertThat(queryService.getIndexes(replicatedRegion, indexType)).isNotNull();
+ assertThat(queryService.getIndexes(replicatedRegion, indexType).isEmpty()).isTrue();
+ });
+
+ // Get All Indexes For Partitioned Region.
+ Arrays.stream(indexTypes).forEach(indexType -> {
+ assertThat(queryService.getIndexes(partitionedRegion, indexType)).isNotNull();
+ assertThat(queryService.getIndexes(partitionedRegion, indexType).isEmpty()).isTrue();
+ });
+ }
}
diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/QueryService.java b/geode-core/src/main/java/org/apache/geode/cache/query/QueryService.java
index 6250308..2e5a8b4 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/query/QueryService.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/query/QueryService.java
@@ -469,7 +469,8 @@ public interface QueryService {
/**
* Get a collection of all the indexes in the Cache.
*
- * @return the collection of all indexes in this Cache
+ * @return the collection of all indexes in this Cache, or an empty (unmodifiable) collection if
+ * no indexes are found.
*/
Collection<Index> getIndexes();
@@ -477,7 +478,8 @@ public interface QueryService {
* Get a collection of all the indexes on the specified Region
*
* @param region the region for the requested indexes
- * @return the collection of indexes on the specified region
+ * @return the collection of indexes on the specified region, or an empty (unmodifiable)
+ * collection if no indexes are found.
*/
Collection<Index> getIndexes(Region<?, ?> region);
@@ -490,7 +492,8 @@ public interface QueryService {
*
* @param region the region for the requested indexes
* @param indexType the type of indexes to get. Currently must be Indexable.FUNCTIONAL
- * @return the collection of indexes for the specified region and type
+ * @return the collection of indexes for the specified region and type, or an empty (unmodifiable)
+ * collection if no indexes are found.
*/
@Deprecated
Collection<Index> getIndexes(Region<?, ?> region, IndexType indexType);
diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQueryService.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQueryService.java
index d91eb1c..2d15795 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQueryService.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQueryService.java
@@ -380,16 +380,14 @@ public class DefaultQueryService implements InternalQueryService {
Iterator rootRegions = cache.rootRegions().iterator();
while (rootRegions.hasNext()) {
Region region = (Region) rootRegions.next();
- Collection indexes = getIndexes(region);
- if (indexes != null)
- allIndexes.addAll(indexes);
+ allIndexes.addAll(getIndexes(region));
+
Iterator subRegions = region.subregions(true).iterator();
while (subRegions.hasNext()) {
- indexes = getIndexes((Region) subRegions.next());
- if (indexes != null)
- allIndexes.addAll(indexes);
+ allIndexes.addAll(getIndexes((Region) subRegions.next()));
}
}
+
return allIndexes;
}
@@ -403,9 +401,12 @@ public class DefaultQueryService implements InternalQueryService {
if (region instanceof PartitionedRegion) {
return ((PartitionedRegion) region).getIndexes();
}
+
IndexManager indexManager = IndexUtils.getIndexManager(cache, region, false);
- if (indexManager == null)
- return null;
+ if (indexManager == null) {
+ return Collections.emptyList();
+ }
+
return indexManager.getIndexes();
}
@@ -417,8 +418,10 @@ public class DefaultQueryService implements InternalQueryService {
}
IndexManager indexManager = IndexUtils.getIndexManager(cache, region, false);
- if (indexManager == null)
- return null;
+ if (indexManager == null) {
+ return Collections.emptyList();
+ }
+
return indexManager.getIndexes(indexType);
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
index a67e220..2693b5c 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
@@ -1971,7 +1971,8 @@ public class CacheCreation implements InternalCache {
@Override
public Collection<Index> getIndexes(Region<?, ?> region) {
- return this.indexes.get(region.getFullPath());
+ Collection<Index> indexes = this.indexes.get(region.getFullPath());
+ return (indexes != null) ? indexes : Collections.emptyList();
}
@Override
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlGenerator.java b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlGenerator.java
index ab22d0b..fe3204b 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlGenerator.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheXmlGenerator.java
@@ -1664,11 +1664,9 @@ public class CacheXmlGenerator extends CacheXml implements XMLReader {
}
// generate index data here
- Collection indexesForRegion = this.cache.getQueryService().getIndexes(region);
- if (indexesForRegion != null) {
- for (Object index : indexesForRegion) {
- generate((Index) index);
- }
+ Collection<Index> indexesForRegion = this.cache.getQueryService().getIndexes(region);
+ for (Index index : indexesForRegion) {
+ generate(index);
}
if (region instanceof PartitionedRegion) {
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DestroyIndexFunction.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DestroyIndexFunction.java
index 7d445e1..326569f 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DestroyIndexFunction.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DestroyIndexFunction.java
@@ -14,7 +14,7 @@
*/
package org.apache.geode.management.internal.cli.functions;
-import java.util.List;
+import java.util.Collection;
import org.apache.geode.cache.Cache;
import org.apache.geode.cache.CacheClosedException;
@@ -96,17 +96,16 @@ public class DestroyIndexFunction extends CliFunction {
* @return true if the index was found and removed/false if the index was not found.
*/
private boolean removeIndexByName(String name, QueryService queryService) {
- List<Index> indexes = (List<Index>) queryService.getIndexes();
boolean removed = false;
+ Collection<Index> indexes = queryService.getIndexes();
- if (indexes != null) {
- for (Index index : indexes) {
- if (index.getName().equals(name)) {
- queryService.removeIndex(index);
- removed = true;
- }
+ for (Index index : indexes) {
+ if (index.getName().equals(name)) {
+ queryService.removeIndex(index);
+ removed = true;
}
}
+
return removed;
}