From add785323e917338873098dc44b8af984c4e7c9a Mon Sep 17 00:00:00 2001 From: Ruslan Matkovskyi <100142572+sc-ruslanmatkovskyi@users.noreply.github.com> Date: Wed, 15 Jan 2025 13:37:22 +0100 Subject: [PATCH] [sitecore-jss-nextjs] Improve performance for redirects (#2003) * [sitecore-jss-nextjs]: The cache of results of pattern has been added * CHANGELOG has been changed * [sitecore-jss-nextjs]: Removed a method that addressed Netlify issues. Added a simpler solution for non-regex patterns to fix Netlify-related problems. Improved redirect performance. --------- Co-authored-by: Ruslan Matkovskyi --- CHANGELOG.md | 4 +- .../middleware/redirects-middleware.test.ts | 10 +- .../src/middleware/redirects-middleware.ts | 232 +++++++----------- packages/sitecore-jss/src/utils/index.ts | 5 +- packages/sitecore-jss/src/utils/utils.test.ts | 144 ++++++++--- packages/sitecore-jss/src/utils/utils.ts | 112 ++++++++- 6 files changed, 306 insertions(+), 201 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be96077b0c..998f0dce53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,10 +18,8 @@ Our versioning strategy is as follows: ### 🐛 Bug Fixes * `[templates/nextjs-sxa]` Fixed font-awesome import issue in custom workspace configuration ([#1998](https://github.com/Sitecore/jss/pull/1998)) - -### 🐛 Bug Fixes - * `[sitecore-jss-nextjs]` Fixed handling of ? inside square brackets [] in regex patterns to prevent incorrect escaping ([#1999](https://github.com/Sitecore/jss/pull/1999)) +* `[sitecore-jss-nextjs]` Improve performance for redirect middleware ([#2003](https://github.com/Sitecore/jss/pull/2003)) ## 22.3.0 / 22.3.1 diff --git a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.test.ts b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.test.ts index c6e572f31f..3e56f01d6a 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.test.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.test.ts @@ -822,7 +822,7 @@ describe('RedirectsMiddleware', () => { request: { nextUrl: { pathname: '/not-found', - search: 'abc=def', + search: '', href: 'http://localhost:3000/not-found', locale: 'en', origin: 'http://localhost:3000', @@ -1361,7 +1361,7 @@ describe('RedirectsMiddleware', () => { expect(finalRes.status).to.equal(res.status); }); - it('should return 301 redirect when queryString is ordered by alphabetic(Netlify feature)', async () => { + it('should return 301 redirect when pattern has special symbols "?"', async () => { const cloneUrl = () => Object.assign({}, req.nextUrl); const url = { clone: cloneUrl, @@ -1389,7 +1389,7 @@ describe('RedirectsMiddleware', () => { const { finalRes, fetchRedirects, siteResolver } = await runTestWithRedirect( { - pattern: '/not-found?w=1&a=1', + pattern: '/[/]?not-found?a=1&w=1/', target: '/found', redirectType: REDIRECT_TYPE_301, isQueryStringPreserved: true, @@ -1412,7 +1412,7 @@ describe('RedirectsMiddleware', () => { expect(finalRes.status).to.equal(res.status); }); - it('should return 301 redirect when pattern has special symbols "?"', async () => { + it('should return 301 redirect when pattern has another order of query string', async () => { const cloneUrl = () => Object.assign({}, req.nextUrl); const url = { clone: cloneUrl, @@ -1440,7 +1440,7 @@ describe('RedirectsMiddleware', () => { const { finalRes, fetchRedirects, siteResolver } = await runTestWithRedirect( { - pattern: '/[/]?not-found?a=1&w=1/', + pattern: '/not-found?w=1&a=1/', target: '/found', redirectType: REDIRECT_TYPE_301, isQueryStringPreserved: true, diff --git a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts index 15a31667c5..a867c6c564 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts @@ -8,15 +8,22 @@ import { RedirectInfo, SiteInfo, } from '@sitecore-jss/sitecore-jss/site'; -import { getPermutations } from '@sitecore-jss/sitecore-jss/utils'; +import { + areURLSearchParamsEqual, + escapeNonSpecialQuestionMarks, + isRegexOrUrl, + mergeURLSearchParams, +} from '@sitecore-jss/sitecore-jss/utils'; +import { NextURL } from 'next/dist/server/web/next-url'; import { NextRequest, NextResponse } from 'next/server'; import regexParser from 'regex-parser'; import { MiddlewareBase, MiddlewareBaseConfig } from './middleware'; -import { NextURL } from 'next/dist/server/web/next-url'; const REGEXP_CONTEXT_SITE_LANG = new RegExp(/\$siteLang/, 'i'); const REGEXP_ABSOLUTE_URL = new RegExp('^(?:[a-z]+:)?//', 'i'); +type RedirectResult = RedirectInfo & { matchedQueryString?: string }; + /** * extended RedirectsMiddlewareConfig config type for RedirectsMiddleware */ @@ -78,15 +85,25 @@ export class RedirectsMiddleware extends MiddlewareBase { }); const createResponse = async () => { + const response = res || NextResponse.next(); + if (this.config.disabled && this.config.disabled(req, res || NextResponse.next())) { debug.redirects('skipped (redirects middleware is disabled)'); - return res || NextResponse.next(); + return response; } if (this.isPreview(req) || this.excludeRoute(pathname)) { debug.redirects('skipped (%s)', this.isPreview(req) ? 'preview' : 'route excluded'); - return res || NextResponse.next(); + return response; + } + + // Skip prefetch requests from Next.js, which are not original client requests + // as they load unnecessary requests that burden the redirects middleware with meaningless traffic + if (this.isPrefetch(req)) { + debug.redirects('skipped (prefetch)'); + response.headers.set('x-middleware-cache', 'no-cache'); + return response; } site = this.getSite(req, res); @@ -97,7 +114,7 @@ export class RedirectsMiddleware extends MiddlewareBase { if (!existsRedirect) { debug.redirects('skipped (redirect does not exist)'); - return res || NextResponse.next(); + return response; } // Find context site language and replace token @@ -120,29 +137,37 @@ export class RedirectsMiddleware extends MiddlewareBase { if (REGEXP_ABSOLUTE_URL.test(existsRedirect.target)) { url.href = existsRedirect.target; } else { - const source = `${url.pathname.replace(/\/*$/gi, '')}${existsRedirect.matchedQueryString}`; - const urlFirstPart = existsRedirect.target.split('/')[1]; + const isUrl = isRegexOrUrl(existsRedirect.pattern) === 'url'; + const targetParts = existsRedirect.target.split('/'); + const urlFirstPart = targetParts[1]; if (this.locales.includes(urlFirstPart)) { req.nextUrl.locale = urlFirstPart; existsRedirect.target = existsRedirect.target.replace(`/${urlFirstPart}`, ''); } - const target = source - .replace(regexParser(existsRedirect.pattern), existsRedirect.target) - .replace(/^\/\//, '/') - .split('?'); - - if (url.search && existsRedirect.isQueryStringPreserved) { - const targetQueryString = target[1] ?? ''; - url.search = '?' + new URLSearchParams(`${url.search}&${targetQueryString}`).toString(); - } else if (target[1]) { - url.search = '?' + target[1]; - } else { - url.search = ''; - } - - const prepareNewURL = new URL(`${target[0]}${url.search}`, url.origin); + const targetSegments = isUrl + ? existsRedirect.target.split('?') + : url.pathname.replace(/\/*$/gi, '') + existsRedirect.matchedQueryString; + + const [targetPath, targetQueryString] = isUrl + ? targetSegments + : (targetSegments as string) + .replace(regexParser(existsRedirect.pattern), existsRedirect.target) + .replace(/^\/\//, '/') + .split('?'); + + const mergedQueryString = existsRedirect.isQueryStringPreserved + ? mergeURLSearchParams( + new URLSearchParams(url.search ?? ''), + new URLSearchParams(targetQueryString || '') + ) + : targetQueryString || ''; + + const prepareNewURL = new URL( + `${targetPath}${mergedQueryString ? '?' + mergedQueryString : ''}`, + url.origin + ); url.href = prepareNewURL.href; url.pathname = prepareNewURL.pathname; @@ -153,16 +178,16 @@ export class RedirectsMiddleware extends MiddlewareBase { /** return Response redirect with http code of redirect type */ switch (existsRedirect.redirectType) { case REDIRECT_TYPE_301: { - return this.createRedirectResponse(url, res, 301, 'Moved Permanently'); + return this.createRedirectResponse(url, response, 301, 'Moved Permanently'); } case REDIRECT_TYPE_302: { - return this.createRedirectResponse(url, res, 302, 'Found'); + return this.createRedirectResponse(url, response, 302, 'Found'); } case REDIRECT_TYPE_SERVER_TRANSFER: { - return this.rewrite(url.href, req, res || NextResponse.next()); + return this.rewrite(url.href, req, response); } default: - return res || NextResponse.next(); + return response; } }; @@ -188,20 +213,37 @@ export class RedirectsMiddleware extends MiddlewareBase { private async getExistsRedirect( req: NextRequest, siteName: string - ): Promise<(RedirectInfo & { matchedQueryString?: string }) | undefined> { - const redirects = await this.redirectsService.fetchRedirects(siteName); + ): Promise { const { pathname: targetURL, search: targetQS = '', locale } = this.normalizeUrl( req.nextUrl.clone() ); + const normalizedPath = targetURL.replace(/\/*$/gi, ''); + const redirects = await this.redirectsService.fetchRedirects(siteName); const language = this.getLanguage(req); const modifyRedirects = structuredClone(redirects); + let matchedQueryString: string | undefined; return modifyRedirects.length - ? modifyRedirects.find((redirect: RedirectInfo & { matchedQueryString?: string }) => { + ? modifyRedirects.find((redirect: RedirectResult) => { + if (isRegexOrUrl(redirect.pattern) === 'url') { + const parseUrlPattern = redirect.pattern.endsWith('/') + ? redirect.pattern.slice(0, -1).split('?') + : redirect.pattern.split('?'); + + return ( + (parseUrlPattern[0] === normalizedPath || + parseUrlPattern[0] === `/${locale}${normalizedPath}`) && + areURLSearchParamsEqual( + new URLSearchParams(parseUrlPattern[1] ?? ''), + new URLSearchParams(targetQS) + ) + ); + } + // Modify the redirect pattern to ignore the language prefix in the path // And escapes non-special "?" characters in a string or regex. - redirect.pattern = this.escapeNonSpecialQuestionMarks( - redirect.pattern.replace(RegExp(`^[^]?/${language}/`, 'gi'), '') + redirect.pattern = escapeNonSpecialQuestionMarks( + redirect.pattern.replace(new RegExp(`^[^]?/${language}/`, 'gi'), '') ); // Prepare the redirect pattern as a regular expression, making it more flexible for matching URLs @@ -211,47 +253,22 @@ export class RedirectsMiddleware extends MiddlewareBase { .replace(/^\^|\$$/g, '') // Further cleans up anchors .replace(/\$\/gi$/g, '')}[\/]?$/i`; // Ensures the pattern allows an optional trailing slash - /** - * This line checks whether the current URL query string (and all its possible permutations) - * matches the redirect pattern. - * - * Query parameters in URLs can come in different orders, but logically they represent the - * same information (e.g., "key1=value1&key2=value2" is the same as "key2=value2&key1=value1"). - * To account for this, the method `isPermutedQueryMatch` generates all possible permutations - * of the query parameters and checks if any of those permutations match the regex pattern for the redirect. - * - * NOTE: This fix is specifically implemented for Netlify, where query parameters are sometimes - * automatically sorted, which can cause issues with matching redirects if the order of query - * parameters is important. By checking every possible permutation, we ensure that redirects - * work correctly on Netlify despite this behavior. - * - * It passes several pieces of information to the function: - * 1. `pathname`: The normalized URL path without query parameters (e.g., '/about'). - * 2. `queryString`: The current query string from the URL, which will be permuted and matched (e.g., '?key1=value1&key2=value2'). - * 3. `pattern`: The regex pattern for the redirect that we are trying to match against the URL (e.g., '/about?key1=value1'). - * 4. `locale`: The locale part of the URL (if any), which helps support multilingual URLs. - * - * If one of the permutations of the query string matches the redirect pattern, the function - * returns the matched query string, which is stored in `matchedQueryString`. If no match is found, - * it returns `undefined`. The `matchedQueryString` is later used to indicate whether the query - * string contributed to a successful redirect match. - */ - const matchedQueryString = this.isPermutedQueryMatch({ - pathname: targetURL, - queryString: targetQS, - pattern: redirect.pattern, - locale, - }); + matchedQueryString = [ + regexParser(redirect.pattern).test(`${normalizedPath}${targetQS}`), + regexParser(redirect.pattern).test(`/${locale}${normalizedPath}${targetQS}`), + ].some(Boolean) + ? targetQS + : undefined; // Save the matched query string (if found) into the redirect object redirect.matchedQueryString = matchedQueryString || ''; - // Return the redirect if the URL path or any query string permutation matches the pattern return ( - (regexParser(redirect.pattern).test(targetURL) || + !!( + regexParser(redirect.pattern).test(targetURL) || regexParser(redirect.pattern).test(`/${req.nextUrl.locale}${targetURL}`) || - matchedQueryString) && - (redirect.locale ? redirect.locale.toLowerCase() === locale.toLowerCase() : true) + matchedQueryString + ) && (redirect.locale ? redirect.locale.toLowerCase() === locale.toLowerCase() : true) ); }) : undefined; @@ -328,87 +345,4 @@ export class RedirectsMiddleware extends MiddlewareBase { } return redirect; } - - /** - * Checks if the current URL query matches the provided pattern, considering all permutations of query parameters. - * It constructs all possible query parameter permutations and tests them against the pattern. - * @param {object} params - The parameters for the URL match. - * @param {string} params.pathname - The current URL pathname. - * @param {string} params.queryString - The current URL query string. - * @param {string} params.pattern - The regex pattern to test the constructed URLs against. - * @param {string} [params.locale] - The locale prefix to include in the URL if present. - * @returns {string | undefined} - return query string if any of the query permutations match the provided pattern, undefined otherwise. - */ - private isPermutedQueryMatch({ - pathname, - queryString, - pattern, - locale, - }: { - pathname: string; - queryString: string; - pattern: string; - locale?: string; - }): string | undefined { - const paramsArray = Array.from(new URLSearchParams(queryString).entries()); - const listOfPermuted = getPermutations(paramsArray).map( - (permutation: [string, string][]) => - '?' + permutation.map(([key, value]) => `${key}=${value}`).join('&') - ); - - const normalizedPath = pathname.replace(/\/*$/gi, ''); - return listOfPermuted.find((query: string) => - [ - regexParser(pattern).test(`${normalizedPath}${query}`), - regexParser(pattern).test(`/${locale}${normalizedPath}${query}`), - ].some(Boolean) - ); - } - - /** - * Escapes non-special "?" characters in a string or regex. - * - * - For regular strings, it escapes all unescaped "?" characters by adding a backslash (`\`). - * - For regex patterns (strings enclosed in `/.../`), it analyzes each "?" to determine if it has special meaning - * (e.g., `?` in `(abc)?`, `.*?`) or is just a literal character. Only literal "?" characters are escaped. - * @param {string} input - The input string or regex pattern. - * @returns {string} - The modified string or regex with non-special "?" characters escaped. - **/ - private escapeNonSpecialQuestionMarks(input: string): string { - const regexPattern = /(? { }); }); - describe('getPermutations', () => { - it('should return all permutations of an array with one pair', () => { - const input: [string, string][] = [['key1', 'value1']]; - const expectedOutput = [[['key1', 'value1']]]; - expect(getPermutations(input)).to.deep.equal(expectedOutput); - }); - - it('should return all permutations of an array with two pairs', () => { - const input: [string, string][] = [ - ['key1', 'value1'], - ['key2', 'value2'], - ]; - const expectedOutput = [ - [ - ['key1', 'value1'], - ['key2', 'value2'], - ], - [ - ['key2', 'value2'], - ['key1', 'value1'], - ], - ]; - expect(getPermutations(input)).to.deep.equal(expectedOutput); - }); - - it('should return an empty array when input is empty', () => { - const input: [string, string][] = []; - const expectedOutput = [[]]; - expect(getPermutations(input)).to.deep.equal(expectedOutput); + describe('isRegexOrUrl', () => { + it('should return "url" for valid URL-like strings', () => { + expect(isRegexOrUrl('/path/to/resource?param=value')).to.equal('url'); + expect(isRegexOrUrl('/another/path')).to.equal('url'); + }); + + it('should return "regex" for non-URL strings', () => { + expect(isRegexOrUrl('/path/.*')).to.equal('regex'); + expect(isRegexOrUrl('^/path/(\\d+)$')).to.equal('regex'); + }); + }); + + describe('areURLSearchParamsEqual', () => { + it('should return true for equal URLSearchParams objects', () => { + const params1 = new URLSearchParams('a=1&b=2&c=3'); + const params2 = new URLSearchParams('c=3&b=2&a=1'); + expect(areURLSearchParamsEqual(params1, params2)).to.be.true; + }); + + it('should return false for different URLSearchParams objects', () => { + const params1 = new URLSearchParams('a=1&b=2'); + const params2 = new URLSearchParams('a=1&b=3'); + expect(areURLSearchParamsEqual(params1, params2)).to.be.false; + }); + + it('should return false if one of the URLSearchParams objects is empty', () => { + const params1 = new URLSearchParams('a=1&b=2'); + const params2 = new URLSearchParams(); + expect(areURLSearchParamsEqual(params1, params2)).to.be.false; + }); + }); + + describe('mergeURLSearchParams', () => { + it('should merge two URLSearchParams objects with unique keys', () => { + const params1 = new URLSearchParams('a=1&b=2'); + const params2 = new URLSearchParams('c=3&d=4'); + + expect(mergeURLSearchParams(params1, params2)).to.equal('a=1&b=2&c=3&d=4'); + }); + + it('should override keys from the first object with keys from the second', () => { + const params1 = new URLSearchParams('a=1&b=2'); + const params2 = new URLSearchParams('b=3&c=4'); + + expect(mergeURLSearchParams(params1, params2)).to.equal('a=1&b=3&c=4'); + }); + + it('should return only the second object if the first is empty', () => { + const params1 = new URLSearchParams(); + const params2 = new URLSearchParams('a=1&b=2'); + + expect(mergeURLSearchParams(params1, params2)).to.equal('a=1&b=2'); + }); + + it('should return only the first object if the second is empty', () => { + const params1 = new URLSearchParams('a=1&b=2'); + const params2 = new URLSearchParams(); + + expect(mergeURLSearchParams(params1, params2)).to.equal('a=1&b=2'); + }); + + it('should return an empty string if both objects are empty', () => { + const params1 = new URLSearchParams(); + const params2 = new URLSearchParams(); + + expect(mergeURLSearchParams(params1, params2)).to.equal(''); + }); + }); + + describe('escapeNonSpecialQuestionMarks', () => { + it('should escape non-special "?" in plain strings', () => { + const input = 'What? is this? really?'; + const expected = 'What\\? is this\\? really\\?'; + expect(escapeNonSpecialQuestionMarks(input)).to.equal(expected); + }); + + it('should not escape special regex "?" characters', () => { + const input = '(abc)? .*? [a-z]?'; + const expected = '(abc)? .*? [a-z]?'; + expect(escapeNonSpecialQuestionMarks(input)).to.equal(expected); + }); + + it('should handle mixed cases with special and non-special "?"', () => { + const input = 'Is this (true)? or false?'; + const expected = 'Is this (true)? or false\\?'; + expect(escapeNonSpecialQuestionMarks(input)).to.equal(expected); + }); + + it('should escape "?" in a string with no special characters', () => { + const input = 'Just a plain string?'; + const expected = 'Just a plain string\\?'; + expect(escapeNonSpecialQuestionMarks(input)).to.equal(expected); + }); + + it('should handle strings with escaped "?" already', () => { + const input = 'This \\? should stay escaped?'; + const expected = 'This \\? should stay escaped\\?'; + expect(escapeNonSpecialQuestionMarks(input)).to.equal(expected); + }); + + it('should handle an empty string', () => { + const input = ''; + const expected = ''; + expect(escapeNonSpecialQuestionMarks(input)).to.equal(expected); + }); + + it('should handle strings without any "?"', () => { + const input = 'No question marks here.'; + const expected = 'No question marks here.'; + expect(escapeNonSpecialQuestionMarks(input)).to.equal(expected); }); }); }); diff --git a/packages/sitecore-jss/src/utils/utils.ts b/packages/sitecore-jss/src/utils/utils.ts index 56f4f56f6c..cfc73c9d17 100644 --- a/packages/sitecore-jss/src/utils/utils.ts +++ b/packages/sitecore-jss/src/utils/utils.ts @@ -161,17 +161,103 @@ export const enforceCors = ( return false; }; - /** - * Generates all possible permutations of an array of key-value pairs. - * This is used to create every possible combination of URL query parameters. - * @param {Array<[string, string]>} array - The array of key-value pairs to permute. - * @returns {Array>} - A 2D array where each inner array is a unique permutation of the input. - */ - export const getPermutations = (array: [string, string][]): [string, string][][] =>{ - if (array.length <= 1) return [array]; - - return array.flatMap((current, i) => { - const remaining = array.filter((_, idx) => idx !== i); - return getPermutations(remaining).map((permutation) => [current, ...permutation]); - }); +/** + * Determines whether the given input is a regular expression or resembles a URL. + * @param {string} input - The input string to evaluate. + * @returns {'regex' | 'url'} - Returns 'url' if the input looks like a URL, otherwise 'regex'. + */ +export const isRegexOrUrl = (input: string): 'regex' | 'url' => { + // Remove the trailing slash. + input = input.slice(0, -1); + + // Check if the string resembles a URL. + const isUrlLike = /^\/[a-zA-Z0-9\-\/]+(\?([a-zA-Z0-9\-_]+=[a-zA-Z0-9\-_]+)(&[a-zA-Z0-9\-_]+=[a-zA-Z0-9\-_]+)*)?$/.test(input); + + if (isUrlLike) { + return 'url'; + } + + // If it doesn't resemble a URL, it's likely a regular expression. + return 'regex'; +}; + +/** + * Compares two URLSearchParams objects to determine if they are equal. + * @param {URLSearchParams} params1 - The first set of URL search parameters. + * @param {URLSearchParams} params2 - The second set of URL search parameters. + * @returns {boolean} - Returns true if the parameters are equal, otherwise false. + */ +export const areURLSearchParamsEqual = (params1: URLSearchParams, params2: URLSearchParams): boolean => { + // Generates a sorted string representation of URL search parameters. + const getSortedParamsString = (params: URLSearchParams): string => { + return [...params.entries()] + .sort(([keyA], [keyB]) => keyA.localeCompare(keyB)) + .map(([key, value]) => `${key}=${value}`) + .join('&'); + }; + + // Compare the sorted strings of both parameter sets. + return getSortedParamsString(params1) === getSortedParamsString(params2); +}; + +/** + * Escapes non-special "?" characters in a string or regex. + * - For regular strings, it escapes all unescaped "?" characters by adding a backslash (`\`). + * - For regex patterns (strings enclosed in `/.../`), it analyzes each "?" to determine if it has special meaning + * (e.g., `?` in `(abc)?`, `.*?`) or is just a literal character. Only literal "?" characters are escaped. + * @param {string} input - The input string or regex pattern. + * @returns {string} - The modified string or regex with non-special "?" characters escaped. + */ +export const escapeNonSpecialQuestionMarks = (input: string): string => { + const regexPattern = /(? { + const merged = new URLSearchParams(); + + // Add all keys and values from the first object. + for (const [key, value] of params1.entries()) { + merged.set(key, value); + } + + // Add all keys and values from the second object, replacing existing ones. + for (const [key, value] of params2.entries()) { + merged.set(key, value); + } + + return merged.toString(); };