Merge branch 'master' of github.com:vaadin/flow

* 'master' of github.com:vaadin/flow: (191 commits) Fix regression in @Push when running in client bootstrapping (#8435) Don't override initial value of hidden HTML Element attribute. (#8419) Cherry pick from 2.2 to master and fix postpone and forward with CCDM (#8342) (#8359) Moved init parameters to new class com.vaadin.flow.server.InitParameters (#8409) Update chrome version to 83 (#8417) Listen detach events on "listenOn" components in shortcut registration (#8391) Correct Javadocs for adding JS and Stylesheets programatically. (#8392) Close client app on change location (#8334) Close UI after refreshing the page in PreserveOnRefresh case and mark it (#8365) remove unused import fix: custom connect client breaks endpoints mapping Wait for intermediate page to load completely in @PreserveOnRefresh IT (#8338) Properly encoded URIs use %20 (#8271) Removed unnecessary private method (#8357) Extract live reload ITs into separate test module (#8324) fix: check for TypeVariable in the ExplicitNullableTypeChecker (#8316) Live reload show message on error (#8206) (#8304) Clarify message about blocking with the session locked (#8312) Url parameter template support. (#7608) Store vaadin hash to node_modules (#8282) ... # Conflicts: # .gitignore # flow-client/src/main/java/com/vaadin/client/ApplicationConnection.java # flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts # flow-client/src/test/frontend/FlowTests.ts # flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/BuildFrontendMojo.java # flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/FlowModeAbstractMojo.java # flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/PrepareFrontendMojo.java # flow-maven-plugin/src/test/java/com/vaadin/flow/plugin/maven/BuildFrontendMojoTest.java # flow-maven-plugin/src/test/java/com/vaadin/flow/plugin/maven/PrepareFrontendMojoTest.java # flow-server/src/main/java/com/vaadin/flow/function/DeploymentConfiguration.java # flow-server/src/main/java/com/vaadin/flow/server/Constants.java # flow-server/src/main/java/com/vaadin/flow/server/DefaultDeploymentConfiguration.java # flow-server/src/main/java/com/vaadin/flow/server/DeploymentConfigurationFactory.java # flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java # flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectController.java # flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java # flow-server/src/main/java/com/vaadin/flow/server/frontend/NodeTasks.java # flow-server/src/main/java/com/vaadin/flow/server/frontend/NodeUpdater.java # flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskGenerateTsConfig.java # flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java # flow-server/src/main/java/com/vaadin/flow/server/frontend/VersionsJsonConverter.java # flow-server/src/main/java/com/vaadin/flow/server/startup/AbstractRouteRegistryInitializer.java # flow-server/src/main/java/com/vaadin/flow/server/startup/DevModeInitializer.java # flow-server/src/main/java/com/vaadin/flow/server/startup/ServletDeployer.java # flow-server/src/main/java/com/vaadin/flow/server/startup/VaadinAppShellInitializer.java # flow-server/src/main/java/com/vaadin/flow/shared/ApplicationConstants.java # flow-server/src/main/resources/pnpmfile.js # flow-server/src/main/resources/webpack.generated.js # flow-server/src/test/java/com/vaadin/flow/server/DefaultDeploymentConfigurationTest.java # flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java # flow-server/src/test/java/com/vaadin/flow/server/communication/WebComponentBootstrapHandlerTest.java # flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerTest.java # flow-server/src/test/java/com/vaadin/flow/server/connect/rest/EndpointWithRestControllerTest.java # flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendUtilsTest.java # flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunNpmInstallTest.java # flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunPnpmInstallTest.java # flow-server/src/test/java/com/vaadin/flow/server/frontend/VersionsJsonConverterTest.java # flow-server/src/test/java/com/vaadin/flow/server/startup/AbstractRouteRegistryInitializerTest.java # flow-server/src/test/java/com/vaadin/flow/server/startup/DevModeInitializerTest.java # flow-server/src/test/java/com/vaadin/flow/server/startup/DevModeInitializerTestBase.java # flow-server/src/test/java/com/vaadin/flow/server/startup/VaadinAppShellInitializerTest.java # flow-server/src/test/resources/versions/no_vaadin_package.json # flow-server/src/test/resources/versions/package.json # flow-server/src/test/resources/versions/user_package.json # flow-server/src/test/resources/versions/user_versions.json # flow-server/src/test/resources/versions/versions.json # flow-test-util/src/main/java/com/vaadin/flow/testutil/AbstractTestBenchTest.java # flow-tests/pom.xml # flow-tests/test-npm-only-features/test-npm-performance-regression/src/test/java/com/vaadin/flow/testnpmonlyfeatures/performanceregression/StartupPerformanceIT.java # flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/PushWithPreserveOnRefreshView.java # flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/StreamResourceView.java # flow-tests/test-root-context/src/test/java/com/vaadin/flow/VerifyBrowserVersionIT.java # flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/NavigationTriggerIT.java # flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/PushWithPreserveOnRefreshIT.java # flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RequestParametersIT.java # flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RouterSessionExpirationIT.java # flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/StreamResourceIT.java # flow-tests/test-root-ui-context/src/main/java/com/vaadin/flow/uitest/servlet/ApplicationRunnerServlet.java # flow-tests/test-root-ui-context/src/test/java/com/vaadin/flow/uitest/ui/push/ReconnectLongPollingIT.java # flow-tests/test-router-custom-context/src/test/java/com/vaadin/flow/contexttest/ui/PushIT.java
vaadin · May 28, 2020 · a29d3c8 · a29d3c8
1 parent 7b072f2
commit a29d3c8
Show file tree

Hide file tree

Showing 6 changed files with 234 additions and 39 deletions.
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
@@ -36,6 +36,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import com.vaadin.flow.component.ComponentUtil;
 import com.vaadin.flow.component.UI;
 import com.vaadin.flow.function.SerializableConsumer;
 import com.vaadin.flow.internal.StateTree.BeforeClientResponseEntry;
@@ -126,6 +127,11 @@ public FeatureSet(FeatureSetKey featureSetKey) {
         }
     }
 
