Update delete graffiti in manager to delete file

Author: courtneyeh

validator/api/src/main/java/tech/pegasys/teku/validator/api/GraffitiManager.java

@@ -16,6 +16,7 @@
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.util.Optional;
 import org.apache.logging.log4j.LogManager;
@@ -34,14 +35,6 @@ public GraffitiManager(final DataDirLayout dataDirLayout) {
     this.graffitiPath = createManagementDirectory(dataDirLayout);
   }
 
-  public Optional<String> setGraffiti(final BLSPublicKey publicKey, final String graffiti) {
-    return updateGraffiti(publicKey, graffiti.strip());
-  }
-
-  public Optional<String> deleteGraffiti(final BLSPublicKey publicKey) {
-    return updateGraffiti(publicKey);
-  }
-
   private Path createManagementDirectory(final DataDirLayout dataDirLayout) {
     final Path graffitiDirectory = dataDirLayout.getValidatorDataDirectory().resolve(GRAFFITI_DIR);
     if (!graffitiDirectory.toFile().exists() && !graffitiDirectory.toFile().mkdirs()) {
@@ -51,22 +44,19 @@ private Path createManagementDirectory(final DataDirLayout dataDirLayout) {
     return graffitiDirectory;
   }
 
-  private Optional<String> updateGraffiti(final BLSPublicKey publicKey) {
-    return updateGraffiti(publicKey, "");
-  }
-
-  private Optional<String> updateGraffiti(final BLSPublicKey publicKey, final String graffiti) {
-    final int graffitiSize = graffiti.getBytes(StandardCharsets.UTF_8).length;
+  public Optional<String> setGraffiti(final BLSPublicKey publicKey, final String graffiti) {
+    final String strippedGraffiti = graffiti.strip();
+    final int graffitiSize = strippedGraffiti.getBytes(StandardCharsets.UTF_8).length;
     if (graffitiSize > 32) {
       throw new IllegalArgumentException(
           String.format(
               "'%s' converts to %s bytes. Input must be 32 bytes or less.",
-              graffiti, graffitiSize));
+              strippedGraffiti, graffitiSize));
     }
 
     try {
       final Path file = graffitiPath.resolve(resolveFileName(publicKey));
-      Files.writeString(file, graffiti);
+      Files.writeString(file, strippedGraffiti);
     } catch (IOException e) {
       final String errorMessage =
           String.format("Unable to update graffiti for validator %s", publicKey);
@@ -76,25 +66,37 @@ private Optional<String> updateGraffiti(final BLSPublicKey publicKey, final Stri
     return Optional.empty();
   }
 
+  public Optional<String> deleteGraffiti(final BLSPublicKey publicKey) {
+    final Path file = graffitiPath.resolve(resolveFileName(publicKey));
+
+    try {
+      Files.delete(file);
+      return Optional.empty();
+    } catch (NoSuchFileException e) {
+      throw new IllegalArgumentException(
+          "Saved graffiti does not exist for validator " + publicKey);
+    } catch (IOException e) {
+      final String errorMessage =
+          String.format("Unable to delete graffiti for validator %s", publicKey);
+      LOG.error(errorMessage, e);
+      return Optional.of(errorMessage);
+    }
+  }
+
   public Optional<Bytes32> getGraffiti(final BLSPublicKey publicKey) {
     final Path filePath = graffitiPath.resolve(resolveFileName(publicKey));
     if (!filePath.toFile().exists()) {
       return Optional.empty();
     }
 
     try {
-      return Optional.of(GraffitiParser.loadFromFile(filePath)).filter(this::graffitiNotEmpty);
+      return Optional.of(GraffitiParser.loadFromFile(filePath));
     } catch (GraffitiLoaderException | IllegalArgumentException e) {
       LOG.error("Unable to read graffiti from storage.", e);
       return Optional.empty();
     }
   }
 
-  private boolean graffitiNotEmpty(final Bytes32 graffiti) {
-    final Bytes32 emptyBytesParsed = Bytes32Parser.toBytes32(new byte[0]);
-    return !graffiti.equals(emptyBytesParsed);
-  }
-
   private String resolveFileName(final BLSPublicKey publicKey) {
     return publicKey.toSSZBytes().toUnprefixedHexString() + ".txt";
   }

validator/api/src/test/java/tech/pegasys/teku/validator/api/GraffitiManagerTest.java

@@ -104,38 +104,43 @@ void setGraffiti_shouldThrowExceptionWhenGraffitiTooBig(@TempDir final Path temp
   }
 
   @Test
-  void deleteGraffiti_shouldSetGraffitiWhenFileNotExist(@TempDir final Path tempDir) {
+  void deleteGraffiti_shouldThrowIllegalArgumentWhenNoGraffitiToDelete(
+      @TempDir final Path tempDir) {
     dataDirLayout = new SimpleDataDirLayout(tempDir);
     manager = new GraffitiManager(dataDirLayout);
     assertThat(getGraffitiManagementDir().toFile().exists()).isTrue();
-    assertThat(manager.deleteGraffiti(publicKey)).isEmpty();
-    checkStoredGraffitiFile(publicKey, "");
+    assertThatThrownBy(() -> manager.deleteGraffiti(publicKey))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Saved graffiti does not exist for validator " + publicKey);
+    checkNoGraffitiFile(publicKey);
   }
 
   @Test
-  void deleteGraffiti_shouldSetGraffitiWhenFileExist(@TempDir final Path tempDir)
+  void deleteGraffiti_shouldDeleteGraffitiWhenFileExist(@TempDir final Path tempDir)
       throws IOException {
     dataDirLayout = new SimpleDataDirLayout(tempDir);
     manager = new GraffitiManager(dataDirLayout);
     assertThat(getGraffitiManagementDir().resolve(getFileName(publicKey)).toFile().createNewFile())
         .isTrue();
 
     assertThat(manager.deleteGraffiti(publicKey)).isEmpty();
-    checkStoredGraffitiFile(publicKey, "");
+    checkNoGraffitiFile(publicKey);
   }
 
   @Test
+  @DisabledOnOs(OS.WINDOWS) // Can't set permissions on Windows
   void deleteGraffiti_shouldReturnErrorMessageWhenUnableToWriteFile(@TempDir final Path tempDir)
       throws IOException {
     dataDirLayout = new SimpleDataDirLayout(tempDir);
     manager = new GraffitiManager(dataDirLayout);
 
     final File file = getGraffitiManagementDir().resolve(getFileName(publicKey)).toFile();
     assertThat(file.createNewFile()).isTrue();
-    assertThat(file.setWritable(false)).isTrue();
+    assertThat(file.getParentFile().setWritable(false)).isTrue();
 
     assertThat(manager.deleteGraffiti(publicKey))
-        .hasValue("Unable to update graffiti for validator " + publicKey);
+        .hasValue("Unable to delete graffiti for validator " + publicKey);
+    assertThat(file.exists()).isTrue();
   }
 
   @Test
@@ -149,7 +154,7 @@ void shouldSetAndDeleteGraffitiWhenManagementPreexisting(@TempDir final Path tem
     checkStoredGraffitiFile(publicKey, graffiti);
 
     assertThat(manager.deleteGraffiti(publicKey)).isEmpty();
-    checkStoredGraffitiFile(publicKey, "");
+    checkNoGraffitiFile(publicKey);
   }
 
   private void checkStoredGraffitiFile(final BLSPublicKey publicKey, final String graffiti) {
@@ -162,6 +167,11 @@ private void checkStoredGraffitiFile(final BLSPublicKey publicKey, final String
     }
   }
 
+  private void checkNoGraffitiFile(final BLSPublicKey publicKey) {
+    final Path filePath = getGraffitiManagementDir().resolve(getFileName(publicKey));
+    assertThat(filePath.toFile().exists()).isFalse();
+  }
+
   @Test
   void getGraffiti_shouldGetGraffitiFromStorage(@TempDir final Path tempDir) throws IOException {
     dataDirLayout = new SimpleDataDirLayout(tempDir);
@@ -207,13 +217,14 @@ void getGraffiti_shouldReturnEmptyWhenNotReadableFile(@TempDir final Path tempDi
   }
 
   @Test
-  void getGraffiti_shouldReturnEmptyWhenFileEmpty(@TempDir final Path tempDir) throws IOException {
+  void getGraffiti_shouldReturnEmptyBytesWhenFileEmpty(@TempDir final Path tempDir)
+      throws IOException {
     dataDirLayout = new SimpleDataDirLayout(tempDir);
     manager = new GraffitiManager(dataDirLayout);
     final Path filePath = getGraffitiManagementDir().resolve(getFileName(publicKey));
     assertThat(filePath.toFile().createNewFile()).isTrue();
 
-    assertThat(manager.getGraffiti(publicKey)).isEmpty();
+    assertThat(manager.getGraffiti(publicKey)).hasValue(Bytes32Parser.toBytes32(new byte[0]));
   }
 
   private Path getGraffitiManagementDir() {