refactor(links): clean up, test UrlRepository

src/server/controllers/UserController.ts

@@ -6,7 +6,7 @@ import {
   UrlEditRequest,
 } from '../../types/server/controllers/UserController'
 import jsonMessage from '../util/json'
-import { UrlManagementServiceInterface } from '../services/interfaces/UrlManagermentServiceInterface'
+import { UrlManagementServiceInterface } from '../services/interfaces/UrlManagementServiceInterface'
 import { DependencyIds } from '../constants'
 import { UserControllerInterface } from './interfaces/UserControllerInterface'
 import {

src/server/repositories/UrlRepository.ts

@@ -74,7 +74,7 @@ export class UrlRepository implements UrlRepositoryInterface {
   }
 
   public update: (
-    originalUrl: StorableUrl,
+    originalUrl: { shortUrl: string },
     changes: Partial<StorableUrl>,
     file?: StorableFile,
   ) => Promise<StorableUrl> = async (originalUrl, changes, file) => {
@@ -134,19 +134,6 @@ export class UrlRepository implements UrlRepositoryInterface {
     }
   }
 
-  public incrementClick: (shortUrl: string) => Promise<void> = async (
-    shortUrl,
-  ) => {
-    const url = await Url.findOne({ where: { shortUrl } })
-    if (!url) {
-      throw new NotFoundError(
-        `shortUrl not found in database:\tshortUrl=${shortUrl}`,
-      )
-    }
-
-    await url.increment('clicks')
-  }
-
   public plainTextSearch: (
     query: string,
     order: SearchResultsSortOrder,

src/server/repositories/interfaces/UrlRepositoryInterface.ts

@@ -41,13 +41,6 @@ export interface UrlRepositoryInterface {
    */
   getLongUrl: (shortUrl: string) => Promise<string>
 
-  /**
-   * Asynchronously increment the number of clicks in the database.
-   * @param {string} shortUrl
-   * @returns Promise that resolves to be empty.
-   */
-  incrementClick: (shortUrl: string) => Promise<void>
-
   /**
    * Performs plain text search on Urls based on their shortUrl and
    * description. The results are ranked in order of relevance based

src/server/services/UrlManagementService.ts

@@ -1,5 +1,5 @@
 import { inject, injectable } from 'inversify'
-import { UrlManagementServiceInterface } from './interfaces/UrlManagermentServiceInterface'
+import { UrlManagementServiceInterface } from './interfaces/UrlManagementServiceInterface'
 import { UpdateUrlOptions } from './types'
 import { UserRepositoryInterface } from '../repositories/interfaces/UserRepositoryInterface'
 import {

test/server/mocks/repositories/LinkStatisticsRepository.ts

@@ -1,7 +1,6 @@
 /* eslint-disable class-methods-use-this */
 
 import { injectable } from 'inversify'
-import { Transaction } from 'sequelize/types'
 import { LinkStatisticsRepositoryInterface } from '../../../../src/server/repositories/interfaces/LinkStatisticsRepositoryInterface'
 import { LinkStatisticsInterface } from '../../../../src/shared/interfaces/link-statistics'
 import { DeviceType } from '../../../../src/server/services/interfaces/DeviceCheckServiceInterface'
@@ -25,11 +24,6 @@ export class MockLinkStatisticsRepository
     })
   }
 
-  incrementClick: (
-    shortUrl: string,
-    transaction?: Transaction,
-  ) => Promise<boolean> = () => Promise.resolve(true)
-
   updateLinkStatistics: (shortUrl: string, device: DeviceType) => void = () =>
     Promise.resolve(true)
 }

test/server/mocks/repositories/UrlRepository.ts

@@ -35,10 +35,6 @@ export class UrlRepositoryMock implements UrlRepositoryInterface {
     throw new Error('Not implemented')
   }
 
