From a6fdb73c7e60d562289a99dd766dbcd3e953e50f Mon Sep 17 00:00:00 2001 From: Ruslan Matkovskyi Date: Fri, 20 Dec 2024 12:37:39 +0100 Subject: [PATCH] [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. --- .../middleware/redirects-middleware.test.ts | 10 +- .../src/middleware/redirects-middleware.ts | 249 ++++++------------ 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 +++++++- 5 files changed, 297 insertions(+), 223 deletions(-) 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 c0b715e5e3..a867c6c564 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts @@ -1,4 +1,4 @@ -import { CacheClient, debug, MemoryCacheClient } from '@sitecore-jss/sitecore-jss'; +import { debug } from '@sitecore-jss/sitecore-jss'; import { GraphQLRedirectsService, GraphQLRedirectsServiceConfig, @@ -8,7 +8,12 @@ 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'; @@ -37,7 +42,6 @@ export type RedirectsMiddlewareConfig = Omit; /** * @param {RedirectsMiddlewareConfig} [config] redirects middleware config @@ -49,10 +53,6 @@ export class RedirectsMiddleware extends MiddlewareBase { // (underlying default 'cross-fetch' is not currently compatible: https://github.com/lquixada/cross-fetch/issues/78) this.redirectsService = new GraphQLRedirectsService({ ...config, fetch: fetch }); this.locales = config.locales; - this.cache = new MemoryCacheClient({ - cacheEnabled: config.cacheEnabled, - cacheTimeout: config.cacheTimeout, - }); } /** @@ -85,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); @@ -104,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 @@ -127,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; @@ -160,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; } }; @@ -199,32 +217,32 @@ export class RedirectsMiddleware extends MiddlewareBase { const { pathname: targetURL, search: targetQS = '', locale } = this.normalizeUrl( req.nextUrl.clone() ); - const cacheKey = `${targetURL}-${targetQS}-${locale}`; - const cachedRedirect = this.cache.getCacheValue(cacheKey); - - if (cachedRedirect !== null) { - return typeof cachedRedirect === 'boolean' ? undefined : cachedRedirect; - } - + 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; - const result = modifyRedirects.length + return modifyRedirects.length ? modifyRedirects.find((redirect: RedirectResult) => { - // generate cache key for the current pattern - const chachedPatternResultKey = `${cacheKey}-${redirect.pattern}-${redirect.target}`; - // Check if the result is already cached - const chachedPatternResult = this.cache.getCacheValue(chachedPatternResultKey); - - if (chachedPatternResult !== null) { - return chachedPatternResult; + 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 = escapeNonSpecialQuestionMarks( redirect.pattern.replace(new RegExp(`^[^]?/${language}/`, 'gi'), '') ); @@ -235,59 +253,25 @@ 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 || ''; - const matchedPatterResult = + return ( !!( regexParser(redirect.pattern).test(targetURL) || regexParser(redirect.pattern).test(`/${req.nextUrl.locale}${targetURL}`) || matchedQueryString - ) && (redirect.locale ? redirect.locale.toLowerCase() === locale.toLowerCase() : true); - - // Save cache the result for the current pattern - this.cache.setCacheValue(chachedPatternResultKey, matchedPatterResult); - - // Return the redirect if the URL path or any query string permutation matches the pattern - return matchedPatterResult; + ) && (redirect.locale ? redirect.locale.toLowerCase() === locale.toLowerCase() : true) + ); }) : undefined; - - this.cache.setCacheValue(cacheKey, result ? result : undefined); - - return result; } /** @@ -361,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(); };