Merge pull request #168 from derek73/fix/pickle-roundtrip

derek73 · web-flow · commit 8b634547db66 · 2026-06-28T11:12:54.000-07:00
Fix pickle/copy round-trip bugs in Constants and config managers
diff --git a/nameparser/config/__init__.py b/nameparser/config/__init__.py
@@ -126,6 +126,13 @@ class TupleManager(dict[str, T]):
     '''
 
     def __getattr__(self, attr: str) -> T | None:
+        # Dunder names are Python's protocol probes (copy looks up __deepcopy__,
+        # inspect.unwrap looks up __wrapped__, ...), never config keys. Report
+        # them as genuinely absent so hasattr() is honest and those probes work;
+        # otherwise the dict default is mistaken for a real protocol hook. See
+        # RegexTupleManager.__getattr__ for the concrete failure this prevents.
+        if attr.startswith("__") and attr.endswith("__"):
+            raise AttributeError(attr)
         return self.get(attr)
 
     __setattr__ = dict.__setitem__
@@ -138,11 +145,22 @@ def __setstate__(self, state: Mapping[str, T]) -> None:
         self.update(state)
 
     def __reduce__(self) -> tuple[type, tuple[()], Mapping[str, T]]:
-        return (TupleManager, (), self.__getstate__())
+        # Use type(self), not TupleManager, so subclasses such as
+        # RegexTupleManager survive a pickle round-trip instead of being
+        # downgraded to a plain TupleManager (which loses the EMPTY_REGEX
+        # default for unknown keys).
+        return (type(self), (), self.__getstate__())
 
 
 class RegexTupleManager(TupleManager[re.Pattern[str]]):
     def __getattr__(self, attr: str) -> re.Pattern[str]:
+        # Dunder names are Python's protocol probes (copy.deepcopy looks up
+        # __deepcopy__, inspect.unwrap looks up __wrapped__, ...), never regex
+        # keys. Report them as genuinely absent; otherwise the EMPTY_REGEX
+        # default is mistaken for a real protocol hook — e.g. copy.deepcopy
+        # tries to call the returned re.Pattern and raises TypeError.
+        if attr.startswith("__") and attr.endswith("__"):
+            raise AttributeError(attr)
         return self.get(attr, EMPTY_REGEX)
 
 
@@ -274,11 +292,33 @@ def __repr__(self) -> str:
         return "<Constants() instance>"
 
     def __setstate__(self, state: Mapping[str, Any]) -> None:
-        Constants.__init__(self, state)
+        # Restore each saved attribute directly. The previous implementation
+        # passed the whole state dict to __init__ as the ``prefixes`` argument,
+        # which silently reverted every collection to its module default on
+        # unpickling.
+        self._pst = None
+        for name, value in state.items():
+            # Migration shim: pickles written before this fix used a dir() sweep
+            # for __getstate__, so their state carries the read-only
+            # ``suffixes_prefixes_titles`` property. Skip any such computed
+            # property rather than raising AttributeError on its missing setter;
+            # the real config is restored from the other keys. We don't promise
+            # to read pre-fix blobs forever — this only smooths migration for
+            # anyone persisting them, and can be dropped a release or two after
+            # 1.2.1 once they've re-pickled.
+            if isinstance(getattr(type(self), name, None), property):
+                continue
+            setattr(self, name, value)
 
     def __getstate__(self) -> Mapping[str, Any]:
-        attrs = [x for x in dir(self) if not x.startswith('_')]
-        return dict([(a, getattr(self, a)) for a in attrs])
+        # Pickle only the instance's own configuration: the collections built in
+        # __init__ plus any instance-level scalar overrides. Class-level scalar
+        # defaults are restored by the class itself, and underscore-prefixed
+        # names such as the ``_pst`` cache are private and rebuilt on demand.
+        # Filtering on ``self.__dict__`` also excludes the computed
+        # ``suffixes_prefixes_titles`` property automatically, since properties
+        # live on the class and never appear in an instance's ``__dict__``.
+        return {k: v for k, v in self.__dict__.items() if not k.startswith('_')}
 
 
 #: A module-level instance of the :py:class:`Constants()` class.
diff --git a/tests/base.py b/tests/base.py
@@ -38,3 +38,6 @@ def assertFalse(self, expr: object, msg: object = None) -> None:
 
     def assertIn(self, member: object, container: object, msg: object = None) -> None:
         assert member in container, msg  # type: ignore[operator]
+
+    def assertNotIn(self, member: object, container: object, msg: object = None) -> None:
+        assert member not in container, msg  # type: ignore[operator]
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -1,10 +1,9 @@
+import copy
 from collections.abc import Iterator
 
 import pytest
 
-from nameparser.config import CONSTANTS, SetManager, TupleManager
-
-ConfigCollection = SetManager | TupleManager
+from nameparser.config import CONSTANTS
 
 # Scalar (non-collection) config attributes that individual tests mutate on the
 # global CONSTANTS singleton. Several tests change these without restoring them;
@@ -38,20 +37,6 @@
 )
 
 
-def _clone_config_collection(value: ConfigCollection) -> ConfigCollection:
-    """Return an independent copy of a config collection manager.
-
-    ``copy.deepcopy`` is not used because ``RegexTupleManager`` carries compiled
-    patterns that its ``__reduce__`` cannot round-trip. Rebuilding from the
-    manager's own contents copies the container while sharing the (immutable)
-    elements, which is all the snapshot needs.
-    """
-    if isinstance(value, SetManager):
-        return SetManager(set(value))
-    # TupleManager / RegexTupleManager are dict subclasses.
-    return type(value)(dict(value))
-
-
 @pytest.fixture(autouse=True, params=['', None], ids=['default', 'none'])
 def empty_attribute_default(request: pytest.FixtureRequest) -> Iterator[str | None]:
     """Run every test under both empty_attribute_default settings, isolating global config.
@@ -66,7 +51,7 @@ def empty_attribute_default(request: pytest.FixtureRequest) -> Iterator[str | No
     """
     scalar_snapshot = {attr: getattr(CONSTANTS, attr) for attr in _SCALAR_CONFIG_ATTRS}
     collection_snapshot = {
-        attr: _clone_config_collection(getattr(CONSTANTS, attr))
+        attr: copy.deepcopy(getattr(CONSTANTS, attr))
         for attr in _COLLECTION_CONFIG_ATTRS
     }
     CONSTANTS.empty_attribute_default = request.param
diff --git a/tests/test_constants.py b/tests/test_constants.py
@@ -1,5 +1,9 @@
+import copy
+import pickle
+
 from nameparser import HumanName
-from nameparser.config import Constants
+from nameparser.config import Constants, RegexTupleManager, SetManager, TupleManager
+from nameparser.config.regexes import EMPTY_REGEX
 
 from tests.base import HumanNameTestBase
 
@@ -104,3 +108,127 @@ def test_add_constant_with_explicit_encoding(self) -> None:
         c = Constants()
         c.titles.add_with_encoding(b'b\351ck', encoding='latin_1')
         self.assertIn('béck', c.titles)
+
+    def test_pickle_roundtrip_preserves_customizations(self) -> None:
+        """A pickled Constants must restore its customized collections.
+
+        Regression test: __setstate__ previously passed the whole state dict
+        to __init__ as the `prefixes` argument, so every collection silently
+        reverted to its module default on unpickling.
+        """
+        c = Constants()
+        c.titles.add('customtitle')
+        c.prefixes.add('customprefix')
+        c.titles.remove('hon')
+
+        # Safe: round-tripping a Constants the test just built, not untrusted data.
+        restored = pickle.loads(pickle.dumps(c))
+
+        self.assertIn('customtitle', restored.titles)
+        self.assertIn('customprefix', restored.prefixes)
+        self.assertNotIn('hon', restored.titles)
+        # The contributing collections must match the original exactly.
+        self.assertEqual(set(restored.titles), set(c.titles))
+        self.assertEqual(set(restored.prefixes), set(c.prefixes))
+        # The collections must also keep their manager type, not just contents.
+        self.assertEqual(type(restored.titles), SetManager)
+        self.assertEqual(type(restored.prefixes), SetManager)
+
+    def test_pickle_roundtrip_preserves_instance_scalar_override(self) -> None:
+        """An instance-level scalar override must survive a pickle round-trip."""
+        c = Constants()
+        c.empty_attribute_default = None
+
+        # Safe: round-tripping a Constants the test just built, not untrusted data.
+        restored = pickle.loads(pickle.dumps(c))
+
+        self.assertEqual(restored.empty_attribute_default, None)
+
+    def test_pickle_roundtrip_preserves_regex_manager_subclass(self) -> None:
+        """regexes must round-trip as a RegexTupleManager, not a plain TupleManager.
+
+        TupleManager.__reduce__ previously hardcoded TupleManager, so the
+        RegexTupleManager subclass was downgraded on unpickling. The difference
+        is observable: RegexTupleManager returns the EMPTY_REGEX default for an
+        unknown key, while a plain TupleManager returns None.
+        """
+        c = Constants()
+
+        # Safe: round-tripping a Constants the test just built, not untrusted data.
+        restored = pickle.loads(pickle.dumps(c))
+
+        self.assertEqual(type(restored.regexes), RegexTupleManager)
+        self.assertEqual(restored.regexes.does_not_exist, EMPTY_REGEX)
+
+    def test_regexes_deepcopy_roundtrip(self) -> None:
+        """copy.deepcopy of a RegexTupleManager must round-trip.
+
+        __getattr__ answered every unknown name with the EMPTY_REGEX default,
+        including the __deepcopy__ probe copy.deepcopy issues. copy then
+        mistook that re.Pattern for a deep-copy hook and tried to call it.
+        """
+        c = Constants()
+
+        dup = copy.deepcopy(c.regexes)
+
+        self.assertEqual(type(dup), RegexTupleManager)
+        self.assertEqual(dict(dup), dict(c.regexes))
+        # The EMPTY_REGEX default still applies to genuinely unknown keys.
+        self.assertEqual(dup.does_not_exist, EMPTY_REGEX)
+
+    def test_regextuplemanager_ignores_dunder_lookups(self) -> None:
+        """Unknown dunder names report as absent, not as the EMPTY_REGEX default.
+
+        Dunder names are Python's protocol probes (copy.deepcopy looks up
+        __deepcopy__, inspect.unwrap looks up __wrapped__, ...), never config
+        keys. Answering them with a regex breaks that machinery.
+        """
+        c = Constants()
+        sentinel = object()
+
+        self.assertEqual(getattr(c.regexes, '__deepcopy__', sentinel), sentinel)
+        # A normal (non-dunder) unknown key still yields the EMPTY_REGEX default.
+        self.assertEqual(c.regexes.unknown_key, EMPTY_REGEX)
+
+    def test_tuplemanager_ignores_dunder_lookups(self) -> None:
+        """Base TupleManager must report unknown dunder names as absent too.
+
+        It returned None for any missing attribute, so `hasattr(tm, '__x__')`
+        was always True — a landmine for any probe that does hasattr-then-call.
+        Guarding dunders keeps the base consistent with RegexTupleManager.
+        """
+        c = Constants()
+        tm = c.capitalization_exceptions  # a plain TupleManager
+        sentinel = object()
+
+        self.assertEqual(type(tm), TupleManager)
+        self.assertFalse(hasattr(tm, '__deepcopy__'))
+        self.assertEqual(getattr(tm, '__wrapped__', sentinel), sentinel)
+        # A normal (non-dunder) unknown key still returns the None default.
+        self.assertEqual(tm.unknown_key, None)
+
+    def test_unpickle_legacy_state_with_property_key(self) -> None:
+        """Pickles written by older versions must still load.
+
+        The previous __getstate__ built its state from a dir() sweep, which
+        always included the computed `suffixes_prefixes_titles` property (no
+        customization required). That property has no setter, so __setstate__
+        must skip such keys instead of raising AttributeError.
+
+        Covers the temporary migration shim in __setstate__; remove this test
+        when that shim is dropped (a release or two after 1.2.1).
+        """
+        c = Constants()
+        c.titles.add('legacytitle')
+        # Reproduce the legacy dir()-sweep state dict, which carries the
+        # read-only `suffixes_prefixes_titles` property alongside the real config.
+        legacy_state = {
+            name: getattr(c, name) for name in dir(c) if not name.startswith('_')
+        }
+        self.assertIn('suffixes_prefixes_titles', legacy_state)
+
+        restored = Constants.__new__(Constants)
+        restored.__setstate__(legacy_state)
+
+        # The real customization is recovered and the property key is ignored.
+        self.assertIn('legacytitle', restored.titles)
diff --git a/tests/test_python_api.py b/tests/test_python_api.py
@@ -1,3 +1,5 @@
+import copy
+import pickle
 import re
 
 import pytest
@@ -45,6 +47,51 @@ def test_name_instance_pickle(self) -> None:
         hn = HumanName("Title First Middle Middle Last, Jr.")
         self.assertTrue(dill.pickles(hn))
 
+    def test_name_instance_pickle_preserves_instance_config(self) -> None:
+        """A HumanName carrying its own config must parse identically after a
+        pickle round-trip.
+
+        HumanName pickles its instance Constants (``.C``) through the default
+        __dict__ path, so a broken Constants round-trip silently produced a
+        differently-configured parser on the other side.
+        """
+        # Passing None as the second argument gives this name its own Constants.
+        hn = HumanName("Smith, Dr. John", None)
+        hn.C.titles.add('chancellor')
+        hn.parse_full_name()
+
+        # Safe: round-tripping a HumanName the test just built, not untrusted data.
+        restored = pickle.loads(pickle.dumps(hn))
+
+        self.assertIn('chancellor', restored.C.titles)
+        restored.full_name = "Chancellor Jane Smith"
+        self.assertEqual(restored.title, "Chancellor")
+
+    def test_name_instance_deepcopy(self) -> None:
+        """copy.deepcopy of a HumanName must round-trip.
+
+        HumanName has no custom copy hooks, so deepcopy recurses into its
+        Constants (`.C`), and previously into `.C.regexes`, whose __getattr__
+        answered copy's __deepcopy__ probe with a re.Pattern — making
+        deepcopy of *any* HumanName raise TypeError.
+        """
+        hn = HumanName("Dr. John P. Doe-Ray, CLU")
+
+        dup = copy.deepcopy(hn)
+
+        self.assertEqual(str(dup), str(hn))
+
+    def test_name_instance_deepcopy_isolates_instance_config(self) -> None:
+        """A deep-copied HumanName with its own config must be independent."""
+        hn = HumanName("Smith, Dr. John", None)
+        hn.C.titles.add('chancellor')
+
+        dup = copy.deepcopy(hn)
+        dup.C.titles.add('marker')
+
+        self.assertIn('chancellor', dup.C.titles)
+        self.assertNotIn('marker', hn.C.titles)
+
     def test_comparison(self) -> None:
         hn1 = HumanName("Doe-Ray, Dr. John P., CLU, CFP, LUTC")
         hn2 = HumanName("Dr. John P. Doe-Ray, CLU, CFP, LUTC")
