From ac2e61569bfb16ba43e5ee8ef5e56130f30ee2bc Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Wed, 1 Aug 2018 11:04:27 -0600 Subject: [PATCH 1/3] Remove UpdateSettingsTestHelper class By making the `settings()` method public on `UpdateSettingsRequest` (I think it should have been in the first place) we can get rid of this class entirely. Mock response objects are now constructed by parsing JSON without making the constructor public. Relates to #29823 --- .../settings/put/UpdateSettingsRequest.java | 2 +- .../put/UpdateSettingsTestHelper.java | 48 ------------------- .../SetSingleNodeAllocateStepTests.java | 43 +++++++++++++---- .../UpdateSettingsStepTests.java | 24 ++++++++-- 4 files changed, 55 insertions(+), 62 deletions(-) delete mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsTestHelper.java diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java index b229e2c9e6a23..e7b5d82207410 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java @@ -88,7 +88,7 @@ public String[] indices() { return indices; } - Settings settings() { + public Settings settings() { return settings; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsTestHelper.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsTestHelper.java deleted file mode 100644 index fd9716dc65766..0000000000000 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsTestHelper.java +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - * - */ -package org.elasticsearch.action.admin.indices.settings.put; - - import org.elasticsearch.common.settings.Settings; - -import java.util.Set; -import java.util.stream.Collectors; - -import static org.hamcrest.Matchers.anyOf; -import static org.hamcrest.Matchers.equalTo; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertThat; - -public final class UpdateSettingsTestHelper { - - // NORELEASE this isn't nice but it's currently the only way to inspect the - // settings in an update settings request. Need to see if we can make the - // getter public in ES - public static void assertSettingsRequest(UpdateSettingsRequest request, Settings expectedSettings, String... expectedIndices) { - assertNotNull(request); - assertArrayEquals(expectedIndices, request.indices()); - assertEquals(expectedSettings, request.settings()); - } - - public static void assertSettingsRequestContainsValueFrom(UpdateSettingsRequest request, String settingsKey, - Set acceptableValues, boolean assertOnlyKeyInSettings, String... expectedIndices) { - assertNotNull(request); - assertArrayEquals(expectedIndices, request.indices()); - assertThat(request.settings().get(settingsKey), anyOf(acceptableValues.stream().map(e -> equalTo(e)).collect(Collectors.toList()))); - if (assertOnlyKeyInSettings) { - assertEquals(1, request.settings().size()); - } - } - - // NORELEASE this isn't nice but it's currently the only way to create an - // UpdateSettingsResponse. Need to see if we can make the constructor public - // in ES - public static UpdateSettingsResponse createMockResponse(boolean acknowledged) { - return new UpdateSettingsResponse(acknowledged); - } -} diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SetSingleNodeAllocateStepTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SetSingleNodeAllocateStepTests.java index b523cdb123567..ac7c9987a17a9 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SetSingleNodeAllocateStepTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SetSingleNodeAllocateStepTests.java @@ -10,7 +10,6 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsRequest; import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsResponse; -import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsTestHelper; import org.elasticsearch.client.AdminClient; import org.elasticsearch.client.Client; import org.elasticsearch.client.IndicesAdminClient; @@ -27,6 +26,9 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.Settings.Builder; import org.elasticsearch.common.transport.TransportAddress; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.shard.ShardId; @@ -39,8 +41,14 @@ import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; +import java.io.IOException; import java.util.HashSet; import java.util.Set; +import java.util.stream.Collectors; + +import static org.elasticsearch.common.xcontent.DeprecationHandler.THROW_UNSUPPORTED_OPERATION; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.equalTo; public class SetSingleNodeAllocateStepTests extends AbstractStepTestCase { @@ -80,7 +88,18 @@ protected SetSingleNodeAllocateStep copyInstance(SetSingleNodeAllocateStep insta return new SetSingleNodeAllocateStep(instance.getKey(), instance.getNextStepKey(), client); } - public void testPerformActionNoAttrs() { + public static void assertSettingsRequestContainsValueFrom(UpdateSettingsRequest request, String settingsKey, + Set acceptableValues, boolean assertOnlyKeyInSettings, + String... expectedIndices) { + assertNotNull(request); + assertArrayEquals(expectedIndices, request.indices()); + assertThat(request.settings().get(settingsKey), anyOf(acceptableValues.stream().map(e -> equalTo(e)).collect(Collectors.toList()))); + if (assertOnlyKeyInSettings) { + assertEquals(1, request.settings().size()); + } + } + + public void testPerformActionNoAttrs() throws IOException { IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10)).settings(settings(Version.CURRENT)) .numberOfShards(randomIntBetween(1, 5)).numberOfReplicas(randomIntBetween(0, 5)).build(); Index index = indexMetaData.getIndex(); @@ -101,7 +120,7 @@ public void testPerformActionNoAttrs() { assertNodeSelected(indexMetaData, index, validNodeNames, nodes); } - public void testPerformActionAttrsAllNodesValid() { + public void testPerformActionAttrsAllNodesValid() throws IOException { int numAttrs = randomIntBetween(1, 10); String[][] validAttrs = new String[numAttrs][2]; for (int i = 0; i < numAttrs; i++) { @@ -132,7 +151,7 @@ public void testPerformActionAttrsAllNodesValid() { assertNodeSelected(indexMetaData, index, validNodeNames, nodes); } - public void testPerformActionAttrsSomeNodesValid() { + public void testPerformActionAttrsSomeNodesValid() throws IOException { String[] validAttr = new String[] { "box_type", "valid" }; String[] invalidAttr = new String[] { "box_type", "not_valid" }; Settings.Builder indexSettings = settings(Version.CURRENT); @@ -237,7 +256,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable { UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0]; @SuppressWarnings("unchecked") ActionListener listener = (ActionListener) invocation.getArguments()[1]; - UpdateSettingsTestHelper.assertSettingsRequestContainsValueFrom(request, + assertSettingsRequestContainsValueFrom(request, IndexMetaData.INDEX_ROUTING_REQUIRE_GROUP_SETTING.getKey() + "_name", validNodeNames, true, indexMetaData.getIndex().getName()); listener.onFailure(exception); @@ -325,7 +344,8 @@ public void onFailure(Exception e) { Mockito.verifyZeroInteractions(client); } - private void assertNodeSelected(IndexMetaData indexMetaData, Index index, Set validNodeNames, DiscoveryNodes.Builder nodes) { + private void assertNodeSelected(IndexMetaData indexMetaData, Index index, + Set validNodeNames, DiscoveryNodes.Builder nodes) throws IOException { ImmutableOpenMap.Builder indices = ImmutableOpenMap. builder().fPut(index.getName(), indexMetaData); IndexRoutingTable.Builder indexRoutingTable = IndexRoutingTable.builder(index) @@ -340,6 +360,13 @@ private void assertNodeSelected(IndexMetaData indexMetaData, Index index, Set() { @Override @@ -347,10 +374,10 @@ public Void answer(InvocationOnMock invocation) throws Throwable { UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0]; @SuppressWarnings("unchecked") ActionListener listener = (ActionListener) invocation.getArguments()[1]; - UpdateSettingsTestHelper.assertSettingsRequestContainsValueFrom(request, + assertSettingsRequestContainsValueFrom(request, IndexMetaData.INDEX_ROUTING_REQUIRE_GROUP_SETTING.getKey() + "_name", validNodeNames, true, indexMetaData.getIndex().getName()); - listener.onResponse(UpdateSettingsTestHelper.createMockResponse(true)); + listener.onResponse(response); return null; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java index b62e5ef062609..3c62e84746fcd 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java @@ -11,12 +11,14 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsRequest; import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsResponse; -import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsTestHelper; import org.elasticsearch.client.AdminClient; import org.elasticsearch.client.Client; import org.elasticsearch.client.IndicesAdminClient; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.xpack.core.indexlifecycle.AsyncActionStep.Listener; import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey; import org.junit.Before; @@ -24,6 +26,9 @@ import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; +import static org.elasticsearch.common.xcontent.DeprecationHandler.THROW_UNSUPPORTED_OPERATION; +import static org.hamcrest.Matchers.equalTo; + public class UpdateSettingsStepTests extends AbstractStepTestCase { private Client client; @@ -70,7 +75,7 @@ public UpdateSettingsStep copyInstance(UpdateSettingsStep instance) { return new UpdateSettingsStep(instance.getKey(), instance.getNextStepKey(), instance.getClient(), instance.getSettings()); } - public void testPerformAction() { + public void testPerformAction() throws Exception { IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10)).settings(settings(Version.CURRENT)) .numberOfShards(randomIntBetween(1, 5)).numberOfReplicas(randomIntBetween(0, 5)).build(); @@ -81,6 +86,13 @@ public void testPerformAction() { Mockito.when(client.admin()).thenReturn(adminClient); Mockito.when(adminClient.indices()).thenReturn(indicesClient); + + final UpdateSettingsResponse response; + try (XContentParser parser = XContentType.JSON.xContent() + .createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, "{\"acknowledged\": true}")) { + response = UpdateSettingsResponse.fromXContent(parser); + } + Mockito.doAnswer(new Answer() { @Override @@ -88,8 +100,9 @@ public Void answer(InvocationOnMock invocation) throws Throwable { UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0]; @SuppressWarnings("unchecked") ActionListener listener = (ActionListener) invocation.getArguments()[1]; - UpdateSettingsTestHelper.assertSettingsRequest(request, step.getSettings(), indexMetaData.getIndex().getName()); - listener.onResponse(UpdateSettingsTestHelper.createMockResponse(true)); + assertThat(request.settings(), equalTo(step.getSettings())); + assertThat(request.indices(), equalTo(indexMetaData.getIndex().getName())); + listener.onResponse(response); return null; } @@ -135,7 +148,8 @@ public Void answer(InvocationOnMock invocation) throws Throwable { UpdateSettingsRequest request = (UpdateSettingsRequest) invocation.getArguments()[0]; @SuppressWarnings("unchecked") ActionListener listener = (ActionListener) invocation.getArguments()[1]; - UpdateSettingsTestHelper.assertSettingsRequest(request, step.getSettings(), indexMetaData.getIndex().getName()); + assertThat(request.settings(), equalTo(step.getSettings())); + assertThat(request.indices(), equalTo(indexMetaData.getIndex().getName())); listener.onFailure(exception); return null; } From f054fe4a61125bd9a37a5756f6f192e418aeea04 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Wed, 1 Aug 2018 12:35:53 -0600 Subject: [PATCH 2/3] Fix array comparison --- .../xpack/core/indexlifecycle/UpdateSettingsStepTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java index 3c62e84746fcd..1b78d7fa98e8c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java @@ -101,7 +101,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable { @SuppressWarnings("unchecked") ActionListener listener = (ActionListener) invocation.getArguments()[1]; assertThat(request.settings(), equalTo(step.getSettings())); - assertThat(request.indices(), equalTo(indexMetaData.getIndex().getName())); + assertThat(request.indices(), equalTo(new String[] {indexMetaData.getIndex().getName()})); listener.onResponse(response); return null; } @@ -149,7 +149,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable { @SuppressWarnings("unchecked") ActionListener listener = (ActionListener) invocation.getArguments()[1]; assertThat(request.settings(), equalTo(step.getSettings())); - assertThat(request.indices(), equalTo(indexMetaData.getIndex().getName())); + assertThat(request.indices(), equalTo(new String[] {indexMetaData.getIndex().getName()})); listener.onFailure(exception); return null; } From d23fe92fa965060f44ba7c68139bc8b3695adba6 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Thu, 2 Aug 2018 11:47:01 -0600 Subject: [PATCH 3/3] Make constructor for UpdateSettingsResponse public, remove JSON parsing --- .../indices/settings/put/UpdateSettingsResponse.java | 2 +- .../SetSingleNodeAllocateStepTests.java | 12 +----------- .../core/indexlifecycle/UpdateSettingsStepTests.java | 12 +----------- 3 files changed, 3 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsResponse.java b/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsResponse.java index 6792d1859260d..6e513a1b3b4e6 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsResponse.java @@ -30,7 +30,7 @@ public class UpdateSettingsResponse extends AcknowledgedResponse { UpdateSettingsResponse() { } - UpdateSettingsResponse(boolean acknowledged) { + public UpdateSettingsResponse(boolean acknowledged) { super(acknowledged); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SetSingleNodeAllocateStepTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SetSingleNodeAllocateStepTests.java index ac7c9987a17a9..78b3cf502356c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SetSingleNodeAllocateStepTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/SetSingleNodeAllocateStepTests.java @@ -26,9 +26,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.Settings.Builder; import org.elasticsearch.common.transport.TransportAddress; -import org.elasticsearch.common.xcontent.NamedXContentRegistry; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.shard.ShardId; @@ -46,7 +43,6 @@ import java.util.Set; import java.util.stream.Collectors; -import static org.elasticsearch.common.xcontent.DeprecationHandler.THROW_UNSUPPORTED_OPERATION; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.equalTo; @@ -361,12 +357,6 @@ private void assertNodeSelected(IndexMetaData indexMetaData, Index index, Mockito.when(client.admin()).thenReturn(adminClient); Mockito.when(adminClient.indices()).thenReturn(indicesClient); - final UpdateSettingsResponse response; - try (XContentParser parser = XContentType.JSON.xContent() - .createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, "{\"acknowledged\": true}")) { - response = UpdateSettingsResponse.fromXContent(parser); - } - Mockito.doAnswer(new Answer() { @Override @@ -377,7 +367,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable { assertSettingsRequestContainsValueFrom(request, IndexMetaData.INDEX_ROUTING_REQUIRE_GROUP_SETTING.getKey() + "_name", validNodeNames, true, indexMetaData.getIndex().getName()); - listener.onResponse(response); + listener.onResponse(new UpdateSettingsResponse(true)); return null; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java index 1b78d7fa98e8c..a592e0bdffb3a 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/UpdateSettingsStepTests.java @@ -16,9 +16,6 @@ import org.elasticsearch.client.IndicesAdminClient; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.xcontent.NamedXContentRegistry; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.xpack.core.indexlifecycle.AsyncActionStep.Listener; import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey; import org.junit.Before; @@ -26,7 +23,6 @@ import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; -import static org.elasticsearch.common.xcontent.DeprecationHandler.THROW_UNSUPPORTED_OPERATION; import static org.hamcrest.Matchers.equalTo; public class UpdateSettingsStepTests extends AbstractStepTestCase { @@ -87,12 +83,6 @@ public void testPerformAction() throws Exception { Mockito.when(client.admin()).thenReturn(adminClient); Mockito.when(adminClient.indices()).thenReturn(indicesClient); - final UpdateSettingsResponse response; - try (XContentParser parser = XContentType.JSON.xContent() - .createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, "{\"acknowledged\": true}")) { - response = UpdateSettingsResponse.fromXContent(parser); - } - Mockito.doAnswer(new Answer() { @Override @@ -102,7 +92,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable { ActionListener listener = (ActionListener) invocation.getArguments()[1]; assertThat(request.settings(), equalTo(step.getSettings())); assertThat(request.indices(), equalTo(new String[] {indexMetaData.getIndex().getName()})); - listener.onResponse(response); + listener.onResponse(new UpdateSettingsResponse(true)); return null; }