Add _in_construct_mode to differentiate container init vs construct

CHANGELOG.md

@@ -6,6 +6,12 @@
 - Allow manual triggering of some GitHub Actions. @rly ([#744](https://github.com/hdmf-dev/hdmf/pull/744))
 - Relax input validation of `HDF5IO` to allow for s3fs support. Existing arguments of `HDF5IO` are modified as follows: i) `mode` was given a default value of "r", ii) `path` was given a default value of `None`, and iii) `file` can now accept an `S3File` type argument. @bendichter ([#746](https://github.com/hdmf-dev/hdmf/pull/746))
 - Added ability to create and get back handle to empty HDF5 dataset. @ajtritt (#747)
+- Added `AbstractContainer._in_construct_mode` that is set and modified only by the ObjectMapper when constructing an
+  object from a builder read from a file. Subclasses of `AbstractContainer` can check `_in_construct_mode` 
+  during the initialization phase as part of ``__init__`` to distinguish between actions during construction
+  (i.e., read from disk) vs. creation by the user, e.g., to determine whether to raise a warning or error when
+  encountering invalid data to support reading and correcting data that is invalid while preventing creation
+  of new data that is invalid. @rly (#751)
 
 ### Bug fixes
 - Fixed PyNWB dev CI. @rly ([#749](https://github.com/hdmf-dev/hdmf/pull/749))

docs/gallery/plot_external_resources.py

@@ -505,13 +505,13 @@
         table = pd.read_sql_query("SELECT * from %s" % table_name, db)
         table.set_index('id', inplace=True)
         ref_table = getattr(er, table_name).to_dataframe()
-        assert(np.all(np.array(table.index) == np.array(ref_table.index) + 1))
+        assert np.all(np.array(table.index) == np.array(ref_table.index) + 1)
         for c in table.columns:
             # NOTE: SQLite uses 1-based row-indices so we need adjust for that
             if np.issubdtype(table[c].dtype, np.integer):
-                assert(np.all(np.array(table[c]) == np.array(ref_table[c]) + 1))
+                assert np.all(np.array(table[c]) == np.array(ref_table[c]) + 1)
             else:
-                assert(np.all(np.array(table[c]) == np.array(ref_table[c])))
+                assert np.all(np.array(table[c]) == np.array(ref_table[c]))
     cursor.close()
 
 ###############################################################################

setup.cfg

@@ -19,9 +19,10 @@ exclude =
   versioneer.py
   src/hdmf/_version.py
   src/hdmf/_due.py
+  docs/source/tutorials/
+  docs/_build/
 per-file-ignores =
   docs/gallery/*:E402,T001
-  docs/source/tutorials/*:E402,T001
   src/hdmf/__init__.py:F401
   src/hdmf/backends/__init__.py:F401
   src/hdmf/backends/hdf5/__init__.py:F401

src/hdmf/build/objectmapper.py

@@ -1254,8 +1254,12 @@ def construct(self, **kwargs):
 
     def __new_container__(self, cls, container_source, parent, object_id, **kwargs):
         """A wrapper function for ensuring a container gets everything set appropriately"""
-        obj = cls.__new__(cls, container_source=container_source, parent=parent, object_id=object_id)
+        obj = cls.__new__(cls, container_source=container_source, parent=parent, object_id=object_id,
+                          in_construct_mode=True)
+        # obj has been created and is in construction mode, indicating that the object is being constructed by
+        # the automatic construct process during read, rather than by the user
         obj.__init__(**kwargs)
+        obj._in_construct_mode = False  # reset to False to indicate that the construction of the object is complete
         return obj
 
     @docval({'name': 'container', 'type': AbstractContainer,

src/hdmf/container.py

@@ -176,6 +176,13 @@ def __gather_fields(cls, name, bases, classdict):
         cls.__fieldsconf = tuple(all_fields_conf)
 
     def __new__(cls, *args, **kwargs):
+        """
+        Static method of the object class called by Python to create the object first and then
+         __init__() is called to initialize the object's attributes.
+
+        NOTE: this method is called directly from ObjectMapper.__new_container__ during the process of
+        constructing the object from builders that are read from a file.
+        """
         inst = super().__new__(cls)
         if cls._experimental:
             warn(_exp_warn_msg(cls))
@@ -184,6 +191,9 @@ def __new__(cls, *args, **kwargs):
         inst.__children = list()
         inst.__modified = True
         inst.__object_id = kwargs.pop('object_id', str(uuid4()))
+        # this variable is being passed in from ObjectMapper.__new_container__ and is
+        # reset to False in that method after the object has been initialized by __init__
+        inst._in_construct_mode = kwargs.pop('in_construct_mode', False)
         inst.parent = kwargs.pop('parent', None)
         return inst
 

tests/unit/test_container.py

@@ -1,4 +1,6 @@
 import numpy as np
+from uuid import uuid4, UUID
+
 from hdmf.container import AbstractContainer, Container, Data
 from hdmf.testing import TestCase
 from hdmf.utils import docval
@@ -10,22 +12,47 @@ class Subcontainer(Container):
 
 class TestContainer(TestCase):
 
-    def test_constructor(self):
-        """Test that constructor properly sets parent and both child and parent have an object_id
+    def test_new(self):
+        """Test that __new__ properly sets parent and other fields.
         """
         parent_obj = Container('obj1')
-        child_obj = Container.__new__(Container, parent=parent_obj)
+        child_object_id = str(uuid4())
+        child_obj = Container.__new__(Container, parent=parent_obj, object_id=child_object_id,
+                                      container_source="test_source")
         self.assertIs(child_obj.parent, parent_obj)
         self.assertIs(parent_obj.children[0], child_obj)
-        self.assertIsNotNone(parent_obj.object_id)
-        self.assertIsNotNone(child_obj.object_id)
+        self.assertEqual(child_obj.object_id, child_object_id)
+        self.assertFalse(child_obj._in_construct_mode)
+        self.assertTrue(child_obj.modified)
 
-    def test_constructor_object_id_none(self):
-        """Test that setting object_id to None in __new__ is OK and the object ID is set on get
+    def test_new_object_id_none(self):
+        """Test that passing object_id=None to __new__ is OK and results in a non-None object ID being assigned.
         """
         parent_obj = Container('obj1')
         child_obj = Container.__new__(Container, parent=parent_obj, object_id=None)
         self.assertIsNotNone(child_obj.object_id)
+        UUID(child_obj.object_id, version=4)  # raises ValueError if invalid
+
+    def test_new_construct_mode(self):
+        """Test that passing in_construct_mode to __new__ sets _in_construct_mode and _in_construct_mode can be reset.
+        """
+        parent_obj = Container('obj1')
+        child_obj = Container.__new__(Container, parent=parent_obj, object_id=None, in_construct_mode=True)
+        self.assertTrue(child_obj._in_construct_mode)
+        child_obj._in_construct_mode = False
+        self.assertFalse(child_obj._in_construct_mode)
+
+    def test_init(self):
+        """Test that __init__ properly sets object ID and other fields.
+        """
+        obj = Container('obj1')
+        self.assertIsNotNone(obj.object_id)
+        UUID(obj.object_id, version=4)  # raises ValueError if invalid
+        self.assertFalse(obj._in_construct_mode)
+        self.assertTrue(obj.modified)
+        self.assertEqual(obj.children, tuple())
+        self.assertIsNone(obj.parent)
+        self.assertEqual(obj.name, 'obj1')
 
     def test_set_parent(self):
         """Test that parent setter properly sets parent

tests/unit/test_io_hdf5_h5tools.py

@@ -1053,7 +1053,7 @@ def tearDown(self):
         # Close all the files
         for i in self.io:
             i.close()
-            del(i)
+            del i
         self.io = None
         self.f = None
         # Make sure the files have been deleted
@@ -1282,7 +1282,7 @@ def setUp(self):
     def tearDown(self):
         if self.io is not None:
             self.io.close()
-            del(self.io)
+            del self.io
         if os.path.exists(self.path):
             os.remove(self.path)
 
@@ -1316,7 +1316,7 @@ def setUp(self):
     def tearDown(self):
         if self.io is not None:
             self.io.close()
-            del(self.io)
+            del self.io
 
         if os.path.exists(self.path):
             os.remove(self.path)
@@ -1358,7 +1358,7 @@ def setUp(self):
     def tearDown(self):
         if self.io is not None:
             self.io.close()
-            del(self.io)
+            del self.io
         if os.path.exists(self.path):
             os.remove(self.path)
 
@@ -1390,7 +1390,7 @@ def setUp(self):
     def tearDown(self):
         if self.io is not None:
             self.io.close()
-            del(self.io)
+            del self.io
 
         if os.path.exists(self.path):
             os.remove(self.path)
@@ -1459,7 +1459,7 @@ def setUp(self):
     def tearDown(self):
         if self.io is not None:
             self.io.close()
-            del(self.io)
+            del self.io
         if os.path.exists(self.path):
             os.remove(self.path)