Avoid obsolete filter syncing [2 days]#7135
Conversation
|
|
One failed test. Which is kind of more a timing issue. In a mode when the popup is configured NOT to be automatically openend, the previous test triggered the filtering more eagerly. Don't know what would be the "correct" behaviour if #6863 gets fixex. Could be fixed either by adding a wait/timeout to the IT or to keep syncing the filter also via event, but make that lazy. Or changing client side so taht by making open event also contain the filter or so that connector calls some method instead (like it does with regular filtering). |
|
Let's consider combining this PR with #7133 so that the context is available in one place. We should also proceed with adding tests. @mstahv, would you be able to add tests, or would you like us to handle them? Also, please specify your compilation issues so we can help resolve them. It might be potentially a small breaking change, since |
|
I'd keep these separatly. This is more like a bugfix or performance improveiment and the other is technically "a feature", so it will probably be require a minor version (and now that 24.7 was just "closed", it will take a looong time). Would be great if you can tackle the possible new tests. As I don't work with selenium daily, I don't have good ideas to how to automatically assert extra requests are made. |
|
|
That appears to be obsolete as the filter is anyways sent to the @ClientCallable method when the filtering should actually be done.
a125906 to
a055bc0
Compare
That appears to be obsolete as the filter is anyways sent to the @ClientCallable method when the filtering should actually be done.
a055bc0 to
29d5749
Compare
|
@vursen Are you going to work on this further? |
No, I'm tied up with other tasks right now. |
Works locally, but maybe now debouncing causes issue in CI
…ow-components into bugix/avoid-many-obsolete-xhrs
|
Checked in the project now builds on my local workstataion and tests pass there. Pushed a blind wait that might happen now as the filtering is debounced. Let's see if that helps. Please explain your concern about ComboBoxDataController. That looks very complex class indeed, but didn't easily see how this could affect that. |
|
For example, consider this scenario:
It would help if ComboBoxDataController was the only place handling filters. |
|
Do you have that available somewhere as regression test? I can't see any failing. Or the original thing that the developer/user is trying to do in that scenario? To me it sounds bit odd the filter is ever taken into account with the dataview (although I haven't really understood what is the purpose of the dataview in general either). The fix is probably easy, but maybe there is some more fundamental isseu in the current implementation 🤷♂️ |
yuriy-fix
changed the title
|
/code-review |
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 3 comments and collected 2 findings below that could not be attached to the diff.
| Finding | |
|---|---|
First open of a non-auto-open lazy combo box fetches with a stale/null filter because getFilter() no longer reflects the typed text. |
|
setDataProvider seeds the initial filter from getFilter(), which is stale in client-side filtering and after the popup closes. |
|
| 🧹 | Test hand-builds a WebDriverWait instead of the inherited waitUntil helper used throughout this module. |
| 👀 | lastKnownFilter is a hand-maintained shadow of the filter that must be poked at every client entry point, and already misses paths on day one. |
| 🧹 | setFilter() is left half-finished: a now-dead filter property write, stale javadoc, and a dated speculative comment. |
null filter because getFilter() no longer reflects the typed text.
A lazy ComboBox with setAutoOpen(false) (the reviewed LazyComboBoxFilterPage): the user types "1", then opens the popup. The opened change runs executeRegistration, which calls setViewportRange(0, pageSize, comboBox.getFilter()). With @Synchronize removed, getFilter() returns lastKnownFilter, which is still null here — the client only sends the filter through the debounced setViewportRange RPC, which has not fired yet. So the first server query runs with the wrong filter ("Filter: <undefined> Count: 99") and only a later round-trip corrects it to "Filter: 1 Count: 10".
This is a behavior regression: before the change the first query already carried the filter. It is exactly why this PR had to weaken the IT from an immediate Assert.assertTrue to a 2-second wait.until. It also adds a wrong initial fetch rather than reducing round-trips. The fix should feed the current client filter into this path (e.g. keep syncing it, or defer the fetch until the client's filtered request arrives).
ComboBoxDataController.java:609 · correctness · confirmed
setDataProvider seeds the initial filter from getFilter(), which is stale in client-side filtering and after the popup closes.
lastKnownFilter is only written by the deprecated setFilter() and by the setViewportRange RPC, and it is never cleared. In client-side filtering mode (item count fits one page, _clientSideFilter true) the connector filters locally and returns without calling setViewportRange (comboBoxConnector.js:70-85), so lastKnownFilter never tracks the typed text. Closing the popup also resets the controller's lastFilter but leaves lastKnownFilter holding the old value.
If the app then calls setItems(...)/setDataProvider(...), line 547 seeds the new DataCommunicator with convertOrNull.apply(comboBox.getFilter()) — a filter that no longer matches what the user sees, so the initial page is wrong until the next client request corrects it. Before the change the synced filter property always held the current value. Confirming this needs a run with a swapped data provider while a client-side filter is active.
ComboBoxDataController.java:547 · correctness · plausible
| Assert.assertTrue(query.getText().contains("Filter: 1")); | ||
| Assert.assertTrue(query.getText().contains("Count: 10")); | ||
|
|
||
| WebDriverWait wait = new WebDriverWait(driver, |
There was a problem hiding this comment.
🧹 Test hand-builds a WebDriverWait instead of the inherited waitUntil helper used throughout this module.
AbstractComponentIT (via com.vaadin.flow.testutil.AbstractComponentIT) already provides waitUntil(ExpectedCondition), and it is the established pattern across this IT module (ComboBoxClearIT, DataProviderIT, ClientSideFilterIT, and others). The new code instead constructs new WebDriverWait(driver, Duration.of(2, ChronoUnit.SECONDS)) and pulls in three extra imports (Duration, ChronoUnit, WebDriverWait).
The cost is inconsistent timeout/polling behavior and boilerplate the shared helper exists to avoid. Replace with waitUntil(ExpectedConditions.textToBePresentInElement(query, "Filter: 1 Count: 10")).
LazyComboBoxFilterIT.java:43 · reuse · confirmed
| HasValidationProperties, HasValidator<TValue>, HasPlaceholder { | ||
| private static final int DEFAULT_FILTER_TIMEOUT = 500; | ||
|
|
||
| private String lastKnownFilter; |
There was a problem hiding this comment.
👀 lastKnownFilter is a hand-maintained shadow of the filter that must be poked at every client entry point, and already misses paths on day one.
The removed @Synchronize was a single source of truth kept current by Flow on every filter-changed event. The replacement is a field updated in only two sites (setFilter() and the setViewportRange RPC) and never cleared — so it already fails to track the client-side-filter path and the popup-close path (see the two correctness findings). ComboBoxDataController also already tracks the requested filter in its own lastFilter field.
The cost is fragility: any future client path that changes the filter, or any new server reader of getFilter(), must remember to update this private field, with nothing enforcing it. A deeper fix would adjust when the client sends the filter (dropping the redundant per-keystroke sync) rather than adding a parallel field that drifts out of sync.
ComboBoxBase.java:102 · altitude · plausible
| * the String value to set | ||
| */ | ||
| @Deprecated | ||
| protected void setFilter(String filter) { |
There was a problem hiding this comment.
🧹 setFilter() is left half-finished: a now-dead filter property write, stale javadoc, and a dated speculative comment.
With @Synchronize removed and getFilter() reading lastKnownFilter, nothing reads the filter element property anymore (no getProperty("filter") remains), so getElement().setProperty("filter", ...) on line 368 is an inert DOM write. The javadoc still says Setter is only required to allow using @Synchronize — the very annotation this PR deletes — and the body carries // MT 20250-02-21 I assume client side never uses this, probably the whole method is obsolete (note the typo year 20250).
The cost is that a future maintainer cannot tell what the method is for or whether it is safe to delete: the javadoc, the inline comment, and the @Deprecated (which has no @deprecated tag or replacement) give three conflicting signals. Resolve it here — confirm the client ignores the property and remove the method or the dead write, and replace the scratch comment with a real deprecation note.
ComboBoxBase.java:364 · simplification · confirmed
|
We spent some time investigating the proposed change and identified several cases that are not currently covered. Addressing them would require significant additional work to complete the solution. For now, we've decided not to proceed with the change and will close the PR. Please feel free to reopen it if you would like to address the review comments. |
5 participants
That appears to be obsolete as the filter is anyways sent to the @ClientCallable method when the filtering should actually be done. After this change there is a lot less XHRs/server-visits happening aka reduces "chattiness" (although there still is some that I don't really know what they are related to (opened changed and some "confirm" requests).
Opening as draft as somebody involved in the current implementation really should look into this (and couldn't run the tests locally).
See also #6863 and #7133