fix: client navigation for server layout

clean error messages. Fixes vaadin/start#3169
vaadin · Sep 5, 2024 · 60942bd · 60942bd
1 parent 912ecec
commit 60942bd
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 20 deletions.
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
@@ -177,7 +177,13 @@ public int handle(NavigationEvent event) {
 
         // If navigation target is Hilla route, terminate Flow navigation logic
         // here.
-        if (MenuRegistry.hasClientRoute(event.getLocation().getPath(), true)) {
+        String route = event.getLocation().getPath().isEmpty()
+                ? event.getLocation().getPath()
+                : event.getLocation().getPath().startsWith("/")
+                        ? event.getLocation().getPath()
+                        : "/" + event.getLocation().getPath();
+        if (MenuRegistry.hasClientRoute(route, true) && !MenuRegistry
+                .getClientRoutes(true).get(route).flowLayout()) {
             return HttpStatusCode.OK.getCode();
         }
 

diff --git a/flow-server/src/main/java/com/vaadin/flow/router/internal/DefaultRouteResolver.java b/flow-server/src/main/java/com/vaadin/flow/router/internal/DefaultRouteResolver.java
@@ -55,7 +55,7 @@ public NavigationState resolve(ResolveRequest request) {
                         .getLayout(path);
                 if (layout == null) {
                     throw new NotFoundException(
-                            "No layout for client path " + path);
+                            "No layout for client path '%s'".formatted(path));
                 }
                 RouteTarget target = new RouteTarget(layout,
                         Collections.emptyList());

diff --git a/flow-server/src/main/java/com/vaadin/flow/server/menu/MenuRegistry.java b/flow-server/src/main/java/com/vaadin/flow/server/menu/MenuRegistry.java
@@ -31,10 +31,12 @@
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import com.vaadin.flow.component.Component;
@@ -60,6 +62,9 @@
  */
 public class MenuRegistry {
 
+    private static final Logger log = LoggerFactory
+            .getLogger(MenuRegistry.class);
+
     /**
      * Collect views with menu annotation for automatic menu population. All
      * client views are collected and any accessible server views.
@@ -430,24 +435,36 @@ public static boolean hasClientRoute(String route) {
      * @return true if a client route is found.
      */
     public static boolean hasClientRoute(String route, boolean excludeLayouts) {
-        if (VaadinSession.getCurrent() == null || route == null) {
+        if (route == null) {
             return false;
         }
         route = route.isEmpty() ? route
                 : route.startsWith("/") ? route : "/" + route;
+        return getClientRoutes(excludeLayouts).containsKey(route);
+    }
+
+    /**
+     * Get available client routes, optionally excluding any layout targets.
+     *
+     * @param excludeLayouts
+     *            {@literal true} to exclude layouts from the check,
+     *            {@literal false} to include them
+     * @return Map of client routes available
+     */
+    public static Map<String, AvailableViewInfo> getClientRoutes(
+            boolean excludeLayouts) {
+        if (VaadinSession.getCurrent() == null) {
+            return Collections.emptyMap();
+        }
         Map<String, AvailableViewInfo> clientItems = MenuRegistry
                 .collectClientMenuItems(true,
                         VaadinSession.getCurrent().getConfiguration());
-        final Set<String> clientRoutes = new HashSet<>();
-        clientItems.forEach((path, info) -> {
-            if (excludeLayouts) {
-                if (info.children() == null || info.children().isEmpty()) {
-                    clientRoutes.add(path);
-                }
-            } else {
-                clientRoutes.add(path);
-            }
-        });
-        return clientRoutes.contains(route);
+        if (excludeLayouts) {
+            clientItems = clientItems.entrySet().stream()
+                    .filter(entry -> entry.getValue().children() == null)
+                    .collect(Collectors.toMap(Map.Entry::getKey,
+                            Map.Entry::getValue));
+        }
+        return clientItems;
     }
 }
diff --git a/flow-server/src/main/java/com/vaadin/flow/server/startup/RouteRegistryInitializer.java b/flow-server/src/main/java/com/vaadin/flow/server/startup/RouteRegistryInitializer.java
@@ -113,7 +113,7 @@ public static void validateLayoutAnnotations(Set<Class<?>> routesSet)
                 .filter(clazz -> !RouterLayout.class.isAssignableFrom(clazz))
                 .collect(Collectors.toList());
         if (!faultyLayouts.isEmpty()) {
-            String message = "Found @Layout on classes { %s } not extending RouterLayout.";
+            String message = "Found @Layout on classes { %s } not implementing RouterLayout.";
             String faultyLayoutsString = faultyLayouts.stream()
                     .map(clazz -> clazz.getName())
                     .collect(Collectors.joining(","));

diff --git a/flow-server/src/test/java/com/vaadin/flow/router/DefaultRouteResolverTest.java b/flow-server/src/test/java/com/vaadin/flow/router/DefaultRouteResolverTest.java
@@ -21,7 +21,9 @@
 import java.util.stream.Stream;
 
 import org.junit.Assert;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.mockito.MockedStatic;
 import org.mockito.Mockito;
 
@@ -36,6 +38,9 @@
 
 public class DefaultRouteResolverTest extends RoutingTestBase {
 
+    @Rule
+    public ExpectedException expectedEx = ExpectedException.none();
+
     private RouteResolver resolver;
 
     @Override
@@ -136,6 +141,22 @@ public void clientRouteRequest_getDefinedLayout() {
         }
     }
 
+    @Test
+    public void clientRouteRequest_noLayoutForPath_Throws() {
+        expectedEx.expect(NotFoundException.class);
+        expectedEx.expectMessage("No layout for client path 'route'");
+
+        String path = "route";
+
+        try (MockedStatic<MenuRegistry> menuRegistry = Mockito
+                .mockStatic(MenuRegistry.class)) {
+            menuRegistry.when(() -> MenuRegistry.hasClientRoute(path))
+                    .thenReturn(true);
+
+            NavigationState greeting = resolveNavigationState(path);
+        }
+    }
+
     @Tag("div")
     @Layout
     private static class DefaultLayout extends Component

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
@@ -27,6 +27,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Collectors;
 
+import com.fasterxml.jackson.annotation.JsonProperty;
 import net.bytebuddy.ByteBuddy;
 import net.bytebuddy.description.modifier.SyntheticState;
 import net.bytebuddy.description.modifier.Visibility;
@@ -79,7 +80,9 @@
 import com.vaadin.flow.server.RouteRegistry;
 import com.vaadin.flow.server.ServiceException;
 import com.vaadin.flow.server.WrappedSession;
+import com.vaadin.flow.server.menu.AvailableViewInfo;
 import com.vaadin.flow.server.menu.MenuRegistry;
+import com.vaadin.flow.server.menu.RouteParamType;
 import com.vaadin.flow.server.startup.ApplicationRouteRegistry;
 import com.vaadin.tests.util.AlwaysLockedVaadinSession;
 import com.vaadin.tests.util.MockDeploymentConfiguration;
@@ -797,10 +800,13 @@ public void handle_clientNavigation_withMatchingFlowRoute() {
         ui.getInternals().clearLastHandledNavigation();
 
         try (MockedStatic<MenuRegistry> menuRegistry = Mockito
-                .mockStatic(MenuRegistry.class)) {
-            menuRegistry.when(
-                    () -> MenuRegistry.hasClientRoute("client-route", true))
-                    .thenReturn(true);
+                .mockStatic(MenuRegistry.class, Mockito.CALLS_REAL_METHODS)) {
+
+            menuRegistry.when(() -> MenuRegistry.getClientRoutes(true))
+                    .thenReturn(Collections.singletonMap("/client-route",
+                            new AvailableViewInfo("", null, false,
+                                    "/client-route", false, false, null, null,
+                                    null, false)));
 
             // This should not call attach or beforeEnter on root route
             renderer.handle(

diff --git a/flow-server/src/test/java/com/vaadin/flow/server/startup/RouteRegistryInitializerTest.java b/flow-server/src/test/java/com/vaadin/flow/server/startup/RouteRegistryInitializerTest.java
@@ -898,7 +898,7 @@ public void layout_annotation_on_non_routelayout_throws()
             throws ServletException {
         expectedEx.expect(InvalidRouteLayoutConfigurationException.class);
         expectedEx.expectMessage(String.format(
-                "Found @Layout on classes { %s } not extending RouterLayout.",
+                "Found @Layout on classes { %s } not implementing RouterLayout.",
                 FaultyParentLayout.class.getName()));
 
         routeRegistryInitializer.process(