wgtmac commented on code in PR #544:
URL: https://github.com/apache/iceberg-cpp/pull/544#discussion_r2786111403
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -48,9 +61,39 @@ std::string InferAuthType(
return AuthProperties::kAuthTypeNone;
}
+/// \brief Authentication manager that performs no authentication.
+class NoopAuthManager : public AuthManager {
+ public:
+ static Result<std::unique_ptr<AuthManager>> Make(
+ [[maybe_unused]] std::string_view name,
+ [[maybe_unused]] const std::unordered_map<std::string, std::string>&
properties) {
+ return std::make_unique<NoopAuthManager>();
+ }
+
+ Result<std::shared_ptr<AuthSession>> CatalogSession(
+ [[maybe_unused]] HttpClient& client,
+ [[maybe_unused]] const std::unordered_map<std::string, std::string>&
properties)
+ override {
+ return AuthSession::MakeDefault({});
+ }
+};
+
+template <typename T>
+AuthManagerFactory MakeAuthFactory() {
+ return []([[maybe_unused]] std::string_view name,
+ [[maybe_unused]] const std::unordered_map<std::string,
std::string>& props)
Review Comment:
```suggestion
return [](std::string_view name,
const std::unordered_map<std::string, std::string>& props)
```
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -139,27 +153,39 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
paths, ResourcePaths::Make(std::string(TrimTrailingSlash(final_uri)),
final_config->Get(RestCatalogProperties::kPrefix)));
- return std::shared_ptr<RestCatalog>(
- new RestCatalog(std::move(final_config), std::move(file_io),
std::move(paths),
- std::move(endpoints)));
+ auto client = std::make_unique<HttpClient>(final_config->ExtractHeaders());
+ ICEBERG_ASSIGN_OR_RAISE(auto catalog_session,
+ auth_manager->CatalogSession(*client,
final_config->configs()));
+
+ return std::shared_ptr<RestCatalog>(new RestCatalog(
+ std::move(final_config), std::move(file_io), std::move(client),
std::move(paths),
+ std::move(endpoints), std::move(auth_manager),
std::move(catalog_session)));
}
RestCatalog::RestCatalog(std::unique_ptr<RestCatalogProperties> config,
std::shared_ptr<FileIO> file_io,
+ std::unique_ptr<HttpClient> client,
std::unique_ptr<ResourcePaths> paths,
- std::unordered_set<Endpoint> endpoints)
+ std::unordered_set<Endpoint> endpoints,
+ std::unique_ptr<auth::AuthManager> auth_manager,
+ std::shared_ptr<auth::AuthSession> catalog_session)
: config_(std::move(config)),
file_io_(std::move(file_io)),
- client_(std::make_unique<HttpClient>(config_->ExtractHeaders())),
+ client_(std::move(client)),
paths_(std::move(paths)),
name_(config_->Get(RestCatalogProperties::kName)),
- supported_endpoints_(std::move(endpoints)) {}
+ supported_endpoints_(std::move(endpoints)),
+ auth_manager_(std::move(auth_manager)),
+ catalog_session_(std::move(catalog_session)) {
+ client_->SetAuthSession(catalog_session_.get());
+}
std::string_view RestCatalog::name() const { return name_; }
Result<std::vector<Namespace>> RestCatalog::ListNamespaces(const Namespace&
ns) const {
ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::ListNamespaces());
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespaces());
+
Review Comment:
Let's revert unrelated change
##########
src/iceberg/catalog/rest/http_client.h:
##########
@@ -78,38 +78,39 @@ class ICEBERG_REST_EXPORT HttpClient {
HttpClient(HttpClient&&) = delete;
HttpClient& operator=(HttpClient&&) = delete;
+ /// \brief Set the authentication session
+ void SetAuthSession(auth::AuthSession* session);
Review Comment:
I don't think this is the right approach to introduce a stateful http
client. Let's remove this function and extend below functions to accept a
session like:
```cpp
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,
AuthSession& session);
```
The input `session` is a reference so it is required and a no-op session
should be used if authentication is not enabled.
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -172,7 +198,7 @@ Result<std::vector<Namespace>>
RestCatalog::ListNamespaces(const Namespace& ns)
}
ICEBERG_ASSIGN_OR_RAISE(
const auto response,
- client_->Get(path, params, /*headers=*/{},
*NamespaceErrorHandler::Instance()));
+ client_->Get(path, params, *NamespaceErrorHandler::Instance()));
Review Comment:
We need to use contextual session as the Java impl does.
--
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]
