feat: oauth support in databricks destination #1912
Conversation
shekhar-rudder
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
ItsSudip,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughThis pull request integrates OAuth-based authentication support into the Delta Lake destination configuration. Changes include the addition of new keys for OAuth in the configuration file, conditional validation of authentication credentials in the schema, and updates to the UI to allow users to toggle between traditional token input and new OAuth fields, adjusting the display and validations accordingly. Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1912 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/configurations/destinations/deltalake/schema.json (1)
930-952: Ensure OAuth Credential Validation and Property Definition
This block conditionally requires"oauthClientId"and"oauthClientSecret"when"useOauth"istrue. It is important to verify that the property"useOauth"is properly defined within the main schema's"properties"(or elsewhere in the configuration) so that these conditional checks are triggered as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/configurations/destinations/deltalake/db-config.json(2 hunks)src/configurations/destinations/deltalake/schema.json(1 hunks)src/configurations/destinations/deltalake/ui-config.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (6)
src/configurations/destinations/deltalake/db-config.json (2)
29-31: New OAuth Keys in Default Configuration
The keys"useOauth","oauthClientId", and"oauthClientSecret"have been added to the"defaultConfig"array. This addition is consistent with the OAuth support introduced in the schema and UI. Please confirm that downstream components correctly handle these new configuration parameters.
136-137: Secure Handling of OAuth Client Secret
Adding"oauthClientSecret"to the"secretKeys"array is a good move to ensure that sensitive OAuth credentials are kept secure. This change aligns with the security practices for configuration management.src/configurations/destinations/deltalake/ui-config.json (4)
33-38: New OAuth Toggle in UI
The new checkbox field labeled "Use M2M OAuth" is introduced to allow users to toggle between PAT and OAuth authentication methods. This addition is clearly presented and sets the foundation for conditionally displaying the appropriate authentication fields.
47-52: Conditional Display of Personal Access Token Field
The "Personal Access Token" field now includes a"preRequisiteField"that ensures it is only shown when"useOauth"isfalse. Additionally, it is marked as"secret": true, which helps protect sensitive data. This change matches the intended UI behavior.
53-65: Addition of OAuth Client ID Field
The new "Client ID" field is configured to appear only when"useOauth"istrueusing the"preRequisiteField"attribute. The field’s regex and error message provide basic validation, aligning with the requirements for OAuth credential input.
66-79: Addition of OAuth Client Secret Field
The added "Client Secret" field is correctly configured with"secret": trueand conditional display via"preRequisiteField", making it visible only when"useOauth"istrue. This configuration meets security and UX requirements for OAuth integrations.
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/data/validation/destinations/deltalake.json (2)
377-395: Test Case "With invalid token type": Validate token data type enforcement.
This test correctly passes a boolean value for the token and expects failure. Note that the error message"token must be string"appears twice along with the"must match \"then\" schema"error. Please verify that duplicate error messages are intentional as per the schema validation design.
396-418: Test Case "With empty token": Ensure token pattern validation.
This test feeds an empty string for the token and expects errors related to pattern matching. As with the previous case, the duplicate error message for the token pattern should be reviewed to confirm it is the desired behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/configurations/destinations/deltalake/schema.json(2 hunks)test/data/validation/destinations/deltalake.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/configurations/destinations/deltalake/schema.json
🔇 Additional comments (4)
test/data/validation/destinations/deltalake.json (4)
419-437: Test Case "With valid token for PAT authentication": Verify successful PAT configuration.
The test validates that whenuseOauthis false and a valid token is provided, configuration validation passes. The setup appears correct for PAT-based authentication.
438-459: Test Case "With empty clientId for Oauth authentication": Check mandatory OAuth client ID.
This test confirms the configuration fails when OAuth is enabled (useOauthis true) butoauthClientIdis empty. The error message ensures that the client ID meets the required pattern.
460-480: Test Case "With missing clientSecret for Oauth authentication": Enforce required OAuth client secret.
The test validates that omittingoauthClientSecretwhen OAuth is activated results in a configuration error. This behavior is correctly captured by the expected error messages.
481-501: Test Case "With valid config for Oauth authentication": Confirm valid OAuth configuration.
The test provides valid OAuth credentials with bothoauthClientIdandoauthClientSecretand expects the configuration to pass validation.Note on Field Naming: Although the PR summary mentions a new toggle field named
authenticationType, the configuration in these tests uses the boolean fielduseOauthto toggle OAuth support. Please verify that the field naming is intentional and consistent across the implementation, schema, and UI.
What are the changes introduced in this PR?
authenticationType, to allow selection between PAT and OAuth authentication.oauthClientIdandoauthClientSecret, which are displayed only when OAuth authentication is selected.tokenfield to be shown only when PAT authentication is chosen.What is the related Linear task?
Resolves INT-XXX
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new checks got introduced or modified in test suites. Please explain the changes.
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
New Features
Bug Fixes