Merge pull request #2300 from opengovsg/release-1.78.2

Release 1.78.2

CHANGELOG.md

@@ -4,8 +4,15 @@ All notable changes to this project will be documented in this file. Dates are d
 
 Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 
+#### [v1.78.2](https://github.com/opengovsg/GoGovSG/compare/v1.78.1...v1.78.2)
+
+- Chore/fix user query [`#2299`](https://github.com/opengovsg/GoGovSG/pull/2299)
+- [develop] 1.78.1 [`#2298`](https://github.com/opengovsg/GoGovSG/pull/2298)
+
 #### [v1.78.1](https://github.com/opengovsg/GoGovSG/compare/v1.78.0...v1.78.1)
 
+> 19 February 2024
+
 - build(deps): bump webpack-dev-server from 3 to 4 [`#2291`](https://github.com/opengovsg/GoGovSG/pull/2291)
 - chore: fix serverless plugin include dependencies to v5 [`#2290`](https://github.com/opengovsg/GoGovSG/pull/2290)
 - build(deps): bump node from 16 to 18 [`#2285`](https://github.com/opengovsg/GoGovSG/pull/2285)

package.json

@@ -1,6 +1,6 @@
 {
   "name": "GoGovSG",
-  "version": "1.78.1",
+  "version": "1.78.2",
   "description": "Link shortener for Singapore government.",
   "main": "src/server/index.js",
   "scripts": {

src/server/modules/user/services/UrlManagementService.ts

@@ -76,8 +76,10 @@ export class UrlManagementService implements interfaces.UrlManagementService {
       shortUrl = await generateShortUrl(API_LINK_RANDOM_STR_LENGTH)
     }
 
-    const owner = await this.userRepository.findUserByUrl(shortUrl)
-    if (owner) {
+    const isShortUrlAvailable = await this.urlRepository.isShortUrlAvailable(
+      shortUrl,
+    )
+    if (!isShortUrlAvailable) {
       throw new AlreadyExistsError(`Short link "${shortUrl}" is already used.`)
     }
 

src/server/modules/user/services/__tests__/UrlManagementService.test.ts

@@ -23,6 +23,7 @@ describe('UrlManagementService', () => {
     update: jest.fn(),
     create: jest.fn(),
     findByShortUrlWithTotalClicks: jest.fn(),
+    isShortUrlAvailable: jest.fn(),
     getLongUrl: jest.fn(),
     plainTextSearch: jest.fn(),
     rawDirectorySearch: jest.fn(),
@@ -40,7 +41,6 @@ describe('UrlManagementService', () => {
 
     beforeEach(() => {
       userRepository.findById.mockReset()
-      userRepository.findUserByUrl.mockReset()
       urlRepository.findByShortUrlWithTotalClicks.mockReset()
       urlRepository.create.mockReset()
     })
@@ -51,34 +51,33 @@ describe('UrlManagementService', () => {
         service.createUrl(userId, sourceConsole, shortUrl, longUrl),
       ).rejects.toBeInstanceOf(NotFoundError)
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(userRepository.findUserByUrl).not.toHaveBeenCalled()
+      expect(urlRepository.isShortUrlAvailable).not.toHaveBeenCalledWith(
+        shortUrl,
+      )
       expect(urlRepository.create).not.toHaveBeenCalled()
     })
 
     it('throws AlreadyExistsError on existing url', async () => {
       userRepository.findById.mockResolvedValue({ id: userId })
-      userRepository.findUserByUrl.mockResolvedValue({
-        shortUrl,
-        longUrl,
-        email: userId,
-      })
+      urlRepository.isShortUrlAvailable.mockResolvedValue(false)
       await expect(
         service.createUrl(userId, sourceConsole, shortUrl, longUrl),
       ).rejects.toBeInstanceOf(AlreadyExistsError)
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(userRepository.findUserByUrl).toHaveBeenCalledWith(shortUrl)
+      expect(urlRepository.isShortUrlAvailable).toHaveBeenCalledWith(shortUrl)
       expect(urlRepository.create).not.toHaveBeenCalled()
     })
 
     it('processes new non-file url', async () => {
       userRepository.findById.mockResolvedValue({ id: userId })
+      urlRepository.isShortUrlAvailable.mockResolvedValue(true)
       urlRepository.findByShortUrlWithTotalClicks.mockResolvedValue(null)
       urlRepository.create.mockResolvedValue({ userId, longUrl, shortUrl })
       await expect(
         service.createUrl(userId, sourceConsole, shortUrl, longUrl),
       ).resolves.toStrictEqual({ userId, longUrl, shortUrl })
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(userRepository.findUserByUrl).toHaveBeenCalledWith(shortUrl)
+      expect(urlRepository.isShortUrlAvailable).toHaveBeenCalledWith(shortUrl)
       expect(urlRepository.create).toHaveBeenCalledWith(
         { userId, longUrl, shortUrl, source: sourceConsole },
         undefined,
@@ -92,13 +91,14 @@ describe('UrlManagementService', () => {
         mimetype: 'application/json',
       }
       userRepository.findById.mockResolvedValue({ id: userId })
+      urlRepository.isShortUrlAvailable.mockResolvedValue(true)
       urlRepository.findByShortUrlWithTotalClicks.mockResolvedValue(null)
       urlRepository.create.mockResolvedValue({ userId, longUrl, shortUrl })
       await expect(
         service.createUrl(userId, sourceConsole, shortUrl, longUrl, file),
       ).resolves.toStrictEqual({ userId, longUrl, shortUrl })
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(userRepository.findUserByUrl).toHaveBeenCalledWith(shortUrl)
+      expect(urlRepository.isShortUrlAvailable).toHaveBeenCalledWith(shortUrl)
       expect(urlRepository.create).toHaveBeenCalledWith(
         { userId, longUrl, shortUrl, source: sourceConsole },
         {
@@ -119,13 +119,14 @@ describe('UrlManagementService', () => {
       const service = new UrlManagementService(userRepository, urlRepository)
 
       userRepository.findById.mockResolvedValue({ id: userId })
+      urlRepository.isShortUrlAvailable.mockResolvedValue(true)
       urlRepository.findByShortUrlWithTotalClicks.mockResolvedValue(null)
       urlRepository.create.mockResolvedValue({ userId, longUrl, shortUrl })
       await expect(
         service.createUrl(userId, sourceApi, undefined, longUrl),
       ).resolves.toStrictEqual({ userId, longUrl, shortUrl })
       expect(userRepository.findById).toHaveBeenCalledWith(userId)
-      expect(userRepository.findUserByUrl).toHaveBeenCalledWith(
+      expect(urlRepository.isShortUrlAvailable).toHaveBeenCalledWith(
         expect.stringMatching(/^.{4}$/),
       )
       expect(urlRepository.create).toHaveBeenCalledWith(

src/server/repositories/UrlRepository.ts

@@ -175,6 +175,28 @@ export class UrlRepository implements UrlRepositoryInterface {
     return this.urlMapper.persistenceToDto(newUrl)
   }
 
+  /**
+   * @param  {string} shortUrl Short url.
+   * @returns {Promise<boolean>} Returns true if shortUrl is available and false otherwise.
+   */
+  public isShortUrlAvailable: (shortUrl: string) => Promise<boolean> = async (
+    shortUrl,
+  ) => {
+    try {
+      // Cache lookup
+      await this.getLongUrlFromCache(shortUrl)
+      // if long url does not exist, throws error
+      // return false if no error since long url exists
+      return false
+    } catch {
+      // Cache failed, look in database
+      const url = await Url.findOne({
+        where: { shortUrl },
+      })
+      return !url
+    }
+  }
+
   public getLongUrl: (shortUrl: string) => Promise<string> = async (
     shortUrl,
   ) => {

src/server/repositories/interfaces/UrlRepositoryInterface.ts

@@ -43,6 +43,12 @@ export interface UrlRepositoryInterface {
     file?: StorableFile,
   ): Promise<StorableUrl>
 
+  /**
+   * Returns true if shortUrl is available, otherwise false.
+   * @param {string} shortUrl The shortUrl.
+   */
+  isShortUrlAvailable: (shortUrl: string) => Promise<boolean>
+
   /**
    * Looks up the longUrl given a shortUrl from the cache, falling back
    * to the database. The cache is re-populated if the database lookup is

test/server/mocks/repositories/UrlRepository.ts

@@ -39,6 +39,10 @@ export class UrlRepositoryMock implements UrlRepositoryInterface {
     throw new Error('Not implemented')
   }
 
+  isShortUrlAvailable: (shortUrl: string) => Promise<boolean> = () => {
+    throw new Error('Not implemented')
+  }
+
   getLongUrl: (shortUrl: string) => Promise<string> = () => {
     throw new Error('Not implemented')
   }