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 #715

Merged
merged 12 commits into from May 24, 2021
Merged

Add page size limit #715

merged 12 commits into from May 24, 2021

Conversation

pulpn0ir
Copy link
Contributor

@pulpn0ir pulpn0ir commented Jan 5, 2021

Adds a means to prevent server overloads by limiting the maximum amount of elements returned in a single page.

Application and Request have been extended with setters for page size limits. Set limits using the newly introduced PageLimit type.

// Set page size limit for the current request. Default is `nil`, which means no limit. 
request.fluent.pagination.pageSizeLimit = 3

// Setting the request-level limit to `nil` will cause the application-level limit to be used instead. 
request.fluent.pagination.pageSizeLimit = nil

// Use `.noLimit` if you intend to override a limit set on application level. 
request.fluent.pagination.pageSizeLimit = .noLimit

// Set application-wide page size limit. Default is `nil`, which means no limit. 
application.fluent.pagination.pageSizeLimit = 3

This PR depends on: vapor/fluent-kit#412

Copy link
Member

@siemensikkema siemensikkema left a comment

Choose a reason for hiding this comment

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

Looks good! I think this is going to be a very useful feature. I added some remarks and ideas ..

Sources/Fluent/Fluent+Pagination.swift Outdated Show resolved Hide resolved
Tests/FluentTests/PaginationTests.swift Outdated Show resolved Hide resolved
Sources/Fluent/FluentProvider.swift Outdated Show resolved Hide resolved
Sources/Fluent/Fluent+Pagination.swift Outdated Show resolved Hide resolved
Sources/Fluent/FluentProvider.swift Outdated Show resolved Hide resolved
@pulpn0ir pulpn0ir changed the title Page Size Limitation Add page size limit Jan 12, 2021
@siemensikkema siemensikkema added the semver-minor Contains new APIs label Jan 12, 2021
storage[AppPaginationKey.self]?.pageSizeLimit
}
nonmutating set {
storage[AppPaginationKey.self] = .init(pageSizeLimit: newValue)
Copy link
Member

Choose a reason for hiding this comment

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

@gwynne do we need to wrap this in a lock?

Copy link
Member

Choose a reason for hiding this comment

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

Since this is a global setting, set once during startup, I'd avoid diving into that too much.

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

The tests are failing due to unrelated changes.

  • Swift nightly 5.2: error: CacheTests.testCacheSet : threw error "invalidValue("bar", Swift.EncodingError.Context(codingPath: [], debugDescription: "Top-level String encoded as string JSON fragment.", underlyingError: nil))" There seems to be a regression in JSON encoding here. Strangely, the nightly 5.3 builds fine.
  • Swift nightly: a compiler crash during type checking of EnumBuilder.swift

Besides reporting these I think we should update CI to not depend on nightly builds. @gwynne @0xTim agreed?

@0xTim
Copy link
Member

0xTim commented May 11, 2021

@siemensikkema agree, for most things we should probably be testing with lowest supported version (5.2 release) and current release (5.4). It might be beneficial to test against latest master to catch Swift regressions but there's no point testing against a nightly build of a released Swift version

@siemensikkema siemensikkema requested a review from 0xTim May 21, 2021 10:35
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

LGTM

@siemensikkema siemensikkema merged commit c67c33a into vapor:main May 24, 2021
@VaporBot
Copy link

These changes are now available in 4.3.0

@siemensikkema siemensikkema deleted the feature/per-page-limit branch June 3, 2021 07:33
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

5 participants