Clean Up Snapshot Create Rest API (#31779)

Make SnapshotInfo and CreateSnapshotResponse parsers lenient for backwards compatibility. Remove extraneous fields from CreateSnapshotRequest toXContent.
elastic · Jul 13, 2018 · fc699c6 · fc699c6
1 parent 6e1b187
commit fc699c6
Show file tree

Hide file tree

Showing 11 changed files with 56 additions and 93 deletions.
diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/SnapshotClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/SnapshotClient.java
@@ -174,7 +174,7 @@ public void verifyRepositoryAsync(VerifyRepositoryRequest verifyRepositoryReques
      * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-snapshots.html"> Snapshot and Restore
      * API on elastic.co</a>
      */
-    public CreateSnapshotResponse createSnapshot(CreateSnapshotRequest createSnapshotRequest, RequestOptions options)
+    public CreateSnapshotResponse create(CreateSnapshotRequest createSnapshotRequest, RequestOptions options)
         throws IOException {
         return restHighLevelClient.performRequestAndParseEntity(createSnapshotRequest, RequestConverters::createSnapshot, options,
             CreateSnapshotResponse::fromXContent, emptySet());
@@ -186,7 +186,7 @@ public CreateSnapshotResponse createSnapshot(CreateSnapshotRequest createSnapsho
      * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-snapshots.html"> Snapshot and Restore
      * API on elastic.co</a>
      */
-    public void createSnapshotAsync(CreateSnapshotRequest createSnapshotRequest, RequestOptions options,
+    public void createAsync(CreateSnapshotRequest createSnapshotRequest, RequestOptions options,
                                     ActionListener<CreateSnapshotResponse> listener) {
         restHighLevelClient.performRequestAsyncAndParseEntity(createSnapshotRequest, RequestConverters::createSnapshot, options,
             CreateSnapshotResponse::fromXContent, listener, emptySet());

diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/SnapshotIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/SnapshotIT.java
@@ -57,8 +57,8 @@ private PutRepositoryResponse createTestRepository(String repository, String typ
     private CreateSnapshotResponse createTestSnapshot(CreateSnapshotRequest createSnapshotRequest) throws IOException {
         // assumes the repository already exists
 
-        return execute(createSnapshotRequest, highLevelClient().snapshot()::createSnapshot,
-            highLevelClient().snapshot()::createSnapshotAsync);
+        return execute(createSnapshotRequest, highLevelClient().snapshot()::create,
+            highLevelClient().snapshot()::createAsync);
     }
 
     public void testCreateRepository() throws IOException {

diff --git a/...l/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java b/...l/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java
@@ -420,14 +420,20 @@ public void testSnapshotCreate() throws IOException {
         // end::create-snapshot-request-waitForCompletion
 
         // tag::create-snapshot-execute
-        CreateSnapshotResponse response = client.snapshot().createSnapshot(request, RequestOptions.DEFAULT);
+        CreateSnapshotResponse response = client.snapshot().create(request, RequestOptions.DEFAULT);
         // end::create-snapshot-execute
 
         // tag::create-snapshot-response
         RestStatus status = response.status(); // <1>
         // end::create-snapshot-response
 
         assertEquals(RestStatus.OK, status);
+
+        // tag::create-snapshot-response-snapshot-info
+        SnapshotInfo snapshotInfo = response.getSnapshotInfo(); // <1>
+        // end::create-snapshot-response-snapshot-info
+
+        assertNotNull(snapshotInfo);
     }
 
     public void testSnapshotCreateAsync() throws InterruptedException {
@@ -455,7 +461,7 @@ public void onFailure(Exception exception) {
             listener = new LatchedActionListener<>(listener, latch);
 
             // tag::create-snapshot-execute-async
-            client.snapshot().createSnapshotAsync(request, RequestOptions.DEFAULT, listener); // <1>
+            client.snapshot().createAsync(request, RequestOptions.DEFAULT, listener); // <1>
             // end::create-snapshot-execute-async
 
             assertTrue(latch.await(30L, TimeUnit.SECONDS));

diff --git a/docs/java-rest/high-level/snapshot/create_snapshot.asciidoc b/docs/java-rest/high-level/snapshot/create_snapshot.asciidoc
@@ -73,11 +73,22 @@ include-tagged::{doc-tests}/SnapshotClientDocumentationIT.java[create-snapshot-r
 [[java-rest-high-snapshot-create-snapshot-sync]]
 ==== Synchronous Execution
 
+Execute a `CreateSnapshotRequest` synchronously to receive a `CreateSnapshotResponse`.
+
 ["source","java",subs="attributes,callouts,macros"]
 --------------------------------------------------
 include-tagged::{doc-tests}/SnapshotClientDocumentationIT.java[create-snapshot-execute]
 --------------------------------------------------
 
+Retrieve the `SnapshotInfo` from a `CreateSnapshotResponse` when the snapshot is fully created.
+(The `waitForCompletion` parameter is `true`).
+
+["source","java",subs="attributes,callouts,macros"]
+--------------------------------------------------
+include-tagged::{doc-tests}/SnapshotClientDocumentationIT.java[create-snapshot-response-snapshot-info]
+--------------------------------------------------
+<1> The `SnapshotInfo` object.
+
 [[java-rest-high-snapshot-create-snapshot-async]]
 ==== Asynchronous Execution
 

diff --git a/...n/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequest.java b/...n/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequest.java
@@ -42,9 +42,9 @@
 
 import static org.elasticsearch.action.ValidateActions.addValidationError;
 import static org.elasticsearch.common.Strings.EMPTY_ARRAY;
+import static org.elasticsearch.common.settings.Settings.Builder.EMPTY_SETTINGS;
 import static org.elasticsearch.common.settings.Settings.readSettingsFromStream;
 import static org.elasticsearch.common.settings.Settings.writeSettingsToStream;
-import static org.elasticsearch.common.settings.Settings.Builder.EMPTY_SETTINGS;
 import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue;
 
 /**
@@ -408,8 +408,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         if (indicesOptions != null) {
             indicesOptions.toXContent(builder, params);
         }
-        builder.field("wait_for_completion", waitForCompletion);
-        builder.field("master_node_timeout", masterNodeTimeout.toString());
         builder.endObject();
         return builder;
     }

diff --git a/.../java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotResponse.java b/.../java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotResponse.java
@@ -21,14 +21,16 @@
 
 import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.common.Nullable;
+import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.xcontent.ObjectParser;
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.common.xcontent.XContentParser.Token;
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.snapshots.SnapshotInfo;
+import org.elasticsearch.snapshots.SnapshotInfo.SnapshotInfoBuilder;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -38,6 +40,14 @@
  */
 public class CreateSnapshotResponse extends ActionResponse implements ToXContentObject {
 
+    private static final ObjectParser<CreateSnapshotResponse, Void> PARSER =
+        new ObjectParser<>(CreateSnapshotResponse.class.getName(), true, CreateSnapshotResponse::new);
+
+    static {
+        PARSER.declareObject(CreateSnapshotResponse::setSnapshotInfoFromBuilder,
+            SnapshotInfo.SNAPSHOT_INFO_PARSER, new ParseField("snapshot"));
+    }
+
     @Nullable
     private SnapshotInfo snapshotInfo;
 
@@ -48,8 +58,8 @@ public class CreateSnapshotResponse extends ActionResponse implements ToXContent
     CreateSnapshotResponse() {
     }
 
-    void setSnapshotInfo(SnapshotInfo snapshotInfo) {
-        this.snapshotInfo = snapshotInfo;
+    private void setSnapshotInfoFromBuilder(SnapshotInfoBuilder snapshotInfoBuilder) {
+        this.snapshotInfo = snapshotInfoBuilder.build();
     }
 
     /**
@@ -101,38 +111,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         return builder;
     }
 
-    public static CreateSnapshotResponse fromXContent(XContentParser parser) throws IOException {
-        CreateSnapshotResponse createSnapshotResponse = new CreateSnapshotResponse();
-
-        parser.nextToken(); // move to '{'
-
-        if (parser.currentToken() != Token.START_OBJECT) {
-            throw new IllegalArgumentException("unexpected token [" + parser.currentToken() + "], expected ['{']");
-        }
-
-        parser.nextToken(); // move to 'snapshot' || 'accepted'
-
-        if ("snapshot".equals(parser.currentName())) {
-            createSnapshotResponse.snapshotInfo = SnapshotInfo.fromXContent(parser);
-        } else if ("accepted".equals(parser.currentName())) {
-            parser.nextToken(); // move to 'accepted' field value
-
-            if (parser.booleanValue()) {
-                // ensure accepted is a boolean value
-            }
-
-            parser.nextToken(); // move past 'true'/'false'
-        } else {
-            throw new IllegalArgumentException("unexpected token [" + parser.currentToken() + "] expected ['snapshot', 'accepted']");
-        }
-
-        if (parser.currentToken() != Token.END_OBJECT) {
-            throw new IllegalArgumentException("unexpected token [" + parser.currentToken() + "], expected ['}']");
-        }
-
-        parser.nextToken(); // move past '}'
-
-        return createSnapshotResponse;
+    public static CreateSnapshotResponse fromXContent(XContentParser parser) {
+        return PARSER.apply(parser, null);
     }
 
     @Override

diff --git a/server/src/main/java/org/elasticsearch/action/support/IndicesOptions.java b/server/src/main/java/org/elasticsearch/action/support/IndicesOptions.java
@@ -22,6 +22,7 @@
 import org.elasticsearch.Version;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.ToXContentFragment;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.rest.RestRequest;
@@ -322,21 +323,6 @@ public static IndicesOptions fromMap(Map<String, Object> map, IndicesOptions def
                 defaultSettings);
     }
 
-    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startArray("expand_wildcards");
-        for (WildcardStates expandWildcard : expandWildcards) {
-            builder.value(expandWildcard.toString().toLowerCase(Locale.ROOT));
-        }
-        builder.endArray();
-        builder.field("ignore_unavailable", ignoreUnavailable());
-        builder.field("allow_no_indices", allowNoIndices());
-        builder.field("forbid_aliases_to_multiple_indices", allowAliasesToMultipleIndices() == false);
-        builder.field("forbid_closed_indices", forbidClosedIndices());
-        builder.field("ignore_aliases", ignoreAliases());
-        return builder;
-    }
-
     /**
      * Returns true if the name represents a valid name for one of the indices option
      * false otherwise
@@ -366,6 +352,18 @@ public static IndicesOptions fromParameters(Object wildcardsString, Object ignor
         );
     }
 
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException {
+        builder.startArray("expand_wildcards");
+        for (WildcardStates expandWildcard : expandWildcards) {
+            builder.value(expandWildcard.toString().toLowerCase(Locale.ROOT));
+        }
+        builder.endArray();
+        builder.field("ignore_unavailable", ignoreUnavailable());
+        builder.field("allow_no_indices", allowNoIndices());
+        return builder;
+    }
+
     /**
      * @return indices options that requires every specified index to exist, expands wildcards only to open indices and
      *         allows that no indices are resolved from wildcard expressions (not returning an error).

diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java
@@ -138,22 +138,6 @@ private void setShardFailures(List<SnapshotShardFailure> shardFailures) {
             this.shardFailures = shardFailures;
         }
 
-        private void ignoreVersion(String version) {
-            // ignore extra field
-        }
-
-        private void ignoreStartTime(String startTime) {
-            // ignore extra field
-        }
-
-        private void ignoreEndTime(String endTime) {
-            // ignore extra field
-        }
-
-        private void ignoreDurationInMillis(long durationInMillis) {
-            // ignore extra field
-        }
-
         public SnapshotInfo build() {
             SnapshotId snapshotId = new SnapshotId(snapshotName, snapshotUUID);
 
@@ -195,10 +179,6 @@ private void setSuccessfulShards(int successfulShards) {
         int getSuccessfulShards() {
             return successfulShards;
         }
-
-        private void ignoreFailedShards(int failedShards) {
-            // ignore extra field
-        }
     }
 
     public static final ObjectParser<SnapshotInfoBuilder, Void> SNAPSHOT_INFO_PARSER =
@@ -220,14 +200,9 @@ private void ignoreFailedShards(int failedShards) {
         SNAPSHOT_INFO_PARSER.declareInt(SnapshotInfoBuilder::setVersion, new ParseField(VERSION_ID));
         SNAPSHOT_INFO_PARSER.declareObjectArray(SnapshotInfoBuilder::setShardFailures, SnapshotShardFailure.SNAPSHOT_SHARD_FAILURE_PARSER,
             new ParseField(FAILURES));
-        SNAPSHOT_INFO_PARSER.declareString(SnapshotInfoBuilder::ignoreVersion, new ParseField(VERSION));
-        SNAPSHOT_INFO_PARSER.declareString(SnapshotInfoBuilder::ignoreStartTime, new ParseField(START_TIME));
-        SNAPSHOT_INFO_PARSER.declareString(SnapshotInfoBuilder::ignoreEndTime, new ParseField(END_TIME));
-        SNAPSHOT_INFO_PARSER.declareLong(SnapshotInfoBuilder::ignoreDurationInMillis, new ParseField(DURATION_IN_MILLIS));
 
         SHARD_STATS_PARSER.declareInt(ShardStatsBuilder::setTotalShards, new ParseField(TOTAL));
         SHARD_STATS_PARSER.declareInt(ShardStatsBuilder::setSuccessfulShards, new ParseField(SUCCESSFUL));
-        SHARD_STATS_PARSER.declareInt(ShardStatsBuilder::ignoreFailedShards, new ParseField(FAILED));
     }
 
     private final SnapshotId snapshotId;

diff --git a/...a/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequestTests.java b/...a/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequestTests.java
@@ -102,8 +102,8 @@ public void testToXContent() throws IOException {
                 NamedXContentRegistry.EMPTY, null, BytesReference.bytes(builder).streamInput());
         Map<String, Object> map = parser.mapOrdered();
         CreateSnapshotRequest processed = new CreateSnapshotRequest((String)map.get("repository"), (String)map.get("snapshot"));
-        processed.waitForCompletion((boolean)map.getOrDefault("wait_for_completion", false));
-        processed.masterNodeTimeout((String)map.getOrDefault("master_node_timeout", "30s"));
+        processed.waitForCompletion(original.waitForCompletion());
+        processed.masterNodeTimeout(original.masterNodeTimeout());
         processed.source(map);
 
         assertEquals(original, processed);

diff --git a/.../org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotResponseTests.java b/.../org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotResponseTests.java
@@ -40,7 +40,7 @@ protected CreateSnapshotResponse doParseInstance(XContentParser parser) throws I
 
     @Override
     protected boolean supportsUnknownFields() {
-        return false;
+        return true;
     }
 
     @Override
@@ -63,9 +63,7 @@ protected CreateSnapshotResponse createTestInstance() {
 
         boolean globalState = randomBoolean();
 
-        CreateSnapshotResponse response = new CreateSnapshotResponse();
-        response.setSnapshotInfo(
+        return new CreateSnapshotResponse(
             new SnapshotInfo(snapshotId, indices, startTime, reason, endTime, totalShards, shardFailures, globalState));
-        return response;
     }
 }
diff --git a/server/src/test/java/org/elasticsearch/action/support/IndicesOptionsTests.java b/server/src/test/java/org/elasticsearch/action/support/IndicesOptionsTests.java
@@ -324,8 +324,5 @@ public void testToXContent() throws IOException {
         }
         assertEquals(map.get("ignore_unavailable"), options.contains(Option.IGNORE_UNAVAILABLE));
         assertEquals(map.get("allow_no_indices"), options.contains(Option.ALLOW_NO_INDICES));
-        assertEquals(map.get("forbid_aliases_to_multiple_indices"), options.contains(Option.FORBID_ALIASES_TO_MULTIPLE_INDICES));
-        assertEquals(map.get("forbid_closed_indices"), options.contains(Option.FORBID_CLOSED_INDICES));
-        assertEquals(map.get("ignore_aliases"), options.contains(Option.IGNORE_ALIASES));
     }
 }