Skip to content

Commit

Permalink
Don't override pathname in transformUrl
Browse files Browse the repository at this point in the history
  • Loading branch information
slvrtrn committed Oct 30, 2023
1 parent 2552dbb commit 8557404
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 9 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## 0.2.5 (Common, Node.js, Web)

### Bug fixes

- `pathname` segment from `host` client configuration parameter is now handled properly when making requests.
See this [comment](https://github.com/ClickHouse/clickhouse-js/issues/164#issuecomment-1785166626) for more details.

## 0.2.4 (Node.js only)

No changes in web/common modules.
Expand Down
25 changes: 25 additions & 0 deletions packages/client-common/__tests__/unit/transform_url.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,22 @@
import { transformUrl } from '@clickhouse/client-common'

describe('transformUrl', () => {
it('only adds the trailing slash to a url without pathname', () => {
const url = new URL('http://clickhouse.com')
const newUrl = transformUrl({
url,
})
expect(newUrl.toString()).toBe('http://clickhouse.com/')
})

it('does nothing with a url with pathname', () => {
const url = new URL('http://clickhouse.com/clickhouse')
const newUrl = transformUrl({
url,
})
expect(newUrl.toString()).toBe('http://clickhouse.com/clickhouse')
})

it('attaches pathname and search params to the url', () => {
const url = new URL('http://clickhouse.com')
const newUrl = transformUrl({
Expand All @@ -20,6 +36,15 @@ describe('transformUrl', () => {
expect(newUrl.toString()).toBe('http://clickhouse.com/foo')
})

it('attaches pathname to an existing pathname', () => {
const url = new URL('http://clickhouse.com/clickhouse')
const newUrl = transformUrl({
url,
pathname: '/foobar',
})
expect(newUrl.toString()).toBe('http://clickhouse.com/clickhouse/foobar')
})

it('does not mutate an original url', () => {
const url = new URL('http://clickhouse.com')
const newUrl = transformUrl({
Expand Down
11 changes: 9 additions & 2 deletions packages/client-common/src/utils/url.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { ClickHouseSettings } from '../settings'
import { formatQueryParams, formatQuerySettings } from '../data_formatter'
import type { ClickHouseSettings } from '../settings'

export function transformUrl({
url,
Expand All @@ -13,7 +13,14 @@ export function transformUrl({
const newUrl = new URL(url)

if (pathname) {
newUrl.pathname = pathname
// See https://developer.mozilla.org/en-US/docs/Web/API/URL/pathname
// > value for such "special scheme" URLs can never be the empty string,
// > but will instead always have at least one / character.
if (newUrl.pathname === '/') {
newUrl.pathname = pathname
} else {
newUrl.pathname += pathname
}
}

if (searchParams) {
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 '0.2.4'
export default '0.2.5'
6 changes: 3 additions & 3 deletions packages/client-node/src/connection/node_base_connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ export abstract class NodeBaseConnection

const stream = await this.request({
method: 'POST',
url: transformUrl({ url: this.params.url, pathname: '/', searchParams }),
url: transformUrl({ url: this.params.url, searchParams }),
body: params.query,
abort_signal: params.abort_signal,
decompress_response: clickhouse_settings.enable_http_compression === 1,
Expand All @@ -343,7 +343,7 @@ export abstract class NodeBaseConnection

const stream = await this.request({
method: 'POST',
url: transformUrl({ url: this.params.url, pathname: '/', searchParams }),
url: transformUrl({ url: this.params.url, searchParams }),
body: params.query,
abort_signal: params.abort_signal,
})
Expand Down Expand Up @@ -403,7 +403,7 @@ export abstract class NodeBaseConnection

const stream = await this.request({
method: 'POST',
url: transformUrl({ url: this.params.url, pathname: '/', searchParams }),
url: transformUrl({ url: this.params.url, searchParams }),
body: params.values,
abort_signal: params.abort_signal,
compress_request: this.params.compression.compress_request,
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 '0.2.4'
export default '0.2.5'
2 changes: 1 addition & 1 deletion packages/client-web/src/connection/web_connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ export class WebConnection implements Connection<ReadableStream> {
}): Promise<Response> {
const url = transformUrl({
url: this.params.url,
pathname: pathname ?? '/',
pathname,
searchParams,
}).toString()

Expand Down
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 '0.2.4'
export default '0.2.5'

0 comments on commit 8557404

Please sign in to comment.