issue liquibase#340 - Liquibase throws IllegalArgumentException "Multiple entries with same key: enablearrow=0 and enablearrow=0" #341
Conversation
…iple entries with same key: enablearrow=0 and enablearrow=0"
📝 WalkthroughWalkthroughReworked URL handling in DatabricksConnection: deduplicate URL parameters (case-insensitive), avoid pre-inserting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (1)
54-55: Parameters are set redundantly in both driverProperties and the URL, creating undefined driver behavior.The code sets
UserAgentEntryandEnableArrowin thedriverPropertiesobject (lines 54-55) and then appends the same parameters to the URL string (line 64), both passed todriver.connect()at line 73. Since JDBC driver behavior for duplicate parameters from different sources is driver-specific and not well-defined, this could cause conflicts or unexpected precedence with the Databricks JDBC driver.Additionally, the
getConnectionUrl()method (lines 160-174) also appends the same parameters, creating further inconsistency.Minor inconsistency: Line 64 appends
EnableArrow=0without a trailing semicolon, while lines 167 and 170 include the trailing semicolon. This creates inconsistent URL formatting.Recommendation: Choose a single source (either driverProperties OR URL parameters) rather than both, or verify that the Databricks JDBC driver explicitly handles this redundancy without conflicts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java(3 hunks)
🔇 Additional comments (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (1)
190-210: LGTM with one note.The refactored URL construction logic correctly uses the
urlContainsParamhelper to avoid duplicate parameters. The semicolon handling here (both parameters get trailing semicolons on lines 203 and 206) is consistent and correct.Note: This method is more consistent with semicolon usage than the
openmethod—see the earlier comment about line 70.
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java(3 hunks)
🔇 Additional comments (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (1)
139-163: LGTM! Previous review feedback addressed.The exact parameter name matching logic correctly addresses the false positive issue raised in the previous review. The implementation properly extracts the parameter name (left of
=) and performs case-insensitive exact matching, avoiding the prefix-matching issue.
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java
Show resolved
Hide resolved
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (1)
197-220:getConnectionUrlis safe now; consider slightly clearer handling of null/empty URLsThe refactor removes the NPE/SIOOB risks and reuses
urlContainsParam(...)to avoid introducing duplicate keys, which is good. One minor behavioral nuance:
- When
con.getMetaData().getURL()returnsnull, you normalize it to""and then synthesize a URL like";UserAgentEntry=Liquibase;EnableArrow=0;". That may be a bit surprising to callers that interpret “no URL” differently from “constructed URL with only tool-specific params”.If you want to keep behavior closer to the underlying metadata, you could early‑return
""(orrawUrl) when it’snull/empty, and only do the appends when you actually have a base URL:String rawUrl = con.getMetaData().getURL(); - // Handle null or empty URL according to JDBC spec - if (rawUrl == null) { - rawUrl = ""; - } - - // Check for ; characters - StringBuilder urlBuilder = new StringBuilder(rawUrl); + if (rawUrl == null || rawUrl.isEmpty()) { + return rawUrl == null ? "" : rawUrl; + } + + // Check for ; characters + StringBuilder urlBuilder = new StringBuilder(rawUrl); // Ensure there's a terminating semicolon for consistent parsing if (rawUrl.isEmpty() || rawUrl.charAt(rawUrl.length() - 1) != ';') { urlBuilder.append(";"); }Not a blocker, just a small clarity/semantics improvement if you care about distinguishing “unknown URL” from “augmented URL”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java(3 hunks)
🔇 Additional comments (2)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (2)
64-73: URL parameter de‑duplication inopenlooks correct and consistentThe new
StringBuilderusage plusurlContainsParam(...)checks correctly avoid appending duplicateUserAgentEntry/EnableArrowkeys while preserving the existing behavior of always setting the driver properties. The added semicolon forEnableArrow=0;also fixes the earlier separator inconsistency. No issues from my side here.
139-163:urlContainsParamhelper is robust against case and prefix collisionsThe helper now:
- Normalizes the URL (terminating
;, removes spaces),- Splits on
;,- Compares only the key portion before the first
=in a case‑insensitive way.This avoids the earlier prefix-matching pitfall (
foovsfoobar) and correctly handlesEnableArrowin any casing. The implementation looks sound and side‑effect free.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (3)
18-23: Alignconfield generalization withopenConnto avoid unnecessary downcastYou’ve generalized the stored connection to
java.sql.Connection, butopenConnstill downcasts the result ofdriverObject.connect(...)toS42Connection. That downcast is now unnecessary for the field and keeps this path tightly coupled to the JDBC42-specific implementation, which can become brittle if the driver type changes.You can keep the S42-specific handling isolated in
getUnderlyingSparkConnectionand letopenConnstore whateverConnectionthe driver returns:- public void openConn(String url, Driver driverObject, Properties driverProperties) throws DatabaseException { + public void openConn(String url, Driver driverObject, Properties driverProperties) throws DatabaseException { String sanitizedUrl = sanitizeUrl(url); try { Scope.getCurrentScope().getLog(this.getClass()).info("opening connection " + sanitizedUrl); - this.con = (S42Connection) driverObject.connect(url, driverProperties); + this.con = driverObject.connect(url, driverProperties); if (this.con == null) { Scope.getCurrentScope().getLog(this.getClass()).severe("Connection could not be created"); throw new DatabaseException("Connection could not be created to " + sanitizedUrl + " with driver " + driverObject.getClass().getName() + ". Possibly the wrong driver for the given database URL"); }
getUnderlyingSparkConnectionalready safely checksinstanceof S42Connection, so this remains compatible while making the connection-opening logic more robust.Also applies to: 92-104
125-151: Optional: consider case-insensitive lookup for consistency with other helpers
getUrlParamValuecurrently does an exact, case-sensitivestartsWith(paramName + "=")match. That’s fine if callers always use the canonical case, but it’s slightly inconsistent with the new case-insensitive handling inurlContainsParam/deduplicateUrlParameters.If you expect mixed-casing in URLs and want consistent behavior, consider normalizing both sides to lower case (similar to
urlContainsParam) before thestartsWithcheck.
268-297: Clarify behavior when the metadata URL is null/emptyThe current flow handles
nullsafely by converting it to"", then synthesizing a URL-like string (e.g.,";UserAgentEntry=Liquibase;EnableArrow=0;"). That’s functionally safe, but a bit surprising if callers ever inspect this value beyond logging.If you prefer a clearer contract, consider an early return for the degenerate case and only run the URL-massaging logic when there’s an actual base URL:
- String rawUrl = con.getMetaData().getURL(); - - // Handle null or empty URL according to JDBC spec - if (rawUrl == null) { - rawUrl = ""; - } - - // First, deduplicate all parameters in the URL to handle any existing duplicates - rawUrl = deduplicateUrlParameters(rawUrl); + String rawUrl = con.getMetaData().getURL(); + if (rawUrl == null || rawUrl.isEmpty()) { + return ""; + } + + // First, deduplicate all parameters in the URL to handle any existing duplicates + rawUrl = deduplicateUrlParameters(rawUrl);The rest of the method can stay as-is, now operating only on a non-empty base URL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java(12 hunks)
🔇 Additional comments (5)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (5)
39-48: Underlying Spark connection unwrapping is safe and focusedThe
instanceof S42Connectionguard and nestedinstanceof SparkJDBCConnectioncheck keep this method safe and nicely scoped: you only expose aSparkJDBCConnectionwhen the underlying type matches, and gracefully returnnullotherwise. No changes needed here.
59-63: URL/driverProperties normalization inopencorrectly prevents duplicate parameter keysThe sequence of:
- stripping
UserAgentEntryandEnableArrowfromdriverProperties,- deduplicating URL parameters,
- enforcing a trailing
;,- appending
UserAgentEntry/EnableArrowonly when they’re missing, and- deduplicating again
is a solid way to avoid
"Multiple entries with same key"errors while still guaranteeing the desired defaults. This should robustly handle both user-supplied URL parameters and driver-injected properties.Also applies to: 67-69, 75-88
153-177: Case-insensitive parameter detection is robust and avoids prefix collisionsThe new
urlContainsParamcorrectly:
- normalizes the URL with a trailing
;,- strips spaces and splits on
;,- isolates the key portion before
=, and- does an exact, case-insensitive comparison.
This avoids false positives on prefix-like parameter names and works well with the deduplication logic.
179-235: URL parameter deduplication logic is correct and order-preserving
deduplicateUrlParameterscleanly:
- separates the base URL from the parameter segment,
- normalizes and splits the parameter string,
- uses a
LinkedHashMapwith lowercase keys to deduplicate case-insensitively while keeping the first occurrence and its original casing, and- reconstructs the URL with a consistent
;-terminated parameter list.This should reliably eliminate duplicate keys like
EnableArrow/enablearrowwithout disturbing parameter order.
347-367: JDBC delegate overloads remain correct after signature reflowThe updated line wrapping in the various
createStatement,prepareCall, andprepareStatementoverloads does not change behavior: each method still delegates directly to the underlyingConnectionand consistently wrapsSQLExceptioninDatabaseException. No issues here.Also applies to: 491-501, 523-577, 545-552, 555-562, 565-572
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (1)
120-146: Consider case-insensitive matching for consistency.The method uses case-sensitive matching (
startsWith(paramName + "=")), while the newurlContainsParammethod uses case-insensitive matching. This inconsistency could cause subtle bugs when parameter names have different casing in the URL.For example, if the URL contains
useragententry=value(lowercase),urlContainsParamwould find it, butgetUrlParamValuewould not.Apply this diff to make the matching case-insensitive:
// Use Java Streams to find the parameter value Optional<String> paramString = Arrays.stream(uriArgs) - .filter(x -> x.startsWith(paramName + "=")) + .filter(x -> { + int equalsIndex = x.indexOf('='); + if (equalsIndex == -1) return false; + String paramPart = x.substring(0, equalsIndex).toLowerCase(); + return paramPart.equals(paramName.toLowerCase()); + }) .findFirst();
♻️ Duplicate comments (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (1)
261-292: Same redundant deduplication pattern as inopenmethod.Similar to the
openmethod, the second deduplication at line 291 is unnecessary ifurlContainsParamreliably prevents duplicate additions. Since you deduplicate at line 271 and only append missing parameters (lines 282-287), the final deduplication should never find duplicates.String result = urlBuilder.toString(); - // Deduplicate again after adding parameters to ensure no duplicates - return deduplicateUrlParameters(result); + return result;
🧹 Nitpick comments (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (1)
62-82: Consider removing the second deduplication call.The deduplication at line 82 is redundant if
urlContainsParamcorrectly prevents duplicate additions. Since you deduplicate at line 63 and only append parameters that don't already exist (lines 72-77), the second deduplication should never find duplicates.This is defensive programming but adds unnecessary overhead.
String updatedUrl = urlBuilder.toString(); - - // Deduplicate again after adding parameters to ensure no duplicates - updatedUrl = deduplicateUrlParameters(updatedUrl); - this.openConn(updatedUrl, driverObject, driverProperties); + this.openConn(updatedUrl, driverObject, driverProperties);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java(10 hunks)
🔇 Additional comments (3)
src/main/java/liquibase/ext/databricks/database/DatabricksConnection.java (3)
54-57: Good defensive approach to prevent duplicate parameters.Removing these properties before they can be duplicated in the URL is the right strategy to address the root cause.
148-172: LGTM!The case-insensitive exact matching logic correctly prevents false positives from parameter name prefixes. The implementation properly handles edge cases (null inputs, missing '=', empty strings).
174-230: Solid deduplication logic with reasonable assumptions.The method correctly handles the typical JDBC URL format and preserves the first occurrence of duplicate parameters (case-insensitive). The use of
LinkedHashMapensures parameter order is maintained.One minor consideration: parameters without an
=sign are silently skipped (lines 208-210). This is reasonable for malformed URLs, but you might consider logging a warning if this occurs during debugging.
|
Just wanted to call out, I addressed this bug in my previous PR #332 My solution was less robust; I just checked if the URL contains the two specific configs and then only sets those configs in |
|
macdhollister Thanks for the heads up! :) |
2 participants
Liquibase throws IllegalArgumentException "Multiple entries with same key: enablearrow=0 and enablearrow=0"