-
Notifications
You must be signed in to change notification settings - Fork 185
fix: Limit the requested pages to a given threshold #12003
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
Conversation
Limits the number of requested items from backend to a given threshold of ten pages.
@@ -65,6 +65,7 @@ | |||
public static final int DEFAULT_PAGE_INCREASE_COUNT = 4; | |||
|
|||
private static final int DEFAULT_PAGE_SIZE = 50; | |||
private static final int MAXIMUM_ALLOWED_ITEMS_FACTOR = 10; |
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.
Factor ? I would simply call it pages e.g. maximum allowed pages since it seems to correspond to that, so why not just name it for that ?
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.
Agree, it's better no name it maximum allowed pages.
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.
done
@@ -317,6 +318,14 @@ public DataCommunicator(DataGenerator<T> dataGenerator, | |||
* the end of the requested range | |||
*/ | |||
public void setRequestedRange(int start, int length) { | |||
final int maximumAllowedItems = getMaximumAllowedItems(); | |||
if (length > maximumAllowedItems) { | |||
throw new IllegalStateException(String.format( |
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.
Why throwing and breaking the UI ? What happens if the length is limited to maximum allowed ?
IIRC, the grid will simply request again if it needs more (?).
And the server side will anyway keep fetching for more until less are returned than what was requested, or until the size has been reached.
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.
Yes, you're right. Better to cut/limit the requested items, than to throw. So I'll just warn about too many items requested and cut the length to acceptable.
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.
done
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 think that was a wrong decision. Otherwise we would likely have seen bug much much earlier.
SonarQube analysis reported 3 issues
|
Limits the number of requested items from backend to a given threshold of ten pages.
Hi @mshabarov and @pleku, when i performed cherry-pick to this commit to 2.7, i have encountered the following issue. Can you take a look and pick it manually? |
Hi @mshabarov and @pleku, when i performed cherry-pick to this commit to 1.0, i have encountered the following issue. Can you take a look and pick it manually? |
Limits the number of requested items from backend to a given threshold of ten pages. (cherry picked from commit 0ec90bf)
Limits the number of requested items from backend to a given threshold of ten pages. (cherry picked from commit 0ec90bf)
Description
Limits the number of requested items from backend to a given threshold of ten pages.
Depends on vaadin/web-components#2801 and web-components new release.
Type of change
Checklist
Additional for
Feature
type of change