ClickHouseConnection#prepareStatement(String, int) should not throw an exception when Statement.RETURN_GENERATED_KEYS is recognized

[linghengqian]

Describe the bug

• I didn't really see a template for feature requests on GitHub Issues, so I used the template for bug reports.
• The com.clickhouse.jdbc.ClickHouseConnection#prepareStatement(String, int) method in the com.clickhouse:clickhouse-jdbc ClickHouse JDBC driver should not throw an exception when java.sql.Statement.RETURN_GENERATED_KEYS is recognized. See https://github.com/ClickHouse/clickhouse-java/blob/v0.7.1-patch1/clickhouse-jdbc/src/main/java/com/clickhouse/jdbc/ClickHouseConnection.java#L126 .

    @Override
    default PreparedStatement prepareStatement(String sql, int autoGeneratedKeys) throws SQLException {
        if (autoGeneratedKeys != Statement.NO_GENERATED_KEYS) {
            // not entirely true, what if the table engine is JDBC?
            throw SqlExceptionUtils.unsupportedError("Only NO_GENERATED_KEYS is supported");
        }

        return prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY,
                ResultSet.HOLD_CURSORS_OVER_COMMIT);
    }

• Some third-party frameworks like apache/shardingsphere try to proxy com.clickhouse.jdbc.ClickHouseConnection and generate distributed IDs from independent logic, which leads to strange handling like Adds documentation for ClickHouse support apache/shardingsphere#33779 .

import com.zaxxer.hikari.HikariConfig;
import com.zaxxer.hikari.HikariDataSource;
import java.sql.*;
public class ExampleTest {
    long test() throws SQLException {
        HikariConfig config = new HikariConfig();
        config.setJdbcUrl("jdbc:shardingsphere:classpath:demo.yaml");
        config.setDriverClassName("org.apache.shardingsphere.driver.ShardingSphereDriver");
        try (HikariDataSource dataSource = new HikariDataSource(config);
             Connection connection = dataSource.getConnection();
             PreparedStatement preparedStatement = connection.prepareStatement(
                     "INSERT INTO t_order (user_id, order_type, address_id, status) VALUES (1, 1, 1, 'INSERT_TEST')",
                     Statement.NO_GENERATED_KEYS
             )) {
            preparedStatement.executeUpdate();
            try (ResultSet resultSet = preparedStatement.getGeneratedKeys()) {
                if (resultSet.next()) {
                    return resultSet.getLong(1);
                }
                throw new RuntimeException();
            }
        }
    }
}

Steps to reproduce

• Doesn't seem necessary?

Expected behaviour

• com.clickhouse.jdbc.ClickHouseConnection#prepareStatement(String, int) does not throw an exception when autoGeneratedKeys is java.sql.Statement.RETURN_GENERATED_KEYS.

Code example

• Probably not.

Error log

• The following exception is intercepted from a unit test of apache/shardingsphere. An early investigation is at Improve GraalVM Reachability Metadata and corresponding nativeTest related unit tests apache/shardingsphere#29052 .

java.sql.SQLFeatureNotSupportedException: Only NO_GENERATED_KEYS is supported
	at com.clickhouse.jdbc.SqlExceptionUtils.unsupportedError(SqlExceptionUtils.java:152)
	at com.clickhouse.jdbc.ClickHouseConnection.prepareStatement(ClickHouseConnection.java:129)
	at com.zaxxer.hikari.pool.ProxyConnection.prepareStatement(ProxyConnection.java:344)
	at com.zaxxer.hikari.pool.HikariProxyConnection.prepareStatement(HikariProxyConnection.java)
	at org.apache.shardingsphere.driver.jdbc.core.statement.StatementManager.prepareStatement(StatementManager.java:81)

Configuration

Environment

• Client version: 0.6.3
• Language version: What is Language version? If the template refers to the JDK version, I use GraalVM CE For 22.0.2.
• OS: Ubuntu 22.04.4 LTS

ClickHouse server

• ClickHouse Server version: clickhouse/clickhouse-server:24.10.2.80
• ClickHouse Server non-default settings, if any: None.
• CREATE TABLE statements for tables involved: None.
• Sample data for all these tables, use clickhouse-obfuscator if necessary

• None.

[linghengqian]

• I just submitted a quick PR. But I can't seem to get the unit tests to start, and things like ~/.m2/toolchains.xml don't recognize the JDK instance installed by SDKMAN!.

[chernser]

Good day, @linghengqian !
Thank you for the PR!

Here is my example of toolchains.xml and I'm using SDKMAN

cat ~/.m2/toolchains.xml
<?xml version="1.0" encoding="UTF-8"?>
<toolchains>

  <!-- JDK toolchains -->
  <toolchain>
    <type>jdk</type>
    <provides>
      <version>17</version>
      <vendor>jetbrais</vendor>
    </provides>
    <configuration>
      <jdkHome> /home/sc/.sdkman/candidates/java/17.0.10-jbr/</jdkHome>
    </configuration>
  </toolchain>
</toolchains>

I will look soon on the PR and the issue.

[chernser]

Hi, @linghengqian!

Unfortunately, ClickHouse has no ability to return anything in response to an INSERT operation. Here is related issue ClickHouse/ClickHouse#21697
So throwing exception is correct behavior required by JDBC spec.

[linghengqian]

• @chernser This involves another question, can the clickhouse jdbc driver allow adding a jdbc parameter to prevent an exception from being thrown at ClickHouseConnection#prepareStatement(String, int)? It sounds like,

jdbc:ch://localhost:8123/ds_demo?throwExWhenReturnKey=false

• Similar to the HiveServer2 JDBC Driver, no exception is thrown so that the third-party framework can take over the org.apache.hive.jdbc.HiveConnection. See https://github.com/apache/hive/blob/master/jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java#L1872 .

/*
   * (non-Javadoc)
   *
   * @see java.sql.Connection#prepareStatement(java.lang.String, int)
   */
  @Override
  public PreparedStatement prepareStatement(String sql, int autoGeneratedKeys)
      throws SQLException {
    if (isClosed) {
      throw new SQLException("Connection is closed");
    }
    return new HivePreparedStatement(this, client, sessHandle, sql);
  }

[chernser]

@linghengqian
I personally think, that JDBC drivers should throw exceptions in case they do not support some features, because it is how JDBC spec suggests. But if we ignore this and accept any value then we will get another issue from users that expect exception explicitly saying that feature is not supported :-)
The bigger problem is that frameworks ignore implementation then they may misuse JDBC drivers what I would like to avoid.

Can framework have an option to set NO_GENERATED_KEYS for DBs that do not support this feature? I think, it will be more correct because the framework is handling keys for such cases.

[linghengqian]

@chernser

• You are right, third-party frameworks should do this. But what apache/shardingsphere does here is special, because all apache/shardingsphere does is to add extra functionality to the existing database, and it deliberately supports both Statement.RETURN_GENERATED_KEYS and Statement.NO_GENERATED_KEYS. These handles happen to be in the common part and are not split into separate database dialect modules.
• I personally prefer to just ask users to be aware of this downstream, as stated in https://shardingsphere.apache.org/document/current/en/user-manual/shardingsphere-jdbc/optional-plugins/clickhouse/#key-generate-restrictions .
• I think I should close this issue and Prevent ClickHouseConnection#prepareStatement(String, int) from throwing exception for Statement.RETURN_GENERATED_KEYS #1969 ?

[chernser]

@linghengqian
yes, please close your PR.
I will close the issue.