SNOW-1571459 parse jbdc properties #909
Conversation
| /** All jdbc properties including proxyProperties */ | ||
| private final Properties properties; | ||
| /** Proxy related properties */ | ||
| private final Properties proxyProperties; |
There was a problem hiding this comment.
There is a special use case for proxyProperties
There was a problem hiding this comment.
this.internalStage =
new SnowflakeInternalStage(
(SnowflakeConnectionV1) this.conn, credentialExpireTimeMillis, proxyProperties);
| if (proxyProperties != null && !proxyProperties.isEmpty()) { | ||
| Properties combinedProperties = | ||
| mergeProxyAndConnectionProperties(this.prop, this.proxyProperties); | ||
| if (!proxyProperties.isEmpty()) { |
There was a problem hiding this comment.
nullability is handled in JdbcProperties create().
| this.connectorName = conf.get(Utils.NAME); | ||
| this.ingestionMethodConfig = IngestionMethodConfig.determineIngestionMethod(conf); | ||
| this.prop = InternalUtils.createProperties(conf, this.url, ingestionMethodConfig); | ||
|
|
There was a problem hiding this comment.
I was thinking about parsing it inside JdbcProperties but it would also require ingestionMethodConfig and url. Hence I am parsing outside of JdbcProperties. I think it's simpler.
| } | ||
|
|
||
| public Properties getProperties() { | ||
| Properties copy = new Properties(); |
There was a problem hiding this comment.
Is there a reason for making a copy here?
There was a problem hiding this comment.
Super anti bug safety? I can remove it. I would not insist on keeping that.
There was a problem hiding this comment.
I reconsidered and removed the copy.
|
|
||
| public class JdbcPropertiesTest { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Please add new tests using JUnit5 (import from junit.jupiter package)
There was a problem hiding this comment.
Done in both classes.
| @@ -134,4 +136,18 @@ public void testResultSize() throws SQLException { | |||
| resultSet = new MockResultSetForSizeTest(100); | |||
| assert InternalUtils.resultSize(resultSet) == 100; | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
It's not mandatory, but you can give a try to migrate this file to JUnit5 as well. Maybe it can be done in 10 minutes.
There was a problem hiding this comment.
Done, only @Test needed migration.
Overview
SNOW-1571459 parse properties
SNOW-1571515 detect overrides
Add new property:
"snowflake.jdbc.map": "isInsecureMode : true, notYetExistingProp : true"This will allow user to set custom snowflake jdbc properties and to handle properties that will be created for JDBC Driver in the future.
The map is not allowed to override other jdbc properties like:
snowflake.database.name, snowflake.schema,name, snowflake.private.keyetc. norjvm.proxy.xxx. In such a case the error is thrown.It is also not designed to replace them.
Properties passed to a map are not validated, for example
"foo: bar"is allowed. In such a case we rely on Jdbc Driver behaviour.Pre-review checklist
snowflake.ingestion.method.Yes- Added end to end and Unit Tests.No- Suggest why it is not param protected