refactor: remove Validator and add Validate methods directly to rest models (#304)

Author: HeartLinked

src/iceberg/catalog/rest/CMakeLists.txt

@@ -15,7 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 
-set(ICEBERG_REST_SOURCES rest_catalog.cc json_internal.cc validator.cc)
+set(ICEBERG_REST_SOURCES rest_catalog.cc json_internal.cc)
 
 set(ICEBERG_REST_STATIC_BUILD_INTERFACE_LIBS)
 set(ICEBERG_REST_SHARED_BUILD_INTERFACE_LIBS)

src/iceberg/catalog/rest/json_internal.cc

@@ -26,7 +26,6 @@
 #include <nlohmann/json.hpp>
 
 #include "iceberg/catalog/rest/types.h"
-#include "iceberg/catalog/rest/validator.h"
 #include "iceberg/json_internal.h"
 #include "iceberg/table_identifier.h"
 #include "iceberg/util/json_util_internal.h"
@@ -88,7 +87,7 @@ Result<CatalogConfig> CatalogConfigFromJson(const nlohmann::json& json) {
   ICEBERG_ASSIGN_OR_RAISE(
       config.endpoints,
       GetJsonValueOrDefault<std::vector<std::string>>(json, kEndpoints));
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(config));
+  ICEBERG_RETURN_UNEXPECTED(config.Validate());
   return config;
 }
 
@@ -111,7 +110,7 @@ Result<ErrorModel> ErrorModelFromJson(const nlohmann::json& json) {
   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));
+  ICEBERG_RETURN_UNEXPECTED(error.Validate());
   return error;
 }
 
@@ -125,7 +124,7 @@ Result<ErrorResponse> ErrorResponseFromJson(const nlohmann::json& json) {
   ErrorResponse response;
   ICEBERG_ASSIGN_OR_RAISE(auto error_json, GetJsonValue<nlohmann::json>(json, kError));
   ICEBERG_ASSIGN_OR_RAISE(response.error, ErrorModelFromJson(error_json));
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(response));
+  ICEBERG_RETURN_UNEXPECTED(response.Validate());
   return response;
 }
 
@@ -144,7 +143,7 @@ Result<CreateNamespaceRequest> CreateNamespaceRequestFromJson(
   ICEBERG_ASSIGN_OR_RAISE(
       request.properties,
       GetJsonValueOrDefault<decltype(request.properties)>(json, kProperties));
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request));
+  ICEBERG_RETURN_UNEXPECTED(request.Validate());
   return request;
 }
 
@@ -162,7 +161,7 @@ Result<UpdateNamespacePropertiesRequest> UpdateNamespacePropertiesRequestFromJso
       request.removals, GetJsonValueOrDefault<std::vector<std::string>>(json, kRemovals));
   ICEBERG_ASSIGN_OR_RAISE(
       request.updates, GetJsonValueOrDefault<decltype(request.updates)>(json, kUpdates));
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request));
+  ICEBERG_RETURN_UNEXPECTED(request.Validate());
   return request;
 }
 
@@ -183,7 +182,7 @@ Result<RegisterTableRequest> RegisterTableRequestFromJson(const nlohmann::json&
                           GetJsonValue<std::string>(json, kMetadataLocation));
   ICEBERG_ASSIGN_OR_RAISE(request.overwrite,
                           GetJsonValueOrDefault<bool>(json, kOverwrite, false));
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request));
+  ICEBERG_RETURN_UNEXPECTED(request.Validate());
   return request;
 }
 
@@ -201,7 +200,7 @@ Result<RenameTableRequest> RenameTableRequestFromJson(const nlohmann::json& json
   ICEBERG_ASSIGN_OR_RAISE(auto dest_json,
                           GetJsonValue<nlohmann::json>(json, kDestination));
   ICEBERG_ASSIGN_OR_RAISE(request.destination, TableIdentifierFromJson(dest_json));
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request));
+  ICEBERG_RETURN_UNEXPECTED(request.Validate());
   return request;
 }
 
