-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for URL connection string in ClickHouse client #174
Conversation
Extended the ClickHouse client configuration to accept a URL connection string which can contain user info, hosts and ports, database name and query parameters. This change provides flexible and standard connections setup. Updated related unit tests to reflect this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that url
might conflict with other params (host, dbname, user, password, params, etc.) and it's easy to make a typo in a string-based configuratuion, I'm not sure it should be supported. This logic might be easily implemented in the application
const url = new URL('clickhouse://clickhouse://john:secret@127.0.0.1:9000')
const client = createClient({ host: url.host, .... });
WDYT @slvrtrn
/** ClickHouse connection string url | ||
* ```clickhouse://[user_info@][hosts_and_ports][/dbname][?query_parameters]``` | ||
*/ | ||
url?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that url
might conflict with other params (host, dbname, user, password, params, etc.) and it's easy to make a typo in a string-based configuratuion, I'm not sure it should be supported. This logic might be easily implemented in the application
const url = new URL('clickhouse://clickhouse://john:secret@127.0.0.1:9000')
const client = createClient({ host: url.host, .... });
password: config.password ?? '', | ||
database: config.database ?? 'default', | ||
clickhouse_settings: config.clickhouse_settings ?? {}, | ||
username: config.username ?? (url && url.username) ?? 'default', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that configuration should take precedence, but I'd expect the client to throw an exception to ask an end user to specify the correct value explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be reflected in documentation, and not throw error cause it's not something that will break execution. It's also very flexible to have, for example following setup:
const client = createClient({ url: 'clickhouse://john@127.0.0.1:9000', password: process.env.CH_PASSWORD });
It's follows logic beside original CH behaviour (ClickHouse/ClickHouse#50689)
Thanks for your contribution, @xrip, but I agree with @mshustov here. This example: const url = new URL('clickhouse://clickhouse://john:secret@127.0.0.1:9000')
const client = createClient({ host: url.host, .... }); is very simple and trivial to implement when needed in the application, so I don't see the necessity to build it into the client itself. I also understand that having a PR declined does not feel good. To avoid situations like this in the future, I think we should start with a feature request issue, where an intention to contribute could be stated. This way, we can discuss the pros and cons in advance and decide if it's worth investing the time into writing the actual code/tests and reviewing it later. I'd also like to add that we have a backlog of triaged issues if you are willing to contribute, for example:
|
After clickhouse-client added connection string support (ClickHouse/ClickHouse#50689) there is now nothing stops to add same support to this client.
Summary
This pull request add support for URL connection string in ClickHouse client.
Extended the ClickHouse client configuration to accept a URL connection string which can contain user info, hosts and ports, database name and query parameters. This change provides flexible and standard connections setup. Updated related unit tests to reflect this change.
Checklist
Delete items not relevant to your PR: