Skip to content

Commit 0deb391

Browse files
committed
Fixed an import reporting its own fault as the row's error
A row's custom field values are validated before its transaction opens, so by the time they are written there is nothing left in them that can be refused. A write that fails there is the database saying no to us, not to the publisher, and it was being handled as though it were the publisher's: the driver's message, query text and all, went into the error file they open next to their spreadsheet, and nobody who could act on it was told. The failure still fails the row, because half a member is worse than none, but the publisher now reads a sentence about what did not save and the original goes to the error tracker, where a lock timeout or a constraint is something someone can go and look at. ref https://linear.app/ghost/issue/BER-3872
1 parent d68240e commit 0deb391

2 files changed

Lines changed: 28 additions & 1 deletion

File tree

ghost/core/core/server/services/members/import-export/import/importer.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ const messages = {
182182
giftCannotCombineWithImportTier: 'Cannot specify both gift_id and import_tier.',
183183
giftCannotCombineWithComplimentary: 'Cannot specify both gift_id and complimentary_plan.',
184184
giftReassignFailed: 'Failed to reassign gift to member.',
185+
customFieldWriteFailed: 'Failed to save the custom field values for this member.',
185186
};
186187

187188
// Columns whose presence makes a row slow to import (they reach out to Stripe), so
@@ -581,7 +582,16 @@ class MembersCSVImporter {
581582
}
582583

583584
// On the row's transaction, so the values commit or roll back with the member.
584-
await this._customFields.applyWrite(member.id, customFieldPlan, trx);
585+
try {
586+
await this._customFields.applyWrite(member.id, customFieldPlan, trx);
587+
} catch (writeError) {
588+
// planWrite passed every value before the transaction opened, so a failure
589+
// here is ours and not the row's. Operators get the original, which a driver
590+
// will have written a query into; the publisher gets a sentence instead, in a
591+
// file they open next to a spreadsheet.
592+
this._report(writeError);
593+
throw new errors.DataImportError({ message: tpl(messages.customFieldWriteFailed) });
594+
}
585595

586596
await trx.commit();
587597
imported += 1;

ghost/core/test/unit/server/services/members/import-export/import/error-handling.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,23 @@ describe('members import error handling', function () {
243243
assert.deepEqual(h.reported, []);
244244
});
245245

246+
// The one row failure whose fault is known: the values were validated before the
247+
// transaction opened, so nothing left in them can be what threw here.
248+
it('keeps a fault of its own out of the error file and sends it to operators', async function () {
249+
const h = harness([row('first@example.com')]);
250+
const fault = new Error('ER_LOCK_WAIT_TIMEOUT: update `members_custom_field_values` set ...');
251+
h.deps.customFields.applyWrite = async () => {
252+
throw fault;
253+
};
254+
255+
await h.run();
256+
257+
assert.equal(h.onlyReport(), fault);
258+
const attached = h.onlyEmail().attachments[0].content;
259+
assert.match(attached, /Failed to save the custom field values for this member/);
260+
assert.doesNotMatch(attached, /ER_LOCK_WAIT_TIMEOUT|members_custom_field_values/);
261+
});
262+
246263
it('reports an import where every row failed as unsuccessful', async function () {
247264
const h = harness();
248265
h.deps.members.create = async () => {

0 commit comments

Comments
 (0)