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

Choose proper HTTP(S) request implementation based on the agent #354

Merged
merged 6 commits into from
Nov 14, 2024
Merged
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# 1.8.1 (Node.js)

## Bug fixes

- When a custom HTTP agent is used, the HTTP or HTTPS request implementation is now correctly chosen based on the URL protocol. ([#352](https://github.com/ClickHouse/clickhouse-js/issues/352))

# 1.8.0 (Common, Node.js, Web)

## New features
Expand Down
2 changes: 1 addition & 1 deletion packages/client-common/src/version.ts
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export default '1.8.0'
export default '1.8.1'
28 changes: 23 additions & 5 deletions packages/client-node/__tests__/tls/tls.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import Http from 'http'
import https from 'node:https'
import type Stream from 'stream'
import { createClient } from '../../src'
import Https from 'https'
import http from 'http'

describe('[Node.js] TLS connection', () => {
let client: ClickHouseClient<Stream.Readable>
Expand Down Expand Up @@ -139,12 +141,8 @@ describe('[Node.js] TLS connection', () => {
})

describe('Custom HTTPS agent', () => {
let httpRequestStub: jasmine.Spy<typeof Http.request>
beforeEach(() => {
httpRequestStub = spyOn(Http, 'request').and.callThrough()
})

it('should work with a custom HTTPS agent', async () => {
const httpsRequestStub = spyOn(Https, 'request').and.callThrough()
const agent = new https.Agent({
maxFreeSockets: 5,
ca: ca_cert,
Expand All @@ -163,6 +161,26 @@ describe('[Node.js] TLS connection', () => {
format: 'JSONEachRow',
})
expect(await rs.json()).toEqual([{ result: 144 }])
expect(httpsRequestStub).toHaveBeenCalledTimes(1)
const callArgs = httpsRequestStub.calls.mostRecent().args
expect(callArgs[1].agent).toBe(agent)
})

// does not really belong to the TLS test; keep it here for consistency
it('should work with a custom HTTP agent', async () => {
const httpRequestStub = spyOn(Http, 'request').and.callThrough()
const agent = new http.Agent({
maxFreeSockets: 5,
})
const client = createClient({
url: 'http://localhost:8123',
http_agent: agent,
})
const rs = await client.query({
query: 'SELECT 144 AS result',
format: 'JSONEachRow',
})
expect(await rs.json()).toEqual([{ result: 144 }])
expect(httpRequestStub).toHaveBeenCalledTimes(1)
const callArgs = httpRequestStub.calls.mostRecent().args
expect(callArgs[1].agent).toBe(agent)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@
import { withCompressionHeaders } from '@clickhouse/client-common'
import Http from 'http'
import Https from 'https'
import type {
NodeConnectionParams,
RequestParams,
} from './node_base_connection'
import { NodeBaseConnection } from './node_base_connection'
import { withCompressionHeaders } from '@clickhouse/client-common'

export class NodeCustomAgentConnection extends NodeBaseConnection {
private readonly httpRequestFn: typeof Http.request | typeof Https.request
constructor(params: NodeConnectionParams) {
if (!params.http_agent) {
throw new Error(
'http_agent is required to create NodeCustomAgentConnection',
)
}
super(params, params.http_agent)

// See https://github.com/ClickHouse/clickhouse-js/issues/352
if (params.url.protocol.startsWith('https')) {
this.httpRequestFn = Https.request
} else {
this.httpRequestFn = Http.request
}
}

protected createClientRequest(params: RequestParams): Http.ClientRequest {
Expand All @@ -22,7 +31,7 @@ export class NodeCustomAgentConnection extends NodeBaseConnection {
enable_request_compression: params.enable_request_compression,
enable_response_compression: params.enable_response_compression,
})
return Http.request(params.url, {
return this.httpRequestFn(params.url, {
method: params.method,
agent: this.agent,
timeout: this.params.request_timeout,
Expand Down
2 changes: 1 addition & 1 deletion packages/client-node/src/version.ts
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export default '1.8.0'
export default '1.8.1'
2 changes: 1 addition & 1 deletion packages/client-web/src/version.ts
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export default '1.8.0'
export default '1.8.1'