Fix deleteGraffiti method

courtneyeh · Apr 21, 2024 · cad8580 · cad8580
1 parent f31b398
commit cad8580
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 35 deletions.
diff --git a/validator/api/src/main/java/tech/pegasys/teku/validator/api/GraffitiManager.java b/validator/api/src/main/java/tech/pegasys/teku/validator/api/GraffitiManager.java
@@ -16,7 +16,6 @@
 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;
@@ -55,21 +54,13 @@ public void setGraffiti(final BLSPublicKey publicKey, final String graffiti) thr
     Files.writeString(file, strippedGraffiti);
   }
 
-  public Optional<String> deleteGraffiti(final BLSPublicKey publicKey) {
+  public void deleteGraffiti(final BLSPublicKey publicKey) throws IOException {
     final Path file = directory.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);
+    if (!file.toFile().exists()) {
+      return;
     }
+
+    Files.delete(file);
   }
 
   public Optional<Bytes32> getGraffiti(final BLSPublicKey publicKey) {

diff --git a/validator/api/src/main/java/tech/pegasys/teku/validator/api/noop/NoOpGraffitiManager.java b/validator/api/src/main/java/tech/pegasys/teku/validator/api/noop/NoOpGraffitiManager.java
@@ -28,9 +28,7 @@ public NoOpGraffitiManager() {
   public void setGraffiti(final BLSPublicKey publicKey, final String graffiti) {}
 
   @Override
-  public Optional<String> deleteGraffiti(final BLSPublicKey publicKey) {
-    return Optional.empty();
-  }
+  public void deleteGraffiti(final BLSPublicKey publicKey) {}
 
   @Override
   public Optional<Bytes32> getGraffiti(final BLSPublicKey publicKey) {

diff --git a/validator/api/src/test/java/tech/pegasys/teku/validator/api/GraffitiManagerTest.java b/validator/api/src/test/java/tech/pegasys/teku/validator/api/GraffitiManagerTest.java
@@ -16,6 +16,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.fail;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static tech.pegasys.teku.validator.api.GraffitiManager.GRAFFITI_DIR;
 
 import java.io.File;
@@ -105,14 +106,12 @@ void setGraffiti_shouldThrowExceptionWhenGraffitiTooBig(@TempDir final Path temp
   }
 
   @Test
-  void deleteGraffiti_shouldThrowIllegalArgumentWhenNoGraffitiToDelete(
-      @TempDir final Path tempDir) {
+  void deleteGraffiti_shouldSucceedWhenNoGraffitiToDelete(@TempDir final Path tempDir) {
     dataDirLayout = new SimpleDataDirLayout(tempDir);
     manager = new GraffitiManager(dataDirLayout);
     assertThat(getGraffitiManagementDir().toFile().exists()).isTrue();
-    assertThatThrownBy(() -> manager.deleteGraffiti(publicKey))
-        .isInstanceOf(IllegalArgumentException.class)
-        .hasMessage("Saved graffiti does not exist for validator " + publicKey);
+
+    assertDoesNotThrow(() -> manager.deleteGraffiti(publicKey));
     checkNoGraffitiFile(publicKey);
   }
 
@@ -124,7 +123,7 @@ void deleteGraffiti_shouldDeleteGraffitiWhenFileExist(@TempDir final Path tempDi
     assertThat(getGraffitiManagementDir().resolve(getFileName(publicKey)).toFile().createNewFile())
         .isTrue();
 
-    assertThat(manager.deleteGraffiti(publicKey)).isEmpty();
+    manager.deleteGraffiti(publicKey);
     checkNoGraffitiFile(publicKey);
   }
 
@@ -139,8 +138,7 @@ void deleteGraffiti_shouldReturnErrorMessageWhenUnableToDeleteFile(@TempDir fina
     assertThat(file.createNewFile()).isTrue();
     assertThat(file.getParentFile().setWritable(false)).isTrue();
 
-    assertThat(manager.deleteGraffiti(publicKey))
-        .hasValue("Unable to delete graffiti for validator " + publicKey);
+    assertThatThrownBy(() -> manager.deleteGraffiti(publicKey)).isInstanceOf(IOException.class);
     assertThat(file.exists()).isTrue();
   }
 
@@ -155,7 +153,7 @@ void shouldSetAndDeleteGraffitiWhenManagementPreexisting(@TempDir final Path tem
     manager.setGraffiti(publicKey, graffiti);
     checkStoredGraffitiFile(publicKey);
 
-    assertThat(manager.deleteGraffiti(publicKey)).isEmpty();
+    manager.deleteGraffiti(publicKey);
     checkNoGraffitiFile(publicKey);
   }
 

diff --git a/.../client/src/main/java/tech/pegasys/teku/validator/client/restapi/apis/DeleteGraffiti.java b/.../client/src/main/java/tech/pegasys/teku/validator/client/restapi/apis/DeleteGraffiti.java
@@ -20,6 +20,7 @@
 import static tech.pegasys.teku.validator.client.restapi.ValidatorTypes.PARAM_PUBKEY_TYPE;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
+import java.io.IOException;
 import java.util.Optional;
 import tech.pegasys.teku.bls.BLSPublicKey;
 import tech.pegasys.teku.infrastructure.restapi.endpoints.EndpointMetadata;
@@ -62,11 +63,12 @@ public void handleRequest(final RestApiRequest request) throws JsonProcessingExc
       return;
     }
 
-    final Optional<String> error = graffitiManager.deleteGraffiti(publicKey);
-    if (error.isPresent()) {
-      request.respondError(SC_INTERNAL_SERVER_ERROR, error.get());
-    } else {
+    try {
+      graffitiManager.deleteGraffiti(publicKey);
       request.respondWithCode(SC_NO_CONTENT);
+    } catch (IOException e) {
+      request.respondError(
+          SC_INTERNAL_SERVER_ERROR, "Unable to delete graffiti for validator " + publicKey);
     }
   }
 }
diff --git a/...ent/src/test/java/tech/pegasys/teku/validator/client/restapi/apis/DeleteGraffitiTest.java b/...ent/src/test/java/tech/pegasys/teku/validator/client/restapi/apis/DeleteGraffitiTest.java
@@ -15,6 +15,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 import static tech.pegasys.teku.infrastructure.http.HttpStatusCodes.SC_BAD_REQUEST;
@@ -55,10 +56,10 @@ class DeleteGraffitiTest {
           .build();
 
   @Test
-  void shouldSuccessfullyDeleteGraffiti() throws JsonProcessingException {
+  void shouldSuccessfullyDeleteGraffiti() throws IOException {
     final Validator validator = new Validator(publicKey, NO_OP_SIGNER, Optional::empty);
     when(keyManager.getValidatorByPublicKey(any())).thenReturn(Optional.of(validator));
-    when(graffitiManager.deleteGraffiti(any())).thenReturn(Optional.empty());
+    graffitiManager.deleteGraffiti(any());
 
     handler.handleRequest(request);
 
@@ -67,16 +68,18 @@ void shouldSuccessfullyDeleteGraffiti() throws JsonProcessingException {
   }
 
   @Test
-  void shouldReturnErrorWhenIssueDeleting() throws JsonProcessingException {
+  void shouldReturnErrorWhenIssueDeleting() throws IOException {
     final Validator validator = new Validator(publicKey, NO_OP_SIGNER, Optional::empty);
     when(keyManager.getValidatorByPublicKey(any())).thenReturn(Optional.of(validator));
-    when(graffitiManager.deleteGraffiti(any())).thenReturn(Optional.of("Error deleting graffiti"));
+    doThrow(IOException.class).when(graffitiManager).deleteGraffiti(any());
 
     handler.handleRequest(request);
 
     assertThat(request.getResponseCode()).isEqualTo(SC_INTERNAL_SERVER_ERROR);
     assertThat(request.getResponseBody())
-        .isEqualTo(new HttpErrorResponse(SC_INTERNAL_SERVER_ERROR, "Error deleting graffiti"));
+        .isEqualTo(
+            new HttpErrorResponse(
+                SC_INTERNAL_SERVER_ERROR, "Unable to delete graffiti for validator " + publicKey));
   }
 
   @Test