From fb3a898b0731e1b7434e6d9f4d56020fa7dea037 Mon Sep 17 00:00:00 2001 From: Zhe Sun <31067185+ZheSun88@users.noreply.github.com> Date: Wed, 21 Sep 2022 16:20:36 +0300 Subject: [PATCH] fix: Use KeyMapper by default to generate unique keys (#13883) (#14596) Delegates to regular KeyMapper whenever the unique key provider is not present and a unique key is needed for a row. Part-of: https://github.com/vaadin/flow-components/issues/3156 (cherry picked from commit 570244d9afadfd185689f9ec0d3ef40e985c02d2) Co-authored-by: Mikhail Shabarov --- .../HierarchicalDataCommunicator.java | 67 +++++++++++-------- .../HierarchicalCommunicatorDataTest.java | 56 +++++++++++++++- 2 files changed, 94 insertions(+), 29 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java index 12c89c6b8ee..749a71ab24e 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java @@ -67,28 +67,6 @@ public class HierarchicalDataCommunicator extends DataCommunicator { private final Map> dataControllers = new HashMap<>(); - private KeyMapper uniqueKeyMapper = new KeyMapper() { - - private T object; - - @Override - public String key(T o) { - this.object = o; - try { - return super.key(o); - } finally { - this.object = null; - } - } - - @Override - protected String createKey() { - return Optional.ofNullable(uniqueKeyProviderSupplier.get()) - .map(provider -> provider.apply(object)) - .orElse(super.createKey()); - } - }; - /** * Construct a new hierarchical data communicator backed by a * {@link TreeDataProvider}. @@ -115,16 +93,16 @@ public HierarchicalDataCommunicator(CompositeDataGenerator dataGenerator, this.stateNode = stateNode; this.uniqueKeyProviderSupplier = uniqueKeyProviderSupplier; - setKeyMapper(uniqueKeyMapper); + KeyMapperWrapper keyMapperWrapper = new KeyMapperWrapper<>(); + setKeyMapper(keyMapperWrapper); dataGenerator.addDataGenerator(this::generateTreeData); setDataProvider(new TreeDataProvider<>(new TreeData<>()), null); } private void generateTreeData(T item, JsonObject jsonObject) { - Optional.ofNullable(getParentItem(item)) - .ifPresent(parent -> jsonObject.put("parentUniqueKey", - uniqueKeyProviderSupplier.get().apply(parent))); + Optional.ofNullable(getParentItem(item)).ifPresent(parent -> jsonObject + .put("parentUniqueKey", getKeyMapper().key(parent))); } private void requestFlush(HierarchicalUpdate update) { @@ -184,7 +162,7 @@ protected void handleDataRefreshEvent(DataChangeEvent.DataRefreshEvent event) if (event.isRefreshChildren()) { T item = event.getItem(); if (isExpanded(item)) { - String parentKey = uniqueKeyProviderSupplier.get().apply(item); + String parentKey = getKeyMapper().key(item); if (!dataControllers.containsKey(parentKey)) { setParentRequestedRange(0, mapper.countChildItems(item), item); @@ -207,7 +185,7 @@ public Stream fetchFromProvider(int offset, int limit) { } public void setParentRequestedRange(int start, int length, T parentItem) { - String parentKey = uniqueKeyProviderSupplier.get().apply(parentItem); + String parentKey = getKeyMapper().key(parentItem); HierarchicalCommunicationController controller = dataControllers .computeIfAbsent(parentKey, @@ -563,6 +541,39 @@ private JsonValue generateJsonForExpandedOrCollapsedItem(T item) { return json; } + /** + * KeyMapper extension delegating row key creation to the + * uniqueKeyProviderSupplier passed to the hierarchical data + * communicator constructor from the component. + *

+ * If uniqueKeyProviderSupplier is not present, this class uses + * {@link KeyMapper#createKey()} for key creation. + * + * @param + * the bean type + */ + private class KeyMapperWrapper extends KeyMapper { + + private T object; + + @Override + public String key(T o) { + this.object = o; + try { + return super.key(o); + } finally { + this.object = null; + } + } + + @Override + protected String createKey() { + return Optional.ofNullable(uniqueKeyProviderSupplier.get()) + .map(provider -> provider.apply(object)) + .orElse(super.createKey()); + } + } + private Collection getHierarchyMapperExpandedItems() { HierarchyMapper hierarchyMapper = getHierarchyMapper(); if (!hierarchyMapper.hasExpandedItems()) { diff --git a/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicatorDataTest.java b/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicatorDataTest.java index 0ba68cba671..1c6db18f890 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicatorDataTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicatorDataTest.java @@ -15,6 +15,17 @@ */ package com.vaadin.flow.data.provider.hierarchy; +import java.io.Serializable; +import java.util.Arrays; +import java.util.List; +import java.util.stream.IntStream; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; + import com.vaadin.flow.component.UI; import com.vaadin.flow.data.provider.CompositeDataGenerator; import com.vaadin.flow.data.provider.DataCommunicatorTest; @@ -24,6 +35,7 @@ import com.vaadin.flow.server.VaadinRequest; import com.vaadin.flow.server.VaadinService; import com.vaadin.flow.server.VaadinSession; + import elemental.json.JsonValue; import org.junit.Assert; import org.junit.Before; @@ -76,6 +88,7 @@ public int hashCode() { private HierarchicalDataCommunicator communicator; private TreeData treeData; private MockUI ui; + private Element element; private static class UpdateQueue implements HierarchicalUpdate { @Override @@ -126,7 +139,7 @@ public void initialize() { public void setUp() { MockitoAnnotations.initMocks(this); ui = new MockUI(); - Element element = new Element("div"); + element = new Element("div"); ui.getElement().appendChild(element); treeData = new TreeData<>(); @@ -168,6 +181,47 @@ public void sameKeyDifferentInstance_latestInstanceUsed() { Assert.assertSame(updatedLeaf, communicator.getKeyMapper().get(key)); } + @Test + public void uniqueKeyProviderIsSet_keysGeneratedByProvider() { + communicator = new HierarchicalDataCommunicator( + Mockito.mock(CompositeDataGenerator.class), arrayUpdater, + json -> { + }, element.getNode(), () -> item -> item.value); + communicator.setDataProvider(dataProvider, null); + // expand test items to force key calculation + communicator.expand(ROOT); + communicator.expand(FOLDER); + // this is needed to force key calculation for leaf item + communicator.setParentRequestedRange(0, 50, LEAF); + fakeClientCommunication(); + + Arrays.asList("ROOT", "FOLDER", "LEAF") + .forEach(key -> Assert.assertNotNull("Expected key '" + key + + "' to be generated when unique key provider used", + communicator.getKeyMapper().get(key))); + } + + @Test + public void uniqueKeyProviderIsNotSet_keysGeneratedByKeyMapper() { + communicator = new HierarchicalDataCommunicator( + Mockito.mock(CompositeDataGenerator.class), arrayUpdater, + json -> { + }, element.getNode(), () -> null); + communicator.setDataProvider(dataProvider, null); + // expand test items to force key calculation + communicator.expand(ROOT); + communicator.expand(FOLDER); + // this is needed to force key calculation for leaf item + communicator.setParentRequestedRange(0, 50, LEAF); + fakeClientCommunication(); + + // key mapper should generate keys 1,2,3 + IntStream.range(1, 4).mapToObj(String::valueOf) + .forEach(i -> Assert.assertNotNull("Expected key '" + i + + "' to be generated when unique key provider is not set", + communicator.getKeyMapper().get(i))); + } + private void fakeClientCommunication() { ui.getInternals().getStateTree().runExecutionsBeforeClientResponse(); ui.getInternals().getStateTree().collectChanges(ignore -> {