From f31b39840b32ca7593371262aedca0d7858e05ca Mon Sep 17 00:00:00 2001 From: courtneyeh Date: Mon, 22 Apr 2024 09:44:56 +1000 Subject: [PATCH] Fix setGraffiti method --- .../teku/validator/api/GraffitiManager.java | 14 +++---------- .../api/noop/NoOpGraffitiManager.java | 4 +--- .../validator/api/GraffitiManagerTest.java | 16 +++++++------- .../client/restapi/apis/SetGraffiti.java | 10 +++++---- .../client/restapi/apis/SetGraffitiTest.java | 21 +++++++++++-------- 5 files changed, 31 insertions(+), 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 index 2c2827e8e71..55a9fad4270 100644 --- 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 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 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 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 index 2d81aa5e11e..7be38f2b5cb 100644 --- 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 setGraffiti(final BLSPublicKey publicKey, final String graffiti) { - return Optional.empty(); - } + public void setGraffiti(final BLSPublicKey publicKey, final String graffiti) {} @Override public Optional 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 index 9106dc9ff25..1698ae5004f 100644 --- 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/validator/client/src/main/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffiti.java b/validator/client/src/main/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffiti.java index f22cc69dd20..9973831b574 100644 --- a/validator/client/src/main/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffiti.java +++ b/validator/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 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/validator/client/src/test/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffitiTest.java b/validator/client/src/test/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffitiTest.java index 5082e9a5abe..7a848d95da3 100644 --- a/validator/client/src/test/java/tech/pegasys/teku/validator/client/restapi/apis/SetGraffitiTest.java +++ b/validator/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)