From ce743cc972b5e16bd2151c3d1b076790714e7acf Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Fri, 12 Dec 2025 16:48:47 +0800 Subject: [PATCH 1/7] feat: Implement server-provided endpoints configuration for REST --- src/iceberg/catalog/rest/CMakeLists.txt | 1 + src/iceberg/catalog/rest/endpoint.cc | 89 ++++++ src/iceberg/catalog/rest/endpoint.h | 148 ++++++++++ src/iceberg/catalog/rest/json_internal.cc | 16 +- src/iceberg/catalog/rest/meson.build | 2 + src/iceberg/catalog/rest/rest_catalog.cc | 131 ++++++--- src/iceberg/catalog/rest/rest_catalog.h | 5 +- src/iceberg/catalog/rest/rest_util.cc | 9 + src/iceberg/catalog/rest/rest_util.h | 10 + src/iceberg/catalog/rest/type_fwd.h | 1 + src/iceberg/catalog/rest/types.h | 11 +- src/iceberg/test/CMakeLists.txt | 5 +- src/iceberg/test/endpoint_test.cc | 290 ++++++++++++++++++++ src/iceberg/test/rest_catalog_test.cc | 38 +++ src/iceberg/test/rest_json_internal_test.cc | 31 ++- src/iceberg/test/rest_util_test.cc | 17 ++ 16 files changed, 751 insertions(+), 53 deletions(-) create mode 100644 src/iceberg/catalog/rest/endpoint.cc create mode 100644 src/iceberg/catalog/rest/endpoint.h create mode 100644 src/iceberg/test/endpoint_test.cc diff --git a/src/iceberg/catalog/rest/CMakeLists.txt b/src/iceberg/catalog/rest/CMakeLists.txt index 881b3d39a..86f8cacd6 100644 --- a/src/iceberg/catalog/rest/CMakeLists.txt +++ b/src/iceberg/catalog/rest/CMakeLists.txt @@ -18,6 +18,7 @@ set(ICEBERG_REST_SOURCES rest_catalog.cc catalog_properties.cc + endpoint.cc error_handlers.cc http_client.cc json_internal.cc diff --git a/src/iceberg/catalog/rest/endpoint.cc b/src/iceberg/catalog/rest/endpoint.cc new file mode 100644 index 000000000..549677c30 --- /dev/null +++ b/src/iceberg/catalog/rest/endpoint.cc @@ -0,0 +1,89 @@ +/* + * 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/endpoint.h" + +#include + +namespace iceberg::rest { + +constexpr std::string_view ToString(HttpMethod method) { + switch (method) { + case HttpMethod::GET: + return "GET"; + case HttpMethod::POST: + return "POST"; + case HttpMethod::PUT: + return "PUT"; + case HttpMethod::DELETE: + return "DELETE"; + case HttpMethod::HEAD: + return "HEAD"; + } + return "UNKNOWN"; +} + +Result Endpoint::Create(HttpMethod method, std::string path_template) { + if (path_template.empty()) { + return InvalidArgument("Path template cannot be empty"); + } + return Endpoint(method, path_template); +} + +Result Endpoint::FromString(std::string_view str) { + auto space_pos = str.find(' '); + if (space_pos == std::string_view::npos || + str.find(' ', space_pos + 1) != std::string_view::npos) { + return InvalidArgument( + "Invalid endpoint format (must consist of two elements separated by a single " + "space): {}", + str); + } + + auto method_str = str.substr(0, space_pos); + auto path_str = str.substr(space_pos + 1); + + if (path_str.empty()) { + return InvalidArgument("Invalid endpoint format: path is empty"); + } + + // Parse HTTP method + HttpMethod method; + if (method_str == "GET") { + method = HttpMethod::GET; + } else if (method_str == "POST") { + method = HttpMethod::POST; + } else if (method_str == "PUT") { + method = HttpMethod::PUT; + } else if (method_str == "DELETE") { + method = HttpMethod::DELETE; + } else if (method_str == "HEAD") { + method = HttpMethod::HEAD; + } else { + return InvalidArgument("Invalid HTTP method: '{}'", method_str); + } + + return Create(method, std::string(path_str)); +} + +std::string Endpoint::ToString() const { + return std::format("{} {}", rest::ToString(method_), path_template_); +} + +} // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/endpoint.h b/src/iceberg/catalog/rest/endpoint.h new file mode 100644 index 000000000..3291ed2f6 --- /dev/null +++ b/src/iceberg/catalog/rest/endpoint.h @@ -0,0 +1,148 @@ +/* + * 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 +#include +#include + +#include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/result.h" + +/// \file iceberg/catalog/rest/endpoint.h +/// Endpoint definitions for Iceberg REST API operations. + +namespace iceberg::rest { + +/// \brief HTTP method enumeration. +enum class HttpMethod : uint8_t { GET, POST, PUT, DELETE, HEAD }; + +/// \brief Convert HttpMethod to string representation. +constexpr std::string_view ToString(HttpMethod method); + +/// \brief An Endpoint is an immutable value object identifying a specific REST API +/// operation. It consists of: +/// - HTTP method (GET, POST, DELETE, etc.) +/// - Path template (e.g., "/v1/{prefix}/namespaces/{namespace}") +class ICEBERG_REST_EXPORT Endpoint { + public: + /// \brief Create an endpoint with method and path template. + /// + /// \param method HTTP method (GET, POST, etc.) + /// \param path_template Path template with placeholders (e.g., "/v1/{prefix}/tables") + /// \return Endpoint instance or error if invalid + static Result Create(HttpMethod method, std::string path_template); + + /// \brief Parse endpoint from string representation. + /// + /// \param str String in format "METHOD /path/template" (e.g., "GET /v1/namespaces") + /// \return Endpoint instance or error if malformed + static Result FromString(std::string_view str); + + /// \brief Get the HTTP method. + constexpr HttpMethod method() const { return method_; } + + /// \brief Get the path template. + std::string_view path_template() const { return path_template_; } + + /// \brief Serialize to "METHOD /path" format. + std::string ToString() const; + + /// \brief Equality comparison operator. + constexpr bool operator==(const Endpoint& other) const { + return method_ == other.method_ && path_template_ == other.path_template_; + } + + /// \brief Three-way comparison operator (C++20). + constexpr auto operator<=>(const Endpoint& other) const { + if (auto cmp = method_ <=> other.method_; cmp != 0) { + return cmp; + } + return path_template_ <=> other.path_template_; + } + + // Namespace endpoints + static constexpr Endpoint ListNamespaces() { + return {HttpMethod::GET, "/v1/{prefix}/namespaces"}; + } + static constexpr Endpoint GetNamespaceProperties() { + return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}"}; + } + static constexpr Endpoint NamespaceExists() { + return {HttpMethod::HEAD, "/v1/{prefix}/namespaces/{namespace}"}; + } + static constexpr Endpoint CreateNamespace() { + return {HttpMethod::POST, "/v1/{prefix}/namespaces"}; + } + static constexpr Endpoint UpdateNamespace() { + return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/properties"}; + } + static constexpr Endpoint DropNamespace() { + return {HttpMethod::DELETE, "/v1/{prefix}/namespaces/{namespace}"}; + } + + // Table endpoints + static constexpr Endpoint ListTables() { + return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}/tables"}; + } + static constexpr Endpoint LoadTable() { + return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + } + static constexpr Endpoint TableExists() { + return {HttpMethod::HEAD, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + } + static constexpr Endpoint CreateTable() { + return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/tables"}; + } + static constexpr Endpoint UpdateTable() { + return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + } + static constexpr Endpoint DeleteTable() { + return {HttpMethod::DELETE, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + } + static constexpr Endpoint RenameTable() { + return {HttpMethod::POST, "/v1/{prefix}/tables/rename"}; + } + static constexpr Endpoint RegisterTable() { + return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/register"}; + } + static constexpr Endpoint ReportMetrics() { + return {HttpMethod::POST, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"}; + } + static constexpr Endpoint TableCredentials() { + return {HttpMethod::GET, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials"}; + } + + // Transaction endpoints + static constexpr Endpoint CommitTransaction() { + return {HttpMethod::POST, "/v1/{prefix}/transactions/commit"}; + } + + private: + Endpoint(HttpMethod method, std::string_view path_template) + : method_(method), path_template_(path_template) {} + + HttpMethod method_; + std::string path_template_; +}; + +} // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/json_internal.cc b/src/iceberg/catalog/rest/json_internal.cc index c60b406d9..9db2ca0a6 100644 --- a/src/iceberg/catalog/rest/json_internal.cc +++ b/src/iceberg/catalog/rest/json_internal.cc @@ -73,7 +73,10 @@ nlohmann::json ToJson(const CatalogConfig& config) { nlohmann::json json; json[kOverrides] = config.overrides; json[kDefaults] = config.defaults; - SetContainerField(json, kEndpoints, config.endpoints); + for (const auto& endpoint : config.endpoints) { + auto endpoint_str = endpoint.ToString(); + json[kEndpoints].push_back(endpoint_str); + } return json; } @@ -85,8 +88,15 @@ Result CatalogConfigFromJson(const nlohmann::json& json) { ICEBERG_ASSIGN_OR_RAISE( config.defaults, GetJsonValueOrDefault(json, kDefaults)); ICEBERG_ASSIGN_OR_RAISE( - config.endpoints, - GetJsonValueOrDefault>(json, kEndpoints)); + auto endpoints, GetJsonValueOrDefault>(json, kEndpoints)); + for (const auto& endpoint_str : endpoints) { + auto endpoint_result = Endpoint::FromString(endpoint_str); + if (!endpoint_result.has_value()) { + // Convert to JsonParseError in JSON deserialization context + return JsonParseError("{}", endpoint_result.error().message); + } + config.endpoints.push_back(std::move(*endpoint_result)); + } ICEBERG_RETURN_UNEXPECTED(config.Validate()); return config; } diff --git a/src/iceberg/catalog/rest/meson.build b/src/iceberg/catalog/rest/meson.build index 8378b2a8c..cda90c616 100644 --- a/src/iceberg/catalog/rest/meson.build +++ b/src/iceberg/catalog/rest/meson.build @@ -17,6 +17,7 @@ iceberg_rest_sources = files( 'catalog_properties.cc', + 'endpoint.cc', 'error_handlers.cc', 'http_client.cc', 'json_internal.cc', @@ -58,6 +59,7 @@ install_headers( [ 'catalog_properties.h', 'constant.h', + 'endpoint.h', 'error_handlers.h', 'http_client.h', 'iceberg_rest_export.h', diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 4a77f6585..5aaffe164 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -20,6 +20,7 @@ #include "iceberg/catalog/rest/rest_catalog.h" #include +#include #include #include @@ -27,6 +28,7 @@ #include "iceberg/catalog/rest/catalog_properties.h" #include "iceberg/catalog/rest/constant.h" +#include "iceberg/catalog/rest/endpoint.h" #include "iceberg/catalog/rest/error_handlers.h" #include "iceberg/catalog/rest/http_client.h" #include "iceberg/catalog/rest/json_internal.h" @@ -44,20 +46,30 @@ namespace iceberg::rest { namespace { -// Fetch server config and merge it with client config -Result> FetchConfig( - const ResourcePaths& paths, const RestCatalogProperties& config) { - ICEBERG_ASSIGN_OR_RAISE(auto config_endpoint, paths.Config()); - HttpClient client(config.ExtractHeaders()); +/// \brief Get the default set of endpoints for backwards compatibility according to the +/// iceberg rest spec. +std::set GetDefaultEndpoints() { + return { + Endpoint::ListNamespaces(), Endpoint::GetNamespaceProperties(), + Endpoint::CreateNamespace(), Endpoint::UpdateNamespace(), + Endpoint::DropNamespace(), Endpoint::ListTables(), + Endpoint::LoadTable(), Endpoint::CreateTable(), + Endpoint::UpdateTable(), Endpoint::DeleteTable(), + Endpoint::RenameTable(), Endpoint::RegisterTable(), + Endpoint::ReportMetrics(), Endpoint::CommitTransaction(), + }; +} + +/// \brief Fetch server config and merge it with client config +Result FetchServerConfig(const ResourcePaths& paths, + const RestCatalogProperties& current_config) { + ICEBERG_ASSIGN_OR_RAISE(auto config_path, paths.Config()); + HttpClient client(current_config.ExtractHeaders()); ICEBERG_ASSIGN_OR_RAISE(const auto response, - client.Get(config_endpoint, /*params=*/{}, /*headers=*/{}, + client.Get(config_path, /*params=*/{}, /*headers=*/{}, *DefaultErrorHandler::Instance())); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); - ICEBERG_ASSIGN_OR_RAISE(auto server_config, CatalogConfigFromJson(json)); - - // Merge priorities: server overrides > client config > server defaults - return RestCatalogProperties::FromMap( - MergeConfigs(server_config.overrides, config.configs(), server_config.defaults)); + return CatalogConfigFromJson(json); } } // namespace @@ -65,32 +77,54 @@ Result> FetchConfig( RestCatalog::~RestCatalog() = default; Result> RestCatalog::Make( - const RestCatalogProperties& config) { - ICEBERG_ASSIGN_OR_RAISE(auto uri, config.Uri()); + const RestCatalogProperties& initial_config) { + ICEBERG_ASSIGN_OR_RAISE(auto uri, initial_config.Uri()); ICEBERG_ASSIGN_OR_RAISE( - auto paths, ResourcePaths::Make(std::string(TrimTrailingSlash(uri)), - config.Get(RestCatalogProperties::kPrefix))); - ICEBERG_ASSIGN_OR_RAISE(auto final_config, FetchConfig(*paths, config)); + auto paths, + ResourcePaths::Make(std::string(TrimTrailingSlash(uri)), + initial_config.Get(RestCatalogProperties::kPrefix))); + // get the raw config from server + ICEBERG_ASSIGN_OR_RAISE(auto server_config, FetchServerConfig(*paths, initial_config)); + + std::unique_ptr final_config = + RestCatalogProperties::FromMap(MergeConfigs( + server_config.overrides, initial_config.configs(), server_config.defaults)); + + std::set endpoints; + if (!server_config.endpoints.empty()) { + // Endpoints are already parsed during JSON deserialization, just convert to set + endpoints = std::set(server_config.endpoints.begin(), + server_config.endpoints.end()); + } else { + // If a server does not send the endpoints field, use default set of endpoints + // for backwards compatibility with legacy servers + endpoints = GetDefaultEndpoints(); + } // Update resource paths based on the final config ICEBERG_ASSIGN_OR_RAISE(auto final_uri, final_config->Uri()); ICEBERG_RETURN_UNEXPECTED(paths->SetBaseUri(std::string(TrimTrailingSlash(final_uri)))); return std::unique_ptr( - new RestCatalog(std::move(final_config), std::move(paths))); + new RestCatalog(std::move(final_config), std::move(paths), endpoints)); } RestCatalog::RestCatalog(std::unique_ptr config, - std::unique_ptr paths) + std::unique_ptr paths, + std::set endpoints) : config_(std::move(config)), client_(std::make_unique(config_->ExtractHeaders())), paths_(std::move(paths)), - name_(config_->Get(RestCatalogProperties::kName)) {} + name_(config_->Get(RestCatalogProperties::kName)), + supported_endpoints_(std::move(endpoints)) {} std::string_view RestCatalog::name() const { return name_; } Result> RestCatalog::ListNamespaces(const Namespace& ns) const { - ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespaces()); + ICEBERG_RETURN_UNEXPECTED( + CheckEndpoint(supported_endpoints_, Endpoint::ListNamespaces())); + + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespaces()); std::vector result; std::string next_token; while (true) { @@ -101,9 +135,9 @@ Result> RestCatalog::ListNamespaces(const Namespace& ns) if (!next_token.empty()) { params[kQueryParamPageToken] = next_token; } - ICEBERG_ASSIGN_OR_RAISE(const auto response, - client_->Get(endpoint, params, /*headers=*/{}, - *NamespaceErrorHandler::Instance())); + ICEBERG_ASSIGN_OR_RAISE( + const auto response, + client_->Get(path, params, /*headers=*/{}, *NamespaceErrorHandler::Instance())); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); ICEBERG_ASSIGN_OR_RAISE(auto list_response, ListNamespacesResponseFromJson(json)); result.insert(result.end(), list_response.namespaces.begin(), @@ -118,11 +152,14 @@ Result> RestCatalog::ListNamespaces(const Namespace& ns) Status RestCatalog::CreateNamespace( const Namespace& ns, const std::unordered_map& properties) { - ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespaces()); + ICEBERG_RETURN_UNEXPECTED( + CheckEndpoint(supported_endpoints_, Endpoint::CreateNamespace())); + + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespaces()); CreateNamespaceRequest request{.namespace_ = ns, .properties = properties}; ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request))); ICEBERG_ASSIGN_OR_RAISE(const auto response, - client_->Post(endpoint, json_request, /*headers=*/{}, + client_->Post(path, json_request, /*headers=*/{}, *NamespaceErrorHandler::Instance())); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); ICEBERG_ASSIGN_OR_RAISE(auto create_response, CreateNamespaceResponseFromJson(json)); @@ -131,9 +168,12 @@ Status RestCatalog::CreateNamespace( Result> RestCatalog::GetNamespaceProperties( const Namespace& ns) const { - ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespace_(ns)); + ICEBERG_RETURN_UNEXPECTED( + CheckEndpoint(supported_endpoints_, Endpoint::GetNamespaceProperties())); + + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns)); ICEBERG_ASSIGN_OR_RAISE(const auto response, - client_->Get(endpoint, /*params=*/{}, /*headers=*/{}, + client_->Get(path, /*params=*/{}, /*headers=*/{}, *NamespaceErrorHandler::Instance())); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); ICEBERG_ASSIGN_OR_RAISE(auto get_response, GetNamespaceResponseFromJson(json)); @@ -141,19 +181,37 @@ Result> RestCatalog::GetNamespacePr } Status RestCatalog::DropNamespace(const Namespace& ns) { - ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespace_(ns)); + ICEBERG_RETURN_UNEXPECTED( + CheckEndpoint(supported_endpoints_, Endpoint::DropNamespace())); + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns)); ICEBERG_ASSIGN_OR_RAISE( const auto response, - client_->Delete(endpoint, /*headers=*/{}, *DropNamespaceErrorHandler::Instance())); + client_->Delete(path, /*headers=*/{}, *DropNamespaceErrorHandler::Instance())); return {}; } Result RestCatalog::NamespaceExists(const Namespace& ns) const { - ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->Namespace_(ns)); - // TODO(Feiyang Li): checks if the server supports the namespace exists endpoint, if - // not, triggers a fallback mechanism + auto check = CheckEndpoint(supported_endpoints_, Endpoint::NamespaceExists()); + // If server does not support HEAD endpoint, fall back to GetNamespaceProperties + if (!check.has_value()) { + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns)); + auto response = client_->Get(path, /*params=*/{}, /*headers=*/{}, + *NamespaceErrorHandler::Instance()); + if (!response.has_value()) { + const auto& error = response.error(); + // catch NoSuchNamespaceException/404 and return false + if (error.kind == ErrorKind::kNoSuchNamespace) { + return false; + } + ICEBERG_RETURN_UNEXPECTED(response); + } + // GET succeeded, namespace exists + return true; + } + + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns)); auto response_or_error = - client_->Head(endpoint, /*headers=*/{}, *NamespaceErrorHandler::Instance()); + client_->Head(path, /*headers=*/{}, *NamespaceErrorHandler::Instance()); if (!response_or_error.has_value()) { const auto& error = response_or_error.error(); // catch NoSuchNamespaceException/404 and return false @@ -168,13 +226,16 @@ Result RestCatalog::NamespaceExists(const Namespace& ns) const { Status RestCatalog::UpdateNamespaceProperties( const Namespace& ns, const std::unordered_map& updates, const std::unordered_set& removals) { - ICEBERG_ASSIGN_OR_RAISE(auto endpoint, paths_->NamespaceProperties(ns)); + ICEBERG_RETURN_UNEXPECTED( + CheckEndpoint(supported_endpoints_, Endpoint::UpdateNamespace())); + + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->NamespaceProperties(ns)); UpdateNamespacePropertiesRequest request{ .removals = std::vector(removals.begin(), removals.end()), .updates = updates}; ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request))); ICEBERG_ASSIGN_OR_RAISE(const auto response, - client_->Post(endpoint, json_request, /*headers=*/{}, + client_->Post(path, json_request, /*headers=*/{}, *NamespaceErrorHandler::Instance())); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); ICEBERG_ASSIGN_OR_RAISE(auto update_response, diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index 4e191e86f..0da888da1 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -20,9 +20,11 @@ #pragma once #include +#include #include #include "iceberg/catalog.h" +#include "iceberg/catalog/rest/endpoint.h" #include "iceberg/catalog/rest/iceberg_rest_export.h" #include "iceberg/catalog/rest/type_fwd.h" #include "iceberg/result.h" @@ -98,12 +100,13 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog { private: RestCatalog(std::unique_ptr config, - std::unique_ptr paths); + std::unique_ptr paths, std::set endpoints); std::unique_ptr config_; std::unique_ptr client_; std::unique_ptr paths_; std::string name_; + std::set supported_endpoints_; }; } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_util.cc b/src/iceberg/catalog/rest/rest_util.cc index a1a63fa1c..918b9e37e 100644 --- a/src/iceberg/catalog/rest/rest_util.cc +++ b/src/iceberg/catalog/rest/rest_util.cc @@ -23,6 +23,7 @@ #include +#include "iceberg/catalog/rest/endpoint.h" #include "iceberg/table_identifier.h" #include "iceberg/util/macros.h" @@ -251,4 +252,12 @@ std::string GetStandardReasonPhrase(int32_t status_code) { } } +Status CheckEndpoint(const std::set& supported_endpoints, + const Endpoint& endpoint) { + if (!supported_endpoints.contains(endpoint)) { + return NotSupported("Server does not support endpoint: {}", endpoint.ToString()); + } + return {}; +} + } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_util.h b/src/iceberg/catalog/rest/rest_util.h index fde67a842..6e416dd7f 100644 --- a/src/iceberg/catalog/rest/rest_util.h +++ b/src/iceberg/catalog/rest/rest_util.h @@ -19,11 +19,13 @@ #pragma once +#include #include #include #include #include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/catalog/rest/type_fwd.h" #include "iceberg/result.h" #include "iceberg/type_fwd.h" @@ -90,4 +92,12 @@ ICEBERG_REST_EXPORT std::unordered_map MergeConfigs( /// Error"). ICEBERG_REST_EXPORT std::string GetStandardReasonPhrase(int32_t status_code); +/// \brief Check whether the given endpoint is in the set of supported endpoints. +/// +/// \param supported_endpoints Set of endpoints advertised by the server +/// \param endpoint Endpoint to validate +/// \return Status::OK if supported, NotSupported error otherwise +ICEBERG_REST_EXPORT Status CheckEndpoint(const std::set& supported_endpoints, + const Endpoint& endpoint); + } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/type_fwd.h b/src/iceberg/catalog/rest/type_fwd.h index f63984b31..e7fddb91a 100644 --- a/src/iceberg/catalog/rest/type_fwd.h +++ b/src/iceberg/catalog/rest/type_fwd.h @@ -26,6 +26,7 @@ namespace iceberg::rest { struct ErrorResponse; +class Endpoint; class ErrorHandler; class HttpClient; class ResourcePaths; diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index 7760e1781..afcd65b97 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -24,6 +24,7 @@ #include #include +#include "iceberg/catalog/rest/endpoint.h" #include "iceberg/catalog/rest/iceberg_rest_export.h" #include "iceberg/result.h" #include "iceberg/table_identifier.h" @@ -39,16 +40,10 @@ namespace iceberg::rest { struct ICEBERG_REST_EXPORT CatalogConfig { std::unordered_map defaults; // required std::unordered_map overrides; // required - std::vector endpoints; + std::vector 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 {}; - } + Status Validate() const { return {}; } }; /// \brief JSON error payload returned in a response with further details on the error. diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index a48567132..c831ce023 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -170,7 +170,10 @@ if(ICEBERG_BUILD_REST) add_test(NAME ${test_name} COMMAND ${test_name}) endfunction() - add_rest_iceberg_test(rest_catalog_test SOURCES rest_json_internal_test.cc + add_rest_iceberg_test(rest_catalog_test + SOURCES + endpoint_test.cc + rest_json_internal_test.cc rest_util_test.cc) if(ICEBERG_BUILD_REST_INTEGRATION_TESTS) diff --git a/src/iceberg/test/endpoint_test.cc b/src/iceberg/test/endpoint_test.cc new file mode 100644 index 000000000..a6b65e6b1 --- /dev/null +++ b/src/iceberg/test/endpoint_test.cc @@ -0,0 +1,290 @@ +/* + * 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/endpoint.h" + +#include +#include + +#include "iceberg/catalog/rest/json_internal.h" +#include "iceberg/test/matchers.h" + +namespace iceberg::rest { + +TEST(EndpointTest, InvalidCreate) { + // Empty path template should fail + auto result = Endpoint::Create(HttpMethod::GET, ""); + EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(result, HasErrorMessage("Path template cannot be empty")); +} + +TEST(EndpointTest, ValidFromString) { + auto result = Endpoint::FromString("GET /path"); + EXPECT_THAT(result, IsOk()); + + auto endpoint = result.value(); + EXPECT_EQ(endpoint.method(), HttpMethod::GET); + EXPECT_EQ(endpoint.path_template(), "/path"); +} + +TEST(EndpointTest, ToStringRepresentation) { + auto endpoint1 = Endpoint::Create(HttpMethod::POST, "/path/of/resource"); + ASSERT_THAT(endpoint1, IsOk()); + EXPECT_EQ(endpoint1->ToString(), "POST /path/of/resource"); + + auto endpoint2 = Endpoint::Create(HttpMethod::GET, "/"); + ASSERT_THAT(endpoint2, IsOk()); + EXPECT_EQ(endpoint2->ToString(), "GET /"); + + auto endpoint3 = Endpoint::Create(HttpMethod::PUT, "/"); + ASSERT_THAT(endpoint3, IsOk()); + EXPECT_EQ(endpoint3->ToString(), "PUT /"); + + auto endpoint4 = Endpoint::Create(HttpMethod::PUT, "/namespaces/{namespace}/{x}"); + ASSERT_THAT(endpoint4, IsOk()); + EXPECT_EQ(endpoint4->ToString(), "PUT /namespaces/{namespace}/{x}"); +} + +// Test all HTTP methods +TEST(EndpointTest, AllHttpMethods) { + auto get = Endpoint::Create(HttpMethod::GET, "/path"); + ASSERT_THAT(get, IsOk()); + EXPECT_EQ(get->ToString(), "GET /path"); + + auto post = Endpoint::Create(HttpMethod::POST, "/path"); + ASSERT_THAT(post, IsOk()); + EXPECT_EQ(post->ToString(), "POST /path"); + + auto put = Endpoint::Create(HttpMethod::PUT, "/path"); + ASSERT_THAT(put, IsOk()); + EXPECT_EQ(put->ToString(), "PUT /path"); + + auto del = Endpoint::Create(HttpMethod::DELETE, "/path"); + ASSERT_THAT(del, IsOk()); + EXPECT_EQ(del->ToString(), "DELETE /path"); + + auto head = Endpoint::Create(HttpMethod::HEAD, "/path"); + ASSERT_THAT(head, IsOk()); + EXPECT_EQ(head->ToString(), "HEAD /path"); +} + +// Test predefined namespace endpoints +TEST(EndpointTest, NamespaceEndpoints) { + auto list_namespaces = Endpoint::ListNamespaces(); + EXPECT_EQ(list_namespaces.method(), HttpMethod::GET); + EXPECT_EQ(list_namespaces.path_template(), "/v1/{prefix}/namespaces"); + EXPECT_EQ(list_namespaces.ToString(), "GET /v1/{prefix}/namespaces"); + + auto get_namespace = Endpoint::GetNamespaceProperties(); + EXPECT_EQ(get_namespace.method(), HttpMethod::GET); + EXPECT_EQ(get_namespace.path_template(), "/v1/{prefix}/namespaces/{namespace}"); + + auto namespace_exists = Endpoint::NamespaceExists(); + EXPECT_EQ(namespace_exists.method(), HttpMethod::HEAD); + EXPECT_EQ(namespace_exists.path_template(), "/v1/{prefix}/namespaces/{namespace}"); + + auto create_namespace = Endpoint::CreateNamespace(); + EXPECT_EQ(create_namespace.method(), HttpMethod::POST); + EXPECT_EQ(create_namespace.path_template(), "/v1/{prefix}/namespaces"); + + auto update_namespace = Endpoint::UpdateNamespace(); + EXPECT_EQ(update_namespace.method(), HttpMethod::POST); + EXPECT_EQ(update_namespace.path_template(), + "/v1/{prefix}/namespaces/{namespace}/properties"); + + auto drop_namespace = Endpoint::DropNamespace(); + EXPECT_EQ(drop_namespace.method(), HttpMethod::DELETE); + EXPECT_EQ(drop_namespace.path_template(), "/v1/{prefix}/namespaces/{namespace}"); +} + +// Test predefined table endpoints +TEST(EndpointTest, TableEndpoints) { + auto list_tables = Endpoint::ListTables(); + EXPECT_EQ(list_tables.method(), HttpMethod::GET); + EXPECT_EQ(list_tables.path_template(), "/v1/{prefix}/namespaces/{namespace}/tables"); + + auto load_table = Endpoint::LoadTable(); + EXPECT_EQ(load_table.method(), HttpMethod::GET); + EXPECT_EQ(load_table.path_template(), + "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + + auto table_exists = Endpoint::TableExists(); + EXPECT_EQ(table_exists.method(), HttpMethod::HEAD); + EXPECT_EQ(table_exists.path_template(), + "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + + auto create_table = Endpoint::CreateTable(); + EXPECT_EQ(create_table.method(), HttpMethod::POST); + EXPECT_EQ(create_table.path_template(), "/v1/{prefix}/namespaces/{namespace}/tables"); + + auto update_table = Endpoint::UpdateTable(); + EXPECT_EQ(update_table.method(), HttpMethod::POST); + EXPECT_EQ(update_table.path_template(), + "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + + auto delete_table = Endpoint::DeleteTable(); + EXPECT_EQ(delete_table.method(), HttpMethod::DELETE); + EXPECT_EQ(delete_table.path_template(), + "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + + auto rename_table = Endpoint::RenameTable(); + EXPECT_EQ(rename_table.method(), HttpMethod::POST); + EXPECT_EQ(rename_table.path_template(), "/v1/{prefix}/tables/rename"); + + auto register_table = Endpoint::RegisterTable(); + EXPECT_EQ(register_table.method(), HttpMethod::POST); + EXPECT_EQ(register_table.path_template(), + "/v1/{prefix}/namespaces/{namespace}/register"); + + auto report_metrics = Endpoint::ReportMetrics(); + EXPECT_EQ(report_metrics.method(), HttpMethod::POST); + EXPECT_EQ(report_metrics.path_template(), + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"); + + auto table_credentials = Endpoint::TableCredentials(); + EXPECT_EQ(table_credentials.method(), HttpMethod::GET); + EXPECT_EQ(table_credentials.path_template(), + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials"); +} + +// Test predefined transaction endpoints +TEST(EndpointTest, TransactionEndpoints) { + auto commit_transaction = Endpoint::CommitTransaction(); + EXPECT_EQ(commit_transaction.method(), HttpMethod::POST); + EXPECT_EQ(commit_transaction.path_template(), "/v1/{prefix}/transactions/commit"); +} + +// Test endpoint equality and comparison +TEST(EndpointTest, EqualityAndComparison) { + auto endpoint1 = Endpoint::Create(HttpMethod::GET, "/path"); + auto endpoint2 = Endpoint::Create(HttpMethod::GET, "/path"); + auto endpoint3 = Endpoint::Create(HttpMethod::POST, "/path"); + auto endpoint4 = Endpoint::Create(HttpMethod::GET, "/other"); + + ASSERT_THAT(endpoint1, IsOk()); + ASSERT_THAT(endpoint2, IsOk()); + ASSERT_THAT(endpoint3, IsOk()); + ASSERT_THAT(endpoint4, IsOk()); + + // Equality + EXPECT_EQ(*endpoint1, *endpoint2); + EXPECT_NE(*endpoint1, *endpoint3); + EXPECT_NE(*endpoint1, *endpoint4); + + // Comparison (for ordering) + EXPECT_LT(*endpoint1, *endpoint3); // GET < POST + EXPECT_LT(*endpoint4, *endpoint1); // "/other" < "/path" lexicographically +} + +// Test string serialization (endpoints are represented as strings) +TEST(EndpointTest, ToStringFormat) { + auto endpoint1 = Endpoint::Create(HttpMethod::GET, "/v1/{prefix}/namespaces"); + ASSERT_THAT(endpoint1, IsOk()); + EXPECT_EQ(endpoint1->ToString(), "GET /v1/{prefix}/namespaces"); + + auto endpoint2 = Endpoint::Create(HttpMethod::POST, "/v1/{prefix}/tables"); + ASSERT_THAT(endpoint2, IsOk()); + EXPECT_EQ(endpoint2->ToString(), "POST /v1/{prefix}/tables"); + + // Test with all HTTP methods + auto endpoint3 = Endpoint::Create(HttpMethod::DELETE, "/path"); + ASSERT_THAT(endpoint3, IsOk()); + EXPECT_EQ(endpoint3->ToString(), "DELETE /path"); + + auto endpoint4 = Endpoint::Create(HttpMethod::PUT, "/path"); + ASSERT_THAT(endpoint4, IsOk()); + EXPECT_EQ(endpoint4->ToString(), "PUT /path"); + + auto endpoint5 = Endpoint::Create(HttpMethod::HEAD, "/path"); + ASSERT_THAT(endpoint5, IsOk()); + EXPECT_EQ(endpoint5->ToString(), "HEAD /path"); +} + +// Test string deserialization +TEST(EndpointTest, FromStringParsing) { + auto result1 = Endpoint::FromString("GET /v1/{prefix}/namespaces"); + ASSERT_THAT(result1, IsOk()); + EXPECT_EQ(result1->method(), HttpMethod::GET); + EXPECT_EQ(result1->path_template(), "/v1/{prefix}/namespaces"); + + auto result2 = Endpoint::FromString("POST /v1/{prefix}/namespaces/{namespace}/tables"); + ASSERT_THAT(result2, IsOk()); + EXPECT_EQ(result2->method(), HttpMethod::POST); + EXPECT_EQ(result2->path_template(), "/v1/{prefix}/namespaces/{namespace}/tables"); + + // Test all HTTP methods + auto result3 = Endpoint::FromString("DELETE /path"); + ASSERT_THAT(result3, IsOk()); + EXPECT_EQ(result3->method(), HttpMethod::DELETE); + + auto result4 = Endpoint::FromString("PUT /path"); + ASSERT_THAT(result4, IsOk()); + EXPECT_EQ(result4->method(), HttpMethod::PUT); + + auto result5 = Endpoint::FromString("HEAD /path"); + ASSERT_THAT(result5, IsOk()); + EXPECT_EQ(result5->method(), HttpMethod::HEAD); +} + +// Test string parsing with invalid inputs +TEST(EndpointTest, FromStringInvalid) { + // Invalid endpoint format should fail - missing space + auto result1 = Endpoint::FromString("/path/without/method"); + EXPECT_THAT(result1, IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(result1, + HasErrorMessage("Invalid endpoint format (must consist of two elements " + "separated by a single space)")); + + // Invalid HTTP method should fail + auto result2 = Endpoint::FromString("INVALID /path"); + EXPECT_THAT(result2, IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(result2, HasErrorMessage("Invalid HTTP method")); + + // Invalid endpoint format - extra element after path + auto result3 = Endpoint::FromString("GET /path INVALID"); + EXPECT_THAT(result3, IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(result3, + HasErrorMessage("Invalid endpoint format (must consist of two elements " + "separated by a single space)")); +} + +// Test string round-trip +TEST(EndpointTest, StringRoundTrip) { + // Create various endpoints and verify they survive string round-trip + std::vector endpoints = { + Endpoint::ListNamespaces(), Endpoint::GetNamespaceProperties(), + Endpoint::CreateNamespace(), Endpoint::LoadTable(), + Endpoint::CreateTable(), Endpoint::DeleteTable(), + }; + + for (const auto& original : endpoints) { + // Serialize to string + std::string str = original.ToString(); + + // Deserialize from string + auto deserialized = Endpoint::FromString(str); + ASSERT_THAT(deserialized, IsOk()); + + // Verify they are equal + EXPECT_EQ(original, *deserialized); + EXPECT_EQ(original.ToString(), deserialized->ToString()); + } +} + +} // namespace iceberg::rest diff --git a/src/iceberg/test/rest_catalog_test.cc b/src/iceberg/test/rest_catalog_test.cc index 49c527f64..725ad7ece 100644 --- a/src/iceberg/test/rest_catalog_test.cc +++ b/src/iceberg/test/rest_catalog_test.cc @@ -32,9 +32,13 @@ #include #include #include +#include #include #include "iceberg/catalog/rest/catalog_properties.h" +#include "iceberg/catalog/rest/error_handlers.h" +#include "iceberg/catalog/rest/http_client.h" +#include "iceberg/catalog/rest/json_internal.h" #include "iceberg/result.h" #include "iceberg/table_identifier.h" #include "iceberg/test/matchers.h" @@ -137,6 +141,40 @@ TEST_F(RestCatalogIntegrationTest, MakeCatalogSuccess) { EXPECT_EQ(catalog->name(), kCatalogName); } +/// Verifies that the server's /v1/config endpoint returns a valid response +/// and that the endpoints field (if present) can be parsed correctly. +TEST_F(RestCatalogIntegrationTest, FetchServerConfigDirect) { + // Create HTTP client and fetch config directly + HttpClient client({}); + std::string config_url = + std::format("{}:{}/v1/config", kLocalhostUri, kRestCatalogPort); + + auto response_result = client.Get(config_url, {}, {}, *DefaultErrorHandler::Instance()); + ASSERT_THAT(response_result, IsOk()); + auto json_result = FromJsonString(response_result->body()); + ASSERT_THAT(json_result, IsOk()); + auto& json = json_result.value(); + + EXPECT_TRUE(json.contains("defaults")); + EXPECT_TRUE(json.contains("overrides")); + + if (json.contains("endpoints")) { + EXPECT_TRUE(json["endpoints"].is_array()); + + // Parse the config to ensure all endpoints are valid + auto config_result = CatalogConfigFromJson(json); + ASSERT_THAT(config_result, IsOk()); + auto& config = config_result.value(); + std::println("[INFO] Server provided {} endpoints", config.endpoints.size()); + EXPECT_GT(config.endpoints.size(), 0) + << "Server should provide at least one endpoint"; + } else { + std::println( + "[INFO] Server did not provide endpoints field, client will use default " + "endpoints"); + } +} + TEST_F(RestCatalogIntegrationTest, ListNamespaces) { auto catalog_result = CreateCatalog(); ASSERT_THAT(catalog_result, IsOk()); diff --git a/src/iceberg/test/rest_json_internal_test.cc b/src/iceberg/test/rest_json_internal_test.cc index ca2671fae..7b4b5d4c4 100644 --- a/src/iceberg/test/rest_json_internal_test.cc +++ b/src/iceberg/test/rest_json_internal_test.cc @@ -783,7 +783,7 @@ INSTANTIATE_TEST_SUITE_P( CatalogConfigParam{.test_name = "BothEmpty", .expected_json_str = R"({"defaults":{},"overrides":{}})", .model = {}}, - // With endpoints + // With valid endpoints CatalogConfigParam{ .test_name = "WithEndpoints", .expected_json_str = @@ -791,13 +791,14 @@ INSTANTIATE_TEST_SUITE_P( .model = {.defaults = {{"warehouse", "s3://bucket/warehouse"}}, .overrides = {{"clients", "5"}}, - .endpoints = {"GET /v1/config", "POST /v1/tables"}}}, + .endpoints = {*Endpoint::Create(HttpMethod::GET, "/v1/config"), + *Endpoint::Create(HttpMethod::POST, "/v1/tables")}}}, // Only endpoints CatalogConfigParam{ .test_name = "OnlyEndpoints", .expected_json_str = R"({"defaults":{},"overrides":{},"endpoints":["GET /v1/config"]})", - .model = {.endpoints = {"GET /v1/config"}}}), + .model = {.endpoints = {*Endpoint::Create(HttpMethod::GET, "/v1/config")}}}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); @@ -834,7 +835,12 @@ INSTANTIATE_TEST_SUITE_P( // Both fields null CatalogConfigDeserializeParam{.test_name = "BothNull", .json_str = R"({"defaults":null,"overrides":null})", - .expected_model = {}}), + .expected_model = {}}, + // Missing endpoints field, client will uses default endpoints + CatalogConfigDeserializeParam{ + .test_name = "MissingEndpoints", + .json_str = R"({"defaults":{},"overrides":{}})", + .expected_model = {.defaults = {}, .overrides = {}, .endpoints = {}}}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); @@ -855,7 +861,22 @@ INSTANTIATE_TEST_SUITE_P( .test_name = "WrongOverridesType", .invalid_json_str = R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":"clients"})", - .expected_error_message = "type must be object, but is string"}), + .expected_error_message = "type must be object, but is string"}, + // Invalid endpoint format - missing space separator + CatalogConfigInvalidParam{ + .test_name = "InvalidEndpointMissingSpace", + .invalid_json_str = R"({"endpoints":["GET_v1/namespaces/{namespace}"]})", + .expected_error_message = + "Invalid endpoint format (must consist of two elements separated by a " + "single space)"}, + // Invalid endpoint format - extra element after path + CatalogConfigInvalidParam{ + .test_name = "InvalidEndpointExtraElement", + .invalid_json_str = + R"({"endpoints":["GET v1/namespaces/{namespace} INVALID"]})", + .expected_error_message = + "Invalid endpoint format (must consist of two elements separated by a " + "single space)"}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); diff --git a/src/iceberg/test/rest_util_test.cc b/src/iceberg/test/rest_util_test.cc index b95a220bb..5b32640b7 100644 --- a/src/iceberg/test/rest_util_test.cc +++ b/src/iceberg/test/rest_util_test.cc @@ -21,6 +21,7 @@ #include +#include "iceberg/catalog/rest/endpoint.h" #include "iceberg/table_identifier.h" #include "iceberg/test/matchers.h" @@ -153,4 +154,20 @@ TEST(RestUtilTest, MergeConfigs) { EXPECT_EQ(merged_empty["key"], "value"); } +TEST(RestUtilTest, CheckEndpointSupported) { + std::set supported = {Endpoint::ListNamespaces(), Endpoint::LoadTable(), + Endpoint::CreateTable()}; + + // Supported endpoints should pass + EXPECT_THAT(CheckEndpoint(supported, Endpoint::ListNamespaces()), IsOk()); + EXPECT_THAT(CheckEndpoint(supported, Endpoint::LoadTable()), IsOk()); + EXPECT_THAT(CheckEndpoint(supported, Endpoint::CreateTable()), IsOk()); + + // Unsupported endpoints should fail + EXPECT_THAT(CheckEndpoint(supported, Endpoint::DeleteTable()), + IsError(ErrorKind::kNotSupported)); + EXPECT_THAT(CheckEndpoint(supported, Endpoint::UpdateTable()), + IsError(ErrorKind::kNotSupported)); +} + } // namespace iceberg::rest From 9f4c6812b8e3e9a0c64d09868ffc37b13da01d2c Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Fri, 12 Dec 2025 17:16:23 +0800 Subject: [PATCH 2/7] fix ci --- src/iceberg/catalog/rest/endpoint.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/iceberg/catalog/rest/endpoint.h b/src/iceberg/catalog/rest/endpoint.h index 3291ed2f6..48efe4b01 100644 --- a/src/iceberg/catalog/rest/endpoint.h +++ b/src/iceberg/catalog/rest/endpoint.h @@ -19,7 +19,6 @@ #pragma once -#include #include #include @@ -138,7 +137,7 @@ class ICEBERG_REST_EXPORT Endpoint { } private: - Endpoint(HttpMethod method, std::string_view path_template) + constexpr Endpoint(HttpMethod method, std::string_view path_template) : method_(method), path_template_(path_template) {} HttpMethod method_; From 8fc6e83f632ba5cf340459b409a92da118be3e0b Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Fri, 12 Dec 2025 18:17:10 +0800 Subject: [PATCH 3/7] 1 --- src/iceberg/catalog/rest/endpoint.h | 36 ++++++++++++++--------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/iceberg/catalog/rest/endpoint.h b/src/iceberg/catalog/rest/endpoint.h index 48efe4b01..0992b3133 100644 --- a/src/iceberg/catalog/rest/endpoint.h +++ b/src/iceberg/catalog/rest/endpoint.h @@ -78,66 +78,66 @@ class ICEBERG_REST_EXPORT Endpoint { } // Namespace endpoints - static constexpr Endpoint ListNamespaces() { + static Endpoint ListNamespaces() { return {HttpMethod::GET, "/v1/{prefix}/namespaces"}; } - static constexpr Endpoint GetNamespaceProperties() { + static Endpoint GetNamespaceProperties() { return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}"}; } - static constexpr Endpoint NamespaceExists() { + static Endpoint NamespaceExists() { return {HttpMethod::HEAD, "/v1/{prefix}/namespaces/{namespace}"}; } - static constexpr Endpoint CreateNamespace() { + static Endpoint CreateNamespace() { return {HttpMethod::POST, "/v1/{prefix}/namespaces"}; } - static constexpr Endpoint UpdateNamespace() { + static Endpoint UpdateNamespace() { return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/properties"}; } - static constexpr Endpoint DropNamespace() { + static Endpoint DropNamespace() { return {HttpMethod::DELETE, "/v1/{prefix}/namespaces/{namespace}"}; } // Table endpoints - static constexpr Endpoint ListTables() { + static Endpoint ListTables() { return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}/tables"}; } - static constexpr Endpoint LoadTable() { + static Endpoint LoadTable() { return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } - static constexpr Endpoint TableExists() { + static Endpoint TableExists() { return {HttpMethod::HEAD, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } - static constexpr Endpoint CreateTable() { + static Endpoint CreateTable() { return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/tables"}; } - static constexpr Endpoint UpdateTable() { + static Endpoint UpdateTable() { return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } - static constexpr Endpoint DeleteTable() { + static Endpoint DeleteTable() { return {HttpMethod::DELETE, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } - static constexpr Endpoint RenameTable() { + static Endpoint RenameTable() { return {HttpMethod::POST, "/v1/{prefix}/tables/rename"}; } - static constexpr Endpoint RegisterTable() { + static Endpoint RegisterTable() { return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/register"}; } - static constexpr Endpoint ReportMetrics() { + static Endpoint ReportMetrics() { return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"}; } - static constexpr Endpoint TableCredentials() { + static Endpoint TableCredentials() { return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials"}; } // Transaction endpoints - static constexpr Endpoint CommitTransaction() { + static Endpoint CommitTransaction() { return {HttpMethod::POST, "/v1/{prefix}/transactions/commit"}; } private: - constexpr Endpoint(HttpMethod method, std::string_view path_template) + Endpoint(HttpMethod method, std::string_view path_template) : method_(method), path_template_(path_template) {} HttpMethod method_; From 305cde86653b8cac5aa21b9e74c3c88d6c6bea3e Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Mon, 15 Dec 2025 10:45:54 +0800 Subject: [PATCH 4/7] 2 --- src/iceberg/catalog/rest/endpoint.cc | 4 ++-- src/iceberg/catalog/rest/endpoint.h | 6 +++--- src/iceberg/test/endpoint_test.cc | 10 +++++----- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/iceberg/catalog/rest/endpoint.cc b/src/iceberg/catalog/rest/endpoint.cc index 549677c30..f59fe9f61 100644 --- a/src/iceberg/catalog/rest/endpoint.cc +++ b/src/iceberg/catalog/rest/endpoint.cc @@ -31,7 +31,7 @@ constexpr std::string_view ToString(HttpMethod method) { return "POST"; case HttpMethod::PUT: return "PUT"; - case HttpMethod::DELETE: + case HttpMethod::DELETE_: return "DELETE"; case HttpMethod::HEAD: return "HEAD"; @@ -72,7 +72,7 @@ Result Endpoint::FromString(std::string_view str) { } else if (method_str == "PUT") { method = HttpMethod::PUT; } else if (method_str == "DELETE") { - method = HttpMethod::DELETE; + method = HttpMethod::DELETE_; } else if (method_str == "HEAD") { method = HttpMethod::HEAD; } else { diff --git a/src/iceberg/catalog/rest/endpoint.h b/src/iceberg/catalog/rest/endpoint.h index 0992b3133..867d2cda9 100644 --- a/src/iceberg/catalog/rest/endpoint.h +++ b/src/iceberg/catalog/rest/endpoint.h @@ -31,7 +31,7 @@ namespace iceberg::rest { /// \brief HTTP method enumeration. -enum class HttpMethod : uint8_t { GET, POST, PUT, DELETE, HEAD }; +enum class HttpMethod : uint8_t { GET, POST, PUT, DELETE_, HEAD }; /// \brief Convert HttpMethod to string representation. constexpr std::string_view ToString(HttpMethod method); @@ -94,7 +94,7 @@ class ICEBERG_REST_EXPORT Endpoint { return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/properties"}; } static Endpoint DropNamespace() { - return {HttpMethod::DELETE, "/v1/{prefix}/namespaces/{namespace}"}; + return {HttpMethod::DELETE_, "/v1/{prefix}/namespaces/{namespace}"}; } // Table endpoints @@ -114,7 +114,7 @@ class ICEBERG_REST_EXPORT Endpoint { return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } static Endpoint DeleteTable() { - return {HttpMethod::DELETE, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + return {HttpMethod::DELETE_, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } static Endpoint RenameTable() { return {HttpMethod::POST, "/v1/{prefix}/tables/rename"}; diff --git a/src/iceberg/test/endpoint_test.cc b/src/iceberg/test/endpoint_test.cc index a6b65e6b1..6b3cd7089 100644 --- a/src/iceberg/test/endpoint_test.cc +++ b/src/iceberg/test/endpoint_test.cc @@ -75,7 +75,7 @@ TEST(EndpointTest, AllHttpMethods) { ASSERT_THAT(put, IsOk()); EXPECT_EQ(put->ToString(), "PUT /path"); - auto del = Endpoint::Create(HttpMethod::DELETE, "/path"); + auto del = Endpoint::Create(HttpMethod::DELETE_, "/path"); ASSERT_THAT(del, IsOk()); EXPECT_EQ(del->ToString(), "DELETE /path"); @@ -109,7 +109,7 @@ TEST(EndpointTest, NamespaceEndpoints) { "/v1/{prefix}/namespaces/{namespace}/properties"); auto drop_namespace = Endpoint::DropNamespace(); - EXPECT_EQ(drop_namespace.method(), HttpMethod::DELETE); + EXPECT_EQ(drop_namespace.method(), HttpMethod::DELETE_); EXPECT_EQ(drop_namespace.path_template(), "/v1/{prefix}/namespaces/{namespace}"); } @@ -139,7 +139,7 @@ TEST(EndpointTest, TableEndpoints) { "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); auto delete_table = Endpoint::DeleteTable(); - EXPECT_EQ(delete_table.method(), HttpMethod::DELETE); + EXPECT_EQ(delete_table.method(), HttpMethod::DELETE_); EXPECT_EQ(delete_table.path_template(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); @@ -203,7 +203,7 @@ TEST(EndpointTest, ToStringFormat) { EXPECT_EQ(endpoint2->ToString(), "POST /v1/{prefix}/tables"); // Test with all HTTP methods - auto endpoint3 = Endpoint::Create(HttpMethod::DELETE, "/path"); + auto endpoint3 = Endpoint::Create(HttpMethod::DELETE_, "/path"); ASSERT_THAT(endpoint3, IsOk()); EXPECT_EQ(endpoint3->ToString(), "DELETE /path"); @@ -231,7 +231,7 @@ TEST(EndpointTest, FromStringParsing) { // Test all HTTP methods auto result3 = Endpoint::FromString("DELETE /path"); ASSERT_THAT(result3, IsOk()); - EXPECT_EQ(result3->method(), HttpMethod::DELETE); + EXPECT_EQ(result3->method(), HttpMethod::DELETE_); auto result4 = Endpoint::FromString("PUT /path"); ASSERT_THAT(result4, IsOk()); From 8a78f4e562fd2a43c1bf2c915f79732a6347c32e Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Mon, 15 Dec 2025 17:20:41 +0800 Subject: [PATCH 5/7] fix review --- src/iceberg/catalog/rest/CMakeLists.txt | 2 +- src/iceberg/catalog/rest/endpoint.cc | 29 ++-- src/iceberg/catalog/rest/endpoint.h | 69 +++++----- src/iceberg/catalog/rest/json_internal.cc | 6 +- src/iceberg/catalog/rest/rest_catalog.cc | 24 ++-- src/iceberg/catalog/rest/rest_catalog.h | 5 +- src/iceberg/catalog/rest/rest_util.cc | 6 +- src/iceberg/catalog/rest/rest_util.h | 8 +- src/iceberg/test/endpoint_test.cc | 139 +++++++++----------- src/iceberg/test/rest_json_internal_test.cc | 6 +- src/iceberg/test/rest_util_test.cc | 4 +- 11 files changed, 143 insertions(+), 155 deletions(-) diff --git a/src/iceberg/catalog/rest/CMakeLists.txt b/src/iceberg/catalog/rest/CMakeLists.txt index 86f8cacd6..7b36298a6 100644 --- a/src/iceberg/catalog/rest/CMakeLists.txt +++ b/src/iceberg/catalog/rest/CMakeLists.txt @@ -16,13 +16,13 @@ # under the License. set(ICEBERG_REST_SOURCES - rest_catalog.cc catalog_properties.cc endpoint.cc error_handlers.cc http_client.cc json_internal.cc resource_paths.cc + rest_catalog.cc rest_util.cc) set(ICEBERG_REST_STATIC_BUILD_INTERFACE_LIBS) diff --git a/src/iceberg/catalog/rest/endpoint.cc b/src/iceberg/catalog/rest/endpoint.cc index f59fe9f61..e0c5564e9 100644 --- a/src/iceberg/catalog/rest/endpoint.cc +++ b/src/iceberg/catalog/rest/endpoint.cc @@ -20,26 +20,27 @@ #include "iceberg/catalog/rest/endpoint.h" #include +#include namespace iceberg::rest { constexpr std::string_view ToString(HttpMethod method) { switch (method) { - case HttpMethod::GET: + case HttpMethod::kGet: return "GET"; - case HttpMethod::POST: + case HttpMethod::kPost: return "POST"; - case HttpMethod::PUT: + case HttpMethod::kPut: return "PUT"; - case HttpMethod::DELETE_: + case HttpMethod::kDelete: return "DELETE"; - case HttpMethod::HEAD: + case HttpMethod::kHead: return "HEAD"; } return "UNKNOWN"; } -Result Endpoint::Create(HttpMethod method, std::string path_template) { +Result Endpoint::Make(HttpMethod method, std::string_view path_template) { if (path_template.empty()) { return InvalidArgument("Path template cannot be empty"); } @@ -52,7 +53,7 @@ Result Endpoint::FromString(std::string_view str) { str.find(' ', space_pos + 1) != std::string_view::npos) { return InvalidArgument( "Invalid endpoint format (must consist of two elements separated by a single " - "space): {}", + "space): '{}'", str); } @@ -66,24 +67,24 @@ Result Endpoint::FromString(std::string_view str) { // Parse HTTP method HttpMethod method; if (method_str == "GET") { - method = HttpMethod::GET; + method = HttpMethod::kGet; } else if (method_str == "POST") { - method = HttpMethod::POST; + method = HttpMethod::kPost; } else if (method_str == "PUT") { - method = HttpMethod::PUT; + method = HttpMethod::kPut; } else if (method_str == "DELETE") { - method = HttpMethod::DELETE_; + method = HttpMethod::kDelete; } else if (method_str == "HEAD") { - method = HttpMethod::HEAD; + method = HttpMethod::kHead; } else { return InvalidArgument("Invalid HTTP method: '{}'", method_str); } - return Create(method, std::string(path_str)); + return Make(method, std::string(path_str)); } std::string Endpoint::ToString() const { - return std::format("{} {}", rest::ToString(method_), path_template_); + return std::format("{} {}", rest::ToString(method_), path_); } } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/endpoint.h b/src/iceberg/catalog/rest/endpoint.h index 867d2cda9..65915e61d 100644 --- a/src/iceberg/catalog/rest/endpoint.h +++ b/src/iceberg/catalog/rest/endpoint.h @@ -31,7 +31,7 @@ namespace iceberg::rest { /// \brief HTTP method enumeration. -enum class HttpMethod : uint8_t { GET, POST, PUT, DELETE_, HEAD }; +enum class HttpMethod : uint8_t { kGet, kPost, kPut, kDelete, kHead }; /// \brief Convert HttpMethod to string representation. constexpr std::string_view ToString(HttpMethod method); @@ -42,106 +42,103 @@ constexpr std::string_view ToString(HttpMethod method); /// - Path template (e.g., "/v1/{prefix}/namespaces/{namespace}") class ICEBERG_REST_EXPORT Endpoint { public: - /// \brief Create an endpoint with method and path template. + /// \brief Make an endpoint with method and path template. /// /// \param method HTTP method (GET, POST, etc.) /// \param path_template Path template with placeholders (e.g., "/v1/{prefix}/tables") /// \return Endpoint instance or error if invalid - static Result Create(HttpMethod method, std::string path_template); + static Result Make(HttpMethod method, std::string_view path_template); - /// \brief Parse endpoint from string representation. + /// \brief Parse endpoint from string representation. "METHOD" have to be all + /// upper-cased. /// /// \param str String in format "METHOD /path/template" (e.g., "GET /v1/namespaces") - /// \return Endpoint instance or error if malformed + /// \return Endpoint instance or error if malformed. static Result FromString(std::string_view str); /// \brief Get the HTTP method. constexpr HttpMethod method() const { return method_; } /// \brief Get the path template. - std::string_view path_template() const { return path_template_; } + std::string_view path() const { return path_; } /// \brief Serialize to "METHOD /path" format. std::string ToString() const; - /// \brief Equality comparison operator. constexpr bool operator==(const Endpoint& other) const { - return method_ == other.method_ && path_template_ == other.path_template_; - } - - /// \brief Three-way comparison operator (C++20). - constexpr auto operator<=>(const Endpoint& other) const { - if (auto cmp = method_ <=> other.method_; cmp != 0) { - return cmp; - } - return path_template_ <=> other.path_template_; + return method_ == other.method_ && path_ == other.path_; } // Namespace endpoints static Endpoint ListNamespaces() { - return {HttpMethod::GET, "/v1/{prefix}/namespaces"}; + return {HttpMethod::kGet, "/v1/{prefix}/namespaces"}; } static Endpoint GetNamespaceProperties() { - return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}"}; + return {HttpMethod::kGet, "/v1/{prefix}/namespaces/{namespace}"}; } static Endpoint NamespaceExists() { - return {HttpMethod::HEAD, "/v1/{prefix}/namespaces/{namespace}"}; + return {HttpMethod::kHead, "/v1/{prefix}/namespaces/{namespace}"}; } static Endpoint CreateNamespace() { - return {HttpMethod::POST, "/v1/{prefix}/namespaces"}; + return {HttpMethod::kPost, "/v1/{prefix}/namespaces"}; } static Endpoint UpdateNamespace() { - return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/properties"}; + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/properties"}; } static Endpoint DropNamespace() { - return {HttpMethod::DELETE_, "/v1/{prefix}/namespaces/{namespace}"}; + return {HttpMethod::kDelete, "/v1/{prefix}/namespaces/{namespace}"}; } // Table endpoints static Endpoint ListTables() { - return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}/tables"}; + return {HttpMethod::kGet, "/v1/{prefix}/namespaces/{namespace}/tables"}; } static Endpoint LoadTable() { - return {HttpMethod::GET, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + return {HttpMethod::kGet, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } static Endpoint TableExists() { - return {HttpMethod::HEAD, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + return {HttpMethod::kHead, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } static Endpoint CreateTable() { - return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/tables"}; + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/tables"}; } static Endpoint UpdateTable() { - return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } static Endpoint DeleteTable() { - return {HttpMethod::DELETE_, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + return {HttpMethod::kDelete, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; } static Endpoint RenameTable() { - return {HttpMethod::POST, "/v1/{prefix}/tables/rename"}; + return {HttpMethod::kPost, "/v1/{prefix}/tables/rename"}; } static Endpoint RegisterTable() { - return {HttpMethod::POST, "/v1/{prefix}/namespaces/{namespace}/register"}; + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/register"}; } static Endpoint ReportMetrics() { - return {HttpMethod::POST, + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"}; } static Endpoint TableCredentials() { - return {HttpMethod::GET, + return {HttpMethod::kGet, "/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials"}; } // Transaction endpoints static Endpoint CommitTransaction() { - return {HttpMethod::POST, "/v1/{prefix}/transactions/commit"}; + return {HttpMethod::kPost, "/v1/{prefix}/transactions/commit"}; } private: - Endpoint(HttpMethod method, std::string_view path_template) - : method_(method), path_template_(path_template) {} + Endpoint(HttpMethod method, std::string_view path) : method_(method), path_(path) {} HttpMethod method_; - std::string path_template_; + std::string path_; +}; + +struct ICEBERG_REST_EXPORT EndpointHash { + std::size_t operator()(const Endpoint& endpoint) const noexcept { + return std::hash()(endpoint.ToString()); + } }; } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/json_internal.cc b/src/iceberg/catalog/rest/json_internal.cc index 9db2ca0a6..66e690258 100644 --- a/src/iceberg/catalog/rest/json_internal.cc +++ b/src/iceberg/catalog/rest/json_internal.cc @@ -74,8 +74,7 @@ nlohmann::json ToJson(const CatalogConfig& config) { json[kOverrides] = config.overrides; json[kDefaults] = config.defaults; for (const auto& endpoint : config.endpoints) { - auto endpoint_str = endpoint.ToString(); - json[kEndpoints].push_back(endpoint_str); + json[kEndpoints].emplace_back(endpoint.ToString()); } return json; } @@ -89,13 +88,14 @@ Result CatalogConfigFromJson(const nlohmann::json& json) { config.defaults, GetJsonValueOrDefault(json, kDefaults)); ICEBERG_ASSIGN_OR_RAISE( auto endpoints, GetJsonValueOrDefault>(json, kEndpoints)); + config.endpoints.reserve(endpoints.size()); for (const auto& endpoint_str : endpoints) { auto endpoint_result = Endpoint::FromString(endpoint_str); if (!endpoint_result.has_value()) { // Convert to JsonParseError in JSON deserialization context return JsonParseError("{}", endpoint_result.error().message); } - config.endpoints.push_back(std::move(*endpoint_result)); + config.endpoints.emplace_back(std::move(endpoint_result.value())); } ICEBERG_RETURN_UNEXPECTED(config.Validate()); return config; diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 5aaffe164..cb4244ec6 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -35,6 +35,7 @@ #include "iceberg/catalog/rest/resource_paths.h" #include "iceberg/catalog/rest/rest_catalog.h" #include "iceberg/catalog/rest/rest_util.h" +#include "iceberg/catalog/rest/types.h" #include "iceberg/json_internal.h" #include "iceberg/partition_spec.h" #include "iceberg/result.h" @@ -48,7 +49,7 @@ namespace { /// \brief Get the default set of endpoints for backwards compatibility according to the /// iceberg rest spec. -std::set GetDefaultEndpoints() { +std::unordered_set GetDefaultEndpoints() { return { Endpoint::ListNamespaces(), Endpoint::GetNamespaceProperties(), Endpoint::CreateNamespace(), Endpoint::UpdateNamespace(), @@ -90,11 +91,11 @@ Result> RestCatalog::Make( RestCatalogProperties::FromMap(MergeConfigs( server_config.overrides, initial_config.configs(), server_config.defaults)); - std::set endpoints; + std::unordered_set endpoints; if (!server_config.endpoints.empty()) { // Endpoints are already parsed during JSON deserialization, just convert to set - endpoints = std::set(server_config.endpoints.begin(), - server_config.endpoints.end()); + endpoints = std::unordered_set( + server_config.endpoints.begin(), server_config.endpoints.end()); } else { // If a server does not send the endpoints field, use default set of endpoints // for backwards compatibility with legacy servers @@ -111,7 +112,7 @@ Result> RestCatalog::Make( RestCatalog::RestCatalog(std::unique_ptr config, std::unique_ptr paths, - std::set endpoints) + std::unordered_set endpoints) : config_(std::move(config)), client_(std::make_unique(config_->ExtractHeaders())), paths_(std::move(paths)), @@ -195,16 +196,11 @@ Result RestCatalog::NamespaceExists(const Namespace& ns) const { // If server does not support HEAD endpoint, fall back to GetNamespaceProperties if (!check.has_value()) { ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns)); - auto response = client_->Get(path, /*params=*/{}, /*headers=*/{}, - *NamespaceErrorHandler::Instance()); - if (!response.has_value()) { - const auto& error = response.error(); - // catch NoSuchNamespaceException/404 and return false - if (error.kind == ErrorKind::kNoSuchNamespace) { - return false; - } - ICEBERG_RETURN_UNEXPECTED(response); + auto result = GetNamespaceProperties(ns); + if (!result.has_value() && result.error().kind == ErrorKind::kNoSuchNamespace) { + return false; } + ICEBERG_RETURN_UNEXPECTED(result); // GET succeeded, namespace exists return true; } diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index 0da888da1..55cad9787 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -100,13 +100,14 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog { private: RestCatalog(std::unique_ptr config, - std::unique_ptr paths, std::set endpoints); + std::unique_ptr paths, + std::unordered_set endpoints); std::unique_ptr config_; std::unique_ptr client_; std::unique_ptr paths_; std::string name_; - std::set supported_endpoints_; + std::unordered_set supported_endpoints_; }; } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_util.cc b/src/iceberg/catalog/rest/rest_util.cc index 918b9e37e..5a6b06a87 100644 --- a/src/iceberg/catalog/rest/rest_util.cc +++ b/src/iceberg/catalog/rest/rest_util.cc @@ -20,6 +20,7 @@ #include "iceberg/catalog/rest/rest_util.h" #include +#include #include @@ -252,8 +253,9 @@ std::string GetStandardReasonPhrase(int32_t status_code) { } } -Status CheckEndpoint(const std::set& supported_endpoints, - const Endpoint& endpoint) { +Status CheckEndpoint( + const std::unordered_set& supported_endpoints, + const Endpoint& endpoint) { if (!supported_endpoints.contains(endpoint)) { return NotSupported("Server does not support endpoint: {}", endpoint.ToString()); } diff --git a/src/iceberg/catalog/rest/rest_util.h b/src/iceberg/catalog/rest/rest_util.h index 6e416dd7f..05b162903 100644 --- a/src/iceberg/catalog/rest/rest_util.h +++ b/src/iceberg/catalog/rest/rest_util.h @@ -19,11 +19,12 @@ #pragma once -#include #include #include #include +#include +#include "iceberg/catalog/rest/endpoint.h" #include "iceberg/catalog/rest/iceberg_rest_export.h" #include "iceberg/catalog/rest/type_fwd.h" #include "iceberg/result.h" @@ -97,7 +98,8 @@ ICEBERG_REST_EXPORT std::string GetStandardReasonPhrase(int32_t status_code); /// \param supported_endpoints Set of endpoints advertised by the server /// \param endpoint Endpoint to validate /// \return Status::OK if supported, NotSupported error otherwise -ICEBERG_REST_EXPORT Status CheckEndpoint(const std::set& supported_endpoints, - const Endpoint& endpoint); +ICEBERG_REST_EXPORT Status +CheckEndpoint(const std::unordered_set& supported_endpoints, + const Endpoint& endpoint); } // namespace iceberg::rest diff --git a/src/iceberg/test/endpoint_test.cc b/src/iceberg/test/endpoint_test.cc index 6b3cd7089..901dc1a62 100644 --- a/src/iceberg/test/endpoint_test.cc +++ b/src/iceberg/test/endpoint_test.cc @@ -22,14 +22,13 @@ #include #include -#include "iceberg/catalog/rest/json_internal.h" #include "iceberg/test/matchers.h" namespace iceberg::rest { TEST(EndpointTest, InvalidCreate) { // Empty path template should fail - auto result = Endpoint::Create(HttpMethod::GET, ""); + auto result = Endpoint::Make(HttpMethod::kGet, ""); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(result, HasErrorMessage("Path template cannot be empty")); } @@ -39,47 +38,47 @@ TEST(EndpointTest, ValidFromString) { EXPECT_THAT(result, IsOk()); auto endpoint = result.value(); - EXPECT_EQ(endpoint.method(), HttpMethod::GET); - EXPECT_EQ(endpoint.path_template(), "/path"); + EXPECT_EQ(endpoint.method(), HttpMethod::kGet); + EXPECT_EQ(endpoint.path(), "/path"); } TEST(EndpointTest, ToStringRepresentation) { - auto endpoint1 = Endpoint::Create(HttpMethod::POST, "/path/of/resource"); + auto endpoint1 = Endpoint::Make(HttpMethod::kPost, "/path/of/resource"); ASSERT_THAT(endpoint1, IsOk()); EXPECT_EQ(endpoint1->ToString(), "POST /path/of/resource"); - auto endpoint2 = Endpoint::Create(HttpMethod::GET, "/"); + auto endpoint2 = Endpoint::Make(HttpMethod::kGet, "/"); ASSERT_THAT(endpoint2, IsOk()); EXPECT_EQ(endpoint2->ToString(), "GET /"); - auto endpoint3 = Endpoint::Create(HttpMethod::PUT, "/"); + auto endpoint3 = Endpoint::Make(HttpMethod::kPut, "/"); ASSERT_THAT(endpoint3, IsOk()); EXPECT_EQ(endpoint3->ToString(), "PUT /"); - auto endpoint4 = Endpoint::Create(HttpMethod::PUT, "/namespaces/{namespace}/{x}"); + auto endpoint4 = Endpoint::Make(HttpMethod::kPut, "/namespaces/{namespace}/{x}"); ASSERT_THAT(endpoint4, IsOk()); EXPECT_EQ(endpoint4->ToString(), "PUT /namespaces/{namespace}/{x}"); } // Test all HTTP methods TEST(EndpointTest, AllHttpMethods) { - auto get = Endpoint::Create(HttpMethod::GET, "/path"); + auto get = Endpoint::Make(HttpMethod::kGet, "/path"); ASSERT_THAT(get, IsOk()); EXPECT_EQ(get->ToString(), "GET /path"); - auto post = Endpoint::Create(HttpMethod::POST, "/path"); + auto post = Endpoint::Make(HttpMethod::kPost, "/path"); ASSERT_THAT(post, IsOk()); EXPECT_EQ(post->ToString(), "POST /path"); - auto put = Endpoint::Create(HttpMethod::PUT, "/path"); + auto put = Endpoint::Make(HttpMethod::kPut, "/path"); ASSERT_THAT(put, IsOk()); EXPECT_EQ(put->ToString(), "PUT /path"); - auto del = Endpoint::Create(HttpMethod::DELETE_, "/path"); + auto del = Endpoint::Make(HttpMethod::kDelete, "/path"); ASSERT_THAT(del, IsOk()); EXPECT_EQ(del->ToString(), "DELETE /path"); - auto head = Endpoint::Create(HttpMethod::HEAD, "/path"); + auto head = Endpoint::Make(HttpMethod::kHead, "/path"); ASSERT_THAT(head, IsOk()); EXPECT_EQ(head->ToString(), "HEAD /path"); } @@ -87,95 +86,89 @@ TEST(EndpointTest, AllHttpMethods) { // Test predefined namespace endpoints TEST(EndpointTest, NamespaceEndpoints) { auto list_namespaces = Endpoint::ListNamespaces(); - EXPECT_EQ(list_namespaces.method(), HttpMethod::GET); - EXPECT_EQ(list_namespaces.path_template(), "/v1/{prefix}/namespaces"); + EXPECT_EQ(list_namespaces.method(), HttpMethod::kGet); + EXPECT_EQ(list_namespaces.path(), "/v1/{prefix}/namespaces"); EXPECT_EQ(list_namespaces.ToString(), "GET /v1/{prefix}/namespaces"); auto get_namespace = Endpoint::GetNamespaceProperties(); - EXPECT_EQ(get_namespace.method(), HttpMethod::GET); - EXPECT_EQ(get_namespace.path_template(), "/v1/{prefix}/namespaces/{namespace}"); + EXPECT_EQ(get_namespace.method(), HttpMethod::kGet); + EXPECT_EQ(get_namespace.path(), "/v1/{prefix}/namespaces/{namespace}"); auto namespace_exists = Endpoint::NamespaceExists(); - EXPECT_EQ(namespace_exists.method(), HttpMethod::HEAD); - EXPECT_EQ(namespace_exists.path_template(), "/v1/{prefix}/namespaces/{namespace}"); + EXPECT_EQ(namespace_exists.method(), HttpMethod::kHead); + EXPECT_EQ(namespace_exists.path(), "/v1/{prefix}/namespaces/{namespace}"); auto create_namespace = Endpoint::CreateNamespace(); - EXPECT_EQ(create_namespace.method(), HttpMethod::POST); - EXPECT_EQ(create_namespace.path_template(), "/v1/{prefix}/namespaces"); + EXPECT_EQ(create_namespace.method(), HttpMethod::kPost); + EXPECT_EQ(create_namespace.path(), "/v1/{prefix}/namespaces"); auto update_namespace = Endpoint::UpdateNamespace(); - EXPECT_EQ(update_namespace.method(), HttpMethod::POST); - EXPECT_EQ(update_namespace.path_template(), - "/v1/{prefix}/namespaces/{namespace}/properties"); + EXPECT_EQ(update_namespace.method(), HttpMethod::kPost); + EXPECT_EQ(update_namespace.path(), "/v1/{prefix}/namespaces/{namespace}/properties"); auto drop_namespace = Endpoint::DropNamespace(); - EXPECT_EQ(drop_namespace.method(), HttpMethod::DELETE_); - EXPECT_EQ(drop_namespace.path_template(), "/v1/{prefix}/namespaces/{namespace}"); + EXPECT_EQ(drop_namespace.method(), HttpMethod::kDelete); + EXPECT_EQ(drop_namespace.path(), "/v1/{prefix}/namespaces/{namespace}"); } // Test predefined table endpoints TEST(EndpointTest, TableEndpoints) { auto list_tables = Endpoint::ListTables(); - EXPECT_EQ(list_tables.method(), HttpMethod::GET); - EXPECT_EQ(list_tables.path_template(), "/v1/{prefix}/namespaces/{namespace}/tables"); + EXPECT_EQ(list_tables.method(), HttpMethod::kGet); + EXPECT_EQ(list_tables.path(), "/v1/{prefix}/namespaces/{namespace}/tables"); auto load_table = Endpoint::LoadTable(); - EXPECT_EQ(load_table.method(), HttpMethod::GET); - EXPECT_EQ(load_table.path_template(), - "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + EXPECT_EQ(load_table.method(), HttpMethod::kGet); + EXPECT_EQ(load_table.path(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); auto table_exists = Endpoint::TableExists(); - EXPECT_EQ(table_exists.method(), HttpMethod::HEAD); - EXPECT_EQ(table_exists.path_template(), - "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + EXPECT_EQ(table_exists.method(), HttpMethod::kHead); + EXPECT_EQ(table_exists.path(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); auto create_table = Endpoint::CreateTable(); - EXPECT_EQ(create_table.method(), HttpMethod::POST); - EXPECT_EQ(create_table.path_template(), "/v1/{prefix}/namespaces/{namespace}/tables"); + EXPECT_EQ(create_table.method(), HttpMethod::kPost); + EXPECT_EQ(create_table.path(), "/v1/{prefix}/namespaces/{namespace}/tables"); auto update_table = Endpoint::UpdateTable(); - EXPECT_EQ(update_table.method(), HttpMethod::POST); - EXPECT_EQ(update_table.path_template(), - "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + EXPECT_EQ(update_table.method(), HttpMethod::kPost); + EXPECT_EQ(update_table.path(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); auto delete_table = Endpoint::DeleteTable(); - EXPECT_EQ(delete_table.method(), HttpMethod::DELETE_); - EXPECT_EQ(delete_table.path_template(), - "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + EXPECT_EQ(delete_table.method(), HttpMethod::kDelete); + EXPECT_EQ(delete_table.path(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); auto rename_table = Endpoint::RenameTable(); - EXPECT_EQ(rename_table.method(), HttpMethod::POST); - EXPECT_EQ(rename_table.path_template(), "/v1/{prefix}/tables/rename"); + EXPECT_EQ(rename_table.method(), HttpMethod::kPost); + EXPECT_EQ(rename_table.path(), "/v1/{prefix}/tables/rename"); auto register_table = Endpoint::RegisterTable(); - EXPECT_EQ(register_table.method(), HttpMethod::POST); - EXPECT_EQ(register_table.path_template(), - "/v1/{prefix}/namespaces/{namespace}/register"); + EXPECT_EQ(register_table.method(), HttpMethod::kPost); + EXPECT_EQ(register_table.path(), "/v1/{prefix}/namespaces/{namespace}/register"); auto report_metrics = Endpoint::ReportMetrics(); - EXPECT_EQ(report_metrics.method(), HttpMethod::POST); - EXPECT_EQ(report_metrics.path_template(), + EXPECT_EQ(report_metrics.method(), HttpMethod::kPost); + EXPECT_EQ(report_metrics.path(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"); auto table_credentials = Endpoint::TableCredentials(); - EXPECT_EQ(table_credentials.method(), HttpMethod::GET); - EXPECT_EQ(table_credentials.path_template(), + EXPECT_EQ(table_credentials.method(), HttpMethod::kGet); + EXPECT_EQ(table_credentials.path(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials"); } // Test predefined transaction endpoints TEST(EndpointTest, TransactionEndpoints) { auto commit_transaction = Endpoint::CommitTransaction(); - EXPECT_EQ(commit_transaction.method(), HttpMethod::POST); - EXPECT_EQ(commit_transaction.path_template(), "/v1/{prefix}/transactions/commit"); + EXPECT_EQ(commit_transaction.method(), HttpMethod::kPost); + EXPECT_EQ(commit_transaction.path(), "/v1/{prefix}/transactions/commit"); } -// Test endpoint equality and comparison -TEST(EndpointTest, EqualityAndComparison) { - auto endpoint1 = Endpoint::Create(HttpMethod::GET, "/path"); - auto endpoint2 = Endpoint::Create(HttpMethod::GET, "/path"); - auto endpoint3 = Endpoint::Create(HttpMethod::POST, "/path"); - auto endpoint4 = Endpoint::Create(HttpMethod::GET, "/other"); +// Test endpoint equality +TEST(EndpointTest, Equality) { + auto endpoint1 = Endpoint::Make(HttpMethod::kGet, "/path"); + auto endpoint2 = Endpoint::Make(HttpMethod::kGet, "/path"); + auto endpoint3 = Endpoint::Make(HttpMethod::kPost, "/path"); + auto endpoint4 = Endpoint::Make(HttpMethod::kGet, "/other"); ASSERT_THAT(endpoint1, IsOk()); ASSERT_THAT(endpoint2, IsOk()); @@ -186,32 +179,28 @@ TEST(EndpointTest, EqualityAndComparison) { EXPECT_EQ(*endpoint1, *endpoint2); EXPECT_NE(*endpoint1, *endpoint3); EXPECT_NE(*endpoint1, *endpoint4); - - // Comparison (for ordering) - EXPECT_LT(*endpoint1, *endpoint3); // GET < POST - EXPECT_LT(*endpoint4, *endpoint1); // "/other" < "/path" lexicographically } // Test string serialization (endpoints are represented as strings) TEST(EndpointTest, ToStringFormat) { - auto endpoint1 = Endpoint::Create(HttpMethod::GET, "/v1/{prefix}/namespaces"); + auto endpoint1 = Endpoint::Make(HttpMethod::kGet, "/v1/{prefix}/namespaces"); ASSERT_THAT(endpoint1, IsOk()); EXPECT_EQ(endpoint1->ToString(), "GET /v1/{prefix}/namespaces"); - auto endpoint2 = Endpoint::Create(HttpMethod::POST, "/v1/{prefix}/tables"); + auto endpoint2 = Endpoint::Make(HttpMethod::kPost, "/v1/{prefix}/tables"); ASSERT_THAT(endpoint2, IsOk()); EXPECT_EQ(endpoint2->ToString(), "POST /v1/{prefix}/tables"); // Test with all HTTP methods - auto endpoint3 = Endpoint::Create(HttpMethod::DELETE_, "/path"); + auto endpoint3 = Endpoint::Make(HttpMethod::kDelete, "/path"); ASSERT_THAT(endpoint3, IsOk()); EXPECT_EQ(endpoint3->ToString(), "DELETE /path"); - auto endpoint4 = Endpoint::Create(HttpMethod::PUT, "/path"); + auto endpoint4 = Endpoint::Make(HttpMethod::kPut, "/path"); ASSERT_THAT(endpoint4, IsOk()); EXPECT_EQ(endpoint4->ToString(), "PUT /path"); - auto endpoint5 = Endpoint::Create(HttpMethod::HEAD, "/path"); + auto endpoint5 = Endpoint::Make(HttpMethod::kHead, "/path"); ASSERT_THAT(endpoint5, IsOk()); EXPECT_EQ(endpoint5->ToString(), "HEAD /path"); } @@ -220,26 +209,26 @@ TEST(EndpointTest, ToStringFormat) { TEST(EndpointTest, FromStringParsing) { auto result1 = Endpoint::FromString("GET /v1/{prefix}/namespaces"); ASSERT_THAT(result1, IsOk()); - EXPECT_EQ(result1->method(), HttpMethod::GET); - EXPECT_EQ(result1->path_template(), "/v1/{prefix}/namespaces"); + EXPECT_EQ(result1->method(), HttpMethod::kGet); + EXPECT_EQ(result1->path(), "/v1/{prefix}/namespaces"); auto result2 = Endpoint::FromString("POST /v1/{prefix}/namespaces/{namespace}/tables"); ASSERT_THAT(result2, IsOk()); - EXPECT_EQ(result2->method(), HttpMethod::POST); - EXPECT_EQ(result2->path_template(), "/v1/{prefix}/namespaces/{namespace}/tables"); + EXPECT_EQ(result2->method(), HttpMethod::kPost); + EXPECT_EQ(result2->path(), "/v1/{prefix}/namespaces/{namespace}/tables"); // Test all HTTP methods auto result3 = Endpoint::FromString("DELETE /path"); ASSERT_THAT(result3, IsOk()); - EXPECT_EQ(result3->method(), HttpMethod::DELETE_); + EXPECT_EQ(result3->method(), HttpMethod::kDelete); auto result4 = Endpoint::FromString("PUT /path"); ASSERT_THAT(result4, IsOk()); - EXPECT_EQ(result4->method(), HttpMethod::PUT); + EXPECT_EQ(result4->method(), HttpMethod::kPut); auto result5 = Endpoint::FromString("HEAD /path"); ASSERT_THAT(result5, IsOk()); - EXPECT_EQ(result5->method(), HttpMethod::HEAD); + EXPECT_EQ(result5->method(), HttpMethod::kHead); } // Test string parsing with invalid inputs diff --git a/src/iceberg/test/rest_json_internal_test.cc b/src/iceberg/test/rest_json_internal_test.cc index 7b4b5d4c4..f95ab09cb 100644 --- a/src/iceberg/test/rest_json_internal_test.cc +++ b/src/iceberg/test/rest_json_internal_test.cc @@ -791,14 +791,14 @@ INSTANTIATE_TEST_SUITE_P( .model = {.defaults = {{"warehouse", "s3://bucket/warehouse"}}, .overrides = {{"clients", "5"}}, - .endpoints = {*Endpoint::Create(HttpMethod::GET, "/v1/config"), - *Endpoint::Create(HttpMethod::POST, "/v1/tables")}}}, + .endpoints = {*Endpoint::Make(HttpMethod::kGet, "/v1/config"), + *Endpoint::Make(HttpMethod::kPost, "/v1/tables")}}}, // Only endpoints CatalogConfigParam{ .test_name = "OnlyEndpoints", .expected_json_str = R"({"defaults":{},"overrides":{},"endpoints":["GET /v1/config"]})", - .model = {.endpoints = {*Endpoint::Create(HttpMethod::GET, "/v1/config")}}}), + .model = {.endpoints = {*Endpoint::Make(HttpMethod::kGet, "/v1/config")}}}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); diff --git a/src/iceberg/test/rest_util_test.cc b/src/iceberg/test/rest_util_test.cc index 5b32640b7..63ecaf965 100644 --- a/src/iceberg/test/rest_util_test.cc +++ b/src/iceberg/test/rest_util_test.cc @@ -155,8 +155,8 @@ TEST(RestUtilTest, MergeConfigs) { } TEST(RestUtilTest, CheckEndpointSupported) { - std::set supported = {Endpoint::ListNamespaces(), Endpoint::LoadTable(), - Endpoint::CreateTable()}; + std::unordered_set supported = { + Endpoint::ListNamespaces(), Endpoint::LoadTable(), Endpoint::CreateTable()}; // Supported endpoints should pass EXPECT_THAT(CheckEndpoint(supported, Endpoint::ListNamespaces()), IsOk()); From f095436a3697cff7f2392a08832dddd534f342b5 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Tue, 16 Dec 2025 16:50:28 +0800 Subject: [PATCH 6/7] fix review --- src/iceberg/catalog/rest/endpoint.cc | 8 ++++---- src/iceberg/catalog/rest/endpoint.h | 7 ++++--- src/iceberg/catalog/rest/rest_catalog.cc | 24 ++++++++++-------------- src/iceberg/test/endpoint_test.cc | 22 ++-------------------- 4 files changed, 20 insertions(+), 41 deletions(-) diff --git a/src/iceberg/catalog/rest/endpoint.cc b/src/iceberg/catalog/rest/endpoint.cc index e0c5564e9..bf457c879 100644 --- a/src/iceberg/catalog/rest/endpoint.cc +++ b/src/iceberg/catalog/rest/endpoint.cc @@ -40,11 +40,11 @@ constexpr std::string_view ToString(HttpMethod method) { return "UNKNOWN"; } -Result Endpoint::Make(HttpMethod method, std::string_view path_template) { - if (path_template.empty()) { - return InvalidArgument("Path template cannot be empty"); +Result Endpoint::Make(HttpMethod method, std::string_view path) { + if (path.empty()) { + return InvalidArgument("Endpoint cannot have empty path"); } - return Endpoint(method, path_template); + return Endpoint(method, path); } Result Endpoint::FromString(std::string_view str) { diff --git a/src/iceberg/catalog/rest/endpoint.h b/src/iceberg/catalog/rest/endpoint.h index 65915e61d..395a80590 100644 --- a/src/iceberg/catalog/rest/endpoint.h +++ b/src/iceberg/catalog/rest/endpoint.h @@ -45,9 +45,9 @@ class ICEBERG_REST_EXPORT Endpoint { /// \brief Make an endpoint with method and path template. /// /// \param method HTTP method (GET, POST, etc.) - /// \param path_template Path template with placeholders (e.g., "/v1/{prefix}/tables") + /// \param path Path template with placeholders (e.g., "/v1/{prefix}/tables") /// \return Endpoint instance or error if invalid - static Result Make(HttpMethod method, std::string_view path_template); + static Result Make(HttpMethod method, std::string_view path); /// \brief Parse endpoint from string representation. "METHOD" have to be all /// upper-cased. @@ -137,7 +137,8 @@ class ICEBERG_REST_EXPORT Endpoint { struct ICEBERG_REST_EXPORT EndpointHash { std::size_t operator()(const Endpoint& endpoint) const noexcept { - return std::hash()(endpoint.ToString()); + return std::hash()(static_cast(endpoint.method())) * 31 + + std::hash()(endpoint.path()); } }; diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index cb4244ec6..b7c150589 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -20,8 +20,8 @@ #include "iceberg/catalog/rest/rest_catalog.h" #include -#include #include +#include #include #include @@ -78,18 +78,15 @@ Result FetchServerConfig(const ResourcePaths& paths, RestCatalog::~RestCatalog() = default; Result> RestCatalog::Make( - const RestCatalogProperties& initial_config) { - ICEBERG_ASSIGN_OR_RAISE(auto uri, initial_config.Uri()); + const RestCatalogProperties& config) { + ICEBERG_ASSIGN_OR_RAISE(auto uri, config.Uri()); ICEBERG_ASSIGN_OR_RAISE( - auto paths, - ResourcePaths::Make(std::string(TrimTrailingSlash(uri)), - initial_config.Get(RestCatalogProperties::kPrefix))); - // get the raw config from server - ICEBERG_ASSIGN_OR_RAISE(auto server_config, FetchServerConfig(*paths, initial_config)); + auto paths, ResourcePaths::Make(std::string(TrimTrailingSlash(uri)), + config.Get(RestCatalogProperties::kPrefix))); + ICEBERG_ASSIGN_OR_RAISE(auto server_config, FetchServerConfig(*paths, config)); - std::unique_ptr final_config = - RestCatalogProperties::FromMap(MergeConfigs( - server_config.overrides, initial_config.configs(), server_config.defaults)); + std::unique_ptr final_config = RestCatalogProperties::FromMap( + MergeConfigs(server_config.overrides, config.configs(), server_config.defaults)); std::unordered_set endpoints; if (!server_config.endpoints.empty()) { @@ -107,7 +104,7 @@ Result> RestCatalog::Make( ICEBERG_RETURN_UNEXPECTED(paths->SetBaseUri(std::string(TrimTrailingSlash(final_uri)))); return std::unique_ptr( - new RestCatalog(std::move(final_config), std::move(paths), endpoints)); + new RestCatalog(std::move(final_config), std::move(paths), std::move(endpoints))); } RestCatalog::RestCatalog(std::unique_ptr config, @@ -193,9 +190,8 @@ Status RestCatalog::DropNamespace(const Namespace& ns) { Result RestCatalog::NamespaceExists(const Namespace& ns) const { auto check = CheckEndpoint(supported_endpoints_, Endpoint::NamespaceExists()); - // If server does not support HEAD endpoint, fall back to GetNamespaceProperties if (!check.has_value()) { - ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns)); + // Fall back to GetNamespaceProperties auto result = GetNamespaceProperties(ns); if (!result.has_value() && result.error().kind == ErrorKind::kNoSuchNamespace) { return false; diff --git a/src/iceberg/test/endpoint_test.cc b/src/iceberg/test/endpoint_test.cc index 901dc1a62..fcdc92a78 100644 --- a/src/iceberg/test/endpoint_test.cc +++ b/src/iceberg/test/endpoint_test.cc @@ -30,7 +30,7 @@ TEST(EndpointTest, InvalidCreate) { // Empty path template should fail auto result = Endpoint::Make(HttpMethod::kGet, ""); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); - EXPECT_THAT(result, HasErrorMessage("Path template cannot be empty")); + EXPECT_THAT(result, HasErrorMessage("Endpoint cannot have empty path")); } TEST(EndpointTest, ValidFromString) { @@ -42,24 +42,6 @@ TEST(EndpointTest, ValidFromString) { EXPECT_EQ(endpoint.path(), "/path"); } -TEST(EndpointTest, ToStringRepresentation) { - auto endpoint1 = Endpoint::Make(HttpMethod::kPost, "/path/of/resource"); - ASSERT_THAT(endpoint1, IsOk()); - EXPECT_EQ(endpoint1->ToString(), "POST /path/of/resource"); - - auto endpoint2 = Endpoint::Make(HttpMethod::kGet, "/"); - ASSERT_THAT(endpoint2, IsOk()); - EXPECT_EQ(endpoint2->ToString(), "GET /"); - - auto endpoint3 = Endpoint::Make(HttpMethod::kPut, "/"); - ASSERT_THAT(endpoint3, IsOk()); - EXPECT_EQ(endpoint3->ToString(), "PUT /"); - - auto endpoint4 = Endpoint::Make(HttpMethod::kPut, "/namespaces/{namespace}/{x}"); - ASSERT_THAT(endpoint4, IsOk()); - EXPECT_EQ(endpoint4->ToString(), "PUT /namespaces/{namespace}/{x}"); -} - // Test all HTTP methods TEST(EndpointTest, AllHttpMethods) { auto get = Endpoint::Make(HttpMethod::kGet, "/path"); @@ -181,7 +163,7 @@ TEST(EndpointTest, Equality) { EXPECT_NE(*endpoint1, *endpoint4); } -// Test string serialization (endpoints are represented as strings) +// Test string serialization TEST(EndpointTest, ToStringFormat) { auto endpoint1 = Endpoint::Make(HttpMethod::kGet, "/v1/{prefix}/namespaces"); ASSERT_THAT(endpoint1, IsOk()); From 910eb508938ecfd2d1f5d3d3ab6034bc53f94415 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Tue, 16 Dec 2025 17:16:19 +0800 Subject: [PATCH 7/7] 1 --- src/iceberg/catalog/rest/endpoint.h | 17 +++++++++++------ src/iceberg/catalog/rest/rest_catalog.cc | 10 +++++----- src/iceberg/catalog/rest/rest_catalog.h | 4 ++-- src/iceberg/catalog/rest/rest_util.cc | 5 ++--- src/iceberg/catalog/rest/rest_util.h | 5 ++--- src/iceberg/test/rest_util_test.cc | 2 +- 6 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/iceberg/catalog/rest/endpoint.h b/src/iceberg/catalog/rest/endpoint.h index 395a80590..7382955ce 100644 --- a/src/iceberg/catalog/rest/endpoint.h +++ b/src/iceberg/catalog/rest/endpoint.h @@ -135,11 +135,16 @@ class ICEBERG_REST_EXPORT Endpoint { std::string path_; }; -struct ICEBERG_REST_EXPORT EndpointHash { - std::size_t operator()(const Endpoint& endpoint) const noexcept { - return std::hash()(static_cast(endpoint.method())) * 31 + - std::hash()(endpoint.path()); +} // namespace iceberg::rest + +// Specialize std::hash for Endpoint +namespace std { +template <> +struct hash { + std::size_t operator()(const iceberg::rest::Endpoint& endpoint) const noexcept { + std::size_t h1 = std::hash{}(static_cast(endpoint.method())); + std::size_t h2 = std::hash{}(endpoint.path()); + return h1 ^ (h2 + 0x9e3779b9 + (h1 << 6) + (h1 >> 2)); } }; - -} // namespace iceberg::rest +} // namespace std diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index b7c150589..b9dfaafc7 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -49,7 +49,7 @@ namespace { /// \brief Get the default set of endpoints for backwards compatibility according to the /// iceberg rest spec. -std::unordered_set GetDefaultEndpoints() { +std::unordered_set GetDefaultEndpoints() { return { Endpoint::ListNamespaces(), Endpoint::GetNamespaceProperties(), Endpoint::CreateNamespace(), Endpoint::UpdateNamespace(), @@ -88,11 +88,11 @@ Result> RestCatalog::Make( std::unique_ptr final_config = RestCatalogProperties::FromMap( MergeConfigs(server_config.overrides, config.configs(), server_config.defaults)); - std::unordered_set endpoints; + std::unordered_set endpoints; if (!server_config.endpoints.empty()) { // Endpoints are already parsed during JSON deserialization, just convert to set - endpoints = std::unordered_set( - server_config.endpoints.begin(), server_config.endpoints.end()); + endpoints = std::unordered_set(server_config.endpoints.begin(), + server_config.endpoints.end()); } else { // If a server does not send the endpoints field, use default set of endpoints // for backwards compatibility with legacy servers @@ -109,7 +109,7 @@ Result> RestCatalog::Make( RestCatalog::RestCatalog(std::unique_ptr config, std::unique_ptr paths, - std::unordered_set endpoints) + std::unordered_set endpoints) : config_(std::move(config)), client_(std::make_unique(config_->ExtractHeaders())), paths_(std::move(paths)), diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index 55cad9787..c8ddca587 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -101,13 +101,13 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog { private: RestCatalog(std::unique_ptr config, std::unique_ptr paths, - std::unordered_set endpoints); + std::unordered_set endpoints); std::unique_ptr config_; std::unique_ptr client_; std::unique_ptr paths_; std::string name_; - std::unordered_set supported_endpoints_; + std::unordered_set supported_endpoints_; }; } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_util.cc b/src/iceberg/catalog/rest/rest_util.cc index 5a6b06a87..62b9bfc33 100644 --- a/src/iceberg/catalog/rest/rest_util.cc +++ b/src/iceberg/catalog/rest/rest_util.cc @@ -253,9 +253,8 @@ std::string GetStandardReasonPhrase(int32_t status_code) { } } -Status CheckEndpoint( - const std::unordered_set& supported_endpoints, - const Endpoint& endpoint) { +Status CheckEndpoint(const std::unordered_set& supported_endpoints, + const Endpoint& endpoint) { if (!supported_endpoints.contains(endpoint)) { return NotSupported("Server does not support endpoint: {}", endpoint.ToString()); } diff --git a/src/iceberg/catalog/rest/rest_util.h b/src/iceberg/catalog/rest/rest_util.h index 05b162903..5734bbc72 100644 --- a/src/iceberg/catalog/rest/rest_util.h +++ b/src/iceberg/catalog/rest/rest_util.h @@ -98,8 +98,7 @@ ICEBERG_REST_EXPORT std::string GetStandardReasonPhrase(int32_t status_code); /// \param supported_endpoints Set of endpoints advertised by the server /// \param endpoint Endpoint to validate /// \return Status::OK if supported, NotSupported error otherwise -ICEBERG_REST_EXPORT Status -CheckEndpoint(const std::unordered_set& supported_endpoints, - const Endpoint& endpoint); +ICEBERG_REST_EXPORT Status CheckEndpoint( + const std::unordered_set& supported_endpoints, const Endpoint& endpoint); } // namespace iceberg::rest diff --git a/src/iceberg/test/rest_util_test.cc b/src/iceberg/test/rest_util_test.cc index 63ecaf965..5d2abf558 100644 --- a/src/iceberg/test/rest_util_test.cc +++ b/src/iceberg/test/rest_util_test.cc @@ -155,7 +155,7 @@ TEST(RestUtilTest, MergeConfigs) { } TEST(RestUtilTest, CheckEndpointSupported) { - std::unordered_set supported = { + std::unordered_set supported = { Endpoint::ListNamespaces(), Endpoint::LoadTable(), Endpoint::CreateTable()}; // Supported endpoints should pass