-  incrementClick: (shortUrl: string) => Promise<void> = () => {
-    throw new Error('Not implemented')
-  }
-
   plainTextSearch: (
     query: string,
     order: SearchResultsSortOrder,

test/server/repositories/UrlRepository.test.ts

@@ -1,4 +1,5 @@
 import { QueryTypes } from 'sequelize'
+import { S3 } from 'aws-sdk'
 import {
   clicksModelMock,
   devicesModelMock,
@@ -8,10 +9,12 @@ import {
   redisMockClient,
   urlModelMock,
 } from '../api/util'
-import { S3InterfaceMock } from '../mocks/services/aws'
 import { UrlRepository } from '../../../src/server/repositories/UrlRepository'
 import { UrlMapper } from '../../../src/server/mappers/UrlMapper'
 import { SearchResultsSortOrder } from '../../../src/shared/search'
+import { FileVisibility, S3ServerSide } from '../../../src/server/services/aws'
+import { NotFoundError } from '../../../src/server/util/error'
+import { StorableUrlState } from '../../../src/server/repositories/enums'
 
 jest.mock('../../../src/server/models/url', () => ({
   Url: urlModelMock,
@@ -35,18 +38,270 @@ jest.mock('../../../src/server/redis', () => ({
 
 jest.mock('../../../src/server/util/sequelize', () => ({
   transaction: mockTransaction,
-  sequelize: { query: mockQuery },
+  sequelize: { query: mockQuery, transaction: mockTransaction },
 }))
 
-const repository = new UrlRepository(new S3InterfaceMock(), new UrlMapper())
+const s3Client = new S3()
+const s3Bucket = 'bucket'
+const fileURLPrefix = 'prefix'
+
+const fileBucket = new S3ServerSide(s3Client, s3Bucket, fileURLPrefix)
+
+const repository = new UrlRepository(fileBucket, new UrlMapper())
 const cacheGetSpy = jest.spyOn(redisMockClient, 'get')
 
-describe('UrlRepository tests', () => {
+describe('UrlRepository', () => {
   beforeEach(async () => {
     redisMockClient.flushall()
     cacheGetSpy.mockClear()
   })
-  describe('getLongUrl tests', () => {
+
+  it('passes findByShortUrl through to Url.findOne', async () => {
+    const findOne = jest.spyOn(urlModelMock, 'findOne')
+    try {
+      const shortUrl = 'abcdef'
+      findOne.mockResolvedValue(null)
+      await expect(repository.findByShortUrl(shortUrl)).resolves.toBeNull()
+      expect(findOne).toHaveBeenCalledWith({ where: { shortUrl } })
+    } finally {
+      // Deliberately not call findOne.mockRestore(), as it seems
+      // to permanently make it unmockable
+    }
+  })
+
+  describe('create', () => {
+    const create = jest.spyOn(urlModelMock, 'create')
+    const putObject = jest.spyOn(s3Client, 'putObject')
+
+    // @ts-ignore
+    putObject.mockReturnValue({ promise: () => Promise.resolve() })
+
+    const userId = 2
+    const shortUrl = 'abcdef'
+    const longUrl = 'https://www.agency.gov.sg'
+
+    beforeEach(() => {
+      create.mockReset()
+      putObject.mockClear()
+    })
+
+    it('creates the specified longUrl', async () => {
+      const storableUrl = {
+        shortUrl,
+        longUrl,
+        state: 'ACTIVE',
+        clicks: 2,
+        isFile: false,
+        createdAt: new Date(),
+        updatedAt: new Date(),
+        isSearchable: true,
+        description: 'An agency of the Singapore Government',
+        contactEmail: '[email protected]',
+      }
+      create.mockResolvedValue(storableUrl)
+      await expect(
+        repository.create({ userId, shortUrl, longUrl }),
+      ).resolves.toStrictEqual(storableUrl)
+      expect(create).toHaveBeenCalledWith(
+        {
+          longUrl,
+          shortUrl,
+          userId,
+          isFile: false,
+        },
+        expect.anything(),
+      )
+      expect(putObject).not.toHaveBeenCalled()
+    })
+
+    it('creates the specified public file', async () => {
+      const file = {
+        data: Buffer.from(''),
+        key: 'key',
+        mimetype: 'text/csv',
+      }
+      const storableUrl = {
+        shortUrl,
+        longUrl: fileBucket.buildFileLongUrl(file.key),
+        state: 'ACTIVE',
+        clicks: 2,
+        isFile: true,
+        createdAt: new Date(),
+        updatedAt: new Date(),
+        isSearchable: true,
+        description: 'An agency of the Singapore Government',
+        contactEmail: '[email protected]',
+      }
+      create.mockResolvedValue(storableUrl)
+      await expect(
+        repository.create({ userId, shortUrl }, file),
+      ).resolves.toStrictEqual(storableUrl)
+      expect(create).toHaveBeenCalledWith(
+        {
+          longUrl: fileBucket.buildFileLongUrl(file.key),
+          shortUrl,
+          userId,
+          isFile: true,
+        },
+        expect.anything(),
+      )
+      expect(putObject).toHaveBeenCalledWith({
+        ContentType: file.mimetype,
+        Bucket: s3Bucket,
+        Body: file.data,
+        Key: file.key,
+        ACL: FileVisibility.Public,
+        CacheControl: 'no-cache',
+      })
+    })
+  })
+
+  describe('update', () => {
+    const findOne = jest.spyOn(urlModelMock, 'findOne')
+    const putObject = jest.spyOn(s3Client, 'putObject')
+    const putObjectAcl = jest.spyOn(s3Client, 'putObjectAcl')
+
+    const shortUrl = 'abcdef'
+
+    beforeEach(() => {
+      findOne.mockReset()
+      putObject.mockClear()
+      putObjectAcl.mockClear()
+    })
+
+    afterAll(() => {
+      findOne.mockRestore()
+    })
+
+    // @ts-ignore
+    putObject.mockReturnValue({ promise: () => Promise.resolve() })
+    // @ts-ignore
+    putObjectAcl.mockReturnValue({ promise: () => Promise.resolve() })
+
+    it('should throw NotFoundError on not found', async () => {
+      findOne.mockResolvedValue(null)
+      await expect(repository.update({ shortUrl }, {})).rejects.toBeInstanceOf(
+        NotFoundError,
+      )
+      expect(findOne).toHaveBeenCalledWith({ where: { shortUrl } })
+      expect(putObject).not.toHaveBeenCalled()
+      expect(putObjectAcl).not.toHaveBeenCalled()
+    })
+
+    it('should update non-file links', async () => {
+      const description = 'Changes made'
+      const update = jest.fn()
+      findOne.mockResolvedValue({ isFile: false, update })
+      await expect(
+        repository.update({ shortUrl }, { description }),
+      ).resolves.toStrictEqual(expect.objectContaining({ isFile: false }))
+      expect(findOne).toHaveBeenCalledWith({ where: { shortUrl } })
+      expect(putObject).not.toHaveBeenCalled()
+      expect(putObjectAcl).not.toHaveBeenCalled()
+      expect(update).toHaveBeenCalledWith({ description }, expect.anything())
+    })
+
+    it('should update non-state changes on file links', async () => {
+      const description = 'Changes made'
+      const longUrl = 'https://files.go.gov.sg/key'
+      const update = jest.fn()
+      findOne.mockResolvedValue({ isFile: true, longUrl, update })
+      await expect(
+        repository.update({ shortUrl }, { description }),
+      ).resolves.toStrictEqual(
+        expect.objectContaining({ isFile: true, longUrl }),
+      )
+      expect(findOne).toHaveBeenCalledWith({ where: { shortUrl } })
+      expect(putObject).not.toHaveBeenCalled()
+      expect(putObjectAcl).not.toHaveBeenCalled()
+      expect(update).toHaveBeenCalledWith({ description }, expect.anything())
+    })
+
+    it('should update state change to Active on file links', async () => {
+      const state = StorableUrlState.Active
+      const longUrl = 'https://files.go.gov.sg/key'
+      const update = jest.fn()
+      findOne.mockResolvedValue({ isFile: true, longUrl, update })
+      await expect(
+        repository.update({ shortUrl }, { state }),
+      ).resolves.toStrictEqual(
+        expect.objectContaining({ isFile: true, longUrl }),
+      )
+      expect(findOne).toHaveBeenCalledWith({ where: { shortUrl } })
+      expect(putObject).not.toHaveBeenCalled()
+      expect(update).toHaveBeenCalledWith({ state }, expect.anything())
+
+      expect(putObjectAcl).toHaveBeenCalledWith({
+        Bucket: s3Bucket,
+        Key: fileBucket.getKeyFromLongUrl(longUrl),
+        ACL: FileVisibility.Public,
+      })
+    })
+
+    it('should update state change to Inactive on file links', async () => {
+      const state = StorableUrlState.Inactive
+      const longUrl = 'https://files.go.gov.sg/key'
+      const update = jest.fn()
+      findOne.mockResolvedValue({ isFile: true, longUrl, update })
+      await expect(
+        repository.update({ shortUrl }, { state }),
+      ).resolves.toStrictEqual(
+        expect.objectContaining({ isFile: true, longUrl }),
+      )
+      expect(findOne).toHaveBeenCalledWith({ where: { shortUrl } })
+      expect(putObject).not.toHaveBeenCalled()
+      expect(update).toHaveBeenCalledWith({ state }, expect.anything())
+
+      expect(putObjectAcl).toHaveBeenCalledWith({
+        Bucket: s3Bucket,
+        Key: fileBucket.getKeyFromLongUrl(longUrl),
+        ACL: FileVisibility.Private,
+      })
+    })
+
+    it('should update file content for file links', async () => {
+      const oldKey = 'old-key'
+      const newKey = 'new-key'
+      const longUrl = fileBucket.buildFileLongUrl(oldKey)
+      const newLongUrl = fileBucket.buildFileLongUrl(newKey)
+
+      const file = {
+        key: newKey,
+        data: Buffer.from(''),
+        mimetype: 'text/csv',
+      }
+
+      const update = jest.fn()
+      findOne.mockResolvedValue({ isFile: true, longUrl, update })
+
+      await expect(
+        repository.update({ shortUrl }, {}, file),
+      ).resolves.toStrictEqual(
+        expect.objectContaining({ isFile: true, longUrl }),
+      )
+      expect(findOne).toHaveBeenCalledWith({ where: { shortUrl } })
+
+      expect(update).toHaveBeenCalledWith(
+        { longUrl: newLongUrl },
+        expect.anything(),
+      )
+      expect(putObjectAcl).toHaveBeenCalledWith({
+        Bucket: s3Bucket,
+        Key: oldKey,
+        ACL: FileVisibility.Private,
+      })
+      expect(putObject).toHaveBeenCalledWith({
+        ContentType: file.mimetype,
+        Bucket: s3Bucket,
+        Body: file.data,
+        Key: file.key,
+        ACL: FileVisibility.Public,
+        CacheControl: 'no-cache',
+      })
+    })
+  })
+
+  describe('getLongUrl', () => {
     it('should return from db when cache is empty', async () => {
       await expect(repository.getLongUrl('a')).resolves.toBe('aa')
     })
@@ -68,7 +323,7 @@ describe('UrlRepository tests', () => {
     })
   })
 
-  describe('plainTextSearch tests', () => {
+  describe('plainTextSearch', () => {
     beforeEach(() => {
       mockQuery.mockClear()
     })