From 52ff341814f6b840c780902e706071fea80dadf7 Mon Sep 17 00:00:00 2001
From: David Turner <david.turner@elastic.co>
Date: Mon, 9 Mar 2020 10:11:17 +0000
Subject: [PATCH] Deprecate passing settings in restore requests (#53268)

Today we accept a `settings` field in snapshot restore requests, but this field
is not used. This commit deprecates it.
---
 .../restore/RestoreSnapshotRequest.java       | 98 +++----------------
 .../RestoreSnapshotRequestBuilder.java        | 54 ----------
 .../restore/RestoreSnapshotRequestTests.java  | 10 --
 .../snapshots/SnapshotRequestsTests.java      |  3 +-
 4 files changed, 16 insertions(+), 149 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java
index 0d2cac9b401..1c6d72c948b 100644
--- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java
+++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java
@@ -19,13 +19,16 @@
 
 package org.elasticsearch.action.admin.cluster.snapshots.restore;
 
+import org.apache.logging.log4j.LogManager;
 import org.elasticsearch.ElasticsearchGenerationException;
+import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.action.support.master.MasterNodeRequest;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -50,6 +53,8 @@ import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBo
  */
 public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotRequest> implements ToXContentObject {
 
+    private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(LogManager.getLogger(RestoreSnapshotRequest.class));
+
     private String snapshot;
     private String repository;
     private String[] indices = Strings.EMPTY_ARRAY;
@@ -60,7 +65,6 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
     private boolean includeGlobalState = false;
     private boolean partial = false;
     private boolean includeAliases = true;
-    private Settings settings = EMPTY_SETTINGS;
     private Settings indexSettings = EMPTY_SETTINGS;
     private String[] ignoreIndexSettings = Strings.EMPTY_ARRAY;
 
@@ -90,7 +94,9 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
         includeGlobalState = in.readBoolean();
         partial = in.readBoolean();
         includeAliases = in.readBoolean();
-        settings = readSettingsFromStream(in);
+        if (in.getVersion().before(Version.V_7_7_0)) {
+            readSettingsFromStream(in); // formerly the unused settings field
+        }
         indexSettings = readSettingsFromStream(in);
         ignoreIndexSettings = in.readStringArray();
     }
@@ -108,7 +114,9 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
         out.writeBoolean(includeGlobalState);
         out.writeBoolean(partial);
         out.writeBoolean(includeAliases);
-        writeSettingsToStream(settings, out);
+        if (out.getVersion().before(Version.V_7_7_0)) {
+            writeSettingsToStream(Settings.EMPTY, out); // formerly the unused settings field
+        }
         writeSettingsToStream(indexSettings, out);
         out.writeStringArray(ignoreIndexSettings);
     }
@@ -128,9 +136,6 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
         if (indicesOptions == null) {
             validationException = addValidationError("indicesOptions is missing", validationException);
         }
-        if (settings == null) {
-            validationException = addValidationError("settings are missing", validationException);
-        }
         if (indexSettings == null) {
             validationException = addValidationError("indexSettings are missing", validationException);
         }
@@ -324,74 +329,6 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
         return this;
     }
 
-    /**
-     * Sets repository-specific restore settings.
-     * <p>
-     * See repository documentation for more information.
-     *
-     * @param settings repository-specific snapshot settings
-     * @return this request
-     */
-    public RestoreSnapshotRequest settings(Settings settings) {
-        this.settings = settings;
-        return this;
-    }
-
-    /**
-     * Sets repository-specific restore settings.
-     * <p>
-     * See repository documentation for more information.
-     *
-     * @param settings repository-specific snapshot settings
-     * @return this request
-     */
-    public RestoreSnapshotRequest settings(Settings.Builder settings) {
-        this.settings = settings.build();
-        return this;
-    }
-
-    /**
-     * Sets repository-specific restore settings in JSON or YAML format
-     * <p>
-     * See repository documentation for more information.
-     *
-     * @param source repository-specific snapshot settings
-     * @param xContentType the content type of the source
-     * @return this request
-     */
-    public RestoreSnapshotRequest settings(String source, XContentType xContentType) {
-        this.settings = Settings.builder().loadFromSource(source, xContentType).build();
-        return this;
-    }
-
-    /**
-     * Sets repository-specific restore settings
-     * <p>
-     * See repository documentation for more information.
-     *
-     * @param source repository-specific snapshot settings
-     * @return this request
-     */
-    public RestoreSnapshotRequest settings(Map<String, Object> source) {
-        try {
-            XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON);
-            builder.map(source);
-            settings(Strings.toString(builder), builder.contentType());
-        } catch (IOException e) {
-            throw new ElasticsearchGenerationException("Failed to generate [" + source + "]", e);
-        }
-        return this;
-    }
-
-    /**
-     * Returns repository-specific restore settings
-     *
-     * @return restore settings
-     */
-    public Settings settings() {
-        return this.settings;
-    }
-
     /**
      * Sets the list of index settings and index settings groups that shouldn't be restored from snapshot
      */
@@ -526,7 +463,8 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
                 if (!(entry.getValue() instanceof Map)) {
                     throw new IllegalArgumentException("malformed settings section");
                 }
