"limit=-1 must be "enabled" with maxLimit=-1" as per #393 not working

[jakobwgnr]

Hey!

I think the changes made in #393 are not fully correct.

SHOULD: My expectation would be that "maxLimit=-1" would enable the caller to use no pagination if he wants to with "limit=-1". My expecation would be that the standardfunctionality with e.g. "limit=2" would still work regardless of maxLimit definition

IS: If I set maxLimit=-1 and limit=2 it will currently give me all entries (hence no pagination) which is not the expected behaviour from my POV.

Using: 9.0.1

I think the issue is somewhere here: https://github.com/ppetzold/nestjs-paginate/blob/25fca938c16baeb04055399ce203cf914613afed/src/paginate.ts#L203 It will result in const limit = -1

BR Jakob

[Helveg]

Thanks for the report, could you open a PR that adds a failing test case for this? From there, fixing it will be easier :)

[jakobwgnr]

@Helveg Please see #959

[jakobwgnr]

@Helveg my proposed change would be following - paginate.ts Line 197 ff:

const limit =
        query.limit === PaginationLimit.COUNTER_ONLY
            ? PaginationLimit.COUNTER_ONLY
            : isPaginated === true // would change that here as otherwise will only validate if "isPaginated" is existing. Not 100& sure tho... In the following we may assume that either maxLimit or query.limit may be NO_PAGINATION, but never both
            ? maxLimit === PaginationLimit.NO_PAGINATION
                ? query.limit ?? defaultLimit // in case of maxLimit = NO_PAGINATION either given limit or defaultLimit will be used
                : query.limit === PaginationLimit.NO_PAGINATION
                ? Math.min(defaultLimit, maxLimit) // query.limit not to be considered if NO_PAGINATION
                : Math.min(query.limit ?? defaultLimit, maxLimit) // happy path
            : defaultLimit

Nevertheless it changes the behaviour of the test "should limit to defaultLimit, if limit is differt FROM NO_PAGINATION ecc...."

But for me it would be the more correct logic to apply the limit if it was given in the query, regardless of the config of "maxLimit"

[ppetzold]

yep. definitely also how I had it envisioned :) let's roll with it. breaking changes ftw :) could you add to PR?

[jakobwgnr]

@ppetzold @Helveg Change was added to the PR as proposed.

Summary of change:

• Adapted logic to define limit in paginate.ts
• removed a now invalid test case
• added a new test case to cover the new logic