lishuxu commented on code in PR #616:
URL: https://github.com/apache/iceberg-cpp/pull/616#discussion_r3080075213
##########
src/iceberg/catalog/rest/auth/auth_session.h:
##########
@@ -50,7 +58,13 @@ class ICEBERG_REST_EXPORT AuthSession {
/// - NotAuthorized: Not authenticated (401)
/// - IOError: Network or connection errors when reaching auth server
/// - RestError: HTTP errors from authentication service
- virtual Status Authenticate(std::unordered_map<std::string, std::string>&
headers) = 0;
+ virtual Status Authenticate(std::unordered_map<std::string, std::string>&
headers,
Review Comment:
The current design splits the request context into two separate parameters
(headers as in-out + HTTPRequestContext as a separate struct).
The Java implementation uses a cleaner "request-in, request-out" pattern
where authenticate() receives the full HTTPRequest and returns a new immutable
request with auth headers, I'd suggest aligning with Java by introducing an
HTTPRequest type that encapsulates method, url, headers, and body together, and
changing the signature to:
virtual Result<HTTPRequest> Authenticate(const HTTPRequest& request) = 0;
I'm open for this
--
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]
