Merge pull request #193 from hed-standard/fix-sidecar-syntax-issues

Fix sidecar syntax issues

bids/types/json.js

@@ -1,8 +1,13 @@
+import isPlainObject from 'lodash/isPlainObject'
+
 import { sidecarValueHasHed } from '../utils'
 import { parseHedString } from '../../parser/main'
 import ParsedHedString from '../../parser/parsedHedString'
 import { BidsFile } from './basic'
 import BidsHedSidecarValidator from '../validator/bidsHedSidecarValidator'
+import { generateIssue, IssueError } from '../../common/issues/issues'
+
+const ILLEGAL_SIDECAR_KEYS = new Set(['hed', 'n/a'])
 
 /**
  * A BIDS JSON file.
@@ -74,16 +79,41 @@ export class BidsSidecar extends BidsJsonFile {
   _filterHedStrings() {
     const sidecarHedTags = Object.entries(this.jsonData)
       .map(([sidecarKey, sidecarValue]) => {
+        const trimmedSidecarKey = sidecarKey.trim()
+        if (ILLEGAL_SIDECAR_KEYS.has(trimmedSidecarKey.toLowerCase())) {
+          throw new IssueError(generateIssue('illegalSidecarHedKey', {}))
+        }
         if (sidecarValueHasHed(sidecarValue)) {
-          return [sidecarKey.trim(), new BidsSidecarKey(sidecarKey.trim(), sidecarValue.HED)]
+          return [trimmedSidecarKey, new BidsSidecarKey(trimmedSidecarKey, sidecarValue.HED, this)]
         } else {
+          this._verifyKeyHasNoDeepHed(sidecarKey, sidecarValue)
           return null
         }
       })
       .filter((x) => x !== null)
     this.sidecarKeys = new Map(sidecarHedTags)
   }
 
+  /**
+   * Verify that a column has no deeply nested "HED" keys.
+   *
+   * @param {string} key An object key.
+   * @param {*} value An object value.
+   * @throws {IssueError} If an invalid "HED" key is found.
+   * @private
+   */
+  _verifyKeyHasNoDeepHed(key, value) {
+    if (key.toUpperCase() === 'HED') {
+      throw new IssueError(generateIssue('illegalSidecarHedDeepKey', {}))
+    }
+    if (!isPlainObject(value)) {
+      return
+    }
+    for (const [subkey, subvalue] of Object.entries(value)) {
+      this._verifyKeyHasNoDeepHed(subkey, subvalue)
+    }
+  }
+
   _categorizeHedStrings() {
     this.hedValueStrings = []
     this.hedCategoricalStrings = []
@@ -176,7 +206,9 @@ export class BidsSidecar extends BidsJsonFile {
           this.columnSpliceMapping.set(sidecarKey, keyReferences)
         }
       } else {
-        throw new Error('Unexpected type found in sidecar parsedHedData map.')
+        throw new IssueError(
+          generateIssue('internalConsistencyError', { message: 'Unexpected type found in sidecar parsedHedData map.' }),
+        )
       }
     }
   }
