From 9ec3fd20b0392a705ae86382fbd56b9bd75063f7 Mon Sep 17 00:00:00 2001 From: Thomas Boutell Date: Tue, 5 Mar 2024 15:56:39 -0500 Subject: [PATCH 1/4] PRO-5712: add migration for targetLocale --- index.js | 49 +++++++++++++++++++++++++++++++++++++++++-------- test/index.js | 5 +++-- 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/index.js b/index.js index 8665166..b6cfa2d 100644 --- a/index.js +++ b/index.js @@ -36,8 +36,8 @@ module.exports = { }, setCurrentLocale(req, doc) { const internalPage = doc._newPage && doc._newPage[0]; - - doc.targetLocale = internalPage && doc.urlType === 'internal' + // Watch out for unlocalized types and missing documents + doc.targetLocale = (doc.urlType === 'internal' && internalPage?.aposLocale) ? internalPage.aposLocale.replace(/:.*$/, '') : null; } @@ -197,15 +197,24 @@ module.exports = { } const foundTarget = results.find(({ redirectSlug }) => redirectSlug === slug) || - results.find(({ - redirectSlug, - ignoreQueryString - }) => redirectSlug === pathOnly && ignoreQueryString); + results.find(({ + redirectSlug, + ignoreQueryString + }) => redirectSlug === pathOnly && ignoreQueryString); + + if (!foundTarget) { + // Query will produce a match if the path matches, but we need + // to implement ignoreQueryString: false properly + return await emitAndRedirectOrNext(); + } const shouldForwardQueryString = foundTarget && foundTarget.forwardQueryString; - const localizedReq = foundTarget.urlType === 'internal' && - req.locale !== foundTarget.targetLocale + const localizedReq = ( + (foundTarget.urlType === 'internal') && + Object.keys(self.apos.i18n.locales).includes(foundTarget.targetLocale) && + (req.locale !== foundTarget.targetLocale) + ) ? req.clone({ locale: foundTarget.targetLocale }) : req; @@ -299,6 +308,30 @@ module.exports = { } }); }, + + addTargetLocaleMigration() { + self.apos.migration.add('@apostrophecms/redirect:addTargetLocale', async () => { + await self.apos.migration.eachDoc({ + type: self.__meta.name, + urlType: 'internal', + targetLocale: { + $exists: 0 + } + }, async redirect => { + const target = await self.apos.doc.db.findOne({ + _id: redirect.newPageIds[0] + }); + await self.apos.doc.db.updateOne({ + _id: redirect._id + }, { + $set: { + targetLocale: target.aposLocale.replace(/:.*$/, '') + } + }); + }); + }); + }, + createIndexes() { self.apos.doc.db.createIndex({ redirectSlug: 1 }); } diff --git a/test/index.js b/test/index.js index 4db947e..43a492e 100644 --- a/test/index.js +++ b/test/index.js @@ -62,17 +62,18 @@ describe('@apostrophecms/redirect', function () { const reqFr = apos.task.getReq({ locale: 'fr' }); const instance = redirectModule.newInstance(); const pageFr = await apos.page.find(reqFr, { title: 'page fr' }).toObject(); - await redirectModule.insert(req, { + const inserted = await redirectModule.insert(req, { ...instance, title: 'internal redirect', urlType: 'internal', redirectSlug: '/page-1', _newPage: [ pageFr ] }); - + assert.strictEqual(inserted.targetLocale, 'fr'); const redirected = await apos.http.get('http://localhost:3000/page-1'); assert.equal(redirected, 'page fr\n'); }); + }); async function insertPages(apos) { From 8343bdc570662c40d49fa8e977239a544012bc2e Mon Sep 17 00:00:00 2001 From: Thomas Boutell Date: Tue, 5 Mar 2024 15:58:45 -0500 Subject: [PATCH 2/4] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5b8891..b7e85c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## UNRELEASED + +* Bug fix to properly migrate older redirects missing a `targetLocale` property, and to tolerate situations where this property is irrelevant or makes reference to a locale that no longer exists in the system. + ## 1.4.0 (2024-02-23) Several fixes and improvements contributed by Stéphane Maccari of Michelin: From e92ddc3ce2ea2cd1049b64bcf9467e359c4e7d6e Mon Sep 17 00:00:00 2001 From: Thomas Boutell Date: Wed, 6 Mar 2024 09:26:21 -0500 Subject: [PATCH 3/4] migrations work better when you actually register them --- index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/index.js b/index.js index b6cfa2d..c414455 100644 --- a/index.js +++ b/index.js @@ -22,6 +22,7 @@ module.exports = { }, init(self) { self.addUnlocalizedMigration(); + self.addTargetLocaleMigration(); self.createIndexes(); }, handlers(self) { From 23575ceabd7447624148c363d54cfcdd4337b130 Mon Sep 17 00:00:00 2001 From: Thomas Boutell Date: Wed, 6 Mar 2024 09:32:25 -0500 Subject: [PATCH 4/4] fix the migration --- index.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index c414455..d1ed1ac 100644 --- a/index.js +++ b/index.js @@ -319,14 +319,17 @@ module.exports = { $exists: 0 } }, async redirect => { - const target = await self.apos.doc.db.findOne({ - _id: redirect.newPageIds[0] - }); await self.apos.doc.db.updateOne({ _id: redirect._id }, { $set: { - targetLocale: target.aposLocale.replace(/:.*$/, '') + // It is in the nature of the original bug that we can't tell exactly + // what locale this should have been (relationships use aposDocId which + // is cross-locale and we were not saving any information about the + // target locale). However the default locale is the most + // likely to be useful and prevents a crash, and if it is not useful + // the user can just edit or remove the redirect + targetLocale: self.apos.i18n.defaultLocale } }); });