diff --git a/docs/example_validation.py b/docs/example_validation.py new file mode 100644 index 0000000..53bfe26 --- /dev/null +++ b/docs/example_validation.py @@ -0,0 +1,111 @@ +"""Runnable example: validate a dataset, or see each filename code demonstrated. + +Two modes: + +* No argument: build a small dataset in a temporary directory that contains one + deliberately broken file name per filename code, run only the name checks (of files + and folders) on it, and print the findings. +* With a path: run every check on your own dataset. + +Usage +----- + python docs/example_validation.py # generated filename demo + python docs/example_validation.py /path/to/dataset # every check, 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.checks import collect_issues +from bids_validator.checks.filenames import filename_issues, folder_issues +from bids_validator.issues import DatasetIssues, Severity +from bids_validator.types.files import FileTree + +# Correctly named files. None of these produce a filename 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 filename 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-01/sub-01_T1w.nii.gz': 'MISSING_DATATYPE', + '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 report(root: Path, issues: DatasetIssues) -> None: + """Print every finding of a validation.""" + 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 or "(dataset)"}') + 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 run the filename checks on it.""" + with tempfile.TemporaryDirectory() as tmp: + root = build_dataset(Path(tmp)) + print('Generated example: one broken file name per filename code.\n') + for relpath, code in BROKEN_FILES.items(): + print(f' {code:32} {relpath}') + print() + tree = FileTree.read_from_filesystem(str(root)) + names_only = collect_issues( + tree, + load_schema(), + dataset_checks=(), + folder_checks=(folder_issues,), + file_checks=(filename_issues,), + ) + report(root, names_only) + + +def main(argv: list[str]) -> int: + """Run the demo, or validate the dataset given as the first argument.""" + if argv: + root = Path(argv[0]) + report(root, collect_issues(FileTree.read_from_filesystem(str(root)), load_schema())) + else: + run_demo() + return 0 + + +if __name__ == '__main__': + raise SystemExit(main(sys.argv[1:])) diff --git a/docs/validation.md b/docs/validation.md new file mode 100644 index 0000000..306d689 --- /dev/null +++ b/docs/validation.md @@ -0,0 +1,641 @@ +# How python-validator checks a dataset + +This describes the full validation layer: what each module is for, how one file is +checked from start to finish, which findings it can produce, how it compares with the +reference validator (bids-validator, written in TypeScript and run with Deno), and every +place where the two differ on purpose. + +It covers filename and folder checks, file-integrity checks, and every rule the BIDS +schema writes with selectors: `rules.checks`, the JSON field rules (`rules.sidecars`, +`rules.json`, `rules.dataset_metadata`) and the table rules (`rules.tabular_data`). Rules +that need associated files (the link from a scan to its events file, issue #82), HED, and +OME/TIFF metadata are held back; section 9 lists them. + +Contents: + +1. [The layout](#1-the-layout) +2. [One file, start to finish](#2-one-file-start-to-finish) +3. [Findings and where their codes come from](#3-findings-and-where-their-codes-come-from) +4. [Which files and folders are visited](#4-which-files-and-folders-are-visited) +5. [Names and places](#5-names-and-places) +6. [Reading files](#6-reading-files) +7. [The schema's rules](#7-the-schemas-rules) +8. [Changes to the expression language and the context](#8-changes-to-the-expression-language-and-the-context) +9. [Comparison with the reference validator](#9-comparison-with-the-reference-validator) +10. [Defects found in other projects](#10-defects-found-in-other-projects) +11. [Adding a check](#11-adding-a-check) +12. [Tests](#12-tests) +13. [Module reference](#13-module-reference) + +--- + +## 1. The layout + +The code is grouped by HOW a check works, not by what it looks at. Names and places are +checked against lookup tables (`rules.files`, `rules.directories`); everything else the +schema says about a file is a rule with selectors, run by one engine that hands each +rule's payload to the module that knows it. That is the reference validator's own +structure (`applyRules.ts`). + +Everything that turns a dataset into findings is one package, `bids_validator/checks/`. +What it builds on stays where upstream has it, beside the package: + +```text +src/bids_validator/ + __main__.py the command line + issues.py what a finding is + context.py what is known about one file (upstream) + expression_language.py what a schema expression evaluates to (upstream) + bids_validator.py, bidsignore.py, types/ (upstream) + checks/ the validation layer + __init__.py collect_issues: which checks run, in what order + walk.py which files and folders are visited + integrity.py inheritance.py filenames.py filename_rules.py + rules.py expressions.py fields.py tables.py values.py schema_cache.py +``` + +| Module (under `src/bids_validator/`) | Question it answers | Reference (bids-validator) | +|---|---|---| +| `checks/__init__.py` | Which checks run, in what order? | `validators/bids.ts` | +| `checks/walk.py` | Which files and folders are visited? | `schema/walk.ts`, `files/ignore.ts`, `validators/bids.ts` | +| `checks/integrity.py` | Can the file be read at all? | `validators/internal/emptyFile.ts`, the loaders in `files/` | +| `checks/inheritance.py` | Does a deeper JSON file change a value set above it? | `loadSidecar` in `schema/context.ts` | +| `checks/filenames.py` | Is the name, and the place, legal? | `validators/filenameValidate.ts`, `filenameCase.ts`, `findDirRuleMatches` | +| `checks/filename_rules.py` | Which `rules.files` rules describe this file's name? | `validators/filenameIdentify.ts` | +| `checks/rules.py` | Which schema rules apply to the file? Hands each payload to its module; reports nothing itself. | `schema/applyRules.ts` | +| `checks/expressions.py` | Payload `checks`: are the rule's expressions true? | `evalRuleChecks` in `applyRules.ts` | +| `checks/fields.py` | Payload `fields`: are the JSON fields there, with valid values? | `evalJsonCheck` in `applyRules.ts` | +| `checks/tables.py` | Payloads `columns`, `initial_columns`, `index_columns` | `schema/tables.ts` | +| `checks/values.py` | Does one value satisfy its schema definition? | `validators/json.ts` (ajv) | +| `checks/schema_cache.py` | Work something out from the schema once, not for every file | (the reference loads its schema into plain objects once) | +| `issues.py` | What is a finding? (data only: no file, no schema) | `types/issues.ts`, `issues/datasetIssues.ts` | +| `context.py` | What is known about one file? (upstream) | `schema/context.ts` | +| `expression_language.py` | What does a schema expression evaluate to? (upstream) | `schema/expressionLanguage.ts` | + +The tests mirror the layout: the tests of `checks/.py` are in +`tests/checks/test_.py`, and `tests/checks/test_collect_issues.py` tests the +package's `collect_issues`. + +```mermaid +flowchart TD + CLI["__main__.py: the command line"] --> CHECKS["checks/__init__.py: the order and the loop"] + CHECKS --> WALK["checks/walk.py: which files"] + CHECKS --> INT["checks/integrity.py"] + CHECKS --> INHERIT["checks/inheritance.py"] + CHECKS --> FN["checks/filenames.py"] + CHECKS --> RE["checks/rules.py"] + FN --> FR["checks/filename_rules.py: which filename rules"] + RE --> FR + RE --> EC["checks/expressions.py: payload checks"] + RE --> FC["checks/fields.py: payload fields"] + RE --> TC["checks/tables.py: payload columns"] + EC --> EL["expression_language.py"] + FC --> VAL["checks/values.py"] + INHERIT --> VAL + INT --> CTX["context.py"] + WALK --> CTX + EL --> CTX +``` + +Each check module lists the codes it reports at its top (section 3) and builds its +findings from them; `issues.py` only says what a finding is. Files are read only through +`context.py`. No check module imports another check module's checks: the two that both +need a file's filename rules get them from `checks/filename_rules.py`, and the rule +engine only imports the payload handlers it dispatches to. + +## 2. One file, start to finish + +`checks.collect_issues(tree, schema)` runs the dataset checks once, then walks the +dataset and gives every visited folder to the folder checks and every visited file to +the file checks, in this order: + +```python +DATASET_CHECKS = (dataset_integrity_issues,) +FOLDER_CHECKS = (folder_issues,) +FILE_CHECKS = (integrity_issues, inheritance_issues, filename_issues, rule_issues) +``` + +That is the reference validator's order: problems reading a file first (its context +loaders report them), then the name, then the schema rules. A directory recording +(`sub-01_task-x_meg.ds/`) is a folder on disk but a file to BIDS, so it gets the file +checks; a file check never has to ask whether it was given a folder. + +```mermaid +flowchart TD + W["checks/walk.py yields a Context"] --> D{"a plain folder?"} + D -- yes --> FOLDER["folder_issues: rules.directories"] + D -- no --> I["integrity_issues: empty? JSON parses? header readable? table shape?"] + I --> H["inheritance_issues: does a deeper sidecar change a value?"] + H --> F["filename_issues: matching_filename_rules, then hold the name to them; letter case"] + F --> R["rule_issues: hide_unused_name_parts, then every rule whose selectors all hold"] + R --> P1["checks payload: checks.expressions"] + R --> P2["fields payload: checks.fields"] + R --> P3["columns payloads: checks.tables"] +``` + +A file's `Context` is built once and shared by every check, so whatever it loads (the +merged sidecar, the NIfTI header, the table) is loaded once. The dataset is walked once, +and the JSON files a file inherits from are found once per file +(`Context.sidecar_files`), however many checks need them. + +From Python: + +```python +from bidsschematools.schema import load_schema +from bids_validator.checks import collect_issues +from bids_validator.types.files import FileTree + +issues = collect_issues(FileTree.read_from_filesystem('my_dataset'), load_schema()) +for issue in issues: + print(issue.severity.value, issue.code, issue.location, issue.rule) +``` + +`collect_issues(..., dataset_checks=(), folder_checks=(), file_checks=(filename_issues,))` +runs one check alone. + +From the command line (`bids-validator my_dataset`), on a small dataset with a few +deliberate problems (recommended-field warnings left out): + +```console +warning: README_FILE_SMALL: README + The recommended file '/README' is very small. Please consider expanding it ... +warning: TOO_FEW_AUTHORS: dataset_description.json + The 'Authors' field of 'dataset_description.json' should contain an array of ... +error: TSV_VALUE_INCORRECT_TYPE: participants.tsv + line 2: 'thirty' +error: T1W_FILE_WITH_TOO_MANY_DIMENSIONS: sub-01/anat/sub-01_T1w.nii.gz + _T1w.nii[.gz] files must have exactly three dimensions. +error: EMPTY_FILE: sub-01/func/sub-01_task-rest_bold.nii.gz + Empty files not allowed. +error: NIFTI_HEADER_UNREADABLE: sub-01/func/sub-01_task-rest_bold.nii.gz + We were unable to parse header data from this NIfTI file. ... +error: SIDECAR_KEY_REQUIRED: sub-01/func/sub-01_task-rest_bold.nii.gz + the required field 'TaskName' is missing +error: JSON_SCHEMA_VALIDATION_ERROR: sub-01/func/sub-01_task-rest_bold.json + RepetitionTime must be > 0 + +6 error(s), 61 warning(s) +``` + +The reference validator reports the same findings on this dataset, except one +`JSON_KEY_RECOMMENDED` (section 9.3, item 2). The exit code is 1 when there is an +error, 0 otherwise; `-v` adds the schema rule of each finding. + +## 3. Findings and where their codes come from + +`issues.py` is the data model and nothing else: it reads no file and no schema. + +* `IssueDefinition(code, severity, reason)` is a KIND of finding, such as `EMPTY_FILE`. +* `Issue` is one finding: `code`, `severity` (`error` or `warning`), `location` (the + dataset-relative path, without a leading `/`; `None` for the dataset as a whole), + `message`, `sub_code` (a field, column or entity name) and `rule` (the schema rule's + dotted path). `definition.make_issue(location, message=..., sub_code=..., rule=...)` + makes one; the message defaults to the definition's `reason`. +* `DatasetIssues` holds every finding of one run, in order. + +A code is defined in exactly one place, and that place is decided by whether the schema +defines it: + +* **The schema defines it**, so it is READ from the schema where it is reported: + `IssueDefinition.from_schema(block)` turns the schema's `{code, level, message}` into a + definition. That covers the `issue` of every `rules.checks` rule, the `issue` some + fields carry (`NO_AUTHORS`, `B0_FIELD_IDENTIFIER_RECOMMENDED` ...), and six entries of + `rules.errors`: `NotIncluded`, `EmptyFile`, `NiftiHeaderUnreadable`, `JsonInvalid`, + `InvalidJsonEncoding`, `JsonSchemaValidationError`. +* **Only the reference validator defines it** (bids-validator `src/issues/list.ts`), so + it is WRITTEN OUT, once, at the top of the module that reports it: + +| Module | Codes it writes out | +|---|---| +| `checks.filenames` | `ENTITY_WITH_NO_LABEL`, `INVALID_ENTITY_LABEL`, `MISSING_REQUIRED_ENTITY`, `ENTITY_NOT_IN_RULE`, `MISSING_DATATYPE`, `DATATYPE_MISMATCH`, `EXTENSION_MISMATCH`, `INVALID_LOCATION`, `FILENAME_MISMATCH`, `ALL_FILENAME_RULES_HAVE_ISSUES`, `CASE_COLLISION` | +| `checks.integrity` | `MISSING_DATASET_DESCRIPTION`, and what the context's loaders raise: `JSON_NOT_AN_OBJECT`, `TSV_EMPTY_LINE`, `TSV_EQUAL_ROWS`, `TSV_COLUMN_HEADER_DUPLICATE`, `INVALID_GZIP`, `MULTIPLE_INHERITABLE_FILES` | +| `checks.inheritance` | `SIDECAR_FIELD_OVERRIDE` | +| `checks.expressions` | `CHECK_ERROR` | +| `checks.fields` | `SIDECAR_KEY_REQUIRED`, `SIDECAR_KEY_RECOMMENDED`, `JSON_KEY_REQUIRED`, `JSON_KEY_RECOMMENDED` | +| `checks.tables` | `TSV_COLUMN_MISSING`, `TSV_COLUMN_ORDER_INCORRECT`, `TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED`, `TSV_ADDITIONAL_COLUMNS_MUST_DEFINE`, `TSV_ADDITIONAL_COLUMNS_UNDEFINED`, `TSV_COLUMN_TYPE_REDEFINED`, `TSV_VALUE_INCORRECT_TYPE`, `TSV_PSEUDO_AGE_DEPRECATED`, `TSV_INDEX_VALUE_NOT_UNIQUE` | + +A check then reports a written-out code by name, +`MISSING_DATATYPE.make_issue(location, ...)`, so a typo is a `NameError` the linter +catches, not a wrong code at run time. + +The context's loaders (`context.py`) raise a `ValidationError` carrying a code (they +cannot import the check modules). `checks.integrity.LOADER_ERRORS` and +`LOADER_ERRORS_IN_SCHEMA` say where each of those codes is defined. + +The reference validator keeps all its codes in one list (`src/issues/list.ts`, which +also copies the six the schema defines). Here each list sits beside the checks that use +it, and `tests/checks/test_codes.py` keeps the one-place rule true: no code is written +out twice, no code the schema defines is written out at all, every definition is named +after its code, every `rules.errors` entry a module reads exists, and every code a +loader raises has a definition. + +## 4. Which files and folders are visited + +`checks.walk.iter_contexts(dataset)` yields a context for every file AND every folder +that is not ignored, each folder's files before its subfolders, as in the reference. + +| What | Treatment | +|---|---| +| `/sourcedata/`, `/derivatives/`, `/code/`, `/stimuli/`, `/log/`, `/doc/` | ignored: the reference's default list, anchored at the dataset root (`DEFAULT_IGNORES`); a deeper `sub-01/log/` is checked | +| hidden files and folders (`.git`, `.DS_Store`, `.datalad`) | ignored at every level (`PRUNED`) | +| whatever `.bidsignore` lists | ignored; `.bidsignore` itself is visited and exempt from `NOT_INCLUDED` | +| a directory recording: entities, a suffix and an extension the schema spells with `/` (`.ds/`, `.mefd/`, `.ome.zarr/`, and a bare `/`) | visited as ONE file, not walked into | +| an opaque folder (`rules.directories`: `docs`, `logs` ...) | visited, not walked into | +| a `derivatives` folder in a raw dataset, at any depth | skipped: a separate dataset with separate rules | +| any other folder | visited, then walked into | + +The bare `/` extension (`objects.extensions.Directory`) means a folder named like a file, +for example `sub-16/log/20200910-165242_99fc.../` in an fMRIPrep output, is one unit: +the reference reports it, and does not look inside. + +`checks.walk.is_plain_folder(context)` tells a plain folder from a file or a recording. + +## 5. Names and places + +Nothing is opened to check a name. + +`checks.filenames.folder_issues(context)`: a FOLDER must match a `rules.directories` rule +by exact name (`phenotype`), by entity (a name starting with `sub-` or `ses-`) or by value +(any datatype), as in the reference; otherwise it is `NOT_INCLUDED`. Like the reference, +the depth is not checked. + +`checks.filenames.filename_issues(context)`: a FILE is first IDENTIFIED. +`checks.filename_rules.matching_filename_rules(context)` returns the `rules.files` rules +that describe it: those it matches by fixed path (`dataset_description.json`), by stem +glob (`participants`) or by suffix (derivative rules only in a derivative dataset), +narrowed to the best candidates (the rule for the file's folder first, then the rules +its entities and extension fit). The rules are read out of the schema once into plain +`checks.filename_rules.FilenameRule` values (entities already as short names, a list the +rule does not have as `None`) and indexed by path and suffix, so a file is compared with +a handful of rules rather than all 179. The name is then held to those rules: + +| Code | When | +|---|---| +| `NOT_INCLUDED` | no rule matches | +| `ENTITY_WITH_NO_LABEL` | `acq-` (empty) or `foo` (no hyphen) in a name whose rule uses entities | +| `INVALID_ENTITY_LABEL` | a label breaks its entity's format (`acq-a!b`) | +| `MISSING_REQUIRED_ENTITY` | a required entity is missing (not checked for files at the root) | +| `ENTITY_NOT_IN_RULE` | an entity the rule does not allow | +| `MISSING_DATATYPE` | a data file in no datatype folder (`sub-01/sub-01_T1w.nii.gz`) | +| `DATATYPE_MISMATCH` | a data file in another datatype's folder | +| `EXTENSION_MISMATCH` | an extension the rule does not allow | +| `INVALID_LOCATION` | a valid name under the wrong `sub-`/`ses-` folders | +| `FILENAME_MISMATCH` | entities out of the schema's order or duplicated | +| `ALL_FILENAME_RULES_HAVE_ISSUES` | several rules match and none fits cleanly | +| `CASE_COLLISION` | another file in the folder differs only by letter case | + +`MISSING_DATATYPE` and `DATATYPE_MISMATCH` never apply to `.json`, `.tsv`, `.bvec` and +`.bval` (`SIDECAR_EXTENSIONS`): those may sit above the data they describe. A sidecar lost +in a bogus folder (`sub-01/ses-1/foo/x.json`) is still caught, through the folder's own +`NOT_INCLUDED`. `MISSING_DATATYPE` is the reference's code since 2026-10-08 (commit +`f0cc83fb`, not yet in a release); before it, this case was reported as +`INVALID_LOCATION` here and missed by the reference. + +`checks.filename_rules.hide_unused_name_parts(context, rules)` returns the context as +the schema's other rules see it once the name is identified: a name part that none of +the file's filename rules uses is hidden (the reference's `cleanContext`). A file that +matched no rule has no entities, suffix or extension for the rules to read, so +`.bidsignore` or an unrecognised file is not held to the sidecar rules of whatever its +name resembles. It is a read-only view: nothing is copied or loaded again. + +Both `filename_issues` and `rule_issues` call `matching_filename_rules`. Because of the +index it takes 0.08 s for all 11,360 files of bids-examples (it took 3.2 s before the +index), so working it out twice costs less than keeping the answer would cost in code. + +## 6. Reading files + +`checks.integrity.integrity_issues(context)` reports a file that cannot be read the way +its type needs. It asks the context's own loaders (so each file is read once) and turns +the coded `ValidationError` they raise into a finding: + +| Code | When | +|---|---| +| `EMPTY_FILE` | 0 bytes; for a directory recording, its files add up to 0 (hidden files not counted) | +| `INVALID_JSON_ENCODING` | not UTF-8, or starts with a byte order mark (JSON forbids one) | +| `JSON_INVALID` | does not parse | +| `JSON_NOT_AN_OBJECT` | parses, but is not an object | +| `NIFTI_HEADER_UNREADABLE` | the NIfTI header cannot be read (an empty image gets this AND `EMPTY_FILE`, as in the reference) | +| `TSV_COLUMN_HEADER_DUPLICATE` | two columns with one name | +| `TSV_EMPTY_LINE` | an empty line that is not the last | +| `TSV_EQUAL_ROWS` | a row with a different number of cells than headers | +| `INVALID_GZIP` | a headerless `.tsv.gz` that does not decompress | +| `MULTIPLE_INHERITABLE_FILES` | two sidecars at one level apply equally well (reported on the first) | + +The JSON files a file inherits from are found once, by `Context.sidecar_files`: this check +reports an ambiguous level, the inheritance check compares the files' values, and +`Context.sidecar` and `Context.sidecar_origins` merge them. + +`dataset_integrity_issues(dataset)` reports `MISSING_DATASET_DESCRIPTION`. + +When a load fails, the context gives the rules an empty value (no header, no columns, an +empty JSON object), so a broken file is reported here once, not again as a failed check +in every rule that reads it. Tables are read up to `context.TSV_MAX_ROWS` rows (1000, the +reference's `--max-rows` default). + +## 7. The schema's rules + +### 7.1 How a rule is applied + +Every rule with selectors has the same shape: when every selector holds, each payload +says what must then be true. + +```yaml +ReadmeFileSmall: # rules.checks.general.ReadmeFileSmall + issue: {code: README_FILE_SMALL, level: warning, message: "The recommended file ..."} + selectors: [match(path, '^/README')] + checks: [size > 150] +``` + +`checks.rules.rule_issues(context)` evaluates each rule's selectors (stopping at the +first that is not true) and hands each payload to its handler, in the rule's own order, +from `checks.rules.PAYLOAD_HANDLERS`: + +| Payload | Handler | Rule groups | +|---|---|---| +| `checks` | `checks.expressions.check_issues` | `rules.checks` | +| `columns`, `initial_columns`, `index_columns` | `checks.tables.column_issues`, `initial_column_issues`, `index_column_issues` | `rules.tabular_data` | +| `fields` | `checks.fields.field_issues` | `rules.sidecars`, `rules.json`, `rules.dataset_metadata` | + +`null` follows the schema's documentation (bids-specification `src/schema/README.md`, +"The special value null"): "if an expression (selector or check) evaluates to null, the +null will be interpreted equivalent to false. That is, a null selector will not apply the +current rule, and a null check will fail." An expression that raises counts as `null`, as +in the reference (`_evalCheck` in `applyRules.ts`); it is logged at `DEBUG` level. A +failed `checks` rule reports its own `issue`, with `{...}` placeholders in the message +filled in for the file; a rule without a complete issue reports `CHECK_ERROR`. + +### 7.2 Which rules run + +`checks.rules.NOT_ENABLED_YET = ('associations', 'ome', 'tiff')`: the parts of the +context not built yet. `names_read_by(rule)` parses a rule's expressions and lists the +context names it reads; a rule reading a held-back name is skipped, because evaluating +it against an empty placeholder would report problems that are not there ("a task scan +needs an events file" would fire on every task scan). With the schema of bidsschematools +1.2.4 (BIDS 1.11.1): + +| Group | Rules | Run | +|---|---|---| +| `rules.checks` | 129 | 82 | +| `rules.sidecars` | 174 | 174 | +| `rules.json` | 35 | 35 | +| `rules.dataset_metadata` | 6 | 6 | +| `rules.tabular_data` | 28 | 28 | + +(`rules.errors` and the derivative `rules.files` entries also carry selectors but no +payload, so they are skipped in the per-file loop.) + +### 7.3 JSON fields + +`checks.fields.field_issues` handles the `fields` payload: + +* `rules.sidecars` describe a data file's SIDECAR (its JSON files merged by the + inheritance principle, `context.sidecar`); a missing field is `SIDECAR_KEY_REQUIRED` + (error) or `SIDECAR_KEY_RECOMMENDED` (warning), on the data file. A `.json` file, and + `README`-like files, have no sidecar, so these rules skip them. +* `rules.json` and `rules.dataset_metadata` describe one JSON file (`context.json`); a + missing field is `JSON_KEY_REQUIRED` or `JSON_KEY_RECOMMENDED`. +* A field may carry its own issue (`NO_AUTHORS`, `B0_FIELD_IDENTIFIER_RECOMMENDED` ...), + used instead, with the field's level as its severity. +* A level addendum "required if `Key` is `Value`" raises the level when the sidecar's + `Key` is that string. (In the current schema no addendum can take effect: the rule's own + selectors exclude the triggering value, and for `rules.json` the reference reads the + addendum's key from the sidecar, which a JSON file does not have. Kept for parity.) +* In a derivative dataset, fields are optional unless the rule selects + `dataset.dataset_description.DatasetType == "derivative"`. +* A present field's value is checked against its `objects.metadata` definition + (`checks.values.value_errors`); a wrong value is `JSON_SCHEMA_VALIDATION_ERROR`, on + the JSON file that holds the key (`Context.sidecar_origins`), and for a sidecar key + once per dataset, not once per data file inheriting it. + +`checks.inheritance.inheritance_issues` warns `SIDECAR_FIELD_OVERRIDE` where a deeper +JSON file gives a field a different value than a JSON file above it (on the deeper file, +once per data file inheriting both). Values are compared by content. + +`checks.values.value_errors` implements the JSON Schema the metadata definitions use, +with the behaviour of ajv as the reference configures it: the first failing keyword only +(in ajv's order: `type`, then `enum` and `anyOf`, then the number, string, array and +object keywords), every branch's first error plus `must match a schema in anyOf` when no +alternative fits, ajv's messages (`must be > 0`), and formats SEARCHED rather than +matched (ajv's `RegExp.test`), so a format pattern without `^`/`$` only needs to occur +somewhere in the value. + +### 7.4 Tables + +`checks.tables` handles the three table payloads, following `schema/tables.ts`: + +| Code | When | +|---|---| +| `TSV_COLUMN_MISSING` | a required column is absent (for an initial column, reported by the order check) | +| `TSV_COLUMN_ORDER_INCORRECT` | an initial column is not where the rule puts it | +| `TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED` | a column the rule does not list, where none are allowed | +| `TSV_ADDITIONAL_COLUMNS_MUST_DEFINE` | the same, where extra columns must be defined in the sidecar and this one is not | +| `TSV_ADDITIONAL_COLUMNS_UNDEFINED` | the same, where they are allowed but this one is not defined (warning) | +| `TSV_COLUMN_TYPE_REDEFINED` | the sidecar contradicts the schema's definition of a column (on the sidecar) | +| `TSV_VALUE_INCORRECT_TYPE` | the first value of a column that breaks its type, levels or bounds | +| `TSV_PSEUDO_AGE_DEPRECATED` | `89+` in an `age` column (once) | +| `TSV_INDEX_VALUE_NOT_UNIQUE` | two rows share the index columns' values | + +A column's type comes from the schema (`objects.columns`), refined by the sidecar's own +definition of the column (`Format`, `Levels`, `Units`, `Minimum`, `Maximum`), which may +narrow it but not contradict it. `n/a` is always accepted. Column names containing a dot +(`key_resp.rt`, as PsychoPy writes them) work: see section 8.2. + +## 8. Changes to the expression language and the context + +Both modules are upstream (the expression language is Dan Brady's, PR #71; the context +is Chris Markiewicz's and Dan's). Every change keeps their structure and follows the +schema's documentation or, where it is silent, the reference implementation. + +### 8.1 The expression language + +The schema's 77 expression test vectors all passed before these changes. They never +compare numbers, never read values loaded from real files, and never test the truth of +an empty array; the schema's rules do all three. + +| Behaviour | Before | After | Rules affected | +|---|---|---|---| +| `<`, `<=`, `>`, `>=` | `KeyError` | numbers as numbers, two strings as text, `null` gives `false` | 30 of 129 `rules.checks` (with `**`) | +| `**` | `KeyError` | power | `func.RepetitionTimeMismatch`, `micr.PixelSizeInconsistent` | +| `null + 1`, `null * 2` | `TypeError` | `null` | any arithmetic on a missing field | +| `1 + "a"` | `TypeError` | `"1a"` | | +| `-3 % 2`, `1 / 0` | `1`, `ZeroDivisionError` | `-1`, infinity | | +| truth of `[]`, `{}` | false (Python's rule) | true (the language's rule) | every `!`, `&&`, `\|\|`, selector and check | +| `type("abc")`, `type()` of numpy arrays, tuples, header objects | `"null"` | `"string"`, `"array"`, `"array"`, `"object"` | 29 rules gated on `type(...) != "null"`, which never ran | +| `[1, 2][5]`, `[1, 2][-1]` | `IndexError`, `2` | `null`, `null` | | +| `"x" in record` (an object) | `TypeError` | field present and not null | | +| `".git" in namespace` | inserted a key into the Namespace | tests the keys only | | +| `min`/`max` of TSV cells (`["0.5", "n/a"]`) | `ValueError` | numbers, `n/a` skipped; infinity if none | `events.*` onset checks | +| `length(3)`, `sorted(null)`, `count(null, 1)` | `TypeError`/`AttributeError` | `null` | | +| `intersects("rad", [...])` | `false` (compared characters) | `["rad"]` | | +| `match(42, "4")`, an invalid pattern | `TypeError`, `re.error` | `true`, `null` | | +| an unknown name | `AttributeError` | `null` | | +| an unknown function (`len`) | `'NoneType' object is not callable` | `TypeError: Unknown function 'len'` | `anat.PDT2Echos` (a schema defect) | + +New helpers: `truthy` (the language's truth test), `normalize` (numpy and tuples to plain +values), `as_number`, `numeric_values` (replaces `filter_strs`, which dropped numeric +text), and a cached `parse`. `LookupProxy` reads a context attribute once instead of +twice. Measured with the 48 new test vectors: 12 right before, 48 after. + +### 8.2 The context + +| Change | Why | +|---|---| +| `ValidationError(code, message, line=, location=)` | the upstream TODO ("Add issue structure"): a loader's error carries the code it is reported under | +| `FileContents`, a `dict` with attribute access, holds file contents (JSON, columns) | a bidsschematools `Namespace` reads `"key_resp.rt"` as a path and cannot even list a mapping holding such a key | +| `load_json` raises `INVALID_JSON_ENCODING`, `JSON_INVALID`, `JSON_NOT_AN_OBJECT` | so integrity checks can report them | +| `load_tsv`/`load_tsv_gz` raise `TSV_COLUMN_HEADER_DUPLICATE`, `TSV_EMPTY_LINE`, `TSV_EQUAL_ROWS`, `INVALID_GZIP`; drop a byte order mark; a header with no rows gives empty columns, not none | as the reference; mne-bids writes a byte order mark | +| `Context.read_columns()` (raises) and `Context.columns` (empty on error), up to `TSV_MAX_ROWS` | one place decides how a table is read | +| `Context.sidecar`: cached, empty for a `.json` file, `RepetitionTime` rounded to milliseconds, a broken sidecar skipped rather than fatal | as the reference's `loadSidecar` | +| `walk_back` prefers a candidate with every entity of the source before calling it ambiguous | as the reference's `walkBack` | +| `Context.sidecar_files`: the JSON files a file inherits from, nearest first, found once per file and read by `Context.sidecar`, `Context.sidecar_origins`, the integrity checks and the inheritance checks; `load_sidecar(file)` builds the same merge for one file on its own | four readers each searched the tree again (1.7 s of a 30 s run on bids-examples; 0.6 s now) | +| `Context.sidecar_origins` | where each sidecar key comes from, for value findings; kept only as long as the file's context | +| `Context.json` empty and `Context.nifti_header` `None` when unreadable | the rule sees "no value" instead of raising | +| `Dataset.dataset_description` defaults `DatasetType` (`derivative` with `GeneratedBy`, else `raw`) and is empty when missing | rules selecting `DatasetType == "raw"` otherwise skip every dataset leaving it out | +| `Dataset.checked_sidecar_values` | so a wrong value in a shared sidecar is reported once; the reference keeps the same set on its dataset (`sidecarKeyValidated`) | +| `datatype_to_modality` returns `None` for `phenotype` | it raised `KeyError` | +| `find_datatypes` tests `datatypes.keys()` | `".git" in namespace` inserted a key into the shared, cached schema, and every later context failed | +| `exists()` resolves paths by hand (`tree_contains`) | a leading `/` was never found (so `DUPLICATE_FILES` could not fire) and a `bids::` URI raised | + +## 9. Comparison with the reference validator + +### 9.1 Method + +Both validators were run on the same datasets and their findings compared by +`(code, location)`. Reference: bids-validator 3.0.2 (Deno), schema 1.2.7. This package: +bidsschematools 1.2.4. The scripts are not part of the repository. + +### 9.2 Results + +**All 84 bids-examples datasets** (at the submodule commit) with a +`dataset_description.json`: + +| | Findings | +|---|---| +| reported by both | 141,420 | +| only by the reference | 1,038 | +| only here | 475 | +| crashes | 0 | + +Every one of the 1,513 differences has a known cause (9.3). + +**A real multimodal dataset** (MRI, fieldmaps, DWI, PET, MEG, EEG, MRS, physiology, +written by BIDS Manager): 50 findings by both, 0 only by the reference, 7 only here (all +explained in 9.3). The reference CRASHES on the full dataset (section 10); the comparison +used a copy without the eye-tracking files. + +**One dataset per test case** (59, built from the unit tests): 54 identical; the other 5 +are explained in 9.3. + +**Speed**: all 84 bids-examples datasets in 27 to 28 s here and 33 s for the reference, +run one after the other on the same machine (2026-11-09). Indexing the filename rules +(section 5) took this package from 32 s to 27 s. + +### 9.3 Every difference, and why + +Only in the reference: + +| Code | Count | Why | +|---|---|---| +| `EVENTS_TSV_MISSING`, `*_CHANNEL_COUNT_MISMATCH`, `MISSING_MAGNITUDE1_FILE`, `REQUIRED_COORDSYSTEM` | 984 | rules reading `associations`: held back until issue #82 | +| `HED_ERROR` | 32 | HED validation is not part of this package yet | +| `SIDECAR_WITHOUT_DATAFILE` | 12 | needs to know which JSON files any rule used, which comes with associations | +| `SIDECAR_FIELD_OVERRIDE` | 6 | the reference compares objects with JavaScript's `!==`, by reference, so the same object written at two levels is reported; here it is compared by content | +| `NOT_INCLUDED` on `sub-*/figures/` | 4 | a reference defect: `figures/` is in that dataset's `.bidsignore`, but the reference tests a folder's path without its trailing `/`, so folder-only patterns never match folders | + +Only here: + +| Code | Count | Why | +|---|---|---| +| `B0_FIELD_SOURCE_RECOMMENDED`, `SIDECAR_KEY_*` for `AnatomicalImage`, `NonlinearGradientCorrection`, `AnatomicalLandmarkCoordinates` | 423 | a reference defect: these rules select on `dataset.datatypes` or `dataset.modalities`, which the reference never fills in the context its rules see, so they can never apply there | +| `JSON_KEY_RECOMMENDED` for `DatasetType` | 51 | a reference defect: it writes its default `DatasetType` into its cached copy of `dataset_description.json`, so the rule checking the file's own content never sees the field missing | +| `MISSING_DATATYPE` | 1 | in the reference's `main` branch, not yet in a release | +| `MRS_NIFTI_CONSISTENCY` | 2 (real dataset) | the reference stops reading a NIfTI header before its extensions, so it never sees the NIfTI-MRS metadata and never runs this rule | +| `DUPLICATE_FILES` | (test cases) | a reference defect: its `exists()` cannot find a path starting with `/`, so this rule can never fire there | +| `SAMPLES_TSV_MISSING` | (test cases) | the `dataset.modalities` defect above | + +The reference's 3.0.2 release also still reports `DATATYPE_MISMATCH` for a `.json` in +another datatype's folder; its `main` branch no longer does, and neither does this +package. + +## 10. Defects found in other projects + +Found while building and comparing; reported here, not fixed (except where this package +needed to work around them). + +| Where | Defect | +|---|---| +| bids-validator (reference) | crashes (`Cannot read properties of undefined (reading 'path')`) on an eye-tracking dataset without an associated events file, while filling the placeholder `{associations.events.path}` of an issue message | +| bids-validator | `exists()` never finds a path starting with `/`, so `DUPLICATE_FILES` cannot fire | +| bids-validator | `dataset.datatypes` and `dataset.modalities` are never filled for the rules, so seven rules can never apply: `checks.dataset.SamplesTSVMissing`, `json.meg.MEGCoordsystemAnatomicalMRI`, `sidecars.meg.MEGwithEEG`, `sidecars.mri.PETMRISequenceSpecifics`, `sidecars.mri.MRIAnatomicalLandmarks`, `sidecars.mri.MRIEchoPlanarImagingAndB0FieldSource`, `sidecars.mrs.MRSConditionalAnatomicalImage` | +| bids-validator | the `DatasetType` default is written into the cached `dataset_description.json`, hiding the missing field | +| bids-validator | `.bidsignore` folder patterns (`figures/`) are not applied to the folders themselves | +| bids-validator | `SIDECAR_FIELD_OVERRIDE` compares objects by reference | +| bidsschematools `Namespace` | `".git" in namespace` inserts an empty key (worked around: `.keys()`) | +| bidsschematools `Namespace` | a key with a dot cannot be held at all (worked around: `FileContents`) | +| BIDS schema `rules.checks.anat.PDT2Echos` | uses `len(...)`, not the language's `length(...)`; the check always fails where it applies | +| BIDS schema `rules.checks.mrs.MRSNiftiConsistency` | compares `SpectrometerFrequency` exactly; the NIfTI-MRS header holds a 32-bit float (`123.26260375976562` against `123.262604`), so it fires on float noise | +| BIDS schema, field level addenda | none can take effect in the current schema (section 7.3) | + +## 11. Adding a check + +* A new check on each file: write a function `Context -> list[Issue]` in the module that + matches how it works (or a new one), and add it to `checks.FILE_CHECKS`. On each + folder: `checks.FOLDER_CHECKS`. On the dataset as a whole: `Dataset -> list[Issue]` + in `checks.DATASET_CHECKS`. +* Its codes: if the schema defines one, read it with `IssueDefinition.from_schema`; + otherwise write it out at the top of the module as + `CODE = IssueDefinition('CODE', Severity..., 'reason')`, with the reference + validator's severity and reason when it has the code. `tests/checks/test_codes.py` + fails if the code is defined anywhere else too. +* A new payload kind in the schema: write a handler `(path, rule, context) -> list[Issue]` + in its own module, and add it to `checks.rules.PAYLOAD_HANDLERS`. +* Something worked out from the schema alone, and needed for every file: decorate the + function with `checks.schema_cache.once_per_schema`. +* When associations exist (#82): remove `'associations'` from + `checks.rules.NOT_ENABLED_YET`; 45 more `rules.checks` rules then run (two more wait for + `ome` and `tiff`). + +## 12. Tests + +| File | Tests | What they pin | +|---|---|---| +| `tests/checks/test_codes.py` | 11 | where every code is defined: once, beside its checks, or in the schema | +| `tests/checks/test_walk.py` | 22 | ignores, `.bidsignore`, recordings, opaque and derivative folders, order, subjects | +| `tests/checks/test_filename_rules.py` | 8 | rules read into plain values, identification by suffix, path and stem, the folder's rule preferred, derivative rules, the index against a full scan | +| `tests/checks/test_filenames.py` | 36 | every filename code, folders, inheritable files, case, `hide_unused_name_parts`, recordings, derivatives | +| `tests/checks/test_integrity.py` | 21 | every integrity code, readable files staying quiet, sidecar ambiguity | +| `tests/checks/test_rules.py` | 38 | the gate, payload order, null and error semantics, a failing and a passing dataset per sample rule | +| `tests/checks/test_fields.py` | 11 | missing fields, a field's own issue, derivatives, addenda, values reported once where written, and the overrides of `checks.inheritance` | +| `tests/checks/test_tables.py` | 17 | every table code, `n/a`, levels, bounds, redefinition, dotted column names | +| `tests/checks/test_values.py` | 32 | each JSON Schema keyword with ajv's messages, `anyOf`, format search, JSON equality | +| `tests/test_expression_language.py` | 145 | the schema's 77 vectors, 48 more behaviours, numpy and tuple values | +| `tests/test_context.py` | 41 | upstream's, plus the context changes above | +| `tests/checks/test_collect_issues.py`, `tests/test_main.py`, `tests/test_issues.py`, `tests/checks/test_schema_cache.py` | 6, 8, 11, 1 | the order, folders and files getting their own checks, one walk equals the checks apart, one inheritance search per file, the CLI, the data model, the schema cache | + +`tests/helpers.py` holds the small dataset builders they share (`write`, `build`, +`clean_dataset` (a dataset both validators find clean), `nifti`, `gzipped`, and +`issues_of`/`findings`, which run some checks or all of them). + +664 pass with the bids-examples data (1 skipped: the MRS data needs Git LFS); 635 pass +and 30 skip without it. `ruff check`, `ruff format`, `codespell` and +`mypy --python-version 3.13 src tests` are clean. + +## 13. Module reference + +| Module | Public names | +|---|---| +| `checks` | `collect_issues(tree, schema, *, dataset_checks, folder_checks, file_checks)`, `DATASET_CHECKS`, `FOLDER_CHECKS`, `FILE_CHECKS` | +| `issues` | `Issue`, `IssueDefinition` (`from_schema`, `make_issue`), `Severity` (`from_level`), `DatasetIssues` | +| `checks.walk` | `iter_contexts(dataset, ignore=None)`, `build_ignore(tree)`, `is_plain_folder(context)`, `DEFAULT_IGNORES`, `PRUNED`, `BIDSIGNORE`, `DERIVATIVES_DIR` | +| `checks.filename_rules` | `matching_filename_rules(context)`, `hide_unused_name_parts(context, rules)`, `all_filename_rules(schema)`, `FilenameRule` | +| `checks.schema_cache` | `once_per_schema` | +| `checks.filenames` | `filename_issues(context)`, `folder_issues(context)`, `SIDECAR_EXTENSIONS`, and the codes of section 3 | +| `checks.integrity` | `integrity_issues(context)`, `dataset_integrity_issues(dataset)`, `LOADER_ERRORS`, `LOADER_ERRORS_IN_SCHEMA`, and the codes of section 3 | +| `checks.rules` | `rule_issues(context)`, `apply_rule(path, rule, context)`, `runnable_rules(schema)`, `names_read_by(rule)`, `PAYLOAD_HANDLERS`, `NOT_ENABLED_YET`, `PreparedRule` | +| `checks.expressions` | `check_issues(path, rule, context)`, `evaluate_or_null(expression, context, path)`, `CHECK_ERROR` | +| `checks.inheritance` | `inheritance_issues(context)`, `SIDECAR_FIELD_OVERRIDE` | +| `checks.fields` | `field_issues(path, rule, context)`, and the codes of section 3 | +| `checks.tables` | `column_issues`, `initial_column_issues`, `index_column_issues` (all `(path, rule, context)`), `AllowedValues`, and the codes of section 3 | +| `checks.values` | `value_errors(value, definition, formats)`, `json_equal(a, b)` | +| `context` (new or changed) | `ValidationError`, `FileContents`, `TSV_MAX_ROWS`, `load_json`, `load_tsv`, `load_tsv_gz`, `tree_contains`, `Context.read_columns`, `Context.columns`, `Context.sidecar_files`, `Context.sidecar_origins`, `Context.sidecar`, `Context.json`, `Context.nifti_header`, `Dataset.dataset_description`, `Dataset.checked_sidecar_values` | +| `expression_language` (new or changed) | `truthy`, `normalize`, `as_number`, `numeric_values`, `parse`, `bin_ops`, `right_ops`, `evaluate` | diff --git a/src/bids_validator/__main__.py b/src/bids_validator/__main__.py index befcc7c..e1ce007 100644 --- a/src/bids_validator/__main__.py +++ b/src/bids_validator/__main__.py @@ -1,6 +1,10 @@ # ruff: noqa: D100 # ruff: noqa: D103 +# (The two lines above tell the linter not to ask for a docstring on this module, D100, +# or on its functions, D103. This file is what runs for ``bids-validator ``.) +# typer is an optional dependency (``pip install bids_validator[cli]``): the library can +# be used without it, so its absence is reported in a sentence, not a traceback. try: import typer except ImportError: @@ -8,52 +12,26 @@ raise SystemExit(1) from None import sys -from collections.abc import Iterator from typing import Annotated from bidsschematools.schema import load_schema -from bidsschematools.types.context import Subject from bidsschematools.types.namespace import Namespace -from bids_validator import BIDSValidator -from bids_validator.context import Context, Dataset, Sessions +from bids_validator.checks import collect_issues +from bids_validator.issues import DatasetIssues, Severity from bids_validator.types.files import FileTree +# The command-line application. ``@app.command()`` below turns ``main``'s parameters into +# the command's arguments and options; ``app()`` at the bottom runs it. app = typer.Typer() -def is_subject_dir(tree: FileTree) -> bool: - return tree.name.startswith('sub-') +def validate(tree: FileTree, schema: Namespace, verbose: bool = False) -> DatasetIssues: + """Run every check on the dataset and report what is wrong. - -def walk(tree: FileTree, dataset: Dataset, subject: Subject | None = None) -> Iterator[Context]: - """Iterate over children of a FileTree and check if they are a directory or file. - - If it's a directory then run again recursively, if it's a file file check the file name is - BIDS compliant. - - Parameters - ---------- - tree : FileTree - FileTree object to iterate over - dataset: Dataset - Object containing properties for entire dataset - subject: Subject - object containing subject and session info - - """ - if subject is None and is_subject_dir(tree): - subject = Subject(Sessions(tree)) - - for child in tree.children.values(): - if child.is_dir: - yield from walk(child, dataset, subject) - else: - yield Context(child, dataset, subject) - - -def validate(tree: FileTree, schema: Namespace) -> None: - """Check if the file path is BIDS compliant. + The walk and every check come from :func:`~bids_validator.checks.collect_issues` + (reading, inheritance, names and places, the schema's rules), so the CLI and any + library caller run exactly the same checks. Parameters ---------- @@ -61,14 +39,32 @@ def validate(tree: FileTree, schema: Namespace) -> None: Full FileTree object to iterate over and check schema : Namespace Schema object to validate dataset against + verbose : bool + Also print the schema rule each finding came from + + Returns + ------- + DatasetIssues + Every finding, so the caller can set an exit code. """ - validator = BIDSValidator() - dataset = Dataset(tree, schema) + issues = collect_issues(tree, schema) - for file in walk(tree, dataset): - if not validator.is_bids(file.path): - print(f'{file.path} is not a valid bids filename') + for issue in issues: + print(f'{issue.severity.value}: {issue.code}: {issue.location or "(dataset)"}') + if issue.message: + print(f' {issue.message}') + if verbose and issue.rule: + print(f' rule: {issue.rule}') + + errors = len(issues.by_severity(Severity.ERROR)) + warnings = len(issues.by_severity(Severity.WARNING)) + if issues: + print(f'\n{errors} error(s), {warnings} warning(s)') + else: + print('No problems found') + + return issues def show_version() -> None: @@ -97,6 +93,11 @@ def version_callback(value: bool) -> None: raise typer.Exit() +# ``@app.command()`` registers ``main`` as the command. typer reads its parameters: +# ``bids_path`` (no default) becomes the required argument, ``schema_path`` the option +# ``--schema-path``, and ``Annotated[bool, typer.Option(...)]`` attaches the flag names and +# help text to ``verbose`` and ``version``. ``is_eager`` runs ``--version`` before anything +# else, and its callback exits. @app.command() def main( bids_path: str, @@ -119,8 +120,15 @@ def main( schema = load_schema(schema_path) - validate(root_path, schema) + issues = validate(root_path, schema, verbose=verbose) + + # A validator is normally run from a script or CI job, so the outcome has to be + # readable from the exit code, not only from the printed text. + if issues.has_errors: + raise typer.Exit(code=1) +# True only when the file is run as a program (``python -m bids_validator``), not when it +# is imported by a test. if __name__ == '__main__': app() diff --git a/src/bids_validator/checks/__init__.py b/src/bids_validator/checks/__init__.py new file mode 100644 index 0000000..6985056 --- /dev/null +++ b/src/bids_validator/checks/__init__.py @@ -0,0 +1,145 @@ +"""Run every check on a dataset: one walk, every check, one list of findings. + +This is the only place that says WHICH checks run and IN WHAT ORDER. Each check is a +plain function in its own module, and there are three kinds: + +========================================= =========================================== +check question it answers +========================================= =========================================== +**the dataset** (``Dataset -> list``) +:func:`.dataset_integrity_issues` is there a ``dataset_description.json``? +**each folder** (``Context -> list``) +:func:`.folder_issues` is the folder's name legal? +**each file** (``Context -> list``) +:func:`.integrity_issues` can the file be read at all? +:func:`.inheritance_issues` does a deeper sidecar change a value? +:func:`.filename_issues` is the name, and the place, legal? +:func:`.rule_issues` do the schema's rules hold (checks, fields, + columns)? +========================================= =========================================== + +The dataset is checked first. Then :mod:`.walk` visits every file and +folder, and each gets the checks of its kind, in the reference validator's order +(``validators/bids.ts``): problems reading the file first, then its name, then the +schema's rules. A directory recording (``sub-01_task-x_meg.ds/``) is a folder on disk +but a file to BIDS, so it gets the file checks. + +A file's context is built once and shared by every check, so whatever it has loaded +(its sidecar, its header) is loaded once. + +The modules of this package +--------------------------- +Grouped by how a check works, one module per family of checks: + +========================= ========================================================= +module what it holds +========================= ========================================================= +:mod:`.walk` which files and folders a validation visits +:mod:`.integrity` files that cannot be read the way their type needs +:mod:`.inheritance` a deeper sidecar changing a value set above it +:mod:`.filenames` names and places of files and folders +:mod:`.filename_rules` which of the schema's filename rules a name matches +:mod:`.rules` the rule engine: which schema rules apply to a file, and + which module checks each part of a rule +:mod:`.expressions` the ``checks`` part of a rule: expressions that must hold +:mod:`.fields` the ``fields`` part: keys of sidecars and JSON files +:mod:`.tables` the ``columns`` parts: the columns of TSV tables +:mod:`.values` one value against its schema definition (for fields and + tables) +:mod:`.schema_cache` reading something out of the schema once per schema +========================= ========================================================= + +What they build on lives outside the package: :mod:`bids_validator.context` (what is +known about one file), :mod:`bids_validator.expression_language` (evaluating the +schema's expressions) and :mod:`bids_validator.issues` (what a finding is). +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +from collections.abc import Callable, Iterable +from typing import TYPE_CHECKING + +# Each import below is one check, from the module that owns it. +from ..context import Dataset +from ..issues import DatasetIssues, Issue +from .filenames import filename_issues, folder_issues +from .inheritance import inheritance_issues +from .integrity import dataset_integrity_issues, integrity_issues +from .rules import rule_issues +from .walk import is_plain_folder, iter_contexts + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from bidsschematools.types.namespace import Namespace + + from ..context import Context + from ..types.files import FileTree + +# The names other modules are meant to import from this one. +__all__ = ['DATASET_CHECKS', 'FILE_CHECKS', 'FOLDER_CHECKS', 'collect_issues'] + +# Two type aliases: names for "the shape of a check function", used in the hints below. +# ``Callable[[Dataset], list[Issue]]`` means "a function that takes a Dataset and returns +# a list of Issue". A dataset check gets the whole dataset; a context check gets one file +# or one folder (its Context). +DatasetCheck = Callable[[Dataset], list[Issue]] +ContextCheck = Callable[['Context'], list[Issue]] + +# The checks themselves, in the order they run. Each is a tuple of functions (not of +# function calls: no brackets), so ``collect_issues`` can call each one on each item. +# ``tuple[DatasetCheck, ...]`` means "a tuple of any length whose items are dataset +# checks". To add a check, write the function and add it to the right tuple. + +# Run once, before the walk. +DATASET_CHECKS: tuple[DatasetCheck, ...] = (dataset_integrity_issues,) + +# Run on every plain folder the walk visits. +FOLDER_CHECKS: tuple[ContextCheck, ...] = (folder_issues,) + +# Run on every file (and directory recording) the walk visits, in this order. +FILE_CHECKS: tuple[ContextCheck, ...] = ( + integrity_issues, # can it be read? integrity.py + inheritance_issues, # inherited values changed? inheritance.py + filename_issues, # legal name and place? filenames.py + rule_issues, # the schema's rules rules.py +) + + +def collect_issues( + tree: FileTree, + schema: Namespace, + *, + dataset_checks: Iterable[DatasetCheck] = DATASET_CHECKS, + folder_checks: Iterable[ContextCheck] = FOLDER_CHECKS, + file_checks: Iterable[ContextCheck] = FILE_CHECKS, +) -> DatasetIssues: + """Validate a dataset: every check, on everything it applies to, in order. + + Parameters + ---------- + tree : FileTree + The dataset root, from ``FileTree.read_from_filesystem(root)``. + schema : Namespace + The BIDS schema to validate against, from ``bidsschematools.schema.load_schema()``. + dataset_checks, folder_checks, file_checks : iterables of callables, optional + The checks to run; all of them by default. Pass fewer to run only some, for + example ``dataset_checks=(), folder_checks=(), file_checks=(filename_issues,)``. + (The ``*`` in the signature makes these keyword-only: they must be given by name.) + + Returns + ------- + DatasetIssues + The dataset's findings first, then each file's and folder's, in walk order. + + """ + dataset = Dataset(tree, schema) + # Iterables can only be read once; tuples can be read for every file. + folder_checks, file_checks = tuple(folder_checks), tuple(file_checks) + issues = DatasetIssues() + for dataset_check in dataset_checks: + issues.extend(dataset_check(dataset)) + for context in iter_contexts(dataset): + for check in folder_checks if is_plain_folder(context) else file_checks: + issues.extend(check(context)) + return issues diff --git a/src/bids_validator/checks/expressions.py b/src/bids_validator/checks/expressions.py new file mode 100644 index 0000000..7393088 --- /dev/null +++ b/src/bids_validator/checks/expressions.py @@ -0,0 +1,145 @@ +"""The ``checks`` payload: expressions about a file that must all be true. + +A ``rules.checks`` rule says WHEN it applies (its selectors, decided by the rule engine +in :mod:`.rules`) and WHAT must then be true (its ``checks``):: + + ReadmeFileSmall: # rules.checks.general.ReadmeFileSmall + issue: + code: README_FILE_SMALL + message: The recommended file '/README' is very small. ... + level: warning + selectors: + - match(path, '^/README') # when does the rule apply? + checks: + - size > 150 # what must then be true? + +When the rule applies and a check is not true, the rule FAILS, and it reports its own +``issue``: the code, level and message come from the schema. + +Every selector and check is an expression evaluated by +:func:`.expression_language.interpret`. :func:`evaluate_or_null` adds the one rule the +schema documents for applying them: "if an expression (selector or check) evaluates to +null, the null will be interpreted equivalent to false. That is, a null selector will +not apply the current rule, and a null check will fail." +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +import logging +import re +from collections.abc import Mapping +from typing import TYPE_CHECKING, Any + +from ..expression_language import interpret, truthy +from ..issues import Issue, IssueDefinition, Severity + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from ..context import Context + +# The names other modules are meant to import from this one. +__all__ = ['CHECK_ERROR', 'check_issues', 'evaluate_or_null'] + +# Where this module writes its debug messages. ``logging.basicConfig(level=logging.DEBUG)`` +# in a script shows them; by default they are hidden. +logger = logging.getLogger(__name__) + +# --- what this module reports -------------------------------------------- +# +# A rules.checks rule defines its own issue (code, level, message) in the schema, read +# from the rule when it fails. CHECK_ERROR is defined only by the reference validator +# (bids-validator, src/issues/list.ts): it is reported for a rule whose issue block is +# incomplete. + +# A rules.checks rule failed, but its issue block has no code or message. +CHECK_ERROR = IssueDefinition( + 'CHECK_ERROR', + Severity.ERROR, + 'generic place holder for errors from failed `checks` evaluated from schema.', +) + +# A ``{...}`` placeholder in an issue message, such as ``{entities.atlas}``. +_PLACEHOLDER = re.compile(r'\{([^{}]+)\}') + + +def check_issues(path: str, rule: Mapping[str, Any], context: Context) -> list[Issue]: + """Payload ``checks``: report the rule's issue if any check is not true. + + Parameters + ---------- + path : str + Dotted schema path of the rule (``rules.checks.general.ReadmeFileSmall``). + rule : Mapping + The rule; its selectors already hold for this file. + context : Context + Everything known about one file. + + Returns + ------- + list of Issue + Nothing when every check is true; otherwise the rule's own issue, once. + + """ + if all(truthy(evaluate_or_null(check, context, path)) for check in rule['checks']): + return [] + block = rule.get('issue') or {} + location = context.file.relative_path + if not block.get('code') or not block.get('message'): + return [CHECK_ERROR.make_issue(location, rule=path)] + definition = IssueDefinition.from_schema(block) + message = _fill_placeholders(definition.reason, context, path) + return [definition.make_issue(location, message=message, rule=path)] + + +def evaluate_or_null(expression: str, context: Context, path: str) -> Any: + """Evaluate one expression for a file, reading any error as null. + + The reference validator does the same (``_evalCheck`` in its ``applyRules.ts``): + an expression that raises counts as null, so a selector that raises does not + apply the rule and a check that raises fails it. The error is logged at DEBUG + level, so its cause can be found. + + Parameters + ---------- + expression : str + A selector or a check, such as ``size > 150``. + context : Context + The file the expression is about. + path : str + The rule it belongs to, only for the debug message. + + Returns + ------- + Any + The value: a boolean, number, string, list, mapping, or ``None`` for null. + + """ + try: + return interpret(expression, context) + except Exception as error: # noqa: BLE001 - any failure means "no value", by the spec + logger.debug('%s: %r raised %r on %s', path, expression, error, context.path) + return None + + +def _fill_placeholders(message: str, context: Context, path: str) -> str: + """Fill in the ``{...}`` placeholders of a rule's message for this file. + + A few messages name a value from the context, such as + ``No /atlas-{entities.atlas}_description.json could be found.`` The placeholder is + an expression, evaluated for this file. + """ + return _PLACEHOLDER.sub( + lambda found: _display(evaluate_or_null(found.group(1), context, path)), message + ) + + +def _display(value: Any) -> str: + """Show a value the way the reference validator prints it in a message.""" + if value is None: + return 'null' + if isinstance(value, bool): + return 'true' if value else 'false' + if isinstance(value, list): + return ','.join(_display(item) for item in value) + return str(value) diff --git a/src/bids_validator/checks/fields.py b/src/bids_validator/checks/fields.py new file mode 100644 index 0000000..a8cd18e --- /dev/null +++ b/src/bids_validator/checks/fields.py @@ -0,0 +1,230 @@ +"""The ``fields`` payload: required and recommended JSON fields, and the values they hold. + +Three schema rule groups list the fields a JSON file should hold, as a ``fields`` +payload:: + + MRIFuncRepetitionTime: # rules.sidecars.mri.MRIFuncRepetitionTime + selectors: + - modality == "mri" + - datatype == "func" + - '!("VolumeTiming" in sidecar)' + fields: + RepetitionTime: + level: required + +* ``rules.sidecars`` describe the SIDECAR of a data file: the JSON fields that apply + to it after the inheritance principle has merged its JSON files + (``context.sidecar``). A missing field is ``SIDECAR_KEY_REQUIRED`` or + ``SIDECAR_KEY_RECOMMENDED``, reported on the data file. +* ``rules.json`` and ``rules.dataset_metadata`` describe one JSON file itself + (``context.json``), such as ``dataset_description.json``. A missing field is + ``JSON_KEY_REQUIRED`` or ``JSON_KEY_RECOMMENDED``. + +A field may carry its own ``issue`` (``B0_FIELD_IDENTIFIER_RECOMMENDED``), used instead +of those generic codes. A field that IS present has its value checked against its +definition in ``schema.objects.metadata`` (:mod:`.values`); a wrong value +is ``JSON_SCHEMA_VALIDATION_ERROR``, reported on the JSON file that holds it, once. + +This follows the reference validator's ``evalJsonCheck`` (``applyRules.ts``). The rule +engine (:mod:`.rules`) calls :func:`field_issues` for every rule with a ``fields`` +payload whose selectors hold for the file. +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +import re +from collections.abc import Mapping +from typing import TYPE_CHECKING, Any + +from ..issues import Issue, IssueDefinition, Severity +from .values import value_errors + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from ..context import Context + +# The names other modules are meant to import from this one. +__all__ = [ + 'JSON_KEY_RECOMMENDED', + 'JSON_KEY_REQUIRED', + 'SIDECAR_KEY_RECOMMENDED', + 'SIDECAR_KEY_REQUIRED', + 'field_issues', +] + +# --- what this module reports -------------------------------------------- +# +# Defined by the schema, and read from it where they are reported: +# +# JSON_SCHEMA_VALIDATION_ERROR rules.errors.JsonSchemaValidationError +# a field's own issue the ``issue`` of a field in a rule, such as +# B0_FIELD_IDENTIFIER_RECOMMENDED +# +# Defined only by the reference validator (bids-validator, src/issues/list.ts): + +# A data file's sidecar lacks a required field. +SIDECAR_KEY_REQUIRED = IssueDefinition( + 'SIDECAR_KEY_REQUIRED', + Severity.ERROR, + "A data file's JSON sidecar is missing a key listed as required.", +) +# A data file's sidecar lacks a recommended field. +SIDECAR_KEY_RECOMMENDED = IssueDefinition( + 'SIDECAR_KEY_RECOMMENDED', + Severity.WARNING, + "A data file's JSON sidecar is missing a key listed as recommended.", +) +# A JSON file (such as dataset_description.json) lacks a required field. +JSON_KEY_REQUIRED = IssueDefinition( + 'JSON_KEY_REQUIRED', Severity.ERROR, 'A JSON file is missing a key listed as required.' +) +# A JSON file lacks a recommended field. +JSON_KEY_RECOMMENDED = IssueDefinition( + 'JSON_KEY_RECOMMENDED', Severity.WARNING, 'A JSON file is missing a key listed as recommended.' +) + +# Extensions of files that cannot have a sidecar: a JSON file is one, and text files at +# the root (README, CHANGES, CITATION.cff ...) describe the dataset, not a recording. +_NO_SIDECAR_EXTENSIONS = frozenset({'.json', '', '.md', '.txt', '.rst', '.cff'}) + +# A requirement level, as a severity. Optional, prohibited and deprecated fields are +# never reported missing. +_LEVELS = {'required': Severity.ERROR, 'recommended': Severity.WARNING} + +# A level addendum, "required if `Key` is `Value`": a field whose level depends on the +# value of another field. Captures the new level, the key and the value. +_LEVEL_ADDENDUM = re.compile(r'(required|recommended) if `(\w+)` is `(\w+)`') + +# In a derivative dataset every field is optional, unless the rule says it is written for +# derivatives by carrying exactly this selector. +_DERIVATIVE_SELECTOR = 'dataset.dataset_description.DatasetType == "derivative"' + + +def field_issues(path: str, rule: Mapping[str, Any], context: Context) -> list[Issue]: + """Payload ``fields``: report missing fields and wrong values. + + Parameters + ---------- + path : str + Dotted schema path of the rule (``rules.sidecars.mri.MRIHardware``). + rule : Mapping + The rule; its selectors already hold for this file. + context : Context + Everything known about one file. + + Returns + ------- + list of Issue + The findings. + + """ + sidecar_rule = path.startswith('rules.sidecars') + if sidecar_rule and context.extension in _NO_SIDECAR_EXTENSIONS: + return [] + data: Mapping[str, Any] = (context.sidecar if sidecar_rule else context.json) or {} + schema = context.schema + metadata = schema['objects']['metadata'] + formats = schema['objects'].get('formats', {}) + derivative = context.dataset.dataset_description.get('DatasetType') == 'derivative' + written_for_derivatives = _DERIVATIVE_SELECTOR in rule.get('selectors', []) + + issues: list[Issue] = [] + for key, requirement in rule['fields'].items(): + definition = metadata[key] + name = str(definition['name']) + if name not in data: + severity = _missing_field_severity(requirement, context) + if severity is None or (derivative and not written_for_derivatives): + continue + issues.append( + _missing_field_issue(context, path, name, requirement, severity, sidecar_rule) + ) + continue + issues += _wrong_value_issues( + context, path, name, data[name], definition, formats, sidecar_rule + ) + return issues + + +def _missing_field_severity(requirement: Any, context: Context) -> Severity | None: + """Return the severity of a missing field, or None when it need not be reported. + + A requirement is a level (``"recommended"``) or ``{level: ..., level_addendum: + ...}``. An addendum of the form "required if `Key` is `Value`" raises the level + when the sidecar's ``Key`` is exactly the string ``Value``. + """ + if isinstance(requirement, str): + return _LEVELS.get(requirement) + if not isinstance(requirement, Mapping): + return None + severity = _LEVELS.get(str(requirement.get('level'))) + addendum = requirement.get('level_addendum') + if addendum: + found = _LEVEL_ADDENDUM.search(str(addendum)) + if found: + level, key, value = found.groups() + if context.sidecar.get(key) == value: + severity = _LEVELS[level] + return severity + + +def _missing_field_issue( + context: Context, + path: str, + name: str, + requirement: Any, + severity: Severity, + sidecar_rule: bool, +) -> Issue: + """Build the finding for a missing field: the field's own issue, or the generic code.""" + location = context.file.relative_path + own = requirement.get('issue') if isinstance(requirement, Mapping) else None + if isinstance(own, Mapping) and own.get('code') and own.get('message'): + definition = IssueDefinition.from_schema(own, severity=severity) + return definition.make_issue(location, sub_code=name, rule=path) + required = severity is Severity.ERROR + if sidecar_rule: + definition = SIDECAR_KEY_REQUIRED if required else SIDECAR_KEY_RECOMMENDED + else: + definition = JSON_KEY_REQUIRED if required else JSON_KEY_RECOMMENDED + level = 'required' if required else 'recommended' + return definition.make_issue( + location, sub_code=name, rule=path, message=f'the {level} field {name!r} is missing' + ) + + +def _wrong_value_issues( + context: Context, + path: str, + name: str, + value: Any, + definition: Mapping[str, Any], + formats: Mapping[str, Any], + sidecar_rule: bool, +) -> list[Issue]: + """Return the findings for a present value that breaks its definition. + + A sidecar value is reported on the JSON file it came from, and only the first time + any data file brings it up: a wrong value in a JSON at the dataset root is one + problem, not one per recording. The dataset remembers which values it has checked, + as the reference validator's does (``sidecarKeyValidated``). + """ + if sidecar_rule: + location = context.sidecar_origins.get(name, '') + address = f'{location}:{name}' + if address in context.dataset.checked_sidecar_values: + return [] + context.dataset.checked_sidecar_values.add(address) + else: + location = context.file.relative_path + problems = value_errors(value, definition, formats) + if not problems: + return [] + invalid = IssueDefinition.from_schema( + context.schema['rules']['errors']['JsonSchemaValidationError'] + ) + return [ + invalid.make_issue(location, sub_code=name, rule=path, message=f'{name} {problem}') + for problem in problems + ] diff --git a/src/bids_validator/checks/filename_rules.py b/src/bids_validator/checks/filename_rules.py new file mode 100644 index 0000000..b80f45a --- /dev/null +++ b/src/bids_validator/checks/filename_rules.py @@ -0,0 +1,315 @@ +"""The schema's filename rules, and which of them describe a file. + +The schema's ``rules.files`` lists every legal kind of filename. One rule, as the +schema writes it:: + + nonparametric: # rules.files.raw.anat.nonparametric + suffixes: [T1w, T2w, PDw, ...] + extensions: [.nii.gz, .nii, .json] + datatypes: [anat] + entities: + subject: required + session: optional + ... + +A file "matches" a rule when its name has one of the rule's suffixes, or when it sits +at the rule's fixed ``path`` (``dataset_description.json``), or when its name fits the +rule's ``stem`` (``participants``). Two kinds of check need to know which rules a file +matches, so the answer is worked out here, for both: + +* the filename checks (:mod:`.filenames`) hold the name to those rules: right + folder, right extension, right entities; +* the rule engine (:mod:`.rules`) shows the file to the schema's other rules + through :func:`hide_unused_name_parts`. + +This follows the reference validator's ``src/validators/filenameIdentify.ts``. +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +import fnmatch +from collections.abc import Mapping +from typing import TYPE_CHECKING, Any, cast + +import attrs + +from .schema_cache import once_per_schema + +# Imported only for type hints; never imported when the program runs, so ``context.py`` +# and this module cannot end up importing each other. +if TYPE_CHECKING: + from bidsschematools.types.namespace import Namespace + + from ..context import Context + +# The names other modules are meant to import from this one. +__all__ = [ + 'FilenameRule', + 'all_filename_rules', + 'hide_unused_name_parts', + 'matching_filename_rules', +] + + +# ``attrs.frozen`` makes a read-only data class from the attributes below. +# ``eq=False`` keeps Python's default comparison by identity: two rules are the same rule +# only if they are the same object, which also lets a rule be put in a ``set``. +@attrs.frozen(eq=False) +class FilenameRule: + """One ``rules.files`` rule, read out of the schema into plain values. + + A list is ``None`` when the rule does not have it at all, which is not the same + as an empty list: a rule without ``extensions`` allows any extension. + + Attributes + ---------- + path : str + Dotted schema path of the rule, such as ``rules.files.raw.anat.nonparametric``. + fixed_path : str or None + The one place a file of this kind may be, for a rule written with ``path`` + (``dataset_description.json``). + stem : str or None + A glob the name before its first dot must fit, for a rule written with + ``stem`` (``participants``, ``*`` for phenotype tables). + suffixes, extensions, datatypes : tuple of str or None + What the rule allows. + entities : tuple of str or None + The entities the rule allows, as SHORT names (``sub``, ``acq``), the form + filenames use. The schema lists them by long name (``subject``). + required_entities : tuple of str + The entities a file of this kind must have, as short names. + + """ + + path: str + fixed_path: str | None + stem: str | None + suffixes: tuple[str, ...] | None + extensions: tuple[str, ...] | None + datatypes: tuple[str, ...] | None + entities: tuple[str, ...] | None + required_entities: tuple[str, ...] + + @property + def for_derivatives(self) -> bool: + """Whether the rule applies only inside a derivative dataset.""" + return self.path.startswith('rules.files.deriv') + + +def matching_filename_rules(context: Context) -> tuple[FilenameRule, ...]: + """Return the ``rules.files`` rules that describe this file, best candidates only. + + Every rule the name matches is found first. When several match (``events.tsv`` is + an events file of many datatypes), the rule for the folder the file is in is + preferred, then the rules whose entities and extensions fit. + + Parameters + ---------- + context : Context + A file, or a directory recording (``sub-01_task-x_meg.ds/``), whose name is + checked like a file's. + + Returns + ------- + tuple of FilenameRule + In schema order. Empty when no rule matches: the file is not part of BIDS. + + """ + return _best(context, _matching(context)) + + +def hide_unused_name_parts(context: Context, rules: tuple[FilenameRule, ...]) -> Context: + """Return the context as the schema's other rules should see it. + + A part of the name that none of the file's ``rules`` uses is hidden: a file that + matched no rule at all has no entities, suffix or extension for the rules to read, + and a ``README`` has no entities. Without this, an unrecognised file such as + ``.bidsignore`` would be held to the sidecar rules of whatever its name happens to + resemble. The reference validator calls this ``cleanContext``. + + Parameters + ---------- + context : Context + The file. + rules : tuple of FilenameRule + What :func:`matching_filename_rules` returned for it. + + Returns + ------- + Context + The context itself when nothing is hidden. Otherwise a read-only view of it: + the hidden parts read as empty, everything else is the context's own, so + nothing is copied or loaded again. + + """ + hidden: dict[str, Any] = {} + if not any(rule.entities for rule in rules): + hidden['entities'] = {} + if not any(rule.extensions for rule in rules): + hidden['extension'] = '' + if not any(rule.suffixes for rule in rules): + hidden['suffix'] = '' + if not hidden: + return context + # ``cast`` only tells the type checker to treat the view as a Context (it behaves + # like one); it does nothing when the program runs. + return cast('Context', _WithHiddenParts(context, hidden)) + + +class _WithHiddenParts: + """A context with some name parts replaced by empty values.""" + + def __init__(self, context: Context, hidden: dict[str, Any]) -> None: + self._context = context + self._hidden = hidden + + # Python calls ``__getattr__`` when an attribute is not found on the object itself: + # ``view.suffix`` comes here, finds ``suffix`` hidden and returns ''; ``view.sidecar`` + # comes here and is passed on to the real context. + def __getattr__(self, name: str) -> Any: + if name in self._hidden: + return self._hidden[name] + return getattr(self._context, name) + + +# --- reading the rules out of the schema ---------------------------------- + + +@once_per_schema +def all_filename_rules(schema: Namespace) -> tuple[FilenameRule, ...]: + """Return every ``rules.files`` rule, in schema order.""" + short = {key: str(entity['name']) for key, entity in schema['objects']['entities'].items()} + rules: list[FilenameRule] = [] + files = schema['rules'].get('files', {}) + for group in files: + for path, node in _leaves(files[group], f'rules.files.{group}'): + entities = node.get('entities') + rules.append( + FilenameRule( + path=path, + fixed_path=_text(node.get('path')), + stem=_text(node.get('stem')), + suffixes=_texts(node.get('suffixes')), + extensions=_texts(node.get('extensions')), + datatypes=_texts(node.get('datatypes')), + entities=None + if entities is None + else tuple(short.get(k, k) for k in entities), + required_entities=tuple( + # A level written as a mapping ({level: ..., enum: ...}) is not + # counted as required, as in the reference (``v === 'required'``). + short.get(k, k) + for k, level in (entities or {}).items() + if level == 'required' + ), + ) + ) + return tuple(rules) + + +def _leaves(node: Any, path: str) -> list[tuple[str, Mapping[str, Any]]]: + """Return the rules below ``node``: the nodes with a ``path``, ``stem`` or ``suffixes``. + + Anything else is a grouping level (``raw``, ``anat``) to descend into. + """ + if not isinstance(node, Mapping): + return [] + keys = node.keys() + if 'path' in keys or 'stem' in keys or 'suffixes' in keys: + return [(path, node)] + return [leaf for key in keys for leaf in _leaves(node[key], f'{path}.{key}')] + + +def _text(value: Any) -> str | None: + """Return a schema value as text, keeping ``None`` for "not given".""" + return None if value is None else str(value) + + +def _texts(values: Any) -> tuple[str, ...] | None: + """Return a schema list as a tuple of text, keeping ``None`` for "not given".""" + return None if values is None else tuple(str(value) for value in values) + + +@attrs.frozen +class _Index: + """Where to look first: rule positions by fixed path and by suffix, and stem rules.""" + + by_fixed_path: dict[str, tuple[int, ...]] + by_suffix: dict[str, tuple[int, ...]] + with_stem: tuple[int, ...] + + +@once_per_schema +def _index(schema: Namespace) -> _Index: + """Index :func:`all_filename_rules`, so a file is compared with a few rules, not all.""" + by_fixed_path: dict[str, list[int]] = {} + by_suffix: dict[str, list[int]] = {} + with_stem: list[int] = [] + for position, rule in enumerate(all_filename_rules(schema)): + if rule.fixed_path is not None: + by_fixed_path.setdefault(rule.fixed_path, []).append(position) + if rule.stem is not None: + with_stem.append(position) + for suffix in rule.suffixes or (): + by_suffix.setdefault(suffix, []).append(position) + return _Index( + by_fixed_path={key: tuple(value) for key, value in by_fixed_path.items()}, + by_suffix={key: tuple(value) for key, value in by_suffix.items()}, + with_stem=tuple(with_stem), + ) + + +# --- matching ------------------------------------------------------------- + + +def _matching(context: Context) -> list[FilenameRule]: + """Return every rule the file's name matches, in schema order. + + A rule matches by its fixed path, by its stem glob, or by suffix. Matching by + suffix ignores the folder, as in the reference, which is why a file in the wrong + folder still finds its rule (and is then reported as in the wrong folder). + Derivative rules only apply inside a derivative dataset. + """ + schema = context.schema + rules = all_filename_rules(schema) + index = _index(schema) + positions = { + *index.by_fixed_path.get(context.path[1:], ()), # path starts with '/' + *index.by_suffix.get(context.suffix or '', ()), + *(position for position in index.with_stem if _fits_stem(rules[position], context)), + } + derivative = context.dataset.dataset_description.get('DatasetType') == 'derivative' + return [ + rules[position] + for position in sorted(positions) + if derivative or not rules[position].for_derivatives + ] + + +def _fits_stem(rule: FilenameRule, context: Context) -> bool: + """Whether the name before its first dot fits the rule's glob, in the rule's folder.""" + stem = context.file.name.split('.')[0] + if not fnmatch.fnmatchcase(stem, str(rule.stem)): + return False + return rule.datatypes is None or context.datatype in rule.datatypes + + +def _best(context: Context, rules: list[FilenameRule]) -> tuple[FilenameRule, ...]: + """Keep the best candidates: the rules for the file's folder, then those that fit.""" + if len(rules) > 1: + in_folder = [ + r for r in rules if r.datatypes is not None and context.datatype in r.datatypes + ] + rules = in_folder or rules + if len(rules) > 1: + fitting = [r for r in rules if _fits(r, context)] + rules = fitting or rules + return tuple(rules) + + +def _fits(rule: FilenameRule, context: Context) -> bool: + """Whether the rule allows the file's extension and every entity in its name.""" + if rule.extensions is not None and context.extension not in rule.extensions: + return False + return rule.entities is None or set(context.entities) <= set(rule.entities) diff --git a/src/bids_validator/checks/filenames.py b/src/bids_validator/checks/filenames.py new file mode 100644 index 0000000..726d71c --- /dev/null +++ b/src/bids_validator/checks/filenames.py @@ -0,0 +1,481 @@ +"""Names and places: whether a file has a legal BIDS name, in a legal place. + +Nothing here opens a file. The schema describes every legal filename in +``rules.files`` (which suffix goes in which datatype folder, which entities are +required or allowed, which extensions) and every legal folder in +``rules.directories`` (``sub-*``, ``ses-*``, a datatype, ``phenotype`` ...). + +* :func:`folder_issues`: a folder only has to match a folder rule. +* :func:`filename_issues`: the filename rules the file's name matches are found first + (:func:`.filename_rules.matching_filename_rules`), then the name is held to them. Each + kind of failure has its own code, rather than a single blanket "bad name". + +The checks follow the reference validator's ``filenameIdentify.ts``, +``filenameValidate.ts`` and ``filenameCase.ts``. +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +import re +from typing import TYPE_CHECKING + +from ..issues import Issue, IssueDefinition, Severity +from .filename_rules import FilenameRule, matching_filename_rules +from .schema_cache import once_per_schema + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from bidsschematools.types.namespace import Namespace + + from ..context import Context + +# The names other modules are meant to import from this one: the two checks +# (``checks.py`` runs them), the sidecar extensions, and the codes defined below. +__all__ = [ + 'ALL_FILENAME_RULES_HAVE_ISSUES', + 'CASE_COLLISION', + 'DATATYPE_MISMATCH', + 'ENTITY_NOT_IN_RULE', + 'ENTITY_WITH_NO_LABEL', + 'EXTENSION_MISMATCH', + 'FILENAME_MISMATCH', + 'INVALID_ENTITY_LABEL', + 'INVALID_LOCATION', + 'MISSING_DATATYPE', + 'MISSING_REQUIRED_ENTITY', + 'SIDECAR_EXTENSIONS', + 'filename_issues', + 'folder_issues', +] + +# --- what this module reports -------------------------------------------- +# +# NOT_INCLUDED is defined by the schema (rules.errors.NotIncluded) and read from it +# where it is reported. The other codes are defined only by the reference validator +# (bids-validator, src/issues/list.ts), so they are written out here, once each, with +# its severity and wording. A check reports one with ``CODE.make_issue(location, ...)``. + +# A name part with no label: ``acq-`` (nothing after the hyphen) or ``foo`` (no hyphen). +ENTITY_WITH_NO_LABEL = IssueDefinition( + 'ENTITY_WITH_NO_LABEL', Severity.ERROR, 'Found an entity with no label.' +) +# A label that breaks its entity's format: ``run-a`` (a run is a number). +INVALID_ENTITY_LABEL = IssueDefinition( + 'INVALID_ENTITY_LABEL', + Severity.ERROR, + "entity label doesn't match format found for files with this suffix", +) +# A required entity is missing: a ``_bold`` file with no ``task-``. +MISSING_REQUIRED_ENTITY = IssueDefinition( + 'MISSING_REQUIRED_ENTITY', + Severity.ERROR, + 'Missing required entity for files with this suffix.', +) +# An entity this kind of file may not have: ``dir-`` on a ``_T1w``. +ENTITY_NOT_IN_RULE = IssueDefinition( + 'ENTITY_NOT_IN_RULE', + Severity.ERROR, + 'Entity not listed as required or optional for files with this suffix', +) +# A data file not in any datatype folder: ``sub-01/sub-01_T1w.nii.gz``. +MISSING_DATATYPE = IssueDefinition( + 'MISSING_DATATYPE', Severity.ERROR, 'The file is not located in a datatype directory.' +) +# A data file in another datatype's folder: a ``_T1w`` in ``func/``. +DATATYPE_MISMATCH = IssueDefinition( + 'DATATYPE_MISMATCH', + Severity.ERROR, + 'The datatype directory does not match datatype of found suffix and extension', +) +# An extension this kind of file may not have: ``_T1w.txt``. +EXTENSION_MISMATCH = IssueDefinition( + 'EXTENSION_MISMATCH', + Severity.ERROR, + 'Extension used by file does not match allowed extensions for its suffix', +) +# A valid name under the wrong ``sub-``/``ses-`` folders. +INVALID_LOCATION = IssueDefinition( + 'INVALID_LOCATION', + Severity.ERROR, + 'The file has a valid name, but is located in an invalid directory.', +) +# Entities out of the schema's order, or written twice. +FILENAME_MISMATCH = IssueDefinition( + 'FILENAME_MISMATCH', + Severity.ERROR, + 'The filename is not formatted correctly. This could result from entity duplication ' + 'or reordering.', +) +# Several filename rules match the name, and every one has a problem. +ALL_FILENAME_RULES_HAVE_ISSUES = IssueDefinition( + 'ALL_FILENAME_RULES_HAVE_ISSUES', + Severity.ERROR, + 'Multiple filename rules were found as potential matches. All of them had at least ' + 'one issue during filename validation.', +) +# Two files in one folder whose names differ only by letter case. +CASE_COLLISION = IssueDefinition( + 'CASE_COLLISION', + Severity.ERROR, + 'Files with the same name but different casing have been found.', +) + +# Extensions of files that describe data rather than hold it. They may sit above the +# data they describe (the inheritance principle), so the reference validator never +# asks them to be in a datatype folder (``sidecarExtensions`` in filenameValidate.ts). +SIDECAR_EXTENSIONS = frozenset({'.json', '.tsv', '.bvec', '.bval'}) + + +def folder_issues(context: Context) -> list[Issue]: + """Report a folder whose name matches no ``rules.directories`` rule. + + A rule matches by exact ``name`` (``phenotype``, ``docs``), by ``entity`` (a name + starting with ``sub-`` or ``ses-``) or by ``value`` (any datatype: ``anat``, + ``func`` ...), as in the reference's ``findDirRuleMatches``. Like the reference, + this does not check how deep the folder sits. + """ + schema = context.schema + name = context.file.name + kind = str(context.dataset.dataset_description.get('DatasetType', 'raw')) + rules = schema['rules']['directories'].get(kind, {}) + for key in rules: + rule = rules[key] + if rule.get('name') == name: + return [] + entity = rule.get('entity') + if entity and name.startswith(f'{schema["objects"]["entities"][entity]["name"]}-'): + return [] + value = rule.get('value') + if value and name in schema['objects'].get(f'{value}s', {}).keys(): # noqa: SIM118 + return [] + return [_not_included(context)] + + +def filename_issues(context: Context) -> list[Issue]: + """Return every name-and-place finding for one file. + + A directory recording (``sub-01_task-x_meg.ds/``) is checked as a file. Its + extension carries a trailing slash (``.ds/``), which is exactly how the schema + spells those extensions, so the ordinary rules apply to the folder's name. + + Letter case is checked whether or not the name is recognised: both spellings of a + colliding pair are reported, as in the reference's separate ``filenameCase`` step. + """ + return _name_issues(context, matching_filename_rules(context)) + _case_collision(context) + + +def _name_issues(context: Context, rules: tuple[FilenameRule, ...]) -> list[Issue]: + """Hold the name to the rules that describe it.""" + if not rules: + if context.file.relative_path == '.bidsignore': + return [] # visited, but it is the ignore list, not part of the dataset + return [_not_included(context)] + + entities = _entities(context, rules) + issues: list[Issue] = [] + issues += _labels_missing(context, rules, entities) + issues += _labels_malformed(context, entities) + issues += _issues_against_rules(context, rules, entities) + issues += _name_out_of_order(context, entities) + return issues + + +def _not_included(context: Context) -> Issue: + """Report a file or folder that is not part of BIDS, in the schema's words.""" + definition = IssueDefinition.from_schema(context.schema['rules']['errors']['NotIncluded']) + return definition.make_issue(context.file.relative_path) + + +# --- the entities in the name --------------------------------------------- + + +def _entities(context: Context, rules: tuple[FilenameRule, ...]) -> dict[str, str]: + """Return the entities to check, as the reference validator sees them. + + If no rule uses entities (``dataset_description.json``, ``README``), the file has + none to check: the ``dataset`` in ``dataset_description`` is not an entity. + Otherwise every key-label part of the name is one, and a part with no hyphen + (``foo`` in ``sub-01_foo_T1w``) is an entity with no label, kept as ``''``. + """ + if all(rule.entities is None for rule in rules): + return {} + return {key: value or '' for key, value in context.entities.items()} + + +def _labels_missing( + context: Context, rules: tuple[FilenameRule, ...], entities: dict[str, str] +) -> list[Issue]: + """``ENTITY_WITH_NO_LABEL``: ``acq-`` (empty), or ``foo`` (no hyphen at all).""" + if all(rule.suffixes is None for rule in rules): + return [] + empty = [key for key, label in entities.items() if not label] + if not empty: + return [] + return [ + ENTITY_WITH_NO_LABEL.make_issue( + context.file.relative_path, + sub_code=', '.join(empty), + message=f'entities with no label: {", ".join(empty)}', + ) + ] + + +def _labels_malformed(context: Context, entities: dict[str, str]) -> list[Issue]: + """``INVALID_ENTITY_LABEL``: a label that breaks its entity's format (``run-a``).""" + patterns = _label_patterns(context.schema) + issues: list[Issue] = [] + for short, label in entities.items(): + pattern = patterns.get(short) + if label and pattern and not re.fullmatch(pattern, label): + issues.append( + INVALID_ENTITY_LABEL.make_issue( + context.file.relative_path, + sub_code=short, + message=f'label {label!r} for entity {short!r} does not match /{pattern}/', + ) + ) + return issues + + +def _name_out_of_order(context: Context, entities: dict[str, str]) -> list[Issue]: + """``FILENAME_MISMATCH``: entities duplicated, or not in the schema's order.""" + if not entities: + return [] + order = [short for short in _entity_order(context.schema) if short in entities] + parts = [f'{short}-{entities[short]}' for short in order] + # A directory recording's extension carries a trailing slash (``.ds/``) because that + # is how the schema spells it, but the folder on disk is named without one. Drop it + # so the rebuilt name is comparable, and readable in the message. + extension = (context.extension or '').rstrip('/') + expected = '_'.join([*parts, (context.suffix or '') + extension]) + if context.file.name == expected: + return [] + return [ + FILENAME_MISMATCH.make_issue( + context.file.relative_path, message=f'expected filename: {expected}' + ) + ] + + +# --- the rules the name matched ------------------------------------------- + + +def _issues_against_rules( + context: Context, rules: tuple[FilenameRule, ...], entities: dict[str, str] +) -> list[Issue]: + """Hold the file to its rule; with several candidates, to any one of them. + + With one candidate, its problems are reported directly. With several, the file is + accepted if any candidate is satisfied; only when every candidate has a problem + is ``ALL_FILENAME_RULES_HAVE_ISSUES`` reported. + """ + if len(rules) == 1: + return _issues_against_rule(context, rules[0], entities) + if any(not _issues_against_rule(context, rule, entities) for rule in rules): + return [] + return [ + ALL_FILENAME_RULES_HAVE_ISSUES.make_issue( + context.file.relative_path, + message='rules that matched with issues: ' + ', '.join(r.path for r in rules), + ) + ] + + +def _issues_against_rule( + context: Context, rule: FilenameRule, entities: dict[str, str] +) -> list[Issue]: + """Entities, datatype folder, extension and place, against one rule.""" + issues: list[Issue] = [] + issues += _entities_against_rule(context, rule, entities) + issues += _datatype_against_rule(context, rule) + issues += _extension_against_rule(context, rule) + issues += _location_issues(context, entities) + return issues + + +def _entities_against_rule( + context: Context, rule: FilenameRule, entities: dict[str, str] +) -> list[Issue]: + """``MISSING_REQUIRED_ENTITY`` and ``ENTITY_NOT_IN_RULE``: too few, or too many.""" + if rule.entities is None: + return [] + location = context.file.relative_path + issues: list[Issue] = [] + # A file at the dataset root is a shared sidecar inherited downward, so it need not + # carry the required entities. This mirrors the reference. + missing = [entity for entity in rule.required_entities if entity not in entities] + if missing and '/' in location: + issues.append( + MISSING_REQUIRED_ENTITY.make_issue( + location, + sub_code=', '.join(missing), + message=f'missing required entities: {", ".join(missing)}', + rule=rule.path, + ) + ) + extra = [entity for entity in entities if entity not in rule.entities] + if extra: + issues.append( + ENTITY_NOT_IN_RULE.make_issue( + location, + sub_code=', '.join(extra), + message=f'entities not allowed for this file type: {", ".join(extra)}', + rule=rule.path, + ) + ) + return issues + + +def _datatype_against_rule(context: Context, rule: FilenameRule) -> list[Issue]: + """``MISSING_DATATYPE`` and ``DATATYPE_MISMATCH``: not in a datatype folder, or the wrong one. + + Only for rules that name datatypes, and never for :data:`SIDECAR_EXTENSIONS`: + a ``.json``, ``.tsv``, ``.bvec`` or ``.bval`` may sit above the data it describes. + A file in a folder that is not a datatype at all (``sub-01/foo/``) is reported + twice over: the folder as ``NOT_INCLUDED``, the file as ``MISSING_DATATYPE``. + """ + if rule.datatypes is None or context.extension in SIDECAR_EXTENSIONS: + return [] + allowed = ', '.join(rule.datatypes) + if not context.datatype: + return [ + MISSING_DATATYPE.make_issue( + context.file.relative_path, + message=f'the file is not in a datatype folder; expected one of: {allowed}', + rule=rule.path, + ) + ] + if context.datatype not in rule.datatypes: + return [ + DATATYPE_MISMATCH.make_issue( + context.file.relative_path, + message=f"the file is in '{context.datatype}'; its suffix belongs in: {allowed}", + rule=rule.path, + ) + ] + return [] + + +def _extension_against_rule(context: Context, rule: FilenameRule) -> list[Issue]: + """``EXTENSION_MISMATCH``: an extension this kind of file may not have.""" + if rule.extensions is None or context.extension in rule.extensions: + return [] + return [ + EXTENSION_MISMATCH.make_issue( + context.file.relative_path, + message=( + f'extension {context.extension!r} is not allowed here; ' + f'allowed: {", ".join(rule.extensions)}' + ), + rule=rule.path, + ) + ] + + +def _location_issues(context: Context, entities: dict[str, str]) -> list[Issue]: + """``INVALID_LOCATION``: a valid name in the wrong folder. + + Two folder hierarchies are checked: ``sub-``/``ses-`` (raw data) and + ``tpl-``/``cohort-`` (templates), each unless the name belongs to the other. + """ + issues: list[Issue] = [] + if 'tpl' not in entities: + issues += _location_issues_in(context, entities, 'sub', 'ses') + if 'sub' not in entities: + issues += _location_issues_in(context, entities, 'tpl', 'cohort') + return issues + + +def _location_issues_in( + context: Context, entities: dict[str, str], top: str, inner: str +) -> list[Issue]: + """Check one hierarchy: ``top`` is ``sub`` or ``tpl``, ``inner`` is ``ses`` or ``cohort``. + + The file must be under the folders its own entities name, and must not be in such + a folder without the matching entity in its name. + """ + path = context.path + top_label = entities.get(top) + inner_label = entities.get(inner) + problems: list[str] = [] + if top_label: + expected = f'/{top}-{top_label}/' + if inner_label: + expected += f'{inner}-{inner_label}/' + if not path.startswith(expected): + problems.append(f'expected to be under {expected}') + if not top_label and re.match(rf'^/{top}-', path): + problems.append(f"in a '{top}-' folder but no '{top}' in the name") + if not inner_label and re.search(rf'/{inner}-', path): + problems.append(f"in a '{inner}-' folder but no '{inner}' in the name") + return [ + INVALID_LOCATION.make_issue( + context.file.relative_path, + message=f'the file has a valid name but is in the wrong place ({problem})', + ) + for problem in problems + ] + + +# --- letter case ---------------------------------------------------------- + + +def _case_collision(context: Context) -> list[Issue]: + """``CASE_COLLISION``: other files in the folder whose names differ only by case. + + ``sub-01_T1w.nii.gz`` and ``sub-01_t1w.nii.gz`` are two files on Linux and one on + a default macOS or Windows disk, so a dataset holding both cannot be copied + safely. Only files are compared, as in the reference's ``filenameCase.ts``. + """ + parent = context.file.parent + if parent is None or context.file.is_dir: + return [] + lowered = context.file.name.lower() + others = [ + node.relative_path + for name, node in parent.children.items() + if not node.is_dir and name != context.file.name and name.lower() == lowered + ] + if not others: + return [] + return [ + CASE_COLLISION.make_issue( + context.file.relative_path, + message='same name, different case: ' + ', '.join(others), + ) + ] + + +# --- what the schema says about entities ---------------------------------- + + +@once_per_schema +def _label_patterns(schema: Namespace) -> dict[str, str]: + """Map each entity's short name (``run``) to the pattern its label must match. + + The schema gives each entity a ``format`` (``index``, ``label``), and each format + a regular expression in ``objects.formats``. + """ + formats = schema['objects'].get('formats', {}) + patterns: dict[str, str] = {} + for entity in schema['objects']['entities'].values(): + name, fmt = entity.get('name'), entity.get('format') + if name and fmt and str(fmt) in formats.keys(): # noqa: SIM118 + pattern = str(formats[str(fmt)].get('pattern', '')) + if pattern: + patterns[str(name)] = pattern + return patterns + + +@once_per_schema +def _entity_order(schema: Namespace) -> list[str]: + """Entity short names in the order the schema says they appear in a filename.""" + entities = schema['objects']['entities'] + order: list[str] = [] + for long_name in schema['rules'].get('entities', []): + if long_name in entities.keys(): # noqa: SIM118 + name = entities[long_name].get('name') + if name: + order.append(str(name)) + return order diff --git a/src/bids_validator/checks/inheritance.py b/src/bids_validator/checks/inheritance.py new file mode 100644 index 0000000..d3e3608 --- /dev/null +++ b/src/bids_validator/checks/inheritance.py @@ -0,0 +1,87 @@ +"""Inherited metadata: whether a deeper JSON file changes a value set above it. + +BIDS lets one JSON file describe many recordings: ``task-rest_bold.json`` at the +dataset root applies to every ``*_task-rest_bold.nii.gz`` below it, and a JSON file +closer to a recording can add to it or change it (the inheritance principle). The +nearest value is the one that applies. Changing a value that a file above already set +is allowed, but it is usually a mistake, so the reference validator warns about it +(``SIDECAR_FIELD_OVERRIDE``, from ``loadSidecar`` in ``src/schema/context.ts``). + +The chain of JSON files a recording inherits from is found once per file, by +:attr:`.context.Context.sidecar_files`. +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from ..context import ValidationError, load_json +from ..issues import Issue, IssueDefinition, Severity +from .values import json_equal + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from ..context import Context + +# The names other modules are meant to import from this one. +__all__ = ['SIDECAR_FIELD_OVERRIDE', 'inheritance_issues'] + +# --- what this module reports -------------------------------------------- +# +# Not defined by the schema; the reference validator's definition (src/issues/list.ts). + +# A deeper JSON file changes a value a JSON file above it set. +SIDECAR_FIELD_OVERRIDE = IssueDefinition( + 'SIDECAR_FIELD_OVERRIDE', + Severity.WARNING, + 'Sidecar files should not override values assigned at a higher level.', +) + + +def inheritance_issues(context: Context) -> list[Issue]: + """Warn where a deeper JSON file changes a value set by a JSON file above it. + + Reported on the deeper file, once per data file that inherits both. Values are + compared by content, so the same list written twice is not an override. + + Parameters + ---------- + context : Context + A file. A ``.json`` file has no chain: it is a sidecar, not something that has + one. + + Returns + ------- + list of Issue + One ``SIDECAR_FIELD_OVERRIDE`` per changed key. + + """ + try: + chain = context.sidecar_files + except ValidationError: + return [] # ambiguous: reported by the integrity checks + applied: dict[str, Any] = {} # key -> the value that applies (the nearest) + origin: dict[str, str] = {} # key -> the file that value comes from + issues: list[Issue] = [] + for json in chain: # nearest first + try: + contents = load_json(json) + except ValidationError: + continue # a broken JSON file is reported by the integrity checks + for key, value in contents.items(): + if key not in applied: + applied[key] = value + origin[key] = json.relative_path + elif not json_equal(applied[key], value): + issues.append( + SIDECAR_FIELD_OVERRIDE.make_issue( + origin[key], + sub_code=key, + message=( + f'{key} = {applied[key]!r} overrides {value!r} ' + f'from {json.relative_path}' + ), + ) + ) + return issues diff --git a/src/bids_validator/checks/integrity.py b/src/bids_validator/checks/integrity.py new file mode 100644 index 0000000..e774f65 --- /dev/null +++ b/src/bids_validator/checks/integrity.py @@ -0,0 +1,199 @@ +"""Whether each file can be read at all. + +These are the checks the schema does not state as rules, because they come before any +rule can be read: a file that is empty, a JSON file that does not parse, a NIfTI +header that cannot be read, a table whose rows have the wrong number of cells, a +dataset with no ``dataset_description.json``. The reference validator raises them +from its file loaders (``src/files/*.ts``) and its ``emptyFile`` check. + +The loaders are the context's own (:mod:`bids_validator.context`), so every file is +read once: these checks call the loader, and turn the error it raises into a +finding. When a load fails, the context gives the schema rules an empty value +instead, so a broken file is reported here once rather than as a failed check in +every rule that reads it. +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +from collections.abc import Callable +from typing import TYPE_CHECKING + +from ..context import ValidationError, load_json +from ..issues import Issue, IssueDefinition, Severity + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from bidsschematools.types.namespace import Namespace + + from ..context import Context, Dataset + from ..types.files import FileTree + +# The names other modules are meant to import from this one. +__all__ = [ + 'INVALID_GZIP', + 'JSON_NOT_AN_OBJECT', + 'LOADER_ERRORS', + 'LOADER_ERRORS_IN_SCHEMA', + 'MISSING_DATASET_DESCRIPTION', + 'MULTIPLE_INHERITABLE_FILES', + 'TSV_COLUMN_HEADER_DUPLICATE', + 'TSV_EMPTY_LINE', + 'TSV_EQUAL_ROWS', + 'dataset_integrity_issues', + 'integrity_issues', +] + +# --- what this module reports -------------------------------------------- +# +# Defined by the schema, in rules.errors, and read from it where they are reported: +# +# EMPTY_FILE rules.errors.EmptyFile +# NIFTI_HEADER_UNREADABLE rules.errors.NiftiHeaderUnreadable +# JSON_INVALID rules.errors.JsonInvalid (raised by a loader) +# INVALID_JSON_ENCODING rules.errors.InvalidJsonEncoding (raised by a loader) +# +# Defined only by the reference validator (bids-validator, src/issues/list.ts): + +# No ``dataset_description.json`` at the root of the dataset. +MISSING_DATASET_DESCRIPTION = IssueDefinition( + 'MISSING_DATASET_DESCRIPTION', + Severity.ERROR, + 'A dataset_description.json file is required in the root of the dataset', +) + +# The context's loaders (``load_json``, ``load_tsv`` ... in context.py) raise these, as +# the ``code`` of a ``ValidationError``; ``_issues_from_loader`` turns them into findings: +# A JSON file that parses, but holds a list or a number instead of ``{...}``. +JSON_NOT_AN_OBJECT = IssueDefinition( + 'JSON_NOT_AN_OBJECT', Severity.ERROR, 'Parsed JSON file does not contain an object.' +) +# An empty line in a table that is not the last line. +TSV_EMPTY_LINE = IssueDefinition( + 'TSV_EMPTY_LINE', Severity.ERROR, 'An empty line was found in the TSV file.' +) +# A table row with more or fewer cells than the header has columns. +TSV_EQUAL_ROWS = IssueDefinition( + 'TSV_EQUAL_ROWS', + Severity.ERROR, + 'All rows must have the same number of columns as there are headers.', +) +# Two columns of one table with the same name. +TSV_COLUMN_HEADER_DUPLICATE = IssueDefinition( + 'TSV_COLUMN_HEADER_DUPLICATE', + Severity.ERROR, + 'Two elements in the first row of a TSV are the same. Each column header must be unique.', +) +# A ``.tsv.gz`` that does not decompress. +INVALID_GZIP = IssueDefinition( + 'INVALID_GZIP', + Severity.ERROR, + 'The gzip file could not be decompressed. It may be corrupt or misnamed.', +) +# Two JSON files at one level that apply equally well to a file. +MULTIPLE_INHERITABLE_FILES = IssueDefinition( + 'MULTIPLE_INHERITABLE_FILES', + Severity.ERROR, + 'Multiple files in a directory were found to be valid candidates for inheritance.', +) + +# Every code a loader can raise, and where its definition is. Two are defined by the +# schema, under these entry names in rules.errors; the others are defined above. A test +# (tests/checks/test_codes.py) checks that every code raised in context.py is in one of the two. +LOADER_ERRORS_IN_SCHEMA = { + 'JSON_INVALID': 'JsonInvalid', + 'INVALID_JSON_ENCODING': 'InvalidJsonEncoding', +} +LOADER_ERRORS = { + definition.code: definition + for definition in ( + JSON_NOT_AN_OBJECT, + TSV_EMPTY_LINE, + TSV_EQUAL_ROWS, + TSV_COLUMN_HEADER_DUPLICATE, + INVALID_GZIP, + MULTIPLE_INHERITABLE_FILES, + ) +} + + +def integrity_issues(context: Context) -> list[Issue]: + """Return the findings for a file that cannot be read the way its type needs. + + An empty image is reported twice (empty, and no header), as the reference + validator does. A file that inherits from two equally good sidecars is reported + as ``MULTIPLE_INHERITABLE_FILES``. + """ + issues: list[Issue] = [] + if _size(context.file) == 0: + issues.append( + _schema_error(context.schema, 'EmptyFile').make_issue(context.file.relative_path) + ) + extension = context.extension + if extension == '.json': + issues += _issues_from_loader(context, lambda: load_json(context.file)) + elif extension in ('.nii', '.nii.gz') and context.nifti_header is None: + definition = _schema_error(context.schema, 'NiftiHeaderUnreadable') + issues.append(definition.make_issue(context.file.relative_path)) + elif extension in ('.tsv', '.tsv.gz'): + issues += _issues_from_loader(context, context.read_columns) + # Finding the JSON files it inherits from raises if one level is ambiguous. + issues += _issues_from_loader(context, lambda: context.sidecar_files) + return issues + + +def dataset_integrity_issues(dataset: Dataset) -> list[Issue]: + """Return the findings about the dataset as a whole: a missing description.""" + if 'dataset_description.json' in dataset.tree.children: + return [] + return [ + MISSING_DATASET_DESCRIPTION.make_issue( + None, message='there is no dataset_description.json at the root of the dataset' + ) + ] + + +def _issues_from_loader(context: Context, load: Callable[[], object]) -> list[Issue]: + """Run a loader; turn the :class:`ValidationError` it raises into a finding. + + Only coded errors are findings. A file that is not valid UTF-8 text, for example, + raises ``UnicodeDecodeError``, which the reference validator does not report + either; the rules that read the file then see an empty value. + """ + try: + load() + except ValidationError as error: + message = error.message + if error.line is not None: + message = f'line {error.line}' + (f': {message}' if message else '') + definition = _definition_of_loader_code(context.schema, error.code) + return [ + definition.make_issue(error.location or context.file.relative_path, message=message) + ] + except (OSError, UnicodeDecodeError): + return [] + return [] + + +def _definition_of_loader_code(schema: Namespace, code: str) -> IssueDefinition: + """Return the definition of a code a loader raised, from the schema or from here.""" + if code in LOADER_ERRORS_IN_SCHEMA: + return _schema_error(schema, LOADER_ERRORS_IN_SCHEMA[code]) + return LOADER_ERRORS[code] + + +def _schema_error(schema: Namespace, name: str) -> IssueDefinition: + """Return the definition the schema gives under ``rules.errors.``.""" + return IssueDefinition.from_schema(schema['rules']['errors'][name]) + + +def _size(node: FileTree) -> int: + """Return the size of a file, or of a directory recording: the sum of its files. + + A recording folder whose files are all empty is an empty file, as in the reference. + Hidden files (``.zattrs`` in an OME-Zarr folder) do not count: the walk never + visits them. + """ + if not node.is_dir: + return node.path_obj.stat().st_size + return sum(_size(child) for child in node.children.values() if not child.name.startswith('.')) diff --git a/src/bids_validator/checks/rules.py b/src/bids_validator/checks/rules.py new file mode 100644 index 0000000..4ea6ffb --- /dev/null +++ b/src/bids_validator/checks/rules.py @@ -0,0 +1,331 @@ +"""The rule engine: which of the schema's rules apply to a file, and who checks them. + +Everything the BIDS schema says about a file's content is written as a RULE WITH +SELECTORS. The selectors say WHEN the rule applies; the rest of the rule, its payload, +says WHAT must then be true. This module reports nothing itself. For one file it finds +every rule whose selectors all hold, and hands each payload of that rule to the module +that knows how to check it: + +==================== ============================================== ==================== +payload where in the schema checked by +==================== ============================================== ==================== +``checks`` ``rules.checks`` :mod:`.expressions` +``fields`` ``rules.sidecars``, ``rules.json``, :mod:`.fields` + ``rules.dataset_metadata`` +``columns`` (and ``rules.tabular_data`` :mod:`.tables` +``initial_columns``, +``index_columns``) +==================== ============================================== ==================== + +That is the reference validator's ``applyRules.ts``: one walk over ``schema.rules``, one +selector step, a table from payload name to handler. + +Which rules run +--------------- +A rule only runs when everything it reads is something this package provides. +:data:`NOT_ENABLED_YET` lists the parts of the context that are not built yet. A rule +that reads any of them is skipped, because evaluating it against a missing value +would report a problem that is not there: "a task scan needs an events file" would +fire on every task scan if the link to the events file is never made. +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +import logging +from collections.abc import Callable, Iterator, Mapping +from typing import TYPE_CHECKING, Any + +import attrs +from bidsschematools import expressions as bst_expr + +from ..expression_language import parse, truthy +from ..issues import Issue +from .expressions import check_issues, evaluate_or_null +from .fields import field_issues +from .filename_rules import hide_unused_name_parts, matching_filename_rules +from .schema_cache import once_per_schema +from .tables import column_issues, index_column_issues, initial_column_issues + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from bidsschematools.types.namespace import Namespace + + from ..context import Context + +# The names other modules are meant to import from this one. +__all__ = [ + 'NOT_ENABLED_YET', + 'PAYLOAD_HANDLERS', + 'PreparedRule', + 'apply_rule', + 'names_read_by', + 'rule_issues', + 'runnable_rules', +] + +# Where this module writes its debug messages (a rule that cannot be parsed). +logger = logging.getLogger(__name__) + +# Context names a rule may NOT read yet. A rule reading any of them, or anything below +# them (``associations.events`` is below ``associations``), is skipped. +NOT_ENABLED_YET: tuple[str, ...] = ( + 'associations', # files that belong with this one (events, bval ...): issue #82 + 'ome', # OME-XML metadata of microscopy files: issue #52 + 'tiff', # TIFF header of microscopy files: issue #53 +) + +# Two type aliases: names for types written out in full below, so a signature can say +# ``-> list[Rule]`` instead of ``-> list[tuple[str, Mapping[str, Any]]]``. +# +# A rule as found in the schema: its dotted path and the rule itself. +Rule = tuple[str, Mapping[str, Any]] +# A payload handler: takes (rule path, rule, file context), returns the findings. +PayloadHandler = Callable[[str, Mapping[str, Any], 'Context'], list[Issue]] + +# Payload name -> the function that checks it, in the reference validator's order +# (``evalMap`` in applyRules.ts). A rule with several payloads runs them in its own order. +PAYLOAD_HANDLERS: dict[str, PayloadHandler] = { + 'checks': check_issues, + 'columns': column_issues, + 'initial_columns': initial_column_issues, + 'index_columns': index_column_issues, + 'fields': field_issues, +} + + +def rule_issues(context: Context) -> list[Issue]: + """Return every finding of every runnable schema rule, for one file. + + The rules see the file as its name identifies it + (:func:`~bids_validator.checks.filename_rules.hide_unused_name_parts`): a file that matched + no filename rule has no suffix or extension for them to read. + + Parameters + ---------- + context : Context + Everything known about one file. + + Returns + ------- + list of Issue + The findings, in schema order. + + """ + context = hide_unused_name_parts(context, matching_filename_rules(context)) + issues: list[Issue] = [] + for rule in _prepared_rules(context.schema): + issues += _issues_from_rule(rule, context) + return issues + + +def apply_rule(path: str, rule: Mapping[str, Any], context: Context) -> list[Issue]: + """Apply one rule to one file: check its selectors, then run each payload. + + Parameters + ---------- + path : str + Dotted schema path of the rule, for example + ``rules.checks.general.ReadmeFileSmall``. Recorded on every finding. + rule : Mapping + The rule itself: ``selectors`` and one or more payloads. + context : Context + Everything known about one file. + + Returns + ------- + list of Issue + Nothing when a selector is not true; otherwise whatever the payloads report. + + """ + return _issues_from_rule(_prepare(path, rule), context) + + +# --- applying a rule ------------------------------------------------------ + + +# A read-only data class (see issues.py for what attrs does). +@attrs.frozen +class PreparedRule: + """A rule read out of the schema once, for the loop that runs it on every file. + + The schema's ``Namespace`` is slow to read; a dataset asks every rule's selectors + about every file, so they are copied out into plain tuples first. + + Attributes + ---------- + path : str + Dotted schema path of the rule. + rule : Mapping + The rule itself, handed to the payload handlers. + selectors : tuple of str + The expressions that must all hold for the rule to apply. + handlers : tuple of PayloadHandler + One per payload of the rule, in the rule's own order, as in the reference. + + """ + + path: str + rule: Mapping[str, Any] + selectors: tuple[str, ...] + handlers: tuple[PayloadHandler, ...] + + +def _prepare(path: str, rule: Mapping[str, Any]) -> PreparedRule: + """Copy a rule's selectors and payload handlers out of the schema.""" + return PreparedRule( + path=path, + rule=rule, + selectors=tuple(str(selector) for selector in rule.get('selectors', [])), + handlers=tuple( + PAYLOAD_HANDLERS[key] + for key in rule.keys() # noqa: SIM118 - see _rules_with_selectors + if key in PAYLOAD_HANDLERS + ), + ) + + +@once_per_schema +def _prepared_rules(schema: Namespace) -> list[PreparedRule]: + """Return the :func:`runnable_rules` that have a payload, prepared. + + ``rules.files`` and ``rules.errors`` entries carry selectors but no payload, so they + can never report anything here; evaluating their selectors on every file would + only cost time. + """ + prepared = (_prepare(path, rule) for path, rule in runnable_rules(schema)) + return [rule for rule in prepared if rule.handlers] + + +def _issues_from_rule(rule: PreparedRule, context: Context) -> list[Issue]: + """Check a rule's selectors, then run each of its payloads.""" + # all() stops at the first false selector, so later selectors, which may assume + # what earlier ones established, are not evaluated for files they do not suit. + if not all(truthy(evaluate_or_null(s, context, rule.path)) for s in rule.selectors): + return [] + issues: list[Issue] = [] + for handler in rule.handlers: + issues += handler(rule.path, rule.rule, context) + return issues + + +# --- which rules run ------------------------------------------------------ + + +@once_per_schema +def runnable_rules(schema: Namespace) -> list[Rule]: + """Return every rule with selectors that reads nothing in :data:`NOT_ENABLED_YET`. + + A rule whose expressions cannot be parsed is left out too, since it could only + ever report a problem with the schema, not the dataset. + + Parameters + ---------- + schema : Namespace + The BIDS schema. + + Returns + ------- + list of (str, Mapping) + ``(dotted rule path, rule)`` pairs, in schema order. + + """ + runnable: list[Rule] = [] + for path, rule in _rules_with_selectors(schema['rules'], 'rules'): + try: + names = names_read_by(rule) + except Exception as error: # noqa: BLE001 - pyparsing raises its own types + logger.debug('Skipping %s: cannot parse it (%s)', path, error) + continue + if not any(_is_inside(name, held) for name in names for held in NOT_ENABLED_YET): + runnable.append((path, rule)) + return runnable + + +def names_read_by(rule: Mapping[str, Any]) -> set[str]: + """Return every context name a rule reads, as dotted paths. + + ``exists(substr(path, 0, length(path) - 3), "dataset")`` reads ``{'path'}``; + ``"micr" in dataset.modalities`` reads ``{'dataset.modalities'}``; + ``nifti_header.dim[0] == 4`` reads ``{'nifti_header.dim'}``. Function names + and string literals are not reads. + + Parameters + ---------- + rule : Mapping + A rule with ``selectors`` and/or ``checks``. + + Returns + ------- + set of str + The dotted names. + + """ + names: set[str] = set() + for expression in [*rule.get('selectors', []), *rule.get('checks', [])]: + names.update(_names_in(parse(expression))) + return names + + +def _rules_with_selectors(node: Any, path: str) -> Iterator[Rule]: + """Yield ``(dotted path, rule)`` for every node with ``selectors`` below ``node``. + + In practice that is ``rules.checks``, ``rules.sidecars``, ``rules.json``, + ``rules.dataset_metadata`` and ``rules.tabular_data`` (``rules.errors`` has + selectors but no payload). Walking to any node with selectors keeps a new group + in a future schema working. ``.keys()`` is used on purpose: a bidsschematools + Namespace reads a dotted key as a path and can insert into the schema. + """ + if not isinstance(node, Mapping): + return + if 'selectors' in node.keys(): # noqa: SIM118 + yield path, node + return + for key in node: + yield from _rules_with_selectors(node[key], f'{path}.{key}') + + +def _names_in(node: Any) -> Iterator[str]: + """Yield the dotted names read anywhere in a parsed expression.""" + name = _dotted_name(node) + if name is not None: + yield name + return + # ``match``/``case`` picks the branch whose pattern fits the node's type, and binds + # its fields: ``case bst_expr.BinOp(lh=lh, rh=rh)`` runs for a binary operation and + # names its two sides ``lh`` and ``rh``. + match node: + case bst_expr.Property(name=base): + yield from _names_in(base) + case bst_expr.Element(name=base, index=index): + yield from _names_in(base) + yield from _names_in(index) + case bst_expr.Function(args=args): + for arg in args: + yield from _names_in(arg) + case bst_expr.BinOp(lh=lh, rh=rh): + yield from _names_in(lh) + yield from _names_in(rh) + case bst_expr.RightOp(rh=rh): + yield from _names_in(rh) + case bst_expr.Array(elements=elements): + for element in elements: + yield from _names_in(element) + + +def _dotted_name(node: Any) -> str | None: + """Return ``a.b.c`` for a bare name or a chain of ``.field`` lookups, else None.""" + match node: + case str() if node[:1] in ('"', "'") or node in ('true', 'false', 'null'): + return None # a string literal or a constant, not a read + case str(): + return node + case bst_expr.Property(name=base, field=field): + prefix = _dotted_name(base) + return f'{prefix}.{field}' if prefix is not None else None + return None + + +def _is_inside(name: str, prefix: str) -> bool: + """Whether ``name`` is ``prefix`` itself or a field inside it.""" + return name == prefix or name.startswith(f'{prefix}.') diff --git a/src/bids_validator/checks/schema_cache.py b/src/bids_validator/checks/schema_cache.py new file mode 100644 index 0000000..1e71c8c --- /dev/null +++ b/src/bids_validator/checks/schema_cache.py @@ -0,0 +1,68 @@ +"""Work something out from the schema once, instead of once for every file. + +The checks ask the schema the same questions for every file of a dataset: which +filename rules exist, which extensions name a directory recording, which rules have +selectors. The answers depend only on the schema, so a function that works one of them +out is decorated with :func:`once_per_schema`: it then runs once per schema, and every +later call returns the answer it already has. +""" + +# Lets a type hint name a class before Python has defined it (``Namespace`` below is only +# imported for type checkers). Without it, annotations are evaluated when the function is +# defined. +from __future__ import annotations + +import functools +from collections.abc import Callable +from typing import TYPE_CHECKING, TypeVar + +# TYPE_CHECKING is False when the program runs and True only for a type checker (mypy). +# Imports placed under it cost nothing at run time and cannot cause import cycles; they +# exist only so the type hints can name these classes. +if TYPE_CHECKING: + from bidsschematools.types.namespace import Namespace + +# The names other modules are meant to import from this one (``from x import *`` also +# imports only these). Anything else here is internal. +__all__ = ['once_per_schema'] + +# "Some type": whatever the decorated function returns, the decorated version returns the +# same type. This lets mypy check the callers of a decorated function. +T = TypeVar('T') + + +def once_per_schema(function: Callable[[Namespace], T]) -> Callable[[Namespace], T]: + """Decorate a function of the schema alone, so it runs once per schema. + + Writing ``@once_per_schema`` above a function is the same as writing + ``function = once_per_schema(function)`` after it: the function is replaced by + ``remembered`` below, which keeps the original and a small table of answers. + + A bidsschematools ``Namespace`` cannot be a dictionary key (it is not hashable), so + each answer is kept under the schema's ``id()``, together with the schema itself. + Keeping the schema keeps it alive, so its ``id()`` can never be given to another + schema while the answer is stored. + + Parameters + ---------- + function : callable + Takes the schema, returns anything. It must not depend on anything else. + + Returns + ------- + callable + The same function, remembering its answer for each schema it is given. + + """ + answers: dict[int, tuple[Namespace, T]] = {} + + # Copies the name and docstring of ``function`` onto ``remembered``, so error + # messages and help() still show the original function. + @functools.wraps(function) + def remembered(schema: Namespace) -> T: + known = answers.get(id(schema)) + if known is None or known[0] is not schema: + known = answers[id(schema)] = (schema, function(schema)) + return known[1] + + return remembered diff --git a/src/bids_validator/checks/tables.py b/src/bids_validator/checks/tables.py new file mode 100644 index 0000000..3ddeb2b --- /dev/null +++ b/src/bids_validator/checks/tables.py @@ -0,0 +1,524 @@ +"""The table payloads: the right columns, in the right order, with the right values. + +``rules.tabular_data`` describes each kind of TSV file:: + + EEGChannels: # rules.tabular_data.eeg.EEGChannels + selectors: [datatype == "eeg", suffix == "channels", extension == ".tsv"] + initial_columns: [name__channels, type__channels, units] + columns: + name__channels: required + type__channels: required + units: required + description: optional + ... + additional_columns: allowed_if_defined + +Three payloads, one function each, all following the reference validator's +``schema/tables.ts``: + +* ``columns`` (:func:`column_issues`): every required column is present + (``TSV_COLUMN_MISSING``); a column the rule does not list is allowed, must be + defined in the sidecar, or is not allowed (``TSV_ADDITIONAL_COLUMNS_*``); and every + value has the type the schema gives the column, refined by the sidecar's own + definition of it (``TSV_VALUE_INCORRECT_TYPE``, ``TSV_COLUMN_TYPE_REDEFINED``). +* ``initial_columns`` (:func:`initial_column_issues`): the first columns are these, in + this order (``TSV_COLUMN_ORDER_INCORRECT``). +* ``index_columns`` (:func:`index_column_issues`): no two rows share the same values + in these columns (``TSV_INDEX_VALUE_NOT_UNIQUE``). + +A cell holding ``n/a`` is always accepted. The rule engine (:mod:`.rules`) calls +these functions for every rule with one of these payloads whose selectors hold. +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +import re +from collections.abc import Mapping +from typing import TYPE_CHECKING, Any + +import attrs + +from ..issues import Issue, IssueDefinition, Severity + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from ..context import Context + +# The names other modules are meant to import from this one: the three payload handlers +# and the codes defined below. +__all__ = [ + 'TSV_ADDITIONAL_COLUMNS_MUST_DEFINE', + 'TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED', + 'TSV_ADDITIONAL_COLUMNS_UNDEFINED', + 'TSV_COLUMN_MISSING', + 'TSV_COLUMN_ORDER_INCORRECT', + 'TSV_COLUMN_TYPE_REDEFINED', + 'TSV_INDEX_VALUE_NOT_UNIQUE', + 'TSV_PSEUDO_AGE_DEPRECATED', + 'TSV_VALUE_INCORRECT_TYPE', + 'AllowedValues', + 'column_issues', + 'index_column_issues', + 'initial_column_issues', +] + +# --- what this module reports -------------------------------------------- +# +# None of these codes is defined by the schema; they are the reference validator's +# (bids-validator, src/issues/list.ts). + +# A required column is not in the table. +TSV_COLUMN_MISSING = IssueDefinition( + 'TSV_COLUMN_MISSING', Severity.ERROR, 'A required column is missing' +) +# A column the rule wants first is somewhere else. +TSV_COLUMN_ORDER_INCORRECT = IssueDefinition( + 'TSV_COLUMN_ORDER_INCORRECT', Severity.ERROR, 'Some TSV columns are in the incorrect order' +) +# A column the rule does not list, where the rule allows none. +TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED = IssueDefinition( + 'TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED', + Severity.ERROR, + 'A TSV file has extra columns which are not allowed for its file type', +) +# An unlisted column the sidecar should define, and does not. +TSV_ADDITIONAL_COLUMNS_MUST_DEFINE = IssueDefinition( + 'TSV_ADDITIONAL_COLUMNS_MUST_DEFINE', + Severity.ERROR, + 'Additional TSV columns must be defined in the associated JSON sidecar for this file type', +) +# An unlisted column, allowed, but not defined in the sidecar. +TSV_ADDITIONAL_COLUMNS_UNDEFINED = IssueDefinition( + 'TSV_ADDITIONAL_COLUMNS_UNDEFINED', + Severity.WARNING, + 'A TSV file has extra columns which are not defined in its associated JSON sidecar', +) +# A sidecar's definition of a column contradicts the schema's. +TSV_COLUMN_TYPE_REDEFINED = IssueDefinition( + 'TSV_COLUMN_TYPE_REDEFINED', + Severity.WARNING, + 'A column required in a TSV file has been redefined in a sidecar file. This redefinition ' + 'is being ignored.', +) +# A cell whose value does not fit its column (text in a number column). +TSV_VALUE_INCORRECT_TYPE = IssueDefinition( + 'TSV_VALUE_INCORRECT_TYPE', + Severity.ERROR, + 'A value in a column did not match the acceptable type for that column headers specified ' + 'format.', +) +# ``89+`` in an ``age`` column. +TSV_PSEUDO_AGE_DEPRECATED = IssueDefinition( + 'TSV_PSEUDO_AGE_DEPRECATED', + Severity.WARNING, + 'Use of the value "89+" in column "age" is deprecated. Use 89 for all ages 89 and over.', +) +# Two rows with the same values in the index columns. +TSV_INDEX_VALUE_NOT_UNIQUE = IssueDefinition( + 'TSV_INDEX_VALUE_NOT_UNIQUE', + Severity.ERROR, + 'An index column(s) was specified for the tsv file and not all of the values for it are ' + 'unique.', +) + +# A column a rule does not list, by the rule's ``additional_columns``: what is reported +# when the sidecar does not define it (or, for ``not_allowed``, at all). +_EXTRA_COLUMN = { + 'not_allowed': TSV_ADDITIONAL_COLUMNS_NOT_ALLOWED, + 'allowed_if_defined': TSV_ADDITIONAL_COLUMNS_MUST_DEFINE, +} + +# The extensions of the files these payloads apply to; a rule may select other files too. +_TABLE_EXTENSIONS = ('.tsv', '.tsv.gz') + +# The leading number of a string, as JavaScript's parseFloat reads it ("12abc" is 12). +_LEADING_NUMBER = re.compile(r'\s*[+-]?(?:\d+\.?\d*|\.\d+)(?:[eE][+-]?\d+)?') + + +# A read-only data class (see issues.py for what attrs does). +@attrs.frozen +class AllowedValues: + """What a column's values must look like, from the schema and the sidecar together. + + Attributes + ---------- + formats : tuple of str + Format names (keys of ``schema.objects.formats``): any one will do. + pattern : str or None + A pattern of its own, used instead of the formats' patterns. + units, levels, maximum, minimum + Further constraints; ``levels`` is the allowed values. + + """ + + formats: tuple[str, ...] + pattern: str | None = None + units: str | None = None + levels: tuple[str, ...] | None = None + maximum: float | None = None + minimum: float | None = None + + +# Raised by _narrowed_by_sidecar and caught in _listed_column, where it becomes a +# TSV_COLUMN_TYPE_REDEFINED finding; an exception lets the reason travel with it. +class _SidecarContradictsSchema(Exception): + """A sidecar definition that conflicts with the schema's, rather than narrowing it.""" + + +def column_issues(path: str, rule: Mapping[str, Any], context: Context) -> list[Issue]: + """Payload ``columns``: missing, extra and badly typed columns. + + Parameters + ---------- + path : str + Dotted schema path of the rule. + rule : Mapping + The rule; its selectors already hold for this file. + context : Context + Everything known about one file. + + Returns + ------- + list of Issue + The findings. Only the first bad value of a column is reported. + + """ + if context.extension not in _TABLE_EXTENSIONS or not rule.get('columns'): + return [] + objects = context.schema['objects']['columns'] + listed = {str(objects[key]['name']): key for key in rule['columns']} + present = dict(context.columns or {}) + extra = [name for name in present if name not in listed] + + issues: list[Issue] = [] + for name in [*listed, *extra]: + if name in listed: + found, allowed = _listed_column(context, path, rule, listed[name], present) + else: + found, allowed = _extra_column(context, path, rule, name) + issues += found + if allowed is not None: + issues += _value_issues(context, path, name, list(present[name]), allowed) + return issues + + +def _listed_column( + context: Context, path: str, rule: Mapping[str, Any], key: str, present: Mapping[str, Any] +) -> tuple[list[Issue], AllowedValues | None]: + """Check a column the rule lists; return its findings and what its values may be. + + The allowed values are ``None`` when the column is absent, so there are no values to + check. A required column is reported missing here unless it is one of the rule's + initial columns, which :func:`initial_column_issues` reports instead. + """ + column = context.schema['objects']['columns'][key] + name = str(column['name']) + location = context.file.relative_path + if name not in present: + required = _requirement(rule, key) == 'required' + if required and key not in (rule.get('initial_columns') or []): + missing = TSV_COLUMN_MISSING.make_issue( + location, sub_code=name, rule=path, message=f'required column {name!r} is missing' + ) + return [missing], None + return [], None + try: + return [], _allowed_values(column, _sidecar_definition(context, name)) + except _SidecarContradictsSchema as error: + redefined = TSV_COLUMN_TYPE_REDEFINED.make_issue( + context.sidecar_origins.get(name, ''), + sub_code=name, + rule=path, + message=str(error), + ) + return [redefined], _allowed_by_schema(column) + + +def _extra_column( + context: Context, path: str, rule: Mapping[str, Any], name: str +) -> tuple[list[Issue], AllowedValues | None]: + """Check a column the rule does not list; return its findings and what its values may be. + + The rule's ``additional_columns`` decides: ``allowed`` (the default), only + ``allowed_if_defined`` in the sidecar, or ``not_allowed``. A column the sidecar + defines has its values checked against that definition. + """ + allowed = str(rule.get('additional_columns', 'n/a')) + definition = _sidecar_definition(context, name) + if allowed == 'not_allowed' or definition is None: + found = _EXTRA_COLUMN.get(allowed, TSV_ADDITIONAL_COLUMNS_UNDEFINED) + location = context.file.relative_path + return [found.make_issue(location, sub_code=name, rule=path, message=name)], None + return [], _allowed_by_sidecar(definition) + + +def _sidecar_definition(context: Context, name: str) -> Mapping[str, Any] | None: + """Return the sidecar's definition of a column (``{"Description": ...}``), if any.""" + definition = context.sidecar.get(name) + return definition if isinstance(definition, Mapping) else None + + +def initial_column_issues(path: str, rule: Mapping[str, Any], context: Context) -> list[Issue]: + """Payload ``initial_columns``: the first columns are these, in this order. + + A listed column that is absent and not required is left out of the count, so an + optional column that is simply not used does not shift the others. + """ + if ( + context.extension not in _TABLE_EXTENSIONS + or not rule.get('columns') + or not rule.get('initial_columns') + ): + return [] + objects = context.schema['objects']['columns'] + headers = list(context.columns or {}) + wanted = [] + for key in rule['initial_columns']: + name = str(objects[key]['name']) + index = headers.index(name) if name in headers else -1 + if _requirement(rule, key) == 'required' or index != -1: + wanted.append((name, index)) + + issues: list[Issue] = [] + for target, (name, index) in enumerate(wanted): + if index == -1: + found, message = TSV_COLUMN_MISSING, f'required initial column {target + 1} not found' + elif index != target: + found = TSV_COLUMN_ORDER_INCORRECT + message = f'initial column {target + 1} found at position {index + 1}' + else: + continue + issues.append( + found.make_issue( + context.file.relative_path, sub_code=name, rule=path, message=f'{name}: {message}' + ) + ) + return issues + + +def index_column_issues(path: str, rule: Mapping[str, Any], context: Context) -> list[Issue]: + """Payload ``index_columns``: no two rows share the same index values. + + Each repeated row is reported, with its line number (the header is line 1). + """ + if ( + context.extension not in _TABLE_EXTENSIONS + or not rule.get('columns') + or not rule.get('index_columns') + ): + return [] + objects = context.schema['objects']['columns'] + columns: Mapping[str, Any] = context.columns or {} + names = [str(objects[key]['name']) for key in rule['index_columns']] + names = [name for name in names if name in columns] + if not names: + return [] + seen: set[str] = set() + issues: list[Issue] = [] + for row in range(len(columns[names[0]])): + value = ''.join(str(columns[name][row]) for name in names) + if value in seen: + issues.append( + TSV_INDEX_VALUE_NOT_UNIQUE.make_issue( + context.file.relative_path, + rule=path, + message=f'row {row + 2}, value {value!r}', + ) + ) + else: + seen.add(value) + return issues + + +# --- value types ---------------------------------------------------------- + + +def _value_issues( + context: Context, path: str, name: str, values: list[str], allowed: AllowedValues +) -> list[Issue]: + """Report the first value of a column that is not allowed. + + ``89+`` in an ``age`` column is the one exception: deprecated rather than wrong, + and warned about once. + """ + if _allows_anything(allowed): + return [] + pattern = allowed.pattern + if pattern is None: + formats = context.schema['objects']['formats'] + if any(fmt not in formats.keys() for fmt in allowed.formats): # noqa: SIM118 + return [] # an unknown format: nothing to check against + pattern = '|'.join(str(formats[fmt]['pattern']) for fmt in allowed.formats) + # Anchored the way the reference validator anchors it: "^" + pattern + "$", + # without grouping the alternatives. + compiled = re.compile(f'^{pattern}$') + + issues: list[Issue] = [] + age_warned = False + for row, value in enumerate(values): + if _is_allowed(value, compiled, allowed): + continue + if name == 'age' and value == '89+': + if not age_warned: + age_warned = True + issues.append( + TSV_PSEUDO_AGE_DEPRECATED.make_issue( + context.file.relative_path, message=f'line {row + 2}' + ) + ) + continue + hint = ", did you mean 'n/a'?" if re.fullmatch(r'\s*(NA|na|nan|NaN|n/a)\s*', value) else '' + issues.append( + TSV_VALUE_INCORRECT_TYPE.make_issue( + context.file.relative_path, + sub_code=name, + rule=path, + message=f'line {row + 2}: {value!r}{hint}', + ) + ) + break + return issues + + +def _is_allowed(value: str, pattern: re.Pattern[str], allowed: AllowedValues) -> bool: + """Whether one cell is allowed. ``n/a`` always is.""" + if value == 'n/a': + return True + if not pattern.search(value): + return False + if allowed.levels is not None and value not in allowed.levels: + return False + if allowed.maximum is not None or allowed.minimum is not None: + number = _parse_float(value) + if allowed.maximum is not None and not number <= allowed.maximum: + return False + if allowed.minimum is not None and not number >= allowed.minimum: + return False + return True + + +def _allowed_values( + column: Mapping[str, Any], definition: Mapping[str, Any] | None +) -> AllowedValues: + """Return what a listed column may hold: the schema's rules, narrowed by the sidecar. + + A column whose schema object has a ``definition`` (a conventional column, such as + ``trial_type``) is fully defined by the sidecar when the sidecar defines it. + """ + if 'definition' in column.keys(): # noqa: SIM118 + return _allowed_by_sidecar(definition or column['definition']) + base = _allowed_by_schema(column) + return ( + _narrowed_by_sidecar(base, _allowed_by_sidecar(definition)) + if definition is not None + else base + ) + + +def _allowed_by_schema(column: Mapping[str, Any]) -> AllowedValues: + """Return what the schema allows in a column.""" + levels = column.get('enum') + return AllowedValues( + formats=tuple(_formats(column)), + pattern=column.get('pattern'), + units=column.get('unit'), + levels=tuple(str(level) for level in levels) if levels is not None else None, + maximum=column.get('maximum'), + minimum=column.get('minimum'), + ) + + +def _allowed_by_sidecar(definition: Mapping[str, Any]) -> AllowedValues: + """Return what a sidecar's column definition allows (``Format``, ``Levels`` ...).""" + fmt = definition.get('Format') or ('number' if definition.get('Units') else 'string') + levels = definition.get('Levels') + return AllowedValues( + formats=(str(fmt),), + units=definition.get('Units'), + levels=tuple(str(level) for level in levels) if isinstance(levels, Mapping) else None, + maximum=definition.get('Maximum'), + minimum=definition.get('Minimum'), + ) + + +def _narrowed_by_sidecar(base: AllowedValues, override: AllowedValues) -> AllowedValues: + """Combine what the schema allows with what the sidecar says, which may only narrow it. + + Raises :class:`_SidecarContradictsSchema` when the sidecar contradicts the schema: another kind + of value (text where the schema wants numbers), levels outside the schema's, + different units, or wider bounds. + """ + kinds = {_kind(fmt) for fmt in base.formats} + if _kind(override.formats[0]) not in kinds: + raise _SidecarContradictsSchema( + f'format {override.formats[0]!r} must be {" or ".join(base.formats)}' + ) + levels = override.levels if override.levels is not None else base.levels + if ( + base.levels is not None + and override.levels is not None + and not set(override.levels) <= set(base.levels) + ): + raise _SidecarContradictsSchema( + f'levels {{{", ".join(override.levels)}}} are not a subset of ' + f'{{{", ".join(base.levels)}}}' + ) + units = override.units if override.units is not None else base.units + if base.units is not None and units != base.units: + raise _SidecarContradictsSchema(f'unit {units!r} must be {base.units!r}') + minimum = override.minimum if override.minimum is not None else base.minimum + if base.minimum is not None and minimum is not None and minimum < base.minimum: + raise _SidecarContradictsSchema(f'minimum {minimum} is less than {base.minimum}') + maximum = override.maximum if override.maximum is not None else base.maximum + if base.maximum is not None and maximum is not None and maximum > base.maximum: + raise _SidecarContradictsSchema(f'maximum {maximum} is greater than {base.maximum}') + return AllowedValues( + formats=override.formats, + pattern=base.pattern, + units=units, + levels=levels, + maximum=maximum, + minimum=minimum, + ) + + +def _formats(column: Mapping[str, Any]) -> list[str]: + """Return the format names a schema column allows, following ``anyOf``.""" + alternatives = column.get('anyOf') + if isinstance(alternatives, list): + return [fmt for alternative in alternatives for fmt in _formats(alternative)] + return [str(column.get('format') or column.get('type') or 'string')] + + +def _kind(fmt: str) -> str: + """Collapse a format to the kind of value it holds: number, boolean or string.""" + return {'integer': 'number', 'number': 'number', 'boolean': 'boolean'}.get(fmt, 'string') + + +def _allows_anything(allowed: AllowedValues) -> bool: + """Whether every value would pass, so there is nothing to check.""" + return ( + allowed.levels is None + and allowed.maximum is None + and allowed.minimum is None + and ( + (allowed.pattern is None and allowed.formats[0] == 'string') or allowed.pattern == '.*' + ) + ) + + +def _requirement(rule: Mapping[str, Any], key: str) -> str: + """Return a column's requirement level in a rule: ``required``, ``recommended`` ...""" + requirement = rule['columns'].get(key) + if isinstance(requirement, Mapping): + return str(requirement.get('level')) + if requirement is not None: + return str(requirement) + return str(rule.get('additional_columns', 'n/a')) + + +def _parse_float(text: str) -> float: + """JavaScript's ``parseFloat``: the leading number of a string, else NaN.""" + found = _LEADING_NUMBER.match(text) + return float(found.group(0)) if found else float('nan') diff --git a/src/bids_validator/checks/values.py b/src/bids_validator/checks/values.py new file mode 100644 index 0000000..2567633 --- /dev/null +++ b/src/bids_validator/checks/values.py @@ -0,0 +1,192 @@ +"""Check a metadata value against its schema definition. + +Every metadata field in ``schema.objects.metadata`` comes with a small JSON Schema: +``RepetitionTime`` must be a number greater than 0, ``Authors`` an array of strings, +``DatasetType`` one of three words. :func:`value_errors` checks one value against one +such definition and returns what is wrong, as messages; an empty list means valid. + +The reference validator does this with the ajv library, the schema's +``objects.formats`` patterns registered as ajv formats. This module implements the +part of JSON Schema those definitions use, with ajv's behaviour, so the findings +match: + +* **One error per value.** ajv stops at the first keyword that fails (its default, + ``allErrors: false``), checking ``type`` first, then the keywords for any type + (``enum``, ``anyOf``), then those for numbers, strings, arrays and objects. +* **``anyOf`` reports every branch.** When no alternative fits, each alternative's + first error is reported, then ``must match a schema in anyOf``. +* **Formats are searched, not matched.** ajv tests a format's pattern anywhere in the + string (``RegExp.test``), so a pattern without ``^``/``$`` only needs to occur + somewhere. Several BIDS formats are written that way and accept almost anything + in the reference validator; this module accepts the same. +* **Messages are ajv's** (``must be number``, ``must be >= 0`` ...), prefixed with the + position inside the value when the problem is nested (``/0 must be string``). +""" + +# Lets type hints name types before they are defined; kept for consistency with the +# other modules. +from __future__ import annotations + +import math +import operator +import re +from collections.abc import Callable, Mapping +from typing import Any, TypeGuard + +# The names other modules are meant to import from this one. +__all__ = ['json_equal', 'value_errors'] + +# The four number bounds of JSON Schema, in ajv's order: (keyword, the sign ajv prints in +# its message, the test the value must pass). ``operator.le(a, b)`` is ``a <= b``. +_COMPARISONS: tuple[tuple[str, str, Callable[[float, float], bool]], ...] = ( + ('maximum', '<=', operator.le), + ('minimum', '>=', operator.ge), + ('exclusiveMaximum', '<', operator.lt), + ('exclusiveMinimum', '>', operator.gt), +) + + +def value_errors(value: Any, definition: Any, formats: Mapping[str, Any]) -> list[str]: + """Return what is wrong with ``value`` according to ``definition``. + + Parameters + ---------- + value : Any + A value read from a JSON file. + definition : Mapping + Its schema definition, such as ``schema.objects.metadata.RepetitionTime``. + formats : Mapping + ``schema.objects.formats``: format name to ``{pattern: ...}``. + + Returns + ------- + list of str + ajv-style messages; empty when the value is valid. + + """ + return _errors(value, definition, formats, '') + + +def _errors(value: Any, schema: Any, formats: Mapping[str, Any], position: str) -> list[str]: + """Return the errors of one value at one position, in ajv's keyword order.""" + if not isinstance(schema, Mapping): + return [] + + expected = schema.get('type') + if expected is not None and not _has_type(value, expected): + names = ','.join(expected) if isinstance(expected, list) else str(expected) + return [_with_position(position, f'must be {names}')] + + if 'enum' in schema.keys() and not any(json_equal(value, option) for option in schema['enum']): # noqa: SIM118 + return [_with_position(position, 'must be equal to one of the allowed values')] + + alternatives = schema.get('anyOf') + if isinstance(alternatives, list): + branches = [_errors(value, alternative, formats, position) for alternative in alternatives] + if all(branches): + return [errors[0] for errors in branches] + [ + _with_position(position, 'must match a schema in anyOf') + ] + + if _is_number(value): + for keyword, sign, holds in _COMPARISONS: + limit = schema.get(keyword) + if _is_number(limit) and not holds(value, limit): + return [_with_position(position, f'must be {sign} {limit}')] + + if isinstance(value, str): + name = schema.get('format') + pattern = formats.get(name, {}).get('pattern') if name else None + if pattern is not None and re.search(str(pattern), value) is None: + return [_with_position(position, f'must match format "{name}"')] + + if isinstance(value, list): + return _array_errors(value, schema, formats, position) + + if isinstance(value, Mapping): + return _object_errors(value, schema, formats, position) + + return [] + + +def _array_errors( + value: list[Any], schema: Mapping[str, Any], formats: Mapping[str, Any], position: str +) -> list[str]: + if _is_number(schema.get('maxItems')) and len(value) > schema['maxItems']: + return [_with_position(position, f'must NOT have more than {schema["maxItems"]} items')] + if _is_number(schema.get('minItems')) and len(value) < schema['minItems']: + return [_with_position(position, f'must NOT have fewer than {schema["minItems"]} items')] + items = schema.get('items') + if isinstance(items, Mapping): + for index, item in enumerate(value): + errors = _errors(item, items, formats, f'{position}/{index}') + if errors: + return errors + return [] + + +def _object_errors( + value: Mapping[str, Any], schema: Mapping[str, Any], formats: Mapping[str, Any], position: str +) -> list[str]: + for name in schema.get('required', []) or []: + if name not in value: + return [_with_position(position, f"must have required property '{name}'")] + properties = schema.get('properties') or {} + if schema.get('additionalProperties') is False and any( + key not in properties.keys() # noqa: SIM118 + for key in value + ): + return [_with_position(position, 'must NOT have additional properties')] + for name in properties.keys(): # noqa: SIM118 + if name in value: + errors = _errors(value[name], properties[name], formats, f'{position}/{name}') + if errors: + return errors + return [] + + +def _has_type(value: Any, expected: Any) -> bool: + """Whether ``value`` has the JSON type ``expected`` (a name or a list of names).""" + if isinstance(expected, list): + return any(_has_type(value, name) for name in expected) + match expected: + case 'number': + return _is_number(value) + case 'integer': + return _is_number(value) and float(value).is_integer() + case 'string': + return isinstance(value, str) + case 'boolean': + return isinstance(value, bool) + case 'array': + return isinstance(value, list) + case 'object': + return isinstance(value, Mapping) + case 'null': + return value is None + return True # an unknown type name: do not flag + + +def _is_number(value: Any) -> TypeGuard[int | float]: + """Return whether a value is a JSON number: int or float, not bool (True counts as 1).""" + return isinstance(value, (int, float)) and not isinstance(value, bool) and math.isfinite(value) + + +def json_equal(value: Any, option: Any) -> bool: + """JSON equality: ``true`` is not ``1``, and arrays and objects compare by content.""" + if isinstance(value, bool) or isinstance(option, bool): + return isinstance(value, bool) and isinstance(option, bool) and value == option + if _is_number(value) and _is_number(option): + return float(value) == float(option) + if isinstance(value, list) and isinstance(option, list): + return len(value) == len(option) and all(map(json_equal, value, option)) + if isinstance(value, Mapping) and isinstance(option, Mapping): + return value.keys() == option.keys() and all( + json_equal(value[k], option[k]) for k in value + ) + return type(value) is type(option) and bool(value == option) + + +def _with_position(position: str, message: str) -> str: + """Prefix a message with the position inside the value (``/0``), when there is one.""" + return f'{position} {message}' if position else message diff --git a/src/bids_validator/checks/walk.py b/src/bids_validator/checks/walk.py new file mode 100644 index 0000000..ddcedff --- /dev/null +++ b/src/bids_validator/checks/walk.py @@ -0,0 +1,190 @@ +"""Which files and folders a validation visits. + +Every check works on one :class:`~bids_validator.context.Context` at a time. This +module decides which ones exist: it walks the dataset and yields a context for every +file and every folder that is not ignored, in the reference validator's order and +with its rules (``src/schema/walk.ts``, ``src/files/ignore.ts`` and +``src/validators/bids.ts`` in bids-validator): + +* **Ignored, never visited:** the folders the reference ignores at the top of the + dataset (:data:`DEFAULT_IGNORES`), hidden files and folders at every level + (:data:`PRUNED`), and whatever the dataset's ``.bidsignore`` lists. ``.bidsignore`` + itself is visited. +* **Visited as one unit:** a directory recording, such as a CTF ``.ds`` folder. Its + name has entities, a suffix, and an extension the schema spells with a trailing + ``/``; it is checked like a file, and its vendor-named contents are not walked. The + schema also lists a bare ``/`` (``objects.extensions.Directory``), so any folder + named like a file, ``sub-01_xyz``, counts as one too, as in the reference. +* **Visited, not walked into:** an opaque folder (``rules.directories`` marks + ``docs`` and ``logs`` opaque, for example). +* **Visited, then walked into:** every other folder. +* **A raw dataset's** ``derivatives`` folders are skipped wherever they are: a + derivative is a separate dataset with separate rules. To check one, validate its + own root. +""" + +# Lets type hints name classes that are only imported for type checkers (below). +from __future__ import annotations + +from collections.abc import Iterator +from typing import TYPE_CHECKING + +from bidsschematools.types.context import Subject + +from ..bidsignore import Ignore, IgnoreMany +from ..context import Context, Dataset, FileParts, Sessions +from .schema_cache import once_per_schema + +# Imported only for type hints; never imported when the program runs. +if TYPE_CHECKING: + from bidsschematools.types.namespace import Namespace + + from ..bidsignore import HasMatch + from ..types.files import FileTree + +# The names other modules are meant to import from this one. +__all__ = [ + 'BIDSIGNORE', + 'DEFAULT_IGNORES', + 'DERIVATIVES_DIR', + 'PRUNED', + 'build_ignore', + 'is_plain_folder', + 'iter_contexts', +] + +# The reference validator's default "ignore" group: folders at the TOP of the dataset +# that BIDS does not constrain (the leading "/" anchors each pattern at the root). +DEFAULT_IGNORES = ('/sourcedata/', '/derivatives/', '/code/', '/stimuli/', '/log/', '/doc/') + +# The reference validator's default "prune" group: hidden files and folders at every +# level (.git, .DS_Store, .datalad ...). Its exception, .bidsignore, is handled by name. +PRUNED = ('.**',) + +# The dataset's own ignore list. It is visited (and checked) although it is hidden. +BIDSIGNORE = '.bidsignore' + +# The folder a raw dataset keeps its derivative datasets in; skipped at any depth. +DERIVATIVES_DIR = 'derivatives' + + +def iter_contexts(dataset: Dataset, ignore: HasMatch | None = None) -> Iterator[Context]: + """Yield a context for every file and folder a validation should look at. + + Parameters + ---------- + dataset : Dataset + The dataset, which pairs the file tree with the schema. + ignore : HasMatch, optional + The ignore matcher; :func:`build_ignore` by default. + + Yields + ------ + Context + One per visited file or folder, depth first, files before folders as in the + reference validator. A folder's context has ``context.file.is_dir`` true; use + :func:`is_plain_folder` to tell a plain folder from a directory recording. + + """ + if ignore is None: + ignore = build_ignore(dataset.tree) + kind = str(dataset.dataset_description.get('DatasetType')) + opaque = _opaque_folders(dataset.schema).get(kind, frozenset()) + yield from _walk(dataset.tree, dataset, ignore, kind == 'raw', opaque, subject=None) + + +def build_ignore(tree: FileTree) -> IgnoreMany: + """Build the ignore matcher: the reference defaults plus the dataset's .bidsignore.""" + ignores = [Ignore(list(DEFAULT_IGNORES + PRUNED))] + bidsignore = tree.children.get(BIDSIGNORE) + if bidsignore is not None: + ignores.append(Ignore.from_file(bidsignore)) + return IgnoreMany(ignores) + + +def is_plain_folder(context: Context) -> bool: + """Whether the context is a plain folder, as opposed to a file or a recording. + + A directory recording (``sub-01_task-x_meg.ds/``) is a folder on disk but a file + to BIDS, so it is not a plain folder. + """ + return context.file.is_dir and not _is_recording(context.file, context.schema) + + +def _walk( + tree: FileTree, + dataset: Dataset, + ignore: HasMatch, + raw: bool, + opaque: frozenset[str], + subject: Subject | None, +) -> Iterator[Context]: + """Yield the contexts below ``tree``: its files first, then each folder and its contents. + + Each context carries the :class:`Subject` of the enclosing ``sub-*`` folder. + """ + if subject is None and tree.name.startswith('sub-'): + subject = Subject(Sessions(tree)) + + children = list(tree.children.values()) + for child in children: + if not child.is_dir and _is_visited(child, ignore): + yield Context(child, dataset, subject) + + for child in children: + if not child.is_dir or not _is_visited(child, ignore): + continue + if raw and child.name == DERIVATIVES_DIR: + continue # a separate dataset with separate rules + yield Context(child, dataset, subject) + if _is_recording(child, dataset.schema): + continue # one unit: its own name is checked, its contents are not + if child.parent is dataset.tree and child.name in opaque: + continue # BIDS does not constrain what is inside + yield from _walk(child, dataset, ignore, raw, opaque, subject) + + +def _is_visited(node: FileTree, ignore: HasMatch) -> bool: + """Whether a file or folder is visited: not ignored, or the .bidsignore itself.""" + if node.relative_path == BIDSIGNORE: + return True + return not ignore.match(node.relative_path) + + +def _is_recording(node: FileTree, schema: Namespace) -> bool: + """Whether a folder is a directory recording, by the reference validator's test. + + Its name must have at least one entity, a suffix, and an extension the schema + lists with a trailing ``/`` (``.ds/``, ``.mefd/``, ``.ome.zarr/``, and ``/`` for a + folder with no extension at all). A folder called ``old.ds`` with no entities is an + ordinary folder. + """ + if not node.is_dir: + return False + parts = FileParts.from_file(node) + return bool( + parts.entities and parts.suffix and parts.extension in _recording_extensions(schema) + ) + + +@once_per_schema +def _recording_extensions(schema: Namespace) -> frozenset[str]: + """Return the schema's directory-recording extensions, with their trailing ``/``.""" + return frozenset( + str(definition['value']) + for definition in schema['objects']['extensions'].values() + if str(definition.get('value', '')).endswith('/') + ) + + +@once_per_schema +def _opaque_folders(schema: Namespace) -> dict[str, frozenset[str]]: + """Top-level folders the schema marks opaque, for each kind of dataset (raw, derivative).""" + opaque: dict[str, frozenset[str]] = {} + for kind, rules in schema['rules']['directories'].items(): + opaque[str(kind)] = frozenset( + str(rules[key]['name']) + for key in rules + if rules[key].get('opaque') and 'name' in rules[key].keys() # noqa: SIM118 + ) + return opaque diff --git a/src/bids_validator/context.py b/src/bids_validator/context.py index afe9752..8d691bd 100644 --- a/src/bids_validator/context.py +++ b/src/bids_validator/context.py @@ -3,8 +3,9 @@ from __future__ import annotations import gzip -import itertools +import math import struct +import zlib from functools import cache import attrs @@ -17,6 +18,8 @@ from .types import _typings as t from .types.files import FileTree +# The same trick as ``typing.TYPE_CHECKING``, without importing ``typing``: False when +# the program runs, so the imports below are only seen by type checkers (mypy, pyright). TYPE_CHECKING = False if TYPE_CHECKING: from collections.abc import Generator @@ -49,48 +52,179 @@ class ValidationError(Exception): - """TODO: Add issue structure.""" + """A problem found while reading a file, with the code it is reported under. + + Attributes + ---------- + code : str + An issue code (``JSON_INVALID``, ``TSV_EQUAL_ROWS``, ``MULTIPLE_INHERITABLE_FILES`` + ...), so a check can turn the error into a finding without knowing which + loader raised it. + message : str or None + Detail for the finding. + line : int or None + The 1-based line of a table problem. + location : str or None + Dataset-relative path of the file the problem belongs to, when that is not the + file being read (an ambiguous sidecar is reported on the sidecar). + """ + + def __init__( + self, + code: str, + message: str | None = None, + *, + line: int | None = None, + location: str | None = None, + ): + super().__init__(message or code) + self.code = code + self.message = message + self.line = line + self.location = location + + +class FileContents(dict[str, t.Any]): + """The contents of a file (a JSON object, a table's columns) with attribute access. + + Like a bidsschematools ``Namespace``, a field can be read as ``contents.Field``. + Unlike one, a key is only ever a key. A ``Namespace`` reads ``"key_resp.rt"`` as a + path into nested objects and cannot even list a mapping that holds such a key, + and table columns written by tools such as PsychoPy contain dots. + """ + + # Called for ``contents.Field`` when ``Field`` is not a normal attribute: read it as + # the key ``'Field'``. The expression language reads ``sidecar.RepetitionTime`` so. + def __getattr__(self, name: str) -> t.Any: + try: + return self[name] + except KeyError: + raise AttributeError(name) from None + def to_dict(self) -> dict[str, t.Any]: + """Return a plain ``dict`` copy.""" + return dict(self) + + +# Datatype -> modality (``anat`` -> ``mri``), filled from the schema on first use by +# ``datatype_to_modality``. _DATATYPE_MAP: dict[str, str] = {} +# Rows of a table read for the checks: the reference validator's ``--max-rows`` +# default. A physiology recording can have millions of rows; the first thousand show +# whether its values have the right type. +TSV_MAX_ROWS = 1000 + + +def datatype_to_modality(datatype: str, schema: Namespace) -> str | None: + """Generate a global map for datatype to modality. -def datatype_to_modality(datatype: str, schema: Namespace) -> str: - """Generate a global map for datatype to modality.""" + Returns None for a datatype that belongs to no modality. ``phenotype`` is one: + it is a datatype in ``objects.datatypes`` but appears in no ``rules.modalities`` + entry, so a dataset with a ``phenotype/`` folder used to raise ``KeyError`` here. + """ global _DATATYPE_MAP if not _DATATYPE_MAP: for mod_name, mod_dtypes in schema.rules.modalities.items(): _DATATYPE_MAP |= dict.fromkeys(mod_dtypes['datatypes'], mod_name) - return _DATATYPE_MAP[datatype] + return _DATATYPE_MAP.get(datatype) + + +def _read_columns( + lines: t.Iterable[str], headers: list[str], *, max_rows: int, first_line: int +) -> FileContents: + """Split table rows into columns, enforcing the shape the reference validator does. + + A row with a different number of cells than there are headers raises + ``TSV_EQUAL_ROWS``; an empty line raises ``TSV_EMPTY_LINE``, unless it is the last + line, since a file is expected to end with a newline. ``first_line`` is the + 1-based number of the first row in the file, for the error's ``line``. + """ + rows: list[list[str]] = [] + pending_empty: int | None = None + for number, line in enumerate(lines, start=first_line): + if max_rows > 0 and len(rows) >= max_rows: + break + text = line.rstrip('\r\n') + if pending_empty is not None: + raise ValidationError('TSV_EMPTY_LINE', line=pending_empty) + if text == '': + pending_empty = number + continue + values = text.split('\t') + if len(values) != len(headers): + raise ValidationError( + 'TSV_EQUAL_ROWS', + f'{len(values)} values for {len(headers)} columns', + line=number, + ) + rows.append(values) + return FileContents( + (header, tuple(row[index] for row in rows)) for index, header in enumerate(headers) + ) @cache -def load_tsv(file: FileTree, *, max_rows: int = 0) -> Namespace: - """Load TSV contents into a Namespace.""" - fobj: t.Iterable[str] - with file.path_obj.open() as fobj: - if max_rows > 0: - fobj = itertools.islice(fobj, max_rows) - contents = (line.rstrip('\r\n').split('\t') for line in fobj) - # Extract headers then transpose rows to columns - return Namespace(zip(next(contents), zip(*contents, strict=False), strict=False)) +def load_tsv(file: FileTree, *, max_rows: int = 0) -> FileContents: + """Load TSV contents into columns, keyed by header. + + A UTF-8 byte order mark is dropped. Raises :class:`ValidationError` with the code + the reference validator reports: ``TSV_COLUMN_HEADER_DUPLICATE``, + ``TSV_EMPTY_LINE`` or ``TSV_EQUAL_ROWS``. A header with no rows gives empty + columns, not a missing one. + """ + with file.path_obj.open(encoding='utf-8-sig', newline='') as fobj: + lines = iter(fobj) + header_line = next(lines, '').rstrip('\r\n') + headers = header_line.split('\t') if header_line else [] + if len(set(headers)) != len(headers): + raise ValidationError('TSV_COLUMN_HEADER_DUPLICATE', ', '.join(headers)) + return _read_columns(lines, headers, max_rows=max_rows, first_line=2) @cache -def load_tsv_gz(file: FileTree, headers: tuple[str], *, max_rows: int = 0) -> Namespace: - """Load TSVGZ contents into a Namespace.""" - with file.path_obj.open('rb') as fobj: - gzobj: t.Iterable[bytes] = gzip.GzipFile(fileobj=fobj, mode='r') - if max_rows > 0: - gzobj = itertools.islice(gzobj, max_rows) - contents = (line.decode().rstrip('\r\n').split('\t') for line in gzobj) - return Namespace(zip(headers, zip(*contents, strict=False), strict=False)) +def load_tsv_gz(file: FileTree, headers: tuple[str], *, max_rows: int = 0) -> FileContents: + """Load a headerless, gzipped TSV, whose headers come from its sidecar's ``Columns``. + + Raises :class:`ValidationError`: ``INVALID_GZIP`` when it does not decompress, + and the shape errors of :func:`load_tsv`. + """ + try: + with file.path_obj.open('rb') as fobj, gzip.GzipFile(fileobj=fobj, mode='r') as gzobj: + lines = (line.decode('utf-8') for line in gzobj) + return _read_columns(lines, list(headers), max_rows=max_rows, first_line=1) + except (gzip.BadGzipFile, EOFError, zlib.error) as error: + raise ValidationError('INVALID_GZIP', str(error)) from error @cache -def load_json(file: FileTree) -> dict[str, t.Any]: - """Load JSON file contents.""" - return orjson.loads(file.path_obj.read_bytes()) # type: ignore[no-any-return] +def load_json(file: FileTree) -> FileContents: + """Load the contents of a JSON file. + + Raises :class:`ValidationError` with the code the reference validator reports: + ``INVALID_JSON_ENCODING`` (not UTF-8, or UTF-8 with a byte order mark, which JSON + forbids), ``JSON_INVALID`` (does not parse) or ``JSON_NOT_AN_OBJECT``. + """ + data = file.path_obj.read_bytes() + if data.startswith(b'\xef\xbb\xbf'): + raise ValidationError('INVALID_JSON_ENCODING', 'starts with a byte order mark') + try: + data.decode('utf-8') + except UnicodeDecodeError as error: + raise ValidationError('INVALID_JSON_ENCODING', str(error)) from error + try: + contents = orjson.loads(data) + except orjson.JSONDecodeError as error: + raise ValidationError('JSON_INVALID', str(error)) from error + if not isinstance(contents, dict): + raise ValidationError('JSON_NOT_AN_OBJECT', type(contents).__name__) + return FileContents(contents) + + +# What nibabel raises for an empty, truncated or mislabelled image, plus the TypeError +# load_image raises for an image that is not NIfTI-1. +NIFTI_LOAD_ERRORS = (nb.filebasedimages.ImageFileError, OSError, EOFError, ValueError, TypeError) def load_image(path: UPath, api: type[ImgT]) -> ImgT: @@ -180,6 +314,34 @@ def load_gzip_header(file: FileTree) -> ctx.Gzip | None: return ctx.Gzip(timestamp=timestamp, filename=filename, comment=comment) +def tree_contains(tree: FileTree, relpath: str) -> bool: + """Return whether a POSIX path, relative to ``tree``, names a file or folder in it. + + The path is split on ``/`` by hand rather than parsed as a path object. Two + reasons, both found through the schema's ``exists()`` rules: + + * A leading ``/`` means the tree root. Every ``path`` in the context starts with + one (``/sub-01/anat/sub-01_T1w.nii.gz``), and ``rules.checks.general.DuplicateFiles`` + passes such a path to ``exists(..., "dataset")``. Parsed as a path object it is + absolute, never matched, and the rule could not fire. + * A value that is not a path, such as a ``bids::`` URI given to the ``"subject"`` + rule, is simply not found. Parsed as a path object it raised + ``ValueError: Protocol not known: 'bids'``. + + Empty and ``.`` segments are skipped, so ``sub-01//anat`` and ``./README`` work; + ``..`` is never a child name, so it is never found. + """ + node = tree + for part in relpath.split('/'): + if part in ('', '.'): + continue + child = node.children.get(part) + if child is None: + return False + node = child + return node is not tree + + class Subjects: """Collections of subjects in the dataset.""" @@ -218,7 +380,10 @@ def phenotype(self) -> list[str] | None: @staticmethod def _get_participant_id(phenotype_file: FileTree) -> list[str] | None: - columns = load_tsv(phenotype_file) + try: + columns = load_tsv(phenotype_file) + except (ValidationError, OSError, UnicodeDecodeError): + return None # reported when the table itself is checked if 'participant_id' not in columns: return None return list(columns['participant_id']) @@ -232,22 +397,40 @@ class Dataset: schema: Namespace ignored: list[str] = attrs.field(factory=list) subjects: Subjects = attrs.field(init=False) + # "location:key" of every sidecar value already checked against the schema, so a + # value in a shared sidecar is reported once, not once per data file using it. The + # reference validator keeps the same set on its dataset (``sidecarKeyValidated``). + checked_sidecar_values: set[str] = attrs.field(factory=set, init=False) def __attrs_post_init__(self) -> None: self.subjects = Subjects(self.tree) @cached_property - def dataset_description(self) -> Namespace: - """Contents of '/dataset_description.json'.""" - return Namespace.from_json( - UPath(self.tree.children['dataset_description.json']).read_text() - ) + def dataset_description(self) -> FileContents: + """Contents of '/dataset_description.json', with ``DatasetType`` filled in. + + A missing or unreadable file gives an empty description (the integrity checks + report it). ``DatasetType`` defaults as in the reference validator: + ``derivative`` when ``GeneratedBy`` is present, otherwise ``raw``. Without the + default, every rule selecting ``DatasetType == "raw"`` skips the many datasets + that leave the field out. + """ + description = FileContents() + node = self.tree.children.get('dataset_description.json') + if node is not None: + try: + description = FileContents(load_json(node)) # a copy: load_json is cached + except (ValidationError, OSError): + pass + if not description.get('DatasetType'): + description['DatasetType'] = 'derivative' if 'GeneratedBy' in description else 'raw' + return description @cached_property def modalities(self) -> list[str]: """List of modalities found in the dataset.""" result = {datatype_to_modality(datatype, self.schema) for datatype in self.datatypes} - return list(result) + return [modality for modality in result if modality is not None] @cached_property def datatypes(self) -> list[str]: @@ -266,7 +449,9 @@ def find_datatypes( if not child_obj.is_dir: continue - if child_name in datatypes: + # ``.keys()``, not the Namespace itself: ``".git" in namespace`` reads the dot as a + # path separator and inserts an empty key into the (cached, shared) schema. + if child_name in datatypes.keys(): # noqa: SIM118 result.add(child_name) elif max_depth == 0: continue @@ -306,16 +491,36 @@ def load_events(file: FileTree) -> None: # -> ctx.Events: def load_sidecar(file: FileTree) -> dict[str, t.Any]: - """Load sidecar metadata, using the inheritance principle.""" - # Uses walk back algorithm - # https://bids-validator.readthedocs.io/en/latest/validation-model/inheritance-principle.html - # Accumulates all sidecars - metadata: dict[str, t.Any] = {} + """Load sidecar metadata, using the inheritance principle. + + For one file on its own. A validation reads :attr:`Context.sidecar` instead, which + reuses the chain of JSON files the context found once. + """ + return _merge_sidecars(_sidecar_files(file)) + + +def _sidecar_files(file: FileTree) -> tuple[FileTree, ...]: + """Return the JSON files ``file`` inherits metadata from, nearest first. + + Uses the walk back algorithm: + https://bids-validator.readthedocs.io/en/latest/validation-model/inheritance-principle.html + Raises :class:`ValidationError` (``MULTIPLE_INHERITABLE_FILES``) when one level has + several equally good candidates. + """ + # walk_back yields lists only when it searches for several targets; not here. + return tuple(json for json in walk_back(file, inherit=True) if isinstance(json, FileTree)) - for json in walk_back(file, inherit=True): - # May need to overload walk_back - metadata = load_json(json) | metadata # type: ignore[arg-type] +def _merge_sidecars(jsons: t.Iterable[FileTree]) -> dict[str, t.Any]: + """Merge JSON files given nearest first: the nearest file's value of a key wins.""" + metadata: dict[str, t.Any] = {} + for json in jsons: + try: + metadata = load_json(json) | metadata + except ValidationError: + # A sidecar that does not parse is reported when it is checked itself; + # the others still apply, as in the reference validator. + continue return metadata @@ -335,7 +540,26 @@ def walk_back( elif len(file_group) == 1: yield file_group[0] elif file_group: - raise ValidationError(f'Multiple matching files: {file_group}') + # Several candidates at one level: one carrying every entity of the source + # wins, as in the reference validator. Only without one is it ambiguous. + source_entities = FileParts.from_file(source).entities + exact = [ + f + for f in file_group + if all( + FileParts.from_file(f).entities.get(key) == value + for key, value in source_entities.items() + ) + ] + if exact: + yield exact[0] + continue + paths = sorted(f.relative_path for f in file_group) + raise ValidationError( + 'MULTIPLE_INHERITABLE_FILES', + 'Candidate files: ' + ', '.join(paths), + location=paths[0], + ) def _walk_back( @@ -477,21 +701,46 @@ def associations(self) -> ctx.Associations: """Associated files, indexed by suffix, selected according to the inheritance principle.""" return ctx.Associations() - @property - def columns(self) -> Namespace | None: - """TSV columns, indexed by column header, values are arrays with column contents.""" + @cached_property + def columns(self) -> FileContents | None: + """TSV columns, indexed by column header, values are arrays with column contents. + + A table that cannot be read gives empty columns; the integrity checks report + why (they call :meth:`read_columns`, which raises). + """ + try: + return self.read_columns() + except (ValidationError, OSError, UnicodeDecodeError): + return FileContents() + + def read_columns(self) -> FileContents | None: + """Read this file's table, raising :class:`ValidationError` if it is malformed. + + The first :data:`TSV_MAX_ROWS` rows are read. A ``.tsv.gz`` has no header row: + its column names come from the sidecar's ``Columns``, and without them there is + nothing to read. A ``_motion.tsv`` has no header row either (its columns are + named by the ``_channels.tsv`` association, not built yet). None for a file + that is not a table. + """ if self.extension == '.tsv': - return load_tsv(self.file) - elif self.extension == '.tsv.gz': - columns = tuple(self.sidecar.Columns) if self.sidecar else () - return load_tsv_gz(self.file, columns) + if self.suffix == 'motion': + return FileContents() + return load_tsv(self.file, max_rows=TSV_MAX_ROWS) + if self.extension == '.tsv.gz': + headers = self.sidecar.get('Columns') + if not isinstance(headers, list) or not headers or self.size == 0: + return FileContents() + return load_tsv_gz(self.file, tuple(str(h) for h in headers), max_rows=TSV_MAX_ROWS) return None - @property - def json(self) -> Namespace | None: - """Contents of the current JSON file.""" + @cached_property + def json(self) -> FileContents | None: + """Contents of the current JSON file; empty when it cannot be read.""" if self.file_parts.extension == '.json': - return Namespace(load_json(self.file)) + try: + return load_json(self.file) + except (ValidationError, OSError): + return FileContents() return None @@ -504,9 +753,15 @@ def gzip(self) -> ctx.Gzip | None: @cached_property def nifti_header(self) -> ctx.NiftiHeader | None: - """Parsed contents of NIfTI header referenced elsewhere in schema.""" + """Parsed contents of NIfTI header referenced elsewhere in schema. + + None when the header cannot be read; the integrity checks report it. + """ if self.extension in ('.nii', '.nii.gz'): - return load_nifti_header(self.file) + try: + return load_nifti_header(self.file) + except NIFTI_LOAD_ERRORS: + return None return None @property @@ -517,27 +772,80 @@ def ome(self) -> None: def tiff(self) -> None: """TIFF file format metadata.""" - @property - def sidecar(self) -> Namespace | None: - """Sidecar metadata constructed via the inheritance principle.""" - sidecar = load_sidecar(self.file) or {} + @cached_property + def sidecar_files(self) -> tuple[FileTree, ...]: + """The JSON files this file inherits metadata from, nearest first. + + Found once per file, because walking up the tree is the costly part, and read + by :attr:`sidecar`, :attr:`sidecar_origins`, the integrity checks (which report + an ambiguous chain) and the inheritance checks (which report a deeper file + changing a value). Kept only as long as this context. + + A JSON file has none: it is a sidecar, it does not have one (as in the + reference validator). Raises :class:`ValidationError` + (``MULTIPLE_INHERITABLE_FILES``) when one level has several equally good + candidates; an error is not cached, so every reader gets it. + """ + if self.extension == '.json': + return () + return _sidecar_files(self.file) - return Namespace(sidecar) + @cached_property + def sidecar_origins(self) -> dict[str, str]: + """Map each key of :attr:`sidecar` to the dataset-relative path of its JSON file. + + A finding about a key's value belongs to the file that holds it, not to every + data file inheriting it. An ambiguous chain gives no origins (its sidecar is + empty too). + """ + try: + files = self.sidecar_files + except ValidationError: + return {} + origins: dict[str, str] = {} + for json in files: + try: + keys = load_json(json).keys() + except ValidationError: + continue + for key in keys: + origins.setdefault(key, json.relative_path) # the nearest file wins + return origins + + @cached_property + def sidecar(self) -> FileContents: + """Sidecar metadata constructed via the inheritance principle. + + As in the reference validator: a JSON file has no sidecar (it is one), so its + sidecar is empty; a sidecar that cannot be read leaves the sidecar empty (the + problem is reported on the JSON file itself); and ``RepetitionTime`` is + rounded to milliseconds, so a header and a sidecar that differ only by float + noise do not disagree. + """ + try: + sidecar = FileContents(_merge_sidecars(self.sidecar_files)) + except (ValidationError, OSError): + return FileContents() + repetition_time = sidecar.get('RepetitionTime') + if isinstance(repetition_time, (int, float)) and not isinstance(repetition_time, bool): + # JavaScript's Math.round (half up), not Python's round (half to even). + sidecar['RepetitionTime'] = math.floor(repetition_time * 1000 + 0.5) / 1000 + return sidecar def exists(self, arg: str | list[str] | None, rule: str | None = 'dataset') -> int: """Count of files in an array that exist in the dataset. String is array with length 1.""" if arg is None: return 0 - prefix = UPath() + prefix = '' fileTree: FileTree = self.file.parent if rule == 'file' else self.dataset.tree # type: ignore[assignment] if rule == 'stimuli': - prefix /= 'stimuli' + prefix = 'stimuli' elif rule == 'subject': if 'sub' not in self.entities: return 0 - prefix /= f'sub-{self.entities["sub"]}' + prefix = f'sub-{self.entities["sub"]}' if isinstance(arg, str): arg = [arg] @@ -572,7 +880,7 @@ def exists(self, arg: str | list[str] | None, rule: str | None = 'dataset') -> i return dataset_files + other_files else: - return sum(str(prefix / file) in fileTree for file in arg) + return sum(tree_contains(fileTree, f'{prefix}/{file}') for file in arg) class Sessions: @@ -600,7 +908,10 @@ def session_id(self) -> list[str] | None: @staticmethod def _get_session_id(phenotype_file: FileTree) -> list[str] | None: - columns = load_tsv(phenotype_file) + try: + columns = load_tsv(phenotype_file) + except (ValidationError, OSError, UnicodeDecodeError): + return None # reported when the table itself is checked if 'session_id' not in columns: return None return list(columns['session_id']) diff --git a/src/bids_validator/expression_language.py b/src/bids_validator/expression_language.py index 5b342da..1d78a23 100644 --- a/src/bids_validator/expression_language.py +++ b/src/bids_validator/expression_language.py @@ -1,20 +1,133 @@ -"""Implementation of the BIDS schema expression language.""" +"""Implementation of the BIDS schema expression language. +The schema writes its rules as small expressions, such as ``size > 150`` or +``"Units" in sidecar``. ``bidsschematools.expressions.parse`` turns the text into a +tree; :func:`evaluate` walks that tree against a file's context and returns a value. + +The language is defined in the schema's own documentation (``src/schema/README.md`` +in bids-specification, section "Expressions") and pinned by the test vectors in +``schema.meta.expression_tests``. Where the two disagree, the test vectors win, +because they are what every implementation is checked against. + +Three properties of the language are easy to get wrong in Python: + +* **Missing values are null.** ``sidecar.MissingValue`` is ``null``, and ``null`` + propagates through most operations (``null + 1`` is ``null``) instead of raising. +* **Truth follows JavaScript.** An empty array or object is true; ``null``, ``0``, + ``NaN`` and the empty string are false. See :func:`truthy`. +* **Values are plain.** The language knows null, booleans, numbers, strings, arrays + and objects. The context hands over numpy arrays (NIfTI headers) and tuples (TSV + columns), so every value read from the context goes through :func:`normalize`. +""" + +import math import operator import re -from collections.abc import Sequence +from collections.abc import Callable, Mapping, Sequence +from functools import lru_cache from typing import Any, TypeVar from typing import Literal as L +import numpy as np + +# The schema's own parser: ``bst_expr.parse('size > 150')`` returns a tree of nodes +# (``BinOp(op='>', lh='size', rh=150)``) that :func:`evaluate` walks. from bidsschematools import expressions as bst_expr from bids_validator.context import Context +# "Some value type": in ``count_(array: list[T], val: T)`` the value counted has the type +# of the array's items. Only used by type hints. T = TypeVar('T', bound=str | int | float) -def filter_strs(arg: list[Any]) -> list[int | float]: - """Filter non-numeric values from a list. +def truthy(value: Any) -> bool: + """Return whether a value counts as true in the expression language. + + The rules follow JavaScript, which the schema's reference implementation uses, + and differ from Python's in two ways: an empty array or object is TRUE, and + ``NaN`` is FALSE. + + Parameters + ---------- + value : Any + Any value an expression can produce + + Returns + ------- + bool + False for null, false, 0, NaN and the empty string; True for everything else + + """ + if value is None or value is False: + return False + if isinstance(value, (int, float)): + return not (value == 0 or math.isnan(value)) + if isinstance(value, str): + return value != '' + return True + + +def normalize(value: Any) -> Any: + """Convert a value read from the context into a plain expression-language value. + + Loaders return numpy arrays and numpy scalars (``nifti_header.dim``) and tuples + (TSV columns). The language only knows arrays, so both become lists, and numpy + numbers become Python numbers. Without this, ``type(nifti_header.dim)`` is not + ``"array"`` and comparing a column to a list literal is always false. + + Parameters + ---------- + value : Any + A value as the context returned it + + Returns + ------- + Any + The same value, with tuples and numpy arrays as lists and numpy scalars as + Python numbers + + """ + if isinstance(value, np.generic): + return value.item() + if isinstance(value, np.ndarray): + return value.tolist() + if isinstance(value, tuple): + return [normalize(item) for item in value] + return value + + +def as_number(val: Any) -> int | float | None: + """Coerce a value to a number, or return None if it is not one. + + Numeric strings count, because TSV cells are read as text: the onset column of + an events file arrives as ``["0.5", "1.0", "n/a"]``. + + Parameters + ---------- + val : Any + value to coerce + + Returns + ------- + int | float | None + The number, or None for null, non-numeric strings and other types + + """ + if isinstance(val, bool): + return int(val) + if isinstance(val, (int, float)): + return val + if isinstance(val, str): + try: + return float(val) + except ValueError: + return None + return None + + +def numeric_values(arg: list[Any]) -> list[int | float]: + """Keep the numeric values of a list, converting numeric strings. Parameters ---------- @@ -24,10 +137,11 @@ def filter_strs(arg: list[Any]) -> list[int | float]: Returns ------- list - list containing just numeric (i.e. int and float) values + list containing just numeric values, with ``n/a`` and NaN dropped """ - return [val for val in arg if isinstance(val, (int, float))] + numbers = (as_number(val) for val in arg) + return [num for num in numbers if num is not None and not math.isnan(num)] def is_numeric(val: Any) -> bool: @@ -52,8 +166,42 @@ def is_numeric(val: Any) -> bool: def in_(a: Any, b: Any) -> bool | None: - """Object lookup, true if RHS is a subfield of LHS.""" - return operator.contains(b, a) if b is not None else None + """Object lookup, true if LHS is a key of the RHS object. + + The schema also uses ``in`` on arrays (``"micr" in dataset.modalities``), meaning + "is a member of", so arrays and strings test membership. Any other object, such + as the ``associations`` record, tests for a field that is present and not null. + + Parameters + ---------- + a : Any + The key or value to look for + b : Any + The object, array or string to look in + + Returns + ------- + bool | None + null if ``b`` is null, otherwise whether ``a`` was found + + """ + if b is None: + return None + if isinstance(b, Mapping): + # Test the keys view, not the mapping: a bidsschematools Namespace reads a + # dotted key as a path, and ``".git" in namespace`` inserts an empty key. + try: + return a in b.keys() # noqa: SIM118 + except TypeError: # an unhashable key + return False + if isinstance(b, (list, str)): + try: + return a in b + except TypeError: # a non-string looked up in a string + return False + if isinstance(a, str): + return getattr(b, a, None) is not None + return False def allequal_(array1: list[object] | None, array2: list[object] | None) -> bool: @@ -81,22 +229,24 @@ def allequal_(array1: list[object] | None, array2: list[object] | None) -> bool: return all(a == b for a, b in zip(array1, array2, strict=True)) -def count_(array: list[T], val: T) -> int: +def count_(array: list[T] | None, val: T | None) -> int | None: """Count the number of times a value is found in an array. Parameters ---------- - array : list + array : list | None Array of elements val : Any value to count Returns ------- - int - Number of elements in an array equal to val + int | None + Number of elements in an array equal to val, null if either argument is null """ + if not isinstance(array, list) or val is None: + return None return array.count(val) @@ -125,9 +275,13 @@ def index_(arg: Sequence[T] | None, val: T | None) -> int | None: return None -def intersects_(a: list[T] | None, b: list[T] | None) -> list[T] | bool: +def intersects_(a: Any, b: Any) -> list[Any] | bool: """Find the common values in two arrays. + A single value is treated as an array of one, as the reference implementation + does, so ``intersects(sidecar.Units, ["rad"])`` works whether ``Units`` holds a + string or a list. + Parameters ---------- a : list | None @@ -144,7 +298,11 @@ def intersects_(a: list[T] | None, b: list[T] | None) -> list[T] | bool: if a is None or b is None: return False - res = list(set(a) & set(b)) + a = a if isinstance(a, list) else [a] + b = b if isinstance(b, list) else [b] + + # Membership by ``==`` rather than a set, so unhashable elements cannot raise. + res = [item for item in a if item in b] if res: return res @@ -152,27 +310,27 @@ def intersects_(a: list[T] | None, b: list[T] | None) -> list[T] | bool: return False -def length_(arg: list[object] | None) -> int | None: - """Find the number of elements in an array. +def length_(arg: Any) -> int | None: + """Find the number of elements in an array, or characters in a string. Parameters ---------- - arg : list | None - An Array + arg : list | str | None + An Array or a string Returns ------- int | None - Number of elements in the array + Number of elements, null for anything that is not an array or a string """ - if arg is not None: + if isinstance(arg, (list, str)): return len(arg) return None -def match_(arg: str | None, pattern: str | None) -> bool | None: +def match_(arg: Any, pattern: Any) -> bool | None: """Evaluate whether a regular expression pattern appears in a string. Parameters @@ -185,7 +343,8 @@ def match_(arg: str | None, pattern: str | None) -> bool | None: Returns ------- bool | None - true if arg matches the regular expression pattern (anywhere in string) + true if arg matches the regular expression pattern (anywhere in string), + null if arg is null or the pattern is not a valid regular expression """ if arg is None: @@ -193,11 +352,14 @@ def match_(arg: str | None, pattern: str | None) -> bool | None: elif pattern is None: return False - res = re.search(pattern, arg) + try: + res = re.search(str(pattern), str(arg)) + except re.error: + return None return res is not None -def max_(arg: int | float | list[int | float] | None) -> int | float | None: +def max_(arg: int | float | list[Any] | None) -> int | float | None: """Find the largest numeric value in an array. Parameters @@ -208,19 +370,20 @@ def max_(arg: int | float | list[int | float] | None) -> int | float | None: Returns ------- int | float | None - The largest non-n/a value in an array + The largest non-n/a value in an array; minus infinity if there is none, as in + the reference implementation """ if arg is None: return None - if isinstance(arg, (float, int)): - return arg + if not isinstance(arg, list): + arg = [arg] - return max(filter_strs(arg)) + return max(numeric_values(arg), default=-math.inf) -def min_(arg: int | float | list[int | float] | None) -> int | float | None: +def min_(arg: int | float | list[Any] | None) -> int | float | None: """Find the smallest numeric value in an array. Parameters @@ -231,19 +394,20 @@ def min_(arg: int | float | list[int | float] | None) -> int | float | None: Returns ------- int | float | None - The smallest non-n/a value in an array + The smallest non-n/a value in an array; infinity if there is none, as in the + reference implementation """ if arg is None: return None - if isinstance(arg, (float, int)): - return arg + if not isinstance(arg, list): + arg = [arg] - return min(filter_strs(arg)) + return min(numeric_values(arg), default=math.inf) -def sorted_(arg: list[T], method: L['lexical', 'numeric'] | None = None) -> list[T]: +def sorted_(arg: list[T] | None, method: L['lexical', 'numeric'] | None = None) -> list[T] | None: """Return sorted input array. Defaults to type-determined sort. @@ -251,17 +415,20 @@ def sorted_(arg: list[T], method: L['lexical', 'numeric'] | None = None) -> list Parameters ---------- - arg : list + arg : list | None Array to sort method : str | None, optional Method to sort by, can be "lexical" or "numeric", by default None Returns ------- - list - The sorted values of the input array + list | None + The sorted values of the input array, null if the input is not an array """ + if not isinstance(arg, list): + return None + contains_str = any(isinstance(x, str) for x in arg) if method is None: @@ -277,7 +444,7 @@ def sorted_(arg: list[T], method: L['lexical', 'numeric'] | None = None) -> list non_numeric_vals = {i: d for i, d in enumerate(arg) if not is_numeric(d)} if non_numeric_vals: numeric_vals = [d for d in arg if is_numeric(d)] - sorted_vals = sorted_(numeric_vals, method='numeric') + sorted_vals = sorted_(numeric_vals, method='numeric') or [] for key, val in non_numeric_vals.items(): sorted_vals.insert(key, val) @@ -287,7 +454,7 @@ def sorted_(arg: list[T], method: L['lexical', 'numeric'] | None = None) -> list return sorted(arg, key=float) -def substr_(arg: str | None, start: int | None, end: int | None) -> str | None: +def substr_(arg: Any, start: int | None, end: int | None) -> str | None: """Extract a sub-string from an existing string. Parameters @@ -306,7 +473,7 @@ def substr_(arg: str | None, start: int | None, end: int | None) -> str | None: """ if arg is not None and start is not None and end is not None: - return arg[start:end] + return str(arg)[start:end] return None @@ -322,20 +489,23 @@ def type_(var: Any) -> str: Returns ------- str - The name of the type, including "array", "object", "null" + One of "null", "boolean", "number", "string", "array" or "object". Anything + that is not one of the first five, such as a parsed NIfTI header, is an object. """ - match var: + match normalize(var): + case None: + return 'null' case bool(): return 'boolean' case int() | float(): return 'number' + case str(): + return 'string' case list(): return 'array' - case dict(): - return 'object' case _: - return 'null' + return 'object' def unique_(arg: list[T] | None) -> list[T] | None: @@ -360,22 +530,109 @@ def unique_(arg: list[T] | None) -> list[T] | None: return None -# Available Binary operations -bin_ops = { - '+': operator.add, - '-': operator.sub, - '*': operator.mul, - '/': operator.truediv, - '%': operator.mod, +def _null_propagating( + func: Callable[[Any, Any], Any], +) -> Callable[[Any, Any], Any]: + """Wrap a numeric operator so it follows the language's rules. + + ``null`` on either side gives ``null`` (``null + 1`` is ``null``), and an operand + that is not a number gives ``null`` instead of raising. Numeric strings count as + numbers, as they do in the reference implementation. + """ + + def apply(a: Any, b: Any) -> Any: + if a is None or b is None: + return None + x, y = as_number(a), as_number(b) + if x is None or y is None: + return None + return func(x, y) + + return apply + + +def _add(a: Any, b: Any) -> Any: + """Add two numbers, or join two strings when either side is a string.""" + if a is None or b is None: + return None + if isinstance(a, str) or isinstance(b, str): + return f'{a}{b}' + return _null_propagating(operator.add)(a, b) + + +def _divide(a: int | float, b: int | float) -> float: + """Divide, giving infinity or NaN for a zero divisor, as the reference does.""" + if b == 0: + return math.nan if a == 0 or math.isnan(a) else math.copysign(math.inf, a) + return a / b + + +def _modulo(a: int | float, b: int | float) -> int | float: + """Remainder with the sign of the dividend, as the reference does (``-3 % 2 == -1``).""" + if b == 0: + return math.nan + result = math.fmod(a, b) + return int(result) if isinstance(a, int) and isinstance(b, int) else result + + +def _power(a: int | float, b: int | float) -> int | float: + """Raise to a power, giving infinity or NaN where Python would raise or go complex.""" + try: + result = a**b + except (ZeroDivisionError, OverflowError): # 0 ** -1, 10.0 ** 1000 + return math.inf + if isinstance(result, complex): # (-8) ** 0.5 + return math.nan + return result + + +def _compare(func: Callable[[Any, Any], bool]) -> Callable[[Any, Any], bool]: + """Wrap an ordering operator (``<``, ``<=``, ``>``, ``>=``). + + The schema's documentation states that comparing with ``null`` is false, like + ``null == 1``. Two strings compare as text; anything else compares as numbers, + and a value that is not a number makes the comparison false. + """ + + def compare(a: Any, b: Any) -> bool: + if a is None or b is None: + return False + if isinstance(a, str) and isinstance(b, str): + return func(a, b) + x, y = as_number(a), as_number(b) + if x is None or y is None: + return False + return func(x, y) + + return compare + + +# The binary operators the language has, from the operator's text to the function that +# computes it: ``evaluate`` looks up ``bin_ops['>']`` for ``size > 150`` and calls it +# with both sides. ``&&`` and ``||`` are not here: ``evaluate`` handles them itself, +# because they must not evaluate their right side when the left decides. +bin_ops: dict[str, Callable[[Any, Any], Any]] = { + '+': _add, + '-': _null_propagating(operator.sub), + '*': _null_propagating(operator.mul), + '/': _null_propagating(_divide), + '%': _null_propagating(_modulo), + '**': _null_propagating(_power), '==': operator.eq, '!=': operator.ne, + '<': _compare(operator.lt), + '<=': _compare(operator.le), + '>': _compare(operator.gt), + '>=': _compare(operator.ge), 'in': in_, } -# Available Right operations -right_ops = {'!': operator.not_} +# The one prefix operator, ``!`` (not), decided by the language's truth test. +right_ops: dict[str, Callable[[Any], Any]] = {'!': lambda value: not truthy(value)} -# Available functions +# The functions an expression may call, from their name in the schema to the Python +# function. Each Python name ends in ``_`` so it does not hide a Python built-in +# (``max_`` is the schema's ``max``, ``max`` stays Python's). functions = { 'allequal': allequal_, 'count': count_, @@ -391,33 +648,59 @@ def unique_(arg: list[T] | None) -> list[T] | None: 'unique': unique_, } +# The language's three constant words, and the Python values they stand for. vals = { 'true': True, 'false': False, 'null': None, } +# Every name an expression can use that is not about a file: the constants and the +# functions. ``interpret`` puts it in front of the file's context, so ``length`` is the +# function and ``sidecar`` is the file's sidecar. el_namespace = vals | functions +# A unique "not found" marker for ``LookupProxy``: unlike None, no context attribute can +# ever be this object, so "absent" and "present but null" stay different. +_MISSING = object() + class LookupProxy: - """Lookup proxy for context class.""" + """Lookup proxy for context class. + + Looks a name up in each of several objects in turn (first ``el_namespace``, then the + file's context) and returns the first match, so an expression sees one namespace. + """ - def __init__(self, *objs: dict[str, Any] | Context) -> None: + def __init__(self, *objs: Mapping[str, Any] | Context) -> None: self.objs = objs def __getitem__(self, key: str) -> Any: for obj in self.objs: - if isinstance(obj, dict) and key in obj: - return obj[key] - if hasattr(obj, key): - return getattr(obj, key) - raise AttributeError + if isinstance(obj, Mapping): + if key in obj: + return obj[key] + continue + # One getattr, not hasattr then getattr: a context property can do real + # work (load a sidecar), and hasattr would do it twice. + value = getattr(obj, key, _MISSING) + if value is not _MISSING: + return value + raise AttributeError(key) # Provide either attr or item lookup __getattr__ = __getitem__ +def _lookup(container: Any, key: Any) -> Any: + """Read a field from an object, or null if it has no such field.""" + if isinstance(container, Mapping): + return container.get(key) + if isinstance(key, str): + return getattr(container, key, None) + return None + + def evaluate(expr: bst_expr.ASTNode | float | str, namespace: object) -> Any: """Evaluate an expression, with a provided namespace for variable lookup.""" match expr: @@ -426,32 +709,49 @@ def evaluate(expr: bst_expr.ASTNode | float | str, namespace: object) -> Any: case str() if expr[0] == expr[-1] and expr[0] in ('"', "'"): return expr[1:-1] case str(): - if isinstance(namespace, dict): - return namespace.get(expr, None) - return getattr(namespace, expr) + # A name the context does not define is null, like any missing value. + if isinstance(namespace, Mapping): + return normalize(namespace.get(expr)) + try: + return normalize(getattr(namespace, expr)) + except AttributeError: + return None case bst_expr.Object(): return {} case bst_expr.Array(elements=elements): return [evaluate(el, namespace) for el in elements] case bst_expr.Element(name=name, index=index): - el_name = evaluate(name, namespace) - if el_name is not None: - return el_name[evaluate(index, namespace)] + target = evaluate(name, namespace) + key = evaluate(index, namespace) + if isinstance(target, (list, str)): + # Out of range (or negative) is null, as in the reference implementation, + # rather than an error or Python's count-from-the-end. + if isinstance(key, int) and not isinstance(key, bool) and 0 <= key < len(target): + return normalize(target[key]) + return None + if target is None: + return None + return normalize(_lookup(target, key)) case bst_expr.Property(name=name, field=field): base_object = evaluate(name, namespace) - if isinstance(base_object, dict): - return base_object.get(field, None) - return getattr(base_object, field, None) + if base_object is None: + return None + return normalize(_lookup(base_object, field)) case bst_expr.Function(name=name, args=args): func = evaluate(name, namespace) + if not callable(func): + raise TypeError(f'Unknown function {name!r}') return func(*[evaluate(arg, namespace) for arg in args]) case bst_expr.BinOp(op=op, lh=lh, rh=rh): match op: - # Short-circuiting operators + # Short-circuiting operators return one of their operands, as in the + # reference implementation: ``null && true`` is null, not false. case '||': - return evaluate(lh, namespace) or evaluate(rh, namespace) + left = evaluate(lh, namespace) + return left if truthy(left) else evaluate(rh, namespace) case '&&': - return evaluate(lh, namespace) and evaluate(rh, namespace) + left = evaluate(lh, namespace) + return evaluate(rh, namespace) if truthy(left) else left case _: return bin_ops[op](evaluate(lh, namespace), evaluate(rh, namespace)) case bst_expr.RightOp(op=op, rh=rh): @@ -460,8 +760,14 @@ def evaluate(expr: bst_expr.ASTNode | float | str, namespace: object) -> Any: raise ValueError(f'Expression type {type(expr)} not supported.') +@lru_cache(maxsize=4096) +def parse(rule: str) -> bst_expr.ASTNode: + """Parse an expression once; the same rule text is evaluated for every file.""" + return bst_expr.parse(rule) + + def interpret(rule: str, context: Context) -> Any: """Interpret a rule from the schema in a given file context.""" namespace = LookupProxy(el_namespace, context) - expr = bst_expr.parse(rule) + expr = parse(rule) return evaluate(expr, namespace) diff --git a/src/bids_validator/issues.py b/src/bids_validator/issues.py new file mode 100644 index 0000000..6d6d6e0 --- /dev/null +++ b/src/bids_validator/issues.py @@ -0,0 +1,238 @@ +"""What a finding is: the data model of everything the validator reports. + +This module holds data only. It reads no file and no schema. The checks that produce +findings, and the codes each of them can report, live in the check modules of +:mod:`bids_validator.checks` (``integrity``, ``inheritance``, ``filenames``, +``expressions``, ``fields``, ``tables``). + +Three types: + +* :class:`IssueDefinition` is a KIND of finding: a code, how serious it is, and the + general reason it exists. ``EMPTY_FILE`` is one. A check module either reads its + definitions from the schema (:meth:`IssueDefinition.from_schema`) or, for a code the + schema does not define, lists it beside the checks that report it. +* :class:`Issue` is one finding: a definition met at one place in a dataset, with a + message about that place. :meth:`IssueDefinition.make_issue` makes one. +* :class:`DatasetIssues` is every finding of one validation, in order. + +The shape follows the reference validator (bids-validator, ``src/types/issues.ts``), +so a report can be compared with its output. +""" + +# Lets a type hint name a class defined further down this file (``IssueDefinition`` +# returns ``Issue``, ``from_level`` returns ``Severity``) without quoting it. +from __future__ import annotations + +from collections.abc import Iterable, Iterator, Mapping +from enum import Enum +from typing import Any + +# attrs writes the boring parts of a data class for us (``__init__``, ``__repr__``, +# ``__eq__``) from the attribute list; see the decorators on the classes below. +import attrs + +# The names other modules are meant to import from this one. +__all__ = ['DatasetIssues', 'Issue', 'IssueDefinition', 'Severity'] + + +class Severity(str, Enum): + """How serious a finding is. + + ``WARNING`` asks for attention; ``ERROR`` means the dataset is not valid BIDS. + Subclassing ``str`` keeps the values JSON-friendly: a member serialises directly + to ``'warning'`` or ``'error'``. + """ + + WARNING = 'warning' + ERROR = 'error' + + @classmethod + def from_level(cls, level: str) -> Severity: + """Return the severity for a schema issue ``level``. + + The schema writes ``level: warning`` or ``level: error``. Anything other than + ``warning`` is treated as an error, so an unexpected value is never silently + downgraded. + """ + return cls.WARNING if level == 'warning' else cls.ERROR + + +# ``attrs.define`` turns the annotated attributes below into a class with an +# ``__init__(code, severity=..., ...)``; ``kw_only=True`` means every argument must be +# given by name (``Issue(code='X', location='a')``), so they cannot be swapped by mistake. +@attrs.define(kw_only=True) +class Issue: + """One finding: a problem met at one place in a dataset. + + Attributes + ---------- + code + Stable identifier of the kind of problem, the reference validator's + (for example ``'FILENAME_MISMATCH'``). + severity + How serious the finding is. Defaults to :attr:`Severity.ERROR`. + location + Dataset-relative path of the file the finding is about, without a leading + ``/``; ``None`` for a finding about the dataset as a whole. + message + What is wrong at this place, for a person to read. + sub_code + Optional finer category within ``code``, such as the name of a field. + 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.frozen`` is ``attrs.define`` with the attributes made read-only: a definition +# is shared by every finding of its kind, so nothing may change it. +@attrs.frozen +class IssueDefinition: + """A kind of finding: a code, its severity, and why it is a problem. + + A check reports one by making an :class:`Issue` from it, with the place it found it:: + + EMPTY_FILE.make_issue('sub-01/anat/sub-01_T1w.nii.gz') + + Attributes + ---------- + code + The identifier every finding of this kind carries. + severity + How serious every finding of this kind is. + reason + Why this kind of finding is a problem, in general. A finding whose check has + nothing more specific to say uses it as its message. + + """ + + code: str + severity: Severity + reason: str + + # A classmethod is called on the class, not on an instance: + # ``IssueDefinition.from_schema(block)``. It is a second way to build one. + @classmethod + def from_schema( + cls, block: Mapping[str, Any], *, severity: Severity | None = None + ) -> IssueDefinition: + """Build a definition from an issue block the schema writes. + + The schema writes the same three keys wherever it defines an issue: in + ``rules.errors``, in each ``rules.checks`` rule, and on some fields of the + ``rules.sidecars`` rules:: + + code: README_FILE_SMALL + level: warning + message: | + The recommended file '/README' is very small. ... + + Parameters + ---------- + block : Mapping + The issue block. + severity : Severity, optional + Used instead of the block's ``level``. A field's issue block has no level + of its own: the field's requirement level decides how serious it is. + + Returns + ------- + IssueDefinition + The definition. The YAML message, written over several lines, becomes one. + + """ + if severity is None: + severity = Severity.from_level(str(block.get('level', ''))) + return cls( + code=str(block['code']), + severity=severity, + reason=' '.join(str(block.get('message', '')).split()), + ) + + def make_issue( + self, + location: str | None, + *, + message: str | None = None, + sub_code: str | None = None, + rule: str | None = None, + ) -> Issue: + """Return a finding of this kind, found at ``location``. + + Parameters + ---------- + location : str or None + Dataset-relative path of the file, without a leading ``/``; ``None`` for + the dataset as a whole. + message : str, optional + What is wrong at this place. Defaults to the definition's general reason. + sub_code, rule : str, optional + Copied onto the finding (see :class:`Issue`). + + Returns + ------- + Issue + The finding. + + """ + return Issue( + code=self.code, + severity=self.severity, + location=location, + message=self.reason if message is None else message, + sub_code=sub_code, + rule=rule, + ) + + +# ``attrs.define`` again; ``attrs.field(factory=list)`` gives every new container its own +# empty list (a plain ``= []`` default would be one list shared by all of them). +@attrs.define +class DatasetIssues: + """Every finding of one validation, in the order the checks reported them. + + A thin wrapper over a list, so a report has a stable container that is easy to + extend (filtering, severity rollup) without changing the code that fills 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] + + # A property is read like an attribute, without brackets: ``issues.has_errors``. + @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) + + # These two make the container behave like a list: ``for issue in issues`` and + # ``len(issues)`` work on it directly. + def __iter__(self) -> Iterator[Issue]: + return iter(self.issues) + + def __len__(self) -> int: + return len(self.issues) diff --git a/tests/checks/__init__.py b/tests/checks/__init__.py new file mode 100644 index 0000000..1892f77 --- /dev/null +++ b/tests/checks/__init__.py @@ -0,0 +1 @@ +"""Tests for bids_validator.checks.""" diff --git a/tests/checks/test_codes.py b/tests/checks/test_codes.py new file mode 100644 index 0000000..b4417d5 --- /dev/null +++ b/tests/checks/test_codes.py @@ -0,0 +1,118 @@ +"""Tests for where the codes are defined: in the schema, or beside the checks that report them. + +The rule is one place per code. A code the schema defines (in ``rules.errors``) is read +from the schema; any other code is written out once, as an ``IssueDefinition``, in the +check module that reports it. +""" + +import re +from pathlib import Path +from types import ModuleType + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator import context +from bids_validator.checks import ( + expressions, + fields, + filenames, + inheritance, + integrity, + tables, +) +from bids_validator.issues import IssueDefinition + +CHECK_MODULES = ( + integrity, + inheritance, + filenames, + expressions, + fields, + tables, +) + + +def definitions(module: ModuleType) -> dict[str, IssueDefinition]: + """The definitions a module writes out, by the name they are given there.""" + return { + name: value for name, value in vars(module).items() if isinstance(value, IssueDefinition) + } + + +def schema_codes(schema: Namespace) -> set[str]: + errors = schema['rules']['errors'] + return {str(errors[name]['code']) for name in errors} + + +@pytest.mark.parametrize('module', CHECK_MODULES, ids=lambda module: module.__name__) +def test_each_definition_is_named_after_its_code(module: ModuleType) -> None: + for name, definition in definitions(module).items(): + assert name == definition.code + assert definition.reason + + +def test_no_code_is_defined_twice() -> None: + codes = [ + definition.code for module in CHECK_MODULES for definition in definitions(module).values() + ] + assert len(codes) == len(set(codes)) + + +def test_no_code_the_schema_defines_is_written_out(schema: Namespace) -> None: + """A code in rules.errors must be read from the schema, not copied into a module.""" + written = { + definition.code for module in CHECK_MODULES for definition in definitions(module).values() + } + assert not written & schema_codes(schema) + + +def test_each_module_writes_out_the_codes_it_reports() -> None: + assert set(definitions(filenames)) == { + 'ENTITY_WITH_NO_LABEL', + 'INVALID_ENTITY_LABEL', + 'MISSING_REQUIRED_ENTITY', + 'ENTITY_NOT_IN_RULE', + 'MISSING_DATATYPE', + 'DATATYPE_MISMATCH', + 'EXTENSION_MISMATCH', + 'INVALID_LOCATION', + 'FILENAME_MISMATCH', + 'ALL_FILENAME_RULES_HAVE_ISSUES', + 'CASE_COLLISION', + } + assert set(definitions(expressions)) == {'CHECK_ERROR'} + assert set(definitions(inheritance)) == {'SIDECAR_FIELD_OVERRIDE'} + assert set(definitions(fields)) == { + 'SIDECAR_KEY_REQUIRED', + 'SIDECAR_KEY_RECOMMENDED', + 'JSON_KEY_REQUIRED', + 'JSON_KEY_RECOMMENDED', + } + + +def test_the_schema_entries_the_modules_read_exist(schema: Namespace) -> None: + """Every ``rules.errors`` entry a module reads by name is in the schema.""" + read = set() + for module in CHECK_MODULES: + source = Path(str(module.__file__)).read_text() + read |= set(re.findall(r"\['errors'\]\['(\w+)'\]", source)) + read |= set(re.findall(r"_schema_error\(\s*\w+(?:\.\w+)*,\s*'(\w+)'", source)) + read |= set(integrity.LOADER_ERRORS_IN_SCHEMA.values()) + assert read == { + 'NotIncluded', + 'EmptyFile', + 'NiftiHeaderUnreadable', + 'JsonInvalid', + 'InvalidJsonEncoding', + 'JsonSchemaValidationError', + } + assert read <= set(schema['rules']['errors'].keys()) + + +def test_every_code_a_loader_raises_is_reported() -> None: + """The context's loaders raise codes; integrity must know each one.""" + source = Path(str(context.__file__)).read_text() + raised = set(re.findall(r"ValidationError\(\s*'([A-Z_]+)'", source)) + known = set(integrity.LOADER_ERRORS) | set(integrity.LOADER_ERRORS_IN_SCHEMA) + assert raised == known diff --git a/tests/checks/test_collect_issues.py b/tests/checks/test_collect_issues.py new file mode 100644 index 0000000..4b74767 --- /dev/null +++ b/tests/checks/test_collect_issues.py @@ -0,0 +1,103 @@ +"""Tests for running every check in one walk.""" + +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator import context as context_module +from bids_validator.checks import DATASET_CHECKS, FILE_CHECKS, FOLDER_CHECKS, collect_issues +from bids_validator.checks.filenames import filename_issues, folder_issues +from bids_validator.checks.inheritance import inheritance_issues +from bids_validator.checks.integrity import dataset_integrity_issues, integrity_issues +from bids_validator.checks.rules import rule_issues +from bids_validator.context import Context +from bids_validator.issues import Issue +from bids_validator.types.files import FileTree + +from ..helpers import build, clean_dataset, findings, write + + +def test_the_order_of_the_checks() -> None: + """Reading problems first, then the name, then the schema rules: the reference order.""" + assert DATASET_CHECKS == (dataset_integrity_issues,) + assert FOLDER_CHECKS == (folder_issues,) + assert FILE_CHECKS == (integrity_issues, inheritance_issues, filename_issues, rule_issues) + + +def test_one_walk_reports_what_each_check_reports( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Running the checks together loses nothing and adds nothing.""" + build( + tmp_path, + 'sub-01/anat/sub-01_T1w.nii.gz', + 'sub-01/anat/sub-01_T1w.nii', # DUPLICATE_FILES (a rule) + 'sub-01/func/sub-01_bold.nii.gz', # MISSING_REQUIRED_ENTITY (a filename check) + 'sub-01/foo/', # NOT_INCLUDED (a folder check) + ) # empty images: EMPTY_FILE and NIFTI_HEADER_UNREADABLE (integrity) + together = sorted(findings(tmp_path, schema)) + apart = sorted( + finding + for check in FOLDER_CHECKS + FILE_CHECKS + for finding in findings(tmp_path, schema, check) + ) + assert together == apart + codes = {code for code, _ in together} + assert {'MISSING_REQUIRED_ENTITY', 'DUPLICATE_FILES', 'EMPTY_FILE', 'NOT_INCLUDED'} <= codes + + +def test_folders_get_the_folder_checks_and_files_the_file_checks( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """A plain folder is never given to a file check; a directory recording is a file.""" + build(tmp_path, 'sub-01/foo/', 'sub-01/meg/sub-01_task-x_meg.ds/x.meg4') + tree = FileTree.read_from_filesystem(str(tmp_path)) + seen: list[str] = [] + + def record(context: Context) -> list[Issue]: + seen.append(context.file.relative_path) + return [] + + collect_issues(tree, schema, dataset_checks=(), folder_checks=(), file_checks=(record,)) + assert 'sub-01/meg/sub-01_task-x_meg.ds/' in seen + assert 'sub-01/foo/' not in seen + assert 'sub-01/' not in seen + + +def test_each_file_s_inheritance_chain_is_found_once( + tmp_path: pathlib.Path, schema: Namespace, monkeypatch: pytest.MonkeyPatch +) -> None: + """The checks that need the JSON files a file inherits from share one search.""" + write( + tmp_path, + { + 'task-rest_bold.json': {'TaskName': 'rest', 'RepetitionTime': 2}, + 'sub-01/func/sub-01_task-rest_bold.json': {'RepetitionTime': 3}, + 'sub-01/func/sub-01_task-rest_bold.nii.gz': b'', + }, + ) + searched: list[str] = [] + search = context_module._sidecar_files + + def counted_search(file: FileTree) -> tuple[FileTree, ...]: + searched.append(file.relative_path) + return search(file) + + monkeypatch.setattr(context_module, '_sidecar_files', counted_search) + issues = collect_issues(FileTree.read_from_filesystem(str(tmp_path)), schema) + assert 'SIDECAR_FIELD_OVERRIDE' in {issue.code for issue in issues} # the chain was read + assert 'sub-01/func/sub-01_task-rest_bold.nii.gz' in searched + assert len(searched) == len(set(searched)) + + +def test_dataset_checks_come_first(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'sub-01/notes.txt') + (tmp_path / 'dataset_description.json').unlink() + issues = collect_issues(FileTree.read_from_filesystem(str(tmp_path)), schema) + assert issues.issues[0].code == 'MISSING_DATASET_DESCRIPTION' + + +def test_a_clean_dataset_is_clean(tmp_path: pathlib.Path, schema: Namespace) -> None: + """The dataset the CLI tests use has no finding at all.""" + assert findings(clean_dataset(tmp_path), schema) == [] diff --git a/tests/checks/test_fields.py b/tests/checks/test_fields.py new file mode 100644 index 0000000..1ea84f2 --- /dev/null +++ b/tests/checks/test_fields.py @@ -0,0 +1,212 @@ +"""Tests for required and recommended JSON fields, their values, and overrides.""" + +import json +import pathlib + +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks import collect_issues +from bids_validator.checks.fields import field_issues +from bids_validator.checks.inheritance import inheritance_issues +from bids_validator.checks.rules import rule_issues +from bids_validator.context import Context, Dataset +from bids_validator.issues import Issue, Severity +from bids_validator.types.files import FileTree + +from ..helpers import FULL_DESCRIPTION, nifti, write + +BOLD = 'sub-01/func/sub-01_task-rest_bold.nii.gz' + + +def rule_findings(root: pathlib.Path, schema: Namespace) -> list[Issue]: + tree = FileTree.read_from_filesystem(str(root)) + return list( + collect_issues( + tree, schema, dataset_checks=(), folder_checks=(), file_checks=(rule_issues,) + ) + ) + + +def keyed(issues: list[Issue]) -> set[tuple[str, str | None, str | None]]: + return {(i.code, i.location, i.sub_code) for i in issues} + + +def test_missing_required_and_recommended_sidecar_fields( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + write(tmp_path, {BOLD: nifti((2, 2, 2, 3)), 'sub-01/func/sub-01_task-rest_bold.json': {}}) + issues = rule_findings(tmp_path, schema) + found = keyed(issues) + assert ('SIDECAR_KEY_REQUIRED', BOLD, 'TaskName') in found + assert ('SIDECAR_KEY_REQUIRED', BOLD, 'RepetitionTime') in found + assert ('SIDECAR_KEY_RECOMMENDED', BOLD, 'Manufacturer') in found + severities = {i.sub_code: i.severity for i in issues if i.code.startswith('SIDECAR_KEY')} + assert severities['TaskName'] is Severity.ERROR + assert severities['Manufacturer'] is Severity.WARNING + + +def test_a_present_field_is_not_reported_missing( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + write( + tmp_path, + { + BOLD: nifti((2, 2, 2, 3)), + 'task-rest_bold.json': {'TaskName': 'rest', 'RepetitionTime': 2.0}, + }, + ) + missing = { + i.sub_code for i in rule_findings(tmp_path, schema) if i.code == 'SIDECAR_KEY_REQUIRED' + } + assert 'TaskName' not in missing + assert 'RepetitionTime' not in missing + + +def test_json_rules_read_the_file_itself(tmp_path: pathlib.Path, schema: Namespace) -> None: + write(tmp_path, {}, description={'Name': 'x', 'BIDSVersion': '1.11.1'}) + found = keyed(rule_findings(tmp_path, schema)) + assert ('JSON_KEY_RECOMMENDED', 'dataset_description.json', 'DatasetType') in found + # Authors carries its own issue in the schema, used instead of JSON_KEY_RECOMMENDED. + assert ('NO_AUTHORS', 'dataset_description.json', 'Authors') in found + + +def test_a_field_can_carry_its_own_issue(tmp_path: pathlib.Path, schema: Namespace) -> None: + """A fieldmap without IntendedFor or B0FieldIdentifier gets the field's own code.""" + write( + tmp_path, + { + 'sub-01/fmap/sub-01_magnitude1.nii.gz': nifti((2, 2, 2)), + 'sub-01/fmap/sub-01_magnitude1.json': {}, + }, + ) + [issue] = [ + i for i in rule_findings(tmp_path, schema) if i.code == 'B0_FIELD_IDENTIFIER_RECOMMENDED' + ] + assert issue.severity is Severity.WARNING # from the field's level: recommended + assert issue.sub_code == 'B0FieldIdentifier' + assert issue.message is not None + assert issue.message.startswith('File is missing B0FieldIdentifier metadata') + + +def test_derivatives_make_fields_optional(tmp_path: pathlib.Path, schema: Namespace) -> None: + """Unless a rule is written for derivatives, as the specification says.""" + write( + tmp_path, + {BOLD: nifti((2, 2, 2, 3)), 'sub-01/func/sub-01_task-rest_bold.json': {}}, + description={**FULL_DESCRIPTION, 'DatasetType': 'derivative'}, + ) + missing = [i for i in rule_findings(tmp_path, schema) if i.code.startswith('SIDECAR_KEY')] + assert missing, 'the derivative rules still apply' + assert all(i.rule and i.rule.startswith('rules.sidecars.derivatives') for i in missing) + + +def test_a_level_addendum_can_raise_the_level(tmp_path: pathlib.Path, schema: Namespace) -> None: + write( + tmp_path, + { + BOLD: nifti((2, 2, 2, 3)), + 'sub-01/func/sub-01_task-rest_bold.json': {'M0Type': 'Estimate'}, + }, + ) + tree = FileTree.read_from_filesystem(str(tmp_path)) + context = Context( + tree / 'sub-01' / 'func' / 'sub-01_task-rest_bold.nii.gz', Dataset(tree, schema), None + ) + rule = { + 'selectors': [], + 'fields': { + 'M0Estimate': { + 'level': 'optional', + 'level_addendum': 'required if `M0Type` is `Estimate`', + } + }, + } + [issue] = field_issues('rules.sidecars.test.Rule', rule, context) + assert (issue.code, issue.severity) == ('SIDECAR_KEY_REQUIRED', Severity.ERROR) + + +def test_a_wrong_sidecar_value_is_reported_once_where_it_is_written( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """A root sidecar shared by two runs: one finding, on the root sidecar.""" + write( + tmp_path, + { + BOLD: nifti((2, 2, 2, 3)), + 'sub-02/func/sub-02_task-rest_bold.nii.gz': nifti((2, 2, 2, 3)), + 'task-rest_bold.json': {'TaskName': 'rest', 'RepetitionTime': -2.0}, + }, + ) + wrong = [ + i for i in rule_findings(tmp_path, schema) if i.code == 'JSON_SCHEMA_VALIDATION_ERROR' + ] + assert [(i.location, i.sub_code, i.message) for i in wrong] == [ + ('task-rest_bold.json', 'RepetitionTime', 'RepetitionTime must be > 0') + ] + + +def test_a_wrong_value_in_a_json_file(tmp_path: pathlib.Path, schema: Namespace) -> None: + write(tmp_path, {}, description={**FULL_DESCRIPTION, 'Authors': 'Ada Lovelace'}) + wrong = [ + i for i in rule_findings(tmp_path, schema) if i.code == 'JSON_SCHEMA_VALIDATION_ERROR' + ] + # Two rules list Authors, and each checks it, as in the reference validator. + assert {(i.location, i.sub_code, i.message) for i in wrong} == { + ('dataset_description.json', 'Authors', 'Authors must be array') + } + + +def test_a_json_file_gets_no_sidecar_rules(tmp_path: pathlib.Path, schema: Namespace) -> None: + """It is a sidecar; it does not have one.""" + write(tmp_path, {BOLD: nifti((2, 2, 2, 3)), 'sub-01/func/sub-01_task-rest_bold.json': {}}) + locations = { + i.location for i in rule_findings(tmp_path, schema) if i.code.startswith('SIDECAR_KEY') + } + assert locations == {BOLD} + + +def overrides(root: pathlib.Path, schema: Namespace) -> list[Issue]: + tree = FileTree.read_from_filesystem(str(root)) + return list( + collect_issues( + tree, schema, dataset_checks=(), folder_checks=(), file_checks=(inheritance_issues,) + ) + ) + + +def test_a_deeper_sidecar_changing_a_value_is_warned( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + write( + tmp_path, + { + BOLD: nifti((2, 2, 2, 3)), + 'task-rest_bold.json': {'RepetitionTime': 2.0}, + 'sub-01/func/sub-01_task-rest_bold.json': {'RepetitionTime': 3.0}, + }, + ) + [issue] = overrides(tmp_path, schema) + assert (issue.code, issue.severity) == ('SIDECAR_FIELD_OVERRIDE', Severity.WARNING) + assert (issue.location, issue.sub_code) == ( + 'sub-01/func/sub-01_task-rest_bold.json', + 'RepetitionTime', + ) + + +def test_the_same_value_twice_is_not_an_override( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Compared by content: the same list written at two levels is fine.""" + filters = {'Notch': {'Frequency': 50}} + write( + tmp_path, + { + BOLD: nifti((2, 2, 2, 3)), + 'task-rest_bold.json': {'SoftwareFilters': filters, 'SliceTiming': [0, 1]}, + 'sub-01/func/sub-01_task-rest_bold.json': { + 'SoftwareFilters': json.loads(json.dumps(filters)), + 'SliceTiming': [0, 1], + }, + }, + ) + assert overrides(tmp_path, schema) == [] diff --git a/tests/checks/test_filename_rules.py b/tests/checks/test_filename_rules.py new file mode 100644 index 0000000..1799c46 --- /dev/null +++ b/tests/checks/test_filename_rules.py @@ -0,0 +1,135 @@ +"""Tests for the filename rules: read from the schema, and which describe a file.""" + +import fnmatch +import json +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks.filename_rules import ( + FilenameRule, + all_filename_rules, + matching_filename_rules, +) +from bids_validator.checks.walk import is_plain_folder, iter_contexts +from bids_validator.context import Context, Dataset +from bids_validator.types.files import FileTree + +from ..helpers import build + + +def rules_by_path(schema: Namespace) -> dict[str, FilenameRule]: + return {rule.path: rule for rule in all_filename_rules(schema)} + + +def contexts(root: pathlib.Path, schema: Namespace) -> dict[str, Context]: + """Every file's context, by dataset-relative path.""" + dataset = Dataset(FileTree.read_from_filesystem(str(root)), schema) + return { + context.file.relative_path: context + for context in iter_contexts(dataset) + if not is_plain_folder(context) + } + + +def test_rules_are_read_into_plain_values(schema: Namespace) -> None: + anat = rules_by_path(schema)['rules.files.raw.anat.nonparametric'] + assert 'T1w' in (anat.suffixes or ()) + assert anat.datatypes == ('anat',) + assert anat.entities is not None + assert 'sub' in anat.entities # short names, as in filenames + assert 'acq' in anat.entities + assert anat.required_entities == ('sub',) + assert anat.fixed_path is None + assert anat.stem is None + + +def test_a_list_the_rule_does_not_have_is_none(schema: Namespace) -> None: + """None means "the rule says nothing", which is not an empty list.""" + description = rules_by_path(schema)['rules.files.common.core.dataset_description'] + assert description.fixed_path == 'dataset_description.json' + assert (description.suffixes, description.entities, description.datatypes) == ( + None, + None, + None, + ) + + +def test_a_file_is_identified_by_suffix_path_or_stem( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + build(tmp_path, 'sub-01/anat/sub-01_T1w.nii.gz', 'participants.tsv', 'README') + found = { + path: [rule.path for rule in matching_filename_rules(c)] + for path, c in contexts(tmp_path, schema).items() + } + assert found['sub-01/anat/sub-01_T1w.nii.gz'] == ['rules.files.raw.anat.nonparametric'] + assert found['dataset_description.json'] == ['rules.files.common.core.dataset_description'] + assert found['participants.tsv'] == ['rules.files.common.tables.participants'] + assert found['README'] == ['rules.files.common.core.README'] + + +def test_a_file_that_is_not_bids_has_no_rule(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'sub-01/notes.txt') + assert matching_filename_rules(contexts(tmp_path, schema)['sub-01/notes.txt']) == () + + +def test_the_rule_for_the_files_folder_is_preferred( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Many datatypes have an events file; the one in func/ is a func events file.""" + build(tmp_path, 'sub-01/func/sub-01_task-x_events.tsv') + rules = matching_filename_rules( + contexts(tmp_path, schema)['sub-01/func/sub-01_task-x_events.tsv'] + ) + assert rules + assert all('func' in (rule.datatypes or ()) for rule in rules) + + +@pytest.mark.parametrize(('derivative', 'expected'), [(False, False), (True, True)]) +def test_derivative_rules_apply_only_to_derivatives( + tmp_path: pathlib.Path, schema: Namespace, derivative: bool, expected: bool +) -> None: + build(tmp_path, 'sub-01/anat/sub-01_desc-brain_mask.nii.gz') + if derivative: + description = {'Name': 'd', 'BIDSVersion': '1.11.1', 'DatasetType': 'derivative'} + (tmp_path / 'dataset_description.json').write_text(json.dumps(description)) + rules = matching_filename_rules( + contexts(tmp_path, schema)['sub-01/anat/sub-01_desc-brain_mask.nii.gz'] + ) + assert any(rule.for_derivatives for rule in rules) is expected + + +def test_the_index_finds_what_comparing_every_rule_finds( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """The index is only a shortcut: it must find exactly the rules a full scan finds.""" + build( + tmp_path, + 'sub-01/anat/sub-01_T1w.nii.gz', + 'sub-01/func/sub-01_task-x_bold.nii.gz', + 'sub-01/func/sub-01_task-x_events.tsv', + 'sub-01/sub-01_scans.tsv', + 'participants.json', + 'phenotype/survey.tsv', + 'task-x_bold.json', + 'sub-01/notes.txt', + 'CHANGES', + ) + for context in contexts(tmp_path, schema).values(): + scanned = [rule for rule in all_filename_rules(schema) if _matches(rule, context)] + found = matching_filename_rules(context) # the best of what matched, so never more + assert set(found) <= set(scanned) + assert bool(found) == bool(scanned) + + +def _matches(rule: FilenameRule, context: Context) -> bool: + """A full, unindexed test of one rule, written out plainly.""" + if rule.for_derivatives: + return False # these datasets are raw + if rule.fixed_path is not None and f'/{rule.fixed_path}' == context.path: + return True + if rule.stem is not None and fnmatch.fnmatchcase(context.file.name.split('.')[0], rule.stem): + return rule.datatypes is None or context.datatype in rule.datatypes + return context.suffix in (rule.suffixes or ()) diff --git a/tests/checks/test_filenames.py b/tests/checks/test_filenames.py new file mode 100644 index 0000000..27dd832 --- /dev/null +++ b/tests/checks/test_filenames.py @@ -0,0 +1,271 @@ +"""Tests for the schema-driven filename checks (names and places only).""" + +import json +import pathlib + +import fsspec +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator import BIDSValidator +from bids_validator.checks import collect_issues +from bids_validator.checks.filename_rules import hide_unused_name_parts, matching_filename_rules +from bids_validator.checks.filenames import SIDECAR_EXTENSIONS, filename_issues, folder_issues +from bids_validator.context import Context, Dataset +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +from ..helpers import build +from ..helpers import codes as all_codes + +VALID = 'sub-01/anat/sub-01_T1w.nii.gz' + + +def codes(root: pathlib.Path, schema: Namespace) -> dict[str, list[str]]: + """Map each filename code found, for files and folders, to its locations.""" + return all_codes(root, schema, filename_issues, folder_issues) + + +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_foo_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-01/sub-01_T1w.nii.gz', 'MISSING_DATATYPE'), + ('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_issues( + tree, schema, dataset_checks=(), folder_checks=(), file_checks=(filename_issues,) + ) + 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)) + issues = collect_issues( + tree, schema, dataset_checks=(), folder_checks=(), file_checks=(filename_issues,) + ) + issue = next(iter(issues)) + assert issue.code == 'MISSING_REQUIRED_ENTITY' + assert issue.rule is not None + assert issue.rule.startswith('rules.files.') + + +# --- folders -------------------------------------------------------------- + + +@pytest.mark.parametrize( + ('folder', 'reported'), + [ + ('sub-01/foo/', True), # not a datatype, not an entity, not a named folder + ('stuff/', True), + ('sub-01/', False), # an entity folder + ('sub-01/ses-1/', False), + ('sub-01/anat/', False), # a datatype folder + ('phenotype/', False), # a named folder + ], +) +def test_folder_names_are_checked( + tmp_path: pathlib.Path, schema: Namespace, folder: str, reported: bool +) -> None: + """A folder must match ``rules.directories``, as in the reference validator.""" + build(tmp_path, folder) + assert (folder in codes(tmp_path, schema).get('NOT_INCLUDED', [])) is reported + + +def test_a_file_in_a_bogus_folder_reports_both(tmp_path: pathlib.Path, schema: Namespace) -> None: + """The folder is not BIDS, and the image beside it is not in a datatype folder.""" + build(tmp_path, 'sub-01/foo/sub-01_T1w.nii.gz') + found = codes(tmp_path, schema) + assert 'sub-01/foo/' in found['NOT_INCLUDED'] + assert 'sub-01/foo/sub-01_T1w.nii.gz' in found['MISSING_DATATYPE'] + # the legacy check agrees this is not a valid BIDS path + assert not BIDSValidator().is_bids('/sub-01/foo/sub-01_T1w.nii.gz') + + +@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 + 'dwi.bvec', # gradient tables are inheritable too + 'dwi.bval', + ], +) +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) == {} + + +def test_sidecar_extensions_are_the_reference_list() -> None: + assert frozenset({'.json', '.tsv', '.bvec', '.bval'}) == SIDECAR_EXTENSIONS + + +def test_a_sidecar_in_the_wrong_datatype_folder_is_not_flagged( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """``func/sub-01_T1w.json`` could describe data elsewhere; the reference skips it.""" + build(tmp_path, VALID, 'sub-01/func/sub-01_T1w.json') + assert codes(tmp_path, schema) == {} + + +# --- letter case ---------------------------------------------------------- + + +@pytest.fixture +def memfs() -> fsspec.AbstractFileSystem: + """An in-memory file system: case-sensitive, unlike a default macOS disk.""" + mem = fsspec.filesystem('memory') + mem.store.clear() + yield mem + mem.store.clear() + + +def test_names_differing_only_by_case_collide( + memfs: fsspec.AbstractFileSystem, schema: Namespace +) -> None: + memfs.pipe( + { + '/ds/dataset_description.json': json.dumps( + {'Name': 't', 'BIDSVersion': '1.11.1'} + ).encode(), + '/ds/sub-01/anat/sub-01_T1w.nii.gz': b'x', + '/ds/sub-01/anat/sub-01_t1w.nii.gz': b'x', + } + ) + tree = FileTree.read_from_filesystem('memory:///ds') + issues = collect_issues( + tree, schema, dataset_checks=(), folder_checks=(), file_checks=(filename_issues,) + ) + collisions = [i.location or '' for i in issues if i.code == 'CASE_COLLISION'] + assert sorted(collisions) == ['sub-01/anat/sub-01_T1w.nii.gz', 'sub-01/anat/sub-01_t1w.nii.gz'] + + +# --- what the schema rules see -------------------------------------------- + + +def test_an_unrecognised_file_has_no_name_parts_for_the_rules( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """``hide_unused_name_parts`` hides what no matched rule uses, like cleanContext.""" + tree = FileTree.read_from_filesystem(str(build(tmp_path, VALID, 'sub-01/anat/x-1_notes.txt'))) + dataset = Dataset(tree, schema) + + def cleaned(context: Context) -> Context: + return hide_unused_name_parts(context, matching_filename_rules(context)) + + unknown = cleaned(Context(tree / 'sub-01' / 'anat' / 'x-1_notes.txt', dataset, None)) + assert (unknown.entities, unknown.suffix, unknown.extension) == ({}, '', '') + + readme_tree = FileTree.read_from_filesystem(str(build(tmp_path / 'r', 'README'))) + readme = cleaned(Context(readme_tree / 'README', Dataset(readme_tree, schema), None)) + assert readme.entities == {} + + image = Context(tree / 'sub-01' / 'anat' / 'sub-01_T1w.nii.gz', dataset, None) + assert cleaned(image) is image # nothing to hide: the context itself + + +# --- derivatives and recordings ------------------------------------------- + + +def test_derivatives_are_not_checked_against_the_parent_dataset_rules( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """A derivative is a separate dataset; its files must not be judged by raw rules.""" + build(tmp_path, VALID) + deriv = tmp_path / 'derivatives' / 'mypipeline' + build(deriv, 'sub-01/anat/sub-01_desc-preproc_T1w.nii.gz', 'logs/run.txt') + assert codes(tmp_path, schema) == {} + + +def test_a_derivative_can_be_validated_as_its_own_dataset( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Pointing the checks at the derivative root makes the derivative rules apply.""" + deriv = build(tmp_path, VALID) / 'derivatives' / 'mypipeline' + build(deriv, 'sub-01/anat/sub-01_desc-preproc_T1w.nii.gz') + (deriv / 'dataset_description.json').write_text( + json.dumps( + { + 'Name': 'deriv', + 'BIDSVersion': '1.11.1', + 'DatasetType': 'derivative', + 'GeneratedBy': [{'Name': 'mypipeline'}], + } + ) + ) + # desc- is a derivative-only entity, so this name is legal here and nowhere else. + assert codes(deriv, schema) == {} + + +def _make_recording(root: pathlib.Path, name: str) -> None: + """Create a CTF-style directory recording with a vendor-named internal file.""" + recording = root / 'sub-01' / 'meg' / name + recording.mkdir(parents=True) + (recording / 'x.meg4').write_bytes(b'') + (recording / 'BadChannels').write_bytes(b'') + + +def test_directory_recording_with_a_valid_name_is_clean( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """The folder name is valid, and its vendor-named internals are never checked.""" + build(tmp_path) + _make_recording(tmp_path, 'sub-01_task-rest_meg.ds') + assert codes(tmp_path, schema) == {} + + +@pytest.mark.parametrize( + ('name', 'expected'), + [ + ('oopsbadname.ds', 'NOT_INCLUDED'), # not a BIDS name: an ordinary, bad folder + ('sub-01_meg.ds', 'MISSING_REQUIRED_ENTITY'), # missing the required task + ], +) +def test_directory_recording_name_is_validated( + tmp_path: pathlib.Path, schema: Namespace, name: str, expected: str +) -> None: + """A directory recording is one unit, but its own name still follows the rules.""" + build(tmp_path) + _make_recording(tmp_path, name) + found = codes(tmp_path, schema) + assert expected in found, f'{name} should raise {expected}, got {sorted(found)}' + assert f'sub-01/meg/{name}/' in found[expected] diff --git a/tests/checks/test_integrity.py b/tests/checks/test_integrity.py new file mode 100644 index 0000000..01b36fb --- /dev/null +++ b/tests/checks/test_integrity.py @@ -0,0 +1,138 @@ +"""Tests for the checks that a file can be read at all.""" + +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks.integrity import dataset_integrity_issues, integrity_issues +from bids_validator.context import Dataset +from bids_validator.types.files import FileTree + +from ..helpers import build, findings, gzipped, nifti, write + + +def found(root: pathlib.Path, schema: Namespace) -> list[tuple[str, str]]: + return findings(root, schema, integrity_issues) + + +@pytest.mark.parametrize( + ('relpath', 'content', 'code'), + [ + ('sub-01/anat/sub-01_T1w.json', b'', 'EMPTY_FILE'), + ('sub-01/anat/sub-01_T1w.json', b'{"a": ', 'JSON_INVALID'), + ('sub-01/anat/sub-01_T1w.json', b'\xef\xbb\xbf{}', 'INVALID_JSON_ENCODING'), # a BOM + ('sub-01/anat/sub-01_T1w.json', b'{"a": "\xff"}', 'INVALID_JSON_ENCODING'), + ('sub-01/anat/sub-01_T1w.json', b'[1, 2]', 'JSON_NOT_AN_OBJECT'), + ('sub-01/anat/sub-01_T1w.nii.gz', b'not an image', 'NIFTI_HEADER_UNREADABLE'), + ('participants.tsv', b'participant_id\tage\tage\n', 'TSV_COLUMN_HEADER_DUPLICATE'), + ('participants.tsv', b'participant_id\nsub-01\n\nsub-02\n', 'TSV_EMPTY_LINE'), + ('participants.tsv', b'participant_id\tage\nsub-01\n', 'TSV_EQUAL_ROWS'), + ], +) +def test_each_code_fires( + tmp_path: pathlib.Path, schema: Namespace, relpath: str, content: bytes, code: str +) -> None: + write(tmp_path, {relpath: content}) + assert (code, relpath) in found(tmp_path, schema) + + +def test_an_empty_image_is_empty_and_unreadable(tmp_path: pathlib.Path, schema: Namespace) -> None: + """Both, as the reference validator reports.""" + build(tmp_path, 'sub-01/anat/sub-01_T1w.nii.gz') + result = found(tmp_path, schema) + assert ('EMPTY_FILE', 'sub-01/anat/sub-01_T1w.nii.gz') in result + assert ('NIFTI_HEADER_UNREADABLE', 'sub-01/anat/sub-01_T1w.nii.gz') in result + + +@pytest.mark.parametrize( + ('relpath', 'content'), + [ + ('sub-01/anat/sub-01_T1w.nii.gz', nifti((2, 2, 2))), + ('sub-01/anat/sub-01_T1w.json', b'{"a": 1}'), + ('participants.tsv', b'participant_id\nsub-01\n'), # ends with a newline + ('participants.tsv', b'\xef\xbb\xbfparticipant_id\r\nsub-01\r\n'), # BOM, CRLF + ('participants.tsv', b'participant_id\tage\n'), # a header and no rows + ], +) +def test_readable_files_are_quiet( + tmp_path: pathlib.Path, schema: Namespace, relpath: str, content: bytes +) -> None: + write(tmp_path, {relpath: content}) + assert found(tmp_path, schema) == [] + + +def test_a_gzipped_table_that_is_not_gzip(tmp_path: pathlib.Path, schema: Namespace) -> None: + """A headerless ``.tsv.gz`` takes its columns from the sidecar, then must decompress.""" + write( + tmp_path, + { + 'sub-01/func/sub-01_task-rest_physio.json': {'Columns': ['cardiac']}, + 'sub-01/func/sub-01_task-rest_physio.tsv.gz': b'plain text, not gzip', + }, + ) + assert ('INVALID_GZIP', 'sub-01/func/sub-01_task-rest_physio.tsv.gz') in found( + tmp_path, schema + ) + + +def test_a_gzipped_table_with_the_wrong_number_of_cells( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + write( + tmp_path, + { + 'sub-01/func/sub-01_task-rest_physio.json': {'Columns': ['cardiac', 'respiratory']}, + 'sub-01/func/sub-01_task-rest_physio.tsv.gz': gzipped('1\t2\n3\n'), + }, + ) + assert ('TSV_EQUAL_ROWS', 'sub-01/func/sub-01_task-rest_physio.tsv.gz') in found( + tmp_path, schema + ) + + +def test_two_equally_good_sidecars_are_ambiguous( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Reported on the first candidate, as in the reference.""" + write( + tmp_path, + { + 'sub-01/anat/sub-01_T1w.nii.gz': nifti((2, 2, 2)), + 'sub-01/anat/sub-01_acq-a_T1w.json': {}, + 'sub-01/anat/sub-01_run-1_T1w.json': {}, + 'sub-01/anat/sub-01_acq-a_run-1_T1w.nii.gz': nifti((2, 2, 2)), + }, + ) + codes = found(tmp_path, schema) + assert ('MULTIPLE_INHERITABLE_FILES', 'sub-01/anat/sub-01_acq-a_T1w.json') in codes + + +def test_an_exact_sidecar_wins_over_partial_ones( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Several candidates, one with every entity of the image: not ambiguous.""" + write( + tmp_path, + { + 'sub-01/anat/sub-01_acq-a_run-1_T1w.nii.gz': nifti((2, 2, 2)), + 'sub-01/anat/sub-01_acq-a_run-1_T1w.json': {}, + 'sub-01/anat/sub-01_acq-a_T1w.json': {}, + 'sub-01/anat/sub-01_run-1_T1w.json': {}, + }, + ) + assert not [c for c in found(tmp_path, schema) if c[0] == 'MULTIPLE_INHERITABLE_FILES'] + + +def test_a_missing_dataset_description(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'README') + (tmp_path / 'dataset_description.json').unlink() + dataset = Dataset(FileTree.read_from_filesystem(str(tmp_path)), schema) + [issue] = dataset_integrity_issues(dataset) + assert issue.code == 'MISSING_DATASET_DESCRIPTION' + assert issue.location is None + + +def test_a_present_dataset_description(tmp_path: pathlib.Path, schema: Namespace) -> None: + dataset = Dataset(FileTree.read_from_filesystem(str(build(tmp_path))), schema) + assert dataset_integrity_issues(dataset) == [] diff --git a/tests/checks/test_rules.py b/tests/checks/test_rules.py new file mode 100644 index 0000000..d621974 --- /dev/null +++ b/tests/checks/test_rules.py @@ -0,0 +1,284 @@ +"""Tests for the schema's rules: which apply, how they are evaluated, what they report.""" + +import pathlib +import typing as ty + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks import collect_issues +from bids_validator.checks.rules import ( + NOT_ENABLED_YET, + PAYLOAD_HANDLERS, + apply_rule, + names_read_by, + rule_issues, + runnable_rules, +) +from bids_validator.context import Context, Dataset +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +from ..helpers import FULL_DESCRIPTION, LONG_README, gzipped, nifti, write + + +def rule_codes(root: pathlib.Path, schema: Namespace) -> set[tuple[str, str]]: + """Return ``{(code, location)}`` for every rule finding in the dataset.""" + tree = FileTree.read_from_filesystem(str(root)) + issues = collect_issues( + tree, schema, dataset_checks=(), folder_checks=(), file_checks=(rule_issues,) + ) + return {(issue.code, issue.location or '') for issue in issues} + + +# --- which rules run -------------------------------------------------------- + + +def test_only_unbuilt_parts_of_the_context_are_held_back() -> None: + assert NOT_ENABLED_YET == ('associations', 'ome', 'tiff') + + +def test_no_runnable_rule_reads_a_held_back_name(schema: Namespace) -> None: + for path, rule in runnable_rules(schema): + for name in names_read_by(rule): + blocked = [p for p in NOT_ENABLED_YET if name == p or name.startswith(f'{p}.')] + assert not blocked, (path, name, blocked) + + +def test_every_kind_of_rule_runs(schema: Namespace) -> None: + groups = {path.split('.')[1] for path, _rule in runnable_rules(schema)} + assert {'checks', 'sidecars', 'json', 'dataset_metadata', 'tabular_data'} <= groups + + +def test_rules_reading_associations_wait(schema: Namespace) -> None: + paths = {path for path, _rule in runnable_rules(schema)} + assert 'rules.checks.events.EventsMissing' not in paths + assert 'rules.checks.general.ReadmeFileSmall' in paths + assert 'rules.checks.anat.T1wFileWithTooManyDimensions' in paths + + +@pytest.mark.parametrize( + ('expression', 'expected'), + [ + ('exists(substr(path, 0, length(path) - 3), "dataset") == 0', {'path'}), + ('"micr" in dataset.modalities', {'dataset.modalities'}), + ('nifti_header.dim[0] == 4', {'nifti_header.dim'}), + ('sidecar.EchoTime[index(x, "a")] > 0', {'sidecar.EchoTime', 'x'}), + ('match(extension, "\\.gz$")', {'extension'}), + ('type(null) == "null"', set()), + ], +) +def test_names_read_by(expression: str, expected: set[str]) -> None: + assert names_read_by({'checks': [expression]}) == expected + + +def test_payloads_in_the_reference_order() -> None: + assert list(PAYLOAD_HANDLERS) == [ + 'checks', + 'columns', + 'initial_columns', + 'index_columns', + 'fields', + ] + + +# --- rules failing on bad data and quiet on good data ----------------------- + +# (code, files that make the rule fail, where it is reported, files that make it pass) +CASES = [ + pytest.param( + 'DUPLICATE_FILES', + {'sub-01/anat/sub-01_T1w.nii.gz': '', 'sub-01/anat/sub-01_T1w.nii': ''}, + 'sub-01/anat/sub-01_T1w.nii.gz', + {'sub-01/anat/sub-01_T1w.nii.gz': ''}, + id='general.DuplicateFiles', + ), + pytest.param( + 'README_FILE_SMALL', + {'README': 'too short'}, + 'README', + {'README': LONG_README}, + id='general.ReadmeFileSmall', + ), + pytest.param( + 'MULTIPLE_README_FILES', + {'README': LONG_README, 'README.md': LONG_README}, + 'README.md', + {'README.md': LONG_README}, + id='general.DuplicateReadmes', + ), + pytest.param( + 'README_FILE_MISSING', + {}, + 'dataset_description.json', + {'README.txt': LONG_README}, + id='hints.ReadmeFileMissing', + ), + pytest.param( + 'PHASE_SUFFIX_DEPRECATED', + {'sub-01/func/sub-01_task-rest_phase.nii.gz': ''}, + 'sub-01/func/sub-01_task-rest_phase.nii.gz', + {'sub-01/func/sub-01_task-rest_bold.nii.gz': ''}, + id='func.PhaseSuffixDeprecated', + ), + pytest.param( + 'SAMPLES_TSV_MISSING', + {'sub-01/micr/sub-01_sample-A_SEM.png': ''}, + 'dataset_description.json', + { + 'samples.tsv': 'sample_id\tparticipant_id\tsample_type\n', + 'sub-01/micr/sub-01_sample-A_SEM.png': '', + }, + id='dataset.SamplesTSVMissing', + ), + pytest.param( + 'EXCESSIVE_ELECTRODE_SPECIFICITY', + {'sub-01/eeg/sub-01_task-rest_electrodes.tsv': ''}, + 'sub-01/eeg/sub-01_task-rest_electrodes.tsv', + {'sub-01/eeg/sub-01_electrodes.tsv': ''}, + id='channels.ElectrodeSpecificity', + ), + pytest.param( + 'T1W_FILE_WITH_TOO_MANY_DIMENSIONS', + {'sub-01/anat/sub-01_T1w.nii.gz': nifti((2, 2, 2, 2))}, + 'sub-01/anat/sub-01_T1w.nii.gz', + {'sub-01/anat/sub-01_T1w.nii.gz': nifti((2, 2, 2))}, + id='anat.T1wFileWithTooManyDimensions', + ), + pytest.param( + 'EVENT_ONSET_ORDER', + {'sub-01/func/sub-01_task-rest_events.tsv': 'onset\tduration\n5\t1\n2\t1\n'}, + 'sub-01/func/sub-01_task-rest_events.tsv', + {'sub-01/func/sub-01_task-rest_events.tsv': 'onset\tduration\n2\t1\n5\t1\n'}, + id='events.SortedOnsets', + ), + pytest.param( + 'SUSPICIOUS_NEGATIVE_EVENT_ONSET', + {'sub-01/func/sub-01_task-rest_events.tsv': 'onset\tduration\n-100\t1\n'}, + 'sub-01/func/sub-01_task-rest_events.tsv', + {'sub-01/func/sub-01_task-rest_events.tsv': 'onset\tduration\n-5\t1\n'}, + id='events.SuspiciousNegativeEventOnset', + ), + pytest.param( + 'GZIP_HEADER_MTIME', + {'sub-01/func/sub-01_task-rest_events.tsv.gz': gzipped('x', mtime=1_700_000_000)}, + 'sub-01/func/sub-01_task-rest_events.tsv.gz', + {'sub-01/func/sub-01_task-rest_events.tsv.gz': gzipped('x', mtime=0)}, + id='privacy.GzipHeaderMtime', + ), + pytest.param( + 'TOO_FEW_AUTHORS', + {'dataset_description.json': {**FULL_DESCRIPTION, 'Authors': ['Ada Lovelace']}}, + 'dataset_description.json', + {'dataset_description.json': FULL_DESCRIPTION}, + id='hints.TooFewAuthors', + ), + pytest.param( + 'UNKNOWN_BIDS_VERSION', + {'dataset_description.json': {**FULL_DESCRIPTION, 'BIDSVersion': '9.9.9'}}, + 'dataset_description.json', + {'dataset_description.json': FULL_DESCRIPTION}, + id='dataset.UnknownVersion', + ), +] + + +@pytest.mark.parametrize(('code', 'failing', 'location', 'passing'), CASES) +def test_rule_fails_on_bad_data_and_passes_on_good_data( + tmp_path: pathlib.Path, + schema: Namespace, + code: str, + failing: dict[str, ty.Any], + location: str, + passing: dict[str, ty.Any], +) -> None: + assert (code, location) in rule_codes(write(tmp_path / 'bad', failing), schema) + assert code not in { + found for found, _ in rule_codes(write(tmp_path / 'good', passing), schema) + } + + +def test_an_unreadable_header_is_not_a_failed_check( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """``nifti_header != null`` is false for an empty image: the rule does not apply. + + The empty image is reported once, by the integrity checks, not again here. + """ + write(tmp_path, {'sub-01/anat/sub-01_T1w.nii.gz': b''}) + codes = {code for code, _ in rule_codes(tmp_path, schema)} + assert 'T1W_FILE_WITH_TOO_MANY_DIMENSIONS' not in codes + + +def test_an_unrecognised_file_is_held_to_no_rule( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """A file matching no filename rule has no suffix or extension for the rules.""" + write(tmp_path, {'sub-01/anat/x-1_y.nii.gz': nifti((2, 2, 2))}) + assert not [ + loc for _, loc in rule_codes(tmp_path, schema) if loc == 'sub-01/anat/x-1_y.nii.gz' + ] + + +# --- how one rule is evaluated ---------------------------------------------- + + +@pytest.fixture +def readme_context(tmp_path: pathlib.Path, schema: Namespace) -> Context: + tree = FileTree.read_from_filesystem(str(write(tmp_path, {'README': 'short'}))) + return Context(tree / 'README', Dataset(tree, schema), None) + + +ISSUE = {'code': 'TEST_CODE', 'message': 'Problem in {path}.', 'level': 'error'} + + +@pytest.mark.parametrize( + ('selectors', 'checks', 'reported'), + [ + pytest.param(['true'], ['true'], False, id='applies-and-holds'), + pytest.param(['true'], ['false'], True, id='applies-and-fails'), + pytest.param(['false'], ['false'], False, id='does-not-apply'), + # The schema documentation: "a null selector will not apply the current rule, + # and a null check will fail." + pytest.param(['null'], ['false'], False, id='null-selector-does-not-apply'), + pytest.param(['true'], ['null'], True, id='null-check-fails'), + # An expression that raises counts as null, as in the reference validator. + pytest.param(['len(path)'], ['false'], False, id='raising-selector-does-not-apply'), + pytest.param(['true'], ['len(path)'], True, id='raising-check-fails'), + # Every check must hold. + pytest.param(['true'], ['true', 'false'], True, id='one-failing-check-is-enough'), + pytest.param([], ['false'], True, id='no-selectors-applies-everywhere'), + ], +) +def test_apply_rule( + readme_context: Context, selectors: list[str], checks: list[str], reported: bool +) -> None: + rule = {'selectors': selectors, 'checks': checks, 'issue': ISSUE} + issues = apply_rule('rules.checks.test.Rule', rule, readme_context) + assert bool(issues) is reported + + +def test_the_finding_comes_from_the_rule(readme_context: Context) -> None: + rule = {'selectors': [], 'checks': ['false'], 'issue': ISSUE} + [issue] = apply_rule('rules.checks.test.Rule', rule, readme_context) + assert (issue.code, issue.severity, issue.location) == ('TEST_CODE', Severity.ERROR, 'README') + assert issue.message == 'Problem in /README.' # the placeholder is filled in + assert issue.rule == 'rules.checks.test.Rule' + + +def test_a_rule_without_a_complete_issue_reports_check_error(readme_context: Context) -> None: + rule = {'selectors': [], 'checks': ['false'], 'issue': {'level': 'error'}} + [issue] = apply_rule('rules.checks.test.Rule', rule, readme_context) + assert issue.code == 'CHECK_ERROR' + + +def test_each_payload_of_a_rule_runs(readme_context: Context) -> None: + """A rule with checks AND fields reports from both, in the rule's own order.""" + rule = { + 'selectors': [], + 'checks': ['false'], + 'issue': ISSUE, + 'fields': {'Authors': 'required'}, # README is not JSON: json is empty + } + codes = [issue.code for issue in apply_rule('rules.json.test.Rule', rule, readme_context)] + assert codes == ['TEST_CODE', 'JSON_KEY_REQUIRED'] diff --git a/tests/checks/test_schema_cache.py b/tests/checks/test_schema_cache.py new file mode 100644 index 0000000..589968a --- /dev/null +++ b/tests/checks/test_schema_cache.py @@ -0,0 +1,21 @@ +"""Tests for reading something out of the schema once per schema.""" + +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks.schema_cache import once_per_schema + + +def test_an_answer_is_worked_out_once_per_schema() -> None: + calls: list[Namespace] = [] + + @once_per_schema + def count_rules(schema: Namespace) -> int: + calls.append(schema) + return len(schema['rules']) + + first = Namespace({'rules': {'a': 1}}) + second = Namespace({'rules': {'a': 1, 'b': 2}}) + assert count_rules(first) == 1 + assert count_rules(first) == 1 + assert count_rules(second) == 2 + assert calls == [first, second] diff --git a/tests/checks/test_tables.py b/tests/checks/test_tables.py new file mode 100644 index 0000000..99fb997 --- /dev/null +++ b/tests/checks/test_tables.py @@ -0,0 +1,152 @@ +"""Tests for table columns: presence, order, extra columns, value types, unique index.""" + +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks import collect_issues +from bids_validator.checks.rules import rule_issues +from bids_validator.issues import Issue, Severity +from bids_validator.types.files import FileTree + +from ..helpers import write + +CHANNELS = 'sub-01/eeg/sub-01_task-rest_channels.tsv' + + +def table_findings(root: pathlib.Path, schema: Namespace) -> list[Issue]: + tree = FileTree.read_from_filesystem(str(root)) + issues = collect_issues( + tree, schema, dataset_checks=(), folder_checks=(), file_checks=(rule_issues,) + ) + return [issue for issue in issues if issue.code.startswith('TSV_')] + + +def keyed(issues: list[Issue]) -> set[tuple[str, str | None, str | None]]: + return {(i.code, i.location, i.sub_code) for i in issues} + + +def test_a_required_initial_column_missing(tmp_path: pathlib.Path, schema: Namespace) -> None: + write(tmp_path, {CHANNELS: 'name\ttype\nFz\tEEG\n'}) + assert ('TSV_COLUMN_MISSING', CHANNELS, 'units') in keyed(table_findings(tmp_path, schema)) + + +def test_initial_columns_out_of_order(tmp_path: pathlib.Path, schema: Namespace) -> None: + write(tmp_path, {CHANNELS: 'type\tname\tunits\nEEG\tFz\tuV\n'}) + found = keyed(table_findings(tmp_path, schema)) + assert ('TSV_COLUMN_ORDER_INCORRECT', CHANNELS, 'name') in found + assert ('TSV_COLUMN_ORDER_INCORRECT', CHANNELS, 'type') in found + + +@pytest.mark.parametrize( + ('files', 'location', 'code', 'severity'), + [ + pytest.param( + {CHANNELS: 'name\ttype\tunits\tfoo\nFz\tEEG\tuV\t1\n'}, + CHANNELS, + 'TSV_ADDITIONAL_COLUMNS_MUST_DEFINE', + Severity.ERROR, + id='allowed-if-defined', + ), + pytest.param( + {'participants.tsv': 'participant_id\tgroup\nsub-01\ta\n'}, + 'participants.tsv', + 'TSV_ADDITIONAL_COLUMNS_UNDEFINED', + Severity.WARNING, + id='allowed', + ), + ], +) +def test_extra_columns( + tmp_path: pathlib.Path, + schema: Namespace, + files: dict[str, str], + location: str, + code: str, + severity: Severity, +) -> None: + write(tmp_path, files) + [issue] = [i for i in table_findings(tmp_path, schema) if i.code == code] + assert (issue.location, issue.severity) == (location, severity) + + +def test_an_extra_column_defined_in_the_sidecar(tmp_path: pathlib.Path, schema: Namespace) -> None: + """Defined, so allowed; and its values follow the sidecar's definition.""" + write( + tmp_path, + { + 'participants.tsv': 'participant_id\tgroup\nsub-01\ta\nsub-02\tc\n', + 'participants.json': {'group': {'Levels': {'a': 'A', 'b': 'B'}}}, + }, + ) + issues = table_findings(tmp_path, schema) + assert [(i.code, i.sub_code, i.message) for i in issues] == [ + ('TSV_VALUE_INCORRECT_TYPE', 'group', "line 3: 'c'") + ] + + +@pytest.mark.parametrize( + ('column', 'value', 'code'), + [ + ('age', '30', None), + ('age', 'n/a', None), # always accepted + ('age', 'thirty', 'TSV_VALUE_INCORRECT_TYPE'), + ('age', '95', 'TSV_VALUE_INCORRECT_TYPE'), # above the schema's Maximum of 89 + ('age', '89+', 'TSV_PSEUDO_AGE_DEPRECATED'), + ('sex', 'F', None), + ('sex', 'X', 'TSV_VALUE_INCORRECT_TYPE'), # not one of the levels + ], +) +def test_value_types( + tmp_path: pathlib.Path, schema: Namespace, column: str, value: str, code: str | None +) -> None: + write(tmp_path, {'participants.tsv': f'participant_id\t{column}\nsub-01\t{value}\n'}) + assert [i.code for i in table_findings(tmp_path, schema)] == ([code] if code else []) + + +def test_a_near_miss_of_n_a_gets_a_hint(tmp_path: pathlib.Path, schema: Namespace) -> None: + write(tmp_path, {'participants.tsv': 'participant_id\tage\nsub-01\tNA\n'}) + [issue] = table_findings(tmp_path, schema) + assert issue.message == "line 2: 'NA', did you mean 'n/a'?" + + +def test_only_the_first_bad_value_of_a_column(tmp_path: pathlib.Path, schema: Namespace) -> None: + write(tmp_path, {'participants.tsv': 'participant_id\tage\nsub-01\tx\nsub-02\ty\n'}) + assert len(table_findings(tmp_path, schema)) == 1 + + +def test_a_sidecar_contradicting_the_schema(tmp_path: pathlib.Path, schema: Namespace) -> None: + """``participant_id`` is text in the schema; a sidecar cannot make it a number.""" + write( + tmp_path, + { + 'participants.tsv': 'participant_id\nsub-01\n', + 'participants.json': {'participant_id': {'Format': 'number'}}, + }, + ) + [issue] = table_findings(tmp_path, schema) + assert (issue.code, issue.location, issue.sub_code) == ( + 'TSV_COLUMN_TYPE_REDEFINED', + 'participants.json', # reported where the redefinition is written + 'participant_id', + ) + + +def test_index_values_must_be_unique(tmp_path: pathlib.Path, schema: Namespace) -> None: + write(tmp_path, {'participants.tsv': 'participant_id\nsub-01\nsub-01\n'}) + [issue] = table_findings(tmp_path, schema) + assert (issue.code, issue.message) == ('TSV_INDEX_VALUE_NOT_UNIQUE', "row 3, value 'sub-01'") + + +def test_a_column_name_with_a_dot(tmp_path: pathlib.Path, schema: Namespace) -> None: + """``key_resp.rt`` (as PsychoPy writes it) is an ordinary column, not a path.""" + write( + tmp_path, + { + 'participants.tsv': 'participant_id\tkey_resp.rt\nsub-01\t0.5\nsub-02\tslow\n', + 'participants.json': {'key_resp.rt': {'Description': 'Response time', 'Units': 's'}}, + }, + ) + issues = table_findings(tmp_path, schema) + assert [(i.code, i.sub_code) for i in issues] == [('TSV_VALUE_INCORRECT_TYPE', 'key_resp.rt')] diff --git a/tests/checks/test_values.py b/tests/checks/test_values.py new file mode 100644 index 0000000..cf53f4e --- /dev/null +++ b/tests/checks/test_values.py @@ -0,0 +1,130 @@ +"""Tests for checking a metadata value against its schema definition.""" + +import typing as ty + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks.values import json_equal, value_errors + +FORMATS = { + 'index': {'pattern': '[0-9]+'}, + 'bids_uri': {'pattern': 'bids:[0-9a-zA-Z/#:?_\\-.]+'}, + 'participant_relative': {'pattern': '(?!/)(?!sub-)[0-9a-zA-Z+/_\\-.]+'}, +} + + +@pytest.mark.parametrize( + ('value', 'definition', 'expected'), + [ + pytest.param(2.0, {'type': 'number'}, [], id='number'), + pytest.param('2.0', {'type': 'number'}, ['must be number'], id='text-is-not-a-number'), + pytest.param(True, {'type': 'number'}, ['must be number'], id='true-is-not-a-number'), + pytest.param(3.0, {'type': 'integer'}, [], id='integral-float-is-integer'), + pytest.param(3.5, {'type': 'integer'}, ['must be integer'], id='fraction-is-not-integer'), + pytest.param('x', {'type': ['string', 'null']}, [], id='type-list'), + pytest.param('raw', {'enum': ['raw', 'derivative']}, [], id='enum-member'), + pytest.param( + 'study', + {'enum': ['raw', 'derivative']}, + ['must be equal to one of the allowed values'], + id='enum-outsider', + ), + pytest.param( + 1, + {'enum': [True]}, + ['must be equal to one of the allowed values'], + id='one-is-not-true', + ), + pytest.param( + 0, {'type': 'number', 'exclusiveMinimum': 0}, ['must be > 0'], id='exclusive-minimum' + ), + pytest.param(-1, {'type': 'number', 'minimum': 0}, ['must be >= 0'], id='minimum'), + pytest.param(5, {'type': 'number', 'maximum': 1}, ['must be <= 1'], id='maximum'), + pytest.param( + [], + {'type': 'array', 'minItems': 1}, + ['must NOT have fewer than 1 items'], + id='min-items', + ), + pytest.param( + [1, 2, 3], + {'type': 'array', 'maxItems': 2}, + ['must NOT have more than 2 items'], + id='max-items', + ), + pytest.param( + ['a', 1], + {'type': 'array', 'items': {'type': 'string'}}, + ['/1 must be string'], + id='item-type-with-position', + ), + pytest.param( + {'Name': 'x'}, + {'type': 'object', 'required': ['Name', 'Version']}, + ["must have required property 'Version'"], + id='required-property', + ), + pytest.param( + {'a': 1, 'b': 2}, + {'type': 'object', 'properties': {'a': {}}, 'additionalProperties': False}, + ['must NOT have additional properties'], + id='additional-properties', + ), + pytest.param('bids::sub-01/x', {'type': 'string', 'format': 'bids_uri'}, [], id='format'), + pytest.param( + 'sub-01/x', + {'type': 'string', 'format': 'bids_uri'}, + ['must match format "bids_uri"'], + id='format-mismatch', + ), + # Searched, not matched, as ajv does: an unanchored pattern occurs somewhere. + pytest.param( + '/sub-01', + {'type': 'string', 'format': 'participant_relative'}, + [], + id='format-searched', + ), + pytest.param('x', {'type': 'string', 'format': 'unknown'}, [], id='unknown-format'), + pytest.param( + 'x', + {'anyOf': [{'type': 'number'}, {'type': 'array'}]}, + ['must be number', 'must be array', 'must match a schema in anyOf'], + id='any-of-reports-every-branch', + ), + pytest.param( + 3, {'anyOf': [{'type': 'number'}, {'type': 'array'}]}, [], id='any-of-passes' + ), + # ajv stops at the first failing keyword: type before bounds. + pytest.param( + 'x', {'type': 'number', 'minimum': 0}, ['must be number'], id='first-error-only' + ), + ], +) +def test_value_errors(value: ty.Any, definition: dict[str, ty.Any], expected: list[str]) -> None: + assert value_errors(value, definition, FORMATS) == expected + + +def test_real_definitions(schema: Namespace) -> None: + """Against the schema's own definitions and formats.""" + metadata, formats = schema.objects.metadata, schema.objects.formats + assert value_errors(2.0, metadata.RepetitionTime, formats) == [] + assert value_errors(-2.0, metadata.RepetitionTime, formats) == ['must be > 0'] + assert value_errors('Me', metadata.Authors, formats) == ['must be array'] + assert value_errors(['Me'], metadata.Authors, formats) == [] + + +@pytest.mark.parametrize( + ('a', 'b', 'equal'), + [ + (1, 1.0, True), + (True, 1, False), + ([1, 2], [1, 2], True), + ([1, 2], [2, 1], False), + ({'a': [1]}, {'a': [1]}, True), + ('1', 1, False), + (None, None, True), + ], +) +def test_json_equal(a: ty.Any, b: ty.Any, equal: bool) -> None: + assert json_equal(a, b) is equal diff --git a/tests/checks/test_walk.py b/tests/checks/test_walk.py new file mode 100644 index 0000000..55d4f05 --- /dev/null +++ b/tests/checks/test_walk.py @@ -0,0 +1,146 @@ +"""Tests for which files and folders a validation visits.""" + +import json +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks.walk import DEFAULT_IGNORES, is_plain_folder, iter_contexts +from bids_validator.context import Dataset +from bids_validator.types.files import FileTree + +from ..helpers import build + + +def visited(root: pathlib.Path, schema: Namespace) -> list[str]: + """The relative path of every context the walk yields, in order.""" + tree = FileTree.read_from_filesystem(str(root)) + return [context.file.relative_path for context in iter_contexts(Dataset(tree, schema))] + + +def test_files_and_folders_are_visited_files_first( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Each folder's files come before its subfolders, as in the reference.""" + build(tmp_path, 'README', 'sub-01/anat/sub-01_T1w.nii.gz') + paths = visited(tmp_path, schema) + assert paths.index('README') < paths.index('sub-01/') + assert paths.index('sub-01/') < paths.index('sub-01/anat/') + assert paths.index('sub-01/anat/') < paths.index('sub-01/anat/sub-01_T1w.nii.gz') + + +@pytest.mark.parametrize( + ('relpath', 'is_visited'), + [ + ('code/run.py', False), # the reference's top-level ignores + ('sourcedata/raw.dcm', False), + ('stimuli/face.png', False), + ('log/run.txt', False), + ('doc/notes.md', False), + ('derivatives/x/file.txt', False), + ('sub-01/log/run.txt', True), # anchored at the root: deeper ones are checked + ('sub-01/code/run.py', True), + ('.DS_Store', False), # hidden files at every level + ('sub-01/.DS_Store', False), + ('.git/config', False), + ], +) +def test_ignores( + tmp_path: pathlib.Path, schema: Namespace, relpath: str, is_visited: bool +) -> None: + build(tmp_path, relpath) + assert (relpath in visited(tmp_path, schema)) is is_visited + + +def test_default_ignores_are_the_reference_list() -> None: + assert DEFAULT_IGNORES == ( + '/sourcedata/', + '/derivatives/', + '/code/', + '/stimuli/', + '/log/', + '/doc/', + ) + + +def test_bidsignore_is_visited_and_applied(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'extras/notes.txt') + (tmp_path / '.bidsignore').write_text('extras/\n') + paths = visited(tmp_path, schema) + assert '.bidsignore' in paths + assert 'extras/' not in paths + assert 'extras/notes.txt' not in paths + + +def test_a_directory_recording_is_one_unit(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'sub-01/meg/sub-01_task-rest_meg.ds/BadChannels') + paths = visited(tmp_path, schema) + assert 'sub-01/meg/sub-01_task-rest_meg.ds/' in paths + assert 'sub-01/meg/sub-01_task-rest_meg.ds/BadChannels' not in paths + + +def test_a_folder_named_like_a_file_is_one_unit(tmp_path: pathlib.Path, schema: Namespace) -> None: + """The schema's bare ``/`` extension: ``x-1_y/`` is a file-like folder, not walked.""" + build(tmp_path, 'sub-01/log/20200910-165242_a0c5/fmriprep.toml') + paths = visited(tmp_path, schema) + assert 'sub-01/log/20200910-165242_a0c5/' in paths + assert 'sub-01/log/20200910-165242_a0c5/fmriprep.toml' not in paths + + +def test_an_ordinary_folder_with_a_recording_extension_is_walked( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """``old.ds`` has no entities, so it is just a folder.""" + build(tmp_path, 'sub-01/meg/old.ds/file.txt') + assert 'sub-01/meg/old.ds/file.txt' in visited(tmp_path, schema) + + +def test_opaque_folders_are_visited_but_not_walked( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """``logs`` and ``docs`` are opaque in ``rules.directories``.""" + build(tmp_path, 'logs/run.txt', 'docs/guide.md') + paths = visited(tmp_path, schema) + assert 'logs/' in paths + assert 'docs/' in paths + assert 'logs/run.txt' not in paths + assert 'docs/guide.md' not in paths + + +def test_a_raw_dataset_skips_nested_derivatives(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'sub-01/derivatives/x.txt') + paths = visited(tmp_path, schema) + assert 'sub-01/derivatives/' not in paths + assert 'sub-01/derivatives/x.txt' not in paths + + +def test_a_derivative_dataset_walks_its_own_folders( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + build(tmp_path, 'sub-01/anat/sub-01_desc-x_T1w.nii.gz') + (tmp_path / 'dataset_description.json').write_text( + json.dumps({'Name': 'd', 'BIDSVersion': '1.11.1', 'DatasetType': 'derivative'}) + ) + assert 'sub-01/anat/sub-01_desc-x_T1w.nii.gz' in visited(tmp_path, schema) + + +def test_is_plain_folder_tells_folders_from_recordings( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + build(tmp_path, 'sub-01/meg/sub-01_task-rest_meg.ds/BadChannels') + tree = FileTree.read_from_filesystem(str(tmp_path)) + contexts = {c.file.relative_path: c for c in iter_contexts(Dataset(tree, schema))} + assert is_plain_folder(contexts['sub-01/meg/']) + assert not is_plain_folder(contexts['sub-01/meg/sub-01_task-rest_meg.ds/']) + assert not is_plain_folder(contexts['dataset_description.json']) + + +def test_each_context_knows_its_subject(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'sub-01/ses-1/anat/sub-01_ses-1_T1w.nii.gz') + tree = FileTree.read_from_filesystem(str(tmp_path)) + contexts = {c.file.relative_path: c for c in iter_contexts(Dataset(tree, schema))} + image = contexts['sub-01/ses-1/anat/sub-01_ses-1_T1w.nii.gz'] + assert image.subject is not None + assert image.subject.sessions.ses_dirs == ['ses-1'] + assert contexts['dataset_description.json'].subject is None diff --git a/tests/helpers.py b/tests/helpers.py new file mode 100644 index 0000000..572bdb3 --- /dev/null +++ b/tests/helpers.py @@ -0,0 +1,153 @@ +"""Helpers shared by the check tests: small datasets written to a temporary folder.""" + +import gzip +import io +import json +import pathlib +from collections.abc import Callable, Iterable, Mapping + +import nibabel as nb +import numpy as np +from bidsschematools.types.namespace import Namespace + +from bids_validator.checks import FOLDER_CHECKS, collect_issues +from bids_validator.context import Context +from bids_validator.issues import Issue +from bids_validator.types.files import FileTree + +DESCRIPTION = {'Name': 'test', 'BIDSVersion': '1.11.1'} + +# Every field rules.dataset_metadata asks for, so dataset_description.json is clean. +FULL_DESCRIPTION = { + 'Name': 'Clean', + 'BIDSVersion': '1.11.1', + 'DatasetType': 'raw', + 'License': 'CC0', + 'Authors': ['Ada Lovelace', 'Alan Turing'], + 'Acknowledgements': 'Thanks.', + 'HowToAcknowledge': 'Cite this.', + 'Funding': ['A grant'], + 'EthicsApprovals': ['An approval'], + 'ReferencesAndLinks': ['https://example.org'], + 'DatasetDOI': 'doi:10.0000/clean', + 'HEDVersion': '8.2.0', + 'GeneratedBy': [{'Name': 'hand'}], + 'SourceDatasets': [{'URL': 'https://example.org'}], +} + +LONG_README = 'A dataset written for the tests. ' * 10 # over 150 bytes + + +def build(root: pathlib.Path, *relpaths: str) -> pathlib.Path: + """Create a minimal dataset holding the given EMPTY files.""" + return write(root, dict.fromkeys(relpaths, b'')) + + +def write( + root: pathlib.Path, + files: Mapping[str, str | bytes | dict[str, object]], + *, + description: Mapping[str, object] = DESCRIPTION, +) -> pathlib.Path: + """Create a dataset from ``{relative path: content}``. + + Content is text, bytes, or a dict written as JSON. ``dataset_description.json`` + is written from ``description`` unless ``files`` names it. A path ending in ``/`` + is created as an empty folder. + """ + root.mkdir(parents=True, exist_ok=True) + if 'dataset_description.json' not in files: + (root / 'dataset_description.json').write_text(json.dumps(description)) + for relpath, content in files.items(): + path = root / relpath + if relpath.endswith('/'): + path.mkdir(parents=True, exist_ok=True) + continue + path.parent.mkdir(parents=True, exist_ok=True) + if isinstance(content, dict): + path.write_text(json.dumps(content)) + elif isinstance(content, bytes): + path.write_bytes(content) + else: + path.write_text(content) + return root + + +def clean_dataset(root: pathlib.Path) -> pathlib.Path: + """Write a small dataset that every check, ours and the reference's, finds clean.""" + return write( + root, + { + 'README': LONG_README, + 'participants.tsv': 'participant_id\nsub-01\n', + 'sub-01/beh/sub-01_task-rest_beh.tsv': 'response_time\n0.5\n', + 'task-rest_beh.json': { + 'TaskName': 'rest', + 'Instructions': 'Rest.', + 'TaskDescription': 'Resting.', + 'CogAtlasID': 'https://www.cognitiveatlas.org/task/id/trm_4c8a834779883', + 'CogPOID': 'http://www.cogpo.org/ontologies/CogPOver1.owl#COGPO_00000', + 'InstitutionName': 'A university', + 'InstitutionAddress': 'A street', + 'InstitutionalDepartmentName': 'A department', + 'response_time': {'Description': 'Response time', 'Units': 's'}, + }, + }, + description=FULL_DESCRIPTION, + ) + + +def nifti(shape: tuple[int, ...], zooms: tuple[float, ...] | None = None) -> bytes: + """Return the bytes of a real, gzipped NIfTI-1 image of ``shape``.""" + image = nb.Nifti1Image(np.zeros(shape, dtype=np.uint8), np.eye(4)) # type: ignore[no-untyped-call] + if zooms is not None: + image.header.set_zooms(zooms) # type: ignore[no-untyped-call] + image.header.set_xyzt_units('mm', 'sec') # type: ignore[no-untyped-call] + return gzip.compress(image.to_bytes(), mtime=0) + + +def gzipped(text: str, *, mtime: int = 0, filename: str | None = None) -> bytes: + """Return ``text`` gzipped, with a chosen header modification time and file name.""" + buffer = io.BytesIO() + with gzip.GzipFile(filename=filename or '', mode='wb', fileobj=buffer, mtime=mtime) as fobj: + fobj.write(text.encode()) + return buffer.getvalue() + + +def issues_of( + root: pathlib.Path, schema: Namespace, *checks: Callable[[Context], list[Issue]] +) -> list[Issue]: + """Return every finding of the given checks, or of all of them. + + A folder check (``folder_issues``) is run on folders, any other check on files. + """ + tree = FileTree.read_from_filesystem(str(root)) + if not checks: + return list(collect_issues(tree, schema)) + issues: Iterable[Issue] = collect_issues( + tree, + schema, + dataset_checks=(), + folder_checks=[check for check in checks if check in FOLDER_CHECKS], + file_checks=[check for check in checks if check not in FOLDER_CHECKS], + ) + return list(issues) + + +def findings( + root: pathlib.Path, + schema: Namespace, + *checks: Callable[[Context], list[Issue]], +) -> list[tuple[str, str]]: + """Return ``(code, location)`` of every finding, from the given checks or all of them.""" + return [(issue.code, issue.location or '') for issue in issues_of(root, schema, *checks)] + + +def codes( + root: pathlib.Path, schema: Namespace, *checks: Callable[[Context], list[Issue]] +) -> dict[str, list[str]]: + """Map each code found to the locations it was found at.""" + out: dict[str, list[str]] = {} + for code, location in findings(root, schema, *checks): + out.setdefault(code, []).append(location) + return out diff --git a/tests/test_context.py b/tests/test_context.py index 15911da..f596f52 100644 --- a/tests/test_context.py +++ b/tests/test_context.py @@ -119,7 +119,7 @@ def test_context(synthetic_dataset: FileTree, schema: Namespace) -> None: assert events_context.json is None assert events_context.gzip is None assert events_context.nifti_header is None - assert isinstance(events_context.columns, Namespace) + assert isinstance(events_context.columns, context.FileContents) assert 'onset' in events_context.columns assert len(events_context.columns.onset) == 42 @@ -130,7 +130,7 @@ def test_context(synthetic_dataset: FileTree, schema: Namespace) -> None: assert stim_context.gzip.filename == stim.name.removesuffix('.gz') assert stim_context.gzip.timestamp != 0 assert stim_context.gzip.comment == '' - assert isinstance(stim_context.columns, Namespace) + assert isinstance(stim_context.columns, context.FileContents) assert list(stim_context.columns.keys()) == stim_context.sidecar.Columns ## Tests for: @@ -357,3 +357,166 @@ def test_exists_bidsuri(examples: Path, schema: Namespace) -> None: assert file_context.json is not None assert file_context.exists(file_context.json.AssociatedEmptyRoom, 'bids-uri') == 1 + + +def _small_dataset(root: Path, *relpaths: str) -> FileTree: + """Write a dataset_description.json, a README and the given empty files or folders.""" + (root / 'dataset_description.json').write_text('{"Name": "t", "BIDSVersion": "1.11.1"}') + (root / 'README').write_text('A test dataset.') + for relpath in relpaths: + path = root / relpath + if relpath.endswith('/'): + path.mkdir(parents=True, exist_ok=True) + else: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b'') + return FileTree.read_from_filesystem(root) + + +def test_find_datatypes_leaves_the_schema_alone(tmp_path: Path, schema: Namespace) -> None: + """A folder name with a dot must not change the (cached, shared) schema. + + ``".git" in namespace`` reads the dot as a path separator and inserts an empty + key into the Namespace. ``load_schema()`` returns the same object to every caller, + so every context built afterwards failed with ``AttributeError: ... 'value'``. + """ + tree = _small_dataset(tmp_path, '.git/', '.datalad/', 'sub-01/anat/sub-01_T1w.nii.gz') + before = list(schema.objects.datatypes.keys()) + + found = context.find_datatypes(tree, schema.objects.datatypes) + + assert found == {'anat'} + assert list(schema.objects.datatypes.keys()) == before + + +def test_modalities_skips_datatypes_without_a_modality(tmp_path: Path, schema: Namespace) -> None: + """``phenotype`` is a datatype that belongs to no modality; it used to raise KeyError.""" + tree = _small_dataset(tmp_path, 'phenotype/survey.tsv', 'sub-01/anat/sub-01_T1w.nii.gz') + dataset = context.Dataset(tree, schema) + + assert set(dataset.datatypes) == {'anat', 'phenotype'} + assert dataset.modalities == ['mri'] + + +@pytest.mark.parametrize( + ('arg', 'rule', 'expected'), + [ + pytest.param('README', 'dataset', 1, id='relative'), + # ``path`` in the context always starts with "/"; it means the dataset root. + pytest.param('/README', 'dataset', 1, id='leading-slash-is-the-root'), + pytest.param('/sub-01/anat/sub-01_T1w.nii.gz', 'dataset', 1, id='leading-slash-deep'), + pytest.param('anat/sub-01_T1w.nii.gz', 'subject', 1, id='subject-relative'), + # Not a path at all: not found, and no "Protocol not known: 'bids'" error. + pytest.param('bids::README', 'dataset', 0, id='uri-given-to-dataset-rule'), + pytest.param('bids::README', 'subject', 0, id='uri-given-to-subject-rule'), + pytest.param('../README', 'dataset', 0, id='parent-is-never-found'), + pytest.param('', 'dataset', 0, id='empty-path'), + ], +) +def test_exists_resolves_paths_by_hand( + tmp_path: Path, schema: Namespace, arg: str, rule: str, expected: int +) -> None: + tree = _small_dataset(tmp_path, 'sub-01/anat/sub-01_T1w.nii.gz') + file_context = context.Context( + tree / 'sub-01' / 'anat' / 'sub-01_T1w.nii.gz', context.Dataset(tree, schema), None + ) + + assert file_context.exists(arg, rule) == expected + + +# --- reading file contents for the schema rules ------------------------------ + + +def _ctx(root: Path, relpath: str, schema: Namespace) -> context.Context: + tree = FileTree.read_from_filesystem(root) + return context.Context(tree / relpath, context.Dataset(tree, schema), None) + + +def _files(root: Path, files: dict[str, object]) -> Path: + (root / 'dataset_description.json').write_text('{"Name": "t", "BIDSVersion": "1.11.1"}') + for relpath, content in files.items(): + path = root / relpath + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content if isinstance(content, str) else json.dumps(content)) + return root + + +def test_a_json_file_has_no_sidecar(tmp_path: Path, schema: Namespace) -> None: + _files(tmp_path, {'sub-01/anat/sub-01_T1w.json': {'EchoTime': 0.01}}) + assert _ctx(tmp_path, 'sub-01/anat/sub-01_T1w.json', schema).sidecar == {} + + +def test_repetition_time_is_rounded_to_milliseconds(tmp_path: Path, schema: Namespace) -> None: + _files( + tmp_path, + { + 'sub-01/func/sub-01_task-x_bold.nii.gz': '', + 'task-x_bold.json': {'RepetitionTime': 2.00049}, + }, + ) + assert ( + _ctx(tmp_path, 'sub-01/func/sub-01_task-x_bold.nii.gz', schema).sidecar.RepetitionTime + == 2.0 + ) + + +def test_a_broken_sidecar_is_skipped_not_fatal(tmp_path: Path, schema: Namespace) -> None: + """The broken JSON is reported on its own; the other levels still apply.""" + _files( + tmp_path, + { + 'sub-01/func/sub-01_task-x_bold.nii.gz': '', + 'task-x_bold.json': {'TaskName': 'x'}, + 'sub-01/func/sub-01_task-x_bold.json': '{"broken": ', + }, + ) + ctx = _ctx(tmp_path, 'sub-01/func/sub-01_task-x_bold.nii.gz', schema) + assert ctx.sidecar == {'TaskName': 'x'} + + +def test_sidecar_origins_name_the_nearest_file(tmp_path: Path, schema: Namespace) -> None: + _files( + tmp_path, + { + 'sub-01/func/sub-01_task-x_bold.nii.gz': '', + 'task-x_bold.json': {'TaskName': 'x', 'RepetitionTime': 2}, + 'sub-01/func/sub-01_task-x_bold.json': {'RepetitionTime': 3}, + }, + ) + ctx = _ctx(tmp_path, 'sub-01/func/sub-01_task-x_bold.nii.gz', schema) + assert ctx.sidecar_origins == { + 'RepetitionTime': 'sub-01/func/sub-01_task-x_bold.json', + 'TaskName': 'task-x_bold.json', + } + + +@pytest.mark.parametrize( + ('description', 'kind'), + [ + ({'Name': 't', 'BIDSVersion': '1.11.1'}, 'raw'), + ({'Name': 't', 'BIDSVersion': '1.11.1', 'GeneratedBy': [{'Name': 'x'}]}, 'derivative'), + ({'Name': 't', 'BIDSVersion': '1.11.1', 'DatasetType': 'study'}, 'study'), + ], +) +def test_dataset_type_defaults_like_the_reference( + tmp_path: Path, schema: Namespace, description: dict[str, object], kind: str +) -> None: + (tmp_path / 'dataset_description.json').write_text(json.dumps(description)) + dataset = context.Dataset(FileTree.read_from_filesystem(tmp_path), schema) + assert dataset.dataset_description.DatasetType == kind + + +def test_a_missing_description_is_empty_not_an_error(tmp_path: Path, schema: Namespace) -> None: + (tmp_path / 'README').write_text('x') + dataset = context.Dataset(FileTree.read_from_filesystem(tmp_path), schema) + assert dataset.dataset_description == {'DatasetType': 'raw'} + + +def test_a_record_keeps_dotted_keys() -> None: + """A Namespace reads "key_resp.rt" as a path; a FileContents keeps it a key.""" + record = context.FileContents({'key_resp.rt': (0.5,), 'onset': (1,)}) + assert list(record) == ['key_resp.rt', 'onset'] + assert record['key_resp.rt'] == (0.5,) + assert record.onset == (1,) + with pytest.raises(AttributeError): + _ = record.missing diff --git a/tests/test_expression_language.py b/tests/test_expression_language.py index dd53ce3..28939e6 100644 --- a/tests/test_expression_language.py +++ b/tests/test_expression_language.py @@ -1,15 +1,18 @@ import json +import math import typing as ty from collections.abc import Generator from pathlib import Path +from types import SimpleNamespace import fsspec +import numpy as np import pytest from bidsschematools.schema import load_schema from bidsschematools.types.namespace import Namespace from bids_validator import context -from bids_validator.expression_language import interpret +from bids_validator.expression_language import el_namespace, evaluate, interpret, parse, truthy from bids_validator.types.files import FileTree @@ -55,3 +58,141 @@ def test_interpret( result = interpret(expr, file_context) assert result == expected + + +# --- Behaviour beyond ``schema.meta.expression_tests`` ---------------------- +# +# The schema's own vectors (above) never compare numbers, never read a value from a +# real file, and never test truthiness of arrays. The schema's rules do all three: 30 +# of 129 ``rules.checks`` use < <= > >= or **, and 29 are gated on +# ``type() != "null"``. Each vector below pins one of those behaviours to +# the schema's documentation (bids-specification ``src/schema/README.md``, section +# "Expressions") or, where it is silent, to the reference implementation. + +EXTRA_VECTORS = [ + # Ordering operators. Two strings compare as text, anything else as numbers. + pytest.param('1 < 2', True, id='less-than'), + pytest.param('2 <= 2', True, id='less-or-equal'), + pytest.param('3 > 2', True, id='greater-than'), + pytest.param('2 >= 3', False, id='greater-or-equal'), + pytest.param('"a" < "b"', True, id='strings-compare-as-text'), + pytest.param('"10" < "9"', True, id='numeric-strings-still-compare-as-text'), + pytest.param('"10" > 9', True, id='string-and-number-compare-as-numbers'), + pytest.param('"n/a" > 0', False, id='not-a-number-compares-false'), + # README: "null == 1 -> false. Also <, >, <= and >=". + pytest.param('null < 1', False, id='null-less-than-is-false'), + pytest.param('null >= 0', False, id='null-greater-or-equal-is-false'), + # Arithmetic. README: null + 1, null - 1, null * 1, null / 1 are all null. + pytest.param('10 ** 2', 100, id='power'), + pytest.param('10 ** -3', 0.001, id='power-negative'), + pytest.param('null + 1', None, id='null-plus'), + pytest.param('null - 1', None, id='null-minus'), + pytest.param('null * 2', None, id='null-times'), + pytest.param('null / 2', None, id='null-divide'), + pytest.param('1 + "a"', '1a', id='plus-joins-when-either-side-is-text'), + pytest.param('-3 % 2', -1, id='remainder-keeps-the-sign-of-the-dividend'), + pytest.param('1 / 0', math.inf, id='divide-by-zero-is-infinity'), + # Truthiness follows JavaScript: empty arrays and objects are TRUE. + pytest.param('![]', False, id='empty-array-is-true'), + pytest.param('!{}', False, id='empty-object-is-true'), + pytest.param('!""', True, id='empty-string-is-false'), + pytest.param('!0', True, id='zero-is-false'), + pytest.param('[] && 1', 1, id='and-returns-right-operand'), + pytest.param('"" || 2', 2, id='or-returns-right-operand'), + # type() names every type the language has. + pytest.param('type("abc")', 'string', id='type-string'), + pytest.param('type(1.5)', 'number', id='type-number'), + pytest.param('type(false)', 'boolean', id='type-boolean'), + pytest.param('type([1])', 'array', id='type-array'), + pytest.param('type(missing_name)', 'null', id='type-of-missing'), + # Indexing out of range is null, not an error and not Python's count-from-the-end. + pytest.param('[1, 2][5]', None, id='index-out-of-range'), + pytest.param('[1, 2][-1]', None, id='negative-index'), + pytest.param('"ab"[1]', 'b', id='string-index'), + # ``in`` on an array tests membership; on null it is null. + pytest.param('"a" in ["a", "b"]', True, id='in-array'), + pytest.param('"x" in null', None, id='in-null'), + # Functions: numeric strings count as numbers (TSV cells are text) ... + pytest.param('min(["0.5", "n/a", "-1"])', -1.0, id='min-of-numeric-strings'), + pytest.param('max(["n/a"])', -math.inf, id='max-with-no-numbers'), + pytest.param('min([])', math.inf, id='min-of-empty'), + # ... and null or the wrong type gives null rather than raising. + pytest.param('length(3)', None, id='length-of-a-number'), + pytest.param('sorted(null)', None, id='sorted-null'), + pytest.param('count(null, 1)', None, id='count-in-null'), + pytest.param('count([1, 1], null)', None, id='count-of-null'), + pytest.param('intersects("rad", ["rad", "arbitrary"])', ['rad'], id='intersects-single-value'), + pytest.param('match(42, "4")', True, id='match-a-number'), + pytest.param('match("a", "(")', None, id='match-invalid-pattern'), + pytest.param('substr(12345, 1, 3)', '23', id='substr-of-a-number'), + # A name the context does not define is null, like a missing field. + pytest.param('missing_name', None, id='unknown-name'), + pytest.param('missing_name.field', None, id='field-of-unknown-name'), +] + + +@pytest.mark.parametrize(('expression', 'expected'), EXTRA_VECTORS) +def test_extra_vectors(expression: str, expected: ty.Any) -> None: + assert evaluate(parse(expression), el_namespace) == expected + + +def test_unknown_function_is_an_error_not_a_silent_null() -> None: + """A function the language does not have (``len`` instead of ``length``) raises. + + The rule engine reads that as null, but here it must stay visible, or a typo in a + rule would look like a passing check. + """ + with pytest.raises(TypeError, match='Unknown function'): + evaluate(parse('len([1, 2])'), el_namespace) + + +@pytest.mark.parametrize( + ('expression', 'expected'), + [ + # The NIfTI loader returns numpy arrays and scalars ... + pytest.param('type(array)', 'array', id='numpy-array-is-an-array'), + pytest.param('array[1]', 2, id='numpy-element-is-a-number'), + pytest.param('type(scalar)', 'number', id='numpy-scalar-is-a-number'), + pytest.param('scalar > 2', True, id='numpy-scalar-compares'), + # ... and the TSV loader returns tuples. + pytest.param('column == ["a", "b"]', True, id='tuple-equals-list'), + pytest.param('length(column)', 2, id='tuple-has-a-length'), + # A record such as ``associations`` is an object; a field holding None is absent. + pytest.param('type(record)', 'object', id='record-is-an-object'), + pytest.param('"bval" in record', True, id='record-field-present'), + pytest.param('"events" in record', False, id='record-field-none-is-absent'), + ], +) +def test_values_from_the_context_are_normalized(expression: str, expected: ty.Any) -> None: + namespace = { + 'array': np.array([1, 2, 3], dtype=np.int16), + 'scalar': np.float32(2.5), + 'column': ('a', 'b'), + 'record': SimpleNamespace(events=None, bval='x'), + } + assert evaluate(parse(expression), el_namespace | namespace) == expected + + +def test_in_does_not_modify_a_namespace() -> None: + """``".git" in namespace`` must not insert a key (bidsschematools reads dots as paths).""" + namespace = Namespace({'a': {'b': 1}}) + assert evaluate(parse('".git" in ns'), el_namespace | {'ns': namespace}) is False + assert list(namespace.keys()) == ['a'] + + +@pytest.mark.parametrize( + ('value', 'expected'), + [ + (None, False), + (False, False), + (0, False), + (math.nan, False), + ('', False), + ([], True), + ({}, True), + ('0', True), + (SimpleNamespace(), True), + ], +) +def test_truthy_follows_javascript(value: ty.Any, expected: bool) -> None: + assert truthy(value) is expected diff --git a/tests/test_issues.py b/tests/test_issues.py new file mode 100644 index 0000000..668004a --- /dev/null +++ b/tests/test_issues.py @@ -0,0 +1,97 @@ +"""Unit tests for the issues model (pure data, no fixtures needed).""" + +import attrs +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator.issues import DatasetIssues, Issue, IssueDefinition, 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' + + +def test_a_definition_makes_findings() -> None: + definition = IssueDefinition('EMPTY_FILE', Severity.ERROR, 'Empty files not allowed.') + issue = definition.make_issue('sub-01/x.nii.gz', sub_code='s', rule='rules.x') + assert issue == Issue( + code='EMPTY_FILE', + severity=Severity.ERROR, + location='sub-01/x.nii.gz', + message='Empty files not allowed.', # the reason, when the check has nothing else + sub_code='s', + rule='rules.x', + ) + assert definition.make_issue(None, message='specific').message == 'specific' + + +def test_a_definition_from_a_schema_issue_block() -> None: + block = {'code': 'X_CODE', 'level': 'warning', 'message': 'A message\nover two lines.\n'} + definition = IssueDefinition.from_schema(block) + assert definition == IssueDefinition('X_CODE', Severity.WARNING, 'A message over two lines.') + + +def test_a_field_issue_block_takes_the_severity_it_is_given() -> None: + """A field's issue leaves out its level: the field's requirement decides it.""" + definition = IssueDefinition.from_schema( + {'code': 'X', 'message': 'm'}, severity=Severity.ERROR + ) + assert definition.severity is Severity.ERROR + assert IssueDefinition.from_schema({'code': 'X', 'message': 'm'}).severity is Severity.ERROR + + +def test_a_definition_from_rules_errors(schema: Namespace) -> None: + definition = IssueDefinition.from_schema(schema.rules.errors.EmptyFile) + assert definition.code == 'EMPTY_FILE' + assert definition.reason == ' '.join(str(schema.rules.errors.EmptyFile.message).split()) diff --git a/tests/test_main.py b/tests/test_main.py new file mode 100644 index 0000000..5c2e1e8 --- /dev/null +++ b/tests/test_main.py @@ -0,0 +1,139 @@ +"""Tests for the command line. + +The CLI is a thin front end over :mod:`bids_validator.checks`. These tests cover the +part that is genuinely the CLI's own: what it prints, and what it exits with. The last +test is the important one. It pins the CLI to the module, so a second walk cannot +creep back into ``__main__``. +""" + +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace +from typer.testing import CliRunner + +from bids_validator.__main__ import app, validate +from bids_validator.checks import collect_issues +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +from .helpers import clean_dataset + +runner = CliRunner() + +BROKEN = 'sub-01/notes.txt' # not a BIDS name: exactly one finding, NOT_INCLUDED + + +def broken_dataset(root: pathlib.Path) -> pathlib.Path: + """The clean dataset plus one file BIDS does not include.""" + clean_dataset(root) + (root / BROKEN).write_text('notes') + return root + + +def test_validate_returns_the_findings_and_prints_them( + tmp_path: pathlib.Path, schema: Namespace, capsys: pytest.CaptureFixture[str] +) -> None: + """Every finding is both returned to the caller and shown to the user.""" + issues = validate(FileTree.read_from_filesystem(str(broken_dataset(tmp_path))), schema) + + assert len(issues) == 1 + assert issues.has_errors + + out = capsys.readouterr().out + assert 'error: NOT_INCLUDED: sub-01/notes.txt' in out + assert '1 error(s), 0 warning(s)' in out + + +def test_validate_says_so_when_there_is_nothing_wrong( + tmp_path: pathlib.Path, schema: Namespace, capsys: pytest.CaptureFixture[str] +) -> None: + """A clean dataset gets a clear answer, not silence.""" + issues = validate(FileTree.read_from_filesystem(str(clean_dataset(tmp_path))), schema) + + assert len(issues) == 0 + assert not issues.has_errors + assert 'No problems found' in capsys.readouterr().out + + +def test_verbose_adds_the_schema_rule( + tmp_path: pathlib.Path, schema: Namespace, capsys: pytest.CaptureFixture[str] +) -> None: + """``-v`` shows which rule produced the finding, so a user can check the standard.""" + clean_dataset(tmp_path) + (tmp_path / 'README').write_text('short') + tree = FileTree.read_from_filesystem(str(tmp_path)) + + validate(tree, schema, verbose=False) + assert 'rule:' not in capsys.readouterr().out + + validate(tree, schema, verbose=True) + assert 'rule: rules.checks.general.ReadmeFileSmall' in capsys.readouterr().out + + +def test_a_dataset_level_finding_has_no_file( + tmp_path: pathlib.Path, schema: Namespace, capsys: pytest.CaptureFixture[str] +) -> None: + clean_dataset(tmp_path) + (tmp_path / 'dataset_description.json').unlink() + validate(FileTree.read_from_filesystem(str(tmp_path)), schema) + assert 'error: MISSING_DATASET_DESCRIPTION: (dataset)' in capsys.readouterr().out + + +def test_cli_exit_code_is_one_when_there_are_errors( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """A CI job reads the exit code, not the printed text.""" + result = runner.invoke(app, [str(broken_dataset(tmp_path))]) + assert result.exit_code == 1 + assert 'NOT_INCLUDED' in result.stdout + + +def test_cli_exit_code_is_zero_with_only_warnings( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Warnings are shown but do not fail the build.""" + clean_dataset(tmp_path) + (tmp_path / 'README').write_text('short') + result = runner.invoke(app, [str(tmp_path)]) + assert result.exit_code == 0 + assert 'README_FILE_SMALL' in result.stdout + + +def test_cli_exit_code_is_zero_on_a_clean_dataset( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + result = runner.invoke(app, [str(clean_dataset(tmp_path))]) + assert result.exit_code == 0 + assert 'No problems found' in result.stdout + + +def test_cli_reports_exactly_what_the_module_reports( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """The CLI must not grow a second implementation of the checks. + + It once had its own tree walk and called ``is_bids`` per path, which could drift + from the module's results. Now it delegates, and this test fails if anything is + ever reported by one and not the other. + """ + broken_dataset(tmp_path) + for relpath in ('sub-01/anat/sub-01_T1w.txt', 'sub-02/anat/sub-01_T1w.nii.gz'): + (tmp_path / relpath).parent.mkdir(parents=True, exist_ok=True) + (tmp_path / relpath).write_bytes(b'') + expected = { + (i.code, i.location) + for i in collect_issues(FileTree.read_from_filesystem(str(tmp_path)), schema) + } + assert len(expected) > 3, 'the fixture should produce findings, otherwise this proves nothing' + + result = runner.invoke(app, [str(tmp_path)]) + + reported = set() + for line in result.stdout.splitlines(): + # "severity: CODE: location" + parts = line.split(': ') + if len(parts) == 3 and parts[0] in {s.value for s in Severity}: + reported.add((parts[1], parts[2])) + + assert reported == expected