feat: redirects to content client

adobe · Sep 16, 2024 · af06daa · af06daa
1 parent fd515cd
commit af06daa
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 17 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/spacecat-shared-content-client/package.json b/packages/spacecat-shared-content-client/package.json
@@ -35,7 +35,7 @@
   },
   "dependencies": {
     "@adobe/helix-universal": "5.0.5",
-    "@adobe/spacecat-helix-content-sdk": "1.1.7",
+    "@adobe/spacecat-helix-content-sdk": "1.1.8",
     "@adobe/spacecat-shared-utils": "1.19.6",
     "graph-data-structure": "4.0.0"
   },

diff --git a/packages/spacecat-shared-content-client/src/clients/content-client.js b/packages/spacecat-shared-content-client/src/clients/content-client.js
@@ -134,7 +134,7 @@ const validateRedirects = (redirects) => {
   }
 };
 
-const removeDuplicatedRedirects = (currentRedirects, newRedirects) => {
+const removeDuplicatedRedirects = (currentRedirects, newRedirects, log) => {
   const redirectsSet = new Set();
   currentRedirects.map(({ from, to }) => `${from}:${to}`)
     .forEach((redirectRule) => redirectsSet.add(redirectRule));
@@ -146,18 +146,30 @@ const removeDuplicatedRedirects = (currentRedirects, newRedirects) => {
     if (!redirectsSet.has(strRedirectRule)) {
       redirectsSet.add(strRedirectRule);
       newRedirectsClean.push(redirectRule);
-    } // TODO: report duplicated entry??
+    } else {
+      log.info(`Duplicate redirect rule detected: ${strRedirectRule}`);
+    }
   });
   return newRedirectsClean;
 };
 
-const detectRedirectLoops = (currentRedirects, newRedirects) => {
+const removeRedirectLoops = (currentRedirects, newRedirects, log) => {
   const redirectsGraph = new Graph();
-  [...currentRedirects, ...newRedirects].forEach((r) => redirectsGraph.addEdge(r.from, r.to));
+  const noCycleRedirects = [];
+  currentRedirects.forEach((r) => redirectsGraph.addEdge(r.from, r.to));
   if (hasCycle(redirectsGraph)) {
-    // TODO: report cycle path??
     throw new Error('Redirect cycle detected');
   }
+  newRedirects.forEach((r) => {
+    redirectsGraph.addEdge(r.from, r.to);
+    if (hasCycle(redirectsGraph)) {
+      log.info(`Redirect loop detected: ${r.from} -> ${r.to}`);
+      redirectsGraph.removeEdge(r.from, r.to);
+    } else {
+      noCycleRedirects.push(r);
+    }
+  });
+  return noCycleRedirects;
 };
 
 export default class ContentClient {
@@ -282,10 +294,21 @@ export default class ContentClient {
     const currentRedirects = await this.getRedirects();
 
     // validate combination of existing and new redirects
-    const cleanNewRedirects = removeDuplicatedRedirects(currentRedirects, redirects);
-    detectRedirectLoops(currentRedirects, cleanNewRedirects);
+    const cleanNewRedirects = removeDuplicatedRedirects(currentRedirects, redirects, this.log);
+    if (cleanNewRedirects.length === 0) {
+      this.log.info('No valid redirects to update');
+      return;
+    }
+    const noCycleRedirects = removeRedirectLoops(currentRedirects, cleanNewRedirects, this.log);
+    if (noCycleRedirects.length === 0) {
+      this.log.info('No valid redirects to update');
+      return;
+    }
+    if (noCycleRedirects.length !== cleanNewRedirects.length) {
+      this.log.info(`Removed ${cleanNewRedirects.length - noCycleRedirects.length} redirect loops`);
+    }
 
-    const response = await this.rawClient.appendRedirects(cleanNewRedirects);
+    const response = await this.rawClient.appendRedirects(noCycleRedirects);
     if (response.status !== 200) {
       throw new Error('Failed to update redirects');
     }

diff --git a/packages/spacecat-shared-content-client/test/clients/content-client.test.js b/packages/spacecat-shared-content-client/test/clients/content-client.test.js
@@ -16,8 +16,10 @@ import { expect, use } from 'chai';
 import chaiAsPromised from 'chai-as-promised';
 import esmock from 'esmock';
 import sinon from 'sinon';
+import sinonChai from 'sinon-chai';
 
 use(chaiAsPromised);
+use(sinonChai);
 
 describe('ContentClient', () => {
   let sandbox;
@@ -394,13 +396,37 @@ describe('ContentClient', () => {
       await client.updateRedirects(newRedirects);
       await expect(client.rawClient.appendRedirects.calledOnceWith(sinon.match([{ from: '/test-X', to: '/test-Y' }]))).to.be.true;
     });
-    it('detect cycles', async () => {
+    it('detect cycles in new redirects', async () => {
       const newRedirects = [
         { from: '/test-D', to: '/test-A' },
+        { from: '/test-C', to: '/test-E' },
       ];
       ContentClient = await createContentClientForRedirects(existingRedirects);
       const client = ContentClient.createFrom(context, siteConfigGoogleDrive);
+      await client.updateRedirects(newRedirects);
+      await expect(client.rawClient.appendRedirects.calledOnceWith(sinon.match([{ from: '/test-C', to: '/test-E' }]))).to.be.true;
+    });
+    it('detect cycles in current redirects', async () => {
+      const newRedirects = [
+        { from: '/test-I', to: '/test-J' },
+      ];
+      const cycleRedirects = [
+        { from: '/test-A', to: '/test-C' },
+        { from: '/test-C', to: '/test-E' },
+        { from: '/test-E', to: '/test-A' },
+      ];
+      ContentClient = await createContentClientForRedirects(cycleRedirects);
+      const client = ContentClient.createFrom(context, siteConfigGoogleDrive);
       await expect(client.updateRedirects(newRedirects)).to.be.rejectedWith('Redirect cycle detected');
     });
+    it('does not call rawClient when there are no valid redirects', async () => {
+      const newRedirects = [
+        { from: '/test-D', to: '/test-A' },
+      ];
+      ContentClient = await createContentClientForRedirects(existingRedirects);
+      const client = ContentClient.createFrom(context, siteConfigGoogleDrive);
+      await client.updateRedirects(newRedirects);
+      await expect(client.rawClient.appendRedirects).to.not.have.been.called;
+    });
   });
 });