Skip to content

Commit

Permalink
fix: correct route path for MenuEntry (#20235) (#20238)
Browse files Browse the repository at this point in the history 
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.

Co-authored-by: Tomi Virtanen <tltv@vaadin.com>
  • Loading branch information
@vaadin-bot @tltv
vaadin-bot and tltv authored Oct 14, 2024
1 parent 4a9d126 commit dad2795
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 30 deletions.
54 changes: 49 additions & 5 deletions flow-server/src/main/java/com/vaadin/flow/internal/menu/MenuRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -82,10 +83,8 @@ public class MenuRegistry {
public static Map<String, AvailableViewInfo> collectMenuItems() {
Map<String, AvailableViewInfo> menuRoutes = MenuRegistry
.getMenuItems(true);
menuRoutes.entrySet()
.removeIf(entry -> Optional.ofNullable(entry.getValue())
.map(AvailableViewInfo::menu).map(MenuData::isExclude)
.orElse(false));
filterMenuItems(menuRoutes);

return menuRoutes;
}

Expand Down Expand Up @@ -115,7 +114,8 @@ public static List<AvailableViewInfo> 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(
Expand Down Expand Up @@ -437,6 +437,17 @@ private static void removeChildren(
}
}

private static boolean hasRequiredParameter(AvailableViewInfo viewInfo) {
final Map<String, RouteParamType> 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.
* <p>
Expand Down Expand Up @@ -540,4 +551,37 @@ private static Comparator<AvailableViewInfo> 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<String, AvailableViewInfo> 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);
}
}
}
}
}
17 changes: 9 additions & 8 deletions flow-server/src/test/java/com/vaadin/flow/server/menu/MenuConfigurationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ public void testWithLoggedInUser_userHasRoles() throws IOException {
List<MenuEntry> 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
Expand All @@ -166,9 +166,10 @@ public void getMenuItemsList_returnsCorrectPaths() throws IOException {
.forEach(routeConfiguration::setAnnotatedRoute);

List<MenuEntry> 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<String, MenuEntry> mapMenuItems = menuEntries.stream()
.collect(Collectors.toMap(MenuEntry::path, item -> item));
Expand Down Expand Up @@ -385,8 +386,8 @@ private void assertOrder(List<MenuEntry> menuEntries,
private void assertClientRoutes(Map<String, MenuEntry> menuOptions,
boolean authenticated, boolean hasRole, boolean excludeExpected) {
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",
Expand Down
89 changes: 72 additions & 17 deletions flow-server/src/test/java/com/vaadin/flow/server/menu/MenuRegistryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public void getMenuItemsContainsExpectedClientPaths() throws IOException {
Map<String, AvailableViewInfo> menuItems = new MenuRegistry()
.getMenuItems(true);

Assert.assertEquals(2, menuItems.size());
Assert.assertEquals(8, menuItems.size());
assertClientRoutes(menuItems);
}

Expand All @@ -157,7 +157,7 @@ public void getMenuItemsNoFilteringContainsAllClientPaths()
Map<String, AvailableViewInfo> 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);
}
Expand All @@ -184,7 +184,7 @@ public void productionMode_getMenuItemsContainsExpectedClientPaths()
Map<String, AvailableViewInfo> menuItems = new MenuRegistry()
.getMenuItems(true);

Assert.assertEquals(2, menuItems.size());
Assert.assertEquals(8, menuItems.size());
assertClientRoutes(menuItems);
}
}
Expand Down Expand Up @@ -218,7 +218,7 @@ public void getMenuItemsContainBothClientAndServerPaths()
Map<String, AvailableViewInfo> menuItems = new MenuRegistry()
.getMenuItems(true);

Assert.assertEquals(4, menuItems.size());
Assert.assertEquals(10, menuItems.size());
assertClientRoutes(menuItems);
assertServerRoutes(menuItems);
}
Expand All @@ -239,7 +239,7 @@ public void collectMenuItems_returnsCorrectPaths() throws IOException {
Map<String, AvailableViewInfo> menuItems = MenuRegistry
.collectMenuItems();

Assert.assertEquals(5, menuItems.size());
Assert.assertEquals(8, menuItems.size());
assertClientRoutes(menuItems, false, false, true);
assertServerRoutes(menuItems);
assertServerRoutesWithParameters(menuItems, true);
Expand All @@ -259,17 +259,17 @@ public void testWithLoggedInUser_userHasRoles() throws IOException {
Map<String, AvailableViewInfo> 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
List<AvailableViewInfo> menuItemsList = MenuRegistry
.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
Expand All @@ -286,7 +286,7 @@ public void testWithLoggedInUser_noMatchingRoles() throws IOException {
Map<String, AvailableViewInfo> menuItems = new MenuRegistry()
.getMenuItems(true);

Assert.assertEquals(3, menuItems.size());
Assert.assertEquals(9, menuItems.size());
assertClientRoutes(menuItems, true, false, false);
}

Expand All @@ -304,14 +304,15 @@ public void getMenuItemsList_returnsCorrectPaths() throws IOException {
.forEach(routeConfiguration::setAnnotatedRoute);

List<AvailableViewInfo> 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<String, AvailableViewInfo> 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);
}
Expand Down Expand Up @@ -343,10 +344,18 @@ private void assertClientRoutes(Map<String, AvailableViewInfo> menuItems) {

private void assertClientRoutes(Map<String, AvailableViewInfo> 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<String, AvailableViewInfo> 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",
Expand Down Expand Up @@ -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": {
Expand Down

0 comments on commit dad2795

Please sign in to comment.