ComboBoxLazyDataView does not honour setIdentifierProvider() properly -- items show twice without overridden equals/hashCode

[enver-haase]

Description of the bug

When I say multiSelectComboBox.getLazyDataView().setIdentifierProvider( Item::getPrimaryKey )

then I still get duplicates in my lazily loaded data.

When I implement equals and hashCode, everything works as expected.

I do wonder if it's a documentation issue, but one would assume that this is exactly why he setIdentifierProvider is there in the first place, so that equals/hashCode will not be used.

Expected behavior

I should not need to implement hasCode/equals in order to not have duplicates.

Minimal reproducible example

[will follow]

Versions

• Vaadin / Flow version: 24.3.10
• Java version: n/a
• OS version: n/a
• Browser version (if applicable): n/a
• Application Server (if applicable): n/a
• IDE (if applicable): n/a

[enver-haase]

package com.example.application.views.helloworld;

import com.example.application.views.MainLayout;
import com.vaadin.flow.component.combobox.ComboBox;
import com.vaadin.flow.component.combobox.MultiSelectComboBox;
import com.vaadin.flow.component.orderedlayout.HorizontalLayout;
import com.vaadin.flow.function.SerializableFunction;
import com.vaadin.flow.router.PageTitle;
import com.vaadin.flow.router.Route;
import com.vaadin.flow.router.RouteAlias;

import java.util.stream.Stream;

@PageTitle("Hello World")
@Route(value = "", layout = MainLayout.class)
@RouteAlias(value = "", layout = MainLayout.class)
public class HelloWorldView extends HorizontalLayout {

    static class Foo {
        private final String privateString;

        Foo(String str) {
            this.privateString = str;
        }

        public String toString() {
            return privateString;
        }

//        @Override
//        public boolean equals(Object obj) {
//            return toString().equals(obj.toString());
//        }
//
//        @Override
//        public int hashCode() {
//            return 0;
//        }
    }

    public HelloWorldView() {
        MultiSelectComboBox<Foo> comboBox = new MultiSelectComboBox<>();
        ComboBox.FetchItemsCallback<Foo> fb = (filter, offset, limit) -> {
            Foo[] foos = {new Foo("abc"), new Foo("def"), new Foo("abc")};
            return Stream.of(foos);
        };
        comboBox.setDataProvider(fb, (SerializableFunction<String, Integer>) s -> 3);
        comboBox.getLazyDataView().setIdentifierProvider(Foo::toString);
        add(comboBox);
    }

}

[mshabarov]

The issue was triaged and currently added to the backlog priority queue for further investigation.

[tepi]

The issue was assigned to a developer and is currently being investigated

[tepi]

Looks like this call comboBox.setDataProvider(fb, (SerializableFunction<String, Integer>) s -> 3); causes the ComboBox to eventually call com.vaadin.flow.data.provider.CallbackDataProvider#CallbackDataProvider(com.vaadin.flow.data.provider.CallbackDataProvider.FetchCallback<T,F>, com.vaadin.flow.data.provider.CallbackDataProvider.CountCallback<T,F>) which uses t -> t as the id provider.

Then, when the actually wanted id provider is set on the next line in the view code, it is never propagated to the CallBackDataProvider which thus keeps using the default id provider. However looks like the custom id provider is used for at least item selection so the state seems a bit invalid at that point.

I'll see what could be the best way to fix this, and also if we have similar issue in other data providers.

[tepi]

Well here's the reason: https://github.com/vaadin/flow-components/blob/9d6a5672b70699a34c0f475244019fe831d1d249/vaadin-combo-box-flow-parent/vaadin-combo-box-flow/src/main/java/com/vaadin/flow/component/combobox/MultiSelectComboBox.java#L128-L133

MultiSelectComboBox does listen to IdentifierProviderChangeEvent but the new provider is only passed to selection model, nothing is done for data provider.

[tepi]

This case seems quite complex to fix properly. In the meantime @enver-haase could you try a workaround by creating your CallBackDataProvider using the constructor that also takes id provider as a parameter, something like this (very simplified and ignores filtering):

comboBox.setItems(new CallbackDataProvider<Foo, String>(query -> {
            Foo[] foos = {new Foo("abc"), new Foo("def"), new Foo("abc")};
            return Stream.of(foos).skip(query.getOffset()).limit(query.getLimit());
        }, query -> 3, Foo::toString));

[enver-haase]

Thank you, but a workaround is already in place.

[tepi]

Alright. Just to confirm, is the workaround the one above or something else?

[enver-haase]

IIRC the core of the workaround is (no access to the source code): "extend data provider and override getId() method" and "This was the original API to do this, and works also with Vaadin 14"

[tepi]

Ok, thanks. Yes, that was/is the original way to do that, the DataView stuff was added much later in 2020 or so.

[mshabarov]

@enver-haase what was the original issue: having duplicates in the list or a confusion with the selection, when you select one item, but another item (duplicate) isn't selected ?

Even if I override equals and hashCode or override data provider's getId(), I do have duplicates in the dropdown always. This works the same way also for ComboBox and Grid.

[enver-haase]

Oh, that's interesting.

I would have thought that an equal second item should not be shown, so I consider the fact that it is shown twice even if overriding equals()/hashCode() wrong. Which was not part of my bug report, but does give rise to another.

Let alone, the identical (which is stronger than equal) item, that should definitely not be listed more than one time. That must be wrong too. [[[Maybe it should have been called 'setEqualityProvider()' as a side-thought? If equality was meant and not identity?!]]] This was my original issue.

if that would be ensured, so that there are no duplicates in the list, then the different selection behaviour between the duplicates (select only one or select all of them - depending on setIdentityProvider or equals&hashCode) would not matter. You are right, I think originally I did not realize having duplicates was wrong in the first place and wanted the selection behaviour aligned. But on a second thought, that's stupid, is it not?

[tepi]

Your data provider should ensure there are no duplicate items returned (e.g. your database probably does not have multiple rows with the same unique id). In the code example here, the size provider returns 3 so the component has no other option than to show three items, even if they are all considered equal to each other.

That said, there was also a bug here - selection and key mapping were not using the same id provider, which is fixed by https://github.com/vaadin/flow-components/pull/6632.

[enver-haase]

Hmmm... nitpicking here... it's not a DataProvider, nor would I see this documented in FetchItemsCallback or its fetchItems(String, int, int) method. But maybe it could be considered logical and to-be-assumed.

Right, so if the selection problem is now solved, can we close this report then?

[tepi]

Hmmm... nitpicking here... it's not a DataProvider, nor would I see this documented in FetchItemsCallback or its fetchItems(String, int, int) method. But maybe it could be considered logical and to-be-assumed.

If it's not there we should document it. In any case, results will be pretty random if you provide (id-wise) duplicate items to grid/combobox by any means.