fix: when catching exceptions don't rely on $metadata being set

src/storage/object/copy.js

@@ -73,7 +73,7 @@ export const copyFile = async (config, env, daCtx, sourceKey, details, isRename)
     const resp = await client.send(new CopyObjectCommand(input));
     return resp;
   } catch (e) {
-    if (e.$metadata.httpStatusCode === 412) {
+    if (e.$metadata?.httpStatusCode === 412) {
       // Not the happy path - something is at the destination already.
       if (!isRename) {
         // This is a copy so just put the source into the target to keep the history.
@@ -98,7 +98,7 @@ export const copyFile = async (config, env, daCtx, sourceKey, details, isRename)
       const client = new S3Client(config);
       // This is a move so copy to the new location
       return /* await */ client.send(new CopyObjectCommand(input));
-    } else if (e.$metadata.httpStatusCode === 404) {
+    } else if (e.$metadata?.httpStatusCode === 404) {
       return { $metadata: e.$metadata };
     }
     throw e;

src/storage/version/put.js

@@ -48,7 +48,10 @@ export async function putVersion(config, {
     const resp = await client.send(command);
     return { status: resp.$metadata.httpStatusCode };
   } catch (e) {
-    return { status: e.$metadata.httpStatusCode };
+    const status = e.$metadata?.httpStatusCode || 500;
+    // eslint-disable-next-line no-console
+    if (status >= 500) console.error('Fail to put version', e);
+    return { status };
   }
 }
 
@@ -97,10 +100,14 @@ export async function putObjectWithVersion(env, daCtx, update, body, guid) {
         ? { status: 201, metadata: { id: ID } }
         : { status: resp.$metadata.httpStatusCode, metadata: { id: ID } };
     } catch (e) {
-      if (e.$metadata.httpStatusCode === 412) {
+      const status = e.$metadata?.httpStatusCode || 500;
+      if (status === 412) {
         return putObjectWithVersion(env, daCtx, update, body);
       }
-      return { status: e.$metadata.httpStatusCode, metadata: { id: ID } };
+
+      // eslint-disable-next-line no-console
+      if (status >= 500) console.error('Failed to put object (in object with version)', e);
+      return { status, metadata: { id: ID } };
     }
   }
 
@@ -143,10 +150,14 @@ export async function putObjectWithVersion(env, daCtx, update, body, guid) {
 
     return { status: resp.$metadata.httpStatusCode, metadata: { id: ID } };
   } catch (e) {
-    if (e.$metadata.httpStatusCode === 412) {
+    const status = e.$metadata?.httpStatusCode || 500;
+    if (status === 412) {
       return putObjectWithVersion(env, daCtx, update, body);
     }
-    return { status: e.$metadata.httpStatusCode, metadata: { id: ID } };
+
+    // eslint-disable-next-line no-console
+    if (status >= 500) console.error('Failed to version (in object with version)', e);
+    return { status, metadata: { id: ID } };
   }
 }
 

test/storage/version/put.test.js

@@ -71,6 +71,44 @@ describe('Version Put', () => {
     assert.strictEqual(sendCalls[1].input.Metadata.Users, JSON.stringify(mockCtx.users));
   });
 
+  it('Test putObjectWithVersion error', async () => {
+    const getObjectCalls = []
+    const mockGetObject = async (e, u, nb) => {
+      getObjectCalls.push({e, u, nb});
+      return {
+        status: 404,
+        metadata: {}
+      };
+    };
+
+    const sendCalls = [];
+    const mockS3Client = {
+      async send(cmd) {
+        sendCalls.push(cmd);
+        const resp = {
+          $metadata: {
+            httpStatusCode: 510
+          }
+        };
+        throw resp;
+      }
+    };
+
+    const { putObjectWithVersion } = await esmock('../../../src/storage/version/put.js', {
+      '../../../src/storage/object/get.js': {
+        default: mockGetObject
+      },
+      '../../../src/storage/utils/version.js': {
+        ifNoneMatch: () => mockS3Client
+      },
+    });
+
+    const mockEnv = { foo: 'bar' };
+    const mockCtx = { users: [{ email: 'foo@acme.com' }] };
+    const resp = await putObjectWithVersion(mockEnv, mockCtx, 'haha', false);
+    assert.equal(510, resp.status);
+  });
+
   it('Test putObjectWithVersion retry on existing document', async () => {
     const getObjectCalls = []
     const mockGetObject = async (e, u, nb) => {
@@ -138,6 +176,50 @@ describe('Version Put', () => {
     assert.strictEqual(sendCalls[1].input.Metadata.Users, JSON.stringify(mockCtx.users));
   });
 
+  it('Test putObjectWithVersion retry fails on existing document', async () => {
+    const getObjectCalls = []
+    const mockGetObject = async (e, u, nb) => {
+      getObjectCalls.push({e, u, nb});
+      return {
+        status: 200,
+        metadata: {}
+      };
+    };
+
+    const sendCalls = [];
+    const mockS3Client = {
+      async send(cmd) {
+        sendCalls.push(cmd);
+        throw new Error('testing 123');
+      }
+    };
+    const mockS3PutClient = {
+      async send(cmd) {
+        return {
+          $metadata: {
+            httpStatusCode: 200
+          }
+        };
+      }
+    };
+
+    const { putObjectWithVersion } = await esmock('../../../src/storage/version/put.js', {
+      '../../../src/storage/object/get.js': {
+        default: mockGetObject
+      },
+      '../../../src/storage/utils/version.js': {
+        ifMatch: () => mockS3Client,
+        ifNoneMatch: () => mockS3PutClient
+      },
+    });
+
+    const mockEnv = { hi: 'ha' };
+    const mockUpdate = 'hoho';
+    const mockCtx = { users: [{ email: 'blah@acme.com' }] };
+    const resp = await putObjectWithVersion(mockEnv, mockCtx, mockUpdate, true);
+    assert.equal(500, resp.status);
+  });
+
   it('Put Object With Version store content', async () => {
     const mockGetObject = async (e, u, h) => {
       if (!h) {
@@ -574,4 +656,44 @@ describe('Version Put', () => {
     assert.equal('[{"email":"hi@acme.com"}]', input2.Metadata.Users);
     assert(input2.Metadata.Version && input2.Metadata.Version !== 101);
   });
+
+  it('exception without metadata', async () => {
+    const s3client1 = {
+      send: async (c) => {
+        const e = new Error('Test error1');
+        e.$metadata = { httpStatusCode: 418 };
+        throw e;
+      }
+    };
+    const s3client2 = {
+      send: async (c) => {
+        throw new Error('Test error2');
+      }
+    };
+    const s3client3 = {
+      send: async (c) => {
+        const e = new Error('Test error3');
+        e.$metadata = {};
+        throw e;
+      }
+    };
+    let s3Client = null;
+    const mockS3Client = () => s3Client;
+
+    const { putVersion } = await esmock('../../../src/storage/version/put.js', {
+      '../../../src/storage/utils/version.js': {
+        ifNoneMatch: mockS3Client,
+      },
+    });
+
+    s3Client = s3client1;
+    const resp = await putVersion({}, { Body: 'hello'});
+    assert.equal(418, resp.status);
+    s3Client = s3client2;
+    const resp2 = await putVersion({}, { Body: 'hello'});
+    assert.equal(500, resp2.status);
+    s3Client = s3client3;
+    const resp3 = await putVersion({}, { Body: 'hello'});
+    assert.equal(500, resp3.status);
+  });
 });