Skip to content

Commit a657a48

Browse files
committed
refactor(serde): return Result from ToJson schema/type/metadata serializers
Single-value (Literal) serialization can fail, and upcoming v3 column default values (#730) invoke it from schema serialization. Switch the schema/type/metadata ToJson serializers (and the REST request/response serializers that embed them) from returning bare nlohmann::json to Result<nlohmann::json>, propagating errors via ICEBERG_ASSIGN_OR_RAISE instead of forcing callers to throw. Behavior is unchanged: every conversion still succeeds today; this only changes the return type so the column-defaults PR can serialize default values without throwing. - ToJson for SchemaField/Type/Schema/TableMetadata/TableUpdate, plus REST CreateTableRequest/CommitTableRequest/LoadTableResult/CommitTableResponse. - The shared ToJsonList template stays bare (also used by infallible types); TableMetadata serializes its schema list with an explicit loop. - Callers bottom out at existing Result boundaries (ToJsonString wrappers, rest_catalog.cc, TableMetadataUtil::Write).
1 parent 9a7439c commit a657a48

9 files changed

Lines changed: 120 additions & 67 deletions

File tree

src/iceberg/catalog/rest/json_serde.cc

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -690,10 +690,10 @@ Result<RenameTableRequest> RenameTableRequestFromJson(const nlohmann::json& json
690690
}
691691

692692
// LoadTableResult (used by CreateTableResponse, LoadTableResponse)
693-
nlohmann::json ToJson(const LoadTableResult& result) {
693+
Result<nlohmann::json> ToJson(const LoadTableResult& result) {
694694
nlohmann::json json;
695695
SetOptionalStringField(json, kMetadataLocation, result.metadata_location);
696-
json[kMetadata] = ToJson(*result.metadata);
696+
ICEBERG_ASSIGN_OR_RAISE(json[kMetadata], ToJson(*result.metadata));
697697
SetContainerField(json, kConfig, result.config);
698698
return json;
699699
}
@@ -820,12 +820,12 @@ Result<ListTablesResponse> ListTablesResponseFromJson(const nlohmann::json& json
820820
return response;
821821
}
822822

823-
nlohmann::json ToJson(const CreateTableRequest& request) {
823+
Result<nlohmann::json> ToJson(const CreateTableRequest& request) {
824824
nlohmann::json json;
825825
json[kName] = request.name;
826826
SetOptionalStringField(json, kLocation, request.location);
827827
if (request.schema) {
828-
json[kSchema] = ToJson(*request.schema);
828+
ICEBERG_ASSIGN_OR_RAISE(json[kSchema], ToJson(*request.schema));
829829
}
830830
if (request.partition_spec) {
831831
json[kPartitionSpec] = ToJson(*request.partition_spec);
@@ -872,7 +872,7 @@ Result<CreateTableRequest> CreateTableRequestFromJson(const nlohmann::json& json
872872
}
873873

874874
// CommitTableRequest serialization
875-
nlohmann::json ToJson(const CommitTableRequest& request) {
875+
Result<nlohmann::json> ToJson(const CommitTableRequest& request) {
876876
nlohmann::json json;
877877
if (!request.identifier.name.empty()) {
878878
json[kIdentifier] = ToJson(request.identifier);
@@ -886,7 +886,8 @@ nlohmann::json ToJson(const CommitTableRequest& request) {
886886

887887
nlohmann::json updates_json = nlohmann::json::array();
888888
for (const auto& update : request.updates) {
889-
updates_json.push_back(ToJson(*update));
889+
ICEBERG_ASSIGN_OR_RAISE(auto update_json, ToJson(*update));
890+
updates_json.push_back(std::move(update_json));
890891
}
891892
json[kUpdates] = std::move(updates_json);
892893

@@ -932,11 +933,11 @@ Result<CommitTableRequest> CommitTableRequestFromJson(const nlohmann::json& json
932933
}
933934

934935
// CommitTableResponse serialization
935-
nlohmann::json ToJson(const CommitTableResponse& response) {
936+
Result<nlohmann::json> ToJson(const CommitTableResponse& response) {
936937
nlohmann::json json;
937938
json[kMetadataLocation] = response.metadata_location;
938939
if (response.metadata) {
939-
json[kMetadata] = ToJson(*response.metadata);
940+
ICEBERG_ASSIGN_OR_RAISE(json[kMetadata], ToJson(*response.metadata));
940941
}
941942
return json;
942943
}

src/iceberg/catalog/rest/json_serde_internal.h

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,16 +57,38 @@ ICEBERG_DECLARE_JSON_SERDE(GetNamespaceResponse)
5757
ICEBERG_DECLARE_JSON_SERDE(UpdateNamespacePropertiesRequest)
5858
ICEBERG_DECLARE_JSON_SERDE(UpdateNamespacePropertiesResponse)
5959
ICEBERG_DECLARE_JSON_SERDE(ListTablesResponse)
60-
ICEBERG_DECLARE_JSON_SERDE(LoadTableResult)
6160
ICEBERG_DECLARE_JSON_SERDE(RegisterTableRequest)
6261
ICEBERG_DECLARE_JSON_SERDE(RenameTableRequest)
63-
ICEBERG_DECLARE_JSON_SERDE(CreateTableRequest)
64-
ICEBERG_DECLARE_JSON_SERDE(CommitTableRequest)
65-
ICEBERG_DECLARE_JSON_SERDE(CommitTableResponse)
6662
ICEBERG_DECLARE_JSON_SERDE(OAuthTokenResponse)
6763

6864
#undef ICEBERG_DECLARE_JSON_SERDE
6965

66+
// These models embed a Schema/TableMetadata whose ToJson returns Result, so their own
67+
// ToJson returns Result too. FromJson is declared like the macro-based models above.
68+
ICEBERG_REST_EXPORT Result<LoadTableResult> LoadTableResultFromJson(
69+
const nlohmann::json& json);
70+
template <>
71+
ICEBERG_REST_EXPORT Result<LoadTableResult> FromJson(const nlohmann::json& json);
72+
ICEBERG_REST_EXPORT Result<nlohmann::json> ToJson(const LoadTableResult& model);
73+
74+
ICEBERG_REST_EXPORT Result<CreateTableRequest> CreateTableRequestFromJson(
75+
const nlohmann::json& json);
76+
template <>
77+
ICEBERG_REST_EXPORT Result<CreateTableRequest> FromJson(const nlohmann::json& json);
78+
ICEBERG_REST_EXPORT Result<nlohmann::json> ToJson(const CreateTableRequest& model);
79+
80+
ICEBERG_REST_EXPORT Result<CommitTableRequest> CommitTableRequestFromJson(
81+
const nlohmann::json& json);
82+
template <>
83+
ICEBERG_REST_EXPORT Result<CommitTableRequest> FromJson(const nlohmann::json& json);
84+
ICEBERG_REST_EXPORT Result<nlohmann::json> ToJson(const CommitTableRequest& model);
85+
86+
ICEBERG_REST_EXPORT Result<CommitTableResponse> CommitTableResponseFromJson(
87+
const nlohmann::json& json);
88+
template <>
89+
ICEBERG_REST_EXPORT Result<CommitTableResponse> FromJson(const nlohmann::json& json);
90+
ICEBERG_REST_EXPORT Result<nlohmann::json> ToJson(const CommitTableResponse& model);
91+
7092
ICEBERG_REST_EXPORT Result<PlanTableScanResponse> PlanTableScanResponseFromJson(
7193
const nlohmann::json& json,
7294
const std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>>&

src/iceberg/catalog/rest/rest_catalog.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -667,7 +667,8 @@ Result<LoadTableResult> RestCatalog::CreateTableInternal(
667667
.properties = properties,
668668
};
669669

670-
ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request)));
670+
ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request));
671+
ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json));
671672
ICEBERG_ASSIGN_OR_RAISE(const auto response,
672673
client_->Post(path, json_request, /*headers=*/{},
673674
*TableErrorHandler::Instance(), session));
@@ -710,7 +711,8 @@ Result<CommitTableResponse> RestCatalog::UpdateTableInternal(
710711
request.updates.push_back(update->Clone());
711712
}
712713

713-
ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request)));
714+
ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request));
715+
ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json));
714716
ICEBERG_ASSIGN_OR_RAISE(auto is_create, TableRequirements::IsCreate(requirements));
715717
const ErrorHandler* error_handler = TableCommitErrorHandler::Instance().get();
716718
if (is_create) {

src/iceberg/json_serde.cc

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -315,27 +315,28 @@ Result<std::unique_ptr<SortOrder>> SortOrderFromJson(const nlohmann::json& json)
315315
return SortOrder::Make(parsed.order_id, std::move(parsed.fields));
316316
}
317317

