fix: throw error if requested page or item count exceeds the Flow limit #3798
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tomivirkki
requested changes
Sep 29, 2022
...flow-parent/vaadin-grid-flow/src/main/resources/META-INF/resources/frontend/gridConnector.js
Outdated
Show resolved
Hide resolved
...flow-parent/vaadin-grid-flow/src/main/resources/META-INF/resources/frontend/gridConnector.js
Outdated
Show resolved
Hide resolved
...flow-parent/vaadin-grid-flow/src/main/resources/META-INF/resources/frontend/gridConnector.js
Outdated
Show resolved
Hide resolved
… the connector Correct length check, add isAllRowsVisible check, carry the code to the server side.
tomivirkki
reviewed
Sep 29, 2022
vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java
Show resolved
Hide resolved
vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java
Outdated
Show resolved
Hide resolved
|
SonarCloud Quality Gate failed.
|
tomivirkki
approved these changes
Oct 4, 2022
There was a problem hiding this comment.
LGTM
Should be merged after the Flow vaadin/flow#14630 is merged.
I think this change would already provide value for users, even before the DataProvider issue in Flow is fixed.
vursen
reviewed
Oct 5, 2022
| | IllegalAccessException | IllegalArgumentException | ||
| | InvocationTargetException e) { | ||
| if (e.getCause() instanceof IllegalArgumentException) { | ||
| throw (IllegalArgumentException) e.getCause(); |
There was a problem hiding this comment.
Did I read it correctly that this just re-throws IllegalArgumentException? Could it be then excluded from the union of exceptions above?
vursen
reviewed
Oct 5, 2022
| if (e.getCause() instanceof IllegalArgumentException) { | ||
| throw (IllegalArgumentException) e.getCause(); | ||
| } | ||
| Assert.fail("Could not call Grid.setRequestedRange"); |
There was a problem hiding this comment.
I guess it would be helpful to know the cause of the failure here. Could we simply not handle these exceptions like here? Does that approach have any negative consequences?
…ter-10-x-pagesize-items
…ter-10-x-pagesize-items
|
SonarCloud Quality Gate failed.
|
ZheSun88
pushed a commit
that referenced
this pull request
Nov 22, 2022
…it (#3798) * fix: throw error if requested page or item count exceeds the Flow limit * fix: correct the if clause and check it in the server side instead of the connector Correct length check, add isAllRowsVisible check, carry the code to the server side. * fix: change the exception message * test: add tests for request variations
ZheSun88
pushed a commit
that referenced
this pull request
Nov 22, 2022
…it (#3798) * fix: throw error if requested page or item count exceeds the Flow limit * fix: correct the if clause and check it in the server side instead of the connector Correct length check, add isAllRowsVisible check, carry the code to the server side. * fix: change the exception message * test: add tests for request variations
ZheSun88
pushed a commit
that referenced
this pull request
Nov 22, 2022
…it (#3798) * fix: throw error if requested page or item count exceeds the Flow limit * fix: correct the if clause and check it in the server side instead of the connector Correct length check, add isAllRowsVisible check, carry the code to the server side. * fix: change the exception message * test: add tests for request variations
ZheSun88
pushed a commit
that referenced
this pull request
Nov 22, 2022
…it (#3798) * fix: throw error if requested page or item count exceeds the Flow limit * fix: correct the if clause and check it in the server side instead of the connector Correct length check, add isAllRowsVisible check, carry the code to the server side. * fix: change the exception message * test: add tests for request variations
ZheSun88
pushed a commit
that referenced
this pull request
Nov 22, 2022
…it (#3798) (#4171) * fix: throw error if requested page or item count exceeds the Flow limit * fix: correct the if clause and check it in the server side instead of the connector Correct length check, add isAllRowsVisible check, carry the code to the server side. * fix: change the exception message * test: add tests for request variations Co-authored-by: Ugur Saglam <106508695+ugur-vaadin@users.noreply.github.com>
ZheSun88
pushed a commit
that referenced
this pull request
Nov 22, 2022
…it (#3798) (#4172) * fix: throw error if requested page or item count exceeds the Flow limit * fix: correct the if clause and check it in the server side instead of the connector Correct length check, add isAllRowsVisible check, carry the code to the server side. * fix: change the exception message * test: add tests for request variations Co-authored-by: Ugur Saglam <106508695+ugur-vaadin@users.noreply.github.com>
ZheSun88
pushed a commit
that referenced
this pull request
Nov 22, 2022
…it (#3798) (#4173) * fix: throw error if requested page or item count exceeds the Flow limit * fix: correct the if clause and check it in the server side instead of the connector Correct length check, add isAllRowsVisible check, carry the code to the server side. * fix: change the exception message * test: add tests for request variations Co-authored-by: Ugur Saglam <106508695+ugur-vaadin@users.noreply.github.com>
ZheSun88
pushed a commit
that referenced
this pull request
Nov 22, 2022
…it (#3798) (#4174) * fix: throw error if requested page or item count exceeds the Flow limit * fix: correct the if clause and check it in the server side instead of the connector Correct length check, add isAllRowsVisible check, carry the code to the server side. * fix: change the exception message * test: add tests for request variations Co-authored-by: Ugur Saglam <106508695+ugur-vaadin@users.noreply.github.com>
|
This ticket/PR has been released with Vaadin 24.0.0.alpha5 and is also targeting the upcoming stable 24.0.0 version. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A recent change in Flow has limited the number of pages in the response to 10. Also, there is an issue to add another constraint to the responses to have a maximum item count of 500. This PR ensures that these constraints are respected in the
Grid. If more pages or items are requested from the, an exception will be thrown.Should be merged after the Flow issue is merged.
Fixes #3708
Type of change
Checklist