fix review

Author: HeartLinked

src/iceberg/catalog/rest/endpoint.cc

@@ -40,11 +40,11 @@ constexpr std::string_view ToString(HttpMethod method) {
   return "UNKNOWN";
 }
 
-Result<Endpoint> Endpoint::Make(HttpMethod method, std::string_view path_template) {
-  if (path_template.empty()) {
-    return InvalidArgument("Path template cannot be empty");
+Result<Endpoint> 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> Endpoint::FromString(std::string_view str) {

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<Endpoint> Make(HttpMethod method, std::string_view path_template);
+  static Result<Endpoint> 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<std::string>()(endpoint.ToString());
+    return std::hash<int32_t>()(static_cast<int32_t>(endpoint.method())) * 31 +
+           std::hash<std::string_view>()(endpoint.path());
   }
 };
 

src/iceberg/catalog/rest/rest_catalog.cc

@@ -20,8 +20,8 @@
 #include "iceberg/catalog/rest/rest_catalog.h"
 
 #include <memory>
-#include <set>
 #include <unordered_map>
+#include <unordered_set>
 #include <utility>
 
 #include <nlohmann/json.hpp>
@@ -78,18 +78,15 @@ Result<CatalogConfig> FetchServerConfig(const ResourcePaths& paths,
 RestCatalog::~RestCatalog() = default;
 
 Result<std::unique_ptr<RestCatalog>> 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<RestCatalogProperties> final_config =
-      RestCatalogProperties::FromMap(MergeConfigs(
-          server_config.overrides, initial_config.configs(), server_config.defaults));
+  std::unique_ptr<RestCatalogProperties> final_config = RestCatalogProperties::FromMap(
+      MergeConfigs(server_config.overrides, config.configs(), server_config.defaults));
 
   std::unordered_set<Endpoint, EndpointHash> endpoints;
   if (!server_config.endpoints.empty()) {
@@ -107,7 +104,7 @@ Result<std::unique_ptr<RestCatalog>> RestCatalog::Make(
   ICEBERG_RETURN_UNEXPECTED(paths->SetBaseUri(std::string(TrimTrailingSlash(final_uri))));
 
   return std::unique_ptr<RestCatalog>(
-      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<RestCatalogProperties> config,
@@ -193,9 +190,8 @@ Status RestCatalog::DropNamespace(const Namespace& ns) {
 
 Result<bool> 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;

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());