[GitHub] [arrow] lidavidm commented on a diff in pull request #35378: GH-35377: [C++][FlightRPC] Add a `ServerCallContext` parameter to `arrow::flight::ServerAuthHandler` methods

["lidavidm (via GitHub)" <gi...@apache.org>]

lidavidm commented on code in PR #35378:
URL: https://github.com/apache/arrow/pull/35378#discussion_r1181451732


##########
cpp/src/arrow/flight/server_auth.h:
##########
@@ -55,8 +57,54 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
   virtual ~ServerAuthHandler();
   /// \brief Authenticate the client on initial connection. The server
   /// can send and read responses from the client at any time.
+  ///
+  /// If this version is implemented, the deprecated Authentication()
+  /// without ServerCallContext version isn't used. So we can
+  /// implement the deprecated version like the following:
+  ///
+  ///   Status Authenticate(ServerAuthSender* outgoing,
+  ///                       ServerAuthReader* incoming) override {
+  ///     return Status::NotImplemented("This version is never used");
+  ///   }
+  ///
+  /// \param[in] context The call context.
+  /// \param[in] outgoing The writer for messages to the client.
+  /// \param[in] incoming The reader for messages from the client.
+  /// \return Status OK if this authentication is succeeded.
+  virtual Status Authenticate(const ServerCallContext& context,
+                              ServerAuthSender* outgoing, ServerAuthReader* incoming) {
+    return Authenticate(outgoing, incoming);
+  }
+  /// \brief Authenticate the client on initial connection. The server
+  /// can send and read responses from the client at any time.
+  /// \param[in] outgoing The writer for messages to the client.
+  /// \param[in] incoming The reader for messages from the client.
+  /// \return Status OK if this authentication is succeeded.
+  /// \deprecated Deprecated since 13.0.0. Implement the Authentication()
+  /// with ServerCallContext version instead.
   virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;

Review Comment:
   I suppose we can't mark this as deprecated to the compiler without triggering a bunch of warnings on ourselves, can we...



##########
cpp/src/arrow/flight/server_auth.h:
##########
@@ -55,8 +57,54 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
   virtual ~ServerAuthHandler();
   /// \brief Authenticate the client on initial connection. The server
   /// can send and read responses from the client at any time.
+  ///
+  /// If this version is implemented, the deprecated Authentication()
+  /// without ServerCallContext version isn't used. So we can
+  /// implement the deprecated version like the following:
+  ///
+  ///   Status Authenticate(ServerAuthSender* outgoing,
+  ///                       ServerAuthReader* incoming) override {
+  ///     return Status::NotImplemented("This version is never used");
+  ///   }

Review Comment:
   We could change the deprecated overload from pure virtual to just virtual, and add a default no-op/always-failing implementation. That way people who only implement the new overload won't have to update code when we remove the deprecated overload. What do you think?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org