Skip to content

Commit

Permalink
refactor: promote not found to an inner class alongside Found
Browse files Browse the repository at this point in the history
Changes `Success` to `Found` and promotes the "item not found" case to
an inner class alongside `Found`. This way, `Found::value` is no
longer an Optional. This removes part a layer of the decision tree
when unpacking a found item.
  • Loading branch information
malandis committed Jun 28, 2024
1 parent e6f41a4 commit 1c9f001
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 86 deletions.
33 changes: 17 additions & 16 deletions momento-sdk/src/intTest/java/momento/sdk/storage/DataTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ void getReturnsValueAsStringAfterPut() {

// Successful Get
final GetResponse getResponse = client.get(storeName, key).join();
assertThat(getResponse).isInstanceOf(GetResponse.Success.class);
assertThat(getResponse.success().get().value().get().getString().get()).isEqualTo(value);
assertThat(getResponse).isInstanceOf(GetResponse.Found.class);
assertThat(getResponse.found().get().value().getString().get()).isEqualTo(value);
}

@Test
Expand All @@ -63,9 +63,8 @@ void getReturnsValueAsByteArrayAfterPut() {

// Successful Get
final GetResponse getResponse = client.get(storeName, key).join();
assertThat(getResponse).isInstanceOf(GetResponse.Success.class);
assertThat(getResponse.success().get().value().get().getByteArray().get())
.isEqualTo(value.getBytes());
assertThat(getResponse).isInstanceOf(GetResponse.Found.class);
assertThat(getResponse.found().get().value().getByteArray().get()).isEqualTo(value.getBytes());
}

@Test
Expand All @@ -79,8 +78,8 @@ void getReturnsValueAsLongAfterPut() {

// Successful Get
final GetResponse getResponse = client.get(storeName, key).join();
assertThat(getResponse).isInstanceOf(GetResponse.Success.class);
assertThat(getResponse.success().get().value().get().getLong().get()).isEqualTo(value);
assertThat(getResponse).isInstanceOf(GetResponse.Found.class);
assertThat(getResponse.found().get().value().getLong().get()).isEqualTo(value);
}

@Test
Expand All @@ -94,16 +93,17 @@ void getReturnsValueAsDoubleAfterPut() {

// Successful Get
final GetResponse getResponse = client.get(storeName, key).join();
assertThat(getResponse).isInstanceOf(GetResponse.Success.class);
assertThat(getResponse.success().get().value().get().getDouble().get()).isEqualTo(value);
assertThat(getResponse).isInstanceOf(GetResponse.Found.class);
assertThat(getResponse.found().get().value().getDouble().get()).isEqualTo(value);
}

@Test
void storeKeyNotFound() {
// Get key that was not set
final GetResponse response = client.get(storeName, randomString("key")).join();
assertThat(response).isInstanceOf(GetResponse.Success.class);
assert !response.success().get().value().isPresent();
assertThat(response).isInstanceOf(GetResponse.NotFound.class);
assert !response.found().isPresent();
assert response instanceof GetResponse.NotFound;
}

@Test
Expand All @@ -125,8 +125,8 @@ public void allowEmptyKeyValuesOnGet() throws Exception {
final String emptyValue = "";
client.put(storeName, emptyKey, emptyValue).get();
final GetResponse response = client.get(storeName, emptyKey).get();
assertThat(response).isInstanceOf(GetResponse.Success.class);
assert response.success().get().value().get().getString().get().isEmpty();
assertThat(response).isInstanceOf(GetResponse.Found.class);
assert response.found().get().value().getString().get().isEmpty();
}

@Test
Expand All @@ -136,14 +136,15 @@ public void deleteHappyPath() throws Exception {

client.put(storeName, key, value).get();
final GetResponse getResponse = client.get(storeName, key).get();
assertThat(getResponse).isInstanceOf(GetResponse.Success.class);
assertThat(getResponse.success().get().value().get().getString().get()).isEqualTo(value);
assertThat(getResponse).isInstanceOf(GetResponse.Found.class);
assertThat(getResponse.found().get().value().getString().get()).isEqualTo(value);

final DeleteResponse deleteResponse = client.delete(storeName, key).get();
assertThat(deleteResponse).isInstanceOf(DeleteResponse.Success.class);

final GetResponse getAfterDeleteResponse = client.get(storeName, key).get();
assert !getAfterDeleteResponse.success().get().value().isPresent();
assert !getAfterDeleteResponse.found().isPresent();
assert getAfterDeleteResponse instanceof GetResponse.NotFound;
}

