Check DynamicTable columns against __columns__

CHANGELOG.md

@@ -12,6 +12,7 @@
 - `HDMFIO.__del__` emits a `ResourceWarning` for an IO that was not closed instead of calling `close()`. Still-open IOs are flushed and closed by an `atexit` handler on the main thread. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547)
 
 ### Fixed
+- Fixed `DynamicTable.__init__` silently accepting a column passed via `columns` whose class conflicts with the class declared for that column in `__columns__` (e.g. a plain `VectorData` where the spec requires a `TimestampVectorData`, or where it requires a `DynamicTableRegion`). Such a mismatch now emits a warning, matching how `add_column` reports the same problem. Relatedly, `add_column` now creates a predefined column with the class from `__columns__` when the caller does not pass `col_cls`, instead of creating a plain `VectorData` and warning about a `col_cls` argument the caller never supplied. @adityasingh2400 [#1557](https://github.com/hdmf-dev/hdmf/pull/1557)
 - Fixed a deadlock when exporting a Zarr file to HDF5 with `HDF5IO.export`. A zarr array is now read into memory before the HDF5 write (in `__list_fill__`/`__scalar_fill__`), so the read does not run while h5py's global lock is held. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547)
 - Fixed `IndexError` when selecting an empty region from an in-memory `DynamicTableRegion` (e.g. `table["region"][i]` for a ragged region row that references no target rows, or an empty slice), both when the target table's columns hold their data as numpy arrays and when the target table has ragged columns. @h-mayorquin [#1549](https://github.com/hdmf-dev/hdmf/pull/1549)
 - Fixed the Jupyter HTML representation (`_repr_html_`) rendering a scalar numpy `bool` or `int` attribute (e.g. `np.bool_`, `np.int64`, as read back from an HDF5 attribute) as an expandable "array" block reporting `Shape: ()`, while `float` and `str` scalars rendered inline. `_unwrap_scalar` now also unwraps numpy scalars (`np.generic`) so every scalar renders inline consistently. @h-mayorquin [#1546](https://github.com/hdmf-dev/hdmf/pull/1546)

src/hdmf/common/table.py

@@ -418,9 +418,7 @@ def __init__(self, **kwargs):  # noqa: C901
             if len(all_targets) != len(set(all_targets)):
                 raise ValueError("'columns' contains index columns with the same target: %s" % all_targets)
 
-            # TODO: check columns against __columns__
-            # mismatches should raise an error (e.g., a VectorData cannot be passed in with the same name as a
-            # prespecified table region column)
+            self.__check_columns_against_spec(columns)
 
             # check column lengths against each other and id length
             # set ids if non-zero cols are provided and ids is empty
@@ -634,6 +632,41 @@ def __set_table_attr(self, col):
 
     __reserved_colspec_keys = ['name', 'description', 'index', 'table', 'required', 'class']
 
+    @classmethod
+    def _get_spec_column_class(cls, colspec):
+        """Return the column class required by a ``__columns__`` entry, or None if it is unconstrained."""
+        col_cls = colspec.get('class')
+        if col_cls is not None:
+            return col_cls
+        if colspec.get('table', False):
+            return DynamicTableRegion
+        if colspec.get('enum', False):
+            return EnumData
+        return None
+
+    def __check_columns_against_spec(self, columns):
+        """
+        Warn for each column passed to the constructor whose class conflicts with __columns__.
+
+        A column whose name matches a predefined column but whose class is not the class
+        required by the spec (or a subclass of it) cannot be written validly, so the mismatch
+        is reported here instead of being silently accepted. A VectorData passed with the name
+        of a predefined table region column is one such case.
+        """
+        spec_by_name = {colspec['name']: colspec for colspec in self.__columns__}
+        for column in columns:
+            colspec = spec_by_name.get(column.name)
+            if colspec is None:
+                continue
+            spec_col_cls = self._get_spec_column_class(colspec)
+            if spec_col_cls is not None and not isinstance(column, spec_col_cls):
+                msg = ("Column '%s' is predefined in %s with class=%s which does not match the class %s of the "
+                       "column passed in the 'columns' argument. "
+                       "Please ensure the new column complies with the spec. "
+                       "This will raise an error in a future version of HDMF."
+                       % (column.name, self.__class__.__name__, spec_col_cls, type(column)))
+                warn(msg, stacklevel=4)
+
     def _init_class_columns(self):
         """
         Process all predefined columns specified in class variable __columns__.
@@ -959,14 +992,20 @@ def add_column(self, **kwargs):  # noqa: C901
                 col_cls = EnumData
             if isinstance(enum, (list, tuple, np.ndarray, VectorData)):
                 ckwargs['elements'] = enum
+
+        # Use the class from the predefined column spec when the caller did not specify one, so that a
+        # predefined typed column is not silently created as a plain VectorData
+        spec_col_cls = self.__uninit_cols[name].get('class') if name in self.__uninit_cols else None
+        if col_cls is None and spec_col_cls is not None:
+            col_cls = spec_col_cls
+
         # Update col_cls to the default VectorData if col_cls is None
         if col_cls is None:
             col_cls = VectorData
 
         if name in self.__uninit_cols:  # column is a predefined optional column from the spec
             # check the given values against the predefined optional column spec. if they do not match, raise a warning
             # and ignore the given arguments. users should not be able to override these values
-            spec_col_cls = self.__uninit_cols[name].get('class')
             if spec_col_cls is not None and col_cls != spec_col_cls:
                 msg = ("Column '%s' is predefined in %s with class=%s which does not match the entered "
                        "col_cls argument. The predefined class spec will be ignored. "

tests/unit/common/test_table.py

@@ -4,6 +4,7 @@
 import os
 import pandas as pd
 import unittest
+import warnings
 
 from hdmf import Container
 from hdmf import TermSet, TermSetWrapper
@@ -2054,19 +2055,109 @@ def test_add_opt_column_mismatched_index_data(self):
         self.assertEqual(type(table.get('col2')), VectorIndex)  # not VectorData
 
     def test_add_opt_column_mismatched_col_cls(self):
-        """Test that adding an optional column from __columns__ with non-matched table raises a warning."""
+        """Test that adding an optional column from __columns__ with a conflicting col_cls raises a warning."""
         table = SubTable(name='subtable', description='subtable description')
         msg = ("Column 'col10' is predefined in SubTable with class=<class 'hdmf.common.table.EnumData'> "
                "which does not match the entered col_cls "
                "argument. The predefined class spec will be ignored. "
                "Please ensure the new column complies with the spec. "
                "This will raise an error in a future version of HDMF.")
         with self.assertWarnsWith(UserWarning, msg):
-            table.add_column(name='col10', description='column #10', index=True)
+            table.add_column(name='col10', description='column #10', index=True, col_cls=VectorData)
         self.assertEqual(table.col10.description, 'column #10')
         self.assertEqual(type(table.col10), VectorData)
         self.assertEqual(type(table.get('col10')), VectorIndex)
 
+    def test_add_opt_column_uses_spec_col_cls(self):
+        """Test that a predefined column is created with the class from __columns__ when col_cls is not given.
+
+        See https://github.com/hdmf-dev/hdmf/issues/1553
+        """
+        table = SubTable(name='subtable', description='subtable description')
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            table.add_column(name='col10', description='column #10', index=True)
+        mismatch_warnings = [w for w in caught if 'does not match the entered col_cls' in str(w.message)]
+        self.assertEqual(mismatch_warnings, [])
+        self.assertEqual(table.col10.description, 'column #10')
+        self.assertEqual(type(table.col10), EnumData)
+        self.assertEqual(type(table.get('col10')), VectorIndex)
+
+    @staticmethod
+    def _spec_compliant_required_columns(target):
+        """Return the required columns of SubTable with the classes that __columns__ requires."""
+        return [
+            VectorData(name='col1', description='column #1', data=[1, 2, 3]),
+            VectorData(name='col3', description='column #3', data=[1, 2, 3]),
+            DynamicTableRegion(name='col5', description='column #5', data=[0, 1, 2], table=target),
+            DynamicTableRegion(name='col7', description='column #7', data=[0, 1, 2], table=target),
+        ]
+
+    @staticmethod
+    def _target_table():
+        return DynamicTable(name='target', description='target table',
+                            columns=[VectorData(name='x', description='x', data=[1, 2, 3])])
+
+    def test_init_columns_mismatched_col_cls(self):
+        """Test that a column passed to the constructor with a class that conflicts with __columns__ warns.
+
+        See https://github.com/hdmf-dev/hdmf/issues/1553
+        """
+        target = self._target_table()
+        # col10 is predefined with class=EnumData, so a plain VectorData is a mismatch
+        msg = ("Column 'col10' is predefined in SubTable with class=<class 'hdmf.common.table.EnumData'> "
+               "which does not match the class <class 'hdmf.common.table.VectorData'> of the column passed "
+               "in the 'columns' argument. "
+               "Please ensure the new column complies with the spec. "
+               "This will raise an error in a future version of HDMF.")
+        columns = self._spec_compliant_required_columns(target)
+        columns.append(VectorData(name='col10', description='column #10', data=[1, 2, 3]))
+        with self.assertWarnsWith(UserWarning, msg):
+            SubTable(name='subtable', description='subtable description', columns=columns)
+
+    def test_init_columns_mismatched_table_region(self):
+        """Test that a VectorData passed with the name of a predefined table region column warns.
+
+        This is the case named in the TODO that https://github.com/hdmf-dev/hdmf/issues/1553 tracks.
+        """
+        target = self._target_table()
+        # col6 is predefined with table=True, so it must be a DynamicTableRegion
+        msg = ("Column 'col6' is predefined in SubTable with class=<class 'hdmf.common.table.DynamicTableRegion'> "
+               "which does not match the class <class 'hdmf.common.table.VectorData'> of the column passed "
+               "in the 'columns' argument. "
+               "Please ensure the new column complies with the spec. "
+               "This will raise an error in a future version of HDMF.")
+        columns = self._spec_compliant_required_columns(target)
+        columns.append(VectorData(name='col6', description='column #6', data=[1, 2, 3]))
+        with self.assertWarnsWith(UserWarning, msg):
+            SubTable(name='subtable', description='subtable description', columns=columns)
+
+    def test_init_columns_matching_spec_does_not_warn(self):
+        """Test that columns whose classes agree with __columns__ produce no mismatch warning."""
+        target = self._target_table()
+        columns = self._spec_compliant_required_columns(target)
+        columns.append(DynamicTableRegion(name='col6', description='column #6', data=[0, 1, 2], table=target))
+        columns.append(EnumData(name='col10', description='column #10', data=[0, 1, 2], elements=['a', 'b', 'c']))
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            table = SubTable(name='subtable', description='subtable description', columns=columns)
+        self.assertEqual([str(w.message) for w in caught if 'is predefined in SubTable' in str(w.message)], [])
+        self.assertEqual(type(table['col6']), DynamicTableRegion)
+        self.assertEqual(type(table['col10']), EnumData)
+
+    def test_init_columns_subclass_of_spec_does_not_warn(self):
+        """Test that a subclass of the predefined column class satisfies the spec."""
+        class SubEnumData(EnumData):
+            pass
+
+        target = self._target_table()
+        columns = self._spec_compliant_required_columns(target)
+        columns.append(SubEnumData(name='col10', description='column #10', data=[0, 1, 2], elements=['a', 'b', 'c']))
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            SubTable(name='subtable', description='subtable description', columns=columns)
+        self.assertEqual([str(w.message) for w in caught if 'is predefined in SubTable' in str(w.message)], [])
+
     def test_add_opt_column_twice(self):
         """Test that adding an optional column from __columns__ twice fails the second time."""
         table = SubTable(name='subtable', description='subtable description')