Change keep_parent to set_parent and add flag on obj indicating p…

…arent override of href (#1155) * Don't override parent link when determining hrefs #1116 * Add an attribute to the class to mark if it is an orphan * Use @m-mohr's suggestion for when to set `self_href` * Add test based on example * Change `keep_parent` to `set_parent` * Add note about self_link to docstrings --------- Co-authored-by: Matthias Mohr <webmaster@mamo-net.de>
stac-utils · Jun 14, 2023 · 522a946 · 522a946
1 parent 6d59297
commit 522a946
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,7 +9,7 @@
 - `recursive` to Catalog and Collection `get_items()` to walk the sub-catalogs and sub-collections ([#1075](https://github.com/stac-utils/pystac/pull/1075))
 - MGRS Extension ([#1088](https://github.com/stac-utils/pystac/pull/1088))
 - All HTTP requests are logged when level is set to `logging.DEBUG` ([#1096](https://github.com/stac-utils/pystac/pull/1096))
-- `keep_parent` to Catalog `add_item` and `add_child` to avoid overriding existing parents ([#1117](https://github.com/stac-utils/pystac/pull/1117))
+- `set_parent` to Catalog `add_item` and `add_child` to avoid overriding existing parents ([#1117](https://github.com/stac-utils/pystac/pull/1117), [#1155](https://github.com/stac-utils/pystac/pull/1155))
 - `owner` attribute to `AssetDefinition` in the item-assets extension ([#1110](https://github.com/stac-utils/pystac/pull/1110))
 - Windows `\\` path delimiters are converted to POSIX style `/` delimiters ([#1125](https://github.com/stac-utils/pystac/pull/1125))
 - Updated raster extension to work with the item_assets extension's AssetDefinition objects ([#1110](https://github.com/stac-utils/pystac/pull/1110))
@@ -44,6 +44,9 @@
 - Catalog `get_item()`. Use `get_items(id)` instead ([#1075](https://github.com/stac-utils/pystac/pull/1075))
 - Catalog and Collection `get_all_items`. Use `get_items(recursive=True)` instead ([#1075](https://github.com/stac-utils/pystac/pull/1075))
 
+### Fixed
+
+
 ## [v1.7.3]
 
 ### Fixed

diff --git a/pystac/catalog.py b/pystac/catalog.py
@@ -230,11 +230,14 @@ def add_child(
         child: Union["Catalog", Collection],
         title: Optional[str] = None,
         strategy: Optional[HrefLayoutStrategy] = None,
-        keep_parent: bool = False,
+        set_parent: bool = True,
     ) -> None:
         """Adds a link to a child :class:`~pystac.Catalog` or
-        :class:`~pystac.Collection`. This method will set the child's parent to this
-        object (except if a parent is set and keep_parent is true).
+        :class:`~pystac.Collection`.
+
+        This method will set the child's parent to this object and potentially
+        override its self_link (unless ``set_parent`` is False).
+
         It will always set its root to this Catalog's root.
 
         Args:
@@ -243,6 +246,8 @@ def add_child(
             strategy : The layout strategy to use for setting the
                 self href of the child. If not provided, defaults to
                 :class:`~pystac.layout.BestPracticesLayoutStrategy`.
+            set_parent : Whether to set the parent on the child as well.
+                Defaults to True.
         """
 
         # Prevent typo confusion
@@ -253,12 +258,14 @@ def add_child(
             strategy = BestPracticesLayoutStrategy()
 
         child.set_root(self.get_root())
-        if child.get_parent() is None or not keep_parent:
+        if set_parent:
             child.set_parent(self)
+        else:
+            child._allow_parent_to_override_href = False
 
         # set self link
         self_href = self.get_self_href()
-        if self_href:
+        if self_href and set_parent:
             child_href = strategy.get_href(child, os.path.dirname(self_href))
             child.set_self_href(child_href)
 
@@ -280,11 +287,13 @@ def add_item(
         item: Item,
         title: Optional[str] = None,
         strategy: Optional[HrefLayoutStrategy] = None,
-        keep_parent: bool = False,
+        set_parent: bool = True,
     ) -> None:
         """Adds a link to an :class:`~pystac.Item`.
-        This method will set the item's parent to this object (except if a parent
-        is set and keep_parent is true).
+
+        This method will set the item's parent to this object and potentially
+        override its self_link (unless ``set_parent`` is False)
+
         It will always set its root to this Catalog's root.
 
         Args:
@@ -293,6 +302,8 @@ def add_item(
             strategy : The layout strategy to use for setting the
                 self href of the item. If not provided, defaults to
                 :class:`~pystac.layout.BestPracticesLayoutStrategy`.
+            set_parent : Whether to set the parent on the item as well.
+                Defaults to True.
         """
 
         # Prevent typo confusion
@@ -303,12 +314,14 @@ def add_item(
             strategy = BestPracticesLayoutStrategy()
 
         item.set_root(self.get_root())
-        if item.get_parent() is None or not keep_parent:
+        if set_parent:
             item.set_parent(self)
+        else:
+            item._allow_parent_to_override_href = False
 
         # set self link
         self_href = self.get_self_href()
-        if self_href:
+        if self_href and set_parent:
             item_href = strategy.get_href(item, os.path.dirname(self_href))
             item.set_self_href(item_href)
 
@@ -717,10 +730,17 @@ def normalize_hrefs(
         if not is_absolute_href(root_href):
             root_href = make_absolute_href(root_href, os.getcwd(), start_is_dir=True)
 
-        def process_item(item: Item, _root_href: str) -> Callable[[], None]:
+        def process_item(
+            item: Item, _root_href: str, parent: Optional[Catalog]
+        ) -> Optional[Callable[[], None]]:
             if not skip_unresolved:
                 item.resolve_links()
 
+            # Abort as the intended parent is not the actual parent
+            # https://github.com/stac-utils/pystac/issues/1116
+            if parent is not None and item.get_parent() != parent:
+                return None
+
             new_self_href = _strategy.get_href(item, _root_href)
 
             def fn() -> None:
@@ -729,13 +749,21 @@ def fn() -> None:
             return fn
 
         def process_catalog(
-            cat: Catalog, _root_href: str, is_root: bool
+            cat: Catalog,
+            _root_href: str,
+            is_root: bool,
+            parent: Optional[Catalog] = None,
         ) -> List[Callable[[], None]]:
             setter_funcs: List[Callable[[], None]] = []
 
             if not skip_unresolved:
                 cat.resolve_links()
 
+            # Abort as the intended parent is not the actual parent
+            # https://github.com/stac-utils/pystac/issues/1116
+            if parent is not None and cat.get_parent() != parent:
+                return setter_funcs
+
             new_self_href = _strategy.get_href(cat, _root_href, is_root)
             new_root = os.path.dirname(new_self_href)
 
@@ -744,16 +772,19 @@ def process_catalog(
                     continue
                 elif link.rel == pystac.RelType.ITEM:
                     link.resolve_stac_object(root=self.get_root())
-                    setter_funcs.append(
-                        process_item(cast(pystac.Item, link.target), new_root)
+                    item_fn = process_item(
+                        cast(pystac.Item, link.target), new_root, cat
                     )
+                    if item_fn is not None:
+                        setter_funcs.append(item_fn)
                 elif link.rel == pystac.RelType.CHILD:
                     link.resolve_stac_object(root=self.get_root())
                     setter_funcs.extend(
                         process_catalog(
                             cast(Union[pystac.Catalog, pystac.Collection], link.target),
                             new_root,
                             is_root=False,
+                            parent=cat,
                         )
                     )
 

diff --git a/pystac/collection.py b/pystac/collection.py
@@ -543,9 +543,9 @@ def add_item(
         item: Item,
         title: Optional[str] = None,
         strategy: Optional[HrefLayoutStrategy] = None,
-        keep_parent: bool = False,
+        set_parent: bool = True,
     ) -> None:
-        super().add_item(item, title, strategy, keep_parent)
+        super().add_item(item, title, strategy, set_parent)
         item.set_collection(self)
 
     def to_dict(

diff --git a/pystac/link.py b/pystac/link.py
@@ -338,7 +338,10 @@ def resolve_stac_object(self, root: Optional[Catalog] = None) -> Link:
             and isinstance(self.owner, pystac.Catalog)
         ):
             assert self._target_object
-            self._target_object.set_parent(self.owner)
+            # Do nothing if the object wants to keep its parent
+            # https://github.com/stac-utils/pystac/issues/1116
+            if self._target_object._allow_parent_to_override_href:
+                self._target_object.set_parent(self.owner)
 
         return self
 

diff --git a/pystac/stac_object.py b/pystac/stac_object.py
@@ -58,6 +58,9 @@ class STACObject(ABC):
 
     STAC_OBJECT_TYPE: STACObjectType
 
+    _allow_parent_to_override_href: bool = True
+    """Private attribute for whether parent objects should override on normalization"""
+
     def __init__(self, stac_extensions: List[str]) -> None:
         self.links = []
         self.stac_extensions = stac_extensions

diff --git a/tests/test_catalog.py b/tests/test_catalog.py
@@ -240,16 +240,17 @@ def test_add_child_override_parent(self) -> None:
         parent2.add_child(child)
         assert child.get_parent() is parent2
 
-    def test_add_child_keep_parent(self) -> None:
+    def test_add_child_set_parent_false(self) -> None:
         parent1 = Catalog(id="parent1", description="test1")
         parent2 = Catalog(id="parent2", description="test2")
         child = Catalog(id="child", description="test3")
         assert child.get_parent() is None
 
-        parent1.add_child(child, keep_parent=True)
-        assert child.get_parent() is parent1
+        parent1.add_child(child, set_parent=False)
+        assert child.get_parent() is not parent1
 
-        parent2.add_child(child, keep_parent=True)
+        parent1.add_child(child)
+        parent2.add_child(child, set_parent=False)
         assert child.get_parent() is parent1
 
     def test_add_item_override_parent(self) -> None:
@@ -270,7 +271,7 @@ def test_add_item_override_parent(self) -> None:
         parent2.add_item(child)
         assert child.get_parent() is parent2
 
-    def test_add_item_keep_parent(self) -> None:
+    def test_add_item_set_parent_false(self) -> None:
         parent1 = Catalog(id="parent1", description="test1")
         parent2 = Catalog(id="parent2", description="test2")
         child = Item(
@@ -282,10 +283,11 @@ def test_add_item_keep_parent(self) -> None:
         )
         assert child.get_parent() is None
 
-        parent1.add_item(child, keep_parent=True)
-        assert child.get_parent() is parent1
+        parent1.add_item(child, set_parent=False)
+        assert child.get_parent() is not parent1
 
-        parent2.add_item(child, keep_parent=True)
+        parent1.add_item(child)
+        parent2.add_item(child, set_parent=False)
         assert child.get_parent() is parent1
 
     def test_add_item_throws_if_child(self) -> None:
@@ -1593,3 +1595,68 @@ def test_validate_all_with_recusive_off(test_case_1_catalog: Catalog) -> None:
     cat = test_case_1_catalog
     assert cat.validate_all() == 8
     assert cat.validate_all(recursive=False) == 0
+
+
+@pytest.fixture
+def nested_catalog() -> pystac.Catalog:
+    """
+    Structure:
+
+    ├── catalog.json
+    ├── products
+    │   ├── catalog.json
+    │   └── product_a
+    │       └── catalog.json
+    └── variables
+        ├── catalog.json
+        └── variable_a
+            ├── catalog.json
+    """
+    root = pystac.Catalog("root", "root")
+    variables = pystac.Catalog("variables", "variables")
+    products = pystac.Catalog("products", "products")
+
+    root.add_child(variables)
+    root.add_child(products)
+
+    variable_a = pystac.Catalog("variable_a", "variable_a")
+    product_a = pystac.Catalog("product_a", "product_a")
+
+    variables.add_child(variable_a)
+    products.add_child(product_a)
+
+    return root
+
+
+def test_set_parent_false_stores_in_proper_place_on_normalize_and_save(
+    nested_catalog: pystac.Catalog, tmp_path: Path
+) -> None:
+    root = nested_catalog
+    product_a = next(root.get_child("products").get_children())  # type: ignore
+    variable_a = next(root.get_child("variables").get_children())  # type: ignore
+
+    variable_a.add_child(product_a, set_parent=False)
+
+    root.normalize_and_save(
+        root_href=str(tmp_path), catalog_type=pystac.CatalogType.ABSOLUTE_PUBLISHED
+    )
+    assert (tmp_path / "products" / "product_a").exists()
+    assert not (tmp_path / "variables" / "variable_a" / "product_a").exists()
+
+
+def test_set_parent_false_stores_in_proper_place_on_save(
+    nested_catalog: pystac.Catalog, tmp_path: Path
+) -> None:
+    nested_catalog.normalize_and_save(
+        root_href=str(tmp_path), catalog_type=pystac.CatalogType.ABSOLUTE_PUBLISHED
+    )
+    root = pystac.Catalog.from_file(str(tmp_path / "catalog.json"))
+    product_a = next(root.get_child("products").get_children())  # type: ignore
+    variable_a = next(root.get_child("variables").get_children())  # type: ignore
+
+    variable_a.add_child(product_a, set_parent=False)
+
+    root.save(pystac.CatalogType.SELF_CONTAINED, dest_href=str(tmp_path))
+
+    assert (tmp_path / "products" / "product_a").exists()
+    assert not (tmp_path / "variables" / "variable_a" / "product_a").exists()