removed default port adjustment#2754
Closed
chernser wants to merge 2 commits into
Closed
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
| Map.of(JdbcConfiguration.USE_SSL_PROP, "true")) | ||
| .withExpectedConnectionURL("https://localhost:8443"), // ssl should not be passed to client | ||
| new JdbcConfigurationTestData("jdbc:clickhouse://[::1]") | ||
| new JdbcConfigurationTestData("jdbc:clickhouse://[::1]:8123") |
There was a problem hiding this comment.
Duplicate test entries introduced during port migration
Low Severity
The newly added test entry at line 39 (jdbc:clickhouse://[::1]:8123) is an exact duplicate of the pre-existing entry at line 41. Similarly, the entry added at line 77 (jdbc:clickhouse://localhost:8123/?custom_key1=val1) duplicates the pre-existing entry at line 88. Both arose from mechanically replacing no-port test URLs with port-explicit versions without noticing identical entries already existed. The old no-port entries tested distinct scenarios; the replacements add no new coverage.
Additional Locations (1)
|
mzitnik
reviewed
Feb 24, 2026
| new JdbcConfigurationTestData("jdbc:clickhouse:http://localhost"), | ||
| new JdbcConfigurationTestData("jdbc:clickhouse:https://localhost") | ||
| .withExpectedConnectionURL("https://localhost:8443"), | ||
| new JdbcConfigurationTestData("jdbc:ch://localhost:8123") |
Contributor
There was a problem hiding this comment.
We only test here the happy path lets add some negative tests
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Closes #2753
Checklist
Delete items not relevant to your PR:
Note
Medium Risk
Breaking change for clients that relied on implicit ports in JDBC URLs; could cause connection failures until URLs are updated.
Overview
Removes JDBC URL default-port normalization in
jdbc-v2soJdbcConfiguration.createConnectionURL()no longer appends:8123/:8443when the port is omitted, relying solely onURI.create(...).toASCIIString().Updates URL parsing tests to require explicit ports in connection URLs (including
jdbc:ch://...) and documents this as a breaking change inCHANGELOG.md(issue #2753).Written by Cursor Bugbot for commit 7879a7d. This will update automatically on new commits. Configure here.