Repository: glotzerlab/signac
Pull request #970 (2024-01-25 21:24:42)

Fix parsing of $not expression on command line
<!-- Provide a general summary of your changes in the title above. -->

## Description
<!-- Describe your changes in detail. -->
<!-- Please indicate if the changes may break existing functionality. -->

- fixes bug
- adds test to `test_shell.py`
- I do not fix the testing code in `test_find_command_line_interface.py` that does not properly test the code path but I left a todo for now.

## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->

The `$not` expression was broken on the command line. I noticed this while working on https://github.com/glotzerlab/signac-docs/pull/199

signac was inserting `sp.` prefixes before the operator

```bash
$ signac find '{"$not": {"a": 1}}'
Error: "Invalid operator placement 'sp.$not.a'."
```

The function `filterparse._add_prefix` does the inserting, which later triggered an error in `_search_indexer._find_expression`. This code path wasn't tested.

## Checklist:
<!-- This checklist must be complete before merging 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/main/CONTRIBUTING.md).
- [x] I agree with the terms of the [Contributor Agreement](https://github.com/glotzerlab/signac/blob/main/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/main/contributors.yaml).
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.
- [x] The [package documentation](https://github.com/glotzerlab/signac/tree/main/doc) and [framework documentation](https://docs.signac.io/) in [signac-docs](https://github.com/glotzerlab/signac-docs) are up to date with these changes.
- [x] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/main/changelog.txt) and added any related issue and pull request numbers for future reference.


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/glotzerlab/signac/pull/970?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) Report
Attention: `2 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`667a47d`)](https://app.codecov.io/gh/glotzerlab/signac/commit/667a47dd0855fed142b965b33ffffadd8f050b56?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) 85.65% compared to head [(`3370d58`)](https://app.codecov.io/gh/glotzerlab/signac/pull/970?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) 85.31%.

| [Files](https://app.codecov.io/gh/glotzerlab/signac/pull/970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) | Patch % | Lines |
|---|---|---|
| [signac/filterparse.py](https://app.codecov.io/gh/glotzerlab/signac/pull/970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2ZpbHRlcnBhcnNlLnB5) | 0.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/glotzerlab/signac/pull/970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #970      +/-   ##
==========================================
- Coverage   85.65%   85.31%   -0.34%     
==========================================
  Files          20       20              
  Lines        3464     3466       +2     
  Branches      759      760       +1     
==========================================
- Hits         2967     2957      -10     
- Misses        338      346       +8     
- Partials      159      163       +4     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/glotzerlab/signac/pull/970?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab).


Patch:
diff --git a/changelog.txt b/changelog.txt
index 801b6a3b..671ccdd9 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -22,6 +22,11 @@ Changed
  - linked views now can contain spaces and other characters except directory separators (#926).
  - linked views now can be created on Windows, if 'Developer mode' is enabled (#430).
 
+Fixed
++++++
+
+ - Fixed parsing of ``$not`` query expressions on command line (#970).
+
 [2.1.0] -- 2023-07-12
 ---------------------
 
diff --git a/signac/filterparse.py b/signac/filterparse.py
index 86540a40..c495ced6 100644
--- a/signac/filterparse.py
+++ b/signac/filterparse.py
@@ -195,6 +195,7 @@ def parse_filter_arg(args):
 
 def _add_prefix(filter):
     """Add prefix "sp." to a (possibly nested) filter."""
+    # Logical operators ($and, $or, $not) should not be prefixed, but their values should.
     for key, value in filter.items():
         if key in ("$and", "$or"):
             if isinstance(value, list) or isinstance(value, tuple):
@@ -203,6 +204,8 @@ def _add_prefix(filter):
                 raise ValueError(
                     "The argument to a logical operator must be a list or a tuple!"
                 )
+        elif key == "$not":
+            yield key, dict(_add_prefix(value))
         elif "." in key and key.split(".", 1)[0] in ("sp", "doc"):
             yield key, value
         elif key in ("sp", "doc"):


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index fac004d0..bda36c69 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -6,3 +6,34 @@ import pytest
 @pytest.fixture
 def testdata():
     return str(uuid.uuid4())
+
+
+@pytest.fixture
+def find_filter():
+    return [
+        {"a": 0},
+        {"a.b": 0},
+        {"a.b": {"$lt": 42}},
+        {"a.b.$lt": 42},
+        {"$or": [{"a.b": 41}, {"a.b.$lt": 42}]},
+        {"$or": [{"a.b": 42}, {"a.b.$lt": 42}]},
+        {"$and": [{"a.b": 42}, {"a.b.$lt": 42}]},
+        {"$and": [{"a.b": 0}, {"a.b.$lt": 42}]},
+        {"$and": [{"a.b.$gte": 0}, {"a.b.$lt": 42}]},
+        {"$not": {"a.b": 0}},
+        {"$and": [{"a.b.$gte": 0}, {"$not": {"a.b.$lt": 42}}]},
+        {"$not": {"$not": {"a.b": 0}}},
+        {"a.b": {"$in": [0, 1]}},
+        {"a.b": {"$nin": [0, 1]}},
+        {"$not": {"a.b": {"$in": [0, 1]}}},
+        {"a.b": {"$exists": True}},
+        {"a.b": {"$exists": False}},
+        {"a": {"$exists": True}},
+        {"a": {"$exists": False}},
+        {"c": {"$regex": r"^\d$"}},
+        {"c": {"$type": "str"}},
+        {"d": {"$type": "list"}},
+        {"a.b": {"$where": "lambda x: x < 10"}},
+        {"a.b": {"$where": "lambda x: isinstance(x, int)"}},
+        {"a": {"$regex": "[a][b][c]"}},
+    ]
diff --git a/tests/test_find_command_line_interface.py b/tests/test_find_command_line_interface.py
index e28fd884..5513d377 100644
--- a/tests/test_find_command_line_interface.py
+++ b/tests/test_find_command_line_interface.py
@@ -11,35 +11,6 @@ import pytest
 
 from signac.filterparse import parse_filter_arg, parse_simple
 
-FILTERS = [
-    {"a": 0},
-    {"a.b": 0},
-    {"a.b": {"$lt": 42}},
-    {"a.b.$lt": 42},
-    {"$or": [{"a.b": 41}, {"a.b.$lt": 42}]},
-    {"$or": [{"a.b": 42}, {"a.b.$lt": 42}]},
-    {"$and": [{"a.b": 42}, {"a.b.$lt": 42}]},
-    {"$and": [{"a.b": 0}, {"a.b.$lt": 42}]},
-    {"$and": [{"a.b.$gte": 0}, {"a.b.$lt": 42}]},
-    {"$not": {"a.b": 0}},
-    {"$and": [{"a.b.$gte": 0}, {"$not": {"a.b.$lt": 42}}]},
-    {"$not": {"$not": {"a.b": 0}}},
-    {"a.b": {"$in": [0, 1]}},
-    {"a.b": {"$nin": [0, 1]}},
-    {"$not": {"a.b": {"$in": [0, 1]}}},
-    {"a.b": {"$exists": True}},
-    {"a.b": {"$exists": False}},
-    {"a": {"$exists": True}},
-    {"a": {"$exists": False}},
-    {"c": {"$regex": r"^\d$"}},
-    {"c": {"$type": "str"}},
-    {"d": {"$type": "list"}},
-    {"a.b": {"$where": "lambda x: x < 10"}},
-    {"a.b": {"$where": "lambda x: isinstance(x, int)"}},
-    {"a": {"$regex": "[a][b][c]"}},
-]
-
-
 VALUES = {"1": 1, "1.0": 1.0, "abc": "abc", "true": True, "false": False, "null": None}
 
 ARITHMETIC_EXPRESSIONS = [
@@ -68,20 +39,24 @@ class TestFindCommandLineInterface:
         with redirect_stderr(StringIO()):
             return parse_filter_arg(args)
 
-    def test_interpret_json(self):
+    @pytest.mark.usefixtures("find_filter")
+    def test_interpret_json(self, find_filter):
         def _assert_equal(q):
+            # TODO: full code path not tested with this test.
+            # _assert_equal and _find_expression, are not tested
             assert q == self._parse([json.dumps(q)])
 
-        for f in FILTERS:
+        for f in find_filter:
             _assert_equal(f)
 
-    def test_interpret_simple(self):
+    @pytest.mark.usefixtures("find_filter")
+    def test_interpret_simple(self, find_filter):
         assert self._parse(["a"]) == {"a": {"$exists": True}}
         assert next(parse_simple(["a"])) == ("a", {"$exists": True})
 
         for s, v in VALUES.items():
             assert self._parse(["a", s]) == {"a": v}
-        for f in FILTERS:
+        for f in find_filter:
             f_ = f.copy()
             key, value = f.popitem()
             if key.startswith("$"):
diff --git a/tests/test_shell.py b/tests/test_shell.py
index 695fdada..172fb1e4 100644
--- a/tests/test_shell.py
+++ b/tests/test_shell.py
@@ -217,7 +217,8 @@ class TestBasicShell:
         )
         assert "duplicate paths" in err
 
-    def test_find(self):
+    @pytest.mark.usefixtures("find_filter")
+    def test_find(self, find_filter):
         self.call("python -m signac init".split())
         project = signac.Project()
         sps = [{"a": i} for i in range(3)]
@@ -280,6 +281,12 @@ class TestBasicShell:
                 == [job.id for job in project.find_jobs({"doc.b": i})][0]
             )
 
+        # ensure that there are no errors due to adding sp and doc prefixes
+        # by testing on all the example complex expressions
+        for f in find_filter:
+            command = "python -m signac find ".split() + [json.dumps(f)]
+            self.call(command).strip()
+
     def test_diff(self):
         self.call("python -m signac init".split())
         project = signac.Project()