Merge pull request #6084 from Shopify/07-09-update_error_message_structure

Update error message structure

Author: odacrem

packages/store/src/services/store/errors/errors.test.ts

@@ -8,7 +8,7 @@ describe('ValidationError', () => {
     expect(error).toBeInstanceOf(ValidationError)
     expect(error.code).toBe(ErrorCodes.FILE_NOT_FOUND)
     expect(error.params).toEqual({filePath: '/test/file.sqlite'})
-    expect(error.message).toBe('File not found: /test/file.sqlite')
+    expect(error.message).toBe('File "/test/file.sqlite" not found.')
   })
 
   test('should create error with code only', () => {
@@ -26,7 +26,7 @@ describe('ValidationError', () => {
     expect(error.code).toBe(ErrorCodes.SHOP_NOT_FOUND)
     expect(error.params).toEqual({shop: 'test-shop.myshopify.com'})
     expect(error.message).toBe(
-      'Shop (test-shop.myshopify.com) not found in any of the Early Access enabled organizations you have access to.',
+      'Shop "test-shop.myshopify.com" not found in any of the Early Access enabled organizations you have access to.',
     )
   })
 })
@@ -62,11 +62,11 @@ describe('OperationError', () => {
 describe('Error message generation', () => {
   test('should generate correct messages for all error codes', () => {
     // Validation errors
-    expect(new ValidationError(ErrorCodes.FILE_TOO_LARGE, {sizeGB: 6}).message).toBe(
-      'File is too large (6GB). Maximum size is 5GB.',
-    )
-    expect(new ValidationError(ErrorCodes.EMPTY_FILE, {filePath: '/test.db'}).message).toBe('File is empty: /test.db')
-    expect(new ValidationError(ErrorCodes.NOT_A_FILE, {filePath: '/test'}).message).toBe('Path is not a file: /test')
+    expect(
+      new ValidationError(ErrorCodes.FILE_TOO_LARGE, {filePath: '/test.db', fileSize: '5MB', maxSize: '4MB'}).message,
+    ).toBe('File "/test.db" (5MB) exceeds maximum size of 4MB.')
+    expect(new ValidationError(ErrorCodes.EMPTY_FILE, {filePath: '/test.db'}).message).toBe('File "/test.db" is empty.')
+    expect(new ValidationError(ErrorCodes.NOT_A_FILE, {filePath: '/test'}).message).toBe('Path "/test" is not a file.')
     expect(new ValidationError(ErrorCodes.DIFFERENT_ORG).message).toBe(
       'Source and target shops must be in the same organization.',
     )

packages/store/src/services/store/errors/errors.ts

@@ -61,23 +61,23 @@ function generateErrorMessage(code: string, params?: ErrorParams): string {
   switch (code) {
     // Validation errors
     case ErrorCodes.SHOP_NOT_FOUND:
-      return `Shop (${params?.shop}) not found in any of the Early Access enabled organizations you have access to.`
+      return `Shop "${params?.shop}" not found in any of the Early Access enabled organizations you have access to.`
     case ErrorCodes.FILE_NOT_FOUND:
-      return `File not found: ${params?.filePath}`
+      return `File "${params?.filePath}" not found.`
     case ErrorCodes.INVALID_FILE_FORMAT:
       return params?.filePath
-        ? `File does not appear to be a valid SQLite database: ${params.filePath}`
+        ? `File "${params.filePath}" does not appear to be a valid SQLite database.`
         : 'Invalid file format'
     case ErrorCodes.SAME_SHOP:
       return 'Source and target shops must not be the same.'
     case ErrorCodes.DIFFERENT_ORG:
       return 'Source and target shops must be in the same organization.'
     case ErrorCodes.FILE_TOO_LARGE:
-      return `File is too large (${params?.sizeGB}GB). Maximum size is 5GB.`
+      return `File "${params?.filePath}" (${params?.fileSize}) exceeds maximum size of ${params?.maxSize}.`
     case ErrorCodes.EMPTY_FILE:
-      return `File is empty: ${params?.filePath}`
+      return `File "${params?.filePath}" is empty.`
     case ErrorCodes.NOT_A_FILE:
-      return `Path is not a file: ${params?.filePath}`
+      return `Path "${params?.filePath}" is not a file.`
     case ErrorCodes.INVALID_KEY_FORMAT:
       return `Key format "${params?.key}" is invalid.\nBuilt-in fields can be specified as <object_type>:<key>\n\nID metafields can be used as key by specifying\n<object_type>:metafield:<metafield_namespace>:<metafield_key>.`
     case ErrorCodes.KEY_DOES_NOT_EXIST:

packages/store/src/services/store/utils/file-uploader.test.ts

@@ -3,11 +3,12 @@ import {createStagedUploadAdmin} from '../../../apis/admin/index.js'
 import {ValidationError, OperationError, ErrorCodes} from '../errors/errors.js'
 import {fetch} from '@shopify/cli-kit/node/http'
 import {describe, test, expect, vi, beforeEach} from 'vitest'
-import {readFileSync, statSync} from 'node:fs'
+import {isDirectory, readFileSync, fileSize, fileExistsSync} from '@shopify/cli-kit/node/fs'
 
 vi.mock('../../../apis/admin/index.js')
 vi.mock('@shopify/cli-kit/node/http')
 vi.mock('node:fs')
+vi.mock('@shopify/cli-kit/node/fs')
 
 describe('FileUploader', () => {
   let fileUploader: FileUploader
@@ -20,10 +21,6 @@ describe('FileUploader', () => {
 
   describe('uploadSqliteFile', () => {
     const mockFileBuffer = Buffer.from('SQLite format 3\0test data')
-    const mockFileStats = {
-      isFile: () => true,
-      size: 1024,
-    }
     const mockStagedUploadResponse = {
       stagedUploadsCreate: {
         stagedTargets: [
@@ -41,8 +38,10 @@ describe('FileUploader', () => {
     }
 
     beforeEach(() => {
-      vi.mocked(statSync).mockReturnValue(mockFileStats as any)
       vi.mocked(readFileSync).mockReturnValue(mockFileBuffer)
+      vi.mocked(fileExistsSync).mockReturnValue(true)
+      vi.mocked(fileSize).mockResolvedValue(1024)
+      vi.mocked(isDirectory).mockResolvedValue(false)
       vi.mocked(createStagedUploadAdmin).mockResolvedValue(mockStagedUploadResponse)
       vi.mocked(fetch).mockResolvedValue({
         ok: true,
@@ -54,7 +53,6 @@ describe('FileUploader', () => {
       const result = await fileUploader.uploadSqliteFile(mockFilePath, mockStoreFqdn)
 
       expect(result).toBe('https://shopify.com/resource/123')
-      expect(statSync).toHaveBeenCalledWith(mockFilePath)
       expect(readFileSync).toHaveBeenCalledWith(mockFilePath)
       expect(createStagedUploadAdmin).toHaveBeenCalledWith(mockStoreFqdn, [
         {
@@ -72,9 +70,7 @@ describe('FileUploader', () => {
     })
 
     test('should throw error when file does not exist', async () => {
-      vi.mocked(statSync).mockImplementation(() => {
-        throw new Error('ENOENT: no such file or directory')
-      })
+      vi.mocked(fileExistsSync).mockReturnValue(false)
 
       const promise = fileUploader.uploadSqliteFile(mockFilePath, mockStoreFqdn)
       await expect(promise).rejects.toThrow(ValidationError)
@@ -85,11 +81,8 @@ describe('FileUploader', () => {
     })
 
     test('should throw error when path is not a file', async () => {
-      vi.mocked(statSync).mockReturnValue({
-        isFile: () => false,
-        size: 1024,
-      } as any)
-
+      vi.mocked(fileSize).mockResolvedValue(1024)
+      vi.mocked(isDirectory).mockResolvedValue(true)
       const promise = fileUploader.uploadSqliteFile(mockFilePath, mockStoreFqdn)
       await expect(promise).rejects.toThrow(ValidationError)
       await expect(promise).rejects.toMatchObject({
@@ -99,10 +92,7 @@ describe('FileUploader', () => {
     })
 
     test('should throw error when file is empty', async () => {
-      vi.mocked(statSync).mockReturnValue({
-        isFile: () => true,
-        size: 0,
-      } as any)
+      vi.mocked(fileSize).mockResolvedValue(0)
 
       const promise = fileUploader.uploadSqliteFile(mockFilePath, mockStoreFqdn)
       await expect(promise).rejects.toThrow(ValidationError)
@@ -113,18 +103,14 @@ describe('FileUploader', () => {
     })
 
     test('should throw error when file is too large', async () => {
-      // 6GB
-      const largeSize = 6 * 1024 * 1024 * 1024
-      vi.mocked(statSync).mockReturnValue({
-        isFile: () => true,
-        size: largeSize,
-      } as any)
+      const largeSize = 30 * 1024 * 1024
+      vi.mocked(fileSize).mockResolvedValue(largeSize)
 
       const promise = fileUploader.uploadSqliteFile(mockFilePath, mockStoreFqdn)
       await expect(promise).rejects.toThrow(ValidationError)
       await expect(promise).rejects.toMatchObject({
         code: ErrorCodes.FILE_TOO_LARGE,
-        params: {sizeGB: 6},
+        params: {filePath: mockFilePath, fileSize: '30MB', maxSize: '20MB'},
       })
     })
 
@@ -187,14 +173,5 @@ describe('FileUploader', () => {
         params: {details: '403 Forbidden. Access denied'},
       })
     })
-
-    test('should handle non-Error exceptions in validateSqliteFile', async () => {
-      vi.mocked(statSync).mockImplementation(() => {
-        // eslint-disable-next-line @typescript-eslint/only-throw-error
-        throw 'string error'
-      })
-
-      await expect(fileUploader.uploadSqliteFile(mockFilePath, mockStoreFqdn)).rejects.toThrow(ValidationError)
-    })
   })
 })

packages/store/src/services/store/utils/file-uploader.ts

@@ -1,21 +1,23 @@
 import {StagedUploadInput, createStagedUploadAdmin} from '../../../apis/admin/index.js'
 import {ValidationError, OperationError, ErrorCodes} from '../errors/errors.js'
 import {fetch} from '@shopify/cli-kit/node/http'
-import {readFileSync, statSync} from 'node:fs'
+import {fileExistsSync, fileSize, isDirectory, readFileSync} from '@shopify/cli-kit/node/fs'
+import {outputDebug} from '@shopify/cli-kit/node/output'
 
 export class FileUploader {
+  private readonly MAX_FILE_SIZE = 20 * 1024 * 1024
+
   async uploadSqliteFile(filePath: string, storeFqdn: string): Promise<string> {
-    this.validateSqliteFile(filePath)
+    await this.validateSqliteFile(filePath)
 
-    const fileStats = statSync(filePath)
     const fileBuffer = readFileSync(filePath)
-
+    const sizeOfFile = await fileSize(filePath)
     const uploadInput: StagedUploadInput = {
       resource: 'FILE',
       filename: 'database.sqlite',
       mimeType: 'application/x-sqlite3',
       httpMethod: 'POST',
-      fileSize: fileStats.size.toString(),
+      fileSize: sizeOfFile.toString(),
     }
 
     const stagedUploadResponse = await createStagedUploadAdmin(storeFqdn, [uploadInput])
@@ -64,21 +66,26 @@ export class FileUploader {
     return resourceUrl
   }
 
-  private validateSqliteFile(filePath: string): void {
+  private async validateSqliteFile(filePath: string): Promise<void> {
     try {
-      const stats = statSync(filePath)
-
-      if (!stats.isFile()) {
+      if (!fileExistsSync(filePath)) {
+        throw new ValidationError(ErrorCodes.FILE_NOT_FOUND, {filePath})
+      }
+      if (await isDirectory(filePath)) {
         throw new ValidationError(ErrorCodes.NOT_A_FILE, {filePath})
       }
+      const sizeOfFile = await fileSize(filePath)
+      outputDebug(`Validating SQLite file at ${filePath} with size ${sizeOfFile} bytes`)
 
-      if (stats.size === 0) {
+      if (sizeOfFile === 0) {
         throw new ValidationError(ErrorCodes.EMPTY_FILE, {filePath})
       }
 
-      if (stats.size > 5 * 1024 * 1024 * 1024) {
+      if (sizeOfFile > this.MAX_FILE_SIZE) {
         throw new ValidationError(ErrorCodes.FILE_TOO_LARGE, {
-          sizeGB: Math.round(stats.size / 1024 / 1024 / 1024),
+          filePath,
+          fileSize: `${Math.round(sizeOfFile / 1024 / 1024)}MB`,
+          maxSize: `${Math.round(this.MAX_FILE_SIZE / 1024 / 1024)}MB`,
         })
       }