Repository: planetlabs/planet-client-python

Issue:
cli download command improperly rejects --filter-json argument
To make the download experience nicer, the download command implicitly creates a permissions filter but does not combine it with the provided filter argument and exits with the message:

`Error: Specify filter options or provide using --filter-json, not both`

It seems like the proper behavior would be:

1. if the provided filter is an AND filter, add the permissions filter as a predicate
2. otherwise create a new top-level AND filter with the permissions and provided filter as predicates


Discussion:
ischneider: Once #130 is merged, the test should pass again.
ischneider: Updated with the ability to extract the filter from a provided saved search in the JSON from the `--filter-json` flag. CI still expected to fail until #131 is merged

Patch:
diff --git a/planet/scripts/util.py b/planet/scripts/util.py
index 421631d..21dddf5 100644
--- a/planet/scripts/util.py
+++ b/planet/scripts/util.py
@@ -79,20 +79,23 @@ def check_writable(dirpath):
 
 
 def filter_from_opts(**kw):
-    '''Build a AND filter from the provided filter_in OR kwargs defaulting to an
-    empty 'and' filter (@todo: API workaround).
+    '''Build a AND filter from the provided kwargs defaulting to an
+    empty 'and' filter (@todo: API workaround) if nothing is provided.
+
+    If the 'filter_json' argument is provided, this will be assumed to contain
+    a filter specification and will be anded with other filters. If the
+    'filter_json' is a search, the search filter value will be used.
+
     All kw values should be tuple or list
     '''
     filter_in = kw.pop('filter_json', None)
     active = and_filter_from_opts(kw)
-    no_filters = len(active['config']) == 0
-    if no_filters and not filter_in:
-        return filters.and_filter()
-    if not no_filters and filter_in:
-        raise click.ClickException(
-            'Specify filter options or provide using --filter-json, not both')
     if filter_in:
-        active = filter_in
+        filter_in = filter_in.get('filter', filter_in)
+        if len(active['config']) > 0:
+            active = filters.and_filter(active, filter_in)
+        else:
+            active = filter_in
     return active
 
 


Test patch:
diff --git a/tests/test_v1_cli.py b/tests/test_v1_cli.py
index 8b56083..838ff07 100644
--- a/tests/test_v1_cli.py
+++ b/tests/test_v1_cli.py
@@ -55,16 +55,47 @@ def test_filter(runner):
     def filt(*opts):
         return runner.invoke(main, ['data', 'filter'] + list(opts))
     assert_success(filt(), {
-        "type": "AndFilter",
-        "config": []
+        'type': 'AndFilter',
+        'config': []
     })
     assert_success(filt('--string-in', 'eff', 'a b c'), {
-        "type": "AndFilter",
-        "config": [
+        'type': 'AndFilter',
+        'config': [
             {'config': ['a', 'b', 'c'], 'field_name': 'eff',
              'type': 'StringInFilter'}
         ]
     })
+    filter_spec = {
+        'type': 'StringInFilter',
+        'config': ['a'],
+        'field_name': 'eff'
+    }
+    # if no other options, filter-json is used as is
+    assert_success(filt('--filter-json', json.dumps(filter_spec)), filter_spec)
+    # verify we extract the filter property of a search
+    assert_success(filt('--filter-json', json.dumps({
+        "filter": filter_spec
+    })), filter_spec)
+    # filters are combined - the --string-in option results in a single AND
+    # filter and this is combined with the provided filter_spec in a top-level
+    # AND filter
+    assert_success(filt('--filter-json', json.dumps(filter_spec),
+                        '--string-in', 'eff', 'b'), {
+        'config': [
+            {
+                'type': 'AndFilter',
+                'config': [
+                    {
+                        'type': 'StringInFilter',
+                        'config': ['b'],
+                        'field_name': 'eff'
+                    }
+                ]
+            },
+            filter_spec
+        ],
+        'type': 'AndFilter'
+    })
     # @todo more cases that are easier to write/maintain