Explore using a Range class instead of an object

Currently we pass around an object  to act as our range, but this puts
the onus on the caller to validate and also ensure that iterating over
the range is accurate - this new Range class aims to encapsulate that
functionality in one place

Author: rossjones

lib/importer/src/dudk/backend.js

@@ -5,6 +5,7 @@ const attributeTypes = require('./types/attribute-types');
 const guesser = require('./types/guesser');
 const store = require("./session_store")
 const errorTypes = require('./util/errors');
+const Range = require('./util/range');
 
 // An implementation of the interface described in https://struct.register-dynamics.co.uk/trac/wiki/DataImporter/API
 
@@ -78,13 +79,12 @@ exports.SessionGetSheets = (sid) => {
 exports.SessionSetHeaderRange = (sid, range) => {
   assert(range != null, "Null range passed to SessionSetHeaderRange");
   assert(range.sheet != null, "Range with null sheet passed to SessionSetHeaderRange");
-
   sessionStore.apply(sid, (s) => { s.headerRanges[range.sheet] = range; })
 };
 
 exports.SessionGetHeaderRange = (sid, sheetName) => {
   let session = sessionStore.get(sid);
-  return session.headerRanges[sheetName]
+  return session.headerRanges[sheetName];
 };
 
 exports.SessionSetFooterRange = (sid, range) => {
@@ -95,7 +95,7 @@ exports.SessionSetFooterRange = (sid, range) => {
 
 exports.SessionGetFooterRange = (sid, sheetName) => {
   let session = sessionStore.get(sid);
-  return session.footerRanges[sheetName]
+  return session.footerRanges[sheetName];
 };
 
 function getDimensions(sid) {
@@ -237,46 +237,28 @@ exports.SessionSuggestDataRange = (sid, headerRange, footerRange) => {
       // the header.
 
       // Header and footer, so just go from the row below the header to the row above the footer
-      return {
-        sheet: sheetName,
-        start: {
-          row: headerRange.start.row + 1,
-          column: headerRange.start.column
-        },
-        end: {
-          row: footerRange.start.row,
-          column: headerRange.end.column
-        }
-      };
+      return new Range(
+        sheetName,
+        { row: headerRange.start.row + 1, column: headerRange.start.column },
+        { row: footerRange.start.row, column: headerRange.end.column }
+      );
     } else {
       // Header, but no footer, so just go from the row below the header to the end
-      return {
-        sheet: sheetName,
-        start: {
-          row: headerRange.start.row + 1,
-          column: headerRange.start.column
-        },
-        end: {
-          row: sheetDimensions.rows - 1,
-          column: headerRange.end.column
-        }
-      };
+      return new Range(
+        sheetName,
+        { row: headerRange.start.row + 1, column: headerRange.start.column },
+        { row: sheetDimensions.rows - 1, column: headerRange.end.column }
+      );
     }
   } else {
     // No header range specified, so take all but the first row of the first sheet
     const sheetName = sessionStore.get(sid).sheetNames[0]; // Find the first sheet
     const sheetDimensions = dimensions.sheetDimensions.get(sheetName);
-    return {
-      sheet: sheetName,
-      start: {
-        row: 1,
-        column: 0
-      },
-      end: {
-        row: sheetDimensions.rows - 1,
-        column: sheetDimensions.columns - 1
-      }
-    };
+    return new Range(
+      sheetName,
+      { row: 1, column: 0 },
+      { row: sheetDimensions.rows - 1, column: sheetDimensions.columns - 1 }
+    );
   }
 }
 
@@ -375,7 +357,8 @@ function cellsToSamples(row) {
   return row.map(cellToSample);
 }
 
-// Returns a sample of rows in a range. range is of the form {sheet: 'Foo', start:{row: X, column: Y}, end:{row: X, column: Y}}.
+// Returns a sample of rows in a range. range is and instance of the Range class and
+// is of the form {sheet: 'Foo', start:{row: X, column: Y}, end:{row: X, column: Y}}.
 
 // Returns three arrays - one with startCount rows from the top of the range,
 // one with a random selectino of middleCount rows from the middle, and another
@@ -385,15 +368,15 @@ function cellsToSamples(row) {
 exports.SessionGetInputSampleRows = (sid, range, startCount, middleCount, endCount) => {
   assert(range != null, "Null range passed to SessionGetInputSampleRows");
   assert(sessionStore.has(sid), `No such session ${sid} when getting input sample rows`);
-  assert(sessionStore.get(sid).wb.Sheets[range.sheet], `No such sheet ${range.sheet} in session ${sid} when getting input sample rows`);
+  assert(sessionStore.get(sid).wb.Sheets[range.sheet], `No such sheet ${range.sheet} in session ${sid} when getting input values`);
+
   if (range.end.row < range.start.row || range.end.column < range.start.column) {
     // Range is empty
     return [[], [], []];
   }
   // If the caller asked for more rows than we have, or if the start/middle/end would overlap, clamp the counts
   const totalRowsInRange = (range.end.row - range.start.row + 1);
   [startCount, middleCount, endCount] = clampCounts(startCount, middleCount, endCount, totalRowsInRange);
-
   const sheet = sessionStore.get(sid).wb.Sheets[range.sheet];
   const data = sheet["!data"];
   const merges = sheet["!merges"];
@@ -418,7 +401,6 @@ exports.SessionGetInputSampleRows = (sid, range, startCount, middleCount, endCou
   let endRows = extractColsFromRows(getMergedRows(sheet, range.end.row - endCount + 1, range.end.row),
     range.start.column, range.end.column + 1);
 
-
   // Given an index generate a function that we can use to map across an array to turn it into
   // an object with both index and the array data.
   const mapWithIndex = (arr, startIndex) => {
@@ -430,12 +412,6 @@ exports.SessionGetInputSampleRows = (sid, range, startCount, middleCount, endCou
     }
   }
 
-  // FIXME: Work out how the xlsx library represents styles and
-  // rowspan/colspan and make sure that what we return does something useful
-  // with that information. We DO want styling information to be available in
-  // the preview, so that users can see their spreadsheet in a more familiar
-  // form, and because styling information might be a significant part of the
-  // data.
   return [
     startRows.map(mapWithIndex(startRows, range.start.row)),
     middleRows.map(mapWithIndex(middleRows, middleStart)),
@@ -532,8 +508,8 @@ exports.SessionGuessTypes = (sid, range) => {
 // objects with string fields "displayName" and "description", and an optional Map
 // field "formats" that maps from format names (as returned by SessionGetTypes)
 // to objects with strings field "displayName" and "description".
-exports.SessionGetSupportedTypes = (sid) => {
-  return types.supportedTypes;
+exports.SessionGetSupportedTypes = (_sid) => {
+  return attributeTypes.supportedTypes;
 };
 
 // Given guesses as returned by SessionGuessTypes and a list of domain-model
@@ -573,8 +549,7 @@ exports.SessionSuggestFields = (sid, typeGuesses, domainModelFields) => {
 exports.SessionGetInputValues = (sid, range, maxValues) => {
   assert(sessionStore.get(sid), `No such session ${sid} when getting input values`);
   assert(sessionStore.get(sid).wb.Sheets[range.sheet], `No such sheet ${range.sheet} in session ${sid} when getting input values`);
-  assert(range.end.row >= range.start.row, `End row (${range.end.row}) must be >= start row (${range.start.row}) to get input values`);
-  assert(range.end.column >= range.start.column, `End column (${range.end.column}) must be >= start column (${range.start.column}) to get input values`);
+  assert(range.isValid(), `Range is invalid: end (${JSON.stringify(range.end)}) must be >= start (${JSON.stringify(range.start)}) to get input values`);
 
   const sheet = sessionStore.get(sid).wb.Sheets[range.sheet];
   const data = sheet["!data"];
@@ -679,7 +654,7 @@ exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false
   validateMapping(range, mapping);
 
   const records = new Array(); // Array of output records
-  const errors = new Array();;
+  const errors = new Array();
   const warnings = new Array();
 
   const sheet = sessionStore.get(sid).wb.Sheets[range.sheet];
@@ -688,15 +663,14 @@ exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false
   const attrMap = Object.entries(mapping.attributeMappings);
   const attrTypes = mapping.attributeTypes || {};
 
-
-  // TODO: Replace this with a map/sum once range is a real type.
+  // Use Range.applyRows to determine expectedColumnCount
   let expectedColumnCount = 0;
-  for (let rowIdx = range.start.row; rowIdx <= range.end.row; rowIdx++) {
+  range.applyRows((rowIdx) => {
     let row = getMergedRow(data, merges, rowIdx);
     if (row && row.length > expectedColumnCount) expectedColumnCount = row.length;
-  }
+  });
 
-  for (let rowIdx = range.start.row; rowIdx <= range.end.row; rowIdx++) {
+  range.applyRows((rowIdx) => {
     let rowErrorCount = 0;
     let row = getMergedRow(data, merges, rowIdx);
     let recordCells = {};
@@ -706,7 +680,7 @@ exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false
       // Ensure we exclude footer rows from the validation as they may not
       // have the same number of columns as the header row.
       if (rowIdx == range.end.row && footerRange) {
-        continue;
+        return;
       }
 
       if (row.length < expectedColumnCount) {
@@ -777,7 +751,7 @@ exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false
     } else {
       warnings.push({ row: rowIdx, type: errorTypes.ValidationError.EmptyRow({}) })
     }
-  }
+  });
 
   let jid = makeAccessToken("j");
   let job = new JobResult(records, errors, warnings);