wgtmac commented on code in PR #544:
URL: https://github.com/apache/iceberg-cpp/pull/544#discussion_r2791184840
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -227,22 +260,25 @@ Result<bool> RestCatalog::NamespaceExists(const
Namespace& ns) const {
}
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns));
+
Review Comment:
```suggestion
```
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -214,9 +245,11 @@ Result<std::unordered_map<std::string, std::string>>
RestCatalog::GetNamespacePr
Status RestCatalog::DropNamespace(const Namespace& ns) {
ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::DropNamespace());
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns));
- ICEBERG_ASSIGN_OR_RAISE(const auto response,
- client_->Delete(path, /*params=*/{}, /*headers=*/{},
-
*DropNamespaceErrorHandler::Instance()));
+
Review Comment:
```suggestion
```
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -26,6 +26,9 @@
#include <nlohmann/json.hpp>
+#include "iceberg/catalog/rest/auth/auth_manager.h"
+#include "iceberg/catalog/rest/auth/auth_managers.h"
+#include "iceberg/catalog/rest/auth/auth_session.h"
Review Comment:
```suggestion
#include "iceberg/catalog/rest/auth/auth_managers.h"
```
We don't use them in this file.
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -146,11 +147,18 @@ HttpClient::HttpClient(std::unordered_map<std::string,
std::string> default_head
HttpClient::~HttpClient() = default;
+Result<std::unordered_map<std::string, std::string>> HttpClient::AuthHeaders(
+ auth::AuthSession& session) {
+ std::unordered_map<std::string, std::string> headers;
+ ICEBERG_RETURN_UNEXPECTED(session.Authenticate(headers));
+ return headers;
+}
+
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) {
- auto final_headers = MergeHeaders(default_headers_, headers);
+ const ErrorHandler& error_handler, auth::AuthSession& session) {
+ ICEBERG_ASSIGN_OR_RAISE(auto auth_headers, AuthHeaders(session));
+ auto final_headers = MergeHeaders(default_headers_, auth_headers);
Review Comment:
According to
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java#L248-L290,
we should prepare all headers and then authenticate with those prepared
headers. So I'd suggest to remove `AuthHeaders` and rename `MergeHeaders` to
`BuildHeaders` to mimic the Java behavior.
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -251,8 +287,8 @@ Status RestCatalog::UpdateNamespaceProperties(
Result<std::vector<TableIdentifier>> RestCatalog::ListTables(const Namespace&
ns) const {
ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::ListTables());
-
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Tables(ns));
+
Review Comment:
```suggestion
```
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -189,11 +216,13 @@ Status RestCatalog::CreateNamespace(
const Namespace& ns, const std::unordered_map<std::string, std::string>&
properties) {
ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::CreateNamespace());
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespaces());
+
Review Comment:
```suggestion
```
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -203,9 +232,11 @@ Result<std::unordered_map<std::string, std::string>>
RestCatalog::GetNamespacePr
const Namespace& ns) const {
ICEBERG_ENDPOINT_CHECK(supported_endpoints_,
Endpoint::GetNamespaceProperties());
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns));
- ICEBERG_ASSIGN_OR_RAISE(const auto response,
- client_->Get(path, /*params=*/{}, /*headers=*/{},
- *NamespaceErrorHandler::Instance()));
+
Review Comment:
```suggestion
```
##########
src/iceberg/catalog/rest/http_client.h:
##########
@@ -81,33 +81,34 @@ class ICEBERG_REST_EXPORT HttpClient {
/// \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,
Review Comment:
Please revert the changes for `headers`. Same for below.
--
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]
