Fix TypeError when validating illegally-linked children (#515)

CHANGELOG.md

@@ -14,6 +14,8 @@
 - Allow `np.bool_` as a valid `bool` dtype when validating. @dsleiter (#505)
 - Fix building of Data objects where the spec has no dtype and the Data object value is a DataIO wrapping an
   AbstractDataChunkIterator. @rly (#512)
+- Fix TypeError when validating a group with an illegally-linked child.
+  @dsleiter (#515)
 - Fix `DynamicTable.get` for compound type columns. @rly (#518)
 - Fix and removed error "Field 'x' cannot be defined in EllipseSeries." when opening files with some extensions. @rly
   (#519)

src/hdmf/validate/errors.py

@@ -155,7 +155,7 @@ class IllegalLinkError(Error):
             {'name': 'location', 'type': str, 'doc': 'the location of the error', 'default': None})
     def __init__(self, **kwargs):
         name = getargs('name', kwargs)
-        reason = "illegal use of link"
+        reason = "illegal use of link (linked object will not be validated)"
         loc = getargs('location', kwargs)
         super().__init__(name, reason, location=loc)
 

src/hdmf/validate/validator.py

@@ -467,11 +467,11 @@ def __validate_child_data_type(self, builder, child_dt, child_spec, grouped_buil
             dt_builders = self.__filter_by_name_if_required(builders_matching_type, child_spec.name)
             for child_builder in dt_builders:
                 if isinstance(child_builder, LinkBuilder):
-                    if isinstance(child_spec, LinkSpec) or child_spec.linkable:
-                        child_builder = child_builder.builder
-                    else:
+                    if self.__cannot_be_link(child_spec):
                         errors.append(IllegalLinkError(self.get_spec_loc(child_spec),
                                                        location=self.get_builder_loc(child_builder)))
+                        continue  # do not validate illegally linked objects
+                    child_builder = child_builder.builder
                 errors.extend(sub_val.validate(child_builder))
                 n_matching_builders += 1
         if n_matching_builders == 0 and child_spec.required:
@@ -482,17 +482,21 @@ def __validate_child_data_type(self, builder, child_dt, child_spec, grouped_buil
                                                  n_matching_builders, location=self.get_builder_loc(builder)))
         return errors
 
+    @staticmethod
+    def __cannot_be_link(spec):
+        return not isinstance(spec, LinkSpec) and not spec.linkable
+
     def __validate_untyped_child(self, builder, child_name, child_validator):
         """Validate the named children of parent_builder which have no defined data type"""
         errors = []
         child_builder = builder.get(child_name)
         child_spec = child_validator.spec
         if isinstance(child_builder, LinkBuilder):
-            if child_spec.linkable:
-                child_builder = child_builder.builder
-            else:
+            if not child_spec.linkable:
                 errors.append(IllegalLinkError(self.get_spec_loc(child_spec), location=self.get_builder_loc(builder)))
-        elif child_builder is None:
+                return errors  # do not validate illegally linked objects
+            child_builder = child_builder.builder
+        if child_builder is None:
             if child_spec.required:
                 errors.append(MissingError(self.get_spec_loc(child_spec), location=self.get_builder_loc(builder)))
         else:

tests/unit/validator_tests/test_validate.py

@@ -1,5 +1,6 @@
 from abc import ABCMeta, abstractmethod
 from datetime import datetime
+from unittest import mock
 
 import numpy as np
 from dateutil.tz import tzlocal
@@ -8,7 +9,8 @@
 from hdmf.spec.spec import ONE_OR_MANY, ZERO_OR_MANY, ZERO_OR_ONE
 from hdmf.testing import TestCase
 from hdmf.validate import ValidatorMap
-from hdmf.validate.errors import DtypeError, MissingError, ExpectedArrayError, MissingDataType, IncorrectQuantityError
+from hdmf.validate.errors import (DtypeError, MissingError, ExpectedArrayError, MissingDataType,
+                                  IncorrectQuantityError, IllegalLinkError)
 
 CORE_NAMESPACE = 'test_core'
 
@@ -490,3 +492,109 @@ def test_empty_nparray(self):
         self.assertEqual(len(results), 0)
 
     # TODO test shape validation more completely
+
+
+class TestLinkable(TestCase):
+
+    def set_up_spec(self):
+        spec_catalog = SpecCatalog()
+        typed_dataset_spec = DatasetSpec('A typed dataset', data_type_def='Foo')
+        typed_group_spec = GroupSpec('A typed group', data_type_def='Bar')
+        spec = GroupSpec('A test group specification with a data type',
+                         data_type_def='Baz',
+                         datasets=[
+                             DatasetSpec('A linkable child dataset', name='untyped_linkable_ds',
+                                         linkable=True, quantity=ZERO_OR_ONE),
+                             DatasetSpec('A non-linkable child dataset', name='untyped_nonlinkable_ds',
+                                         linkable=False, quantity=ZERO_OR_ONE),
+                             DatasetSpec('A linkable child dataset', data_type_inc='Foo',
+                                         name='typed_linkable_ds', linkable=True, quantity=ZERO_OR_ONE),
+                             DatasetSpec('A non-linkable child dataset', data_type_inc='Foo',
+                                         name='typed_nonlinkable_ds', linkable=False, quantity=ZERO_OR_ONE),
+                         ],
+                         groups=[
+                             GroupSpec('A linkable child group', name='untyped_linkable_group',
+                                       linkable=True, quantity=ZERO_OR_ONE),
+                             GroupSpec('A non-linkable child group', name='untyped_nonlinkable_group',
+                                       linkable=False, quantity=ZERO_OR_ONE),
+                             GroupSpec('A linkable child group', data_type_inc='Bar',
+                                       name='typed_linkable_group', linkable=True, quantity=ZERO_OR_ONE),
+                             GroupSpec('A non-linkable child group', data_type_inc='Bar',
+                                       name='typed_nonlinkable_group', linkable=False, quantity=ZERO_OR_ONE),
+                         ])
+        spec_catalog.register_spec(spec, 'test.yaml')
+        spec_catalog.register_spec(typed_dataset_spec, 'test.yaml')
+        spec_catalog.register_spec(typed_group_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_linkability(self, link, expect_error):
+        """Execute a linkability test and assert whether or not an IllegalLinkError is returned"""
+        self.set_up_spec()
+        builder = GroupBuilder('my_baz', attributes={'data_type': 'Baz'}, links=[link])
+        result = self.vmap.validate(builder)
+        if expect_error:
+            self.assertEqual(len(result), 1)
+            self.assertIsInstance(result[0], IllegalLinkError)
+        else:
+            self.assertEqual(len(result), 0)
+
+    def test_untyped_linkable_dataset_accepts_link(self):
+        """Test that the validator accepts a link when the spec has an untyped linkable dataset"""
+        link = LinkBuilder(name='untyped_linkable_ds', builder=DatasetBuilder('foo'))
+        self.validate_linkability(link, expect_error=False)
+
+    def test_untyped_nonlinkable_dataset_does_not_accept_link(self):
+        """Test that the validator returns an IllegalLinkError when the spec has an untyped non-linkable dataset"""
+        link = LinkBuilder(name='untyped_nonlinkable_ds', builder=DatasetBuilder('foo'))
+        self.validate_linkability(link, expect_error=True)
+
+    def test_typed_linkable_dataset_accepts_link(self):
+        """Test that the validator accepts a link when the spec has a typed linkable dataset"""
+        link = LinkBuilder(name='typed_linkable_ds',
+                           builder=DatasetBuilder('foo', attributes={'data_type': 'Foo'}))
+        self.validate_linkability(link, expect_error=False)
+
+    def test_typed_nonlinkable_dataset_does_not_accept_link(self):
+        """Test that the validator returns an IllegalLinkError when the spec has a typed non-linkable dataset"""
+        link = LinkBuilder(name='typed_nonlinkable_ds',
+                           builder=DatasetBuilder('foo', attributes={'data_type': 'Foo'}))
+        self.validate_linkability(link, expect_error=True)
+
+    def test_untyped_linkable_group_accepts_link(self):
+        """Test that the validator accepts a link when the spec has an untyped linkable group"""
+        link = LinkBuilder(name='untyped_linkable_group', builder=GroupBuilder('foo'))
+        self.validate_linkability(link, expect_error=False)
+
+    def test_untyped_nonlinkable_group_does_not_accept_link(self):
+        """Test that the validator returns an IllegalLinkError when the spec has an untyped non-linkable group"""
+        link = LinkBuilder(name='untyped_nonlinkable_group', builder=GroupBuilder('foo'))
+        self.validate_linkability(link, expect_error=True)
+
+    def test_typed_linkable_group_accepts_link(self):
+        """Test that the validator accepts a link when the spec has a typed linkable group"""
+        link = LinkBuilder(name='typed_linkable_group',
+                           builder=GroupBuilder('foo', attributes={'data_type': 'Bar'}))
+        self.validate_linkability(link, expect_error=False)
+
+    def test_typed_nonlinkable_group_does_not_accept_link(self):
+        """Test that the validator returns an IllegalLinkError when the spec has a typed non-linkable group"""
+        link = LinkBuilder(name='typed_nonlinkable_group',
+                           builder=GroupBuilder('foo', attributes={'data_type': 'Bar'}))
+        self.validate_linkability(link, expect_error=True)
+
+    @mock.patch("hdmf.validate.validator.DatasetValidator.validate")
+    def test_should_not_validate_illegally_linked_objects(self, mock_validator):
+        """Test that an illegally linked child dataset is not validated
+
+        Note: this behavior is expected to change in the future:
+        https://github.com/hdmf-dev/hdmf/issues/516
+        """
+        self.set_up_spec()
+        typed_link = LinkBuilder(name='typed_nonlinkable_ds',
+                                 builder=DatasetBuilder('foo', attributes={'data_type': 'Foo'}))
+        untyped_link = LinkBuilder(name='untyped_nonlinkable_ds', builder=DatasetBuilder('foo'))
+        builder = GroupBuilder('my_baz', attributes={'data_type': 'Baz'}, links=[typed_link, untyped_link])
+        _ = self.vmap.validate(builder)
+        assert not mock_validator.called