@Test
Expand Down
10 changes: 5 additions & 5 deletions momento-sdk/src/main/java/momento/sdk/StorageDataClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -136,16 +136,16 @@ public void onSuccess(_StoreGetResponse rsp) {
_StoreValue value = rsp.getValue();
switch (value.getValueCase()) {
case BYTES_VALUE:
returnFuture.complete(GetResponse.Success.of(value.getBytesValue().toByteArray()));
returnFuture.complete(GetResponse.Found.of(value.getBytesValue().toByteArray()));
break;
case STRING_VALUE:
returnFuture.complete(GetResponse.Success.of(value.getStringValue()));
returnFuture.complete(GetResponse.Found.of(value.getStringValue()));
break;
case INTEGER_VALUE:
returnFuture.complete(GetResponse.Success.of(value.getIntegerValue()));
returnFuture.complete(GetResponse.Found.of(value.getIntegerValue()));
break;
case DOUBLE_VALUE:
returnFuture.complete(GetResponse.Success.of(value.getDoubleValue()));
returnFuture.complete(GetResponse.Found.of(value.getDoubleValue()));
break;
case VALUE_NOT_SET:
returnFuture.complete(
Expand All @@ -159,7 +159,7 @@ public void onSuccess(_StoreGetResponse rsp) {
public void onFailure(@Nonnull Throwable e) {
final SdkException sdkException = CacheServiceExceptionMapper.convert(e, metadata);
if (sdkException instanceof momento.sdk.exceptions.StoreItemNotFoundException) {
returnFuture.complete(GetResponse.Success.of());
returnFuture.complete(new GetResponse.NotFound());
} else {
returnFuture.complete(new GetResponse.Error(CacheServiceExceptionMapper.convert(e)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@

import java.util.Optional;
import momento.sdk.exceptions.SdkException;
import momento.sdk.internal.StringHelpers;

/**
* Response for a get operation.
*
* <p>The response can be either a {@link Success} or an {@link Error}.
* <p>The response can be either a {@link Found}, {@link NotFound}, or an {@link Error}.
*
* <p>To shortcut access to the success response, use {@link #success()}. If the operation was
* successful, the response will be an optional of {@link Success}, otherwise it will be an empty
* optional.
* <p>To shortcut access to found response, use {@link #found()}. If the operation was successful
* but the key was not found, the response will be an empty optional. If the operation failed, the
* response will be an empty optional.
*
* <p>To handle the response otherwise, use pattern matching or an instanceof to check if the
* response is a {@link Success} or an {@link Error}.
* response is a {@link Found}, {@link NotFound}, or an {@link Error}.
*
* <p>Upon a success, the value can be retrieved with {@link Success#value()}. If the value was
* found in the store, it will be present, otherwise it will be empty.
* <p>Upon a found response, the value can be retrieved with {@link Found#value()}.
*/
public interface GetResponse {
/**
Expand All @@ -27,7 +27,7 @@ public interface GetResponse {
*
* @return The success response, or an empty optional if the operation failed.
*/
Optional<Success> success();
Optional<Found> found();

/**
* A successful get operation.
Expand All @@ -38,50 +38,60 @@ public interface GetResponse {
* <p>Use the appropriate type-based accessor on the value to retrieve the value in its
* corresponding type.
*/
class Success implements GetResponse {
private final Optional<StorageValue> value;
class Found implements GetResponse {
private final StorageValue value;

private Success(Optional<StorageValue> value) {
private Found(StorageValue value) {
this.value = value;
}

public static Success of() {
return new Success(Optional.empty());
public static Found of(byte[] value) {
return new Found(StorageValue.of(value));
}

public static Success of(byte[] value) {
return new Success(Optional.of(StorageValue.of(value)));
public static Found of(String value) {
return new Found(StorageValue.of(value));
}

public static Success of(String value) {
return new Success(Optional.of(StorageValue.of(value)));
public static Found of(long value) {
return new Found(StorageValue.of(value));
}

public static Success of(long value) {
return new Success(Optional.of(StorageValue.of(value)));
}

public static Success of(double value) {
return new Success(Optional.of(StorageValue.of(value)));
public static Found of(double value) {
return new Found(StorageValue.of(value));
}

/**
* Returns the value if it exists, or an empty optional if the value does not exist.
*
* @return The value, or an empty optional if the value does not exist.
*/
public Optional<StorageValue> value() {
public StorageValue value() {
return value;
}

@Override
public Optional<Success> success() {
public Optional<Found> found() {
return Optional.of(this);
}

@Override
public String toString() {
return "GetResponse.Success{value=" + value + "}";
return "GetResponse.Found{value=" + value + "}";
}
}

class NotFound implements GetResponse {
public NotFound() {}

@Override
public Optional<Found> found() {
return Optional.empty();
}

@Override
public String toString() {
return StringHelpers.emptyToString("GetResponse.NotFound");
}
}

Expand All @@ -102,7 +112,7 @@ public Error(SdkException cause) {
}

@Override
public Optional<Success> success() {
public Optional<Found> found() {
return Optional.empty();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,57 +8,56 @@

public class GetResponseTest {
@Test
public void testGetResponseSuccessWorksOnTheRightType() {
GetResponse.Success response = GetResponse.Success.of(new byte[] {0, 1, 2, 3});
assert response.value().get().getByteArray().get().length == 4;
assert !response.value().get().getString().isPresent();
assert !response.value().get().getLong().isPresent();
assert !response.value().get().getDouble().isPresent();

response = GetResponse.Success.of("string");
assert !response.value().get().getByteArray().isPresent();
assert response.value().get().getString().get().equals("string");
assert !response.value().get().getLong().isPresent();
assert !response.value().get().getDouble().isPresent();

response = GetResponse.Success.of(42L);
assert !response.value().get().getByteArray().isPresent();
assert !response.value().get().getString().isPresent();
assert response.value().get().getLong().get() == 42L;
assert !response.value().get().getDouble().isPresent();

response = GetResponse.Success.of(3.14);
assert !response.value().get().getByteArray().isPresent();
assert !response.value().get().getString().isPresent();
assert !response.value().get().getLong().isPresent();
assert response.value().get().getDouble().get() == 3.14;
public void testGetResponseFoundWorksOnTheRightType() {
GetResponse.Found response = GetResponse.Found.of(new byte[] {0, 1, 2, 3});
assert response.value().getByteArray().get().length == 4;
assert !response.value().getString().isPresent();
assert !response.value().getLong().isPresent();
assert !response.value().getDouble().isPresent();

response = GetResponse.Found.of("string");
assert !response.value().getByteArray().isPresent();
assert response.value().getString().get().equals("string");
assert !response.value().getLong().isPresent();
assert !response.value().getDouble().isPresent();

response = GetResponse.Found.of(42L);
assert !response.value().getByteArray().isPresent();
assert !response.value().getString().isPresent();
assert response.value().getLong().get() == 42L;
assert !response.value().getDouble().isPresent();

response = GetResponse.Found.of(3.14);
assert !response.value().getByteArray().isPresent();
assert !response.value().getString().isPresent();
assert !response.value().getLong().isPresent();
assert response.value().getDouble().get() == 3.14;
}

@Test
public void testConvenienceMethodsOnGetResponse() {
GetResponse.Success response = GetResponse.Success.of(new byte[] {0, 1, 2, 3});
assert response.success().isPresent();
assert response.success().get().value().get().getByteArray().get().length == 4;
GetResponse.Found response = GetResponse.Found.of(new byte[] {0, 1, 2, 3});
assert response.found().isPresent();
assert response.found().get().value().getByteArray().get().length == 4;

response = GetResponse.Success.of("string");
assert response.success().isPresent();
assert response.success().get().value().get().getString().get() == "string";
response = GetResponse.Found.of("string");
assert response.found().isPresent();
assert response.found().get().value().getString().get() == "string";

response = GetResponse.Success.of(42L);
assert response.success().isPresent();
assert response.success().get().value().get().getLong().get() == 42L;
response = GetResponse.Found.of(42L);
assert response.found().isPresent();
assert response.found().get().value().getLong().get() == 42L;

response = GetResponse.Success.of(3.14);
assert response.success().isPresent();
assert response.success().get().value().get().getDouble().get() == 3.14;
response = GetResponse.Found.of(3.14);
assert response.found().isPresent();
assert response.found().get().value().getDouble().get() == 3.14;

// TODO distinguish store not found from key not found
GetResponse.Error error =
new GetResponse.Error(
new StoreNotFoundException(
new Exception(),
new MomentoTransportErrorDetails(
new MomentoGrpcErrorDetails(Status.Code.NOT_FOUND, "not found"))));
assert !error.success().isPresent();
assert !error.found().isPresent();
}
}

0 comments on commit 1c9f001

Please sign in to comment.