wgtmac commented on code in PR #406: URL: https://github.com/apache/iceberg-cpp/pull/406#discussion_r2622091081
########## src/iceberg/catalog/rest/endpoint.h: ########## @@ -0,0 +1,144 @@ +/* + * 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 <string> +#include <string_view> + +#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 { kGet, kPost, kPut, kDelete, kHead }; + +/// \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 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<Endpoint> Make(HttpMethod method, std::string_view path_template); Review Comment: ```suggestion /// \param path Path with placeholders (e.g., "/v1/{prefix}/tables") /// \return Endpoint instance or error if invalid static Result<Endpoint> Make(HttpMethod method, std::string_view path); ``` ########## src/iceberg/catalog/rest/rest_catalog.cc: ########## @@ -44,53 +47,85 @@ namespace iceberg::rest { namespace { -// Fetch server config and merge it with client config -Result<std::unique_ptr<RestCatalogProperties>> 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::unordered_set<Endpoint, EndpointHash> 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<CatalogConfig> 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 RestCatalog::~RestCatalog() = default; Result<std::unique_ptr<RestCatalog>> 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 Review Comment: ```suggestion ``` This comment seems unnecessary. ########## src/iceberg/catalog/rest/endpoint.cc: ########## @@ -0,0 +1,90 @@ +/* + * 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 <format> +#include <string_view> + +namespace iceberg::rest { + +constexpr std::string_view ToString(HttpMethod method) { + switch (method) { + case HttpMethod::kGet: + return "GET"; + case HttpMethod::kPost: + return "POST"; + case HttpMethod::kPut: + return "PUT"; + case HttpMethod::kDelete: + return "DELETE"; + case HttpMethod::kHead: + return "HEAD"; + } + return "UNKNOWN"; +} + +Result<Endpoint> Endpoint::Make(HttpMethod method, std::string_view path_template) { + if (path_template.empty()) { + return InvalidArgument("Path template cannot be empty"); + } + return Endpoint(method, path_template); +} Review Comment: ```suggestion Result<Endpoint> Endpoint::Make(HttpMethod method, std::string_view path) { if (path.empty()) { return InvalidArgument("Endpoint cannot have empty path"); } return Endpoint(method, path); } ``` ########## src/iceberg/catalog/rest/rest_catalog.cc: ########## @@ -44,53 +47,85 @@ namespace iceberg::rest { namespace { -// Fetch server config and merge it with client config -Result<std::unique_ptr<RestCatalogProperties>> 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::unordered_set<Endpoint, EndpointHash> 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<CatalogConfig> 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 RestCatalog::~RestCatalog() = default; Result<std::unique_ptr<RestCatalog>> 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<RestCatalogProperties> final_config = + RestCatalogProperties::FromMap(MergeConfigs( + server_config.overrides, initial_config.configs(), server_config.defaults)); + + std::unordered_set<Endpoint, EndpointHash> endpoints; + if (!server_config.endpoints.empty()) { + // Endpoints are already parsed during JSON deserialization, just convert to set + endpoints = std::unordered_set<Endpoint, EndpointHash>( + 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<RestCatalog>( - new RestCatalog(std::move(final_config), std::move(paths))); + new RestCatalog(std::move(final_config), std::move(paths), endpoints)); Review Comment: ```suggestion new RestCatalog(std::move(final_config), std::move(paths), std::move(endpoints))); ``` ########## src/iceberg/catalog/rest/rest_catalog.cc: ########## @@ -131,29 +169,45 @@ Status RestCatalog::CreateNamespace( Result<std::unordered_map<std::string, std::string>> 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)); return get_response.properties; } 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<bool> 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 Review Comment: ```suggestion // Fall back to GetNamespaceProperties ``` ########## src/iceberg/catalog/rest/endpoint.h: ########## @@ -0,0 +1,144 @@ +/* + * 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 <string> +#include <string_view> + +#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 { kGet, kPost, kPut, kDelete, kHead }; + +/// \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 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<Endpoint> Make(HttpMethod method, std::string_view path_template); + + /// \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. + static Result<Endpoint> 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() const { return path_; } + + /// \brief Serialize to "METHOD /path" format. + std::string ToString() const; + + constexpr bool operator==(const Endpoint& other) const { + return method_ == other.method_ && path_ == other.path_; + } + + // Namespace endpoints + static Endpoint ListNamespaces() { + return {HttpMethod::kGet, "/v1/{prefix}/namespaces"}; + } + static Endpoint GetNamespaceProperties() { + return {HttpMethod::kGet, "/v1/{prefix}/namespaces/{namespace}"}; + } + static Endpoint NamespaceExists() { + return {HttpMethod::kHead, "/v1/{prefix}/namespaces/{namespace}"}; + } + static Endpoint CreateNamespace() { + return {HttpMethod::kPost, "/v1/{prefix}/namespaces"}; + } + static Endpoint UpdateNamespace() { + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/properties"}; + } + static Endpoint DropNamespace() { + return {HttpMethod::kDelete, "/v1/{prefix}/namespaces/{namespace}"}; + } + + // Table endpoints + static Endpoint ListTables() { + return {HttpMethod::kGet, "/v1/{prefix}/namespaces/{namespace}/tables"}; + } + static Endpoint LoadTable() { + return {HttpMethod::kGet, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + } + static Endpoint TableExists() { + return {HttpMethod::kHead, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + } + static Endpoint CreateTable() { + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/tables"}; + } + static Endpoint UpdateTable() { + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + } + static Endpoint DeleteTable() { + return {HttpMethod::kDelete, "/v1/{prefix}/namespaces/{namespace}/tables/{table}"}; + } + static Endpoint RenameTable() { + return {HttpMethod::kPost, "/v1/{prefix}/tables/rename"}; + } + static Endpoint RegisterTable() { + return {HttpMethod::kPost, "/v1/{prefix}/namespaces/{namespace}/register"}; + } + static Endpoint ReportMetrics() { + return {HttpMethod::kPost, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"}; + } + static Endpoint TableCredentials() { + return {HttpMethod::kGet, + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials"}; + } + + // Transaction endpoints + static Endpoint CommitTransaction() { + return {HttpMethod::kPost, "/v1/{prefix}/transactions/commit"}; + } + + private: + Endpoint(HttpMethod method, std::string_view path) : method_(method), path_(path) {} + + HttpMethod method_; + std::string path_; +}; + +struct ICEBERG_REST_EXPORT EndpointHash { + std::size_t operator()(const Endpoint& endpoint) const noexcept { + return std::hash<std::string>()(endpoint.ToString()); Review Comment: This is inefficient since we create a temp string which is unnecessary. It would be better to write it as: ``` return std::hash<int32_t>(static_cast<int32_t>(endpoint.method_)) * 31 + StringHash{}(endpoint.path_); ``` ########## src/iceberg/test/endpoint_test.cc: ########## @@ -0,0 +1,279 @@ +/* + * 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 <gtest/gtest.h> +#include <nlohmann/json.hpp> + +#include "iceberg/test/matchers.h" + +namespace iceberg::rest { + +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")); +} + +TEST(EndpointTest, ValidFromString) { + auto result = Endpoint::FromString("GET /path"); + EXPECT_THAT(result, IsOk()); + + auto endpoint = result.value(); + EXPECT_EQ(endpoint.method(), HttpMethod::kGet); + 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"); + ASSERT_THAT(get, IsOk()); + EXPECT_EQ(get->ToString(), "GET /path"); + + auto post = Endpoint::Make(HttpMethod::kPost, "/path"); + ASSERT_THAT(post, IsOk()); + EXPECT_EQ(post->ToString(), "POST /path"); + + auto put = Endpoint::Make(HttpMethod::kPut, "/path"); + ASSERT_THAT(put, IsOk()); + EXPECT_EQ(put->ToString(), "PUT /path"); + + auto del = Endpoint::Make(HttpMethod::kDelete, "/path"); + ASSERT_THAT(del, IsOk()); + EXPECT_EQ(del->ToString(), "DELETE /path"); + + auto head = Endpoint::Make(HttpMethod::kHead, "/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::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::kGet); + EXPECT_EQ(get_namespace.path(), "/v1/{prefix}/namespaces/{namespace}"); + + auto namespace_exists = Endpoint::NamespaceExists(); + 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::kPost); + EXPECT_EQ(create_namespace.path(), "/v1/{prefix}/namespaces"); + + auto update_namespace = Endpoint::UpdateNamespace(); + 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::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::kGet); + EXPECT_EQ(list_tables.path(), "/v1/{prefix}/namespaces/{namespace}/tables"); + + auto load_table = Endpoint::LoadTable(); + 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::kHead); + EXPECT_EQ(table_exists.path(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + + auto create_table = Endpoint::CreateTable(); + 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::kPost); + EXPECT_EQ(update_table.path(), "/v1/{prefix}/namespaces/{namespace}/tables/{table}"); + + auto delete_table = Endpoint::DeleteTable(); + 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::kPost); + EXPECT_EQ(rename_table.path(), "/v1/{prefix}/tables/rename"); + + auto register_table = Endpoint::RegisterTable(); + 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::kPost); + EXPECT_EQ(report_metrics.path(), + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"); + + auto table_credentials = Endpoint::TableCredentials(); + 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::kPost); + EXPECT_EQ(commit_transaction.path(), "/v1/{prefix}/transactions/commit"); +} + +// 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()); + ASSERT_THAT(endpoint3, IsOk()); + ASSERT_THAT(endpoint4, IsOk()); + + // Equality + EXPECT_EQ(*endpoint1, *endpoint2); + EXPECT_NE(*endpoint1, *endpoint3); + EXPECT_NE(*endpoint1, *endpoint4); +} + +// Test string serialization (endpoints are represented as strings) +TEST(EndpointTest, ToStringFormat) { Review Comment: Isn't it duplicated with `TEST(EndpointTest, ToStringRepresentation)`? ########## src/iceberg/catalog/rest/rest_catalog.cc: ########## @@ -20,19 +20,22 @@ #include "iceberg/catalog/rest/rest_catalog.h" #include <memory> +#include <set> Review Comment: ```suggestion ``` Should we include `unordered_set` instead? ########## src/iceberg/catalog/rest/rest_catalog.cc: ########## @@ -131,29 +169,45 @@ Status RestCatalog::CreateNamespace( Result<std::unordered_map<std::string, std::string>> 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)); return get_response.properties; } 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<bool> 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)); Review Comment: ```suggestion ``` ########## src/iceberg/catalog/rest/rest_catalog.cc: ########## @@ -44,53 +47,85 @@ namespace iceberg::rest { namespace { -// Fetch server config and merge it with client config -Result<std::unique_ptr<RestCatalogProperties>> 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::unordered_set<Endpoint, EndpointHash> 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<CatalogConfig> 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 RestCatalog::~RestCatalog() = default; Result<std::unique_ptr<RestCatalog>> 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()); Review Comment: ```suggestion const RestCatalogProperties& config) { ICEBERG_ASSIGN_OR_RAISE(auto uri, config.Uri()); ``` This is inconsistent with the function declaration. I think its original name is better. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
