-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Apply MaxPageSize for PageSize if PageSize > MaxPageSize #12240
Conversation
@MikeAlhayek I think you worked on this last few days |
@@ -240,6 +240,10 @@ public class AdminController : Controller | |||
|
|||
var routeData = new RouteData(options.RouteValues); | |||
|
|||
if (_pagerOptions.MaxPageSize > 0 && pager.PageSize > _pagerOptions.MaxPageSize) | |||
{ | |||
pager.PageSize = _pagerOptions.MaxPageSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's difference between MaxPageSize and MaxPagedCount? both seems to set PageSize line 245/249
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ns8482e I think MaxPageSize is the max of items per pager page, MaxPagedCount is the max of total items of all pages. So if the max per page is > total max, meaning that in one page you can have more than the allowed total max, it makes sense to limit MaxPageSize to MaxPagedCount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe there are many other places where we don't take MaxPagedCount and or MaxPageSize into account.
Maybe create a PagerOptions.GetPageSize()
method that would check the MaxPagedCount limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now PagerOptions MaxPagedCount is only used in this admin controller and PagerOptions MaxPageSize never used, so makes sense to add it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtkech I actually don't think there is a need for MaxPagedCount
. I understand why it’s used. But if we set it to something like 1000 and we have more than 1000 content items in the database, we’ll limit the viewable items. But there maybe a valid reason that I don’t see.
The MaxPageSize
isn't editable in the UI. so MaxPageSize
should be checked to determine the correct page size.
So the condition should be something like
if (_pagerOptions.MaxPageSize > 0 && pager.PageSize > _pagerOptions.MaxPageSize )
{
pager.PageSize = _pagerOptions.MaxPageSize;
}
But that same check should be applied everywhere in the app. So move that into the PageOption.PageSize getter would apply the check everywhere
But, can't we just remove MaxPagedCount
altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is another situation:
if MaxPagedCount < MaxPageSize, my code defend this situation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move all those logic to PagerOptions.GetPageSize() or property is also good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not suggest to create GetPageSize()
unless you want to replace everywhere PageSize
is currently used with this new function. I would just put that logic in the PageSize getter. Check out the comment I left in the review for example
@@ -240,6 +240,10 @@ public class AdminController : Controller | |||
|
|||
var routeData = new RouteData(options.RouteValues); | |||
|
|||
if (_pagerOptions.MaxPageSize > 0 && pager.PageSize > _pagerOptions.MaxPageSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you remove this check from the controller. Instead, add it to the getter of PagerOption.PageSize so that this check is used everywhere in the app.
Using something like this
private int _pageSize = 10;
public int MaxPageSize { get; set; } = 100;
public int PageSize
{
get
{
return _pageSize > MaxPageSize ? MaxPageSize : _pageSize;
}
set { _pageSize = value; }
}
fix #12239