diff --git a/CHANGELOG.md b/CHANGELOG.md index a2f467cf0..0e229346f 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/hdmf/validate/errors.py b/src/hdmf/validate/errors.py index 02ef274ee..1a6aeb904 100644 --- a/src/hdmf/validate/errors.py +++ b/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) diff --git a/src/hdmf/validate/validator.py b/src/hdmf/validate/validator.py index 19a86c56f..6cb90cf39 100644 --- a/src/hdmf/validate/validator.py +++ b/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: diff --git a/tests/unit/validator_tests/test_validate.py b/tests/unit/validator_tests/test_validate.py index a71ad34bc..b079c881f 100644 --- a/tests/unit/validator_tests/test_validate.py +++ b/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