Update multi-site redirect caching

codeception.yml

@@ -1,6 +1,7 @@
 actor: Tester
 paths:
   tests: tests
+  output: tests/_output
   log: tests/_output
   data: tests/_data
   support: tests/_support

src/services/Redirects.php

@@ -329,27 +329,36 @@ public function findRedirectMatch(string $fullUrl, string $pathOnly, $siteId = n
                 $siteId = $primarySite->id;
             }
         }
+        $siteId = (int)$siteId;
         // Try getting the full URL redirect from the cache
         $redirect = $this->getRedirectFromCache($fullUrl, $siteId);
         if ($redirect) {
-            $this->incrementRedirectHitCount($redirect);
-            $this->saveRedirectToCache($fullUrl, $redirect);
+            if (!$this->redirectAppliesToSite($redirect, $siteId, $fullUrl)) {
+                $this->deleteRedirectFromCache($fullUrl, $siteId);
+            } else {
+                $this->incrementRedirectHitCount($redirect);
+                $this->saveRedirectToCache($fullUrl, $redirect, $siteId);
 
-            return $redirect;
+                return $redirect;
+            }
         }
         // Try getting the path only redirect from the cache
         $redirect = $this->getRedirectFromCache($pathOnly, $siteId);
         if ($redirect) {
-            $this->incrementRedirectHitCount($redirect);
-            $this->saveRedirectToCache($pathOnly, $redirect);
+            if (!$this->redirectAppliesToSite($redirect, $siteId, $pathOnly)) {
+                $this->deleteRedirectFromCache($pathOnly, $siteId);
+            } else {
+                $this->incrementRedirectHitCount($redirect);
+                $this->saveRedirectToCache($pathOnly, $redirect, $siteId);
 
-            return $redirect;
+                return $redirect;
+            }
         }
 
         $redirect = $this->getStaticRedirect($fullUrl, $pathOnly, $siteId, true);
         if ($redirect) {
             $this->incrementRedirectHitCount($redirect);
-            $this->saveRedirectToCache($pathOnly, $redirect);
+            $this->saveRedirectToCache($pathOnly, $redirect, $siteId);
 
             return $redirect;
         }
