Merge remote-tracking branch 'origin/production' into staging

huridocs · Feb 13, 2025 · 8a91eae · 8a91eae
2 parents edb5b87 + 4959207
commit 8a91eae
Show file tree

Hide file tree

Showing 5 changed files with 108 additions and 46 deletions.
diff --git a/app/api/files/S3Storage.ts b/app/api/files/S3Storage.ts
@@ -4,24 +4,30 @@ import {
   ListObjectsV2Command,
   PutObjectCommand,
   S3Client,
+  S3ServiceException,
   _Object,
 } from '@aws-sdk/client-s3';
 import { config } from 'api/config';
 
-class S3TimeoutError extends Error {
-  constructor(cause: Error) {
+class S3Error extends Error {
+  constructor(cause: S3ServiceException) {
     super(cause.message, { cause });
   }
+
+  get originalError() {
+    return this.cause as S3ServiceException;
+  }
+
+  get httpStatusCode() {
+    return this.originalError.$metadata.httpStatusCode;
+  }
 }
 
 const catchS3Errors = async <T>(cb: () => Promise<T>): Promise<T> => {
   try {
     return await cb();
   } catch (err) {
-    if (err.name === 'TimeoutError') {
-      throw new S3TimeoutError(err);
-    }
-    throw err;
+    throw new S3Error(err);
   }
 };
 
@@ -92,4 +98,4 @@ export class S3Storage {
   }
 }
 
-export { S3TimeoutError };
+export { S3Error };
diff --git a/app/api/files/specs/s3Storage.spec.ts b/app/api/files/specs/s3Storage.spec.ts
@@ -1,45 +1,81 @@
-import { S3Storage, S3TimeoutError } from '../S3Storage';
+// eslint-disable-next-line max-classes-per-file
+import { S3Client } from '@aws-sdk/client-s3';
+import { S3Storage, S3Error } from '../S3Storage';
 
-let s3Storage: S3Storage;
+const expectedMetadata = {
+  requestId: 'mock-request-123',
+  cfId: 'mock-cf-456',
+  httpStatusCode: 500,
+  attempts: 3,
+  totalRetryDelay: 1000,
+};
 
-class S3TimeoutClient {
+class MockS3Error extends Error {
+  $metadata = expectedMetadata;
+
+  constructor() {
+    super('Mock S3 Error');
+    this.name = 'S3ServiceError';
+  }
+}
+
+class MockS3Client {
   // eslint-disable-next-line class-methods-use-this
   send() {
-    const error = new Error();
-    error.name = 'TimeoutError';
-    throw error;
+    throw new MockS3Error();
   }
 }
 
-describe('s3Storage', () => {
-  beforeAll(async () => {
-    // @ts-ignore
-    s3Storage = new S3Storage(new S3TimeoutClient());
+describe('s3Storage error handling', () => {
+  let s3Storage: S3Storage;
+
+  beforeEach(() => {
+    s3Storage = new S3Storage(new MockS3Client() as unknown as S3Client);
   });
 
-  describe('get', () => {
-    it('should throw S3TimeoutError on timeout', async () => {
-      await expect(s3Storage.get('dummy_key')).rejects.toBeInstanceOf(S3TimeoutError);
+  describe('error wrapping', () => {
+    it('should wrap S3 errors with metadata for get operation', async () => {
+      try {
+        await s3Storage.get('some_key');
+        fail('Should have thrown an error');
+      } catch (error) {
+        expect(error).toBeInstanceOf(S3Error);
+        expect(error.originalError.$metadata).toEqual(expectedMetadata);
+        expect(error.httpStatusCode).toBe(500);
+      }
     });
-  });
 
-  describe('upload', () => {
-    it('should throw S3TimeoutError on timeout', async () => {
-      await expect(
-        s3Storage.upload('dummy_key', Buffer.from('dummy buffer', 'utf-8'))
-      ).rejects.toBeInstanceOf(S3TimeoutError);
+    it('should wrap S3 errors with metadata for upload operation', async () => {
+      try {
+        await s3Storage.upload('some_key', Buffer.from('test'));
+        fail('Should have thrown an error');
+      } catch (error) {
+        expect(error).toBeInstanceOf(S3Error);
+        expect(error.originalError.$metadata).toEqual(expectedMetadata);
+        expect(error.httpStatusCode).toBe(500);
+      }
     });
-  });
 
-  describe('delete', () => {
-    it('should throw S3TimeoutError on timeout', async () => {
-      await expect(s3Storage.delete('dummy_key')).rejects.toBeInstanceOf(S3TimeoutError);
+    it('should wrap S3 errors with metadata for delete operation', async () => {
+      try {
+        await s3Storage.delete('some_key');
+        fail('Should have thrown an error');
+      } catch (error) {
+        expect(error).toBeInstanceOf(S3Error);
+        expect(error.originalError.$metadata).toEqual(expectedMetadata);
+        expect(error.httpStatusCode).toBe(500);
+      }
     });
-  });
 
-  describe('list', () => {
-    it('should throw S3TimeoutError on timeout', async () => {
-      await expect(s3Storage.list()).rejects.toBeInstanceOf(S3TimeoutError);
+    it('should wrap S3 errors with metadata for list operation', async () => {
+      try {
+        await s3Storage.list('some_prefix');
+        fail('Should have thrown an error');
+      } catch (error) {
+        expect(error).toBeInstanceOf(S3Error);
+        expect(error.originalError.$metadata).toEqual(expectedMetadata);
+        expect(error.httpStatusCode).toBe(500);
+      }
     });
   });
 });
diff --git a/app/api/files/storage.ts b/app/api/files/storage.ts
@@ -23,7 +23,7 @@ import {
   deleteFile,
   uploadsPath,
 } from './filesystem';
-import { S3Storage } from './S3Storage';
+import { S3Error, S3Storage } from './S3Storage';
 
 export type FileTypes = NonNullable<FileType['type']> | 'activitylog' | 'segmentation';
 
@@ -81,7 +81,10 @@ const catchFileNotFound = async <T>(cb: () => Promise<T>, filename: string): Pro
   try {
     return await cb();
   } catch (err) {
-    if (err?.code === 'ENOENT' || err instanceof NoSuchKey) {
+    if (
+      err?.code === 'ENOENT' ||
+      (err instanceof S3Error && err.originalError instanceof NoSuchKey)
+    ) {
       throw new FileNotFound(filename, storageType);
     }
     throw err;
@@ -126,7 +129,10 @@ export const storage = {
         await access(paths[type](filename));
       }
     } catch (err) {
-      if (err?.code === 'ENOENT' || err instanceof NoSuchKey) {
+      if (
+        err?.code === 'ENOENT' ||
+        (err instanceof S3Error && err.originalError instanceof NoSuchKey)
+      ) {
         return false;
       }
       if (err) {

diff --git a/app/api/utils/handleError.js b/app/api/utils/handleError.js
@@ -2,7 +2,7 @@ import Ajv from 'ajv';
 import { UnauthorizedError } from 'api/authorization.v2/errors/UnauthorizedError';
 import { ValidationError } from 'api/common.v2/validation/ValidationError';
 import { FileNotFound } from 'api/files/FileNotFound';
-import { S3TimeoutError } from 'api/files/S3Storage';
+import { S3Error } from 'api/files/S3Storage';
 import { legacyLogger } from 'api/log';
 import { appContext } from 'api/utils/AppContext';
 import { createError } from 'api/utils/index';
@@ -64,8 +64,8 @@ const prettifyError = (error, { req = {}, uncaught = false } = {}) => {
     result = { code: 500, message: util.inspect(error), logLevel: 'error' };
   }
 
-  if (error instanceof S3TimeoutError) {
-    result = { code: 408, message: util.inspect(error), logLevel: 'debug' };
+  if (error instanceof S3Error) {
+    result = { code: error.httpStatusCode || 503, message: util.inspect(error), logLevel: 'debug' };
   }
 
   if (error instanceof Ajv.ValidationError) {

diff --git a/app/api/utils/specs/handleError.spec.js b/app/api/utils/specs/handleError.spec.js
@@ -2,7 +2,7 @@ import { legacyLogger } from 'api/log';
 import { createError } from 'api/utils';
 
 import { errors as elasticErrors } from '@elastic/elasticsearch';
-import { S3TimeoutError } from 'api/files/S3Storage';
+import { S3Error } from 'api/files/S3Storage';
 import { appContext } from 'api/utils/AppContext';
 import util from 'node:util';
 import { handleError, prettifyError } from '../handleError';
@@ -20,17 +20,31 @@ describe('handleError', () => {
   });
 
   describe('errors by type', () => {
-    describe('and is instance of S3TimeoutError', () => {
-      it('should be a debug logLevel and a 408 http code', () => {
-        const errorInstance = new S3TimeoutError(new Error('timeout'));
+    describe('and is instance of S3Error', () => {
+      it('should be a debug logLevel and use the error httpStatusCode', () => {
+        const originalError = new Error('original error');
+        originalError.$metadata = { httpStatusCode: 404 };
+        const errorInstance = new S3Error(originalError);
         const error = handleError(errorInstance);
         expect(error).toMatchObject({
-          code: 408,
+          code: 404,
           logLevel: 'debug',
         });
-        expect(legacyLogger.debug.mock.calls[0][0]).toContain('timeout');
+        expect(legacyLogger.debug.mock.calls[0][0]).toContain('original error');
+      });
+      it('should use 503 status code if httpStatusCode is undefined', () => {
+        const originalError = new Error('original error');
+        originalError.$metadata = {};
+        const errorInstance = new S3Error(originalError);
+        const error = handleError(errorInstance);
+        expect(error).toMatchObject({
+          code: 503,
+          logLevel: 'debug',
+        });
+        expect(legacyLogger.debug.mock.calls[0][0]).toContain('original error');
       });
     });
+
     describe('when error is instance of Error', () => {
       it('should return the error with 500 code without the original error and error stack', () => {
         const errorInstance = new Error('error');