fix: do not update as JSON object yet

Author: dcshzj

src/server/modules/redirect/__tests__/RedirectController.test.ts

@@ -21,7 +21,7 @@ import {
   CrawlerCheckService,
   RedirectService,
 } from '../services'
-import { RedirectDestination } from '../../../repositories/types'
+// import { RedirectDestination } from '../../../repositories/types'
 
 const redisMockClient = redisMock.createClient()
 const sequelizeMock = new SequelizeMock()
@@ -520,40 +520,50 @@ describe('redirect API tests', () => {
   test('url does exists in cache but not db', async () => {
     const req = createRequestWithShortUrl('Aaa')
     const res = httpMocks.createResponse()
-    const redirectDestination: RedirectDestination = {
-      longUrl: 'aa',
-      isFile: false,
-      safeBrowsingExpiry: new Date(Date.now() + 1000).toISOString(),
-    }
-
-    redisMockClient.set('aaa', JSON.stringify(redirectDestination))
+    // FIXME: Update to use the new RedirectDestination type
+    // const redirectDestination: RedirectDestination = {
+    //   longUrl: 'aa',
+    //   isFile: false,
+    //   safeBrowsingExpiry: new Date(Date.now() + 1000).toISOString(),
+    // }
+
+    // redisMockClient.set('aaa', JSON.stringify(redirectDestination))
+    redisMockClient.set('aaa', 'aa')
     mockDbEmpty()
 
     await container
       .get<RedirectController>(DependencyIds.redirectController)
       .redirect(req, res)
 
-    expect(res.statusCode).toBe(302)
-    expect(res._getRedirectUrl()).toBe('aa')
+    // expect(res.statusCode).toBe(302)
+    // NOTE: This is 404 now as the safe browsing repository needs to be updated
+    // but it will be 302 once the safe browsing expiry is stored in redis
+    expect(res.statusCode).toBe(404)
+    // expect(res._getRedirectUrl()).toBe('aa')
   })
 
   test('url in cache and db is down', async () => {
     const req = createRequestWithShortUrl('Aaa')
     const res = httpMocks.createResponse()
-    const redirectDestination: RedirectDestination = {
-      longUrl: 'aa',
-      isFile: false,
-      safeBrowsingExpiry: new Date(Date.now() + 1000).toISOString(),
-    }
+    // FIXME: Update to use the new RedirectDestination type
+    // const redirectDestination: RedirectDestination = {
+    //   longUrl: 'aa',
+    //   isFile: false,
+    //   safeBrowsingExpiry: new Date(Date.now() + 1000).toISOString(),
+    // }
 
     mockDbDown()
-    redisMockClient.set('aaa', JSON.stringify(redirectDestination))
+    // redisMockClient.set('aaa', JSON.stringify(redirectDestination))
+    redisMockClient.set('aaa', 'aa')
 
     await container
       .get<RedirectController>(DependencyIds.redirectController)
       .redirect(req, res)
-    expect(res.statusCode).toBe(302)
-    expect(res._getRedirectUrl()).toBe('aa')
+    // expect(res.statusCode).toBe(302)
+    // NOTE: This is 404 now as the safe browsing repository needs to be updated
+    // but it will be 302 once the safe browsing expiry is stored in redis
+    expect(res.statusCode).toBe(404)
+    // expect(res._getRedirectUrl()).toBe('aa')
   })
 
   test('retrieval of gtag for transition page', async () => {

src/server/repositories/UrlRepository.ts

@@ -208,8 +208,10 @@ export class UrlRepository implements UrlRepositoryInterface {
       } catch {
         // Cache failed, look in database
         const redirectDestination = await this.getLongUrlFromDatabase(shortUrl)
-        this.cacheShortUrl(shortUrl, redirectDestination).catch((error) =>
-          logger.error(`Unable to cache short URL: ${error}`),
+        // FIXME: Cache the entire RedirectDestination object once all app
+        // clients are updated to use the new RedirectDestination type.
+        this.cacheShortUrl(shortUrl, redirectDestination.longUrl).catch(
+          (error) => logger.error(`Unable to cache short URL: ${error}`),
         )
         return redirectDestination
       }
@@ -533,11 +535,22 @@ export class UrlRepository implements UrlRepositoryInterface {
             const redirectDestination = JSON.parse(cacheLongUrl)
             resolve(redirectDestination)
           } catch (_) {
-            reject(
-              new NotFoundError(
-                `longUrl not found in cache:\tshortUrl=${shortUrl}`,
-              ),
+            logger.info(
+              `Cache lookup returned a string instead of an object:\tshortUrl=${shortUrl}`,
             )
+            resolve({
+              longUrl: cacheLongUrl,
+              isFile: false,
+              safeBrowsingExpiry: null,
+            })
+
+            // FIXME: Throw NotFoundError once all app clients are updated to
+            // use the new RedirectDestination type.
+            // reject(
+            //   new NotFoundError(
+            //     `longUrl not found in cache:\tshortUrl=${shortUrl}`,
+            //   ),
+            // )
           }
         }
       }),
@@ -549,23 +562,35 @@ export class UrlRepository implements UrlRepositoryInterface {
    * @param  {string} shortUrl Short url.
    * @param  {string} longUrl Long url.
    */
-  private cacheShortUrl: (
-    shortUrl: string,
-    redirectDestination: RedirectDestination,
-  ) => Promise<void> = (shortUrl, redirectDestination) => {
-    return new Promise((resolve, reject) => {
-      redirectClient.set(
-        shortUrl,
-        JSON.stringify(redirectDestination),
-        'EX',
-        redirectExpiry,
-        (err) => {
+  private cacheShortUrl: (shortUrl: string, longUrl: string) => Promise<void> =
+    (shortUrl, longUrl) => {
+      return new Promise((resolve, reject) => {
+        redirectClient.set(shortUrl, longUrl, 'EX', redirectExpiry, (err) => {
           if (err) reject(err)
           else resolve()
-        },
-      )
-    })
-  }
+        })
+      })
+    }
+
+  // FIXME: This is the future implementation of the cacheShortUrl method to be
+  // used once all app clients are updated to use the new RedirectDestination
+  // private cacheShortUrl: (
+  //   shortUrl: string,
+  //   redirectDestination: RedirectDestination,
+  // ) => Promise<void> = (shortUrl, redirectDestination) => {
+  //   return new Promise((resolve, reject) => {
+  //     redirectClient.set(
+  //       shortUrl,
+  //       JSON.stringify(redirectDestination),
+  //       'EX',
+  //       redirectExpiry,
+  //       (err) => {
+  //         if (err) reject(err)
+  //         else resolve()
+  //       },
+  //     )
+  //   })
+  // }
 
   /**
    * Generates the ranking algorithm to be used in the ORDER BY clause in the