diff --git a/src/server/modules/bulk/services/BulkService.ts b/src/server/modules/bulk/services/BulkService.ts index 5c3842ed4..938abae41 100644 --- a/src/server/modules/bulk/services/BulkService.ts +++ b/src/server/modules/bulk/services/BulkService.ts @@ -35,7 +35,7 @@ export class BulkService implements interfaces.BulkService { return new Promise((resolve, reject) => { Papa.parse(dataString, { - skipEmptyLines: false, + skipEmptyLines: true, delimiter: ',', complete: () => { // check for empty file @@ -75,60 +75,38 @@ export class BulkService implements interfaces.BulkService { ) const noParsingError = step.errors.length === 0 - switch (true) { - case !acceptableLinkCount: - dogstatsd.increment(BULK_VALIDATION_ERROR, 1, 1, [ - `${BULK_VALIDATION_ERROR_TAGS.acceptableLinkCount}`, - ]) - throw new Error( - `File exceeded ${BULK_UPLOAD_MAX_NUM} original URLs to shorten`, - ) - case !onlyOneColumn: - dogstatsd.increment(BULK_VALIDATION_ERROR, 1, 1, [ - `${BULK_VALIDATION_ERROR_TAGS.onlyOneColumn}`, - ]) - throw new Error( - `Row ${ - counter + 1 - }: ${rowData} contains more than one column of data`, - ) - case !isNotEmpty: - dogstatsd.increment(BULK_VALIDATION_ERROR, 1, 1, [ - `${BULK_VALIDATION_ERROR_TAGS.isNotEmpty}`, - ]) - throw new Error(`Row ${counter + 1} is empty`) - case !isValidUrl: - dogstatsd.increment(BULK_VALIDATION_ERROR, 1, 1, [ - `${BULK_VALIDATION_ERROR_TAGS.isValidUrl}`, - ]) - throw new Error( - `Row ${counter + 1}: ${stringData} is not valid`, - ) - case !isNotBlacklisted: - dogstatsd.increment(BULK_VALIDATION_ERROR, 1, 1, [ - `${BULK_VALIDATION_ERROR_TAGS.isNotBlacklisted}`, - ]) - throw new Error( - `Row ${counter + 1}: ${stringData} is blacklisted`, - ) - case !isNotCircularRedirect: - dogstatsd.increment(BULK_VALIDATION_ERROR, 1, 1, [ - `${BULK_VALIDATION_ERROR_TAGS.isNotCircularRedirect}`, - ]) - throw new Error( - `Row ${ - counter + 1 - }: ${stringData} redirects back to ${ogHostname}`, - ) - case !noParsingError: - dogstatsd.increment(BULK_VALIDATION_ERROR, 1, 1, [ - `${BULK_VALIDATION_ERROR_TAGS.noParsingError}`, - ]) - throw new Error('Parsing error') - default: - // no error, do nothing + if ( + acceptableLinkCount && + onlyOneColumn && + isNotBlacklisted && + isNotEmpty && + isValidUrl && + isNotCircularRedirect && + noParsingError + ) { + longUrls.push(stringData) + } else { + let errorMessage = `Row ${counter + 1}: ` + if (!acceptableLinkCount) { + errorMessage += 'exceeds maximum number of links' + } else if (!onlyOneColumn) { + errorMessage += 'has more than one column' + } else if (!isNotBlacklisted) { + errorMessage += 'contains blacklisted link' + } else if (!isNotEmpty) { + errorMessage += 'is empty' + } else if (!isValidUrl) { + errorMessage += 'contains invalid url' + } else if (isNotCircularRedirect) { + errorMessage += 'contains circular redirect' + } else if (!noParsingError) { + errorMessage += 'has parsing error' + } + dogstatsd.increment(BULK_VALIDATION_ERROR, 1, 1, [ + `${BULK_VALIDATION_ERROR_TAGS.isValidUrl}`, + ]) + throw new Error(errorMessage) } - longUrls.push(stringData) } counter += 1 }, @@ -140,10 +118,8 @@ export class BulkService implements interfaces.BulkService { async (longUrls) => { return Promise.all( longUrls.map(async (longUrl) => { - return { - longUrl, - shortUrl: await generateShortUrl(BULK_UPLOAD_RANDOM_STR_LENGTH), - } + const shortUrl = await generateShortUrl(BULK_UPLOAD_RANDOM_STR_LENGTH) + return { shortUrl, longUrl } }), ) } diff --git a/src/server/modules/bulk/services/__tests__/BulkService.test.ts b/src/server/modules/bulk/services/__tests__/BulkService.test.ts index 52c720e40..bf2ed7da2 100644 --- a/src/server/modules/bulk/services/__tests__/BulkService.test.ts +++ b/src/server/modules/bulk/services/__tests__/BulkService.test.ts @@ -2,6 +2,8 @@ import { UploadedFile } from 'express-fileupload' import { BULK_UPLOAD_HEADER } from '../../../../../shared/constants' import blackListedSites from '../../../../resources/blacklist' +import { BulkService } from '../BulkService' +import { ogHostname } from '../../../../config' /** * Unit tests for BulkService. @@ -63,66 +65,108 @@ const validUrlTests: UrlTest[] = [ }, ] -describe('BulkService tests', () => { - afterAll(jest.resetModules) +describe('BulkService', () => { + let bulkService: BulkService - describe('parseCsv tests', () => { - jest.resetModules() - jest.mock('../../../../config', () => ({ - bulkUploadMaxNum: BULK_UPLOAD_MAX_NUM, - ogHostname: OG_HOST_NAME, - })) - const { BulkService } = require('..') - const service = new BulkService() + beforeEach(() => { + bulkService = new BulkService() + }) + + describe('parseCsv', () => { + it('should parse valid CSV with empty rows', async () => { + const csvContent = `${BULK_UPLOAD_HEADER}\nhttps://www.data.gov.sg\n\nhttps://www.tech.gov.sg` + const file = { + data: Buffer.from(csvContent), + } as UploadedFile - it('fails if file data string is empty', async () => { - await expect(service.parseCsv({})).rejects.toThrowError() + const result = await bulkService.parseCsv(file) + expect(result).toEqual([ + 'https://www.data.gov.sg', + 'https://www.tech.gov.sg', + ]) }) - it('fails if header does not match BULK_UPLOAD_HEADER', async () => { + it('should reject empty file', async () => { const file = { - data: Buffer.from(`Hello, this is ${BULK_UPLOAD_HEADER}\n`), - name: 'file.csv', + data: Buffer.from(''), } as UploadedFile - await expect(service.parseCsv(file)).rejects.toThrowError() + await expect(bulkService.parseCsv(file)).rejects.toThrow( + 'csv file is empty', + ) }) - validUrlTests.forEach((validUrlTest) => { - it(validUrlTest.testName, async () => { - const file = { - data: Buffer.from(`${BULK_UPLOAD_HEADER}\n${validUrlTest.url}`), - name: 'file.csv', - } as UploadedFile + it('should reject file with invalid header', async () => { + const csvContent = 'Invalid Header\nhttps://www.data.gov.sg' + const file = { + data: Buffer.from(csvContent), + } as UploadedFile - await expect(service.parseCsv(file)).resolves.not.toThrow() - }) + await expect(bulkService.parseCsv(file)).rejects.toThrow( + 'Row 1: bulk upload header is invalid', + ) }) - invalidUrlTests.forEach((invalidUrlTest) => { - it(invalidUrlTest.testName, async () => { - const file = { - data: Buffer.from(`${BULK_UPLOAD_HEADER}\n${invalidUrlTest.url}`), - name: 'file.csv', - } as UploadedFile - - await expect(service.parseCsv(file)).rejects.toThrowError() - }) + + it('should reject file with invalid URL', async () => { + const csvContent = `${BULK_UPLOAD_HEADER}\ninvalid-url` + const file = { + data: Buffer.from(csvContent), + } as UploadedFile + + await expect(bulkService.parseCsv(file)).rejects.toThrow( + 'Row 2: contains invalid url', + ) + }) + + it('should reject file with circular redirect', async () => { + const csvContent = `${BULK_UPLOAD_HEADER}\nhttps://${ogHostname}/redirect` + const file = { + data: Buffer.from(csvContent), + } as UploadedFile + + await expect(bulkService.parseCsv(file)).rejects.toThrow( + 'Row 2: contains circular redirect', + ) + }) + + it('should reject file with multiple columns', async () => { + const csvContent = `${BULK_UPLOAD_HEADER}\nhttps://www.data.gov.sg,extra-column` + const file = { + data: Buffer.from(csvContent), + } as UploadedFile + + await expect(bulkService.parseCsv(file)).rejects.toThrow( + 'Row 2: has more than one column', + ) }) }) - describe('generateUrlMappings tests', () => { - jest.resetModules() - jest.mock('../../../../config', () => ({ - bulkUploadRandomStrLength: BULK_UPLOAD_RANDOM_STR_LENGTH, - })) - const { BulkService } = require('..') - const service = new BulkService() - - it('generateUrlMappings should return shortUrls of specified length', async () => { - const [urlMapping] = await service.generateUrlMappings([ - 'https://google.com', - ]) - expect(urlMapping.shortUrl).toHaveLength(BULK_UPLOAD_RANDOM_STR_LENGTH) + describe('generateUrlMappings', () => { + it('should generate unique short URLs for each long URL', async () => { + const longUrls = [ + 'https://www.data.gov.sg', + 'https://www.tech.gov.sg', + 'https://www.open.gov.sg', + ] + + const result = await bulkService.generateUrlMappings(longUrls) + + expect(result).toHaveLength(3) + result.forEach((mapping) => { + expect(mapping).toHaveProperty('shortUrl') + expect(mapping).toHaveProperty('longUrl') + expect(mapping.shortUrl).toMatch(/^[a-zA-Z0-9]+$/) + }) + + // Check that all short URLs are unique + const shortUrls = result.map((mapping) => mapping.shortUrl) + const uniqueShortUrls = new Set(shortUrls) + expect(uniqueShortUrls.size).toBe(shortUrls.length) + }) + + it('should handle empty array of URLs', async () => { + const result = await bulkService.generateUrlMappings([]) + expect(result).toEqual([]) }) }) })