From ca5c04a00355c770ba123ad1639222cd4d8a65bf Mon Sep 17 00:00:00 2001 From: Marco Collovati Date: Thu, 3 Oct 2024 17:13:20 +0200 Subject: [PATCH] fix: reload route configuration upon layout changes When a [At]Layout annotated class is modified, the changes are not propagated to the route registry after hotswap happens. This change updates Route registry layouts configuration and re-registers routes potentially impacted by the change to apply the new settings. It also checks the route target chain for active UIs in order to trigger a page refresh if the layout changes should be applied. Fixes #20111 --- .../com/vaadin/flow/hotswap/Hotswapper.java | 15 ++ .../internal/AbstractRouteRegistry.java | 15 +- .../flow/router/internal/RouteUtil.java | 32 +++- .../vaadin/flow/hotswap/HotswapperTest.java | 56 +++++++ .../flow/router/internal/RouteUtilTest.java | 158 +++++++++++++++++- 5 files changed, 273 insertions(+), 3 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java b/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java index 73776c031f8..5e183eddbb8 100644 --- a/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java +++ b/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java @@ -39,6 +39,8 @@ import com.vaadin.flow.di.Lookup; import com.vaadin.flow.internal.BrowserLiveReload; import com.vaadin.flow.internal.BrowserLiveReloadAccessor; +import com.vaadin.flow.router.internal.RouteTarget; +import com.vaadin.flow.server.RouteRegistry; import com.vaadin.flow.server.ServiceDestroyEvent; import com.vaadin.flow.server.ServiceDestroyListener; import com.vaadin.flow.server.ServiceException; @@ -309,6 +311,19 @@ private UIRefreshStrategy computeRefreshStrategy(UI ui, refreshStrategy = computeRefreshStrategyForUITree(ui, changedClasses, targetsChain, route); } + // A different layout might have been applied after hotswap + if (refreshStrategy == UIRefreshStrategy.SKIP) { + RouteRegistry registry = ui.getInternals().getRouter() + .getRegistry(); + RouteTarget routeTarget = registry + .getNavigationRouteTarget( + ui.getActiveViewLocation().getPath()) + .getRouteTarget(); + if (routeTarget != null && routeTarget.getParentLayouts().stream() + .anyMatch(changedClasses::contains)) { + refreshStrategy = UIRefreshStrategy.PUSH_REFRESH_CHAIN; + } + } // If push is not enabled we can only request a full page refresh if (refreshStrategy != UIRefreshStrategy.SKIP && !pushEnabled) { diff --git a/flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractRouteRegistry.java b/flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractRouteRegistry.java index 38893846d44..af70a7fb7fb 100644 --- a/flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractRouteRegistry.java +++ b/flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractRouteRegistry.java @@ -35,6 +35,7 @@ import com.vaadin.flow.internal.ReflectTools; import com.vaadin.flow.router.BeforeEnterListener; import com.vaadin.flow.router.HasErrorParameter; +import com.vaadin.flow.router.Layout; import com.vaadin.flow.router.Menu; import com.vaadin.flow.router.MenuData; import com.vaadin.flow.router.NotFoundException; @@ -55,7 +56,6 @@ import com.vaadin.flow.server.auth.NavigationAccessControl; import com.vaadin.flow.server.auth.NavigationContext; import com.vaadin.flow.server.auth.ViewAccessChecker; -import com.vaadin.flow.router.Layout; import com.vaadin.flow.server.menu.MenuRegistry; import com.vaadin.flow.shared.Registration; @@ -610,6 +610,19 @@ public void setLayout(Class layout) { } } + void updateLayout(Class layout) { + if (layout == null) { + return; + } + synchronized (layouts) { + layouts.entrySet() + .removeIf(entry -> layout.equals(entry.getValue())); + if (layout.isAnnotationPresent(Layout.class)) { + layouts.put(layout.getAnnotation(Layout.class).value(), layout); + } + } + } + @Override public Class getLayout(String path) { Optional first = layouts.keySet().stream() diff --git a/flow-server/src/main/java/com/vaadin/flow/router/internal/RouteUtil.java b/flow-server/src/main/java/com/vaadin/flow/router/internal/RouteUtil.java index f411ef6fe75..f24ebb43dc8 100644 --- a/flow-server/src/main/java/com/vaadin/flow/router/internal/RouteUtil.java +++ b/flow-server/src/main/java/com/vaadin/flow/router/internal/RouteUtil.java @@ -41,6 +41,7 @@ import com.vaadin.flow.router.ParentLayout; import com.vaadin.flow.router.Route; import com.vaadin.flow.router.RouteAlias; +import com.vaadin.flow.router.RouteBaseData; import com.vaadin.flow.router.RouteConfiguration; import com.vaadin.flow.router.RoutePathProvider; import com.vaadin.flow.router.RoutePrefix; @@ -125,7 +126,7 @@ public static List> getParentLayouts( if (hasRouteAndPathMatches && !route.get().layout().equals(UI.class)) { list.addAll(collectRouteParentLayouts(route.get().layout())); - } else if (route.get().autoLayout() && hasRouteAndPathMatches + } else if (hasRouteAndPathMatches && route.get().autoLayout() && route.get().layout().equals(UI.class) && handledRegistry.hasLayout(path)) { list.addAll( @@ -435,11 +436,31 @@ public static void updateRouteRegistry(RouteRegistry registry, modifiedClasses.stream() .filter(clazz -> !Component.class.isAssignableFrom(clazz)) .forEach(nonFlowComponentsToRemove::add); + Set> layouts = new HashSet<>(); boolean isSessionRegistry = registry instanceof SessionRouteRegistry; Predicate> modifiedClassesRouteRemovalFilter = clazz -> !isSessionRegistry; if (registry instanceof AbstractRouteRegistry abstractRouteRegistry) { + + // update layouts + filterLayoutClasses(deletedClasses).forEach(layouts::add); + filterLayoutClasses(modifiedClasses).forEach(layouts::add); + filterLayoutClasses(addedClasses).forEach(layouts::add); + layouts.forEach(abstractRouteRegistry::updateLayout); + if (!layouts.isEmpty()) { + // Gather routes that don't have a layout or reference a layout + // that has been changed. + // Mark these routes as modified so they can be re-registered + // with the correct layouts applied. + registry.getRegisteredRoutes().stream() + .filter(rd -> rd.getParentLayouts().isEmpty() + || rd.getParentLayouts().stream() + .anyMatch(layouts::contains)) + .map(RouteBaseData::getNavigationTarget) + .forEach(modifiedClasses::add); + } + Map routesMap = abstractRouteRegistry .getConfiguration().getRoutesMap(); Map, RouteTarget> routeTargets = registry @@ -447,6 +468,7 @@ public static void updateRouteRegistry(RouteRegistry registry, .map(routeData -> routesMap.get(routeData.getTemplate())) .filter(Objects::nonNull).collect(Collectors.toMap( RouteTarget::getTarget, Function.identity())); + modifiedClassesRouteRemovalFilter = modifiedClassesRouteRemovalFilter .and(clazz -> { RouteTarget routeTarget = routeTargets.get(clazz); @@ -522,6 +544,14 @@ public static void updateRouteRegistry(RouteRegistry registry, }); } + @SuppressWarnings("unchecked") + private static Stream> filterLayoutClasses( + Set> classes) { + return filterComponentClasses(classes) + .filter(RouterLayout.class::isAssignableFrom) + .map(clazz -> (Class) clazz); + } + @SuppressWarnings("unchecked") private static Stream> filterComponentClasses( Set> classes) { diff --git a/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java b/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java index a17fe5dd2b5..c5518832c74 100644 --- a/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java @@ -38,9 +38,11 @@ import com.vaadin.flow.di.Lookup; import com.vaadin.flow.internal.BrowserLiveReload; import com.vaadin.flow.internal.BrowserLiveReloadAccessor; +import com.vaadin.flow.router.RouteConfiguration; import com.vaadin.flow.router.RouterLayout; import com.vaadin.flow.server.MockVaadinServletService; import com.vaadin.flow.server.MockVaadinSession; +import com.vaadin.flow.server.RouteRegistry; import com.vaadin.flow.server.ServiceDestroyEvent; import com.vaadin.flow.server.ServiceDestroyListener; import com.vaadin.flow.server.ServiceException; @@ -281,6 +283,32 @@ public void onHotswap_pushDisabled_routeLayoutClassChanged_UINotRefreshedButLive Mockito.verify(liveReload).refresh(anyBoolean()); } + @Test + public void onHotswap_pushDisabled_routeTargetChainChanged_UINotRefreshedButLiveReloadTriggered() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + + class NewLayout extends Component implements RouterLayout { + } + RefreshTestingUI ui = initUIAndNavigateTo(session, MyRoute.class, + MyLayout.class); + RouteRegistry registry = ui.getInternals().getRouter().getRegistry(); + RouteConfiguration routeConfiguration = RouteConfiguration + .forRegistry(registry); + routeConfiguration.update(() -> { + String path = ui.getActiveViewLocation().getPath(); + routeConfiguration.removeRoute(path); + routeConfiguration.setRoute(path, MyRoute.class, + List.of(NewLayout.class)); + }); + + hotswapper.onHotswap(new String[] { NewLayout.class.getName() }, true); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload).refresh(anyBoolean()); + } + @Test public void onHotswap_pushDisabled_routeChildClassChanged_UINotRefreshedButLiveReloadTriggered() throws ServiceException { @@ -389,6 +417,34 @@ public void onHotswap_pushEnabled_routeLayoutClassChanged_activeChainRefreshed() Mockito.verify(liveReload, never()).refresh(anyBoolean()); } + @Test + public void onHotswap_pushEnabled_routeTargetChainChanged_activeChainRefreshed() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + + class NewLayout extends Component implements RouterLayout { + } + RefreshTestingUI ui = initUIAndNavigateTo(session, MyRoute.class, + MyLayout.class); + ui.enablePush(); + RouteRegistry registry = ui.getInternals().getRouter().getRegistry(); + RouteConfiguration routeConfiguration = RouteConfiguration + .forRegistry(registry); + routeConfiguration.update(() -> { + String path = ui.getActiveViewLocation().getPath(); + routeConfiguration.removeRoute(path); + routeConfiguration.setRoute(path, MyRoute.class, + List.of(NewLayout.class)); + }); + + hotswapper.onHotswap(new String[] { NewLayout.class.getName() }, true); + + ui.assertChainRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushEnabled_routeChildrenClassChanged_routeRefreshed() throws ServiceException { diff --git a/flow-server/src/test/java/com/vaadin/flow/router/internal/RouteUtilTest.java b/flow-server/src/test/java/com/vaadin/flow/router/internal/RouteUtilTest.java index ee3bd95d85a..6ead27eeb49 100644 --- a/flow-server/src/test/java/com/vaadin/flow/router/internal/RouteUtilTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/router/internal/RouteUtilTest.java @@ -20,6 +20,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Consumer; import org.hamcrest.MatcherAssert; import org.hamcrest.collection.IsIterableContainingInOrder; @@ -28,16 +29,18 @@ import com.vaadin.flow.component.Component; import com.vaadin.flow.component.Tag; +import com.vaadin.flow.internal.ReflectTools; +import com.vaadin.flow.router.Layout; import com.vaadin.flow.router.ParentLayout; import com.vaadin.flow.router.Route; import com.vaadin.flow.router.RouteAlias; +import com.vaadin.flow.router.RouteConfiguration; import com.vaadin.flow.router.RoutePrefix; import com.vaadin.flow.router.RouterLayout; import com.vaadin.flow.server.MockVaadinContext; import com.vaadin.flow.server.MockVaadinServletService; import com.vaadin.flow.server.SessionRouteRegistry; import com.vaadin.flow.server.VaadinContext; -import com.vaadin.flow.router.Layout; import com.vaadin.flow.server.startup.ApplicationRouteRegistry; import com.vaadin.tests.util.AlwaysLockedVaadinSession; @@ -916,6 +919,159 @@ public VaadinContext getContext() { Assert.assertTrue(registry.getConfiguration().hasRoute("aa")); } + @Test + public void newLayoutAnnotatedComponent_updateRouteRegistry_routeIsUpdated() { + MockVaadinServletService service = new MockVaadinServletService() { + @Override + public VaadinContext getContext() { + return new MockVaadinContext(); + } + }; + ApplicationRouteRegistry registry = ApplicationRouteRegistry + .getInstance(service.getContext()); + registry.update(() -> { + RouteConfiguration routeConfiguration = RouteConfiguration + .forRegistry(registry); + routeConfiguration.setAnnotatedRoute(AutoLayoutView.class); + }); + + List> parentLayouts = registry + .getNavigationRouteTarget("auto").getRouteTarget() + .getParentLayouts(); + + Assert.assertEquals( + "AutoLayoutView should not have layout because no auto layout is registered", + 0, parentLayouts.size()); + + RouteUtil.updateRouteRegistry(registry, + Collections.singleton(AutoLayout.class), Collections.emptySet(), + Collections.emptySet()); + + parentLayouts = registry.getNavigationRouteTarget("auto") + .getRouteTarget().getParentLayouts(); + Assert.assertEquals("AutoLayoutView should now get automatic layout", 1, + parentLayouts.size()); + Assert.assertEquals( + "AutoLayoutView layout should be the @Layout annotated RouterLayout", + AutoLayout.class, parentLayouts.get(0)); + } + + @Test + public void removeAnnotationsFromLayoutAnnotatedComponent_updateRouteRegistry_routeIsUpdated() { + + MockVaadinServletService service = new MockVaadinServletService() { + @Override + public VaadinContext getContext() { + return new MockVaadinContext(); + } + }; + class A extends Component implements RouterLayout { + } + ApplicationRouteRegistry registry = ApplicationRouteRegistry + .getInstance(service.getContext()); + tamperLayouts(registry, layouts -> { + layouts.put("/", A.class); + }); + registry.update(() -> { + RouteConfiguration.forRegistry(registry) + .setAnnotatedRoute(AutoLayoutView.class); + }); + List> parentLayouts = registry + .getNavigationRouteTarget("auto").getRouteTarget() + .getParentLayouts(); + Assert.assertEquals( + "Route should have layout because auto layout is registered", 1, + parentLayouts.size()); + Assert.assertEquals( + "Layout should be the @Layout annotated RouterLayout", A.class, + parentLayouts.get(0)); + + RouteUtil.updateRouteRegistry(registry, Collections.singleton(A.class), + Collections.emptySet(), Collections.emptySet()); + + parentLayouts = registry.getNavigationRouteTarget("auto") + .getRouteTarget().getParentLayouts(); + Assert.assertEquals( + "Route with no layout should not get automatic layout now", 0, + parentLayouts.size()); + } + + @Test + public void layoutAnnotatedComponent_modifiedValue_updateRouteRegistry_routeIsUpdated() { + + MockVaadinServletService service = new MockVaadinServletService() { + @Override + public VaadinContext getContext() { + return new MockVaadinContext(); + } + }; + + @Route("hey/view") + class View extends Component { + + } + ApplicationRouteRegistry registry = ApplicationRouteRegistry + .getInstance(service.getContext()); + tamperLayouts(registry, layouts -> { + layouts.put("/hey", AutoLayout.class); + }); + registry.update(() -> { + RouteConfiguration routeConfiguration = RouteConfiguration + .forRegistry(registry); + routeConfiguration.setAnnotatedRoute(AutoLayoutView.class); + routeConfiguration.setAnnotatedRoute(View.class); + }); + + List> parentLayouts = registry + .getNavigationRouteTarget("auto").getRouteTarget() + .getParentLayouts(); + Assert.assertEquals( + "AutoLayoutView should not have layout because path does not match", + 0, parentLayouts.size()); + parentLayouts = registry.getNavigationRouteTarget("hey/view") + .getRouteTarget().getParentLayouts(); + Assert.assertEquals("View should have layout because path matches", 1, + parentLayouts.size()); + Assert.assertEquals( + "View layout should be the @Layout annotated RouterLayout", + AutoLayout.class, parentLayouts.get(0)); + + RouteUtil.updateRouteRegistry(registry, + Collections.singleton(AutoLayout.class), Collections.emptySet(), + Collections.emptySet()); + + parentLayouts = registry.getNavigationRouteTarget("auto") + .getRouteTarget().getParentLayouts(); + Assert.assertEquals("AutoLayoutView should now get automatic layout", 1, + parentLayouts.size()); + Assert.assertEquals( + "AutoLayoutView layout should be the @Layout annotated RouterLayout", + AutoLayout.class, parentLayouts.get(0)); + + parentLayouts = registry.getNavigationRouteTarget("hey/view") + .getRouteTarget().getParentLayouts(); + Assert.assertEquals( + "View should still have layout because path matches", 1, + parentLayouts.size()); + Assert.assertEquals( + "View layout should be the @Layout annotated RouterLayout", + AutoLayout.class, parentLayouts.get(0)); + } + + @SuppressWarnings("unchecked") + private void tamperLayouts(ApplicationRouteRegistry registry, + Consumer>> consumer) { + try { + Field layoutsField = AbstractRouteRegistry.class + .getDeclaredField("layouts"); + Map> layouts = (Map>) ReflectTools + .getJavaFieldValue(registry, layoutsField); + consumer.accept(layouts); + } catch (Exception ex) { + Assert.fail(ex.getMessage()); + } + } + private static void mutableRoutesMap(AbstractRouteRegistry registry) { ConfiguredRoutes configuration = registry.getConfiguration(); try {