drop take and skip (#545)

BREAKING CHANGE: We remove `paginationType` from config and always use limit/offset. Most current issues seem to be related take/skip. The alleged side effects of limit/offset on many-to-many relations seem to not exist anymore as coverage shows.
This commit is contained in:
Philipp 2023-03-17 21:39:13 +01:00 committed by GitHub
parent 55a738c592
commit c42fde65c3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 9 additions and 37 deletions

View File

@ -254,20 +254,6 @@ const paginateConfig: PaginateConfig<CatEntity> {
*/ */
origin: 'http://cats.example', origin: 'http://cats.example',
/**
* Required: false
* Type: string
* Description: Allow user to choose between limit/offset and take/skip.
* Default: PaginationType.TAKE_AND_SKIP
*
* However, using limit/offset can return unexpected results.
* For more information see:
* [#477](https://github.com/ppetzold/nestjs-paginate/issues/477)
* [#4742](https://github.com/typeorm/typeorm/issues/4742)
* [#5670](https://github.com/typeorm/typeorm/issues/5670)
*/
paginationType: PaginationType.LIMIT_AND_OFFSET,
/** /**
* Required: false * Required: false
* Type: boolean * Type: boolean

View File

@ -548,9 +548,9 @@ describe('paginate', () => {
const result = await paginate<CatEntity>(query, catRepo, config) const result = await paginate<CatEntity>(query, catRepo, config)
expect(result.meta.search).toStrictEqual('Mouse') expect(result.meta.search).toStrictEqual('Mouse')
const toy = clone(catToys[2]) const toy = clone(catToys[1])
delete toy.cat delete toy.cat
const toy2 = clone(catToys[1]) const toy2 = clone(catToys[2])
delete toy2.cat delete toy2.cat
expect(result.data).toStrictEqual([Object.assign(clone(cats[0]), { toys: [toy2, toy] })]) expect(result.data).toStrictEqual([Object.assign(clone(cats[0]), { toys: [toy2, toy] })])
@ -942,7 +942,7 @@ describe('paginate', () => {
delete copy.cat delete copy.cat
return copy return copy
}) })
copyCats[0].toys = [copyToys[0], copyToys[1], copyToys[2]] copyCats[0].toys = [copyToys[0], copyToys[2], copyToys[1]]
copyCats[1].toys = [copyToys[3]] copyCats[1].toys = [copyToys[3]]
const orderedCats = [copyCats[3], copyCats[1], copyCats[2], copyCats[0], copyCats[4]] const orderedCats = [copyCats[3], copyCats[1], copyCats[2], copyCats[0], copyCats[4]]
@ -1967,6 +1967,7 @@ describe('paginate', () => {
await catRepo.softDelete({ id: cats[0].id }) await catRepo.softDelete({ id: cats[0].id })
const result = await paginate<CatEntity>(query, catRepo, config) const result = await paginate<CatEntity>(query, catRepo, config)
expect(result.meta.totalItems).toBe(cats.length) expect(result.meta.totalItems).toBe(cats.length)
await catRepo.restore({ id: cats[0].id })
}) })
it('should return only undeleted items', async () => { it('should return only undeleted items', async () => {
@ -1980,6 +1981,7 @@ describe('paginate', () => {
await catRepo.softDelete({ id: cats[0].id }) await catRepo.softDelete({ id: cats[0].id })
const result = await paginate<CatEntity>(query, catRepo, config) const result = await paginate<CatEntity>(query, catRepo, config)
expect(result.meta.totalItems).toBe(cats.length - 1) expect(result.meta.totalItems).toBe(cats.length - 1)
await catRepo.restore({ id: cats[0].id })
}) })
it('should return the specified columns only', async () => { it('should return the specified columns only', async () => {
@ -2064,7 +2066,9 @@ describe('paginate', () => {
const result = await paginate<CatEntity>(query, catRepo, config) const result = await paginate<CatEntity>(query, catRepo, config)
expect(result.data.length).toBe(4) expect(result.meta.totalItems).toBe(5)
expect(result.data.length).toBe(5)
expect(result.data[0].friends.length).toBe(4)
}) })
it('should return eager relations when set the property `loadEagerRelations` as true', async () => { it('should return eager relations when set the property `loadEagerRelations` as true', async () => {

View File

@ -54,11 +54,6 @@ export class Paginated<T> {
} }
} }
export enum PaginationType {
LIMIT_AND_OFFSET = 'limit',
TAKE_AND_SKIP = 'take',
}
export interface PaginateConfig<T> { export interface PaginateConfig<T> {
relations?: FindOptionsRelations<T> | RelationColumn<T>[] relations?: FindOptionsRelations<T> | RelationColumn<T>[]
sortableColumns: Column<T>[] sortableColumns: Column<T>[]
@ -76,13 +71,11 @@ export interface PaginateConfig<T> {
withDeleted?: boolean withDeleted?: boolean
relativePath?: boolean relativePath?: boolean
origin?: string origin?: string
paginationType?: PaginationType
} }
export const DEFAULT_MAX_LIMIT = 100 export const DEFAULT_MAX_LIMIT = 100
export const DEFAULT_LIMIT = 20 export const DEFAULT_LIMIT = 20
export const NO_PAGINATION = 0 export const NO_PAGINATION = 0
export const DEFAULT_PAGINATE_TYPE = PaginationType.TAKE_AND_SKIP
export async function paginate<T extends ObjectLiteral>( export async function paginate<T extends ObjectLiteral>(
query: PaginateQuery, query: PaginateQuery,
@ -94,7 +87,6 @@ export async function paginate<T extends ObjectLiteral>(
const defaultLimit = config.defaultLimit || DEFAULT_LIMIT const defaultLimit = config.defaultLimit || DEFAULT_LIMIT
const maxLimit = positiveNumberOrDefault(config.maxLimit, DEFAULT_MAX_LIMIT) const maxLimit = positiveNumberOrDefault(config.maxLimit, DEFAULT_MAX_LIMIT)
const queryLimit = positiveNumberOrDefault(query.limit, defaultLimit) const queryLimit = positiveNumberOrDefault(query.limit, defaultLimit)
const paginationType = config.paginationType || DEFAULT_PAGINATE_TYPE
const isPaginated = !(queryLimit === NO_PAGINATION && maxLimit === NO_PAGINATION) const isPaginated = !(queryLimit === NO_PAGINATION && maxLimit === NO_PAGINATION)
@ -151,17 +143,7 @@ export async function paginate<T extends ObjectLiteral>(
} }
if (isPaginated) { if (isPaginated) {
// Allow user to choose between limit/offset and take/skip. queryBuilder.limit(limit).offset((page - 1) * limit)
// However, using limit/offset can return unexpected results.
// For more information see:
// [#477](https://github.com/ppetzold/nestjs-paginate/issues/477)
// [#4742](https://github.com/typeorm/typeorm/issues/4742)
// [#5670](https://github.com/typeorm/typeorm/issues/5670)
if (paginationType === PaginationType.LIMIT_AND_OFFSET) {
queryBuilder.limit(limit).offset((page - 1) * limit)
} else {
queryBuilder.take(limit).skip((page - 1) * limit)
}
} }
if (config.relations) { if (config.relations) {