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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Apply MaxPageSize for PageSize if PageSize > MaxPageSize #12240

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@ public async Task<IActionResult> List(

var routeData = new RouteData(options.RouteValues);

if (_pagerOptions.MaxPageSize > 0 && pager.PageSize > _pagerOptions.MaxPageSize)
Copy link
Member

@MikeAlhayek MikeAlhayek Aug 24, 2022 •
edited
Loading

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; }
     }

{
pager.PageSize = _pagerOptions.MaxPageSize;
Copy link
Contributor

@ns8482e ns8482e Aug 23, 2022 •
edited
Loading

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

Copy link
Member

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.

Copy link
Member

@jtkech jtkech Aug 23, 2022 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

@MikeAlhayek MikeAlhayek Aug 24, 2022 •
edited
Loading

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

@MikeAlhayek MikeAlhayek Aug 24, 2022 •
edited
Loading

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

}
if (_pagerOptions.MaxPagedCount > 0 && pager.PageSize > _pagerOptions.MaxPagedCount)
{
pager.PageSize = _pagerOptions.MaxPagedCount;
Expand Down