Validation (#141)

Added a validation mechanism, invoke by providing attribute type validators when setting up the import mapping.

If none are specified, it defaults to everything being strings with no validation, matching current behaviour.

The mapping results may now include errors that cause input rows to be rejected, or warnings where a row could be interpreted but there might be a problem with it (eg, an attribute value was malformed for the required type, but it was marked as optional anyway so we can live without it).

Adding support to display these warnings/errors in the frontend will be a separate bit of work.

Author: alaric-rd

fixtures/test-validation.csv

@@ -0,0 +1,9 @@
+TEST SPREADSHEET,,,,,,
+required string,required number,optional string,optional number,,,
+Name,Age,Egginess,Weight,,Valid?,Comment
+Boris,13,High,5.5,,TRUE,All fields present
+,,,,,FALSE,All fields blank; row will be silently ignored
+Nelly,10,,,,TRUE,Optional fields absent
+Sid,twelve,,,,FALSE,Invalid required number
+,nine,,,,FALSE,"Missing required string, invalid required number"
+Dave,15,,unknown,,TRUE,Invalid optional number

fixtures/test-validation.ods

@@ -0,0 +1,81 @@
+// "input values" are what's found in the input data, and will generally be
+// simple string/boolean/number/date/etc things. "attribute values" are the
+// actual attributes in our output data model and can be some arbitrary type.
+
+// An attribute type is a function from an input value to a result.
+
+class AttributeMappingResult {
+  constructor(value, warnings, errors) {
+    this.value = value;
+    this.warnings = warnings || [];
+    this.errors = errors;
+  }
+
+  get valid() {
+    return !this.errors;
+  }
+
+  get empty() {
+    return this.value !== undefined;
+  }
+}
+
+// These helpers define the three kinds of results:
+
+function emptyMapping(warnings) {
+  return new AttributeMappingResult(undefined, warnings);
+}
+
+function successfulMapping(outputValue, warnings) {
+  return new AttributeMappingResult(outputValue, warnings);
+}
+
+function failedMapping(warnings, errors) {
+  return new AttributeMappingResult(undefined, warnings, errors);
+}
+
+// Create an optional version of an existing type, that allows empty strings or
+// undefined values and maps then to "undefined", and converts any validation errors to warnings
+exports.optionalType = (baseType) => {
+  return (inputValue) => {
+    if(inputValue !== undefined && inputValue != "") {
+      const result = baseType(inputValue);
+      if(result.valid) {
+        return result;
+      } else {
+        // Convert any errors into warnings, as this is an optional field
+        return emptyMapping(result.warnings.concat(result.errors));
+      }
+    }
+    else {
+      return emptyMapping();
+    }
+  };
+}
+
+// Create a required version of an existing type, that reports an undefined or empty-string input value as an error
+exports.requiredType = (baseType) => {
+  return (inputValue) => {
+    if(inputValue !== undefined && inputValue != "") {
+      return baseType(inputValue);
+    }
+    else {
+      return failedMapping([], ["A value must be provided"]);
+    }
+  };
+}
+
+// The default string type
+exports.basicStringType = (inputValue) => {
+  return successfulMapping(String(inputValue));
+};
+
+// The default numeric type
+exports.basicNumberType = (inputValue) => {
+  const result = parseFloat(inputValue);
+  if(isNaN(result)) {
+    return failedMapping([], ["This is not a valid number"]);
+  } else {
+    return successfulMapping(result);
+  }
+};

fixtures/test-validation.xlsx

@@ -0,0 +1,80 @@
+const attributeTypes = require('./attribute-types');
+
+test('required basic string', () => {
+  // Also tests the workings of the requiredType system
+  const t = attributeTypes.requiredType(attributeTypes.basicStringType);
+
+  let r = t(undefined);
+  expect(r.valid).toBeFalsy();
+  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject(["A value must be provided"]);
+
+  r = t("");
+  expect(r.valid).toBeFalsy();
+  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject(["A value must be provided"]);
+
+  r = t("foo");
+  expect(r.valid).toBeTruthy();
+  expect(r.value).toEqual("foo");
+  expect(r.warnings).toMatchObject([]);
+
+  r = t(123);
+  expect(r.valid).toBeTruthy();
+  expect(r.value).toEqual("123");
+  expect(r.warnings).toMatchObject([]);
+});
+
+test('optional basic string', () => {
+  // Also tests the workings of the optionalType system
+  const t = attributeTypes.optionalType(attributeTypes.basicStringType);
+
+  let r = t(undefined);
+  expect(r.valid).toBeTruthy();
+  expect(r.warnings).toMatchObject([]);
+  expect(r.value).toEqual(undefined);
+
+  r = t("");
+  expect(r.valid).toBeTruthy();
+  expect(r.warnings).toMatchObject([]);
+  expect(r.value).toEqual(undefined);
+
+  r = t("foo");
+  expect(r.valid).toBeTruthy();
+  expect(r.value).toEqual("foo");
+  expect(r.warnings).toMatchObject([]);
+
+  r = t(123);
+  expect(r.valid).toBeTruthy();
+  expect(r.value).toEqual("123");
+  expect(r.warnings).toMatchObject([]);
+});
+
+test('required basic number', () => {
+  const t = attributeTypes.requiredType(attributeTypes.basicNumberType);
+
+  let r = t("foo");
+  expect(r.valid).toBeFalsy();
+  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject(["This is not a valid number"]);
+
+  r = t(123);
+  expect(r.valid).toBeTruthy();
+  expect(r.value).toEqual(123);
+  expect(r.warnings).toMatchObject([]);
+});
+
+test('optional basic number', () => {
+  // Also tests that optionalType converts errors into warnings
+  const t = attributeTypes.optionalType(attributeTypes.basicNumberType);
+
+  let r = t("foo");
+  expect(r.valid).toBeTruthy();
+  expect(r.value).toBeUndefined();
+  expect(r.warnings).toMatchObject(["This is not a valid number"]);
+
+  r = t(123);
+  expect(r.valid).toBeTruthy();
+  expect(r.value).toEqual(123);
+  expect(r.warnings).toMatchObject([]);
+});

lib/importer/attribute-types.js

@@ -1,6 +1,7 @@
 const xlsx = require("xlsx");
 const crypto = require("crypto");
 const assert = require('node:assert').strict;
+const attributeTypes = require('./attribute-types');
 
 // An implementation of the interface described in https://struct.register-dynamics.co.uk/trac/wiki/DataImporter/API
 
@@ -456,6 +457,14 @@ function validateMapping(range, mapping) {
     assert(attrSource >= 0);
     assert(attrSource < columnsInRange);
   }
+
+  // Output types are optional, we fall back to basicStringType if not specified
+  if(mapping.attributeTypes) {
+    // eslint-disable-next-line no-unused-vars
+    for(const [attribute, attrType] of Object.entries(mapping.attributeTypes)) {
+      assert(attrType instanceof Function);
+    }
+  }
 }
 
 function mapCellValue(cell) {
@@ -479,6 +488,7 @@ exports.SessionPerformMappingJob = (sid, range, mapping) => {
   const data = sheet["!data"];
   const merges = sheet["!merges"];
   const attrMap = Object.entries(mapping.attributeMappings);
+  const attrTypes = mapping.attributeTypes || {};
 
   for(let rowIdx=range.start.row; rowIdx <= range.end.row; rowIdx++) {
     let row = getMergedRow(data, merges, rowIdx);
@@ -489,15 +499,14 @@ exports.SessionPerformMappingJob = (sid, range, mapping) => {
     if (row) {
       attrMap.forEach((element) => {
         const [attr, m] = element;
+
         // For now, attribute mappings are just integer column offsets
         const inputColumn = range.start.column + m;
-        if (inputColumn >= row.length) {
-          // If a row is missing values at the end, this may be
-          // represented as a "short" row array.
-          record[attr] = undefined;
-        } else {
-          const cell = row[range.start.column + m];
-          if(cell.v) {
+        // If a row is missing values at the end, this may be
+        // represented as a "short" row array.
+        if (inputColumn < row.length) {
+          const cell = row[inputColumn];
+          if(cell && cell.v) {
             record[attr] = mapCellValue(cell);
             foundSomeValues = true;
           }
@@ -506,7 +515,35 @@ exports.SessionPerformMappingJob = (sid, range, mapping) => {
     }
 
     if (foundSomeValues) {
-      records.push(record);
+      // Only if we found something do we validate and map the types
+      let mappedRecord = {};
+
+      Object.entries(record).forEach((element) => {
+        const [attr, inputVal] = element;
+        const attrType = attrTypes[attr] || attributeTypes.basicStringType;
+
+        const result = attrType(inputVal);
+
+        result.warnings.forEach((text) => {
+          rowWarnings.push([attr,text]);
+        });
+
+        if(result.valid) {
+          // Succeeded, but maybe an empty result
+          if(result.value !== undefined) {
+            mappedRecord[attr] = result.value;
+          }
+        } else {
+          // Failed
+          result.errors.forEach((text) => {
+            rowErrors.push([attr,text]);
+          });
+        }
+      });
+
+      if (rowErrors.length == 0) {
+        records.push(mappedRecord);
+      }
     }
 
     if(rowWarnings.length > 0) {

lib/importer/attribute-types.test.js

@@ -1,5 +1,5 @@
 const backend = require('./backend');
-
+const attributeTypes = require('./attribute-types');
 
 const testFiles = new Map([
   ["test", [
@@ -16,7 +16,12 @@ const testFiles = new Map([
     ["tribbles.xlsx", "My lovely tribbles"],
     ["tribbles.csv", "Sheet1"],
     ["tribbles.ods", "My lovely tribbles"],
-  ]]
+  ]],
+  ["test-validation",
+   [["test-validation.xlsx", "Cool Data"]],
+   [["test-validation.csv", "Sheet1"]],
+   [["test-validation.ods", "Cool Data"]],
+  ]
 ]);
 
 const prefix_fixture = ([fixture_name, sheetname]) => {
@@ -74,6 +79,11 @@ describe("Backend tests", () => {
           Age: 1,
           Egginess: 2
         },
+        attributeTypes: {
+          Name: attributeTypes.requiredType(attributeTypes.basicStringType),
+          Age: attributeTypes.requiredType(attributeTypes.basicNumberType),
+          Egginess: attributeTypes.requiredType(attributeTypes.basicStringType)
+        },
       };
 
       // Do a mapping
@@ -208,6 +218,11 @@ describe("Backend tests", () => {
         Age: 2,
         Egginess: 3
       },
+      attributeTypes: {
+        Name: attributeTypes.requiredType(attributeTypes.basicStringType),
+        Age: attributeTypes.requiredType(attributeTypes.basicNumberType),
+        Egginess: attributeTypes.requiredType(attributeTypes.basicStringType)
+      }
     };
 
     const jid = backend.SessionPerformMappingJob(sid, dataRange, mapping);
@@ -338,6 +353,11 @@ describe("Backend tests", () => {
           Age: 1,
           Egginess: 2
         },
+        attributeTypes: {
+          Name: attributeTypes.requiredType(attributeTypes.basicStringType),
+          Age: attributeTypes.requiredType(attributeTypes.basicNumberType),
+          Egginess: attributeTypes.requiredType(attributeTypes.basicStringType)
+        },
       };
 
       const jid = backend.SessionPerformMappingJob(sid, dataRange, mapping);
@@ -445,3 +465,64 @@ describe("Backend tests", () => {
   });
 
 });
+
+test('validation', () => {
+  const fixtures = testFiles.get("test-validation").map(prefix_fixture)
+  for( const [filename, sheetname] of fixtures) {
+    const sid = backend.CreateSession();
+    backend.SessionSetFile(sid, filename);
+
+    const dataRange = {
+      sheet: sheetname,
+      start: {row: 3, column: 0},
+      end: {row: 8, column: 3}};
+
+    const mapping = {
+      attributeMappings: {
+        Name: 0,
+        Age: 1,
+        Egginess: 2,
+        Weight: 3,
+      },
+      attributeTypes: {
+        Name: attributeTypes.requiredType(attributeTypes.basicStringType),
+        Age: attributeTypes.requiredType(attributeTypes.basicNumberType),
+        Egginess: attributeTypes.optionalType(attributeTypes.basicStringType),
+        Weight: attributeTypes.optionalType(attributeTypes.basicNumberType)
+      }
+    };
+
+    // Do a mapping
+
+    const jid = backend.SessionPerformMappingJob(sid, dataRange, mapping);
+
+    // Look at the job results
+
+    const jobSummary = backend.JobGetSummary(jid);
+    expect(jobSummary).toMatchObject({
+      recordCount: 3,
+      errorCount: 2,
+      warningCount: 1
+    });
+
+    const warnings = backend.JobGetWarnings(jid); // Weight is optional so errors here become warnings
+    expect(warnings).toMatchObject(new Map([[8, [['Weight', 'This is not a valid number']]]]));
+
+    const errors = backend.JobGetErrors(jid);
+    expect(errors).toMatchObject(new Map([[6, [['Age', 'This is not a valid number']]],
+                                          [7, [['Age', 'This is not a valid number']]]]));
+
+    const allRecords = backend.JobGetRecords(jid, 0, 3);
+    expect(allRecords).toMatchObject([
+      { Name: 'Boris', Age: 13, Egginess: 'High', Weight: 5.5 },
+      { Name: 'Nelly', Age: 10},
+      { Name: 'Dave', Age: 15}
+    ]);
+
+    backend.JobDelete(sid, jid);
+
+    backend.SessionDelete(sid);
+
+  }
+
+});