Merge pull request #322 from VisLab/group_bug

VisLab · web-flow · commit 86b92f096bc1 · 2025-03-14T16:40:26.000-05:00
Fixed issue with top-level tag used in splice issue #321
diff --git a/src/bids/types/json.js b/src/bids/types/json.js
@@ -217,7 +217,7 @@ export class BidsSidecar extends BidsJsonFile {
         this.parsedHedData.set(name, sidecarKey.parsedCategoryMap)
       }
     }
-    this.#generateSidecarColumnSpliceMap()
+    this._generateSidecarColumnSpliceMap()
     return [errors, warnings]
   }
 
@@ -226,33 +226,33 @@ export class BidsSidecar extends BidsJsonFile {
    *
    * @private
    */
-  #generateSidecarColumnSpliceMap() {
+  _generateSidecarColumnSpliceMap() {
     this.columnSpliceMapping = new Map()
     this.columnSpliceReferences = new Set()
 
     for (const [sidecarKey, hedData] of this.parsedHedData) {
       if (hedData instanceof ParsedHedString) {
-        this.#parseValueSplice(sidecarKey, hedData)
+        this._(sidecarKey, hedData)
       } else if (hedData instanceof Map) {
-        this.#parseCategorySplice(sidecarKey, hedData)
+        this._parseCategorySplice(sidecarKey, hedData)
       } else if (hedData) {
         IssueError.generateAndThrowInternalError('Unexpected type found in sidecar parsedHedData map.')
       }
     }
   }
 
-  #parseValueSplice(sidecarKey, hedData) {
+  _(sidecarKey, hedData) {
     if (hedData.columnSplices.length > 0) {
-      const keyReferences = this.#processColumnSplices(new Set(), hedData.columnSplices)
+      const keyReferences = this._processColumnSplices(new Set(), hedData.columnSplices)
       this.columnSpliceMapping.set(sidecarKey, keyReferences)
     }
   }
 
-  #parseCategorySplice(sidecarKey, hedData) {
+  _parseCategorySplice(sidecarKey, hedData) {
     let keyReferences = null
     for (const valueString of hedData.values()) {
       if (valueString?.columnSplices.length > 0) {
-        keyReferences = this.#processColumnSplices(keyReferences, valueString.columnSplices)
+        keyReferences = this._processColumnSplices(keyReferences, valueString.columnSplices)
       }
     }
     if (keyReferences instanceof Set) {
@@ -267,7 +267,7 @@ export class BidsSidecar extends BidsJsonFile {
    * @returns {Set<string>}
    * @private
    */
-  #processColumnSplices(keyReferences, columnSplices) {
+  _processColumnSplices(keyReferences, columnSplices) {
     keyReferences ??= new Set()
     for (const columnSplice of columnSplices) {
       keyReferences.add(columnSplice.originalTag)
@@ -358,9 +358,9 @@ export class BidsSidecarKey {
    */
   parseHed(hedSchemas) {
     if (this.isValueKey) {
-      return this.#parseValueString(hedSchemas)
+      return this._parseValueString(hedSchemas)
     }
-    return this.#parseCategory(hedSchemas)
+    return this._parseCategory(hedSchemas)
   }
 
   /**
@@ -373,8 +373,8 @@ export class BidsSidecarKey {
    * @returns {Array} - [Issue[], Issue[]] - Errors due for the value.
    * @private
    */
-  #parseValueString(hedSchemas) {
-    const [parsedString, errorIssues, warningIssues] = parseHedString(this.valueString, hedSchemas, false, true)
+  _parseValueString(hedSchemas) {
+    const [parsedString, errorIssues, warningIssues] = parseHedString(this.valueString, hedSchemas, false, true, false)
     this.parsedValueString = parsedString
     return [errorIssues, warningIssues]
   }
@@ -385,7 +385,7 @@ export class BidsSidecarKey {
    * @returns {Array} - Array[Issue[], Issue[]] A list of error issues and warning issues.
    * @private
    */
-  #parseCategory(hedSchemas) {
+  _parseCategory(hedSchemas) {
     this.parsedCategoryMap = new Map()
     const errors = []
     const warnings = []
@@ -399,7 +399,7 @@ export class BidsSidecarKey {
           file: this.sidecar?.file?.relativePath,
         })
       }
-      const [parsedString, errorIssues, warningIssues] = parseHedString(string, hedSchemas, true, true)
+      const [parsedString, errorIssues, warningIssues] = parseHedString(string, hedSchemas, true, true, false)
       this.parsedCategoryMap.set(value, parsedString)
       warnings.push(...warningIssues)
       errors.push(...errorIssues)
diff --git a/src/bids/validator/tsvValidator.js b/src/bids/validator/tsvValidator.js
@@ -353,6 +353,7 @@ export class BidsHedTsvParser {
         this.hedSchemas,
         false,
         false,
+        true,
       )
       element.parsedHedString = parsedHedString
       errors.push(...BidsHedIssue.fromHedIssues(errorIssues, this.tsvFile.file, { tsvLine: element.tsvLine }))
diff --git a/src/parser/parsedHedString.js b/src/parser/parsedHedString.js
@@ -13,36 +13,43 @@ export class ParsedHedString {
    * @type {string}
    */
   hedString
+
   /**
    * The parsed substring data in unfiltered form.
    * @type {ParsedHedSubstring[]}
    */
   parseTree
+
   /**
    * The tag groups in the string (top-level).
    * @type {ParsedHedGroup[]}
    */
   tagGroups
+
   /**
    * All the top-level tags in the string.
    * @type {ParsedHedTag[]}
    */
   topLevelTags
+
   /**
    * All the tags in the string at all levels
    * @type {ParsedHedTag[]}
    */
   tags
+
   /**
    * All the column splices in the string at all levels.
    * @type {ParsedHedColumnSplice[]}
    */
   columnSplices
+
   /**
    * The tags in the top-level tag groups in the string, split into arrays.
    * @type {ParsedHedTag[][]}
    */
   topLevelGroupTags
+
   /**
    * The top-level definition tag groups in the string.
    * @type {ParsedHedGroup[]}
diff --git a/src/parser/parser.js b/src/parser/parser.js
@@ -50,11 +50,12 @@ class HedStringParser {
   /**
    * Parse a full HED string.
    *
+   * @param {boolean} fullValidation - True if full full validation should be performed -- with assemploy
    * ###Note: now separates errors and warnings for easier handling.
    *
    * @returns {Array} - [ParsedHedString|null, Issue[], Issue[]] representing the parsed HED string and any parsing issues.
    */
-  parse() {
+  parse(fullValidation) {
     if (this.hedString === null || this.hedString === undefined) {
       return [null, [generateIssue('invalidTagString', {})], []]
     }
@@ -89,7 +90,7 @@ class HedStringParser {
     }
 
     // Check the other reserved tags requirements
-    const checkIssues = ReservedChecker.getInstance().checkHedString(parsedString)
+    const checkIssues = ReservedChecker.getInstance().checkHedString(parsedString, fullValidation)
     if (checkIssues.length > 0) {
       return [null, checkIssues, []]
     }
@@ -182,10 +183,11 @@ class HedStringParser {
  * @param {Schemas} hedSchemas - The collection of HED schemas.
  * @param {boolean} definitionsAllowed - True if definitions are allowed.
  * @param {boolean} placeholdersAllowed - True if placeholders are allowed.
+ * @param {boolean} fullValidation - True if full validation is required.
  * @returns {Array} - [ParsedHedString, Issue[], Issue[]] representing the parsed HED string and any issues found.
  */
-export function parseHedString(hedString, hedSchemas, definitionsAllowed, placeholdersAllowed) {
-  return new HedStringParser(hedString, hedSchemas, definitionsAllowed, placeholdersAllowed).parse()
+export function parseHedString(hedString, hedSchemas, definitionsAllowed, placeholdersAllowed, fullValidation) {
+  return new HedStringParser(hedString, hedSchemas, definitionsAllowed, placeholdersAllowed).parse(fullValidation)
 }
 
 /**
@@ -199,6 +201,12 @@ export function parseHedString(hedString, hedSchemas, definitionsAllowed, placeh
  * @param {boolean} placeholdersAllowed - True if placeholders are allowed
  * @returns {Array} - [ParsedHedString[], Issue[], Issue[]] representing the parsed HED strings and any issues found.
  */
-export function parseHedStrings(hedStrings, hedSchemas, definitionsAllowed, placeholdersAllowed) {
-  return HedStringParser.parseHedStrings(hedStrings, hedSchemas, definitionsAllowed, placeholdersAllowed)
+export function parseHedStrings(hedStrings, hedSchemas, definitionsAllowed, placeholdersAllowed, fullValidation) {
+  return HedStringParser.parseHedStrings(
+    hedStrings,
+    hedSchemas,
+    definitionsAllowed,
+    placeholdersAllowed,
+    fullValidation,
+  )
 }
diff --git a/src/parser/reservedChecker.js b/src/parser/reservedChecker.js
@@ -45,12 +45,13 @@ export class ReservedChecker {
    * Perform syntactical checks on the provided HED string to detect violations.
    *
    * @param {ParsedHedString} hedString - The HED string to be checked.
+   * @param {boolean} fullValidation - If true, perform full validation; otherwise, perform a quick check.
    * @returns {Issue[]} - An array of issues if violations are found otherwise, an empty array.
    */
-  checkHedString(hedString) {
+  checkHedString(hedString, fullValidation) {
     const checks = [
       () => this.checkUnique(hedString),
-      () => this.checkTagGroupLevels(hedString),
+      () => this.checkTagGroupLevels(hedString, fullValidation),
       () => this.checkTopGroupRequirements(hedString),
       () => this.checkNonTopGroups(hedString),
     ]
@@ -85,9 +86,10 @@ export class ReservedChecker {
    * Check whether tags are not in groups -- or top-level groups as required
    *
    * @param {ParsedHedString} hedString - The HED string to be checked for reserved tag syntax.
+   * @param {boolean} fullValidation - If true, perform full validation; otherwise, perform a quick check.
    * @returns {Issue[]} An array of `Issue` objects if there are violations; otherwise, an empty array.
    */
-  checkTagGroupLevels(hedString) {
+  checkTagGroupLevels(hedString, fullValidation) {
     const issues = []
     const topGroupTags = hedString.topLevelGroupTags.flat()
 
@@ -98,14 +100,22 @@ export class ReservedChecker {
         return
       }
 
-      // If this is a top tag group tag, we know it isn't in a top tag group.
-      if (ReservedChecker.hasTopLevelTagGroupAttribute(tag)) {
-        issues.push(generateIssue('invalidTopLevelTagGroupTag', { tag: tag.originalTag, string: hedString.hedString }))
+      // This is a top-level tag group tag that is in a lower level or ungrouped top level
+      if (
+        ReservedChecker.hasTopLevelTagGroupAttribute(tag) &&
+        (!hedString.topLevelTags.includes(tag) || fullValidation)
+      ) {
+        issues.push(
+          generateIssue('invalidTopLevelTagGroupTag', {
+            tag: tag.originalTag,
+            string: hedString.hedString,
+          }),
+        )
         return
       }
 
       // In final form --- if not in a group (not just a top group) but has the group tag attribute
-      if (hedString.topLevelTags.includes(tag) && ReservedChecker.hasGroupAttribute(tag)) {
+      if (hedString.topLevelTags.includes(tag) && ReservedChecker.hasGroupAttribute(tag) && fullValidation) {
         issues.push(generateIssue('missingTagGroup', { tag: tag.originalTag, string: hedString.hedString }))
       }
     })
diff --git a/tests/bidsTests.spec.js b/tests/bidsTests.spec.js
@@ -14,7 +14,7 @@ import { DefinitionManager } from '../src/parser/definitionManager'
 //const skipMap = new Map([['definition-tests', ['invalid-missing-definition-for-def', 'invalid-nested-definition']]])
 const skipMap = new Map()
 const runAll = true
-const runMap = new Map([['curly-brace-tests', ['valid-curly-brace-in-sidecar-with-value-splice']]])
+const runMap = new Map([['curly-brace-tests', ['splice-of-top-level-tag']]])
 
 describe('BIDS validation', () => {
   const schemaMap = new Map([['8.3.0', undefined]])
diff --git a/tests/stringParserTests.spec.js b/tests/stringParserTests.spec.js
@@ -13,7 +13,7 @@ import { shouldRun, getHedString } from './testUtilities'
 const skipMap = new Map()
 const runAll = true
 //const runMap = new Map([['valid-tags', ['single-tag-extension']]])
-const runMap = new Map([['valid-tags', ['deprecated-tag']]])
+const runMap = new Map([['special-tag-group-tests', ['event-context-in-subgroup']]])
 
 describe('Null schema objects should cause parsing to bail', () => {
   it('Should not proceed if no schema and valid string', () => {
@@ -82,6 +82,7 @@ describe('Parse HED string tests', () => {
           thisSchema,
           test.definitionsAllowed,
           test.placeholdersAllowed,
+          test.fullValidation,
         )
       } catch (error) {
         issues = [error.issue]
diff --git a/tests/testData/bidsTests.data.js b/tests/testData/bidsTests.data.js
@@ -596,6 +596,90 @@ export const bidsTestData = [
         tsvErrors: [],
         comboErrors: [],
       },
+      {
+        testname: 'splice-of-top-level-tag',
+        explanation: 'A top-level tag group tag is part of a splice',
+        schemaVersion: '8.3.0',
+        definitions: ['(Definition/Acc/#, (Acceleration/# m-per-s^2, Red))', '(Definition/MyColor, (Label/Pie))'],
+        sidecar: {
+          duration: {
+            HED: 'Duration/#',
+          },
+          event_code: {
+            HED: {
+              face: '({duration}, ((Red, Blue), {ball_type}))',
+              ball: '{ball_type}, Black',
+            },
+          },
+          ball_type: {
+            Description: 'Has description with HED',
+            HED: 'Label/#',
+          },
+        },
+        eventsString: 'onset\tduration\tevent_code\tball_type\n' + '19\t6\tball\tbig-one\n25\t5\tface\tother-one\n',
+        sidecarErrors: [],
+        tsvErrors: [],
+        comboErrors: [],
+      },
+      {
+        testname: 'bad-splice-of-top-level-tag',
+        explanation: 'A top-level tag group tag is part of a splice but is invalid',
+        schemaVersion: '8.3.0',
+        definitions: ['(Definition/Acc/#, (Acceleration/# m-per-s^2, Red))', '(Definition/MyColor, (Label/Pie))'],
+        sidecar: {
+          duration: {
+            HED: '(Duration/#, (Red, Blue))',
+          },
+          event_code: {
+            HED: {
+              face: '({duration}, ((Red, Blue), {ball_type}))',
+              ball: '{ball_type}, Black',
+            },
+          },
+          ball_type: {
+            Description: 'Has description with HED',
+            HED: 'Label/#',
+          },
+        },
+        eventsString: 'onset\tduration\tevent_code\tball_type\n' + '19\t6\tball\tbig-one\n25\t5\tface\tother-one\n',
+        sidecarErrors: [],
+        tsvErrors: [],
+        comboErrors: [
+          BidsHedIssue.fromHedIssue(
+            generateIssue('invalidTopLevelTagGroupTag', {
+              tag: 'Duration/5',
+              string: '((Duration/5, (Red, Blue)), ((Red, Blue), Label/other-one))',
+            }),
+            { path: 'bad-splice-of-top-level-tag.tsv', relativePath: 'bad-splice-of-top-level-tag.tsv' },
+            { tsvLine: 3 },
+          ),
+        ],
+      },
+      {
+        testname: 'splice-of-non-top-level-tag',
+        explanation: 'A top-level tag group tag is part of a splice',
+        schemaVersion: '8.3.0',
+        definitions: ['(Definition/Acc/#, (Acceleration/# m-per-s^2, Red))', '(Definition/MyColor, (Label/Pie))'],
+        sidecar: {
+          duration: {
+            HED: 'Parameter-value/#',
+          },
+          event_code: {
+            HED: {
+              face: '({duration}, ((Red, Blue), {ball_type}))',
+              ball: '{ball_type}, Black',
+            },
+          },
+          ball_type: {
+            Description: 'Has description with HED',
+            HED: 'Label/#',
+          },
+        },
+        eventsString: 'onset\tduration\tevent_code\tball_type\n' + '19\t6\tball\tbig-one\n25\t5\tface\tother-one\n',
+        sidecarErrors: [],
+        tsvErrors: [],
+        comboErrors: [],
+      },
       {
         testname: 'valid-curly-brace-in-sidecar-with-tsv-n/a',
         explanation: 'Valid curly brace in sidecar and valid tsv with n/a',
diff --git a/tests/testData/stringParserTests.data.js b/tests/testData/stringParserTests.data.js

Original file line number	Diff line number	Diff line change
`@@ -353,6 +353,7 @@ export class BidsHedTsvParser {`
`353`	`353`	`this.hedSchemas,`
`354`	`354`	`false,`
`355`	`355`	`false,`
	`356`	`+ true,`
`356`	`357`	`)`
`357`	`358`	`element.parsedHedString = parsedHedString`
`358`	`359`	`errors.push(...BidsHedIssue.fromHedIssues(errorIssues, this.tsvFile.file, { tsvLine: element.tsvLine }))`