Skip to content

fix(dart_frog_gen): order route mounts by specificity #1969

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.

Sign up for GitHub

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

Open
Yusufihsangorgel wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(dart_frog_gen): order route mounts by specificity #1969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 20 additions & 2 deletions packages/dart_frog_gen/lib/src/build_route_configuration.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ RouteConfiguration buildRouteConfiguration(Directory directory) {
];
final routes = <RouteFile>[];
final rogueRoutes = <RouteFile>[];
// Order the directory mounts so that more specific mounts are registered on
// the root router before the shorter prefixes they share. `Router.mount`
// also matches `prefix/<rest>` and falls through on a 404, so without this a
// request to `/tasks/add` could be captured by a `/tasks` mount that has a
// dynamic `/<taskId>` child (see #1959). Ties fall back to the route string
// for a stable, deterministic order.
final directories = _getRouteDirectories(
directory: routesDirectory,
routesDirectory: routesDirectory,
Expand All @@ -43,7 +49,15 @@ RouteConfiguration buildRouteConfiguration(Directory directory) {
}
},
onRogueRoute: rogueRoutes.add,
);
)..sort((a, b) {
final specificity = compareRouteDirectorySpecificity(
[...a.route.segments],
[...b.route.segments],
);
if (specificity != 0) return specificity;
return a.route.compareTo(b.route);
});

final publicDirectory = Directory(path.join(directory.path, 'public'));
final mainDartFile = File(path.join(directory.path, 'main.dart'));

