From ff9203a54fcc88c8c3441c53d951b813ab25fa46 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Fri, 23 Aug 2024 13:56:31 +0100 Subject: [PATCH] fix some bugs in our error transformation --- .../src/import/import-csv.spec.ts | 25 +++++++++++++++---- .../src/import/import-json.spec.ts | 23 ++++++++++++++--- .../src/import/import-utils.ts | 8 +++++- .../src/import/import-writer.spec.ts | 0 .../src/import/import-writer.ts | 23 +++++++++-------- 5 files changed, 58 insertions(+), 21 deletions(-) create mode 100644 packages/compass-import-export/src/import/import-writer.spec.ts diff --git a/packages/compass-import-export/src/import/import-csv.spec.ts b/packages/compass-import-export/src/import/import-csv.spec.ts index 43069baacc3..a97d715ebf0 100644 --- a/packages/compass-import-export/src/import/import-csv.spec.ts +++ b/packages/compass-import-export/src/import/import-csv.spec.ts @@ -630,7 +630,12 @@ describe('importCSV', function () { errorCallback, }); - expect(result.docsWritten).to.equal(1); + expect(omit(result, 'biggestDocSize')).to.deep.equal({ + docsErrored: 2, + docsProcessed: 3, + docsWritten: 1, + hasUnboundArray: false, + }); expect(progressCallback.callCount).to.equal(3); expect(errorCallback.callCount).to.equal(2); @@ -738,26 +743,31 @@ describe('importCSV', function () { errorCallback, }); - expect(result.docsWritten).to.equal(0); + expect(omit(result, 'biggestDocSize')).to.deep.equal({ + docsErrored: 3, + docsProcessed: 3, + docsWritten: 0, + hasUnboundArray: false, + }); expect(progressCallback.callCount).to.equal(3); expect(errorCallback.callCount).to.equal(3); const expectedErrors: ErrorJSON[] = [ { - name: 'WriteConcernError', + name: 'WriteError', message: 'Document failed validation', index: 0, code: 121, }, { - name: 'WriteConcernError', + name: 'WriteError', message: 'Document failed validation', index: 1, code: 121, }, { - name: 'WriteConcernError', + name: 'WriteError', message: 'Document failed validation', index: 2, code: 121, @@ -765,6 +775,11 @@ describe('importCSV', function () { ]; const errors = errorCallback.args.map((args) => args[0]); + for (const [index, error] of errors.entries()) { + expect(error.op).to.exist; + // cheat and copy them over because it is big and with buffers + expectedErrors[index].op = error.op; + } expect(errors).to.deep.equal(expectedErrors); const errorsText = await fs.promises.readFile(output.path, 'utf8'); diff --git a/packages/compass-import-export/src/import/import-json.spec.ts b/packages/compass-import-export/src/import/import-json.spec.ts index 8e92e09a933..a348e6814a9 100644 --- a/packages/compass-import-export/src/import/import-json.spec.ts +++ b/packages/compass-import-export/src/import/import-json.spec.ts @@ -440,7 +440,12 @@ describe('importJSON', function () { errorCallback, }); - expect(result.docsWritten).to.equal(1); + expect(omit(result, 'biggestDocSize')).to.deep.equal({ + docsErrored: 1, + docsProcessed: 2, + docsWritten: 1, + hasUnboundArray: false, + }); expect(progressCallback.callCount).to.equal(2); expect(errorCallback.callCount).to.equal(1); @@ -522,20 +527,25 @@ describe('importJSON', function () { errorCallback, }); - expect(result.docsWritten).to.equal(0); + expect(omit(result, 'biggestDocSize')).to.deep.equal({ + docsErrored: 2, + docsProcessed: 2, + docsWritten: 0, + hasUnboundArray: false, + }); expect(progressCallback.callCount).to.equal(2); expect(errorCallback.callCount).to.equal(2); const expectedErrors: ErrorJSON[] = [ { - name: 'WriteConcernError', + name: 'WriteError', message: 'Document failed validation', index: 0, code: 121, }, { - name: 'WriteConcernError', + name: 'WriteError', message: 'Document failed validation', index: 1, code: 121, @@ -543,6 +553,11 @@ describe('importJSON', function () { ]; const errors = errorCallback.args.map((args) => args[0]); + for (const [index, error] of errors.entries()) { + expect(error.op).to.exist; + // cheat and copy them over because it is big and with buffers + expectedErrors[index].op = error.op; + } expect(errors).to.deep.equal(expectedErrors); const errorsText = await fs.promises.readFile(output.path, 'utf8'); diff --git a/packages/compass-import-export/src/import/import-utils.ts b/packages/compass-import-export/src/import/import-utils.ts index b712f6926dd..4100b94bfaf 100644 --- a/packages/compass-import-export/src/import/import-utils.ts +++ b/packages/compass-import-export/src/import/import-utils.ts @@ -14,11 +14,12 @@ const debug = createDebug('import'); export function makeImportResult( importWriter: ImportWriter, numProcessed: number, + numParseErrors: number, docStatsStream: DocStatsCollector, aborted?: boolean ): ImportResult { const result: ImportResult = { - docsErrored: importWriter.docsErrored, + docsErrored: numParseErrors + importWriter.docsErrored, docsWritten: importWriter.docsWritten, ...docStatsStream.getStats(), // docsProcessed is not on importWriter so that it includes docs that @@ -134,6 +135,7 @@ export async function doImport( const importWriter = new ImportWriter(dataService, ns, stopOnErrors); let numProcessed = 0; + let numParseErrors = 0; // Stream errors just get thrown synchronously unless we listen for the event // on each stream we use in the pipeline. By destroying the stream we're @@ -181,6 +183,7 @@ export async function doImport( try { doc = transformer.transform(chunk); } catch (err: unknown) { + ++numParseErrors; // deal with transform error // rethrow with the line number / array index appended to aid debugging @@ -271,6 +274,7 @@ export async function doImport( const result = makeImportResult( importWriter, numProcessed, + numParseErrors, docStatsCollector, true ); @@ -282,6 +286,7 @@ export async function doImport( err.result = makeImportResult( importWriter, numProcessed, + numParseErrors, docStatsCollector ); @@ -293,6 +298,7 @@ export async function doImport( const result = makeImportResult( importWriter, numProcessed, + numParseErrors, docStatsCollector ); debug('import:completed', result); diff --git a/packages/compass-import-export/src/import/import-writer.spec.ts b/packages/compass-import-export/src/import/import-writer.spec.ts new file mode 100644 index 00000000000..e69de29bb2d diff --git a/packages/compass-import-export/src/import/import-writer.ts b/packages/compass-import-export/src/import/import-writer.ts index 04e595a7617..9f5ceb59eb0 100644 --- a/packages/compass-import-export/src/import/import-writer.ts +++ b/packages/compass-import-export/src/import/import-writer.ts @@ -40,23 +40,24 @@ type ImportWriterProgressError = Error & { errInfo: MongoServerError['errInfo']; }; -function mongodbServerErrorToJSError({ - index, - code, - errmsg, - op, +function writeErrorToJSError({ errInfo, -}: Pick & - Partial< - Pick - >): ImportWriterProgressError { + errmsg, + err, + code, + index, +}: WriteError): ImportWriterProgressError { + const op = err.op; + const e: ImportWriterProgressError = new Error(errmsg) as any; e.index = index; e.code = code; e.op = op; e.errInfo = errInfo; + // https://www.mongodb.com/docs/manual/reference/method/BulkWriteResult/#mongodb-data-BulkWriteResult.writeErrors - e.name = index && op ? 'WriteError' : 'WriteConcernError'; + e.name = index !== undefined && op ? 'WriteError' : 'WriteConcernError'; + return e; } @@ -156,7 +157,7 @@ export class ImportWriter { const bulkOpResult = this._getBulkOpResult(bulkWriteResult); const writeErrors = (bulkWriteResult?.getWriteErrors?.() || []).map( - mongodbServerErrorToJSError + writeErrorToJSError ); this.docsWritten += bulkOpResult.insertedCount;