Merge pull request #1917 from Remi-Gau/check_metadata

[SCHEMA] Check sidecar fields mentioned in rule checks exist in the schema

src/schema/rules/checks/asl.yaml

@@ -196,21 +196,21 @@ ASLBackgroundSuppressionNumberPulses:
     - length(sidecar.BackgroundSuppressionPulseTime) == sidecar.BackgroundSuppressionNumberPulses
 
 # 181
-ASLTotalAcquiredVolumesASLContextLength:
+ASLTotalAcquiredPairsASLContextLength:
   issue:
     code: TOTAL_ACQUIRED_VOLUMES_NOT_CONSISTENT
     message: |
-      The number of values for 'TotalAcquiredVolumes' for this file does not match number of
+      The number of values for 'TotalAcquiredPairs' for this file does not match number of
       volumes in the associated 'aslcontext.tsv'.
-      'TotalAcquiredVolumes' is the original number of 3D volumes acquired for each volume defined in the
+      'TotalAcquiredPairs' is the original number of 3D volumes acquired for each volume defined in the
       associated 'aslcontext.tsv'.
     level: warning
   selectors:
     - suffix == "asl"
     - type(associations.aslcontext) != "null"
-    - '"TotalAcquiredVolumes" in sidecar'
+    - '"TotalAcquiredPairs" in sidecar'
   checks:
-    - aslcontext.n_rows == sidecar.TotalAcquiredVolumes
+    - aslcontext.n_rows == sidecar.TotalAcquiredPairs
 
 # 184
 PostLabelingDelayGreater:

src/schema/rules/checks/micr.yaml

@@ -10,17 +10,17 @@ PixelSizeInconsistent:
   selectors:
     - ome != null
     - sidecar.PixelSize != null
-    - sidecar.PixelSizeUnit != null
+    - sidecar.PixelSizeUnits != null
   checks:
     - |
       ome.PhysicalSizeX * 10 ** (-3 * index(["mm", "um", "nm"], ome.PhysicalSizeXUnit))
-      == sidecar.PixelSize[0] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnit))
+      == sidecar.PixelSize[0] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnits))
     - |
       ome.PhysicalSizeY * 10 ** (-3 * index(["mm", "um", "nm"], ome.PhysicalSizeYUnit))
-      == sidecar.PixelSize[1] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnit))
+      == sidecar.PixelSize[1] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnits))
     - |
       ome.PhysicalSizeZ * 10 ** (-3 * index(["mm", "um", "nm"], ome.PhysicalSizeZUnit))
-      == sidecar.PixelSize[2] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnit))
+      == sidecar.PixelSize[2] * 10 ** (-3 * index(["mm", "um", "nm"], sidecar.PixelSizeUnits))
 
 # 227
 InconsistentTiffExtension:

src/schema/rules/checks/mri.yaml

@@ -109,9 +109,9 @@ BolusCutOffDelayTimeNotMonotonicallyIncreasing:
     level: error
   selectors:
     - modality == "mri"
-    - sidecar.BolusCutoffDelayTime != null
+    - sidecar.BolusCutOffDelayTime != null
   checks:
-    - allequal(sorted(sidecar.BolusCutoffDelayTime), sidecar.BolusCutoffDelayTime)
+    - allequal(sorted(sidecar.BolusCutOffDelayTime), sidecar.BolusCutOffDelayTime)
 
 # 201
 RepetitionTimePreparationNotConsistent:

tools/schemacode/bidsschematools/tests/test_expressions.py

@@ -1,7 +1,21 @@
+from collections.abc import Mapping
+from functools import singledispatch
+from typing import Union
+
 import pytest
 from pyparsing.exceptions import ParseException
 
-from ..expressions import ASTNode, expression
+from ..expressions import (
+    Array,
+    ASTNode,
+    BinOp,
+    Element,
+    Function,
+    Property,
+    RightOp,
+    expression,
+)
+from ..types import Namespace
 
 
 def test_schema_expressions(schema_obj):
@@ -77,3 +91,95 @@ def test_checks(schema_obj):
 def test_expected_failures(expr):
     with pytest.raises(ParseException):
         expression.parse_string(expr)
+
+
+def walk_schema(schema_obj, predicate):
+    for key, value in schema_obj.items():
+        if predicate(key, value):
+            yield key, value
+        if isinstance(value, Mapping):
+            for subkey, value in walk_schema(value, predicate):
+                yield f"{key}.{subkey}", value
+
+
+def test_valid_sidecar_field(schema_obj):
+    """Check sidecar fields actually exist in the metadata listed in the schema.
+
+    Test failures are usually due to typos.
+    """
+    field_names = {field.name for key, field in schema_obj.objects.metadata.items()}
+
+    for key, rule in walk_schema(
+        schema_obj.rules, lambda k, v: isinstance(v, Mapping) and v.get("selectors")
+    ):
+        for selector in rule["selectors"]:
+            ast = expression.parse_string(selector)[0]
+            for name in find_names(ast):
+                if name.startswith(("json.", "sidecar.")):
+                    assert (
+                        name.split(".", 1)[1] in field_names
+                    ), f"Bad field in selector: {name} ({key})"
+        for check in rule.get("checks", []):
+            ast = expression.parse_string(check)[0]
+            for name in find_names(ast):
+                if name.startswith(("json.", "sidecar.")):
+                    assert (
+                        name.split(".", 1)[1] in field_names
+                    ), f"Bad field in check: {name} ({key})"
+
+
+def test_test_valid_sidecar_field():
+    schema_obj = Namespace.build(
+        {
+            "objects": {
+                "metadata": {
+                    "a": {"name": "a"},
+                }
+            },
+            "rules": {"myruleA": {"selectors": ["sidecar.a"], "checks": ["json.a == sidecar.a"]}},
+        }
+    )
+    test_valid_sidecar_field(schema_obj)
+
+    schema_obj.objects.metadata.a["name"] = "b"
+    with pytest.raises(AssertionError):
+        test_valid_sidecar_field(schema_obj)
+
+
+@singledispatch
+def find_names(node: Union[ASTNode, str]):
+    # Walk AST nodes
+    if isinstance(node, BinOp):
+        yield from find_names(node.lh)
+        yield from find_names(node.rh)
+    elif isinstance(node, RightOp):
+        yield from find_names(node.rh)
+    elif isinstance(node, Array):
+        for element in node.elements:
+            yield from find_names(element)
+    elif isinstance(node, Element):
+        yield from find_names(node.name)
+        yield from find_names(node.index)
+    elif isinstance(node, (int, float)):
+        return
+    else:
+        raise TypeError(f"Unexpected node type: {node!r}")
+
+
+@find_names.register
+def find_function_names(node: Function):
+    yield node.name
+    for arg in node.args:
+        yield from find_names(arg)
+
+
+@find_names.register
+def find_property_name(node: Property):
+    # Properties are left-associative, so expand the left side
+    yield f"{next(find_names(node.name))}.{node.field}"
+
+
+@find_names.register
+def find_identifiers(node: str):
+    if not node.startswith(('"', "'")):
+        yield node