From a1e3e7eaf9bbcfed9e4cbee02b58a0eca97123a6 Mon Sep 17 00:00:00 2001 From: illiakovalenko Date: Wed, 8 Mar 2023 13:00:23 +0200 Subject: [PATCH 1/3] [sitecore-jss-nextjs] Implemented MiddlewareBase abstraction. Skip Redirects middleware during editing. --- .../src/middleware/middleware.test.ts | 174 ++++++++++++++++++ .../src/middleware/middleware.ts | 76 ++++++++ .../src/middleware/multisite-middleware.ts | 53 +----- .../src/middleware/personalize-middleware.ts | 64 +------ .../middleware/redirects-middleware.test.ts | 62 ++++++- .../src/middleware/redirects-middleware.ts | 94 ++++------ 6 files changed, 366 insertions(+), 157 deletions(-) create mode 100644 packages/sitecore-jss-nextjs/src/middleware/middleware.test.ts create mode 100644 packages/sitecore-jss-nextjs/src/middleware/middleware.ts diff --git a/packages/sitecore-jss-nextjs/src/middleware/middleware.test.ts b/packages/sitecore-jss-nextjs/src/middleware/middleware.test.ts new file mode 100644 index 0000000000..274fa16d64 --- /dev/null +++ b/packages/sitecore-jss-nextjs/src/middleware/middleware.test.ts @@ -0,0 +1,174 @@ +/* eslint-disable dot-notation */ +import chai, { use } from 'chai'; +import sinonChai from 'sinon-chai'; +import chaiString from 'chai-string'; +import { MiddlewareBase } from './middleware'; +import { NextRequest } from 'next/server'; + +use(sinonChai); +const expect = chai.use(chaiString).expect; + +describe('MiddlewareBase', () => { + class SampleMiddleware extends MiddlewareBase {} + + const createReq = (props: any = {}) => { + return { + cookies: { + get(cookieName: string) { + const cookies = { ...props?.cookieValues }; + return { value: cookies[cookieName] }; + }, + }, + headers: { + get(key: string) { + const headers = { + ...props?.headerValues, + }; + return headers[key]; + }, + }, + nextUrl: { + ...props?.nextUrl, + }, + } as NextRequest; + }; + + describe('defaultHostname', () => { + it('should set default hostname', () => { + const middleware = new SampleMiddleware(); + + expect(middleware['defaultHostname']).to.equal('localhost'); + }); + + it('should set custom hostname', () => { + const middleware = new SampleMiddleware({ + defaultHostname: 'foo', + }); + + expect(middleware['defaultHostname']).to.equal('foo'); + }); + }); + + describe('isPreview', () => { + it('should return true prerender bypass cookie is provided', () => { + const middleware = new SampleMiddleware(); + const req = createReq({ + cookieValues: { + __prerender_bypass: true, + }, + }); + + expect(middleware['isPreview'](req)).to.equal(true); + }); + + it('should return true when preview data cookie is provided', () => { + const middleware = new SampleMiddleware(); + const req = createReq({ + cookieValues: { + __next_preview_data: true, + }, + }); + + expect(middleware['isPreview'](req)).to.equal(true); + }); + + it('should return false when required cookie is not provided', () => { + const middleware = new SampleMiddleware(); + const req = createReq(); + + expect(middleware['isPreview'](req)).to.equal(false); + }); + }); + + describe('excludeRoute', () => { + it('default', () => { + const middleware = new SampleMiddleware(); + + expect(middleware['excludeRoute']('/src/image.png')).to.equal(true); + expect(middleware['excludeRoute']('/api/layout/render')).to.equal(true); + expect(middleware['excludeRoute']('/sitecore/render')).to.equal(true); + expect(middleware['excludeRoute']('/_next/webpack')).to.equal(true); + }); + + it('custom function', () => { + const middleware = new SampleMiddleware({ + excludeRoute(path: string) { + return path === 'foo'; + }, + }); + + expect(middleware['excludeRoute']('bar')).to.equal(false); + expect(middleware['excludeRoute']('foo')).to.equal(true); + }); + }); + + it('extractDebugHeaders', () => { + const middleware = new SampleMiddleware(); + + const headers = new Headers({}); + headers.set('foo', 'net'); + headers.set('bar', 'one'); + + expect(middleware['extractDebugHeaders'](headers)).to.deep.equal({ + foo: 'net', + bar: 'one', + }); + }); + + describe('getHostHeader', () => { + it('should return default hostname when header is not present', () => { + const middleware = new SampleMiddleware(); + const req = createReq({ + headerValues: { + foo: 'one', + }, + }); + + expect(middleware['getHostHeader'](req)).to.equal(undefined); + }); + + it('should return host header', () => { + const middleware = new SampleMiddleware(); + const req = createReq({ + headerValues: { + foo: 'one', + host: 'bar.net:9999', + }, + }); + + expect(middleware['getHostHeader'](req)).to.equal('bar.net'); + }); + }); + + describe('getLanguage', () => { + it('should return defined language', () => { + const middleware = new SampleMiddleware(); + const req = createReq({ + nextUrl: { + locale: 'be', + defaultLocale: 'fr', + }, + }); + + expect(middleware['getLanguage'](req)).to.equal('be'); + }); + + it('should return defined default language', () => { + const middleware = new SampleMiddleware(); + const req = createReq({ + nextUrl: { + defaultLocale: 'fr', + }, + }); + + expect(middleware['getLanguage'](req)).to.equal('fr'); + }); + + it('should return fallback language', () => { + const middleware = new SampleMiddleware(); + const req = createReq(); + + expect(middleware['getLanguage'](req)).to.equal('en'); + }); + }); +}); diff --git a/packages/sitecore-jss-nextjs/src/middleware/middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/middleware.ts new file mode 100644 index 0000000000..c057e84920 --- /dev/null +++ b/packages/sitecore-jss-nextjs/src/middleware/middleware.ts @@ -0,0 +1,76 @@ +import { NextRequest } from 'next/server'; + +export type MiddlewareBaseConfig = { + /** + * Function used to determine if route should be excluded. + * By default, files (pathname.includes('.')), Next.js API routes (pathname.startsWith('/api/')), and Sitecore API routes (pathname.startsWith('/sitecore/')) are ignored. + * This is an important performance consideration since Next.js Edge middleware runs on every request. + * @param {string} pathname The pathname + * @returns {boolean} Whether to exclude the route + */ + excludeRoute?: (pathname: string) => boolean; + /** + * Fallback hostname in case `host` header is not present + * @default localhost + */ + defaultHostname?: string; +}; + +export abstract class MiddlewareBase { + protected SITE_SYMBOL = 'sc_site'; + protected defaultHostname: string; + + constructor(protected config?: MiddlewareBaseConfig) { + this.defaultHostname = config?.defaultHostname || 'localhost'; + } + + /** + * Determines if mode is preview + * @param {NextRequest} req request + * @returns {boolean} is preview + */ + protected isPreview(req: NextRequest) { + return !!( + req.cookies.get('__prerender_bypass')?.value || req.cookies.get('__next_preview_data')?.value + ); + } + + protected excludeRoute(pathname: string) { + return ( + pathname.includes('.') || // Ignore files + pathname.startsWith('/api/') || // Ignore Next.js API calls + pathname.startsWith('/sitecore/') || // Ignore Sitecore API calls + pathname.startsWith('/_next') || // Ignore next service calls + (this.config?.excludeRoute && this.config?.excludeRoute(pathname)) + ); + } + + /** + * Safely extract all headers for debug logging + * Necessary to avoid middleware issue https://github.com/vercel/next.js/issues/39765 + * @param {Headers} incomingHeaders Incoming headers + * @returns Object with headers as key/value pairs + */ + protected extractDebugHeaders(incomingHeaders: Headers) { + const headers = {} as { [key: string]: string }; + incomingHeaders.forEach((value, key) => (headers[key] = value)); + return headers; + } + + /** + * Provides used language + * @param {NextRequest} req request + * @returns {string} language + */ + protected getLanguage(req: NextRequest) { + return req.nextUrl.locale || req.nextUrl.defaultLocale || 'en'; + } + + /** + * Extract 'host' header + * @param {NextRequest} req request + */ + protected getHostHeader(req: NextRequest) { + return req.headers.get('host')?.split(':')[0]; + } +} diff --git a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts index a7e9f365d3..edbdb122de 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts @@ -1,25 +1,13 @@ import { NextResponse, NextRequest } from 'next/server'; import { getSiteRewrite, SiteInfo } from '@sitecore-jss/sitecore-jss/site'; import { debug } from '@sitecore-jss/sitecore-jss'; +import { MiddlewareBase, MiddlewareBaseConfig } from './middleware'; -export type MultisiteMiddlewareConfig = { - /** - * Function used to determine if route should be excluded during execution. - * By default, files (pathname.includes('.')), Next.js API routes (pathname.startsWith('/api/')), and Sitecore API routes (pathname.startsWith('/sitecore/')) are ignored. - * This is an important performance consideration since Next.js Edge middleware runs on every request. - * @param {string} pathname The pathname - * @returns {boolean} Whether to exclude the route - */ - excludeRoute?: (pathname: string) => boolean; +export type MultisiteMiddlewareConfig = MiddlewareBaseConfig & { /** * Function used to resolve site for given hostname */ getSite: (hostname: string) => SiteInfo; - /** - * Fallback hostname in case `host` header is not present - * @default localhost - */ - defaultHostname?: string; /** * Function used to determine if site should be resolved from sc_site cookie when present */ @@ -29,14 +17,12 @@ export type MultisiteMiddlewareConfig = { /** * Middleware / handler for multisite support */ -export class MultisiteMiddleware { - private defaultHostname: string; - +export class MultisiteMiddleware extends MiddlewareBase { /** * @param {MultisiteMiddlewareConfig} [config] Multisite middleware config */ constructor(protected config: MultisiteMiddlewareConfig) { - this.defaultHostname = config.defaultHostname || 'localhost'; + super({ defaultHostname: config.defaultHostname, excludeRoute: config.excludeRoute }); } /** @@ -55,27 +41,9 @@ export class MultisiteMiddleware { }; } - protected excludeRoute(pathname: string) { - if ( - pathname.includes('.') || // Ignore files - pathname.startsWith('/api/') || // Ignore Next.js API calls - pathname.startsWith('/sitecore/') || // Ignore Sitecore API calls - pathname.startsWith('/_next') // Ignore next service calls - ) { - return true; - } - return false; - } - - protected extractDebugHeaders(incomingHeaders: Headers) { - const headers = {} as { [key: string]: string }; - incomingHeaders.forEach((value, key) => (headers[key] = value)); - return headers; - } - private handler = async (req: NextRequest, res?: NextResponse): Promise => { const pathname = req.nextUrl.pathname; - const hostHeader = req.headers.get('host')?.split(':')[0]; + const hostHeader = this.getHostHeader(req); const hostname = hostHeader || this.defaultHostname; debug.multisite('multisite middleware start: %o', { pathname, @@ -89,20 +57,17 @@ export class MultisiteMiddleware { // Response will be provided if other middleware is run before us let response = res || NextResponse.next(); - if ( - this.excludeRoute(pathname) || - (this.config.excludeRoute && this.config.excludeRoute(pathname)) - ) { + if (this.excludeRoute(pathname)) { debug.multisite('skipped (route excluded)'); return response; } // Site name can be forced by query string parameter or cookie const siteName = - req.nextUrl.searchParams.get('sc_site') || + req.nextUrl.searchParams.get(this.SITE_SYMBOL) || (this.config.useCookieResolution && this.config.useCookieResolution(req) && - req.cookies.get('sc_site')?.value) || + req.cookies.get(this.SITE_SYMBOL)?.value) || this.config.getSite(hostname).name; // Rewrite to site specific path @@ -118,7 +83,7 @@ export class MultisiteMiddleware { response = NextResponse.rewrite(rewriteUrl); // Share site name with the following executed middlewares - response.cookies.set('sc_site', siteName); + response.cookies.set(this.SITE_SYMBOL, siteName); // Share rewrite path with following executed middlewares response.headers.set('x-sc-rewrite', rewritePath); diff --git a/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts index 946f96d55c..15b920104b 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts @@ -9,16 +9,9 @@ import { } from '@sitecore-jss/sitecore-jss/personalize'; import { SiteResolver } from '@sitecore-jss/sitecore-jss/site'; import { debug, NativeDataFetcher } from '@sitecore-jss/sitecore-jss'; +import { MiddlewareBase, MiddlewareBaseConfig } from './middleware'; -export type PersonalizeMiddlewareConfig = { - /** - * Function used to determine if route should be excluded from personalization. - * By default, files (pathname.includes('.')), Next.js API routes (pathname.startsWith('/api/')), and Sitecore API routes (pathname.startsWith('/sitecore/')) are ignored. - * This is an important performance consideration since Next.js Edge middleware runs on every request. - * @param {string} pathname The pathname - * @returns {boolean} Whether to exclude the route from personalization - */ - excludeRoute?: (pathname: string) => boolean; +export type PersonalizeMiddlewareConfig = MiddlewareBaseConfig & { /** * Configuration for your Sitecore Experience Edge endpoint */ @@ -38,25 +31,21 @@ export type PersonalizeMiddlewareConfig = { * Site resolution implementation by name/hostname */ siteResolver: SiteResolver; - /** - * fallback hostname in case `host` header is not present - * @default localhost - */ - defaultHostname?: string; }; /** * Middleware / handler to support Sitecore Personalize */ -export class PersonalizeMiddleware { +export class PersonalizeMiddleware extends MiddlewareBase { private personalizeService: GraphQLPersonalizeService; private cdpService: CdpService; - private defaultHostname: string; /** * @param {PersonalizeMiddlewareConfig} [config] Personalize middleware config */ constructor(protected config: PersonalizeMiddlewareConfig) { + super({ defaultHostname: config.defaultHostname, excludeRoute: config.excludeRoute }); + // NOTE: we provide native fetch for compatibility on Next.js Edge Runtime // (underlying default 'cross-fetch' is not currently compatible: https://github.com/lquixada/cross-fetch/issues/78) this.personalizeService = new GraphQLPersonalizeService({ @@ -81,8 +70,6 @@ export class PersonalizeMiddleware { return (url: string, data?: unknown) => fetcher.fetch(url, data); }, }); - - this.defaultHostname = config.defaultHostname || 'localhost'; } /** @@ -128,42 +115,12 @@ export class PersonalizeMiddleware { }; } - protected excludeRoute(pathname: string) { - if ( - pathname.includes('.') || // Ignore files - pathname.startsWith('/api/') || // Ignore Next.js API calls - pathname.startsWith('/sitecore/') || // Ignore Sitecore API calls - pathname.startsWith('/_next') // Ignore next service calls - ) { - return true; - } - return false; - } - - protected isPreview(req: NextRequest) { - return ( - req.cookies.get('__prerender_bypass')?.value || req.cookies.get('__next_preview_data')?.value - ); - } - - /** - * Safely extract all headers for debug logging - * Necessary to avoid middleware issue https://github.com/vercel/next.js/issues/39765 - * @param {Headers} incomingHeaders Incoming headers - * @returns Object with headers as key/value pairs - */ - protected extractDebugHeaders(incomingHeaders: Headers) { - const headers = {} as { [key: string]: string }; - incomingHeaders.forEach((value, key) => (headers[key] = value)); - return headers; - } - private handler = async (req: NextRequest, res?: NextResponse): Promise => { - const hostHeader = req.headers.get('host')?.split(':')[0]; + const hostHeader = this.getHostHeader(req); const hostname = hostHeader || this.defaultHostname; const pathname = req.nextUrl.pathname; - const language = req.nextUrl.locale || req.nextUrl.defaultLocale || 'en'; - let siteName = res?.cookies.get('sc_site')?.value; + const language = this.getLanguage(req); + let siteName = res?.cookies.get(this.SITE_SYMBOL)?.value; let browserId = this.getBrowserId(req); debug.personalize('personalize middleware start: %o', { @@ -186,8 +143,7 @@ export class PersonalizeMiddleware { if ( response.redirected || // Don't attempt to personalize a redirect this.isPreview(req) || // No need to personalize for preview (layout data is already prepared for preview) - this.excludeRoute(pathname) || - (this.config.excludeRoute && this.config.excludeRoute(pathname)) + this.excludeRoute(pathname) ) { debug.personalize( 'skipped (%s)', @@ -275,7 +231,7 @@ export class PersonalizeMiddleware { // Set browserId cookie on the response this.setBrowserId(response, browserId); // Share site name with the following executed middlewares - response.cookies.set('sc_site', siteName); + response.cookies.set(this.SITE_SYMBOL, siteName); debug.personalize('personalize middleware end: %o', { rewritePath, 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 4554d78dfb..027c1c17b8 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.test.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.test.ts @@ -3,15 +3,20 @@ import chai, { use } from 'chai'; import chaiString from 'chai-string'; import sinonChai from 'sinon-chai'; -import sinon from 'sinon'; +import sinon, { spy } from 'sinon'; import { NextRequest, NextResponse } from 'next/server'; -import { RedirectsMiddleware } from './redirects-middleware'; +import { debug } from '@sitecore-jss/sitecore-jss'; import { REDIRECT_TYPE_SERVER_TRANSFER } from '@sitecore-jss/sitecore-jss/site'; +import { RedirectsMiddleware } from './redirects-middleware'; use(sinonChai); const expect = chai.use(chaiString).expect; describe('RedirectsMiddleware', () => { + const debugSpy = spy(debug, 'redirects'); + const validateDebugLog = (message, ...params) => + expect(debugSpy.args.find((log) => log[0] === message)).to.deep.equal([message, ...params]); + const referrer = 'http://localhost:3000'; const hostname = 'foo.net'; const siteName = 'nextjs-app'; @@ -103,11 +108,47 @@ describe('RedirectsMiddleware', () => { // Stub for NextResponse generation, see https://github.com/vercel/next.js/issues/42374 (Headers.prototype as any).getAll = () => []; - afterEach(() => { - sinon.restore(); + beforeEach(() => { + debugSpy.resetHistory(); }); describe('redirects middleware - getHandler', () => { + describe('preview', () => { + it('prerender bypass cookie is present', async () => { + const { middleware } = createMiddleware(); + const res = NextResponse.next(); + + const req = createRequest({ + cookies: { + __prerender_bypass: true, + }, + }); + + const finalRes = await middleware.getHandler()(req, res); + + validateDebugLog('skipped (%s)', 'preview'); + + expect(finalRes).to.deep.equal(res); + }); + + it('preview data cookie is present', async () => { + const { middleware } = createMiddleware(); + const res = NextResponse.next(); + + const req = createRequest({ + cookies: { + __next_preview_data: true, + }, + }); + + const finalRes = await middleware.getHandler()(req, res); + + validateDebugLog('skipped (%s)', 'preview'); + + expect(finalRes).to.deep.equal(res); + }); + }); + describe('exclude route', () => { const res = NextResponse.next(); @@ -120,6 +161,8 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req, res); + validateDebugLog('skipped (%s)', 'route excluded'); + expect(finalRes).to.deep.equal(res); }; it('default', async () => { @@ -161,6 +204,8 @@ describe('RedirectsMiddleware', () => { const { middleware } = createMiddleware(props); const finalRes = await middleware.getHandler()(req); + validateDebugLog('skipped (redirects middleware is disabled)'); + expect(finalRes).to.deep.equal(res); nextStub.restore(); @@ -175,6 +220,8 @@ describe('RedirectsMiddleware', () => { const { middleware, fetchRedirects, getSite } = createMiddleware(); const finalRes = await middleware.getHandler()(req); + validateDebugLog('skipped (redirect does not exist)'); + expect(getSite).to.be.calledWith(hostname); // eslint-disable-next-line no-unused-expressions expect(fetchRedirects.called).to.be.true; @@ -519,6 +566,9 @@ describe('RedirectsMiddleware', () => { }); const finalRes = await middleware.getHandler()(req, res); + + validateDebugLog('skipped (redirects middleware is disabled)'); + // eslint-disable-next-line no-unused-expressions expect(getSite).to.not.be.called; // eslint-disable-next-line no-unused-expressions @@ -564,6 +614,8 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req); + validateDebugLog('host header is missing, default localhost is used'); + expect(getSite).to.be.calledWith('localhost'); expect(fetchRedirects).to.be.calledWith(siteName); expect(finalRes).to.deep.equal(res); @@ -611,6 +663,8 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req); + validateDebugLog('host header is missing, default foobar is used'); + expect(getSite).to.be.calledWith('foobar'); expect(fetchRedirects).to.be.calledWith(siteName); expect(finalRes).to.deep.equal(res); diff --git a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts index 89ffb79293..c1b1989a56 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts @@ -9,55 +9,45 @@ import { REDIRECT_TYPE_SERVER_TRANSFER, SiteInfo, } from '@sitecore-jss/sitecore-jss/site'; +import { debug } from '@sitecore-jss/sitecore-jss'; +import { MiddlewareBase, MiddlewareBaseConfig } from './middleware'; /** * extended RedirectsMiddlewareConfig config type for RedirectsMiddleware */ -export type RedirectsMiddlewareConfig = Omit & { - locales: string[]; - /** - * Function used to determine if route should be excluded from RedirectsMiddleware. - * By default, files (pathname.includes('.')), Next.js API routes (pathname.startsWith('/api/')), and Sitecore API routes (pathname.startsWith('/sitecore/')) are ignored. - * This is an important performance consideration since Next.js Edge middleware runs on every request. - * @param {string} pathname The pathname - * @returns {boolean} Whether to exclude the route from RedirectsMiddleware - */ - excludeRoute?: (pathname: string) => boolean; - /** - * function, determines if middleware should be turned off, based on cookie, header, or other considerations - * @param {NextRequest} [req] optional: request object from middleware handler - * @param {NextResponse} [res] optional: response object from middleware handler - * @returns {boolean} false by default - */ - disabled?: (req?: NextRequest, res?: NextResponse) => boolean; - /** - * function used to resolve site for given hostname - */ - getSite: (hostname: string) => SiteInfo; - /** - * fallback hostname in case `host` header is not present - * @default localhost - */ - defaultHostname?: string; -}; +export type RedirectsMiddlewareConfig = Omit & + MiddlewareBaseConfig & { + locales: string[]; + /** + * function, determines if middleware should be turned off, based on cookie, header, or other considerations + * @param {NextRequest} [req] optional: request object from middleware handler + * @param {NextResponse} [res] optional: response object from middleware handler + * @returns {boolean} false by default + */ + disabled?: (req?: NextRequest, res?: NextResponse) => boolean; + /** + * function used to resolve site for given hostname + */ + getSite: (hostname: string) => SiteInfo; + }; /** * Middleware / handler fetches all redirects from Sitecore instance by grapqhl service * compares with current url and redirects to target url */ -export class RedirectsMiddleware { +export class RedirectsMiddleware extends MiddlewareBase { private redirectsService: GraphQLRedirectsService; private locales: string[]; - private defaultHostname: string; /** * @param {RedirectsMiddlewareConfig} [config] redirects middleware config */ constructor(protected config: RedirectsMiddlewareConfig) { + super({ defaultHostname: config.defaultHostname, excludeRoute: config.excludeRoute }); + // NOTE: we provide native fetch for compatibility on Next.js Edge Runtime // (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.defaultHostname = config.defaultHostname || 'localhost'; } /** @@ -68,39 +58,33 @@ export class RedirectsMiddleware { return this.handler; } - protected excludeRoute(pathname: string) { - if ( - pathname.includes('.') || // Ignore files - pathname.startsWith('/api/') || // Ignore Next.js API calls - pathname.startsWith('/sitecore/') || // Ignore Sitecore API calls - pathname.startsWith('/_next') // Ignore next service calls - ) { - return true; - } - return false; - } - - protected getHostname(req: NextRequest) { - const hostHeader = req.headers.get('host')?.split(':')[0]; - return hostHeader || this.defaultHostname; - } - private handler = async (req: NextRequest, res?: NextResponse): Promise => { - const hostname = this.getHostname(req); - const siteName = res?.cookies.get('sc_site')?.value || this.config.getSite(hostname).name; + const hostHeader = this.getHostHeader(req); + const hostname = hostHeader || this.defaultHostname; + const siteName = + res?.cookies.get(this.SITE_SYMBOL)?.value || this.config.getSite(hostname).name; const createResponse = async () => { - if ( - (this.config.disabled && this.config.disabled(req, NextResponse.next())) || - this.excludeRoute(req.nextUrl.pathname) || - (this.config.excludeRoute && this.config.excludeRoute(req.nextUrl.pathname)) - ) { + if (!hostHeader) { + debug.redirects(`host header is missing, default ${hostname} is used`); + } + + if (this.config.disabled && this.config.disabled(req, NextResponse.next())) { + debug.redirects('skipped (redirects middleware is disabled)'); + return res || NextResponse.next(); + } + + if (this.isPreview(req) || this.excludeRoute(req.nextUrl.pathname)) { + debug.redirects('skipped (%s)', this.isPreview(req) ? 'preview' : 'route excluded'); + return res || NextResponse.next(); } // Find the redirect from result of RedirectService const existsRedirect = await this.getExistsRedirect(req, siteName); if (!existsRedirect) { + debug.redirects('skipped (redirect does not exist)'); + return res || NextResponse.next(); } @@ -138,7 +122,7 @@ export class RedirectsMiddleware { const response = await createResponse(); // Share site name with the following executed middlewares - response.cookies.set('sc_site', siteName); + response.cookies.set(this.SITE_SYMBOL, siteName); return response; }; From ce5bb7ca612da1008af313b2e2c9937e02d1c9a3 Mon Sep 17 00:00:00 2001 From: illiakovalenko Date: Thu, 9 Mar 2023 11:34:29 +0200 Subject: [PATCH 2/3] Changes according to review --- .../src/lib/middleware/plugins/multisite.ts | 6 +- .../src/lib/middleware/plugins/redirects.ts | 4 +- .../src/middleware/middleware.test.ts | 104 ++++++++++++++++-- .../src/middleware/middleware.ts | 33 +++++- .../middleware/multisite-middleware.test.ts | 72 +++++++----- .../src/middleware/multisite-middleware.ts | 12 +- .../middleware/personalize-middleware.test.ts | 10 +- .../src/middleware/personalize-middleware.ts | 36 ++---- .../middleware/redirects-middleware.test.ts | 83 +++++++------- .../src/middleware/redirects-middleware.ts | 30 ++--- 10 files changed, 246 insertions(+), 144 deletions(-) diff --git a/packages/create-sitecore-jss/src/templates/nextjs-multisite/src/lib/middleware/plugins/multisite.ts b/packages/create-sitecore-jss/src/templates/nextjs-multisite/src/lib/middleware/plugins/multisite.ts index 850a93c7ac..375a8838b3 100644 --- a/packages/create-sitecore-jss/src/templates/nextjs-multisite/src/lib/middleware/plugins/multisite.ts +++ b/packages/create-sitecore-jss/src/templates/nextjs-multisite/src/lib/middleware/plugins/multisite.ts @@ -24,10 +24,10 @@ class MultisitePlugin implements MiddlewarePlugin { // Certain paths are ignored by default (e.g. files and Next.js API routes), but you may wish to exclude more. // This is an important performance consideration since Next.js Edge middleware runs on every request. excludeRoute: () => false, - // This function resolves site based on hostname - getSite: siteResolver.getByHost, + // Site resolver implementation + siteResolver, // This function allows resolving site from sc_site cookie, which could be useful in case of Vercel preview URLs. Accepts NextRequest. - useCookieResolution: () => process.env.VERCEL_ENV === 'preview', + useCookieResolution: () => process.env.VERCEL_ENV === 'preview', }); } diff --git a/packages/create-sitecore-jss/src/templates/nextjs-sxa/src/lib/middleware/plugins/redirects.ts b/packages/create-sitecore-jss/src/templates/nextjs-sxa/src/lib/middleware/plugins/redirects.ts index 0ee2fd780b..825bcc434d 100644 --- a/packages/create-sitecore-jss/src/templates/nextjs-sxa/src/lib/middleware/plugins/redirects.ts +++ b/packages/create-sitecore-jss/src/templates/nextjs-sxa/src/lib/middleware/plugins/redirects.ts @@ -22,8 +22,8 @@ class RedirectsPlugin implements MiddlewarePlugin { // This function determines if the middleware should be turned off. // By default it is disabled while in development mode. disabled: () => process.env.NODE_ENV === 'development', - // This function resolves site based on hostname - getSite: siteResolver.getByHost, + // Site resolver implementation + siteResolver, }); } diff --git a/packages/sitecore-jss-nextjs/src/middleware/middleware.test.ts b/packages/sitecore-jss-nextjs/src/middleware/middleware.test.ts index 274fa16d64..a975c7076d 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/middleware.test.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/middleware.test.ts @@ -1,9 +1,11 @@ /* eslint-disable dot-notation */ import chai, { use } from 'chai'; import sinonChai from 'sinon-chai'; +import sinon from 'sinon'; import chaiString from 'chai-string'; import { MiddlewareBase } from './middleware'; -import { NextRequest } from 'next/server'; +import { NextRequest, NextResponse } from 'next/server'; +import { SiteResolver } from '@sitecore-jss/sitecore-jss/site'; use(sinonChai); const expect = chai.use(chaiString).expect; @@ -11,6 +13,20 @@ const expect = chai.use(chaiString).expect; describe('MiddlewareBase', () => { class SampleMiddleware extends MiddlewareBase {} + class MockSiteResolver extends SiteResolver { + getByName = sinon.stub().callsFake((siteName: string) => ({ + name: siteName, + language: 'en', + hostName: 'foo.net', + })); + + getByHost = sinon.stub().callsFake((hostName: string) => ({ + name: 'foo', + language: 'en', + hostName, + })); + } + const createReq = (props: any = {}) => { return { cookies: { @@ -33,15 +49,28 @@ describe('MiddlewareBase', () => { } as NextRequest; }; + const createRes = (props: any = {}) => { + return { + ...props, + cookies: { + get(cookieName: string) { + const cookies = { ...props.cookies }; + return { value: cookies[cookieName] }; + }, + }, + } as NextResponse; + }; + describe('defaultHostname', () => { it('should set default hostname', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); expect(middleware['defaultHostname']).to.equal('localhost'); }); it('should set custom hostname', () => { const middleware = new SampleMiddleware({ + siteResolver: new MockSiteResolver([]), defaultHostname: 'foo', }); @@ -51,7 +80,7 @@ describe('MiddlewareBase', () => { describe('isPreview', () => { it('should return true prerender bypass cookie is provided', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const req = createReq({ cookieValues: { __prerender_bypass: true, @@ -62,7 +91,7 @@ describe('MiddlewareBase', () => { }); it('should return true when preview data cookie is provided', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const req = createReq({ cookieValues: { __next_preview_data: true, @@ -73,7 +102,7 @@ describe('MiddlewareBase', () => { }); it('should return false when required cookie is not provided', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const req = createReq(); expect(middleware['isPreview'](req)).to.equal(false); @@ -82,7 +111,7 @@ describe('MiddlewareBase', () => { describe('excludeRoute', () => { it('default', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); expect(middleware['excludeRoute']('/src/image.png')).to.equal(true); expect(middleware['excludeRoute']('/api/layout/render')).to.equal(true); @@ -92,6 +121,7 @@ describe('MiddlewareBase', () => { it('custom function', () => { const middleware = new SampleMiddleware({ + siteResolver: new MockSiteResolver([]), excludeRoute(path: string) { return path === 'foo'; }, @@ -103,7 +133,7 @@ describe('MiddlewareBase', () => { }); it('extractDebugHeaders', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const headers = new Headers({}); headers.set('foo', 'net'); @@ -117,7 +147,7 @@ describe('MiddlewareBase', () => { describe('getHostHeader', () => { it('should return default hostname when header is not present', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const req = createReq({ headerValues: { foo: 'one', @@ -128,7 +158,7 @@ describe('MiddlewareBase', () => { }); it('should return host header', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const req = createReq({ headerValues: { foo: 'one', @@ -142,7 +172,7 @@ describe('MiddlewareBase', () => { describe('getLanguage', () => { it('should return defined language', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const req = createReq({ nextUrl: { locale: 'be', @@ -154,7 +184,7 @@ describe('MiddlewareBase', () => { }); it('should return defined default language', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const req = createReq({ nextUrl: { defaultLocale: 'fr', @@ -165,10 +195,60 @@ describe('MiddlewareBase', () => { }); it('should return fallback language', () => { - const middleware = new SampleMiddleware(); + const middleware = new SampleMiddleware({ siteResolver: new MockSiteResolver([]) }); const req = createReq(); expect(middleware['getLanguage'](req)).to.equal('en'); }); }); + + describe('getSite', () => { + it('should get site by name when site cookie is provided', () => { + const req = createReq(); + const res = createRes({ + cookies: { + sc_site: 'xxx', + }, + }); + const siteResolver = new MockSiteResolver([]); + const middleware = new SampleMiddleware({ siteResolver }); + + expect(middleware['getSite'](req, res).name).to.equal('xxx'); + expect(siteResolver.getByName).to.be.calledWith('xxx'); + }); + }); + + it('should get site by host header', () => { + const req = createReq({ + headerValues: { + host: 'xxx.net:9999', + }, + }); + const res = createRes(); + const siteResolver = new MockSiteResolver([]); + const middleware = new SampleMiddleware({ siteResolver }); + + expect(middleware['getSite'](req, res).hostName).to.equal('xxx.net'); + expect(siteResolver.getByHost).to.be.calledWith('xxx.net'); + }); + + it('should get site by default host', () => { + const req = createReq(); + const res = createRes(); + const siteResolver = new MockSiteResolver([]); + const middleware = new SampleMiddleware({ siteResolver }); + + expect(middleware['getSite'](req, res).hostName).to.equal('localhost'); + expect(siteResolver.getByHost).to.be.calledWith('localhost'); + }); + + it('should get site by custom default host', () => { + const req = createReq(); + const res = createRes(); + const siteResolver = new MockSiteResolver([]); + const middleware = new SampleMiddleware({ siteResolver, defaultHostname: 'yyy.net' }); + + expect(middleware['getSite'](req, res).hostName).to.equal('yyy.net'); + expect(siteResolver.getByHost).to.be.calledWith('yyy.net'); + }); }); diff --git a/packages/sitecore-jss-nextjs/src/middleware/middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/middleware.ts index c057e84920..4f7b71f2d1 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/middleware.ts @@ -1,6 +1,13 @@ -import { NextRequest } from 'next/server'; +import { SiteInfo, SiteResolver } from '@sitecore-jss/sitecore-jss/site'; +import { NextRequest, NextResponse } from 'next/server'; export type MiddlewareBaseConfig = { + /** + * function, determines if middleware should be turned off, based on cookie, header, or other considerations + * @param {NextRequest} [req] request object from middleware handler + * @param {NextResponse} [res] response object from middleware handler + */ + disabled?: (req?: NextRequest, res?: NextResponse) => boolean; /** * Function used to determine if route should be excluded. * By default, files (pathname.includes('.')), Next.js API routes (pathname.startsWith('/api/')), and Sitecore API routes (pathname.startsWith('/sitecore/')) are ignored. @@ -14,14 +21,18 @@ export type MiddlewareBaseConfig = { * @default localhost */ defaultHostname?: string; + /** + * Site resolution implementation by name/hostname + */ + siteResolver: SiteResolver; }; export abstract class MiddlewareBase { protected SITE_SYMBOL = 'sc_site'; protected defaultHostname: string; - constructor(protected config?: MiddlewareBaseConfig) { - this.defaultHostname = config?.defaultHostname || 'localhost'; + constructor(protected config: MiddlewareBaseConfig) { + this.defaultHostname = config.defaultHostname || 'localhost'; } /** @@ -73,4 +84,20 @@ export abstract class MiddlewareBase { protected getHostHeader(req: NextRequest) { return req.headers.get('host')?.split(':')[0]; } + + /** + * Get site information + * @param {NextRequest} req request + * @param {NextResponse} [res] response + * @returns {SiteInfo} site information + */ + protected getSite(req: NextRequest, res?: NextResponse): SiteInfo { + const siteNameCookie = res?.cookies.get(this.SITE_SYMBOL)?.value; + + if (siteNameCookie) return this.config.siteResolver.getByName(siteNameCookie); + + const hostname = this.getHostHeader(req) || this.defaultHostname; + + return this.config.siteResolver.getByHost(hostname); + } } diff --git a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.test.ts b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.test.ts index f5207e7636..dd5d4896ff 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.test.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.test.ts @@ -8,7 +8,7 @@ import nextjs, { NextRequest, NextResponse } from 'next/server'; import { debug } from '@sitecore-jss/sitecore-jss'; import { MultisiteMiddleware } from './multisite-middleware'; -import { SiteInfo } from '@sitecore-jss/sitecore-jss/site'; +import { SiteResolver } from '@sitecore-jss/sitecore-jss/site'; use(sinonChai); const expect = chai.use(chaiString).expect; @@ -88,17 +88,29 @@ describe('MultisiteMiddleware', () => { return res; }; - const createMiddleware = (props = {}) => { + const createMiddleware = (props: { [key: string]: any; siteResolver?: SiteResolver } = {}) => { + class MockSiteResolver extends SiteResolver { + getByName = sinon.stub().returns({ + name: siteName, + language: props.language || '', + hostName: props.hostName, + }); + + getByHost = sinon.stub().returns({ + name: siteName, + language: props.language || '', + hostName: props.hostName, + }); + } + + const siteResolver = props.siteResolver || new MockSiteResolver([]); + const middleware = new MultisiteMiddleware({ - getSite(hostName) { - return { name: siteName, hostName } as SiteInfo; - }, + siteResolver, ...props, }); - const getSite = spy(middleware['config'], 'getSite'); - - return { middleware, getSite }; + return { middleware, siteResolver }; }; // Stub for NextResponse generation, see https://github.com/vercel/next.js/issues/42374 @@ -174,7 +186,7 @@ describe('MultisiteMiddleware', () => { nextRewriteStub = sinon.stub(nextjs.NextResponse, 'rewrite').returns(res); - const { middleware, getSite } = createMiddleware({ + const { middleware, siteResolver } = createMiddleware({ defaultHostname: 'bar.net', }); @@ -199,7 +211,7 @@ describe('MultisiteMiddleware', () => { }, }); - expect(getSite).to.be.calledWith('bar.net'); + expect(siteResolver.getByHost).to.be.calledWith('bar.net'); expect(finalRes).to.deep.equal(res); @@ -218,7 +230,7 @@ describe('MultisiteMiddleware', () => { nextRewriteStub = sinon.stub(nextjs.NextResponse, 'rewrite').returns(res); - const { middleware, getSite } = createMiddleware(); + const { middleware, siteResolver } = createMiddleware(); const finalRes = await middleware.getHandler()(req, res); @@ -241,7 +253,7 @@ describe('MultisiteMiddleware', () => { }, }); - expect(getSite).to.be.calledWith('localhost'); + expect(siteResolver.getByHost).to.be.calledWith('localhost'); expect(finalRes).to.deep.equal(res); @@ -258,7 +270,7 @@ describe('MultisiteMiddleware', () => { nextRewriteStub = sinon.stub(nextjs.NextResponse, 'rewrite').returns(res); - const { middleware, getSite } = createMiddleware(); + const { middleware, siteResolver } = createMiddleware(); const finalRes = await middleware.getHandler()(req, res); @@ -279,7 +291,7 @@ describe('MultisiteMiddleware', () => { }, }); - expect(getSite).to.be.calledWith('foo.net'); + expect(siteResolver.getByHost).to.be.calledWith('foo.net'); expect(finalRes).to.deep.equal(res); @@ -296,7 +308,7 @@ describe('MultisiteMiddleware', () => { nextRewriteStub = sinon.stub(nextjs.NextResponse, 'rewrite').returns(res); - const { middleware, getSite } = createMiddleware({}); + const { middleware, siteResolver } = createMiddleware({}); const finalRes = await middleware.getHandler()(req); @@ -317,7 +329,7 @@ describe('MultisiteMiddleware', () => { }, }); - expect(getSite).to.be.calledWith('foo.net'); + expect(siteResolver.getByHost).to.be.calledWith('foo.net'); expect(finalRes).to.deep.equal(res); @@ -336,7 +348,7 @@ describe('MultisiteMiddleware', () => { nextRewriteStub = sinon.stub(nextjs.NextResponse, 'rewrite').returns(res); - const { middleware, getSite } = createMiddleware({ + const { middleware, siteResolver } = createMiddleware({ useCookieResolution: () => true, }); @@ -359,7 +371,8 @@ describe('MultisiteMiddleware', () => { }, }); - expect(getSite.notCalled).equal(true); + expect(siteResolver.getByHost).not.called.equal(true); + expect(siteResolver.getByName).not.called.equal(true); expect(finalRes).to.deep.equal(res); @@ -378,7 +391,7 @@ describe('MultisiteMiddleware', () => { nextRewriteStub = sinon.stub(nextjs.NextResponse, 'rewrite').returns(res); - const { middleware, getSite } = createMiddleware({ + const { middleware, siteResolver } = createMiddleware({ useCookieResolution: () => true, }); @@ -401,7 +414,8 @@ describe('MultisiteMiddleware', () => { }, }); - expect(getSite.notCalled).equal(true); + expect(siteResolver.getByHost).not.called.equal(true); + expect(siteResolver.getByName).not.called.equal(true); expect(finalRes).to.deep.equal(res); @@ -420,7 +434,7 @@ describe('MultisiteMiddleware', () => { nextRewriteStub = sinon.stub(nextjs.NextResponse, 'rewrite').returns(res); - const { middleware, getSite } = createMiddleware(); + const { middleware, siteResolver } = createMiddleware(); const finalRes = await middleware.getHandler()(req, res); @@ -441,7 +455,7 @@ describe('MultisiteMiddleware', () => { }, }); - expect(getSite).to.be.calledWith('foo.net'); + expect(siteResolver.getByHost).to.be.calledWith('foo.net'); expect(finalRes).to.deep.equal(res); @@ -473,10 +487,18 @@ describe('MultisiteMiddleware', () => { it('should handle error', async () => { const error = new Error('Custom error'); - const { middleware } = createMiddleware({ - getSite() { + class SampleSiteResolver extends SiteResolver { + constructor(sites) { + super(sites); + } + + getByHost = () => { throw error; - }, + }; + } + + const { middleware } = createMiddleware({ + siteResolver: new SampleSiteResolver([]), }); const finalRes = await middleware.getHandler()(req, res); diff --git a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts index edbdb122de..53fdc190bd 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts @@ -1,13 +1,9 @@ import { NextResponse, NextRequest } from 'next/server'; -import { getSiteRewrite, SiteInfo } from '@sitecore-jss/sitecore-jss/site'; +import { getSiteRewrite } from '@sitecore-jss/sitecore-jss/site'; import { debug } from '@sitecore-jss/sitecore-jss'; import { MiddlewareBase, MiddlewareBaseConfig } from './middleware'; -export type MultisiteMiddlewareConfig = MiddlewareBaseConfig & { - /** - * Function used to resolve site for given hostname - */ - getSite: (hostname: string) => SiteInfo; +export type MultisiteMiddlewareConfig = Omit & { /** * Function used to determine if site should be resolved from sc_site cookie when present */ @@ -22,7 +18,7 @@ export class MultisiteMiddleware extends MiddlewareBase { * @param {MultisiteMiddlewareConfig} [config] Multisite middleware config */ constructor(protected config: MultisiteMiddlewareConfig) { - super({ defaultHostname: config.defaultHostname, excludeRoute: config.excludeRoute }); + super(config); } /** @@ -68,7 +64,7 @@ export class MultisiteMiddleware extends MiddlewareBase { (this.config.useCookieResolution && this.config.useCookieResolution(req) && req.cookies.get(this.SITE_SYMBOL)?.value) || - this.config.getSite(hostname).name; + this.config.siteResolver.getByHost(hostname).name; // Rewrite to site specific path const rewritePath = getSiteRewrite(pathname, { diff --git a/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.test.ts b/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.test.ts index 92bf0c57db..229c06a85e 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.test.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.test.ts @@ -157,23 +157,23 @@ describe('PersonalizeMiddleware', () => { }; class MockSiteResolver extends SiteResolver { - getByName = sinon.stub().returns({ + getByName = sinon.stub().callsFake((siteName: string) => ({ name: siteName, language: props.language || '', hostName: hostname, pointOfSale: { [props.language || defaultLang]: pointOfSale, }, - }); + })); - getByHost = sinon.stub().returns({ + getByHost = sinon.stub().callsFake((hostName: string) => ({ name: siteName, language: props.language || '', - hostName: hostname, + hostName, pointOfSale: { [props.language || defaultLang]: pointOfSale, }, - }); + })); } const siteResolver: SiteResolver = props.siteResolver || new MockSiteResolver([]); diff --git a/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts index 15b920104b..d1d4e72300 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/personalize-middleware.ts @@ -7,7 +7,6 @@ import { ExperienceParams, getPersonalizedRewrite, } from '@sitecore-jss/sitecore-jss/personalize'; -import { SiteResolver } from '@sitecore-jss/sitecore-jss/site'; import { debug, NativeDataFetcher } from '@sitecore-jss/sitecore-jss'; import { MiddlewareBase, MiddlewareBaseConfig } from './middleware'; @@ -20,17 +19,6 @@ export type PersonalizeMiddlewareConfig = MiddlewareBaseConfig & { * Configuration for your Sitecore CDP endpoint */ cdpConfig: Omit; - /** - * function, determines if middleware should be turned off, based on cookie, header, or other considerations - * @param {NextRequest} [req] optional: request object from middleware handler - * @param {NextResponse} [res] optional: response object from middleware handler - * @returns {boolean} false by default - */ - disabled?: (req?: NextRequest, res?: NextResponse) => boolean; - /** - * Site resolution implementation by name/hostname - */ - siteResolver: SiteResolver; }; /** @@ -44,7 +32,7 @@ export class PersonalizeMiddleware extends MiddlewareBase { * @param {PersonalizeMiddlewareConfig} [config] Personalize middleware config */ constructor(protected config: PersonalizeMiddlewareConfig) { - super({ defaultHostname: config.defaultHostname, excludeRoute: config.excludeRoute }); + super(config); // NOTE: we provide native fetch for compatibility on Next.js Edge Runtime // (underlying default 'cross-fetch' is not currently compatible: https://github.com/lquixada/cross-fetch/issues/78) @@ -116,11 +104,8 @@ export class PersonalizeMiddleware extends MiddlewareBase { } private handler = async (req: NextRequest, res?: NextResponse): Promise => { - const hostHeader = this.getHostHeader(req); - const hostname = hostHeader || this.defaultHostname; const pathname = req.nextUrl.pathname; const language = this.getLanguage(req); - let siteName = res?.cookies.get(this.SITE_SYMBOL)?.value; let browserId = this.getBrowserId(req); debug.personalize('personalize middleware start: %o', { @@ -128,10 +113,6 @@ export class PersonalizeMiddleware extends MiddlewareBase { language, }); - if (!hostHeader) { - debug.personalize(`host header is missing, default ${hostname} is used`); - } - // Response will be provided if other middleware is run before us (e.g. redirects) let response = res || NextResponse.next(); @@ -152,20 +133,17 @@ export class PersonalizeMiddleware extends MiddlewareBase { return response; } - let site; - - if (!siteName) { - site = this.config.siteResolver.getByHost(hostname); - siteName = site.name; - } else { - site = this.config.siteResolver.getByName(siteName); + if (!this.getHostHeader(req)) { + debug.personalize(`host header is missing, default ${this.defaultHostname} is used`); } + const site = this.getSite(req, res); + // Get personalization info from Experience Edge const personalizeInfo = await this.personalizeService.getPersonalizeInfo( pathname, language, - siteName + site.name ); if (!personalizeInfo) { @@ -231,7 +209,7 @@ export class PersonalizeMiddleware extends MiddlewareBase { // Set browserId cookie on the response this.setBrowserId(response, browserId); // Share site name with the following executed middlewares - response.cookies.set(this.SITE_SYMBOL, siteName); + response.cookies.set(this.SITE_SYMBOL, site.name); debug.personalize('personalize middleware end: %o', { rewritePath, 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 027c1c17b8..dee707c8d0 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.test.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.test.ts @@ -1,4 +1,5 @@ -/* eslint-disable @typescript-eslint/no-empty-function */ +/* eslint-disable no-unused-expressions */ +/* eslint-disable @typescript-eslint/no-empty-function */ /* eslint-disable dot-notation */ import chai, { use } from 'chai'; import chaiString from 'chai-string'; @@ -6,7 +7,7 @@ import sinonChai from 'sinon-chai'; import sinon, { spy } from 'sinon'; import { NextRequest, NextResponse } from 'next/server'; import { debug } from '@sitecore-jss/sitecore-jss'; -import { REDIRECT_TYPE_SERVER_TRANSFER } from '@sitecore-jss/sitecore-jss/site'; +import { REDIRECT_TYPE_SERVER_TRANSFER, SiteResolver } from '@sitecore-jss/sitecore-jss/site'; import { RedirectsMiddleware } from './redirects-middleware'; use(sinonChai); @@ -70,18 +71,27 @@ describe('RedirectsMiddleware', () => { locale?: string; fetchRedirectsStub?: sinon.SinonStub; defaultHostname?: string; + siteResolver?: SiteResolver; } = {} ) => { - const getSite: any = - props.getSite || - sinon.stub().returns({ + class MockSiteResolver extends SiteResolver { + getByName = sinon.stub().callsFake((siteName: string) => ({ name: siteName, - language: '', + language: props.language || '', hostName: hostname, - }); + })); + + getByHost = sinon.stub().callsFake((hostName: string) => ({ + name: siteName, + language: props.language || '', + hostName, + })); + } + + const siteResolver = props.siteResolver || new MockSiteResolver([]); const middleware = new RedirectsMiddleware({ - getSite, + siteResolver, ...props, apiKey: 'edge-api-key', endpoint: 'http://edge-endpoint/api/graph/edge', @@ -102,7 +112,7 @@ describe('RedirectsMiddleware', () => { ) )); - return { middleware, fetchRedirects, getSite }; + return { middleware, fetchRedirects, siteResolver }; }; // Stub for NextResponse generation, see https://github.com/vercel/next.js/issues/42374 @@ -217,12 +227,12 @@ describe('RedirectsMiddleware', () => { }); const nextStub = sinon.stub(NextResponse, 'next').returns((res as unknown) as NextResponse); const req = createRequest(); - const { middleware, fetchRedirects, getSite } = createMiddleware(); + const { middleware, fetchRedirects, siteResolver } = createMiddleware(); const finalRes = await middleware.getHandler()(req); validateDebugLog('skipped (redirect does not exist)'); - expect(getSite).to.be.calledWith(hostname); + expect(siteResolver.getByHost).to.be.calledWith(hostname); // eslint-disable-next-line no-unused-expressions expect(fetchRedirects.called).to.be.true; expect(finalRes).to.deep.equal(res); @@ -255,7 +265,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: 'REDIRECT_301', @@ -265,7 +275,7 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req); - expect(getSite).to.be.calledWith(hostname); + expect(siteResolver.getByHost).to.be.calledWith(hostname); // eslint-disable-next-line no-unused-expressions expect(fetchRedirects.called).to.be.true; expect(finalRes).to.deep.equal(res); @@ -298,7 +308,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/ua/found', redirectType: REDIRECT_TYPE_SERVER_TRANSFER, @@ -308,7 +318,7 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req); - expect(getSite).to.be.calledWith(hostname); + expect(siteResolver.getByHost).to.be.calledWith(hostname); // eslint-disable-next-line no-unused-expressions expect(fetchRedirects.called).to.be.true; expect(finalRes).to.deep.equal(res); @@ -342,7 +352,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: '/found', redirectType: REDIRECT_TYPE_SERVER_TRANSFER, @@ -351,7 +361,7 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req); - expect(getSite).to.be.calledWith(hostname); + expect(siteResolver.getByHost).to.be.calledWith(hostname); // eslint-disable-next-line no-unused-expressions expect(fetchRedirects.called).to.be.true; expect(finalRes).to.deep.equal(res); @@ -385,7 +395,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: 'REDIRECT_302', @@ -395,7 +405,7 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req); - expect(getSite).to.be.calledWith(hostname); + expect(siteResolver.getByHost).to.be.calledWith(hostname); // eslint-disable-next-line no-unused-expressions expect(fetchRedirects.called).to.be.true; expect(finalRes).to.deep.equal(res); @@ -424,7 +434,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: 'default', @@ -434,7 +444,7 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req); - expect(getSite).to.be.calledWith(hostname); + expect(siteResolver.getByHost).to.be.calledWith(hostname); // eslint-disable-next-line no-unused-expressions expect(fetchRedirects.called).to.be.true; expect(finalRes).to.deep.equal(res); @@ -464,7 +474,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: REDIRECT_TYPE_SERVER_TRANSFER, @@ -474,7 +484,7 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req); - expect(getSite).to.be.calledWith(hostname); + expect(siteResolver.getByHost).to.be.calledWith(hostname); // eslint-disable-next-line no-unused-expressions expect(fetchRedirects.called).to.be.true; expect(finalRes).to.deep.equal(res); @@ -496,7 +506,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: 'REDIRECT_301', @@ -506,7 +516,8 @@ describe('RedirectsMiddleware', () => { const finalRes = await middleware.getHandler()(req, res); - expect(getSite).not.called.to.equal(true); + expect(siteResolver.getByHost).not.called.to.equal(true); + expect(siteResolver.getByName).to.be.calledWith(siteName); expect(fetchRedirects).to.be.calledWith(siteName); expect(finalRes).to.deep.equal(res); expect(finalRes.status).to.equal(res.status); @@ -526,7 +537,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: 'default', @@ -535,9 +546,8 @@ describe('RedirectsMiddleware', () => { }); const finalRes = await middleware.getHandler()(req, res); - // eslint-disable-next-line no-unused-expressions - expect(getSite).to.not.be.called; - // eslint-disable-next-line no-unused-expressions + expect(siteResolver.getByHost).to.not.be.called; + expect(siteResolver.getByName).to.be.calledWith(site); expect(fetchRedirects.called).to.be.true; expect(finalRes.cookies.get('sc_site')?.value).to.equal(site); }); @@ -556,7 +566,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: 'default', @@ -569,9 +579,8 @@ describe('RedirectsMiddleware', () => { validateDebugLog('skipped (redirects middleware is disabled)'); - // eslint-disable-next-line no-unused-expressions - expect(getSite).to.not.be.called; - // eslint-disable-next-line no-unused-expressions + expect(siteResolver.getByHost).to.not.be.called; + expect(siteResolver.getByName).to.not.be.called; expect(fetchRedirects.called).to.be.false; expect(finalRes.cookies.get('sc_site')?.value).to.equal(site); }); @@ -604,7 +613,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: 'REDIRECT_301', @@ -616,7 +625,7 @@ describe('RedirectsMiddleware', () => { validateDebugLog('host header is missing, default localhost is used'); - expect(getSite).to.be.calledWith('localhost'); + expect(siteResolver.getByHost).to.be.calledWith('localhost'); expect(fetchRedirects).to.be.calledWith(siteName); expect(finalRes).to.deep.equal(res); expect(finalRes.status).to.equal(res.status); @@ -652,7 +661,7 @@ describe('RedirectsMiddleware', () => { }, }); - const { middleware, fetchRedirects, getSite } = createMiddleware({ + const { middleware, fetchRedirects, siteResolver } = createMiddleware({ pattern: 'not-found', target: 'http://localhost:3000/found', redirectType: 'REDIRECT_301', @@ -665,7 +674,7 @@ describe('RedirectsMiddleware', () => { validateDebugLog('host header is missing, default foobar is used'); - expect(getSite).to.be.calledWith('foobar'); + expect(siteResolver.getByHost).to.be.calledWith('foobar'); expect(fetchRedirects).to.be.calledWith(siteName); expect(finalRes).to.deep.equal(res); expect(finalRes.status).to.equal(res.status); diff --git a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts index c1b1989a56..86d3a4458a 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts @@ -18,17 +18,6 @@ import { MiddlewareBase, MiddlewareBaseConfig } from './middleware'; export type RedirectsMiddlewareConfig = Omit & MiddlewareBaseConfig & { locales: string[]; - /** - * function, determines if middleware should be turned off, based on cookie, header, or other considerations - * @param {NextRequest} [req] optional: request object from middleware handler - * @param {NextResponse} [res] optional: response object from middleware handler - * @returns {boolean} false by default - */ - disabled?: (req?: NextRequest, res?: NextResponse) => boolean; - /** - * function used to resolve site for given hostname - */ - getSite: (hostname: string) => SiteInfo; }; /** * Middleware / handler fetches all redirects from Sitecore instance by grapqhl service @@ -42,7 +31,7 @@ export class RedirectsMiddleware extends MiddlewareBase { * @param {RedirectsMiddlewareConfig} [config] redirects middleware config */ constructor(protected config: RedirectsMiddlewareConfig) { - super({ defaultHostname: config.defaultHostname, excludeRoute: config.excludeRoute }); + super(config); // NOTE: we provide native fetch for compatibility on Next.js Edge Runtime // (underlying default 'cross-fetch' is not currently compatible: https://github.com/lquixada/cross-fetch/issues/78) @@ -59,14 +48,11 @@ export class RedirectsMiddleware extends MiddlewareBase { } private handler = async (req: NextRequest, res?: NextResponse): Promise => { - const hostHeader = this.getHostHeader(req); - const hostname = hostHeader || this.defaultHostname; - const siteName = - res?.cookies.get(this.SITE_SYMBOL)?.value || this.config.getSite(hostname).name; + let site: SiteInfo | undefined; const createResponse = async () => { - if (!hostHeader) { - debug.redirects(`host header is missing, default ${hostname} is used`); + if (!this.getHostHeader(req)) { + debug.redirects(`host header is missing, default ${this.defaultHostname} is used`); } if (this.config.disabled && this.config.disabled(req, NextResponse.next())) { @@ -79,8 +65,11 @@ export class RedirectsMiddleware extends MiddlewareBase { return res || NextResponse.next(); } + + site = this.getSite(req, res); + // Find the redirect from result of RedirectService - const existsRedirect = await this.getExistsRedirect(req, siteName); + const existsRedirect = await this.getExistsRedirect(req, site.name); if (!existsRedirect) { debug.redirects('skipped (redirect does not exist)'); @@ -122,7 +111,8 @@ export class RedirectsMiddleware extends MiddlewareBase { const response = await createResponse(); // Share site name with the following executed middlewares - response.cookies.set(this.SITE_SYMBOL, siteName); + // Don't need to set when middleware is disabled + site && response.cookies.set(this.SITE_SYMBOL, site.name); return response; }; From 111a7b867c216428e839f2c8fc370865de7c3d8c Mon Sep 17 00:00:00 2001 From: illiakovalenko Date: Thu, 9 Mar 2023 17:52:48 +0200 Subject: [PATCH 3/3] Changes according to review comments --- .../src/middleware/middleware.ts | 3 +- .../middleware/multisite-middleware.test.ts | 38 ++++++++++++++++++- .../src/middleware/multisite-middleware.ts | 5 ++- .../src/middleware/redirects-middleware.ts | 4 ++ 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/packages/sitecore-jss-nextjs/src/middleware/middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/middleware.ts index 4f7b71f2d1..faf0c91c48 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/middleware.ts @@ -86,7 +86,8 @@ export abstract class MiddlewareBase { } /** - * Get site information + * Get site information. + * Can not be used in **Preview** mode, since site will not be resolved * @param {NextRequest} req request * @param {NextResponse} [res] response * @returns {SiteInfo} site information diff --git a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.test.ts b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.test.ts index dd5d4896ff..60ff705640 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.test.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.test.ts @@ -138,7 +138,7 @@ describe('MultisiteMiddleware', () => { hostname: 'foo.net', }); - validateDebugLog('skipped (route excluded)'); + validateDebugLog('skipped (%s)', 'route excluded'); expect(finalRes).to.deep.equal(res); @@ -168,6 +168,42 @@ describe('MultisiteMiddleware', () => { await test('/crazypath/luna', middleware); }); }); + + describe('preview', () => { + it('prerender bypass cookie is present', async () => { + const { middleware } = createMiddleware(); + const res = NextResponse.next(); + + const req = createRequest({ + cookieValues: { + __prerender_bypass: true, + }, + }); + + const finalRes = await middleware.getHandler()(req, res); + + validateDebugLog('skipped (%s)', 'preview'); + + expect(finalRes).to.deep.equal(res); + }); + + it('preview data cookie is present', async () => { + const { middleware } = createMiddleware(); + const res = NextResponse.next(); + + const req = createRequest({ + cookieValues: { + __next_preview_data: true, + }, + }); + + const finalRes = await middleware.getHandler()(req, res); + + validateDebugLog('skipped (%s)', 'preview'); + + expect(finalRes).to.deep.equal(res); + }); + }); }); describe('request passed', () => { diff --git a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts index 53fdc190bd..c4020c6af1 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/multisite-middleware.ts @@ -53,8 +53,9 @@ export class MultisiteMiddleware extends MiddlewareBase { // Response will be provided if other middleware is run before us let response = res || NextResponse.next(); - if (this.excludeRoute(pathname)) { - debug.multisite('skipped (route excluded)'); + if (this.isPreview(req) || this.excludeRoute(pathname)) { + debug.multisite('skipped (%s)', this.isPreview(req) ? 'preview' : 'route excluded'); + return response; } diff --git a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts index 86d3a4458a..1de89e4f5c 100644 --- a/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts +++ b/packages/sitecore-jss-nextjs/src/middleware/redirects-middleware.ts @@ -17,6 +17,10 @@ import { MiddlewareBase, MiddlewareBaseConfig } from './middleware'; */ export type RedirectsMiddlewareConfig = Omit & MiddlewareBaseConfig & { + /** + * These are all the locales you support in your application. + * These should match those in your next.config.js (i18n.locales). + */ locales: string[]; }; /**