Enforcing routing priority during production builds (#3407)

* WIP: have a few test failures to track down

* WIP: still a few failures to fix

* WIP: fixes the issue of dynamic routes stepping on static routes

* Resolve route priority before building routes for `getStaticPaths()`

* chore: adding comments explaining why this filter is needed

* chore: adding changeset

* got too fancy with the test suite, these routes weren't valid

* simplifying the test cases

* TEMP: is this test breaking my CI run?

* Revert "TEMP: is this test breaking my CI run?"

This reverts commit 291af2a1b6f075ebfc74002886e43110731b3e1b.

* slots-preact didn't list @astrojs/preact as a dep

* reverting copy/paste error

.changeset/blue-buckets-attack.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Adds a check during build to make sure routing priority is always enforced in the final dist output

packages/astro/src/core/build/page-data.ts

@@ -10,7 +10,9 @@ import { debug } from '../logger/core.js';
 import { preload as ssrPreload } from '../render/dev/index.js';
 import { generateRssFunction } from '../render/rss.js';
 import { callGetStaticPaths, RouteCache, RouteCacheEntry } from '../render/route-cache.js';
+import { removeTrailingForwardSlash } from '../path.js';
 import { isBuildingToSSR } from '../util.js';
+import { matchRoute } from '../routing/match.js';
 
 export interface CollectPagesDataOptions {
 	astroConfig: AstroConfig;
@@ -35,6 +37,7 @@ export async function collectPagesData(
 
 	const assets: Record<string, string> = {};
 	const allPages: AllPagesData = {};
+	const builtPaths = new Set<string>();
 
 	const buildMode = isBuildingToSSR(astroConfig) ? 'ssr' : 'static';
 
@@ -60,6 +63,7 @@ export async function collectPagesData(
 				);
 				clearInterval(routeCollectionLogTimeout);
 			}, 10000);
+			builtPaths.add(route.pathname);
 			allPages[route.component] = {
 				component: route.component,
 				route,
@@ -138,7 +142,28 @@ export async function collectPagesData(
 		}
 		const finalPaths = result.staticPaths
 			.map((staticPath) => staticPath.params && route.generate(staticPath.params))
-			.filter(Boolean);
+			.filter((staticPath) => {
+				// Remove empty or undefined paths
+				if (!staticPath) {
+					return false;
+				}
+
+				// The path hasn't been built yet, include it
+				if (!builtPaths.has(removeTrailingForwardSlash(staticPath))) {
+					return true;
+				}
+
+				// The path was already built once. Check the manifest to see if
+				// this route takes priority for the final URL.
+				// NOTE: The same URL may match multiple routes in the manifest.
+				// Routing priority needs to be verified here for any duplicate
+				// paths to ensure routing priority rules are enforced in the final build.
+				const matchedRoute = matchRoute(staticPath, manifest);
+				return matchedRoute === route;
+			});
+
+		finalPaths.map((staticPath) => builtPaths.add(removeTrailingForwardSlash(staticPath)));
+		
 		allPages[route.component] = {
 			component: route.component,
 			route,

packages/astro/src/vite-plugin-astro-server/index.ts

@@ -199,13 +199,10 @@ async function handleRequest(
 	const devRoot = site ? site.pathname : '/';
 	const origin = `${viteServer.config.server.https ? 'https' : 'http'}://${req.headers.host}`;
 	const buildingToSSR = isBuildingToSSR(config);
-	// When file-based build format is used, pages will be built to `/blog.html`
+	// Ignore `.html` extensions and `index.html` in request URLS to ensure that
-	// rather than `/blog/index.html`. The dev server should handle this as well
+	// routing behavior matches production builds. This supports both file and directory
-	// to match production deployments.
+	// build formats, and is necessary based on how the manifest tracks build targets.
-	const url =
+	const url = new URL(origin + req.url?.replace(/(index)?\.html$/, ''));
-		config.build.format === 'file'
-			? new URL(origin + req.url?.replace(/(index)?\.html$/, ''))
-			: new URL(origin + req.url);
 	const pathname = decodeURI(url.pathname);
 	const rootRelativeUrl = pathname.substring(devRoot.length - 1);
 	if (!buildingToSSR) {

packages/astro/test/fixtures/routing-priority/astro.config.mjs

@@ -0,0 +1,4 @@
+import { defineConfig } from 'astro/config';
+
+// https://astro.build/config
+export default defineConfig({});

packages/astro/test/fixtures/routing-priority/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@test/routing-priority",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*"
+  }
+}

packages/astro/test/fixtures/routing-priority/src/pages/[lang]/[...catchall].astro

@@ -0,0 +1,23 @@
+---
+	export async function getStaticPaths() {
+		return [
+			{ params: { lang: 'de', catchall: '1/2' } },
+			{ params: { lang: 'en', catchall: '1/2' } }
+		]
+	}
+---
+
+<html lang="en">
+
+<head>
+	<meta charset="utf-8" />
+	<meta name="viewport" content="width=device-width" />
+	<title>Routing</title>
+</head>
+
+<body>
+	<h1>[lang]/[...catchall].astro</h1>
+	<p>{Astro.params.lang} | {Astro.params.catchall}</p>
+</body>
+
+</html>

packages/astro/test/fixtures/routing-priority/src/pages/[lang]/index.astro

@@ -0,0 +1,23 @@
+---
+	export async function getStaticPaths() {
+		return [
+			{ params: { lang: 'de' } }, // always shadowed by /de/index.astro
+			{ params: { lang: 'en' } }
+		];
+	}
+---
+
+<html lang="en">
+
+<head>
+	<meta charset="utf-8" />
+	<meta name="viewport" content="width=device-width" />
+	<title>Routing</title>
+</head>
+
+<body>
+	<h1>[lang]/index.astro</h1>
+	<p>{Astro.params.lang}</p>
+</body>
+
+</html>

packages/astro/test/fixtures/routing-priority/src/pages/de/index.astro

@@ -0,0 +1,12 @@
+---
+---
+<html lang="en">
+	<head>
+		<meta charset="utf-8" />
+		<meta name="viewport" content="width=device-width" />
+		<title>Routing</title>
+	</head>
+	<body>
+		<h1>de/index.astro</h1>
+	</body>
+</html>

packages/astro/test/fixtures/routing-priority/src/pages/index.astro

@@ -0,0 +1,12 @@
+---
+---
+<html lang="en">
+	<head>
+		<meta charset="utf-8" />
+		<meta name="viewport" content="width=device-width" />
+		<title>Routing</title>
+	</head>
+	<body>
+		<h1>index.astro</h1>
+	</body>
+</html>

packages/astro/test/fixtures/routing-priority/src/pages/posts/[...slug].astro

@@ -0,0 +1,24 @@
+---
+	export async function getStaticPaths() {
+		return [
+			{
+				params: { slug: "1/2" },
+			}
+		]
+	}
+---
+
+<html lang="en">
+
+<head>
+	<meta charset="utf-8" />
+	<meta name="viewport" content="width=device-width" />
+	<title>Routing</title>
+</head>
+
+<body>
+	<h1>posts/[...slug].astro</h1>
+	<p>{Astro.params.slug}</p>
+</body>
+
+</html>

packages/astro/test/fixtures/routing-priority/src/pages/posts/[pid].astro

@@ -0,0 +1,23 @@
+---
+	export async function getStaticPaths() {
+		return [
+			{ params: { pid: 'post-1' } },
+			{ params: { pid: 'post-2' } }
+		]
+	}
+---
+
+<html lang="en">
+
+<head>
+	<meta charset="utf-8" />
+	<meta name="viewport" content="width=device-width" />
+	<title>Routing</title>
+</head>
+
+<body>
+	<h1>posts/[pid].astro</h1>
+	<p>{Astro.params.pid}</p>
+</body>
+
+</html>

packages/astro/test/routing-priority.test.js

@@ -0,0 +1,236 @@
+import { expect } from 'chai';
+import { load as cheerioLoad } from 'cheerio';
+import path from 'path';
+import { loadFixture } from './test-utils.js';
+
+let fixture;
+
+const routes = [
+	{
+		url: '/',
+		h1: 'index.astro'
+	},
+	{
+		url: '/posts/post-1',
+		h1: 'posts/[pid].astro',
+		p: 'post-1'
+	},
+	{
+		url: '/posts/post-2',
+		h1: 'posts/[pid].astro',
+		p: 'post-2'
+	},
+	{
+		url: '/posts/1/2',
+		h1: 'posts/[...slug].astro',
+		p: '1/2'
+	},
+	{	
+		url: '/de',
+		h1: 'de/index.astro'
+	},
+	{
+		url: '/de/',
+		h1: 'de/index.astro'
+	},
+	{
+		url: '/de/index.html',
+		h1: 'de/index.astro'
+	},
+	{
+		url: '/en',
+		h1: '[lang]/index.astro',
+		p: 'en'
+	},
+	{
+		url: '/en/',
+		h1: '[lang]/index.astro',
+		p: 'en'
+	},
+	{
+		url: '/en/index.html',
+		h1: '[lang]/index.astro',
+		p: 'en'
+	},
+	{
+		url: '/de/1/2',
+		h1: '[lang]/[...catchall].astro',
+		p: 'de | 1/2'
+	},
+	{
+		url: '/en/1/2',
+		h1: '[lang]/[...catchall].astro',
+		p: 'en | 1/2'
+	}
+]
+
+describe('Routing priority', () => {
+	before(async () => {
+		fixture = await loadFixture({
+			root: './fixtures/routing-priority/',
+		});
+	});
+
+	describe('build', () => {
+		before(async () => {
+			await fixture.build();
+		});
+
+		it('matches / to index.astro', async () => {
+			const html = await fixture.readFile('/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('index.astro');
+		});
+
+		it('matches /posts/post-1 to posts/[pid].astro', async () => {
+			const html = await fixture.readFile('/posts/post-1/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('posts/[pid].astro');
+			expect($('p').text()).to.equal('post-1');
+		});
+
+		it('matches /posts/1/2 to posts/[...slug].astro', async () => {
+			const html = await fixture.readFile('/posts/1/2/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('posts/[...slug].astro');
+			expect($('p').text()).to.equal('1/2');
+		});
+
+		it('matches /de to de/index.astro', async () => {
+			const html = await fixture.readFile('/de/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('de/index.astro');
+		});
+
+		it('matches /en to [lang]/index.astro', async () => {
+			const html = await fixture.readFile('/en/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('[lang]/index.astro');
+			expect($('p').text()).to.equal('en');
+		});
+
+		it('matches /de/1/2 to [lang]/[...catchall].astro', async () => {
+			const html = await fixture.readFile('/de/1/2/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('[lang]/[...catchall].astro');
+			expect($('p').text()).to.equal('de | 1/2')
+		});
+
+		it('matches /en/1/2 to [lang]/[...catchall].astro', async () => {
+			const html = await fixture.readFile('/en/1/2/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('[lang]/[...catchall].astro');
+			expect($('p').text()).to.equal('en | 1/2')
+		});
+	});
+
+	describe('dev', () => {
+		let devServer;
+
+		before(async () => {
+			devServer = await fixture.startDevServer();
+		});
+
+		after(async () => {
+			await devServer.stop();
+		});
+
+		it('matches / to index.astro', async () => {
+			const html = await fixture.fetch('/').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('index.astro');
+		});
+
+		it('matches /posts/post-1 to /posts/[pid].astro', async () => {
+			const html = await fixture.fetch('/posts/post-1').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('posts/[pid].astro');
+				expect($('p').text()).to.equal('post-1');
+		});
+
+		it('matches /posts/1/2 to /posts/[...slug].astro', async () => {
+			const html = await fixture.fetch('/posts/1/2').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('posts/[...slug].astro');
+				expect($('p').text()).to.equal('1/2');
+		});
+
+		it('matches /de to de/index.astro', async () => {
+			const html = await fixture.fetch('/de').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('de/index.astro');
+		});
+
+		it('matches /de to de/index.astro', async () => {
+			const html = await fixture.fetch('/de').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('de/index.astro');
+		});
+
+		it('matches /de/ to de/index.astro', async () => {
+			const html = await fixture.fetch('/de/').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('de/index.astro');
+		});
+
+		it('matches /de/index.html to de/index.astro', async () => {
+			const html = await fixture.fetch('/de/index.html').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('de/index.astro');
+		});
+
+		it('matches /en to [lang]/index.astro', async () => {
+			const html = await fixture.fetch('/en').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('[lang]/index.astro');
+				expect($('p').text()).to.equal('en');
+		});
+
+		it('matches /en/ to [lang]/index.astro', async () => {
+			const html = await fixture.fetch('/en/').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('[lang]/index.astro');
+				expect($('p').text()).to.equal('en');
+		});
+
+		it('matches /en/index.html to de/index.astro', async () => {
+			const html = await fixture.fetch('/en/index.html').then((res) => res.text());
+				const $ = cheerioLoad(html);
+
+				expect($('h1').text()).to.equal('[lang]/index.astro');
+				expect($('p').text()).to.equal('en');
+		});
+
+		it('matches /de/1/2 to [lang]/[...catchall].astro', async () => {
+			const html = await fixture.fetch('/de/1/2/index.html').then((res) => res.text());
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('[lang]/[...catchall].astro');
+			expect($('p').text()).to.equal('de | 1/2');
+		});
+
+		it('matches /en/1/2 to [lang]/[...catchall].astro', async () => {
+			const html = await fixture.fetch('/en/1/2/index.html').then((res) => res.text());
+			const $ = cheerioLoad(html);
+
+			expect($('h1').text()).to.equal('[lang]/[...catchall].astro');
+			expect($('p').text()).to.equal('en | 1/2');
+		});
+	});
+});

pnpm-lock.yaml

@@ -1162,6 +1162,12 @@ importers:
     dependencies:
       astro: link:../../..
 
+  packages/astro/test/fixtures/routing-priority:
+    specifiers:
+      astro: workspace:*
+    dependencies:
+      astro: link:../../..
+
   packages/astro/test/fixtures/sass:
     specifiers:
       astro: workspace:*