Fix issue with resolving attribute specs with same name (#1122)

* Fix issue with resolving attribute specs with same name

* fix codespell

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

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

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},

src/hdmf/spec/spec.py

@@ -385,32 +385,28 @@ 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.
 
         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.
 
         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'})

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):