Repository: glotzerlab/signac
Pull request #205 (2019-07-08 17:36:06)

Fix/issue 168
## Description

Fixes issue #168 and #170 .

## Motivation and Context

This patch refactors the state point management with in the `Job` class and also adds a general key type check and conversion to the SyncedDict class. That means that the `Job.sp`, `Job.doc`, and `Project.doc` attributes and their aliases should now behave consistently with respect to their key conversion and validation behavior.

Valid key types are now explicitly listed in the source code, namely: `str`, `int`, `bool`, and `NoneType`.
Trying to use a key of different type should trigger a `KeyTypeError`.

There are two minor incompatibilities introduced with this patch:

1) I discovered a bug where a value of type `tuple` would not be immediately converted to a list when assigned to a state point key and remain a tuple. The value would be a list after reloading the job. The unit test actually expected this odd behavior, I therefore changed a unit test with respect to this bug.
2) Instead of raising an `InvalidKeyError`, which is a `ValueError`, trying to use a floating point number as a key will now raise an `KeyTypeError`, which is a `TypeError`. Using floats as keys was already illegal, since they must contain dots in their JSON representation. However, this would have triggered a `InvalidKeyError`, instead of a `KeyTypeError`.

## Types of Changes
<!-- Please select all items that apply either now or after creating the pull request: -->
- [ ] Documentation update
- [x] Bug fix
- [ ] New feature
- [x] Breaking change<sup>1</sup>

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
<!-- Please select all items that apply either now or after creating the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [x] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.txt).
- [x] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:
- [ ] I have updated the API documentation as part of the package doc-strings.
- [ ] I have created a separate pull request to update the [framework documentation](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [x] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt).


codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/205?src=pr&el=h1) Report
> Merging [#205](https://codecov.io/gh/glotzerlab/signac/pull/205?src=pr&el=desc) into [master](https://codecov.io/gh/glotzerlab/signac/commit/774333de153ef0a1fe850ff36bf5f6eaf133ee66?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `96.15%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/205/graphs/tree.svg?width=650&token=FghbjTyzgM&height=150&src=pr)](https://codecov.io/gh/glotzerlab/signac/pull/205?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
- Coverage   65.03%   65.02%   -0.01%     
==========================================
  Files          37       37              
  Lines        5582     5590       +8     
==========================================
+ Hits         3630     3635       +5     
- Misses       1952     1955       +3
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/205?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/job.py](https://codecov.io/gh/glotzerlab/signac/pull/205/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvam9iLnB5) | `90.83% <100%> (+0.36%)` | :arrow_up: |
| [signac/core/synceddict.py](https://codecov.io/gh/glotzerlab/signac/pull/205/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkZGljdC5weQ==) | `94.66% <100%> (-0.27%)` | :arrow_down: |
| [signac/errors.py](https://codecov.io/gh/glotzerlab/signac/pull/205/diff?src=pr&el=tree#diff-c2lnbmFjL2Vycm9ycy5weQ==) | `90.9% <100%> (+0.28%)` | :arrow_up: |
| [signac/contrib/project.py](https://codecov.io/gh/glotzerlab/signac/pull/205/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvcHJvamVjdC5weQ==) | `90.62% <50%> (ø)` | :arrow_up: |
| [signac/core/json.py](https://codecov.io/gh/glotzerlab/signac/pull/205/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvanNvbi5weQ==) | `87.5% <0%> (-4.17%)` | :arrow_down: |
| [signac/contrib/import\_export.py](https://codecov.io/gh/glotzerlab/signac/pull/205/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvaW1wb3J0X2V4cG9ydC5weQ==) | `84.67% <0%> (-0.26%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/205?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/205?src=pr&el=footer). Last update [774333d...da1b0ca](https://codecov.io/gh/glotzerlab/signac/pull/205?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

csadorf: @bdice I think I've addressed all of your comments.
csadorf: * [ ] Move all `Job.doc` and `Project.doc` IO logic into the `JSONDict `class.
csadorf: @bdice @vyasr Since this patch is touching core logic, I'd appreciate if you could give this PR another pass.

@tcmoore3 I've followed your suggestion and refactored the reset-document logic into the `JSONDict` class. This also means that the buffered mode will be respected for document resets.

Patch:
diff --git a/changelog.txt b/changelog.txt
index f857270d..f1542e26 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -24,6 +24,9 @@ next
  - Reduce the logging verbosity about a missing default host key in the configuration (#201).
  - Add ``read_json()`` and ``to_json()`` methods to Collection class (#104).
  - Fix issue with incorrect detection of dict-like files managed with the ``DictManager`` class (e.g. ``job.stores``) (#203).
+ - Fix issue causing a failure of the automatic conversion of valid key types (#168, #205).
+ - Improve the 'dots in keys' error message to make it easier to fix related issues (#170, #205).
+
 
 [1.1.0] -- 2019-05-19
 ---------------------
diff --git a/signac/contrib/job.py b/signac/contrib/job.py
index 60816f9d..54e1444a 100644
--- a/signac/contrib/job.py
+++ b/signac/contrib/job.py
@@ -5,7 +5,6 @@ import os
 import errno
 import logging
 import shutil
-import uuid
 
 from ..common import six
 from ..core import json
@@ -16,10 +15,7 @@ from .hashing import calc_id
 from .utility import _mkdir_p
 from .errors import DestinationExistsError, JobsCorruptedError
 from ..sync import sync_jobs
-if six.PY2:
-    from collections import Mapping
-else:
-    from collections.abc import Mapping
+
 
 logger = logging.getLogger(__name__)
 
@@ -58,16 +54,9 @@ class Job(object):
     def __init__(self, project, statepoint, _id=None):
         self._project = project
 
-        # Ensure that the job id is configured
-        if _id is None:
-            self._statepoint = json.loads(json.dumps(statepoint))
-            self._id = calc_id(self._statepoint)
-        else:
-            self._statepoint = dict(statepoint)
-            self._id = _id
-
-        # Prepare job statepoint
-        self._sp = SyncedAttrDict(self._statepoint, parent=_sp_save_hook(self))
+        # Set statepoint and id
+        self._statepoint = SyncedAttrDict(statepoint, parent=_sp_save_hook(self))
+        self._id = calc_id(self._statepoint()) if _id is None else _id
 
         # Prepare job working directory
         self._wd = os.path.join(project.workspace(), self._id)
@@ -155,9 +144,8 @@ class Job(object):
             else:
                 raise
         # Update this instance
-        self._statepoint = dst._statepoint
+        self._statepoint = SyncedAttrDict(dst._statepoint._as_dict(), parent=_sp_save_hook(self))
         self._id = dst._id
-        self._sp = SyncedAttrDict(self._statepoint, parent=_sp_save_hook(self))
         self._wd = dst._wd
         self._fn_doc = dst._fn_doc
         self._document = None
@@ -214,9 +202,7 @@ class Job(object):
             `sp_dict = job.statepoint()` instead of `sp = job.statepoint`.
             For more information, see :class:`~signac.JSONDict`.
         """
-        if self._sp is None:
-            self._sp = SyncedAttrDict(self._statepoint, parent=_sp_save_hook(self))
-        return self._sp
+        return self._statepoint
 
     @statepoint.setter
     def statepoint(self, new_sp):
@@ -224,33 +210,13 @@ class Job(object):
 
     @property
     def sp(self):
-        """ Alias for :attr:`Job.statepoint`.
-
-        .. warning::
-
-            As with :attr:`Job.statepoint`, use `job.sp()` instead of
-            `job.sp` if you need a deep copy that will not modify the
-            underlying persistent JSON file.
-        """
+        "Alias for :attr:`Job.statepoint`."
         return self.statepoint
 
     @sp.setter
     def sp(self, new_sp):
         self.statepoint = new_sp
 
-    def _reset_document(self, new_doc):
-        if not isinstance(new_doc, Mapping):
-            raise ValueError("The document must be a mapping.")
-        dirname, filename = os.path.split(self._fn_doc)
-        fn_tmp = os.path.join(dirname, '._{uid}_{fn}'.format(
-            uid=uuid.uuid4(), fn=filename))
-        with open(fn_tmp, 'wb') as tmpfile:
-            tmpfile.write(json.dumps(new_doc).encode())
-        if six.PY2:
-            os.rename(fn_tmp, self._fn_doc)
-        else:
-            os.replace(fn_tmp, self._fn_doc)
-
     @property
     def document(self):
         """The document associated with this job.
@@ -274,7 +240,7 @@ class Job(object):
 
     @document.setter
     def document(self, new_doc):
-        self._reset_document(new_doc)
+        self.document.reset(new_doc)
 
     @property
     def doc(self):
diff --git a/signac/contrib/project.py b/signac/contrib/project.py
index 987e6cc9..95ad366a 100644
--- a/signac/contrib/project.py
+++ b/signac/contrib/project.py
@@ -34,9 +34,9 @@ from .errors import WorkspaceError
 from .errors import DestinationExistsError
 from .errors import JobsCorruptedError
 if six.PY2:
-    from collections import Mapping, Iterable
+    from collections import Iterable
 else:
-    from collections.abc import Mapping, Iterable
+    from collections.abc import Iterable
 
 logger = logging.getLogger(__name__)
 
@@ -259,17 +259,7 @@ class Project(object):
         return os.path.isfile(self.fn(filename))
 
     def _reset_document(self, new_doc):
-        if not isinstance(new_doc, Mapping):
-            raise ValueError("The document must be a mapping.")
-        dirname, filename = os.path.split(self._fn_doc)
-        fn_tmp = os.path.join(dirname, '._{uid}_{fn}'.format(
-            uid=uuid.uuid4(), fn=filename))
-        with open(fn_tmp, 'wb') as tmpfile:
-            tmpfile.write(json.dumps(new_doc).encode())
-        if six.PY2:
-            os.rename(fn_tmp, self._fn_doc)
-        else:
-            os.replace(fn_tmp, self._fn_doc)
+        self.document.reset(new_doc)
 
     @property
     def document(self):
@@ -372,7 +362,7 @@ class Project(object):
             # second best case
             job = self.Job(project=self, statepoint=statepoint)
             if job._id not in self._sp_cache:
-                self._sp_cache[job._id] = dict(job._statepoint)
+                self._sp_cache[job._id] = job.statepoint._as_dict()
             return job
         elif id in self._sp_cache:
             # optimal case
@@ -719,7 +709,7 @@ class Project(object):
 
     def _register(self, job):
         "Register the job within the local index."
-        self._sp_cache[job._id] = dict(job._statepoint)
+        self._sp_cache[job._id] = job._statepoint._as_dict()
 
     def _get_statepoint_from_workspace(self, jobid):
         "Attempt to read the statepoint from the workspace."
diff --git a/signac/core/jsondict.py b/signac/core/jsondict.py
index b2146692..2e2e3859 100644
--- a/signac/core/jsondict.py
+++ b/signac/core/jsondict.py
@@ -10,11 +10,16 @@ import hashlib
 import logging
 from tempfile import mkstemp
 from contextlib import contextmanager
+from copy import copy
 
 from .errors import Error
 from . import json
 from .attrdict import SyncedAttrDict
 from ..common import six
+if six.PY2:
+    from collections import Mapping
+else:
+    from collections.abc import Mapping
 
 
 logger = logging.getLogger(__name__)
@@ -297,6 +302,23 @@ class JSONDict(SyncedAttrDict):
                 with open(self._filename, 'wb') as file:
                     file.write(blob)
 
+    def reset(self, data):
+        """Replace the document contents with data."""
+        if isinstance(data, Mapping):
+            with self._suspend_sync():
+                backup = copy(self._data)
+                try:
+                    self._data = {
+                        self._validate_key(k): self._dfs_convert(v)
+                        for k, v in data.items()
+                    }
+                    self._save()
+                except BaseException:  # rollback
+                    self._data = backup
+                    raise
+        else:
+            raise ValueError("The document must be a mapping.")
+
     @contextmanager
     def buffered(self):
         buffered_dict = BufferedSyncedAttrDict(self, parent=self)
diff --git a/signac/core/synceddict.py b/signac/core/synceddict.py
index 37a58748..fe808e52 100644
--- a/signac/core/synceddict.py
+++ b/signac/core/synceddict.py
@@ -76,6 +76,8 @@ class _SyncedList(list):
 
 class _SyncedDict(MutableMapping):
 
+    VALID_KEY_TYPES = six.string_types + (int, bool, type(None))
+
     def __init__(self, initialdata=None, parent=None):
         self._suspend_sync_ = 1
         self._parent = parent
@@ -89,16 +91,22 @@ class _SyncedDict(MutableMapping):
             }
         self._suspend_sync_ = 0
 
-    @staticmethod
-    def _validate_key(key):
+    @classmethod
+    def _validate_key(cls, key):
         "Emit a warning or raise an exception if key is invalid. Returns key."
-        if '.' in key:
-            from ..errors import InvalidKeyError
-            raise InvalidKeyError(
-                "\nThe use of '.' (dots) in keys is invalid.\n\n"
-                "See https://signac.io/document-wide-migration/ "
-                "for a recipe on how to replace dots in existing keys.")
-        return key
+        if isinstance(key, six.string_types):
+            if '.' in key:
+                from ..errors import InvalidKeyError
+                raise InvalidKeyError(
+                    "keys may not contain dots ('.'): {}".format(key))
+            else:
+                return key
+        elif isinstance(key, cls.VALID_KEY_TYPES):
+            return cls._validate_key(str(key))
+        else:
+            from ..errors import KeyTypeError
+            raise KeyTypeError(
+                "keys must be str, int, bool or None, not {}".format(type(key).__name__))
 
     def _dfs_convert(self, root):
         if type(root) is type(self):
@@ -110,6 +118,8 @@ class _SyncedDict(MutableMapping):
                 for k in root:
                     ret[k] = root[k]
             return ret
+        elif type(root) is tuple:
+            return _SyncedList(root, self)
         elif type(root) is list:
             return _SyncedList(root, self)
         elif NUMPY:
diff --git a/signac/errors.py b/signac/errors.py
index 437eaa03..8b413d28 100644
--- a/signac/errors.py
+++ b/signac/errors.py
@@ -56,6 +56,10 @@ class InvalidKeyError(ValueError):
     """Raised when a user uses a non-conforming key."""
 
 
+class KeyTypeError(TypeError):
+    """Raised when a user uses a key of invalid type."""
+
+
 __all__ = [
     'Error',
     'BufferException',


Test patch:
diff --git a/tests/test_job.py b/tests/test_job.py
index 5f6cafd5..04a4f931 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -19,6 +19,7 @@ from signac.common import six
 from signac.errors import DestinationExistsError
 from signac.errors import JobsCorruptedError
 from signac.errors import InvalidKeyError
+from signac.errors import KeyTypeError
 
 if six.PY2:
     from tempdir import TemporaryDirectory
@@ -192,13 +193,16 @@ class JobSPInterfaceTest(BaseJobTest):
             self.assertEqual(getattr(job.sp.g, x), sp['g'][x])
             self.assertEqual(job.sp[x], sp[x])
         a = [1, 1.0, '1.0', True, None]
-        b = list(a) + [a] + [tuple(a)]
-        for v in b:
-            for x in ('a', 'b', 'c', 'd', 'e'):
-                setattr(job.sp, x, v)
-                self.assertEqual(getattr(job.sp, x), v)
-                setattr(job.sp.g, x, v)
-                self.assertEqual(getattr(job.sp.g, x), v)
+        for x in ('a', 'b', 'c', 'd', 'e'):
+            setattr(job.sp, x, a)
+            self.assertEqual(getattr(job.sp, x), a)
+            setattr(job.sp.g, x, a)
+            self.assertEqual(getattr(job.sp.g, x), a)
+        t = (1, 2, 3)  # tuple
+        job.sp.t = t
+        self.assertEqual(job.sp.t, list(t))  # implicit conversion
+        job.sp.g.t = t
+        self.assertEqual(job.sp.g.t, list(t))
 
     def test_interface_job_identity_change(self):
         job = self.open_job({'a': 0})
@@ -328,6 +332,56 @@ class JobSPInterfaceTest(BaseJobTest):
             self.assertEqual(job.sp, job2.sp)
             self.assertEqual(job.get_id(), job2.get_id())
 
+    def test_valid_sp_key_types(self):
+        job = self.open_job(dict(invalid_key=True)).init()
+
+        class A:
+            pass
+        for key in ('0', 0, True, False, None):
+            job.sp[key] = 'test'
+            self.assertIn(str(key), job.sp)
+
+    def test_invalid_sp_key_types(self):
+        job = self.open_job(dict(invalid_key=True)).init()
+
+        class A:
+            pass
+        for key in (0.0, A(), (1, 2, 3)):
+            with self.assertRaises(KeyTypeError):
+                job.sp[key] = 'test'
+            with self.assertRaises(KeyTypeError):
+                job.sp = {key: 'test'}
+        for key in ([], {}, dict()):
+            with self.assertRaises(TypeError):
+                job.sp[key] = 'test'
+            with self.assertRaises(TypeError):
+                job.sp = {key: 'test'}
+
+    def test_valid_doc_key_types(self):
+        job = self.open_job(dict(invalid_key=True)).init()
+
+        class A:
+            pass
+        for key in ('0', 0, True, False, None):
+            job.doc[key] = 'test'
+            self.assertIn(str(key), job.doc)
+
+    def test_invalid_doc_key_types(self):
+        job = self.open_job(dict(invalid_key=True)).init()
+
+        class A:
+            pass
+        for key in (0.0, A(), (1, 2, 3)):
+            with self.assertRaises(KeyTypeError):
+                job.doc[key] = 'test'
+            with self.assertRaises(KeyTypeError):
+                job.doc = {key: 'test'}
+        for key in ([], {}, dict()):
+            with self.assertRaises(TypeError):
+                job.doc[key] = 'test'
+            with self.assertRaises(TypeError):
+                job.doc = {key: 'test'}
+
 
 class ConfigTest(BaseJobTest):
 
diff --git a/tests/test_jsondict.py b/tests/test_jsondict.py
index b3019643..a6ceee48 100644
--- a/tests/test_jsondict.py
+++ b/tests/test_jsondict.py
@@ -8,6 +8,7 @@ import uuid
 from signac.core.jsondict import JSONDict
 from signac.common import six
 from signac.errors import InvalidKeyError
+from signac.errors import KeyTypeError
 
 if six.PY2:
     from tempdir import TemporaryDirectory
@@ -231,6 +232,28 @@ class JSONDictTest(BaseJSONDictTest):
         with self.assertRaises(InvalidKeyError):
             jsd['a.b'] = None
 
+    def test_keys_valid_type(self):
+        jsd = self.get_json_dict()
+
+        class MyStr(str):
+            pass
+        for key in ('key', MyStr('key'), 0, None, True):
+            d = jsd[key] = self.get_testdata()
+            self.assertIn(str(key), jsd)
+            self.assertEqual(jsd[str(key)], d)
+
+    def test_keys_invalid_type(self):
+        jsd = self.get_json_dict()
+
+        class A:
+            pass
+        for key in (0.0, A(), (1, 2, 3)):
+            with self.assertRaises(KeyTypeError):
+                jsd[key] = self.get_testdata()
+        for key in ([], {}, dict()):
+            with self.assertRaises(TypeError):
+                jsd[key] = self.get_testdata()
+
 
 class JSONDictWriteConcernTest(JSONDictTest):