@@ -224,19 +256,26 @@ export class BidsSidecarKey {
    * @type {ParsedHedString}
    */
   parsedValueString
+  /**
+   * Weak reference to the sidecar.
+   * @type {WeakRef<BidsSidecar>}
+   */
+  sidecar
 
   /**
    * Constructor.
    *
    * @param {string} key The name of this key.
    * @param {string|Object<string, string>} data The data for this key.
+   * @param {BidsSidecar} sidecar The parent sidecar.
    */
-  constructor(key, data) {
+  constructor(key, data, sidecar) {
     this.name = key
+    this.sidecar = new WeakRef(sidecar)
     if (typeof data === 'string') {
       this.valueString = data
-    } else if (data !== Object(data)) {
-      throw new Error('Non-object passed as categorical data.')
+    } else if (!isPlainObject(data)) {
+      throw new IssueError(generateIssue('illegalSidecarHedType', { key: key, file: sidecar.file.relativePath }))
     } else {
       this.categoryMap = data
     }
@@ -266,6 +305,14 @@ export class BidsSidecarKey {
     const issues = []
     this.parsedCategoryMap = new Map()
     for (const [value, string] of Object.entries(this.categoryMap)) {
+      const trimmedValue = value.trim()
+      if (ILLEGAL_SIDECAR_KEYS.has(trimmedValue.toLowerCase())) {
+        throw new IssueError(generateIssue('illegalSidecarHedCategoricalValue', {}))
+      } else if (typeof string !== 'string') {
+        throw new IssueError(
+          generateIssue('illegalSidecarHedType', { key: value, file: this.sidecar.deref()?.file?.relativePath }),
+        )
+      }
       const [parsedString, parsingIssues] = parseHedString(string, hedSchemas)
       this.parsedCategoryMap.set(value, parsedString)
       issues.push(...Object.values(parsingIssues).flat())

bids/types/tsv.js

@@ -5,6 +5,7 @@ import { convertParsedTSVData, parseTSV } from '../tsvParser'
 import { BidsSidecar } from './json'
 import ParsedHedString from '../../parser/parsedHedString'
 import BidsHedTsvValidator from '../validator/bidsHedTsvValidator'
+import { generateIssue, IssueError } from '../../common/issues/issues'
 
 /**
  * A BIDS TSV file.
@@ -57,11 +58,11 @@ export class BidsTsvFile extends BidsFile {
     } else if (isPlainObject(tsvData)) {
       this.parsedTsv = convertParsedTSVData(tsvData)
     } else {
-      throw new Error('parsedTsv has an invalid type')
+      throw new IssueError(generateIssue('internalError', { message: 'parsedTsv has an invalid type' }))
     }
 
     this.potentialSidecars = potentialSidecars
-    this.mergedSidecar = new BidsSidecar(name, mergedDictionary, null)
+    this.mergedSidecar = new BidsSidecar(name, mergedDictionary, this.file)
     this.sidecarHedData = this.mergedSidecar.hedData
     this._parseHedColumn()
   }
@@ -195,7 +196,9 @@ export class BidsTsvRow extends ParsedHedString {
   get onset() {
     const value = Number(this.rowCells.get('onset'))
     if (Number.isNaN(value)) {
-      throw new Error('Attempting to access the onset of a TSV row without one.')
+      throw new IssueError(
+        generateIssue('internalError', { message: 'Attempting to access the onset of a TSV row without one.' }),
+      )
     }
     return value
   }

bids/validator/bidsHedSidecarValidator.js

@@ -126,6 +126,14 @@ export class BidsHedSidecarValidator {
             ),
           )
         }
+        if (!this.sidecar.parsedHedData.has(referredKey) && referredKey !== 'HED') {
+          issues.push(
+            BidsHedIssue.fromHedIssue(
+              generateIssue('undefinedCurlyBraces', { column: referredKey }),
+              this.sidecar.file,
+            ),
+          )
+        }
       }
     }
 

common/issues/data.js

@@ -313,6 +313,21 @@ export default {
     level: 'error',
     message: stringTemplate`The HED data for sidecar key "${'key'}" of file "${'file'}" is not either a key-value dictionary or a string.`,
   },
+  illegalSidecarHedKey: {
+    hedCode: 'SIDECAR_INVALID',
+    level: 'error',
+    message: stringTemplate`The string 'HED' or 'n/a' was illegally used as a sidecar key.`,
+  },
+  illegalSidecarHedCategoricalValue: {
+    hedCode: 'SIDECAR_INVALID',
+    level: 'error',
+    message: stringTemplate`The string 'HED' or 'n/a' was illegally used as a sidecar categorical value.`,
+  },
+  illegalSidecarHedDeepKey: {
+    hedCode: 'SIDECAR_INVALID',
+    level: 'error',
+    message: stringTemplate`The key 'HED' was illegally used within a non-HED sidecar column.`,
+  },
   // Generic errors
   genericError: {
     hedCode: 'GENERIC_ERROR',

spec_tests/jsonTests.spec.js

@@ -8,6 +8,7 @@ import { buildSchemas } from '../validator/schema/init'
 import { SchemaSpec, SchemasSpec } from '../common/schema/types'
 import path from 'path'
 import { BidsSidecar, BidsTsvFile } from '../bids'
+import { generateIssue, IssueError } from '../common/issues/issues'
 const fs = require('fs')
 
 const displayLog = process.env.DISPLAY_LOG === 'true'
@@ -162,40 +163,79 @@ describe('HED validation using JSON tests', () => {
         const status = expectError ? 'Expect fail' : 'Expect pass'
         const header = `\n[${eCode} ${eName}](${status})\tCOMBO\t"${side}"\n"${events}"`
         const mergedSide = getMergedSidecar(side, defs)
-        const bidsSide = new BidsSidecar(`sidecar`, mergedSide, null)
-        const bidsTsv = new BidsTsvFile(`events`, events, null, [side], mergedSide)
-        const sidecarIssues = bidsSide.validate(schema)
-        const eventsIssues = bidsTsv.validate(schema)
+        let sidecarIssues = []
+        try {
+          const bidsSide = new BidsSidecar(`sidecar`, mergedSide, { relativePath: 'combo test sidecar' })
+          sidecarIssues = bidsSide.validate(schema)
+        } catch (e) {
+          sidecarIssues = [convertIssue(e)]
+        }
+        let eventsIssues = []
+        try {
+          const bidsTsv = new BidsTsvFile(`events`, events, { relativePath: 'combo test tsv' }, [side], mergedSide)
+          eventsIssues = bidsTsv.validate(schema)
+        } catch (e) {
+          eventsIssues = [convertIssue(e)]
+        }
         const allIssues = [...sidecarIssues, ...eventsIssues]
         assertErrors(eCode, altCodes, expectError, iLog, header, allIssues)
       }
 
       const eventsValidator = function (eCode, altCodes, eName, events, schema, defs, expectError, iLog) {
         const status = expectError ? 'Expect fail' : 'Expect pass'
         const header = `\n[${eCode} ${eName}](${status})\tEvents:\n"${events}"`
-        const bidsTsv = new BidsTsvFile(`events`, events, null, [], defs)
-        const eventsIssues = bidsTsv.validate(schema)
+        let eventsIssues = []
+        try {
+          const bidsTsv = new BidsTsvFile(`events`, events, { relativePath: 'events test' }, [], defs)
+          eventsIssues = bidsTsv.validate(schema)
+        } catch (e) {
+          eventsIssues = [convertIssue(e)]
+        }
         assertErrors(eCode, altCodes, expectError, iLog, header, eventsIssues)
       }
 
       const sideValidator = function (eCode, altCodes, eName, side, schema, defs, expectError, iLog) {
         const status = expectError ? 'Expect fail' : 'Expect pass'
         const header = `\n[${eCode} ${eName}](${status})\tSIDECAR "${side}"`
         const side1 = getMergedSidecar(side, defs)
-        const bidsSide = new BidsSidecar(`sidecar`, side1, null)
-        const sidecarIssues = bidsSide.validate(schema)
+        let sidecarIssues = []
+        try {
+          const bidsSide = new BidsSidecar(`sidecar`, side1, { relativePath: 'sidecar test' })
+          sidecarIssues = bidsSide.validate(schema)
+        } catch (e) {
+          sidecarIssues = [convertIssue(e)]
+        }
         assertErrors(eCode, altCodes, expectError, iLog, header, sidecarIssues)
       }
 
       const stringValidator = function (eCode, altCodes, eName, str, schema, defs, expectError, iLog) {
         const status = expectError ? 'Expect fail' : 'Expect pass'
         const header = `\n[${eCode} ${eName}](${status})\tSTRING: "${str}"`
         const hTsv = `HED\n${str}\n`
-        const bidsTsv = new BidsTsvFile(`events`, hTsv, null, [], defs)
-        const stringIssues = bidsTsv.validate(schema)
+        let stringIssues = []
+        try {
+          const bidsTsv = new BidsTsvFile(`events`, hTsv, { relativePath: 'string test tsv' }, [], defs)
+          stringIssues = bidsTsv.validate(schema)
+        } catch (e) {
+          stringIssues = [convertIssue(e)]
+        }
         assertErrors(eCode, altCodes, expectError, iLog, header, stringIssues)
       }
 
+      /**
+       * Convert an Error into an Issue.
+       *
+       * @param {Error} issueError A thrown error.
+       * @returns {Issue} A HED issue.
+       */
+      const convertIssue = function (issueError) {
+        if (issueError instanceof IssueError) {
+          return issueError.issue
+        } else {
+          return generateIssue('internalError', { message: issueError.message })
+        }
+      }
+
       beforeAll(async () => {
         hedSchema = schemaMap.get(schema)
         defs = { definitions: { HED: { defList: definitions.join(',') } } }

tests/bids.spec.js

@@ -550,6 +550,18 @@ describe('BIDS datasets', () => {
             }),
             standaloneSidecars[1].file,
           ),
+          BidsHedIssue.fromHedIssue(
+            generateIssue('undefinedCurlyBraces', {
+              column: 'event_code',
+            }),
+            standaloneSidecars[1].file,
+          ),
+          BidsHedIssue.fromHedIssue(
+            generateIssue('undefinedCurlyBraces', {
+              column: 'response_time',
+            }),
+            standaloneSidecars[1].file,
+          ),
           BidsHedIssue.fromHedIssue(
             generateIssue('recursiveCurlyBracesWithKey', {
               column: 'response_time',
@@ -626,14 +638,12 @@ describe('BIDS datasets', () => {
               column: 'response_time',
             }),
             combinedDatasets[0].file,
-            { tsvLine: 3 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('undefinedCurlyBraces', {
               column: 'response_time',
             }),
             combinedDatasets[1].file,
-            { tsvLine: 3 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('duplicateTag', {