@@ -373,7 +382,7 @@ public function findRedirectMatch(string $fullUrl, string $pathOnly, $siteId = n
     public function getRedirectFromCache(string $url, int $siteId = 0): bool|array
     {
         $cache = Craft::$app->getCache();
-        $cacheKey = $this::CACHE_KEY . md5($url) . $siteId;
+        $cacheKey = $this->buildRedirectCacheKey($url, $siteId);
         $redirect = $cache->get($cacheKey);
         Craft::info(
             Craft::t(
@@ -428,23 +437,26 @@ public function incrementRedirectHitCount(&$redirectConfig): void
     /**
      * @param string $url
      * @param array $redirect
+     * @param int|null $cacheSiteId
      */
-    public function saveRedirectToCache(string $url, array $redirect): void
+    public function saveRedirectToCache(string $url, array $redirect, ?int $cacheSiteId = null): void
     {
         $cache = Craft::$app->getCache();
-        // Get the current site id
-        $sites = Craft::$app->getSites();
-        try {
-            $siteId = $sites->getCurrentSite()->id;
-        } catch (SiteNotFoundException $e) {
-            $siteId = 1;
+        if ($cacheSiteId === null) {
+            $sites = Craft::$app->getSites();
+            try {
+                $cacheSiteId = $sites->getCurrentSite()->id;
+            } catch (SiteNotFoundException $e) {
+                $cacheSiteId = $sites->getPrimarySite()->id;
+            }
         }
-        $cacheKey = $this::CACHE_KEY . md5($url) . $siteId;
+        $cacheSiteId = (int)$cacheSiteId;
+        $cacheKey = $this->buildRedirectCacheKey($url, $cacheSiteId);
         // Create the dependency tags
         $dependency = new TagDependency([
             'tags' => [
                 $this::GLOBAL_REDIRECTS_CACHE_TAG,
-                $this::GLOBAL_REDIRECTS_CACHE_TAG . $siteId,
+                $this::GLOBAL_REDIRECTS_CACHE_TAG . $cacheSiteId,
             ],
         ]);
         $cache->set($cacheKey, $redirect, Retour::$cacheDuration, $dependency);
@@ -458,6 +470,60 @@ public function saveRedirectToCache(string $url, array $redirect): void
         );
     }
 
+    /**
+     * Build the cache key used for a URL + site partition
+     */
+    protected function buildRedirectCacheKey(string $url, int $siteId): string
+    {
+        return $this::CACHE_KEY . md5($url) . $siteId;
+    }
+
+    /**
+     * Remove a cached redirect for the given URL/site partition (e.g. stale cross-site data)
+     */
+    protected function deleteRedirectFromCache(string $url, int $siteId): void
+    {
+        try {
+            Craft::$app->getCache()->delete($this->buildRedirectCacheKey($url, $siteId));
+        } catch (\Exception $_) {
+            Craft::warning(
+                Craft::t(
+                    'retour',
+                    'Failed to delete cached redirect for {url} and site ID {siteId}',
+                    ['url' => $url, 'siteId' => $siteId]
+                ),
+                __METHOD__
+            );
+        }
+    }
+
+    /**
+     * Determine whether a redirect row may be served for the given site
+     */
+    protected function redirectAppliesToSite(array $redirect, int $siteId, string $urlOrPath): bool
+    {
+        if (!array_key_exists('siteId', $redirect)) {
+            return true;
+        }
+        $redirectSiteId = $redirect['siteId'];
+        if ($redirectSiteId === null || $redirectSiteId === '' || (int)$redirectSiteId === 0) {
+            return true;
+        }
+
+        $shouldApply = (int)$redirectSiteId === (int)$siteId;
+        if (!$shouldApply) {
+            Craft::warning(
+                Craft::t(
+                    'retour',
+                    'Redirect {urlOrPath} attempted for site ID {siteId} but redirect is for site ID {redirectSiteId}',
+                    ['urlOrPath' => $urlOrPath, 'siteId' => $siteId, 'redirectSiteId' => $redirectSiteId]
+                ),
+                __METHOD__
+            );
+        }
+        return $shouldApply;
+    }
+
     /**
      * @param string $fullUrl
      * @param string $pathOnly
@@ -657,7 +723,7 @@ public function resolveRedirect(string $fullUrl, string $pathOnly, array $redire
                     case 'exactmatch':
                         if (strcasecmp($redirect['redirectSrcUrlParsed'], $url) === 0) {
                             $this->incrementRedirectHitCount($redirect);
-                            $this->saveRedirectToCache($url, $redirect);
+                            $this->saveRedirectToCache($url, $redirect, (int)$siteId);
 
                             // Throw the Redirects::EVENT_REDIRECT_RESOLVED event
                             $event = new RedirectResolvedEvent([
@@ -692,7 +758,7 @@ public function resolveRedirect(string $fullUrl, string $pathOnly, array $redire
                                     );
                                 }
                                 $url = preg_replace('/([^:])(\/{2,})/', '$1/', $url);
-                                $this->saveRedirectToCache($url, $redirect);
+                                $this->saveRedirectToCache($url, $redirect, (int)$siteId);
 
                                 // Throw the Redirects::EVENT_REDIRECT_RESOLVED event
                                 $event = new RedirectResolvedEvent([
@@ -729,7 +795,7 @@ public function resolveRedirect(string $fullUrl, string $pathOnly, array $redire
                             $result = call_user_func_array([$plugin, 'retourMatch'], $args);
                             if ($result) {
                                 $this->incrementRedirectHitCount($redirect);
-                                $this->saveRedirectToCache($url, $redirect);
+                                $this->saveRedirectToCache($url, $redirect, (int)$siteId);
 
                                 // Throw the Redirects::EVENT_REDIRECT_RESOLVED event
                                 $event = new RedirectResolvedEvent([
@@ -798,7 +864,7 @@ public function resolveEventRedirect(ResolveRedirectEvent $event, ?string $url =
                 // Save the modified redirect to the cache
                 $redirect['redirectDestUrl'] = $event->redirectDestUrl;
                 $redirect['redirectHttpCode'] = $event->redirectHttpCode;
-                $this->saveRedirectToCache($url, $redirect);
+                $this->saveRedirectToCache($url, $redirect, $event->siteId);
             }
         }
 

tests/_bootstrap.php

@@ -12,6 +12,7 @@
 define('CRAFT_TRANSLATIONS_PATH', __DIR__ . '/_craft/translations');
 define('CRAFT_VENDOR_PATH', dirname(__DIR__) . '/vendor');
 define('CRAFT_TESTS_PATH', __DIR__);
+define('CRAFT_ROOT_PATH', dirname(__DIR__));
 
 $devMode = true;
 

tests/_craft/config/db.php

@@ -1,9 +1,14 @@
 <?php
 
+use craft\helpers\App;
+
+$dsn = App::env('DB_DSN');
+
 return [
-    'dsn' => getenv('DB_DSN'),
-    'password' => getenv('DB_PASSWORD'),
-    'user' => getenv('DB_USER'),
-    'tablePrefix' => getenv('DB_TABLE_PREFIX'),
-    'schema' => getenv('DB_SCHEMA'),
+    'dsn' => $dsn,
+    'password' => App::env('DB_PASSWORD'),
+    'user' => App::env('DB_USER'),
+    'tablePrefix' => App::env('DB_TABLE_PREFIX'),
+    'schema' => App::env('DB_SCHEMA'),
+    'driver' => str_contains($dsn, 'mysql:') ? 'mysql' : null,
 ];

tests/unit/RedirectsServiceCacheTest.php

@@ -0,0 +1,84 @@
+<?php
+
+namespace nystudio107\retourtests\unit;
+
+use nystudio107\retour\Retour;
+use nystudio107\retour\services\Redirects;
+
+use Craft;
+
+use Codeception\Test\Unit;
+use UnitTester;
+
+/**
+ * Regression tests for multi-site redirect cache partitioning
+ */
+class RedirectsServiceCacheTest extends Unit
+{
+    protected UnitTester $tester;
+
+    private Redirects $redirects;
+
+    protected function _before(): void
+    {
+        parent::_before();
+        $this->redirects = Retour::$plugin->redirects;
+    }
+
+    public function testSaveRedirectToCacheUsesExplicitSitePartition(): void
+    {
+        $sites = Craft::$app->getSites();
+        $primarySiteId = (int)$sites->getPrimarySite()->id;
+        $otherSiteId = $primarySiteId === 1 ? 2 : 1;
+
+        $row = [
+            'id' => 999001,
+            'siteId' => $otherSiteId,
+            'enabled' => 1,
+            'redirectSrcUrl' => '/locations',
+            'redirectSrcUrlParsed' => '/locations',
+            'redirectSrcMatch' => 'pathonly',
+            'redirectMatchType' => 'exactmatch',
+            'redirectDestUrl' => '/our-locations',
+            'redirectHttpCode' => 301,
+        ];
+
+        $path = '/locations';
+        $this->redirects->saveRedirectToCache($path, $row, $otherSiteId);
+
+        $fromPartition = $this->redirects->getRedirectFromCache($path, $otherSiteId);
+        $this->assertIsArray($fromPartition);
+        $this->assertSame($otherSiteId, (int)$fromPartition['siteId']);
+
+        $wrongPartition = $this->redirects->getRedirectFromCache($path, $primarySiteId);
+        $this->assertNotTrue((bool)$wrongPartition);
+    }
+
+    public function testFindRedirectMatchIgnoresCacheEntryForDifferentSite(): void
+    {
+        $sites = Craft::$app->getSites();
+        $primarySiteId = (int)$sites->getPrimarySite()->id;
+        $otherSiteId = $primarySiteId === 1 ? 2 : 1;
+
+        $path = '/retour-cache-regression-' . bin2hex(random_bytes(4));
+        $row = [
+            'id' => 999002,
+            'siteId' => $otherSiteId,
+            'enabled' => 1,
+            'redirectSrcUrl' => $path,
+            'redirectSrcUrlParsed' => $path,
+            'redirectSrcMatch' => 'pathonly',
+            'redirectMatchType' => 'exactmatch',
+            'redirectDestUrl' => '/somewhere-else',
+            'redirectHttpCode' => 301,
+        ];
+
+        $this->redirects->saveRedirectToCache($path, $row, $primarySiteId);
+
+        $match = $this->redirects->findRedirectMatch('https://example.test' . $path, $path, $primarySiteId);
+        $this->assertNull($match);
+
+        $stillPoisoned = $this->redirects->getRedirectFromCache($path, $primarySiteId);
+        $this->assertNotTrue((bool)$stillPoisoned);
+    }
+}