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.

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

Server should not allow setting values that are not in data provider #6605

Open
vursen opened this issue Sep 3, 2024 · 4 comments
Open

Server should not allow setting values that are not in data provider #6605

vursen opened this issue Sep 3, 2024 · 4 comments
Labels
bug Something isn't working Impact: Low requires new major This would be a breaking change Severity: Minor vaadin-combo-box

Comments

@vursen
Copy link
Contributor

vursen commented Sep 3, 2024

Description

Some components like Select, RadioButtonGroup, CheckboxGroup (and possibly ComboBox), allow setting values that aren't in the data provider and then getting them back through getValue(). After being set, these values are only available on the server, while the web component keeps the previous valid value. Each of these components also has a test that relies on this behavior, for example:

@Test
public void setIdentifierProviderOnId_setItemWithNullId_shouldFailToSelectExistingItemById() {
CustomItem first = new CustomItem(1L, "First");
CustomItem second = new CustomItem(2L, "Second");
CustomItem third = new CustomItem(3L, "Third");
List<CustomItem> items = new ArrayList<>(
Arrays.asList(first, second, third));
Select<CustomItem> select = new Select<>();
SelectListDataView<CustomItem> listDataView = select.setItems(items);
// Setting the following Identifier Provider makes the component
// independent from the CustomItem's equals method implementation:
listDataView.setIdentifierProvider(CustomItem::getId);
select.setValue(new CustomItem(null, "First"));
Assert.assertNull(select.getValue().getId());
}

Expected outcome

The tests assert behavior that shouldn't be supported because it leads to an inconsistent state between server and client and thus is misleading. When trying to set such a value, the component should either throw an exception or revert it and log a warning.

Minimal reproducible example

You can observe the issue in the mentioned test.

Additional context

This issue is why vaadin/flow#19310 had to be reverted despite being a correct solution.

Environment

Vaadin version(s): 24.5 and earlier
OS: Mac OS

Browsers

No response

@vursen vursen changed the title Server should not allow setting values that do not exist in data provider Server should not allow setting values that are not in data provider Sep 3, 2024
@sissbruecker
Copy link
Contributor

For ComboBox there are use-cases where this is useful: #6442 (comment). So basically as an auto-complete where you don't want to add custom values to the list of options.

@stefanuebe
Copy link
Contributor

For ComboBox there are use-cases where this is useful: #6442 (comment). So basically as an auto-complete where you don't want to add custom values to the list of options.

When custom values are explicitly allowed, I would see this as a valid behaviour. But without, I would also rather expect the CB to not allow setting a "custom" value on the server side. Or with other words, it should allow the same input programmatically, that is allowed from the user.

@yuriy-fix yuriy-fix added bug Something isn't working vaadin-combo-box refactor Internal improvement requires new major This would be a breaking change Severity: Minor Impact: Low and removed refactor Internal improvement labels Sep 5, 2024
@web-padawan
Copy link
Member

See also #1546

@stefanuebe
Copy link
Contributor

The multi CB also has this problem. It feels even more "present" due to its displayment as chips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Impact: Low requires new major This would be a breaking change Severity: Minor vaadin-combo-box
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants