diff --git a/changelog.d/20260421_095017_yarikoptic_bf_recurse.md b/changelog.d/20260421_095017_yarikoptic_bf_recurse.md new file mode 100644 index 000000000..85e4a4825 --- /dev/null +++ b/changelog.d/20260421_095017_yarikoptic_bf_recurse.md @@ -0,0 +1,26 @@ +### Changed + +- Nested BIDS datasets discovered under `derivatives/`, `rawbids/`, or + `sourcedata/` are now recursed into with `-r`, whether the container + directory itself is a BIDS dataset (e.g. `rawbids/dataset_description.json`) + or it holds one or more BIDS datasets as immediate subdirectories + (e.g. `derivatives/fmriprep/dataset_description.json`, + `sourcedata/ds00003/dataset_description.json`). In the returned + `ValidationResult`, derivatives populate `derivativesSummary` (as + before) and `rawbids/`/`sourcedata/` nested datasets populate the new + `sourcesSummary` field. In text output they render as `Derivative:` + and `Source:` sections respectively. Sections are now printed in + bottom-up order — Sources (`sourcedata/` before `rawbids/`) → + Derivatives → Root dataset — so the parent dataset's status and a + roll-up of nested errors/warnings appear at the end of the output. + See [#390](https://github.com/bids-standard/bids-validator/pull/390). + +### Fixed + +- Normalize an invalid `DatasetType` value (not one of `raw`, `derivative`, + `study`) to the spec default for internal rule lookups so + `rules.directories[DatasetType]` resolves and downstream validation does + not cascade into spurious `NOT_INCLUDED` errors on legitimate subjects. + The original value in `dataset_description.json` is preserved so JSON + schema validation still flags the invalid enum value. See + [#390](https://github.com/bids-standard/bids-validator/pull/390). diff --git a/src/schema/context.test.ts b/src/schema/context.test.ts index 480ed3e43..5566fb3d5 100644 --- a/src/schema/context.test.ts +++ b/src/schema/context.test.ts @@ -44,7 +44,10 @@ Deno.test('test context LoadSidecar', async (t) => { Deno.test('BIDSContextDataset opaqueDirectories respects DatasetType', async (t) => { const schema = { - objects: { extensions: {} }, + objects: { + extensions: {}, + metadata: { DatasetType: { enum: ['raw', 'derivative'] } }, + }, rules: { directories: { raw: { @@ -104,6 +107,26 @@ Deno.test('BIDSContextDataset opaqueDirectories respects DatasetType', async (t) assertEquals(ds.opaqueDirectories.has('/c'), true) assertEquals(ds.opaqueDirectories.has('/a'), false) }) + + await t.step('invalid DatasetType normalizes to raw without GeneratedBy', () => { + const ds = new BIDSContextDataset({ + schema, + dataset_description: { DatasetType: 'Raw' }, + }) + assertEquals(ds.dataset_description.DatasetType, 'raw') + assertEquals(ds.opaqueDirectories.has('/a'), true) + assertEquals(ds.opaqueDirectories.has('/c'), false) + }) + + await t.step('invalid DatasetType normalizes to derivative with GeneratedBy', () => { + const ds = new BIDSContextDataset({ + schema, + dataset_description: { DatasetType: 'Derivative', GeneratedBy: [{ Name: 'x' }] }, + }) + assertEquals(ds.dataset_description.DatasetType, 'derivative') + assertEquals(ds.opaqueDirectories.has('/c'), true) + assertEquals(ds.opaqueDirectories.has('/a'), false) + }) }) Deno.test('test context loadColumns for headerless motion.tsv', async (t) => { diff --git a/src/schema/context.ts b/src/schema/context.ts index 3dbadcd16..dc1ab697e 100644 --- a/src/schema/context.ts +++ b/src/schema/context.ts @@ -77,12 +77,17 @@ export class BIDSContextDataset implements Dataset { } set dataset_description(value: Record) { - this.#dataset_description = value - if (!this.dataset_description.DatasetType) { - this.dataset_description.DatasetType = this.dataset_description.GeneratedBy - ? 'derivative' - : 'raw' + // Shallow copy: don't mutate the memoized JSON; schema validation still + // flags enum violations on the original. + const copy: Record = { ...value } + // @ts-expect-error metadata is not declared on SchemaObjects + const validTypes = this.schema?.objects?.metadata?.DatasetType?.enum as string[] | undefined + // Unknown/mis-cased DatasetType falls back to the spec default; legacy + // GeneratedBy-without-DatasetType still infers 'derivative'. + if (validTypes && !validTypes.includes(copy.DatasetType as string)) { + copy.DatasetType = copy.GeneratedBy ? 'derivative' : 'raw' } + this.#dataset_description = copy const datasetType = this.dataset_description.DatasetType as string this.opaqueDirectories = new Set( Object.values(this.schema?.rules?.directories[datasetType] ?? {}) diff --git a/src/summary/summary.test.ts b/src/summary/summary.test.ts index 4ec651fc0..6067a7b0f 100644 --- a/src/summary/summary.test.ts +++ b/src/summary/summary.test.ts @@ -140,3 +140,25 @@ Deno.test('detectErrors returns false when no errors found', () => { const errors = detectErrors(mockValidationResultNoErrors) assertEquals(errors, false) }) + +const mockValidationResultBadSources: ValidationResult = { + issues: new DatasetIssues({ + issues: [], + codeMessages: new Map(), + }), + summary: json_mock_validation_result.summary, + sourcesSummary: { + '/sourcedata/mock/': { + issues: new DatasetIssues({ + issues: json_mock_validation_result.derivativesSummary['/derivatives/mock/'].issues + .issues as Issue[], + }), + summary: json_mock_validation_result.derivativesSummary['/derivatives/mock/'].summary, + }, + }, +} + +Deno.test('detectErrors collects errors from nested sources', () => { + const errors = detectErrors(mockValidationResultBadSources) + assertEquals(errors, true) +}) diff --git a/src/summary/summary.ts b/src/summary/summary.ts index 151cfda73..4232bb42b 100644 --- a/src/summary/summary.ts +++ b/src/summary/summary.ts @@ -182,5 +182,6 @@ export class Summary { */ export function detectErrors(result: ValidationResult): boolean { return result.issues.get({ severity: 'error' }).length > 0 || - Object.values(result.derivativesSummary ?? {}).some((res) => detectErrors(res)) + Object.values(result.derivativesSummary ?? {}).some((res) => detectErrors(res)) || + Object.values(result.sourcesSummary ?? {}).some((res) => detectErrors(res)) } diff --git a/src/tests/local/derivatives.test.ts b/src/tests/local/derivatives.test.ts index 92a113433..a3d007da9 100644 --- a/src/tests/local/derivatives.test.ts +++ b/src/tests/local/derivatives.test.ts @@ -11,3 +11,16 @@ Deno.test('recursive option works as expected', async (t) => { result = await validatePath(t, path, options) assert(Object.hasOwn(result.result, 'derivativesSummary')) }) + +Deno.test('recursive option populates sourcesSummary from sourcedata/', async (t) => { + // atlas-4S has sourcedata/atlas-4S/dataset_description.json — the + // multiple-nested shape under sourcedata/. + const path = 'tests/data/bids-examples/atlas-4S/' + const options = await parseOptions(['fake_dataset_arg', ...Deno.args, '-r']) + const result = await validatePath(t, path, options) + assert(Object.hasOwn(result.result, 'sourcesSummary')) + assert( + Object.keys(result.result.sourcesSummary ?? {}).some((k) => k.includes('sourcedata')), + 'expected a sourcedata/ entry in sourcesSummary', + ) +}) diff --git a/src/types/validation-result.ts b/src/types/validation-result.ts index 7953ba5bc..d9d2698f4 100644 --- a/src/types/validation-result.ts +++ b/src/types/validation-result.ts @@ -67,4 +67,7 @@ export interface ValidationResult { * `dataset_description.json`. */ derivativesSummary?: Record + /** Per-source validation results (nested datasets under `rawbids/` or + * `sourcedata/`), keyed by their relative path. */ + sourcesSummary?: Record } diff --git a/src/utils/output.ts b/src/utils/output.ts index 4dec0f737..d158fb5c8 100644 --- a/src/utils/output.ts +++ b/src/utils/output.ts @@ -5,6 +5,7 @@ import { Table } from '@cliffy/table' import * as colors from '@std/fmt/colors' import { format as prettyBytes } from '@std/fmt/bytes' import { marked } from 'marked' +import pluralize from 'pluralize' import supportsHyperlinks from 'supports-hyperlinks' import type { SummaryOutput, ValidationResult } from '../types/validation-result.ts' import type { Issue, Severity } from '../types/issues.ts' @@ -28,14 +29,59 @@ interface LoggingOptions { * @param result - The validation result to format. * @param options - Logging options; `verbose` includes extra detail per issue. * @returns The full ANSI-coloured output string with newlines. + * + * Output order: nested datasets first (Sources before Derivatives; + * within Sources, `sourcedata/` entries before `rawbids/`), then the + * root dataset itself with a roll-up of nested-dataset issue counts. + * The root section comes last so a quick `tail` shows the + * top-level dataset's status and aggregate health at a glance. */ export function consoleFormat( result: ValidationResult, options?: LoggingOptions, ): string { - const output = [] + const output: string[] = [] + const formatNested = ( + label: string, + nested: Record, + keyOrder: string[], + ) => { + for (const key of keyOrder) { + const nestedResult = nested[key] + if (!nestedResult) continue + output.push(colors.blue(`${label}: ${key}`)) + + if (nestedResult.issues.size === 0) { + output.push(colors.green(`\tThis ${label.toLowerCase()} appears to be BIDS compatible.`)) + } else { + ;(['warning', 'error'] as Severity[]).map((severity) => { + output.push(...formatIssues(nestedResult.issues.filter({ severity }), options, severity)) + }) + } + output.push(formatSummary(nestedResult.summary)) + output.push('') + } + } + + // Sources: sourcedata first, then rawbids; alphabetical within each group. + if (result.sourcesSummary) { + const sourceKeys = Object.keys(result.sourcesSummary).sort((a, b) => { + const rank = (k: string) => + k.startsWith('/sourcedata') ? 0 : k.startsWith('/rawbids') ? 1 : 2 + return rank(a) - rank(b) || a.localeCompare(b) + }) + formatNested('Source', result.sourcesSummary, sourceKeys) + } + // Derivatives: alphabetical. + if (result.derivativesSummary) { + const derivKeys = Object.keys(result.derivativesSummary).sort() + formatNested('Derivative', result.derivativesSummary, derivKeys) + } + + // Root dataset section, last. + output.push(colors.blue('Root dataset:')) if (result.issues.size === 0) { - output.push(colors.green('This dataset appears to be BIDS compatible.')) + output.push(colors.green('\tThis dataset appears to be BIDS compatible.')) } else { ;(['warning', 'error'] as Severity[]).map((severity) => { output.push(...formatIssues(result.issues.filter({ severity }), options, severity)) @@ -43,26 +89,53 @@ export function consoleFormat( } output.push('') output.push(formatSummary(result.summary)) - output.push('') - if (result.derivativesSummary) { - for (const [key, derivResult] of Object.entries(result.derivativesSummary)) { - output.push(colors.blue(`Derivative: ${key}`)) - if (derivResult.issues.size === 0) { - output.push(colors.green('\tThis derivative appears to be BIDS compatible.')) - } else { - ;(['warning', 'error'] as Severity[]).map((severity) => { - output.push(...formatIssues(derivResult.issues.filter({ severity }), options, severity)) - }) - } - output.push(formatSummary(derivResult.summary)) - output.push('') - } + // Roll-up of nested-dataset issue counts, embedded in the root section. + const rollup = formatNestedRollup(result) + if (rollup) { + output.push(rollup) } return output.join('\n') } +/** Tally errors/warnings across nested datasets and render a brief table. */ +function formatNestedRollup(result: ValidationResult): string { + const tally = (nested: Record | undefined) => { + if (!nested) return undefined + let errors = 0 + let warnings = 0 + for (const r of Object.values(nested)) { + errors += r.issues.get({ severity: 'error' }).length + warnings += r.issues.get({ severity: 'warning' }).length + } + return { count: Object.keys(nested).length, errors, warnings } + } + const sources = tally(result.sourcesSummary) + const derivs = tally(result.derivativesSummary) + if (!sources && !derivs) return '' + + const rows: string[][] = [] + const row = (label: string, t: { count: number; errors: number; warnings: number }) => [ + colors.magenta(label), + pluralize('dataset', t.count, true), + pluralize('error', t.errors, true), + pluralize('warning', t.warnings, true), + ] + if (sources) { + rows.push(row('Sources:', sources)) + } + if (derivs) { + rows.push(row('Derivatives:', derivs)) + } + return [ + '', + colors.magenta('Nested datasets summary:'), + new Table().body(rows).border(false).padding(2).indent(2).toString(), + '', + ].join('\n') +} + /** * Render marked tokens to ANSI strings */ diff --git a/src/validators/bids.ts b/src/validators/bids.ts index 86584c210..0fb499d9b 100644 --- a/src/validators/bids.ts +++ b/src/validators/bids.ts @@ -43,12 +43,17 @@ const perDSChecks: DSCheckFunction[] = [ * * Loads the BIDS schema, walks the file tree, and applies file-level and * dataset-level checks, accumulating any issues into the returned - * {@link ValidationResult}. Derivative datasets nested under - * `derivatives/` are detected via their own `dataset_description.json`; - * when `options.recursive` is set, BIDS-conformant derivatives are - * validated and their results attached to `derivativesSummary` on the - * returned object. Non-BIDS derivatives and the `sourcedata`, `code` - * directories are ignored. + * {@link ValidationResult}. Nested BIDS datasets are detected via their + * own `dataset_description.json` under any of `derivatives/`, + * `rawbids/`, or `sourcedata/` — either at the container's immediate + * level (e.g. `rawbids/dataset_description.json`) or one level deeper + * (e.g. `derivatives/fmriprep/dataset_description.json`). When + * `options.recursive` is set, each nested BIDS dataset is validated + * and its result is attached to the returned object: + * - Derivatives under `derivatives/` go into `derivativesSummary`. + * - Sources under `rawbids/` or `sourcedata/` go into `sourcesSummary`. + * The `code` directory is always ignored, as are non-BIDS contents of + * the nesting containers. * * `validate` does not throw on validation failures — it records them as * issues on the result. The returned `issues` collection can be filtered @@ -126,23 +131,43 @@ export async function validate( } } + // Directories that may contain nested BIDS datasets — either the directory + // itself is a BIDS dataset (e.g. `rawbids/dataset_description.json`) or its + // immediate subdirectories are (e.g. `derivatives/fmriprep/`, + // `sourcedata/ds00003/`). Split into two buckets so the returned + // ValidationResult can separate derivatives from sources. + type NestedBucket = 'derivatives' | 'sources' + const nestingContainerBucket: Record = { + derivatives: 'derivatives', + rawbids: 'sources', + sourcedata: 'sources', + } const bidsDerivatives: Promise[] = [] + const bidsSources: Promise[] = [] + // TODO: include in dataset-wide totals (file count / size). See #390 review. const nonstdDerivatives: FileTree[] = [] fileTree.directories = fileTree.directories.filter((dir) => { - if (['sourcedata', 'code'].includes(dir.name)) { + if (dir.name === 'code') { return false } - if (dir.name !== 'derivatives') { + const bucket = nestingContainerBucket[dir.name] + if (!bucket) { return true } - for (const deriv of dir.directories) { - if (deriv.get('dataset_description.json')) { - bidsDerivatives.push(subtree(deriv)) - } else { - nonstdDerivatives.push(deriv) + const collect = bucket === 'derivatives' ? bidsDerivatives : bidsSources + if (dir.get('dataset_description.json')) { + collect.push(subtree(dir)) + } else { + for (const sub of dir.directories) { + if (sub.get('dataset_description.json')) { + collect.push(subtree(sub)) + } else { + nonstdDerivatives.push(sub) + } } } - // Remove derivatives from the main fileTree + // Always remove nesting containers from the main fileTree; their own + // validation (if any) happens recursively below. return false }) @@ -195,14 +220,23 @@ export async function validate( }) const derivativesSummary: Record = {} + const sourcesSummary: Record = {} if (options.recursive) { - await Promise.allSettled( - bidsDerivatives.map(async (promise) => { - const deriv = await promise - derivativesSummary[deriv.name] = await validate(deriv, options) - return derivativesSummary[deriv.name] - }), - ) + const validateInto = ( + trees: Promise[], + into: Record, + ) => + Promise.allSettled( + trees.map(async (promise) => { + const nested = await promise + into[nested.name] = await validate(nested, options) + return into[nested.name] + }), + ) + await Promise.all([ + validateInto(bidsDerivatives, derivativesSummary), + validateInto(bidsSources, sourcesSummary), + ]) } if (config) { @@ -227,5 +261,8 @@ export async function validate( if (Object.keys(derivativesSummary).length) { output['derivativesSummary'] = derivativesSummary } + if (Object.keys(sourcesSummary).length) { + output['sourcesSummary'] = sourcesSummary + } return output }