From 049bfdf87082b75e08dcbe47e2c774b6ab86b89c Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Thu, 16 Jan 2025 08:32:01 +0200 Subject: [PATCH] fix: make route config errors more prominent (#20847) When route configuration errors happen, make sure to output the actual error message as the first output at ERROR level. When this is detected at startup, we can't prevent printing out the stack trace too since the container does that when initialization throws. Also checked all the messages for different route misconfiguration cases, and they seem to be fine. Fixes #20589 --- .../flow/router/internal/RouteSegment.java | 4 +- .../flow/router/internal/RouteUtil.java | 6 +++ .../flow/server/DefaultErrorHandler.java | 43 +++++++++--------- ...aadinServletContextStartupInitializer.java | 14 ++++++ .../internal/AbstractRouteRegistryTest.java | 45 +++++++++---------- .../router/internal/ConfigureRoutesTest.java | 20 +++++---- .../flow/server/SessionRouteRegistryTest.java | 7 ++- .../startup/RouteRegistryInitializerTest.java | 8 ++-- .../VaadinServletContextInitializer.java | 2 + 9 files changed, 87 insertions(+), 62 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/router/internal/RouteSegment.java b/flow-server/src/main/java/com/vaadin/flow/router/internal/RouteSegment.java index 547fbcedf0b..ed38d3039d5 100644 --- a/flow-server/src/main/java/com/vaadin/flow/router/internal/RouteSegment.java +++ b/flow-server/src/main/java/com/vaadin/flow/router/internal/RouteSegment.java @@ -791,9 +791,9 @@ private RuntimeException ambigousTarget(Class target) { String messageFormat; if (isParameter()) { - messageFormat = "Navigation targets must have unique routes, found navigation targets '%s' and '%s' with parameter have the same route."; + messageFormat = RouteUtil.ROUTE_CONFLICT_WITH_PARAMS; } else { - messageFormat = "Navigation targets must have unique routes, found navigation targets '%s' and '%s' with the same route."; + messageFormat = RouteUtil.ROUTE_CONFLICT; } String message = String.format(messageFormat, 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 f9c25539a8c..cae7e4c164a 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 @@ -70,6 +70,12 @@ */ public class RouteUtil { + private static final String ROUTE_CONFLICT_PREFIX = "Navigation target paths (considering @Route, @RouteAlias and @RoutePrefix values) must be unique, found navigation targets '%s' and '%s' "; + public static final String ROUTE_CONFLICT = ROUTE_CONFLICT_PREFIX + + "having the same route."; + public static final String ROUTE_CONFLICT_WITH_PARAMS = ROUTE_CONFLICT_PREFIX + + "with parameter having the same route."; + protected RouteUtil() { } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/DefaultErrorHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/DefaultErrorHandler.java index 696add50c2d..fa3cca3b72c 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/DefaultErrorHandler.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/DefaultErrorHandler.java @@ -19,33 +19,15 @@ import java.io.EOFException; import java.net.SocketException; import java.net.SocketTimeoutException; -import java.util.ArrayList; -import java.util.List; -import java.util.Optional; +import java.util.HashSet; import java.util.Set; -import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.slf4j.Marker; import org.slf4j.MarkerFactory; -import com.vaadin.experimental.FeatureFlags; -import com.vaadin.flow.component.Component; -import com.vaadin.flow.component.HasElement; -import com.vaadin.flow.component.UI; -import com.vaadin.flow.di.Instantiator; -import com.vaadin.flow.router.BeforeEnterEvent; -import com.vaadin.flow.router.ErrorParameter; -import com.vaadin.flow.router.HasErrorParameter; import com.vaadin.flow.router.InvalidLocationException; -import com.vaadin.flow.router.NavigationEvent; -import com.vaadin.flow.router.NavigationTrigger; -import com.vaadin.flow.router.RouteConfiguration; -import com.vaadin.flow.router.RouterLayout; -import com.vaadin.flow.router.internal.ErrorTargetEntry; -import com.vaadin.flow.router.internal.RouteUtil; -import com.vaadin.flow.server.startup.ApplicationRouteRegistry; /** * The default implementation of {@link ErrorHandler}. @@ -74,9 +56,18 @@ public class DefaultErrorHandler implements ErrorHandler { private final Set ignoredExceptions; + private final Set routeConfigurationExceptions; protected DefaultErrorHandler(Set ignoredExceptions) { this.ignoredExceptions = Set.copyOf(ignoredExceptions); + this.routeConfigurationExceptions = new HashSet<>(); + } + + protected DefaultErrorHandler(Set ignoredExceptions, + Set routeConfigurationExceptions) { + this.ignoredExceptions = Set.copyOf(ignoredExceptions); + this.routeConfigurationExceptions = Set + .copyOf(routeConfigurationExceptions); } public DefaultErrorHandler() { @@ -85,6 +76,10 @@ public DefaultErrorHandler() { EOFException.class.getName(), "org.eclipse.jetty.io.EofException", "org.apache.catalina.connector.ClientAbortException"); + this.routeConfigurationExceptions = Set.of( + AmbiguousRouteConfigurationException.class.getName(), + InvalidRouteConfigurationException.class.getName(), + InvalidRouteLayoutConfigurationException.class.getName()); } @Override @@ -101,8 +96,14 @@ public void error(ErrorEvent event) { getLogger().warn(marker, "", throwable); } } else { - // print the error on console - getLogger().error("", throwable); + if (routeConfigurationExceptions + .contains(throwable.getClass().getName())) { + getLogger().error("Route configuration error found:"); + getLogger().error(throwable.getMessage()); + } else { + // print the error on console + getLogger().error("", throwable); + } } } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/startup/VaadinServletContextStartupInitializer.java b/flow-server/src/main/java/com/vaadin/flow/server/startup/VaadinServletContextStartupInitializer.java index 3d0406d0fdd..07944567ff3 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/startup/VaadinServletContextStartupInitializer.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/startup/VaadinServletContextStartupInitializer.java @@ -15,12 +15,17 @@ */ package com.vaadin.flow.server.startup; +import com.vaadin.flow.server.InvalidRouteConfigurationException; +import com.vaadin.flow.server.InvalidRouteLayoutConfigurationException; import com.vaadin.flow.server.VaadinServletContext; import jakarta.servlet.ServletContext; import jakarta.servlet.ServletException; import java.util.Set; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + /** * Allows a library/runtime to be notified of a web application's startup phase * and perform any required programmatic registration of servlets, filters, and @@ -41,8 +46,17 @@ default void process(Set> classSet, ServletContext context) try { initialize(classSet, new VaadinServletContext(context)); } catch (VaadinInitializerException e) { + if (e.getCause() instanceof InvalidRouteConfigurationException || e + .getCause() instanceof InvalidRouteLayoutConfigurationException) { + getLogger().error("Route configuration error found:"); + getLogger().error(e.getCause().getMessage()); + } throw new ServletException(e); } } + private static Logger getLogger() { + return LoggerFactory + .getLogger(VaadinServletContextStartupInitializer.class); + } } diff --git a/flow-server/src/test/java/com/vaadin/flow/router/internal/AbstractRouteRegistryTest.java b/flow-server/src/test/java/com/vaadin/flow/router/internal/AbstractRouteRegistryTest.java index 1ca76cfa047..73497aa030b 100644 --- a/flow-server/src/test/java/com/vaadin/flow/router/internal/AbstractRouteRegistryTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/router/internal/AbstractRouteRegistryTest.java @@ -6,27 +6,27 @@ import java.util.List; import java.util.concurrent.CountDownLatch; -import com.vaadin.flow.router.BeforeEvent; -import com.vaadin.flow.router.HasUrlParameter; -import com.vaadin.flow.router.NotFoundException; -import com.vaadin.flow.router.OptionalParameter; -import com.vaadin.flow.router.RouteParameterRegex; -import com.vaadin.flow.router.WildcardParameter; -import com.vaadin.flow.server.InvalidRouteConfigurationException; import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import com.vaadin.flow.component.Component; import com.vaadin.flow.component.Tag; +import com.vaadin.flow.router.BeforeEvent; +import com.vaadin.flow.router.HasUrlParameter; +import com.vaadin.flow.router.Layout; +import com.vaadin.flow.router.NotFoundException; +import com.vaadin.flow.router.OptionalParameter; import com.vaadin.flow.router.Route; import com.vaadin.flow.router.RouteBaseData; +import com.vaadin.flow.router.RouteParameterRegex; import com.vaadin.flow.router.RouterLayout; import com.vaadin.flow.router.RoutesChangedEvent; -import com.vaadin.flow.router.Layout; +import com.vaadin.flow.router.WildcardParameter; +import com.vaadin.flow.server.InvalidRouteConfigurationException; import com.vaadin.flow.shared.Registration; -import org.junit.rules.ExpectedException; public class AbstractRouteRegistryTest { @@ -620,8 +620,7 @@ private void assertHasUrlAndWildcard() { public void multiple_normal_routes_throw_exception() throws InvalidRouteConfigurationException { expectedEx.expect(InvalidRouteConfigurationException.class); - expectedEx.expectMessage(String.format( - "Navigation targets must have unique routes, found navigation targets '%s' and '%s' with the same route.", + expectedEx.expectMessage(String.format(RouteUtil.ROUTE_CONFLICT, NormalRoute.class.getName(), SecondNormalRoute.class.getName())); @@ -648,10 +647,10 @@ public void normal_and_optional_throws_exception() public void two_optionals_throw_exception() throws InvalidRouteConfigurationException { expectedEx.expect(InvalidRouteConfigurationException.class); - expectedEx.expectMessage(String.format( - "Navigation targets must have unique routes, found navigation targets '%s' and '%s' with parameter have the same route.", - OptionalRoute.class.getName(), - SecondOptionalRoute.class.getName())); + expectedEx.expectMessage( + String.format(RouteUtil.ROUTE_CONFLICT_WITH_PARAMS, + OptionalRoute.class.getName(), + SecondOptionalRoute.class.getName())); addTarget(OptionalRoute.class); addTarget(SecondOptionalRoute.class); @@ -675,10 +674,10 @@ public void optional_and_normal_throws_exception() public void two_has_route_parameters_throw_exception() throws InvalidRouteConfigurationException { expectedEx.expect(InvalidRouteConfigurationException.class); - expectedEx.expectMessage(String.format( - "Navigation targets must have unique routes, found navigation targets '%s' and '%s' with parameter have the same route.", - HasUrlRoute.class.getName(), - SecondHasUrlRoute.class.getName())); + expectedEx.expectMessage( + String.format(RouteUtil.ROUTE_CONFLICT_WITH_PARAMS, + HasUrlRoute.class.getName(), + SecondHasUrlRoute.class.getName())); addTarget(HasUrlRoute.class); addTarget(SecondHasUrlRoute.class); @@ -689,10 +688,10 @@ public void two_has_route_parameters_throw_exception() public void two_wildcard_parameters_throw_exception() throws InvalidRouteConfigurationException { expectedEx.expect(InvalidRouteConfigurationException.class); - expectedEx.expectMessage(String.format( - "Navigation targets must have unique routes, found navigation targets '%s' and '%s' with parameter have the same route.", - WildcardRoute.class.getName(), - SecondWildcardRoute.class.getName())); + expectedEx.expectMessage( + String.format(RouteUtil.ROUTE_CONFLICT_WITH_PARAMS, + WildcardRoute.class.getName(), + SecondWildcardRoute.class.getName())); addTarget(WildcardRoute.class); addTarget(SecondWildcardRoute.class); diff --git a/flow-server/src/test/java/com/vaadin/flow/router/internal/ConfigureRoutesTest.java b/flow-server/src/test/java/com/vaadin/flow/router/internal/ConfigureRoutesTest.java index 757812fe934..a6561c56358 100644 --- a/flow-server/src/test/java/com/vaadin/flow/router/internal/ConfigureRoutesTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/router/internal/ConfigureRoutesTest.java @@ -1,5 +1,10 @@ package com.vaadin.flow.router.internal; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + import com.vaadin.flow.component.Component; import com.vaadin.flow.component.Tag; import com.vaadin.flow.router.BeforeEnterEvent; @@ -8,10 +13,6 @@ import com.vaadin.flow.router.HasErrorParameter; import com.vaadin.flow.router.HasUrlParameter; import com.vaadin.flow.server.AmbiguousRouteConfigurationException; -import org.junit.Assert; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.ExpectedException; public class ConfigureRoutesTest { @@ -109,8 +110,9 @@ public void duplicateRootPathRegistration_throwsException() { exceptionRule.expect(AmbiguousRouteConfigurationException.class); exceptionRule.reportMissingExceptionWithMessage( "Duplicate routes shouldn't be accepted."); - exceptionRule.expectMessage( - "Navigation targets must have unique routes, found navigation targets 'com.vaadin.flow.router.internal.ConfigureRoutesTest$BaseTarget' and 'com.vaadin.flow.router.internal.ConfigureRoutesTest$BaseTarget' with the same route."); + exceptionRule.expectMessage(String.format(RouteUtil.ROUTE_CONFLICT, + "com.vaadin.flow.router.internal.ConfigureRoutesTest$BaseTarget", + "com.vaadin.flow.router.internal.ConfigureRoutesTest$BaseTarget")); mutable.setRoute("", BaseTarget.class); } @@ -126,8 +128,10 @@ public void duplicateParameterPathRegistration_throwsException() { exceptionRule.expect(AmbiguousRouteConfigurationException.class); exceptionRule.reportMissingExceptionWithMessage( "Duplicate parameter routes shouldn't be accepted."); - exceptionRule.expectMessage( - "Navigation targets must have unique routes, found navigation targets 'com.vaadin.flow.router.internal.ConfigureRoutesTest$ParamTarget' and 'com.vaadin.flow.router.internal.ConfigureRoutesTest$ParamTarget' with parameter have the same route."); + exceptionRule.expectMessage(String.format( + RouteUtil.ROUTE_CONFLICT_WITH_PARAMS, + "com.vaadin.flow.router.internal.ConfigureRoutesTest$ParamTarget", + "com.vaadin.flow.router.internal.ConfigureRoutesTest$ParamTarget")); mutable.setRoute(":param", ParamTarget.class); } diff --git a/flow-server/src/test/java/com/vaadin/flow/server/SessionRouteRegistryTest.java b/flow-server/src/test/java/com/vaadin/flow/server/SessionRouteRegistryTest.java index 9a6066a1c5e..84be5785a89 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/SessionRouteRegistryTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/SessionRouteRegistryTest.java @@ -49,6 +49,7 @@ import com.vaadin.flow.router.RouterLayout; import com.vaadin.flow.router.RoutesChangedEvent; import com.vaadin.flow.router.internal.HasUrlParameterFormat; +import com.vaadin.flow.router.internal.RouteUtil; import com.vaadin.flow.server.startup.ApplicationConfiguration; import com.vaadin.flow.server.startup.ApplicationRouteRegistry; import com.vaadin.flow.shared.Registration; @@ -488,8 +489,7 @@ public void setSameRouteValueFromDifferentThreads_ConcurrencyTest() Assert.assertEquals( "Expected 4 route already exists exceptions due to route target validation", THREADS - 1, exceptions.size()); - String expected = String.format( - "Navigation targets must have unique routes, found navigation targets '%s' and '%s' with the same route.", + String expected = String.format(RouteUtil.ROUTE_CONFLICT, MyRoute.class.getName(), MyRoute.class.getName()); for (String exception : exceptions) { Assert.assertEquals(expected, exception); @@ -540,8 +540,7 @@ public void useRouteResolutionFromDifferentThreads_ConcurrencyTest() Assert.assertEquals( "Expected 4 route already exists exceptions due to route target validation", THREADS - 1, exceptions.size()); - String expected = String.format( - "Navigation targets must have unique routes, found navigation targets '%s' and '%s' with the same route.", + String expected = String.format(RouteUtil.ROUTE_CONFLICT, MyRoute.class.getName(), MyRoute.class.getName()); for (String exception : exceptions) { Assert.assertEquals(expected, exception); 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 index 73f4ede858a..29766a4715e 100644 --- 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 @@ -69,6 +69,7 @@ import com.vaadin.flow.router.internal.ErrorTargetEntry; import com.vaadin.flow.router.internal.HasUrlParameterFormat; import com.vaadin.flow.router.internal.PathUtil; +import com.vaadin.flow.router.internal.RouteUtil; import com.vaadin.flow.server.InvalidRouteConfigurationException; import com.vaadin.flow.server.InvalidRouteLayoutConfigurationException; import com.vaadin.flow.server.MockVaadinContext; @@ -169,10 +170,9 @@ public void process_duplicate_routesViaAlias_throws() @Test public void routeRegistry_fails_for_multiple_registration_of_same_route() { expectedEx.expect(InvalidRouteConfigurationException.class); - expectedEx.expectMessage( - "Navigation targets must have unique routes, found navigation targets " - + "'com.vaadin.flow.server.startup.RouteRegistryInitializerTest$NavigationTargetFoo' and " - + "'com.vaadin.flow.server.startup.RouteRegistryInitializerTest$NavigationTargetFoo2' with the same route."); + expectedEx.expectMessage(String.format(RouteUtil.ROUTE_CONFLICT, + "com.vaadin.flow.server.startup.RouteRegistryInitializerTest$NavigationTargetFoo", + "com.vaadin.flow.server.startup.RouteRegistryInitializerTest$NavigationTargetFoo2")); RouteConfiguration.forRegistry(registry) .setAnnotatedRoute(NavigationTargetFoo.class); diff --git a/vaadin-spring/src/main/java/com/vaadin/flow/spring/VaadinServletContextInitializer.java b/vaadin-spring/src/main/java/com/vaadin/flow/spring/VaadinServletContextInitializer.java index 70b20c0b338..7dd7a460b7c 100644 --- a/vaadin-spring/src/main/java/com/vaadin/flow/spring/VaadinServletContextInitializer.java +++ b/vaadin-spring/src/main/java/com/vaadin/flow/spring/VaadinServletContextInitializer.java @@ -393,6 +393,8 @@ public void failFastContextInitialized(ServletContextEvent event) { } catch (InvalidRouteConfigurationException | InvalidRouteLayoutConfigurationException e) { + getLogger().error("Route configuration error found:"); + getLogger().error(e.getMessage()); throw new IllegalStateException(e); } } else {