From d3a05dd6a6bdf52c2bb32f6b61c83cc69b2298ca Mon Sep 17 00:00:00 2001 From: Hai Yan Date: Tue, 13 Feb 2024 16:43:59 -0600 Subject: [PATCH] Address review comments; check empty string for put and delete; add more tests Signed-off-by: Hai Yan --- .../dataprepper/model/event/JacksonEvent.java | 23 ++++---- .../model/event/JacksonEventTest.java | 57 ++++++++++--------- 2 files changed, 43 insertions(+), 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 index a0c670fb61..62b31ad0c5 100644 --- 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 keys = new LinkedList<>(Arrays.asList(trimmedKey.split(SEPARATOR))); + final LinkedList keys = new LinkedList<>(Arrays.asList(trimmedKey.split(SEPARATOR, -1))); JsonNode parentNode = jsonNode; @@ -200,13 +202,8 @@ public T get(final String key, final Class 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 mapNodeToObject(final String key, final JsonNode node, final Class clazz) { @@ -251,7 +248,12 @@ private List 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 index 51572db424..c808e00482 100644 --- 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,21 +279,9 @@ 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); @@ -294,9 +289,9 @@ public void testDelete_withNestedKey() { 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