From 42dcad186c89e3fc37b48a16bbd1d1c71f0f3318 Mon Sep 17 00:00:00 2001 From: Mickael Lecoq Date: Tue, 17 May 2022 21:15:01 +0200 Subject: [PATCH] feat: support sortBy on joined table feat: update ts definition for sortBy method fix: throw error when sort by is used on joined table with loki test: add integration test on sort on joined table --- CHANGELOG-Unreleased.md | 2 +- src/QueryDescription/__tests__/test.js | 13 ++++++++++++ src/QueryDescription/operators.d.ts | 3 ++- src/QueryDescription/operators.js | 9 +++++++-- src/QueryDescription/type.d.ts | 2 ++ src/QueryDescription/type.js | 3 +++ src/__tests__/databaseTests.js | 23 ++++++++++++++++++++++ src/adapters/lokijs/worker/executeQuery.js | 6 ++++++ src/adapters/sqlite/encodeQuery/index.js | 2 +- src/adapters/sqlite/encodeQuery/test.js | 5 +++++ 10 files changed, 63 insertions(+), 5 deletions(-) diff --git a/CHANGELOG-Unreleased.md b/CHANGELOG-Unreleased.md index 7c1dc252c..c18bcd19d 100644 --- a/CHANGELOG-Unreleased.md +++ b/CHANGELOG-Unreleased.md @@ -5,7 +5,7 @@ ### Deprecations ### New features - +- [Query] Add `Q.sortBy({column:'columnName', table:'tableName'})` to support sorting on joined tables ### Fixes ### Performance diff --git a/src/QueryDescription/__tests__/test.js b/src/QueryDescription/__tests__/test.js index 15222acb3..ab694cc91 100644 --- a/src/QueryDescription/__tests__/test.js +++ b/src/QueryDescription/__tests__/test.js @@ -461,6 +461,19 @@ describe('buildQueryDescription', () => { sortBy: [{ type: 'sortBy', sortColumn: 'sortable_column', sortOrder: 'desc' }], }) }) + it('supports sorting query on joined table', () => { + const query = Q.buildQueryDescription([ + Q.sortBy({ column: 'sortable_column', table: 'joinedTable' }, Q.desc), + ]) + expect(query).toEqual({ + where: [], + joinTables: [], + nestedJoinTables: [], + sortBy: [ + { type: 'sortBy', sortColumn: 'sortable_column', sortOrder: 'desc', table: 'joinedTable' }, + ], + }) + }) it('does not support skip operator without take operator', () => { expect(() => { Q.buildQueryDescription([Q.skip(100)]) diff --git a/src/QueryDescription/operators.d.ts b/src/QueryDescription/operators.d.ts index 2c112b595..43048ac7f 100644 --- a/src/QueryDescription/operators.d.ts +++ b/src/QueryDescription/operators.d.ts @@ -15,6 +15,7 @@ import type { On, SortOrder, SortBy, + SortColumn, Take, Skip, JoinTables, @@ -114,7 +115,7 @@ export const or: ArrayOrSpreadFn export const asc: SortOrder export const desc: SortOrder -export function sortBy(sortColumn: ColumnName, sortOrder?: SortOrder): SortBy +export function sortBy(sortColumn: SortColumn, sortOrder?: SortOrder): SortBy export function take(count: number): Take diff --git a/src/QueryDescription/operators.js b/src/QueryDescription/operators.js index 5204406a6..27a5767a5 100644 --- a/src/QueryDescription/operators.js +++ b/src/QueryDescription/operators.js @@ -23,6 +23,7 @@ import type { On, SortOrder, SortBy, + SortColumn, Take, Skip, JoinTables, @@ -212,12 +213,16 @@ export const or: ArrayOrSpreadFn = (...args): Or => { export const asc: SortOrder = 'asc' export const desc: SortOrder = 'desc' -export function sortBy(sortColumn: ColumnName, sortOrder: SortOrder = asc): SortBy { +export function sortBy(sortColumn: SortColumn, sortOrder: SortOrder = asc): SortBy { invariant( sortOrder === 'asc' || sortOrder === 'desc', `Invalid sortOrder argument received in Q.sortBy (valid: asc, desc)`, ) - return { type: 'sortBy', sortColumn: checkName(sortColumn), sortOrder } + + const sortCol = typeof sortColumn === 'object' ? sortColumn.column : sortColumn + const table = typeof sortColumn === 'object' ? sortColumn.table : undefined + + return { type: 'sortBy', sortColumn: checkName(sortCol), sortOrder, table } } export function take(count: number): Take { diff --git a/src/QueryDescription/type.d.ts b/src/QueryDescription/type.d.ts index e4e868eba..693256496 100644 --- a/src/QueryDescription/type.d.ts +++ b/src/QueryDescription/type.d.ts @@ -50,10 +50,12 @@ export type On = $RE<{ export type SortOrder = 'asc' | 'desc' export const asc: SortOrder export const desc: SortOrder +export type SortColumn = ColumnName | $RE<{column: ColumnName, table: TableName}> export type SortBy = $RE<{ type: 'sortBy' sortColumn: ColumnName sortOrder: SortOrder + table?: TableName }> export type Take = $RE<{ type: 'take' diff --git a/src/QueryDescription/type.js b/src/QueryDescription/type.js index 366895c62..af878bc39 100644 --- a/src/QueryDescription/type.js +++ b/src/QueryDescription/type.js @@ -49,10 +49,13 @@ export type On = $RE<{ conditions: Where[], }> export type SortOrder = 'asc' | 'desc' + +export type SortColumn = ColumnName | $RE<{column: ColumnName, table: TableName}> export type SortBy = $RE<{ type: 'sortBy', sortColumn: ColumnName, sortOrder: SortOrder, + table?: TableName, }> export type Take = $RE<{ type: 'take', diff --git a/src/__tests__/databaseTests.js b/src/__tests__/databaseTests.js index 1c779633c..f237b6d0a 100644 --- a/src/__tests__/databaseTests.js +++ b/src/__tests__/databaseTests.js @@ -991,6 +991,7 @@ function joinTest( skipCount?: boolean, skipLoki?: boolean, skipSqlite?: boolean, + checkOrder?: boolean, }>, ): void { joinTests.push(options) @@ -1140,6 +1141,28 @@ joinTest({ { id: 'n6' }, // bad TT ], }) +joinTest({ + name: `can perform Q.sort on joined table`, + query: [ + Q.experimentalJoinTables(['tag_assignments']), + Q.sortBy({ column: 'text1', table: 'tag_assignments' }), + ], + extraRecords: { + tag_assignments: [ + { id: 'tt1', text1: 'z', task_id: 'm6' }, + { id: 'tt2', text1: 'y', task_id: 'm8' }, + { id: 'tt3', text1: 'x', task_id: 'm7' }, + { id: 'tt4', text1: 'w', task_id: 'n5' }, + { id: 'tt5', text1: 'v', task_id: 'n6' }, + { id: 'tt6', text1: 'u', task_id: 'm2' }, + { id: 'tt7', text1: 'z', task_id: 'm2' }, + ], + }, + matching: [{ id: 'm2' }, { id: 'n6' }, { id: 'n5' }, { id: 'm7' }, { id: 'm8' }, { id: 'm6' }], + nonMatching: [], + checkOrder: true, + skipLoki: true, +}) joinTest({ name: `can perform Q.on's nested in Q.on`, query: [ diff --git a/src/adapters/lokijs/worker/executeQuery.js b/src/adapters/lokijs/worker/executeQuery.js index f29e9e758..369761244 100644 --- a/src/adapters/lokijs/worker/executeQuery.js +++ b/src/adapters/lokijs/worker/executeQuery.js @@ -1,5 +1,7 @@ // @flow +import { invariant } from '../../../utils/common' + import type { SerializedQuery } from '../../../Query' import type { DirtyRaw } from '../../../RawRecord' @@ -31,6 +33,10 @@ function performQuery(query: SerializedQuery, loki: Loki): LokiResultset { // Step three: sort, skip, take const { sortBy, take, skip } = query.description if (sortBy.length) { + if (process.env.NODE_ENV !== 'production') { + invariant(!sortBy.some((sort) => sort.table), 'sortBy is not supported on joined table') + } + resultset = resultset.compoundsort( sortBy.map(({ sortColumn, sortOrder }) => [sortColumn, sortOrder === 'desc']), ) diff --git a/src/adapters/sqlite/encodeQuery/index.js b/src/adapters/sqlite/encodeQuery/index.js index 7d61639c3..b152c2b30 100644 --- a/src/adapters/sqlite/encodeQuery/index.js +++ b/src/adapters/sqlite/encodeQuery/index.js @@ -194,7 +194,7 @@ const encodeOrderBy = (table: TableName, sortBys: SortBy[]) => { } const orderBys = sortBys .map((sortBy) => { - return `"${table}"."${sortBy.sortColumn}" ${sortBy.sortOrder}` + return `"${sortBy.table ?? table}"."${sortBy.sortColumn}" ${sortBy.sortOrder}` }) .join(', ') return ` order by ${orderBys}` diff --git a/src/adapters/sqlite/encodeQuery/test.js b/src/adapters/sqlite/encodeQuery/test.js index dbd29c6f3..a1ffc9a56 100644 --- a/src/adapters/sqlite/encodeQuery/test.js +++ b/src/adapters/sqlite/encodeQuery/test.js @@ -256,6 +256,11 @@ describe('SQLite encodeQuery', () => { `select "tasks".* from "tasks" where "tasks"."_status" is not 'deleted' order by "tasks"."sortable_column" desc, "tasks"."sortable_column2" asc`, ) }) + it('encodes order by clause on table', () => { + expect(encoded([Q.sortBy({ column: 'sortable_column', table: 'table' }, Q.desc)])).toBe( + `select "tasks".* from "tasks" where "tasks"."_status" is not 'deleted' order by "table"."sortable_column" desc`, + ) + }) it('encodes limit clause', () => { expect(encoded([Q.take(100)])).toBe( `select "tasks".* from "tasks" where "tasks"."_status" is not 'deleted' limit 100`,