refactor(server): Importing products handles errors without throwing

server/e2e/fixtures/product-import.csv

@@ -5,6 +5,7 @@ Perfect Paper Stretcher , perfect-paper-stretcher , A great device for stretchin
 Mabef M/02 Studio Easel ,                         , Mabef description                    ,                      ,                ,                  , M02    , 910.7 , standard    ,
 Giotto Mega Pencils     ,                         , Really mega pencils                  ,                      , box size       , Box of 8         , 225400 , 4.16  , standard    , "box-of-8.jpg"
                         ,                         ,                                      ,                      ,                , Box of 12        , 225600 , 6.24  , standard    , "box-of-12.jpg"
+
 Artists Smock           ,                         , Keeps the paint off the clothes      ,                      , "size, colour" , "small, beige"   , 10112  , 11.99 , reduced     ,
                         ,                         ,                                      ,                      ,                , "large, beige"   , 10113  , 11.99 , reduced     ,
                         ,                         ,                                      ,                      ,                , "small, navy"    , 10114  , 11.99 , reduced     ,

server/e2e/import.e2e-spec.ts

@@ -38,7 +38,9 @@ describe('Import resolver', () => {
         const csvFile = path.join(__dirname, 'fixtures', 'product-import.csv');
         const result = await client.importProducts(csvFile);
 
-        expect(result.importProducts.errors).toEqual([]);
+        expect(result.importProducts.errors).toEqual([
+            'Invalid Record Length: header length is 10, got 1 on line 8',
+        ]);
         expect(result.importProducts.importedCount).toBe(4);
 
         const productResult = await client.query(

server/src/data-import/providers/import-parser/import-parser.spec.ts

@@ -11,7 +11,7 @@ describe('ImportParser', () => {
             const input = await loadTestFixture('single-product-single-variant.csv');
             const result = await importParser.parseProducts(input);
 
-            expect(result).toMatchSnapshot();
+            expect(result.results).toMatchSnapshot();
         });
 
         it('single product with a multiple variants', async () => {
@@ -20,7 +20,7 @@ describe('ImportParser', () => {
             const input = await loadTestFixture('single-product-multiple-variants.csv');
             const result = await importParser.parseProducts(input);
 
-            expect(result).toMatchSnapshot();
+            expect(result.results).toMatchSnapshot();
         });
 
         it('multiple products with multiple variants', async () => {
@@ -29,7 +29,7 @@ describe('ImportParser', () => {
             const input = await loadTestFixture('multiple-products-multiple-variants.csv');
             const result = await importParser.parseProducts(input);
 
-            expect(result).toMatchSnapshot();
+            expect(result.results).toMatchSnapshot();
         });
 
         it('works with streamed input', async () => {
@@ -39,36 +39,47 @@ describe('ImportParser', () => {
             const input = fs.createReadStream(filename);
             const result = await importParser.parseProducts(input);
 
-            expect(result).toMatchSnapshot();
+            expect(result.results).toMatchSnapshot();
         });
 
         describe('error conditions', () => {
-            it('throws on invalid option values', async () => {
+            it('reports errors on invalid option values', async () => {
                 const importParser = new ImportParser();
 
                 const input = await loadTestFixture('invalid-option-values.csv');
-                try {
-                    await importParser.parseProducts(input);
-                    fail('should have thrown');
-                } catch (err) {
-                    expect(err.message).toBe(
-                        'The number of optionValues must match the number of optionGroups for the product "Artists Smock"',
-                    );
-                }
+                const result = await importParser.parseProducts(input);
+
+                expect(result.errors).toEqual([
+                    'The number of optionValues must match the number of optionGroups on line 2',
+                    'The number of optionValues must match the number of optionGroups on line 3',
+                    'The number of optionValues must match the number of optionGroups on line 4',
+                    'The number of optionValues must match the number of optionGroups on line 5',
+                ]);
             });
 
-            it('throws on ivalid columns', async () => {
+            it('reports error on ivalid columns', async () => {
                 const importParser = new ImportParser();
 
                 const input = await loadTestFixture('invalid-columns.csv');
-                try {
-                    await importParser.parseProducts(input);
-                    fail('should have thrown');
-                } catch (err) {
-                    expect(err.message).toBe(
-                        'The import file is missing the following columns: "slug", "assets"',
-                    );
-                }
+                const result = await importParser.parseProducts(input);
+
+                expect(result.results).toEqual([]);
+                expect(result.errors).toEqual([
+                    'The import file is missing the following columns: "slug", "assets"',
+                ]);
+            });
+
+            it('reports error on ivalid row length', async () => {
+                const importParser = new ImportParser();
+
+                const input = await loadTestFixture('invalid-row-length.csv');
+                const result = await importParser.parseProducts(input);
+
+                expect(result.errors).toEqual([
+                    'Invalid Record Length: header length is 10, got 8 on line 3',
+                    'Invalid Record Length: header length is 10, got 1 on line 4',
+                ]);
+                expect(result.results.length).toBe(2);
             });
         });
     });

server/src/data-import/providers/import-parser/import-parser.ts

@@ -41,33 +41,53 @@ export interface ParsedProductWithVariants {
     variants: ParsedProductVariant[];
 }
 
+export interface ParseResult<T> {
+    results: T[];
+    errors: string[];
+}
+
+const requiredColumns: Array<keyof RawProductRecord> = [
+    'name',
+    'slug',
+    'description',
+    'assets',
+    'optionGroups',
+    'optionValues',
+    'sku',
+    'price',
+    'taxCategory',
+    'variantAssets',
+];
+
 /**
  * Validates and parses CSV files into a data structure which can then be used to created new entities.
  */
 @Injectable()
 export class ImportParser {
-    async parseProducts(input: string | Stream): Promise<ParsedProductWithVariants[]> {
+    async parseProducts(input: string | Stream): Promise<ParseResult<ParsedProductWithVariants>> {
         const options: parse.Options = {
-            columns: true,
             trim: true,
+            relax_column_count: true,
         };
-        return new Promise<ParsedProductWithVariants[]>((resolve, reject) => {
+        return new Promise<ParseResult<ParsedProductWithVariants>>((resolve, reject) => {
+            let errors: string[] = [];
+
             if (typeof input === 'string') {
-                parse(input, options, (err, records: RawProductRecord[]) => {
+                parse(input, options, (err: any, records: string[][]) => {
                     if (err) {
-                        reject(err);
+                        errors = errors.concat(err);
                     }
-
-                    try {
-                        const output = this.processRawRecords(records);
-                        resolve(output);
-                    } catch (err) {
-                        reject(err);
+                    if (records) {
+                        const parseResult = this.processRawRecords(records);
+                        errors = errors.concat(parseResult.errors);
+                        resolve({ results: parseResult.results, errors });
+                    } else {
+                        resolve({ results: [], errors });
                     }
                 });
             } else {
                 const parser = parse(options);
-                const records: RawProductRecord[] = [];
+                const records: string[][] = [];
                 // input.on('open', () => input.pipe(parser));
                 input.pipe(parser);
                 parser.on('readable', () => {
@@ -79,26 +99,36 @@ export class ImportParser {
                 });
                 parser.on('error', reject);
                 parser.on('end', () => {
-                    try {
-                        const output = this.processRawRecords(records);
-                        resolve(output);
-                    } catch (err) {
-                        reject(err);
-                    }
+                    const parseResult = this.processRawRecords(records);
+                    errors = errors.concat(parseResult.errors);
+                    resolve({ results: parseResult.results, errors });
                 });
             }
         });
     }
 
-    private processRawRecords(records: RawProductRecord[]): ParsedProductWithVariants[] {
-        const output: ParsedProductWithVariants[] = [];
+    private processRawRecords(records: string[][]): ParseResult<ParsedProductWithVariants> {
+        const results: ParsedProductWithVariants[] = [];
+        const errors: string[] = [];
         let currentRow: ParsedProductWithVariants | undefined;
-        validateColumns(records[0]);
-        for (const r of records) {
+        const headerRow = records.shift() as string[];
+        const columnError = validateRequiredColumns(headerRow);
+        if (columnError) {
+            return { results: [], errors: [columnError] };
+        }
+        let line = 1;
+        for (const record of records) {
+            line++;
+            const columnCountError = validateColumnCount(headerRow, record);
+            if (columnCountError) {
+                errors.push(columnCountError + ` on line ${line}`);
+                continue;
+            }
+            const r = mapRowToObject(headerRow, record);
             if (r.name) {
                 if (currentRow) {
                     populateOptionGroupValues(currentRow);
-                    output.push(currentRow);
+                    results.push(currentRow);
                 }
                 currentRow = {
                     product: parseProductFromRecord(r),
@@ -109,13 +139,16 @@ export class ImportParser {
                     currentRow.variants.push(parseVariantFromRecord(r));
                 }
             }
-            validateOptionValueCount(r, currentRow);
+            const optionError = validateOptionValueCount(r, currentRow);
+            if (optionError) {
+                errors.push(optionError + ` on line ${line}`);
+            }
         }
         if (currentRow) {
             populateOptionGroupValues(currentRow);
-            output.push(currentRow);
+            results.push(currentRow);
         }
-        return output;
+        return { results, errors };
     }
 }
 
@@ -127,20 +160,8 @@ function populateOptionGroupValues(currentRow: ParsedProductWithVariants) {
     });
 }
 
-function validateColumns(r: RawProductRecord) {
-    const requiredColumns: Array<keyof RawProductRecord> = [
-        'name',
-        'slug',
-        'description',
-        'assets',
-        'optionGroups',
-        'optionValues',
-        'sku',
-        'price',
-        'taxCategory',
-        'variantAssets',
-    ];
-    const rowKeys = Object.keys(r);
+function validateRequiredColumns(r: string[]): string | undefined {
+    const rowKeys = r;
     const missing: string[] = [];
     for (const col of requiredColumns) {
         if (!rowKeys.includes(col)) {
@@ -148,21 +169,32 @@ function validateColumns(r: RawProductRecord) {
         }
     }
     if (missing.length) {
-        throw new Error(
-            `The import file is missing the following columns: ${missing.map(m => `"${m}"`).join(', ')}`,
-        );
+        return `The import file is missing the following columns: ${missing.map(m => `"${m}"`).join(', ')}`;
     }
 }
 
-function validateOptionValueCount(r: RawProductRecord, currentRow?: ParsedProductWithVariants) {
+function validateColumnCount(columns: string[], row: string[]): string | undefined {
+    if (columns.length !== row.length) {
+        return `Invalid Record Length: header length is ${columns.length}, got ${row.length}`;
+    }
+}
+
+function mapRowToObject(columns: string[], row: string[]): { [key: string]: string } {
+    return row.reduce((obj, val, i) => {
+        return { ...obj, [columns[i]]: val };
+    }, {});
+}
+
+function validateOptionValueCount(
+    r: RawProductRecord,
+    currentRow?: ParsedProductWithVariants,
+): string | undefined {
     if (!currentRow) {
         return;
     }
     const optionValues = parseStringArray(r.optionValues);
     if (currentRow.product.optionGroups.length !== optionValues.length) {
-        throw new Error(
-            `The number of optionValues must match the number of optionGroups for the product "${r.name}"`,
-        );
+        return `The number of optionValues must match the number of optionGroups`;
     }
 }
 

server/src/data-import/providers/import-parser/test-fixtures/invalid-row-length.csv

@@ -0,0 +1,5 @@
+name                    , slug                    , description                          , assets , optionGroups   , optionValues     , sku    , price , taxCategory , variantAssets
+Mabef M/02 Studio Easel ,                         , Mabef description                    ,        ,                ,                  , M02    , 910.7 , standard    ,
+Giotto Mega Pencils     ,                         , Really mega pencils                  ,        , 225400 , 4.16  , standard    ,
+
+Artists Smock           ,                         , Keeps the paint off the clothes      ,        ,  ,    , 10112  , 11.99 , standard    ,

server/src/data-import/providers/importer/importer.ts

@@ -51,22 +51,13 @@ export class Importer {
             });
         }
 
-        let parsed: ParsedProductWithVariants[];
-        try {
-            parsed = await this.importParser.parseProducts(input);
-        } catch (err) {
-            return {
-                errors: [err.message],
-                importedCount: 0,
-            };
-        }
-
-        if (parsed) {
+        const parsed = await this.importParser.parseProducts(input);
+        if (parsed && parsed.results.length) {
             try {
-                const result = await this.importProducts(ctx, parsed);
+                const result = await this.importProducts(ctx, parsed.results);
                 return {
-                    errors: [],
-                    importedCount: parsed.length,
+                    errors: parsed.errors,
+                    importedCount: parsed.results.length,
                 };
             } catch (err) {
                 return {