Rework log level, and simplify cleanup checks

- Also, some test implementation was improved to reduce
  duplication.

Issue: CLDSRV-669

Author: williamlardier

lib/api/apiUtils/object/abortMultipartUpload.js

@@ -78,10 +78,10 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
                     }
                     // for Azure and GCP we do not need to delete data
                     // for all other backends, skipDataDelete will be set to false
-                    return next(null, mpuBucket, mpuOverviewObj, destBucket, objectMD, skipDataDelete);
+                    return next(null, mpuBucket, destBucket, objectMD, skipDataDelete);
                 });
         },
-        function getPartLocations(mpuBucket, mpuOverviewObj, destBucket, objectMD, skipDataDelete,
+        function getPartLocations(mpuBucket, destBucket, objectMD, skipDataDelete,
             next) {
             services.getMPUparts(mpuBucket.getName(), uploadId, log,
                 (err, result) => {
@@ -90,7 +90,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
                         return next(err, destBucket);
                     }
                     const storedParts = result.Contents;
-                    return next(null, mpuBucket, mpuOverviewObj, storedParts, destBucket, objectMD,
+                    return next(null, mpuBucket, storedParts, destBucket, objectMD,
                         skipDataDelete);
                 });
         },
@@ -103,13 +103,8 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
         // to honor the uploadId filter in standardMetadataValidateBucketAndObj
         // ensuring the objMD returned has the right uploadId. But this is not
         // supported by Metadata.
-        function ensureCleanupIsRequired(mpuBucket, mpuOverviewObj, storedParts, destBucket,
+        function ensureCleanupIsRequired(mpuBucket, storedParts, destBucket,
             objectMD, skipDataDelete, next) {
-            // If no overview object, skip - this wasn't a failed completeMPU
-            if (!mpuOverviewObj) {
-                return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete);
-            }
-
             // If objectMD exists and has matching uploadId, use it directly
             // This handles all non-versioned cases, and some versioned cases.
             if (objectMD && objectMD.uploadId === uploadId) {
@@ -235,7 +230,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
             return async.eachLimit(locations, 5, (loc, cb) => {
                 data.delete(loc, log, err => {
                     if (err) {
-                        log.fatal('delete ObjectPart failed', { err });
+                        log.warn('delete ObjectPart failed', { err });
                     }
                     cb();
                 });

tests/functional/aws-node-sdk/test/object/abortMPU.js

@@ -26,7 +26,8 @@ describe('Abort MPU', () => {
             s3 = bucketUtil.s3;
             return s3.createBucket({ Bucket: bucket }).promise()
                 .then(() => s3.createMultipartUpload({
-                    Bucket: bucket, Key: key
+                    Bucket: bucket,
+                    Key: key,
                 }).promise())
                 .then(res => {
                     uploadId = res.UploadId;
@@ -322,28 +323,25 @@ describe('Abort MPU - Versioned Bucket Cleanup', function testSuite() {
                     Key: upload.Key,
                     UploadId: upload.UploadId,
                 }).promise().catch(err => {
-                    if (err.code !== 'NoSuchUpload') throw err;
-                })
+                    if (err.code !== 'NoSuchUpload') {
+                        throw err;
+                    }
+                }),
             ));
 
             // Clean up all object versions
             const listVersionsResponse = await s3.listObjectVersions({ Bucket: bucketName }).promise();
-            await Promise.all([
-                ...listVersionsResponse.Versions.map(version =>
-                    s3.deleteObject({
-                        Bucket: bucketName,
-                        Key: version.Key,
-                        VersionId: version.VersionId,
-                    }).promise()
-                ),
-                ...listVersionsResponse.DeleteMarkers.map(marker =>
-                    s3.deleteObject({
-                        Bucket: bucketName,
-                        Key: marker.Key,
-                        VersionId: marker.VersionId,
-                    }).promise()
-                ),
-            ]);
+            const allObjects = [
+                ...listVersionsResponse.Versions,
+                ...listVersionsResponse.DeleteMarkers,
+            ];
+            await Promise.all(allObjects.map(obj =>
+                s3.deleteObject({
+                    Bucket: bucketName,
+                    Key: obj.Key,
+                    VersionId: obj.VersionId,
+                }).promise()
+            ));
 
             await bucketUtil.deleteOne(bucketName);
         });

tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js

@@ -2,6 +2,7 @@ const assert = require('assert');
 const sinon = require('sinon');
 const { parseString } = require('xml2js');
 const { errors } = require('arsenal');
+const async = require('async');
 const crypto = require('crypto');
 
 const abortMultipartUpload = require('../../../../../lib/api/apiUtils/object/abortMultipartUpload');
@@ -75,48 +76,24 @@ describe('abortMultipartUpload', () => {
 
     // Helper to create bucket and MPU, returns uploadId
     function createBucketAndMPU(versioned, callback) {
+        const tasks = [
+            next => bucketPut(authInfo, bucketRequest, log, err => next(err)),
+        ];
+
         if (versioned) {
-            bucketPut(authInfo, bucketRequest, log, err => {
-                if (err) {
-                    return callback(err);
-                }
-                return bucketPutVersioning(authInfo, enableVersioningRequest, log, err => {
-                    if (err) {
-                        return callback(err);
-                    }
-                    return initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => {
-                        if (err) {
-                            return callback(err);
-                        }
-                        return parseString(result, (err, json) => {
-                            if (err) {
-                                return callback(err);
-                            }
-                            const uploadId = json.InitiateMultipartUploadResult.UploadId[0];
-                            return callback(null, uploadId);
-                        });
-                    });
-                });
-            });
-        } else {
-            bucketPut(authInfo, bucketRequest, log, err => {
-                if (err) {
-                    return callback(err);
-                }
-                return initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => {
-                    if (err) {
-                        return callback(err);
-                    }
-                    return parseString(result, (err, json) => {
-                        if (err) {
-                            return callback(err);
-                        }
-                        const uploadId = json.InitiateMultipartUploadResult.UploadId[0];
-                        return callback(null, uploadId);
-                    });
-                });
-            });
+            tasks.push(next => bucketPutVersioning(authInfo, enableVersioningRequest, log, err => next(err)));
         }
+
+        tasks.push(
+            next => initiateMultipartUpload(authInfo, initiateRequest, log, (err, result) => next(err, result)),
+            (result, next) => parseString(result, (err, json) => next(err, json)),
+            (json, next) => {
+                const uploadId = json.InitiateMultipartUploadResult.UploadId[0];
+                next(null, uploadId);
+            }
+        );
+
+        async.waterfall(tasks, callback);
     }
 
     describe('basic functionality', () => {
@@ -202,6 +179,12 @@ describe('abortMultipartUpload', () => {
                         assert.strictEqual(err, null);
                         sinon.assert.calledOnce(dataAbortMPUStub);
                         sinon.assert.called(dataDeleteStub); // Should delete part data
+                        // ensure the right part was deleted - data.delete is called with location object
+                        const firstCall = dataDeleteStub.getCalls()[0];
+                        const locationArg = firstCall.args[0];
+                        assert(typeof locationArg === 'object', 'First argument should be location object');
+                        assert(locationArg.dataStoreName, 'Location should have dataStoreName');
+                        assert(locationArg.key, 'Location should have key');
                         done();
                     }, abortRequest);
                 });