From 227cd83a51bbd451dc223fd16f4cf1b87b8e44f8 Mon Sep 17 00:00:00 2001
From: Luiz Ferraz <luiz@lferraz.com>
Date: Wed, 14 Feb 2024 07:06:38 -0300
Subject: [PATCH] Fixes regression on routing priority for multi-layer index
 pages (#10096)

* Reproduce regression

* Simplify sorting algorithm

* Add changeset

* Fix changeset typo

* Rename assertion utility function

* Fix index detection

* Add changeset for index fix

---------

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
---
 .changeset/early-windows-accept.md            |  23 +++
 .changeset/lemon-cobras-swim.md               |   8 +
 .../astro/src/core/routing/manifest/create.ts |  74 ++++-----
 .../test/units/routing/manifest.nodetest.js   | 150 +++++++++++-------
 4 files changed, 151 insertions(+), 104 deletions(-)
 create mode 100644 .changeset/early-windows-accept.md
 create mode 100644 .changeset/lemon-cobras-swim.md

diff --git a/.changeset/early-windows-accept.md b/.changeset/early-windows-accept.md
new file mode 100644
index 0000000000..de3f177231
--- /dev/null
+++ b/.changeset/early-windows-accept.md
@@ -0,0 +1,23 @@
+---
+"astro": patch
+---
+
+Fixes regression on routing priority for multi-layer index pages
+
+The sorting algorithm positions more specific routes before less specific routes, and considers index pages to be more specific than a dynamic route with a rest parameter inside of it.
+This means that `/blog` is considered more specific than `/blog/[...slug]`.
+
+But this special case was being applied incorrectly to indexes, which could cause a problem in scenarios like the following:
+- `/`
+- `/blog`
+- `/blog/[...slug]`
+
+The algorithm would make the following comparisons:
+- `/` is more specific than `/blog` (incorrect)
+- `/blog/[...slug]` is more specific than `/` (correct)
+- `/blog` is more specific than `/blog/[...slug]` (correct)
+
+Although the incorrect first comparison is not a problem by itself, it could cause the algorithm to make the wrong decision.
+Depending on the other routes in the project, the sorting could perform just the last two comparisons and by transitivity infer the inverse of the third (`/blog/[...slug` > `/` > `/blog`), which is incorrect.
+
+Now the algorithm doesn't have a special case for index pages and instead does the comparison soleley for rest parameter segments and their immediate parents, which is consistent with the transitivity property.
diff --git a/.changeset/lemon-cobras-swim.md b/.changeset/lemon-cobras-swim.md
new file mode 100644
index 0000000000..58eef28fd0
--- /dev/null
+++ b/.changeset/lemon-cobras-swim.md
@@ -0,0 +1,8 @@
+---
+"astro": patch
+---
+
+Fixes edge case on i18n fallback routes
+
+Previously index routes deeply nested in the default locale, like `/some/nested/index.astro` could be mistaked as the root index for the default locale, resulting in an incorrect redirect on `/`.
+
diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts
index 6a1064c052..a3170a2cea 100644
--- a/packages/astro/src/core/routing/manifest/create.ts
+++ b/packages/astro/src/core/routing/manifest/create.ts
@@ -227,55 +227,33 @@ function routeComparator(a: RouteData, b: RouteData) {
 		}
 	}
 
