diff --git a/CHANGELOG.md b/CHANGELOG.md index 89c937e80..5ed9984db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # HDMF Changelog +## HDMF 3.14.1 (June 3, 2024) + +### Bug Fixes +- Fixed issue with resolving attribute specs that have the same name at different levels of a spec hierarchy. + @rly [#1122](https://github.com/hdmf-dev/hdmf/pull/1122) + + ## HDMF 3.14.0 (May 20, 2024) ### Enhancements @@ -548,7 +555,7 @@ the fields (i.e., when the constructor sets some fields to fixed values). @rly Each sub-table is itself a DynamicTable that is aligned with the main table by row index. Each subtable defines a sub-category in the main table effectively creating a table with sub-headings to organize columns. @oruebel (#551) -- Add tutoral for new `AlignedDynamicTable` type. @oruebel (#571) +- Add tutorial for new `AlignedDynamicTable` type. @oruebel (#571) - Equality check for `DynamicTable` now also checks that the name and description of the table are the same. @rly (#566) ### Internal improvements diff --git a/pyproject.toml b/pyproject.toml index 67b13350b..f0a0b7fb7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -81,7 +81,7 @@ norecursedirs = "tests/unit/helpers" [tool.codespell] skip = "htmlcov,.git,.mypy_cache,.pytest_cache,.coverage,*.pdf,*.svg,venvs,.tox,hdmf-common-schema,./docs/_build/*,*.ipynb" -ignore-words-list = "datas" +ignore-words-list = "datas,assertIn" [tool.coverage.run] branch = true diff --git a/src/hdmf/data_utils.py b/src/hdmf/data_utils.py index 0e83bde2d..c03665caa 100644 --- a/src/hdmf/data_utils.py +++ b/src/hdmf/data_utils.py @@ -938,7 +938,7 @@ class ShapeValidatorResult: {'name': 'message', 'type': str, 'doc': 'Message describing the result of the shape validation', 'default': None}, {'name': 'ignored', 'type': tuple, - 'doc': 'Axes that have been ignored in the validaton process', 'default': tuple(), 'shape': (None,)}, + 'doc': 'Axes that have been ignored in the validation process', 'default': tuple(), 'shape': (None,)}, {'name': 'unmatched', 'type': tuple, 'doc': 'List of axes that did not match during shape validation', 'default': tuple(), 'shape': (None,)}, {'name': 'error', 'type': str, 'doc': 'Error that may have occurred. One of ERROR_TYPE', 'default': None}, diff --git a/src/hdmf/spec/spec.py b/src/hdmf/spec/spec.py index 585fc6494..1a6e8d987 100644 --- a/src/hdmf/spec/spec.py +++ b/src/hdmf/spec/spec.py @@ -385,7 +385,7 @@ def resolve_spec(self, **kwargs): self.set_attribute(attribute) self.__resolved = True - @docval({'name': 'spec', 'type': (Spec, str), 'doc': 'the specification to check'}) + @docval({'name': 'spec', 'type': Spec, 'doc': 'the specification to check'}) def is_inherited_spec(self, **kwargs): ''' Return True if this spec was inherited from the parent type, False otherwise. @@ -393,13 +393,11 @@ def is_inherited_spec(self, **kwargs): Returns False if the spec is not found. ''' spec = getargs('spec', kwargs) - if isinstance(spec, Spec): - spec = spec.name - if spec in self.__attributes: - return self.is_inherited_attribute(spec) + if spec.parent is self and spec.name in self.__attributes: + return self.is_inherited_attribute(spec.name) return False - @docval({'name': 'spec', 'type': (Spec, str), 'doc': 'the specification to check'}) + @docval({'name': 'spec', 'type': Spec, 'doc': 'the specification to check'}) def is_overridden_spec(self, **kwargs): ''' Return True if this spec overrides a specification from the parent type, False otherwise. @@ -407,10 +405,8 @@ def is_overridden_spec(self, **kwargs): Returns False if the spec is not found. ''' spec = getargs('spec', kwargs) - if isinstance(spec, Spec): - spec = spec.name - if spec in self.__attributes: - return self.is_overridden_attribute(spec) + if spec.parent is self and spec.name in self.__attributes: + return self.is_overridden_attribute(spec.name) return False @docval({'name': 'name', 'type': str, 'doc': 'the name of the attribute to check'}) @@ -1011,85 +1007,92 @@ def is_overridden_link(self, **kwargs): raise ValueError("Link '%s' not found in spec" % name) return name in self.__overridden_links - @docval({'name': 'spec', 'type': (Spec, str), 'doc': 'the specification to check'}) + @docval({'name': 'spec', 'type': Spec, 'doc': 'the specification to check'}) def is_inherited_spec(self, **kwargs): ''' Returns 'True' if specification was inherited from a parent type ''' spec = getargs('spec', kwargs) - if isinstance(spec, Spec): - name = spec.name - if name is None and hasattr(spec, 'data_type_def'): - name = spec.data_type_def - if name is None: # NOTE: this will return the target type for LinkSpecs - name = spec.data_type_inc - if name is None: # pragma: no cover - # this should not be possible - raise ValueError('received Spec with wildcard name but no data_type_inc or data_type_def') - spec = name + spec_name = spec.name + if spec_name is None and hasattr(spec, 'data_type_def'): + spec_name = spec.data_type_def + if spec_name is None: # NOTE: this will return the target type for LinkSpecs + spec_name = spec.data_type_inc + if spec_name is None: # pragma: no cover + # this should not be possible + raise ValueError('received Spec with wildcard name but no data_type_inc or data_type_def') # if the spec has a name, it will be found in __links/__groups/__datasets before __data_types/__target_types - if spec in self.__links: - return self.is_inherited_link(spec) - elif spec in self.__groups: - return self.is_inherited_group(spec) - elif spec in self.__datasets: - return self.is_inherited_dataset(spec) - elif spec in self.__data_types: + if spec_name in self.__links: + return self.is_inherited_link(spec_name) + elif spec_name in self.__groups: + return self.is_inherited_group(spec_name) + elif spec_name in self.__datasets: + return self.is_inherited_dataset(spec_name) + elif spec_name in self.__data_types: # NOTE: the same data type can be both an unnamed data type and an unnamed target type - return self.is_inherited_type(spec) - elif spec in self.__target_types: - return self.is_inherited_target_type(spec) + return self.is_inherited_type(spec_name) + elif spec_name in self.__target_types: + return self.is_inherited_target_type(spec_name) else: + # attribute spec if super().is_inherited_spec(spec): return True else: - for s in self.__datasets: - if self.is_inherited_dataset(s): - if self.__datasets[s].get_attribute(spec) is not None: - return True - for s in self.__groups: - if self.is_inherited_group(s): - if self.__groups[s].get_attribute(spec) is not None: - return True + parent_name = spec.parent.name + if parent_name is None: + parent_name = spec.parent.data_type + if isinstance(spec.parent, DatasetSpec): + if parent_name in self.__datasets: + if self.is_inherited_dataset(parent_name): + if self.__datasets[parent_name].get_attribute(spec_name) is not None: + return True + else: + if parent_name in self.__groups: + if self.is_inherited_group(parent_name): + if self.__groups[parent_name].get_attribute(spec_name) is not None: + return True return False - @docval({'name': 'spec', 'type': (Spec, str), 'doc': 'the specification to check'}) + @docval({'name': 'spec', 'type': Spec, 'doc': 'the specification to check'}) def is_overridden_spec(self, **kwargs): # noqa: C901 ''' Returns 'True' if specification overrides a specification from the parent type ''' spec = getargs('spec', kwargs) - if isinstance(spec, Spec): - name = spec.name - if name is None: - if isinstance(spec, LinkSpec): # unnamed LinkSpec cannot be overridden - return False - if spec.is_many(): # this is a wildcard spec, so it cannot be overridden - return False - name = spec.data_type_def - if name is None: # NOTE: this will return the target type for LinkSpecs - name = spec.data_type_inc - if name is None: # pragma: no cover - # this should not happen - raise ValueError('received Spec with wildcard name but no data_type_inc or data_type_def') - spec = name + spec_name = spec.name + if spec_name is None: + if isinstance(spec, LinkSpec): # unnamed LinkSpec cannot be overridden + return False + if spec.is_many(): # this is a wildcard spec, so it cannot be overridden + return False + spec_name = spec.data_type_def + if spec_name is None: # NOTE: this will return the target type for LinkSpecs + spec_name = spec.data_type_inc + if spec_name is None: # pragma: no cover + # this should not happen + raise ValueError('received Spec with wildcard name but no data_type_inc or data_type_def') # if the spec has a name, it will be found in __links/__groups/__datasets before __data_types/__target_types - if spec in self.__links: - return self.is_overridden_link(spec) - elif spec in self.__groups: - return self.is_overridden_group(spec) - elif spec in self.__datasets: - return self.is_overridden_dataset(spec) - elif spec in self.__data_types: - return self.is_overridden_type(spec) + if spec_name in self.__links: + return self.is_overridden_link(spec_name) + elif spec_name in self.__groups: + return self.is_overridden_group(spec_name) + elif spec_name in self.__datasets: + return self.is_overridden_dataset(spec_name) + elif spec_name in self.__data_types: + return self.is_overridden_type(spec_name) else: if super().is_overridden_spec(spec): # check if overridden attribute return True else: - for s in self.__datasets: - if self.is_overridden_dataset(s): - if self.__datasets[s].is_overridden_spec(spec): - return True - for s in self.__groups: - if self.is_overridden_group(s): - if self.__groups[s].is_overridden_spec(spec): - return True + parent_name = spec.parent.name + if parent_name is None: + parent_name = spec.parent.data_type + if isinstance(spec.parent, DatasetSpec): + if parent_name in self.__datasets: + if self.is_overridden_dataset(parent_name): + if self.__datasets[parent_name].is_overridden_spec(spec): + return True + else: + if parent_name in self.__groups: + if self.is_overridden_group(parent_name): + if self.__groups[parent_name].is_overridden_spec(spec): + return True return False @docval({'name': 'spec', 'type': (BaseStorageSpec, str), 'doc': 'the specification to check'}) diff --git a/tests/unit/spec_tests/test_group_spec.py b/tests/unit/spec_tests/test_group_spec.py index 9c117fa1f..00a937538 100644 --- a/tests/unit/spec_tests/test_group_spec.py +++ b/tests/unit/spec_tests/test_group_spec.py @@ -365,26 +365,22 @@ def test_resolved(self): self.assertTrue(self.inc_group_spec.resolved) def test_is_inherited_spec(self): - self.assertFalse(self.def_group_spec.is_inherited_spec('attribute1')) - self.assertFalse(self.def_group_spec.is_inherited_spec('attribute2')) - self.assertTrue(self.inc_group_spec.is_inherited_spec( - AttributeSpec('attribute1', 'my first attribute', 'text') - )) - self.assertTrue(self.inc_group_spec.is_inherited_spec('attribute1')) - self.assertTrue(self.inc_group_spec.is_inherited_spec('attribute2')) - self.assertFalse(self.inc_group_spec.is_inherited_spec('attribute3')) - self.assertFalse(self.inc_group_spec.is_inherited_spec('attribute4')) + self.assertFalse(self.def_group_spec.is_inherited_spec(self.def_group_spec.attributes[0])) + self.assertFalse(self.def_group_spec.is_inherited_spec(self.def_group_spec.attributes[1])) + + attr_spec_map = {attr.name: attr for attr in self.inc_group_spec.attributes} + self.assertTrue(self.inc_group_spec.is_inherited_spec(attr_spec_map["attribute1"])) + self.assertTrue(self.inc_group_spec.is_inherited_spec(attr_spec_map["attribute2"])) + self.assertFalse(self.inc_group_spec.is_inherited_spec(attr_spec_map["attribute3"])) def test_is_overridden_spec(self): - self.assertFalse(self.def_group_spec.is_overridden_spec('attribute1')) - self.assertFalse(self.def_group_spec.is_overridden_spec('attribute2')) - self.assertFalse(self.inc_group_spec.is_overridden_spec( - AttributeSpec('attribute1', 'my first attribute', 'text') - )) - self.assertFalse(self.inc_group_spec.is_overridden_spec('attribute1')) - self.assertTrue(self.inc_group_spec.is_overridden_spec('attribute2')) - self.assertFalse(self.inc_group_spec.is_overridden_spec('attribute3')) - self.assertFalse(self.inc_group_spec.is_overridden_spec('attribute4')) + self.assertFalse(self.def_group_spec.is_overridden_spec(self.def_group_spec.attributes[0])) + self.assertFalse(self.def_group_spec.is_overridden_spec(self.def_group_spec.attributes[0])) + + attr_spec_map = {attr.name: attr for attr in self.inc_group_spec.attributes} + self.assertFalse(self.inc_group_spec.is_overridden_spec(attr_spec_map["attribute1"])) + self.assertTrue(self.inc_group_spec.is_overridden_spec(attr_spec_map["attribute2"])) + self.assertFalse(self.inc_group_spec.is_overridden_spec(attr_spec_map["attribute3"])) def test_is_inherited_attribute(self): self.assertFalse(self.def_group_spec.is_inherited_attribute('attribute1')) @@ -405,6 +401,95 @@ def test_is_overridden_attribute(self): self.inc_group_spec.is_overridden_attribute('attribute4') +class TestResolveGroupSameAttributeName(TestCase): + # https://github.com/hdmf-dev/hdmf/issues/1121 + + def test_is_inherited_two_different_datasets(self): + self.def_group_spec = GroupSpec( + doc='A test group', + data_type_def='MyGroup', + datasets=[ + DatasetSpec( + name='dset1', + doc="dset1", + dtype='int', + attributes=[AttributeSpec('attr1', 'MyGroup.dset1.attr1', 'text')] + ), + ] + ) + self.inc_group_spec = GroupSpec( + doc='A test subgroup', + data_type_def='SubGroup', + data_type_inc='MyGroup', + datasets=[ + DatasetSpec( + name='dset2', + doc="dset2", + dtype='int', + attributes=[AttributeSpec('attr1', 'SubGroup.dset2.attr1', 'text')] + ), + ] + ) + self.inc_group_spec.resolve_spec(self.def_group_spec) + + self.assertFalse(self.def_group_spec.is_inherited_spec(self.def_group_spec.datasets[0].attributes[0])) + + dset_spec_map = {dset.name: dset for dset in self.inc_group_spec.datasets} + self.assertFalse(self.inc_group_spec.is_inherited_spec(dset_spec_map["dset2"].attributes[0])) + self.assertTrue(self.inc_group_spec.is_inherited_spec(dset_spec_map["dset1"].attributes[0])) + + def test_is_inherited_different_groups_and_datasets(self): + self.def_group_spec = GroupSpec( + doc='A test group', + data_type_def='MyGroup', + attributes=[AttributeSpec('attr1', 'MyGroup.attr1', 'text')], # <-- added from above + datasets=[ + DatasetSpec( + name='dset1', + doc="dset1", + dtype='int', + attributes=[AttributeSpec('attr1', 'MyGroup.dset1.attr1', 'text')] + ), + ] + ) + self.inc_group_spec = GroupSpec( + doc='A test subgroup', + data_type_def='SubGroup', + data_type_inc='MyGroup', + attributes=[AttributeSpec('attr1', 'SubGroup.attr1', 'text')], # <-- added from above + datasets=[ + DatasetSpec( + name='dset2', + doc="dset2", + dtype='int', + attributes=[AttributeSpec('attr1', 'SubGroup.dset2.attr1', 'text')] + ), + ] + ) + self.inc_group_spec.resolve_spec(self.def_group_spec) + + self.assertFalse(self.def_group_spec.is_inherited_spec(self.def_group_spec.datasets[0].attributes[0])) + + dset_spec_map = {dset.name: dset for dset in self.inc_group_spec.datasets} + self.assertFalse(self.inc_group_spec.is_inherited_spec(dset_spec_map["dset2"].attributes[0])) + self.assertTrue(self.inc_group_spec.is_inherited_spec(dset_spec_map["dset1"].attributes[0])) + self.assertTrue(self.inc_group_spec.is_inherited_spec(self.inc_group_spec.attributes[0])) + + self.inc_group_spec2 = GroupSpec( + doc='A test subsubgroup', + data_type_def='SubSubGroup', + data_type_inc='SubGroup', + ) + self.inc_group_spec2.resolve_spec(self.inc_group_spec) + + dset_spec_map = {dset.name: dset for dset in self.inc_group_spec2.datasets} + self.assertTrue(self.inc_group_spec2.is_inherited_spec(dset_spec_map["dset1"].attributes[0])) + self.assertTrue(self.inc_group_spec2.is_inherited_spec(dset_spec_map["dset2"].attributes[0])) + self.assertTrue(self.inc_group_spec2.is_inherited_spec(self.inc_group_spec2.attributes[0])) + + + + class GroupSpecWithLinksTest(TestCase): def test_constructor(self):