Expand Down Expand Up @@ -323,7 +337,11 @@ class RouteConfiguration {
final List<MiddlewareFile> middleware;

/// List of all route directories.
/// Sorted from leaf nodes to root.
///
/// Ordered by mount specificity (most specific first) so that, when the
/// directories are mounted on the root router in order, a longer literal
/// path (e.g. `/tasks/add`) is registered before a shorter prefix
/// (e.g. `/tasks`) that could otherwise capture it via a dynamic child.
final List<RouteDirectory> directories;

/// List of all route files.
Expand Down
29 changes: 29 additions & 0 deletions packages/dart_frog_gen/lib/src/route_specificity.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,35 @@ int compareRouteSpecificity(List<String> a, List<String> b) {
return 0;
}

/// Compares route directories [a] and [b] to determine the order in which
/// their routers should be mounted on the root router.
///
/// Unlike [compareRouteSpecificity] (which orders sibling route *files* within
/// a single router), this orders the flat list of directory mounts registered
/// on the root router. Because `Router.mount('/prefix', ...)` also matches
/// `'/prefix/<rest>'` and falls through on a 404, a mount must be registered
/// *before* any shorter prefix mount that could otherwise capture its path via
/// a dynamic child. Each path segment is ranked so the most specific mount is
/// registered first:
///
/// * a static segment (rank 2) is more specific than a shorter route that ends
/// here (rank 1), so `'/tasks/add'` is registered before `'/tasks'`;
/// * a shorter route that ends here (rank 1) is more specific than a dynamic
/// segment (rank 0), so `'/books'` is registered before `'/books/<id>'`.
///
/// Returns a negative value if [a] should be registered before [b], a positive
/// value if [b] should be registered before [a], and `0` if they are equally
/// specific.
int compareRouteDirectorySpecificity(List<String> a, List<String> b) {
final maxLength = a.length > b.length ? a.length : b.length;
for (var i = 0; i < maxLength; i++) {
final rankA = i < a.length ? (a[i].isDynamic ? 0 : 2) : 1;
final rankB = i < b.length ? (b[i].isDynamic ? 0 : 2) : 1;
if (rankA != rankB) return rankB - rankA;
}
return 0;
}

/// Extension that helps determine whether a route
/// segment belongs to a dynamic route.
extension IsDynamicRouteExtension on String {
Expand Down
163 changes: 111 additions & 52 deletions packages/dart_frog_gen/test/src/build_route_configuration_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,29 +233,29 @@ Future<void>init(InternetAddress ip,int port)async{}
test('includes nested routes', () {
const expected = [
{
'name': '_',
'route': '/',
'name': '_echo',
'route': '/echo',
'middleware': [],
'files': [
{
'name': 'index',
'path': '../routes/index.dart',
'route': '/',
'name': 'echo_message',
'path': '../routes/echo/message.dart',
'route': '/message',
'file_params': [],
'wildcard': false,
}
],
'directory_params': [],
},
{
'name': '_echo',
'route': '/echo',
'name': '_',
'route': '/',
'middleware': [],
'files': [
{
'name': 'echo_message',
'path': '../routes/echo/message.dart',
'route': '/message',
'name': 'index',
'path': '../routes/index.dart',
'route': '/',
'file_params': [],
'wildcard': false,
}
Expand Down Expand Up @@ -328,30 +328,30 @@ Future<void>init(InternetAddress ip,int port)async{}
test('includes dynamic route', () {
const expected = [
{
'name': '_',
'route': '/',
'name': '_echo',
'route': '/echo',
'middleware': [],
'files': [
{
'name': 'index',
'path': '../routes/index.dart',
'route': '/',
'file_params': [],
'name': r'echo_$message',
'path': '../routes/echo/[message].dart',
'route': '/<message>',
'file_params': ['message'],
'wildcard': false,
}
],
'directory_params': [],
},
{
'name': '_echo',
'route': '/echo',
'name': '_',
'route': '/',
'middleware': [],
'files': [
{
'name': r'echo_$message',
'path': '../routes/echo/[message].dart',
'route': '/<message>',
'file_params': ['message'],
'name': 'index',
'path': '../routes/index.dart',
'route': '/',
'file_params': [],
'wildcard': false,
}
],
Expand Down Expand Up @@ -473,29 +473,29 @@ Future<void>init(InternetAddress ip,int port)async{}
test('includes dynamic nested directory routes w/cascading middleware', () {
const expected = [
{
'name': '_',
'route': '/',
'name': '_api',
'route': '/api',
'middleware': [],
'files': [
{
'name': 'index',
'path': '../routes/index.dart',
'route': '/',
'name': 'api_v1',
'path': '../routes/api/v1.dart',
'route': '/v1',
'file_params': [],
'wildcard': false,
}
],
'directory_params': [],
},
{
'name': '_api',
'route': '/api',
'name': '_',
'route': '/',
'middleware': [],
'files': [
{
'name': 'api_v1',
'path': '../routes/api/v1.dart',
'route': '/v1',
'name': 'index',
'path': '../routes/index.dart',
'route': '/',
'file_params': [],
'wildcard': false,
}
Expand Down Expand Up @@ -1045,14 +1045,14 @@ Future<void>init(InternetAddress ip,int port)async{}
test('detects rogue routes.', () {
const expected = [
{
'name': '_',
'route': '/',
'name': '_api_v1',
'route': '/api/v1',
'middleware': [],
'files': [
{
'name': 'api',
'path': '../routes/api.dart',
'route': '/api',
'name': 'api_v1_hello',
'path': '../routes/api/v1/hello.dart',
'route': '/hello',
'file_params': [],
'wildcard': false,
}
Expand Down Expand Up @@ -1082,14 +1082,14 @@ Future<void>init(InternetAddress ip,int port)async{}
'directory_params': [],
},
{
'name': '_api_v1',
'route': '/api/v1',
'name': '_',
'route': '/',
'middleware': [],
'files': [
{
'name': 'api_v1_hello',
'path': '../routes/api/v1/hello.dart',
'route': '/hello',
'name': 'api',
'path': '../routes/api.dart',
'route': '/api',
'file_params': [],
'wildcard': false,
}
Expand Down Expand Up @@ -1166,29 +1166,29 @@ Future<void>init(InternetAddress ip,int port)async{}
test('does not report rogue route when index.dart already exists', () {
const expected = [
{
'name': '_',
'route': '/',
'name': '_api',
'route': '/api',
'middleware': [],
'files': [
{
'name': 'api',
'path': '../routes/api.dart',
'route': '/api',
'name': 'api_index',
'path': '../routes/api/index.dart',
'route': '/',
'file_params': [],
'wildcard': false,
}
],
'directory_params': [],
},
{
'name': '_api',
'route': '/api',
'name': '_',
'route': '/',
'middleware': [],
'files': [
{
'name': 'api_index',
'path': '../routes/api/index.dart',
'route': '/',
'name': 'api',
'path': '../routes/api.dart',
'route': '/api',
'file_params': [],
'wildcard': false,
}
Expand Down Expand Up @@ -1325,6 +1325,65 @@ Future<void>init(InternetAddress ip,int port)async{}

expect(fileRoutes, equals(['/a', '/b']));
});

test(
'mounts static sibling directories before a dynamic sibling file '
'https://github.com/dart-frog-dev/dart_frog/issues/1959', () {
final configuration = buildRouteConfiguration(
createTempDir(
files: [
'routes/tasks/index.dart',
'routes/tasks/[taskId].dart',
'routes/tasks/add/index.dart',
'routes/tasks/start/index.dart',
],
),
);

final mountOrder = configuration.directories.map((d) => d.route).toList();

// `/tasks/add` and `/tasks/start` must be mounted before `/tasks`.
// Otherwise `Router.mount('/tasks', ...)` matches `/tasks/add` first and
// its dynamic `/<taskId>` child resolves the request with
// `taskId == 'add'` instead of hitting the `add/` route.
expect(mountOrder, equals(['/tasks/add', '/tasks/start', '/tasks']));
expect(
mountOrder.indexOf('/tasks/add'),
lessThan(mountOrder.indexOf('/tasks')),
);
expect(
mountOrder.indexOf('/tasks/start'),
lessThan(mountOrder.indexOf('/tasks')),
);
});

test(
'mounts a deeply nested static sibling before its dynamic '
'sibling file '
'https://github.com/dart-frog-dev/dart_frog/issues/1959', () {
final configuration = buildRouteConfiguration(
createTempDir(
files: [
'routes/api/v1/event/[eventId]/tasks/index.dart',
'routes/api/v1/event/[eventId]/tasks/[taskId].dart',
'routes/api/v1/event/[eventId]/tasks/add/index.dart',
'routes/api/v1/event/[eventId]/tasks/start/index.dart',
],
),
);

final mountOrder = configuration.directories.map((d) => d.route).toList();

const tasks = '/api/v1/event/<eventId>/tasks';
expect(
mountOrder.indexOf('$tasks/add'),
lessThan(mountOrder.indexOf(tasks)),
);
expect(
mountOrder.indexOf('$tasks/start'),
lessThan(mountOrder.indexOf(tasks)),
);
});
});

group('RouteConfiguration', () {
Expand Down