mirror of
https://github.com/TryGhost/Ghost.git
synced 2025-04-08 02:52:39 -05:00
Made custom-redirects middleware testable
refs https://linear.app/tryghost/issue/CORE-35/refactor-route-and-redirect-settings
refs 7528ec8c3b
- The way the custom redirects middleware was organized made it extremely hard to unit test it (had to stub the redirects service methods etc). With a new organization it's possible to provide needed redirects configs to the method which makes the actual redirects Router logic testable and the code less coupled with redirects services
- This was meant to be an attempt to extract more of the slow redirects regression tests, which failed. Instead found this weak spot that could be improved and gained:
- shaved 4s of time as two slow regression test cases are now gone
- there's now a base to build upon when getting more coverage for the custom redirects middleware
This commit is contained in:
Naz
parent
0d5b836557
commit
0962b3ed45
4 changed files with 126 additions and 97 deletions
|
|
@ -17,77 +17,90 @@ const _private = {};
|
|||
|
||||
let customRedirectsRouter;
|
||||
|
||||
_private.registerRoutes = () => {
|
||||
/**
|
||||
*
|
||||
* @param {Object} redirects valid redirects JSON
|
||||
* @returns {Object} instance of express.Router express router handling redirects based on config
|
||||
*/
|
||||
_private.registerRoutes = (redirects) => {
|
||||
debug('redirects loading');
|
||||
|
||||
customRedirectsRouter = express.Router('redirects');
|
||||
const redirectsRouter = express.Router('redirects');
|
||||
|
||||
try {
|
||||
const redirects = redirectsService.loadRedirectsFile();
|
||||
if (labsService.isSet('offers')) {
|
||||
redirects.unshift({
|
||||
from: '/zimo50',
|
||||
to: '/#/portal/offers/abcdefuckoff'
|
||||
});
|
||||
}
|
||||
|
||||
if (labsService.isSet('offers')) {
|
||||
redirects.unshift({
|
||||
from: '/zimo50',
|
||||
to: '/#/portal/offers/abcdefuckoff'
|
||||
});
|
||||
redirects.forEach((redirect) => {
|
||||
/**
|
||||
* Detect case insensitive modifier when regex is enclosed by
|
||||
* / ... /i
|
||||
*/
|
||||
let options = '';
|
||||
if (redirect.from.match(/^\/.*\/i$/)) {
|
||||
redirect.from = redirect.from.slice(1, -2);
|
||||
options = 'i';
|
||||
}
|
||||
|
||||
/**
|
||||
* always delete trailing slashes, doesn't matter if regex or not
|
||||
* Example:
|
||||
* - you define /my-blog-post-1/ as from property
|
||||
* - /my-blog-post-1 or /my-blog-post-1/ should work
|
||||
*/
|
||||
|
||||
if (redirect.from.match(/\/$/)) {
|
||||
redirect.from = redirect.from.slice(0, -1);
|
||||
}
|
||||
|
||||
if (redirect.from[redirect.from.length - 1] !== '$') {
|
||||
redirect.from += '/?$';
|
||||
}
|
||||
|
||||
debug('register', redirect.from);
|
||||
redirectsRouter.get(new RegExp(redirect.from, options), function (req, res) {
|
||||
const maxAge = redirect.permanent ? config.get('caching:customRedirects:maxAge') : 0;
|
||||
const toURL = url.parse(redirect.to);
|
||||
const toURLParams = querystring.parse(toURL.query);
|
||||
const currentURL = url.parse(req.url);
|
||||
const currentURLParams = querystring.parse(currentURL.query);
|
||||
const params = Object.assign({}, currentURLParams, toURLParams);
|
||||
const search = querystring.stringify(params);
|
||||
|
||||
toURL.pathname = currentURL.pathname.replace(new RegExp(redirect.from, options), toURL.pathname);
|
||||
toURL.search = search !== '' ? `?${search}` : null;
|
||||
|
||||
/**
|
||||
* Only if the URL is internal should we prepend the Ghost subdirectory
|
||||
* @see https://github.com/TryGhost/Ghost/issues/10776
|
||||
*/
|
||||
if (!toURL.hostname) {
|
||||
toURL.pathname = urlUtils.urlJoin(urlUtils.getSubdir(), toURL.pathname);
|
||||
}
|
||||
|
||||
res.set({
|
||||
'Cache-Control': `public, max-age=${maxAge}`
|
||||
});
|
||||
|
||||
res.redirect(redirect.permanent ? 301 : 302, url.format(toURL));
|
||||
});
|
||||
});
|
||||
|
||||
debug('redirects loaded');
|
||||
|
||||
return redirectsRouter;
|
||||
};
|
||||
|
||||
const loadRoutes = () => {
|
||||
try {
|
||||
const redirects = redirectsService.loadRedirectsFile();
|
||||
redirectsService.validate(redirects);
|
||||
|
||||
redirects.forEach((redirect) => {
|
||||
/**
|
||||
* Detect case insensitive modifier when regex is enclosed by
|
||||
* / ... /i
|
||||
*/
|
||||
let options = '';
|
||||
if (redirect.from.match(/^\/.*\/i$/)) {
|
||||
redirect.from = redirect.from.slice(1, -2);
|
||||
options = 'i';
|
||||
}
|
||||
|
||||
/**
|
||||
* always delete trailing slashes, doesn't matter if regex or not
|
||||
* Example:
|
||||
* - you define /my-blog-post-1/ as from property
|
||||
* - /my-blog-post-1 or /my-blog-post-1/ should work
|
||||
*/
|
||||
|
||||
if (redirect.from.match(/\/$/)) {
|
||||
redirect.from = redirect.from.slice(0, -1);
|
||||
}
|
||||
|
||||
if (redirect.from[redirect.from.length - 1] !== '$') {
|
||||
redirect.from += '/?$';
|
||||
}
|
||||
|
||||
debug('register', redirect.from);
|
||||
customRedirectsRouter.get(new RegExp(redirect.from, options), function (req, res) {
|
||||
const maxAge = redirect.permanent ? config.get('caching:customRedirects:maxAge') : 0;
|
||||
const toURL = url.parse(redirect.to);
|
||||
const toURLParams = querystring.parse(toURL.query);
|
||||
const currentURL = url.parse(req.url);
|
||||
const currentURLParams = querystring.parse(currentURL.query);
|
||||
const params = Object.assign({}, currentURLParams, toURLParams);
|
||||
const search = querystring.stringify(params);
|
||||
|
||||
toURL.pathname = currentURL.pathname.replace(new RegExp(redirect.from, options), toURL.pathname);
|
||||
toURL.search = search !== '' ? `?${search}` : null;
|
||||
|
||||
/**
|
||||
* Only if the URL is internal should we prepend the Ghost subdirectory
|
||||
* @see https://github.com/TryGhost/Ghost/issues/10776
|
||||
*/
|
||||
if (!toURL.hostname) {
|
||||
toURL.pathname = urlUtils.urlJoin(urlUtils.getSubdir(), toURL.pathname);
|
||||
}
|
||||
|
||||
res.set({
|
||||
'Cache-Control': `public, max-age=${maxAge}`
|
||||
});
|
||||
|
||||
res.redirect(redirect.permanent ? 301 : 302, url.format(toURL));
|
||||
});
|
||||
});
|
||||
const redirectsRouter = _private.registerRoutes(redirects);
|
||||
customRedirectsRouter = redirectsRouter;
|
||||
} catch (err) {
|
||||
if (errors.utils.isIgnitionError(err)) {
|
||||
logging.error(err);
|
||||
|
|
@ -100,8 +113,6 @@ _private.registerRoutes = () => {
|
|||
}));
|
||||
}
|
||||
}
|
||||
|
||||
debug('redirects loaded');
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
@ -110,7 +121,7 @@ _private.registerRoutes = () => {
|
|||
* - file loads synchronously, because we need to register the routes before anything else
|
||||
*/
|
||||
exports.use = function use(siteApp) {
|
||||
_private.registerRoutes();
|
||||
loadRoutes();
|
||||
|
||||
// Recommended approach by express, see https://github.com/expressjs/express/issues/2596#issuecomment-81353034.
|
||||
// As soon as the express router get's re-instantiated, the old router instance is not used anymore.
|
||||
|
|
@ -120,5 +131,5 @@ exports.use = function use(siteApp) {
|
|||
};
|
||||
|
||||
exports.reload = function reload() {
|
||||
_private.registerRoutes();
|
||||
loadRoutes();
|
||||
};
|
||||
|
|
|
|||
|
|
@ -383,21 +383,6 @@ describe('Redirects API', function () {
|
|||
});
|
||||
});
|
||||
|
||||
// https://github.com/TryGhost/Ghost/issues/10898
|
||||
describe('Merge querystring', function () {
|
||||
it('toURL param takes precedence, other params pass through', function () {
|
||||
return startGhost({forceStart: true, redirectsFileExt: '.json'})
|
||||
.then(function () {
|
||||
return request
|
||||
.get('/test-params/?q=123&lang=js')
|
||||
.expect(301)
|
||||
.then(function (res) {
|
||||
res.headers.location.should.eql('/result?q=abc&lang=js');
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// TODO: For backward compatibility, we only check if download, upload endpoints work here.
|
||||
// when updating to v4, the old endpoints should be updated to the new ones.
|
||||
// And the tests below should be removed.
|
||||
|
|
|
|||
|
|
@ -383,21 +383,6 @@ describe('Redirects API', function () {
|
|||
});
|
||||
});
|
||||
|
||||
// https://github.com/TryGhost/Ghost/issues/10898
|
||||
describe('Merge querystring', function () {
|
||||
it('toURL param takes precedence, other params pass through', function () {
|
||||
return startGhost({forceStart: true, redirectsFileExt: '.json'})
|
||||
.then(function () {
|
||||
return request
|
||||
.get('/test-params/?q=123&lang=js')
|
||||
.expect(301)
|
||||
.then(function (res) {
|
||||
res.headers.location.should.eql('/result?q=abc&lang=js');
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// TODO: For backward compatibility, we only check if download, upload endpoints work here.
|
||||
// when updating to v4, the old endpoints should be updated to the new ones.
|
||||
// And the tests below should be removed.
|
||||
|
|
|
|||
48
test/unit/web/shared/middleware/custom-redirects.test.js
Normal file
48
test/unit/web/shared/middleware/custom-redirects.test.js
Normal file
|
|
@ -0,0 +1,48 @@
|
|||
const should = require('should');
|
||||
const sinon = require('sinon');
|
||||
const rewire = require('rewire');
|
||||
|
||||
const customRedirects = rewire('../../../../../core/server/web/shared/middlewares/custom-redirects');
|
||||
const registerRoutes = customRedirects.__get__('_private.registerRoutes');
|
||||
|
||||
describe('UNIT: custom redirects', function () {
|
||||
let res;
|
||||
let req;
|
||||
let next;
|
||||
|
||||
beforeEach(function () {
|
||||
req = {
|
||||
method: 'GET'
|
||||
};
|
||||
res = {
|
||||
redirect: sinon.spy(),
|
||||
set: sinon.spy()
|
||||
};
|
||||
|
||||
next = sinon.spy();
|
||||
});
|
||||
|
||||
afterEach(function () {
|
||||
sinon.restore();
|
||||
});
|
||||
|
||||
// Related to: https://github.com/TryGhost/Ghost/issues/10898
|
||||
it('toURL param takes precedence, other params pass through', function () {
|
||||
const redirectsConfig = [{
|
||||
permanent: true,
|
||||
from: '/test-params',
|
||||
to: '/result?q=abc'
|
||||
}];
|
||||
const redirect = registerRoutes(redirectsConfig);
|
||||
|
||||
req.url = '/test-params/?q=123&lang=js';
|
||||
redirect(req, res, next);
|
||||
|
||||
next.called.should.be.false();
|
||||
res.redirect.called.should.be.true();
|
||||
res.redirect.calledWith(301, '/result?q=abc&lang=js').should.be.true();
|
||||
res.set.calledWith({
|
||||
'Cache-Control': `public, max-age=0`
|
||||
});
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue