Snapshot/Restore: Add snapshot name validation logic to all snapshot operation

Make sure snapshot name validation occurs earlier in all snapshot operations.

core/src/main/java/org/elasticsearch/snapshots/InvalidSnapshotNameException.java

@@ -19,14 +19,13 @@
 
 package org.elasticsearch.snapshots;
 
-import org.elasticsearch.ElasticsearchWrapperException;
 import org.elasticsearch.cluster.metadata.SnapshotId;
 import org.elasticsearch.rest.RestStatus;
 
 /**
  * Thrown on the attempt to create a snapshot with invalid name
  */
-public class InvalidSnapshotNameException extends SnapshotException implements ElasticsearchWrapperException {
+public class InvalidSnapshotNameException extends SnapshotException {
 
     public InvalidSnapshotNameException(SnapshotId snapshot, String desc) {
         super(snapshot, "Invalid snapshot name [" + snapshot.getSnapshot() + "], " + desc);

core/src/main/java/org/elasticsearch/snapshots/SnapshotCreationException.java

@@ -25,7 +25,7 @@ import org.elasticsearch.cluster.metadata.SnapshotId;
 /**
  * Thrown when snapshot creation fails completely
  */
-public class SnapshotCreationException extends SnapshotException implements ElasticsearchWrapperException {
+public class SnapshotCreationException extends SnapshotException {
 
     public SnapshotCreationException(SnapshotId snapshot, String message) {
         super(snapshot, message);

core/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

@@ -135,6 +135,7 @@ public class SnapshotsService extends AbstractLifecycleComponent<SnapshotsServic
      * @throws SnapshotMissingException if snapshot is not found
      */
     public Snapshot snapshot(SnapshotId snapshotId) {
+        validate(snapshotId);
         List<SnapshotsInProgress.Entry> entries = currentSnapshots(snapshotId.getRepository(), new String[]{snapshotId.getSnapshot()});
         if (!entries.isEmpty()) {
             return inProgressSnapshot(entries.iterator().next());
@@ -191,6 +192,7 @@ public class SnapshotsService extends AbstractLifecycleComponent<SnapshotsServic
      */
     public void createSnapshot(final SnapshotRequest request, final CreateSnapshotListener listener) {
         final SnapshotId snapshotId = new SnapshotId(request.repository(), request.name());
+        validate(snapshotId);
         clusterService.submitStateUpdateTask(request.cause(), new TimeoutClusterStateUpdateTask() {
 
             private SnapshotsInProgress.Entry newSnapshot = null;
@@ -253,26 +255,31 @@ public class SnapshotsService extends AbstractLifecycleComponent<SnapshotsServic
         if (repositoriesMetaData == null || repositoriesMetaData.repository(request.repository()) == null) {
             throw new RepositoryMissingException(request.repository());
         }
-        if (!Strings.hasLength(request.name())) {
+        validate(new SnapshotId(request.repository(), request.name()));
-            throw new InvalidSnapshotNameException(new SnapshotId(request.repository(), request.name()), "cannot be empty");
     }
-        if (request.name().contains(" ")) {
+    
-            throw new InvalidSnapshotNameException(new SnapshotId(request.repository(), request.name()), "must not contain whitespace");
+    private static void validate(SnapshotId snapshotId) {
+        String name = snapshotId.getSnapshot();
+        if (!Strings.hasLength(name)) {
+            throw new InvalidSnapshotNameException(snapshotId, "cannot be empty");
         }
-        if (request.name().contains(",")) {
+        if (name.contains(" ")) {
-            throw new InvalidSnapshotNameException(new SnapshotId(request.repository(), request.name()), "must not contain ','");
+            throw new InvalidSnapshotNameException(snapshotId, "must not contain whitespace");
         }
-        if (request.name().contains("#")) {
+        if (name.contains(",")) {
-            throw new InvalidSnapshotNameException(new SnapshotId(request.repository(), request.name()), "must not contain '#'");
+            throw new InvalidSnapshotNameException(snapshotId, "must not contain ','");
         }
-        if (request.name().charAt(0) == '_') {
+        if (name.contains("#")) {
-            throw new InvalidSnapshotNameException(new SnapshotId(request.repository(), request.name()), "must not start with '_'");
+            throw new InvalidSnapshotNameException(snapshotId, "must not contain '#'");
         }
-        if (!request.name().toLowerCase(Locale.ROOT).equals(request.name())) {
+        if (name.charAt(0) == '_') {
-            throw new InvalidSnapshotNameException(new SnapshotId(request.repository(), request.name()), "must be lowercase");
+            throw new InvalidSnapshotNameException(snapshotId, "must not start with '_'");
         }
-        if (!Strings.validFileName(request.name())) {
+        if (!name.toLowerCase(Locale.ROOT).equals(name)) {
-            throw new InvalidSnapshotNameException(new SnapshotId(request.repository(), request.name()), "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
+            throw new InvalidSnapshotNameException(snapshotId, "must be lowercase");
+        }
+        if (!Strings.validFileName(name)) {
+            throw new InvalidSnapshotNameException(snapshotId, "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
         }
     }
 
@@ -316,7 +323,6 @@ public class SnapshotsService extends AbstractLifecycleComponent<SnapshotsServic
 
                 @Override
                 public ClusterState execute(ClusterState currentState) {
-                    MetaData metaData = currentState.metaData();
                     SnapshotsInProgress snapshots = currentState.custom(SnapshotsInProgress.TYPE);
                     ImmutableList.Builder<SnapshotsInProgress.Entry> entries = ImmutableList.builder();
                     for (SnapshotsInProgress.Entry entry : snapshots.entries()) {
@@ -472,6 +478,7 @@ public class SnapshotsService extends AbstractLifecycleComponent<SnapshotsServic
      * @return map of shard id to snapshot status
      */
     public ImmutableMap<ShardId, IndexShardSnapshotStatus> currentSnapshotShards(SnapshotId snapshotId) {
+        validate(snapshotId);
         SnapshotShards snapshotShards = shardSnapshots.get(snapshotId);
         if (snapshotShards == null) {
             return null;
@@ -491,6 +498,7 @@ public class SnapshotsService extends AbstractLifecycleComponent<SnapshotsServic
      * @return map of shard id to snapshot status
      */
     public ImmutableMap<ShardId, IndexShardSnapshotStatus> snapshotShards(SnapshotId snapshotId) throws IOException {
+        validate(snapshotId);
         ImmutableMap.Builder<ShardId, IndexShardSnapshotStatus> shardStatusBuilder = ImmutableMap.builder();
         Repository repository = repositoriesService.repository(snapshotId.getRepository());
         IndexShardRepository indexShardRepository = repositoriesService.indexShardRepository(snapshotId.getRepository());
@@ -1174,6 +1182,7 @@ public class SnapshotsService extends AbstractLifecycleComponent<SnapshotsServic
      * @param listener   listener
      */
     public void deleteSnapshot(final SnapshotId snapshotId, final DeleteSnapshotListener listener) {
+        validate(snapshotId);
         clusterService.submitStateUpdateTask("delete snapshot", new ProcessedClusterStateUpdateTask() {
 
             boolean waitForSnapshot = false;

core/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreTests.java

@@ -1887,4 +1887,45 @@ public class SharedClusterSnapshotRestoreTests extends AbstractSnapshotTests {
         // Check that cluster state update task was called only once
         assertEquals(1, restoreListener.count());
     }
+
+    @Test
+    public void snapshotNameTest() throws Exception {
+
+        final Client client = client();
+
+        logger.info("-->  creating repository");
+        assertAcked(client.admin().cluster().preparePutRepository("test-repo")
+                .setType("fs").setSettings(Settings.settingsBuilder()
+                        .put("location", randomRepoPath())
+                        .put("compress", randomBoolean())
+                        .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES)));
+
+        try {
+            client.admin().cluster().prepareGetSnapshots("test-repo").setSnapshots("_foo").get();
+            fail("shouldn't be here");
+        } catch (InvalidSnapshotNameException ex) {
+            assertThat(ex.getMessage(), containsString("Invalid snapshot name"));
+        }
+
+        try {
+            client.admin().cluster().prepareCreateSnapshot("test-repo", "_foo").get();
+            fail("shouldn't be here");
+        } catch (InvalidSnapshotNameException ex) {
+            assertThat(ex.getMessage(), containsString("Invalid snapshot name"));
+        }
+
+        try {
+            client.admin().cluster().prepareDeleteSnapshot("test-repo", "_foo").get();
+            fail("shouldn't be here");
+        } catch (InvalidSnapshotNameException ex) {
+            assertThat(ex.getMessage(), containsString("Invalid snapshot name"));
+        }
+
+        try {
+            client.admin().cluster().prepareSnapshotStatus("test-repo").setSnapshots("_foo").get();
+            fail("shouldn't be here");
+        } catch (InvalidSnapshotNameException ex) {
+            assertThat(ex.getMessage(), containsString("Invalid snapshot name"));
+        }
+    }
 }