Throw NoSuchKeyException when key doesn't exist in get

Motivation: Such checks are best performed in backend than
having client checking empty or null value. This is also in-line
with more critical requirement for functioning of NotFound
introduced by MonitorUpdatingPersister Design.
Even though there are some popular KVStore's which do not throw
such exception on keyNotFound such as AWS-DDB (arguably the most popular one),
we can still figure out that key didn't exist if value was
returned as null or by similar means in other stores.

Author: G8XSU

app/src/main/java/org/vss/api/AbstractVssApi.java

@@ -8,6 +8,7 @@
 import org.vss.ErrorResponse;
 import org.vss.KVStore;
 import org.vss.exception.ConflictException;
+import org.vss.exception.NoSuchKeyException;
 
 public abstract class AbstractVssApi {
   final KVStore kvStore;
@@ -32,6 +33,8 @@ Response toErrorResponse(Exception e) {
     } else if (e instanceof IllegalArgumentException
         || e instanceof InvalidProtocolBufferException) {
       errorCode = ErrorCode.INVALID_REQUEST_EXCEPTION;
+    } else if (e instanceof NoSuchKeyException) {
+      errorCode = ErrorCode.NO_SUCH_KEY_EXCEPTION;
     } else {
       errorCode = ErrorCode.INTERNAL_SERVER_EXCEPTION;
     }

app/src/main/java/org/vss/exception/NoSuchKeyException.java

@@ -0,0 +1,7 @@
+package org.vss.exception;
+
+public class NoSuchKeyException extends RuntimeException {
+  public NoSuchKeyException(String message) {
+    super(message);
+  }
+}

app/src/main/java/org/vss/impl/postgres/PostgresBackendImpl.java

@@ -23,6 +23,7 @@
 import org.vss.PutObjectRequest;
 import org.vss.PutObjectResponse;
 import org.vss.exception.ConflictException;
+import org.vss.exception.NoSuchKeyException;
 import org.vss.postgres.tables.records.VssDbRecord;
 
 import static org.vss.postgres.tables.VssDb.VSS_DB;
@@ -31,6 +32,11 @@
 public class PostgresBackendImpl implements KVStore {
 
   private static final int LIST_KEY_VERSIONS_MAX_PAGE_SIZE = 100;
+  private static final KeyValue DEFAULT_GLOBAL_VERSION_KV = KeyValue.newBuilder()
+      .setKey(GLOBAL_VERSION_KEY)
+      .setValue(ByteString.EMPTY)
+      .setVersion(0L)
+      .build();
   private final DSLContext context;
 
   @Inject
@@ -47,16 +53,19 @@ public GetObjectResponse get(GetObjectRequest request) {
         .fetchOne();
 
     final KeyValue keyValue;
-
-    if (vssDbRecord != null) {
+    if (vssDbRecord == null) {
+      if (GLOBAL_VERSION_KEY.equals(request.getKey())) {
+        keyValue = DEFAULT_GLOBAL_VERSION_KV;
+      } else {
+        throw new NoSuchKeyException(
+            "Specified key: " + request.getKey() + " in request does not exist.");
+      }
+    } else {
       keyValue = KeyValue.newBuilder()
           .setKey(vssDbRecord.getKey())
           .setValue(ByteString.copyFrom(vssDbRecord.getValue()))
           .setVersion(vssDbRecord.getVersion())
           .build();
-    } else {
-      keyValue = KeyValue.newBuilder()
-          .setKey(request.getKey()).build();
     }
 
     return GetObjectResponse.newBuilder()

app/src/main/proto/vss.proto

@@ -16,6 +16,9 @@ message GetObjectRequest {
 
   // `Key` for which the value is to be fetched.
   //
+  // If the specified `key` does not exist, returns `ErrorResponse` with `NO_SUCH_KEY_EXCEPTION` as
+  // `ErrorCode`.
+  //
   // Consistency Guarantee:
   // Get(read) operations against a `key` are consistent reads and will reflect all previous writes,
   // since Put/Write provides read-after-write and read-after-update consistency guarantees.
@@ -276,6 +279,9 @@ enum ErrorCode {
   // An internal server error occurred, client is probably at no fault and can safely retry this
   // error with exponential backoff.
   INTERNAL_SERVER_EXCEPTION = 3;
+
+  // NO_SUCH_KEY_EXCEPTION is used when the specified `key` in a `GetObjectRequest` does not exist.
+  NO_SUCH_KEY_EXCEPTION = 4;
 }
 
 // Represents a key-value pair to be stored or retrieved.

app/src/test/java/org/vss/AbstractKVStoreIntegrationTest.java

@@ -11,6 +11,7 @@
 import org.junit.jupiter.api.Test;
 import org.testcontainers.shaded.org.apache.commons.lang3.StringUtils;
 import org.vss.exception.ConflictException;
+import org.vss.exception.NoSuchKeyException;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
@@ -19,7 +20,6 @@
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public abstract class AbstractKVStoreIntegrationTest {
 
@@ -153,25 +153,25 @@ void putAndDeleteShouldSucceedAsAtomicTransaction() {
     assertThat(response.getVersion(), is(1L));
     assertThat(response.getValue().toStringUtf8(), is("k2v1"));
 
-    assertTrue(getObject("k1").getValue().isEmpty());
+    assertThrows(NoSuchKeyException.class, () -> getObject("k1"));
 
     // Delete fails (and hence put as well) due to mismatched version for the deleted item.
     assertThrows(ConflictException.class, () -> putAndDeleteObjects(null, List.of(kv("k3", "k3v1", 0)), List.of(kv("k2", "", 3))));
 
-    assertTrue(getObject("k3").getValue().isEmpty());
-    assertFalse(getObject("k2").getValue().isEmpty());
+    assertThrows(NoSuchKeyException.class, () -> getObject("k3"));
+    assertDoesNotThrow(() -> getObject("k2"));
 
     // Put fails (and hence delete as well) due to mismatched version for the put item.
     assertThrows(ConflictException.class, () -> putAndDeleteObjects(null, List.of(kv("k3", "k3v1", 1)), List.of(kv("k2", "", 1))));
 
-    assertTrue(getObject("k3").getValue().isEmpty());
-    assertFalse(getObject("k2").getValue().isEmpty());
+    assertThrows(NoSuchKeyException.class, () -> getObject("k3"));
+    assertDoesNotThrow(() -> getObject("k2"));
 
     // Put and delete both fail due to mismatched global version.
     assertThrows(ConflictException.class, () -> putAndDeleteObjects(2L, List.of(kv("k3", "k3v1", 0)), List.of(kv("k2", "", 1))));
 
-    assertTrue(getObject("k3").getValue().isEmpty());
-    assertFalse(getObject("k2").getValue().isEmpty());
+    assertThrows(NoSuchKeyException.class, () -> getObject("k3"));
+    assertDoesNotThrow(() -> getObject("k2"));
 
     assertThat(getObject(KVStore.GLOBAL_VERSION_KEY).getVersion(), is(0L));
   }
@@ -182,18 +182,14 @@ void deleteShouldSucceedWhenItemExists() {
     // Conditional Delete
     assertDoesNotThrow(() -> deleteObject(kv("k1", "", 1)));
 
-    KeyValue response = getObject("k1");
-    assertThat(response.getKey(), is("k1"));
-    assertTrue(response.getValue().isEmpty());
+    assertThrows(NoSuchKeyException.class, () -> getObject("k1"));
 
     assertDoesNotThrow(() -> putObjects(null, List.of(kv("k1", "k1v1", 0))));
     assertDoesNotThrow(() -> putObjects(null, List.of(kv("k1", "k1v2", 1))));
     // NonConditional Delete
     assertDoesNotThrow(() -> deleteObject(kv("k1", "", -1)));
 
-    response = getObject("k1");
-    assertThat(response.getKey(), is("k1"));
-    assertTrue(response.getValue().isEmpty());
+    assertThrows(NoSuchKeyException.class, () -> getObject("k1"));
   }
 
   @Test
@@ -207,18 +203,12 @@ void deleteShouldBeIdempotent() {
     assertDoesNotThrow(() -> deleteObject(kv("k1", "", 1)));
     assertDoesNotThrow(() -> deleteObject(kv("k1", "", 1)));
 
-    KeyValue response = getObject("k1");
-    assertThat(response.getKey(), is("k1"));
-    assertTrue(response.getValue().isEmpty());
+    assertThrows(NoSuchKeyException.class, () -> getObject("k1"));
   }
 
   @Test
-  void getShouldReturnEmptyResponseWhenKeyDoesNotExist() {
-    KeyValue response = getObject("non_existent_key");
-
-    assertThat(response.getKey(), is("non_existent_key"));
-    assertTrue(response.getValue().isEmpty());
-    assertThat(response.getVersion(), is(0L));
+  void getShouldThrowNoSuchKeyExceptionWhenKeyDoesNotExist() {
+    assertThrows(NoSuchKeyException.class, () -> getObject("non_existent_key"));
   }
 
   @Test

app/src/test/java/org/vss/api/GetObjectApiTest.java

@@ -16,6 +16,7 @@
 import org.vss.KVStore;
 import org.vss.KeyValue;
 import org.vss.exception.ConflictException;
+import org.vss.exception.NoSuchKeyException;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
@@ -80,6 +81,7 @@ private static Stream<Arguments> provideErrorTestCases() {
     return Stream.of(
         Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION),
         Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION),
+        Arguments.of(new NoSuchKeyException(""), ErrorCode.NO_SUCH_KEY_EXCEPTION),
         Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION)
     );
   }