From bde54dec2d2f9ef761d2e4afe7a9b9fcf3471ec9 Mon Sep 17 00:00:00 2001 From: Rob Emanuele Date: Sat, 14 Nov 2020 13:46:40 -0500 Subject: [PATCH 1/8] Add planet metadata properties that don't map to STAC metadata. This commit adds any properties that are included in the Planet item metadata that doesn't map directly to a STAC metadata property. It will append a "pl:" prefix on these properties and include them in the STAC Item's properties. --- stactools_planet/stactools/planet/constants.py | 2 ++ stactools_planet/stactools/planet/planet_item.py | 5 +++++ tests/planet/test_commands.py | 2 ++ 3 files changed, 9 insertions(+) diff --git a/stactools_planet/stactools/planet/constants.py b/stactools_planet/stactools/planet/constants.py index 4d034999..3c14c647 100644 --- a/stactools_planet/stactools/planet/constants.py +++ b/stactools_planet/stactools/planet/constants.py @@ -1,5 +1,7 @@ import pystac +PLANET_EXTENSION_PREFIX = 'pl' + PLANET_PROVIDER = pystac.Provider( name='Planet', description=( diff --git a/stactools_planet/stactools/planet/planet_item.py b/stactools_planet/stactools/planet/planet_item.py index 72eeb71b..6d74566f 100644 --- a/stactools_planet/stactools/planet/planet_item.py +++ b/stactools_planet/stactools/planet/planet_item.py @@ -8,6 +8,7 @@ from shapely.geometry import shape from stactools.planet import PLANET_PROVIDER +from stactools.planet.constants import PLANET_EXTENSION_PREFIX logger = logging.getLogger(__name__) @@ -64,6 +65,10 @@ def to_stac(self): item.ext.enable('projection') item.ext.projection.epsg = props.pop('epsg_code') + # Add all additional properties with Planet extension designation. + for k, v in props.items(): + item.properties['{}:{}'.format(PLANET_EXTENSION_PREFIX, k)] = v + for planet_asset in self.item_assets: href = make_absolute_href(planet_asset['path'], start_href=self.base_dir, diff --git a/tests/planet/test_commands.py b/tests/planet/test_commands.py index e9301d55..a592d15e 100644 --- a/tests/planet/test_commands.py +++ b/tests/planet/test_commands.py @@ -46,3 +46,5 @@ def test_converts(self): self.assertTrue(os.path.exists(asset.get_absolute_href())) self.assertEqual(os.path.dirname(asset.get_absolute_href()), os.path.dirname(item.get_self_href())) + + self.assertEqual(item.properties.get('pl:quality_category'), 'standard') From 64fd5d053d7d7f5d8ce93e87695269ed712b711d Mon Sep 17 00:00:00 2001 From: Rob Emanuele Date: Sat, 14 Nov 2020 14:53:38 -0500 Subject: [PATCH 2/8] Refactor to help make CLI tests work the same. I ran into an instance where I forgot to put 'catch_exceptions=False' in a new unit test, which ate some time up to debug. Refactor how we run CLI tests to standardize the way they run to avoid future issues and make it simpler to write new tests. --- tests/cli/commands/test_copy.py | 19 +++++-------------- tests/planet/test_commands.py | 25 ++++++++----------------- tests/utils/__init__.py | 1 + tests/utils/cli_test.py | 25 +++++++++++++++++++++++++ 4 files changed, 39 insertions(+), 31 deletions(-) create mode 100644 tests/utils/cli_test.py diff --git a/tests/cli/commands/test_copy.py b/tests/cli/commands/test_copy.py index 50d9c55b..0cc56876 100644 --- a/tests/cli/commands/test_copy.py +++ b/tests/cli/commands/test_copy.py @@ -1,30 +1,21 @@ import os -import unittest from tempfile import TemporaryDirectory -import click -from click.testing import CliRunner import pystac from stactools.cli.commands.copy import create_copy_command -from tests.utils import TestCases +from tests.utils import (TestCases, CliTestCase) -class CopyTest(unittest.TestCase): - def setUp(self): - @click.group() - def cli(): - pass - - create_copy_command(cli) - self.cli = cli +class CopyTest(CliTestCase): + def create_subcommand_functions(self): + return [create_copy_command] def test_copy(self): cat = TestCases.planet_disaster() item_ids = set([i.id for i in cat.get_all_items()]) with TemporaryDirectory() as tmp_dir: - runner = CliRunner() - runner.invoke(self.cli, ['copy', cat.get_self_href(), tmp_dir]) + self.run_command(['copy', cat.get_self_href(), tmp_dir]) copy_cat = pystac.read_file( os.path.join(tmp_dir, 'collection.json')) diff --git a/tests/planet/test_commands.py b/tests/planet/test_commands.py index a592d15e..70c5db86 100644 --- a/tests/planet/test_commands.py +++ b/tests/planet/test_commands.py @@ -1,36 +1,26 @@ import os -import unittest from tempfile import TemporaryDirectory -import click -from click.testing import CliRunner import pystac from stactools.planet.commands import create_planet_command -from tests.utils import TestCases +from tests.utils import (TestCases, CliTestCase) -class ConvertOrderTest(unittest.TestCase): - def setUp(self): - @click.group() - def cli(): - pass - - create_planet_command(cli) - self.cli = cli +class ConvertOrderTest(CliTestCase): + def create_subcommand_functions(self): + return [create_planet_command] def test_converts(self): test_order_manifest = TestCases.get_path( 'data-files/planet-order/manifest.json') with TemporaryDirectory() as tmp_dir: - runner = CliRunner() - result = runner.invoke(self.cli, [ + result = self.run_command([ 'planet', 'convert-order', test_order_manifest, tmp_dir, 'test_id', 'A test catalog', '--title', 'test-catalog', '-a', 'copy' - ], - catch_exceptions=False) + ]) self.assertEqual(result.exit_code, 0, @@ -47,4 +37,5 @@ def test_converts(self): self.assertEqual(os.path.dirname(asset.get_absolute_href()), os.path.dirname(item.get_self_href())) - self.assertEqual(item.properties.get('pl:quality_category'), 'standard') + self.assertEqual(item.properties.get('pl:quality_category'), + 'standard') diff --git a/tests/utils/__init__.py b/tests/utils/__init__.py index 0f23a407..d2add667 100644 --- a/tests/utils/__init__.py +++ b/tests/utils/__init__.py @@ -1,3 +1,4 @@ # flake8: noqa from tests.utils.test_cases import TestCases +from tests.utils.cli_test import CliTestCase diff --git a/tests/utils/cli_test.py b/tests/utils/cli_test.py new file mode 100644 index 00000000..607e3eb6 --- /dev/null +++ b/tests/utils/cli_test.py @@ -0,0 +1,25 @@ +from abc import (ABC, abstractmethod) +import unittest + +import click +from click.testing import CliRunner + + +class CliTestCase(unittest.TestCase, ABC): + def setUp(self): + @click.group() + def cli(): + pass + + for create_subcommand in self.create_subcommand_functions(): + create_subcommand(cli) + self.cli = cli + + def run_command(self, cmd): + runner = CliRunner() + return runner.invoke(self.cli, cmd, catch_exceptions=False) + + @abstractmethod + def create_subcommand_functions(self): + """Return list of 'create_command' functions from implementations""" + pass From ae6b185cd06cabc492186694f01bf55c0fab5f1a Mon Sep 17 00:00:00 2001 From: Rob Emanuele Date: Sat, 14 Nov 2020 15:15:55 -0500 Subject: [PATCH 3/8] Fixed issue where param name and method were the same. --- stactools_core/stactools/core/merge.py | 9 +++-- tests/cli/commands/test_merge.py | 56 ++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 tests/cli/commands/test_merge.py diff --git a/stactools_core/stactools/core/merge.py b/stactools_core/stactools/core/merge.py index 31adfca2..8959c6a2 100644 --- a/stactools_core/stactools/core/merge.py +++ b/stactools_core/stactools/core/merge.py @@ -5,7 +5,8 @@ from pystac.utils import (is_absolute_href, make_relative_href) from shapely.geometry import shape, mapping -from stactools.core.copy import (move_asset_file_to_item) +from stactools.core.copy import (move_asset_file_to_item, move_assets as + do_move_assets) def merge_items(source_item, @@ -113,8 +114,8 @@ def merge_all_items(source_catalog, item.set_collection(None) if move_assets: - move_assets(item_copy, - href_type=pystac.LinkType.RELATIVE, - copy=False) + do_move_assets(item_copy, + href_type=pystac.LinkType.RELATIVE, + copy=False) return target_catalog diff --git a/tests/cli/commands/test_merge.py b/tests/cli/commands/test_merge.py new file mode 100644 index 00000000..2866dd9b --- /dev/null +++ b/tests/cli/commands/test_merge.py @@ -0,0 +1,56 @@ +import os +from tempfile import TemporaryDirectory + +import pystac + +from stactools.cli.commands.merge import create_merge_command +from stactools.core import move_all_assets +from tests.utils import (TestCases, CliTestCase) + + +def copy_two_planet_disaster_subsets(tmp_dir): + """Test setup util that makes two copies of subset of the planet + disaster data, each containing a single item. Updates the collection + extents to match the single items. + + Returns a list of collection paths in the temporary directory. + """ + item_ids = ['20170831_172754_101c', '20170831_162740_ssc1d1'] + new_cols = [] + for item_id in item_ids: + col = TestCases.planet_disaster() + for item in list(col.get_all_items()): + if item.id != item_id: + item.get_parent().remove_item(item.id) + col.update_extent_from_items() + col.normalize_hrefs(os.path.join(tmp_dir, item_id)) + col.save(catalog_type=pystac.CatalogType.SELF_CONTAINED) + col = move_all_assets(col, copy=True) + col.save() + new_cols.append(col.get_self_href()) + + return new_cols + + +class MergeTest(CliTestCase): + def create_subcommand_functions(self): + return [create_merge_command] + + def test_merge_moves_assets(self): + with TemporaryDirectory() as tmp_dir: + col_paths = copy_two_planet_disaster_subsets(tmp_dir) + + cmd = ['merge', '-a', col_paths[0], col_paths[1]] + + self.run_command(cmd) + + target_col = pystac.read_file(col_paths[1]) + + items = list(target_col.get_all_items()) + self.assertEqual(len(items), 2) + + for item in items: + for asset in item.assets.values(): + self.assertEqual( + os.path.dirname(asset.get_absolute_href()), + os.path.dirname(item.get_self_href())) From a636e13419fd7be3ac7fe6300c4ebb7e053b7cdf Mon Sep 17 00:00:00 2001 From: Rob Emanuele Date: Sat, 14 Nov 2020 15:20:03 -0500 Subject: [PATCH 4/8] Fix issue with collection being set on the wrong item. The collection should be set on the item_copy, not the original item. --- stactools_core/stactools/core/merge.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stactools_core/stactools/core/merge.py b/stactools_core/stactools/core/merge.py index 8959c6a2..e7b83534 100644 --- a/stactools_core/stactools/core/merge.py +++ b/stactools_core/stactools/core/merge.py @@ -109,9 +109,9 @@ def merge_all_items(source_catalog, target_catalog.add_item(item_copy) if isinstance(target_catalog, pystac.Collection): - item.set_collection(target_catalog) + item_copy.set_collection(target_catalog) else: - item.set_collection(None) + item_copy.set_collection(None) if move_assets: do_move_assets(item_copy, From cd5ebcbae43b6b05e8db09af3560a25efe08d3f5 Mon Sep 17 00:00:00 2001 From: Rob Emanuele Date: Sat, 14 Nov 2020 15:20:39 -0500 Subject: [PATCH 5/8] Make move_all_assets mutate the catalog. This function was using 'map_items', which makes a copy of the catalog, and so did not mutate the input catalog. This was inconsistent with the other functions in the library and could easily cause confusion. --- stactools_core/stactools/core/copy.py | 10 ++++++---- tests/cli/commands/test_merge.py | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/stactools_core/stactools/core/copy.py b/stactools_core/stactools/core/copy.py index cb604dbd..7514f7e6 100644 --- a/stactools_core/stactools/core/copy.py +++ b/stactools_core/stactools/core/copy.py @@ -193,12 +193,14 @@ def move_all_assets(catalog, this function will throw an error unless ignore_conflicts is True. Returns: - [Catalog or Collection]: Returns an updated catalog or collection. - Note that this method does not mutate the original catalog. + [Catalog or Collection]: Returns the updated catalog. + This operation mutates the catalog. """ + for item in catalog.get_all_items(): + move_assets(item, asset_subdirectory, href_type, copy, + ignore_conflicts) - return catalog.map_items(lambda item: move_assets( - item, asset_subdirectory, href_type, copy, ignore_conflicts)) + return catalog def copy_catalog(source_catalog, diff --git a/tests/cli/commands/test_merge.py b/tests/cli/commands/test_merge.py index 2866dd9b..84127ca9 100644 --- a/tests/cli/commands/test_merge.py +++ b/tests/cli/commands/test_merge.py @@ -25,7 +25,7 @@ def copy_two_planet_disaster_subsets(tmp_dir): col.update_extent_from_items() col.normalize_hrefs(os.path.join(tmp_dir, item_id)) col.save(catalog_type=pystac.CatalogType.SELF_CONTAINED) - col = move_all_assets(col, copy=True) + move_all_assets(col, copy=True) col.save() new_cols.append(col.get_self_href()) From e1fa7ef665161fb87a828e45235936a85c548b02 Mon Sep 17 00:00:00 2001 From: Rob Emanuele Date: Sat, 14 Nov 2020 15:41:59 -0500 Subject: [PATCH 6/8] When merging items into collections, update collection extent. --- stactools_core/stactools/core/merge.py | 3 ++ tests/cli/commands/test_merge.py | 43 ++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/stactools_core/stactools/core/merge.py b/stactools_core/stactools/core/merge.py index e7b83534..c89a7a2d 100644 --- a/stactools_core/stactools/core/merge.py +++ b/stactools_core/stactools/core/merge.py @@ -118,4 +118,7 @@ def merge_all_items(source_catalog, href_type=pystac.LinkType.RELATIVE, copy=False) + if target_catalog.STAC_OBJECT_TYPE == pystac.STACObjectType.COLLECTION: + target_catalog.update_extent_from_items() + return target_catalog diff --git a/tests/cli/commands/test_merge.py b/tests/cli/commands/test_merge.py index 84127ca9..3c5f2aa1 100644 --- a/tests/cli/commands/test_merge.py +++ b/tests/cli/commands/test_merge.py @@ -54,3 +54,46 @@ def test_merge_moves_assets(self): self.assertEqual( os.path.dirname(asset.get_absolute_href()), os.path.dirname(item.get_self_href())) + + def test_merge_updates_collection_extent(self): + with TemporaryDirectory() as tmp_dir: + col_paths = copy_two_planet_disaster_subsets(tmp_dir) + + extent1 = pystac.read_file(col_paths[0]).extent + extent2 = pystac.read_file(col_paths[1]).extent + + xmin = min( + [extent1.spatial.bboxes[0][0], extent2.spatial.bboxes[0][0]]) + time_max = max([ + extent1.temporal.intervals[0][1], + extent2.temporal.intervals[0][1], + ]) + + cmd = ['merge', col_paths[0], col_paths[1]] + + self.run_command(cmd) + + result_extent = pystac.read_file(col_paths[1]).extent + + def set_of_values(x): + result = set([]) + if type(x) is dict: + result |= set_of_values(list(x.values())) + elif type(x) is list: + for e in x: + if type(e) is list: + result |= set_of_values(e) + else: + result.add(e) + return result + + # Make sure it didn't just carry forward the old extent + self.assertNotEqual(set_of_values(result_extent.spatial.bboxes), + set_of_values(extent2.spatial.bboxes)) + + self.assertNotEqual( + set_of_values(result_extent.temporal.intervals), + set_of_values(extent2.temporal.intervals)) + + self.assertEqual(result_extent.spatial.bboxes[0][0], xmin) + self.assertEqual(result_extent.temporal.intervals[0][1], time_max) From f76841d4c6d51270c994b0f48271e3a27ad93ab8 Mon Sep 17 00:00:00 2001 From: Rob Emanuele Date: Sat, 14 Nov 2020 15:43:15 -0500 Subject: [PATCH 7/8] Use -h as short flag for --include-hrefs --- stactools_cli/stactools/cli/commands/info.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/stactools_cli/stactools/cli/commands/info.py b/stactools_cli/stactools/cli/commands/info.py index b21e4a10..513b248f 100644 --- a/stactools_cli/stactools/cli/commands/info.py +++ b/stactools_cli/stactools/cli/commands/info.py @@ -57,7 +57,8 @@ def create_describe_command(cli): short_help='Prints out a list of all catalogs, collections and items ' 'in this STAC.') @click.argument('catalog_path') - @click.option('--include-hrefs', + @click.option('-h', + '--include-hrefs', is_flag=True, help='Include HREFs in description.') def describe_command(catalog_path, include_hrefs): From 62c35350b72b08e2ee45727da9ce0a2556de35d1 Mon Sep 17 00:00:00 2001 From: Rob Emanuele Date: Sat, 14 Nov 2020 15:45:04 -0500 Subject: [PATCH 8/8] Add a note in the CLI documentation that copy will migrate catalog --- stactools_cli/stactools/cli/commands/copy.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/stactools_cli/stactools/cli/commands/copy.py b/stactools_cli/stactools/cli/commands/copy.py index 0fdbd61c..5f9d16ac 100644 --- a/stactools_cli/stactools/cli/commands/copy.py +++ b/stactools_cli/stactools/cli/commands/copy.py @@ -51,7 +51,9 @@ def create_copy_command(cli): 'be alongside the new item location.')) def copy_command(src, dst, catalog_type, copy_assets): """Copy a STAC Catalog or Collection at SRC to the directory - at DST.""" + at DST. + + Note: Copying a catalog will upgrade it to the latest version of STAC.""" source_catalog = pystac.read_file(src) copy_catalog(source_catalog, dst, catalog_type, copy_assets)