Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sitecore-jss-nextjs]: Improve performance for redirects #SXA-7834 #2003

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1383,6 +1383,7 @@ describe('RedirectsMiddleware', () => {
origin: 'http://localhost:3000',
clone: cloneUrl,
},
headerValues: { 'cdn-loop': 'netlify' },
},
});
setupRedirectStub(301);
Expand Down
75 changes: 49 additions & 26 deletions packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@ import {
SiteInfo,
} from '@sitecore-jss/sitecore-jss/site';
import { getPermutations } 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');
const NAME_NETLIFY = 'netlify';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case you leave netlify check (see my question below)

Suggested change
const NAME_NETLIFY = 'netlify';
const NETLIFY_CDN_HEADER = 'netlify';


type RedirectResult = RedirectInfo & { matchedQueryString?: string };

/**
* extended RedirectsMiddlewareConfig config type for RedirectsMiddleware
Expand Down Expand Up @@ -78,15 +81,24 @@ 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
if (this.isPrefetch(req)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some explanation on the reason of skipping prefetch requests, for better understanding if other people will need to figure out the scenario

debug.redirects('skipped (prefetch)');
response.headers.set('x-middleware-cache', 'no-cache');
return response;
}

site = this.getSite(req, res);
Expand All @@ -97,7 +109,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
Expand Down Expand Up @@ -153,16 +165,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;
}
};

Expand All @@ -188,20 +200,22 @@ 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<RedirectResult | undefined> {
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) => {
// 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.replace(new RegExp(`^[^]?/${language}/`, 'gi'), '')
);

// Prepare the redirect pattern as a regular expression, making it more flexible for matching URLs
Expand Down Expand Up @@ -236,22 +250,32 @@ export class RedirectsMiddleware extends MiddlewareBase {
* 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,
});
if (req.headers.get('cdn-loop') === NAME_NETLIFY) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to safely check for netlify? Env variables?
Another question - do we really need to do this check, since :261 line does almost the same what getPermutedQueryMatch does. Does it really optimize something? since the same reg exp is executed

matchedQueryString = this.getPermutedQueryMatch({
pathname: normalizedPath,
queryString: targetQS,
pattern: redirect.pattern,
locale,
});
} else {
matchedQueryString = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me looks like this bunch of code does almost the same what getPermutedQueryMatch does
do you just need to use the same method?

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;
Expand Down Expand Up @@ -339,7 +363,7 @@ export class RedirectsMiddleware extends MiddlewareBase {
* @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({
private getPermutedQueryMatch({
pathname,
queryString,
pattern,
Expand All @@ -356,11 +380,10 @@ export class RedirectsMiddleware extends MiddlewareBase {
'?' + 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}`),
regexParser(pattern).test(`${pathname}${query}`),
regexParser(pattern).test(`/${locale}${pathname}${query}`),
].some(Boolean)
);
}
Expand All @@ -373,7 +396,7 @@ export class RedirectsMiddleware extends MiddlewareBase {
* (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 = /(?<!\\)\?/g; // Find unescaped "?" characters
const isRegex = input.startsWith('/') && input.endsWith('/'); // Check if the string is a regex
Expand Down