Skip to content

Commit b252b72

Browse files
committed
refactor(serde): return Result from ToJson for schema/type/metadata serializers
Column default values make single-value (Literal) serialization fallible, so the schema/type serializers must propagate errors instead of throwing. Change ToJson for SchemaField, Type, Schema, TableMetadata and TableUpdate (plus the REST CreateTableRequest/CommitTableRequest/LoadTableResult/CommitTableResponse, which embed them) to return Result<nlohmann::json>, replacing the ICEBERG_ASSIGN_OR_THROW in SchemaField default serialization with ICEBERG_ASSIGN_OR_RAISE. - The shared ToJsonList template (also used by infallible serializers) is left unchanged; TableMetadata serializes its schema list with an explicit loop. - REST adds an ICEBERG_DECLARE_JSON_SERDE_FALLIBLE macro variant for the four models that embed a schema/metadata; the other REST models keep the bare-json ToJson. - Callers bottom out at existing Result boundaries (ToJsonString wrappers, rest_catalog.cc, TableMetadataUtil::Write). - Tests unwrap via ICEBERG_UNWRAP_OR_FAIL.
1 parent 0b4d452 commit b252b72

9 files changed

Lines changed: 143 additions & 82 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: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,16 @@ Result<Model> FromJson(const nlohmann::json& json);
4646
\
4747
ICEBERG_REST_EXPORT nlohmann::json ToJson(const Model& model);
4848

49+
// Same as ICEBERG_DECLARE_JSON_SERDE, but ToJson returns Result because the model
50+
// embeds a Schema/TableMetadata whose default-value serialization can fail.
51+
#define ICEBERG_DECLARE_JSON_SERDE_FALLIBLE(Model) \
52+
ICEBERG_REST_EXPORT Result<Model> Model##FromJson(const nlohmann::json& json); \
53+
\
54+
template <> \
55+
ICEBERG_REST_EXPORT Result<Model> FromJson(const nlohmann::json& json); \
56+
\
57+
ICEBERG_REST_EXPORT Result<nlohmann::json> ToJson(const Model& model);
58+
4959
/// \note Don't forget to add `ICEBERG_DEFINE_FROM_JSON` to the end of
5060
/// `json_internal.cc` to define the `FromJson` function for the model.
5161
ICEBERG_DECLARE_JSON_SERDE(CatalogConfig)
@@ -57,15 +67,16 @@ ICEBERG_DECLARE_JSON_SERDE(GetNamespaceResponse)
5767
ICEBERG_DECLARE_JSON_SERDE(UpdateNamespacePropertiesRequest)
5868
ICEBERG_DECLARE_JSON_SERDE(UpdateNamespacePropertiesResponse)
5969
ICEBERG_DECLARE_JSON_SERDE(ListTablesResponse)
60-
ICEBERG_DECLARE_JSON_SERDE(LoadTableResult)
70+
ICEBERG_DECLARE_JSON_SERDE_FALLIBLE(LoadTableResult)
6171
ICEBERG_DECLARE_JSON_SERDE(RegisterTableRequest)
6272
ICEBERG_DECLARE_JSON_SERDE(RenameTableRequest)
63-
ICEBERG_DECLARE_JSON_SERDE(CreateTableRequest)
64-
ICEBERG_DECLARE_JSON_SERDE(CommitTableRequest)
65-
ICEBERG_DECLARE_JSON_SERDE(CommitTableResponse)
73+
ICEBERG_DECLARE_JSON_SERDE_FALLIBLE(CreateTableRequest)
74+
ICEBERG_DECLARE_JSON_SERDE_FALLIBLE(CommitTableRequest)
75+
ICEBERG_DECLARE_JSON_SERDE_FALLIBLE(CommitTableResponse)
6676
ICEBERG_DECLARE_JSON_SERDE(OAuthTokenResponse)
6777

6878
#undef ICEBERG_DECLARE_JSON_SERDE
79+
#undef ICEBERG_DECLARE_JSON_SERDE_FALLIBLE
6980

