Fixes: #18150 - Get pagination limit with default 0

[bctiemann]

Fixes: #18150

If limit is not specified in an API list view, the value raises a KeyError which is silently swallowed without processing MAX_PAGE_SIZE, leading to inconsistent pagination results. This change uses .get() with a default of 0 to ensure no exception is raised and the configured page limit is enforced.

Specifically: in our current behavior, we have these scenarios:

    defaults:
    PAGINATE_COUNT: 50
    MAX_PAGE_SIZE: 1000

    limit=0 => MAX_PAGE_SIZE (this can functionally be "unlimited" if MAX_PAGE_SIZE is set very high)
    limit=n => min(n, MAX_PAGE_SIZE)
    limit omitted => PAGINATE_COUNT

With this behavior, if a script hits a list endpoint with no limit, the next URL will be paginated by PAGINATE_COUNT; but if the script follows that link, the next page will use the limit=n case, and the overall page count and next and previous links are paginated by MAX_PAGE_SIZE rather than PAGINATE_COUNT, which would re-flow all the pages and potentially lead to miscounts or other unexpected script behavior. This can be worked around by a script always specifying a limit, but it seems more sensible for omitting limit to be consistent with limit=0.

After this change:

    limit=0 (or omitted) => min(PAGINATE_COUNT, MAX_PAGE_SIZE)
    limit=n => min(n, MAX_PAGE_SIZE)
    No way to get "unlimited" except by setting both limit and MAX_PAGE_SIZE very high

Open question(s):

• Should we support a use case for "unlimited" results, and if so, how? Is the above suggestion good?
• Is limit=0 being a synonym for "unlimited" more intuitive than having limit=0 behave the same as omitting limit?
• Will limit=0 being interpreted as limit=50 be surprising to the user?

NOTE: The fix has changed; leaving the above to preserve the discussion history.

This fix maintains the existing (separate) behaviors of limit=0 and limit omitted. However, it changes the "limit omitted" case to return min(PAGINATE_COUNT, MAX_PAGE_SIZE), i.e. to ensure MAX_PAGE_SIZE is enforced.

[DanSheps]

Documentation on the parameters: https://demo.netbox.dev/static/docs/rest-api/overview/

The maximum number of objects that can be returned is limited by the MAX_PAGE_SIZE configuration parameter, which is 1000 by default. Setting this to 0 or None will remove the maximum limit. An API consumer can then pass ?limit=0 to retrieve all matching objects with a single request.

[DanSheps]

My take:

limit=0: Limit up to MAX_PAGE_SIZE
limit=n: Limit up to min(limit, MAX_PAGE_SIZE)
limit=None: Limit up to min(PAGINATE_COUNT, MAX_PAGE_SIZE)

[jeremystretch]

Can this bug be resolved simply by checking that the configured MAX_PAGE_SIZE is greater than or equal to PAGINATE_COUNT?

[bctiemann]

@jeremystretch This update leaves the original logic intact but just ensures that MAX_PAGE_SIZE is at least PAGINATE_COUNT (or 0, meaning ignored). The unit tests now cover both cases:

• 0 < MAX_PAGE_SIZE < PAGINATE_COUNT < num_records
• 0 < PAGINATE_COUNT < MAX_PAGE_SIZE < num_records

[jeremystretch]

@bctiemann I was thinking we could just compare MAX_PAGE_SIZE to PAGINATE_COUNT in settings.py and raise an ImproerlyConfigured exception if it's larger. Do you think that works?

[bctiemann]

@jeremystretch I think you're right; after working through the use cases I think this does solve it. Note this also puts both variables explicitly into settings.py so their default values will be easier to find than by relying on their DRF library defaults.

[bctiemann]

Also note that I'm leaving the unit test changes in place because with @override_settings they exercise the logic without getting stopped by the settings.py validation.

[bctiemann]

Reverting to the previous solution (validating the two variables against each other in pagination.py), with unit tests handling both cases accordingly