Fix issue with resolving attribute specs with same name

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

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