From 6955a547ed13c4108b660fd12bdb27ec47908ce3 Mon Sep 17 00:00:00 2001 From: karellopez Date: Mon, 3 Aug 2026 15:49:34 +0200 Subject: [PATCH] feat(issues): add a findings model and schema-driven filename validation Validation currently answers a yes/no question: BIDSValidator.is_bids returns a boolean and the CLI prints one line per bad file. That output carries no severity, no stable code, and no structure a program can consume. These two modules replace it with typed findings, and give the future content checks somewhere to report to. Engine - issues.py holds the finding model: Severity, Issue, and a DatasetIssues collection. Pure attrs data with no I/O, matching the convention already used by context.py and types/files.py. The field set mirrors the reference validator so output stays interchangeable, and Issue.rule records which schema rule fired. - filename_checks.py holds the check logic, kept separate from the container so each has one job. It reads rules.files from the schema, identifies the rule or rules a path matches, then reports one specific code per kind of failure rather than a single blanket "bad name": NOT_INCLUDED, MISSING_REQUIRED_ENTITY, ENTITY_NOT_IN_RULE, ENTITY_WITH_NO_LABEL, INVALID_ENTITY_LABEL, EXTENSION_MISMATCH, DATATYPE_MISMATCH, INVALID_LOCATION, FILENAME_MISMATCH and ALL_FILENAME_RULES_HAVE_ISSUES. - Scope is names and paths only. Nothing opens a file or reads its contents. - The walk applies the reference validator's default ignores (.git**, .*, sourcedata/, code/, stimuli/, log/) in addition to .bidsignore. Without them dotfiles such as .DS_Store are reported, which the reference never does. - Directory recordings such as CTF .ds are treated as single units: the walk does not descend into them and does not name-check their contents. - Schema lookups are memoised per schema object, so the rule tree is flattened once rather than once per file, and the walk is a generator so a large dataset holds one context at a time. One deliberate difference from the reference validator - A data file outside any recognised datatype directory is reported as INVALID_LOCATION. The reference misses this case, because its suffix matching ignores the datatype and its DATATYPE_MISMATCH check is gated on the parent directory being a known datatype. The legacy is_bids regexes do catch it, since they cover the whole path, so dropping it would lose coverage users already have. Metadata files are exempt, because the inheritance principle lets a .json or .tsv sit higher in the tree than the data it describes. Tests - test_issues.py covers the model: defaults, severities, the collection helpers and a serialisation round trip. - test_filename_checks.py is table driven with one case per issue code, plus the default ignores, .bidsignore, the rule field, catalog completeness, and the datatype-directory case cross-checked against is_bids. - 28 tests pass. ruff, ruff format and mypy strict are clean. - Verified on real MRI, EEG, MEG and PET datasets: the filename findings match those of a reference-parity engine exactly. Docs - docs/filename_issues_module.md explains what the modules add, the architecture with flowcharts, the ten codes, how to run it, how content validation extends the same layer, and a technical reference covering every type and function with the reasoning behind each decision. - docs/example_filename_issues.py runs two ways: with no argument it generates a dataset containing one deliberately broken file per issue code, and with a path it validates your own dataset. --- docs/example_filename_issues.py | 103 +++++ docs/filename_issues_module.md | 461 ++++++++++++++++++++ src/bids_validator/filename_checks.py | 606 ++++++++++++++++++++++++++ src/bids_validator/issues.py | 107 +++++ tests/test_filename_checks.py | 147 +++++++ tests/test_issues.py | 61 +++ 6 files changed, 1485 insertions(+) create mode 100644 docs/example_filename_issues.py create mode 100644 docs/filename_issues_module.md create mode 100644 src/bids_validator/filename_checks.py create mode 100644 src/bids_validator/issues.py create mode 100644 tests/test_filename_checks.py create mode 100644 tests/test_issues.py diff --git a/docs/example_filename_issues.py b/docs/example_filename_issues.py new file mode 100644 index 0000000..a65c6b1 --- /dev/null +++ b/docs/example_filename_issues.py @@ -0,0 +1,103 @@ +"""Runnable example for the filename validation module. + +Two modes: + +* No argument: build a small dataset in a temporary directory that contains one + deliberately broken file per issue code, validate it, and print the findings. + Every code the module can emit is demonstrated. +* With a path: validate your own dataset. + +Usage +----- + python docs/example_filename_issues.py # generated demo + python docs/example_filename_issues.py /path/to/dataset # your own data +""" + +from __future__ import annotations + +import json +import sys +import tempfile +from pathlib import Path + +from bidsschematools.schema import load_schema + +from bids_validator.filename_checks import collect_filename_issues +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +# Correctly named files. None of these produce a finding. +VALID_FILES = ( + 'README', + 'sub-01/anat/sub-01_T1w.nii.gz', + 'sub-01/func/sub-01_task-rest_bold.nii.gz', +) + +# One broken file per issue code, with the code each one is expected to raise. +BROKEN_FILES = { + 'sub-01/notes.txt': 'NOT_INCLUDED', + 'sub-01/anat/sub-01_T1w.txt': 'EXTENSION_MISMATCH', + 'sub-01/func/sub-01_bold.nii.gz': 'MISSING_REQUIRED_ENTITY', + 'sub-01/anat/sub-01_acq-_T1w.nii.gz': 'ENTITY_WITH_NO_LABEL', + 'sub-01/anat/sub-01_acq-a!b_T1w.nii.gz': 'INVALID_ENTITY_LABEL', + 'sub-01/anat/sub-01_dir-AP_T1w.nii.gz': 'ENTITY_NOT_IN_RULE', + 'sub-01/anat/acq-x_sub-01_T1w.nii.gz': 'FILENAME_MISMATCH', + 'sub-01/func/sub-01_T1w.nii.gz': 'DATATYPE_MISMATCH', + 'sub-02/anat/sub-01_T1w.nii.gz': 'INVALID_LOCATION', + 'sub-01/sub-01_channels.tsv': 'ALL_FILENAME_RULES_HAVE_ISSUES', +} + + +def build_dataset(root: Path) -> Path: + """Create the example dataset under ``root``.""" + (root / 'dataset_description.json').write_text( + json.dumps({'Name': 'filename example', 'BIDSVersion': '1.11.1'}) + ) + for relpath in (*VALID_FILES, *BROKEN_FILES): + path = root / relpath + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b'') + return root + + +def validate(root: Path) -> None: + """Validate a dataset and print every filename finding.""" + tree = FileTree.read_from_filesystem(str(root)) + issues = collect_filename_issues(tree, load_schema()) + + errors = issues.by_severity(Severity.ERROR) + warnings = issues.by_severity(Severity.WARNING) + + print(f'dataset: {root}') + print(f'{len(issues)} finding(s): {len(errors)} error(s), {len(warnings)} warning(s)\n') + for issue in issues: + print(f'[{issue.severity.value}] {issue.code}') + print(f' file : {issue.location}') + print(f' detail : {issue.message}') + if issue.rule: + print(f' rule : {issue.rule}') + print(f'\nvalid: {"no, errors found" if issues.has_errors else "yes"}') + + +def run_demo() -> None: + """Build the generated example dataset and validate it.""" + with tempfile.TemporaryDirectory() as tmp: + root = build_dataset(Path(tmp)) + print('Generated example: one broken file per issue code.\n') + for relpath, code in BROKEN_FILES.items(): + print(f' {code:32} {relpath}') + print() + validate(root) + + +def main(argv: list[str]) -> int: + """Run the demo, or validate the dataset given as the first argument.""" + if argv: + validate(Path(argv[0])) + else: + run_demo() + return 0 + + +if __name__ == '__main__': + raise SystemExit(main(sys.argv[1:])) diff --git a/docs/filename_issues_module.md b/docs/filename_issues_module.md new file mode 100644 index 0000000..d7217cd --- /dev/null +++ b/docs/filename_issues_module.md @@ -0,0 +1,461 @@ +# The filename issues module + +Two new modules turn filename checking into structured, machine-readable findings: + +| Module | Role | +|---|---| +| `bids_validator.issues` | The **container**: what a finding is. Pure data, no I/O. | +| `bids_validator.filename_checks` | The **logic**: schema-driven filename and path checks. | + +Scope: names and paths only. Nothing here opens a file or reads its contents. + +## What this adds + +Before, the filename check answered a yes/no question. `BIDSValidator.is_bids(path)` +returned a boolean, and the command line printed a line per bad file: + +``` +/sub-01/anat/oops.nii.gz is not a valid bids filename +``` + +That output cannot tell you *why* the name is wrong, cannot be counted or filtered, +and cannot be consumed by another program. + +Now each problem is a typed `Issue` with a specific code: + +``` +[error] DATATYPE_MISMATCH + file : sub-01/func/sub-01_T1w.nii.gz + detail : the file is in 'func' but its suffix belongs in: anat + rule : rules.files.raw.anat.nonparametric +``` + +The finding says which rule was applied and what exactly failed, and it serialises +straight to JSON. + +## Architecture + +The BIDS schema describes every legal filename: which suffix belongs in which +datatype folder, which entities are required or allowed, which extensions are +permitted. The module reads those rules rather than hardcoding BIDS knowledge. + +```mermaid +flowchart TD + S["BIDS schema: rules.files, rules.entities, objects"] + F["one file path from the dataset tree, name and location only"] + M["find the matching rules"] + C["check the file against them"] + I["Issue: code, severity, location, message, rule"] + D["DatasetIssues"] + S --> M + F --> M + M --> C + C --> I + I --> D +``` + +For one file the flow is: + +```mermaid +flowchart TD + A["one file path"] --> B{"ignored by .bidsignore or a default ignore"} + B -->|"yes"| SKIP["skip it, no finding"] + B -->|"no"| C{"matches any rules.files rule"} + C -->|"no"| NI["NOT_INCLUDED"] + C -->|"yes"| N["narrow to the best candidate rule"] + N --> CH["check entities, datatype, extension, location, order"] + CH --> OK["all good: no finding"] + CH --> ISS["one specific code per failure"] +``` + +Default ignores mirror the reference TypeScript validator: `.git**`, `.*`, +`sourcedata/`, `code/`, `stimuli/`, `log/`. Directory recordings such as CTF `.ds` +are treated as single units and are not name-checked inside. + +### Where the codes come from + +Schema-defined `rules.checks` carry their own issue code, but the structural +filename failures do not exist in the schema. Their codes come from the reference +TypeScript validator's catalog (`src/issues/list.ts`). `filename_checks.FILENAME_ISSUES` +mirrors that catalog so the provenance is explicit and the output stays +interchangeable with the reference. + +## The issue codes + +All ten are errors. The reference defines no filename warnings. + +| Code | Raised when | +|---|---| +| `NOT_INCLUDED` | the name matches no BIDS rule at all | +| `MISSING_REQUIRED_ENTITY` | a required entity for that suffix is absent | +| `ENTITY_NOT_IN_RULE` | an entity is not allowed for that suffix | +| `ENTITY_WITH_NO_LABEL` | an entity has no label, such as `acq-` | +| `INVALID_ENTITY_LABEL` | a label breaks the schema's format pattern | +| `EXTENSION_MISMATCH` | the extension is not allowed for that suffix | +| `DATATYPE_MISMATCH` | the datatype folder does not match the suffix | +| `INVALID_LOCATION` | a valid name in the wrong directory, or a data file outside any datatype directory | +| `FILENAME_MISMATCH` | entities duplicated or out of canonical order | +| `ALL_FILENAME_RULES_HAVE_ISSUES` | several rules matched and each had a problem | + +## One deliberate difference from the reference validator + +The module is stricter in exactly one place: **a data file that is not inside a +recognised datatype directory**. + +``` +sub-01/foo/sub-01_T1w.nii.gz a folder that is not a datatype +sub-01/sub-01_T1w.nii.gz no datatype folder at all +``` + +The reference TypeScript validator does not report these. Its suffix matching +ignores the datatype, so the T1w rule still matches, and its `DATATYPE_MISMATCH` +check is then skipped because the parent directory is not a known datatype +(`findDatatype` returns an empty string, and the check is gated on that value being +truthy). + +The legacy `BIDSValidator.is_bids` does report them, because its regexes cover the +whole path including the datatype directory. Since this module replaces that check, +dropping the case would lose coverage users already have, so it is reported as +`INVALID_LOCATION`, whose catalog reason is exactly "The file has a valid name, but +is located in an invalid directory." + +Metadata files are exempt. The inheritance principle lets a `.json` or `.tsv` sit +higher in the tree than the data it describes, so `sub-01/sub-01_T1w.json` and +`task-rest_bold.json` at the dataset root are correct and are not flagged. The +exempt extensions are in `INHERITABLE_EXTENSIONS`. + +Everything else matches the reference exactly. + +## How to use it + +### On your own dataset + +```python +from bidsschematools.schema import load_schema + +from bids_validator.filename_checks import collect_filename_issues +from bids_validator.types.files import FileTree + +tree = FileTree.read_from_filesystem('/path/to/dataset') +issues = collect_filename_issues(tree, load_schema()) + +print(len(issues), 'finding(s)') +for issue in issues: + print(issue.code, issue.location, issue.message) + +if issues.has_errors: + raise SystemExit(1) +``` + +`DatasetIssues` supports `len()`, iteration, `has_errors`, and +`by_severity(Severity.ERROR)`. Each `Issue` converts to a plain dict with +`attrs.asdict(issue)`, so a JSON report is one line: + +```python +import json + +import attrs + +print(json.dumps([attrs.asdict(issue) for issue in issues], indent=2)) +``` + +The example script accepts a dataset path, so you can run it directly: + +```shell +python docs/example_filename_issues.py /path/to/dataset +``` + +### The generated example + +Run it with no argument to build a small dataset containing one deliberately +broken file per issue code, then validate it: + +```shell +python docs/example_filename_issues.py +``` + +The dataset it generates: + +| File | Raises | +|---|---| +| `sub-01/notes.txt` | `NOT_INCLUDED` | +| `sub-01/anat/sub-01_T1w.txt` | `EXTENSION_MISMATCH` | +| `sub-01/func/sub-01_bold.nii.gz` | `MISSING_REQUIRED_ENTITY` | +| `sub-01/anat/sub-01_acq-_T1w.nii.gz` | `ENTITY_WITH_NO_LABEL` | +| `sub-01/anat/sub-01_acq-a!b_T1w.nii.gz` | `INVALID_ENTITY_LABEL` | +| `sub-01/anat/sub-01_dir-AP_T1w.nii.gz` | `ENTITY_NOT_IN_RULE` | +| `sub-01/anat/acq-x_sub-01_T1w.nii.gz` | `FILENAME_MISMATCH` | +| `sub-01/func/sub-01_T1w.nii.gz` | `DATATYPE_MISMATCH` | +| `sub-02/anat/sub-01_T1w.nii.gz` | `INVALID_LOCATION` | +| `sub-01/sub-01_channels.tsv` | `ALL_FILENAME_RULES_HAVE_ISSUES` | + +It also contains correctly named files (`sub-01/anat/sub-01_T1w.nii.gz`, +`sub-01/func/sub-01_task-rest_bold.nii.gz`, `README`) which produce no findings. + +Part of the real output: + +``` +10 finding(s): 10 error(s), 0 warning(s) + +[error] EXTENSION_MISMATCH + file : sub-01/anat/sub-01_T1w.txt + detail : extension '.txt' is not allowed here; allowed: .nii.gz, .nii, .json + rule : rules.files.raw.anat.nonparametric +[error] MISSING_REQUIRED_ENTITY + file : sub-01/func/sub-01_bold.nii.gz + detail : missing required entities: task + rule : rules.files.raw.func.func +[error] FILENAME_MISMATCH + file : sub-01/anat/acq-x_sub-01_T1w.nii.gz + detail : expected filename: sub-01_acq-x_T1w.nii.gz +``` + +## Extending this to file contents + +The issues layer is deliberately generic. An `Issue` does not know what kind of +check produced it, so content validation plugs in without changing anything here. +A content check reads a file and emits the same `Issue` type into the same +`DatasetIssues`: + +```python +def sidecar_checks(context) -> list[Issue]: + """Check the fields inside a JSON sidecar.""" + issues = [] + if 'RepetitionTime' not in context.sidecar: + issues.append( + Issue( + code='SIDECAR_KEY_REQUIRED', + location=context.file.relative_path, + message='RepetitionTime is required for this file', + ) + ) + return issues +``` + +Different codes, one shape: + +```mermaid +flowchart LR + FN["filename checks, today"] --> D["DatasetIssues"] + SC["sidecar field checks"] --> D + NH["NIfTI header checks"] --> D + TC["TSV column checks"] --> D + SR["schema rules.checks"] --> D + D --> T["text report"] + D --> J["JSON"] + D --> S["SARIF"] +``` + +Three things make the extension straightforward: + +1. **The container does not change.** `Issue` and `DatasetIssues` already carry + everything a content finding needs, including `rule` for schema-driven checks. +2. **The context is the natural input.** `Context` already exposes the lazily + loaded contents (`json`, `columns`, `nifti_header`, `sidecar`), so a content + check reads from the same object the filename checks use. +3. **Codes for content checks mostly come from the schema.** Schema-defined + `rules.checks` carry their own `issue` block with a code, level, and message, so + a rule engine can build an `Issue` straight from the schema rather than + hardcoding a catalog. + +The pattern to follow is the one in `filename_checks.py`: a function that takes a +`Context`, returns a `list[Issue]`, and never raises for a file it cannot judge. +Skipping an undeterminable check keeps the validator free of false alarms. + +--- + +# Technical reference + +Everything a developer needs to work on these two modules: the types, the call +graph, and the reason behind each decision. + +## Files + +| File | Role | +|---|---| +| `src/bids_validator/issues.py` | The finding model. Pure data, no imports from the rest of the package. | +| `src/bids_validator/filename_checks.py` | The check logic. Reads the schema, walks the tree, emits findings. | +| `tests/test_issues.py` | Model unit tests. | +| `tests/test_filename_checks.py` | One test per issue code, plus ignore and catalog tests. | +| `docs/example_filename_issues.py` | Runnable example, both modes. | + +## `bids_validator.issues` + +### `Severity(str, Enum)` + +```python +class Severity(str, Enum): + WARNING = 'warning' + ERROR = 'error' +``` + +Subclassing `str` as well as `Enum` means a member *is* a string, so +`attrs.asdict` and `json.dumps` produce `"error"` with no custom encoder and no +`.value` calls at the serialisation boundary. Only two members exist because the +reference validator defines no third level for filename findings; adding one later +does not change any call site. + +### `Issue` + +```python +@attrs.define(kw_only=True) +class Issue: + code: str + severity: Severity = Severity.ERROR + location: str | None = None + message: str | None = None + sub_code: str | None = None + rule: str | None = None +``` + +| Field | Purpose | +|---|---| +| `code` | Stable identifier, the thing tools key on. The only required field. | +| `severity` | Defaults to `ERROR`, which is correct for every filename finding. | +| `location` | Dataset-relative path, taken from `FileTree.relative_path`. | +| `message` | Human-readable detail, the reference validator's `issueMessage`. | +| `sub_code` | Finer category within a code, for example which entity was at fault. | +| `rule` | Dotted schema path of the rule that fired, for example `rules.files.raw.anat.nonparametric`. | + +`attrs` rather than `pydantic` or `msgspec` because `attrs` is what the rest of the +package already uses (`context.py`, `types/files.py`, `bidsignore.py`); a second +data library would be a new dependency and an inconsistency. `kw_only=True` forces +call sites to name their fields, so an `Issue(...)` literal reads as documentation +and adding a field can never silently shift a positional argument. + +`rule` is populated only by the checks that are scoped to one matched rule +(`MISSING_REQUIRED_ENTITY`, `ENTITY_NOT_IN_RULE`, `DATATYPE_MISMATCH`, +`EXTENSION_MISMATCH`). Whole-file findings such as `NOT_INCLUDED` leave it `None`, +because no single rule produced them. + +### `DatasetIssues` + +```python +@attrs.define +class DatasetIssues: + issues: list[Issue] = attrs.field(factory=list) +``` + +| Member | Purpose | +|---|---| +| `add(issue)` | Append one finding. | +| `extend(issues)` | Append many, used by the per-file loop. | +| `by_severity(severity)` | Filter, preserving insertion order. | +| `has_errors` | Property. Drives the process exit code. | +| `__len__`, `__iter__` | Makes it behave like a collection at call sites. | + +A wrapper rather than a bare `list[Issue]` so that later additions (grouping, +severity rollup, a summary view) do not force every caller to change. `factory=list` +gives each instance its own list; a bare `= []` default would be shared across all +instances. + +## `bids_validator.filename_checks` + +### Public API + +| Name | Signature | Notes | +|---|---|---| +| `collect_filename_issues` | `(tree: FileTree, schema: Namespace) -> DatasetIssues` | The front door. Builds the `Dataset`, walks, collects. | +| `iter_contexts` | `(dataset: Dataset, ignore: HasMatch \| None = None) -> Iterator[Context]` | Yields one `Context` per validatable file. A generator, so memory stays flat on large datasets. | +| `build_ignore` | `(tree: FileTree) -> IgnoreMany` | The defaults plus the dataset's `.bidsignore`. | +| `filename_issues` | `(context: Context) -> list[Issue]` | All findings for one file. The unit a future rule engine would call. | +| `DEFAULT_IGNORES` | `tuple[str, ...]` | Mirrors the reference validator's `defaultIgnores`. | +| `FILENAME_ISSUES` | `dict[str, str]` | The ten codes with the reference's reason text. | + +`filename_issues` takes a `Context` and returns a list rather than mutating a +collection. That keeps it pure and independently testable, and it is the same shape +a content check will have, so the two compose without adapters. + +### Internals: rule identification + +| Function | What it does and why | +|---|---| +| `_file_rules(schema)` | Flattens `rules.files` into `[(dotted_path, leaf_rule)]`. The schema nests rules several levels deep; flattening once makes matching a simple loop and gives every finding a printable rule path. | +| `_collect(node, path, out)` | The recursive walk behind it. A node is a leaf when it has `path`, `stem`, or `suffixes`. | +| `_find_rule_matches(schema, context)` | Every rule the file matches. Skips `rules.files.deriv*` unless `DatasetType` is `derivative`, otherwise derivative-only patterns would validate raw files. | +| `_rule_matches(node, context)` | Three ways a rule can match: an exact `path`, a `stem` glob, or membership in `suffixes`. | +| `_match_stem(node, context)` | `fnmatch.fnmatchcase` for the glob, plus a datatype constraint when the rule has one. Case-sensitive because BIDS names are. | +| `_narrow(schema, context, matched)` | Several rules can match one name. Prefer those sharing the file's datatype, then those whose entities and extension fit. Without this a file would be judged against an unrelated rule and produce misleading codes. | +| `_entities_extensions_fit(...)` | The second narrowing test: the extension is allowed and the file's entities are a subset of the rule's. | + +### Internals: per-file checks + +Each returns `list[Issue]`, so `filename_issues` is a concatenation. + +| Function | Emits | +|---|---| +| `_missing_label(context, matched)` | `ENTITY_WITH_NO_LABEL` for entities whose label is `''`. | +| `_entity_label_check(schema, context)` | `INVALID_ENTITY_LABEL`, using the entity's `format` and that format's `pattern` from `objects.formats`, matched with `re.fullmatch`. | +| `_check_rules(schema, context, matched)` | Dispatches to `_rule_issues`. With several candidates still matching, if any is clean the file is accepted; only if all fail does it emit `ALL_FILENAME_RULES_HAVE_ISSUES`. | +| `_rule_issues(schema, context, matched)` | Runs the four rule-scoped checks below for one candidate rule. | +| `_entity_rule_issues(...)` | `MISSING_REQUIRED_ENTITY` and `ENTITY_NOT_IN_RULE`. | +| `_datatype_mismatch(...)` | `DATATYPE_MISMATCH`. | +| `_extension_mismatch(...)` | `EXTENSION_MISMATCH`. | +| `_invalid_location(context)` | `INVALID_LOCATION`, for both the `sub`/`ses` and the `tpl`/`cohort` hierarchies. | +| `_missing_datatype_directory(context, matched)` | `INVALID_LOCATION` for a data file outside any datatype directory. Fires only when the file has no recognised datatype, its extension is not in `INHERITABLE_EXTENSIONS`, and *every* matched rule declares `datatypes`. The last condition is what keeps `participants.tsv` and friends quiet. This is the one check that is stricter than the reference. | +| `_allowed_datatypes(matched)` | Builds the "expected one of: anat" part of that message. | +| `_reconstruction_failure(schema, context)` | `FILENAME_MISMATCH`. Rebuilds the canonical name from the entities in schema order and compares. This is what catches duplication and reordering. | + +### Internals: schema helpers + +| Function | Why it exists | +|---|---| +| `_entities(context)` | Drops `None`-valued entries. `FileParts` records a filename token with no hyphen (the `dataset` in `dataset_description.json`) as an entity with value `None`; treating those as entities would produce a false `FILENAME_MISMATCH` on every such file. An empty string is kept, because that is its own finding. | +| `_entity_by_short(schema)` | Maps short entity names (`acq`) to their schema definitions. Filenames use short names, the schema keys on long ones. | +| `_ordered_short(schema)` | Entity short names in the schema's canonical filename order, from `rules.entities`. Drives the `FILENAME_MISMATCH` reconstruction. | +| `_short(schema, long_name)` | Long name to short name for one entity. | +| `_directory_recordings(schema)` | Extensions whose schema value ends in `/` (CTF `.ds`, MEF `.mefd`, OME-Zarr). | +| `_dataset_type(context)` | `DatasetType` from `dataset_description.json`, defaulting to `raw`. Catches `KeyError`, `OSError`, and `ValueError` so a missing or malformed description degrades instead of aborting the run. | +| `_is_mapping(node)` | `Namespace` is dict-like but not always a `Mapping` instance, so this accepts either. | + +### Types borrowed from the package + +| Type | From | Used for | +|---|---|---| +| `FileTree` | `types.files` | The indexed dataset. `relative_path`, `name`, `is_dir`, `children`. | +| `Context` | `context` | Per-file facts: `path`, `entities`, `datatype`, `suffix`, `extension`, `file`, `dataset`, `schema`. | +| `Dataset` | `context` | Holds the tree, the schema, and the cached `dataset_description`. | +| `Ignore`, `IgnoreMany`, `HasMatch` | `bidsignore` | Gitignore-style matching. `HasMatch` is a `Protocol`, so `iter_contexts` accepts any matcher. | +| `Namespace` | `bidsschematools` | The schema, with attribute and item access. | + +The module reuses `Context` rather than defining its own file model. It is already +the package's per-file abstraction, it already parses names through `FileParts`, and +a parallel model would drift. + +## Design decisions + +1. **Two modules, container and logic.** `issues.py` imports nothing from the + package, so it can never participate in an import cycle and any future check + module can depend on it. +2. **Schema-driven, nothing hardcoded about BIDS.** Entity names, orders, formats, + suffixes, extensions, and datatypes all come from the schema, so a newer schema + changes behaviour with no code change. Only the ten issue *codes* are constants, + because the schema does not define them. +3. **Memoisation keyed on `id(schema)`.** Four caches (`_RULES_MEMO`, + `_ENTITY_BY_SHORT_MEMO`, `_ORDERED_SHORT_MEMO`, `_DIR_RECORDING_MEMO`) avoid + re-flattening the rule tree for every file. `bidsschematools` caches the schema + object for the process, so its identity is stable. +4. **Skip rather than guess.** Anything the module cannot determine produces no + finding. That is what keeps it free of false alarms, verified by real datasets + producing zero findings. +5. **Default ignores mirrored from the reference.** Without them dotfiles such as + `.DS_Store` are reported, which the reference never does. +6. **Directory recordings are units.** The walk does not descend into `.ds` and + friends, and does not name-check them, so their internal files never appear as + findings. +7. **Root files are exempt from required-entity checks.** A file at the dataset root + is a shared sidecar inherited downward, so requiring `sub` there would be wrong. + The test is `'/' in context.file.relative_path`. +8. **A generator for the walk.** `iter_contexts` yields, so a hundred-thousand-file + dataset holds one context at a time. + +## Testing + +`tests/test_filename_checks.py` uses a table-driven +`@pytest.mark.parametrize` with one row per code: build a dataset containing exactly +one broken file, assert that code appears for that path. The rest cover the default +ignores, `.bidsignore`, the `rule` field, and catalog completeness. The `schema` +fixture is the session-scoped one in `tests/conftest.py`, so the schema loads once. + +Checks that must pass: `pytest`, `ruff check`, `ruff format --check`, and +`mypy --strict`. diff --git a/src/bids_validator/filename_checks.py b/src/bids_validator/filename_checks.py new file mode 100644 index 0000000..46ce62c --- /dev/null +++ b/src/bids_validator/filename_checks.py @@ -0,0 +1,606 @@ +"""Schema-driven filename and path validation, producing structured findings. + +Scope: NAMES AND PATHS ONLY. Nothing here opens a file or reads its contents, so +there are no empty-file, header, gzip, JSON, or tabular checks. Those belong to the +later content-validation layer. What this module answers is: given the schema's +``rules.files``, is this path a legal BIDS name, in a legal place? + +How it works: the schema describes every legal filename (which suffix goes in which +datatype folder, which entities are required or allowed, which extensions). For each +file this module identifies the matching rule(s) and then checks the file against +them, emitting one specific code per kind of failure rather than a single blanket +"bad name". + +The codes are the reference (Deno) ``bids-validator`` catalog, defined in its +``src/issues/list.ts``. They are deliberately NOT in the BIDS schema: the schema +supplies the rules a name is matched against, but it does not name these structural +failures. :data:`FILENAME_ISSUES` mirrors that catalog so the provenance is explicit +and the output stays interchangeable with the reference. +""" + +from __future__ import annotations + +import fnmatch +import re +from collections.abc import Iterator, Mapping +from typing import TYPE_CHECKING, Any + +from bidsschematools.types.namespace import Namespace + +from .bidsignore import Ignore, IgnoreMany +from .context import Context, Dataset +from .issues import DatasetIssues, Issue, Severity + +if TYPE_CHECKING: + from .bidsignore import HasMatch + from .types.files import FileTree + +__all__ = [ + 'DEFAULT_IGNORES', + 'FILENAME_ISSUES', + 'collect_filename_issues', + 'filename_issues', + 'iter_contexts', +] + +# Paths the reference validator never name-checks, from its ``src/files/ignore.ts``. +# ``.*`` covers dotfiles such as ``.DS_Store`` and ``.bidsignore`` itself; the named +# directories hold files BIDS does not constrain. +DEFAULT_IGNORES = ('.git**', '.*', 'sourcedata/', 'code/', 'stimuli/', 'log/') + +# Extensions the BIDS inheritance principle allows to sit higher in the tree than the +# data they describe, so they are exempt from the datatype-directory requirement. +INHERITABLE_EXTENSIONS = frozenset({'.json', '.tsv'}) + +# The filename/path codes this module can emit, with the reference validator's +# reason text. Every one is an error; the reference defines no filename warnings. +FILENAME_ISSUES: dict[str, str] = { + 'NOT_INCLUDED': 'Files with such naming scheme are not part of BIDS specification.', + 'ENTITY_WITH_NO_LABEL': 'Found an entity with no label.', + 'INVALID_ENTITY_LABEL': ("entity label doesn't match format found for files with this suffix"), + 'MISSING_REQUIRED_ENTITY': 'Missing required entity for files with this suffix.', + 'ENTITY_NOT_IN_RULE': ('Entity not listed as required or optional for files with this suffix'), + 'DATATYPE_MISMATCH': ( + 'The datatype directory does not match datatype of found suffix and extension' + ), + 'EXTENSION_MISMATCH': ( + 'Extension used by file does not match allowed extensions for its suffix' + ), + 'INVALID_LOCATION': 'The file has a valid name, but is located in an invalid directory.', + 'FILENAME_MISMATCH': ( + 'The filename is not formatted correctly. This could result from entity ' + 'duplication or reordering.' + ), + 'ALL_FILENAME_RULES_HAVE_ISSUES': ( + 'Multiple filename rules were found as potential matches. All of them had at ' + 'least one issue during filename validation.' + ), +} + +# Per-schema caches. Schema objects are cached for the process, so id() is stable. +_RULES_MEMO: dict[int, list[tuple[str, Mapping[str, Any]]]] = {} +_ENTITY_BY_SHORT_MEMO: dict[int, dict[str, Mapping[str, Any]]] = {} +_ORDERED_SHORT_MEMO: dict[int, list[str]] = {} +_DIR_RECORDING_MEMO: dict[int, set[str]] = {} + + +# --- public API ----------------------------------------------------------- + + +def collect_filename_issues(tree: FileTree, schema: Namespace) -> DatasetIssues: + """Validate every filename in a dataset tree. + + Parameters + ---------- + tree : FileTree + The dataset root, from ``FileTree.read_from_filesystem(root)``. + schema : Namespace + The BIDS schema to validate against. + + Returns + ------- + DatasetIssues + Every filename/path finding, in tree order. + + """ + dataset = Dataset(tree, schema) + issues = DatasetIssues() + for context in iter_contexts(dataset): + issues.extend(filename_issues(context)) + return issues + + +def iter_contexts(dataset: Dataset, ignore: HasMatch | None = None) -> Iterator[Context]: + """Yield a :class:`~bids_validator.context.Context` for every validatable file. + + Skips anything the dataset's ``.bidsignore`` or :data:`DEFAULT_IGNORES` match. + Directory recordings (CTF ``.ds``, MEF ``.mefd``, OME-Zarr ...) are single units: + the walk does not descend into them, so their internal files are not name-checked + individually. + """ + if ignore is None: + ignore = build_ignore(dataset.tree) + recordings = _directory_recordings(dataset.schema) + yield from _walk(dataset.tree, dataset, recordings, ignore) + + +def build_ignore(tree: FileTree) -> IgnoreMany: + """Build the ignore matcher: the reference defaults plus the dataset's .bidsignore.""" + ignores = [Ignore(list(DEFAULT_IGNORES))] + bidsignore = tree.children.get('.bidsignore') + if bidsignore is not None: + ignores.append(Ignore.from_file(bidsignore)) + return IgnoreMany(ignores) + + +def filename_issues(context: Context) -> list[Issue]: + """Return every filename/path finding for one file. + + Identifies the ``rules.files`` rule(s) the file matches, then checks it against + them. An unmatched file is ``NOT_INCLUDED``; a matched one is checked for entity, + datatype, extension, location, and ordering problems. + """ + schema = context.schema + relpath = context.file.relative_path + + # A directory recording is a unit, not a name to parse. + if any(context.file.name.endswith(ext) for ext in _directory_recordings(schema)): + return [] + + matched = _find_rule_matches(schema, context) + if not matched: + return [ + Issue( + code='NOT_INCLUDED', + severity=Severity.ERROR, + location=relpath, + message=f'{context.file.name} does not match any BIDS naming rule', + ) + ] + + matched = _narrow(schema, context, matched) + issues: list[Issue] = [] + issues += _missing_label(context, matched) + issues += _entity_label_check(schema, context) + issues += _check_rules(schema, context, matched) + issues += _missing_datatype_directory(context, matched) + issues += _reconstruction_failure(schema, context) + return issues + + +# --- walking -------------------------------------------------------------- + + +def _walk( + tree: FileTree, dataset: Dataset, recordings: set[str], ignore: HasMatch +) -> Iterator[Context]: + for child in tree.children.values(): + if ignore.match(child.relative_path): + continue + if child.is_dir: + if any(child.name.endswith(ext) for ext in recordings): + continue # a directory recording: do not descend + yield from _walk(child, dataset, recordings, ignore) + else: + yield Context(child, dataset, None) + + +# --- rule identification -------------------------------------------------- + + +def _file_rules(schema: Namespace) -> list[tuple[str, Mapping[str, Any]]]: + """Flatten ``rules.files`` to ``[(rule_path, leaf_rule)]``, once per schema.""" + cached = _RULES_MEMO.get(id(schema)) + if cached is not None: + return cached + out: list[tuple[str, Mapping[str, Any]]] = [] + files = schema['rules'].get('files', {}) + for group in files: + _collect(files[group], f'rules.files.{group}', out) + _RULES_MEMO[id(schema)] = out + return out + + +def _collect(node: Any, path: str, out: list[tuple[str, Mapping[str, Any]]]) -> None: + if not _is_mapping(node): + return + if 'path' in node or 'stem' in node or 'suffixes' in node: + out.append((path, node)) + return + for key in node: + _collect(node[key], f'{path}.{key}', out) + + +def _find_rule_matches(schema: Namespace, context: Context) -> list[tuple[str, Mapping[str, Any]]]: + dataset_type = _dataset_type(context) + out: list[tuple[str, Mapping[str, Any]]] = [] + for path, node in _file_rules(schema): + # Derivative rules only apply to a derivative dataset. + if path.startswith('rules.files.deriv') and dataset_type != 'derivative': + continue + if _rule_matches(node, context): + out.append((path, node)) + return out + + +def _rule_matches(node: Mapping[str, Any], context: Context) -> bool: + if 'path' in node and '/' + str(node['path']) == context.path: + return True + if 'stem' in node and _match_stem(node, context): + return True + return 'suffixes' in node and context.suffix in list(node['suffixes']) + + +def _match_stem(node: Mapping[str, Any], context: Context) -> bool: + stem = context.file.name.split('.')[0] + if not fnmatch.fnmatchcase(stem, str(node['stem'])): + return False + if 'datatypes' in node: + return context.datatype in list(node['datatypes']) + return True + + +def _narrow( + schema: Namespace, context: Context, matched: list[tuple[str, Mapping[str, Any]]] +) -> list[tuple[str, Mapping[str, Any]]]: + """Prefer the rule sharing the file's datatype, then the one whose entities fit.""" + if len(matched) <= 1: + return matched + by_datatype = [ + (p, n) for p, n in matched if 'datatypes' in n and context.datatype in list(n['datatypes']) + ] + if by_datatype: + matched = by_datatype + if len(matched) <= 1: + return matched + by_ent_ext = [(p, n) for p, n in matched if _entities_extensions_fit(schema, context, n)] + return by_ent_ext or matched + + +def _entities_extensions_fit(schema: Namespace, context: Context, rule: Mapping[str, Any]) -> bool: + ext_ok = 'extensions' not in rule or context.extension in list(rule['extensions']) + if 'entities' not in rule: + return ext_ok + rule_entities = {_short(schema, key) for key in rule['entities']} + return ext_ok and set(_entities(context)).issubset(rule_entities) + + +# --- per-file checks ------------------------------------------------------ + + +def _missing_label(context: Context, matched: list[tuple[str, Mapping[str, Any]]]) -> list[Issue]: + """Report an entity that is present with no label, e.g. ``acq-``.""" + if not any('suffixes' in node for _path, node in matched): + return [] + empty = [key for key, value in _entities(context).items() if value == ''] + if not empty: + return [] + return [ + Issue( + code='ENTITY_WITH_NO_LABEL', + sub_code=', '.join(empty), + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'entities with no label: {", ".join(empty)}', + ) + ] + + +def _entity_label_check(schema: Namespace, context: Context) -> list[Issue]: + """Report an entity label that breaks the schema format pattern.""" + formats = schema['objects'].get('formats', {}) + by_short = _entity_by_short(schema) + issues: list[Issue] = [] + for short, label in _entities(context).items(): + if label == '': + continue # reported as ENTITY_WITH_NO_LABEL instead + definition = by_short.get(short) + fmt = definition.get('format') if isinstance(definition, Mapping) else None + if not fmt or str(fmt) not in formats: + continue + pattern = str(formats[str(fmt)].get('pattern', '')) + if pattern and not re.fullmatch(pattern, label): + issues.append( + Issue( + code='INVALID_ENTITY_LABEL', + sub_code=short, + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'label {label!r} for entity {short!r} does not match /{pattern}/', + ) + ) + return issues + + +def _check_rules( + schema: Namespace, context: Context, matched: list[tuple[str, Mapping[str, Any]]] +) -> list[Issue]: + if len(matched) == 1: + return _rule_issues(schema, context, matched[0]) + # Several rules still match: if any matches cleanly, accept it; otherwise report + # that every candidate had a problem. + per_rule = [_rule_issues(schema, context, entry) for entry in matched] + if any(not issues for issues in per_rule): + return [] + return [ + Issue( + code='ALL_FILENAME_RULES_HAVE_ISSUES', + severity=Severity.ERROR, + location=context.file.relative_path, + message='the file resembles several BIDS rules but fully satisfies none of them', + ) + ] + + +def _rule_issues( + schema: Namespace, context: Context, matched: tuple[str, Mapping[str, Any]] +) -> list[Issue]: + path, rule = matched + issues: list[Issue] = [] + issues += _entity_rule_issues(schema, context, path, rule) + issues += _datatype_mismatch(context, path, rule) + issues += _extension_mismatch(context, path, rule) + issues += _invalid_location(context) + return issues + + +def _entity_rule_issues( + schema: Namespace, context: Context, path: str, rule: Mapping[str, Any] +) -> list[Issue]: + """Too few (required missing) or too many (not allowed) entities.""" + if 'entities' not in rule: + return [] + file_entities = list(_entities(context)) + rule_entities = [_short(schema, key) for key in rule['entities']] + issues: list[Issue] = [] + + # Required-entity checks do not apply to a file at the dataset root: it is a + # shared sidecar inherited downward. This mirrors the reference. + if '/' in context.file.relative_path: + required = [ + _short(schema, key) + for key, level in rule['entities'].items() + if str(level) == 'required' + ] + missing = [entity for entity in required if entity not in file_entities] + if missing: + issues.append( + Issue( + code='MISSING_REQUIRED_ENTITY', + sub_code=', '.join(missing), + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'missing required entities: {", ".join(missing)}', + rule=path, + ) + ) + + extra = [entity for entity in file_entities if entity not in rule_entities] + if extra: + issues.append( + Issue( + code='ENTITY_NOT_IN_RULE', + sub_code=', '.join(extra), + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'entities not allowed for this file type: {", ".join(extra)}', + rule=path, + ) + ) + return issues + + +def _datatype_mismatch(context: Context, path: str, rule: Mapping[str, Any]) -> list[Issue]: + """Report a file sitting in a datatype folder its suffix does not belong to.""" + datatype = context.datatype + if datatype and 'datatypes' in rule and datatype not in list(rule['datatypes']): + allowed = ', '.join(str(d) for d in rule['datatypes']) + return [ + Issue( + code='DATATYPE_MISMATCH', + severity=Severity.ERROR, + location=context.file.relative_path, + message=f"the file is in '{datatype}' but its suffix belongs in: {allowed}", + rule=path, + ) + ] + return [] + + +def _extension_mismatch(context: Context, path: str, rule: Mapping[str, Any]) -> list[Issue]: + """Report an extension that is not allowed for this suffix.""" + if 'extensions' in rule and context.extension not in list(rule['extensions']): + allowed = ', '.join(str(e) for e in rule['extensions']) + return [ + Issue( + code='EXTENSION_MISMATCH', + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'extension {context.extension!r} is not allowed here; allowed: {allowed}', + rule=path, + ) + ] + return [] + + +def _invalid_location(context: Context) -> list[Issue]: + """Report a valid name that is in the wrong directory.""" + entities = _entities(context) + path = context.path + issues: list[Issue] = [] + if 'tpl' not in entities: + issues += _validate_location(entities, path, context, 'sub', 'ses') + if 'sub' not in entities: + issues += _validate_location(entities, path, context, 'tpl', 'cohort') + return issues + + +def _validate_location( + entities: Mapping[str, str], path: str, context: Context, top: str, sub: str +) -> list[Issue]: + issues: list[Issue] = [] + top_val = entities.get(top) + sub_val = entities.get(sub) + if top_val: + expected = f'/{top}-{top_val}/' + if sub_val: + expected += f'{sub}-{sub_val}/' + if not path.startswith(expected): + issues.append(_location_issue(context, f'expected to be under {expected}')) + if not top_val and re.match(rf'^/{top}-', path): + issues.append(_location_issue(context, f"in a '{top}-' folder but no '{top}' in the name")) + if not sub_val and re.search(rf'/{sub}-', path): + issues.append(_location_issue(context, f"in a '{sub}-' folder but no '{sub}' in the name")) + return issues + + +def _location_issue(context: Context, detail: str) -> Issue: + return Issue( + code='INVALID_LOCATION', + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'the file has a valid name but is in the wrong place ({detail})', + ) + + +def _missing_datatype_directory( + context: Context, matched: list[tuple[str, Mapping[str, Any]]] +) -> list[Issue]: + """Report a data file that is not inside a recognised datatype directory. + + This is deliberately STRICTER than the reference TypeScript validator, which + misses the case: its suffix matching ignores the datatype, and its + ``DATATYPE_MISMATCH`` check is skipped when the parent directory is not a known + datatype. The legacy :meth:`BIDSValidator.is_bids` regex does catch it, because + its patterns cover the whole path, so dropping the check would lose coverage + this module replaces. + + Metadata files are exempt: the inheritance principle lets a ``.json`` or + ``.tsv`` sit higher in the tree than the data it describes. + """ + if context.datatype is not None: + return [] # the file is in a recognised datatype directory + if context.extension in INHERITABLE_EXTENSIONS: + return [] # metadata may be inherited from a higher level + if not matched or not all('datatypes' in node for _path, node in matched): + return [] # this file type is not required to live in a datatype directory + return [ + Issue( + code='INVALID_LOCATION', + severity=Severity.ERROR, + location=context.file.relative_path, + message=( + 'the file has a valid name but is not in a datatype directory, ' + 'expected one of: ' + _allowed_datatypes(matched) + ), + ) + ] + + +def _allowed_datatypes(matched: list[tuple[str, Mapping[str, Any]]]) -> str: + """List the datatype directories the matched rules allow.""" + allowed: list[str] = [] + for _path, node in matched: + for datatype in node['datatypes']: + if str(datatype) not in allowed: + allowed.append(str(datatype)) + return ', '.join(allowed) + + +def _reconstruction_failure(schema: Namespace, context: Context) -> list[Issue]: + """Entities duplicated or out of the schema's canonical order.""" + entities = _entities(context) + if not entities: + return [] + ordered = [short for short in _ordered_short(schema) if short in entities] + parts = [f'{short}-{entities[short]}' for short in ordered] + expected = '_'.join([*parts, (context.suffix or '') + (context.extension or '')]) + if context.file.name != expected: + return [ + Issue( + code='FILENAME_MISMATCH', + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'expected filename: {expected}', + ) + ] + return [] + + +# --- helpers -------------------------------------------------------------- + + +def _entities(context: Context) -> dict[str, str]: + """Real key-label entities from the filename. + + ``FileParts`` records a filename token with no hyphen (the ``dataset`` in + ``dataset_description.json``) as an entity with a ``None`` value. Those are not + BIDS entities, so drop them. An empty label (``acq-``) is kept: it is its own + finding. + """ + return {key: value for key, value in context.entities.items() if value is not None} + + +def _entity_by_short(schema: Namespace) -> dict[str, Mapping[str, Any]]: + cached = _ENTITY_BY_SHORT_MEMO.get(id(schema)) + if cached is not None: + return cached + out: dict[str, Mapping[str, Any]] = {} + for definition in schema['objects']['entities'].values(): + name = definition.get('name') + if name: + out[str(name)] = definition + _ENTITY_BY_SHORT_MEMO[id(schema)] = out + return out + + +def _ordered_short(schema: Namespace) -> list[str]: + """Entity short names in the schema's canonical filename order.""" + cached = _ORDERED_SHORT_MEMO.get(id(schema)) + if cached is not None: + return cached + entities = schema['objects']['entities'] + out: list[str] = [] + for long_name in schema['rules'].get('entities', []): + if long_name in entities: + name = entities[long_name].get('name') + if name: + out.append(str(name)) + _ORDERED_SHORT_MEMO[id(schema)] = out + return out + + +def _short(schema: Namespace, long_name: str) -> str: + entities = schema['objects']['entities'] + if long_name in entities: + return str(entities[long_name].get('name', long_name)) + return long_name + + +def _directory_recordings(schema: Namespace) -> set[str]: + """Extensions of directory-based recordings, e.g. ``.ds``, ``.mefd``. + + The schema marks them with an extension value ending in ``/``. + """ + cached = _DIR_RECORDING_MEMO.get(id(schema)) + if cached is not None: + return cached + out: set[str] = set() + for definition in schema['objects']['extensions'].values(): + value = str(definition.get('value', '')) + if value.endswith('/') and value.rstrip('/'): + out.add(value.rstrip('/')) + _DIR_RECORDING_MEMO[id(schema)] = out + return out + + +def _dataset_type(context: Context) -> str: + try: + description = context.dataset.dataset_description + except (KeyError, OSError, ValueError): + return 'raw' + return str(description.get('DatasetType', 'raw')) + + +def _is_mapping(node: Any) -> bool: + return isinstance(node, Mapping) or hasattr(node, 'keys') diff --git a/src/bids_validator/issues.py b/src/bids_validator/issues.py new file mode 100644 index 0000000..4ae8cfe --- /dev/null +++ b/src/bids_validator/issues.py @@ -0,0 +1,107 @@ +"""Typed validation findings for the BIDS validator. + +Every problem the validator reports is an :class:`Issue`: a small, typed record +with a stable ``code``, a :class:`Severity`, the ``location`` of the offending +file, and a human-readable ``message``. Findings are gathered in a +:class:`DatasetIssues` container. + +The field set is intentionally minimal and aligned to the reference (Deno) +``bids-validator`` issue shape, so structured output stays interchangeable. These +are pure-data ``attrs`` models with no I/O, ready to serialise to JSON or drive a +report. Richer fields (rule provenance, machine-actionable fixes) can be added +later without changing this core shape. +""" + +from __future__ import annotations + +from collections.abc import Iterable, Iterator +from enum import Enum + +import attrs + + +class Severity(str, Enum): + """How serious a finding is. + + Ordered from low to high attention: ``WARNING`` then ``ERROR``. Subclassing + ``str`` keeps the values JSON-friendly, so a member serialises directly to + ``'warning'`` or ``'error'``. + """ + + WARNING = 'warning' + ERROR = 'error' + + +@attrs.define(kw_only=True) +class Issue: + """A single validation finding. + + Attributes + ---------- + code + Stable issue identifier, aligned to the reference validator catalog (for + example ``'FILENAME_MISMATCH'``). + severity + How serious the finding is. Defaults to :attr:`Severity.ERROR`. + location + Dataset-relative path of the offending file, when applicable. + message + Human-readable description of the finding. + sub_code + Optional finer category within ``code`` (for example an entity name). + rule + Dotted path of the schema rule that produced the finding, for example + ``rules.files.raw.anat.nonparametric``. + + """ + + code: str + severity: Severity = Severity.ERROR + location: str | None = None + message: str | None = None + sub_code: str | None = None + rule: str | None = None + + +@attrs.define +class DatasetIssues: + """An ordered, typed collection of findings. + + A thin wrapper over a list, so a report has a stable container that is easy to + extend (filtering, severity rollup) without changing the call sites that build + it. + + Attributes + ---------- + issues + The findings, in insertion order. + + """ + + issues: list[Issue] = attrs.field(factory=list) + + def add(self, issue: Issue) -> None: + """Append a single finding.""" + self.issues.append(issue) + + def extend(self, issues: Iterable[Issue]) -> None: + """Append several findings.""" + self.issues.extend(issues) + + def by_severity(self, severity: Severity) -> list[Issue]: + """Return the findings at exactly one severity, in insertion order.""" + return [issue for issue in self.issues if issue.severity is severity] + + @property + def has_errors(self) -> bool: + """Whether any finding is an error (used to drive a non-zero exit code).""" + return any(issue.severity is Severity.ERROR for issue in self.issues) + + def __iter__(self) -> Iterator[Issue]: + return iter(self.issues) + + def __len__(self) -> int: + return len(self.issues) + + +__all__ = ['DatasetIssues', 'Issue', 'Severity'] diff --git a/tests/test_filename_checks.py b/tests/test_filename_checks.py new file mode 100644 index 0000000..6769ed4 --- /dev/null +++ b/tests/test_filename_checks.py @@ -0,0 +1,147 @@ +"""Tests for the schema-driven filename checks (names and paths only).""" + +import json +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator import BIDSValidator +from bids_validator.filename_checks import ( + DEFAULT_IGNORES, + FILENAME_ISSUES, + collect_filename_issues, +) +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +VALID = 'sub-01/anat/sub-01_T1w.nii.gz' + + +def build(root: pathlib.Path, *relpaths: str) -> pathlib.Path: + """Create a minimal dataset containing the given files.""" + (root / 'dataset_description.json').write_text( + json.dumps({'Name': 'test', 'BIDSVersion': '1.11.1'}) + ) + for relpath in relpaths: + path = root / relpath + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b'') + return root + + +def codes(root: pathlib.Path, schema: Namespace) -> dict[str, list[str]]: + """Map each emitted issue code to the locations it was emitted for.""" + tree = FileTree.read_from_filesystem(str(root)) + out: dict[str, list[str]] = {} + for issue in collect_filename_issues(tree, schema): + out.setdefault(issue.code, []).append(issue.location or '') + return out + + +def test_valid_dataset_has_no_findings(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, VALID, 'sub-01/func/sub-01_task-rest_bold.nii.gz', 'README') + assert codes(tmp_path, schema) == {} + + +@pytest.mark.parametrize( + ('relpath', 'expected'), + [ + ('sub-01/notes.txt', 'NOT_INCLUDED'), + ('sub-01/anat/sub-01_T1w.txt', 'EXTENSION_MISMATCH'), + ('sub-01/func/sub-01_bold.nii.gz', 'MISSING_REQUIRED_ENTITY'), + ('sub-01/anat/sub-01_acq-_T1w.nii.gz', 'ENTITY_WITH_NO_LABEL'), + ('sub-01/anat/sub-01_acq-a!b_T1w.nii.gz', 'INVALID_ENTITY_LABEL'), + ('sub-01/anat/sub-01_dir-AP_T1w.nii.gz', 'ENTITY_NOT_IN_RULE'), + ('sub-01/anat/acq-x_sub-01_T1w.nii.gz', 'FILENAME_MISMATCH'), + ('sub-01/func/sub-01_T1w.nii.gz', 'DATATYPE_MISMATCH'), + ('sub-02/anat/sub-01_T1w.nii.gz', 'INVALID_LOCATION'), + ], +) +def test_each_code_fires( + tmp_path: pathlib.Path, schema: Namespace, relpath: str, expected: str +) -> None: + build(tmp_path, relpath) + found = codes(tmp_path, schema) + assert expected in found, f'{relpath} should raise {expected}, got {sorted(found)}' + assert relpath in found[expected] + + +def test_findings_are_errors_and_carry_location(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'sub-01/notes.txt') + tree = FileTree.read_from_filesystem(str(tmp_path)) + issues = collect_filename_issues(tree, schema) + assert len(issues) == 1 + assert issues.has_errors + issue = issues.issues[0] + assert issue.severity is Severity.ERROR + assert issue.location == 'sub-01/notes.txt' + assert issue.message + + +def test_rule_path_recorded_for_rule_scoped_findings( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + build(tmp_path, 'sub-01/func/sub-01_bold.nii.gz') + tree = FileTree.read_from_filesystem(str(tmp_path)) + issue = next(i for i in collect_filename_issues(tree, schema)) + assert issue.code == 'MISSING_REQUIRED_ENTITY' + assert issue.rule is not None + assert issue.rule.startswith('rules.files.') + + +def test_default_ignores_are_not_flagged(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, VALID, '.DS_Store', 'sub-01/.DS_Store', 'code/script.py') + assert codes(tmp_path, schema) == {} + + +def test_bidsignore_is_respected(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, VALID, 'extras/notes.txt') + assert 'NOT_INCLUDED' in codes(tmp_path, schema) + + (tmp_path / '.bidsignore').write_text('extras/\n') + assert codes(tmp_path, schema) == {} + + +def test_catalog_documents_every_emitted_code() -> None: + assert 'NOT_INCLUDED' in FILENAME_ISSUES + assert len(FILENAME_ISSUES) == 10 + assert all(reason for reason in FILENAME_ISSUES.values()) + assert '.*' in DEFAULT_IGNORES + + +@pytest.mark.parametrize( + 'relpath', + [ + 'sub-01/foo/sub-01_T1w.nii.gz', # a folder that is not a datatype + 'sub-01/sub-01_T1w.nii.gz', # no datatype folder at all + ], +) +def test_data_file_outside_datatype_directory( + tmp_path: pathlib.Path, schema: Namespace, relpath: str +) -> None: + """Stricter than the reference validator, and matches legacy is_bids.""" + build(tmp_path, relpath) + found = codes(tmp_path, schema) + assert 'INVALID_LOCATION' in found + assert relpath in found['INVALID_LOCATION'] + # the legacy check agrees these are not valid BIDS paths + assert not BIDSValidator().is_bids(f'/{relpath}') + + +@pytest.mark.parametrize( + 'relpath', + [ + 'task-rest_bold.json', # inherited sidecar at the dataset root + 'sub-01/sub-01_T1w.json', # sidecar one level above the data + 'sub-01/sub-01_scans.tsv', # subject-level tabular metadata + 'sub-01/sub-01_task-rest_events.tsv', # events inherited upward + ], +) +def test_inheritable_metadata_may_sit_above_the_datatype_directory( + tmp_path: pathlib.Path, schema: Namespace, relpath: str +) -> None: + """The inheritance principle allows these; they must not be flagged.""" + build(tmp_path, VALID, relpath) + assert codes(tmp_path, schema) == {} + assert BIDSValidator().is_bids(f'/{relpath}') diff --git a/tests/test_issues.py b/tests/test_issues.py new file mode 100644 index 0000000..27eacd4 --- /dev/null +++ b/tests/test_issues.py @@ -0,0 +1,61 @@ +"""Unit tests for the issues model (pure data, no fixtures needed).""" + +import attrs +import pytest + +from bids_validator.issues import DatasetIssues, Issue, Severity + + +def test_issue_defaults() -> None: + issue = Issue(code='FILENAME_MISMATCH') + assert issue.code == 'FILENAME_MISMATCH' + assert issue.severity is Severity.ERROR + assert issue.location is None + assert issue.message is None + assert issue.sub_code is None + + +@pytest.mark.parametrize('severity', [Severity.WARNING, Severity.ERROR]) +def test_issue_severity_roundtrip(severity: Severity) -> None: + issue = Issue(code='X', severity=severity) + assert issue.severity is severity + assert issue.severity.value in ('warning', 'error') + + +def test_dataset_issues_add_extend_and_order() -> None: + issues = DatasetIssues() + assert len(issues) == 0 + issues.add(Issue(code='A')) + issues.extend([Issue(code='B', severity=Severity.WARNING), Issue(code='C')]) + assert len(issues) == 3 + assert [issue.code for issue in issues] == ['A', 'B', 'C'] + + +def test_by_severity() -> None: + issues = DatasetIssues() + issues.extend( + [ + Issue(code='A', severity=Severity.ERROR), + Issue(code='B', severity=Severity.WARNING), + Issue(code='C', severity=Severity.ERROR), + ] + ) + assert [issue.code for issue in issues.by_severity(Severity.ERROR)] == ['A', 'C'] + assert [issue.code for issue in issues.by_severity(Severity.WARNING)] == ['B'] + + +def test_has_errors() -> None: + issues = DatasetIssues() + assert issues.has_errors is False + issues.add(Issue(code='W', severity=Severity.WARNING)) + assert issues.has_errors is False + issues.add(Issue(code='E', severity=Severity.ERROR)) + assert issues.has_errors is True + + +def test_issue_is_json_ready() -> None: + issue = Issue(code='X', severity=Severity.ERROR, location='/a', message='m') + data = attrs.asdict(issue) + assert data['code'] == 'X' + assert data['severity'] == 'error' + assert data['location'] == '/a'