Validate a name-matching builder against its named spec instead of a wildcard sibling

CHANGELOG.md

@@ -12,6 +12,7 @@
 - `HDMFIO.__del__` emits a `ResourceWarning` for an IO that was not closed instead of calling `close()`. Still-open IOs are flushed and closed by an `atexit` handler on the main thread. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547)
 
 ### Fixed
+- Fixed the validator letting a builder whose name matches an optional named sub-spec fall through to a sibling wildcard spec of a parent type, so a type mismatch validated clean (e.g. an `EventsTable` whose `duration` column is a plain `VectorData` rather than a `DurationVectorData`). An explicit name match in `SpecMatcher._filter_by_name` is now authoritative, and a builder that matches a named spec by name but not by data type is reported as an `IncorrectDataType` error. @adityasingh2400 [#1556](https://github.com/hdmf-dev/hdmf/pull/1556)
 - Fixed a deadlock when exporting a Zarr file to HDF5 with `HDF5IO.export`. A zarr array is now read into memory before the HDF5 write (in `__list_fill__`/`__scalar_fill__`), so the read does not run while h5py's global lock is held. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547)
 - Fixed `IndexError` when selecting an empty region from an in-memory `DynamicTableRegion` (e.g. `table["region"][i]` for a ragged region row that references no target rows, or an empty slice), both when the target table's columns hold their data as numpy arrays and when the target table has ragged columns. @h-mayorquin [#1549](https://github.com/hdmf-dev/hdmf/pull/1549)
 - Fixed the Jupyter HTML representation (`_repr_html_`) rendering a scalar numpy `bool` or `int` attribute (e.g. `np.bool_`, `np.int64`, as read back from an HDF5 attribute) as an expandable "array" block reporting `Shape: ()`, while `float` and `str` scalars rendered inline. `_unwrap_scalar` now also unwraps numpy scalars (`np.generic`) so every scalar renders inline consistently. @h-mayorquin [#1546](https://github.com/hdmf-dev/hdmf/pull/1546)

src/hdmf/validate/validator.py

@@ -608,11 +608,26 @@ def __validate_children(self, parent_builder):
                                  parent_builder.links.values())
         matcher.assign_to_specs(builder_children)
 
+        for child_spec, child_builder in matcher.type_mismatched_builders:
+            yield self.__construct_incorrect_data_type_error(child_spec, child_builder, parent_builder)
+
         for child_spec, matched_builders in matcher.spec_matches:
             yield from self.__validate_presence_and_quantity(child_spec, len(matched_builders), parent_builder)
             for child_builder in matched_builders:
                 yield from self.__validate_child_builder(child_spec, child_builder, parent_builder)
 
+    def __construct_incorrect_data_type_error(self, child_spec, child_builder, parent_builder):
+        """Returns an IncorrectDataType for a builder that matches a named child spec
+        by name but whose data type is not consistent with that spec.
+        """
+        if isinstance(child_builder, LinkBuilder):
+            child_builder = child_builder.builder
+        received = child_builder.attributes.get(self.spec.type_key())
+        return IncorrectDataType(self.get_spec_loc(child_spec),
+                                 expected=_resolve_data_type(child_spec),
+                                 received=received if received is not None else 'untyped',
+                                 location=self.get_builder_loc(parent_builder))
+
     def __validate_presence_and_quantity(self, child_spec, n_builders, parent_builder):
         """Validate that at least one matching builder exists if the spec is
         required and that the number of builders agrees with the spec quantity
@@ -750,6 +765,7 @@ def __init__(self, vmap, specs):
         self.vmap = vmap
         self._spec_matches = [SpecMatches(spec) for spec in specs]
         self._unmatched_builders = SpecMatches(None)
+        self._type_mismatched_builders = list()
 
     @property
     def unmatched_builders(self):
@@ -760,6 +776,16 @@ def unmatched_builders(self):
         """
         return self._unmatched_builders.builders
 
+    @property
+    def type_mismatched_builders(self):
+        """Returns a list of tuples of (spec, builder) for builders that matched a
+        named spec by name but whose data type is not consistent with that spec.
+
+        These builders are not assigned to any spec, so without reporting them here
+        the mismatch would be silent whenever the named spec is optional.
+        """
+        return self._type_mismatched_builders
+
     @property
     def spec_matches(self):
         """Returns a list of tuples of (spec, assigned builders)"""
@@ -795,9 +821,10 @@ def _best_matching_spec(self, builder):
         inheritance hierarchy. Future improvements to this matching algorithm
         should resolve these discrepancies.
         """
-        candidates = self._filter_by_name(self._spec_matches, builder)
-        candidates = self._filter_by_type(candidates, builder)
+        name_candidates = self._filter_by_name(self._spec_matches, builder)
+        candidates = self._filter_by_type(name_candidates, builder)
         if len(candidates) == 0:
+            self._record_type_mismatch(name_candidates, builder)
             return None
         elif len(candidates) == 1:
             return candidates[0]
@@ -811,12 +838,30 @@ def _best_matching_spec(self, builder):
     def _filter_by_name(self, candidates, builder):
         """Returns the candidate specs that either have the same name as the
         builder or do not specify a name.
+
+        A spec that names the builder explicitly is authoritative, so when any
+        candidate does so, the candidates that do not specify a name are dropped.
+        Without this, a builder named after an optional named sub-spec whose data
+        type does not match would fall through to an unnamed sibling spec, such as
+        a wildcard spec of a parent type, and validate clean.
         """
-        def name_is_consistent(spec_matches):
-            spec = spec_matches.spec
-            return spec.name is None or spec.name == builder.name
+        named_candidates = [sm for sm in candidates if sm.spec.name == builder.name]
+        if named_candidates:
+            return named_candidates
+        return [sm for sm in candidates if sm.spec.name is None]
 
-        return list(filter(name_is_consistent, candidates))
+    def _record_type_mismatch(self, name_candidates, builder):
+        """Records a builder that matched a named spec by name but not by data type.
+
+        Such a builder is left unassigned, and an optional spec with no assigned
+        builders reports nothing, so the mismatch is recorded here in order for the
+        validator to report it.
+        """
+        for spec_matches in name_candidates:
+            spec = spec_matches.spec
+            if spec.name == builder.name and _resolve_data_type(spec) is not None:
+                self._type_mismatched_builders.append((spec, builder))
+                return
 
     def _filter_by_type(self, candidates, builder):
         """Returns the candidate specs which have a data type consistent with

tests/unit/validator_tests/test_validate.py

@@ -878,6 +878,70 @@ def test_no_errors_when_all_children_satisfied(self):
         self.assertEqual(len(result), 0)
 
 
+class TestNamedSubspecTakesPrecedenceOverWildcard(TestCase):
+    """A builder whose name matches a named sub-spec is validated against that
+    sub-spec instead of falling through to an unnamed sibling wildcard spec of a
+    parent type. See https://github.com/hdmf-dev/hdmf/issues/1554
+    """
+
+    def set_up_spec(self, named_spec_quantity):
+        spec_catalog = SpecCatalog()
+        base_spec = DatasetSpec('A base vector', data_type_def='BaseVector', dtype='int')
+        typed_spec = DatasetSpec('A typed vector', data_type_def='TypedVector', data_type_inc='BaseVector')
+        container_spec = GroupSpec(
+            'A container holding vectors',
+            data_type_def='Container',
+            datasets=[
+                DatasetSpec('Any number of vectors', data_type_inc='BaseVector', quantity=ZERO_OR_MANY),
+                DatasetSpec('A named typed vector', name='col1', data_type_inc='TypedVector',
+                            quantity=named_spec_quantity),
+            ],
+        )
+        for spec in (base_spec, typed_spec, container_spec):
+            spec_catalog.register_spec(spec, 'test.yaml')
+        self.namespace = SpecNamespace(
+            'a test namespace', CORE_NAMESPACE, [{'source': 'test.yaml'}], version='0.1.0', catalog=spec_catalog)
+        self.vmap = ValidatorMap(self.namespace)
+
+    def validate_datasets(self, dataset_names_and_types, named_spec_quantity=ZERO_OR_ONE):
+        """Validate a Container builder holding the given (name, data_type) datasets"""
+        self.set_up_spec(named_spec_quantity)
+        datasets = [DatasetBuilder(name, [1, 2, 3], attributes={'data_type': data_type})
+                    for name, data_type in dataset_names_and_types]
+        builder = GroupBuilder('my_container', attributes={'data_type': 'Container'}, datasets=datasets)
+        return self.vmap.validate(builder)
+
+    def test_named_subspec_with_matching_type_is_valid(self):
+        """A builder named col1 whose type matches the named spec validates cleanly"""
+        result = self.validate_datasets([('col1', 'TypedVector')])
+        self.assertEqual(result, [])
+
+    def test_optional_named_subspec_with_wrong_type_returns_error(self):
+        """A builder named col1 of the wildcard's parent type does not fall through to
+        the wildcard spec, so the type mismatch is reported
+        """
+        result = self.validate_datasets([('col1', 'BaseVector')])
+        self.assertEqual(len(result), 1)
+        self.assertIsInstance(result[0], IncorrectDataType)
+        self.assertEqual(result[0].name, 'Container/col1')
+        self.assertEqual(result[0].reason, "incorrect data_type - expected 'TypedVector', got 'BaseVector'")
+
+    def test_required_named_subspec_with_wrong_type_returns_error(self):
+        """The same mismatch is reported when the named spec is required"""
+        result = self.validate_datasets([('col1', 'BaseVector')], named_spec_quantity=1)
+        self.assertTrue(any(isinstance(error, IncorrectDataType) for error in result))
+
+    def test_unnamed_builder_still_matches_wildcard(self):
+        """A builder whose name does not match the named spec still matches the wildcard"""
+        result = self.validate_datasets([('other', 'BaseVector')])
+        self.assertEqual(result, [])
+
+    def test_named_and_wildcard_builders_together_are_valid(self):
+        """Both specs are satisfied when each builder has the right type"""
+        result = self.validate_datasets([('col1', 'TypedVector'), ('other', 'BaseVector')])
+        self.assertEqual(result, [])
+
+
 class TestLinkAndChildMatchingDataType(TestCase):
     """If a link and a child dataset/group have the same specified data type,
     both the link and the child need to be validated