Added migration helper project - #3084
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| <version>${revision}</version> | ||
| </parent> | ||
|
|
||
| <artifactId>migration-helpers</artifactId> |
There was a problem hiding this comment.
New module omitted from Maven reactor
High Severity
The new migration-helpers artifact is never registered in the parent pom.xml <modules> list, so a root mvn install or deploy does not compile, test, or publish it even though the changelog advertises the module.
Reviewed by Cursor Bugbot for commit 5210019. Configure here.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Composite settings parser ignores escapes
Medium Severity
parseAndAddKeyValuePairs splits custom_settings and custom_http_headers on commas and the first =, without honoring the backslash escaping used by v1 ClickHouseOption.toKeyValuePairs and v2 ClientConfigProperties.toKeyValuePairs. Values that contain \, or \= are split into the wrong keys and values.
Reviewed by Cursor Bugbot for commit 5210019. Configure here.
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-simple</artifactId> | ||
| <version>${slf4j.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
slf4j-simple pulled in at compile scope
Medium Severity
slf4j-simple is declared as a default-scope dependency. Other modules such as jdbc-v2 and clickhouse-client keep it test-scoped. Consumers of migration-helpers inherit that binding and can get duplicate or hijacked SLF4J logging.
Reviewed by Cursor Bugbot for commit 5210019. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 5 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5314c1c. Configure here.
| sslkey=ssl_key | ||
| proxy_username=proxy_user | ||
| alive_timeout=http_keep_alive_timeout | ||
| http_keep_alive=http_keep_alive_timeout |
There was a problem hiding this comment.
Boolean keep-alive mapped to timeout
High Severity
The new http_keep_alive mapping treats a v1 boolean keep-alive flag as http_keep_alive_timeout. Typical v1 values like true become a Long timeout, and v2 config parsing then fails with NumberFormatException during client or JDBC startup. The v1 timeout that actually maps to http_keep_alive_timeout is alive_timeout.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5314c1c. Configure here.
| Class<?> clazz = Class.forName("com.clickhouse.jdbc.JdbcConfig", false, getClass().getClassLoader()); | ||
| Method getDriverPropertiesMethod = clazz.getMethod("getDriverProperties"); | ||
| Object driverProps = getDriverPropertiesMethod.invoke(null); | ||
| if (driverProps != null && driverProps.getClass().isArray()) { |
There was a problem hiding this comment.
JdbcConfig reflection expects an array
Low Severity
The new classpath loader for JdbcConfig.getDriverProperties() only handles arrays, but that method returns a List. The isArray() check never succeeds, so runtime enrichment of v1 JDBC property names is a silent no-op.
Reviewed by Cursor Bugbot for commit 5314c1c. Configure here.
1 participant
Summary
Checklist
Delete items not relevant to your PR: