From 99d051f5178afdf99eb4d4b08dc8cc4da8933660 Mon Sep 17 00:00:00 2001 From: Steven Wu Date: Wed, 27 May 2026 16:29:26 -0700 Subject: [PATCH 1/2] Core: Validate non-string elements in JsonUtil.getStringArray Add an isTextual() check on each array element to fail fast with a clear error message when a non-string element is encountered, matching the validation already performed by JsonStringArrayIterator (used by getStringList, getStringSet, getStringListOrNull). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../org/apache/iceberg/util/JsonUtil.java | 5 ++++- .../org/apache/iceberg/util/TestJsonUtil.java | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/util/JsonUtil.java b/core/src/main/java/org/apache/iceberg/util/JsonUtil.java index d8bb1f919096..b371508adc1d 100644 --- a/core/src/main/java/org/apache/iceberg/util/JsonUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/JsonUtil.java @@ -272,7 +272,10 @@ public static String[] getStringArray(JsonNode node) { ArrayNode arrayNode = (ArrayNode) node; String[] arr = new String[arrayNode.size()]; for (int i = 0; i < arr.length; i++) { - arr[i] = arrayNode.get(i).asText(); + JsonNode element = arrayNode.get(i); + Preconditions.checkArgument( + element.isTextual(), "Cannot parse string from non-text value: %s", element); + arr[i] = element.asText(); } return arr; } diff --git a/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java b/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java index 91cd96e9088f..b965fc7ee98c 100644 --- a/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java +++ b/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java @@ -430,6 +430,26 @@ public void getLongSetOrNull() throws JsonProcessingException { .containsExactlyElementsOf(Arrays.asList(23L, 45L)); } + @Test + public void getStringArray() throws JsonProcessingException { + assertThatThrownBy(() -> JsonUtil.getStringArray(JsonUtil.mapper().readTree("null"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse string array from non-array: null"); + + assertThatThrownBy(() -> JsonUtil.getStringArray(JsonUtil.mapper().readTree("23"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse string array from non-array: 23"); + + assertThatThrownBy(() -> JsonUtil.getStringArray(JsonUtil.mapper().readTree("[\"23\", 45]"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse string from non-text value: 45"); + + assertThat(JsonUtil.getStringArray(JsonUtil.mapper().readTree("[\"23\", \"45\"]"))) + .containsExactly("23", "45"); + + assertThat(JsonUtil.getStringArray(JsonUtil.mapper().readTree("[]"))).isEmpty(); + } + @Test public void getStringList() throws JsonProcessingException { assertThatThrownBy(() -> JsonUtil.getStringList("items", JsonUtil.mapper().readTree("{}"))) From a3a82fcc1d07eef3c3e2d753ae80886983d0dd9b Mon Sep 17 00:00:00 2001 From: Steven Wu Date: Thu, 28 May 2026 08:07:04 -0700 Subject: [PATCH 2/2] Core: Add property-aware getStringArray and migrate two callers Add an overload getStringArray(String property, JsonNode node) that delegates to getStringList so its error messages include the field name (e.g. "Cannot parse string from non-text value in default-namespace: 45") matching the convention used by getStringList/getStringSet. Migrate ViewVersionParser and RemoteSignRequestParser, which both have a property name in scope. RESTSerializers.NamespaceDeserializer deserializes a top-level Namespace with no field name in scope and keeps the existing 1-arg overload. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../requests/RemoteSignRequestParser.java | 2 +- .../org/apache/iceberg/util/JsonUtil.java | 4 +++ .../iceberg/view/ViewVersionParser.java | 3 +-- .../org/apache/iceberg/util/TestJsonUtil.java | 27 +++++++++++++++++++ .../iceberg/view/TestViewVersionParser.java | 2 +- 5 files changed, 34 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/rest/requests/RemoteSignRequestParser.java b/core/src/main/java/org/apache/iceberg/rest/requests/RemoteSignRequestParser.java index 61b44cc177d1..a29326a76b6f 100644 --- a/core/src/main/java/org/apache/iceberg/rest/requests/RemoteSignRequestParser.java +++ b/core/src/main/java/org/apache/iceberg/rest/requests/RemoteSignRequestParser.java @@ -133,7 +133,7 @@ public static Map> headersFromJson(String property, JsonNod .forEach( entry -> { String key = entry.getKey(); - List values = Arrays.asList(JsonUtil.getStringArray(entry.getValue())); + List values = Arrays.asList(JsonUtil.getStringArray(key, headersNode)); headers.put(key, values); }); return headers; diff --git a/core/src/main/java/org/apache/iceberg/util/JsonUtil.java b/core/src/main/java/org/apache/iceberg/util/JsonUtil.java index b371508adc1d..58d878b94901 100644 --- a/core/src/main/java/org/apache/iceberg/util/JsonUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/JsonUtil.java @@ -280,6 +280,10 @@ public static String[] getStringArray(JsonNode node) { return arr; } + public static String[] getStringArray(String property, JsonNode node) { + return getStringList(property, node).toArray(new String[0]); + } + public static List getStringList(String property, JsonNode node) { Preconditions.checkArgument(node.has(property), "Cannot parse missing list: %s", property); return ImmutableList.builder() diff --git a/core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java b/core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java index 06ee3b2648d2..69208ce34062 100644 --- a/core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java +++ b/core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java @@ -96,8 +96,7 @@ public static ViewVersion fromJson(JsonNode node) { String defaultCatalog = JsonUtil.getStringOrNull(DEFAULT_CATALOG, node); - Namespace defaultNamespace = - Namespace.of(JsonUtil.getStringArray(JsonUtil.get(DEFAULT_NAMESPACE, node))); + Namespace defaultNamespace = Namespace.of(JsonUtil.getStringArray(DEFAULT_NAMESPACE, node)); return ImmutableViewVersion.builder() .versionId(versionId) diff --git a/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java b/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java index b965fc7ee98c..27cf6fb2994e 100644 --- a/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java +++ b/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java @@ -450,6 +450,33 @@ public void getStringArray() throws JsonProcessingException { assertThat(JsonUtil.getStringArray(JsonUtil.mapper().readTree("[]"))).isEmpty(); } + @Test + public void getStringArrayWithProperty() throws JsonProcessingException { + assertThatThrownBy(() -> JsonUtil.getStringArray("items", JsonUtil.mapper().readTree("{}"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse missing list: items"); + + assertThatThrownBy( + () -> JsonUtil.getStringArray("items", JsonUtil.mapper().readTree("{\"items\": null}"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse JSON array from non-array value: items: null"); + + assertThatThrownBy( + () -> + JsonUtil.getStringArray( + "items", JsonUtil.mapper().readTree("{\"items\": [\"23\", 45]}"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse string from non-text value in items: 45"); + + assertThat( + JsonUtil.getStringArray( + "items", JsonUtil.mapper().readTree("{\"items\": [\"23\", \"45\"]}"))) + .containsExactly("23", "45"); + + assertThat(JsonUtil.getStringArray("items", JsonUtil.mapper().readTree("{\"items\": []}"))) + .isEmpty(); + } + @Test public void getStringList() throws JsonProcessingException { assertThatThrownBy(() -> JsonUtil.getStringList("items", JsonUtil.mapper().readTree("{}"))) diff --git a/core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java b/core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java index a68b99a6797b..a46e63401632 100644 --- a/core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java +++ b/core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java @@ -127,7 +127,7 @@ public void missingDefaultCatalog() { "{\"version-id\":1,\"timestamp-ms\":12345,\"schema-id\":1," + "\"summary\":{\"operation\":\"create\"},\"representations\":[]}")) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot parse missing field: default-namespace"); + .hasMessage("Cannot parse missing list: default-namespace"); } @Test