fix: handle empty rows in bulk upload CSV files and refactor generate…

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 }
         }),
       )
     }

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([])
     })
   })
 })