fix review

Author: HeartLinked

src/iceberg/catalog/rest/json_internal.cc

@@ -20,7 +20,6 @@
 #include "iceberg/catalog/rest/json_internal.h"
 
 #include <string>
-#include <unordered_map>
 #include <utility>
 #include <vector>
 
@@ -75,9 +74,7 @@ nlohmann::json ToJson(const CatalogConfig& config) {
   nlohmann::json json;
   json[kOverrides] = config.overrides;
   json[kDefaults] = config.defaults;
-  if (!config.endpoints.empty()) {
-    json[kEndpoints] = config.endpoints;
-  }
+  SetContainerField(json, kEndpoints, config.endpoints);
   return json;
 }
 
@@ -100,17 +97,18 @@ nlohmann::json ToJson(const ErrorModel& error) {
   json[kMessage] = error.message;
   json[kType] = error.type;
   json[kCode] = error.code;
-  if (!error.stack.empty()) {
-    json[kStack] = error.stack;
-  }
+  SetContainerField(json, kStack, error.stack);
   return json;
 }
 
 Result<ErrorModel> ErrorModelFromJson(const nlohmann::json& json) {
   ErrorModel error;
+  // NOTE: Iceberg's Java implementation allows missing required fields (message, type,
+  // code) during deserialization, which deviates from the REST spec. We enforce strict
+  // validation here.
   ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValue<std::string>(json, kMessage));
   ICEBERG_ASSIGN_OR_RAISE(error.type, GetJsonValue<std::string>(json, kType));
-  ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue<uint16_t>(json, kCode));
+  ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue<uint32_t>(json, kCode));
   ICEBERG_ASSIGN_OR_RAISE(error.stack,
                           GetJsonValueOrDefault<std::vector<std::string>>(json, kStack));
   ICEBERG_RETURN_UNEXPECTED(Validator::Validate(error));

src/iceberg/catalog/rest/types.h

