Skip to content

Commit 066b701

Browse files
HeartLinkedHeartLinked
committed
fix review
1 parent 9f15a10 commit 066b701

File tree

6 files changed

+58
-41
lines changed

6 files changed

+58
-41
lines changed

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,7 @@ nlohmann::json ToJson(const CatalogConfig& config) {
7575
nlohmann::json json;
7676
json[kOverrides] = config.overrides;
7777
json[kDefaults] = config.defaults;
78-
if (!config.endpoints.empty()) {
79-
json[kEndpoints] = config.endpoints;
80-
}
78+
SetContainerField(json, kEndpoints, config.endpoints);
8179
return json;
8280
}
8381

@@ -100,17 +98,18 @@ nlohmann::json ToJson(const ErrorModel& error) {
10098
json[kMessage] = error.message;
10199
json[kType] = error.type;
102100
json[kCode] = error.code;
103-
if (!error.stack.empty()) {
104-
json[kStack] = error.stack;
105-
}
101+
SetContainerField(json, kStack, error.stack);
106102
return json;
107103
}
108104

109105
Result<ErrorModel> ErrorModelFromJson(const nlohmann::json& json) {
110106
ErrorModel error;
107+
// NOTE: Iceberg's Java implementation allows missing required fields (message, type,
108+
// code) during deserialization, which deviates from the REST spec. We enforce strict
109+
// validation here.
111110
ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValue<std::string>(json, kMessage));
112111
ICEBERG_ASSIGN_OR_RAISE(error.type, GetJsonValue<std::string>(json, kType));
113-
ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue<uint16_t>(json, kCode));
112+
ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue<uint32_t>(json, kCode));
114113
ICEBERG_ASSIGN_OR_RAISE(error.stack,
115114
GetJsonValueOrDefault<std::vector<std::string>>(json, kStack));
116115
ICEBERG_RETURN_UNEXPECTED(Validator::Validate(error));

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,16 +35,16 @@ namespace iceberg::rest {
3535

3636
/// \brief Server-provided configuration for the catalog.
3737
struct ICEBERG_REST_EXPORT CatalogConfig {
38-
std::unordered_map<std::string, std::string> overrides; // required
3938
std::unordered_map<std::string, std::string> defaults; // required
39+
std::unordered_map<std::string, std::string> overrides; // required
4040
std::vector<std::string> endpoints;
4141
};
4242

4343
/// \brief JSON error payload returned in a response with further details on the error.
4444
struct ICEBERG_REST_EXPORT ErrorModel {
4545
std::string message; // required
4646
std::string type; // required
47-
uint16_t code; // required
47+
uint32_t code; // required
4848
std::vector<std::string> stack;
4949
};
5050

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

Lines changed: 38 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@
1919

2020
#include "iceberg/catalog/rest/validator.h"
2121

22+
#include <algorithm>
2223
#include <format>
2324
#include <ranges>
24-
#include <unordered_set>
25-
#include <utility>
2625

2726
#include "iceberg/catalog/rest/types.h"
2827
#include "iceberg/result.h"
28+
#include "iceberg/util/macros.h"
2929

3030
namespace iceberg::rest {
3131

@@ -40,18 +40,20 @@ Status Validator::Validate(const CatalogConfig& config) {
4040
}
4141

4242
Status Validator::Validate(const ErrorModel& error) {
43-
if (error.message.empty() || error.type.empty()) [[unlikely]] {
43+
if (error.message.empty() || error.type.empty()) {
4444
return Invalid("Invalid error model: missing required fields");
4545
}
4646

47-
if (error.code < 400 || error.code > 600) [[unlikely]] {
48-
return Invalid("Invalid error model: code must be between 400 and 600");
47+
if (error.code < 400 || error.code > 600) {
48+
return Invalid("Invalid error model: code {} is out of range [400, 600]", error.code);
4949
}
5050

5151
// stack is optional, no validation needed
5252
return {};
5353
}
5454

55+
// We don't validate the error field because ErrorModel::Validate has been called in the
56+
// FromJson.
5557
Status Validator::Validate(const ErrorResponse& response) { return {}; }
5658

5759
// Namespace operations
@@ -66,30 +68,35 @@ Status Validator::Validate(const GetNamespaceResponse& response) { return {}; }
6668

6769
Status Validator::Validate(const UpdateNamespacePropertiesRequest& request) {
6870
// keys in updates and removals must not overlap
69-
if (request.removals.empty() || request.updates.empty()) [[unlikely]] {
71+
if (request.removals.empty() || request.updates.empty()) {
7072
return {};
7173
}
7274

73-
std::unordered_set<std::string> remove_set(request.removals.begin(),
74-
request.removals.end());
75-
std::vector<std::string> common;
75+
auto sorted_removals =
76+
request.removals |
77+
std::views::transform([](const std::string& s) { return std::string_view{s}; }) |
78+
std::ranges::to<std::vector>();
79+
std::ranges::sort(sorted_removals);
7680

77-
for (const std::string& k : request.updates | std::views::keys) {
78-
if (remove_set.contains(k)) {
79-
common.push_back(k);
80-
}
81-
}
81+
auto sorted_update_keys =
82+
request.updates | std::views::keys |
83+
std::views::transform([](const std::string& s) { return std::string_view{s}; }) |
84+
std::ranges::to<std::vector>();
85+
std::ranges::sort(sorted_update_keys);
86+
87+
std::vector<std::string_view> common;
88+
std::ranges::set_intersection(sorted_update_keys, sorted_removals,
89+
std::back_inserter(common));
8290

8391
if (!common.empty()) {
8492
std::string keys;
85-
bool first = true;
86-
for (const std::string& s : common) {
87-
if (!std::exchange(first, false)) keys += ", ";
88-
keys += s;
93+
for (size_t i = 0; i < common.size(); ++i) {
94+
if (i) keys += ", ";
95+
keys += common[i];
8996
}
9097

9198
return Invalid(
92-
"Invalid namespace properties update: cannot simultaneously set and remove keys: "
99+
"Invalid namespace update: cannot simultaneously set and remove keys: "
93100
"[{}]",
94101
keys);
95102
}
@@ -105,34 +112,34 @@ Status Validator::Validate(const UpdateNamespacePropertiesResponse& response) {
105112
Status Validator::Validate(const ListTablesResponse& response) { return {}; }
106113

107114
Status Validator::Validate(const LoadTableResult& result) {
108-
if (!result.metadata) [[unlikely]] {
115+
if (!result.metadata) {
109116
return Invalid("Invalid metadata: null");
110117
}
111118
return {};
112119
}
113120

114121
Status Validator::Validate(const RegisterTableRequest& request) {
115-
if (request.name.empty()) [[unlikely]] {
116-
return Invalid("Invalid table name: empty");
122+
if (request.name.empty()) {
123+
return Invalid("Missing table name");
117124
}
118125

119-
if (request.metadata_location.empty()) [[unlikely]] {
120-
return Invalid("Invalid metadata location: empty");
126+
if (request.metadata_location.empty()) {
127+
return Invalid("Empty metadata location");
121128
}
122129

123130
return {};
124131
}
125132

126133
Status Validator::Validate(const RenameTableRequest& request) {
127-
if (request.source.ns.levels.empty() || request.source.name.empty()) [[unlikely]] {
128-
return Invalid("Invalid source identifier");
129-
}
134+
ICEBERG_RETURN_UNEXPECTED(Validate(request.source));
135+
ICEBERG_RETURN_UNEXPECTED(Validate(request.destination));
136+
return {};
137+
}
130138

131-
if (request.destination.ns.levels.empty() || request.destination.name.empty())
132-
[[unlikely]] {
133-
return Invalid("Invalid destination identifier");
139+
Status Validator::Validate(const TableIdentifier& identifier) {
140+
if (identifier.name.empty()) {
141+
return Invalid("Invalid table identifier: missing table name");
134142
}
135-
136143
return {};
137144
}
138145

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,11 @@ class ICEBERG_REST_EXPORT Validator {
7777

7878
/// \brief Validates a RenameTableRequest object.
7979
static Status Validate(const RenameTableRequest& request);
80+
81+
// Other types
82+
83+
/// \brief Validates a TableIdentifier object.
84+
static Status Validate(const TableIdentifier& identifier);
8085
};
8186

8287
} // namespace iceberg::rest

‎src/iceberg/test/meson.build‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,12 @@ if get_option('rest').enabled()
9696
}
9797
endif
9898

99+
cpp = meson.get_compiler('cpp')
100+
cpp_bigobj_args = []
101+
if cpp.get_id() == 'msvc'
102+
iceberg_common_cpp_args += cpp.get_supported_arguments(['/bigobj'])
103+
endif
104+
99105
foreach test_name, values : iceberg_tests
100106
exc = executable(
101107
test_name,
@@ -104,6 +110,7 @@ foreach test_name, values : iceberg_tests
104110
'dependencies',
105111
[],
106112
),
113+
cpp_args: cpp_bigobj_args,
107114
)
108115
test(test_name, exc)
109116
endforeach

‎src/iceberg/test/rest_json_internal_test.cc‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -940,7 +940,6 @@ INSTANTIATE_TEST_SUITE_P(
940940
return info.param.test_name;
941941
});
942942

943-
// ErrorModel tests - testing the nested error model structure
944943
DECLARE_INVALID_TEST(ErrorModel)
945944

946945
INSTANTIATE_TEST_SUITE_P(

0 commit comments

Comments
 (0)