From 879fb4180f6d7bed1f413a8326c09e0e7c0b9e1b Mon Sep 17 00:00:00 2001 From: Igal Klebanov Date: Thu, 13 Apr 2023 21:49:00 +0300 Subject: [PATCH 1/4] add multi-column order bys. --- src/operation-node/delete-query-node.ts | 8 +-- src/operation-node/order-by-node.ts | 11 +-- src/operation-node/over-node.ts | 9 ++- src/operation-node/select-query-node.ts | 8 +-- src/parser/order-by-parser.ts | 81 ++++++++++++++++++++--- src/parser/reference-parser.ts | 2 +- src/query-builder/delete-query-builder.ts | 4 +- src/query-builder/over-builder.ts | 4 +- src/query-builder/select-query-builder.ts | 52 ++++++++++++--- 9 files changed, 140 insertions(+), 39 deletions(-) diff --git a/src/operation-node/delete-query-node.ts b/src/operation-node/delete-query-node.ts index fb44ca400..977dde1f4 100644 --- a/src/operation-node/delete-query-node.ts +++ b/src/operation-node/delete-query-node.ts @@ -40,15 +40,15 @@ export const DeleteQueryNode = freeze({ }) }, - cloneWithOrderByItem( + cloneWithOrderByItems( deleteNode: DeleteQueryNode, - item: OrderByItemNode + items: ReadonlyArray ): DeleteQueryNode { return freeze({ ...deleteNode, orderBy: deleteNode.orderBy - ? OrderByNode.cloneWithItem(deleteNode.orderBy, item) - : OrderByNode.create(item), + ? OrderByNode.cloneWithItems(deleteNode.orderBy, items) + : OrderByNode.create(items), }) }, diff --git a/src/operation-node/order-by-node.ts b/src/operation-node/order-by-node.ts index 9108ea2c6..1ef7d0456 100644 --- a/src/operation-node/order-by-node.ts +++ b/src/operation-node/order-by-node.ts @@ -15,17 +15,20 @@ export const OrderByNode = freeze({ return node.kind === 'OrderByNode' }, - create(item: OrderByItemNode): OrderByNode { + create(items: ReadonlyArray): OrderByNode { return freeze({ kind: 'OrderByNode', - items: freeze([item]), + items: freeze([...items]), }) }, - cloneWithItem(orderBy: OrderByNode, item: OrderByItemNode): OrderByNode { + cloneWithItems( + orderBy: OrderByNode, + items: ReadonlyArray + ): OrderByNode { return freeze({ ...orderBy, - items: freeze([...orderBy.items, item]), + items: freeze([...orderBy.items, ...items]), }) }, }) diff --git a/src/operation-node/over-node.ts b/src/operation-node/over-node.ts index 325a20c76..b2407bf9d 100644 --- a/src/operation-node/over-node.ts +++ b/src/operation-node/over-node.ts @@ -25,12 +25,15 @@ export const OverNode = freeze({ }) }, - cloneWithOrderByItem(overNode: OverNode, item: OrderByItemNode): OverNode { + cloneWithOrderByItems( + overNode: OverNode, + items: ReadonlyArray + ): OverNode { return freeze({ ...overNode, orderBy: overNode.orderBy - ? OrderByNode.cloneWithItem(overNode.orderBy, item) - : OrderByNode.create(item), + ? OrderByNode.cloneWithItems(overNode.orderBy, items) + : OrderByNode.create(items), }) }, diff --git a/src/operation-node/select-query-node.ts b/src/operation-node/select-query-node.ts index 712e062b7..8188f198a 100644 --- a/src/operation-node/select-query-node.ts +++ b/src/operation-node/select-query-node.ts @@ -102,15 +102,15 @@ export const SelectQueryNode = freeze({ }) }, - cloneWithOrderByItem( + cloneWithOrderByItems( selectNode: SelectQueryNode, - item: OrderByItemNode + items: ReadonlyArray ): SelectQueryNode { return freeze({ ...selectNode, orderBy: selectNode.orderBy - ? OrderByNode.cloneWithItem(selectNode.orderBy, item) - : OrderByNode.create(item), + ? OrderByNode.cloneWithItems(selectNode.orderBy, items) + : OrderByNode.create(items), }) }, diff --git a/src/parser/order-by-parser.ts b/src/parser/order-by-parser.ts index a4566e3c8..cfc5efd29 100644 --- a/src/parser/order-by-parser.ts +++ b/src/parser/order-by-parser.ts @@ -1,11 +1,11 @@ +import { isDynamicReferenceBuilder } from '../dynamic/dynamic-reference-builder.js' import { Expression } from '../expression/expression.js' import { OperationNode } from '../operation-node/operation-node.js' import { OrderByItemNode } from '../operation-node/order-by-item-node.js' import { RawNode } from '../operation-node/raw-node.js' -import { - parseReferenceExpression, - ReferenceExpression, -} from './reference-parser.js' +import { isExpressionOrFactory, parseExpression } from './expression-parser.js' +import { StringReference, parseStringReference } from './reference-parser.js' +import { ReferenceExpression } from './reference-parser.js' export type OrderByDirection = 'asc' | 'desc' @@ -13,18 +13,56 @@ export function isOrderByDirection(thing: unknown): thing is OrderByDirection { return thing === 'asc' || thing === 'desc' } -export type OrderByExpression = +export type DirectedOrderByStringReference = `${ + | StringReference + | (keyof O & string)} ${OrderByDirection}` + +export type UndirectedOrderByExpression = | ReferenceExpression | (keyof O & string) +export type OrderByExpression = + | UndirectedOrderByExpression + | DirectedOrderByStringReference + export type OrderByDirectionExpression = OrderByDirection | Expression -export function parseOrderBy( - orderBy: OrderByExpression, +export function parseOrderBy(args: any[]): OrderByItemNode[] { + if (args.length === 2) { + return [parseOrderByItem(args[0], args[1])] + } + + if (args.length === 1) { + const [orderBy] = args + + if (Array.isArray(orderBy)) { + return orderBy.map((item) => parseOrderByItem(item)) + } + + return [parseOrderByItem(orderBy)] + } + + throw new Error( + `Invalid number of arguments at order by! expected 1-2, received ${args.length}` + ) +} + +export function parseOrderByItem( + ref: ReferenceExpression, direction?: OrderByDirectionExpression ): OrderByItemNode { + const parsedRef = parseOrderByExpression(ref) + + if (OrderByItemNode.is(parsedRef)) { + if (direction) { + throw new Error('Cannot specify direction twice!') + } + + return parsedRef + } + return OrderByItemNode.create( - parseOrderByExpression(orderBy), + parsedRef, parseOrderByDirectionExpression(direction) ) } @@ -32,7 +70,28 @@ export function parseOrderBy( function parseOrderByExpression( expr: OrderByExpression ): OperationNode { - return parseReferenceExpression(expr) + if (isExpressionOrFactory(expr)) { + return parseExpression(expr) + } + + if (isDynamicReferenceBuilder(expr)) { + return expr.toOperationNode() + } + + const [ref, direction] = expr.split(' ') + + if (direction) { + if (!isOrderByDirection(direction)) { + throw new Error(`Invalid order by direction: ${direction}`) + } + + return OrderByItemNode.create( + parseStringReference(ref), + parseOrderByDirectionExpression(direction as any) + ) + } + + return parseStringReference(expr) } function parseOrderByDirectionExpression( @@ -44,7 +103,7 @@ function parseOrderByDirectionExpression( if (expr === 'asc' || expr === 'desc') { return RawNode.createWithSql(expr) - } else { - return expr.toOperationNode() } + + return expr.toOperationNode() } diff --git a/src/parser/reference-parser.ts b/src/parser/reference-parser.ts index f55574d00..5479539d0 100644 --- a/src/parser/reference-parser.ts +++ b/src/parser/reference-parser.ts @@ -177,7 +177,7 @@ export function parseOrderedColumnName(column: string): OperationNode { ) } - return parseOrderBy(columnName, order) + return parseOrderBy([columnName, order])[0] } else { return parseColumnName(column) } diff --git a/src/query-builder/delete-query-builder.ts b/src/query-builder/delete-query-builder.ts index 74e08629a..c96f869a3 100644 --- a/src/query-builder/delete-query-builder.ts +++ b/src/query-builder/delete-query-builder.ts @@ -657,9 +657,9 @@ export class DeleteQueryBuilder ): DeleteQueryBuilder { return new DeleteQueryBuilder({ ...this.#props, - queryNode: DeleteQueryNode.cloneWithOrderByItem( + queryNode: DeleteQueryNode.cloneWithOrderByItems( this.#props.queryNode, - parseOrderBy(orderBy, direction) + parseOrderBy([orderBy, direction]) ), }) } diff --git a/src/query-builder/over-builder.ts b/src/query-builder/over-builder.ts index 91ada55d2..6c2f07250 100644 --- a/src/query-builder/over-builder.ts +++ b/src/query-builder/over-builder.ts @@ -49,9 +49,9 @@ export class OverBuilder direction?: OrderByDirectionExpression ): OverBuilder { return new OverBuilder({ - overNode: OverNode.cloneWithOrderByItem( + overNode: OverNode.cloneWithOrderByItems( this.#props.overNode, - parseOrderBy(orderBy, direction) + parseOrderBy([orderBy, direction]) ), }) } diff --git a/src/query-builder/select-query-builder.ts b/src/query-builder/select-query-builder.ts index c608ef3a9..036d1246c 100644 --- a/src/query-builder/select-query-builder.ts +++ b/src/query-builder/select-query-builder.ts @@ -32,6 +32,8 @@ import { import { OrderByDirectionExpression, OrderByExpression, + DirectedOrderByStringReference, + UndirectedOrderByExpression, parseOrderBy, } from '../parser/order-by-parser.js' import { preventAwait } from '../util/prevent-await.js' @@ -1093,14 +1095,28 @@ export class SelectQueryBuilder /** * Adds an `order by` clause to the query. * - * `orderBy` calls are additive. To order by multiple columns, call `orderBy` - * multiple times. + * `orderBy` calls are additive. Meaning, additional `orderBy` calls append to + * the existing order by clause. * - * The first argument is the expression to order by and the second is the - * order (`asc` or `desc`). + * In a single call you can add a single column/expression or multiple columns/expressions. + * + * Single column/expression calls can have 1-2 arguments. The first argument is + * the expression to order by (optionally including the direction) while the second + * optional argument is the direction (`asc` or `desc`). * * ### Examples * + * Single column/expression per call: + * + * ```ts + * await db + * .selectFrom('person') + * .select('person.first_name as fn') + * .orderBy('id') + * .orderBy('fn desc') + * .execute() + * ``` + * * ```ts * await db * .selectFrom('person') @@ -1118,6 +1134,16 @@ export class SelectQueryBuilder * order by "id" asc, "fn" desc * ``` * + * Multiple columns/expressions per call: + * + * ```ts + * await db + * .selectFrom('person') + * .select('person.first_name as fn') + * .orderBy(['id', 'fn desc']) + * .execute() + * ``` + * * The order by expression can also be a raw sql expression or a subquery * in addition to column references: * @@ -1178,14 +1204,24 @@ export class SelectQueryBuilder * ``` */ orderBy( - orderBy: OrderByExpression, + orderBy: UndirectedOrderByExpression, direction?: OrderByDirectionExpression - ): SelectQueryBuilder { + ): SelectQueryBuilder + + orderBy( + ref: DirectedOrderByStringReference + ): SelectQueryBuilder + + orderBy( + refs: ReadonlyArray> + ): SelectQueryBuilder + + orderBy(...args: any[]): SelectQueryBuilder { return new SelectQueryBuilder({ ...this.#props, - queryNode: SelectQueryNode.cloneWithOrderByItem( + queryNode: SelectQueryNode.cloneWithOrderByItems( this.#props.queryNode, - parseOrderBy(orderBy, direction) + parseOrderBy(args) ), }) } From c2e7553517c72e6a98053407e2fc89e547e762b7 Mon Sep 17 00:00:00 2001 From: Igal Klebanov Date: Fri, 14 Apr 2023 02:07:18 +0300 Subject: [PATCH 2/4] add multi-column order by tests. --- test/node/src/order-by.test.ts | 114 ++++++++++++++++++++++++++++++--- 1 file changed, 105 insertions(+), 9 deletions(-) diff --git a/test/node/src/order-by.test.ts b/test/node/src/order-by.test.ts index e42121d60..36dbb0296 100644 --- a/test/node/src/order-by.test.ts +++ b/test/node/src/order-by.test.ts @@ -32,7 +32,7 @@ for (const dialect of DIALECTS) { await destroyTest(ctx) }) - it('order by one column', async () => { + it('should order by one column', async () => { const query = ctx.db .selectFrom('person') .selectAll() @@ -63,7 +63,7 @@ for (const dialect of DIALECTS) { ]) }) - it('order by two columns', async () => { + it('should order by two columns in two invocations', async () => { const query = ctx.db .selectFrom('person') .selectAll() @@ -88,23 +88,119 @@ for (const dialect of DIALECTS) { await query.execute() }) - it('order by aliased column', async () => { + it('should order by two columns in one invocations', async () => { const query = ctx.db .selectFrom('person') - .select('first_name as fn') + .selectAll() + .orderBy(['first_name', 'last_name desc']) + + testSql(query, dialect, { + postgres: { + sql: 'select * from "person" order by "first_name", "last_name" desc', + parameters: [], + }, + mysql: { + sql: 'select * from `person` order by `first_name`, `last_name` desc', + parameters: [], + }, + sqlite: { + sql: 'select * from "person" order by "first_name", "last_name" desc', + parameters: [], + }, + }) + + await query.execute() + }) + + it('should order by aliased columns', async () => { + const query = ctx.db + .selectFrom('person') + .select([ + 'first_name as fn', + 'middle_name as mn', + 'last_name as ln', + 'gender as g', + ]) .orderBy('fn') + .orderBy('mn asc') + .orderBy(['ln desc', 'g']) + + testSql(query, dialect, { + postgres: { + sql: [ + 'select "first_name" as "fn",', + '"middle_name" as "mn",', + '"last_name" as "ln",', + '"gender" as "g"', + 'from "person" order by "fn", "mn" asc, "ln" desc, "g"', + ], + parameters: [], + }, + mysql: { + sql: [ + 'select `first_name` as `fn`,', + '`middle_name` as `mn`,', + '`last_name` as `ln`,', + '`gender` as `g`', + 'from `person` order by `fn`, `mn` asc, `ln` desc, `g`', + ], + parameters: [], + }, + sqlite: { + sql: [ + 'select "first_name" as "fn",', + '"middle_name" as "mn",', + '"last_name" as "ln",', + '"gender" as "g"', + 'from "person" order by "fn", "mn" asc, "ln" desc, "g"', + ], + parameters: [], + }, + }) + + await query.execute() + }) + + it('should order by expressions', async () => { + const query = ctx.db + .selectFrom('person') + .selectAll() + .orderBy(sql`coalesce(${sql.ref('first_name')}, ${sql.lit('foo')}) asc`) + .orderBy((eb) => eb.fn.coalesce('first_name', sql.lit('foo'))) + .orderBy([ + sql`coalesce(${sql.ref('first_name')}, ${sql.lit('foo')})`, + (eb) => sql`${eb.fn.coalesce('first_name', sql.lit('foo'))} desc`, + ]) testSql(query, dialect, { postgres: { - sql: 'select "first_name" as "fn" from "person" order by "fn"', + sql: [ + 'select * from "person"', + `order by coalesce("first_name", 'foo') asc,`, + `coalesce("first_name", 'foo'),`, + `coalesce("first_name", 'foo'),`, + `coalesce("first_name", 'foo') desc`, + ], parameters: [], }, mysql: { - sql: 'select `first_name` as `fn` from `person` order by `fn`', + sql: [ + 'select * from `person`', + "order by coalesce(`first_name`, 'foo') asc,", + "coalesce(`first_name`, 'foo'),", + "coalesce(`first_name`, 'foo'),", + "coalesce(`first_name`, 'foo') desc", + ], parameters: [], }, sqlite: { - sql: 'select "first_name" as "fn" from "person" order by "fn"', + sql: [ + 'select * from "person"', + `order by coalesce("first_name", 'foo') asc,`, + `coalesce("first_name", 'foo'),`, + `coalesce("first_name", 'foo'),`, + `coalesce("first_name", 'foo') desc`, + ], parameters: [], }, }) @@ -112,11 +208,11 @@ for (const dialect of DIALECTS) { await query.execute() }) - it('order by raw expression', async () => { + it('order by raw expression and a direction', async () => { const query = ctx.db .selectFrom('person') .selectAll() - .orderBy(sql`coalesce(${sql.ref('first_name')}, 'foo')`, 'asc') + .orderBy((eb) => eb.fn.coalesce('first_name', sql.lit('foo')), 'asc') testSql(query, dialect, { postgres: { From e5b4c19f5b7aa3ea6b1649285036c19c615066c1 Mon Sep 17 00:00:00 2001 From: igalklebanov Date: Mon, 17 Jul 2023 00:38:22 +0300 Subject: [PATCH 3/4] fix conflict resolution mistake. --- package-lock.json | 4 ++-- src/query-builder/select-query-builder.ts | 19 +++---------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/package-lock.json b/package-lock.json index 1e2334020..0c4597dd1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "kysely", - "version": "0.25.0", + "version": "0.26.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "kysely", - "version": "0.25.0", + "version": "0.26.1", "license": "MIT", "devDependencies": { "@types/better-sqlite3": "^7.6.4", diff --git a/src/query-builder/select-query-builder.ts b/src/query-builder/select-query-builder.ts index b50b2ddc7..c8acae744 100644 --- a/src/query-builder/select-query-builder.ts +++ b/src/query-builder/select-query-builder.ts @@ -888,16 +888,6 @@ export interface SelectQueryBuilder refs: ReadonlyArray> ): SelectQueryBuilder - orderBy(...args: any[]): SelectQueryBuilder { - return new SelectQueryBuilder({ - ...this.#props, - queryNode: SelectQueryNode.cloneWithOrderByItems( - this.#props.queryNode, - parseOrderBy(args) - ), - }) - } - /** * Adds a `group by` clause to the query. * @@ -1794,15 +1784,12 @@ class SelectQueryBuilderImpl }) } - orderBy( - orderBy: OrderByExpression, - direction?: OrderByDirectionExpression - ): SelectQueryBuilder { + orderBy(...args: any[]): SelectQueryBuilder { return new SelectQueryBuilderImpl({ ...this.#props, - queryNode: SelectQueryNode.cloneWithOrderByItem( + queryNode: SelectQueryNode.cloneWithOrderByItems( this.#props.queryNode, - parseOrderBy(orderBy, direction) + parseOrderBy(args) ), }) } From 2e3d3385ab9832e1f84e724453d56096dfe48120 Mon Sep 17 00:00:00 2001 From: igalklebanov Date: Mon, 17 Jul 2023 20:32:45 +0300 Subject: [PATCH 4/4] remove joined builder re-assignment typings test. --- test/typings/test-d/index.test-d.ts | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/test/typings/test-d/index.test-d.ts b/test/typings/test-d/index.test-d.ts index ea7c8de84..8111687a7 100644 --- a/test/typings/test-d/index.test-d.ts +++ b/test/typings/test-d/index.test-d.ts @@ -21,31 +21,6 @@ import { import { Database, Person } from '../shared' import { expectType, expectError, expectAssignable } from 'tsd' -async function testConditionalJoinWhere(db: Kysely) { - let qb = db.selectFrom('person') - let petName: string | undefined = 'catto' - let petSpecies: 'cat' | 'dog' | undefined = 'cat' - - if (petName || petSpecies) { - let qb2 = qb.innerJoin('pet', 'person.id', 'pet.owner_id') - - if (petName) { - qb2 = qb2.where('pet.name', '=', petName) - } - - if (petSpecies) { - qb2 = qb2.where('pet.species', '=', petSpecies) - } - - // This is the actual test. The query builder with `pet` - // table joined should still be assignable to the original - // query builder. - qb = qb2 - } - - const res = await qb.selectAll('person').execute() -} - async function testInsert(db: Kysely) { const person = { first_name: 'Jennifer',