fix review

Author: HeartLinked

src/iceberg/catalog/rest/json_internal.cc

@@ -71,8 +71,6 @@ constexpr std::string_view kType = "type";
 constexpr std::string_view kCode = "code";
 constexpr std::string_view kStack = "stack";
 constexpr std::string_view kError = "error";
-
-// CommitTableRequest field constants
 constexpr std::string_view kIdentifier = "identifier";
 constexpr std::string_view kRequirements = "requirements";
 
@@ -400,18 +398,18 @@ Result<CreateTableRequest> CreateTableRequestFromJson(const nlohmann::json& json
 nlohmann::json ToJson(const CommitTableRequest& request) {
   nlohmann::json json;
   if (!request.identifier.name.empty()) {
-    json[kIdentifier] = iceberg::ToJson(request.identifier);
+    json[kIdentifier] = ToJson(request.identifier);
   }
 
   nlohmann::json requirements_json = nlohmann::json::array();
   for (const auto& req : request.requirements) {
-    requirements_json.push_back(iceberg::ToJson(*req));
+    requirements_json.push_back(ToJson(*req));
   }
   json[kRequirements] = std::move(requirements_json);
 
   nlohmann::json updates_json = nlohmann::json::array();
   for (const auto& update : request.updates) {
-    updates_json.push_back(iceberg::ToJson(*update));
+    updates_json.push_back(ToJson(*update));
   }
   json[kUpdates] = std::move(updates_json);
 
@@ -425,24 +423,39 @@ Result<CommitTableRequest> CommitTableRequestFromJson(const nlohmann::json& json
                             GetJsonValue<nlohmann::json>(json, kIdentifier));
     ICEBERG_ASSIGN_OR_RAISE(request.identifier, TableIdentifierFromJson(identifier_json));
   }
-  // Note: requirements and updates deserialization would be complex
-  // and is not typically needed for the client side
+
+  ICEBERG_ASSIGN_OR_RAISE(auto requirements_json,
+                          GetJsonValue<nlohmann::json>(json, kRequirements));
+  for (const auto& req_json : requirements_json) {
+    ICEBERG_ASSIGN_OR_RAISE(auto requirement, TableRequirementFromJson(req_json));
+    request.requirements.push_back(std::move(requirement));
+  }
+
+  ICEBERG_ASSIGN_OR_RAISE(auto updates_json,
+                          GetJsonValue<nlohmann::json>(json, kUpdates));
+  for (const auto& update_json : updates_json) {
+    ICEBERG_ASSIGN_OR_RAISE(auto update, TableUpdateFromJson(update_json));
+    request.updates.push_back(std::move(update));
+  }
+
   ICEBERG_RETURN_UNEXPECTED(request.Validate());
   return request;
 }
 
 // CommitTableResponse serialization
 nlohmann::json ToJson(const CommitTableResponse& response) {
   nlohmann::json json;
-  SetOptionalStringField(json, kMetadataLocation, response.metadata_location);
-  json[kMetadata] = ToJson(*response.metadata);
+  json[kMetadataLocation] = response.metadata_location;
+  if (response.metadata) {
+    json[kMetadata] = ToJson(*response.metadata);
+  }
   return json;
 }
 
 Result<CommitTableResponse> CommitTableResponseFromJson(const nlohmann::json& json) {
   CommitTableResponse response;
   ICEBERG_ASSIGN_OR_RAISE(response.metadata_location,
-                          GetJsonValueOrDefault<std::string>(json, kMetadataLocation));
+                          GetJsonValue<std::string>(json, kMetadataLocation));
   ICEBERG_ASSIGN_OR_RAISE(auto metadata_json,
                           GetJsonValue<nlohmann::json>(json, kMetadata));
   ICEBERG_ASSIGN_OR_RAISE(response.metadata, TableMetadataFromJson(metadata_json));

src/iceberg/catalog/rest/rest_catalog.cc

@@ -42,6 +42,8 @@
 #include "iceberg/schema.h"
 #include "iceberg/sort_order.h"
 #include "iceberg/table.h"
