No pystac serialization — avoids all pystac link-reordering and href-transformation side effects

TejasMorbagal · TejasMorbagal · commit 92465569a622 · 2026-03-06T19:19:52.000+01:00
diff --git a/CHANGES.md b/CHANGES.md
@@ -73,8 +73,10 @@
     `zarr-consolidated-metadata` (`.zmetadata`).
   - The S3 STAC catalog and item are written directly to S3 via `fsspec`/`s3fs`
     independently of the GitHub PR.
-  - The OSC STAC Collection gains a `child` link pointing to the S3 catalog root,
-    connecting the two levels of the hierarchy.
+  - The OSC STAC Collection gains a `via` link pointing to the S3 catalog root,
+    connecting the two levels of the hierarchy. (`child` is intentionally avoided
+    because the OSC validator requires every `child` link to resolve to a file inside
+    the metadata repository.)
   - Opt-in via the new `stac_catalog_s3_root` field in `dataset_config.yaml`
     (e.g. `stac_catalog_s3_root: s3://my-bucket/stac/my-collection/`).
   - S3 write credentials are resolved from `S3_USER_STORAGE_KEY`/`S3_USER_STORAGE_SECRET`,
diff --git a/deep_code/tests/tools/test_publish.py b/deep_code/tests/tools/test_publish.py
@@ -190,10 +190,10 @@ def test_get_stac_s3_storage_options_prefers_xcube_env_vars(self):
             opts = self.publisher._get_stac_s3_storage_options()
         self.assertEqual(opts["key"], "xcube-key")
         self.assertEqual(opts["secret"], "xcube-secret")
+        self.assertEqual(opts["s3_additional_kwargs"], {"ACL": ""})
 
     def test_get_stac_s3_storage_options_falls_back_to_aws_env_vars(self):
         env = {"AWS_ACCESS_KEY_ID": "aws-key", "AWS_SECRET_ACCESS_KEY": "aws-secret"}
