Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make route config errors more prominent #20847

Merged
merged 5 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading