From 7f079932adcca058b4b0b4f8b257d117d7ba6272 Mon Sep 17 00:00:00 2001 From: Chris Price Date: Wed, 10 Jul 2024 14:55:21 -0700 Subject: [PATCH] fix: storage SDK orElseThrow should take a supplier (#383) The `orElseThrow` signature of Java Optionals requires a single arg, `supplier`, that is a lambda function returning the exception that the user wishes to have thrown if the optional is empty. In our current storage client code we have an empty-args signature, which would be surprising to Java users familiar with Optionals. This commit updates the signature to accept a supplier. --- Makefile | 5 +- .../java/momento/sdk/storage/DataTests.java | 7 +- .../sdk/responses/storage/StorageValue.java | 17 ++-- .../momento/sdk/utils/MomentoOptional.java | 11 ++- .../responses/storage/GetResponseTest.java | 79 ++++++++++++++----- 5 files changed, 86 insertions(+), 33 deletions(-) diff --git a/Makefile b/Makefile index 0f8c0864..c6e30d49 100644 --- a/Makefile +++ b/Makefile @@ -1,3 +1,6 @@ +.PHONY: all +all: precommit + .PHONY: clean ## Clean the project clean: @@ -34,7 +37,7 @@ lint: .PHONY: precommit ## Run the precommit checks -precommit: format lint test +precommit: format lint build test .PHONY: help # See for explanation. diff --git a/momento-sdk/src/intTest/java/momento/sdk/storage/DataTests.java b/momento-sdk/src/intTest/java/momento/sdk/storage/DataTests.java index 289cb66f..bbe44f7f 100644 --- a/momento-sdk/src/intTest/java/momento/sdk/storage/DataTests.java +++ b/momento-sdk/src/intTest/java/momento/sdk/storage/DataTests.java @@ -80,7 +80,12 @@ void storeKeyNotFound() { assertThat(response).isInstanceOf(GetResponse.NotFound.class); assert response.valueWhenFound().isEmpty(); assert response instanceof GetResponse.NotFound; - assertThrows(ClientSdkException.class, response.valueWhenFound()::orElseThrow); + assertThrows(ClientSdkException.class, response.valueWhenFound()::get); + assertThrows( + RuntimeException.class, + () -> { + response.valueWhenFound().orElseThrow(() -> new RuntimeException("derp")); + }); } @Test diff --git a/momento-sdk/src/main/java/momento/sdk/responses/storage/StorageValue.java b/momento-sdk/src/main/java/momento/sdk/responses/storage/StorageValue.java index c5d71bbe..1516c8a0 100644 --- a/momento-sdk/src/main/java/momento/sdk/responses/storage/StorageValue.java +++ b/momento-sdk/src/main/java/momento/sdk/responses/storage/StorageValue.java @@ -1,5 +1,6 @@ package momento.sdk.responses.storage; +import java.util.function.Supplier; import momento.sdk.utils.MomentoOptional; /** @@ -55,8 +56,8 @@ public StorageItemType getType() { * Get the value as a byte array. * * @return the value as an optional byte array. If the value is not a byte array, an empty - * optional will be returned. Call {@link MomentoOptional#orElseThrow()} to short circuit the - * operation and throw an exception. + * optional will be returned. Call {@link MomentoOptional#orElseThrow(Supplier)} to short + * circuit the operation and throw an exception. */ public MomentoOptional getByteArray() { if (itemType != StorageItemType.BYTE_ARRAY) { @@ -69,8 +70,8 @@ public MomentoOptional getByteArray() { * Get the value as a string. * * @return the value as an optional string. If the value is not a string, an empty optional will - * be returned. Call {@link MomentoOptional#orElseThrow()} to short circuit the operation and - * throw an exception. + * be returned. Call {@link MomentoOptional#orElseThrow(Supplier)} to short circuit the + * operation and throw an exception. */ public MomentoOptional getString() { if (itemType != StorageItemType.STRING) { @@ -83,8 +84,8 @@ public MomentoOptional getString() { * Get the value as a long. * * @return the value as an optional long. If the value is not a long, an empty optional will be - * returned. Call {@link MomentoOptional#orElseThrow()} to short circuit the operation and - * throw an exception. + * returned. Call {@link MomentoOptional#orElseThrow(Supplier)} to short circuit the operation + * and throw an exception. */ public MomentoOptional getLong() { if (itemType != StorageItemType.LONG) { @@ -98,8 +99,8 @@ public MomentoOptional getLong() { * Get the value as a double. * * @return the value as an optional double. If the value is not a double, an empty optional will - * be returned. Call {@link MomentoOptional#orElseThrow()} to short circuit the operation and - * throw an exception. + * be returned. Call {@link MomentoOptional#orElseThrow(Supplier)} to short circuit the + * operation and throw an exception. */ public MomentoOptional getDouble() { if (itemType != StorageItemType.DOUBLE) { diff --git a/momento-sdk/src/main/java/momento/sdk/utils/MomentoOptional.java b/momento-sdk/src/main/java/momento/sdk/utils/MomentoOptional.java index 54dc9d25..2401b2e4 100644 --- a/momento-sdk/src/main/java/momento/sdk/utils/MomentoOptional.java +++ b/momento-sdk/src/main/java/momento/sdk/utils/MomentoOptional.java @@ -1,6 +1,7 @@ package momento.sdk.utils; import java.util.Optional; +import java.util.function.Supplier; import momento.sdk.exceptions.ClientSdkException; /** @@ -64,7 +65,7 @@ public static MomentoOptional empty(String onEmptyExceptionMessage) { * @return the value. */ public T get() { - return this.orElseThrow(); + return optional.orElseThrow(() -> new ClientSdkException(onEmptyExceptionMessage)); } /** @@ -72,8 +73,12 @@ public T get() { * * @return the value. */ - public T orElseThrow() { - return optional.orElseThrow(() -> new ClientSdkException(onEmptyExceptionMessage)); + public T orElseThrow(Supplier exceptionSupplier) throws X { + if (this.isEmpty()) { + throw exceptionSupplier.get(); + } else { + return this.get(); + } } /** diff --git a/momento-sdk/src/test/java/momento/sdk/responses/storage/GetResponseTest.java b/momento-sdk/src/test/java/momento/sdk/responses/storage/GetResponseTest.java index d14677bf..e73c4e10 100644 --- a/momento-sdk/src/test/java/momento/sdk/responses/storage/GetResponseTest.java +++ b/momento-sdk/src/test/java/momento/sdk/responses/storage/GetResponseTest.java @@ -12,29 +12,65 @@ public class GetResponseTest { @Test public void testGetResponseFoundWorksOnTheRightType() { - GetResponse.Found response = GetResponse.Found.of(new byte[] {0, 1, 2, 3}); + final GetResponse.Found response = GetResponse.Found.of(new byte[] {0, 1, 2, 3}); assert response.value().getByteArray().get().length == 4; - assertThrows(ClientSdkException.class, response.value().getString()::orElseThrow); - assertThrows(ClientSdkException.class, response.value().getLong()::orElseThrow); - assertThrows(ClientSdkException.class, response.value().getDouble()::orElseThrow); + assertThrows(ClientSdkException.class, response.value().getString()::get); + assertThrows( + RuntimeException.class, + () -> response.value().getString().orElseThrow(() -> new RuntimeException("derp"))); + assertThrows(ClientSdkException.class, response.value().getLong()::get); + assertThrows( + RuntimeException.class, + () -> response.value().getLong().orElseThrow(() -> new RuntimeException("derp"))); + assertThrows(ClientSdkException.class, response.value().getDouble()::get); + assertThrows( + RuntimeException.class, + () -> response.value().getDouble().orElseThrow(() -> new RuntimeException("derp"))); - response = GetResponse.Found.of("string"); - assertThrows(ClientSdkException.class, response.value().getByteArray()::orElseThrow); - assert response.value().getString().get().equals("string"); - assertThrows(ClientSdkException.class, response.value().getLong()::orElseThrow); - assertThrows(ClientSdkException.class, response.value().getDouble()::orElseThrow); + final GetResponse.Found response2 = GetResponse.Found.of("string"); + assertThrows(ClientSdkException.class, response2.value().getByteArray()::get); + assertThrows( + RuntimeException.class, + () -> response2.value().getByteArray().orElseThrow(() -> new RuntimeException("derp"))); + assert response2.value().getString().get().equals("string"); + assertThrows(ClientSdkException.class, response2.value().getLong()::get); + assertThrows( + RuntimeException.class, + () -> response2.value().getLong().orElseThrow(() -> new RuntimeException("derp"))); + assertThrows(ClientSdkException.class, response2.value().getDouble()::get); + assertThrows( + RuntimeException.class, + () -> response2.value().getDouble().orElseThrow(() -> new RuntimeException("derp"))); - response = GetResponse.Found.of(42L); - assertThrows(ClientSdkException.class, response.value().getByteArray()::orElseThrow); - assertThrows(ClientSdkException.class, response.value().getString()::orElseThrow); - assert response.value().getLong().get() == 42L; - assertThrows(ClientSdkException.class, response.value().getDouble()::orElseThrow); + final GetResponse.Found response3 = GetResponse.Found.of(42L); + assertThrows(ClientSdkException.class, response3.value().getByteArray()::get); + assertThrows( + RuntimeException.class, + () -> response3.value().getByteArray().orElseThrow(() -> new RuntimeException("derp"))); + assertThrows(ClientSdkException.class, response3.value().getString()::get); + assertThrows( + RuntimeException.class, + () -> response3.value().getString().orElseThrow(() -> new RuntimeException("derp"))); + assert response3.value().getLong().get() == 42L; + assertThrows(ClientSdkException.class, response3.value().getDouble()::get); + assertThrows( + RuntimeException.class, + () -> response3.value().getDouble().orElseThrow(() -> new RuntimeException("derp"))); - response = GetResponse.Found.of(3.14); - assertThrows(ClientSdkException.class, response.value().getByteArray()::orElseThrow); - assertThrows(ClientSdkException.class, response.value().getString()::orElseThrow); - assertThrows(ClientSdkException.class, response.value().getLong()::orElseThrow); - assert response.value().getDouble().get() == 3.14; + final GetResponse.Found response4 = GetResponse.Found.of(3.14); + assertThrows(ClientSdkException.class, response4.value().getByteArray()::get); + assertThrows( + RuntimeException.class, + () -> response4.value().getByteArray().orElseThrow(() -> new RuntimeException("derp"))); + assertThrows(ClientSdkException.class, response4.value().getString()::get); + assertThrows( + RuntimeException.class, + () -> response4.value().getString().orElseThrow(() -> new RuntimeException("derp"))); + assertThrows(ClientSdkException.class, response4.value().getLong()::get); + assertThrows( + RuntimeException.class, + () -> response4.value().getLong().orElseThrow(() -> new RuntimeException("derp"))); + assert response4.value().getDouble().get() == 3.14; } @Test @@ -62,6 +98,9 @@ public void testConvenienceMethodsOnGetResponse() { new MomentoTransportErrorDetails( new MomentoGrpcErrorDetails(Status.Code.NOT_FOUND, "not found")))); assert error.valueWhenFound().isEmpty(); - assertThrows(ClientSdkException.class, error.valueWhenFound()::orElseThrow); + assertThrows(ClientSdkException.class, error.valueWhenFound()::get); + assertThrows( + RuntimeException.class, + () -> error.valueWhenFound().orElseThrow(() -> new RuntimeException("derp"))); } }