-                settings((Map<String, Object>) entry.getValue());
+                DEPRECATION_LOGGER.deprecatedAndMaybeLog("RestoreSnapshotRequest#settings",
+                    "specifying [settings] when restoring a snapshot has no effect and will not be supported in a future version");
             } else if (name.equals("include_global_state")) {
                 includeGlobalState = nodeBooleanValue(entry.getValue(), "include_global_state");
             } else if (name.equals("include_aliases")) {
@@ -586,13 +524,6 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
         builder.field("include_global_state", includeGlobalState);
         builder.field("partial", partial);
         builder.field("include_aliases", includeAliases);
-        if (settings != null) {
-            builder.startObject("settings");
-            if (settings.isEmpty() == false) {
-                settings.toXContent(builder, params);
-            }
-            builder.endObject();
-        }
         if (indexSettings != null) {
             builder.startObject("index_settings");
             if (indexSettings.isEmpty() == false) {
@@ -629,7 +560,6 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
             Objects.equals(indicesOptions, that.indicesOptions) &&
             Objects.equals(renamePattern, that.renamePattern) &&
             Objects.equals(renameReplacement, that.renameReplacement) &&
-            Objects.equals(settings, that.settings) &&
             Objects.equals(indexSettings, that.indexSettings) &&
             Arrays.equals(ignoreIndexSettings, that.ignoreIndexSettings);
     }
@@ -637,7 +567,7 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
     @Override
     public int hashCode() {
         int result = Objects.hash(snapshot, repository, indicesOptions, renamePattern, renameReplacement, waitForCompletion,
-            includeGlobalState, partial, includeAliases, settings, indexSettings);
+            includeGlobalState, partial, includeAliases, indexSettings);
         result = 31 * result + Arrays.hashCode(indices);
         result = 31 * result + Arrays.hashCode(ignoreIndexSettings);
         return result;
diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java
index f5302616447..08d509ce3ac 100644
--- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java
+++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java
@@ -127,60 +127,6 @@ public class RestoreSnapshotRequestBuilder extends MasterNodeOperationRequestBui
         return this;
     }
 
-
-    /**
-     * Sets repository-specific restore settings.
-     * <p>
-     * See repository documentation for more information.
-     *
-     * @param settings repository-specific snapshot settings
-     * @return this builder
-     */
-    public RestoreSnapshotRequestBuilder setSettings(Settings settings) {
-        request.settings(settings);
-        return this;
-    }
-
-    /**
-     * Sets repository-specific restore settings.
-     * <p>
-     * See repository documentation for more information.
-     *
-     * @param settings repository-specific snapshot settings
-     * @return this builder
-     */
-    public RestoreSnapshotRequestBuilder setSettings(Settings.Builder settings) {
-        request.settings(settings);
-        return this;
-    }
-
-    /**
-     * Sets repository-specific restore settings in JSON or YAML format
-     * <p>
-     * See repository documentation for more information.
-     *
-     * @param source repository-specific snapshot settings
-     * @param xContentType the content type of the source
-     * @return this builder
-     */
-    public RestoreSnapshotRequestBuilder setSettings(String source, XContentType xContentType) {
-        request.settings(source, xContentType);
-        return this;
-    }
-
-    /**
-     * Sets repository-specific restore settings
-     * <p>
-     * See repository documentation for more information.
-     *
-     * @param source repository-specific snapshot settings
-     * @return this builder
-     */
-    public RestoreSnapshotRequestBuilder setSettings(Map<String, Object> source) {
-        request.settings(source);
-        return this;
-    }
-
     /**
      * If this parameter is set to true the operation will wait for completion of restore process before returning.
      *
diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java
index 59518b6e996..2254c6ba103 100644
--- a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java
+++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java
@@ -61,16 +61,6 @@ public class RestoreSnapshotRequestTests extends AbstractWireSerializingTestCase
         instance.partial(randomBoolean());
         instance.includeAliases(randomBoolean());
 
-        if (randomBoolean()) {
-            Map<String, Object> settings = new HashMap<>();
-            int count = randomInt(3) + 1;
-
-            for (int i = 0; i < count; ++i) {
-                settings.put(randomAlphaOfLengthBetween(2, 5), randomAlphaOfLengthBetween(2, 5));
-            }
-
-            instance.settings(settings);
-        }
         if (randomBoolean()) {
             Map<String, Object> indexSettings = new HashMap<>();
             int count = randomInt(3) + 1;
diff --git a/server/src/test/java/org/elasticsearch/snapshots/SnapshotRequestsTests.java b/server/src/test/java/org/elasticsearch/snapshots/SnapshotRequestsTests.java
index 44fe0d4dd5c..f70bfdcae40 100644
--- a/server/src/test/java/org/elasticsearch/snapshots/SnapshotRequestsTests.java
+++ b/server/src/test/java/org/elasticsearch/snapshots/SnapshotRequestsTests.java
@@ -91,12 +91,13 @@ public class SnapshotRequestsTests extends ESTestCase {
         assertEquals("rename-from", request.renamePattern());
         assertEquals("rename-to", request.renameReplacement());
         assertEquals(partial, request.partial());
-        assertEquals("val1", request.settings().get("set1"));
         assertArrayEquals(request.ignoreIndexSettings(), new String[]{"set2", "set3"});
         boolean expectedIgnoreAvailable = includeIgnoreUnavailable
             ? indicesOptions.ignoreUnavailable()
             : IndicesOptions.strictExpandOpen().ignoreUnavailable();
         assertEquals(expectedIgnoreAvailable, request.indicesOptions().ignoreUnavailable());
+
+        assertWarnings("specifying [settings] when restoring a snapshot has no effect and will not be supported in a future version");
     }
 
     public void testCreateSnapshotRequestParsing() throws IOException {