You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by ga...@apache.org on 2026/10/19 03:26:53 UTC
(iceberg-cpp) branch main updated: fix(json): omit JSON values from parse errors (#1003)
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 20f24071 fix(json): omit JSON values from parse errors (#1003)
20f24071 is described below
commit 20f2407105d1a918e7084daf0e224d3497c4af91
Author: lishuxu <li...@gmail.com>
AuthorDate: Mon Oct 19 11:26:47 2026 +0800
fix(json): omit JSON values from parse errors (#1003)
Malformed REST responses and metadata can produce parse errors containing entire JSON documents or raw field values. Report field names, JSON types, and structural reasons instead, and report only the input byte count when JSON parsing fails.
The shared helpers also omit underlying exception text. Add regression coverage for REST table responses, OAuth token responses, and malformed JSON, and update existing assertions to use the new diagnostics. Successful parsing behavior is unchanged.
---
src/iceberg/catalog/rest/json_serde.cc | 28 +++---
src/iceberg/json_serde.cc | 74 ++++++++-------
src/iceberg/test/rest_json_serde_test.cc | 150 +++++++++++++++++++++++--------
src/iceberg/util/json_util_internal.h | 41 +++++----
4 files changed, 195 insertions(+), 98 deletions(-)
diff --git a/src/iceberg/catalog/rest/json_serde.cc b/src/iceberg/catalog/rest/json_serde.cc
index 7e90bf35..7b862e72 100644
--- a/src/iceberg/catalog/rest/json_serde.cc
+++ b/src/iceberg/catalog/rest/json_serde.cc
@@ -176,14 +176,16 @@ Result<std::vector<std::shared_ptr<FileScanTask>>> FileScanTasksFromJson(
partition_spec_by_id,
const Schema& schema) {
if (!json.is_array()) {
- return JsonParseError("Cannot parse file scan tasks from non-array: {}",
- SafeDumpJson(json));
+ return JsonParseError(
+ "Cannot parse file scan tasks from non-array (JSON value of type {})",
+ json.type_name());
}
std::vector<std::shared_ptr<FileScanTask>> file_scan_tasks;
for (const auto& task_json : json) {
if (!task_json.is_object()) {
- return JsonParseError("Cannot parse file scan task from a non-object: {}",
- SafeDumpJson(task_json));
+ return JsonParseError(
+ "Cannot parse file scan task from a non-object (JSON value of type {})",
+ task_json.type_name());
}
ICEBERG_ASSIGN_OR_RAISE(auto data_file_json,
@@ -353,8 +355,9 @@ Status ScanTaskFieldsFromJson(
GetJsonValue<nlohmann::json>(json, kDeleteFiles));
}
if (!delete_files_json.is_array()) {
- return JsonParseError("Cannot parse delete files from non-array: {}",
- SafeDumpJson(delete_files_json));
+ return JsonParseError(
+ "Cannot parse delete files from non-array (JSON value of type {})",
+ delete_files_json.type_name());
}
for (const auto& entry_json : delete_files_json) {
ICEBERG_ASSIGN_OR_RAISE(
@@ -522,8 +525,9 @@ nlohmann::json ToJson(const FieldLabel& field_label) {
Result<FieldLabel> FieldLabelFromJson(const nlohmann::json& json) {
if (!json.is_object()) {
- return JsonParseError("Cannot parse field label from non-object: {}",
- SafeDumpJson(json));
+ return JsonParseError(
+ "Cannot parse field label from non-object (JSON value of type {})",
+ json.type_name());
}
FieldLabel field_label;
ICEBERG_ASSIGN_OR_RAISE(field_label.field_id, GetJsonInteger<int32_t>(json, kFieldId));
@@ -555,8 +559,8 @@ Result<Labels> LabelsFromJson(const nlohmann::json& json) {
GetJsonValueOrDefault<decltype(labels.object_labels)>(json, kObjectLabels));
if (auto it = json.find(kFields); it != json.end() && !it->is_null()) {
if (!it->is_array()) {
- return JsonParseError("Cannot parse '{}' from non-array: {}", kFields,
- SafeDumpJson(*it));
+ return JsonParseError("Cannot parse '{}' from non-array (JSON value of type {})",
+ kFields, it->type_name());
}
for (const auto& entry : *it) {
ICEBERG_ASSIGN_OR_RAISE(auto field_label, FieldLabelFromJson(entry));
@@ -812,7 +816,7 @@ Result<CommitTableRequest> CommitTableRequestFromJson(const nlohmann::json& json
if (requirements_json.has_value()) {
if (!requirements_json->is_array()) {
return JsonParseError("Expected '{}' to be an array, got {}", kRequirements,
- SafeDumpJson(*requirements_json));
+ requirements_json->type_name());
}
for (const auto& req_json : *requirements_json) {
ICEBERG_ASSIGN_OR_RAISE(auto requirement, TableRequirementFromJson(req_json));
@@ -825,7 +829,7 @@ Result<CommitTableRequest> CommitTableRequestFromJson(const nlohmann::json& json
if (updates_json.has_value()) {
if (!updates_json->is_array()) {
return JsonParseError("Expected '{}' to be an array, got {}", kUpdates,
- SafeDumpJson(*updates_json));
+ updates_json->type_name());
}
for (const auto& update_json : *updates_json) {
ICEBERG_ASSIGN_OR_RAISE(auto update, TableUpdateFromJson(update_json));
diff --git a/src/iceberg/json_serde.cc b/src/iceberg/json_serde.cc
index 0cc4226c..e852060f 100644
--- a/src/iceberg/json_serde.cc
+++ b/src/iceberg/json_serde.cc
@@ -439,8 +439,9 @@ Result<PartitionValues> PartitionValuesFromJson(const nlohmann::json& json,
}
} else {
return JsonParseError(
- "Invalid partition data for content file: expected array or object ({})",
- SafeDumpJson(json));
+ "Invalid partition data for content file: expected array or object (JSON value "
+ "of type {})",
+ json.type_name());
}
return PartitionValues(std::move(values));
@@ -930,8 +931,9 @@ Status ValidateTimestamptzDefaultIsUtc(const Type& type, const nlohmann::json& v
return {};
}
if (!value.is_string()) {
- return JsonParseError("Invalid timestamptz default {} for {}: expected a string",
- SafeDumpJson(value), type.ToString());
+ return JsonParseError(
+ "Invalid timestamptz default for {}: expected a string (JSON value of type {})",
+ type.ToString(), value.type_name());
}
const auto str = value.get<std::string>();
ICEBERG_ASSIGN_OR_RAISE(bool is_utc, TemporalUtils::IsUtcOffset(str));
@@ -985,7 +987,7 @@ Result<std::unique_ptr<Schema>> SchemaFromJson(const nlohmann::json& json) {
ICEBERG_ASSIGN_OR_RAISE(auto type, TypeFromJson(json));
if (type->type_id() != TypeId::kStruct) [[unlikely]] {
- return JsonParseError("Schema must be a struct type, but got {}", SafeDumpJson(json));
+ return JsonParseError("Schema must be a struct type, but got {}", type->ToString());
}
auto& struct_type = internal::checked_cast<StructType&>(*type);
@@ -1142,12 +1144,13 @@ Result<std::unique_ptr<Snapshot>> SnapshotFromJson(const nlohmann::json& json) {
// return JsonParseError("Invalid snapshot summary field: {}", key);
// }
if (!value.is_string()) {
- return JsonParseError("Invalid snapshot summary field value: {}",
- SafeDumpJson(value));
+ return JsonParseError(
+ "Invalid snapshot summary field value for '{}' (JSON value of type {})", key,
+ value.type_name());
}
if (key == SnapshotSummaryFields::kOperation &&
!kValidDataOperation.contains(value.get<std::string>())) {
- return JsonParseError("Invalid snapshot operation: {}", SafeDumpJson(value));
+ return JsonParseError("Invalid snapshot operation in snapshot summary");
}
summary[key] = value.get<std::string>();
}
@@ -1320,8 +1323,9 @@ Result<EncryptedKey> EncryptedKeyFromJson(const nlohmann::json& json) {
using StringMap = std::unordered_map<std::string, std::string>;
if (!json.is_object()) {
- return JsonParseError("Invalid encryption key, must be non-null object: {}",
- SafeDumpJson(json));
+ return JsonParseError(
+ "Invalid encryption key, must be non-null object (JSON value of type {})",
+ json.type_name());
}
ICEBERG_ASSIGN_OR_RAISE(auto key_id, GetJsonValue<std::string>(json, kKeyId));
@@ -1445,8 +1449,8 @@ Result<std::shared_ptr<Schema>> ParseSchemas(
ICEBERG_ASSIGN_OR_RAISE(auto schema_array,
GetJsonValue<nlohmann::json>(json, kSchemas));
if (!schema_array.is_array()) {
- return JsonParseError("Cannot parse schemas from non-array: {}",
- SafeDumpJson(schema_array));
+ return JsonParseError("Cannot parse schemas from non-array (JSON value of type {})",
+ schema_array.type_name());
}
ICEBERG_ASSIGN_OR_RAISE(current_schema_id,
@@ -1461,8 +1465,8 @@ Result<std::shared_ptr<Schema>> ParseSchemas(
schemas.push_back(std::move(schema));
}
if (!current_schema) {
- return JsonParseError("Cannot find schema with {}={} from {}", kCurrentSchemaId,
- current_schema_id, SafeDumpJson(schema_array));
+ return JsonParseError("Cannot find schema with {}={}", kCurrentSchemaId,
+ current_schema_id);
}
} else {
if (format_version != 1) {
@@ -1493,8 +1497,9 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version,
ICEBERG_ASSIGN_OR_RAISE(auto spec_array,
GetJsonValue<nlohmann::json>(json, kPartitionSpecs));
if (!spec_array.is_array()) {
- return JsonParseError("Cannot parse partition specs from non-array: {}",
- SafeDumpJson(spec_array));
+ return JsonParseError(
+ "Cannot parse partition specs from non-array (JSON value of type {})",
+ spec_array.type_name());
}
ICEBERG_ASSIGN_OR_RAISE(default_spec_id,
GetJsonInteger<int32_t>(json, kDefaultSpecId));
@@ -1513,8 +1518,9 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version,
ICEBERG_ASSIGN_OR_RAISE(auto partition_spec_json,
GetJsonValue<nlohmann::json>(json, kPartitionSpec));
if (!partition_spec_json.is_array()) {
- return JsonParseError("Cannot parse v1 partition spec from non-array: {}",
- SafeDumpJson(partition_spec_json));
+ return JsonParseError(
+ "Cannot parse v1 partition spec from non-array (JSON value of type {})",
+ partition_spec_json.type_name());
}
int32_t next_partition_field_id = PartitionSpec::kLegacyPartitionDataIdStart;
@@ -1581,8 +1587,9 @@ Status ParseSortOrders(const nlohmann::json& json, int8_t format_version,
Result<std::unique_ptr<TableMetadata>> TableMetadataFromJson(const nlohmann::json& json) {
if (!json.is_object()) {
- return JsonParseError("Cannot parse metadata from a non-object: {}",
- SafeDumpJson(json));
+ return JsonParseError(
+ "Cannot parse metadata from a non-object (JSON value of type {})",
+ json.type_name());
}
auto table_metadata = std::make_unique<TableMetadata>();
@@ -1701,7 +1708,8 @@ Result<nlohmann::json> FromJsonString(const std::string& json_string) {
auto json =
nlohmann::json::parse(json_string, /*cb=*/nullptr, /*allow_exceptions=*/false);
if (json.is_discarded()) [[unlikely]] {
- return JsonParseError("Failed to parse JSON string: {}", json_string);
+ // Report the size only, not the raw input.
+ return JsonParseError("Failed to parse JSON string of {} bytes", json_string.size());
}
return json;
}
@@ -1734,8 +1742,8 @@ nlohmann::json ToJson(const MappedField& field) {
Result<MappedField> MappedFieldFromJson(const nlohmann::json& json) {
if (!json.is_object()) [[unlikely]] {
- return JsonParseError("Cannot parse non-object mapping field: {}",
- SafeDumpJson(json));
+ return JsonParseError("Cannot parse non-object mapping field (JSON value of type {})",
+ json.type_name());
}
ICEBERG_ASSIGN_OR_RAISE(std::optional<int32_t> field_id,
@@ -1768,8 +1776,8 @@ nlohmann::json ToJson(const MappedFields& mapped_fields) {
Result<std::unique_ptr<MappedFields>> MappedFieldsFromJson(const nlohmann::json& json) {
if (!json.is_array()) [[unlikely]] {
- return JsonParseError("Cannot parse non-array mapping fields: {}",
- SafeDumpJson(json));
+ return JsonParseError("Cannot parse non-array mapping fields (JSON value of type {})",
+ json.type_name());
}
std::vector<MappedField> fields;
@@ -1822,7 +1830,8 @@ nlohmann::json ToJson(const Namespace& ns) { return ns.levels; }
Result<Namespace> NamespaceFromJson(const nlohmann::json& json) {
if (!json.is_array()) [[unlikely]] {
- return JsonParseError("Cannot parse namespace from non-array:{}", SafeDumpJson(json));
+ return JsonParseError("Cannot parse namespace from non-array (JSON value of type {})",
+ json.type_name());
}
Namespace ns;
ICEBERG_ASSIGN_OR_RAISE(ns.levels, GetTypedJsonValue<std::vector<std::string>>(json));
@@ -2279,7 +2288,8 @@ Result<DataFile> DataFileFromJson(
partition_spec_by_id,
const Schema& schema) {
if (!json.is_object()) {
- return JsonParseError("DataFile must be a JSON object: {}", SafeDumpJson(json));
+ return JsonParseError("DataFile must be a JSON object (JSON value of type {})",
+ json.type_name());
}
DataFile data_file;
@@ -2476,8 +2486,9 @@ Result<nlohmann::json> ToJson(
Result<std::shared_ptr<FileScanTask>> FileScanTaskFromJson(const nlohmann::json& json) {
if (!json.is_object()) {
- return JsonParseError("Cannot parse file scan task from a non-object: {}",
- SafeDumpJson(json));
+ return JsonParseError(
+ "Cannot parse file scan task from a non-object (JSON value of type {})",
+ json.type_name());
}
ICEBERG_ASSIGN_OR_RAISE(auto task_type,
GetJsonValueOptional<std::string>(json, kTaskType));
@@ -2522,8 +2533,9 @@ Result<std::shared_ptr<FileScanTask>> FileScanTaskFromJson(const nlohmann::json&
ICEBERG_ASSIGN_OR_RAISE(auto delete_files_json,
GetJsonValue<nlohmann::json>(json, kDeleteFiles));
if (!delete_files_json.is_array()) {
- return JsonParseError("Cannot parse delete files from non-array: {}",
- SafeDumpJson(delete_files_json));
+ return JsonParseError(
+ "Cannot parse delete files from non-array (JSON value of type {})",
+ delete_files_json.type_name());
}
for (const auto& delete_file_json : delete_files_json) {
ICEBERG_ASSIGN_OR_RAISE(
diff --git a/src/iceberg/test/rest_json_serde_test.cc b/src/iceberg/test/rest_json_serde_test.cc
index 1a593ffc..ae5ba1a7 100644
--- a/src/iceberg/test/rest_json_serde_test.cc
+++ b/src/iceberg/test/rest_json_serde_test.cc
@@ -17,6 +17,7 @@
* under the License.
*/
+#include <format>
#include <limits>
#include <memory>
#include <string>
@@ -228,12 +229,14 @@ INSTANTIATE_TEST_SUITE_P(
CreateNamespaceRequestInvalidParam{
.test_name = "WrongNamespaceType",
.invalid_json_str = R"({"namespace":"accounting%1Ftax","properties":null})",
- .expected_error_message = "type must be array, but is string"},
+ .expected_error_message =
+ "Failed to parse 'namespace' (JSON value of type string)"},
// Incorrect type for properties field
CreateNamespaceRequestInvalidParam{
.test_name = "WrongPropertiesType",
.invalid_json_str = R"({"namespace":["accounting","tax"],"properties":[]})",
- .expected_error_message = "type must be object, but is array"},
+ .expected_error_message =
+ "Failed to parse 'properties' (JSON value of type array)"},
// Misspelled required field
CreateNamespaceRequestInvalidParam{
.test_name = "MisspelledKeys",
@@ -321,12 +324,14 @@ INSTANTIATE_TEST_SUITE_P(
CreateNamespaceResponseInvalidParam{
.test_name = "WrongNamespaceType",
.invalid_json_str = R"({"namespace":"accounting%1Ftax","properties":null})",
- .expected_error_message = "type must be array, but is string"},
+ .expected_error_message =
+ "Failed to parse 'namespace' (JSON value of type string)"},
// Incorrect type for properties field
CreateNamespaceResponseInvalidParam{
.test_name = "WrongPropertiesType",
.invalid_json_str = R"({"namespace":["accounting","tax"],"properties":[]})",
- .expected_error_message = "type must be object, but is array"},
+ .expected_error_message =
+ "Failed to parse 'properties' (JSON value of type array)"},
// Empty JSON object
CreateNamespaceResponseInvalidParam{
.test_name = "EmptyJson",
@@ -380,12 +385,14 @@ INSTANTIATE_TEST_SUITE_P(
GetNamespaceResponseInvalidParam{
.test_name = "WrongNamespaceType",
.invalid_json_str = R"({"namespace":"accounting%1Ftax","properties":null})",
- .expected_error_message = "type must be array, but is string"},
+ .expected_error_message =
+ "Failed to parse 'namespace' (JSON value of type string)"},
// Incorrect type for properties field
GetNamespaceResponseInvalidParam{
.test_name = "WrongPropertiesType",
.invalid_json_str = R"({"namespace":["accounting","tax"],"properties":[]})",
- .expected_error_message = "type must be object, but is array"},
+ .expected_error_message =
+ "Failed to parse 'properties' (JSON value of type array)"},
// Empty JSON object
GetNamespaceResponseInvalidParam{
.test_name = "EmptyJson",
@@ -506,12 +513,14 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "WrongRemovalsType",
.invalid_json_str =
R"({"removals":{"foo":"bar"},"updates":{"owner":"Hank"}})",
- .expected_error_message = "type must be array, but is object"},
+ .expected_error_message =
+ "Failed to parse 'removals' (JSON value of type object)"},
// Incorrect type for updates field
UpdateNamespacePropertiesRequestInvalidParam{
.test_name = "WrongUpdatesType",
.invalid_json_str = R"({"removals":["foo","bar"],"updates":["owner"]})",
- .expected_error_message = "type must be object, but is array"}),
+ .expected_error_message =
+ "Failed to parse 'updates' (JSON value of type array)"}),
[](const ::testing::TestParamInfo<UpdateNamespacePropertiesRequestInvalidParam>&
info) { return info.param.test_name; });
@@ -578,19 +587,22 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "WrongRemovedType",
.invalid_json_str =
R"({"removed":{"foo":true},"updated":["owner"],"missing":["bar"]})",
- .expected_error_message = "type must be array, but is object"},
+ .expected_error_message =
+ "Failed to parse 'removed' (JSON value of type object)"},
// Incorrect type for updated field
UpdateNamespacePropertiesResponseInvalidParam{
.test_name = "WrongUpdatedType",
.invalid_json_str = R"({"updated":"owner","missing":["bar"]})",
- .expected_error_message = "type must be array, but is string"},
+ .expected_error_message =
+ "Failed to parse 'updated' (JSON value of type string)"},
// Valid top-level (array) types, but at least one entry in the list is not the
// expected type
UpdateNamespacePropertiesResponseInvalidParam{
.test_name = "InvalidArrayEntryType",
.invalid_json_str =
R"({"removed":["foo", "bar", 123456],"updated":["owner"],"missing":["bar"]})",
- .expected_error_message = " type must be string, but is number"}),
+ .expected_error_message =
+ "Failed to parse 'removed' (JSON value of type array)"}),
[](const ::testing::TestParamInfo<UpdateNamespacePropertiesResponseInvalidParam>&
info) { return info.param.test_name; });
@@ -642,7 +654,8 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "InvalidIdentifierNamespaceType",
.invalid_json_str =
R"({"identifiers":[{"namespace":"accounting.tax","name":"paid"}]})",
- .expected_error_message = "type must be array, but is string"}),
+ .expected_error_message =
+ "Failed to parse 'namespace' (JSON value of type string)"}),
[](const ::testing::TestParamInfo<ListTablesResponseInvalidParam>& info) {
return info.param.test_name;
});
@@ -852,13 +865,15 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "WrongDefaultsType",
.invalid_json_str =
R"({"defaults":["warehouse","s3://bucket/warehouse"],"overrides":{"clients":"5"}})",
- .expected_error_message = "type must be object, but is array"},
+ .expected_error_message =
+ "Failed to parse 'defaults' (JSON value of type array)"},
// Overrides has wrong type (string instead of object)
CatalogConfigInvalidParam{
.test_name = "WrongOverridesType",
.invalid_json_str =
R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":"clients"})",
- .expected_error_message = "type must be object, but is string"},
+ .expected_error_message =
+ "Failed to parse 'overrides' (JSON value of type string)"},
// Invalid endpoint format - missing space separator
CatalogConfigInvalidParam{
.test_name = "InvalidEndpointMissingSpace",
@@ -968,7 +983,8 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "WrongMessageType",
.invalid_json_str =
R"({"error":{"message":123,"type":"NoSuchNamespaceException","code":404}})",
- .expected_error_message = "type must be string, but is number"}),
+ .expected_error_message =
+ "Failed to parse 'message' (JSON value of type number)"}),
[](const ::testing::TestParamInfo<ErrorResponseInvalidParam>& info) {
return info.param.test_name;
});
@@ -1089,7 +1105,8 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "WrongNameType",
.invalid_json_str =
R"({"name":123,"schema":{"type":"struct","fields":[{"id":1,"name":"id","type":"int","required":true}]}})",
- .expected_error_message = "type must be string, but is number"},
+ .expected_error_message =
+ "Failed to parse 'name' (JSON value of type number)"},
// Wrong type for schema field
CreateTableRequestInvalidParam{
.test_name = "WrongSchemaType",
@@ -1234,18 +1251,21 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "WrongMetadataLocationType",
.invalid_json_str =
R"({"metadata-location":123,"metadata":{"format-version":2,"table-uuid":"test","location":"s3://test","last-sequence-number":0,"schemas":[{"type":"struct","schema-id":1,"fields":[{"id":1,"name":"id","type":"int","required":true}]}],"current-schema-id":1,"default-spec-id":0,"last-partition-id":0,"default-sort-order-id":0}})",
- .expected_error_message = "type must be string, but is number"},
+ .expected_error_message =
+ "Failed to parse 'metadata-location' (JSON value of type number)"},
// Wrong type for config field
LoadTableResultInvalidParam{
.test_name = "WrongConfigType",
.invalid_json_str =
R"({"config":"invalid","metadata":{"format-version":2,"table-uuid":"test","location":"s3://test","last-sequence-number":0,"last-column-id":1,"last-updated-ms":0,"schemas":[{"type":"struct","schema-id":1,"fields":[{"id":1,"name":"id","type":"int","required":true}]}],"current-schema-id":1,"partition-specs":[{"spec-id":0,"fields":[]}],"default-spec-id":0,"last-partition-id":0,"sort-orders":[{"order-id":0,"fields":[]}],"default-sort-order-id":0}})",
- .expected_error_message = "type must be object, but is string"},
+ .expected_error_message =
+ "Failed to parse 'config' (JSON value of type string)"},
// Invalid metadata content
LoadTableResultInvalidParam{
.test_name = "InvalidMetadataContent",
.invalid_json_str = R"({"metadata":{"format-version":"invalid"}})",
- .expected_error_message = "type must be number, but is string"},
+ .expected_error_message =
+ "Failed to parse 'format-version' (JSON value of type string)"},
LoadTableResultInvalidParam{
.test_name = "LabelsFieldsNotArray",
.invalid_json_str = LoadTableJsonWithLabels(R"({"fields":"oops"})"),
@@ -1394,7 +1414,8 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "InvalidIdentifierNameType",
.invalid_json_str =
R"({"identifier":{"namespace":["ns1"],"name":23},"requirements":[],"updates":[]})",
- .expected_error_message = "type must be string, but is number"},
+ .expected_error_message =
+ "Failed to parse 'name' (JSON value of type number)"},
// Invalid requirements - non-object value in requirements array
CommitTableRequestInvalidParam{
.test_name = "InvalidRequirementsNonObject",
@@ -1506,7 +1527,8 @@ INSTANTIATE_TEST_SUITE_P(
.test_name = "WrongMetadataLocationType",
.invalid_json_str =
R"({"metadata-location":123,"metadata":{"format-version":2,"table-uuid":"test","location":"s3://test","last-sequence-number":0,"last-column-id":1,"last-updated-ms":0,"schemas":[{"type":"struct","schema-id":1,"fields":[{"id":1,"name":"id","type":"int","required":true}]}],"current-schema-id":1,"partition-specs":[{"spec-id":0,"fields":[]}],"default-spec-id":0,"last-partition-id":0,"sort-orders":[{"order-id":0,"fields":[]}],"default-sort-order-id":0}})",
- .expected_error_message = "type must be string, but is number"},
+ .expected_error_message =
+ "Failed to parse 'metadata-location' (JSON value of type number)"},
// Wrong type for metadata field
CommitTableResponseInvalidParam{
.test_name = "WrongMetadataType",
@@ -2850,12 +2872,16 @@ INSTANTIATE_TEST_SUITE_P(
FieldLabelInvalidParam{.test_name = "NullLabels",
.invalid_json_str = R"({"field-id":1,"labels":null})",
.expected_error_message = "Missing 'labels'"},
- FieldLabelInvalidParam{.test_name = "LabelsNotObject",
- .invalid_json_str = R"({"field-id":1,"labels":"x"})",
- .expected_error_message = "type must be object"},
- FieldLabelInvalidParam{.test_name = "NonStringLabelValue",
- .invalid_json_str = R"({"field-id":1,"labels":{"k":1}})",
- .expected_error_message = "type must be string"}),
+ FieldLabelInvalidParam{
+ .test_name = "LabelsNotObject",
+ .invalid_json_str = R"({"field-id":1,"labels":"x"})",
+ .expected_error_message =
+ "Failed to parse 'labels' (JSON value of type string)"},
+ FieldLabelInvalidParam{
+ .test_name = "NonStringLabelValue",
+ .invalid_json_str = R"({"field-id":1,"labels":{"k":1}})",
+ .expected_error_message =
+ "Failed to parse 'labels' (JSON value of type object)"}),
[](const ::testing::TestParamInfo<FieldLabelInvalidParam>& info) {
return info.param.test_name;
});
@@ -2919,15 +2945,21 @@ DECLARE_INVALID_TEST(Labels)
INSTANTIATE_TEST_SUITE_P(
LabelsInvalidCases, LabelsInvalidTest,
::testing::Values(
- LabelsInvalidParam{.test_name = "ObjectLabelsNotObject",
- .invalid_json_str = R"({"object-labels":"x"})",
- .expected_error_message = "type must be object"},
- LabelsInvalidParam{.test_name = "ObjectLabelsNonStringValue",
- .invalid_json_str = R"({"object-labels":{"k":1}})",
- .expected_error_message = "type must be string"},
- LabelsInvalidParam{.test_name = "ObjectLabelsNullValue",
- .invalid_json_str = R"({"object-labels":{"k":null}})",
- .expected_error_message = "type must be string"},
+ LabelsInvalidParam{
+ .test_name = "ObjectLabelsNotObject",
+ .invalid_json_str = R"({"object-labels":"x"})",
+ .expected_error_message =
+ "Failed to parse 'object-labels' (JSON value of type string)"},
+ LabelsInvalidParam{
+ .test_name = "ObjectLabelsNonStringValue",
+ .invalid_json_str = R"({"object-labels":{"k":1}})",
+ .expected_error_message =
+ "Failed to parse 'object-labels' (JSON value of type object)"},
+ LabelsInvalidParam{
+ .test_name = "ObjectLabelsNullValue",
+ .invalid_json_str = R"({"object-labels":{"k":null}})",
+ .expected_error_message =
+ "Failed to parse 'object-labels' (JSON value of type object)"},
LabelsInvalidParam{
.test_name = "FieldsNotArray",
.invalid_json_str = R"({"fields":{}})",
@@ -2948,4 +2980,50 @@ INSTANTIATE_TEST_SUITE_P(
return info.param.test_name;
});
+// Parse errors describe the offending key and JSON type; they must not
+// reproduce JSON values from the response.
+TEST(ParseErrorMessageTest, LoadTableResultErrorOmitsValues) {
+ constexpr std::string_view kConfigValue = "config-value-1234";
+ constexpr std::string_view kCredentialValue = "credential-value-5678";
+ nlohmann::json without_metadata = {
+ {"metadata-location", "s3://bucket/metadata/v1.json"},
+ {"config", {{"s3.secret-access-key", kConfigValue}}},
+ {"storage-credentials",
+ nlohmann::json::array(
+ {{{"prefix", "s3"}, {"config", {{"s3.session-token", kCredentialValue}}}}})}};
+ nlohmann::json mistyped_metadata = without_metadata;
+ mistyped_metadata["metadata"] = 1;
+
+ for (const auto& json : {without_metadata, mistyped_metadata}) {
+ auto result = LoadTableResultFromJson(json);
+ ASSERT_THAT(result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(result, HasErrorMessage("metadata"));
+ EXPECT_THAT(result, ::testing::Not(HasErrorMessage(kConfigValue)));
+ EXPECT_THAT(result, ::testing::Not(HasErrorMessage(kCredentialValue)));
+ }
+}
+
+TEST(ParseErrorMessageTest, OAuthTokenResponseErrorOmitsValues) {
+ constexpr std::string_view kAccessToken = "access-token-1234";
+ constexpr std::string_view kRefreshToken = "refresh-token-5678";
+ nlohmann::json json = {{"access_token", kAccessToken},
+ {"token_type", "bearer"},
+ {"expires_in", "3600"},
+ {"refresh_token", kRefreshToken}};
+
+ auto result = OAuthTokenResponseFromJson(json);
+ ASSERT_THAT(result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(result, HasErrorMessage("'expires_in'"));
+ EXPECT_THAT(result, HasErrorMessage("string"));
+ EXPECT_THAT(result, ::testing::Not(HasErrorMessage(kAccessToken)));
+ EXPECT_THAT(result, ::testing::Not(HasErrorMessage(kRefreshToken)));
+}
+
+TEST(ParseErrorMessageTest, InvalidJsonStringIsNotEchoed) {
+ constexpr std::string_view kValue = "access-token-1234";
+ auto result = FromJsonString(std::format(R"({{"access_token":"{}",)", kValue));
+ ASSERT_THAT(result, IsError(ErrorKind::kJsonParseError));
+ EXPECT_THAT(result, ::testing::Not(HasErrorMessage(kValue)));
+}
+
} // namespace iceberg::rest
diff --git a/src/iceberg/util/json_util_internal.h b/src/iceberg/util/json_util_internal.h
index b1edf583..adb6b2fa 100644
--- a/src/iceberg/util/json_util_internal.h
+++ b/src/iceberg/util/json_util_internal.h
@@ -57,6 +57,10 @@ inline void SetOptionalStringField(nlohmann::json& json, std::string_view key,
}
}
+/// \brief Dump a JSON document to a string, ignoring encoding errors.
+///
+/// Not intended for error messages: parse errors describe the offending key
+/// and JSON type instead of reproducing JSON values.
inline std::string SafeDumpJson(const nlohmann::json& json) {
return json.dump(/*indent=*/-1, /*indent_char=*/' ', /*ensure_ascii=*/false,
nlohmann::detail::error_handler_t::ignore);
@@ -66,8 +70,8 @@ template <typename T>
Result<T> GetTypedJsonValue(const nlohmann::json& json) {
try {
return json.get<T>();
- } catch (const std::exception& ex) {
- return JsonParseError("Failed to parse {}: {}", SafeDumpJson(json), ex.what());
+ } catch (const std::exception&) {
+ return JsonParseError("Failed to parse JSON value of type {}", json.type_name());
}
}
@@ -75,9 +79,10 @@ template <typename T>
Result<T> GetJsonValueImpl(const nlohmann::json& json, std::string_view key) {
try {
return json.at(key).get<T>();
- } catch (const std::exception& ex) {
- return JsonParseError("Failed to parse '{}' from {}: {}", key, SafeDumpJson(json),
- ex.what());
+ } catch (const std::exception&) {
+ // Report the key and the JSON type found, not the enclosing document.
+ return JsonParseError("Failed to parse '{}' (JSON value of type {})", key,
+ json.contains(key) ? json.at(key).type_name() : "missing");
}
}
@@ -94,7 +99,7 @@ Result<std::optional<T>> GetJsonValueOptional(const nlohmann::json& json,
template <typename T>
Result<T> GetJsonValue(const nlohmann::json& json, std::string_view key) {
if (!json.contains(key) || json.at(key).is_null()) {
- return JsonParseError("Missing '{}' in {}", key, SafeDumpJson(json));
+ return JsonParseError("Missing '{}' in JSON object", key);
}
return GetJsonValueImpl<T>(json, key);
}
@@ -123,8 +128,7 @@ Result<Integer> IntegerFromJson(const nlohmann::json& json,
if (json.is_number_unsigned()) {
const auto value = json.get<uint64_t>();
if (value > static_cast<uint64_t>(std::numeric_limits<Integer>::max())) {
- return JsonParseError("{} integer is out of range: {}", description,
- SafeDumpJson(json));
+ return JsonParseError("{} integer is out of range", description);
}
return static_cast<Integer>(value);
}
@@ -132,8 +136,7 @@ Result<Integer> IntegerFromJson(const nlohmann::json& json,
const auto value = json.get<int64_t>();
if (value < static_cast<int64_t>(std::numeric_limits<Integer>::min()) ||
value > static_cast<int64_t>(std::numeric_limits<Integer>::max())) {
- return JsonParseError("{} integer is out of range: {}", description,
- SafeDumpJson(json));
+ return JsonParseError("{} integer is out of range", description);
}
return static_cast<Integer>(value);
}
@@ -225,8 +228,8 @@ Result<std::vector<T>> FromJsonList(
if (json.contains(key)) {
ICEBERG_ASSIGN_OR_RAISE(auto list_json, GetJsonValue<nlohmann::json>(json, key));
if (!list_json.is_array()) {
- return JsonParseError("Cannot parse '{}' from non-array: {}", key,
- SafeDumpJson(list_json));
+ return JsonParseError("Cannot parse '{}' from non-array JSON value of type {}", key,
+ list_json.type_name());
}
for (const auto& entry_json : list_json) {
ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(entry_json));
@@ -250,8 +253,8 @@ Result<std::vector<std::shared_ptr<T>>> FromJsonList(
if (json.contains(key)) {
ICEBERG_ASSIGN_OR_RAISE(auto list_json, GetJsonValue<nlohmann::json>(json, key));
if (!list_json.is_array()) {
- return JsonParseError("Cannot parse '{}' from non-array: {}", key,
- SafeDumpJson(list_json));
+ return JsonParseError("Cannot parse '{}' from non-array JSON value of type {}", key,
+ list_json.type_name());
}
for (const auto& entry_json : list_json) {
ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(entry_json));
@@ -299,17 +302,17 @@ Result<std::unordered_map<std::string, T>> FromJsonMap(
static_assert(std::is_same_v<T, std::string>, "T must be std::string");
try {
return json.get<std::string>();
- } catch (const std::exception& ex) {
- return JsonParseError("Cannot parse {} to a string value: {}", SafeDumpJson(json),
- ex.what());
+ } catch (const std::exception&) {
+ return JsonParseError("Cannot parse JSON value of type {} to a string value",
+ json.type_name());
}
}) {
std::unordered_map<std::string, T> map{};
if (json.contains(key)) {
ICEBERG_ASSIGN_OR_RAISE(auto map_json, GetJsonValue<nlohmann::json>(json, key));
if (!map_json.is_object()) {
- return JsonParseError("Cannot parse '{}' from non-object: {}", key,
- SafeDumpJson(map_json));
+ return JsonParseError("Cannot parse '{}' from non-object JSON value of type {}",
+ key, map_json.type_name());
}
for (const auto& [key, value] : map_json.items()) {
ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(value));