Fix setGraffiti method

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

Hide file tree

Showing 5 changed files with 31 additions and 34 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
@@ -41,7 +41,7 @@ public GraffitiManager(final Path directory) {
     }
   }
 
-  public Optional<String> setGraffiti(final BLSPublicKey publicKey, final String graffiti) {
+  public void setGraffiti(final BLSPublicKey publicKey, final String graffiti) throws IOException {
     final String strippedGraffiti = graffiti.strip();
     final int graffitiSize = strippedGraffiti.getBytes(StandardCharsets.UTF_8).length;
     if (graffitiSize > 32) {
@@ -51,16 +51,8 @@ public Optional<String> setGraffiti(final BLSPublicKey publicKey, final String g
               strippedGraffiti, graffitiSize));
     }
 
-    try {
-      final Path file = directory.resolve(resolveFileName(publicKey));
-      Files.writeString(file, strippedGraffiti);
-    } catch (IOException e) {
-      final String errorMessage =
-          String.format("Unable to update graffiti for validator %s", publicKey);
-      LOG.error(errorMessage, e);
-      return Optional.of(errorMessage);
-    }
-    return Optional.empty();
+    final Path file = directory.resolve(resolveFileName(publicKey));
+    Files.writeString(file, strippedGraffiti);
   }
 
   public Optional<String> deleteGraffiti(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
@@ -25,9 +25,7 @@ public NoOpGraffitiManager() {
   }
 
   @Override
-  public Optional<String> setGraffiti(final BLSPublicKey publicKey, final String graffiti) {
-    return Optional.empty();
-  }
+  public void setGraffiti(final BLSPublicKey publicKey, final String graffiti) {}
 
   @Override
   public Optional<String> deleteGraffiti(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
@@ -54,12 +54,13 @@ void shouldThrowExceptionWhenUnableToCreateManagementDirectory(@TempDir final Pa
   }
 
   @Test
-  void setGraffiti_shouldSetGraffitiWhenFileNotExist(@TempDir final Path tempDir) {
+  void setGraffiti_shouldSetGraffitiWhenFileNotExist(@TempDir final Path tempDir)
+      throws IOException {
     dataDirLayout = new SimpleDataDirLayout(tempDir);
     manager = new GraffitiManager(dataDirLayout);
     assertThat(getGraffitiManagementDir().toFile().exists()).isTrue();
 
-    assertThat(manager.setGraffiti(publicKey, graffiti)).isEmpty();
+    manager.setGraffiti(publicKey, graffiti);
     checkStoredGraffitiFile(publicKey);
   }
 
@@ -70,7 +71,7 @@ void setGraffiti_shouldSetGraffitiWhenFileExist(@TempDir final Path tempDir) thr
     assertThat(getGraffitiManagementDir().resolve(getFileName(publicKey)).toFile().createNewFile())
         .isTrue();
 
-    assertThat(manager.setGraffiti(publicKey, graffiti)).isEmpty();
+    manager.setGraffiti(publicKey, graffiti);
     checkStoredGraffitiFile(publicKey);
   }
 
@@ -86,8 +87,8 @@ void setGraffiti_shouldReturnErrorMessageWhenUnableToWriteFile(@TempDir final Pa
     assertThat(file.createNewFile()).isTrue();
     assertThat(file.setWritable(false)).isTrue();
 
-    assertThat(manager.setGraffiti(publicKey, graffiti))
-        .hasValue("Unable to update graffiti for validator " + publicKey);
+    assertThatThrownBy(() -> manager.setGraffiti(publicKey, graffiti))
+        .isInstanceOf(IOException.class);
   }
 
   @Test
@@ -144,13 +145,14 @@ void deleteGraffiti_shouldReturnErrorMessageWhenUnableToDeleteFile(@TempDir fina
   }
 
   @Test
-  void shouldSetAndDeleteGraffitiWhenManagementPreexisting(@TempDir final Path tempDir) {
+  void shouldSetAndDeleteGraffitiWhenManagementPreexisting(@TempDir final Path tempDir)
+      throws IOException {
     dataDirLayout = new SimpleDataDirLayout(tempDir);
     final Path managementDir = getGraffitiManagementDir();
     assertThat(managementDir.toFile().mkdirs()).isTrue();
     manager = new GraffitiManager(dataDirLayout);
 
-    assertThat(manager.setGraffiti(publicKey, graffiti)).isEmpty();
+    manager.setGraffiti(publicKey, graffiti);
     checkStoredGraffitiFile(publicKey);
 
     assertThat(manager.deleteGraffiti(publicKey)).isEmpty();

diff --git a/...tor/client/src/main/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffiti.java b/...tor/client/src/main/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffiti.java
@@ -21,6 +21,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;
@@ -63,11 +64,12 @@ public void handleRequest(final RestApiRequest request) throws JsonProcessingExc
       return;
     }
 
-    final Optional<String> error = graffitiManager.setGraffiti(publicKey, graffiti);
-    if (error.isPresent()) {
-      request.respondError(SC_INTERNAL_SERVER_ERROR, error.get());
-    } else {
+    try {
+      graffitiManager.setGraffiti(publicKey, graffiti);
       request.respondWithCode(SC_NO_CONTENT);
+    } catch (IOException e) {
+      request.respondError(
+          SC_INTERNAL_SERVER_ERROR, "Unable to update graffiti for validator " + publicKey);
     }
   }
 }
diff --git a/...client/src/test/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffitiTest.java b/...client/src/test/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffitiTest.java
@@ -17,6 +17,7 @@
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+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;
@@ -30,6 +31,7 @@
 import static tech.pegasys.teku.spec.generator.signatures.NoOpLocalSigner.NO_OP_SIGNER;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
+import java.io.IOException;
 import java.util.Optional;
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.bls.BLSPublicKey;
@@ -57,12 +59,11 @@ class SetGraffitiTest {
           .build();
 
   @Test
-  void shouldSuccessfullySetGraffiti() throws JsonProcessingException {
+  void shouldSuccessfullySetGraffiti() throws IOException {
     request.setRequestBody(graffiti);
 
     final Validator validator = new Validator(publicKey, NO_OP_SIGNER, Optional::empty);
     when(keyManager.getValidatorByPublicKey(any())).thenReturn(Optional.of(validator));
-    when(graffitiManager.setGraffiti(any(), any())).thenReturn(Optional.empty());
 
     handler.handleRequest(request);
 
@@ -71,23 +72,24 @@ void shouldSuccessfullySetGraffiti() throws JsonProcessingException {
   }
 
   @Test
-  void shouldReturnErrorWhenIssueSetting() throws JsonProcessingException {
+  void shouldReturnErrorWhenIssueSetting() throws IOException {
     request.setRequestBody(graffiti);
 
     final Validator validator = new Validator(publicKey, NO_OP_SIGNER, Optional::empty);
     when(keyManager.getValidatorByPublicKey(any())).thenReturn(Optional.of(validator));
-    when(graffitiManager.setGraffiti(any(), eq(graffiti)))
-        .thenReturn(Optional.of("Error deleting graffiti"));
+    doThrow(IOException.class).when(graffitiManager).setGraffiti(any(), eq(graffiti));
 
     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 update graffiti for validator " + publicKey));
   }
 
   @Test
-  void shouldThrowExceptionWhenInvalidGraffitiInput() {
+  void shouldThrowExceptionWhenInvalidGraffitiInput() throws IOException {
     final String invalidGraffiti = "This graffiti is a bit too long!!";
     final String errorMessage =
         String.format(
@@ -96,8 +98,9 @@ void shouldThrowExceptionWhenInvalidGraffitiInput() {
 
     final Validator validator = new Validator(publicKey, NO_OP_SIGNER, Optional::empty);
     when(keyManager.getValidatorByPublicKey(any())).thenReturn(Optional.of(validator));
-    when(graffitiManager.setGraffiti(any(), eq(invalidGraffiti)))
-        .thenThrow(new IllegalArgumentException(errorMessage));
+    doThrow(new IllegalArgumentException(errorMessage))
+        .when(graffitiManager)
+        .setGraffiti(any(), eq(invalidGraffiti));
 
     assertThatThrownBy(() -> handler.handleRequest(request))
         .isInstanceOf(IllegalArgumentException.class)