Skip to content

Commit

Permalink
fix: make route config errors more prominent (#20847)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tepi authored Jan 16, 2025
1 parent 8dd3a20 commit 049bfdf
Show file tree
Hide file tree
Showing 9 changed files with 87 additions and 62 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -791,9 +791,9 @@ private RuntimeException ambigousTarget(Class<? extends Component> 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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}.
Expand Down Expand Up @@ -74,9 +56,18 @@
public class DefaultErrorHandler implements ErrorHandler {

private final Set<String> ignoredExceptions;
private final Set<String> routeConfigurationExceptions;

protected DefaultErrorHandler(Set<String> ignoredExceptions) {
this.ignoredExceptions = Set.copyOf(ignoredExceptions);
this.routeConfigurationExceptions = new HashSet<>();
}

protected DefaultErrorHandler(Set<String> ignoredExceptions,
Set<String> routeConfigurationExceptions) {
this.ignoredExceptions = Set.copyOf(ignoredExceptions);
this.routeConfigurationExceptions = Set
.copyOf(routeConfigurationExceptions);
}

public DefaultErrorHandler() {
Expand All @@ -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
Expand All @@ -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);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -41,8 +46,17 @@ default void process(Set<Class<?>> 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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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()));

Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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 {

Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 049bfdf

Please sign in to comment.