Improved error handling for batch inserted member records (#12146)

no issue

- When batch insert fails handling should be more granular and aim to retry and insert as many records from the batch as possible.  
- Added retry logic for failed member's batch inserts. It's a sequential insert for each record in the batch. This implementation was chosen to keep it as simple as possible
- Added filtering of "toCreate" records when member fails to insert. We should not try inserting related members_labels/members_stripe_customers/members_stripe_customer_subscriptions records because they would definitely fail insertion without associated member record

core/server/api/canary/members.js

@@ -304,8 +304,8 @@ module.exports = {
             } catch (error) {
                 if (error.code && error.message.toLowerCase().indexOf('unique') !== -1) {
                     throw new errors.ValidationError({
-                        message: i18n.t('errors.api.members.memberAlreadyExists.message'),
+                        message: i18n.t('errors.models.member.memberAlreadyExists.message'),
-                        context: i18n.t('errors.api.members.memberAlreadyExists.context')
+                        context: i18n.t('errors.models.member.memberAlreadyExists.context')
                     });
                 }
 

core/server/models/member.js

@@ -1,6 +1,8 @@
 const ghostBookshelf = require('./base');
 const uuid = require('uuid');
 const _ = require('lodash');
+const {i18n} = require('../lib/common');
+const errors = require('@tryghost/errors');
 const {sequence} = require('@tryghost/promise');
 const config = require('../../shared/config');
 const crypto = require('crypto');
@@ -259,6 +261,36 @@ const Member = ghostBookshelf.Model.extend({
         return options;
     },
 
+    async insertChunkSequential(chunk, result, unfilteredOptions) {
+        for (const member of chunk) {
+            try {
+                await (unfilteredOptions.transacting || ghostBookshelf.knex)(this.prototype.tableName).insert(member);
+                result.successful += 1;
+            } catch (err) {
+                if (err.code === 'ER_DUP_ENTRY') {
+                    result.errors.push(new errors.ValidationError({
+                        message: i18n.t('errors.models.member.memberAlreadyExists.message'),
+                        context: i18n.t('errors.models.member.memberAlreadyExists.context')
+                    }));
+                } else {
+                    result.errors.push(err);
+                }
+
+                result.unsuccessfulIds.push(member.id);
+                result.unsuccessful += 1;
+            }
+        }
+    },
+
+    async insertChunk(chunk, result, unfilteredOptions) {
+        try {
+            await (unfilteredOptions.transacting || ghostBookshelf.knex)(this.prototype.tableName).insert(chunk);
+            result.successful += chunk.length;
+        } catch (err) {
+            await this.insertChunkSequential(chunk, result, unfilteredOptions);
+        }
+    },
+
     async bulkAdd(data, unfilteredOptions = {}) {
         if (!unfilteredOptions.transacting) {
             return ghostBookshelf.transaction((transacting) => {
@@ -268,20 +300,16 @@ const Member = ghostBookshelf.Model.extend({
         const result = {
             successful: 0,
             unsuccessful: 0,
+            unsuccessfulIds: [],
             errors: []
         };
 
         const CHUNK_SIZE = 100;
 
         for (const chunk of _.chunk(data, CHUNK_SIZE)) {
-            try {
+            await this.insertChunk(chunk, result, unfilteredOptions);
-                await ghostBookshelf.knex(this.prototype.tableName).insert(chunk);
-                result.successful += chunk.length;
-            } catch (err) {
-                result.unsuccessful += chunk.length;
-                result.errors.push(err);
-            }
         }
+
         return result;
     },
 

core/server/services/members/importer/index.js

@@ -16,7 +16,7 @@ const doImport = async ({members, allLabelModels, importSetLabels, createdBy}) =
     const deleteMembers = createDeleter('members');
     const insertLabelAssociations = createInserter('members_labels');
 
-    const {
+    let {
         invalidMembers,
         membersToInsert,
         stripeCustomersToFetch,
@@ -24,21 +24,36 @@ const doImport = async ({members, allLabelModels, importSetLabels, createdBy}) =
         labelAssociationsToInsert
     } = getMemberData({members, allLabelModels, importSetLabels, createdBy});
 
+    // NOTE: member insertion has to happen before the rest of insertions to handle validation
+    //       errors - remove failed members from label/stripe sets
+    const insertedMembers = await models.Member.bulkAdd(membersToInsert).then((insertResult) => {
+        if (insertResult.unsuccessfulIds.length) {
+            labelAssociationsToInsert = labelAssociationsToInsert
+                .filter(la => !insertResult.unsuccessfulIds.includes(la.member_id));
+
+            stripeCustomersToFetch = stripeCustomersToFetch
+                .filter(sc => !insertResult.unsuccessfulIds.includes(sc.member_id));
+
+            stripeCustomersToCreate = stripeCustomersToCreate
+                .filter(sc => !insertResult.unsuccessfulIds.includes(sc.member_id));
+        }
+
+        return insertResult;
+    });
+
     const fetchedStripeCustomersPromise = fetchStripeCustomers(stripeCustomersToFetch);
     const createdStripeCustomersPromise = createStripeCustomers(stripeCustomersToCreate);
-    const insertedMembersPromise = models.Member.bulkAdd(membersToInsert);
+    const insertedLabelsPromise = insertLabelAssociations(labelAssociationsToInsert);
-
-    const insertedLabelsPromise = insertedMembersPromise
-        .then(() => insertLabelAssociations(labelAssociationsToInsert));
 
     const insertedCustomersPromise = Promise.all([
         fetchedStripeCustomersPromise,
-        createdStripeCustomersPromise,
+        createdStripeCustomersPromise
-        insertedMembersPromise
     ]).then(
-        ([fetchedStripeCustomers, createdStripeCustomers]) => models.MemberStripeCustomer.bulkAdd(
+        ([fetchedStripeCustomers, createdStripeCustomers]) => {
-            fetchedStripeCustomers.customersToInsert.concat(createdStripeCustomers.customersToInsert)
+            return models.MemberStripeCustomer.bulkAdd(
-        )
+                fetchedStripeCustomers.customersToInsert.concat(createdStripeCustomers.customersToInsert)
+            );
+        }
     );
 
     const insertedSubscriptionsPromise = Promise.all([
@@ -53,8 +68,7 @@ const doImport = async ({members, allLabelModels, importSetLabels, createdBy}) =
 
     const deletedMembersPromise = Promise.all([
         fetchedStripeCustomersPromise,
-        createdStripeCustomersPromise,
+        createdStripeCustomersPromise
-        insertedMembersPromise
     ]).then(
         ([fetchedStripeCustomers, createdStripeCustomers]) => deleteMembers(
             fetchedStripeCustomers.membersToDelete.concat(createdStripeCustomers.membersToDelete)
@@ -64,7 +78,6 @@ const doImport = async ({members, allLabelModels, importSetLabels, createdBy}) =
     // This looks sequential, but at the point insertedCustomersPromise has resolved so have all the others
     const insertedSubscriptions = await insertedSubscriptionsPromise;
     const insertedCustomers = await insertedCustomersPromise;
-    const insertedMembers = await insertedMembersPromise;
     const deletedMembers = await deletedMembersPromise;
     const fetchedCustomers = await fetchedStripeCustomersPromise;
     const insertedLabels = await insertedLabelsPromise;

core/server/translations/en.json

@@ -247,6 +247,12 @@
                 "emailNotFound": "Email not found.",
                 "retryNotAllowed": "Only failed emails can be retried"
             },
+            "member": {
+                "memberAlreadyExists": {
+                    "message": "Member already exists",
+                    "context": "Attempting to add member with existing email address."
+                }
+            },
             "base": {
                 "index": {
                     "missingContext": "missing context"
@@ -368,10 +374,6 @@
             },
             "members": {
                 "memberNotFound": "Member not found.",
-                "memberAlreadyExists": {
-                    "message": "Member already exists",
-                    "context": "Attempting to add member with existing email address."
-                },
                 "stripeNotConnected": {
                     "message": "Missing Stripe connection",
                     "context": "Attempting to import members with Stripe data when there is no Stripe account connected",

test/regression/api/canary/admin/members_spec.js

@@ -488,6 +488,31 @@ describe('Members API', function () {
             });
     });
 
+    it('Fails to import memmber duplicate emails', function () {
+        return request
+            .post(localUtils.API.getApiQuery(`members/upload/`))
+            .attach('membersfile', path.join(__dirname, '/../../../../utils/fixtures/csv/members-duplicate-emails.csv'))
+            .set('Origin', config.get('url'))
+            .expect('Content-Type', /json/)
+            .expect('Cache-Control', testUtils.cacheRules.private)
+            .expect(201)
+            .then((res) => {
+                should.not.exist(res.headers['x-cache-invalidate']);
+                const jsonResponse = res.body;
+
+                should.exist(jsonResponse);
+                should.exist(jsonResponse.meta);
+                should.exist(jsonResponse.meta.stats);
+
+                jsonResponse.meta.stats.imported.count.should.equal(1);
+                jsonResponse.meta.stats.invalid.count.should.equal(1);
+
+                should.equal(jsonResponse.meta.stats.invalid.errors.length, 1);
+                jsonResponse.meta.stats.invalid.errors[0].message.should.equal('Member already exists');
+                jsonResponse.meta.stats.invalid.errors[0].count.should.equal(1);
+            });
+    });
+
     it('Can fetch stats with no ?days param', function () {
         return request
             .get(localUtils.API.getApiQuery('members/stats/'))
@@ -507,8 +532,8 @@ describe('Members API', function () {
                 should.exist(jsonResponse.total_on_date);
                 should.exist(jsonResponse.new_today);
 
-                // 3 from fixtures and 5 imported in previous tests
+                // 3 from fixtures and 6 imported in previous tests
-                jsonResponse.total.should.equal(8);
+                jsonResponse.total.should.equal(9);
             });
     });
 
@@ -531,8 +556,8 @@ describe('Members API', function () {
                 should.exist(jsonResponse.total_on_date);
                 should.exist(jsonResponse.new_today);
 
-                // 3 from fixtures and 5 imported in previous tests
+                // 3 from fixtures and 6 imported in previous tests
-                jsonResponse.total.should.equal(8);
+                jsonResponse.total.should.equal(9);
             });
     });
 
@@ -555,8 +580,8 @@ describe('Members API', function () {
                 should.exist(jsonResponse.total_on_date);
                 should.exist(jsonResponse.new_today);
 
-                // 3 from fixtures and 5 imported in previous tests
+                // 3 from fixtures and 6 imported in previous tests
-                jsonResponse.total.should.equal(8);
+                jsonResponse.total.should.equal(9);
             });
     });
 

test/utils/fixtures/csv/members-duplicate-emails.csv

@@ -0,0 +1,3 @@
+email
+duplicate@example.com,
+duplicate@example.com,