+    private static class ReplacedViaPreserveOnRefresh implements Serializable {
+    }
+
+    private static final ReplacedViaPreserveOnRefresh REPLACED_MARKER = new ReplacedViaPreserveOnRefresh();
+
     /**
      * Cache of immutable node feature type set instances.
      */
@@ -369,6 +375,10 @@ protected void setTree(StateTree tree) {
      * the state tree.
      */
     public void removeFromTree() {
+        if (getOwner() instanceof StateTree) {
+            ComponentUtil.setData(((StateTree) getOwner()).getUI(),
+                    ReplacedViaPreserveOnRefresh.class, REPLACED_MARKER);
+        }
         visitNodeTree(StateNode::reset);
         setParent(null);
     }
@@ -693,16 +703,25 @@ void visitNodeTreeBottomUp(Consumer<StateNode> visitor) {
     }
 
     private void doSetTree(StateTree tree) {
-        if (tree == owner) {
+        if (tree == getOwner()) {
             return;
         }
 
-        if (owner instanceof StateTree) {
-            throw new IllegalStateException(
-                    "Can't move a node from one state tree to another. "
-                            + "If this is intentional, first remove the "
-                            + "node from its current state tree by calling "
-                            + "removeFromTree");
+        if (getOwner() instanceof StateTree) {
+            boolean isOwnerAttached = ((StateTree) getOwner()).getRootNode()
+                    .isAttached();
+            boolean isNotReplaced = ComponentUtil.getData(
+                    ((StateTree) getOwner()).getUI(),
+                    ReplacedViaPreserveOnRefresh.class) == null;
+            if (isOwnerAttached || isNotReplaced) {
+                throw new IllegalStateException(
+                        "Can't move a node from one state tree to another. "
+                                + "If this is intentional, first remove the "
+                                + "node from its current state tree by calling "
+                                + "removeFromTree");
+            } else {
+                id = -1;
+            }
         }
         owner = tree;
     }

diff --git a/...server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java b/...server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java
@@ -28,6 +28,9 @@
 import java.util.Optional;
 import java.util.Set;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import com.vaadin.flow.component.Component;
 import com.vaadin.flow.component.HasElement;
 import com.vaadin.flow.component.UI;
@@ -59,8 +62,6 @@
 import com.vaadin.flow.router.Router;
 import com.vaadin.flow.router.RouterLayout;
 import com.vaadin.flow.server.VaadinSession;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 /**
  * Base class for navigation handlers that target a navigation state.
@@ -74,8 +75,7 @@ private enum TransitionOutcome {
         FORWARDED, FINISHED, REROUTED, POSTPONED
     }
 
-    static final String NOT_SUPPORT_FORWARD_BEFORELEAVE =
-            "The event.forwardTo() API in beforeLeave is not supported, "
+    static final String NOT_SUPPORT_FORWARD_BEFORELEAVE = "The event.forwardTo() API in beforeLeave is not supported, "
             + "you can use the combination between postpone() and "
             + "getUI().get().getPage().setLocation(\"{}\") "
             + " API in order to forward to other location";
@@ -129,7 +129,7 @@ public NavigationState getNavigationState() {
     @SuppressWarnings("unchecked")
     // Non-private for testing purposes
     static <T extends HasElement> T getRouteTarget(Class<T> routeTargetType,
-                                                   NavigationEvent event) {
+            NavigationEvent event) {
         UI ui = event.getUI();
         Optional<HasElement> currentInstance = ui.getInternals()
                 .getActiveRouterTargetsChain().stream()
@@ -245,17 +245,16 @@ public int handle(NavigationEvent event) {
 
         if (forwardToUrl != null) {
             // Change the UI according to the navigation Component chain.
-            ui.getInternals().showRouteTarget(new Location(removeFirstSlash(forwardToUrl)),
-                    forwardToUrl, componentInstance,
-                    routerLayouts);
+            ui.getInternals().showRouteTarget(
+                    new Location(removeFirstSlash(forwardToUrl)), forwardToUrl,
+                    componentInstance, routerLayouts);
         } else {
             // Change the UI according to the navigation Component chain.
             ui.getInternals().showRouteTarget(event.getLocation(),
                     navigationState.getResolvedPath(), componentInstance,
                     routerLayouts);
         }
 
-
         updatePageTitle(event, componentInstance);
 
         int statusCode = locationChangeEvent.getStatusCode();
@@ -293,8 +292,8 @@ private String removeFirstSlash(String route) {
      *            is rerouted
      */
     protected abstract void notifyNavigationTarget(Component componentInstance,
-                                                   NavigationEvent navigationEvent, BeforeEnterEvent beforeEnterEvent,
-                                                   LocationChangeEvent locationChangeEvent);
+            NavigationEvent navigationEvent, BeforeEnterEvent beforeEnterEvent,
+            LocationChangeEvent locationChangeEvent);
 
     /**
      * Gets the router layout types to show for the given route target type,
@@ -319,9 +318,12 @@ private Optional<Integer> handleTransactionOutcome(
         if (TransitionOutcome.FORWARDED.equals(transitionOutcome)) {
             // inform that is BeforeEnterEvent
             if (beforeNavigation instanceof BeforeLeaveEvent) {
-                String forwardOnBeforeLeave = beforeNavigation.getForwardToUrl() != null
-                        ? beforeNavigation.getForwardToUrl() : beforeNavigation.getLocation().getPath();
-                getLogger().warn(NOT_SUPPORT_FORWARD_BEFORELEAVE, forwardOnBeforeLeave);
+                String forwardOnBeforeLeave = beforeNavigation
+                        .getForwardToUrl() != null
+                                ? beforeNavigation.getForwardToUrl()
+                                : beforeNavigation.getLocation().getPath();
+                getLogger().warn(NOT_SUPPORT_FORWARD_BEFORELEAVE,
+                        forwardOnBeforeLeave);
             }
             if (beforeNavigation.isUnknownRoute()) {
                 forwardToUrl = beforeNavigation.getForwardToUrl();
@@ -361,7 +363,7 @@ private void clearContinueNavigationAction(UI ui) {
     }
 
     private void storeContinueNavigationAction(UI ui,
-                                               ContinueNavigationAction currentAction) {
+            ContinueNavigationAction currentAction) {
         ContinueNavigationAction previousAction = ui.getInternals()
                 .getContinueNavigationAction();
         if (previousAction != null && previousAction != currentAction) {
@@ -373,7 +375,7 @@ private void storeContinueNavigationAction(UI ui,
     }
 
     private void fireAfterNavigationListeners(AfterNavigationEvent event,
-                                              List<AfterNavigationHandler> afterNavigationHandlers) {
+            List<AfterNavigationHandler> afterNavigationHandlers) {
         afterNavigationHandlers
                 .forEach(listener -> listener.afterNavigation(event));
     }
@@ -555,7 +557,7 @@ private Optional<TransitionOutcome> sendBeforeEnterEvent(
             // children.
             if (notifyNavigationTarget
                     && (isComponentElementEqualsOrChild(eventHandler,
-                    componentInstance))) {
+                            componentInstance))) {
 
                 Optional<TransitionOutcome> transitionOutcome = notifyNavigationTarget(
                         event, beforeNavigation, locationChangeEvent,
@@ -635,22 +637,22 @@ private Optional<TransitionOutcome> getTransitionOutcome(
             BeforeEvent beforeEvent) {
         if (beforeEvent.hasForwardTarget()
                 && !isSameNavigationState(beforeEvent.getForwardTargetType(),
-                beforeEvent.getForwardTargetParameters())
+                        beforeEvent.getForwardTargetParameters())
                 || beforeEvent.isUnknownRoute()) {
             return Optional.of(TransitionOutcome.FORWARDED);
         }
 
         if (beforeEvent.hasRerouteTarget()
                 && !isSameNavigationState(beforeEvent.getRerouteTargetType(),
-                beforeEvent.getRerouteTargetParameters())) {
+                        beforeEvent.getRerouteTargetParameters())) {
             return Optional.of(TransitionOutcome.REROUTED);
         }
 
         return Optional.empty();
     }
 
     private boolean isSameNavigationState(Class<? extends Component> targetType,
-                                          List<String> targetParameters) {
+            List<String> targetParameters) {
         final boolean sameTarget = navigationState.getNavigationTarget()
                 .equals(targetType);
 
@@ -681,7 +683,7 @@ private int reroute(NavigationEvent event, BeforeEvent beforeNavigation) {
     }
 
     private NavigationEvent getNavigationEvent(NavigationEvent event,
-                                               BeforeEvent beforeNavigation) {
+            BeforeEvent beforeNavigation) {
         if (beforeNavigation.hasErrorParameter()) {
             ErrorParameter<?> errorParameter = beforeNavigation
                     .getErrorParameter();
@@ -759,8 +761,10 @@ private Optional<ArrayList<HasElement>> getPreservedChain(
 
                 // Transfer all remaining UI child elements (typically dialogs
                 // and notifications) to the new UI
-                maybePrevUI.ifPresent(
-                        prevUi -> ui.getInternals().moveElementsFrom(prevUi));
+                maybePrevUI.ifPresent(prevUi -> {
+                    ui.getInternals().moveElementsFrom(prevUi);
+                    prevUi.close();
+                });
 
                 return Optional.of(chain);
             }
@@ -774,7 +778,7 @@ private Optional<ArrayList<HasElement>> getPreservedChain(
      * {@link #handle(NavigationEvent)} method created it.
      */
     private void setPreservedChain(ArrayList<HasElement> chain,
-                                   NavigationEvent event) {
+            NavigationEvent event) {
 
         final Location location = event.getLocation();
         final UI ui = event.getUI();
@@ -797,7 +801,7 @@ private void setPreservedChain(ArrayList<HasElement> chain,
     }
 
     private static void validateStatusCode(int statusCode,
-                                           Class<? extends Component> targetClass) {
+            Class<? extends Component> targetClass) {
         if (!statusCodes.contains(statusCode)) {
             String msg = String.format(
                     "Error state code must be a valid HttpServletResponse value. Received invalid value of '%s' for '%s'",
@@ -827,7 +831,7 @@ private static void checkForDuplicates(
     }
 
     private static void updatePageTitle(NavigationEvent navigationEvent,
-                                        Component routeTarget) {
+            Component routeTarget) {
         String title;
 
         if (routeTarget instanceof HasDynamicTitle) {
@@ -850,7 +854,7 @@ private static boolean isPreserveOnRefreshTarget(
             List<Class<? extends RouterLayout>> routeLayoutTypes) {
         return routeTargetType.isAnnotationPresent(PreserveOnRefresh.class)
                 || routeLayoutTypes.stream().anyMatch(layoutType -> layoutType
-                .isAnnotationPresent(PreserveOnRefresh.class));
+                        .isAnnotationPresent(PreserveOnRefresh.class));
     }
 
     // maps window.name to (location, chain)
@@ -865,7 +869,7 @@ static boolean hasPreservedChain(VaadinSession session) {
     }
 
     static boolean hasPreservedChainOfLocation(VaadinSession session,
-                                               Location location) {
+            Location location) {
         final PreservedComponentCache cache = session
                 .getAttribute(PreservedComponentCache.class);
         return cache != null && cache.values().stream()
@@ -885,7 +889,7 @@ static Optional<ArrayList<HasElement>> getPreservedChain(
     }
 
     static void setPreservedChain(VaadinSession session, String windowName,
-                                  Location location, ArrayList<HasElement> chain) {
+            Location location, ArrayList<HasElement> chain) {
         PreservedComponentCache cache = session
                 .getAttribute(PreservedComponentCache.class);
         if (cache == null) {

diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java
@@ -1103,7 +1103,7 @@ public void removeFromTree_nodeAttached_detachedAndDescendantsReset() {
         Assert.assertNull(parent.getParent());
 
         // then parent and its descendants are reset
-        assertNodesReset(parent,child);
+        assertNodesReset(parent, child);
     }
 
     /**
@@ -1129,7 +1129,72 @@ public void removeFromTree_nodeAttachedAndInDetachListener_detachedAndDescendant
         parent.setParent(null);
 
         // then parent and its descendants are reset
-        assertNodesReset(parent,child);
+        assertNodesReset(parent, child);
+    }
+
+    @Test
+    public void removeFromTree_closeUI_allowsToSetANewTree() {
+        UI ui = new UI();
+
+        AtomicBoolean isRootAttached = new AtomicBoolean();
+        isRootAttached.set(true);
+
+        StateNode root = new StateNode(ElementChildrenList.class) {
+            @Override
+            public boolean isAttached() {
+                return isRootAttached.get();
+            }
+
+        };
+
+        StateTree stateTree = new StateTree(ui.getInternals(),
+                ElementChildrenList.class) {
+
+            @Override
+            public StateNode getRootNode() {
+                return root;
+            }
+
+            @Override
+            public boolean hasNode(StateNode node) {
+                if (getRootNode().equals(node)) {
+                    return true;
+                }
+                return super.hasNode(node);
+            }
+        };
+
+        root.setTree(stateTree);
+
+        StateNode child = createEmptyNode("child");
+        StateNode anotherChild = createEmptyNode("anotherChild");
+
+        addChild(root, child);
+        addChild(root, anotherChild);
+
+        // remove the second child from its parent (don't remove it from the
+        // tree!)
+        removeFromParent(anotherChild);
+
+        // Once a child is added to a tree its id is not negative
+        Assert.assertNotEquals(-1, anotherChild.getId());
+
+        // Remove the first child from the tree (as it's done on preserve on
+        // refresh)
+        child.removeFromTree();
+        // emulate closed UI
+        isRootAttached.set(false);
+
+        // At this point the second child still refers to the stateTree and
+        // normally it's not allowed to move nodes from one tree to another but
+        // <code>stateTree</code> is "detached" and marked as replaced on
+        // preserve on refresh via <code>removeFromTree</code> called on another
+        // node
+        anotherChild.setTree(new TestStateTree());
+
+        // It's possible to set a new tree for the child whose owner is detached
+        // as marked replaced via preserved on refresh, its id is reset to -1
+        Assert.assertEquals(-1, anotherChild.getId());
     }
 
     private void assertNodesReset(StateNode... nodes) {

diff --git a/flow-server/src/test/java/com/vaadin/flow/router/internal/NavigationStateRendererTest.java b/flow-server/src/test/java/com/vaadin/flow/router/internal/NavigationStateRendererTest.java
@@ -361,7 +361,7 @@ public void handle_preserveOnRefresh_otherUIChildrenAreMoved() {
     }
 
     @Test
-    public void handle_preserveOnRefreshView_routerLayoutIsPreserved() {
+    public void handle_preserveOnRefreshView_routerLayoutIsPreserved_oldUiIsClosed() {
         // given a service with instantiator
         MockVaadinServletService service = createMockServiceWithInstantiator();
 
@@ -382,6 +382,10 @@ public void handle_preserveOnRefreshView_routerLayoutIsPreserved() {
         // given the session has a cache of PreservedNestedView at this location
         final PreservedLayout layout = new PreservedLayout();
         final PreservedNestedView nestedView = new PreservedNestedView();
+
+        MockUI previousUi = new MockUI(session);
+        previousUi.add(nestedView);
+
         AbstractNavigationStateRenderer.setPreservedChain(session, "ROOT.123",
                 new Location("preservedNested"),
                 new ArrayList<>(Arrays.asList(nestedView, layout)));
@@ -403,6 +407,8 @@ public void handle_preserveOnRefreshView_routerLayoutIsPreserved() {
                 ui.getInternals().getActiveRouterTargetsChain().get(0));
         Assert.assertEquals("Expected same router layout", layout,
                 ui.getInternals().getActiveRouterTargetsChain().get(1));
+
+        Assert.assertTrue(previousUi.isClosing());
     }
 
     private MockVaadinServletService createMockServiceWithInstantiator() {