-	// Special case to have `/[foo].astro` be equivalent to `/[foo]/index.astro`
-	// when compared against `/[foo]/[...rest].astro`.
-	if (Math.abs(a.segments.length - b.segments.length) === 1) {
+	const aLength = a.segments.length;
+	const bLength = b.segments.length;
+
+	if (aLength !== bLength) {
 		const aEndsInRest = a.segments.at(-1)?.some((part) => part.spread);
 		const bEndsInRest = b.segments.at(-1)?.some((part) => part.spread);
 
-		// Routes with rest parameters are less specific than their parent route.
-		// For example, `/foo/[...bar]` is sorted after `/foo`.
+		if (aEndsInRest !== bEndsInRest && Math.abs(aLength - bLength) === 1) {
+			// If only one of the routes ends in a rest parameter
+			// and the difference in length is exactly 1
+			// and the shorter route is the one that ends in a rest parameter
+			// the shorter route is considered more specific.
+			// I.e. `/foo` is considered more specific than `/foo/[...bar]`
+			if (aLength > bLength && aEndsInRest) {
+				// b: /foo
+				// a: /foo/[...bar]
+				return 1;
+			}
 
-		if (a.segments.length > b.segments.length && !bEndsInRest) {
-			return 1;
+			if (bLength > aLength && bEndsInRest) {
+				// a: /foo
+				// b: /foo/[...bar]
+				return -1;
+			}
 		}
 
-		if (b.segments.length > a.segments.length && !aEndsInRest) {
-			return -1;
-		}
-	}
-
-	if (a.isIndex !== b.isIndex) {
-		// Index pages are lower priority than other static segments in the same prefix.
-		// They match the path up to their parent, but are more specific than the parent.
-		// For example:
-		//  - `/foo/index.astro` is sorted before `/foo`
-		//  - `/foo/index.astro` is sorted before `/foo/[bar].astro`
-		//  - `/[...foo]/index.astro` is sorted after `/[...foo]/bar.astro`
-
-		if (a.isIndex) {
-			const followingBSegment = b.segments.at(a.segments.length);
-			const followingBSegmentIsStatic = followingBSegment?.every(
-				(part) => !part.dynamic && !part.spread
-			);
-
-			return followingBSegmentIsStatic ? 1 : -1;
-		}
-
-		const followingASegment = a.segments.at(b.segments.length);
-		const followingASegmentIsStatic = followingASegment?.every(
-			(part) => !part.dynamic && !part.spread
-		);
-
-		return followingASegmentIsStatic ? -1 : 1;
-	}
-
-	// For sorting purposes, an index route is considered to have one more segment than the URL it represents.
-	const aLength = a.isIndex ? a.segments.length + 1 : a.segments.length;
-	const bLength = b.isIndex ? b.segments.length + 1 : b.segments.length;
-
-	if (aLength !== bLength) {
-		// Routes are equal up to the smaller of the two lengths, so the longer route is more specific
+		// Sort routes by length
 		return aLength > bLength ? -1 : 1;
 	}
 
@@ -781,10 +759,12 @@ export function createRouteManifest(
 			// we attempt to retrieve the index page of the default locale
 			const defaultLocaleRoutes = routesByLocale.get(i18n.defaultLocale);
 			if (defaultLocaleRoutes) {
-				const indexDefaultRoute = defaultLocaleRoutes.find((routeData) => {
-					// it should be safe to assume that an index page has "index" in their name
-					return routeData.component.includes('index');
-				});
+				// The index for the default locale will be either already at the root path
+				// or at the root of the locale.
+				const indexDefaultRoute = defaultLocaleRoutes
+					.find(({route}) => route === '/')
+					?? defaultLocaleRoutes.find(({route}) => route === `/${i18n.defaultLocale}`);
+				
 				if (indexDefaultRoute) {
 					// we found the index of the default locale, now we create a root index that will redirect to the index of the default locale
 					const pathname = '/';
diff --git a/packages/astro/test/units/routing/manifest.nodetest.js b/packages/astro/test/units/routing/manifest.nodetest.js
index d7bbc8c0fa..110582bfb8 100644
--- a/packages/astro/test/units/routing/manifest.nodetest.js
+++ b/packages/astro/test/units/routing/manifest.nodetest.js
@@ -26,6 +26,19 @@ function getLogger() {
 	};
 }
 
+function assertRouteRelations(routes, relations) {
+	const routePaths = routes.map((route) => route.route);
+
+	for (const [before, after] of relations) {
+		const beforeIndex = routePaths.indexOf(before);
+		const afterIndex = routePaths.indexOf(after);
+
+		if (beforeIndex > afterIndex) {
+			assert.fail(`${before} should be higher priority than ${after}`);
+		}
+	}
+}
+
 describe('routing - createRouteManifest', () => {
 	it('using trailingSlash: "never" does not match the index route when it contains a trailing slash', async () => {
 		const fs = createFs(
@@ -128,23 +141,58 @@ describe('routing - createRouteManifest', () => {
 			fsMod: fs,
 		});
 
-		assert.deepEqual(getManifestRoutes(manifest), [
+		assertRouteRelations(getManifestRoutes(manifest), [
+			['/', '/[...rest]'],
+			['/static', '/[dynamic]'],
+			['/static', '/[...rest]'],
+			['/[dynamic]', '/[...rest]'],
+		]);
+	});
+	
+	it('route sorting with multi-layer index page conflict', async () => {
+		// Reproducing regression from https://github.com/withastro/astro/issues/10071
+		const fs = createFs(
 			{
-				route: '/',
-				type: 'page',
+				'/src/pages/a/1.astro': `<h1>test</h1>`,
+				'/src/pages/a/2.astro': `<h1>test</h1>`,
+				'/src/pages/a/3.astro': `<h1>test</h1>`,
+				'/src/pages/modules/[...slug].astro': `<h1>test</h1>`,
+				'/src/pages/modules/index.astro': `<h1>test</h1>`,
+				'/src/pages/test/[...slug].astro': `<h1>test</h1>`,
+				'/src/pages/test/index.astro': `<h1>test</h1>`,
+				'/src/pages/index.astro': `<h1>test</h1>`,
 			},
-			{
-				route: '/static',
-				type: 'page',
-			},
-			{
-				route: '/[dynamic]',
-				type: 'page',
-			},
-			{
-				route: '/[...rest]',
-				type: 'page',
+			root
+		);
+		const settings = await createBasicSettings({
+			root: fileURLToPath(root),
+			base: '/search',
+			trailingSlash: 'never',
+			experimental: {
+				globalRoutePriority: true,
 			},
+		});
+
+		const manifest = createRouteManifest({
+			cwd: fileURLToPath(root),
+			settings,
+			fsMod: fs,
+		});
+
+		assertRouteRelations(getManifestRoutes(manifest), [
+			// Parent route should come before rest parameters
+			['/test', '/test/[...slug]'],
+			['/modules', '/modules/[...slug]'],
+
+			// More specific routes should come before less specific routes
+			['/a/1', '/'],
+			['/a/2', '/'],
+			['/a/3', '/'],
+			['/test', '/'],
+			['/modules', '/'],
+			
+			// Alphabetical order
+			['/modules', '/test'],
 		]);
 	});
 
@@ -157,6 +205,7 @@ describe('routing - createRouteManifest', () => {
 				'/src/pages/[...rest]/static.astro': `<h1>test</h1>`,
 				'/src/pages/[...rest]/index.astro': `<h1>test</h1>`,
 				'/src/pages/blog/index.astro': `<h1>test</h1>`,
+				'/src/pages/blog/[...slug].astro': `<h1>test</h1>`,
 				'/src/pages/[dynamic_file].astro': `<h1>test</h1>`,
 				'/src/pages/[...other].astro': `<h1>test</h1>`,
 				'/src/pages/static.astro': `<h1>test</h1>`,
@@ -179,47 +228,34 @@ describe('routing - createRouteManifest', () => {
 			fsMod: fs,
 		});
 
-		assert.deepEqual(getManifestRoutes(manifest), [
-			{
-				route: '/',
-				type: 'page',
-			},
-			{
-				route: '/blog',
-				type: 'page',
-			},
-			{
-				route: '/static',
-				type: 'page',
-			},
-			{
-				route: '/[dynamic_folder]',
-				type: 'page',
-			},
-			{
-				route: '/[dynamic_file]',
-				type: 'page',
-			},
-			{
-				route: '/[dynamic_folder]/static',
-				type: 'page',
-			},
-			{
-				route: '/[dynamic_folder]/[...rest]',
-				type: 'page',
-			},
-			{
-				route: '/[...rest]/static',
-				type: 'page',
-			},
-			{
-				route: '/[...rest]',
-				type: 'page',
-			},
-			{
-				route: '/[...other]',
-				type: 'page',
-			},
+		assertRouteRelations(getManifestRoutes(manifest), [
+			// Parent route should come before rest parameters
+			['/', '/[...rest]'],
+			['/', '/[...other]'],
+			['/blog', '/blog/[...slug]'],
+			['/[dynamic_file]', '/[dynamic_folder]/[...rest]'],
+			['/[dynamic_folder]', '/[dynamic_folder]/[...rest]'],
+
+			// Static should come before dynamic
+			['/static', '/[dynamic_folder]'],
+			['/static', '/[dynamic_file]'],
+
+			// Static should come before rest parameters
+			['/blog', '/[...rest]'],
+			['/blog', '/[...other]'],
+			['/static', '/[...rest]'],
+			['/static', '/[...other]'],
+			['/static', '/[...rest]/static'],
+			['/[dynamic_folder]/static', '/[dynamic_folder]/[...rest]'],
+
+			// Dynamic should come before rest parameters
+			['/[dynamic_file]', '/[dynamic_folder]/[...rest]'],
+
+			// More specific routes should come before less specific routes
+			['/[dynamic_folder]/[...rest]', '/[...rest]'],
+			['/[dynamic_folder]/[...rest]', '/[...other]'],
+			['/blog/[...slug]', '/[...rest]'],
+			['/blog/[...slug]', '/[...other]'],
 		]);
 	});
 
@@ -317,11 +353,11 @@ describe('routing - createRouteManifest', () => {
 				type: 'page',
 			},
 			{
-				route: '/',
+				route: '/contributing',
 				type: 'page',
 			},
 			{
-				route: '/contributing',
+				route: '/',
 				type: 'page',
 			},
 			{