From 9d5a73d3442f8d2ec78c710ca60e997d0df1a987 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Mon, 10 Nov 2025 17:58:51 +0800 Subject: [PATCH 1/3] refactor: Move validation logic to struct member methods --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/catalog/rest/CMakeLists.txt | 2 +- src/iceberg/catalog/rest/json_internal.cc | 21 +++-- src/iceberg/catalog/rest/meson.build | 8 +- .../catalog/rest/{validator.cc => types.cc} | 61 ++++++------- src/iceberg/catalog/rest/types.h | 40 +++++++++ src/iceberg/catalog/rest/validator.h | 87 ------------------- src/iceberg/meson.build | 1 + src/iceberg/table_identifier.cc | 33 +++++++ src/iceberg/table_identifier.h | 4 + 10 files changed, 118 insertions(+), 140 deletions(-) rename src/iceberg/catalog/rest/{validator.cc => types.cc} (60%) delete mode 100644 src/iceberg/catalog/rest/validator.h create mode 100644 src/iceberg/table_identifier.cc diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index a13f095aa..7543d55cd 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -50,6 +50,7 @@ set(ICEBERG_SOURCES sort_order.cc statistics_file.cc table.cc + table_identifier.cc table_metadata.cc table_properties.cc table_requirement.cc diff --git a/src/iceberg/catalog/rest/CMakeLists.txt b/src/iceberg/catalog/rest/CMakeLists.txt index 02440514e..664212f19 100644 --- a/src/iceberg/catalog/rest/CMakeLists.txt +++ b/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 types.cc) set(ICEBERG_REST_STATIC_BUILD_INTERFACE_LIBS) set(ICEBERG_REST_SHARED_BUILD_INTERFACE_LIBS) diff --git a/src/iceberg/catalog/rest/json_internal.cc b/src/iceberg/catalog/rest/json_internal.cc index 404fe21bf..bdec822cc 100644 --- a/src/iceberg/catalog/rest/json_internal.cc +++ b/src/iceberg/catalog/rest/json_internal.cc @@ -26,7 +26,6 @@ #include #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 CatalogConfigFromJson(const nlohmann::json& json) { ICEBERG_ASSIGN_OR_RAISE( config.endpoints, GetJsonValueOrDefault>(json, kEndpoints)); - ICEBERG_RETURN_UNEXPECTED(Validator::Validate(config)); + ICEBERG_RETURN_UNEXPECTED(config.Validate()); return config; } @@ -111,7 +110,7 @@ Result ErrorModelFromJson(const nlohmann::json& json) { ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue(json, kCode)); ICEBERG_ASSIGN_OR_RAISE(error.stack, GetJsonValueOrDefault>(json, kStack)); - ICEBERG_RETURN_UNEXPECTED(Validator::Validate(error)); + ICEBERG_RETURN_UNEXPECTED(error.Validate()); return error; } @@ -125,7 +124,7 @@ Result ErrorResponseFromJson(const nlohmann::json& json) { ErrorResponse response; ICEBERG_ASSIGN_OR_RAISE(auto error_json, GetJsonValue(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 CreateNamespaceRequestFromJson( ICEBERG_ASSIGN_OR_RAISE( request.properties, GetJsonValueOrDefault(json, kProperties)); - ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request)); + ICEBERG_RETURN_UNEXPECTED(request.Validate()); return request; } @@ -162,7 +161,7 @@ Result UpdateNamespacePropertiesRequestFromJso request.removals, GetJsonValueOrDefault>(json, kRemovals)); ICEBERG_ASSIGN_OR_RAISE( request.updates, GetJsonValueOrDefault(json, kUpdates)); - ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request)); + ICEBERG_RETURN_UNEXPECTED(request.Validate()); return request; } @@ -183,7 +182,7 @@ Result RegisterTableRequestFromJson(const nlohmann::json& GetJsonValue(json, kMetadataLocation)); ICEBERG_ASSIGN_OR_RAISE(request.overwrite, GetJsonValueOrDefault(json, kOverwrite, false)); - ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request)); + ICEBERG_RETURN_UNEXPECTED(request.Validate()); return request; } @@ -201,7 +200,7 @@ Result RenameTableRequestFromJson(const nlohmann::json& json ICEBERG_ASSIGN_OR_RAISE(auto dest_json, GetJsonValue(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 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 UpdateNamespacePropertiesResponseFromJ response.removed, GetJsonValueOrDefault>(json, kRemoved)); ICEBERG_ASSIGN_OR_RAISE( response.missing, GetJsonValueOrDefault>(json, kMissing)); - ICEBERG_RETURN_UNEXPECTED(Validator::Validate(response)); + ICEBERG_RETURN_UNEXPECTED(response.Validate()); return response; } @@ -329,7 +328,7 @@ Result 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; } diff --git a/src/iceberg/catalog/rest/meson.build b/src/iceberg/catalog/rest/meson.build index e8edc35c0..3cb3dbe5e 100644 --- a/src/iceberg/catalog/rest/meson.build +++ b/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', 'types.cc') # cpr does not export symbols, so on Windows it must # be used as a static lib cpr_needs_static = ( @@ -51,6 +47,6 @@ 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'], + ['rest_catalog.h', 'types.h', 'json_internal.h'], subdir: 'iceberg/catalog/rest', ) diff --git a/src/iceberg/catalog/rest/validator.cc b/src/iceberg/catalog/rest/types.cc similarity index 60% rename from src/iceberg/catalog/rest/validator.cc rename to src/iceberg/catalog/rest/types.cc index de25fa334..9050d221a 100644 --- a/src/iceberg/catalog/rest/validator.cc +++ b/src/iceberg/catalog/rest/types.cc @@ -17,13 +17,13 @@ * under the License. */ -#include "iceberg/catalog/rest/validator.h" +#include "iceberg/catalog/rest/types.h" #include #include -#include "iceberg/catalog/rest/types.h" #include "iceberg/result.h" +#include "iceberg/table_identifier.h" #include "iceberg/util/formatter_internal.h" #include "iceberg/util/macros.h" @@ -31,7 +31,7 @@ namespace iceberg::rest { // Configuration and Error types -Status Validator::Validate(const CatalogConfig& config) { +Status CatalogConfig::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 @@ -39,13 +39,13 @@ Status Validator::Validate(const CatalogConfig& config) { return {}; } -Status Validator::Validate(const ErrorModel& error) { - if (error.message.empty() || error.type.empty()) { +Status ErrorModel::Validate() const { + if (message.empty() || type.empty()) { return Invalid("Invalid error model: missing required fields"); } - if (error.code < 400 || error.code > 600) { - return Invalid("Invalid error model: code {} is out of range [400, 600]", error.code); + if (code < 400 || code > 600) { + return Invalid("Invalid error model: code {} is out of range [400, 600]", code); } // stack is optional, no validation needed @@ -54,21 +54,21 @@ Status Validator::Validate(const ErrorModel& error) { // We don't validate the error field because ErrorModel::Validate has been called in the // FromJson. -Status Validator::Validate(const ErrorResponse& response) { return {}; } +Status ErrorResponse::Validate() const { return {}; } // Namespace operations -Status Validator::Validate(const ListNamespacesResponse& response) { return {}; } +Status ListNamespacesResponse::Validate() const { return {}; } -Status Validator::Validate(const CreateNamespaceRequest& request) { return {}; } +Status CreateNamespaceRequest::Validate() const { return {}; } -Status Validator::Validate(const CreateNamespaceResponse& response) { return {}; } +Status CreateNamespaceResponse::Validate() const { return {}; } -Status Validator::Validate(const GetNamespaceResponse& response) { return {}; } +Status GetNamespaceResponse::Validate() const { return {}; } -Status Validator::Validate(const UpdateNamespacePropertiesRequest& request) { +Status UpdateNamespacePropertiesRequest::Validate() const { // keys in updates and removals must not overlap - if (request.removals.empty() || request.updates.empty()) { + if (removals.empty() || updates.empty()) { return {}; } @@ -83,9 +83,9 @@ Status Validator::Validate(const UpdateNamespacePropertiesRequest& request) { }; auto sorted_removals = - extract_and_sort(request.removals, [](const auto& s) -> const auto& { return s; }); + extract_and_sort(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; }); + updates, [](const auto& pair) -> const auto& { return pair.first; }); std::vector common; std::ranges::set_intersection(sorted_removals, sorted_update_keys, @@ -99,43 +99,34 @@ Status Validator::Validate(const UpdateNamespacePropertiesRequest& request) { return {}; } -Status Validator::Validate(const UpdateNamespacePropertiesResponse& response) { - return {}; -} +Status UpdateNamespacePropertiesResponse::Validate() const { return {}; } // Table operations -Status Validator::Validate(const ListTablesResponse& response) { return {}; } +Status ListTablesResponse::Validate() const { return {}; } -Status Validator::Validate(const LoadTableResult& result) { - if (!result.metadata) { +Status LoadTableResult::Validate() const { + if (!metadata) { return Invalid("Invalid metadata: null"); } return {}; } -Status Validator::Validate(const RegisterTableRequest& request) { - if (request.name.empty()) { +Status RegisterTableRequest::Validate() const { + if (name.empty()) { return Invalid("Missing table name"); } - if (request.metadata_location.empty()) { + if (metadata_location.empty()) { return Invalid("Empty metadata location"); } return {}; } -Status Validator::Validate(const RenameTableRequest& request) { - ICEBERG_RETURN_UNEXPECTED(Validate(request.source)); - ICEBERG_RETURN_UNEXPECTED(Validate(request.destination)); - return {}; -} - -Status Validator::Validate(const TableIdentifier& identifier) { - if (identifier.name.empty()) { - return Invalid("Invalid table identifier: missing table name"); - } +Status RenameTableRequest::Validate() const { + ICEBERG_RETURN_UNEXPECTED(source.Validate()); + ICEBERG_RETURN_UNEXPECTED(destination.Validate()); return {}; } diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index 15e9831b8..9612327ab 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -25,6 +25,7 @@ #include #include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/result.h" #include "iceberg/table_identifier.h" #include "iceberg/type_fwd.h" @@ -38,6 +39,9 @@ struct ICEBERG_REST_EXPORT CatalogConfig { std::unordered_map defaults; // required std::unordered_map overrides; // required std::vector endpoints; + + /// \brief Validates the CatalogConfig. + Status Validate() const; }; /// \brief JSON error payload returned in a response with further details on the error. @@ -46,23 +50,35 @@ struct ICEBERG_REST_EXPORT ErrorModel { std::string type; // required uint32_t code; // required std::vector stack; + + /// \brief Validates the ErrorModel. + Status Validate() const; }; /// \brief Error response body returned in a response. struct ICEBERG_REST_EXPORT ErrorResponse { ErrorModel error; // required + + /// \brief Validates the ErrorResponse. + Status Validate() const; }; /// \brief Request to create a namespace. struct ICEBERG_REST_EXPORT CreateNamespaceRequest { Namespace namespace_; // required std::unordered_map properties; + + /// \brief Validates the CreateNamespaceRequest. + Status Validate() const; }; /// \brief Update or delete namespace properties request. struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesRequest { std::vector removals; std::unordered_map updates; + + /// \brief Validates the UpdateNamespacePropertiesRequest. + Status Validate() const; }; /// \brief Request to register a table. @@ -70,12 +86,18 @@ struct ICEBERG_REST_EXPORT RegisterTableRequest { std::string name; // required std::string metadata_location; // required bool overwrite = false; + + /// \brief Validates the RegisterTableRequest. + Status Validate() const; }; /// \brief Request to rename a table. struct ICEBERG_REST_EXPORT RenameTableRequest { TableIdentifier source; // required TableIdentifier destination; // required + + /// \brief Validates the RenameTableRequest. + Status Validate() const; }; /// \brief An opaque token that allows clients to make use of pagination for list APIs. @@ -87,6 +109,9 @@ struct ICEBERG_REST_EXPORT LoadTableResult { std::shared_ptr metadata; // required std::unordered_map config; // TODO(Li Feiyang): Add std::shared_ptr storage_credential; + + /// \brief Validates the LoadTableResult. + Status Validate() const; }; /// \brief Alias of LoadTableResult used as the body of CreateTableResponse @@ -99,18 +124,27 @@ using LoadTableResponse = LoadTableResult; struct ICEBERG_REST_EXPORT ListNamespacesResponse { PageToken next_page_token; std::vector namespaces; + + /// \brief Validates the ListNamespacesResponse. + Status Validate() const; }; /// \brief Response body after creating a namespace. struct ICEBERG_REST_EXPORT CreateNamespaceResponse { Namespace namespace_; // required std::unordered_map properties; + + /// \brief Validates the CreateNamespaceResponse. + Status Validate() const; }; /// \brief Response body for loading namespace properties. struct ICEBERG_REST_EXPORT GetNamespaceResponse { Namespace namespace_; // required std::unordered_map properties; + + /// \brief Validates the GetNamespaceResponse. + Status Validate() const; }; /// \brief Response body after updating namespace properties. @@ -118,12 +152,18 @@ struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesResponse { std::vector updated; // required std::vector removed; // required std::vector missing; + + /// \brief Validates the UpdateNamespacePropertiesResponse. + Status Validate() const; }; /// \brief Response body for listing tables in a namespace. struct ICEBERG_REST_EXPORT ListTablesResponse { PageToken next_page_token; std::vector identifiers; + + /// \brief Validates the ListTablesResponse. + Status Validate() const; }; } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/validator.h b/src/iceberg/catalog/rest/validator.h deleted file mode 100644 index 2c80cabc0..000000000 --- a/src/iceberg/catalog/rest/validator.h +++ /dev/null @@ -1,87 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -#pragma once - -#include "iceberg/catalog/rest/iceberg_rest_export.h" -#include "iceberg/catalog/rest/types.h" -#include "iceberg/result.h" - -/// \file iceberg/catalog/rest/validator.h -/// Validator for REST Catalog API types. - -namespace iceberg::rest { - -/// \brief Validator for REST Catalog API types. Validation should be called after -/// deserializing objects from external sources to ensure data integrity before the -/// objects are used. -class ICEBERG_REST_EXPORT Validator { - public: - // Configuration and Error types - - /// \brief Validates a CatalogConfig object. - static Status Validate(const CatalogConfig& config); - - /// \brief Validates an ErrorModel object. - static Status Validate(const ErrorModel& error); - - /// \brief Validates an ErrorResponse object. - static Status Validate(const ErrorResponse& response); - - // Namespace operations - - /// \brief Validates a ListNamespacesResponse object. - static Status Validate(const ListNamespacesResponse& response); - - /// \brief Validates a CreateNamespaceRequest object. - static Status Validate(const CreateNamespaceRequest& request); - - /// \brief Validates a CreateNamespaceResponse object. - static Status Validate(const CreateNamespaceResponse& response); - - /// \brief Validates a GetNamespaceResponse object. - static Status Validate(const GetNamespaceResponse& response); - - /// \brief Validates an UpdateNamespacePropertiesRequest object. - static Status Validate(const UpdateNamespacePropertiesRequest& request); - - /// \brief Validates an UpdateNamespacePropertiesResponse object. - static Status Validate(const UpdateNamespacePropertiesResponse& response); - - // Table operations - - /// \brief Validates a ListTablesResponse object. - static Status Validate(const ListTablesResponse& response); - - /// \brief Validates a LoadTableResult object. - static Status Validate(const LoadTableResult& result); - - /// \brief Validates a RegisterTableRequest object. - static Status Validate(const RegisterTableRequest& request); - - /// \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 diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 1b24f85fb..19f22d212 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -72,6 +72,7 @@ iceberg_sources = files( 'sort_order.cc', 'statistics_file.cc', 'table.cc', + 'table_identifier.cc', 'table_metadata.cc', 'table_properties.cc', 'table_requirement.cc', diff --git a/src/iceberg/table_identifier.cc b/src/iceberg/table_identifier.cc new file mode 100644 index 000000000..adff16bfb --- /dev/null +++ b/src/iceberg/table_identifier.cc @@ -0,0 +1,33 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/table_identifier.h" + +#include "iceberg/result.h" + +namespace iceberg { + +Status TableIdentifier::Validate() const { + if (name.empty()) { + return Invalid("Invalid table identifier: missing table name"); + } + return {}; +} + +} // namespace iceberg diff --git a/src/iceberg/table_identifier.h b/src/iceberg/table_identifier.h index 9aa5770a1..de30dfbe4 100644 --- a/src/iceberg/table_identifier.h +++ b/src/iceberg/table_identifier.h @@ -26,6 +26,7 @@ #include #include "iceberg/iceberg_export.h" +#include "iceberg/result.h" namespace iceberg { @@ -38,6 +39,9 @@ struct ICEBERG_EXPORT Namespace { struct ICEBERG_EXPORT TableIdentifier { Namespace ns; std::string name; + + /// \brief Validates the TableIdentifier. + Status Validate() const; }; } // namespace iceberg From 66f748ae3042820d35a3c469ffeab110b3b8428e Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Tue, 11 Nov 2025 11:52:22 +0800 Subject: [PATCH 2/3] remove cc file --- src/iceberg/CMakeLists.txt | 1 - src/iceberg/catalog/rest/CMakeLists.txt | 2 +- src/iceberg/catalog/rest/meson.build | 7 +- src/iceberg/catalog/rest/types.cc | 133 ------------------------ src/iceberg/catalog/rest/types.h | 99 +++++++++++++++--- src/iceberg/meson.build | 1 - src/iceberg/table_identifier.cc | 33 ------ src/iceberg/table_identifier.h | 7 +- 8 files changed, 95 insertions(+), 188 deletions(-) delete mode 100644 src/iceberg/catalog/rest/types.cc delete mode 100644 src/iceberg/table_identifier.cc diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 7543d55cd..a13f095aa 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -50,7 +50,6 @@ set(ICEBERG_SOURCES sort_order.cc statistics_file.cc table.cc - table_identifier.cc table_metadata.cc table_properties.cc table_requirement.cc diff --git a/src/iceberg/catalog/rest/CMakeLists.txt b/src/iceberg/catalog/rest/CMakeLists.txt index 664212f19..38d897270 100644 --- a/src/iceberg/catalog/rest/CMakeLists.txt +++ b/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 types.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) diff --git a/src/iceberg/catalog/rest/meson.build b/src/iceberg/catalog/rest/meson.build index 3cb3dbe5e..5f1f635ab 100644 --- a/src/iceberg/catalog/rest/meson.build +++ b/src/iceberg/catalog/rest/meson.build @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -iceberg_rest_sources = files('json_internal.cc', 'rest_catalog.cc', 'types.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 = ( @@ -46,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'], - subdir: 'iceberg/catalog/rest', -) +install_headers(['rest_catalog.h', 'types.h'], subdir: 'iceberg/catalog/rest') diff --git a/src/iceberg/catalog/rest/types.cc b/src/iceberg/catalog/rest/types.cc deleted file mode 100644 index 9050d221a..000000000 --- a/src/iceberg/catalog/rest/types.cc +++ /dev/null @@ -1,133 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -#include "iceberg/catalog/rest/types.h" - -#include -#include - -#include "iceberg/result.h" -#include "iceberg/table_identifier.h" -#include "iceberg/util/formatter_internal.h" -#include "iceberg/util/macros.h" - -namespace iceberg::rest { - -// Configuration and Error types - -Status CatalogConfig::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 {}; -} - -Status ErrorModel::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 {}; -} - -// We don't validate the error field because ErrorModel::Validate has been called in the -// FromJson. -Status ErrorResponse::Validate() const { return {}; } - -// Namespace operations - -Status ListNamespacesResponse::Validate() const { return {}; } - -Status CreateNamespaceRequest::Validate() const { return {}; } - -Status CreateNamespaceResponse::Validate() const { return {}; } - -Status GetNamespaceResponse::Validate() const { return {}; } - -Status UpdateNamespacePropertiesRequest::Validate() const { - // keys in updates and removals must not overlap - if (removals.empty() || updates.empty()) { - return {}; - } - - auto extract_and_sort = [](const auto& container, auto key_extractor) { - std::vector 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(removals, [](const auto& s) -> const auto& { return s; }); - auto sorted_update_keys = extract_and_sort( - updates, [](const auto& pair) -> const auto& { return pair.first; }); - - std::vector common; - std::ranges::set_intersection(sorted_removals, sorted_update_keys, - std::back_inserter(common)); - - if (!common.empty()) { - return Invalid( - "Invalid namespace update: cannot simultaneously set and remove keys: {}", - common); - } - return {}; -} - -Status UpdateNamespacePropertiesResponse::Validate() const { return {}; } - -// Table operations - -Status ListTablesResponse::Validate() const { return {}; } - -Status LoadTableResult::Validate() const { - if (!metadata) { - return Invalid("Invalid metadata: null"); - } - return {}; -} - -Status RegisterTableRequest::Validate() const { - if (name.empty()) { - return Invalid("Missing table name"); - } - - if (metadata_location.empty()) { - return Invalid("Empty metadata location"); - } - - return {}; -} - -Status RenameTableRequest::Validate() const { - ICEBERG_RETURN_UNEXPECTED(source.Validate()); - ICEBERG_RETURN_UNEXPECTED(destination.Validate()); - return {}; -} - -} // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index 9612327ab..0ee51e0ef 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -19,6 +19,8 @@ #pragma once +#include +#include #include #include #include @@ -28,6 +30,8 @@ #include "iceberg/result.h" #include "iceberg/table_identifier.h" #include "iceberg/type_fwd.h" +#include "iceberg/util/formatter_internal.h" +#include "iceberg/util/macros.h" /// \file iceberg/catalog/rest/types.h /// Request and response types for Iceberg REST Catalog API. @@ -41,7 +45,13 @@ struct ICEBERG_REST_EXPORT CatalogConfig { std::vector endpoints; /// \brief Validates the CatalogConfig. - Status Validate() const; + 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. @@ -52,7 +62,18 @@ struct ICEBERG_REST_EXPORT ErrorModel { std::vector stack; /// \brief Validates the ErrorModel. - Status Validate() const; + 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. @@ -60,7 +81,9 @@ struct ICEBERG_REST_EXPORT ErrorResponse { ErrorModel error; // required /// \brief Validates the ErrorResponse. - Status Validate() const; + // 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. @@ -69,7 +92,7 @@ struct ICEBERG_REST_EXPORT CreateNamespaceRequest { std::unordered_map properties; /// \brief Validates the CreateNamespaceRequest. - Status Validate() const; + Status Validate() const { return {}; } }; /// \brief Update or delete namespace properties request. @@ -78,7 +101,38 @@ struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesRequest { std::unordered_map updates; /// \brief Validates the UpdateNamespacePropertiesRequest. - Status Validate() const; + Status Validate() const { + // keys in updates and removals must not overlap + if (removals.empty() || updates.empty()) { + return {}; + } + + auto extract_and_sort = [](const auto& container, auto key_extractor) { + std::vector 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(removals, [](const auto& s) -> const auto& { return s; }); + auto sorted_update_keys = extract_and_sort( + updates, [](const auto& pair) -> const auto& { return pair.first; }); + + std::vector common; + std::ranges::set_intersection(sorted_removals, sorted_update_keys, + std::back_inserter(common)); + + if (!common.empty()) { + return Invalid( + "Invalid namespace update: cannot simultaneously set and remove keys: {}", + common); + } + return {}; + } }; /// \brief Request to register a table. @@ -88,7 +142,17 @@ struct ICEBERG_REST_EXPORT RegisterTableRequest { bool overwrite = false; /// \brief Validates the RegisterTableRequest. - Status Validate() const; + 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. @@ -97,7 +161,11 @@ struct ICEBERG_REST_EXPORT RenameTableRequest { TableIdentifier destination; // required /// \brief Validates the RenameTableRequest. - Status Validate() const; + 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. @@ -111,7 +179,12 @@ struct ICEBERG_REST_EXPORT LoadTableResult { // TODO(Li Feiyang): Add std::shared_ptr storage_credential; /// \brief Validates the LoadTableResult. - Status Validate() const; + Status Validate() const { + if (!metadata) { + return Invalid("Invalid metadata: null"); + } + return {}; + } }; /// \brief Alias of LoadTableResult used as the body of CreateTableResponse @@ -126,7 +199,7 @@ struct ICEBERG_REST_EXPORT ListNamespacesResponse { std::vector namespaces; /// \brief Validates the ListNamespacesResponse. - Status Validate() const; + Status Validate() const { return {}; } }; /// \brief Response body after creating a namespace. @@ -135,7 +208,7 @@ struct ICEBERG_REST_EXPORT CreateNamespaceResponse { std::unordered_map properties; /// \brief Validates the CreateNamespaceResponse. - Status Validate() const; + Status Validate() const { return {}; } }; /// \brief Response body for loading namespace properties. @@ -144,7 +217,7 @@ struct ICEBERG_REST_EXPORT GetNamespaceResponse { std::unordered_map properties; /// \brief Validates the GetNamespaceResponse. - Status Validate() const; + Status Validate() const { return {}; } }; /// \brief Response body after updating namespace properties. @@ -154,7 +227,7 @@ struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesResponse { std::vector missing; /// \brief Validates the UpdateNamespacePropertiesResponse. - Status Validate() const; + Status Validate() const { return {}; } }; /// \brief Response body for listing tables in a namespace. @@ -163,7 +236,7 @@ struct ICEBERG_REST_EXPORT ListTablesResponse { std::vector identifiers; /// \brief Validates the ListTablesResponse. - Status Validate() const; + Status Validate() const { return {}; } }; } // namespace iceberg::rest diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 19f22d212..1b24f85fb 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -72,7 +72,6 @@ iceberg_sources = files( 'sort_order.cc', 'statistics_file.cc', 'table.cc', - 'table_identifier.cc', 'table_metadata.cc', 'table_properties.cc', 'table_requirement.cc', diff --git a/src/iceberg/table_identifier.cc b/src/iceberg/table_identifier.cc deleted file mode 100644 index adff16bfb..000000000 --- a/src/iceberg/table_identifier.cc +++ /dev/null @@ -1,33 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -#include "iceberg/table_identifier.h" - -#include "iceberg/result.h" - -namespace iceberg { - -Status TableIdentifier::Validate() const { - if (name.empty()) { - return Invalid("Invalid table identifier: missing table name"); - } - return {}; -} - -} // namespace iceberg diff --git a/src/iceberg/table_identifier.h b/src/iceberg/table_identifier.h index de30dfbe4..b145e75f3 100644 --- a/src/iceberg/table_identifier.h +++ b/src/iceberg/table_identifier.h @@ -41,7 +41,12 @@ struct ICEBERG_EXPORT TableIdentifier { std::string name; /// \brief Validates the TableIdentifier. - Status Validate() const; + Status Validate() const { + if (name.empty()) { + return Invalid("Invalid table identifier: missing table name"); + } + return {}; + } }; } // namespace iceberg From 14c700704b9f9def21f02549ea5635f8ba27f969 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Tue, 11 Nov 2025 15:45:39 +0800 Subject: [PATCH 3/3] fix review --- src/iceberg/catalog/rest/types.h | 31 +++---------------------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index 0ee51e0ef..2e32f967f 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -30,7 +30,6 @@ #include "iceberg/result.h" #include "iceberg/table_identifier.h" #include "iceberg/type_fwd.h" -#include "iceberg/util/formatter_internal.h" #include "iceberg/util/macros.h" /// \file iceberg/catalog/rest/types.h @@ -102,34 +101,10 @@ struct ICEBERG_REST_EXPORT UpdateNamespacePropertiesRequest { /// \brief Validates the UpdateNamespacePropertiesRequest. Status Validate() const { - // keys in updates and removals must not overlap - if (removals.empty() || updates.empty()) { - return {}; - } - - auto extract_and_sort = [](const auto& container, auto key_extractor) { - std::vector result; - result.reserve(container.size()); - for (const auto& item : container) { - result.push_back(std::string_view{key_extractor(item)}); + for (const auto& key : removals) { + if (updates.contains(key)) { + return Invalid("Duplicate key to update and remove: {}", key); } - std::ranges::sort(result); - return result; - }; - - auto sorted_removals = - extract_and_sort(removals, [](const auto& s) -> const auto& { return s; }); - auto sorted_update_keys = extract_and_sort( - updates, [](const auto& pair) -> const auto& { return pair.first; }); - - std::vector common; - std::ranges::set_intersection(sorted_removals, sorted_update_keys, - std::back_inserter(common)); - - if (!common.empty()) { - return Invalid( - "Invalid namespace update: cannot simultaneously set and remove keys: {}", - common); } return {}; }