318-
nlohmann::json ToJson(const SchemaField& field) {
318+
Result<nlohmann::json> ToJson(const SchemaField& field) {
319319
nlohmann::json json;
320320
json[kId] = field.field_id();
321321
json[kName] = field.name();
322322
json[kRequired] = !field.optional();
323-
json[kType] = ToJson(*field.type());
323+
ICEBERG_ASSIGN_OR_RAISE(json[kType], ToJson(*field.type()));
324324
if (!field.doc().empty()) {
325325
json[kDoc] = field.doc();
326326
}
327327
return json;
328328
}
329329

330-
nlohmann::json ToJson(const Type& type) {
330+
Result<nlohmann::json> ToJson(const Type& type) {
331331
switch (type.type_id()) {
332332
case TypeId::kStruct: {
333333
const auto& struct_type = internal::checked_cast<const StructType&>(type);
334334
nlohmann::json json;
335335
json[kType] = kStruct;
336336
nlohmann::json fields_json = nlohmann::json::array();
337337
for (const auto& field : struct_type.fields()) {
338-
fields_json.push_back(ToJson(field));
338+
ICEBERG_ASSIGN_OR_RAISE(auto field_json, ToJson(field));
339+
fields_json.push_back(std::move(field_json));
339340
// TODO(gangwu): add default values
340341
}
341342
json[kFields] = fields_json;
@@ -349,7 +350,7 @@ nlohmann::json ToJson(const Type& type) {
349350
const auto& element_field = list_type.fields().front();
350351
json[kElementId] = element_field.field_id();
351352
json[kElementRequired] = !element_field.optional();
352-
json[kElement] = ToJson(*element_field.type());
353+
ICEBERG_ASSIGN_OR_RAISE(json[kElement], ToJson(*element_field.type()));
353354
return json;
354355
}
355356
case TypeId::kMap: {
@@ -359,12 +360,12 @@ nlohmann::json ToJson(const Type& type) {
359360

360361
const auto& key_field = map_type.key();
361362
json[kKeyId] = key_field.field_id();
362-
json[kKey] = ToJson(*key_field.type());
363+
ICEBERG_ASSIGN_OR_RAISE(json[kKey], ToJson(*key_field.type()));
363364

364365
const auto& value_field = map_type.value();
365366
json[kValueId] = value_field.field_id();
366367
json[kValueRequired] = !value_field.optional();
367-
json[kValue] = ToJson(*value_field.type());
368+
ICEBERG_ASSIGN_OR_RAISE(json[kValue], ToJson(*value_field.type()));
368369
return json;
369370
}
370371
case TypeId::kBoolean:
@@ -416,8 +417,9 @@ nlohmann::json ToJson(const Type& type) {
416417
std::unreachable();
417418
}
418419

419-
nlohmann::json ToJson(const Schema& schema) {
420-
nlohmann::json json = ToJson(internal::checked_cast<const Type&>(schema));
420+
Result<nlohmann::json> ToJson(const Schema& schema) {
421+
ICEBERG_ASSIGN_OR_RAISE(nlohmann::json json,
422+
ToJson(internal::checked_cast<const Type&>(schema)));
421423
json[kSchemaId] = schema.schema_id();
422424
if (!schema.IdentifierFieldIds().empty()) {
423425
json[kIdentifierFieldIds] = schema.IdentifierFieldIds();
@@ -426,7 +428,8 @@ nlohmann::json ToJson(const Schema& schema) {
426428
}
427429

428430
Result<std::string> ToJsonString(const Schema& schema) {
429-
return ToJsonString(ToJson(schema));
431+
ICEBERG_ASSIGN_OR_RAISE(auto json, ToJson(schema));
432+
return ToJsonString(json);
430433
}
431434

432435
nlohmann::json ToJson(const SnapshotRef& ref) {
@@ -966,7 +969,7 @@ Result<EncryptedKey> EncryptedKeyFromJson(const nlohmann::json& json) {
966969
};
967970
}
968971

969-
nlohmann::json ToJson(const TableMetadata& table_metadata) {
972+
Result<nlohmann::json> ToJson(const TableMetadata& table_metadata) {
970973
nlohmann::json json;
971974

972975
json[kFormatVersion] = table_metadata.format_version;
@@ -984,15 +987,22 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) {
984987
if (table_metadata.format_version == 1) {
985988
for (const auto& schema : table_metadata.schemas) {
986989
if (schema->schema_id() == table_metadata.current_schema_id) {
987-
json[kSchema] = ToJson(*schema);
990+
ICEBERG_ASSIGN_OR_RAISE(json[kSchema], ToJson(*schema));
988991
break;
989992
}
990993
}
991994
}
992995

993996
// write the current schema ID and schema list
994997
json[kCurrentSchemaId] = table_metadata.current_schema_id;
995-
json[kSchemas] = ToJsonList(table_metadata.schemas);
998+
// ToJson(Schema) is fallible, so the shared ToJsonList helper (which assumes an
999+
// infallible ToJson) cannot be used here; build the array with an explicit loop.
1000+
nlohmann::json schemas_json = nlohmann::json::array();
1001+
for (const auto& schema : table_metadata.schemas) {
1002+
ICEBERG_ASSIGN_OR_RAISE(auto schema_json, ToJson(*schema));
1003+
schemas_json.push_back(std::move(schema_json));
1004+
}
1005+
json[kSchemas] = std::move(schemas_json);
9961006

9971007
// for older readers, continue writing the default spec as "partition-spec"
9981008
if (table_metadata.format_version == 1) {
@@ -1042,7 +1052,8 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) {
10421052
}
10431053

10441054
Result<std::string> ToJsonString(const TableMetadata& table_metadata) {
1045-
return ToJsonString(ToJson(table_metadata));
1055+
ICEBERG_ASSIGN_OR_RAISE(auto json, ToJson(table_metadata));
1056+
return ToJsonString(json);
10461057
}
10471058

10481059
namespace {
@@ -1446,7 +1457,7 @@ Result<Namespace> NamespaceFromJson(const nlohmann::json& json) {
14461457
return ns;
14471458
}
14481459

1449-
nlohmann::json ToJson(const TableUpdate& update) {
1460+
Result<nlohmann::json> ToJson(const TableUpdate& update) {
14501461
nlohmann::json json;
14511462
switch (update.kind()) {
14521463
case TableUpdate::Kind::kAssignUUID: {
@@ -1465,7 +1476,7 @@ nlohmann::json ToJson(const TableUpdate& update) {
14651476
const auto& u = internal::checked_cast<const table::AddSchema&>(update);
14661477
json[kAction] = kActionAddSchema;
14671478
if (u.schema()) {
1468-
json[kSchema] = ToJson(*u.schema());
1479+
ICEBERG_ASSIGN_OR_RAISE(json[kSchema], ToJson(*u.schema()));
14691480
} else {
14701481
json[kSchema] = nlohmann::json::value_t::null;
14711482
}

src/iceberg/json_serde_internal.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ ICEBERG_EXPORT Result<std::unique_ptr<SortOrder>> SortOrderFromJson(
9393
///
9494
/// \param schema The Iceberg schema to convert.
9595
/// \return The JSON representation of the schema.
96-
ICEBERG_EXPORT nlohmann::json ToJson(const Schema& schema);
96+
ICEBERG_EXPORT Result<nlohmann::json> ToJson(const Schema& schema);
9797

9898
/// \brief Convert an Iceberg Schema to JSON.
9999
///
@@ -111,7 +111,7 @@ ICEBERG_EXPORT Result<std::unique_ptr<Schema>> SchemaFromJson(const nlohmann::js
111111
///
112112
/// \param type The Iceberg type to convert.
113113
/// \return The JSON representation of the type.
114-
ICEBERG_EXPORT nlohmann::json ToJson(const Type& type);
114+
ICEBERG_EXPORT Result<nlohmann::json> ToJson(const Type& type);
115115

116116
/// \brief Convert JSON to an Iceberg Type.
117117
///
@@ -123,7 +123,7 @@ ICEBERG_EXPORT Result<std::unique_ptr<Type>> TypeFromJson(const nlohmann::json&
123123
///
124124
/// \param field The Iceberg field to convert.
125125
/// \return The JSON representation of the field.
126-
ICEBERG_EXPORT nlohmann::json ToJson(const SchemaField& field);
126+
ICEBERG_EXPORT Result<nlohmann::json> ToJson(const SchemaField& field);
127127

128128
/// \brief Convert JSON to an Iceberg SchemaField.
129129
///
@@ -300,7 +300,7 @@ ICEBERG_EXPORT Result<EncryptedKey> EncryptedKeyFromJson(const nlohmann::json& j
300300
///
301301
/// \param table_metadata The `TableMetadata` object to be serialized.
302302
/// \return A JSON object representing the `TableMetadata`.
303-
ICEBERG_EXPORT nlohmann::json ToJson(const TableMetadata& table_metadata);
303+
ICEBERG_EXPORT Result<nlohmann::json> ToJson(const TableMetadata& table_metadata);
304304

305305
/// \brief Serializes a `TableMetadata` object to JSON.
306306
///
@@ -404,7 +404,7 @@ ICEBERG_EXPORT Result<Namespace> NamespaceFromJson(const nlohmann::json& json);
404404
///
405405
/// \param update The `TableUpdate` object to be serialized.
406406
/// \return A JSON object representing the `TableUpdate`.
407-
ICEBERG_EXPORT nlohmann::json ToJson(const TableUpdate& update);
407+
ICEBERG_EXPORT Result<nlohmann::json> ToJson(const TableUpdate& update);
408408

409409
/// \brief Deserializes a JSON object into a `TableUpdate` object.
410410
///

src/iceberg/table_metadata.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -489,7 +489,7 @@ Result<std::string> TableMetadataUtil::Write(FileIO& io, const TableMetadata* ba
489489

490490
Status TableMetadataUtil::Write(FileIO& io, const std::string& location,
491491
const TableMetadata& metadata) {
492-
auto json = ToJson(metadata);
492+
ICEBERG_ASSIGN_OR_RAISE(auto json, ToJson(metadata));
493493
ICEBERG_ASSIGN_OR_RAISE(auto json_string, ToJsonString(json));
494494
return io.WriteFile(location, json_string);
495495
}

0 commit comments

Comments
 (0)