From 878e34ad5ba5993557e7c8a4f6cf3d2ae5be60a6 Mon Sep 17 00:00:00 2001 From: Philipp Date: Sun, 2 Apr 2023 21:50:28 +0200 Subject: [PATCH] fix: make take/skip default (#573) --- README.md | 9 +- src/paginate.spec.ts | 324 +++++++++++++++++++++++-------------------- src/paginate.ts | 8 +- 3 files changed, 179 insertions(+), 162 deletions(-) diff --git a/README.md b/README.md index 1335b85..46b58a4 100644 --- a/README.md +++ b/README.md @@ -260,14 +260,11 @@ const paginateConfig: PaginateConfig { * Required: false * Type: string * Description: Allow user to choose between limit/offset and take/skip. - * Default: PaginationType.LIMIT_AND_OFFSET + * Default: PaginationType.TAKE_AND_SKIP * - * However, using take/skip can cause problems with sorting and selections. - * For more information see: - * [#4742](https://github.com/typeorm/typeorm/issues/4742) - * [#5670](https://github.com/typeorm/typeorm/issues/5670) + * However, using limit/offset can cause problems with relations. */ - paginationType: PaginationType.TAKE_AND_SKIP, + paginationType: PaginationType.LIMIT_AND_OFFSET, /** * Required: false diff --git a/src/paginate.spec.ts b/src/paginate.spec.ts index 552e600..16278ad 100644 --- a/src/paginate.spec.ts +++ b/src/paginate.spec.ts @@ -319,6 +319,22 @@ describe('paginate', () => { expect(result.data).toStrictEqual(cats.slice(0, 2)) }) + it('should return correct result for limited one-to-many relations', async () => { + const config: PaginateConfig = { + relations: ['toys'], + sortableColumns: ['id', 'toys.id'], + searchableColumns: ['name', 'toys.name'], + defaultLimit: 4, + } + const query: PaginateQuery = { + path: '', + } + + const result = await paginate(query, catRepo, config) + + expect(result.data.length).toStrictEqual(4) + }) + it('should return correct links for some results', async () => { const config: PaginateConfig = { sortableColumns: ['id'], @@ -1828,60 +1844,6 @@ describe('paginate', () => { }) } - it('should return result based on virtualcolumn filter', async () => { - const config: PaginateConfig = { - sortableColumns: ['id'], - filterableColumns: { - 'home.countCat': [FilterOperator.GT], - }, - relations: ['home'], - } - const query: PaginateQuery = { - path: '', - filter: { - 'home.countCat': '$gt:0', - }, - sortBy: [['id', 'ASC']], - } - - const result = await paginate(query, catRepo, config) - const expectedResult = [0, 1].map((i) => { - const ret = Object.assign(clone(cats[i]), { home: Object.assign(clone(catHomes[i])) }) - delete ret.home.cat - return ret - }) - - expect(result.data).toStrictEqual(expectedResult) - expect(result.links.current).toBe('?page=1&limit=20&sortBy=id:ASC&filter.home.countCat=$gt:0') - }) - - it('should return result sorted by a virtualcolumn', async () => { - const config: PaginateConfig = { - sortableColumns: ['home.countCat'], - relations: ['home'], - } - const query: PaginateQuery = { - path: '', - sortBy: [['home.countCat', 'ASC']], - } - - const result = await paginate(query, catRepo, config) - const expectedResult = [2, 3, 4, 0, 1].map((i) => { - const ret = clone(cats[i]) - if (i == 0 || i == 1) { - ret.home = clone(catHomes[i]) - ret.home.countCat = cats.filter((cat) => cat.id === ret.home.cat.id).length - delete ret.home.cat - } else { - ret.home = null - } - return ret - }) - - expect(result.data).toStrictEqual(expectedResult) - expect(result.links.current).toBe('?page=1&limit=20&sortBy=home.countCat:ASC') - }) - it('should return result based on or between range filter', async () => { const config: PaginateConfig = { sortableColumns: ['id'], @@ -1922,104 +1884,6 @@ describe('paginate', () => { ) }) - it('should return result sorted and filter by a virtualcolumn in main entity', async () => { - const config: PaginateConfig = { - sortableColumns: ['countCat'], - relations: ['cat'], - filterableColumns: { - countCat: [FilterOperator.GT], - }, - } - const query: PaginateQuery = { - path: '', - filter: { - countCat: '$gt:0', - }, - sortBy: [['countCat', 'ASC']], - } - - const result = await paginate(query, catHomeRepo, config) - - expect(result.data).toStrictEqual([catHomes[0], catHomes[1]]) - expect(result.links.current).toBe('?page=1&limit=20&sortBy=countCat:ASC&filter.countCat=$gt:0') - }) - - it('should return result based on virtualcolumn filter', async () => { - const config: PaginateConfig = { - sortableColumns: ['id'], - filterableColumns: { - 'home.countCat': [FilterOperator.GT], - }, - relations: ['home'], - } - const query: PaginateQuery = { - path: '', - filter: { - 'home.countCat': '$gt:0', - }, - sortBy: [['id', 'ASC']], - } - - const result = await paginate(query, catRepo, config) - const expectedResult = [0, 1].map((i) => { - const ret = Object.assign(clone(cats[i]), { home: Object.assign(clone(catHomes[i])) }) - delete ret.home.cat - return ret - }) - - expect(result.data).toStrictEqual(expectedResult) - expect(result.links.current).toBe('?page=1&limit=20&sortBy=id:ASC&filter.home.countCat=$gt:0') - }) - - it('should return result sorted by a virtualcolumn', async () => { - const config: PaginateConfig = { - sortableColumns: ['home.countCat'], - relations: ['home'], - } - const query: PaginateQuery = { - path: '', - sortBy: [['home.countCat', 'ASC']], - } - - const result = await paginate(query, catRepo, config) - const expectedResult = [2, 3, 4, 0, 1].map((i) => { - const ret = clone(cats[i]) - if (i == 0 || i == 1) { - ret.home = clone(catHomes[i]) - ret.home.countCat = cats.filter((cat) => cat.id === ret.home.cat.id).length - delete ret.home.cat - } else { - ret.home = null - } - return ret - }) - - expect(result.data).toStrictEqual(expectedResult) - expect(result.links.current).toBe('?page=1&limit=20&sortBy=home.countCat:ASC') - }) - - it('should return result sorted and filter by a virtualcolumn in main entity', async () => { - const config: PaginateConfig = { - sortableColumns: ['countCat'], - relations: ['cat'], - filterableColumns: { - countCat: [FilterOperator.GT], - }, - } - const query: PaginateQuery = { - path: '', - filter: { - countCat: '$gt:0', - }, - sortBy: [['countCat', 'ASC']], - } - - const result = await paginate(query, catHomeRepo, config) - - expect(result.data).toStrictEqual([catHomes[0], catHomes[1]]) - expect(result.links.current).toBe('?page=1&limit=20&sortBy=countCat:ASC&filter.countCat=$gt:0') - }) - it("should return all columns if select doesn't contain all primary columns", async () => { const config: PaginateConfig = { sortableColumns: ['id', 'name'], @@ -2572,4 +2436,160 @@ describe('paginate', () => { } }) }) + + if (process.env.DB !== 'postgres') { + describe('should return result based on virtual column', () => { + it('should return result sorted and filter by a virtual column in main entity', async () => { + const config: PaginateConfig = { + sortableColumns: ['countCat'], + relations: ['cat'], + filterableColumns: { + countCat: [FilterOperator.GT], + }, + } + const query: PaginateQuery = { + path: '', + filter: { + countCat: '$gt:0', + }, + sortBy: [['countCat', 'ASC']], + } + + const result = await paginate(query, catHomeRepo, config) + + expect(result.data).toStrictEqual([catHomes[0], catHomes[1]]) + expect(result.links.current).toBe('?page=1&limit=20&sortBy=countCat:ASC&filter.countCat=$gt:0') + }) + + it('should return result based on virtual column filter', async () => { + const config: PaginateConfig = { + sortableColumns: ['id'], + filterableColumns: { + 'home.countCat': [FilterOperator.GT], + }, + relations: ['home'], + } + const query: PaginateQuery = { + path: '', + filter: { + 'home.countCat': '$gt:0', + }, + sortBy: [['id', 'ASC']], + } + + const result = await paginate(query, catRepo, config) + const expectedResult = [0, 1].map((i) => { + const ret = Object.assign(clone(cats[i]), { home: Object.assign(clone(catHomes[i])) }) + delete ret.home.cat + return ret + }) + + expect(result.data).toStrictEqual(expectedResult) + expect(result.links.current).toBe('?page=1&limit=20&sortBy=id:ASC&filter.home.countCat=$gt:0') + }) + + it('should return result sorted by a virtual column', async () => { + const config: PaginateConfig = { + sortableColumns: ['home.countCat'], + relations: ['home'], + } + const query: PaginateQuery = { + path: '', + sortBy: [['home.countCat', 'ASC']], + } + + const result = await paginate(query, catRepo, config) + const expectedResult = [2, 3, 4, 0, 1].map((i) => { + const ret = clone(cats[i]) + if (i == 0 || i == 1) { + ret.home = clone(catHomes[i]) + ret.home.countCat = cats.filter((cat) => cat.id === ret.home.cat.id).length + delete ret.home.cat + } else { + ret.home = null + } + return ret + }) + + expect(result.data).toStrictEqual(expectedResult) + expect(result.links.current).toBe('?page=1&limit=20&sortBy=home.countCat:ASC') + }) + + it('should return result sorted and filter by a virtual column in main entity', async () => { + const config: PaginateConfig = { + sortableColumns: ['countCat'], + relations: ['cat'], + filterableColumns: { + countCat: [FilterOperator.GT], + }, + } + const query: PaginateQuery = { + path: '', + filter: { + countCat: '$gt:0', + }, + sortBy: [['countCat', 'ASC']], + } + + const result = await paginate(query, catHomeRepo, config) + + expect(result.data).toStrictEqual([catHomes[0], catHomes[1]]) + expect(result.links.current).toBe('?page=1&limit=20&sortBy=countCat:ASC&filter.countCat=$gt:0') + }) + + it('should return result based on virtual column filter', async () => { + const config: PaginateConfig = { + sortableColumns: ['id'], + filterableColumns: { + 'home.countCat': [FilterOperator.GT], + }, + relations: ['home'], + } + const query: PaginateQuery = { + path: '', + filter: { + 'home.countCat': '$gt:0', + }, + sortBy: [['id', 'ASC']], + } + + const result = await paginate(query, catRepo, config) + const expectedResult = [0, 1].map((i) => { + const ret = Object.assign(clone(cats[i]), { home: Object.assign(clone(catHomes[i])) }) + delete ret.home.cat + return ret + }) + + expect(result.data).toStrictEqual(expectedResult) + expect(result.links.current).toBe('?page=1&limit=20&sortBy=id:ASC&filter.home.countCat=$gt:0') + }) + + it('should return result sorted by a virtual column', async () => { + const config: PaginateConfig = { + sortableColumns: ['home.countCat'], + relations: ['home'], + } + const query: PaginateQuery = { + path: '', + sortBy: [['home.countCat', 'ASC']], + } + + const result = await paginate(query, catRepo, config) + const expectedResult = [2, 3, 4, 0, 1].map((i) => { + const ret = clone(cats[i]) + if (i == 0 || i == 1) { + ret.home = clone(catHomes[i]) + ret.home.countCat = cats.filter((cat) => cat.id === ret.home.cat.id).length + delete ret.home.cat + } else { + ret.home = null + } + return ret + }) + + expect(result.data).toStrictEqual(expectedResult) + expect(result.links.current).toBe('?page=1&limit=20&sortBy=home.countCat:ASC') + }) + }) + } }) diff --git a/src/paginate.ts b/src/paginate.ts index c25ebe3..31aff01 100644 --- a/src/paginate.ts +++ b/src/paginate.ts @@ -114,11 +114,11 @@ export async function paginate( if (isPaginated) { // Allow user to choose between limit/offset and take/skip. - // However, using take/skip can cause problems with sorting and selections. - if (config.paginationType === PaginationType.TAKE_AND_SKIP) { - queryBuilder.take(limit).skip((page - 1) * limit) - } else { + // However, using limit/offset can cause problems when joining one-to-many etc. + if (config.paginationType === PaginationType.LIMIT_AND_OFFSET) { queryBuilder.limit(limit).offset((page - 1) * limit) + } else { + queryBuilder.take(limit).skip((page - 1) * limit) } }