[Add Check]: string-valued table elements contain dictionaries (#163)

* added util, check, and tests for both

* yield over cols

* narrowed logic

* added failing test

* fix test names

* fixed test

* added json sub-case and test

Co-authored-by: Ben Dichter <[email protected]>

Author: CodyCBakerPhD

nwbinspector/checks/tables.py

@@ -8,7 +8,7 @@
 from pynwb.file import TimeIntervals, Units
 
 from ..register_checks import register_check, InspectorMessage, Importance
-from ..utils import format_byte_size, is_ascending_series
+from ..utils import format_byte_size, is_ascending_series, is_dict_in_string, is_string_json_loadable
 
 
 @register_check(importance=Importance.CRITICAL, neurodata_type=DynamicTableRegion)
@@ -159,6 +159,23 @@ def check_single_row(
         )
 
 
+@register_check(importance=Importance.BEST_PRACTICE_VIOLATION, neurodata_type=DynamicTable)
+def check_table_values_for_dict(table: DynamicTable, nelems: int = 200):
+    """Check if any values in a row or column of a table contain a string casting of a Python dictionary."""
+    for column in table.columns:
+        if not hasattr(column, "data") or isinstance(column, VectorIndex) or not isinstance(column.data[0], str):
+            continue
+        for string in column.data[:nelems]:
+            if is_dict_in_string(string=string):
+                message = (
+                    f"The column '{column.name}' contains a string value that contains a dictionary! Please "
+                    "unpack dictionaries as additional rows or columns of the table."
+                )
+                if is_string_json_loadable(string=string):
+                    message += " This string is also JSON loadable, so call `json.loads(...)` on the string to unpack."
+                yield InspectorMessage(message=message)
+
+
 # @register_check(importance="Best Practice Violation", neurodata_type=pynwb.core.DynamicTable)
 # def check_column_data_is_not_none(nwbfile):
 #     """Check column values in DynamicTable to enssure they are not None."""

nwbinspector/utils.py

@@ -1,4 +1,6 @@
 """Commonly reused logic for evaluating conditions; must not have external dependencies."""
+import re
+import json
 import numpy as np
 from typing import TypeVar, Optional, List
 from pathlib import Path
@@ -7,6 +9,8 @@
 FilePathType = TypeVar("FilePathType", str, Path)
 OptionalListOfStrings = Optional[List[str]]
 
+dict_regex = r"({.+:.+})"
+
 
 def format_byte_size(byte_size: int, units: str = "SI"):
     """
@@ -46,3 +50,25 @@ def check_regular_series(series: np.ndarray, tolerance_decimals: int = 9):
 
 def is_ascending_series(series: np.ndarray, nelems=None):
     return np.all(np.diff(series[:nelems]) > 0)
+
+
+def is_dict_in_string(string: str):
+    """
+    Determine if the string value contains an encoded Python dictionary.
+
+    Can also be the direct results of string casting a dictionary, *e.g.*, ``str(dict(a=1))``.
+    """
+    return any(re.findall(pattern=dict_regex, string=string))
+
+
+def is_string_json_loadable(string: str):
+    """
+    Determine if the serialized dictionary is a JSON object.
+
+    Rather than constructing a complicated regex pattern, a simple try/except of the json.load should suffice.
+    """
+    try:
+        json.loads(string)
+        return True
+    except json.JSONDecodeError:
+        return False

tests/test_utils.py

@@ -1,6 +1,7 @@
 from hdmf.testing import TestCase
 
-from nwbinspector.utils import format_byte_size, check_regular_series
+from nwbinspector import Importance
+from nwbinspector.utils import format_byte_size, check_regular_series, is_dict_in_string
 
 
 def test_format_byte_size():
@@ -23,3 +24,73 @@ def test_format_byte_size_units_exception(self):
 def test_check_regular_series():
     assert check_regular_series(series=[1, 2, 3])
     assert not check_regular_series(series=[1, 2, 4])
+
+
+def test_is_dict_in_string_false_1():
+    string = "not a dict"
+    assert is_dict_in_string(string=string) is False
+
+
+def test_is_dict_in_string_false_2():
+    string = "not a dict, {but also fancy format text!}"
+    assert is_dict_in_string(string=string) is False
+
+
+def test_is_dict_in_string_false_3():
+    string = "[not] a dict, {[but] also} fancy format text!"
+    assert is_dict_in_string(string=string) is False
+
+
+def test_is_dict_in_string_true_1():
+    string = str(dict(a=1))
+    assert is_dict_in_string(string=string) is True
+
+
+def test_is_dict_in_string_true_2():
+    string = str([dict(a=1), dict(b=2)])
+    assert is_dict_in_string(string=string) is True
+
+
+def test_is_dict_in_string_true_3():
+    string = str(dict(a=dict(b=2)))
+    assert is_dict_in_string(string=string) is True
+
+
+def test_is_dict_in_string_true_4():
+    string = "some text: {'then': 'a dict'}"
+    assert is_dict_in_string(string=string) is True
+
+
+def test_is_dict_in_string_true_5():
+    string = "{'then': 'a dict'} more text"
+    assert is_dict_in_string(string=string) is True
+
+
+def test_is_dict_in_string_true_6():
+    """Not a JSON encodable object."""
+    string = str({1.2: Importance.CRITICAL})
+    assert is_dict_in_string(string=string) is True
+
+
+def test_is_dict_in_string_true_7():
+    """
+    A more aggressive demonstration of the general dictionary regex.
+
+    But it is technically possible to achieve via `str({custom_object_1: custom_object_2})` if 'custom_object_1' is
+    hashable and both custom objects have manual `__repr__` that do not include apostrophe's within their return.
+
+    Example
+    -------
+    from dataclasses import dataclass
+
+    @dataclass
+    class Test():
+        prop = 1
+
+        def __repr__(self):
+            return "This is a test"
+
+    str({1: Test()})
+    """
+    string = "example, {this is not a dict: but it sure looks like one}!"
+    assert is_dict_in_string(string=string) is True

tests/unit_tests/test_tables.py

@@ -1,4 +1,5 @@
 import platform
+import json
 from unittest import TestCase
 
 import pytest
@@ -13,6 +14,7 @@
     check_dynamic_table_region_data_validity,
     check_column_binary_capability,
     check_single_row,
+    check_table_values_for_dict,
 )
 from nwbinspector.register_checks import InspectorMessage, Importance
 
@@ -258,6 +260,55 @@ def test_check_single_row_fail():
     )
 
 
+def test_check_table_values_for_dict_non_str():
+    table = DynamicTable(name="test_table", description="")
+    table.add_column(name="test_column", description="")
+    table.add_row(test_column=123)
+    assert check_table_values_for_dict(table=table) is None
+
+
+def test_check_table_values_for_dict_pass():
+    table = DynamicTable(name="test_table", description="")
+    table.add_column(name="test_column", description="")
+    table.add_row(test_column="123")
+    assert check_table_values_for_dict(table=table) is None
+
+
+def test_check_table_values_for_dict():
+    table = DynamicTable(name="test_table", description="")
+    table.add_column(name="test_column", description="")
+    table.add_row(test_column=str(dict(a=1)))
+    assert check_table_values_for_dict(table=table)[0] == InspectorMessage(
+        message=(
+            "The column 'test_column' contains a string value that contains a dictionary! Please unpack "
+            "dictionaries as additional rows or columns of the table."
+        ),
+        importance=Importance.BEST_PRACTICE_VIOLATION,
+        check_function_name="check_table_values_for_dict",
+        object_type="DynamicTable",
+        object_name="test_table",
+        location="/",
+    )
+
+
+def test_check_table_values_for_dict_json_case():
+    table = DynamicTable(name="test_table", description="")
+    table.add_column(name="test_column", description="")
+    table.add_row(test_column=json.dumps(dict(a=1)))
+    assert check_table_values_for_dict(table=table)[0] == InspectorMessage(
+        message=(
+            "The column 'test_column' contains a string value that contains a dictionary! Please unpack "
+            "dictionaries as additional rows or columns of the table. This string is also JSON loadable, so call "
+            "`json.loads(...)` on the string to unpack."
+        ),
+        importance=Importance.BEST_PRACTICE_VIOLATION,
+        check_function_name="check_table_values_for_dict",
+        object_type="DynamicTable",
+        object_name="test_table",
+        location="/",
+    )
+
+
 @pytest.mark.skip(reason="TODO")
 def test_check_column_data_is_not_none():
     pass