Implement review feedback

adminSiteClient/SiteRedirectsIndexPage.tsx

@@ -5,9 +5,12 @@ import { useForm } from "react-hook-form"
 import { AdminAppContext } from "./AdminAppContext.js"
 import { AdminLayout } from "./AdminLayout.js"
 
-const VALID_PATTERN = /^\/$|^.*[^\/]+$/
-const INVALID_PATTERN_MESSAGE =
-    "URL cannot end with a slash unless it's the root."
+const SOURCE_PATTERN = /^\/$|^\/.*[^\/]+$/
+const INVALID_SOURCE_MESSAGE =
+    "URL must start with a slash and cannot end with a slash, unless it's the root."
+const TARGET_PATTERN = /^\/$|^(https?:\/\/|\/).*[^\/]+$/
+const INVALID_TARGET_MESSAGE =
+    "URL must start with a slash or http(s):// and cannot end with a slash, unless it's the root."
 
 type Redirect = {
     id: number
@@ -55,6 +58,7 @@ export default function SiteRedirectsIndexPage() {
         register,
         formState: { errors, isSubmitting },
         handleSubmit,
+        setValue,
         watch,
     } = useForm<FormData>()
     const source = watch("source")
@@ -77,11 +81,14 @@ export default function SiteRedirectsIndexPage() {
         return undefined
     }
 
-    async function onSubmit(data: FormData) {
+    async function onSubmit({ source, target }: FormData) {
+        const sourceUrl = new URL(source, window.location.origin)
+        const sourcePath = sourceUrl.pathname
+        setValue("source", sourcePath)
         setError(null)
         const json = await admin.requestJSON(
             "/api/site-redirects/new",
-            data,
+            { source: sourcePath, target },
             "POST"
         )
         if (json.success) {
@@ -110,6 +117,21 @@ export default function SiteRedirectsIndexPage() {
     return (
         <AdminLayout title="Site Redirects">
             <main>
+                <div className="row">
+                    <div className="col-12 col-md-8">
+                        <p>
+                            Source has to start with a slash and any query
+                            parameters (?a=b) or fragments (#c) will be
+                            stripped, if present. The target can contain a full
+                            URL or a relative URL starting with a slash and both
+                            query parameters and fragments are allowed.
+                        </p>
+                        <p>
+                            To redirect to our homepage use a single slash as
+                            the target.
+                        </p>
+                    </div>
+                </div>
                 <form onSubmit={handleSubmit(onSubmit)}>
                     <div className="form-row">
                         <div className="form-group col-12 col-md-4">
@@ -121,8 +143,8 @@ export default function SiteRedirectsIndexPage() {
                                 {...register("source", {
                                     required: "Please provide a source.",
                                     pattern: {
-                                        value: VALID_PATTERN,
-                                        message: INVALID_PATTERN_MESSAGE,
+                                        value: SOURCE_PATTERN,
+                                        message: INVALID_SOURCE_MESSAGE,
                                     },
                                 })}
                             />
@@ -139,8 +161,8 @@ export default function SiteRedirectsIndexPage() {
                                 {...register("target", {
                                     required: "Please provide a target.",
                                     pattern: {
-                                        value: VALID_PATTERN,
-                                        message: INVALID_PATTERN_MESSAGE,
+                                        value: TARGET_PATTERN,
+                                        message: INVALID_TARGET_MESSAGE,
                                     },
                                     validate: validateTarget,
                                 })}

adminSiteServer/apiRouter.ts

@@ -127,10 +127,10 @@ import {
 } from "./functionalRouterHelpers.js"
 import { getPublishedLinksTo } from "../db/model/Link.js"
 import {
+    getChainedRedirect,
     getRedirectById,
     getRedirects,
     redirectWithSourceExists,
-    wouldCreateRedirectCycle,
 } from "../db/model/Redirect.js"
 import {
     GdocLinkUpdateMode,
@@ -1777,9 +1777,18 @@ postRouteWithRWTransaction(
                 400
             )
         }
-        if (await wouldCreateRedirectCycle(trx, source, target)) {
+        const chainedRedirect = await getChainedRedirect(trx, source, target)
+        if (chainedRedirect) {
             throw new JsonError(
-                "Creating this redirect would create a cycle",
+                "Creating this redirect would create a chain, redirect from " +
+                    `${chainedRedirect.source} to ${chainedRedirect.target} ` +
+                    "already exists. " +
+                    (target === chainedRedirect.source
+                        ? `Please create the redirect from ${source} to ` +
+                          `${chainedRedirect.target} directly instead.`
+                        : `Please delete the existing redirect and create a ` +
+                          `new redirect from ${chainedRedirect.source} to ` +
+                          `${target} instead.`),
                 400
             )
         }

db/migration/1713447826006-UniqueRedirectSource.ts

@@ -0,0 +1,15 @@
+import { MigrationInterface, QueryRunner } from "typeorm"
+
+export class UniqueRedirectSource1713447826006 implements MigrationInterface {
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(
+            `ALTER TABLE redirects ADD CONSTRAINT source_unique UNIQUE (source(255))`
+        )
+    }
+
+    public async down(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(
+            `ALTER TABLE redirects DROP CONSTRAINT source_unique`
+        )
+    }
+}

db/model/Redirect.ts

@@ -39,32 +39,14 @@ export async function redirectWithSourceExists(
     return Boolean(result)
 }
 
-export async function wouldCreateRedirectCycle(
+export async function getChainedRedirect(
     knex: db.KnexReadonlyTransaction,
     source: string,
     target: string
-): Promise<boolean> {
-    const result: { cycleExists: number } | undefined = await db.knexRawFirst(
+): Promise<Pick<DbPlainRedirect, "source" | "target"> | undefined> {
+    return await db.knexRawFirst(
         knex,
-        `WITH RECURSIVE redirect_chain AS (
-            -- Base case: Start with the target of the proposed new redirect.
-            SELECT source, target
-            FROM redirects
-            WHERE source = ?
-
-            UNION ALL
-
-            -- Recursive step: Follow the chain of redirects.
-            SELECT r.source, r.target
-            FROM redirects r
-            INNER JOIN redirect_chain rc ON rc.target = r.source
-        )
-         -- Check if the new source appears in the redirect chain starting from
-         -- the new target.
-         SELECT COUNT(*) > 0 AS cycleExists
-         FROM redirect_chain
-         WHERE source = ? OR target = ?`,
-        [target, source, source]
+        `SELECT source, target FROM redirects WHERE source = ? OR target = ?`,
+        [target, source]
     )
-    return Boolean(result?.cycleExists)
 }