Address review comments; check empty string for put and delete; add m…

…ore tests Signed-off-by: Hai Yan <[email protected]>
opensearch-project · Feb 13, 2024 · d3a05dd · d3a05dd
1 parent 934ce99
commit d3a05dd
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 37 deletions.
diff --git a/data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/JacksonEvent.java b/data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/JacksonEvent.java
@@ -35,6 +35,7 @@
 import java.util.Objects;
 import java.util.StringJoiner;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 
 /**
@@ -135,10 +136,11 @@ public JsonNode getJsonNode() {
      */
     @Override
     public void put(final String key, final Object value) {
+        checkArgument(!key.isEmpty(), "key cannot be an empty string for put method");
 
         final String trimmedKey = checkAndTrimKey(key);
 
-        final LinkedList<String> keys = new LinkedList<>(Arrays.asList(trimmedKey.split(SEPARATOR)));
+        final LinkedList<String> keys = new LinkedList<>(Arrays.asList(trimmedKey.split(SEPARATOR, -1)));
 
         JsonNode parentNode = jsonNode;
 
@@ -200,13 +202,8 @@ public <T> T get(final String key, final Class<T> clazz) {
     }
 
     private JsonNode getNode(final String key) {
-        final String jsonPointerKey;
-        if (key.isEmpty() || key.startsWith("/")) {
-            jsonPointerKey = key;
-        } else {
-            jsonPointerKey = SEPARATOR + key;
-        }
-        return jsonNode.at(jsonPointerKey);
+        final JsonPointer jsonPointer = toJsonPointer(key);
+        return jsonNode.at(jsonPointer);
     }
 
     private <T> T mapNodeToObject(final String key, final JsonNode node, final Class<T> clazz) {
@@ -251,7 +248,12 @@ private <T> List<T> mapNodeToList(final String key, final JsonNode node, final C
     }
 
     private JsonPointer toJsonPointer(final String key) {
-        String jsonPointerExpression = SEPARATOR + key;
+        final String jsonPointerExpression;
+        if (key.isEmpty() || key.startsWith("/")) {
+            jsonPointerExpression = key;
+        } else {
+            jsonPointerExpression = SEPARATOR + key;
+        }
         return JsonPointer.compile(jsonPointerExpression);
     }
 
@@ -263,6 +265,7 @@ private JsonPointer toJsonPointer(final String key) {
     @Override
     public void delete(final String key) {
 
+        checkArgument(!key.isEmpty(), "key cannot be an empty string for delete method");
         final String trimmedKey = checkAndTrimKey(key);
         final int index = trimmedKey.lastIndexOf(SEPARATOR);
 
@@ -427,7 +430,7 @@ private String trimKey(final String key) {
     }
 
     private String trimTrailingSlashInKey(final String key) {
-        return key.endsWith(SEPARATOR) ? key.substring(0, key.length() - 1) : key;
+        return key.length() > 1 && key.endsWith(SEPARATOR) ? key.substring(0, key.length() - 1) : key;
     }
 
     private static boolean isValidKey(final String key) {

diff --git a/data-prepper-api/src/test/java/org/opensearch/dataprepper/model/event/JacksonEventTest.java b/data-prepper-api/src/test/java/org/opensearch/dataprepper/model/event/JacksonEventTest.java
@@ -23,6 +23,7 @@
 import java.util.Random;
 import java.util.UUID;
 
+import static org.hamcrest.CoreMatchers.containsStringIgnoringCase;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
@@ -69,8 +70,14 @@ public void testPutAndGet_withRandomString() {
         assertThat(result, is(equalTo(value)));
     }
 
+    @Test
+    public void testSplit() {
+        String key = "/";
+        System.out.println(key.split("/"));
+    }
+
     @ParameterizedTest
-    @ValueSource(strings = {"foo", "foo-bar", "foo_bar", "foo.bar", "/foo", "/foo/", "a1K.k3-01_02"})
+    @ValueSource(strings = {"/", "foo", "foo-bar", "foo_bar", "foo.bar", "/foo", "/foo/", "a1K.k3-01_02"})
     void testPutAndGet_withStrings(final String key) {
         final UUID value = UUID.randomUUID();
 
@@ -272,31 +279,19 @@ public void testOverwritingExistingKey() {
         assertThat(result, is(equalTo(value)));
     }
 
-    @Test
-    public void testDeletingKey() {
-        final String key = "foo";
-
-        event.put(key, UUID.randomUUID());
-        event.delete(key);
-        final UUID result = event.get(key, UUID.class);
-
-        assertThat(result, is(nullValue()));
-    }
-
-    @Test
-    public void testDelete_withNestedKey() {
-        final String key = "foo/bar";
-
+    @ParameterizedTest
+    @ValueSource(strings = {"/", "foo", "/foo", "/foo/bar", "foo/bar", "foo/bar/", "/foo/bar/leaf/key"})
+    public void testDeleteKey(final String key) {
         event.put(key, UUID.randomUUID());
         event.delete(key);
         final UUID result = event.get(key, UUID.class);
 
         assertThat(result, is(nullValue()));
     }
 
-    @Test
-    public void testDelete_withNonexistentKey() {
-        final String key = "foo/bar";
+    @ParameterizedTest
+    @ValueSource(strings = {"/", "foo", "/foo", "/foo/bar", "foo/bar", "foo/bar/", "/foo/bar/leaf/key"})
+    public void testDelete_withNonexistentKey(final String key) {
         UUID result = event.get(key, UUID.class);
         assertThat(result, is(nullValue()));
 
@@ -307,19 +302,27 @@ public void testDelete_withNonexistentKey() {
     }
 
     @Test
-    public void testContainsKey_withKey() {
-        final String key = "foo";
-
-        event.put(key, UUID.randomUUID());
-        assertThat(event.containsKey(key), is(true));
+    public void testDeleteKeyCannotBeEmptyString() {
+        Throwable exception = assertThrows(IllegalArgumentException.class, () -> event.delete(""));
+        assertThat(exception.getMessage(), containsStringIgnoringCase("key cannot be an empty string"));
     }
 
     @Test
-    public void testContainsKey_withouthKey() {
-        final String key = "foo";
+    public void testContainsKeyReturnsTrueForEmptyStringKey() {
+        assertThat(event.containsKey(""), is(true));
+    }
 
+    @ParameterizedTest
+    @ValueSource(strings = {"/", "foo", "/foo", "/foo/bar", "foo/bar", "foo/bar/", "/foo/bar/leaf/key"})
+    public void testContainsKey_withKey(final String key) {
         event.put(key, UUID.randomUUID());
-        assertThat(event.containsKey("bar"), is(false));
+        assertThat(event.containsKey(key), is(true));
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"/", "foo", "/foo", "/foo/bar", "foo/bar", "foo/bar/", "/foo/bar/leaf/key"})
+    public void testContainsKey_withouthKey(final String key) {
+        assertThat(event.containsKey(key), is(false));
     }
 
     @Test