-        # Ensure xcube vars are absent
         patched_env = {
             k: v
             for k, v in os.environ.items()
@@ -204,8 +204,9 @@ def test_get_stac_s3_storage_options_falls_back_to_aws_env_vars(self):
             opts = self.publisher._get_stac_s3_storage_options()
         self.assertEqual(opts["key"], "aws-key")
         self.assertEqual(opts["secret"], "aws-secret")
+        self.assertEqual(opts["s3_additional_kwargs"], {"ACL": ""})
 
-    def test_get_stac_s3_storage_options_returns_empty_for_boto3_chain(self):
+    def test_get_stac_s3_storage_options_returns_acl_suppression_for_boto3_chain(self):
         no_cred_env = {
             k: v
             for k, v in os.environ.items()
@@ -219,7 +220,7 @@ def test_get_stac_s3_storage_options_returns_empty_for_boto3_chain(self):
         }
         with patch.dict(os.environ, no_cred_env, clear=True):
             opts = self.publisher._get_stac_s3_storage_options()
-        self.assertEqual(opts, {})
+        self.assertEqual(opts, {"s3_additional_kwargs": {"ACL": ""}})
 
     # ------------------------------------------------------------------
     # S3 write helper
diff --git a/deep_code/tests/utils/test_dataset_stac_generator.py b/deep_code/tests/utils/test_dataset_stac_generator.py
@@ -8,7 +8,7 @@
 from unittest.mock import MagicMock, patch
 
 import numpy as np
-from pystac import Catalog, Collection, Item
+from pystac import Catalog, Item
 from xarray import DataArray, Dataset
 
 from deep_code.constants import (
@@ -141,42 +141,115 @@ def test_build_variable_catalog(self, mock_add_themes, mock_add_gcmd):
         # Self href ends with var1/catalog.json
         self.assertTrue(catalog.self_href.endswith("/var1/catalog.json"))
 
-    @patch("pystac.Catalog.from_file")
-    def test_update_product_base_catalog(self, mock_from_file):
-        """Test linking product catalog."""
-        mock_cat = MagicMock(spec=Catalog)
-        mock_from_file.return_value = mock_cat
-
-        result = self.generator.update_product_base_catalog("path.json")
-        self.assertIs(result, mock_cat)
-        mock_cat.add_link.assert_called_once()
-        mock_cat.set_self_href.assert_called_once_with(PRODUCT_BASE_CATALOG_SELF_HREF)
-
-    @patch("pystac.Catalog.from_file")
-    def test_update_variable_base_catalog(self, mock_from_file):
-        """Test linking variable base catalog."""
-        mock_cat = MagicMock(spec=Catalog)
-        mock_from_file.return_value = mock_cat
+    def test_update_product_base_catalog(self):
+        """Child link is appended; existing links (including self) are untouched."""
+        base = {
+            "type": "Catalog",
+            "id": "products",
+            "stac_version": "1.0.0",
+            "description": "Products",
+            "links": [
+                {
+                    "rel": "self",
+                    "href": PRODUCT_BASE_CATALOG_SELF_HREF,
+                    "type": "application/json",
+                }
+            ],
+        }
+        import tempfile, json as _json
+
+        with tempfile.NamedTemporaryFile(
+            mode="w", suffix=".json", delete=False
+        ) as tmp:
+            _json.dump(base, tmp)
+            tmp_path = tmp.name
+
+        result = self.generator.update_product_base_catalog(tmp_path)
+
+        import os
+
+        os.unlink(tmp_path)
+
+        self.assertIsInstance(result, dict)
+        rels = [lnk["rel"] for lnk in result["links"]]
+        # self link must still be present and still first
+        self.assertEqual(result["links"][0]["rel"], "self")
+        self.assertEqual(result["links"][0]["href"], PRODUCT_BASE_CATALOG_SELF_HREF)
+        self.assertIn("child", rels)
+        child = next(lnk for lnk in result["links"] if lnk["rel"] == "child")
+        self.assertIn("mock-collection-id", child["href"])
+
+    def test_update_variable_base_catalog(self):
+        """Child links for each variable are appended."""
+        base = {
+            "type": "Catalog",
+            "id": "variables",
+            "stac_version": "1.0.0",
+            "description": "Variables",
+            "links": [
+                {
+                    "rel": "self",
+                    "href": VARIABLE_BASE_CATALOG_SELF_HREF,
+                    "type": "application/json",
+                }
+            ],
+        }
+        import tempfile, json as _json, os
+
+        with tempfile.NamedTemporaryFile(
+            mode="w", suffix=".json", delete=False
+        ) as tmp:
+            _json.dump(base, tmp)
+            tmp_path = tmp.name
 
         vars_ = ["v1", "v2"]
-        result = self.generator.update_variable_base_catalog("vars.json", vars_)
-        self.assertIs(result, mock_cat)
-        # Expect one add_link per variable
-        self.assertEqual(mock_cat.add_link.call_count, len(vars_))
-        mock_cat.set_self_href.assert_called_once_with(VARIABLE_BASE_CATALOG_SELF_HREF)
-
-    @patch("pystac.Collection.from_file")
-    def test_update_deepesdl_collection(self, mock_from_file):
-        """Test updating DeepESDL collection."""
-        mock_coll = MagicMock(spec=Collection)
-        mock_from_file.return_value = mock_coll
-
-        result = self.generator.update_deepesdl_collection("deep.json")
-        self.assertIs(result, mock_coll)
-        # Expect child and theme related links for each theme
-        calls = mock_coll.add_link.call_count
-        self.assertGreaterEqual(calls, 1 + len(self.generator.osc_themes))
-        mock_coll.set_self_href.assert_called_once_with(DEEPESDL_COLLECTION_SELF_HREF)
+        result = self.generator.update_variable_base_catalog(tmp_path, vars_)
+        os.unlink(tmp_path)
+
+        self.assertIsInstance(result, dict)
+        child_hrefs = [
+            lnk["href"] for lnk in result["links"] if lnk["rel"] == "child"
+        ]
+        self.assertEqual(len(child_hrefs), len(vars_))
+        # self link must remain in place
+        self.assertEqual(result["links"][0]["rel"], "self")
+
+    def test_update_deepesdl_collection(self):
+        """Child and theme-related links are appended; existing links kept."""
+        base = {
+            "type": "Collection",
+            "id": "deep-esdl",
+            "stac_version": "1.0.0",
+            "description": "DeepESDL",
+            "extent": {},
+            "links": [
+                {
+                    "rel": "self",
+                    "href": DEEPESDL_COLLECTION_SELF_HREF,
+                    "type": "application/json",
+                }
+            ],
+        }
+        import tempfile, json as _json, os
+
+        with tempfile.NamedTemporaryFile(
+            mode="w", suffix=".json", delete=False
+        ) as tmp:
+            _json.dump(base, tmp)
+            tmp_path = tmp.name
+
+        result = self.generator.update_deepesdl_collection(tmp_path)
+        os.unlink(tmp_path)
+
+        self.assertIsInstance(result, dict)
+        rels = [lnk["rel"] for lnk in result["links"]]
+        # child link added
+        self.assertIn("child", rels)
+        # one related link per theme
+        related = [lnk for lnk in result["links"] if lnk["rel"] == "related"]
+        self.assertGreaterEqual(len(related), len(self.generator.osc_themes))
+        # self link still present
+        self.assertEqual(result["links"][0]["rel"], "self")
 
     # ------------------------------------------------------------------
     # Zarr STAC Item / Catalog generation
@@ -280,31 +353,39 @@ def test_build_zarr_stac_catalog_file_dict_content(self):
         self.assertIn("zarr-data", item_dict["assets"])
         self.assertIn("zarr-consolidated-metadata", item_dict["assets"])
 
-    def test_build_dataset_stac_collection_adds_s3_catalog_child_link(self):
-        """Child link to S3 catalog is added when stac_catalog_s3_root is provided."""
+    def test_build_dataset_stac_collection_adds_s3_catalog_via_link(self):
+        """A 'via' link to the S3 catalog is added when stac_catalog_s3_root is provided.
+
+        rel='via' is used (not 'child') because the OSC validator requires every
+        'child' link to resolve to a file inside the metadata repository.
+        """
         s3_root = "s3://test-bucket/stac/my-collection/"
         collection = self.generator.build_dataset_stac_collection(
             mode="dataset", stac_catalog_s3_root=s3_root
         )
-        child_links = [lnk for lnk in collection.links if lnk.rel == "child"]
-        s3_child = next(
-            (lnk for lnk in child_links if "s3://" in str(lnk.target)), None
+        s3_via = next(
+            (
+                lnk
+                for lnk in collection.links
+                if lnk.rel == "via" and "catalog.json" in str(lnk.target)
+            ),
+            None,
         )
-        self.assertIsNotNone(s3_child, "Expected a child link pointing to S3 catalog")
+        self.assertIsNotNone(s3_via, "Expected a 'via' link pointing to S3 catalog")
         self.assertEqual(
-            s3_child.target,
+            s3_via.target,
             "s3://test-bucket/stac/my-collection/catalog.json",
         )
 
-    def test_build_dataset_stac_collection_no_s3_child_link_by_default(self):
-        """No S3 child link is added when stac_catalog_s3_root is absent."""
+    def test_build_dataset_stac_collection_no_s3_via_link_by_default(self):
+        """No S3 catalog 'via' link is added when stac_catalog_s3_root is absent."""
         collection = self.generator.build_dataset_stac_collection(mode="dataset")
-        s3_child_links = [
+        s3_catalog_links = [
             lnk
             for lnk in collection.links
-            if lnk.rel == "child" and "s3://" in str(getattr(lnk, "target", ""))
+            if lnk.rel == "via" and "catalog.json" in str(getattr(lnk, "target", ""))
         ]
-        self.assertEqual(len(s3_child_links), 0)
+        self.assertEqual(len(s3_catalog_links), 0)
 
 
 class TestFormatString(unittest.TestCase):
diff --git a/deep_code/tools/publish.py b/deep_code/tools/publish.py
@@ -215,8 +215,7 @@ def _update_and_add_to_file_dict(
         full_path = (
             Path(self.gh_publisher.github_automation.local_clone_dir) / catalog_path
         )
-        updated_catalog = update_method(full_path, *args)
-        file_dict[full_path] = updated_catalog.to_dict()
+        file_dict[full_path] = update_method(full_path, *args)
 
     def _update_variable_catalogs(self, generator, file_dict, variable_ids):
         """Update or create variable catalogs and add them to file_dict.
@@ -243,10 +242,9 @@ def _update_variable_catalogs(self, generator, file_dict, variable_ids):
                     Path(self.gh_publisher.github_automation.local_clone_dir)
                     / var_file_path
                 )
-                updated_catalog = generator.update_existing_variable_catalog(
+                file_dict[var_file_path] = generator.update_existing_variable_catalog(
                     full_path, var_id
                 )
-                file_dict[var_file_path] = updated_catalog.to_dict()
 
     def publish_dataset(
         self,
@@ -545,10 +543,14 @@ def _get_stac_s3_storage_options(self) -> dict:
         secret = os.environ.get("S3_USER_STORAGE_SECRET") or os.environ.get(
             "AWS_SECRET_ACCESS_KEY"
         )
+        # s3_additional_kwargs={"ACL": ""} suppresses the ACL header that s3fs
+        # adds by default; required when Object Ownership is set to
+        # BucketOwnerEnforced (ACLs disabled) to avoid AccessDenied errors.
+        base = {"s3_additional_kwargs": {"ACL": ""}}
         if key and secret:
-            return {"key": key, "secret": secret}
+            return {"key": key, "secret": secret, **base}
         # Fall through to boto3 chain (IAM role / ~/.aws/credentials)
-        return {}
+        return base
 
     def _write_stac_catalog_to_s3(
         self, file_dict: dict[str, dict], storage_options: dict
diff --git a/deep_code/utils/dataset_stac_generator.py b/deep_code/utils/dataset_stac_generator.py
diff --git a/deep_code/version.py b/deep_code/version.py
