Repository: glotzerlab/signac
Pull request #233 (2019-10-10 22:52:09)

check if the statepoint key or values contain a path separator (a sla…
…sh?), and raise an error if they do, also prints the bad items

<!-- Provide a general summary of your changes in the Title above -->

## Description
<!-- Describe your changes in detail -->

## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
prevents unexpected behavior in `signac view`
<!-- If it fixes an open issue, please link to the issue here. -->
issue #213

## Types of Changes
<!-- Please select all items that apply either now or after creating the pull request: -->
- [ ] Documentation update
- [x] Bug fix
- [ ] New feature
- [ ] 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).
- [ ] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [ ] 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.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt).


vyasr: @atravitz would appreciate a second review if you don't mind! This PR is meant to resolve #213.
atravitz: are there any other symbols (other than / or \) that could cause trouble while we're fixing this? Like * or spaces in strings?
vyasr: That's a good call. @jennyfothergill could you add a test for this branch of the code? You'll basically want to add a test that creates a signac project with some odd statepoint keys, try calling signac view, and make sure that either views generate correctly or that signac throws an error.
jennyfothergill: @mikemhenry had a cool idea to whitelist the character used for POSIX compliant filenames instead of worrying about which characters we should check for, so I've switched it to that.
 
jennyfothergill: This is probably outside the scope of this PR, but how should statepoint key/values that are lists, arrays, etc be handled?
vyasr: Those types of values are fine, they just turn into folders with parenthetical lists e.g. `a/(1, 2, 3)/b/(4, 5, 6)`. Which are valid but ugly filenames, and perhaps not perfectly portable.

I'm fine with the whitelisting solution. I'm curious whether @csadorf thinks it's too restrictive, particularly w.r.t. backwards compatibility.
csadorf: > I'm fine with the whitelisting solution. I'm curious whether @csadorf thinks it's too restrictive, particularly w.r.t. backwards compatibility.

Yes, I think that is too restrictive. Many [modern file systems](https://en.wikipedia.org/wiki/Ext4) are able to handle arbitrary Unicode characters just fine. I don't think we should unnecessarily restrict users, especially those who are not from the anglophone areas of the world, to effectively use the view feature by limiting valid characters to the small set of ASCII chars.

With respect to the original issue, let's focus on checking for the file path separation characters (`os.sep)` and otherwise be not too restrictive. If we are really concerned about checking valid file paths, then we could resort to a library like [pathvalidate](https://pypi.org/project/pathvalidate/), but I don't think this is necessary within the scope of this PR.
mikemhenry: It is only restrictive in the sense that `view` is picky about what values it allows for keys & values. I'm fine with going back to `os.sep` as the only string we prohibit, but I feel like we will have to periodically add strings to our prohibited list as users run into them. Maybe this is a case of YAGNI and we will just worry about it when we get a new issue.  
csadorf: @jennyfothergill Thank you very much for your work. I would like to encourage you to add your name to the list of contributors. It helps us track who contributed to the project and gives you some additional exposure in the open-source sphere. ;)
jennyfothergill: Thanks very much! 

Patch:
diff --git a/contributors.yaml b/contributors.yaml
index 6adad1ee..b1732bfb 100644
--- a/contributors.yaml
+++ b/contributors.yaml
@@ -61,4 +61,9 @@ contributors:
     family-names: Malek
     given-names: Ali
     affiliation: "University of Goettingen"
+  -
+    family-names: Fothergill
+    given-names: Jenny
+    affiliation: "Boise State University"
+    orcid: "https://orcid.org/0000-0001-9665-5420"
 ...
diff --git a/signac/contrib/linked_view.py b/signac/contrib/linked_view.py
index 25bdfc65..a2b760aa 100644
--- a/signac/contrib/linked_view.py
+++ b/signac/contrib/linked_view.py
@@ -39,6 +39,28 @@ def create_linked_view(project, prefix=None, job_ids=None, index=None, path=None
         if not job_ids.issubset({doc['_id'] for doc in index}):
             raise ValueError("Insufficient index for selected data space.")
 
+    key_list = [
+            k for job in jobs for k in job.statepoint().keys()
+            ]
+    value_list = [
+            v for job in jobs for v in job.statepoint().values()
+            ]
+    item_list = key_list + value_list
+    bad_chars = [os.sep, " ", "*"]
+    bad_items = [
+            item for item in item_list for char in bad_chars
+            if isinstance(item, str) and char in item
+            ]
+
+    if any(bad_items):
+        err_msg = " ".join([
+            "In order to use view, statepoints should not contain {}:".format(
+                bad_chars
+            ),
+            *bad_items
+        ])
+        raise RuntimeError(err_msg)
+
     path_function = _make_path_function(jobs, path)
 
     links = dict()


Test patch:
diff --git a/tests/test_project.py b/tests/test_project.py
index dbdf7a8b..7eeefb4f 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -1731,6 +1731,17 @@ class LinkedViewProjectTest(BaseProjectTest):
         with self.assertRaises(RuntimeError):
             self.project.create_linked_view(view_prefix)
 
+    def test_create_linked_view_with_slash_raises_error(self):
+        bad_chars = [os.sep, " ", "*"]
+        statepoints = [{
+            'a{}b'.format(i): 0, 'b': 'bad{}val'.format(i)
+            } for i in bad_chars]
+        view_prefix = os.path.join(self._tmp_pr, 'view')
+        for sp in statepoints:
+            self.project.open_job(sp).init()
+            with self.assertRaises(RuntimeError):
+                self.project.create_linked_view(prefix=view_prefix)
+
 
 class UpdateCacheAfterInitJob(signac.contrib.job.Job):