Jinchul81 commented on code in PR #296:
URL: https://github.com/apache/iceberg-cpp/pull/296#discussion_r2554152578
##########
src/iceberg/catalog/rest/meson.build:
##########
@@ -61,7 +61,7 @@ install_headers(
'json_internal.h',
'types.h',
'util.h',
- 'resource_path.h',
+ 'resource_paths.h',
Review Comment:
Sort the list alphabetically.
##########
src/iceberg/catalog/rest/error_handlers.h:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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 <format>
+#include <string>
+
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/catalog/rest/types.h"
+#include "iceberg/result.h"
+
+/// \file iceberg/catalog/rest/error_handlers.h
+/// Error handlers for different HTTP error types in Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief Error handler interface for processing REST API error responses.
Maps HTTP
+/// status codes to appropriate ErrorKind values following the Iceberg REST
specification.
+class ICEBERG_REST_EXPORT ErrorHandler {
+ public:
+ virtual ~ErrorHandler() = default;
+
+ /// \brief Process an error response and return an appropriate Error.
+ ///
+ /// \param error The error model parsed from the HTTP response body
+ /// \return An Error object with appropriate ErrorKind and message
+ virtual Status Accept(const ErrorModel& error) const = 0;
+};
+
+/// \brief Default error handler for REST API responses.
+class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "IllegalArgumentException") {
+ return InvalidArgument("{}", error.message);
+ }
+ return BadRequest("Malformed request: {}", error.message);
+
+ case 401:
+ return NotAuthorized("Not authorized: {}", error.message);
+
+ case 403:
+ return Forbidden("Forbidden: {}", error.message);
+
+ case 405:
+ case 406:
+ break;
+
+ case 500:
+ return ServiceFailure("Server error: {}: {}", error.type,
error.message);
+
+ case 501:
+ return NotSupported("{}", error.message);
+
+ case 503:
+ return ServiceUnavailable("Service unavailable: {}", error.message);
+ }
+ return RESTError("Unable to process: {}", error.message);
+ }
+};
+
+/// \brief Namespace-specific error handler for create/read/update operations.
+class ICEBERG_REST_EXPORT NamespaceErrorHandler : public DefaultErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "NamespaceNotEmptyException") {
Review Comment:
Add `[[unlikely]]` if the condition unlikely will be satisfied.
##########
src/iceberg/catalog/rest/meson.build:
##########
@@ -57,11 +57,14 @@ install_headers(
'catalog.h',
'config.h',
'constant.h',
- 'http_client_interal.h',
+ 'http_client.h',
+ 'http_response.h',
'json_internal.h',
+ 'iceberg_rest_export.h',
Review Comment:
Sort the list alphabetically.
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -82,7 +82,7 @@ TEST_F(RestCatalogTest, DISABLED_MakeCatalogEmptyUri) {
EXPECT_THAT(catalog_result, HasErrorMessage("uri"));
}
-TEST_F(RestCatalogTest, MakeCatalogWithCustomProperties) {
+TEST_F(RestCatalogTest, DISABLED_MakeCatalogWithCustomProperties) {
Review Comment:
Why did you disable the test? Leave a comment with a reason.
##########
src/iceberg/catalog/rest/constant.h:
##########
@@ -28,17 +28,16 @@
namespace iceberg::rest {
-constexpr std::string_view kHeaderContentType = "Content-Type";
-constexpr std::string_view kHeaderAccept = "Accept";
-constexpr std::string_view kHeaderXClientVersion = "X-Client-Version";
-constexpr std::string_view kHeaderUserAgent = "User-Agent";
+inline const std::string kHeaderContentType = "Content-Type";
Review Comment:
Why did you apply this change? I would prefer `constexpr std::string_view`
instead of `const std::string`.
##########
src/iceberg/catalog/rest/error_handlers.h:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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 <format>
+#include <string>
+
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/catalog/rest/types.h"
+#include "iceberg/result.h"
+
+/// \file iceberg/catalog/rest/error_handlers.h
+/// Error handlers for different HTTP error types in Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief Error handler interface for processing REST API error responses.
Maps HTTP
+/// status codes to appropriate ErrorKind values following the Iceberg REST
specification.
+class ICEBERG_REST_EXPORT ErrorHandler {
+ public:
+ virtual ~ErrorHandler() = default;
+
+ /// \brief Process an error response and return an appropriate Error.
+ ///
+ /// \param error The error model parsed from the HTTP response body
+ /// \return An Error object with appropriate ErrorKind and message
+ virtual Status Accept(const ErrorModel& error) const = 0;
+};
+
+/// \brief Default error handler for REST API responses.
+class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "IllegalArgumentException") {
+ return InvalidArgument("{}", error.message);
+ }
+ return BadRequest("Malformed request: {}", error.message);
+
+ case 401:
+ return NotAuthorized("Not authorized: {}", error.message);
+
+ case 403:
+ return Forbidden("Forbidden: {}", error.message);
+
+ case 405:
+ case 406:
+ break;
+
+ case 500:
+ return ServiceFailure("Server error: {}: {}", error.type,
error.message);
+
+ case 501:
+ return NotSupported("{}", error.message);
+
+ case 503:
+ return ServiceUnavailable("Service unavailable: {}", error.message);
+ }
+ return RESTError("Unable to process: {}", error.message);
+ }
+};
+
+/// \brief Namespace-specific error handler for create/read/update operations.
+class ICEBERG_REST_EXPORT NamespaceErrorHandler : public DefaultErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "NamespaceNotEmptyException") {
+ return NamespaceNotEmpty("{}", error.message);
+ }
+ return BadRequest("Malformed request: {}", error.message);
+
+ case 404:
+ return NoSuchNamespace("{}", error.message);
+
+ case 409:
+ return AlreadyExists("{}", error.message);
+
+ case 422:
+ return RESTError("Unable to process: {}", error.message);
+ }
+
+ return DefaultErrorHandler::Accept(error);
+ }
+};
+
+/// \brief Error handler for drop namespace operations.
+class ICEBERG_REST_EXPORT DropNamespaceErrorHandler : public
NamespaceErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ if (error.code == 409) {
+ return NamespaceNotEmpty("{}", error.message);
+ }
+
+ // Delegate to parent handler
+ return NamespaceErrorHandler::Accept(error);
+ }
+};
+
+/// \brief Table-level error handler.
+class ICEBERG_REST_EXPORT TableErrorHandler : public DefaultErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 404:
+ if (error.type == "NoSuchNamespaceException") {
Review Comment:
Add `[[unlikely]]` if the condition unlikely will be satisfied.
##########
src/iceberg/catalog/rest/endpoint_util.h:
##########
@@ -31,11 +31,11 @@ namespace iceberg::rest {
/// \brief Trim a single trailing slash from a URI string_view.
/// \details If \p uri_sv ends with '/', remove that last character; otherwise
the input
/// is returned unchanged.
-/// \param uri_sv The URI string view to trim.
-/// \return The (possibly) trimmed URI string view.
-inline std::string_view TrimTrailingSlash(std::string_view uri_sv) {
- if (uri_sv.ends_with('/')) {
- uri_sv.remove_suffix(1);
+/// \param uri_sv The URI string to trim.
+/// \return The (possibly) trimmed URI string.
+inline std::string TrimTrailingSlash(std::string uri_sv) {
+ if (!uri_sv.empty() && uri_sv.back() == '/') {
Review Comment:
Why don't we use substr on the string_view? I think find the slash at the
end of the line. I think we don't have to create a new string here.
##########
src/iceberg/catalog/rest/error_handlers.h:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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 <format>
+#include <string>
+
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/catalog/rest/types.h"
+#include "iceberg/result.h"
+
+/// \file iceberg/catalog/rest/error_handlers.h
+/// Error handlers for different HTTP error types in Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief Error handler interface for processing REST API error responses.
Maps HTTP
+/// status codes to appropriate ErrorKind values following the Iceberg REST
specification.
+class ICEBERG_REST_EXPORT ErrorHandler {
+ public:
+ virtual ~ErrorHandler() = default;
+
+ /// \brief Process an error response and return an appropriate Error.
+ ///
+ /// \param error The error model parsed from the HTTP response body
+ /// \return An Error object with appropriate ErrorKind and message
+ virtual Status Accept(const ErrorModel& error) const = 0;
+};
+
+/// \brief Default error handler for REST API responses.
+class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "IllegalArgumentException") {
+ return InvalidArgument("{}", error.message);
+ }
+ return BadRequest("Malformed request: {}", error.message);
+
+ case 401:
+ return NotAuthorized("Not authorized: {}", error.message);
+
+ case 403:
+ return Forbidden("Forbidden: {}", error.message);
+
+ case 405:
+ case 406:
+ break;
+
+ case 500:
+ return ServiceFailure("Server error: {}: {}", error.type,
error.message);
+
+ case 501:
+ return NotSupported("{}", error.message);
+
+ case 503:
+ return ServiceUnavailable("Service unavailable: {}", error.message);
+ }
+ return RESTError("Unable to process: {}", error.message);
+ }
+};
+
+/// \brief Namespace-specific error handler for create/read/update operations.
+class ICEBERG_REST_EXPORT NamespaceErrorHandler : public DefaultErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "NamespaceNotEmptyException") {
+ return NamespaceNotEmpty("{}", error.message);
+ }
+ return BadRequest("Malformed request: {}", error.message);
+
+ case 404:
+ return NoSuchNamespace("{}", error.message);
+
+ case 409:
+ return AlreadyExists("{}", error.message);
+
+ case 422:
+ return RESTError("Unable to process: {}", error.message);
+ }
+
+ return DefaultErrorHandler::Accept(error);
+ }
+};
+
+/// \brief Error handler for drop namespace operations.
+class ICEBERG_REST_EXPORT DropNamespaceErrorHandler : public
NamespaceErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ if (error.code == 409) {
Review Comment:
Add `[[unlikely]]` if the condition unlikely will be satisfied.
##########
src/iceberg/catalog/rest/error_handlers.h:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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 <format>
+#include <string>
+
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/catalog/rest/types.h"
+#include "iceberg/result.h"
+
+/// \file iceberg/catalog/rest/error_handlers.h
+/// Error handlers for different HTTP error types in Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief Error handler interface for processing REST API error responses.
Maps HTTP
+/// status codes to appropriate ErrorKind values following the Iceberg REST
specification.
+class ICEBERG_REST_EXPORT ErrorHandler {
+ public:
+ virtual ~ErrorHandler() = default;
+
+ /// \brief Process an error response and return an appropriate Error.
+ ///
+ /// \param error The error model parsed from the HTTP response body
+ /// \return An Error object with appropriate ErrorKind and message
+ virtual Status Accept(const ErrorModel& error) const = 0;
+};
+
+/// \brief Default error handler for REST API responses.
+class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "IllegalArgumentException") {
Review Comment:
Add `[[unlikely]]` if the condition unlikely will be satisfied.
##########
src/iceberg/catalog/rest/endpoint_util.h:
##########
@@ -31,11 +31,11 @@ namespace iceberg::rest {
/// \brief Trim a single trailing slash from a URI string_view.
/// \details If \p uri_sv ends with '/', remove that last character; otherwise
the input
/// is returned unchanged.
-/// \param uri_sv The URI string view to trim.
-/// \return The (possibly) trimmed URI string view.
-inline std::string_view TrimTrailingSlash(std::string_view uri_sv) {
- if (uri_sv.ends_with('/')) {
- uri_sv.remove_suffix(1);
+/// \param uri_sv The URI string to trim.
+/// \return The (possibly) trimmed URI string.
+inline std::string TrimTrailingSlash(std::string uri_sv) {
Review Comment:
Why did you use `std::string` instead of `std::string_view`?
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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/http_client.h"
+
+#include <nlohmann/json.hpp>
+
+#include "cpr/body.h"
+#include "cpr/cprtypes.h"
+#include "iceberg/catalog/rest/config.h"
+#include "iceberg/catalog/rest/json_internal.h"
+#include "iceberg/json_internal.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::rest {
+
+namespace {
+
+/// \brief Merges global default headers with request-specific headers.
+///
+/// Combines the global headers derived from RestCatalogConfig with the headers
+/// passed in the specific request. Request-specific headers have higher
priority
+/// and will override global defaults if the keys conflict (e.g., overriding
+/// the default "Content-Type").
+cpr::Header MergeHeaders(const std::unordered_map<std::string, std::string>&
defaults,
+ const std::unordered_map<std::string, std::string>&
overrides) {
+ cpr::Header combined_headers = {defaults.begin(), defaults.end()};
+ for (const auto& [key, val] : overrides) {
+ combined_headers.insert_or_assign(key, val);
+ }
+ return combined_headers;
+}
+
+/// \brief Converts a map of string key-value pairs to cpr::Parameters.
+cpr::Parameters GetParameters(
+ const std::unordered_map<std::string, std::string>& params) {
+ cpr::Parameters cpr_params;
+ for (const auto& [key, val] : params) {
+ cpr_params.Add({key, val});
+ }
+ return cpr_params;
+}
+
+bool IsSuccessful(int32_t status_code) {
+ return status_code == 200 // OK
+ || status_code == 202 // Accepted
+ || status_code == 204 // No Content
+ || status_code == 304; // Not Modified
+}
+
+Status HandleFailureResponse(const cpr::Response& response,
+ const ErrorHandler& error_handler) {
+ if (!IsSuccessful(response.status_code)) {
+ ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.text));
+ ICEBERG_ASSIGN_OR_RAISE(auto error_response, ErrorResponseFromJson(json));
+ return error_handler.Accept(error_response.error);
+ }
+ return {};
+}
+
+} // namespace
+
+void HttpClient::PrepareSession(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& request_headers,
+ const std::unordered_map<std::string, std::string>& params) {
+ session_->SetUrl(cpr::Url{path});
+ session_->SetParameters(GetParameters(params));
+ session_->RemoveContent();
+ auto final_headers = MergeHeaders(default_headers_, request_headers);
+ session_->SetHeader(final_headers);
+}
+
+HttpClient::HttpClient(const RestCatalogConfig& config)
+ : default_headers_{config.GetExtraHeaders()},
+ session_{std::make_unique<cpr::Session>()} {}
+
+Result<HttpResponse> HttpClient::Get(
+ const std::string& path, const std::unordered_map<std::string,
std::string>& params,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers, params);
+ cpr::Response response = session_->Get();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::Post(
+ const std::string& path, const std::string& body,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
Review Comment:
Do we have a minimum critical section?
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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/http_client.h"
+
+#include <nlohmann/json.hpp>
+
+#include "cpr/body.h"
+#include "cpr/cprtypes.h"
+#include "iceberg/catalog/rest/config.h"
+#include "iceberg/catalog/rest/json_internal.h"
+#include "iceberg/json_internal.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::rest {
+
+namespace {
+
+/// \brief Merges global default headers with request-specific headers.
+///
+/// Combines the global headers derived from RestCatalogConfig with the headers
+/// passed in the specific request. Request-specific headers have higher
priority
+/// and will override global defaults if the keys conflict (e.g., overriding
+/// the default "Content-Type").
+cpr::Header MergeHeaders(const std::unordered_map<std::string, std::string>&
defaults,
+ const std::unordered_map<std::string, std::string>&
overrides) {
+ cpr::Header combined_headers = {defaults.begin(), defaults.end()};
+ for (const auto& [key, val] : overrides) {
+ combined_headers.insert_or_assign(key, val);
+ }
+ return combined_headers;
+}
+
+/// \brief Converts a map of string key-value pairs to cpr::Parameters.
+cpr::Parameters GetParameters(
+ const std::unordered_map<std::string, std::string>& params) {
+ cpr::Parameters cpr_params;
+ for (const auto& [key, val] : params) {
+ cpr_params.Add({key, val});
+ }
+ return cpr_params;
+}
+
+bool IsSuccessful(int32_t status_code) {
+ return status_code == 200 // OK
+ || status_code == 202 // Accepted
+ || status_code == 204 // No Content
+ || status_code == 304; // Not Modified
+}
+
+Status HandleFailureResponse(const cpr::Response& response,
+ const ErrorHandler& error_handler) {
+ if (!IsSuccessful(response.status_code)) {
+ ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.text));
+ ICEBERG_ASSIGN_OR_RAISE(auto error_response, ErrorResponseFromJson(json));
+ return error_handler.Accept(error_response.error);
+ }
+ return {};
+}
+
+} // namespace
+
+void HttpClient::PrepareSession(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& request_headers,
+ const std::unordered_map<std::string, std::string>& params) {
+ session_->SetUrl(cpr::Url{path});
+ session_->SetParameters(GetParameters(params));
+ session_->RemoveContent();
+ auto final_headers = MergeHeaders(default_headers_, request_headers);
+ session_->SetHeader(final_headers);
+}
+
+HttpClient::HttpClient(const RestCatalogConfig& config)
+ : default_headers_{config.GetExtraHeaders()},
+ session_{std::make_unique<cpr::Session>()} {}
+
+Result<HttpResponse> HttpClient::Get(
+ const std::string& path, const std::unordered_map<std::string,
std::string>& params,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers, params);
+ cpr::Response response = session_->Get();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::Post(
+ const std::string& path, const std::string& body,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers);
+ session_->SetBody(cpr::Body{body});
+ cpr::Response response = session_->Post();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::PostForm(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& form_data,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
Review Comment:
Do we have a minimum critical section?
##########
src/iceberg/catalog/rest/error_handlers.h:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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 <format>
+#include <string>
+
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/catalog/rest/types.h"
+#include "iceberg/result.h"
+
+/// \file iceberg/catalog/rest/error_handlers.h
+/// Error handlers for different HTTP error types in Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief Error handler interface for processing REST API error responses.
Maps HTTP
+/// status codes to appropriate ErrorKind values following the Iceberg REST
specification.
+class ICEBERG_REST_EXPORT ErrorHandler {
+ public:
+ virtual ~ErrorHandler() = default;
+
+ /// \brief Process an error response and return an appropriate Error.
+ ///
+ /// \param error The error model parsed from the HTTP response body
+ /// \return An Error object with appropriate ErrorKind and message
+ virtual Status Accept(const ErrorModel& error) const = 0;
+};
+
+/// \brief Default error handler for REST API responses.
+class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "IllegalArgumentException") {
+ return InvalidArgument("{}", error.message);
+ }
+ return BadRequest("Malformed request: {}", error.message);
+
+ case 401:
+ return NotAuthorized("Not authorized: {}", error.message);
+
+ case 403:
+ return Forbidden("Forbidden: {}", error.message);
+
+ case 405:
+ case 406:
+ break;
+
+ case 500:
+ return ServiceFailure("Server error: {}: {}", error.type,
error.message);
+
+ case 501:
+ return NotSupported("{}", error.message);
+
+ case 503:
+ return ServiceUnavailable("Service unavailable: {}", error.message);
+ }
+ return RESTError("Unable to process: {}", error.message);
+ }
+};
+
+/// \brief Namespace-specific error handler for create/read/update operations.
+class ICEBERG_REST_EXPORT NamespaceErrorHandler : public DefaultErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 400:
+ if (error.type == "NamespaceNotEmptyException") {
+ return NamespaceNotEmpty("{}", error.message);
+ }
+ return BadRequest("Malformed request: {}", error.message);
+
+ case 404:
+ return NoSuchNamespace("{}", error.message);
+
+ case 409:
+ return AlreadyExists("{}", error.message);
+
+ case 422:
+ return RESTError("Unable to process: {}", error.message);
+ }
+
+ return DefaultErrorHandler::Accept(error);
+ }
+};
+
+/// \brief Error handler for drop namespace operations.
+class ICEBERG_REST_EXPORT DropNamespaceErrorHandler : public
NamespaceErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ if (error.code == 409) {
+ return NamespaceNotEmpty("{}", error.message);
+ }
+
+ // Delegate to parent handler
+ return NamespaceErrorHandler::Accept(error);
+ }
+};
+
+/// \brief Table-level error handler.
+class ICEBERG_REST_EXPORT TableErrorHandler : public DefaultErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 404:
+ if (error.type == "NoSuchNamespaceException") {
+ return NoSuchNamespace("{}", error.message);
+ }
+ return NoSuchTable("{}", error.message);
+
+ case 409:
+ return AlreadyExists("{}", error.message);
+ }
+
+ return DefaultErrorHandler::Accept(error);
+ }
+};
+
+/// \brief View-level error handler.
+///
+/// Handles view-specific errors including NoSuchView, NoSuchNamespace,
+/// and AlreadyExists scenarios.
+class ICEBERG_REST_EXPORT ViewErrorHandler : public DefaultErrorHandler {
+ public:
+ Status Accept(const ErrorModel& error) const override {
+ switch (error.code) {
+ case 404:
+ if (error.type == "NoSuchNamespaceException") {
Review Comment:
Add `[[unlikely]]` if the condition unlikely will be satisfied.
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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/http_client.h"
+
+#include <nlohmann/json.hpp>
+
+#include "cpr/body.h"
+#include "cpr/cprtypes.h"
+#include "iceberg/catalog/rest/config.h"
+#include "iceberg/catalog/rest/json_internal.h"
+#include "iceberg/json_internal.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::rest {
+
+namespace {
+
+/// \brief Merges global default headers with request-specific headers.
+///
+/// Combines the global headers derived from RestCatalogConfig with the headers
+/// passed in the specific request. Request-specific headers have higher
priority
+/// and will override global defaults if the keys conflict (e.g., overriding
+/// the default "Content-Type").
+cpr::Header MergeHeaders(const std::unordered_map<std::string, std::string>&
defaults,
+ const std::unordered_map<std::string, std::string>&
overrides) {
+ cpr::Header combined_headers = {defaults.begin(), defaults.end()};
+ for (const auto& [key, val] : overrides) {
+ combined_headers.insert_or_assign(key, val);
+ }
+ return combined_headers;
+}
+
+/// \brief Converts a map of string key-value pairs to cpr::Parameters.
+cpr::Parameters GetParameters(
+ const std::unordered_map<std::string, std::string>& params) {
+ cpr::Parameters cpr_params;
+ for (const auto& [key, val] : params) {
+ cpr_params.Add({key, val});
+ }
+ return cpr_params;
+}
+
+bool IsSuccessful(int32_t status_code) {
+ return status_code == 200 // OK
+ || status_code == 202 // Accepted
+ || status_code == 204 // No Content
+ || status_code == 304; // Not Modified
+}
+
+Status HandleFailureResponse(const cpr::Response& response,
+ const ErrorHandler& error_handler) {
+ if (!IsSuccessful(response.status_code)) {
+ ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.text));
+ ICEBERG_ASSIGN_OR_RAISE(auto error_response, ErrorResponseFromJson(json));
+ return error_handler.Accept(error_response.error);
+ }
+ return {};
+}
+
+} // namespace
+
+void HttpClient::PrepareSession(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& request_headers,
+ const std::unordered_map<std::string, std::string>& params) {
+ session_->SetUrl(cpr::Url{path});
+ session_->SetParameters(GetParameters(params));
+ session_->RemoveContent();
+ auto final_headers = MergeHeaders(default_headers_, request_headers);
+ session_->SetHeader(final_headers);
+}
+
+HttpClient::HttpClient(const RestCatalogConfig& config)
+ : default_headers_{config.GetExtraHeaders()},
+ session_{std::make_unique<cpr::Session>()} {}
+
+Result<HttpResponse> HttpClient::Get(
+ const std::string& path, const std::unordered_map<std::string,
std::string>& params,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
Review Comment:
Do we have a minimum critical section?
##########
src/iceberg/catalog/rest/http_response.h:
##########
@@ -34,13 +35,16 @@ class ICEBERG_REST_EXPORT HttpResponse {
explicit HttpResponse(cpr::Response response) :
response_(std::move(response)) {}
/// \brief Get the HTTP status code of the response.
- /// \return The HTTP status code.
Review Comment:
Why did you remove the description for the return?
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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/http_client.h"
+
+#include <nlohmann/json.hpp>
+
+#include "cpr/body.h"
+#include "cpr/cprtypes.h"
+#include "iceberg/catalog/rest/config.h"
+#include "iceberg/catalog/rest/json_internal.h"
+#include "iceberg/json_internal.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::rest {
+
+namespace {
+
+/// \brief Merges global default headers with request-specific headers.
+///
+/// Combines the global headers derived from RestCatalogConfig with the headers
+/// passed in the specific request. Request-specific headers have higher
priority
+/// and will override global defaults if the keys conflict (e.g., overriding
+/// the default "Content-Type").
+cpr::Header MergeHeaders(const std::unordered_map<std::string, std::string>&
defaults,
+ const std::unordered_map<std::string, std::string>&
overrides) {
+ cpr::Header combined_headers = {defaults.begin(), defaults.end()};
+ for (const auto& [key, val] : overrides) {
+ combined_headers.insert_or_assign(key, val);
+ }
+ return combined_headers;
+}
+
+/// \brief Converts a map of string key-value pairs to cpr::Parameters.
+cpr::Parameters GetParameters(
+ const std::unordered_map<std::string, std::string>& params) {
+ cpr::Parameters cpr_params;
+ for (const auto& [key, val] : params) {
+ cpr_params.Add({key, val});
+ }
+ return cpr_params;
+}
+
+bool IsSuccessful(int32_t status_code) {
+ return status_code == 200 // OK
+ || status_code == 202 // Accepted
+ || status_code == 204 // No Content
+ || status_code == 304; // Not Modified
+}
+
+Status HandleFailureResponse(const cpr::Response& response,
+ const ErrorHandler& error_handler) {
+ if (!IsSuccessful(response.status_code)) {
+ ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.text));
+ ICEBERG_ASSIGN_OR_RAISE(auto error_response, ErrorResponseFromJson(json));
+ return error_handler.Accept(error_response.error);
+ }
+ return {};
+}
+
+} // namespace
+
+void HttpClient::PrepareSession(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& request_headers,
+ const std::unordered_map<std::string, std::string>& params) {
+ session_->SetUrl(cpr::Url{path});
+ session_->SetParameters(GetParameters(params));
+ session_->RemoveContent();
+ auto final_headers = MergeHeaders(default_headers_, request_headers);
+ session_->SetHeader(final_headers);
+}
+
+HttpClient::HttpClient(const RestCatalogConfig& config)
+ : default_headers_{config.GetExtraHeaders()},
+ session_{std::make_unique<cpr::Session>()} {}
+
+Result<HttpResponse> HttpClient::Get(
+ const std::string& path, const std::unordered_map<std::string,
std::string>& params,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers, params);
+ cpr::Response response = session_->Get();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::Post(
+ const std::string& path, const std::string& body,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers);
+ session_->SetBody(cpr::Body{body});
+ cpr::Response response = session_->Post();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::PostForm(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& form_data,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers);
+ std::vector<cpr::Pair> pair_list;
+ pair_list.reserve(form_data.size());
+ for (const auto& [key, val] : form_data) {
+ pair_list.emplace_back(key, val);
+ }
+ session_->SetPayload(cpr::Payload(pair_list.begin(), pair_list.end()));
+ cpr::Response response = session_->Post();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::Head(
+ const std::string& path, const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
Review Comment:
Do we have a minimum critical section?
##########
src/iceberg/catalog/rest/resource_paths.h:
##########
@@ -125,8 +107,8 @@ class ICEBERG_REST_EXPORT ResourcePaths {
// Helper to build path with optional prefix: {base_uri_}/{prefix_?}/{path}
std::string BuildPath(std::string_view path) const;
- std::string base_uri_; // URI with /v1, e.g., "http://localhost:8181/v1";
- std::string prefix_; // Optional prefix from config
+ std::string base_uri_;
Review Comment:
Add a description for this member.
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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/http_client.h"
+
+#include <nlohmann/json.hpp>
+
+#include "cpr/body.h"
+#include "cpr/cprtypes.h"
+#include "iceberg/catalog/rest/config.h"
+#include "iceberg/catalog/rest/json_internal.h"
+#include "iceberg/json_internal.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::rest {
+
+namespace {
+
+/// \brief Merges global default headers with request-specific headers.
+///
+/// Combines the global headers derived from RestCatalogConfig with the headers
+/// passed in the specific request. Request-specific headers have higher
priority
+/// and will override global defaults if the keys conflict (e.g., overriding
+/// the default "Content-Type").
+cpr::Header MergeHeaders(const std::unordered_map<std::string, std::string>&
defaults,
+ const std::unordered_map<std::string, std::string>&
overrides) {
+ cpr::Header combined_headers = {defaults.begin(), defaults.end()};
+ for (const auto& [key, val] : overrides) {
+ combined_headers.insert_or_assign(key, val);
+ }
+ return combined_headers;
+}
+
+/// \brief Converts a map of string key-value pairs to cpr::Parameters.
+cpr::Parameters GetParameters(
+ const std::unordered_map<std::string, std::string>& params) {
+ cpr::Parameters cpr_params;
+ for (const auto& [key, val] : params) {
+ cpr_params.Add({key, val});
+ }
+ return cpr_params;
+}
+
+bool IsSuccessful(int32_t status_code) {
+ return status_code == 200 // OK
+ || status_code == 202 // Accepted
+ || status_code == 204 // No Content
+ || status_code == 304; // Not Modified
+}
+
+Status HandleFailureResponse(const cpr::Response& response,
+ const ErrorHandler& error_handler) {
+ if (!IsSuccessful(response.status_code)) {
Review Comment:
Add `[[unlikely]]` if the condition unlikely will be satisfied.
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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/http_client.h"
+
+#include <nlohmann/json.hpp>
+
+#include "cpr/body.h"
+#include "cpr/cprtypes.h"
+#include "iceberg/catalog/rest/config.h"
+#include "iceberg/catalog/rest/json_internal.h"
+#include "iceberg/json_internal.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::rest {
+
+namespace {
+
+/// \brief Merges global default headers with request-specific headers.
+///
+/// Combines the global headers derived from RestCatalogConfig with the headers
+/// passed in the specific request. Request-specific headers have higher
priority
+/// and will override global defaults if the keys conflict (e.g., overriding
+/// the default "Content-Type").
+cpr::Header MergeHeaders(const std::unordered_map<std::string, std::string>&
defaults,
+ const std::unordered_map<std::string, std::string>&
overrides) {
+ cpr::Header combined_headers = {defaults.begin(), defaults.end()};
+ for (const auto& [key, val] : overrides) {
+ combined_headers.insert_or_assign(key, val);
+ }
+ return combined_headers;
+}
+
+/// \brief Converts a map of string key-value pairs to cpr::Parameters.
+cpr::Parameters GetParameters(
+ const std::unordered_map<std::string, std::string>& params) {
+ cpr::Parameters cpr_params;
+ for (const auto& [key, val] : params) {
+ cpr_params.Add({key, val});
+ }
+ return cpr_params;
+}
+
+bool IsSuccessful(int32_t status_code) {
+ return status_code == 200 // OK
+ || status_code == 202 // Accepted
+ || status_code == 204 // No Content
+ || status_code == 304; // Not Modified
+}
+
+Status HandleFailureResponse(const cpr::Response& response,
+ const ErrorHandler& error_handler) {
+ if (!IsSuccessful(response.status_code)) {
+ ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.text));
+ ICEBERG_ASSIGN_OR_RAISE(auto error_response, ErrorResponseFromJson(json));
+ return error_handler.Accept(error_response.error);
+ }
+ return {};
+}
+
+} // namespace
+
+void HttpClient::PrepareSession(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& request_headers,
+ const std::unordered_map<std::string, std::string>& params) {
+ session_->SetUrl(cpr::Url{path});
+ session_->SetParameters(GetParameters(params));
+ session_->RemoveContent();
+ auto final_headers = MergeHeaders(default_headers_, request_headers);
+ session_->SetHeader(final_headers);
+}
+
+HttpClient::HttpClient(const RestCatalogConfig& config)
+ : default_headers_{config.GetExtraHeaders()},
+ session_{std::make_unique<cpr::Session>()} {}
+
+Result<HttpResponse> HttpClient::Get(
+ const std::string& path, const std::unordered_map<std::string,
std::string>& params,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers, params);
+ cpr::Response response = session_->Get();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::Post(
+ const std::string& path, const std::string& body,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers);
+ session_->SetBody(cpr::Body{body});
+ cpr::Response response = session_->Post();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::PostForm(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& form_data,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers);
+ std::vector<cpr::Pair> pair_list;
+ pair_list.reserve(form_data.size());
+ for (const auto& [key, val] : form_data) {
+ pair_list.emplace_back(key, val);
+ }
+ session_->SetPayload(cpr::Payload(pair_list.begin(), pair_list.end()));
+ cpr::Response response = session_->Post();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::Head(
+ const std::string& path, const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
+
+ PrepareSession(path, headers);
+ cpr::Response response = session_->Head();
+ ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
+ return HttpResponse(std::move(response));
+}
+
+Result<HttpResponse> HttpClient::Delete(
+ const std::string& path, const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler) {
+ std::lock_guard<std::mutex> lock(session_mutex_);
Review Comment:
Do we have a minimum critical section?
##########
src/iceberg/catalog/rest/http_client.h:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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 <memory>
+#include <mutex>
+#include <string>
+#include <unordered_map>
+
+#include <cpr/cpr.h>
+#include <nlohmann/json.hpp>
+
+#include "iceberg/catalog/rest/config.h"
+#include "iceberg/catalog/rest/error_handlers.h"
+#include "iceberg/catalog/rest/http_response.h"
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/result.h"
+
+/// \file iceberg/catalog/rest/http_client.h
+/// \brief Http client for Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief HTTP client for making requests to Iceberg REST Catalog API.
+class ICEBERG_REST_EXPORT HttpClient {
+ public:
+ explicit HttpClient(const RestCatalogConfig&);
+
+ HttpClient(const HttpClient&) = delete;
+ HttpClient& operator=(const HttpClient&) = delete;
+ HttpClient(HttpClient&&) = delete;
+ HttpClient& operator=(HttpClient&&) = delete;
+
+ /// \brief Sends a GET request.
+ Result<HttpResponse> Get(const std::string& path,
+ const std::unordered_map<std::string, std::string>&
params,
+ const std::unordered_map<std::string, std::string>&
headers,
+ const ErrorHandler& error_handler);
+
+ /// \brief Sends a POST request.
+ Result<HttpResponse> Post(const std::string& path, const std::string& body,
+ const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler);
+
+ /// \brief Sends a POST request with form data.
+ Result<HttpResponse> PostForm(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& form_data,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler);
+
+ /// \brief Sends a HEAD request.
+ Result<HttpResponse> Head(const std::string& path,
+ const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler);
+
+ /// \brief Sends a DELETE request.
+ Result<HttpResponse> Delete(const std::string& path,
+ const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler);
+
+ private:
+ void PrepareSession(const std::string& path,
+ const std::unordered_map<std::string, std::string>&
request_headers,
+ const std::unordered_map<std::string, std::string>&
params = {});
+
+ std::unordered_map<std::string, std::string> default_headers_;
+
+ // Mutex to protect the non-thread-safe cpr::Session.
+ mutable std::mutex session_mutex_;
+
+ // TODO(Li Feiyang): use connection pool to support external multi-threaded
concurrent
Review Comment:
Please file an issue and leave a tracking id instead of user id. It's much
easier to grasp the context.
##########
src/iceberg/result.h:
##########
@@ -42,13 +44,19 @@ enum class ErrorKind {
kInvalidManifestList,
kIOError,
kJsonParseError,
+ kNamespaceNotEmpty,
kNoSuchNamespace,
kNoSuchTable,
+ kNoSuchView,
kNotAllowed,
+ kNotAuthorized,
kNotFound,
kNotImplemented,
kNotSupported,
+ kServiceFailure,
+ kServiceUnavailable,
kUnknownError,
+ kRESTError,
Review Comment:
Sort the list alphabetically.
##########
src/iceberg/catalog/rest/http_client.h:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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 <memory>
+#include <mutex>
+#include <string>
+#include <unordered_map>
+
+#include <cpr/cpr.h>
+#include <nlohmann/json.hpp>
+
+#include "iceberg/catalog/rest/config.h"
+#include "iceberg/catalog/rest/error_handlers.h"
+#include "iceberg/catalog/rest/http_response.h"
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/result.h"
+
+/// \file iceberg/catalog/rest/http_client.h
+/// \brief Http client for Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief HTTP client for making requests to Iceberg REST Catalog API.
+class ICEBERG_REST_EXPORT HttpClient {
+ public:
+ explicit HttpClient(const RestCatalogConfig&);
+
+ HttpClient(const HttpClient&) = delete;
+ HttpClient& operator=(const HttpClient&) = delete;
+ HttpClient(HttpClient&&) = delete;
+ HttpClient& operator=(HttpClient&&) = delete;
+
+ /// \brief Sends a GET request.
+ Result<HttpResponse> Get(const std::string& path,
+ const std::unordered_map<std::string, std::string>&
params,
+ const std::unordered_map<std::string, std::string>&
headers,
+ const ErrorHandler& error_handler);
+
+ /// \brief Sends a POST request.
+ Result<HttpResponse> Post(const std::string& path, const std::string& body,
+ const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler);
+
+ /// \brief Sends a POST request with form data.
+ Result<HttpResponse> PostForm(
+ const std::string& path,
+ const std::unordered_map<std::string, std::string>& form_data,
+ const std::unordered_map<std::string, std::string>& headers,
+ const ErrorHandler& error_handler);
+
+ /// \brief Sends a HEAD request.
+ Result<HttpResponse> Head(const std::string& path,
+ const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler);
+
+ /// \brief Sends a DELETE request.
+ Result<HttpResponse> Delete(const std::string& path,
+ const std::unordered_map<std::string,
std::string>& headers,
+ const ErrorHandler& error_handler);
+
+ private:
+ void PrepareSession(const std::string& path,
+ const std::unordered_map<std::string, std::string>&
request_headers,
+ const std::unordered_map<std::string, std::string>&
params = {});
+
+ std::unordered_map<std::string, std::string> default_headers_;
Review Comment:
Add a description of this member. What are the key and the value?
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -61,7 +61,7 @@ class RestCatalogTest : public ::testing::Test {
RestCatalogConfig config_;
};
-TEST_F(RestCatalogTest, MakeCatalogSuccess) {
+TEST_F(RestCatalogTest, DISABLED_MakeCatalogSuccess) {
Review Comment:
Why did you disable the test? Leave a comment with a reason.
##########
src/iceberg/result.h:
##########
@@ -91,13 +101,19 @@ DEFINE_ERROR_FUNCTION(InvalidManifest)
DEFINE_ERROR_FUNCTION(InvalidManifestList)
DEFINE_ERROR_FUNCTION(IOError)
DEFINE_ERROR_FUNCTION(JsonParseError)
+DEFINE_ERROR_FUNCTION(NamespaceNotEmpty)
DEFINE_ERROR_FUNCTION(NoSuchNamespace)
DEFINE_ERROR_FUNCTION(NoSuchTable)
+DEFINE_ERROR_FUNCTION(NoSuchView)
DEFINE_ERROR_FUNCTION(NotAllowed)
+DEFINE_ERROR_FUNCTION(NotAuthorized)
DEFINE_ERROR_FUNCTION(NotFound)
DEFINE_ERROR_FUNCTION(NotImplemented)
DEFINE_ERROR_FUNCTION(NotSupported)
+DEFINE_ERROR_FUNCTION(ServiceFailure)
+DEFINE_ERROR_FUNCTION(ServiceUnavailable)
DEFINE_ERROR_FUNCTION(UnknownError)
+DEFINE_ERROR_FUNCTION(RESTError)
Review Comment:
Sort the list alphabetically.
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -93,7 +93,7 @@ TEST_F(RestCatalogTest, MakeCatalogWithCustomProperties) {
EXPECT_THAT(catalog_result, IsOk());
}
-TEST_F(RestCatalogTest, ListNamespaces) {
+TEST_F(RestCatalogTest, DISABLED_ListNamespaces) {
Review Comment:
Why did you disable the test? Leave a comment with a reason.
--
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]
