GH-35377: [C++][FlightRPC] Add a `ServerCallContext` parameter to `arrow::flight::ServerAuthHandler` methods by kou · Pull Request #35378 · apache/arrow

kou · 2023-05-01T07:18:25Z

Rationale for this change

Because they are also RPC calls to like others such as ListFlights() and DoGet().

What changes are included in this PR?

Add with ServerCallContext versions.

Old signature versions still exist for keeping backward compatibility.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

This PR includes breaking changes to public APIs.

C++ API: Don't include any breaking changes.
C GLib API: Includes a breaking change. context argument is added.
Closes: [C++][FlightRPC] Add a ServerCallContext parameter to arrow::flight::ServerAuthHandler methods #35377

github-actions · 2023-05-01T07:18:45Z

Closes: [C++][FlightRPC] Add a ServerCallContext parameter to arrow::flight::ServerAuthHandler methods #35377

github-actions · 2023-05-01T07:18:47Z

⚠️ GitHub issue #35377 has been automatically assigned in GitHub to PR creator.

…to `arrow::flight::ServerAuthHandler` methods Because they are also RPC calls to like other `ListFlights()`, `DoGet()` and so on.

lidavidm

LGTM.

lidavidm · 2023-05-01T08:03:36Z

cpp/src/arrow/flight/server_auth.h

+  /// 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");
+  ///   }


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?

Oh, it's a good idea! I'll do it.

lidavidm · 2023-05-01T08:04:23Z

cpp/src/arrow/flight/server_auth.h

+  /// \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;


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

We can mark this as deprecated but client codes that overrides this don't receive any deprecated warning from their compiler. Because client codes don't call this method. Our server implementations call this method instead.

Anyway, I've added ARROW_DEPRECATED() because a -Wdocumentation-deprecated-sync warning is reported without it: https://github.com/kou/arrow/actions/runs/4849400904/jobs/8641350382#step:9:1437

/Users/runner/work/arrow/arrow/cpp/src/arrow/flight/server_auth.h:83:8: error: declaration is marked with '\deprecated' command but does not have a deprecation attribute [-Werror,-Wdocumentation-deprecated-sync] /// \deprecated Deprecated since 13.0.0. Implement the Authentication() ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/runner/work/arrow/arrow/cpp/src/arrow/flight/server_auth.h:85:3: note: add a deprecation attribute to the declaration to silence this warning virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0; ^

lidavidm · 2023-05-01T11:34:43Z

cpp/src/arrow/flight/server_auth.h

