-
Notifications
You must be signed in to change notification settings - Fork 30
Refactor SessionCredentials #470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor SessionCredentials #470
Conversation
databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/Consent.java
Outdated
Show resolved
Hide resolved
databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/Consent.java
Outdated
Show resolved
Hide resolved
* | ||
* <p>Implements the refresh_token OAuth grant type with optional token caching. | ||
*/ | ||
public class SessionCredentialsTokenSource extends RefreshableTokenSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we AutoValue
this and use an auto value builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some reading up, it seems like AutoValue does not support constructors that call parent constructors with arguments: https://stackoverflow.com/questions/45069608/autovalue-how-to-call-super. In this case, we need super(token)
in the constructor, so I have opted for removing the builder class and just have the two optional fields as Optional
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG, thanks @emmyzhou-db
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the sessionCredentials used now? I am seeing all its usages are changed to sessionCredentialsTokenSource. Maybe I am missing something.
So SessionCredentials is not used internally as a CredentialsProvider, but based on the examples from https://github.com/databricks/databricks-sdk-java/blob/main/examples/spring-boot-oauth-u2m-demo/src/main/java/com/databricks/sdk/RootController.java it seems like the intention is that a user can get a SessionCredentials and set it as the CredentialsProvider to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo Renaud comments.
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
* | ||
* <p>Implements the refresh_token OAuth grant type with optional token caching. | ||
*/ | ||
public class SessionCredentialsTokenSource extends RefreshableTokenSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG, thanks @emmyzhou-db
What changes are proposed in this pull request?
This PR refactors the
SessionCredentials
class to achieve clearer separation of concerns by splitting its responsibilities into two distinct components: aCredentialsProvider
implementation and a dedicatedTokenSource
implementation. This refactoring is necessary for introducing token cache wrappers in future PRs.Current Implementation
SessionCredentials
both implementsCredentialsProvider
and extendsRefreshableTokenSource
.Proposed Changes
SessionCredentials
now implements onlyCredentialsProvider
and delegates all token operations to a newSessionCredentialsTokenSource
class.SessionCredentialsTokenSource
extendsRefreshableTokenSource
and is solely responsible for token refresh logic.Benefits
SessionCredentials
remains a publicCredentialsProvider
as intended with no breaking changes to the public API.How is this tested?
Existing unit tests.
NO_CHANGELOG=true