/windsurf-review

@enqueue thank you for the contribution!
I think the new test class can be removed - anyway you have test in configuration section.
I will appreciate if you test other symbols in configuration test as well. (But not required)

Thanks!

@enqueue
Unit tests failed:

Error:  Failures: 
Error:    JdbcConfigurationTest.testConfigurationProperties:73 expected [default1] but found [clickhouse?client_name=test_application&database=default1]
Error:    JdbcConfigurationTest.testConnectionUrl:24 Maps do not have the same size:3 != 5
Error:    JdbcConfigurationTest.testConnectionUrl:23 expected [https://localhost:8443] but found [http://localhost:8443]
Error:    JdbcConfigurationTest.testConnectionUrl:24 Maps do not have the same size:3 != 5
Error:    JdbcConfigurationTest.testConnectionUrl:24 Maps do not have the same size:3 != 5
Error:    JdbcConfigurationTest.testConnectionUrl:24 Maps do not have the same size:2 != 3
Error:    JdbcConfigurationTest.testConnectionUrl:24 Maps do not have the same size:3 != 5

Would you please fix?

I'd also would like to know if this tests failing for your local build? Please let me know if it is not failing - I want to make sure tests are working in all environments. Thanks!

Hello @chernser I am still working on this PR, and am going to enhance implementation and tests. As always, some easy looking issue leads you into a rabbit hole 🤷‍♂️ .

I think I was able to get properly encoded HTTP header values work.

[felix@fmueller2021 jdbc-v2]$ echo "SELECT 1" |  curl 'http://localhost:8123/' -H "X-ClickHouse-Database*: UTF-8''%F0%9F%98%8B" --data-binary @-
1

Are additional ("phantasy") JDBC URL parameters actually taken into account, i.e. should they end up as query parameters or headers in HTTP call? There is a test for parsing them into JdbcConfiguration but I could not find any use of them further down the road.

Is it intended behavior that JDBC URL allows invalid URLs, e.g. non URL-encoded query parameters? This would be different to other popular JDBC drivers, e.g. MySQL, PostgreSQL

Furthermore, it looks to me that JdbcConfiguration does not support multiple hosts via JDBC URL, while the old implementation does. Is this intentional? If not, perhaps a follow-up issue / PR would be good.

@enqueue I've merged. Thank you!
Yes, some simple issues may lead to a rabbit hole but is very important to understand how deep is it to plan next steps. It is fine to do a partial fix and comment with what else should be done if fix will take very long time.

My opinion on the JDBC URL: it is not HTTP one and should not follow HTTP rules. It is very inconvenient to observe encoded values in JDBC connect string ("URL").
Besides old drive did not require encoded value and had very relaxed rules - so we left values as is and encode them later.

@enqueue I've fixed #2486

@chernser Thanks, I tried to limit changes to fixing issue at hand.

⚠️ Note that query parameters are currently expected to be URL encoded. To check this simply add an entry to JdbcConfigurationTest#VALID_URLs like so:

new JdbcConfigurationTestData("jdbc:clickhouse://localhost?key1=not enc/öd',e&d")
    .withAdditionalExpectedClientProperties(
        Map.of(
            "key1", "not enc/öd',e&d"
        ))

The test will fail, because general URI format according to RFC 2396 is violated (also see JDK JavaDoc URI).

If we want to allow non-encoded values we will have to establish proprietary escaping rules for some characters ("&", "=" come to mind).

@enqueue
Thank you for pointing this out!

URL encoded parameter seems a correct way. At least everyone expects it and other JDBC drivers have this rule.
We will highlight this change so who affected will encode parameters.

Using special escaping rules would be hard to adopt.