+  /// authentication method supports it.
+  /// \return Status OK if the token is valid, any other status if
+  /// validation failed
+  virtual Status IsValid(const ServerCallContext& context, const std::string& token,


Looks like doxygen complains unless there's a \param for this

Good catch!
I forgot to add it...

ursabot · 2023-05-02T07:22:49Z

Benchmark runs are scheduled for baseline = be12888 and contender = 42527e1. 42527e1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.66% ⬆️0.06%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.93% ⬆️0.12%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 42527e15 ec2-t3-xlarge-us-east-2
[Failed] 42527e15 test-mac-arm
[Failed] 42527e15 ursa-i9-9960x
[Finished] 42527e15 ursa-thinkcentre-m75q
[Finished] be128889 ec2-t3-xlarge-us-east-2
[Failed] be128889 test-mac-arm
[Failed] be128889 ursa-i9-9960x
[Finished] be128889 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot · 2023-05-02T07:24:16Z

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

…to `arrow::flight::ServerAuthHandler` methods (apache#35378) ### Rationale for this change Because they are also RPC calls to like others such as `ListFlights()` and `DoGet()`. ### What changes are included in this PR? Add with `ServerCallContext` versions. Old signature versions still exist for keeping backward compatibility. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * C++ API: Don't include any breaking changes. * C GLib API: Includes a breaking change. `context` argument is added. * Closes: apache#35377 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

This PR removes C++ APIs deprecated in releases before January 1, 2025, following Arrow's deprecation policy (>1 year old deprecations). **Rationale for This Change** Cleans up deprecated APIs per maintainer request in apacheGH-49153. **What Changes Are Included in This PR?** 1. **ServerAuthHandler::IsValid() (old signature)** - **PR**: apache#35378 (merged May 2, 2023) - **Released**: v13.0.0 (August 17, 2023) - **Age**: 2.5 years old - **Removed**: Old IsValid(token, peer_identity) method - **Made pure virtual**: IsValid(context, token, peer_identity) signature - **Replacement**: Implement the new signature with ServerCallContext parameter 2. **decimal() factory function** - **PR**: apache#43957 (merged September 30, 2024) - **Released**: v18.0.0 (October 16, 2024) - **Age**: 1.3 years old - **Removed**: �rrow::decimal(precision, scale) function - **Replacement**: Use smallest_decimal(precision, scale) instead 3. **FlightSqlServerBase::CancelQuery()** - **PR**: apache#36009 (merged July 3, 2023) - **Released**: v13.0.0 (August 17, 2023) - **Age**: 2.5 years old - **Removed**: - CancelQuery() method - ActionCancelQueryRequest struct - CancelResult enum - kCancelQueryActionType action type - **Replacement**: Use CancelFlightInfo() instead 4. **FlightSqlClient::CancelQuery()** - **PR**: apache#36009 (merged July 3, 2023) - **Released**: v13.0.0 (August 17, 2023) - **Age**: 2.5 years old - **Removed**: Client-side CancelQuery() method - **Replacement**: Use CancelFlightInfo() instead **Are These Changes Tested?** Yes. Verified via codebase search that: - All removed symbols have zero remaining internal usages - Derived ServerAuthHandler classes use the new IsValid() signature - Tests already use the replacement APIs **Are There Any User-Facing Changes?** Yes (breaking changes for C++ users): - Code using deprecated APIs must migrate to documented replacements - No behavior changes for code already using replacement APIs Closes apache#49153

kou requested a review from lidavidm as a code owner May 1, 2023 07:18

github-actions bot added Component: C++ Component: FlightRPC awaiting committer review Awaiting committer review labels May 1, 2023

apacheGH-35377: [C++][FlightRPC] Add a ServerCallContext parameter …

b8bfd16

…to `arrow::flight::ServerAuthHandler` methods Because they are also RPC calls to like other `ListFlights()`, `DoGet()` and so on.

kou force-pushed the cpp-flight-server-auth-handler-call-context branch from 48b71e7 to b8bfd16 Compare May 1, 2023 07:22

lidavidm reviewed May 1, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 1, 2023

Use NotImplemented implementation instead of pure virtual function

4f5428b

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels May 1, 2023

lidavidm reviewed May 1, 2023

View reviewed changes

kou added 2 commits May 2, 2023 06:25

Fix document

36d22f2

GLib: follow changes

cdc11ac

github-actions bot added Component: GLib awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 2, 2023

lidavidm approved these changes May 2, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 2, 2023

kou merged commit 42527e1 into apache:main May 2, 2023

kou deleted the cpp-flight-server-auth-handler-call-context branch May 2, 2023 03:12

AliRana30 mentioned this pull request Feb 18, 2026

GH-49356: [C++] Remove deprecated APIs from v13.0.0 and v18.0.0 #49171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

GH-35377: [C++][FlightRPC] Add a `ServerCallContext` parameter to `arrow::flight::ServerAuthHandler` methods#35378

GH-35377: [C++][FlightRPC] Add a `ServerCallContext` parameter to `arrow::flight::ServerAuthHandler` methods#35378
kou merged 4 commits intoapache:mainfrom
kou:cpp-flight-server-auth-handler-call-context

kou commented May 1, 2023 •

edited

Loading

Uh oh!

github-actions bot commented May 1, 2023

Uh oh!

github-actions bot commented May 1, 2023

Uh oh!

lidavidm left a comment

Uh oh!

lidavidm May 1, 2023

Uh oh!

kou May 1, 2023

Uh oh!

kou May 1, 2023

Uh oh!

lidavidm May 1, 2023

Uh oh!

kou May 1, 2023

Uh oh!

lidavidm May 1, 2023

Uh oh!

kou May 1, 2023

Uh oh!

ursabot commented May 2, 2023

Uh oh!

ursabot commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

kou commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented May 1, 2023

Uh oh!

github-actions bot commented May 1, 2023

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm May 1, 2023

Choose a reason for hiding this comment

Uh oh!

kou May 1, 2023

Choose a reason for hiding this comment

Uh oh!

kou May 1, 2023

Choose a reason for hiding this comment

Uh oh!

lidavidm May 1, 2023

Choose a reason for hiding this comment

Uh oh!

kou May 1, 2023

Choose a reason for hiding this comment

Uh oh!

lidavidm May 1, 2023

Choose a reason for hiding this comment

Uh oh!

kou May 1, 2023

Choose a reason for hiding this comment

Uh oh!

ursabot commented May 2, 2023

Uh oh!

ursabot commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kou commented May 1, 2023 •

edited

Loading