Skip to content

Commit e25ff49

Browse files
HeartLinkedHeartLinked
committed
fix review and ci
1 parent 9f15a10 commit e25ff49

File tree

6 files changed

+60
-48
lines changed

6 files changed

+60
-48
lines changed

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
#include "iceberg/catalog/rest/json_internal.h"
2121

2222
#include <string>
23-
#include <unordered_map>
2423
#include <utility>
2524
#include <vector>
2625

@@ -75,9 +74,7 @@ nlohmann::json ToJson(const CatalogConfig& config) {
7574
nlohmann::json json;
7675
json[kOverrides] = config.overrides;
7776
json[kDefaults] = config.defaults;
78-
if (!config.endpoints.empty()) {
79-
json[kEndpoints] = config.endpoints;
80-
}
77+
SetContainerField(json, kEndpoints, config.endpoints);
8178
return json;
8279
}
8380

@@ -100,17 +97,18 @@ nlohmann::json ToJson(const ErrorModel& error) {
10097
json[kMessage] = error.message;
10198
json[kType] = error.type;
10299
json[kCode] = error.code;
103-
if (!error.stack.empty()) {
104-
json[kStack] = error.stack;
105-
}
100+
SetContainerField(json, kStack, error.stack);
106101
return json;
107102
}
108103

109104
Result<ErrorModel> ErrorModelFromJson(const nlohmann::json& json) {
110105
ErrorModel error;
106+
// NOTE: Iceberg's Java implementation allows missing required fields (message, type,
107+
// code) during deserialization, which deviates from the REST spec. We enforce strict
108+
// validation here.
111109
ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValue<std::string>(json, kMessage));
112110
ICEBERG_ASSIGN_OR_RAISE(error.type, GetJsonValue<std::string>(json, kType));
113-
ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue<uint16_t>(json, kCode));
111+
ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue<uint32_t>(json, kCode));
114112
ICEBERG_ASSIGN_OR_RAISE(error.stack,
115113
GetJsonValueOrDefault<std::vector<std::string>>(json, kStack));
116114
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: 40 additions & 37 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>
23-
#include <ranges>
24-
#include <unordered_set>
25-
#include <utility>
2624

2725
#include "iceberg/catalog/rest/types.h"
2826
#include "iceberg/result.h"
27+
#include "iceberg/util/formatter_internal.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,32 +68,33 @@ 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;
76-
77-
for (const std::string& k : request.updates | std::views::keys) {
78-
if (remove_set.contains(k)) {
79-
common.push_back(k);
75+
auto extract_and_sort = [](const auto& container, auto key_extractor) {
76+
std::vector<std::string_view> result;
77+
result.reserve(container.size());
78+
for (const auto& item : container) {
79+
result.push_back(std::string_view{key_extractor(item)});
8080
}
81-
}
81+
std::ranges::sort(result);
82+
return result;
83+
};
8284

83-
if (!common.empty()) {
84-
std::string keys;
85-
bool first = true;
86-
for (const std::string& s : common) {
87-
if (!std::exchange(first, false)) keys += ", ";
88-
keys += s;
89-
}
85+
auto sorted_removals =
86+
extract_and_sort(request.removals, [](const auto& s) -> const auto& { return s; });
87+
auto sorted_update_keys = extract_and_sort(
88+
request.updates, [](const auto& pair) -> const auto& { return pair.first; });
89+
90+
std::vector<std::string_view> common;
91+
std::ranges::set_intersection(sorted_removals, sorted_update_keys,
92+
std::back_inserter(common));
9093

94+
if (!common.empty()) {
9195
return Invalid(
92-
"Invalid namespace properties update: cannot simultaneously set and remove keys: "
93-
"[{}]",
94-
keys);
96+
"Invalid namespace update: cannot simultaneously set and remove keys: {}",
97+
common);
9598
}
9699
return {};
97100
}
@@ -105,34 +108,34 @@ Status Validator::Validate(const UpdateNamespacePropertiesResponse& response) {
105108
Status Validator::Validate(const ListTablesResponse& response) { return {}; }
106109

107110
Status Validator::Validate(const LoadTableResult& result) {
108-
if (!result.metadata) [[unlikely]] {
111+
if (!result.metadata) {
109112
return Invalid("Invalid metadata: null");
110113
}
111114
return {};
112115
}
113116

114117
Status Validator::Validate(const RegisterTableRequest& request) {
115-
if (request.name.empty()) [[unlikely]] {
116-
return Invalid("Invalid table name: empty");
118+
if (request.name.empty()) {
119+
return Invalid("Missing table name");
117120
}
118121

119-
if (request.metadata_location.empty()) [[unlikely]] {
120-
return Invalid("Invalid metadata location: empty");
122+
if (request.metadata_location.empty()) {
123+
return Invalid("Empty metadata location");
121124
}
122125

123126
return {};
124127
}
125128

126129
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-
}
130+
ICEBERG_RETURN_UNEXPECTED(Validate(request.source));
131+
ICEBERG_RETURN_UNEXPECTED(Validate(request.destination));
132+
return {};
133+
}
130134

131-
if (request.destination.ns.levels.empty() || request.destination.name.empty())
132-
[[unlikely]] {
133-
return Invalid("Invalid destination identifier");
135+
Status Validator::Validate(const TableIdentifier& identifier) {
136+
if (identifier.name.empty()) {
137+
return Invalid("Invalid table identifier: missing table name");
134138
}
135-
136139
return {};
137140
}
138141

‎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+
cpp_bigobj_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)