+#include "iceberg/table_requirement.h"
+#include "iceberg/table_update.h"
 #include "iceberg/util/macros.h"
 
 namespace iceberg::rest {
@@ -175,7 +177,7 @@ Result<std::vector<Namespace>> RestCatalog::ListNamespaces(const Namespace& ns)
     if (list_response.next_page_token.empty()) {
       return result;
     }
-    next_token = list_response.next_page_token;
+    next_token = std::move(list_response.next_page_token);
   }
   return result;
 }
@@ -245,7 +247,7 @@ Status RestCatalog::UpdateNamespaceProperties(
 }
 
 Result<std::vector<TableIdentifier>> RestCatalog::ListTables(const Namespace& ns) const {
-  ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::CreateTable());
+  ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::ListTables());
 
   ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Tables(ns));
   std::vector<TableIdentifier> result;
@@ -265,7 +267,7 @@ Result<std::vector<TableIdentifier>> RestCatalog::ListTables(const Namespace& ns
     if (list_response.next_page_token.empty()) {
       return result;
     }
-    next_token = list_response.next_page_token;
+    next_token = std::move(list_response.next_page_token);
   }
   return result;
 }
@@ -304,18 +306,17 @@ Result<std::shared_ptr<Table>> RestCatalog::UpdateTable(
     const TableIdentifier& identifier,
     const std::vector<std::unique_ptr<TableRequirement>>& requirements,
     const std::vector<std::unique_ptr<TableUpdate>>& updates) {
-  ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::CreateTable());
+  ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::UpdateTable());
   ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Table(identifier));
 
-  // Build request with non-owning shared_ptr (using no-op deleter)
   CommitTableRequest request{.identifier = identifier};
   request.requirements.reserve(requirements.size());
   for (const auto& req : requirements) {
-    request.requirements.emplace_back(req.get(), [](auto*) {});
+    request.requirements.push_back(req->Clone());
   }
   request.updates.reserve(updates.size());
   for (const auto& update : updates) {
-    request.updates.emplace_back(update.get(), [](auto*) {});
+    request.updates.push_back(update->Clone());
   }
 
   ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request)));
@@ -419,10 +420,9 @@ Result<std::shared_ptr<Table>> RestCatalog::RegisterTable(
 
   ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body()));
   ICEBERG_ASSIGN_OR_RAISE(auto load_result, LoadTableResultFromJson(json));
-  auto non_const_catalog = std::const_pointer_cast<RestCatalog>(shared_from_this());
-  return Table::Make(identifier, load_result.metadata,
+  return Table::Make(identifier, std::move(load_result.metadata),
                      std::move(load_result.metadata_location), file_io_,
-                     non_const_catalog);
+                     shared_from_this());
 }
 
 }  // namespace iceberg::rest

src/iceberg/catalog/rest/types.cc

@@ -23,6 +23,8 @@
 #include "iceberg/schema.h"
 #include "iceberg/sort_order.h"
 #include "iceberg/table_metadata.h"
