This is an automated email from the ASF dual-hosted git repository.
wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new cbca7d26 fix(serde): reject invalid schema and partition IDs in JSON
(#940)
cbca7d26 is described below
commit cbca7d268f5a96848eb40de5fa5bfe8c763e1e98
Author: kamcheungting-db <[email protected]>
AuthorDate: Sun Oct 11 08:24:58 2026 -0700
fix(serde): reject invalid schema and partition IDs in JSON (#940)
---
src/iceberg/json_serde.cc | 37 +++++----
src/iceberg/test/json_serde_test.cc | 140 ++++++++++++++++++++++++++++++++
src/iceberg/test/metadata_serde_test.cc | 18 ++++
src/iceberg/util/json_util_internal.h | 12 +++
4 files changed, 190 insertions(+), 17 deletions(-)
diff --git a/src/iceberg/json_serde.cc b/src/iceberg/json_serde.cc
index c7d0cb43..c1c3ffb7 100644
--- a/src/iceberg/json_serde.cc
+++ b/src/iceberg/json_serde.cc
@@ -22,6 +22,7 @@
#include <cstdint>
#include <format>
#include <map>
+#include <optional>
#include <regex>
#include <string>
#include <unordered_set>
@@ -768,7 +769,7 @@ Result<std::unique_ptr<Type>> ListTypeFromJson(const
nlohmann::json& json) {
ICEBERG_ASSIGN_OR_RAISE(
auto element_type,
GetJsonValue<nlohmann::json>(json, kElement).and_then(TypeFromJson));
- ICEBERG_ASSIGN_OR_RAISE(auto element_id, GetJsonValue<int32_t>(json,
kElementId));
+ ICEBERG_ASSIGN_OR_RAISE(auto element_id, GetJsonInteger<int32_t>(json,
kElementId));
ICEBERG_ASSIGN_OR_RAISE(auto element_required,
GetJsonValue<bool>(json, kElementRequired));
@@ -784,8 +785,8 @@ Result<std::unique_ptr<Type>> MapTypeFromJson(const
nlohmann::json& json) {
ICEBERG_ASSIGN_OR_RAISE(
auto value_type, GetJsonValue<nlohmann::json>(json,
kValue).and_then(TypeFromJson));
- ICEBERG_ASSIGN_OR_RAISE(auto key_id, GetJsonValue<int32_t>(json, kKeyId));
- ICEBERG_ASSIGN_OR_RAISE(auto value_id, GetJsonValue<int32_t>(json,
kValueId));
+ ICEBERG_ASSIGN_OR_RAISE(auto key_id, GetJsonInteger<int32_t>(json, kKeyId));
+ ICEBERG_ASSIGN_OR_RAISE(auto value_id, GetJsonInteger<int32_t>(json,
kValueId));
ICEBERG_ASSIGN_OR_RAISE(auto value_required, GetJsonValue<bool>(json,
kValueRequired));
SchemaField key_field(key_id, std::string(MapType::kKeyName),
std::move(key_type),
@@ -946,7 +947,7 @@ Status ValidateTimestamptzDefaultIsUtc(const Type& type,
const nlohmann::json& v
Result<std::unique_ptr<SchemaField>> FieldFromJson(const nlohmann::json& json)
{
ICEBERG_ASSIGN_OR_RAISE(
auto type, GetJsonValue<nlohmann::json>(json,
kType).and_then(TypeFromJson));
- ICEBERG_ASSIGN_OR_RAISE(auto field_id, GetJsonValue<int32_t>(json, kId));
+ ICEBERG_ASSIGN_OR_RAISE(auto field_id, GetJsonInteger<int32_t>(json, kId));
ICEBERG_ASSIGN_OR_RAISE(auto name, GetJsonValue<std::string>(json, kName));
ICEBERG_ASSIGN_OR_RAISE(auto required, GetJsonValue<bool>(json, kRequired));
ICEBERG_ASSIGN_OR_RAISE(auto doc, GetJsonValueOrDefault<std::string>(json,
kDoc));
@@ -979,7 +980,7 @@ Result<std::unique_ptr<SchemaField>> FieldFromJson(const
nlohmann::json& json) {
Result<std::unique_ptr<Schema>> SchemaFromJson(const nlohmann::json& json) {
ICEBERG_ASSIGN_OR_RAISE(auto schema_id_opt,
- GetJsonValueOptional<int32_t>(json, kSchemaId));
+ GetJsonIntegerOptional<int32_t>(json, kSchemaId));
ICEBERG_ASSIGN_OR_RAISE(auto type, TypeFromJson(json));
if (type->type_id() != TypeId::kStruct) [[unlikely]] {
@@ -993,9 +994,11 @@ Result<std::unique_ptr<Schema>> SchemaFromJson(const
nlohmann::json& json) {
fields.emplace_back(std::move(field));
}
- ICEBERG_ASSIGN_OR_RAISE(
- auto identifier_field_ids,
- GetJsonValueOrDefault<std::vector<int32_t>>(json, kIdentifierFieldIds));
+ std::vector<int32_t> identifier_field_ids;
+ if (json.contains(kIdentifierFieldIds) &&
!json.at(kIdentifierFieldIds).is_null()) {
+ ICEBERG_ASSIGN_OR_RAISE(identifier_field_ids,
+ IntegerVectorFromJson<int32_t>(json,
kIdentifierFieldIds));
+ }
return Schema::Make(std::move(fields),
schema_id_opt.value_or(Schema::kInitialSchemaId),
std::move(identifier_field_ids));
@@ -1028,14 +1031,13 @@ Result<std::string> ToJsonString(const PartitionSpec&
partition_spec) {
Result<std::unique_ptr<PartitionField>> PartitionFieldFromJson(
const nlohmann::json& json, bool allow_field_id_missing) {
- ICEBERG_ASSIGN_OR_RAISE(auto source_id, GetJsonValue<int32_t>(json,
kSourceId));
+ ICEBERG_ASSIGN_OR_RAISE(auto source_id, GetJsonInteger<int32_t>(json,
kSourceId));
int32_t field_id;
- if (allow_field_id_missing) {
+ if (allow_field_id_missing && !json.contains(kFieldId)) {
// Partition field id in v1 is not tracked, so we use -1 to indicate that.
- ICEBERG_ASSIGN_OR_RAISE(field_id, GetJsonValueOrDefault<int32_t>(
- json, kFieldId,
SchemaField::kInvalidFieldId));
+ field_id = SchemaField::kInvalidFieldId;
} else {
- ICEBERG_ASSIGN_OR_RAISE(field_id, GetJsonValue<int32_t>(json, kFieldId));
+ ICEBERG_ASSIGN_OR_RAISE(field_id, GetJsonInteger<int32_t>(json, kFieldId));
}
ICEBERG_ASSIGN_OR_RAISE(
auto transform,
@@ -1067,13 +1069,13 @@ Result<std::unique_ptr<PartitionSpec>>
ParseBoundPartitionSpec(
Result<std::unique_ptr<PartitionSpec>> PartitionSpecFromJson(
const std::shared_ptr<Schema>& schema, const nlohmann::json& json,
int32_t default_spec_id) {
- ICEBERG_ASSIGN_OR_RAISE(auto spec_id, GetJsonValue<int32_t>(json, kSpecId));
+ ICEBERG_ASSIGN_OR_RAISE(auto spec_id, GetJsonInteger<int32_t>(json,
kSpecId));
return ParseBoundPartitionSpec(*schema, json, spec_id,
/*allow_missing_fields=*/spec_id !=
default_spec_id);
}
Result<std::unique_ptr<PartitionSpec>> PartitionSpecFromJson(const
nlohmann::json& json) {
- ICEBERG_ASSIGN_OR_RAISE(auto spec_id, GetJsonValue<int32_t>(json, kSpecId));
+ ICEBERG_ASSIGN_OR_RAISE(auto spec_id, GetJsonInteger<int32_t>(json,
kSpecId));
ICEBERG_ASSIGN_OR_RAISE(auto fields, GetJsonValue<nlohmann::json>(json,
kFields));
std::vector<PartitionField> partition_fields;
@@ -1438,7 +1440,7 @@ Result<std::shared_ptr<Schema>> ParseSchemas(
}
ICEBERG_ASSIGN_OR_RAISE(current_schema_id,
- GetJsonValue<int32_t>(json, kCurrentSchemaId));
+ GetJsonInteger<int32_t>(json, kCurrentSchemaId));
for (const auto& schema_json : schema_array) {
ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr<Schema> schema,
SchemaFromJson(schema_json));
@@ -1484,7 +1486,8 @@ Status ParsePartitionSpecs(const nlohmann::json& json,
int8_t format_version,
return JsonParseError("Cannot parse partition specs from non-array: {}",
SafeDumpJson(spec_array));
}
- ICEBERG_ASSIGN_OR_RAISE(default_spec_id, GetJsonValue<int32_t>(json,
kDefaultSpecId));
+ ICEBERG_ASSIGN_OR_RAISE(default_spec_id,
+ GetJsonInteger<int32_t>(json, kDefaultSpecId));
for (const auto& spec_json : spec_array) {
ICEBERG_ASSIGN_OR_RAISE(
diff --git a/src/iceberg/test/json_serde_test.cc
b/src/iceberg/test/json_serde_test.cc
index be363ea3..51bc2af1 100644
--- a/src/iceberg/test/json_serde_test.cc
+++ b/src/iceberg/test/json_serde_test.cc
@@ -17,6 +17,7 @@
* under the License.
*/
+#include <array>
#include <memory>
#include <optional>
#include <string>
@@ -209,6 +210,72 @@ TEST(JsonInternalTest, ListTypeFromJsonMissingElement) {
EXPECT_THAT(result, HasErrorMessage("Missing 'element'"));
}
+TEST(JsonInternalTest, SchemaIdsRejectInvalidJsonIntegers) {
+ const nlohmann::json valid_schema = R"({
+ "type": "struct",
+ "schema-id": 10,
+ "identifier-field-ids": [20],
+ "fields": [
+ {"id": 20, "name": "id", "required": true, "type": "int"},
+ {
+ "id": 30,
+ "name": "items",
+ "required": false,
+ "type": {
+ "type": "list",
+ "element-id": 40,
+ "element-required": false,
+ "element": {
+ "type": "map",
+ "key-id": 50,
+ "key": "string",
+ "value-id": 60,
+ "value": "long",
+ "value-required": false
+ }
+ }
+ }
+ ]
+ })"_json;
+
+ struct IdPath {
+ nlohmann::json::json_pointer pointer;
+ std::string_view key;
+ };
+ const std::array<IdPath, 6> id_paths = {{
+ {.pointer = nlohmann::json::json_pointer("/schema-id"), .key =
"schema-id"},
+ {.pointer = nlohmann::json::json_pointer("/identifier-field-ids/0"),
+ .key = "identifier-field-ids"},
+ {.pointer = nlohmann::json::json_pointer("/fields/0/id"), .key = "id"},
+ {.pointer = nlohmann::json::json_pointer("/fields/1/type/element-id"),
+ .key = "element-id"},
+ {.pointer =
nlohmann::json::json_pointer("/fields/1/type/element/key-id"),
+ .key = "key-id"},
+ {.pointer =
nlohmann::json::json_pointer("/fields/1/type/element/value-id"),
+ .key = "value-id"},
+ }};
+ const std::array<std::pair<nlohmann::json, std::string_view>, 5> invalid_ids
= {{
+ {nlohmann::json(1.5), "must be an integer"},
+ {nlohmann::json(true), "must be an integer"},
+ {nlohmann::json(-2147483649LL), "out of range"},
+ {nlohmann::json(2147483648LL), "out of range"},
+ {nlohmann::json(2147483648ULL), "out of range"},
+ }};
+
+ for (const auto& id_path : id_paths) {
+ for (const auto& [invalid_id, expected_error] : invalid_ids) {
+ SCOPED_TRACE(id_path.pointer.to_string() + "=" + invalid_id.dump());
+ auto invalid_schema = valid_schema;
+ invalid_schema[id_path.pointer] = invalid_id;
+
+ auto result = SchemaFromJson(invalid_schema);
+ EXPECT_THAT(result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(result, HasErrorMessage(id_path.key));
+ EXPECT_THAT(result, HasErrorMessage(expected_error));
+ }
+ }
+}
+
TEST(JsonInternalTest, SortField) {
auto identity_transform = Transform::Identity();
@@ -322,6 +389,79 @@ TEST(JsonInternalTest, PartitionSpecFromJson) {
EXPECT_EQ(*spec, *parsed);
}
+TEST(JsonInternalTest, PartitionIdsRejectInvalidJsonIntegers) {
+ const nlohmann::json valid_spec = R"({
+ "spec-id": 10,
+ "fields": [{
+ "source-id": 20,
+ "field-id": 1000,
+ "transform": "identity",
+ "name": "id"
+ }]
+ })"_json;
+ auto schema = std::make_shared<Schema>(
+ std::vector<SchemaField>{SchemaField(20, "id", int32(), false)},
+ /*schema_id=*/10);
+
+ struct IdPath {
+ nlohmann::json::json_pointer pointer;
+ std::string_view key;
+ };
+ const std::array<IdPath, 3> id_paths = {{
+ {.pointer = nlohmann::json::json_pointer("/spec-id"), .key = "spec-id"},
+ {.pointer = nlohmann::json::json_pointer("/fields/0/source-id"),
+ .key = "source-id"},
+ {.pointer = nlohmann::json::json_pointer("/fields/0/field-id"), .key =
"field-id"},
+ }};
+ const std::array<std::pair<nlohmann::json, std::string_view>, 5> invalid_ids
= {{
+ {nlohmann::json(1.5), "must be an integer"},
+ {nlohmann::json(true), "must be an integer"},
+ {nlohmann::json(-2147483649LL), "out of range"},
+ {nlohmann::json(2147483648LL), "out of range"},
+ {nlohmann::json(2147483648ULL), "out of range"},
+ }};
+
+ for (const auto& id_path : id_paths) {
+ for (const auto& [invalid_id, expected_error] : invalid_ids) {
+ SCOPED_TRACE(id_path.pointer.to_string() + "=" + invalid_id.dump());
+ auto invalid_spec = valid_spec;
+ invalid_spec[id_path.pointer] = invalid_id;
+
+ auto unbound_result = PartitionSpecFromJson(invalid_spec);
+ EXPECT_THAT(unbound_result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(unbound_result, HasErrorMessage(id_path.key));
+ EXPECT_THAT(unbound_result, HasErrorMessage(expected_error));
+
+ auto bound_result = PartitionSpecFromJson(schema, invalid_spec, 10);
+ EXPECT_THAT(bound_result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(bound_result, HasErrorMessage(id_path.key));
+ EXPECT_THAT(bound_result, HasErrorMessage(expected_error));
+ }
+ }
+
+ auto legacy_field = valid_spec["fields"][0];
+ legacy_field.erase("field-id");
+ ICEBERG_UNWRAP_OR_FAIL(
+ auto parsed_legacy_field,
+ PartitionFieldFromJson(legacy_field, /*allow_field_id_missing=*/true));
+ EXPECT_EQ(parsed_legacy_field->field_id(), SchemaField::kInvalidFieldId);
+
+ for (const auto& [invalid_id, expected_error] : invalid_ids) {
+ SCOPED_TRACE("legacy field-id=" + invalid_id.dump());
+ legacy_field["field-id"] = invalid_id;
+ auto result = PartitionFieldFromJson(legacy_field,
/*allow_field_id_missing=*/true);
+ EXPECT_THAT(result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(result, HasErrorMessage("field-id"));
+ EXPECT_THAT(result, HasErrorMessage(expected_error));
+ }
+
+ legacy_field["field-id"] = nullptr;
+ auto null_result =
+ PartitionFieldFromJson(legacy_field, /*allow_field_id_missing=*/true);
+ EXPECT_THAT(null_result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(null_result, HasErrorMessage("field-id"));
+}
+
TEST(JsonInternalTest, SnapshotRefBranch) {
SnapshotRef ref(1234567890, SnapshotRef::Branch{.min_snapshots_to_keep = 10,
.max_snapshot_age_ms =
123456789,
diff --git a/src/iceberg/test/metadata_serde_test.cc
b/src/iceberg/test/metadata_serde_test.cc
index 10529d12..88272866 100644
--- a/src/iceberg/test/metadata_serde_test.cc
+++ b/src/iceberg/test/metadata_serde_test.cc
@@ -327,6 +327,24 @@ TEST(MetadataSerdeTest, DeserializeV2ValidMinimal) {
ASSERT_FALSE(metadata->Snapshot().has_value());
}
+TEST(MetadataSerdeTest, RejectsFractionalSchemaAndDefaultSpecIds) {
+ ICEBERG_UNWRAP_OR_FAIL(
+ auto metadata,
ReadTableMetadataFromResource("TableMetadataV2ValidMinimal.json"));
+ ICEBERG_UNWRAP_OR_FAIL(auto valid_json, ToJson(*metadata));
+ ASSERT_THAT(TableMetadataFromJson(valid_json), IsOk());
+
+ for (const auto* key : {"current-schema-id", "default-spec-id"}) {
+ SCOPED_TRACE(key);
+ auto invalid_json = valid_json;
+ invalid_json[key] = 0.5;
+
+ auto result = TableMetadataFromJson(invalid_json);
+ EXPECT_THAT(result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(result, HasErrorMessage(key));
+ EXPECT_THAT(result, HasErrorMessage("must be an integer"));
+ }
+}
+
TEST(MetadataSerdeTest, DeserializeStatisticsFiles) {
ICEBERG_UNWRAP_OR_FAIL(
auto metadata,
ReadTableMetadataFromResource("TableMetadataStatisticsFiles.json"));
diff --git a/src/iceberg/util/json_util_internal.h
b/src/iceberg/util/json_util_internal.h
index 0c30b449..b1edf583 100644
--- a/src/iceberg/util/json_util_internal.h
+++ b/src/iceberg/util/json_util_internal.h
@@ -145,6 +145,18 @@ Result<Integer> GetJsonInteger(const nlohmann::json& json,
std::string_view key)
return IntegerFromJson<Integer>(value, std::format("'{}'", key));
}
+/// \brief Parses an optional JSON integer field with `IntegerFromJson`
strictness,
+/// returning `std::nullopt` when the key is missing or null.
+template <std::signed_integral Integer>
+Result<std::optional<Integer>> GetJsonIntegerOptional(const nlohmann::json&
json,
+ std::string_view key) {
+ if (!json.contains(key) || json.at(key).is_null()) {
+ return std::nullopt;
+ }
+ ICEBERG_ASSIGN_OR_RAISE(auto value, GetJsonInteger<Integer>(json, key));
+ return value;
+}
+
/// \brief Parses an optional `int64_t` field, returning `default_value` when
the key is
/// missing or null.
inline Result<int64_t> GetJsonIntegerOrDefault(const nlohmann::json& json,
