Skip to content
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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions __tests__/unit/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,28 @@ describe('createClient', () => {
)
})

it('should not mutate provided configuration', async () => {
it('throws on incorrect "url" config value', () => {
expect(() => createClient({ url: 'bar' })).toThrowError(
'Configuration parameter "url" contains malformed url.'
)
})

it('should accept url connection string and not mutate provided configuration', async () => {
const config: ClickHouseClientConfigOptions = {
host: 'http://localhost',
url: 'clickhouse://default:password@localhost/default',
password: 'foobar',
}
createClient(config)
// none of the initial configuration settings are overridden
// by the defaults we assign when we normalize the specified config object
expect(config).toEqual({
host: 'http://localhost',
url: 'clickhouse://default:password@localhost/default',
request_timeout: undefined,
max_open_connections: undefined,
tls: undefined,
compression: undefined,
username: undefined,
password: undefined,
password: 'foobar',
application: undefined,
database: undefined,
clickhouse_settings: undefined,
Expand Down
29 changes: 21 additions & 8 deletions src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ import type { ClickHouseSettings } from './settings'
import type { InputJSON, InputJSONObjectEachRow } from './clickhouse_types'

export interface ClickHouseClientConfigOptions {
/** ClickHouse connection string url
* ```clickhouse://[user_info@][hosts_and_ports][/dbname][?query_parameters]```
*/
url?: string
Copy link
Member

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, .... });

/** A ClickHouse instance URL.
* <br/> Default value: `http://localhost:8123`. */
host?: string
Expand Down Expand Up @@ -135,11 +139,11 @@ function validateConfig({ url }: NormalizedConfig): void {
// TODO add SSL validation
}

function createUrl(host: string): URL {
function createUrl(url: string, type: 'host' | 'url'): URL {
try {
return new URL(host)
return new URL(url)
} catch (err) {
throw new Error('Configuration parameter "host" contains malformed url.')
throw new Error(`Configuration parameter "${type}" contains malformed url.`)
}
}

Expand All @@ -158,20 +162,29 @@ function normalizeConfig(config: ClickHouseClientConfigOptions) {
}
}
}

const url = config.url && createUrl(config.url, 'url')

return {
application_id: config.application,
url: createUrl(config.host ?? 'http://localhost:8123'),
url: createUrl(
config.host ?? (url && `http://${url.host}`) ?? 'http://localhost:8123',
'host'
),
request_timeout: config.request_timeout ?? 300_000,
max_open_connections: config.max_open_connections ?? Infinity,
tls,
compression: {
decompress_response: config.compression?.response ?? true,
compress_request: config.compression?.request ?? false,
},
username: config.username ?? 'default',
password: config.password ?? '',
database: config.database ?? 'default',
clickhouse_settings: config.clickhouse_settings ?? {},
username: config.username ?? (url && url.username) ?? 'default',
Copy link
Member

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.

Copy link
Author

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)

password: config.password ?? (url && url.password) ?? '',
database: config.database ?? (url && url.pathname.slice(1)) ?? 'default',
clickhouse_settings:
config.clickhouse_settings ??
(url && Object.fromEntries(url.searchParams)) ??
{},
log: {
LoggerClass: config.log?.LoggerClass ?? DefaultLogger,
},
Expand Down