[PECOBLR-595][PECOBLR-184] Add UID validation to always be token #909
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
vikrantpuppala
changed the title
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds validation to ensure the UID parameter is always set to "token" or omitted entirely. This change enforces stricter parameter validation for JDBC connections and replaces the legacy USER parameter with UID across test files.
- Replaces USER parameter with UID in all test files for consistency
- Introduces new validation logic to enforce UID parameter must be "token" or empty
- Adds comprehensive test coverage for UID validation scenarios
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| DatabricksJdbcUrlParams.java | Changes USER parameter to UID parameter |
| DatabricksJdbcConstants.java | Adds VALID_UID_VALUE constant |
| ValidationUtil.java | Implements UID validation logic |
| DatabricksVendorCodes.java | New enum for centralized error codes |
| DatabricksConnectionContext.java | Integrates validation into connection parsing |
| Exception classes | Adds constructors supporting vendor codes |
| Test files | Updates test code to use UID instead of USER |
| DatabricksConnectionContextTest.java | Adds comprehensive UID validation tests |
| NEXT_CHANGELOG.md | Documents the new validation feature |
Comments suppressed due to low confidence (1)
src/test/java/com/databricks/jdbc/api/impl/DatabricksConnectionContextTest.java:694
- The test only checks if the error message contains a substring. Consider also verifying the exact vendor code (DatabricksVendorCodes.INCORRECT_UID.getCode()) for more robust validation, similar to the other test methods.
assertTrue(exception.getMessage().contains("Expected 'token' or omit UID parameter entirely"));
src/main/java/com/databricks/jdbc/common/util/ValidationUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/databricks/jdbc/common/util/ValidationUtil.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
jprakash-db
reviewed
Jul 24, 2025
| @@ -8,7 +8,7 @@ public enum DatabricksJdbcUrlParams { | |||
| LOG_PATH("logpath", "Path to the log file"), | |||
| LOG_FILE_SIZE("LogFileSize", "Maximum size of the log file", "10"), // 10 MB | |||
| LOG_FILE_COUNT("LogFileCount", "Number of log files to retain", "10"), | |||
| USER("user", "Username for authentication"), | |||
| UID("uid", "UID for authentication"), | |||
There was a problem hiding this comment.
Won't this break for users using user currently ?
There was a problem hiding this comment.
- User is not a documented param in the JDBC driver
- it was just being used in tests
- if any user is nevertheless still using this field, it would just be ignored
jprakash-db
approved these changes
Jul 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds validation to ensure the UID parameter is always set to "token" or omitted entirely. This change enforces stricter parameter validation for JDBC connections and replaces the legacy USER parameter with UID across test files.
Replaces USER parameter with UID in all test files for consistency
Introduces new validation logic to enforce UID parameter must be "token" or empty
Adds comprehensive test coverage for UID validation scenarios
Testing
Unit + manual tests
Additional Notes to the Reviewer