@@ -35,16 +35,16 @@ namespace iceberg::rest {
 
 /// \brief Server-provided configuration for the catalog.
 struct ICEBERG_REST_EXPORT CatalogConfig {
-  std::unordered_map<std::string, std::string> overrides;  // required
   std::unordered_map<std::string, std::string> defaults;   // required
+  std::unordered_map<std::string, std::string> overrides;  // required
   std::vector<std::string> endpoints;
 };
 
 /// \brief JSON error payload returned in a response with further details on the error.
 struct ICEBERG_REST_EXPORT ErrorModel {
   std::string message;  // required
   std::string type;     // required
-  uint16_t code;        // required
+  uint32_t code;        // required
   std::vector<std::string> stack;
 };
 

src/iceberg/catalog/rest/validator.cc

@@ -19,13 +19,12 @@
 
 #include "iceberg/catalog/rest/validator.h"
 
+#include <algorithm>
 #include <format>
-#include <ranges>
-#include <unordered_set>
-#include <utility>
 
 #include "iceberg/catalog/rest/types.h"
 #include "iceberg/result.h"
+#include "iceberg/util/macros.h"
 
 namespace iceberg::rest {
 
@@ -40,18 +39,20 @@ Status Validator::Validate(const CatalogConfig& config) {
 }
 
 Status Validator::Validate(const ErrorModel& error) {
-  if (error.message.empty() || error.type.empty()) [[unlikely]] {
+  if (error.message.empty() || error.type.empty()) {
     return Invalid("Invalid error model: missing required fields");
   }
 
-  if (error.code < 400 || error.code > 600) [[unlikely]] {
-    return Invalid("Invalid error model: code must be between 400 and 600");
+  if (error.code < 400 || error.code > 600) {
+    return Invalid("Invalid error model: code {} is out of range [400, 600]", error.code);
   }
 
   // stack is optional, no validation needed
   return {};
 }
 
+// We don't validate the error field because ErrorModel::Validate has been called in the
+// FromJson.
 Status Validator::Validate(const ErrorResponse& response) { return {}; }
 
 // Namespace operations
@@ -66,30 +67,38 @@ Status Validator::Validate(const GetNamespaceResponse& response) { return {}; }
 
 Status Validator::Validate(const UpdateNamespacePropertiesRequest& request) {
   // keys in updates and removals must not overlap
-  if (request.removals.empty() || request.updates.empty()) [[unlikely]] {
+  if (request.removals.empty() || request.updates.empty()) {
     return {};
   }
 
-  std::unordered_set<std::string> remove_set(request.removals.begin(),
-                                             request.removals.end());
-  std::vector<std::string> common;
-
-  for (const std::string& k : request.updates | std::views::keys) {
-    if (remove_set.contains(k)) {
-      common.push_back(k);
+  auto extract_and_sort = [](const auto& container, auto key_extractor) {
+    std::vector<std::string_view> result;
+    result.reserve(container.size());
+    for (const auto& item : container) {
+      result.push_back(std::string_view{key_extractor(item)});
     }
-  }
+    std::ranges::sort(result);
+    return result;
+  };
+
+  auto sorted_removals =
+      extract_and_sort(request.removals, [](const auto& s) -> const auto& { return s; });
+  auto sorted_update_keys = extract_and_sort(
+      request.updates, [](const auto& pair) -> const auto& { return pair.first; });
+
+  std::vector<std::string_view> common;
+  std::ranges::set_intersection(sorted_removals, sorted_update_keys,
+                                std::back_inserter(common));
 
   if (!common.empty()) {
     std::string keys;
-    bool first = true;
-    for (const std::string& s : common) {
-      if (!std::exchange(first, false)) keys += ", ";
-      keys += s;
+    for (size_t i = 0; i < common.size(); ++i) {
+      if (i) keys += ", ";
+      keys += common[i];
     }
 
     return Invalid(
-        "Invalid namespace properties update: cannot simultaneously set and remove keys: "
+        "Invalid namespace update: cannot simultaneously set and remove keys: "
         "[{}]",
         keys);
   }
@@ -105,34 +114,34 @@ Status Validator::Validate(const UpdateNamespacePropertiesResponse& response) {
 Status Validator::Validate(const ListTablesResponse& response) { return {}; }
 
 Status Validator::Validate(const LoadTableResult& result) {
-  if (!result.metadata) [[unlikely]] {
+  if (!result.metadata) {
     return Invalid("Invalid metadata: null");
   }
   return {};
 }
 
 Status Validator::Validate(const RegisterTableRequest& request) {
-  if (request.name.empty()) [[unlikely]] {
-    return Invalid("Invalid table name: empty");
+  if (request.name.empty()) {
+    return Invalid("Missing table name");
   }
 
-  if (request.metadata_location.empty()) [[unlikely]] {
-    return Invalid("Invalid metadata location: empty");
+  if (request.metadata_location.empty()) {
+    return Invalid("Empty metadata location");
   }
 
   return {};
 }
 
 Status Validator::Validate(const RenameTableRequest& request) {
-  if (request.source.ns.levels.empty() || request.source.name.empty()) [[unlikely]] {
-    return Invalid("Invalid source identifier");
-  }
+  ICEBERG_RETURN_UNEXPECTED(Validate(request.source));
+  ICEBERG_RETURN_UNEXPECTED(Validate(request.destination));
+  return {};
+}
 
-  if (request.destination.ns.levels.empty() || request.destination.name.empty())
-      [[unlikely]] {
-    return Invalid("Invalid destination identifier");
+Status Validator::Validate(const TableIdentifier& identifier) {
+  if (identifier.name.empty()) {
+    return Invalid("Invalid table identifier: missing table name");
   }
-
   return {};
 }
 

src/iceberg/catalog/rest/validator.h

@@ -77,6 +77,11 @@ class ICEBERG_REST_EXPORT Validator {
 
   /// \brief Validates a RenameTableRequest object.
   static Status Validate(const RenameTableRequest& request);
+
+  // Other types
+
+  /// \brief Validates a TableIdentifier object.
+  static Status Validate(const TableIdentifier& identifier);
 };
 
 }  // namespace iceberg::rest

src/iceberg/test/meson.build

@@ -96,6 +96,12 @@ if get_option('rest').enabled()
     }
 endif
 
+cpp = meson.get_compiler('cpp')
+cpp_bigobj_args = []
+if cpp.get_id() == 'msvc'
+    iceberg_common_cpp_args += cpp.get_supported_arguments(['/bigobj'])
+endif
+
 foreach test_name, values : iceberg_tests
     exc = executable(
         test_name,
@@ -104,6 +110,7 @@ foreach test_name, values : iceberg_tests
             'dependencies',
             [],
         ),
+        cpp_args: cpp_bigobj_args,
     )
     test(test_name, exc)
 endforeach

src/iceberg/test/rest_json_internal_test.cc

@@ -940,7 +940,6 @@ INSTANTIATE_TEST_SUITE_P(
       return info.param.test_name;
     });
 
-// ErrorModel tests - testing the nested error model structure
 DECLARE_INVALID_TEST(ErrorModel)
 
 INSTANTIATE_TEST_SUITE_P(