From c93e0748fd2192cbe49c0ba20da21d54dc54fa2d Mon Sep 17 00:00:00 2001
From: Sean Gillies <sean.gillies@gmail.com>
Date: Thu, 6 Jul 2023 15:18:48 -0600
Subject: [PATCH] Don't clip to source if there is a configured clip tool

Also temporarily pin click to < 8.1.4
---
 planet/subscription_request.py          | 34 ++++++++++++++---------
 setup.py                                |  4 ++-
 tests/unit/test_subscription_request.py | 36 ++++++++++++++++++++++---
 3 files changed, 56 insertions(+), 18 deletions(-)

diff --git a/planet/subscription_request.py b/planet/subscription_request.py
index ce822ce7..391f168a 100644
--- a/planet/subscription_request.py
+++ b/planet/subscription_request.py
@@ -50,7 +50,7 @@ def build_request(name: str,
                   notifications: Optional[Mapping] = None,
                   tools: Optional[List[Mapping]] = None,
                   clip_to_source=False) -> dict:
-    """Prepare a subscription request.
+    """Construct a Subscriptions API request.
 
     The return value can be passed to
     [planet.clients.subscriptions.SubscriptionsClient.create_subscription][].
@@ -76,6 +76,10 @@ def build_request(name: str,
         A Python dict representation of a Subscriptions API request for
         a new subscription.
 
+    Raises:
+        ClientError when a valid Subscriptions API request can't be
+        constructed.
+
     Examples:
     ```python
     >>> from datetime import datetime
@@ -117,18 +121,22 @@ def build_request(name: str,
 
         # If clip_to_source is True a clip configuration will be added
         # to the list of requested tools unless an existing clip tool
-        # exists. NOTE: the next version of the Subscription API will
-        # remove the clip tool option and always clip to the source
-        # geometry. Thus this is a preview of the next API version's
-        # default behavior.
-        if clip_to_source and not any(
-                tool.get('type', None) == 'clip' for tool in tool_list):
-            tool_list.append({
-                'type': 'clip',
-                'parameters': {
-                    'aoi': source['parameters']['geometry']
-                }
-            })
+        # exists. In that case an exception is raised. NOTE: the next
+        # version of the Subscription API will remove the clip tool
+        # option and always clip to the source geometry. Thus this is a
+        # preview of the next API version's default behavior.
+        if clip_to_source:
+            if any(tool.get('type', None) == 'clip' for tool in tool_list):
+                raise ClientError(
+                    "clip_to_source option conflicts with a configured clip tool."
+                )
+            else:
+                tool_list.append({
+                    'type': 'clip',
+                    'parameters': {
+                        'aoi': source['parameters']['geometry']
+                    }
+                })
 
         details['tools'] = tool_list
 
diff --git a/setup.py b/setup.py
index 656323ce..c7f0b69b 100644
--- a/setup.py
+++ b/setup.py
@@ -24,7 +24,9 @@
             continue
 
 install_requires = [
-    'click>=8.0.0',
+    # click 8.1.4 breaks our mypy check, see
+    # https://github.com/pallets/click/issues/2558.
+    'click>8.0,<8.1.4',
     'geojson',
     'httpx>=0.23.0',
     'jsonschema',
diff --git a/tests/unit/test_subscription_request.py b/tests/unit/test_subscription_request.py
index fc1afe45..b22983b3 100644
--- a/tests/unit/test_subscription_request.py
+++ b/tests/unit/test_subscription_request.py
@@ -65,7 +65,8 @@ def test_build_request_success(geom_geojson):
     assert res == expected
 
 
-def test_build_request_clip_to_source(geom_geojson):
+def test_build_request_clip_to_source_success(geom_geojson):
+    """Without a clip tool we can clip to source."""
     source = {
         "type": "catalog",
         "parameters": {
@@ -77,7 +78,7 @@ def test_build_request_clip_to_source(geom_geojson):
             "asset_types": ["ortho_analytic_4b"]
         }
     }
-    res = subscription_request.build_request(
+    req = subscription_request.build_request(
         'test',
         source=source,
         delivery={},
@@ -86,8 +87,35 @@ def test_build_request_clip_to_source(geom_geojson):
         }],
         clip_to_source=True,
     )
-    assert res["tools"][1]["type"] == "clip"
-    assert res["tools"][1]["parameters"]["aoi"] == geom_geojson
+    assert req["tools"][1]["type"] == "clip"
+    assert req["tools"][1]["parameters"]["aoi"] == geom_geojson
+
+
+def test_build_request_clip_to_source_failure(geom_geojson):
+    """With a clip tool we can not clip to source."""
+    source = {
+        "type": "catalog",
+        "parameters": {
+            "geometry": geom_geojson,
+            "start_time": "2021-03-01T00:00:00Z",
+            "end_time": "2023-11-01T00:00:00Z",
+            "rrule": "FREQ=MONTHLY;BYMONTH=3,4,5,6,7,8,9,10",
+            "item_types": ["PSScene"],
+            "asset_types": ["ortho_analytic_4b"]
+        }
+    }
+    with pytest.raises(exceptions.ClientError):
+        subscription_request.build_request(
+            'test',
+            source=source,
+            delivery={},
+            tools=[{
+                'type': 'clip'
+            }, {
+                'type': 'hammer'
+            }],
+            clip_to_source=True,
+        )
 
 
 def test_catalog_source_success(geom_geojson):