From 0ccf31cdb5ccecf46f5b9f957cc1aaf444598032 Mon Sep 17 00:00:00 2001 From: Naz Date: Wed, 3 Nov 2021 13:45:43 +0400 Subject: [PATCH] Fixed error message when booting with no redirects refs https://github.com/TryGhost/Ghost/commit/91efa4605cad20c808ccc792a2893d4adc463934 - When the instance is booted without any redirects files configured it's not supposed to error but rather default to an "empty" [] redirects configuration. - Ideally the logic shoudl not contain try/catch block at all and fail as soon as there's any error during the initialization. This wasn't changed at this time due to possible break of existing Ghost instances --- core/server/services/redirects/api.js | 20 ++++++++++++------- .../server/services/redirects/api.test.js | 20 +++++++++++++++++++ 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/core/server/services/redirects/api.js b/core/server/services/redirects/api.js index 727942c607..94588fc73b 100644 --- a/core/server/services/redirects/api.js +++ b/core/server/services/redirects/api.js @@ -140,16 +140,22 @@ class CustomRedirectsAPI { } async init() { + // NOTE: the try/catch block here is due to possible breaking change for existing misconfigured + // instances in the wild. Would be a good idea to remove it during v5 migration to enforce + // fail-fast initialization. try { const filePath = await this.getRedirectsFilePath(); - const content = await readRedirectsFile(filePath); - const ext = path.extname(filePath); - const redirects = parseRedirectsFile(content, ext); - validation.validate(redirects); - this.redirectManager.removeAllRedirects(); - for (const redirect of redirects) { - this.redirectManager.addRedirect(redirect.from, redirect.to, {permanent: redirect.permanent}); + if (filePath !== null) { + const content = await readRedirectsFile(filePath); + const ext = path.extname(filePath); + const redirects = parseRedirectsFile(content, ext); + validation.validate(redirects); + + this.redirectManager.removeAllRedirects(); + for (const redirect of redirects) { + this.redirectManager.addRedirect(redirect.from, redirect.to, {permanent: redirect.permanent}); + } } } catch (err) { if (errors.utils.isIgnitionError(err)) { diff --git a/test/unit/server/services/redirects/api.test.js b/test/unit/server/services/redirects/api.test.js index a9a9eb5745..d104c5d57a 100644 --- a/test/unit/server/services/redirects/api.test.js +++ b/test/unit/server/services/redirects/api.test.js @@ -3,6 +3,7 @@ const sinon = require('sinon'); const path = require('path'); const fs = require('fs-extra'); +const logging = require('@tryghost/logging'); const DynamicRedirectManager = require('@tryghost/express-dynamic-redirects'); const CustomRedirectsAPI = require('../../../../../core/server/services/redirects/api'); @@ -26,12 +27,31 @@ describe('UNIT: redirects CustomRedirectsAPI class', function () { beforeEach(function () { sinon.stub(fs, 'pathExists'); sinon.stub(fs, 'readFile'); + sinon.spy(logging, 'error'); }); afterEach(function () { sinon.restore(); }); + describe('init', function () { + it('initializes without errors when redirects file is not present', async function () { + const redirectManager = new DynamicRedirectManager({ + permanentMaxAge: 100, + getSubdirectoryURL: (pathname) => { + return `/ghost/${pathname}`; + } + }); + + customRedirectsAPI = new CustomRedirectsAPI({ + basePath + }, redirectManager); + + await customRedirectsAPI.init(); + logging.error.called.should.be.false(); + }); + }); + describe('get', function () { it('returns empty array if file does not exist', async function () { fs.pathExists.withArgs(`${basePath}redirects.yaml`).resolves(false);