+#include "iceberg/table_requirement.h"
+#include "iceberg/table_update.h"
 
 namespace iceberg::rest {
 
@@ -79,8 +81,25 @@ bool CommitTableRequest::operator==(const CommitTableRequest& other) const {
   if (updates.size() != other.updates.size()) {
     return false;
   }
-  // Note: Deep comparison of requirements and updates is not implemented
-  // as they contain polymorphic types. This is primarily for testing.
+
+  for (size_t i = 0; i < requirements.size(); ++i) {
+    if (!requirements[i] != !other.requirements[i]) {
+      return false;
+    }
+    if (requirements[i] && !requirements[i]->Equals(*other.requirements[i])) {
+      return false;
+    }
+  }
+
+  for (size_t i = 0; i < updates.size(); ++i) {
+    if (!updates[i] != !other.updates[i]) {
+      return false;
+    }
+    if (updates[i] && !updates[i]->Equals(*other.updates[i])) {
+      return false;
+    }
+  }
+
   return true;
 }
 

src/iceberg/catalog/rest/types.h

@@ -29,8 +29,6 @@
 #include "iceberg/result.h"
 #include "iceberg/schema.h"
 #include "iceberg/table_identifier.h"
-#include "iceberg/table_requirement.h"
-#include "iceberg/table_update.h"
 #include "iceberg/type_fwd.h"
 #include "iceberg/util/macros.h"
 
@@ -61,11 +59,12 @@ struct ICEBERG_REST_EXPORT ErrorResponse {
   /// \brief Validates the ErrorResponse.
   Status Validate() const {
     if (message.empty() || type.empty()) {
-      return Invalid("Invalid error response: missing required fields");
+      return ValidationFailed("Invalid error response: missing required fields");
     }
 
     if (code < 400 || code > 600) {
-      return Invalid("Invalid error response: code {} is out of range [400, 600]", code);
+      return ValidationFailed(
+          "Invalid error response: code {} is out of range [400, 600]", code);
     }
 
     // stack is optional, no validation needed
@@ -95,7 +94,7 @@ struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesRequest {
   Status Validate() const {
     for (const auto& key : removals) {
       if (updates.contains(key)) {
-        return Invalid("Duplicate key to update and remove: {}", key);
+        return ValidationFailed("Duplicate key to update and remove: {}", key);
       }
     }
     return {};
@@ -113,11 +112,11 @@ struct ICEBERG_REST_EXPORT RegisterTableRequest {
   /// \brief Validates the RegisterTableRequest.
   Status Validate() const {
     if (name.empty()) {
-      return Invalid("Missing table name");
+      return ValidationFailed("Missing table name");
     }
 
     if (metadata_location.empty()) {
-      return Invalid("Empty metadata location");
+      return ValidationFailed("Empty metadata location");
     }
 
     return {};
@@ -154,10 +153,10 @@ struct ICEBERG_REST_EXPORT CreateTableRequest {
   /// \brief Validates the CreateTableRequest.
   Status Validate() const {
     if (name.empty()) {
-      return Invalid("Missing table name");
+      return ValidationFailed("Missing table name");
     }
     if (!schema) {
-      return Invalid("Missing schema");
+      return ValidationFailed("Missing schema");
     }
     return {};
   }
@@ -178,7 +177,7 @@ struct ICEBERG_REST_EXPORT LoadTableResult {
   /// \brief Validates the LoadTableResult.
   Status Validate() const {
     if (!metadata) {
-      return Invalid("Invalid metadata: null");
+      return ValidationFailed("Invalid metadata: null");
     }
     return {};
   }
@@ -251,8 +250,8 @@ struct ICEBERG_REST_EXPORT ListTablesResponse {
 /// \brief Request to commit changes to a table.
 struct ICEBERG_REST_EXPORT CommitTableRequest {
   TableIdentifier identifier;
-  std::vector<std::shared_ptr<TableRequirement>> requirements;
-  std::vector<std::shared_ptr<TableUpdate>> updates;
+  std::vector<std::shared_ptr<TableRequirement>> requirements;  // required
+  std::vector<std::shared_ptr<TableUpdate>> updates;            // required
 
   /// \brief Validates the CommitTableRequest.
   Status Validate() const { return {}; }
@@ -262,14 +261,16 @@ struct ICEBERG_REST_EXPORT CommitTableRequest {
 
 /// \brief Response from committing changes to a table.
 struct ICEBERG_REST_EXPORT CommitTableResponse {
-  std::string metadata_location;
+  std::string metadata_location;            // required
   std::shared_ptr<TableMetadata> metadata;  // required
-  // TODO(Li Feiyang): Add std::shared_ptr<StorageCredential> storage_credential;
 
   /// \brief Validates the CommitTableResponse.
   Status Validate() const {
+    if (metadata_location.empty()) {
+      return ValidationFailed("Invalid metadata location: empty");
+    }
     if (!metadata) {
-      return Invalid("Invalid metadata: null");
+      return ValidationFailed("Invalid metadata: null");
     }
     return {};
   }