Repository: planetlabs/planet-client-python

Issue:
Refactor download checksum validation
Before we finalize the Python API for 2.0, I'd like to separate the download validation, where we compare asset checksums to values in their accompanying manifest, from the orders client so that it's more generally available to users. There are use cases that don't need an orders client, such as re-validating the data after copying it to another computer. 

Discussion:

Patch:
diff --git a/docs/guide.md b/docs/guide.md
index 3eb33ee..370f6dd 100644
--- a/docs/guide.md
+++ b/docs/guide.md
@@ -221,6 +221,27 @@ from planet import reporting
 >>> asyncio.run(create_poll_and_download())
 ```
 
+#### Validating Checksums
+
+Checksum validation provides for verification that the files in an order have
+been downloaded successfully and are not missing, currupted, or changed. This
+functionality is included in the OrderClient, but does not require an instance
+of the class to be used.
+
+
+To perform checksum validation:
+
+```python
+from pathlib import Path
+
+# path includes order id
+order_path = Path('193e5bd1-dedc-4c65-a539-6bc70e55d928')
+OrdersClient.validate_checksum(order_path, 'md5')
+```
+
+
+
+
 ### Data Client
 
 The Data Client mostly mirrors the
diff --git a/planet/cli/orders.py b/planet/cli/orders.py
index a290c63..daae180 100644
--- a/planet/cli/orders.py
+++ b/planet/cli/orders.py
@@ -15,6 +15,7 @@
 from contextlib import asynccontextmanager
 import json
 import logging
+from pathlib import Path
 
 import click
 
@@ -200,15 +201,23 @@ async def wait(ctx, order_id, delay, max_attempts, state):
 async def download(ctx, order_id, overwrite, directory, checksum):
     """Download order by order ID.
 
-If --checksum is provided, the associated checksums given in the manifest
-are compared against the downloaded files to verify that they match."""
+    If --checksum is provided, the associated checksums given in the manifest
+    are compared against the downloaded files to verify that they match.
+
+    If --checksum is provided, files are already downloaded, and --overwrite is
+    not specified, this will simply validate the checksums of the files against
+    the manifest.
+    """
     quiet = ctx.obj['QUIET']
     async with orders_client(ctx) as cl:
-        await cl.download_order(str(order_id),
-                                directory=directory,
-                                overwrite=overwrite,
-                                progress_bar=not quiet,
-                                checksum=checksum)
+        await cl.download_order(
+            str(order_id),
+            directory=Path(directory),
+            overwrite=overwrite,
+            progress_bar=not quiet,
+        )
+        if checksum:
+            cl.validate_checksum(Path(directory, str(order_id)), checksum)
 
 
 def read_file_geojson(ctx, param, value):
diff --git a/planet/clients/orders.py b/planet/clients/orders.py
index b09c7fb..6096a1c 100644
--- a/planet/clients/orders.py
+++ b/planet/clients/orders.py
@@ -274,42 +274,6 @@ class OrdersClient:
                              progress_bar=progress_bar)
         return dl_path
 
-    @staticmethod
-    def _validate_checksum(manifest_data: dict, directory: Path,
-                           checksum: str):
-        """For each file entry in the manifest, validates checksum against the
-        downloaded file.
-
-        Parameters:
-            manifest_data: The contents of the opened manifest.json file.
-            filenames: List of downloaded assets.
-            checksum: The type of checksum hash- 'MD5' or 'SHA256'.
-
-        Raises:
-            planet.exceptions.ClientError: If the checksum fails.
-        """
-        if checksum.upper() == 'MD5':
-            hash_type = hashlib.md5
-        elif checksum.upper() == 'SHA256':
-            hash_type = hashlib.sha256
-        else:
-            raise exceptions.ClientError(
-                'Checksum ({checksum}) must be one of MD5 or SHA256.')
-
-        for json_entry in manifest_data['files']:
-            origin_hash = json_entry['digests'][checksum.lower()]
-            filename = Path(directory / json_entry['path'])
-
-            try:
-                returned_hash = hash_type(filename.read_bytes()).hexdigest()
-            except FileNotFoundError:
-                raise exceptions.ClientError(
-                    'Checksum failed. File ({filename}) does not exist.')
-
-            if origin_hash != returned_hash:
-                raise exceptions.ClientError(
-                    'File ({filename}) checksums do not match.')
-
     async def download_order(self,
                              order_id: str,
                              directory: Path = Path('.'),
@@ -318,19 +282,12 @@ class OrdersClient:
                              checksum: str = None) -> typing.List[Path]:
         """Download all assets in an order.
 
-        If 'checksum' is specified, the checksums given in the manifest will
-        be validated against checksums calculated from the downloaded files.
-        Checksum validation will fail if any of the files are missing or if the
-        checksums do not match.
-
         Parameters:
             order_id: The ID of the order.
             directory: Base directory for file download. This directory must
                 already exist.
             overwrite: Overwrite files if they already exist.
             progress_bar: Show progress bar during download.
-            checksum: Perform checksum validation against the specified
-                checksum hash type - MD5 or SHA256.
 
         Returns:
             Paths to downloaded files.
@@ -338,7 +295,7 @@ class OrdersClient:
         Raises:
             planet.exceptions.APIError: On API error.
             planet.exceptions.ClientError: If the order is not in a final
-                state or if checksum validation fails.
+                state.
         """
         order = await self.get_order(order_id)
         order_state = order['state']
@@ -360,21 +317,6 @@ class OrdersClient:
                                       progress_bar=progress_bar) for i in info
         ]
 
-        if checksum:
-            manifest_files = [
-                f for f in filenames if f.name == 'manifest.json'
-            ]
-            manifest_count = len(manifest_files)
-            if manifest_count > 1:
-                raise exceptions.ClientError(
-                    'Only 1 manifest.json expected per order.\
-                                             Recieved: {manifest_count}')
-            elif manifest_count is None:
-                raise exceptions.ClientError('No manifest file found.')
-
-            manifest_file = manifest_files[0]
-            manifest_data = json.loads(manifest_file.read_bytes())
-            self._validate_checksum(manifest_data, directory, checksum)
         return filenames
 
     @staticmethod
@@ -399,6 +341,56 @@ class OrdersClient:
             info = []
         return info
 
+    @staticmethod
+    def validate_checksum(directory: Path, checksum: str):
+        """Validate checksums of downloaded files against order manifest.
+
+        For each file entry in the order manifest, the specified checksum given
+        in the manifest file will be validated against the checksum calculated
+        from the downloaded file.
+
+        Parameters:
+            directory: Path to order directory.
+            checksum: The type of checksum hash- 'MD5' or 'SHA256'.
+
+        Raises:
+            planet.exceptions.ClientError: If a file is missing or if checksums
+                do not match.
+        """
+        manifest_path = directory / 'manifest.json'
+
+        try:
+            manifest_data = json.loads(manifest_path.read_text())
+        except FileNotFoundError:
+            raise exceptions.ClientError(
+                f'File ({manifest_path}) does not exist.')
+        except json.decoder.JSONDecodeError:
+            raise exceptions.ClientError(
+                f'Manifest file ({manifest_path}) does not contain valid JSON.'
+            )
+
+        if checksum.upper() == 'MD5':
+            hash_type = hashlib.md5
+        elif checksum.upper() == 'SHA256':
+            hash_type = hashlib.sha256
+        else:
+            raise exceptions.ClientError(
+                f'Checksum ({checksum}) must be one of MD5 or SHA256.')
+
+        for json_entry in manifest_data['files']:
+            origin_hash = json_entry['digests'][checksum.lower()]
+
+            filename = directory / json_entry['path']
+            try:
+                returned_hash = hash_type(filename.read_bytes()).hexdigest()
+            except FileNotFoundError:
+                raise exceptions.ClientError(
+                    f'Checksum failed. File ({filename}) does not exist.')
+
+            if origin_hash != returned_hash:
+                raise exceptions.ClientError(
+                    f'File ({filename}) checksums do not match.')
+
     async def wait(self,
                    order_id: str,
                    state: str = None,


Test patch:
diff --git a/tests/integration/test_orders_api.py b/tests/integration/test_orders_api.py
index 588a8a0..db4b199 100644
--- a/tests/integration/test_orders_api.py
+++ b/tests/integration/test_orders_api.py
@@ -12,15 +12,16 @@
 # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations under
 # the License.
+from contextlib import nullcontext as does_not_raise
 import copy
 import json
+import hashlib
 from http import HTTPStatus
 import logging
 import math
 import os
 from pathlib import Path
 from unittest.mock import call, create_autospec
-import hashlib
 
 import httpx
 import pytest
@@ -552,176 +553,86 @@ async def test_download_asset_md(tmpdir, session):
 @respx.mock
 @pytest.mark.asyncio
 @pytest.mark.parametrize("checksum", [("MD5"), ("SHA256")])
-async def test_download_order_checksum_success(tmpdir,
-                                               order_description,
-                                               oid,
-                                               session,
-                                               checksum):
-    # Mock an HTTP response for download
-    order_description['state'] = 'success'
-    dl_url1 = TEST_DOWNLOAD_URL + '/asset1'
-    dl_url2 = TEST_DOWNLOAD_URL + '/asset2'
-    dl_url3 = TEST_DOWNLOAD_URL + '/manifest'
-    order_description['_links']['results'] = [
-        {
-            'location': dl_url1, 'name': 'oid/itemtype1/asset1.tif'
-        }, {
-            'location': dl_url2, 'name': 'oid/itemtype1/asset2.json'
-        }, {
-            'location': dl_url3, 'name': 'oid/itemtype1/manifest.json'
-        }]  # yapf: disable
-
-    get_url = f'{TEST_ORDERS_URL}/{oid}'
-    get_order_response = httpx.Response(HTTPStatus.OK, json=order_description)
-    respx.get(get_url).return_value = get_order_response
-
-    # The first asset in the order:
-    # A 1-byte binary file containing the value "1".
-    asset1_content = b"1"
-    asset1_response = httpx.Response(HTTPStatus.OK,
-                                     content=asset1_content,
-                                     headers={
-                                         'Content-Type':
-                                         'image/tiff',
-                                         'Content-Disposition':
-                                         'attachment; filename="asset1.tif"'
-                                     })
-    respx.get(dl_url1).return_value = asset1_response
-
-    asset2_response = httpx.Response(HTTPStatus.OK,
-                                     json={'foo': 'bar'},
-                                     headers={
-                                         'Content-Type':
-                                         'application/json',
-                                         'Content-Disposition':
-                                         'attachment; filename="asset2.json"'
-                                     })
-    respx.get(dl_url2).return_value = asset2_response
-    # Create a mock manifest which has the correct hashkeys for each asset
-    manifest = httpx.Response(
-        HTTPStatus.OK,
-        json={
-            "name":
-            "",
-            "files":
-            [{
-                "path": "oid/itemtype1/asset1.tif",
+@pytest.mark.parametrize(
+    "asset1_bytes, expectation",
+    [(b"1", does_not_raise()), (b"1", does_not_raise()),
+     (b"does not match", pytest.raises(exceptions.ClientError))])
+async def test_validate_checksum_checksum(tmpdir,
+                                          asset1_bytes,
+                                          expectation,
+                                          checksum):
+
+    itemtype1_dir = Path(tmpdir, 'itemtype1')
+    itemtype1_dir.mkdir()
+
+    asset1 = itemtype1_dir / 'asset1.tif'
+    asset1.write_bytes(b"1")
+
+    asset2 = itemtype1_dir / 'asset2.json'
+    asset2.write_bytes(b'{"foo": "bar"}')
+    asset2_bytes = asset2.read_bytes()
+
+    manifest_data = {
+        "name": "",
+        "files": [
+            {
+                "path": "itemtype1/asset1.tif",
                 "digests": {
-                    "md5": hashlib.md5(asset1_response.content).hexdigest(),
-                    "sha256":
-                    hashlib.sha256(asset1_response.content).hexdigest()
-                },
-            },
-             {
-                 "path": "oid/itemtype1/asset2.json",
-                 "digests": {
-                     "md5": hashlib.md5(asset2_response.content).hexdigest(),
-                     "sha256":
-                     hashlib.sha256(asset2_response.content).hexdigest()
-                 }
-             }]
-        },
-        headers={
-            'Content-Type': 'application/json',
-            'Content-Disposition': 'attachment; filename="manifest.json"'
-        })
-    respx.get(dl_url3).return_value = manifest
+                    "md5": hashlib.md5(asset1_bytes).hexdigest(),
+                    "sha256": hashlib.sha256(asset1_bytes).hexdigest()}
+            }, {
+                "path": "itemtype1/asset2.json",
+                "digests": {
+                    "md5": hashlib.md5(asset2_bytes).hexdigest(),
+                    "sha256": hashlib.sha256(asset2_bytes).hexdigest()}
+            }]
+    }  # yapf: disable
+    Path(tmpdir, 'manifest.json').write_text(json.dumps(manifest_data))
 
-    cl = OrdersClient(session, base_url=TEST_URL)
-    await cl.download_order(oid, directory=Path(tmpdir), checksum=checksum)
+    with expectation:
+        OrdersClient.validate_checksum(Path(tmpdir), checksum)
 
 
 @respx.mock
 @pytest.mark.asyncio
-@pytest.mark.parametrize("checksum", [("MD5"), ("SHA256")])
-async def test_download_order_checksum_failure(tmpdir,
-                                               order_description,
-                                               oid,
-                                               session,
-                                               checksum):
-    # Note: the hashkeys in the mock manifest below were changed
-    # from the correct keys to temporary keys
-    # This should cause the checksum to fail.
-    # Mock an HTTP response for download
-    order_description['state'] = 'success'
-    dl_url1 = TEST_DOWNLOAD_URL + '/asset1'
-    dl_url2 = TEST_DOWNLOAD_URL + '/asset2'
-    dl_url3 = TEST_DOWNLOAD_URL + '/manifest'
-    order_description['_links']['results'] = [{
-        'location': dl_url1, 'name': 'oid/itemtype1/asset.tif'
-    },
-                                              {
-                                                  'location':
-                                                  dl_url2,
-                                                  'name':
-                                                  'oid/itemtype1/asset2.json'
-                                              },
-                                              {
-                                                  'location':
-                                                  dl_url3,
-                                                  'name':
-                                                  'oid/itemtype1/manifest.json'
-                                              }]
-    get_url = f'{TEST_ORDERS_URL}/{oid}'
-    get_order_response = httpx.Response(HTTPStatus.OK, json=order_description)
-    respx.get(get_url).return_value = get_order_response
-
-    # The first asset in the order:
-    # A 1-byte binary file containing the value "1".
-    asset1_content = b"1"
-    asset1_response = httpx.Response(HTTPStatus.OK,
-                                     content=asset1_content,
-                                     headers={
-                                         'Content-Type':
-                                         'image/tiff',
-                                         'Content-Disposition':
-                                         'attachment; filename="asset1.tif"'
-                                     })
-    respx.get(dl_url1).return_value = asset1_response
-
-    # The second asset in the order is a json file
-    asset2_response = httpx.Response(HTTPStatus.OK,
-                                     json={'foo': 'bar'},
-                                     headers={
-                                         'Content-Type':
-                                         'application/json',
-                                         'Content-Disposition':
-                                         'attachment; filename="asset2.json"'
-                                     })
-    respx.get(dl_url2).return_value = asset2_response
-
-    manifest = httpx.Response(HTTPStatus.OK,
-                              json={
-                                  "name":
-                                  "",
-                                  "files": [{
-                                      "path": "asset1.tif",
-                                      "digests": {
-                                          "md5": "manifest_key1_md5",
-                                          "sha256": "manifest_key1_sha256"
-                                      },
-                                  },
-                                            {
-                                                "path": "asset2.json",
-                                                "digests": {
-                                                    "md5":
-                                                    "manifest_key2_md5",
-                                                    "sha256":
-                                                    "manifest_key2_sha256"
-                                                }
-                                            }]
-                              },
-                              headers={
-                                  'Content-Type':
-                                  'application/json',
-                                  'Content-Disposition':
-                                  'attachment; filename="manifest.json"'
-                              })
-    respx.get(dl_url3).return_value = manifest
-    # Test Checksum
-    cl = OrdersClient(session, base_url=TEST_URL)
-    with pytest.raises(exceptions.ClientError):
-        await cl.download_order(oid, directory=Path(tmpdir), checksum=checksum)
+@pytest.mark.parametrize(
+    "create, corrupt, expectation",
+    [(True, False, does_not_raise()),
+     (True, True, pytest.raises(exceptions.ClientError)),
+     (False, False, pytest.raises(exceptions.ClientError))])
+async def test_validate_checksum_manifest(
+    tmpdir,
+    create,
+    corrupt,
+    expectation,
+):
+    itemtype1_dir = Path(tmpdir, 'itemtype1')
+    itemtype1_dir.mkdir()
+
+    asset1 = itemtype1_dir / 'asset1.tif'
+    asset1.write_bytes(b"1")
+
+    Path(tmpdir, 'asset1.tif').write_bytes(b"1")
+
+    manifest_data = {
+        "name":
+        "",
+        "files": [{
+            "path": "itemtype1/asset1.tif",
+            "digests": {
+                "md5": hashlib.md5(b"1").hexdigest(),
+                "sha256": hashlib.sha256(b"1").hexdigest()
+            },
+        }]
+    }
+    if create:
+        if corrupt:
+            Path(tmpdir, 'manifest.json').write_text('not json')
+        else:
+            Path(tmpdir, 'manifest.json').write_text(json.dumps(manifest_data))
+
+    with expectation:
+        OrdersClient.validate_checksum(Path(tmpdir), 'md5')
 
 
 @respx.mock
diff --git a/tests/integration/test_orders_cli.py b/tests/integration/test_orders_cli.py
index 38275c6..4aa5ed8 100644
--- a/tests/integration/test_orders_cli.py
+++ b/tests/integration/test_orders_cli.py
@@ -13,6 +13,7 @@
 # the License.
 '''Test Orders CLI'''
 import copy
+import hashlib
 from http import HTTPStatus
 import json
 from pathlib import Path
@@ -256,17 +257,18 @@ def mock_download_response(oid, order_description):
         order_description['state'] = 'success'
         dl_url1 = TEST_DOWNLOAD_URL + '/1?token=IAmAToken'
         dl_url2 = TEST_DOWNLOAD_URL + '/2?token=IAmAnotherToken'
+        dl_url3 = TEST_DOWNLOAD_URL + '/manifest'
+
         order_description['_links']['results'] = [{
-            'location':
-            dl_url1, 'name':
-            'oid/itemtype/m1.json'
-        },
-                                                  {
-                                                      'location':
-                                                      dl_url2,
-                                                      'name':
-                                                      'oid/itemtype/m2.json'
-                                                  }]
+            'location': dl_url1,
+            'name': 'oid/itemtype/m1.json'
+        }, {
+            'location': dl_url2,
+            'name': 'oid/itemtype/m2.json'
+        }, {
+            'location': dl_url3,
+            'name': 'oid/manifest.json'
+        }]  # yapf: disable
 
         get_url = f'{TEST_ORDERS_URL}/{oid}'
         mock_resp = httpx.Response(HTTPStatus.OK, json=order_description)
@@ -292,10 +294,36 @@ def mock_download_response(oid, order_description):
                                     })
         respx.get(dl_url2).return_value = mock_resp2
 
+        m1_bytes = b'{"key": "value"}'
+        m2_bytes = b'{"key2": "value2"}'
+        manifest_data = {
+            "name": "",
+            "files": [
+                {
+                    "path": "itemtype/m1.json",
+                    "digests": {
+                        "md5": hashlib.md5(m1_bytes).hexdigest(),
+                        "sha256": hashlib.sha256(m1_bytes).hexdigest()}
+                }, {
+                    "path": "itemtype/m2.json",
+                    "digests": {
+                        "md5": hashlib.md5(m2_bytes).hexdigest(),
+                        "sha256": hashlib.sha256(m2_bytes).hexdigest()}
+                }]
+        }  # yapf: disable
+        mock_resp3 = httpx.Response(HTTPStatus.OK,
+                                    json=manifest_data,
+                                    headers={
+                                        'Content-Type':
+                                        'application/json',
+                                        'Content-Disposition':
+                                        'attachment; filename="manifest.json"'
+                                    })
+        respx.get(dl_url3).return_value = mock_resp3
+
     return _func
 
 
-# TODO: add test for --checksum (see gh-432).
 @respx.mock
 def test_cli_orders_download_default(invoke, mock_download_response, oid):
     mock_download_response()
@@ -315,6 +343,25 @@ def test_cli_orders_download_default(invoke, mock_download_response, oid):
         assert json.load(open(f2_path)) == {'key2': 'value2'}
 
 
+@respx.mock
+def test_cli_orders_download_checksum(invoke, mock_download_response, oid):
+    mock_download_response()
+
+    runner = CliRunner()
+    with runner.isolated_filesystem() as folder:
+        result = invoke(['download', oid], runner=runner)
+        assert not result.exception
+
+        # basic check of progress reporting
+        assert 'm1.json' in result.output
+
+        # Check that the files were downloaded and have the correct contents
+        f1_path = Path(folder) / 'oid/itemtype/m1.json'
+        assert json.load(open(f1_path)) == {'key': 'value'}
+        f2_path = Path(folder) / 'oid/itemtype/m2.json'
+        assert json.load(open(f2_path)) == {'key2': 'value2'}
+
+
 @respx.mock
 def test_cli_orders_download_dest(invoke, mock_download_response, oid):
     mock_download_response()