Allow reassigning and resetting AbstractContainer attributes/fields

CHANGELOG.md

@@ -1,5 +1,19 @@
 # HDMF Changelog
 
+## HDMF 3.9.0 (Upcoming)
+
+### New features and minor improvements
+- Allow reassigning and resetting attributes on `AbstractContainer` objects, including the `data` attribute of `Data`
+  objects and `table` attribute of `DynamicTableRegion` objects. If the attributes were read from a file (i.e.,
+  `container_source` is not None), then a warning is raised. Note that type, shape, and is-required validation are NOT
+  performed when attributes are reassigned. This should be done carefully and may result in an error on write.
+  @rly [#868](https://github.com/hdmf-dev/hdmf/pull/868)
+- When calling an attribute setter for an `AbstractContainer`, if the value is None, then the `fields` dictionary will
+  be updated and set the field to None. Previously, the `fields` dictionary was not updated.
+  @rly [#868](https://github.com/hdmf-dev/hdmf/pull/868)
+- Deprecated `Data.set_dataio`, which has been broken for some use cases. Reassign the `data` attribute on the
+  `Data` object to the new `DataIO` object instead. @rly [#868](https://github.com/hdmf-dev/hdmf/pull/868)
+
 ## HDMF 3.8.1 (July 25, 2023)
 
 ### Bug fixes

src/hdmf/common/table.py

@@ -1288,15 +1288,15 @@ def table(self, val):
         :raises: AttributeError if table is already in fields
         :raises: IndexError if the current indices are out of bounds for the new table given by val
         """
-        if val is None:
-            return
-        if 'table' in self.fields:
-            msg = "can't set attribute 'table' -- already set"
-            raise AttributeError(msg)
+        if self.container_source is not None and not self._in_construct_mode:
+            warn(f"Container was read from file '{self.container_source}'. "
+                    f"Changing the value of attribute 'table' will not change the value in the file. "
+                    f"Use the export function to write the modified container to a new file.")
+        self.fields["table"] = val
+        self.set_modified()
         dat = self.data
         if isinstance(dat, DataIO):
             dat = dat.data
-        self.fields['table'] = val
 
     def __getitem__(self, arg):
         return self.get(arg)

src/hdmf/container.py

@@ -80,12 +80,12 @@ def _setter(cls, field):
             return None
 
         def setter(self, val):
-            if val is None:
-                return
-            if name in self.fields:
-                msg = "can't set attribute '%s' -- already set" % name
-                raise AttributeError(msg)
+            if self.container_source is not None and not self._in_construct_mode:
+                warn(f"Container was read from file '{self.container_source}'. "
+                     f"Changing the value of attribute '{name}' will not change the value in the file. "
+                     f"Use the export function to write the modified container to a new file.")
             self.fields[name] = val
+            self.set_modified()
 
         return setter
 
@@ -727,6 +727,18 @@ def __init__(self, **kwargs):
     def data(self):
         return self.__data
 
+    @data.setter
+    def data(self, val):
+        if val is None:
+            raise ValueError("data cannot be set to None.")
+
+        if self.container_source is not None and not self._in_construct_mode:
+            warn(f"Container was read from file '{self.container_source}'. "
+                    f"Changing the value of attribute 'data' will not change the value in the file. "
+                    f"Use the export function to write the modified container to a new file.")
+        self.__data = val
+        self.set_modified()
+
     @property
     def shape(self):
         """
@@ -741,6 +753,9 @@ def set_dataio(self, **kwargs):
         """
         Apply DataIO object to the data held by this Data object
         """
+        # TODO deprecate this in favor of the user wrapping the data themselves in a non-buggy way
+        warn("set_dataio is deprecated. Use the DataIO constructor to wrap the data and reassign this object's "
+             "data field to the DataIO object.", DeprecationWarning)
         dataio = getargs('dataio', kwargs)
         dataio.data = self.__data
         self.__data = dataio

tests/unit/common/test_table.py

@@ -1076,6 +1076,19 @@ def test_no_df_nested(self):
         with self.assertRaisesWith(ValueError, msg):
             dynamic_table_region.get(0, df=False, index=False)
 
+    def test_reset_table_attr(self):
+        table = self.with_columns_and_data()
+        dynamic_table_region = DynamicTableRegion(name='dtr', data=[1, 2, 2], description='desc', table=table)
+        dynamic_table_region.table = None  # can set to None value as of HDMF 3.7.0
+        self.assertIsNone(dynamic_table_region.table)
+
+    def test_reassign_table_attr(self):
+        table1 = self.with_columns_and_data()
+        table2 = self.with_columns_and_data()
+        dynamic_table_region = DynamicTableRegion(name='dtr', data=[1, 2, 2], description='desc', table=table1)
+        dynamic_table_region.table = table2  # can reassign table attribute as of HDMF 3.7.0
+        self.assertIs(dynamic_table_region.table, table2)   # check that table was reassigned
+
 
 class DynamicTableRegionRoundTrip(H5RoundTripMixin, TestCase):
 
@@ -1300,6 +1313,25 @@ def test_getitem_str(self):
         rec = table['dtr']['qux']
         self.assertIs(rec, mc.containers['target_table']['qux'])
 
+    def test_reset_table_attr(self):
+        mc = self.roundtripContainer()
+        table = mc.containers['table_with_dtr']
+        msg = (r"Container was read from file '.*'\. Changing the value of attribute 'table' will not change the "
+               r"value in the file\. Use the export function to write the modified container to a new file\.")
+        with self.assertWarnsRegex(UserWarning, msg):
+            table['dtr'].table = None
+        self.assertIsNone(table['dtr'].table)
+
+    def test_reassign_table_attr(self):
+        mc = self.roundtripContainer()
+        table = mc.containers['table_with_dtr']
+        table2, _ = self.make_tables()
+        msg = (r"Container was read from file '.*'\. Changing the value of attribute 'table' will not change the "
+               r"value in the file\. Use the export function to write the modified container to a new file\.")
+        with self.assertWarnsRegex(UserWarning, msg):
+            table['dtr'].table = table2
+        self.assertIs(table['dtr'].table, table2)   # check that table was reassigned
+
 
 class TestElementIdentifiers(TestCase):
 

tests/unit/helpers/utils.py

@@ -45,6 +45,9 @@ def get_temp_filepath():
 #  Foo example data containers and specs
 ###########################################
 class Foo(Container):
+
+    __fields__ = ("my_data", "attr1", "attr2", "attr3")
+
     @docval(
         {"name": "name", "type": str, "doc": "the name of this Foo"},
         {"name": "my_data", "type": ("array_data", "data"), "doc": "some data"},
@@ -55,10 +58,10 @@ class Foo(Container):
     def __init__(self, **kwargs):
         name, my_data, attr1, attr2, attr3 = getargs("name", "my_data", "attr1", "attr2", "attr3", kwargs)
         super().__init__(name=name)
-        self.__data = my_data
-        self.__attr1 = attr1
-        self.__attr2 = attr2
-        self.__attr3 = attr3
+        self.my_data = my_data
+        self.attr1 = attr1
+        self.attr2 = attr2
+        self.attr3 = attr3
 
     def __eq__(self, other):
         attrs = ("name", "my_data", "attr1", "attr2", "attr3")
@@ -68,22 +71,6 @@ def __str__(self):
         attrs = ("name", "my_data", "attr1", "attr2", "attr3")
         return "<" + ",".join("%s=%s" % (a, getattr(self, a)) for a in attrs) + ">"
 
-    @property
-    def my_data(self):
-        return self.__data
-
-    @property
-    def attr1(self):
-        return self.__attr1
-
-    @property
-    def attr2(self):
-        return self.__attr2
-
-    @property
-    def attr3(self):
-        return self.__attr3
-
     def __hash__(self):
         return hash(self.name)
 

tests/unit/test_container.py

@@ -4,9 +4,9 @@
 
 from hdmf.container import AbstractContainer, Container, Data, ExternalResourcesManager
 from hdmf.common.resources import ExternalResources
-from hdmf.testing import TestCase
+from hdmf.testing import H5RoundTripMixin, TestCase
 from hdmf.utils import docval
-from hdmf.common import (DynamicTable, VectorData, DynamicTableRegion)
+from hdmf.common import (DynamicTable, VectorData, DynamicTableRegion, SimpleMultiContainer)
 import unittest
 from hdmf.term_set import TermSet
 from hdmf.backends.hdf5.h5tools import HDF5IO
@@ -468,6 +468,53 @@ def test_shape_list(self):
         data_obj = Data('my_data', [[0, 1, 2, 3, 4], [0, 1, 2, 3, 4]])
         self.assertTupleEqual(data_obj.shape, (2, 5))
 
+    def test_reset_data(self):
+        """
+        Test that resetting data to None raises an error.
+        """
+        data_obj = Data(name='my_data', data='foobar')
+        with self.assertRaisesWith(ValueError, "data cannot be set to None."):
+            data_obj.data = None
+        self.assertEqual(data_obj.data, 'foobar')
+
+    def test_reassign_data(self):
+        """
+        Test that reassigning data works correctly.
+        """
+        data_obj = Data(name='my_data', data='foobar')
+        data_obj.data = 'barfoo'
+        self.assertEqual(data_obj.data, 'barfoo')
+
+
+class TestDataRoundTrip(H5RoundTripMixin, TestCase):
+
+    def setUpContainer(self):
+        data = Data(name='data0', data='foobar')
+        multi_container = SimpleMultiContainer(name='multi', containers=[data])
+        return multi_container
+
+    def test_reset_data(self):
+        """
+        Test that resetting data to None raises an error.
+        """
+        read_mc = self.roundtripContainer()
+        read_data_obj = read_mc.containers["data0"]
+        with self.assertRaisesWith(ValueError, "data cannot be set to None."):
+            read_data_obj.data = None
+        self.assertIsNotNone(read_data_obj.data)
+
+    def test_reassign_data(self):
+        """
+        Test that reassigning data works correctly.
+        """
+        read_mc = self.roundtripContainer()
+        read_data_obj = read_mc.containers["data0"]
+        msg = (r"Container was read from file '.*\'. Changing the value of attribute 'data' will not change the "
+               r"value in the file\. Use the export function to write the modified container to a new file\.")
+        with self.assertWarnsRegex(UserWarning, msg):
+            read_data_obj.data = ["barfoo1", "barfoo2"]
+        self.assertEqual(read_data_obj.data, ["barfoo1", "barfoo2"])
+
     @unittest.skipIf(not LINKML_INSTALLED, "optional LinkML module is not installed")
     def test_validate(self):
         terms = TermSet(term_schema_path='tests/unit/example_test_term_set.yaml')
@@ -577,11 +624,42 @@ def __init__(self, **kwargs):
         self.assertEqual(obj.field2, 'field2 value')
 
         obj.field1 = 'field1 value'
-        msg = "can't set attribute 'field2' -- already set"
-        with self.assertRaisesWith(AttributeError, msg):
-            obj.field2 = 'field2 value'
-        obj.field2 = None  # None value does nothing
-        self.assertEqual(obj.field2, 'field2 value')
+        obj.field1 = 'new field1 value'  # can reassign attributes as of HDMF 3.7.0
+        self.assertEqual(obj.field1, 'new field1 value')
+
+        obj.field1 = None  # can set to None value as of HDMF 3.7.0
+        self.assertIsNone(obj.field1)
+
+    def test_set_attribute_with_container_source(self):
+
+        class NamedFields(AbstractContainer):
+            __fields__ = ('field1', 'field2')
+
+            @docval({'name': 'field2', 'doc': 'field2 doc', 'type': str})
+            def __init__(self, **kwargs):
+                super().__init__('test name')
+                self.field2 = kwargs['field2']
+
+        obj = NamedFields.__new__(
+            NamedFields,
+            container_source="source",
+            parent=None,
+            object_id=str(uuid4()),
+            in_construct_mode=True
+        )
+        obj.__init__('field2 value')
+        obj._in_construct_mode = False
+
+        msg = ("Container was read from file 'source'. Changing the value of attribute 'field1' will not change the "
+               "value in the file. Use the export function to write the modified container to a new file.")
+        with self.assertWarnsWith(UserWarning, msg):
+            obj.field1 = 'field1 value'
+
+        msg = ("Container was read from file 'source'. Changing the value of attribute 'field2' will not change the "
+               "value in the file. Use the export function to write the modified container to a new file.")
+        with self.assertWarnsWith(UserWarning, msg):
+            obj.field2 = 'new field2 value'
+
 
     def test_with_doc(self):
         """Test that __fields__ related attributes are set correctly.

tests/unit/test_io_hdf5_h5tools.py

@@ -806,6 +806,23 @@ def test_roundtrip_pathlib_path(self):
             self.assertListEqual(foofile.buckets['bucket1'].foos['foo1'].my_data,
                                  read_foofile.buckets['bucket1'].foos['foo1'].my_data[:].tolist())
 
+    def test_roundtrip_basic_warn_reset_attr(self):
+        # Setup all the data we need
+        foo1 = Foo('foo1', [1, 2, 3, 4, 5], "I am foo1", 17, 3.14)
+        foobucket = FooBucket('bucket1', [foo1])
+        foofile = FooFile(buckets=[foobucket])
+
+        with HDF5IO(self.path, manager=self.manager, mode='w') as io:
+            io.write(foofile)
+
+        with HDF5IO(self.path, manager=self.manager, mode='r') as io:
+            read_foofile = io.read()
+
+            msg = (r"Container was read from file '.*'\. Changing the value of attribute 'attr1' will not change "
+                   r"the value in the file\. Use the export function to write the modified container to a new file\.")
+            with self.assertWarnsRegex(UserWarning, msg):
+                read_foofile.buckets['bucket1'].foos['foo1'].attr1 = "I am new foo1"
+
 
 class TestHDF5IO(TestCase):
 

tests/unit/utils_test/test_core_DataIO.py

@@ -42,7 +42,8 @@ def test_set_dataio(self):
         dataio = DataIO()
         data = np.arange(30).reshape(5, 2, 3)
         container = Data('wrapped_data', data)
-        container.set_dataio(dataio)
+        with self.assertWarns(DeprecationWarning):
+            container.set_dataio(dataio)
         self.assertIs(dataio.data, data)
         self.assertIs(dataio, container.data)
 
@@ -54,7 +55,8 @@ def test_set_dataio_data_already_set(self):
         data = np.arange(30).reshape(5, 2, 3)
         container = Data('wrapped_data', data)
         with self.assertRaisesWith(ValueError, "cannot overwrite 'data' on DataIO"):
-            container.set_dataio(dataio)
+            with self.assertWarns(DeprecationWarning):
+                container.set_dataio(dataio)
 
     def test_dataio_options(self):
         """