From a2dceef20502fed6dfea669877e84aa7e066db7a Mon Sep 17 00:00:00 2001 From: Alexander Jones Date: Fri, 13 Sep 2024 02:07:30 -0500 Subject: [PATCH 1/3] Check for non-existent HED in referenced splice columns in sidecar validation Also use TSV file for merged sidecar object. --- bids/types/tsv.js | 2 +- bids/validator/bidsHedSidecarValidator.js | 8 ++++++++ tests/bids.spec.js | 14 ++++++++++++-- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/bids/types/tsv.js b/bids/types/tsv.js index f70dce53..d13cb258 100644 --- a/bids/types/tsv.js +++ b/bids/types/tsv.js @@ -61,7 +61,7 @@ export class BidsTsvFile extends BidsFile { } 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() } diff --git a/bids/validator/bidsHedSidecarValidator.js b/bids/validator/bidsHedSidecarValidator.js index 29b7364b..c78669f6 100644 --- a/bids/validator/bidsHedSidecarValidator.js +++ b/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, + ), + ) + } } } diff --git a/tests/bids.spec.js b/tests/bids.spec.js index cbaaec20..19e2cd6d 100644 --- a/tests/bids.spec.js +++ b/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', { From a6ff87689d10853c9f894e29d37d99dd32f7d6d7 Mon Sep 17 00:00:00 2001 From: Alexander Jones Date: Fri, 13 Sep 2024 05:15:00 -0500 Subject: [PATCH 2/3] Fix nested HED / n/a issues and convert some plain errors to IssueError Also adjust JSON spec tests to use IssueErrors thrown by BidsSidecar and BidsTsvFile constructors. --- bids/types/json.js | 57 +++++++++++++++++++++++++++++++--- bids/types/tsv.js | 7 +++-- common/issues/data.js | 15 +++++++++ spec_tests/jsonTests.spec.js | 60 ++++++++++++++++++++++++++++++------ 4 files changed, 122 insertions(+), 17 deletions(-) diff --git a/bids/types/json.js b/bids/types/json.js index c82d1583..3aad6193 100644 --- a/bids/types/json.js +++ b/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,9 +79,14 @@ 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 } }) @@ -84,6 +94,26 @@ export class BidsSidecar extends BidsJsonFile { 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} + */ + sidecar /** * Constructor. * * @param {string} key The name of this key. * @param {string|Object} 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()) diff --git a/bids/types/tsv.js b/bids/types/tsv.js index d13cb258..73ca5da7 100644 --- a/bids/types/tsv.js +++ b/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,7 +58,7 @@ 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 @@ -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 } diff --git a/common/issues/data.js b/common/issues/data.js index a7877d1e..2ef06151 100644 --- a/common/issues/data.js +++ b/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', diff --git a/spec_tests/jsonTests.spec.js b/spec_tests/jsonTests.spec.js index 78f5257f..48b6cdd8 100644 --- a/spec_tests/jsonTests.spec.js +++ b/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,10 +163,20 @@ 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, null) + sidecarIssues = bidsSide.validate(schema) + } catch (e) { + sidecarIssues = [convertIssue(e)] + } + let eventsIssues = [] + try { + const bidsTsv = new BidsTsvFile(`events`, events, null, [side], mergedSide) + eventsIssues = bidsTsv.validate(schema) + } catch (e) { + eventsIssues = [convertIssue(e)] + } const allIssues = [...sidecarIssues, ...eventsIssues] assertErrors(eCode, altCodes, expectError, iLog, header, allIssues) } @@ -173,8 +184,13 @@ describe('HED validation using JSON tests', () => { 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, null, [], defs) + eventsIssues = bidsTsv.validate(schema) + } catch (e) { + eventsIssues = [convertIssue(e)] + } assertErrors(eCode, altCodes, expectError, iLog, header, eventsIssues) } @@ -182,8 +198,13 @@ describe('HED validation using JSON tests', () => { 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, null) + sidecarIssues = bidsSide.validate(schema) + } catch (e) { + sidecarIssues = [convertIssue(e)] + } assertErrors(eCode, altCodes, expectError, iLog, header, sidecarIssues) } @@ -191,11 +212,30 @@ describe('HED validation using JSON tests', () => { 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, null, [], 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(',') } } } From 558f97ec0a77489bc8170b59e90882b8c7b111ab Mon Sep 17 00:00:00 2001 From: Alexander Jones Date: Fri, 13 Sep 2024 05:37:16 -0500 Subject: [PATCH 3/3] Pass actual mock file objects from JSON spec tests instead of null Passing null was causing null pointer errors in the validation, which was the source of the failing invalid SIDECAR_KEY_MISSING test. --- spec_tests/jsonTests.spec.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec_tests/jsonTests.spec.js b/spec_tests/jsonTests.spec.js index 48b6cdd8..277484aa 100644 --- a/spec_tests/jsonTests.spec.js +++ b/spec_tests/jsonTests.spec.js @@ -165,14 +165,14 @@ describe('HED validation using JSON tests', () => { const mergedSide = getMergedSidecar(side, defs) let sidecarIssues = [] try { - const bidsSide = new BidsSidecar(`sidecar`, mergedSide, null) + 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, null, [side], mergedSide) + const bidsTsv = new BidsTsvFile(`events`, events, { relativePath: 'combo test tsv' }, [side], mergedSide) eventsIssues = bidsTsv.validate(schema) } catch (e) { eventsIssues = [convertIssue(e)] @@ -186,7 +186,7 @@ describe('HED validation using JSON tests', () => { const header = `\n[${eCode} ${eName}](${status})\tEvents:\n"${events}"` let eventsIssues = [] try { - const bidsTsv = new BidsTsvFile(`events`, events, null, [], defs) + const bidsTsv = new BidsTsvFile(`events`, events, { relativePath: 'events test' }, [], defs) eventsIssues = bidsTsv.validate(schema) } catch (e) { eventsIssues = [convertIssue(e)] @@ -200,7 +200,7 @@ describe('HED validation using JSON tests', () => { const side1 = getMergedSidecar(side, defs) let sidecarIssues = [] try { - const bidsSide = new BidsSidecar(`sidecar`, side1, null) + const bidsSide = new BidsSidecar(`sidecar`, side1, { relativePath: 'sidecar test' }) sidecarIssues = bidsSide.validate(schema) } catch (e) { sidecarIssues = [convertIssue(e)] @@ -214,7 +214,7 @@ describe('HED validation using JSON tests', () => { const hTsv = `HED\n${str}\n` let stringIssues = [] try { - const bidsTsv = new BidsTsvFile(`events`, hTsv, null, [], defs) + const bidsTsv = new BidsTsvFile(`events`, hTsv, { relativePath: 'string test tsv' }, [], defs) stringIssues = bidsTsv.validate(schema) } catch (e) { stringIssues = [convertIssue(e)]