@@ -248,7 +247,7 @@ Result<ListNamespacesResponse> ListNamespacesResponseFromJson(
     ICEBERG_ASSIGN_OR_RAISE(auto ns, NamespaceFromJson(ns_json));
     response.namespaces.push_back(std::move(ns));
   }
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(response));
+  ICEBERG_RETURN_UNEXPECTED(response.Validate());
   return response;
 }
 
@@ -304,7 +303,7 @@ Result<UpdateNamespacePropertiesResponse> UpdateNamespacePropertiesResponseFromJ
       response.removed, GetJsonValueOrDefault<std::vector<std::string>>(json, kRemoved));
   ICEBERG_ASSIGN_OR_RAISE(
       response.missing, GetJsonValueOrDefault<std::vector<std::string>>(json, kMissing));
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(response));
+  ICEBERG_RETURN_UNEXPECTED(response.Validate());
   return response;
 }
 
@@ -329,7 +328,7 @@ Result<ListTablesResponse> ListTablesResponseFromJson(const nlohmann::json& json
     ICEBERG_ASSIGN_OR_RAISE(auto identifier, TableIdentifierFromJson(id_json));
     response.identifiers.push_back(std::move(identifier));
   }
-  ICEBERG_RETURN_UNEXPECTED(Validator::Validate(response));
+  ICEBERG_RETURN_UNEXPECTED(response.Validate());
   return response;
 }
 

src/iceberg/catalog/rest/meson.build

