feat(import): include the error count in the import toast COMPASS-7826 (#5659)

* include the error count in the import toast

* minus the console.log

* add comments

* only appears once now

Author: lerouxb

packages/compass-e2e-tests/tests/collection-import.test.ts

@@ -1158,12 +1158,12 @@ describe('Collection import', function () {
       .$(Selectors.closeToastButton(Selectors.ImportToast))
       .waitForDisplayed();
 
-    // Displays first two errors in the toast and view log.
+    // Displays first error in the toast and view log.
     const toastText = await toastElement.getText();
     expect(toastText).to.include('Import completed 0/3 with errors:');
     expect(
       (toastText.match(/E11000 duplicate key error collection/g) || []).length
-    ).to.equal(2);
+    ).to.equal(1);
     expect(toastText).to.include('VIEW LOG');
 
     const logFilePath = path.resolve(

packages/compass-import-export/src/components/import-toast.tsx

@@ -15,24 +15,32 @@ export function showInProgressToast({
   fileName,
   cancelImport,
   docsWritten,
+  numErrors,
   bytesProcessed,
   bytesTotal,
 }: {
   fileName: string;
   cancelImport: () => void;
   docsWritten: number;
+  numErrors: number;
   bytesProcessed: number;
   bytesTotal: number;
 }) {
   // Update the toast with the new progress.
   const progress = bytesTotal ? bytesProcessed / bytesTotal : undefined;
+
+  let statusMessage = `${docsWritten} document${
+    docsWritten !== 1 ? 's' : ''
+  } written.`;
+  if (numErrors) {
+    statusMessage += ` ${numErrors} error${numErrors !== 1 ? 's' : ''}.`;
+  }
+
   openToast(importToastId, {
     title: `Importing ${path.basename(fileName)}…`,
     description: (
       <ToastBody
-        statusMessage={`${docsWritten} document${
-          docsWritten !== 1 ? 's' : ''
-        } written.`}
+        statusMessage={statusMessage}
         actionHandler={cancelImport}
         actionText="stop"
       />

packages/compass-import-export/src/import/import-csv.spec.ts

@@ -781,48 +781,39 @@ describe('importCSV', function () {
     expect(result.dbStats.insertedCount).to.equal(0);
 
     expect(progressCallback.callCount).to.equal(3);
-    expect(errorCallback.callCount).to.equal(4); // yes one more MongoBulkWriteError than items in the batch
+    expect(errorCallback.callCount).to.equal(1); // once for the batch
 
-    const expectedErrors = [
+    const expectedErrors: ErrorJSON[] = [
       {
-        // first one speems to relate to the batch as there's no index
         name: 'MongoBulkWriteError',
         message: 'Document failed validation',
         code: 121,
-      },
-      {
-        name: 'WriteConcernError',
-        message: 'Document failed validation',
-        index: 0,
-        code: 121,
-      },
-      {
-        name: 'WriteConcernError',
-        message: 'Document failed validation',
-        index: 1,
-        code: 121,
-      },
-      {
-        name: 'WriteConcernError',
-        message: 'Document failed validation',
-        index: 2,
-        code: 121,
+        numErrors: 3,
       },
     ];
 
-    const errorLog = await fs.promises.readFile(output.path, 'utf8');
-    expect(errorLog).to.equal(
-      expectedErrors.map((e) => JSON.stringify(e)).join(os.EOL) + os.EOL
-    );
-
     const errors = errorCallback.args.map((args) => args[0]);
-
-    try {
-      expect(errors).to.deep.equal(expectedErrors);
-    } catch (err: any) {
-      console.log(JSON.stringify({ errors, expectedErrors }, null, 2));
-      throw err;
-    }
+    expect(errors).to.deep.equal(expectedErrors);
+
+    // the log file has one for each error in the bulk write too
+    expectedErrors.push({
+      name: 'WriteConcernError',
+      message: 'Document failed validation',
+      index: 0,
+      code: 121,
+    });
+    expectedErrors.push({
+      name: 'WriteConcernError',
+      message: 'Document failed validation',
+      index: 1,
+      code: 121,
+    });
+    expectedErrors.push({
+      name: 'WriteConcernError',
+      message: 'Document failed validation',
+      index: 2,
+      code: 121,
+    });
 
     const errorsText = await fs.promises.readFile(output.path, 'utf8');
     expect(errorsText).to.equal(formatErrorLines(expectedErrors));

packages/compass-import-export/src/import/import-csv.ts

@@ -131,7 +131,8 @@ export async function importCSV({
   const collectionStream = createCollectionWriteStream(
     dataService,
     ns,
-    stopOnErrors ?? false
+    stopOnErrors ?? false,
+    errorCallback
   );
 
   const parseStream = Papa.parse(Papa.NODE_STREAM_INPUT, {
@@ -166,7 +167,6 @@ export async function importCSV({
       await processWriteStreamErrors({
         collectionStream,
         output,
-        errorCallback,
       });
 
       const result = makeImportResult(
@@ -194,7 +194,6 @@ export async function importCSV({
   await processWriteStreamErrors({
     collectionStream,
     output,
-    errorCallback,
   });
 
   const result = makeImportResult(

packages/compass-import-export/src/import/import-json.spec.ts

@@ -555,33 +555,34 @@ describe('importJSON', function () {
     expect(result.dbStats.insertedCount).to.equal(0);
 
     expect(progressCallback.callCount).to.equal(2);
-    expect(errorCallback.callCount).to.equal(3); // yes one more MongoBulkWriteError than items in the batch
+    expect(errorCallback.callCount).to.equal(1); // once for the batch
 
-    const expectedErrors = [
+    const expectedErrors: ErrorJSON[] = [
       {
-        // first one speems to relate to the batch as there's no index
         name: 'MongoBulkWriteError',
         message: 'Document failed validation',
         code: 121,
-      },
-      {
-        name: 'WriteConcernError',
-        message: 'Document failed validation',
-        index: 0,
-        code: 121,
-      },
-      {
-        name: 'WriteConcernError',
-        message: 'Document failed validation',
-        index: 1,
-        code: 121,
+        numErrors: 2,
       },
     ];
 
     const errors = errorCallback.args.map((args) => args[0]);
-
     expect(errors).to.deep.equal(expectedErrors);
 
+    // the log file has one for each error in the bulk write too
+    expectedErrors.push({
+      name: 'WriteConcernError',
+      message: 'Document failed validation',
+      index: 0,
+      code: 121,
+    });
+    expectedErrors.push({
+      name: 'WriteConcernError',
+      message: 'Document failed validation',
+      index: 1,
+      code: 121,
+    });
+
     const errorsText = await fs.promises.readFile(output.path, 'utf8');
     expect(errorsText).to.equal(formatErrorLines(expectedErrors));
   });

packages/compass-import-export/src/import/import-json.ts

@@ -100,7 +100,8 @@ export async function importJSON({
   const collectionStream = createCollectionWriteStream(
     dataService,
     ns,
-    stopOnErrors ?? false
+    stopOnErrors ?? false,
+    errorCallback
   );
 
   const parserStreams = [];
@@ -132,7 +133,6 @@ export async function importJSON({
       await processWriteStreamErrors({
         collectionStream,
         output,
-        errorCallback,
       });
 
       return makeImportResult(
@@ -156,7 +156,6 @@ export async function importJSON({
   await processWriteStreamErrors({
     collectionStream,
     output,
-    errorCallback,
   });
 
   return makeImportResult(collectionStream, numProcessed, docStatsStream);

packages/compass-import-export/src/import/import-types.ts

@@ -22,6 +22,7 @@ export type ErrorJSON = {
   code?: string | number;
   op?: any;
   errorInfo?: Document;
+  numErrors?: number;
 };
 
 export type ImportProgress = {

packages/compass-import-export/src/import/import-utils.ts

@@ -46,13 +46,18 @@ export function errorToJSON(error: any): ErrorJSON {
     }
   }
 
+  // For bulk write errors we include the number of errors that were in the
+  // batch. So one error actually maps to (potentially) many failed documents.
+  if (error.writeErrors && Array.isArray(error.writeErrors)) {
+    obj.numErrors = error.writeErrors.length;
+  }
+
   return obj;
 }
 
 export async function processWriteStreamErrors({
   collectionStream,
   output,
-  errorCallback,
 }: {
   collectionStream: WritableCollectionStream;
   output?: Writable;
@@ -68,9 +73,9 @@ export async function processWriteStreamErrors({
     .concat(stats.writeConcernErrors);
 
   for (const error of allErrors) {
+    debug('write error', error);
+
     const transformedError = errorToJSON(error);
-    debug('write error', transformedError);
-    errorCallback?.(transformedError);
 
     if (!output) {
       continue;

packages/compass-import-export/src/modules/import.ts

@@ -275,7 +275,12 @@ export const startImport = (): ImportThunkAction<Promise<void>> => {
 
     let promise: Promise<ImportResult>;
 
+    let numErrors = 0;
     const errorCallback = (err: ErrorJSON) => {
+      // For bulk write errors we'll get one callback for the whole batch and
+      // then numErrors is the number of documents that failed for that batch.
+      // Usually but not necessarily the entire batch.
+      numErrors += err.numErrors ?? 1;
       if (errors.length < 5) {
         // Only store the first few errors in memory.
         // The log file tracks all of them.
@@ -295,6 +300,7 @@ export const startImport = (): ImportThunkAction<Promise<void>> => {
       showInProgressToast({
         cancelImport: () => dispatch(cancelImport()),
         docsWritten,
+        numErrors,
         fileName,
         bytesProcessed,
         bytesTotal: fileSize,

packages/compass-import-export/src/utils/collection-stream.ts

@@ -10,6 +10,8 @@ import type {
   BulkWriteResult,
 } from 'mongodb';
 import type { DataService } from 'mongodb-data-service';
+import type { ErrorJSON } from '../import/import-types';
+import { errorToJSON } from '../import/import-utils';
 
 import { createDebug } from './logger';
 
@@ -88,11 +90,13 @@ export class WritableCollectionStream extends Writable {
   _batchCounter: number;
   _stats: CollectionStreamStats;
   _errors: CollectionStreamProgressError[];
+  errorCallback?: (error: ErrorJSON) => void;
 
   constructor(
     dataService: Pick<DataService, 'bulkWrite' | 'insertOne'>,
     ns: string,
-    stopOnErrors: boolean
+    stopOnErrors: boolean,
+    errorCallback?: (error: ErrorJSON) => void
   ) {
     super({ objectMode: true });
     this.dataService = dataService;
@@ -117,6 +121,7 @@ export class WritableCollectionStream extends Writable {
     };
 
     this._errors = [];
+    this.errorCallback = errorCallback;
   }
 
   _write(
@@ -181,7 +186,8 @@ export class WritableCollectionStream extends Writable {
             await this.dataService.insertOne(this.ns, doc);
             insertedCount += 1;
           } catch (insertOneByOneError: any) {
-            this._errors.push(insertOneByOneError as Error);
+            this._errors.push(insertOneByOneError);
+            this.errorCallback?.(errorToJSON(insertOneByOneError));
 
             if (this.stopOnErrors) {
               break;
@@ -195,7 +201,9 @@ export class WritableCollectionStream extends Writable {
         // will not return any result, but server might write some docs and bulk
         // result can still be accessed on the error instance
         result = (bulkWriteError as MongoBulkWriteError).result;
+
         this._errors.push(bulkWriteError);
+        this.errorCallback?.(errorToJSON(bulkWriteError));
       }
     }
 
@@ -281,7 +289,13 @@ export class WritableCollectionStream extends Writable {
 export const createCollectionWriteStream = function (
   dataService: Pick<DataService, 'bulkWrite' | 'insertOne'>,
   ns: string,
-  stopOnErrors: boolean
+  stopOnErrors: boolean,
+  errorCallback?: (error: ErrorJSON) => void
 ) {
-  return new WritableCollectionStream(dataService, ns, stopOnErrors);
+  return new WritableCollectionStream(
+    dataService,
+    ns,
+    stopOnErrors,
+    errorCallback
+  );
 };