From f71e34b9994b51082dbf6cccd4a330250f875151 Mon Sep 17 00:00:00 2001 From: gweiying <39231249+gweiying@users.noreply.github.com> Date: Mon, 18 Mar 2024 18:18:04 +0800 Subject: [PATCH 1/2] Chore/fix user query (#2299) * chore: replace query to check if shortlink is available * chore: add tests * chore: make return more readable --- .../user/services/UrlManagementService.ts | 6 +++-- .../__tests__/UrlManagementService.test.ts | 23 ++++++++++--------- src/server/repositories/UrlRepository.ts | 22 ++++++++++++++++++ .../interfaces/UrlRepositoryInterface.ts | 6 +++++ .../mocks/repositories/UrlRepository.ts | 4 ++++ 5 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/server/modules/user/services/UrlManagementService.ts b/src/server/modules/user/services/UrlManagementService.ts index 9578bb3a1..71023d8e1 100644 --- a/src/server/modules/user/services/UrlManagementService.ts +++ b/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.`) } diff --git a/src/server/modules/user/services/__tests__/UrlManagementService.test.ts b/src/server/modules/user/services/__tests__/UrlManagementService.test.ts index 1bdcf381e..d77900984 100644 --- a/src/server/modules/user/services/__tests__/UrlManagementService.test.ts +++ b/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( diff --git a/src/server/repositories/UrlRepository.ts b/src/server/repositories/UrlRepository.ts index ef1d2249b..23988d729 100644 --- a/src/server/repositories/UrlRepository.ts +++ b/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} Returns true if shortUrl is available and false otherwise. + */ + public isShortUrlAvailable: (shortUrl: string) => Promise = 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 = async ( shortUrl, ) => { diff --git a/src/server/repositories/interfaces/UrlRepositoryInterface.ts b/src/server/repositories/interfaces/UrlRepositoryInterface.ts index 1a39f70a7..b2a084d8a 100644 --- a/src/server/repositories/interfaces/UrlRepositoryInterface.ts +++ b/src/server/repositories/interfaces/UrlRepositoryInterface.ts @@ -43,6 +43,12 @@ export interface UrlRepositoryInterface { file?: StorableFile, ): Promise + /** + * Returns true if shortUrl is available, otherwise false. + * @param {string} shortUrl The shortUrl. + */ + isShortUrlAvailable: (shortUrl: string) => Promise + /** * 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 diff --git a/test/server/mocks/repositories/UrlRepository.ts b/test/server/mocks/repositories/UrlRepository.ts index f9b46f255..b488d2b7e 100644 --- a/test/server/mocks/repositories/UrlRepository.ts +++ b/test/server/mocks/repositories/UrlRepository.ts @@ -39,6 +39,10 @@ export class UrlRepositoryMock implements UrlRepositoryInterface { throw new Error('Not implemented') } + isShortUrlAvailable: (shortUrl: string) => Promise = () => { + throw new Error('Not implemented') + } + getLongUrl: (shortUrl: string) => Promise = () => { throw new Error('Not implemented') } From efd86f65f6765e22a2ef22fdf05ddc18722176df Mon Sep 17 00:00:00 2001 From: gweiying Date: Mon, 18 Mar 2024 18:18:59 +0800 Subject: [PATCH 2/2] 1.78.2 --- CHANGELOG.md | 7 +++++++ package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed267989e..c3c4fa85a 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/package-lock.json b/package-lock.json index c65bd850d..1998bcbf6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "GoGovSG", - "version": "1.78.1", + "version": "1.78.2", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "GoGovSG", - "version": "1.78.1", + "version": "1.78.2", "license": "MIT", "dependencies": { "@datadog/browser-rum": "^4.15.0", diff --git a/package.json b/package.json index b57f7286c..956d15a81 100644 --- a/package.json +++ b/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": {