From 46d81cb23429076e452484c155433882d0df6575 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Mon, 5 Feb 2024 01:17:19 -0800 Subject: [PATCH] Fix validator not validating on spec of builder data type (#1050) --- CHANGELOG.md | 1 + src/hdmf/validate/validator.py | 21 +++++++-- tests/unit/validator_tests/test_validate.py | 52 +++++++++++++++++++++ 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5894f38ae..8e7888a54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Fixed retrieving the correct path for a `HERD` zip file on read. [#1046](https://github.com/hdmf-dev/hdmf/pull/1046) - Fixed internal links in docstrings and tutorials. @stephprince [#1031](https://github.com/hdmf-dev/hdmf/pull/1031) - Fixed issue with creating documentation links to classes in docval arguments. @rly [#1036](https://github.com/hdmf-dev/hdmf/pull/1036) +- Fixed issue with validator not validating against the spec that defines the data type of the builder. @rly [#1050](https://github.com/hdmf-dev/hdmf/pull/1050) ## HDMF 3.12.0 (January 16, 2024) diff --git a/src/hdmf/validate/validator.py b/src/hdmf/validate/validator.py index 6bea85975..bdfc15f8f 100644 --- a/src/hdmf/validate/validator.py +++ b/src/hdmf/validate/validator.py @@ -545,7 +545,8 @@ def __validate_child_builder(self, child_spec, child_builder, parent_builder): yield self.__construct_illegal_link_error(child_spec, parent_builder) return # do not validate illegally linked objects child_builder = child_builder.builder - for child_validator in self.__get_child_validators(child_spec): + child_builder_data_type = child_builder.attributes.get(self.spec.type_key()) + for child_validator in self.__get_child_validators(child_spec, child_builder_data_type): yield from child_validator.validate(child_builder) def __construct_illegal_link_error(self, child_spec, parent_builder): @@ -557,7 +558,7 @@ def __construct_illegal_link_error(self, child_spec, parent_builder): def __cannot_be_link(spec): return not isinstance(spec, LinkSpec) and not spec.linkable - def __get_child_validators(self, spec): + def __get_child_validators(self, spec, builder_data_type): """Returns the appropriate list of validators for a child spec Due to the fact that child specs can both inherit a data type via data_type_inc @@ -572,9 +573,21 @@ def __get_child_validators(self, spec): returned. If the spec is a LinkSpec, no additional Validator is returned because the LinkSpec cannot add or modify fields and the target_type will be validated by the Validator returned from the ValidatorMap. + + For example, if the spec is: + {'doc': 'Acquired, raw data.', 'quantity': '*', 'data_type_inc': 'NWBDataInterface'} + then the returned validators will be: + - a validator for the spec for the builder data type + - a validator for the spec for data_type_def: NWBDataInterface + - a validator for the above spec which might have extended properties + on top of data_type_def: NWBDataInterface """ - if _resolve_data_type(spec) is not None: - yield self.vmap.get_validator(_resolve_data_type(spec)) + if builder_data_type is not None: + yield self.vmap.get_validator(builder_data_type) + + spec_data_type = _resolve_data_type(spec) + if spec_data_type is not None: + yield self.vmap.get_validator(spec_data_type) if isinstance(spec, GroupSpec): yield GroupValidator(spec, self.vmap) diff --git a/tests/unit/validator_tests/test_validate.py b/tests/unit/validator_tests/test_validate.py index 7002ebd6f..95ff5d98e 100644 --- a/tests/unit/validator_tests/test_validate.py +++ b/tests/unit/validator_tests/test_validate.py @@ -1217,3 +1217,55 @@ def test_empty_int_dataset(self): DatasetBuilder(name='dataInt', data=[], dtype='int') # <-- Empty int dataset ]) self.runBuilderRoundTrip(builder) + + +class TestValidateSubspec(ValidatorTestBase): + """When a subtype satisfies a subspec, the validator should also validate + that the subtype satisfies its spec. + """ + + def getSpecs(self): + dataset_spec = DatasetSpec('A dataset', data_type_def='Foo') + sub_dataset_spec = DatasetSpec( + doc='A subtype of Foo', + data_type_def='Bar', + data_type_inc='Foo', + attributes=[ + AttributeSpec(name='attr1', doc='an example attribute', dtype='text') + ], + ) + spec = GroupSpec( + doc='A group that contains a Foo', + data_type_def='Baz', + datasets=[ + DatasetSpec(doc='Child Dataset', data_type_inc='Foo'), + ]) + return (spec, dataset_spec, sub_dataset_spec) + + def test_validate_subtype(self): + """Test that when spec A contains dataset B, and C is a subtype of B, using a C builder is valid. + """ + builder = GroupBuilder( + name='my_baz', + attributes={'data_type': 'Baz'}, + datasets=[ + DatasetBuilder(name='bar', attributes={'data_type': 'Bar', 'attr1': 'value'}) + ], + ) + result = self.vmap.validate(builder) + self.assertEqual(len(result), 0) + + def test_validate_subtype_error(self): + """Test that when spec A contains dataset B, and C is a subtype of B, using a C builder validates + against spec C. + """ + builder = GroupBuilder( + name='my_baz', + attributes={'data_type': 'Baz'}, + datasets=[ + DatasetBuilder(name='bar', attributes={'data_type': 'Bar'}) + ], + ) + result = self.vmap.validate(builder) + self.assertEqual(len(result), 1) + self.assertValidationError(result[0], MissingError, name='Bar/attr1')