@@ -15,11 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 
-iceberg_rest_sources = files(
-    'json_internal.cc',
-    'rest_catalog.cc',
-    'validator.cc',
-)
+iceberg_rest_sources = files('json_internal.cc', 'rest_catalog.cc')
 # cpr does not export symbols, so on Windows it must
 # be used as a static lib
 cpr_needs_static = (
@@ -50,7 +46,4 @@ iceberg_rest_dep = declare_dependency(
 meson.override_dependency('iceberg-rest', iceberg_rest_dep)
 pkg.generate(iceberg_rest_lib)
 
-install_headers(
-    ['rest_catalog.h', 'types.h', 'json_internal.h', 'validator.h'],
-    subdir: 'iceberg/catalog/rest',
-)
+install_headers(['rest_catalog.h', 'types.h'], subdir: 'iceberg/catalog/rest')

src/iceberg/catalog/rest/types.h

@@ -19,14 +19,18 @@
 
 #pragma once
 
+#include <algorithm>
+#include <format>
 #include <memory>
 #include <string>
 #include <unordered_map>
 #include <vector>
 
 #include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/result.h"
 #include "iceberg/table_identifier.h"
 #include "iceberg/type_fwd.h"
+#include "iceberg/util/macros.h"
 
 /// \file iceberg/catalog/rest/types.h
 /// Request and response types for Iceberg REST Catalog API.
@@ -38,6 +42,15 @@ struct ICEBERG_REST_EXPORT CatalogConfig {
   std::unordered_map<std::string, std::string> defaults;   // required
   std::unordered_map<std::string, std::string> overrides;  // required
   std::vector<std::string> endpoints;
+
+  /// \brief Validates the CatalogConfig.
+  Status Validate() const {
+    // TODO(Li Feiyang): Add an invalidEndpoint test that validates endpoint format.
+    // See:
+    // https://github.com/apache/iceberg/blob/main/core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java#L164
+    // for reference.
+    return {};
+  }
 };
 
 /// \brief JSON error payload returned in a response with further details on the error.
@@ -46,36 +59,88 @@ struct ICEBERG_REST_EXPORT ErrorModel {
   std::string type;     // required
   uint32_t code;        // required
   std::vector<std::string> stack;
+
+  /// \brief Validates the ErrorModel.
+  Status Validate() const {
+    if (message.empty() || type.empty()) {
+      return Invalid("Invalid error model: missing required fields");
+    }
+
+    if (code < 400 || code > 600) {
+      return Invalid("Invalid error model: code {} is out of range [400, 600]", code);
+    }
+
+    // stack is optional, no validation needed
+    return {};
+  }
 };
 
 /// \brief Error response body returned in a response.
 struct ICEBERG_REST_EXPORT ErrorResponse {
   ErrorModel error;  // required
+
+  /// \brief Validates the ErrorResponse.
+  // We don't validate the error field because ErrorModel::Validate has been called in the
+  // FromJson.
+  Status Validate() const { return {}; }
 };
 
 /// \brief Request to create a namespace.
 struct ICEBERG_REST_EXPORT CreateNamespaceRequest {
   Namespace namespace_;  // required
   std::unordered_map<std::string, std::string> properties;
+
+  /// \brief Validates the CreateNamespaceRequest.
+  Status Validate() const { return {}; }
 };
 
 /// \brief Update or delete namespace properties request.
 struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesRequest {
   std::vector<std::string> removals;
   std::unordered_map<std::string, std::string> updates;
+
+  /// \brief Validates the UpdateNamespacePropertiesRequest.
+  Status Validate() const {
+    for (const auto& key : removals) {
+      if (updates.contains(key)) {
+        return Invalid("Duplicate key to update and remove: {}", key);
+      }
+    }
+    return {};
+  }
 };
 
 /// \brief Request to register a table.
 struct ICEBERG_REST_EXPORT RegisterTableRequest {
   std::string name;               // required
   std::string metadata_location;  // required
   bool overwrite = false;
+
+  /// \brief Validates the RegisterTableRequest.
+  Status Validate() const {
+    if (name.empty()) {
+      return Invalid("Missing table name");
+    }
+
+    if (metadata_location.empty()) {
+      return Invalid("Empty metadata location");
+    }
+
+    return {};
+  }
 };
 
 /// \brief Request to rename a table.
 struct ICEBERG_REST_EXPORT RenameTableRequest {
   TableIdentifier source;       // required
   TableIdentifier destination;  // required
+
+  /// \brief Validates the RenameTableRequest.
+  Status Validate() const {
+    ICEBERG_RETURN_UNEXPECTED(source.Validate());
+    ICEBERG_RETURN_UNEXPECTED(destination.Validate());
+    return {};
+  }
 };
 
 /// \brief An opaque token that allows clients to make use of pagination for list APIs.
@@ -87,6 +152,14 @@ struct ICEBERG_REST_EXPORT LoadTableResult {
   std::shared_ptr<TableMetadata> metadata;  // required
   std::unordered_map<std::string, std::string> config;
   // TODO(Li Feiyang): Add std::shared_ptr<StorageCredential> storage_credential;
+
+  /// \brief Validates the LoadTableResult.
+  Status Validate() const {
+    if (!metadata) {
+      return Invalid("Invalid metadata: null");
+    }
+    return {};
+  }
 };
 
 /// \brief Alias of LoadTableResult used as the body of CreateTableResponse
@@ -99,31 +172,46 @@ using LoadTableResponse = LoadTableResult;
 struct ICEBERG_REST_EXPORT ListNamespacesResponse {
   PageToken next_page_token;
   std::vector<Namespace> namespaces;
+
+  /// \brief Validates the ListNamespacesResponse.
+  Status Validate() const { return {}; }
 };
 
 /// \brief Response body after creating a namespace.
 struct ICEBERG_REST_EXPORT CreateNamespaceResponse {
   Namespace namespace_;  // required
   std::unordered_map<std::string, std::string> properties;
+
+  /// \brief Validates the CreateNamespaceResponse.
+  Status Validate() const { return {}; }
 };
 
 /// \brief Response body for loading namespace properties.
 struct ICEBERG_REST_EXPORT GetNamespaceResponse {
   Namespace namespace_;  // required
   std::unordered_map<std::string, std::string> properties;
+
+  /// \brief Validates the GetNamespaceResponse.
+  Status Validate() const { return {}; }
 };
 
 /// \brief Response body after updating namespace properties.
 struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesResponse {
   std::vector<std::string> updated;  // required
   std::vector<std::string> removed;  // required
   std::vector<std::string> missing;
+
+  /// \brief Validates the UpdateNamespacePropertiesResponse.
+  Status Validate() const { return {}; }
 };
 
 /// \brief Response body for listing tables in a namespace.
 struct ICEBERG_REST_EXPORT ListTablesResponse {
   PageToken next_page_token;
   std::vector<TableIdentifier> identifiers;
+
+  /// \brief Validates the ListTablesResponse.
+  Status Validate() const { return {}; }
 };
 
 }  // namespace iceberg::rest