Merge pull request #254 from effigies/feat/custom-columns

rf: Validate additional columns when defined

Author: rwblair

changelog.d/20250909_143325_markiewicz_custom_columns.md

@@ -0,0 +1,48 @@
+<!--
+A new scriv changelog fragment.
+
+Uncomment the section that is right (remove the HTML comment wrapper).
+For top level release notes, leave all the headers commented out.
+-->
+
+### Added
+
+- Additional (dataset-defined) columns are now validated against the
+  definitions in the sidecar files.
+
+<!--
+### Changed
+
+- A bullet item for the Changed category.
+
+-->
+<!--
+### Fixed
+
+- A bullet item for the Fixed category.
+
+-->
+<!--
+### Deprecated
+
+- A bullet item for the Deprecated category.
+
+-->
+<!--
+### Removed
+
+- A bullet item for the Removed category.
+
+-->
+<!--
+### Security
+
+- A bullet item for the Security category.
+
+-->
+<!--
+### Infrastructure
+
+- A bullet item for the Infrastructure category.
+
+-->

src/schema/applyRules.ts

@@ -6,7 +6,6 @@ import { logger } from '../utils/logger.ts'
 import { compile } from '../validators/json.ts'
 import type { DefinedError } from '@ajv'
 import {
-  evalAdditionalColumns,
   evalColumns,
   evalIndexColumns,
   evalInitialColumns,
@@ -123,8 +122,6 @@ const evalMap: Record<
   // @ts-expect-error
   columns: evalColumns,
   // @ts-expect-error
-  additional_columns: evalAdditionalColumns,
-  // @ts-expect-error
   initial_columns: evalInitialColumns,
   // @ts-expect-error
   index_columns: evalIndexColumns,

src/schema/tables.test.ts

@@ -2,7 +2,6 @@
 import { assertEquals } from '@std/assert'
 import { loadSchema } from '../setup/loadSchema.ts'
 import {
-  evalAdditionalColumns,
   evalColumns,
   evalIndexColumns,
   evalInitialColumns,
@@ -186,6 +185,48 @@ Deno.test('tables eval* tests', async (t) => {
     assertEquals(issues.length, 1)
   })
 
+  await t.step('validate additional column with sidecar', () => {
+    const context = {
+      path: '/participants.tsv',
+      extension: '.tsv',
+      sidecar: {
+        myAge: {
+          Description: 'Age in weeks',
+          Format: 'number',
+        },
+        mySex: {
+          Description: 'Phenotypic sex',
+          Format: 'string',
+          Levels: {
+            'F': { Description: 'Female' },
+            'M': { Description: 'Male' },
+            'O': { Description: 'Other' },
+          },
+        },
+      },
+      sidecarKeyOrigin: {
+        myAge: '/participants.json',
+        mySex: '/participants.json',
+      },
+      columns: new ColumnsMap(Object.entries({
+        participant_id: ['sub-01', 'sub-02', 'sub-03'],
+        myAge: ['10', '20', '89+'],
+        mySex: ['M', 'F', 'O'],
+      })),
+      dataset: { issues: new DatasetIssues() },
+    }
+    const rule = schemaDefs.rules.tabular_data.modality_agnostic.Participants
+    evalColumns(rule, context, schema, 'rules.tabular_data.modality_agnostic.Participants')
+
+    // myAge does not get the pseudo-age warning
+    // mySex doesn't raise issues
+    const issues = context.dataset.issues.get({ code: 'TSV_VALUE_INCORRECT_TYPE' })
+    assertEquals(issues.length, 1)
+    assertEquals(issues[0].subCode, 'myAge')
+    assertEquals(issues[0].line, 4)
+    assertEquals(issues[0].issueMessage, "'89+'")
+  })
+
   await t.step('verify column ordering', () => {
     const context = {
       path: '/sub-01/sub-01_scans.tsv',
@@ -246,11 +287,11 @@ Deno.test('tables eval* tests', async (t) => {
       dataset: { issues: new DatasetIssues() },
     }
     const rule = schemaDefs.rules.tabular_data.made_up.MadeUp
-    evalAdditionalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
+    evalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
     assertEquals(context.dataset.issues.size, 0)
 
     context.columns['extra'] = [1, 2, 3]
-    evalAdditionalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
+    evalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
     assertEquals(
       context.dataset.issues.get({ code: 'TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED' }).length,
       1,
@@ -270,18 +311,18 @@ Deno.test('tables eval* tests', async (t) => {
     }
     const rule = schemaDefs.rules.tabular_data.made_up.MadeUp
     rule.additional_columns = 'allowed_if_defined'
-    evalAdditionalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
+    evalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
     assertEquals(context.dataset.issues.size, 0)
 
     context['sidecar'] = {}
-    evalAdditionalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
+    evalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
     assertEquals(
       context.dataset.issues.get({ code: 'TSV_ADDITIONAL_COLUMNS_MUST_DEFINE' }).length,
       1,
     )
 
     rule.additional_columns = 'allowed'
-    evalAdditionalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
+    evalColumns(rule, context, schema, 'rules.tabular_data.made_up.MadeUp')
     assertEquals(
       context.dataset.issues.get({ code: 'TSV_ADDITIONAL_COLUMNS_UNDEFINED' }).length,
       1,

src/schema/tables.ts

@@ -218,46 +218,56 @@ export function evalColumns(
   schemaPath: string,
 ): void {
   if (!rule.columns || !['.tsv', '.tsv.gz'].includes(context.extension)) return
-  const headers = [...Object.keys(context.columns)]
-  for (const [ruleHeader, requirement] of Object.entries(rule.columns)) {
-    const columnObject: ColumnSchema = schema.objects.columns[ruleHeader]
+  const columns = rule.columns as Record<string, string | { level: string }>
 
-    // What is this?
-    if (!('name' in columnObject) || !columnObject['name']) {
-      return
-    }
+  const columnLookup = Object.fromEntries(
+    Object.keys(rule.columns).map((col) => [schema.objects.columns[col].name, col]),
+  )
+  const additionalColumns = Object.keys(context.columns).filter((name) => !(name in columnLookup))
 
-    const name = columnObject.name
-    if (!headers.includes(name)) {
-      if (
-        getRequirement(rule as ColumnRule, ruleHeader) === 'required' &&
-        // A more precise error will be generated by evalInitialColumns
-        !(rule?.initial_columns && rule.initial_columns.includes(ruleHeader))
-      ) {
-        context.dataset.issues.add({
-          code: 'TSV_COLUMN_MISSING',
-          subCode: name,
-          location: context.path,
-          rule: schemaPath,
-        })
-      }
-      return
-    }
+  for (const name of new Set([...Object.keys(columnLookup), ...additionalColumns])) {
+    const ruleHeader = columnLookup[name]
+    const sidecarDef = context.sidecar[name] as ColumnDefinition | undefined
+    const requirement = getRequirement(rule as ColumnRule, ruleHeader)
+    const column = context.columns[name] as string[]
+
+    const issue = { subCode: name, location: context.path, rule: schemaPath }
 
     let signature: ValueSignature
-    try {
-      signature = getValueSignature(columnObject, context?.sidecar[name])
-    } catch (e: any) {
-      if (e?.code) {
-        context.dataset.issues.add({
-          ...e,
-          subCode: name,
-          location: context.sidecarKeyOrigin[name],
-          rule: schemaPath,
-        })
-        signature = getValueSignature(columnObject, undefined)
-      } else {
-        throw e
+    if (!ruleHeader) { // Additional column
+      if (requirement === 'not_allowed' || !sidecarDef) {
+        const code = requirement === 'not_allowed'
+          ? 'TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED'
+          : requirement === 'allowed_if_defined'
+          ? 'TSV_ADDITIONAL_COLUMNS_MUST_DEFINE'
+          : 'TSV_ADDITIONAL_COLUMNS_UNDEFINED'
+        context.dataset.issues.add({ code, ...issue })
+        continue
+      }
+      signature = extractDefinition(sidecarDef)
+    } else {
+      const columnObject = schema.objects.columns[ruleHeader]
+      if (!column) {
+        if (requirement === 'required' && !(rule?.initial_columns?.includes(ruleHeader))) {
+          context.dataset.issues.add({ code: 'TSV_COLUMN_MISSING', ...issue })
+        }
+        continue
+      }
+
+      try {
+        signature = getValueSignature(columnObject, sidecarDef)
+      } catch (e: any) {
+        if (e?.code) {
+          context.dataset.issues.add({
+            ...e,
+            subCode: name,
+            location: context.sidecarKeyOrigin[name],
+            rule: schemaPath,
+          })
+          signature = extractSchema(columnObject)
+        } else {
+          throw e
+        }
       }
     }
 
@@ -267,17 +277,9 @@ export function evalColumns(
 
     const spec = compileSignature(signature, schema.objects.formats)
 
-    const issue = {
-      code: 'TSV_VALUE_INCORRECT_TYPE',
-      subCode: name,
-      location: context.path,
-      rule: schemaPath,
-    }
-
     const ageCheck = name === 'age'
     let ageWarningIssued = false
 
-    const column = context.columns[name] as string[]
     for (const [index, value] of column.entries()) {
       if (!checkValue(value, spec)) {
         if (ageCheck && value === '89+') {
@@ -295,6 +297,7 @@ export function evalColumns(
           (value.match(/^\s*(NA|na|nan|NaN|n\/a)\s*$/) ? ", did you mean 'n/a'?" : '')
         context.dataset.issues.add({
           ...issue,
+          code: 'TSV_VALUE_INCORRECT_TYPE',
           line: index + 2,
           issueMessage,
         })
@@ -352,46 +355,6 @@ export function evalInitialColumns(
   })
 }
 
-export function evalAdditionalColumns(
-  rule: GenericRule,
-  context: BIDSContext,
-  schema: Schema,
-  schemaPath: string,
-): void {
-  if (!['.tsv', '.tsv.gz'].includes(context.extension)) return
-  const headers = Object.keys(context?.columns)
-  if (rule.columns) {
-    if (!rule.additional_columns || rule.additional_columns === 'n/a') {
-      // Old schemas might be missing the field, so be permissive.
-      // New schemas indicate it is not applicable with 'n/a'.
-      return
-    }
-    const ruleHeadersNames = Object.keys(rule.columns).map(
-      (x) => schema.objects.columns[x].name,
-    )
-    let extraCols = headers.filter(
-      (header) => !ruleHeadersNames.includes(header),
-    )
-
-    if (rule.additional_columns?.startsWith('allowed')) {
-      extraCols = extraCols.filter((header) => !(header in context.sidecar))
-    }
-    const code = rule.additional_columns === 'not_allowed'
-      ? 'TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED'
-      : rule.additional_columns === 'allowed_if_defined'
-      ? 'TSV_ADDITIONAL_COLUMNS_MUST_DEFINE'
-      : 'TSV_ADDITIONAL_COLUMNS_UNDEFINED'
-    const issue = {
-      code,
-      location: context.path,
-      rule: schemaPath,
-    }
-    for (const col of extraCols) {
-      context.dataset.issues.add({ ...issue, subCode: col })
-    }
-  }
-}
-
 export function evalIndexColumns(
   rule: GenericRule,
   context: BIDSContext,