7081
ICEBERG_REST_EXPORT Result<PlanTableScanResponse> PlanTableScanResponseFromJson(
7182
const nlohmann::json& json,

src/iceberg/catalog/rest/rest_catalog.cc

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

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

712-
ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request)));
713+
ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request));
714+
ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json));
713715
ICEBERG_ASSIGN_OR_RAISE(const auto response,
714716
client_->Post(path, json_request, /*headers=*/{},
715717
*TableErrorHandler::Instance(), session));

src/iceberg/json_serde.cc

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -301,36 +301,35 @@ Result<std::unique_ptr<SortOrder>> SortOrderFromJson(const nlohmann::json& json)
301301
return SortOrder::Make(order_id, std::move(sort_fields));
302302
}
303303

304-
nlohmann::json ToJson(const SchemaField& field) {
304+
Result<nlohmann::json> ToJson(const SchemaField& field) {
305305
nlohmann::json json;
306306
json[kId] = field.field_id();
307307
json[kName] = field.name();
308308
json[kRequired] = !field.optional();
309-
json[kType] = ToJson(*field.type());
309+
ICEBERG_ASSIGN_OR_RAISE(json[kType], ToJson(*field.type()));
310310
if (!field.doc().empty()) {
311311
json[kDoc] = field.doc();
312312
}
313-
// Defaults are validated to be primitive literals matching the field type, so
314-
// single-value serialization cannot fail here.
315313
if (field.initial_default().has_value()) {
316-
ICEBERG_ASSIGN_OR_THROW(json[kInitialDefault],
314+
ICEBERG_ASSIGN_OR_RAISE(json[kInitialDefault],
317315
ToJson(field.initial_default()->get()));
318316
}
319317
if (field.write_default().has_value()) {
320-
ICEBERG_ASSIGN_OR_THROW(json[kWriteDefault], ToJson(field.write_default()->get()));
318+
ICEBERG_ASSIGN_OR_RAISE(json[kWriteDefault], ToJson(field.write_default()->get()));
321319
}
322320
return json;
323321
}
324322

325-
nlohmann::json ToJson(const Type& type) {
323+
Result<nlohmann::json> ToJson(const Type& type) {
326324
switch (type.type_id()) {
327325
case TypeId::kStruct: {
328326
const auto& struct_type = internal::checked_cast<const StructType&>(type);
329327
nlohmann::json json;
330328
json[kType] = kStruct;
331329
nlohmann::json fields_json = nlohmann::json::array();
332330
for (const auto& field : struct_type.fields()) {
333-
fields_json.push_back(ToJson(field));
331+
ICEBERG_ASSIGN_OR_RAISE(auto field_json, ToJson(field));
332+
fields_json.push_back(std::move(field_json));
334333
}
335334
json[kFields] = fields_json;
336335
return json;
@@ -343,7 +342,7 @@ nlohmann::json ToJson(const Type& type) {
343342
const auto& element_field = list_type.fields().front();
344343
json[kElementId] = element_field.field_id();
345344
json[kElementRequired] = !element_field.optional();
346-
json[kElement] = ToJson(*element_field.type());
345+
ICEBERG_ASSIGN_OR_RAISE(json[kElement], ToJson(*element_field.type()));
347346
return json;
348347
}
349348
case TypeId::kMap: {
@@ -353,12 +352,12 @@ nlohmann::json ToJson(const Type& type) {
353352

354353
const auto& key_field = map_type.key();
355354
json[kKeyId] = key_field.field_id();
356-
json[kKey] = ToJson(*key_field.type());
355+
ICEBERG_ASSIGN_OR_RAISE(json[kKey], ToJson(*key_field.type()));
357356

358357
const auto& value_field = map_type.value();
359358
json[kValueId] = value_field.field_id();
360359
json[kValueRequired] = !value_field.optional();
361-
json[kValue] = ToJson(*value_field.type());
360+
ICEBERG_ASSIGN_OR_RAISE(json[kValue], ToJson(*value_field.type()));
362361
return json;
363362
}
364363
case TypeId::kBoolean:
@@ -404,8 +403,9 @@ nlohmann::json ToJson(const Type& type) {
404403
std::unreachable();
405404
}
406405

407-
nlohmann::json ToJson(const Schema& schema) {
408-
nlohmann::json json = ToJson(internal::checked_cast<const Type&>(schema));
406+
Result<nlohmann::json> ToJson(const Schema& schema) {
407+
ICEBERG_ASSIGN_OR_RAISE(nlohmann::json json,
408+
ToJson(internal::checked_cast<const Type&>(schema)));
409409
json[kSchemaId] = schema.schema_id();
410410
if (!schema.IdentifierFieldIds().empty()) {
411411
json[kIdentifierFieldIds] = schema.IdentifierFieldIds();
@@ -414,7 +414,8 @@ nlohmann::json ToJson(const Schema& schema) {
414414
}
415415

416416
Result<std::string> ToJsonString(const Schema& schema) {
417-
return ToJsonString(ToJson(schema));
417+
ICEBERG_ASSIGN_OR_RAISE(auto json, ToJson(schema));
418+
return ToJsonString(json);
418419
}
419420

420421
nlohmann::json ToJson(const SnapshotRef& ref) {
@@ -956,7 +957,7 @@ Result<EncryptedKey> EncryptedKeyFromJson(const nlohmann::json& json) {
956957
};
957958
}
958959

959-
nlohmann::json ToJson(const TableMetadata& table_metadata) {
960+
Result<nlohmann::json> ToJson(const TableMetadata& table_metadata) {
960961
nlohmann::json json;
961962

962963
json[kFormatVersion] = table_metadata.format_version;
@@ -974,15 +975,22 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) {
974975
if (table_metadata.format_version == 1) {
975976
for (const auto& schema : table_metadata.schemas) {
976977
if (schema->schema_id() == table_metadata.current_schema_id) {
977-
json[kSchema] = ToJson(*schema);
978+
ICEBERG_ASSIGN_OR_RAISE(json[kSchema], ToJson(*schema));
978979
break;
979980
}
980981
}
981982
}
982983

983984
// write the current schema ID and schema list
984985
json[kCurrentSchemaId] = table_metadata.current_schema_id;
985-
json[kSchemas] = ToJsonList(table_metadata.schemas);
986+
// Schemas can carry fallible default-value serialization, so the shared ToJsonList
987+
// helper (which assumes infallible ToJson) is not used here.
988+
nlohmann::json schemas_json = nlohmann::json::array();
989+
for (const auto& schema : table_metadata.schemas) {
990+
ICEBERG_ASSIGN_OR_RAISE(auto schema_json, ToJson(*schema));
991+
schemas_json.push_back(std::move(schema_json));
992+
}
993+
json[kSchemas] = std::move(schemas_json);
986994

987995
// for older readers, continue writing the default spec as "partition-spec"
988996
if (table_metadata.format_version == 1) {
@@ -1032,7 +1040,8 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) {
10321040
}
10331041

10341042
Result<std::string> ToJsonString(const TableMetadata& table_metadata) {
1035-
return ToJsonString(ToJson(table_metadata));
1043+
ICEBERG_ASSIGN_OR_RAISE(auto json, ToJson(table_metadata));
1044+
return ToJsonString(json);
10361045
}
10371046

10381047
namespace {
@@ -1435,7 +1444,7 @@ Result<Namespace> NamespaceFromJson(const nlohmann::json& json) {
14351444
return ns;
14361445
}
14371446

1438-
nlohmann::json ToJson(const TableUpdate& update) {
1447+
Result<nlohmann::json> ToJson(const TableUpdate& update) {
14391448
nlohmann::json json;
14401449
switch (update.kind()) {
14411450
case TableUpdate::Kind::kAssignUUID: {
@@ -1454,7 +1463,7 @@ nlohmann::json ToJson(const TableUpdate& update) {
14541463
const auto& u = internal::checked_cast<const table::AddSchema&>(update);
14551464
json[kAction] = kActionAddSchema;
14561465
if (u.schema()) {
1457-
json[kSchema] = ToJson(*u.schema());
1466+
ICEBERG_ASSIGN_OR_RAISE(json[kSchema], ToJson(*u.schema()));
14581467
} else {
14591468
json[kSchema] = nlohmann::json::value_t::null;
14601469
}

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)