From 57bd2ea3443cbefe1b40558c509486c80158ebc6 Mon Sep 17 00:00:00 2001 From: caalador Date: Mon, 29 Jan 2024 08:54:44 +0200 Subject: [PATCH 1/5] fix: no serverSideRoutes check if no Routes (#18546) * fix: no serverSideRoutes check if no Routes Do not check for the serverSideRoutes in routes.tsx in react mode if no `@Route` annotated Flow routes found. * Add test case Make more readable. --- .../frontend/TaskGenerateReactFiles.java | 22 +++++-- .../frontend/TaskGenerateReactFilesTest.java | 60 ++++++++++++++++++- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskGenerateReactFiles.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskGenerateReactFiles.java index 06fc9dd4c39..785e9f89e60 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskGenerateReactFiles.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskGenerateReactFiles.java @@ -31,6 +31,7 @@ import com.vaadin.experimental.FeatureFlags; import com.vaadin.flow.internal.UsageStatistics; +import com.vaadin.flow.router.Route; import com.vaadin.flow.server.Constants; import com.vaadin.flow.server.ExecutionFailedException; import com.vaadin.flow.server.Version; @@ -59,7 +60,7 @@ */ public class TaskGenerateReactFiles implements FallibleCommand { - private final File frontendDirectory; + private Options options; protected static String NO_IMPORT = """ Faulty configuration of serverSideRoutes. The server route definition is missing from the '%1$s' file @@ -93,11 +94,12 @@ public class TaskGenerateReactFiles implements FallibleCommand { * the task options */ TaskGenerateReactFiles(Options options) { - this.frontendDirectory = options.getFrontendDirectory(); + this.options = options; } @Override public void execute() throws ExecutionFailedException { + File frontendDirectory = options.getFrontendDirectory(); File appTsx = new File(frontendDirectory, "App.tsx"); File flowTsx = new File( new File(frontendDirectory, FrontendUtils.GENERATED), @@ -114,9 +116,8 @@ public void execute() throws ExecutionFailedException { } else { String routesContent = FileUtils.readFileToString(routesTsx, UTF_8); - Pattern serverImport = Pattern.compile( - "import[\\s\\S]?\\{[\\s\\S]?serverSideRoutes[\\s\\S]?\\}[\\s\\S]?from[\\s\\S]?(\"|'|`)Frontend\\/generated\\/flow\\/Flow\\1;"); - if (!serverImport.matcher(routesContent).find()) { + if (missingServerImport(routesContent) + && serverRoutesAvailable()) { throw new ExecutionFailedException( String.format(NO_IMPORT, routesTsx.getPath())); } @@ -130,6 +131,17 @@ public void execute() throws ExecutionFailedException { } } + private boolean missingServerImport(String routesContent) { + Pattern serverImport = Pattern.compile( + "import[\\s\\S]?\\{[\\s\\S]?serverSideRoutes[\\s\\S]?\\}[\\s\\S]?from[\\s\\S]?(\"|'|`)Frontend\\/generated\\/flow\\/Flow\\1;"); + return !serverImport.matcher(routesContent).find(); + } + + private boolean serverRoutesAvailable() { + return !options.getClassFinder().getAnnotatedClasses(Route.class) + .isEmpty(); + } + private void writeFile(File target, String content) throws ExecutionFailedException { diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskGenerateReactFilesTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskGenerateReactFilesTest.java index ae131ee1b58..cca70ca068c 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskGenerateReactFilesTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskGenerateReactFilesTest.java @@ -19,6 +19,7 @@ import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Collections; import org.apache.commons.io.FileUtils; import org.junit.Assert; @@ -28,8 +29,12 @@ import org.junit.rules.TemporaryFolder; import org.mockito.Mockito; +import com.vaadin.flow.component.Component; +import com.vaadin.flow.component.Tag; import com.vaadin.flow.di.Lookup; +import com.vaadin.flow.router.Route; import com.vaadin.flow.server.ExecutionFailedException; +import com.vaadin.flow.server.frontend.scanner.ClassFinder; public class TaskGenerateReactFilesTest { @@ -38,11 +43,19 @@ public class TaskGenerateReactFilesTest { Options options; File routesTsx, frontend; + ClassFinder classFinder; @Before public void setup() throws IOException { - options = new Options(Mockito.mock(Lookup.class), - temporaryFolder.getRoot()).withBuildDirectory("target"); + classFinder = Mockito.mock(ClassFinder.class); + Lookup lookup = Mockito.mock(Lookup.class); + Mockito.when(lookup.lookup(ClassFinder.class)).thenReturn(classFinder); + + Mockito.when(classFinder.getAnnotatedClasses(Route.class)) + .thenReturn(Collections.singleton(TestRoute.class)); + + options = new Options(lookup, temporaryFolder.getRoot()) + .withBuildDirectory("target"); frontend = temporaryFolder.newFolder("frontend"); options.withFrontendDirectory(frontend); routesTsx = new File(frontend, "routes.tsx"); @@ -176,6 +189,44 @@ public void routesContainsRoutesExport_noExpectionThrown() task.execute(); } + @Test + public void missingImport_noServerRoutesDefined_noExpectionThrown() + throws IOException, ExecutionFailedException { + String content = """ + import HelloWorldView from 'Frontend/views/helloworld/HelloWorldView.js'; + import MainLayout from 'Frontend/views/MainLayout.js'; + import { lazy } from 'react'; + import { createBrowserRouter, RouteObject } from 'react-router-dom'; + import {protectRoutes} from "@hilla/react-auth"; + import LoginView from "Frontend/views/LoginView"; + + const AboutView = lazy(async () => import('Frontend/views/about/AboutView.js')); + + export const routes: RouteObject[] = protectRoutes([ + { + element: , + handle: { title: 'Main' }, + children: [ + { path: '/', element: , handle: { title: 'Hello World', rolesAllowed: ['USER'] } }, + { path: '/about', element: , handle: { title: 'About' } } + ], + }, + { path: '/login', element: }, + ]); + + export default createBrowserRouter(routes); + """; + + FileUtils.write(routesTsx, content, StandardCharsets.UTF_8); + + Mockito.when(classFinder.getAnnotatedClasses(Route.class)) + .thenReturn(Collections.emptySet()); + + TaskGenerateReactFiles task = new TaskGenerateReactFiles(options); + + task.execute(); + } + @Test public void routesexportMissing_expectionThrown() throws IOException { String content = """ @@ -214,4 +265,9 @@ public void routesexportMissing_expectionThrown() throws IOException { Assert.assertEquals(TaskGenerateReactFiles.MISSING_ROUTES_EXPORT, exception.getMessage()); } + + @Tag("div") + @Route("test") + private class TestRoute extends Component { + } } \ No newline at end of file From 16425e730ce796e7fe3432b39414a122dbd50c3c Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Mon, 29 Jan 2024 10:34:12 +0200 Subject: [PATCH 2/5] fix: Allow moving node to another tree after removeFromTree call (#18486) Enables moving a component to another UI by just calling Element.removeFromTree() and then adding the component to the other UI. Fixes #18390 --------- Co-authored-by: caalador Co-authored-by: Tatu Lund --- .../java/com/vaadin/flow/internal/StateNode.java | 5 +++++ .../test/java/com/vaadin/flow/dom/ElementTest.java | 13 +++++++++++++ .../com/vaadin/flow/internal/StateTreeTest.java | 14 ++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java index 766151d1429..156d6aa2814 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java @@ -793,6 +793,11 @@ private void doSetTree(StateTree tree) { } else { id = -1; } + } else if (id > -1 && getOwner() == NullOwner.get()) { + // When id is set but owner is NullOwner, removeFromTree has been + // called, so we should clear node id and attached state, thus allow + // moving the node to another StateTree + reset(false); } owner = tree; } diff --git a/flow-server/src/test/java/com/vaadin/flow/dom/ElementTest.java b/flow-server/src/test/java/com/vaadin/flow/dom/ElementTest.java index 3ff2ce5ab5d..0add1e5744c 100644 --- a/flow-server/src/test/java/com/vaadin/flow/dom/ElementTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/dom/ElementTest.java @@ -2047,6 +2047,19 @@ public void testDetachEvent_stateTreeCanFound() { Assert.assertEquals(1, detached.get()); } + @Test + public void testMoveFromUiToUi_doesNotThrow() { + Element body = new UI().getElement(); + Element child = ElementFactory.createDiv(); + body.appendChild(child); + + child.removeFromTree(); + + body = new UI().getElement(); + body.appendChild(child); + Assert.assertEquals(body, child.getParent()); + } + @Test public void testRemoveFromTree_inDetachListener_removedFromParent() { Element body = new UI().getElement(); diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/StateTreeTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/StateTreeTest.java index 6dc7b8de51e..b012a99a2b9 100644 --- a/flow-server/src/test/java/com/vaadin/flow/internal/StateTreeTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/internal/StateTreeTest.java @@ -170,6 +170,20 @@ public void moveNodeToOtherRoot_throws() { StateNodeTest.setParent(node, anotherTree.getRootNode()); } + @Test + public void moveNodeToOtherRoot_removeFromTree_doesNotThrow() { + StateNode node = StateNodeTest.createEmptyNode(); + + StateNodeTest.setParent(node, tree.getRootNode()); + + node.removeFromTree(); + + StateTree anotherTree = new StateTree(new UI().getInternals(), + ElementChildrenList.class); + + StateNodeTest.setParent(node, anotherTree.getRootNode()); + } + @Test public void testNoRootAttachChange() { List changes = collectChangesExceptChildrenAddRemove(); From 1b5d958e298fc78bac253381086f4b0ee4c42c8a Mon Sep 17 00:00:00 2001 From: Marco Collovati Date: Mon, 29 Jan 2024 13:47:53 +0100 Subject: [PATCH 3/5] fix: Include translation files in Spring native compile hints (#18547) Fixes #18545 --- .../vaadin/flow/spring/springnative/VaadinHintsRegistrar.java | 1 + 1 file changed, 1 insertion(+) diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/springnative/VaadinHintsRegistrar.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/springnative/VaadinHintsRegistrar.java index fedafa5324e..b9413988fcc 100644 --- a/vaadin-spring/src/main/java/com/vaadin/flow/spring/springnative/VaadinHintsRegistrar.java +++ b/vaadin-spring/src/main/java/com/vaadin/flow/spring/springnative/VaadinHintsRegistrar.java @@ -23,6 +23,7 @@ public void registerHints(RuntimeHints hints, ClassLoader classLoader) { // Bundles, build info etc hints.resources().registerPattern("META-INF/VAADIN/*"); + hints.resources().registerPattern("vaadin-i18n/*"); // Random classes that need reflection for (String cls : getClasses()) { From dc68b28938f08369796061f45b1a1b885d56d548 Mon Sep 17 00:00:00 2001 From: Marco Collovati Date: Mon, 29 Jan 2024 13:57:57 +0100 Subject: [PATCH 4/5] fix: consider local npm packages in bundle validation (#18368) If a project package.json contains references to local npm pacakges instead of parsable version, the bundle validation fails. This change update the version check in bundle validation to accept also local packages. For those packages, a bundle re-creation is triggered only if the reference in package.json does not match the one in the stats. Fixes #18106 --- .../server/frontend/BundleValidationUtil.java | 27 +++++- .../server/frontend/BundleValidationTest.java | 86 +++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/BundleValidationUtil.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/BundleValidationUtil.java index 26231fd2c59..1c6be44657b 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/BundleValidationUtil.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/BundleValidationUtil.java @@ -13,6 +13,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -449,8 +450,30 @@ public static boolean hashAndBundleModulesEqual(JsonObject statsJson, } private static boolean versionAccepted(String expected, String actual) { - FrontendVersion expectedVersion = new FrontendVersion(expected); - FrontendVersion actualVersion = new FrontendVersion(actual); + FrontendVersion expectedVersion; + try { + expectedVersion = new FrontendVersion(expected); + } catch (NumberFormatException ex) { + expectedVersion = null; + } + FrontendVersion actualVersion; + try { + actualVersion = new FrontendVersion(actual); + } catch (NumberFormatException ex) { + actualVersion = null; + } + + if (expectedVersion == null && actualVersion == null) { + return Objects.equals(expected, actual); + } else if (expectedVersion == null || actualVersion == null) { + // expected or actual version is referencing a local package + // while the other one is a parsable version + getLogger().debug( + "Version '{}' cannot be parsed and compared to '{}'", + expectedVersion == null ? expected : actual, + expectedVersion == null ? actual : expected); + return false; + } if (expected.startsWith("~")) { boolean correctRange = expectedVersion diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/BundleValidationTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/BundleValidationTest.java index 28fe2bbf33e..36f4eef430b 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/frontend/BundleValidationTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/frontend/BundleValidationTest.java @@ -39,6 +39,7 @@ import elemental.json.Json; import elemental.json.JsonArray; import elemental.json.JsonObject; + import static com.vaadin.flow.server.Constants.DEV_BUNDLE_JAR_PATH; import static com.vaadin.flow.server.Constants.PROD_BUNDLE_JAR_PATH; @@ -1795,6 +1796,91 @@ public void flowFrontendPackageInPackageJson_noBundleRebuild() needsBuild); } + @Test + public void localPackageInPackageJson_notChanged_noBundleRebuild() + throws IOException { + createPackageJsonStub( + "{\"dependencies\": {\"my-pkg\": \"file:my-pkg\"}, \"vaadin\": { \"hash\": \"aHash\"} }"); + + final FrontendDependenciesScanner depScanner = Mockito + .mock(FrontendDependenciesScanner.class); + + JsonObject stats = getBasicStats(); + stats.getObject(PACKAGE_JSON_DEPENDENCIES).put("my-pkg", "file:my-pkg"); + + setupFrontendUtilsMock(stats); + + boolean needsBuild = BundleValidationUtil.needsBuild(options, + depScanner, finder, mode); + Assert.assertFalse( + "Shouldn't re-bundle when referencing local packages in package.json", + needsBuild); + } + + @Test + public void localPackageInPackageJson_differentReference_bundleRebuild() + throws IOException { + createPackageJsonStub( + "{\"dependencies\": {\"my-pkg\": \"file:my-pkg\"}, \"vaadin\": { \"hash\": \"aHash\"} }"); + + final FrontendDependenciesScanner depScanner = Mockito + .mock(FrontendDependenciesScanner.class); + + JsonObject stats = getBasicStats(); + stats.getObject(PACKAGE_JSON_DEPENDENCIES).put("my-pkg", + "./another-folder"); + + setupFrontendUtilsMock(stats); + + boolean needsBuild = BundleValidationUtil.needsBuild(options, + depScanner, finder, mode); + Assert.assertTrue( + "Should re-bundle when local packages have different values", + needsBuild); + } + + @Test + public void localPackageInPackageJson_parsableVersionInStats_bundleRebuild() + throws IOException { + createPackageJsonStub( + "{\"dependencies\": {\"my-pkg\": \"file:my-pkg\"}, \"vaadin\": { \"hash\": \"aHash\"} }"); + + final FrontendDependenciesScanner depScanner = Mockito + .mock(FrontendDependenciesScanner.class); + + JsonObject stats = getBasicStats(); + stats.getObject(PACKAGE_JSON_DEPENDENCIES).put("my-pkg", "1.0.0"); + + setupFrontendUtilsMock(stats); + + boolean needsBuild = BundleValidationUtil.needsBuild(options, + depScanner, finder, mode); + Assert.assertTrue( + "Should re-bundle when local package in package.json but parsable version in stats", + needsBuild); + } + + @Test + public void localPackageInStats_parsableVersionInPackageJson_bundleRebuild() + throws IOException { + createPackageJsonStub( + "{\"dependencies\": {\"my-pkg\": \"1.0.0\"}, \"vaadin\": { \"hash\": \"aHash\"} }"); + + final FrontendDependenciesScanner depScanner = Mockito + .mock(FrontendDependenciesScanner.class); + + JsonObject stats = getBasicStats(); + stats.getObject(PACKAGE_JSON_DEPENDENCIES).put("my-pkg", "file:my-pkg"); + + setupFrontendUtilsMock(stats); + + boolean needsBuild = BundleValidationUtil.needsBuild(options, + depScanner, finder, mode); + Assert.assertTrue( + "Should re-bundle when local package in stats but parsable version in package.json", + needsBuild); + } + @Test public void bundleMissesSomeEntries_devMode_skipBundleBuildSet_noBundleRebuild() throws IOException { From ec47e68923f8447bf97d98115a5c5675a1956153 Mon Sep 17 00:00:00 2001 From: Giovanni Lovato Date: Tue, 30 Jan 2024 10:34:12 +0100 Subject: [PATCH 5/5] fix: Close streams fetched from data-providers (#18552) Fixes DataCommunicator API by handling the streams fetched from data-providers inside try-with-resources blocks. Unclosed streams might still leak from the DataView API, that should be fixed in each component using that API. Fixes #18551 --- .../flow/data/provider/DataCommunicator.java | 63 +++++++++++-------- .../vaadin/flow/data/provider/DataView.java | 16 +++++ .../HierarchicalCommunicationController.java | 22 ++++--- .../provider/hierarchy/HierarchyMapper.java | 10 +-- .../data/provider/DataCommunicatorTest.java | 32 +++++++++- .../HierarchicalCommunicatorDataTest.java | 29 +++++++++ .../HierarchyMapperWithDataTest.java | 37 +++++++++++ 7 files changed, 167 insertions(+), 42 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/DataCommunicator.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/DataCommunicator.java index e6d78a9231c..b29d3b13fb4 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/DataCommunicator.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/DataCommunicator.java @@ -610,8 +610,10 @@ public T getItem(int index) { * because the backend can have the item on that index (we simply * not yet fetched this item during the scrolling). */ - return (T) getDataProvider().fetch(buildQuery(index, 1)).findFirst() - .orElse(null); + try (Stream stream = getDataProvider() + .fetch(buildQuery(index, 1))) { + return stream.findFirst().orElse(null); + } } } @@ -1010,18 +1012,20 @@ protected Stream fetchFromProvider(int offset, int limit) { int page = 0; do { final int newOffset = offset + page * pageSize; - Stream dataProviderStream = doFetchFromDataProvider( - newOffset, pageSize); - // Stream.Builder is not thread safe, so for parallel stream - // we need to first collect items before adding them - if (dataProviderStream.isParallel()) { - getLogger().debug( - "Data provider {} has returned parallel stream on 'fetch' call", - getDataProvider().getClass()); - dataProviderStream.collect(Collectors.toList()) - .forEach(addItemAndCheckConsumer); - } else { - dataProviderStream.forEach(addItemAndCheckConsumer); + try (Stream dataProviderStream = doFetchFromDataProvider( + newOffset, pageSize)) { + // Stream.Builder is not thread safe, so for parallel + // stream we need to first collect items before adding + // them + if (dataProviderStream.isParallel()) { + getLogger().debug( + "Data provider {} has returned parallel stream on 'fetch' call", + getDataProvider().getClass()); + dataProviderStream.collect(Collectors.toList()) + .forEach(addItemAndCheckConsumer); + } else { + dataProviderStream.forEach(addItemAndCheckConsumer); + } } page++; } while (page < pages @@ -1040,8 +1044,10 @@ protected Stream fetchFromProvider(int offset, int limit) { getLogger().debug( "Data provider {} has returned parallel stream on 'fetch' call", getDataProvider().getClass()); - stream = stream.collect(Collectors.toList()).stream(); - assert !stream.isParallel(); + try (Stream parallelStream = stream) { + stream = parallelStream.collect(Collectors.toList()).stream(); + assert !stream.isParallel(); + } } SizeVerifier verifier = new SizeVerifier<>(limit); @@ -1476,17 +1482,20 @@ private Activation activate(Range range) { // XXX Explicitly refresh anything that is updated List activeKeys = new ArrayList<>(range.length()); - fetchFromProvider(range.getStart(), range.length()).forEach(bean -> { - boolean mapperHasKey = keyMapper.has(bean); - String key = keyMapper.key(bean); - if (mapperHasKey) { - // Ensure latest instance from provider is used - keyMapper.refresh(bean); - passivatedByUpdate.values().stream() - .forEach(set -> set.remove(key)); - } - activeKeys.add(key); - }); + try (Stream stream = fetchFromProvider(range.getStart(), + range.length())) { + stream.forEach(bean -> { + boolean mapperHasKey = keyMapper.has(bean); + String key = keyMapper.key(bean); + if (mapperHasKey) { + // Ensure latest instance from provider is used + keyMapper.refresh(bean); + passivatedByUpdate.values().stream() + .forEach(set -> set.remove(key)); + } + activeKeys.add(key); + }); + } boolean needsSizeRecheck = activeKeys.size() < range.length(); return new Activation(activeKeys, needsSizeRecheck); } diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/DataView.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/DataView.java index a2bf6dfea90..b3a4cc9e04d 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/DataView.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/DataView.java @@ -59,6 +59,22 @@ public interface DataView extends Serializable { /** * Get the full data available to the component. Data is filtered and sorted * the same way as in the component. + *

+ * Consumers of the returned stream are responsible for closing it when all + * the stream operations are done to ensure that any resources feeding the + * stream are properly released. Failure to close the stream might lead to + * resource leaks. + *

+ * It is strongly recommended to use a try-with-resources block to + * automatically close the stream after its terminal operation has been + * executed. Below is an example of how to properly use and close the + * stream: + * + * 

{@code
+     * try (Stream stream = dataView.getItems()) {
+     *     stream.forEach(System.out::println); // Example terminal operation
+     * }
+     * }
* * @return filtered and sorted data set */ diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java index 5e111a671f8..416cb98bb37 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java @@ -255,16 +255,18 @@ private List activate(Range range) { // XXX Explicitly refresh anything that is updated List activeKeys = new ArrayList<>(range.length()); - fetchItems.apply(parentKey, range).forEach(bean -> { - boolean mapperHasKey = keyMapper.has(bean); - String key = keyMapper.key(bean); - if (mapperHasKey) { - // Ensure latest instance from provider is used - keyMapper.refresh(bean); - passivatedByUpdate.values().forEach(set -> set.remove(key)); - } - activeKeys.add(key); - }); + try (Stream stream = fetchItems.apply(parentKey, range)) { + stream.forEach(bean -> { + boolean mapperHasKey = keyMapper.has(bean); + String key = keyMapper.key(bean); + if (mapperHasKey) { + // Ensure latest instance from provider is used + keyMapper.refresh(bean); + passivatedByUpdate.values().forEach(set -> set.remove(key)); + } + activeKeys.add(key); + }); + } return activeKeys; } diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapper.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapper.java index b47bd6fd9b8..ae8cae9a57a 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapper.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapper.java @@ -534,8 +534,9 @@ private Stream getFlatChildrenStream(T parent) { private Stream getFlatChildrenStream(T parent, boolean includeParent) { List childList = Collections.emptyList(); if (isExpanded(parent)) { - childList = doFetchDirectChildren(parent) - .collect(Collectors.toList()); + try (Stream stream = doFetchDirectChildren(parent)) { + childList = stream.collect(Collectors.toList()); + } if (childList.isEmpty()) { removeChildren(parent == null ? null : getDataProvider().getId(parent)); @@ -563,8 +564,9 @@ private Stream getChildrenStream(T parent, Range range, boolean includeParent) { List childList = Collections.emptyList(); if (isExpanded(parent)) { - childList = doFetchDirectChildren(parent, range) - .collect(Collectors.toList()); + try (Stream stream = doFetchDirectChildren(parent, range)) { + childList = stream.collect(Collectors.toList()); + } if (childList.isEmpty()) { removeChildren(parent == null ? null : getDataProvider().getId(parent)); diff --git a/flow-data/src/test/java/com/vaadin/flow/data/provider/DataCommunicatorTest.java b/flow-data/src/test/java/com/vaadin/flow/data/provider/DataCommunicatorTest.java index c3a92b323f3..f6e575dc49e 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/provider/DataCommunicatorTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/provider/DataCommunicatorTest.java @@ -996,6 +996,18 @@ public void getItem_withUndefinedSizeAndSorting() { dataCommunicator.getItem(2)); } + @Test + public void getItem_streamIsClosed() { + AtomicBoolean streamIsClosed = new AtomicBoolean(); + dataCommunicator.setDataProvider(createDataProvider(streamIsClosed), + null); + + fakeClientCommunication(); + dataCommunicator.getItem(0); + + Assert.assertTrue(streamIsClosed.get()); + } + @Test public void itemCountEstimateAndStep_defaults() { Assert.assertEquals(dataCommunicator.getItemCountEstimate(), @@ -1353,6 +1365,18 @@ public void fetchFromProvider_itemCountLessThanTwoPages_correctItemsReturned() { } + @Test + public void fetchFromProvider_streamIsClosed() { + AtomicBoolean streamIsClosed = new AtomicBoolean(); + dataCommunicator.setDataProvider(createDataProvider(streamIsClosed), + null); + dataCommunicator.setRequestedRange(0, 50); + + fakeClientCommunication(); + + Assert.assertTrue(streamIsClosed.get()); + } + @Test public void fetchEnabled_getItemCount_stillReturnsItemsCount() { dataCommunicator.setFetchEnabled(false); @@ -1737,6 +1761,11 @@ public Stream fetch(Query query) { } private AbstractDataProvider createDataProvider() { + return createDataProvider(new AtomicBoolean()); + } + + private AbstractDataProvider createDataProvider( + AtomicBoolean streamIsClosed) { return new AbstractDataProvider() { @Override public boolean isInMemory() { @@ -1752,7 +1781,8 @@ public int size(Query query) { public Stream fetch(Query query) { return asParallelIfRequired(IntStream.range(query.getOffset(), query.getLimit() + query.getOffset())) - .mapToObj(Item::new); + .mapToObj(Item::new) + .onClose(() -> streamIsClosed.set(true)); } }; } 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 74142f79714..4274e2d2207 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 @@ -18,7 +18,9 @@ import java.io.Serializable; import java.util.Arrays; import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.IntStream; +import java.util.stream.Stream; import org.junit.Assert; import org.junit.Before; @@ -221,6 +223,33 @@ public void uniqueKeyProviderIsNotSet_keysGeneratedByKeyMapper() { communicator.getKeyMapper().get(i))); } + @Test + public void expandRoot_streamIsClosed() { + AtomicBoolean streamIsClosed = new AtomicBoolean(); + + dataProvider = new TreeDataProvider<>(treeData) { + + @Override + public Stream fetchChildren( + HierarchicalQuery> query) { + return super.fetchChildren(query) + .onClose(() -> streamIsClosed.set(true)); + } + }; + + communicator.setDataProvider(dataProvider, null); + + communicator.expand(ROOT); + fakeClientCommunication(); + + communicator.setParentRequestedRange(0, 50, ROOT); + fakeClientCommunication(); + + communicator.reset(); + + Assert.assertTrue(streamIsClosed.get()); + } + @Test public void expandRoot_filterOutAllChildren_clearCalled() { parentClearCalled = false; diff --git a/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapperWithDataTest.java b/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapperWithDataTest.java index 6da7a8e211e..fe31e5ee0db 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapperWithDataTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapperWithDataTest.java @@ -21,6 +21,7 @@ import java.util.Comparator; import java.util.List; import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -288,6 +289,42 @@ public void getExpandedItems_tryToAddItemsToCollection_shouldThrowException() { expandedItems.add(new TreeNode("third-1")); } + @Test + public void fetchHierarchyItems_streamIsClosed() { + AtomicBoolean streamIsClosed = new AtomicBoolean(); + mapper = new HierarchyMapper<>(new TreeDataProvider<>(data) { + @Override + public Stream fetchChildren( + HierarchicalQuery> query) { + return super.fetchChildren(query) + .onClose(() -> streamIsClosed.set(true)); + } + }); + Node rootNode = testData.get(0); + mapper.expand(rootNode); + mapper.fetchHierarchyItems(rootNode, Range.between(0, 10)).count(); + + Assert.assertTrue(streamIsClosed.get()); + } + + @Test + public void fetchChildItems_streamIsClosed() { + AtomicBoolean streamIsClosed = new AtomicBoolean(); + mapper = new HierarchyMapper<>(new TreeDataProvider<>(data) { + @Override + public Stream fetchChildren( + HierarchicalQuery> query) { + return super.fetchChildren(query) + .onClose(() -> streamIsClosed.set(true)); + } + }); + Node rootNode = testData.get(0); + mapper.expand(rootNode); + mapper.fetchChildItems(rootNode, Range.between(0, 10)); + + Assert.assertTrue(streamIsClosed.get()); + } + private void expand(Node node) { insertRows(mapper.expand(node, mapper.getIndexOf(node).orElse(null))); }