fix2

Author: HeartLinked

src/iceberg/catalog/rest/catalog_properties.cc

@@ -19,8 +19,6 @@
 
 #include "iceberg/catalog/rest/catalog_properties.h"
 
-#include "iceberg/catalog/rest/constant.h"
-
 namespace iceberg::rest {
 
 std::unique_ptr<RestCatalogConfig> RestCatalogConfig::default_properties() {
@@ -34,11 +32,8 @@ std::unique_ptr<RestCatalogConfig> RestCatalogConfig::FromMap(
   return rest_catalog_config;
 }
 
-std::unordered_map<std::string, std::string> RestCatalogConfig::GetExtraHeaders() const {
+std::unordered_map<std::string, std::string> RestCatalogConfig::ExtractHeaders() const {
   std::unordered_map<std::string, std::string> headers;
-  headers[kHeaderContentType] = kMimeTypeApplicationJson;
-  headers[kHeaderUserAgent] = kUserAgent;
-
   constexpr std::string_view prefix = "header.";
   for (const auto& [key, value] : configs_) {
     if (key.starts_with(prefix)) {

src/iceberg/catalog/rest/catalog_properties.h

@@ -41,13 +41,13 @@ class ICEBERG_REST_EXPORT RestCatalogConfig : public ConfigBase<RestCatalogConfi
   using Entry = const ConfigBase<RestCatalogConfig>::Entry<T>;
 
   /// \brief The URI of the REST catalog server.
-  inline static std::string_view kUri{"uri"};
+  inline static Entry<std::string> kUri{"uri", ""};
 
   /// \brief The name of the catalog.
-  inline static std::string_view kName{"name"};
+  inline static Entry<std::string> kName{"name", ""};
 
   /// \brief The warehouse path.
-  inline static std::string_view kWarehouse{"warehouse"};
+  inline static Entry<std::string> kWarehouse{"warehouse", ""};
 
   /// \brief Create a default RestCatalogConfig instance.
   static std::unique_ptr<RestCatalogConfig> default_properties();
@@ -56,13 +56,11 @@ class ICEBERG_REST_EXPORT RestCatalogConfig : public ConfigBase<RestCatalogConfi
   static std::unique_ptr<RestCatalogConfig> FromMap(
       const std::unordered_map<std::string, std::string>& properties);
 
-  /// \brief Generates extra HTTP headers to be added to every request from the
-  /// configuration.
+  /// \brief Returns HTTP headers to be added to every request.
   ///
-  /// This includes default headers like Content-Type, User-Agent, X-Client-Version and
-  /// any custom headers prefixed with "header." in the properties.
-  /// \return A map of header names to values.
-  std::unordered_map<std::string, std::string> GetExtraHeaders() const;
+  /// This includes any key prefixed with "header." in the properties.
+  /// \return A map of headers with the prefix removed from the keys.
+  std::unordered_map<std::string, std::string> ExtractHeaders() const;
 };
 
 }  // namespace iceberg::rest

src/iceberg/catalog/rest/http_client.cc

@@ -24,6 +24,7 @@
 #include "cpr/body.h"
 #include "cpr/cprtypes.h"
 #include "iceberg/catalog/rest/catalog_properties.h"
+#include "iceberg/catalog/rest/constant.h"
 #include "iceberg/catalog/rest/json_internal.h"
 #include "iceberg/json_internal.h"
 #include "iceberg/util/macros.h"
@@ -88,8 +89,11 @@ void HttpClient::PrepareSession(
 }
 
 HttpClient::HttpClient(const RestCatalogConfig& config)
-    : default_headers_{config.GetExtraHeaders()},
-      session_{std::make_unique<cpr::Session>()} {}
+    : default_headers_{config.ExtractHeaders()},
+      session_{std::make_unique<cpr::Session>()} {
+  default_headers_[kHeaderContentType] = kMimeTypeApplicationJson;
+  default_headers_[kHeaderUserAgent] = kUserAgent;
+}
 
 Result<HttpResponse> HttpClient::Get(
     const std::string& path, const std::unordered_map<std::string, std::string>& params,

src/iceberg/catalog/rest/resource_paths.cc

@@ -30,7 +30,7 @@ namespace iceberg::rest {
 Result<std::unique_ptr<ResourcePaths>> ResourcePaths::Make(
     const RestCatalogConfig& config) {
   // Validate and extract URI
-  auto it = config.configs().find(std::string(RestCatalogConfig::kUri));
+  auto it = config.configs().find(RestCatalogConfig::kUri.key());
   if (it == config.configs().end() || it->second.empty()) {
     return InvalidArgument("Rest catalog configuration property 'uri' is required.");
   }
@@ -117,15 +117,4 @@ std::string ResourcePaths::CommitTransaction() const {
   return BuildPath("transactions/commit");
 }
 
-std::string ResourcePaths::Views(const Namespace& ns) const {
-  return BuildPath(std::format("namespaces/{}/views", EncodeNamespaceForUrl(ns)));
-}
-
-std::string ResourcePaths::View(const TableIdentifier& ident) const {
-  return BuildPath(
-      std::format("namespaces/{}/views/{}", EncodeNamespaceForUrl(ident.ns), ident.name));
-}
-
-std::string ResourcePaths::RenameView() const { return BuildPath("views/rename"); }
-
 }  // namespace iceberg::rest

src/iceberg/catalog/rest/resource_paths.h

@@ -92,15 +92,6 @@ class ICEBERG_REST_EXPORT ResourcePaths {
   /// \brief Get the /v1/{prefix}/transactions/commit endpoint path.
   std::string CommitTransaction() const;
 
-  /// \brief Get the /v1/{prefix}/namespaces/{namespace}/views endpoint path.
-  std::string Views(const Namespace& ns) const;
-
-  /// \brief Get the /v1/{prefix}/namespaces/{namespace}/views/{view} endpoint path.
-  std::string View(const TableIdentifier& ident) const;
-
-  /// \brief Get the /v1/{prefix}/views/rename endpoint path.
-  std::string RenameView() const;
-
  private:
   explicit ResourcePaths(std::string base_uri, std::string prefix);
 

src/iceberg/catalog/rest/rest_catalog.cc

@@ -41,44 +41,46 @@
 
 namespace iceberg::rest {
 
-Result<std::unique_ptr<RestCatalog>> RestCatalog::Make(const RestCatalogConfig& config) {
-  // Create ResourcePaths and validate URI
-  ICEBERG_ASSIGN_OR_RAISE(auto paths, ResourcePaths::Make(config));
-
+Result<std::unique_ptr<RestCatalogConfig>> RestCatalog::FetchAndMergeConfig(
+    const RestCatalogConfig& config, const ResourcePaths& paths) {
+  // Fetch server configuration
   auto tmp_client = std::make_unique<HttpClient>(config);
-  const std::string endpoint = paths->Config();
-  ICEBERG_ASSIGN_OR_RAISE(const HttpResponse& response,
+  const std::string endpoint = paths.Config();
+  ICEBERG_ASSIGN_OR_RAISE(const auto response,
                           tmp_client->Get(endpoint, {}, {}, DefaultErrorHandler()));
   ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body()));
   ICEBERG_ASSIGN_OR_RAISE(auto server_config, CatalogConfigFromJson(json));
+
   // Merge server config into client config, server config overrides > client config
   // properties > server config defaults
-  auto final_props = std::move(server_config.defaults);
-  for (const auto& kv : config.configs()) {
-    final_props.insert_or_assign(kv.first, kv.second);
-  }
+  auto final_props =
+      MergeConfigs(server_config.defaults, config.configs(), server_config.overrides);
+  return RestCatalogConfig::FromMap(final_props);
+}
+
+Result<std::unique_ptr<RestCatalog>> RestCatalog::Make(const RestCatalogConfig& config) {
+  ICEBERG_ASSIGN_OR_RAISE(auto paths, ResourcePaths::Make(config));
+  // Fetch and merge server configuration
+  ICEBERG_ASSIGN_OR_RAISE(auto final_config, FetchAndMergeConfig(config, *paths));
 
-  for (const auto& kv : server_config.overrides) {
-    final_props.insert_or_assign(kv.first, kv.second);
-  }
-  auto final_config = RestCatalogConfig::FromMap(final_props);
   auto client = std::make_unique<HttpClient>(*final_config);
   ICEBERG_ASSIGN_OR_RAISE(auto final_paths, ResourcePaths::Make(*final_config));
-  return std::unique_ptr<RestCatalog>(new RestCatalog(
-      std::move(final_config), std::move(client), std::move(*final_paths)));
+
+  std::string catalog_name = final_config->Get(RestCatalogConfig::kName);
+  return std::unique_ptr<RestCatalog>(
+      new RestCatalog(std::move(final_config), std::move(client), std::move(*final_paths),
+                      std::move(catalog_name)));
 }
 
 RestCatalog::RestCatalog(std::unique_ptr<RestCatalogConfig> config,
-                         std::unique_ptr<HttpClient> client, ResourcePaths paths)
-    : config_(std::move(config)), client_(std::move(client)), paths_(std::move(paths)) {}
-
-std::string_view RestCatalog::name() const {
-  auto it = config_->configs().find(std::string(RestCatalogConfig::kName));
-  if (it == config_->configs().end() || it->second.empty()) {
-    return {""};
-  }
-  return std::string_view(it->second);
-}
+                         std::unique_ptr<HttpClient> client, ResourcePaths paths,
+                         std::string name)
+    : config_(std::move(config)),
+      client_(std::move(client)),
+      paths_(std::move(paths)),
+      name_(std::move(name)) {}
+
+std::string_view RestCatalog::name() const { return name_; }
 
 Result<std::vector<Namespace>> RestCatalog::ListNamespaces(const Namespace& ns) const {
   const std::string endpoint = paths_.Namespaces();

src/iceberg/catalog/rest/rest_catalog.h

@@ -100,11 +100,20 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog {
 
  private:
   RestCatalog(std::unique_ptr<RestCatalogConfig> config,
-              std::unique_ptr<HttpClient> client, ResourcePaths paths);
+              std::unique_ptr<HttpClient> client, ResourcePaths paths, std::string name);
+
+  /// \brief Fetch server configuration and merge with client config
+  ///
+  /// \param config the initial client configuration
+  /// \param paths the resource paths for REST endpoints
+  /// \return the final merged configuration
+  static Result<std::unique_ptr<RestCatalogConfig>> FetchAndMergeConfig(
+      const RestCatalogConfig& config, const ResourcePaths& paths);
 
   std::unique_ptr<RestCatalogConfig> config_;
   std::unique_ptr<HttpClient> client_;
   ResourcePaths paths_;
+  std::string name_;  // Cached catalog name
 };
 
 }  // namespace iceberg::rest

src/iceberg/catalog/rest/rest_util.h

@@ -23,6 +23,7 @@
 #include <format>
 #include <string>
 #include <string_view>
+#include <unordered_map>
 
 #include "iceberg/table_identifier.h"
 
@@ -78,4 +79,25 @@ inline std::string EncodeNamespaceForUrl(const Namespace& ns) {
   return EncodeUriComponent(joined_string);
 }
 
+/// \brief Merge catalog configuration properties.
+/// \details Merges three sets of configuration properties following the precedence order:
+/// server overrides > client configs > server defaults.
+/// \param server_defaults Default properties provided by the server.
+/// \param client_configs Configuration properties from the client.
+/// \param server_overrides Override properties enforced by the server.
+/// \return A merged map containing all properties with correct precedence.
+inline std::unordered_map<std::string, std::string> MergeConfigs(
+    const std::unordered_map<std::string, std::string>& server_defaults,
+    const std::unordered_map<std::string, std::string>& client_configs,
+    const std::unordered_map<std::string, std::string>& server_overrides) {
+  auto merged = server_defaults;
+  for (const auto& kv : client_configs) {
+    merged.insert_or_assign(kv.first, kv.second);
+  }
+  for (const auto& kv : server_overrides) {
+    merged.insert_or_assign(kv.first, kv.second);
+  }
+  return merged;
+}
+
 }  // namespace iceberg::rest

src/iceberg/test/rest_catalog_test.cc

@@ -44,25 +44,17 @@ class RestCatalogTest : public ::testing::Test {
     std::string uri = uri_env ? uri_env : "http://localhost:8181";
     std::string warehouse = warehouse_env ? warehouse_env : "default";
 
-    config_
-        .Set(RestCatalogConfig::Entry<std::string>{std::string(RestCatalogConfig::kUri),
-                                                   ""},
-             uri)
-        .Set(RestCatalogConfig::Entry<std::string>{std::string(RestCatalogConfig::kName),
-                                                   ""},
-             std::string("test_catalog"))
-        .Set(
-            RestCatalogConfig::Entry<std::string>{
-                std::string(RestCatalogConfig::kWarehouse), ""},
-            warehouse);
+    config_.Set(RestCatalogConfig::kUri, uri)
+        .Set(RestCatalogConfig::kName, std::string("test_catalog"))
+        .Set(RestCatalogConfig::kWarehouse, warehouse);
   }
 
   void TearDown() override {}
 
   RestCatalogConfig config_;
 };
 
-TEST_F(RestCatalogTest, DISABLED_MakeCatalogSuccess) {
+TEST_F(RestCatalogTest, MakeCatalogSuccess) {
   auto catalog_result = RestCatalog::Make(config_);
   EXPECT_THAT(catalog_result, IsOk());
 
@@ -74,16 +66,14 @@ TEST_F(RestCatalogTest, DISABLED_MakeCatalogSuccess) {
 
 TEST_F(RestCatalogTest, DISABLED_MakeCatalogEmptyUri) {
   RestCatalogConfig invalid_config = config_;
-  invalid_config.Set(
-      RestCatalogConfig::Entry<std::string>{std::string(RestCatalogConfig::kUri), ""},
-      std::string(""));
+  invalid_config.Set(RestCatalogConfig::kUri, std::string(""));
 
   auto catalog_result = RestCatalog::Make(invalid_config);
   EXPECT_THAT(catalog_result, IsError(ErrorKind::kInvalidArgument));
   EXPECT_THAT(catalog_result, HasErrorMessage("uri"));
 }
 
-TEST_F(RestCatalogTest, DISABLED_MakeCatalogWithCustomProperties) {
+TEST_F(RestCatalogTest, MakeCatalogWithCustomProperties) {
   RestCatalogConfig custom_config = config_;
   custom_config
       .Set(RestCatalogConfig::Entry<std::string>{"custom_prop", ""},
@@ -94,7 +84,7 @@ TEST_F(RestCatalogTest, DISABLED_MakeCatalogWithCustomProperties) {
   EXPECT_THAT(catalog_result, IsOk());
 }
 
-TEST_F(RestCatalogTest, DISABLED_ListNamespaces) {
+TEST_F(RestCatalogTest, ListNamespaces) {
   auto catalog_result = RestCatalog::Make(config_);
   ASSERT_THAT(catalog_result, IsOk());
   auto& catalog = catalog_result.value();