From a006792ddce25a7f0bce4f84877a05f4aaef3b36 Mon Sep 17 00:00:00 2001 From: Tomi Virtanen Date: Fri, 11 Oct 2024 16:47:30 +0300 Subject: [PATCH] fix: correct route path for MenuEntry MenuEntry should not have route parameters included in the path for client routes. It should omit route path in same way as with Hilla automatic menu. Menu should not include nested routes for excluded menu item or for route with required parameter. --- .../flow/internal/menu/MenuRegistry.java | 54 +++++++++-- .../server/menu/MenuConfigurationTest.java | 17 ++-- .../flow/server/menu/MenuRegistryTest.java | 89 +++++++++++++++---- 3 files changed, 130 insertions(+), 30 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/menu/MenuRegistry.java b/flow-server/src/main/java/com/vaadin/flow/internal/menu/MenuRegistry.java index 76eac093ea1..887da2db67e 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/menu/MenuRegistry.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/menu/MenuRegistry.java @@ -35,6 +35,7 @@ import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; +import java.util.stream.Stream; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.DeserializationFeature; @@ -82,10 +83,8 @@ public class MenuRegistry { public static Map collectMenuItems() { Map menuRoutes = MenuRegistry .getMenuItems(true); - menuRoutes.entrySet() - .removeIf(entry -> Optional.ofNullable(entry.getValue()) - .map(AvailableViewInfo::menu).map(MenuData::isExclude) - .orElse(false)); + filterMenuItems(menuRoutes); + return menuRoutes; } @@ -115,7 +114,8 @@ public static List collectMenuItemsList(Locale locale) { return collectMenuItems().entrySet().stream().map(entry -> { AvailableViewInfo value = entry.getValue(); return new AvailableViewInfo(value.title(), value.rolesAllowed(), - value.loginRequired(), entry.getKey(), value.lazy(), + value.loginRequired(), + getMenuLink(entry.getValue(), entry.getKey()), value.lazy(), value.register(), value.menu(), value.children(), value.routeParameters(), value.flowLayout()); }).sorted(getMenuOrderComparator( @@ -437,6 +437,17 @@ private static void removeChildren( } } + private static boolean hasRequiredParameter(AvailableViewInfo viewInfo) { + final Map routeParameters = viewInfo + .routeParameters(); + if (routeParameters != null && !routeParameters.isEmpty() + && routeParameters.values().stream().anyMatch( + paramType -> paramType == RouteParamType.REQUIRED)) { + return true; + } + return false; + } + /** * Check view against authentication state. *

@@ -540,4 +551,37 @@ private static Comparator getMenuOrderComparator( : collator.compare(o1.route(), o2.route()); }; } + + private static String getMenuLink(AvailableViewInfo info, + String defaultMenuLink) { + if (info.routeParameters() == null + || info.routeParameters().isEmpty()) { + return (defaultMenuLink.startsWith("/")) ? defaultMenuLink + : "/" + defaultMenuLink; + } + // menu link with omitted route parameters + final var parameterNames = info.routeParameters().keySet(); + return Stream.of(defaultMenuLink.split("/")).filter( + part -> parameterNames.stream().noneMatch(part::startsWith)) + .collect(Collectors.joining("/")); + } + + private static void filterMenuItems( + Map menuRoutes) { + for (var path : new HashSet<>(menuRoutes.keySet())) { + if (!menuRoutes.containsKey(path)) { + continue; + } + var viewInfo = menuRoutes.get(path); + // Remove following, including nested ones: + // - routes with required parameters + // - routes with exclude=true + if (viewInfo.menu().isExclude() || hasRequiredParameter(viewInfo)) { + menuRoutes.remove(path); + if (viewInfo.children() != null) { + removeChildren(menuRoutes, viewInfo, path); + } + } + } + } } diff --git a/flow-server/src/test/java/com/vaadin/flow/server/menu/MenuConfigurationTest.java b/flow-server/src/test/java/com/vaadin/flow/server/menu/MenuConfigurationTest.java index 863b3ea9e5b..b6a128c4504 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/menu/MenuConfigurationTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/menu/MenuConfigurationTest.java @@ -143,9 +143,9 @@ public void testWithLoggedInUser_userHasRoles() throws IOException { List menuEntries = MenuConfiguration.getMenuEntries(); Assert.assertEquals( "List of menu items has incorrect size. Excluded menu item like /login is not expected.", - 4, menuEntries.size()); - assertOrder(menuEntries, - new String[] { "", "/about", "/hilla", "/hilla/sub" }); + 7, menuEntries.size()); + assertOrder(menuEntries, new String[] { "/", "/about", "/hilla", + "/hilla/sub", "/opt_params", "/params", "/wc_params" }); } @Test @@ -166,9 +166,10 @@ public void getMenuItemsList_returnsCorrectPaths() throws IOException { .forEach(routeConfiguration::setAnnotatedRoute); List menuEntries = MenuConfiguration.getMenuEntries(); - Assert.assertEquals(5, menuEntries.size()); - assertOrder(menuEntries, new String[] { "", "/home", "/info", "/param", - "/param/varargs" }); + Assert.assertEquals(8, menuEntries.size()); + assertOrder(menuEntries, + new String[] { "/", "/home", "/info", "/opt_params", "/param", + "/param/varargs", "/params", "/wc_params" }); Map mapMenuItems = menuEntries.stream() .collect(Collectors.toMap(MenuEntry::path, item -> item)); @@ -385,8 +386,8 @@ private void assertOrder(List menuEntries, private void assertClientRoutes(Map menuOptions, boolean authenticated, boolean hasRole) { Assert.assertTrue("Client route '' missing", - menuOptions.containsKey("")); - Assert.assertEquals("Public", menuOptions.get("").title()); + menuOptions.containsKey("/")); + Assert.assertEquals("Public", menuOptions.get("/").title()); if (authenticated) { Assert.assertTrue("Client route 'about' missing", diff --git a/flow-server/src/test/java/com/vaadin/flow/server/menu/MenuRegistryTest.java b/flow-server/src/test/java/com/vaadin/flow/server/menu/MenuRegistryTest.java index 36078d7b620..bb2dcd582bc 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/menu/MenuRegistryTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/menu/MenuRegistryTest.java @@ -130,7 +130,7 @@ public void getMenuItemsContainsExpectedClientPaths() throws IOException { Map menuItems = new MenuRegistry() .getMenuItems(true); - Assert.assertEquals(2, menuItems.size()); + Assert.assertEquals(8, menuItems.size()); assertClientRoutes(menuItems); } @@ -157,7 +157,7 @@ public void getMenuItemsNoFilteringContainsAllClientPaths() Map menuItems = new MenuRegistry() .getMenuItems(false); - Assert.assertEquals(5, menuItems.size()); + Assert.assertEquals(11, menuItems.size()); // Validate as if logged in as all routes should be available assertClientRoutes(menuItems, true, true, false); } @@ -184,7 +184,7 @@ public void productionMode_getMenuItemsContainsExpectedClientPaths() Map menuItems = new MenuRegistry() .getMenuItems(true); - Assert.assertEquals(2, menuItems.size()); + Assert.assertEquals(8, menuItems.size()); assertClientRoutes(menuItems); } } @@ -218,7 +218,7 @@ public void getMenuItemsContainBothClientAndServerPaths() Map menuItems = new MenuRegistry() .getMenuItems(true); - Assert.assertEquals(4, menuItems.size()); + Assert.assertEquals(10, menuItems.size()); assertClientRoutes(menuItems); assertServerRoutes(menuItems); } @@ -239,7 +239,7 @@ public void collectMenuItems_returnsCorrectPaths() throws IOException { Map menuItems = MenuRegistry .collectMenuItems(); - Assert.assertEquals(5, menuItems.size()); + Assert.assertEquals(8, menuItems.size()); assertClientRoutes(menuItems, false, false, true); assertServerRoutes(menuItems); assertServerRoutesWithParameters(menuItems, true); @@ -259,7 +259,7 @@ public void testWithLoggedInUser_userHasRoles() throws IOException { Map menuItems = new MenuRegistry() .getMenuItems(true); - Assert.assertEquals(5, menuItems.size()); + Assert.assertEquals(11, menuItems.size()); assertClientRoutes(menuItems, true, true, false); // Verify that getMenuItemsList returns the same data @@ -267,9 +267,9 @@ public void testWithLoggedInUser_userHasRoles() throws IOException { .collectMenuItemsList(); Assert.assertEquals( "List of menu items has incorrect size. Excluded menu item like /login is not expected.", - 4, menuItemsList.size()); - assertOrder(menuItemsList, - new String[] { "", "/about", "/hilla", "/hilla/sub" }); + 7, menuItemsList.size()); + assertOrder(menuItemsList, new String[] { "/", "/about", "/hilla", + "/hilla/sub", "/opt_params", "/params", "/wc_params" }); } @Test @@ -286,7 +286,7 @@ public void testWithLoggedInUser_noMatchingRoles() throws IOException { Map menuItems = new MenuRegistry() .getMenuItems(true); - Assert.assertEquals(3, menuItems.size()); + Assert.assertEquals(9, menuItems.size()); assertClientRoutes(menuItems, true, false, false); } @@ -304,14 +304,15 @@ public void getMenuItemsList_returnsCorrectPaths() throws IOException { .forEach(routeConfiguration::setAnnotatedRoute); List menuItems = MenuRegistry.collectMenuItemsList(); - Assert.assertEquals(5, menuItems.size()); - assertOrder(menuItems, new String[] { "", "/home", "/info", "/param", - "/param/varargs" }); + Assert.assertEquals(8, menuItems.size()); + assertOrder(menuItems, + new String[] { "/", "/home", "/info", "/opt_params", "/param", + "/param/varargs", "/params", "/wc_params" }); // verifying that data is same as with collectMenuItems Map mapMenuItems = menuItems.stream() .collect(Collectors.toMap(AvailableViewInfo::route, item -> item)); - assertClientRoutes(mapMenuItems, false, false, true); + assertClientRoutes(mapMenuItems, false, false, true, "/"); assertServerRoutes(mapMenuItems); assertServerRoutesWithParameters(mapMenuItems, true); } @@ -343,10 +344,18 @@ private void assertClientRoutes(Map menuItems) { private void assertClientRoutes(Map menuItems, boolean authenticated, boolean hasRole, boolean excludeExpected) { - Assert.assertTrue("Client route '' missing", menuItems.containsKey("")); - Assert.assertEquals("Public", menuItems.get("").title()); + assertClientRoutes(menuItems, authenticated, hasRole, excludeExpected, + ""); + } + + private void assertClientRoutes(Map menuItems, + boolean authenticated, boolean hasRole, boolean excludeExpected, + String expectedRootPath) { + Assert.assertTrue("Client route '" + expectedRootPath + "' missing", + menuItems.containsKey(expectedRootPath)); + Assert.assertEquals("Public", menuItems.get(expectedRootPath).title()); Assert.assertNotNull("Public should contain default menu data", - menuItems.get("").menu()); + menuItems.get(expectedRootPath).menu()); if (authenticated) { Assert.assertTrue("Client route 'about' missing", @@ -576,6 +585,52 @@ public Instantiator getInstantiator() { } ] }, + { + "route": "wc_params/:param?", + "loginRequired": false, + "params": { + ":param": "*" + }, + "title": "wc_params path is included in menu" + }, + { + "route": "opt_params/:param?", + "loginRequired": false, + "params": { + ":param": "opt" + }, + "title": "opt_params path is included in menu" + }, + { + "route": "req_params/:param", + "loginRequired": false, + "params": { + ":param": "req" + }, + "title": "req_params path is excluded from menu" + }, + { + "route": "params", + "loginRequired": false, + "title": "params path is shown in menu", + "children": [ + { + "route": ":param", + "loginRequired": false, + "params": { + ":param": "req" + }, + "title": "params/:param path is excluded from menu", + "children": [ + { + "route": "sub", + "loginRequired": false, + "title": "params/:param/sub path is excluded from menu" + } + ] + } + ] + }, { "route": "login", "menu": {