From 49251588f2379381ea4b7367a6002f9099952bcc Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 16 Dec 2016 11:19:29 -0500 Subject: [PATCH] elasticsearch: Always set up both change and account indexes Change query tests subtly depend on the account index for the account setup step: omitting the account index caused AccountManager#authenticate to throw an exception if an account external ID was repeated, instead of returning the account with that ID. Always set up all indexes, using a helper in ElasticTestUtils. The delete code can be simplified as well using the special string "_all" to delete all indexes. Change-Id: I7ab7be37a8cf953b6873745014e781e6b5e910f8 --- gerrit-elasticsearch/BUILD | 1 + .../ElasticQueryAccountsTest.java | 27 ++--------- .../ElasticQueryChangesTest.java | 35 ++------------ .../elasticsearch/ElasticTestUtils.java | 48 ++++++++++++++++++- 4 files changed, 53 insertions(+), 58 deletions(-) diff --git a/gerrit-elasticsearch/BUILD b/gerrit-elasticsearch/BUILD index 9323bf0cda..049b2e5f22 100644 --- a/gerrit-elasticsearch/BUILD +++ b/gerrit-elasticsearch/BUILD @@ -34,6 +34,7 @@ java_library( testonly = 1, srcs = glob(["src/test/java/**/ElasticTestUtils.java"]), deps = [ + ":elasticsearch", "//gerrit-extension-api:api", "//gerrit-server:server", "//lib:gson", diff --git a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryAccountsTest.java b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryAccountsTest.java index 62c1a57883..68759dffbb 100644 --- a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryAccountsTest.java +++ b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryAccountsTest.java @@ -14,11 +14,7 @@ package com.google.gerrit.elasticsearch; -import static com.google.gerrit.elasticsearch.ElasticAccountIndex.ACCOUNTS_PREFIX; - -import com.google.gerrit.elasticsearch.ElasticAccountIndex.AccountMapping; import com.google.gerrit.elasticsearch.ElasticTestUtils.ElasticNodeInfo; -import com.google.gerrit.server.index.account.AccountSchemaDefinitions; import com.google.gerrit.server.query.account.AbstractQueryAccountsTest; import com.google.gerrit.testutil.InMemoryModule; import com.google.inject.Guice; @@ -32,9 +28,6 @@ import org.junit.BeforeClass; import java.util.concurrent.ExecutionException; public class ElasticQueryAccountsTest extends AbstractQueryAccountsTest { - private static final String INDEX_NAME = - String.format("%s%04d", ACCOUNTS_PREFIX, - AccountSchemaDefinitions.INSTANCE.getLatest().getVersion()); private static ElasticNodeInfo nodeInfo; @BeforeClass @@ -45,21 +38,7 @@ public class ElasticQueryAccountsTest extends AbstractQueryAccountsTest { return; } nodeInfo = ElasticTestUtils.startElasticsearchNode(); - createIndexes(); - } - - private static void createIndexes() { - AccountMapping accountMapping = - new AccountMapping(AccountSchemaDefinitions.INSTANCE.getLatest()); - nodeInfo.node - .client() - .admin() - .indices() - .prepareCreate(INDEX_NAME) - .addMapping(ElasticAccountIndex.ACCOUNTS, - ElasticTestUtils.gson.toJson(accountMapping)) - .execute() - .actionGet(); + ElasticTestUtils.createAllIndexes(nodeInfo); } @AfterClass @@ -74,8 +53,8 @@ public class ElasticQueryAccountsTest extends AbstractQueryAccountsTest { @After public void cleanupIndex() { if (nodeInfo != null) { - ElasticTestUtils.deleteIndexes(nodeInfo.node, INDEX_NAME); - createIndexes(); + ElasticTestUtils.deleteAllIndexes(nodeInfo); + ElasticTestUtils.createAllIndexes(nodeInfo); } } diff --git a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryChangesTest.java b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryChangesTest.java index 4b76e4bfa5..95dbe5bb44 100644 --- a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryChangesTest.java +++ b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryChangesTest.java @@ -14,13 +14,7 @@ package com.google.gerrit.elasticsearch; -import static com.google.gerrit.elasticsearch.ElasticChangeIndex.CHANGES_PREFIX; -import static com.google.gerrit.elasticsearch.ElasticChangeIndex.CLOSED_CHANGES; -import static com.google.gerrit.elasticsearch.ElasticChangeIndex.OPEN_CHANGES; - -import com.google.gerrit.elasticsearch.ElasticChangeIndex.ChangeMapping; import com.google.gerrit.elasticsearch.ElasticTestUtils.ElasticNodeInfo; -import com.google.gerrit.server.index.change.ChangeSchemaDefinitions; import com.google.gerrit.server.query.change.AbstractQueryChangesTest; import com.google.gerrit.testutil.InMemoryModule; import com.google.gerrit.testutil.InMemoryRepositoryManager.Repo; @@ -37,9 +31,6 @@ import org.junit.Test; import java.util.concurrent.ExecutionException; public class ElasticQueryChangesTest extends AbstractQueryChangesTest { - private static final String INDEX_NAME = - String.format("%s%04d", CHANGES_PREFIX, - ChangeSchemaDefinitions.INSTANCE.getLatest().getVersion()); private static ElasticNodeInfo nodeInfo; @BeforeClass @@ -51,14 +42,14 @@ public class ElasticQueryChangesTest extends AbstractQueryChangesTest { } nodeInfo = ElasticTestUtils.startElasticsearchNode(); - createIndexes(); + ElasticTestUtils.createAllIndexes(nodeInfo); } @After public void cleanupIndex() { if (nodeInfo != null) { - ElasticTestUtils.deleteIndexes(nodeInfo.node, INDEX_NAME); - createIndexes(); + ElasticTestUtils.deleteAllIndexes(nodeInfo); + ElasticTestUtils.createAllIndexes(nodeInfo); } } @@ -80,26 +71,6 @@ public class ElasticQueryChangesTest extends AbstractQueryChangesTest { new InMemoryModule(elasticsearchConfig, notesMigration)); } - private static void createIndexes() { - ChangeMapping openChangesMapping = - new ChangeMapping(ChangeSchemaDefinitions.INSTANCE.getLatest()); - ChangeMapping closedChangesMapping = - new ChangeMapping(ChangeSchemaDefinitions.INSTANCE.getLatest()); - openChangesMapping.closedChanges = null; - closedChangesMapping.openChanges = null; - nodeInfo.node - .client() - .admin() - .indices() - .prepareCreate(INDEX_NAME) - .addMapping(OPEN_CHANGES, - ElasticTestUtils.gson.toJson(openChangesMapping)) - .addMapping(CLOSED_CHANGES, - ElasticTestUtils.gson.toJson(closedChangesMapping)) - .execute() - .actionGet(); - } - @Test public void byOwnerInvalidQuery() throws Exception { TestRepository repo = createProject("repo"); diff --git a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticTestUtils.java b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticTestUtils.java index 7c26edc35c..9d6c808c7d 100644 --- a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticTestUtils.java +++ b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticTestUtils.java @@ -15,10 +15,21 @@ package com.google.gerrit.elasticsearch; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.elasticsearch.ElasticAccountIndex.ACCOUNTS_PREFIX; +import static com.google.gerrit.elasticsearch.ElasticChangeIndex.CHANGES_PREFIX; +import static com.google.gerrit.elasticsearch.ElasticChangeIndex.CLOSED_CHANGES; +import static com.google.gerrit.elasticsearch.ElasticChangeIndex.OPEN_CHANGES; import com.google.common.base.Strings; import com.google.common.io.Files; +import com.google.gerrit.elasticsearch.ElasticAccountIndex.AccountMapping; +import com.google.gerrit.elasticsearch.ElasticChangeIndex.ChangeMapping; +import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.index.IndexModule.IndexType; +import com.google.gerrit.server.index.Schema; +import com.google.gerrit.server.index.account.AccountSchemaDefinitions; +import com.google.gerrit.server.index.change.ChangeSchemaDefinitions; +import com.google.gerrit.server.query.change.ChangeData; import com.google.gson.FieldNamingPolicy; import com.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -100,8 +111,8 @@ final class ElasticTestUtils { return new ElasticNodeInfo(node, elasticDir, getHttpPort(node)); } - static void deleteIndexes(Node node, String index) { - node.client().admin().indices().prepareDelete(index).execute(); + static void deleteAllIndexes(ElasticNodeInfo nodeInfo) { + nodeInfo.node.client().admin().indices().prepareDelete("_all").execute(); } static class NodeInfo { @@ -112,6 +123,39 @@ final class ElasticTestUtils { Map nodes; } + static void createAllIndexes(ElasticNodeInfo nodeInfo) { + Schema changeSchema = + ChangeSchemaDefinitions.INSTANCE.getLatest(); + ChangeMapping openChangesMapping = new ChangeMapping(changeSchema); + ChangeMapping closedChangesMapping = new ChangeMapping(changeSchema); + openChangesMapping.closedChanges = null; + closedChangesMapping.openChanges = null; + nodeInfo.node + .client() + .admin() + .indices() + .prepareCreate( + String.format("%s%04d", CHANGES_PREFIX, changeSchema.getVersion())) + .addMapping(OPEN_CHANGES, gson.toJson(openChangesMapping)) + .addMapping(CLOSED_CHANGES, gson.toJson(closedChangesMapping)) + .execute() + .actionGet(); + + Schema accountSchema = + AccountSchemaDefinitions.INSTANCE.getLatest(); + AccountMapping accountMapping = new AccountMapping(accountSchema); + nodeInfo.node + .client() + .admin() + .indices() + .prepareCreate( + String.format( + "%s%04d", ACCOUNTS_PREFIX, accountSchema.getVersion())) + .addMapping(ElasticAccountIndex.ACCOUNTS, gson.toJson(accountMapping)) + .execute() + .actionGet(); + } + private static String getHttpPort(Node node) throws InterruptedException, ExecutionException { String nodes = node.client().admin().cluster()