Update DriverDataSource to not convert all provided property values to String.

[aoutzen-snap]

This PR updates DriverDataSource to preserve the original driver property values provided to DriverDataSource's constructor instead of converting all property values to String representations. This addresses issues like those brought up in #1496 and #1242 where non-String Object property values are being provided.

This is particularly important for the latter issue of compatibility with the Snowflake JDBC driver, which supports a "privateKey" property requiring a value of type java.security.PrivateKey but throws an exception due to DriverDataSource's current behavior of converting the property value to a String representation.

Updates have also been madeto PropertyElf for consistency, as suggested in comments on an older unmerged PR (#1557) for this issue.

[alex-rentz]

Nice job getting this fix out.

Future thought - I wonder if configuring properties that can be should be masked is something worthwhile... I suppose it's not too often we'd see a sensitive field pop up in there, but any kind of secret or key that might be sensitive should not be logged.

[alex-rentz]

Looks like we have a build failure @aoutzen-snap .

[sujitrect]

Hi @alex-rentz it looks like the build issue is unrelated to this PR. Its present in current dev as well.
Can you please see how this fix can be released asap.
We are using HikariCP and blocked on this issue, specifically for supporting Snowflake jdbc.

[alex-rentz]

Hi @alex-rentz it looks like the build issue is unrelated to this PR. Its present in current dev as well. Can you please see how this fix can be released asap. We are using HikariCP and blocked on this issue, specifically for supporting Snowflake jdbc.

Thanks for pointing that out. My approval doesn't mean much in this repo - will need a maintainer to approve.

[sujitrect]

@brettwooldridge can I draw ur attention to this PR for an early merge?

[aoutzen-snap]

@brettwooldridge Any chance I could get your thoughts on this? It's a pretty significant blocker to using Hikari to connect to Snowflake.

[smanoharan1]

LGTM

[bernalvarela]

Same here!!, we need this pr to be merged to use HikariCP with snowflake.

We have the same problem using the private key as property.

There's anything we can do to unblock this pr?.

Thank you for your work and regards!!

[dariocazas]

Hi. Any progress or blockage with this PR? We need this change so we don't have to work with a forked version of HikariCP

[lfbayer]

I would imagine that this could be a breaking change for any user that happened to be passing non-string values into the configuration currently.

The JDBC spec is defined to use a Properties class for the driver properties. This would imply that to be compliant with the spec, all driver properties must be a string. Is Snowflake using functionality that technically isn't right?

The JavaDoc for the Properties class refers to adding non-String values as a "compromised" Properties object

[alex-rentz]

I would imagine that this could be a breaking change for any user that happened to be passing non-string values into the configuration currently.

The JDBC spec is defined to use a Properties class for the driver properties. This would imply that to be compliant with the spec, all driver properties must be a string. Is Snowflake using functionality that technically isn't right?

The JavaDoc for the Properties class refers to adding non-String values as a "compromised" Properties object

This is a great point regarding it potentially breaking some users, but I don't see anything in the JDBC spec (relating to Datasources) that requires a String property value. Can you share what it is that indicates this requirement?

I was referring to the latest spec here in section 9.6.1 - https://download.oracle.com/otndocs/jcp/jdbc-4_2-mrel2-spec/index.html

[dariocazas]

Yes, Snowflake defines a property that is not a String: privateKey

This property requires a java.security.PrivateKey object to auth with Snowflake using key-pair: https://docs.snowflake.com/en/user-guide/jdbc-configure.html#privatekey-property-in-connection-properties

They provide another forms to run auth, but in our case we don't build the application with the PEM file inside (to reference directly a file), the credentials will be provided at runtime and generating the instance of the java.security.PrivateKey requires by Snowflake.

I agree with you that Properties class say explicity that "Ok, don't use put as entry to work with non-String key or value", but probably Snowflake is not the unique database driver that use this approach.

About if is a breaking change... ok, as I see the changes follow this:

• Copy a Properties object into a new Properties object
• The target properties convert every key and value to String object

In this context... why you need convert to String a Properties that you receive as a non-String? If you really pass a Properties that is not a String, IMHO, you should know it, and pass this configuration as is required by the driver.

[Arax89]

Hello everyone,
this is also a blocking issue for my client's project.
When, if at all, can we expect a new release with this issue solved?

[lfbayer]

In my opinion the responsibility to fix this lies with Snowflake. As far as I know, no other JDBC driver uses non-string properties. And there is nothing in the JDBC spec that implies that anything other than String properties should be supported. (JDBC spec uses the Properties class and the Properties class specifically states that using non-string objects will cause undefined behavior).

Also as I stated before, implementing this has the chance of being a breaking change. So it feels like modifying HikariCP to support this cases isn't the Correct™ solution to this problem.

Is there a different way to provide the PrivateKey to Snowflake without needing to pass the object itself as a property?

[alex-rentz]

I can appreciate that you keep things running smoothly for existing consumers of HikariCP. It would be up to Snowflake to implement an alternative to using a PrivateKey directly - let's say, using a URL from which a private key could be loaded at runtime. However, this String-casting decision is effectively preventing use of Snowflake, which would otherwise be supported.

In addition, due to the security concerns of use of private key, I'd advocate for the continued use of Java's PrivateKey interface in this context. I suspect the use of keys or other authentication methods will become more common in JDBC, so this won't be a Snowflake-only concern in the future.

[brettwooldridge]

As @lfbayer pointed out, this would be a breaking change. The first alternative would be to request that snowflake accept a base64 encoded key. The second is to use programatic configuration of the HikariConfig rather than Properties-based. Through programmatic configuration there is a HikariConfig.setDataSource​(javax.sql.DataSource dataSource) method. This method exists to allow the user to configure and create their DataSource as desired, and then provide it to the pool.

[alex-rentz]

As @lfbayer pointed out, this would be a breaking change. The first alternative would be to request that snowflake accept a base64 encoded key. The second is to use programatic configuration of the HikariConfig rather than Properties-based. Through programmatic configuration there is a HikariConfig.setDataSource​(javax.sql.DataSource dataSource) method. This method exists to allow the user to configure and create their DataSource as desired, and then provide it to the pool.

I can look into whether programmatic option makes sense for us. It may require a special case compared to what we do for other JDBC connection creations, or a re-write of the code we had for that. Though it seems like a good workaround.

How would we feel about a configuration option "Preserve Datasource Property Types" or something of that nature to allow flexing on this behavior?

[lfbayer]

I'm going to close this pull request. As long as the JDBC spec uses a Properties class to pass these property values, I think it would be incorrect to use anything other than Strings. As stated above there is a way to handle this by setting the DataSource programmatically.

There is also a ticket open against Snowflake for this issue: snowflakedb/snowflake-jdbc#1053.