Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add page size limit #412

Merged
merged 11 commits into from May 10, 2021
Merged

Add page size limit #412

merged 11 commits into from May 10, 2021

Conversation

pulpn0ir
Copy link
Contributor

Allows setting a page size limit when creating a DatabaseContext to prevent server overloads.

If the requested page size exceeds the limit, the response page is capped to the defined limit. The default value is nil, allowing for an unlimited page size.

This PR relates to vapor/fluent#715

@siemensikkema siemensikkema added the semver-minor Contains new APIs label Jan 12, 2021
@siemensikkema
Copy link
Member

This PR is the continuation of #406

Base automatically changed from master to main March 12, 2021 02:50
@Joannis
Copy link
Member

Joannis commented Mar 22, 2021

@0xTim any reason this wasn't merged (yet)?

@0xTim
Copy link
Member

0xTim commented Mar 23, 2021

@siemensikkema I was waiting for you to give the final OK for this, this good to merge? Don't merge just yet if so, just approve. I'll be sorting out the release bot in a couple of hours so we can wait for that

@Joannis
Copy link
Member

Joannis commented Mar 23, 2021

After a small discussion with @0xTim it seems he wants at least @gwynne or @siemensikkema to look into this as well.

gwynne
gwynne previously requested changes Mar 23, 2021
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly fine to me, but I'd like the unit tests to include a test for a page other than the first (in particular, a test that pages after the first are at the correct offset relative to the given limits).

@siemensikkema
Copy link
Member

@gwynne good idea. I changed the test slightly to ensure this is covered. I chose to reduce the page limit so I could request page 2 while still getting a full page. The other choice I could have made would have been to add one more row to the test output.

@siemensikkema siemensikkema dismissed gwynne’s stale review May 10, 2021 06:45

accepted as per discussion on Discord

@siemensikkema siemensikkema merged commit ea23e12 into vapor:main May 10, 2021
@VaporBot
Copy link

These changes are now available in 1.12.0

@siemensikkema siemensikkema deleted the pagination-max-per branch May 10, 2021 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Contains new APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants