Repository: glotzerlab/signac
Pull request #144 (2019-02-25 18:22:32)

Misc/validate collections documents

codecov-io: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/144?src=pr&el=h1) Report
> Merging [#144](https://codecov.io/gh/glotzerlab/signac/pull/144?src=pr&el=desc) into [release/1.0.0](https://codecov.io/gh/glotzerlab/signac/commit/c1ec67094e7f0df8783be5dfb6cc0c9c89ff5808?src=pr&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `100%`.

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

```diff
@@                Coverage Diff                @@
##           release/1.0.0     #144      +/-   ##
=================================================
+ Coverage          64.86%   64.94%   +0.07%     
=================================================
  Files                 37       37              
  Lines               5465     5477      +12     
=================================================
+ Hits                3545     3557      +12     
  Misses              1920     1920
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/144?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/core/synceddict.py](https://codecov.io/gh/glotzerlab/signac/pull/144/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkZGljdC5weQ==) | `95.6% <ø> (ø)` | :arrow_up: |
| [signac/contrib/collection.py](https://codecov.io/gh/glotzerlab/signac/pull/144/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvY29sbGVjdGlvbi5weQ==) | `88.75% <100%> (+0.26%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/144?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/144?src=pr&el=footer). Last update [c1ec670...7cc74cd](https://codecov.io/gh/glotzerlab/signac/pull/144?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/README.md b/README.md
index 88bd0c81..11ad026f 100644
--- a/README.md
+++ b/README.md
@@ -1,58 +1,72 @@
-# signac - simple data management
+# <img src="doc/images/logo.png" width="75" height="75"> signac - simple data management
 
 [![DOI](https://zenodo.org/badge/72946496.svg)](https://zenodo.org/badge/latestdoi/72946496)
-[![PyPi](https://img.shields.io/pypi/v/signac.svg)](https://pypi.org/project/signac/)
-[![Anaconda-Server Badge](https://anaconda.org/conda-forge/signac/badges/version.svg)](https://anaconda.org/conda-forge/signac)
-[![conda-forge-downloads](https://img.shields.io/conda/dn/conda-forge/signac.svg)](https://anaconda.org/conda-forge/signac)
-[![RTD](https://readthedocs.org/projects/signac/badge/?version=latest)](https://docs.signac.io)
-[![License](https://img.shields.io/github/license/csadorf/signac.svg)](https://github.com/glotzerlab/signac/blob/master/LICENSE.txt)
+[![PyPI](https://img.shields.io/pypi/v/signac.svg)](https://pypi.org/project/signac/)
+[![conda-forge](https://img.shields.io/conda/vn/conda-forge/signac.svg?style=flat)](https://anaconda.org/conda-forge/signac)
+![CircleCI](https://img.shields.io/circleci/project/github/glotzerlab/signac/master.svg)
+[![RTD](https://img.shields.io/readthedocs/signac.svg?style=flat)](https://docs.signac.io)
+[![License](https://img.shields.io/github/license/glotzerlab/signac.svg)](https://github.com/glotzerlab/signac/blob/master/LICENSE.txt)
+[![PyPI-downloads](https://img.shields.io/pypi/dm/signac.svg?style=flat)](https://pypistats.org/packages/signac)
+[![Gitter](https://img.shields.io/gitter/room/signac/Lobby.svg?style=flat)](https://gitter.im/signac/Lobby)
 
-## About
 
-The [signac framework](http://www.signac.io) aids in the management of large and heterogeneous data spaces.
+The [**signac** framework](https://signac.io) helps users manage and scale file-based workflows, facilitating data reuse, sharing, and reproducibility.
 
 It provides a simple and robust data model to create a well-defined indexable storage layout for data and metadata.
 This makes it easier to operate on large data spaces, streamlines post-processing and analysis and makes data collectively accessible.
 
-**The documentation is available at: [https://docs.signac.io](https://docs.signac.io)**
+## Resources
+
+- [Framework documentation](https://docs.signac.io/):
+  Examples, tutorials, topic guides, and package Python APIs.
+- [Chat Support](https://gitter.im/signac/Lobby):
+  Get help and ask questions on the **signac** gitter channel.
+- [**signac** website](https://signac.io/):
+  Framework overview and news.
 
 ## Installation
 
-The recommendend installation method for **signac** is through **conda** or **pip**.
-The software is tested for python versions 2.7.x and 3.x and is built for all major platforms.
+The recommended installation method for **signac** is through **conda** or **pip**.
+The software is tested for Python versions 2.7 and 3.4+ and is built for all major platforms.
 
 To install **signac** *via* the [conda-forge](https://conda-forge.github.io/) channel, execute:
 
-    conda install -c conda-forge signac
+```bash
+conda install -c conda-forge signac
+```
 
 To install **signac** *via* **pip**, execute:
 
-    pip install signac
+```bash
+pip install signac
+```
 
 **Detailed information about alternative installation methods can be found in the [documentation](https://docs.signac.io/en/latest/installation.html).**
 
 ## Quickstart
 
 The framework facilitates a project-based workflow.
-Setup a new project:
+Set up a new project:
 
-    $ mkdir my_project
-    $ cd my_project
-    $ signac init MyProject
+```bash
+$ mkdir my_project
+$ cd my_project
+$ signac init MyProject
+```
 
 and access the project handle:
 
-    >>> project = signac.get_project()
-
-## Documentation
-
-The documentation is hosted at [https://docs.signac.io](https://docs.signac.io).
+```python
+>>> project = signac.get_project()
+```
 
 ## Testing
 
 You can test this package by executing:
 
-    $ python -m unittest discover tests/
+```bash
+$ python -m unittest discover tests/
+```
 
 ## Acknowledgment
 
diff --git a/signac/contrib/collection.py b/signac/contrib/collection.py
index 07e9d334..d2abac4b 100644
--- a/signac/contrib/collection.py
+++ b/signac/contrib/collection.py
@@ -202,9 +202,10 @@ def _build_index(docs, key, primary_key):
             else:
                 from ..errors import InvalidKeyError
                 raise InvalidKeyError(
-                    "\nThe use of '.' (dots) in keys is invalid.\n\n"
-                    "See http://www.signac.io/document-wide-migration/ "
-                    "for a recipe on how to replace dots in all keys.")
+                    "\nThe document contains invalid keys. "
+                    "Specifically keys with dots ('.').\n\n"
+                    "See https://signac.io/document-wide-migration/ "
+                    "for a recipe on how to replace dots in existing keys.")
                 # inlined for performance
                 if type(v) is list:     # performance
                     index[_to_tuples(v)].add(doc[primary_key])
@@ -287,6 +288,10 @@ class _CollectionSearchResults(object):
     count = __len__
 
 
+class JSONParseError(ValueError):
+    pass
+
+
 class Collection(object):
     """A collection of documents.
 
@@ -493,6 +498,24 @@ class Collection(object):
             self._assert_open()
             raise
 
+    @staticmethod
+    def _validate_key(key):
+        "Emit a warning or raise an exception if key is invalid. Returns key."
+        if '.' in key:
+            from ..errors import InvalidKeyError
+            raise InvalidKeyError("Keys may not contain dots ('.').")
+        return key
+
+    @classmethod
+    def _validate_doc(cls, doc):
+        "Emit a warning or raise an exception if the document is invalid. Returns doc."
+        try:
+            for key in doc.keys():
+                cls._validate_doc(doc[cls._validate_key(key)])
+        except AttributeError:
+            return
+        return doc
+
     def __setitem__(self, _id, doc, _trust=False):
         self._assert_open()
         if not isinstance(_id, six.string_types):
@@ -503,7 +526,12 @@ class Collection(object):
         if _trust:
             self._docs[_id] = doc
         else:
-            self._docs[_id] = json.loads(json.dumps(doc))
+            try:
+                doc_ = json.loads(json.dumps(doc))
+            except TypeError as error:
+                raise TypeError(
+                    "Serialization of document '{}' failed with error: {}".format(doc, error))
+            self._docs[_id] = self._validate_doc(doc_)
         self._dirty.add(_id)
         self._requires_flush = True
 
@@ -917,6 +945,13 @@ class Collection(object):
                 collection = cls()
             else:
                 raise error
+        except ValueError as error:
+            if hasattr(file, 'name'):
+                raise JSONParseError(
+                    "Error while trying to parse file '{}': {}.".format(file.name, error))
+            else:
+                raise JSONParseError(
+                    "Error while trying to parse '{}': {}.".format(file, error))
         except AttributeError as e:
             # This error occurs in python27 and has been evaluated as being
             # fine to accept in this manner
diff --git a/signac/core/synceddict.py b/signac/core/synceddict.py
index 8b77aced..2546fe6a 100644
--- a/signac/core/synceddict.py
+++ b/signac/core/synceddict.py
@@ -90,9 +90,9 @@ class _SyncedDict(MutableMapping):
         if '.' in key:
             from ..errors import InvalidKeyError
             raise InvalidKeyError(
-                "\nThe use of '.' (dots) in keys is deprecated and may lead to "
-                "unexpected behavior!\nSee http://www.signac.io/document-wide-migration/ "
-                "for a recipe on how to replace dots in all keys.")
+                "\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
 
     def _dfs_convert(self, root):


Test patch:
diff --git a/tests/test_collection.py b/tests/test_collection.py
index 3a40b75b..9c3fa7d4 100644
--- a/tests/test_collection.py
+++ b/tests/test_collection.py
@@ -2,10 +2,12 @@ from __future__ import division
 import os
 import io
 import unittest
+import array
 from collections import OrderedDict
 from itertools import islice
 
 from signac import Collection
+from signac.contrib.collection import JSONParseError
 from signac.common import six
 from signac.errors import InvalidKeyError
 if six.PY2:
@@ -99,6 +101,11 @@ class CollectionTest(unittest.TestCase):
         for doc in docs:
             self.assertIn(doc['_id'], self.c)
 
+    def test_init_with_non_serializable(self):
+        docs = [dict(a=array.array('f', [1, 2, 3])) for i in range(10)]
+        with self.assertRaises(TypeError):
+            self.c = Collection(docs)
+
     def test_insert(self):
         doc = dict(a=0)
         self.c['0'] = doc
@@ -112,6 +119,11 @@ class CollectionTest(unittest.TestCase):
         with self.assertRaises(TypeError):
             self.c[1.0] = dict(a=0)
 
+    def test_insert_non_serializable(self):
+        doc = dict(a=array.array('f', [1, 2, 3]))
+        with self.assertRaises(TypeError):
+            self.c['0'] = doc
+
     def test_insert_multiple(self):
         doc = dict(a=0)
         self.assertEqual(len(self.c), 0)
@@ -292,22 +304,37 @@ class CollectionTest(unittest.TestCase):
         self.assertEqual(self.c.find_one({'a': {'$type': 'int'}})['_id'], id_int)
         self.assertEqual(self.c.find_one({'a': {'$type': 'float'}})['_id'], id_float)
 
-    def test_find_with_dots(self):
+    def test_insert_docs_with_dots(self):
+        with self.assertRaises(InvalidKeyError):
+            self.c.__setitem__('0', {'a.b': 0})
+        with self.assertRaises(InvalidKeyError):
+            self.c.insert_one({'a.b': 0})
+        with self.assertRaises(InvalidKeyError):
+            self.c['0'] = {'a.b': 0}
         with self.assertRaises(InvalidKeyError):
-            self.assertEqual(len(self.c.find()), 0)
-            self.assertEqual(list(self.c.find()), [])
-            self.assertEqual(len(self.c.find({'a.b': 0})), 0)
-            docs = [{'a.b': i} for i in range(10)]
-            self.c.update(docs)
-            self.assertEqual(len(self.c.find()), len(docs))
-            self.assertEqual(len(self.c.find({'a.b': 0})), 1)
-            self.assertEqual(list(self.c.find({'a.b': 0}))[0], docs[0])
-            self.assertEqual(len(self.c.find({'a.b': -1})), 0)
-            docs = [{'a': {'b': i}} for i in range(10)]
-            self.c.update(docs)
-            self.assertEqual(len(self.c.find()), 2 * len(docs))
-            self.assertEqual(len(self.c.find({'a.b': 0})), 2)
-            self.assertEqual(len(self.c.find({'a.b': -1})), 0)
+            self.c.insert_one({'a': {'b.c': 0}})
+        with self.assertRaises(InvalidKeyError):
+            self.c['0'] = {'a': {'b.c': 0}}
+        with self.assertRaises(InvalidKeyError):
+            self.c.update([{'_id': '0', 'a.b': 0}])
+        with self.assertRaises(InvalidKeyError):
+            self.c.update([{'_id': '0', 'a': {'b.c': 0}}])
+
+    def test_replace_docs_with_dots(self):
+        self.c.insert_one({'a': 0})
+        with self.assertRaises(InvalidKeyError):
+            self.c.replace_one({'a': 0}, {'a.b': 0})
+
+    def test_insert_docs_with_dots_force(self):
+        self.c.__setitem__('0', {'a.b': 0}, _trust=True)
+
+        # These searches will not catch the error:
+        self.c.find()
+        self.c.find({'a': 0})
+
+        # This one will:
+        with self.assertRaises(InvalidKeyError):
+            self.c.find({'a.b': 0})
 
     def test_find_types(self):
         # Note: All of the iterables will be normalized to lists!
@@ -565,6 +592,26 @@ class CompressedCollectionTest(CollectionTest):
         self.assertGreater(compresslevel, 1.0)
 
 
+class FileCollectionTestBadJson(unittest.TestCase):
+
+    def setUp(self):
+        self._tmp_dir = TemporaryDirectory(prefix='signac_collection_')
+        self._fn_collection = os.path.join(self._tmp_dir.name, 'test.txt')
+        self.addCleanup(self._tmp_dir.cleanup)
+        with open(self._fn_collection, 'w') as file:
+            for i in range(3):
+                file.write('{"a": 0}\n')
+
+    def test_read(self):
+        with Collection.open(self._fn_collection, mode='r') as c:
+            self.assertEqual(len(list(c)), 3)
+        with open(self._fn_collection, 'a') as file:
+            file.write("{'a': 0}\n")      # ill-formed JSON (single quotes instead of double quotes)
+        with self.assertRaises(JSONParseError):
+            with Collection.open(self._fn_collection, mode='r') as c:
+                pass
+
+
 class FileCollectionTestReadOnly(unittest.TestCase):
 
     def setUp(self):