Lazy handling of early setClientInfo/setNetworkTimeout calls in LazyConnectionDataSourceProxy

[guanchengang]

Extend LazyConnectionInvocationHandler to cache early calls to:

• setClientInfo(String, String)
• setNetworkTimeout(Executor, int)

These methods now defer physical connection acquisition until Statement creation, consistent with existing lazy behavior for autoCommit, readOnly, transactionIsolation, catalog, and schema.

Accept and lazily cache calls to setNetworkTimeout even when the provided Executor is null. Since some JDBC driver implementations completely ignore the Executor parameter (or fall back to a default executor), we cannot meaningfully validate or handle a null Executor before the physical connection is obtained.

getClientInfo() and getClientInfo(String) remains non-lazy (triggers immediate connection fetch)because it is a read operation whose value cannot be reliably cached due to driver defaults, pooled connection remnants, or external session modifications.

setClientInfo(Properties) remains non-lazy. The reason is that JDBC driver implementations are inconsistent. Some treat it as overwrite, others as append/merge. To guarantee behavior identical to non-lazy execution across all driver, we choose not to cache or replay it, avoiding any risk of semantic mismatch.

Closes gh-37258

[sbrannen]

Thanks for the PR!

I've requested a few changes and have a question.

Accept and lazily cache calls to setNetworkTimeout even when the provided Executor is null. Since some JDBC driver implementations completely ignore the Executor parameter (or fall back to a default executor), we cannot meaningfully validate or handle a null Executor before the physical connection is obtained.

The Javadoc for java.sql.Connection.setNetworkTimeout(Executor, int) explicitly states that it will throw a java.sql.SQLException if "the executor is null". So, are you claiming that a java.sqlConnection returned from LazyConnectionDataSourceProxy.getConnection(String, String) should not comply with the contract of the JDBC specification? In other words, shouldn't we rather eagerly throw an SQLException for a null Executor?

In any case, please add tests for the null Executor use case.

Once you've addressed these issues I'll take another look.

Cheers,

Sam

[sbrannen]

Note how all other "get" methods only return the cached value if it's non-null and otherwise delegate to the actual connection with comments like the following.

// Else fetch actual Connection and check there.

This same logic should be applied here instead of unconditionally returning 0, and we need a test in place to verify that behavior.

[sbrannen]

This and other things in this file break the build due to Checkstyle violations.

Make sure you run ./gradlew check or ./gradlew build before submitting PR or pushing changes.

[sbrannen]

Suggested change

	void lazyHandingCatalog() throws SQLException {
	void lazyHandlingForCatalog() throws SQLException {

Note "handing" -> "handling".

Please make similar changes to the other test method names.

[sbrannen]

Instead of implementing a custom MockConnection (which is more of a "stub" than a "mock"), please use Mockito.

For examples, see DataSourceTransactionManagerTests in the same package.

[sbrannen]

I also noticed that the Javadoc should be updated. The current wording only mentions auto-commit mode, transaction isolation, and read-only mode:

Connection initialization properties like auto-commit mode, transaction isolation and read-only mode will be kept and applied to the actual JDBC Connection as soon as an actual Connection is fetched (if ever).

That sentence is already outdated. It doesn't mention catalog, schema, or holdability, which were added in 6.1.2, and this PR now adds two more deferred properties (client info and network timeout).

So, please update the class-level Javadoc by enumerating all of the deferred properties there and explicitly calling out setClientInfo(Properties), getClientInfo(), and getClientInfo(String) as the exceptions that force immediate acquisition of the physical Connection. That way users can rely on the Javadoc to know what's actually lazy instead of having to read the implementation.

[guanchengang]

The Javadoc for java.sql.Connection.setNetworkTimeout(Executor, int) explicitly states that it will throw a java.sql.SQLException if "the executor is null". So, are you claiming that a java.sqlConnection returned from LazyConnectionDataSourceProxy.getConnection(String, String) should not comply with the contract of the JDBC specification? In other words, shouldn't we rather eagerly throw an SQLException for a null Executor?

I did notice that the spec explicitly says null Executor should throw SQLException. However, after looking into how various JDBC drivers actually implement this method, I found that behavior is inconsistent:

• Some drivers strictly follow the spec and throw an exception when Executor is null.
• But others (like PostgreSQL and SQL Server) completely ignore the Executor parameter and simply set the socket timeout without using the provided Executor at all.

If we eagerly throw an SQLException at the proxy layer for a null Executor, we would be introducing inconsistent behavior compared to using a non-lazy connection directly. A user would get an exception when using LazyConnectionDataSourceProxy, but the same code would work fine against the raw physical connection from those drivers.

To maintain behavioral consistency between lazy and non-lazy usage, I think it's more reasonable to cache the call lazily and defer the exception (if any) to the underlying physical connection. This way, the actual driver decides whether to throw or ignore, and we don't break users who rely on drivers that accept null.

What are your thoughts?

[guanchengang]

Thanks for the review. I’ve updated the PR based on all your suggestions, with the exception of the setNetworkTimeout part. I believe we should discuss that further before making a change.

Actually, the behavior of setNetworkTimeout is now consistent with that of setHoldability and setTransactionIsolation. When an out-of-range value is set on the LazyConnection returned by our proxy, such as setTransactionIsolation(1000), the exception is likewise deferred until the actual Connection is fetched.

Looking forward to your thoughts.