Skip to content

Commit 0dbb593

Browse files
wgtmacwgtmac
authored
refactor: simplify test structure for rest catalog models (#290)
- simplified rest catalog model definitions. - added common functions to operate json ser/de. - refactored cases to be able to be shared.
1 parent 018df25 commit 0dbb593

File tree

5 files changed

+541
-746
lines changed

5 files changed

+541
-746
lines changed

‎src/iceberg/catalog/rest/json_internal.cc‎

Lines changed: 33 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,7 @@ constexpr std::string_view kIdentifiers = "identifiers";
6565
nlohmann::json ToJson(const CreateNamespaceRequest& request) {
6666
nlohmann::json json;
6767
json[kNamespace] = request.namespace_.levels;
68-
if (!request.properties.empty()) {
69-
json[kProperties] = request.properties;
70-
}
68+
SetContainerField(json, kProperties, request.properties);
7169
return json;
7270
}
7371

@@ -83,15 +81,9 @@ Result<CreateNamespaceRequest> CreateNamespaceRequestFromJson(
8381
}
8482

8583
nlohmann::json ToJson(const UpdateNamespacePropertiesRequest& request) {
86-
// Initialize as an empty object so that when all optional fields are absent we return
87-
// {} instead of null
8884
nlohmann::json json = nlohmann::json::object();
89-
if (!request.removals.empty()) {
90-
json[kRemovals] = request.removals;
91-
}
92-
if (!request.updates.empty()) {
93-
json[kUpdates] = request.updates;
94-
}
85+
SetContainerField(json, kRemovals, request.removals);
86+
SetContainerField(json, kUpdates, request.updates);
9587
return json;
9688
}
9789

@@ -145,13 +137,9 @@ Result<RenameTableRequest> RenameTableRequestFromJson(const nlohmann::json& json
145137
// LoadTableResult (used by CreateTableResponse, LoadTableResponse)
146138
nlohmann::json ToJson(const LoadTableResult& result) {
147139
nlohmann::json json;
148-
if (!result.metadata_location.empty()) {
149-
json[kMetadataLocation] = result.metadata_location;
150-
}
140+
SetOptionalStringField(json, kMetadataLocation, result.metadata_location);
151141
json[kMetadata] = ToJson(*result.metadata);
152-
if (!result.config.empty()) {
153-
json[kConfig] = result.config;
154-
}
142+
SetContainerField(json, kConfig, result.config);
155143
return json;
156144
}
157145

@@ -162,17 +150,14 @@ Result<LoadTableResult> LoadTableResultFromJson(const nlohmann::json& json) {
162150
ICEBERG_ASSIGN_OR_RAISE(auto metadata_json,
163151
GetJsonValue<nlohmann::json>(json, kMetadata));
164152
ICEBERG_ASSIGN_OR_RAISE(result.metadata, TableMetadataFromJson(metadata_json));
165-
ICEBERG_ASSIGN_OR_RAISE(
166-
result.config, (GetJsonValueOrDefault<std::unordered_map<std::string, std::string>>(
167-
json, kConfig)));
153+
ICEBERG_ASSIGN_OR_RAISE(result.config,
154+
GetJsonValueOrDefault<decltype(result.config)>(json, kConfig));
168155
return result;
169156
}
170157

171158
nlohmann::json ToJson(const ListNamespacesResponse& response) {
172159
nlohmann::json json;
173-
if (!response.next_page_token.empty()) {
174-
json[kNextPageToken] = response.next_page_token;
175-
}
160+
SetOptionalStringField(json, kNextPageToken, response.next_page_token);
176161
nlohmann::json namespaces = nlohmann::json::array();
177162
for (const auto& ns : response.namespaces) {
178163
namespaces.push_back(ToJson(ns));
@@ -198,9 +183,7 @@ Result<ListNamespacesResponse> ListNamespacesResponseFromJson(
198183
nlohmann::json ToJson(const CreateNamespaceResponse& response) {
199184
nlohmann::json json;
200185
json[kNamespace] = response.namespace_.levels;
201-
if (!response.properties.empty()) {
202-
json[kProperties] = response.properties;
203-
}
186+
SetContainerField(json, kProperties, response.properties);
204187
return json;
205188
}
206189

@@ -218,9 +201,7 @@ Result<CreateNamespaceResponse> CreateNamespaceResponseFromJson(
218201
nlohmann::json ToJson(const GetNamespaceResponse& response) {
219202
nlohmann::json json;
220203
json[kNamespace] = response.namespace_.levels;
221-
if (!response.properties.empty()) {
222-
json[kProperties] = response.properties;
223-
}
204+
SetContainerField(json, kProperties, response.properties);
224205
return json;
225206
}
226207

@@ -238,29 +219,25 @@ nlohmann::json ToJson(const UpdateNamespacePropertiesResponse& response) {
238219
nlohmann::json json;
239220
json[kUpdated] = response.updated;
240221
json[kRemoved] = response.removed;
241-
if (!response.missing.empty()) {
242-
json[kMissing] = response.missing;
243-
}
222+
SetContainerField(json, kMissing, response.missing);
244223
return json;
245224
}
246225

247226
Result<UpdateNamespacePropertiesResponse> UpdateNamespacePropertiesResponseFromJson(
248227
const nlohmann::json& json) {
249228
UpdateNamespacePropertiesResponse response;
250-
ICEBERG_ASSIGN_OR_RAISE(response.updated,
251-
GetJsonValue<std::vector<std::string>>(json, kUpdated));
252-
ICEBERG_ASSIGN_OR_RAISE(response.removed,
253-
GetJsonValue<std::vector<std::string>>(json, kRemoved));
229+
ICEBERG_ASSIGN_OR_RAISE(
230+
response.updated, GetJsonValueOrDefault<std::vector<std::string>>(json, kUpdated));
231+
ICEBERG_ASSIGN_OR_RAISE(
232+
response.removed, GetJsonValueOrDefault<std::vector<std::string>>(json, kRemoved));
254233
ICEBERG_ASSIGN_OR_RAISE(
255234
response.missing, GetJsonValueOrDefault<std::vector<std::string>>(json, kMissing));
256235
return response;
257236
}
258237

259238
nlohmann::json ToJson(const ListTablesResponse& response) {
260239
nlohmann::json json;
261-
if (!response.next_page_token.empty()) {
262-
json[kNextPageToken] = response.next_page_token;
263-
}
240+
SetOptionalStringField(json, kNextPageToken, response.next_page_token);
264241
nlohmann::json identifiers_json = nlohmann::json::array();
265242
for (const auto& identifier : response.identifiers) {
266243
identifiers_json.push_back(ToJson(identifier));
@@ -282,4 +259,21 @@ Result<ListTablesResponse> ListTablesResponseFromJson(const nlohmann::json& json
282259
return response;
283260
}
284261

262+
#define ICEBERG_DEFINE_FROM_JSON(Model) \
263+
template <> \
264+
Result<Model> FromJson<Model>(const nlohmann::json& json) { \
265+
return Model##FromJson(json); \
266+
}
267+
268+
ICEBERG_DEFINE_FROM_JSON(ListNamespacesResponse)
269+
ICEBERG_DEFINE_FROM_JSON(CreateNamespaceRequest)
270+
ICEBERG_DEFINE_FROM_JSON(CreateNamespaceResponse)
271+
ICEBERG_DEFINE_FROM_JSON(GetNamespaceResponse)
272+
ICEBERG_DEFINE_FROM_JSON(UpdateNamespacePropertiesRequest)
273+
ICEBERG_DEFINE_FROM_JSON(UpdateNamespacePropertiesResponse)
274+
ICEBERG_DEFINE_FROM_JSON(ListTablesResponse)
275+
ICEBERG_DEFINE_FROM_JSON(LoadTableResult)
276+
ICEBERG_DEFINE_FROM_JSON(RegisterTableRequest)
277+
ICEBERG_DEFINE_FROM_JSON(RenameTableRequest)
278+
285279
} // namespace iceberg::rest

‎src/iceberg/catalog/rest/json_internal.h‎

Lines changed: 26 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -21,81 +21,36 @@
2121

2222
#include <nlohmann/json_fwd.hpp>
2323

24+
#include "iceberg/catalog/rest/iceberg_rest_export.h"
2425
#include "iceberg/catalog/rest/types.h"
2526
#include "iceberg/result.h"
2627

2728
namespace iceberg::rest {
2829

29-
/// \brief Serializes a `ListNamespacesResponse` object to JSON.
30-
ICEBERG_REST_EXPORT nlohmann::json ToJson(const ListNamespacesResponse& response);
31-
32-
/// \brief Deserializes a JSON object into a `ListNamespacesResponse` object.
33-
ICEBERG_REST_EXPORT Result<ListNamespacesResponse> ListNamespacesResponseFromJson(
34-
const nlohmann::json& json);
35-
36-
/// \brief Serializes a `CreateNamespaceRequest` object to JSON.
37-
ICEBERG_REST_EXPORT nlohmann::json ToJson(const CreateNamespaceRequest& request);
38-
39-
/// \brief Deserializes a JSON object into a `CreateNamespaceRequest` object.
40-
ICEBERG_REST_EXPORT Result<CreateNamespaceRequest> CreateNamespaceRequestFromJson(
41-
const nlohmann::json& json);
42-
43-
/// \brief Serializes a `CreateNamespaceResponse` object to JSON.
44-
ICEBERG_REST_EXPORT nlohmann::json ToJson(const CreateNamespaceResponse& response);
45-
46-
/// \brief Deserializes a JSON object into a `CreateNamespaceResponse` object.
47-
ICEBERG_REST_EXPORT Result<CreateNamespaceResponse> CreateNamespaceResponseFromJson(
48-
const nlohmann::json& json);
49-
50-
/// \brief Serializes a `GetNamespaceResponse` object to JSON.
51-
ICEBERG_REST_EXPORT nlohmann::json ToJson(const GetNamespaceResponse& response);
52-
53-
/// \brief Deserializes a JSON object into a `GetNamespaceResponse` object.
54-
ICEBERG_REST_EXPORT Result<GetNamespaceResponse> GetNamespaceResponseFromJson(
55-
const nlohmann::json& json);
56-
57-
/// \brief Serializes an `UpdateNamespacePropertiesRequest` object to JSON.
58-
ICEBERG_REST_EXPORT nlohmann::json ToJson(
59-
const UpdateNamespacePropertiesRequest& request);
60-
61-
/// \brief Deserializes a JSON object into an `UpdateNamespacePropertiesRequest` object.
62-
ICEBERG_REST_EXPORT Result<UpdateNamespacePropertiesRequest>
63-
UpdateNamespacePropertiesRequestFromJson(const nlohmann::json& json);
64-
65-
/// \brief Serializes an `UpdateNamespacePropertiesResponse` object to JSON.
66-
ICEBERG_REST_EXPORT nlohmann::json ToJson(
67-
const UpdateNamespacePropertiesResponse& response);
68-
69-
/// \brief Deserializes a JSON object into an `UpdateNamespacePropertiesResponse` object.
70-
ICEBERG_REST_EXPORT Result<UpdateNamespacePropertiesResponse>
71-
UpdateNamespacePropertiesResponseFromJson(const nlohmann::json& json);
72-
73-
/// \brief Serializes a `ListTablesResponse` object to JSON.
74-
ICEBERG_REST_EXPORT nlohmann::json ToJson(const ListTablesResponse& response);
75-
76-
/// \brief Deserializes a JSON object into a `ListTablesResponse` object.
77-
ICEBERG_REST_EXPORT Result<ListTablesResponse> ListTablesResponseFromJson(
78-
const nlohmann::json& json);
79-
80-
/// \brief Serializes a `LoadTableResult` object to JSON.
81-
ICEBERG_REST_EXPORT nlohmann::json ToJson(const LoadTableResult& result);
82-
83-
/// \brief Deserializes a JSON object into a `LoadTableResult` object.
84-
ICEBERG_REST_EXPORT Result<LoadTableResult> LoadTableResultFromJson(
85-
const nlohmann::json& json);
86-
87-
/// \brief Serializes a `RegisterTableRequest` object to JSON.
88-
ICEBERG_REST_EXPORT nlohmann::json ToJson(const RegisterTableRequest& request);
89-
90-
/// \brief Deserializes a JSON object into a `RegisterTableRequest` object.
91-
ICEBERG_REST_EXPORT Result<RegisterTableRequest> RegisterTableRequestFromJson(
92-
const nlohmann::json& json);
93-
94-
/// \brief Serializes a `RenameTableRequest` object to JSON.
95-
ICEBERG_REST_EXPORT nlohmann::json ToJson(const RenameTableRequest& request);
96-
97-
/// \brief Deserializes a JSON object into a `RenameTableRequest` object.
98-
ICEBERG_REST_EXPORT Result<RenameTableRequest> RenameTableRequestFromJson(
99-
const nlohmann::json& json);
30+
template <typename Model>
31+
Result<Model> FromJson(const nlohmann::json& json);
32+
33+
#define ICEBERG_DECLARE_JSON_SERDE(Model) \
34+
ICEBERG_REST_EXPORT Result<Model> Model##FromJson(const nlohmann::json& json); \
35+
\
36+
template <> \
37+
ICEBERG_REST_EXPORT Result<Model> FromJson(const nlohmann::json& json); \
38+
\
39+
ICEBERG_REST_EXPORT nlohmann::json ToJson(const Model& model);
40+
41+
/// \note Don't forget to add `ICEBERG_DEFINE_FROM_JSON` to the end of
42+
/// `json_internal.cc` to define the `FromJson` function for the model.
43+
ICEBERG_DECLARE_JSON_SERDE(ListNamespacesResponse)
44+
ICEBERG_DECLARE_JSON_SERDE(CreateNamespaceRequest)
45+
ICEBERG_DECLARE_JSON_SERDE(CreateNamespaceResponse)
46+
ICEBERG_DECLARE_JSON_SERDE(GetNamespaceResponse)
47+
ICEBERG_DECLARE_JSON_SERDE(UpdateNamespacePropertiesRequest)
48+
ICEBERG_DECLARE_JSON_SERDE(UpdateNamespacePropertiesResponse)
49+
ICEBERG_DECLARE_JSON_SERDE(ListTablesResponse)
50+
ICEBERG_DECLARE_JSON_SERDE(LoadTableResult)
51+
ICEBERG_DECLARE_JSON_SERDE(RegisterTableRequest)
52+
ICEBERG_DECLARE_JSON_SERDE(RenameTableRequest)
53+
54+
#undef ICEBERG_DECLARE_JSON_SERDE
10055

10156
} // namespace iceberg::rest

‎src/iceberg/test/rest_catalog_test.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ class RestCatalogIntegrationTest : public ::testing::Test {
5151
std::thread server_thread_;
5252
};
5353

54-
TEST_F(RestCatalogIntegrationTest, GetConfigSuccessfully) {
54+
TEST_F(RestCatalogIntegrationTest, DISABLED_GetConfigSuccessfully) {
5555
server_->Get("/v1/config", [](const httplib::Request&, httplib::Response& res) {
5656
res.status = 200;
5757
res.set_content(R"({"warehouse": "s3://test-bucket"})", "application/json");
@@ -68,7 +68,7 @@ TEST_F(RestCatalogIntegrationTest, GetConfigSuccessfully) {
6868
EXPECT_EQ(json_body["warehouse"], "s3://test-bucket");
6969
}
7070

71-
TEST_F(RestCatalogIntegrationTest, ListNamespacesReturnsMultipleResults) {
71+
TEST_F(RestCatalogIntegrationTest, DISABLED_ListNamespacesReturnsMultipleResults) {
7272
server_->Get("/v1/namespaces", [](const httplib::Request&, httplib::Response& res) {
7373
res.status = 200;
7474
res.set_content(R"({
@@ -93,7 +93,7 @@ TEST_F(RestCatalogIntegrationTest, ListNamespacesReturnsMultipleResults) {
9393
EXPECT_THAT(json_body["namespaces"][0][0], "accounting");
9494
}
9595

96-
TEST_F(RestCatalogIntegrationTest, HandlesServerError) {
96+
TEST_F(RestCatalogIntegrationTest, DISABLED_HandlesServerError) {
9797
server_->Get("/v1/config", [](const httplib::Request&, httplib::Response& res) {
9898
res.status = 500;
9999
res.set_content("Internal Server Error", "text/plain");

0 commit comments

Comments
 (0)