[CALCITE-6135] BEARER authentication support by richardantal · Pull Request #299 · apache/calcite-avatica

richardantal · 2026-02-03T16:11:05Z

No description provided.

richardantal · 2026-02-03T16:11:46Z

This is the rebase and clean up version of #232

mihaibudiu · 2026-02-03T17:06:50Z

core/src/main/java/org/apache/calcite/avatica/remote/BearerTokenProvider.java

+
+  void init(ConnectionConfig config) throws IOException;
+
+  String obtain(String username);


Can you please document this method?
What is the result?

Sure, I'll do that

It returns the Token used for Authentication.

mihaibudiu · 2026-02-03T17:13:38Z

core/src/main/java/org/apache/calcite/avatica/remote/AvaticaHttpClientFactoryImpl.java

+          if (null == username) {
+            username = System.getProperty("user.name");
+          }
+          ((BearerAuthenticateable) client).setTokenProvider(username, tokenProvider);


can userName by null here?
The method that follows does not expect them.

The userName can not be null when calling setTokenProvider
We need it later when we get the JWT token:
new BearerToken(tokenProvider.obtain(username))

But can't System.getProperty return null?

mihaibudiu · 2026-02-03T17:27:41Z

core/src/main/java/org/apache/calcite/avatica/BuiltInConnectionProperty.java

+  /** Bearer token to use to perform Bearer authentication. */
+  BEARER_TOKEN("bearer_token", Type.STRING, null, false),
+
+  /** File containing bearer tokens to use to perform Bearer authentication. */


Not obvious how the file is represented using a string, presumably it's some kind of path or URI.

Yes, a path
With this I would like to add the possibility to add an other implementation for BearerTokenProvider In the future.
Where instead of storing the Token, we can store the token file's location and load the token(s) from a file.

Can you please add this information to JavaDoc?

mihaibudiu · 2026-02-03T17:28:38Z

core/src/main/java/org/apache/calcite/avatica/ConnectionPropertyValue.java

+  int getInt();
+
+  /**
+   * Returns the int value of this property. Throws if not set and no


No default means defaultValue is null?

Probably You are right it can only happen if the defaultValue is set to null

mihaibudiu · 2026-02-03T17:29:15Z

core/src/main/java/org/apache/calcite/avatica/ConnectionPropertyValue.java

+  String getString(String defaultValue);
+
+  /**
+   * Returns the int value of this property. Throws if not set and no


What does "default" mean here?
Is this a per-property notion?

It has been copied from PropEnv, the same applies to all similar functions.
Should I remove the trows if not set and no default part?
Do you think a comment like this would be better?
Returns the long value of this property or the defaultValue.

Well, it's not "long"
And I still don't know where the default value is coming from.

property is a ConnectionProperty which has a defaultValue functions.

mihaibudiu · 2026-02-03T17:29:50Z

core/src/main/java/org/apache/calcite/avatica/ConnectionPropertyValue.java

+   * Returns the string value of this property, or null if not specified and
+   * no default.
+   */
+  String getString(String defaultValue);


this is the only one that never throws, why this asymmetry?

I think either getString methods Throw.
It has been copied from PropEnv which is the implementation of this ConnectionPropertyValue interface

I know you didn't write that documentation, but is there a way to check whether this is correct?
If not, maybe you can fix it there too.

mihaibudiu · 2026-02-03T17:33:41Z

core/src/test/java/org/apache/calcite/avatica/remote/BearerTokenProviderFactoryTest.java

+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class BearerTokenProviderFactoryTest {


are all tests in this file negative?

Can you please be more specific?

In testCustomBearerToken and testConstantBearerToken I test the happy path to validate the correct behavior of tokenProvider.obtain.
There is message property provided if the expected value is different from the actual.

Co-authored-by: Aron Meszaros <meszaros.aron.attila@gmail.com>

mihaibudiu · 2026-02-04T19:43:11Z

Please avoid force-pushing until the review is finished; it makes it harder to see what is changed.
Once the PR is accepted you can squash all the commits into one.

BearerToken edge case handling, user is null, bearer token is null more tests added for tokenProvider and setTokenProvider

mihaibudiu

Now you can squash the commits for merging.

richardantal force-pushed the BEARER_Rebased branch from f869d41 to 069d250 Compare February 3, 2026 16:22

mihaibudiu reviewed Feb 3, 2026

View reviewed changes

[CALCITE-6135] BEARER authentication support

c461b58

Co-authored-by: Aron Meszaros <meszaros.aron.attila@gmail.com>

richardantal force-pushed the BEARER_Rebased branch from 069d250 to c461b58 Compare February 4, 2026 13:43

improve javadoc

380825e

BearerToken edge case handling, user is null, bearer token is null more tests added for tokenProvider and setTokenProvider

richardantal requested a review from mihaibudiu February 6, 2026 07:35

mihaibudiu approved these changes Feb 6, 2026

View reviewed changes

mihaibudiu added the LGTM and would merge soon label Feb 6, 2026


		void init(ConnectionConfig config) throws IOException;

		String obtain(String username);

Conversation

richardantal commented Feb 3, 2026

Uh oh!

richardantal commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaibudiu commented Feb 4, 2026

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

richardantal commented Feb 3, 2026 •

edited

Loading