From 2bc671a1531e26fb446a36cc05f7222d05e2c6f6 Mon Sep 17 00:00:00 2001
From: Wei Ji <23487320+weiji14@users.noreply.github.com>
Date: Mon, 27 Mar 2023 16:10:45 +1300
Subject: [PATCH 1/2] Fix opening STAC Assets with xarray:open_kwargs engine
 field

Update the core if-then logic to only set the 'engine' keyword argument when it is not already set in the `open_kwargs` dict. Included a regression unit test that extends the existing simple_zarr test to ensure that the fix works.
---
 tests/conftest.py  |  6 ++++++
 tests/test_core.py |  7 ++++++-
 xpystac/core.py    | 16 ++++++++--------
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/tests/conftest.py b/tests/conftest.py
index 49bd5a8..7e7e4fb 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -49,3 +49,9 @@ def simple_zarr() -> pystac.Asset:
         catalog = pystac_client.Client.open(STAC_URLS["PLANETARY-COMPUTER"])
         collection = catalog.get_collection("daymet-daily-hi")
         return collection.assets["zarr-abfs"]
+
+
+@pytest.fixture(scope="module")
+def complex_zarr(simple_zarr) -> pystac.Asset:
+    simple_zarr.extra_fields["xarray:open_kwargs"]["engine"] = "zarr"
+    return simple_zarr
diff --git a/tests/test_core.py b/tests/test_core.py
index e8c13e8..bb6559a 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -30,4 +30,9 @@ def test_to_xarray_reference_file(simple_reference_file):
 
 def test_to_xarray_zarr(simple_zarr):
     ds = to_xarray(simple_zarr)
-    ds
+    assert ds
+
+
+def test_to_xarray_zarr_with_open_kwargs_engine(complex_zarr):
+    ds = to_xarray(complex_zarr)
+    assert ds
diff --git a/xpystac/core.py b/xpystac/core.py
index f825b44..d389498 100644
--- a/xpystac/core.py
+++ b/xpystac/core.py
@@ -55,14 +55,14 @@ def _(obj: pystac.Asset, **kwargs) -> xarray.Dataset:
         default_kwargs = {"engine": "zarr", "consolidated": False, "chunks": {}}
         return xarray.open_dataset(mapper, **default_kwargs, **open_kwargs, **kwargs)
 
-    if obj.media_type == pystac.MediaType.COG:
-        _import_optional_dependency("rioxarray")
-        default_kwargs = {"engine": "rasterio"}
-    elif obj.media_type == "application/vnd+zarr":
-        _import_optional_dependency("zarr")
-        default_kwargs = {"engine": "zarr"}
-    else:
-        default_kwargs = {}
+    default_kwargs = {}
+    if open_kwargs.get("engine") is None:
+        if obj.media_type == pystac.MediaType.COG:
+            _import_optional_dependency("rioxarray")
+            default_kwargs["engine"] = "rasterio"
+        elif obj.media_type == "application/vnd+zarr":
+            _import_optional_dependency("zarr")
+            default_kwargs["engine"] = "zarr"
 
     ds = xarray.open_dataset(obj.href, **default_kwargs, **open_kwargs, **kwargs)
     return ds

From 791ffbc651086816b860e12e8ab7a3e11d239a68 Mon Sep 17 00:00:00 2001
From: Wei Ji <23487320+weiji14@users.noreply.github.com>
Date: Tue, 28 Mar 2023 08:31:23 +1300
Subject: [PATCH 2/2] Use dictionary unpacking to merge duplicate keys Order of
 priority from lowest to highest is default_kwargs -> open_kwargs -> kwargs.
 See https://peps.python.org/pep-0448.

Co-Authored-By: Julia Signell <jsignell@gmail.com>
---
 xpystac/core.py | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xpystac/core.py b/xpystac/core.py
index d389498..4d5a18c 100644
--- a/xpystac/core.py
+++ b/xpystac/core.py
@@ -55,14 +55,14 @@ def _(obj: pystac.Asset, **kwargs) -> xarray.Dataset:
         default_kwargs = {"engine": "zarr", "consolidated": False, "chunks": {}}
         return xarray.open_dataset(mapper, **default_kwargs, **open_kwargs, **kwargs)
 
-    default_kwargs = {}
-    if open_kwargs.get("engine") is None:
-        if obj.media_type == pystac.MediaType.COG:
-            _import_optional_dependency("rioxarray")
-            default_kwargs["engine"] = "rasterio"
-        elif obj.media_type == "application/vnd+zarr":
-            _import_optional_dependency("zarr")
-            default_kwargs["engine"] = "zarr"
+    if obj.media_type == pystac.MediaType.COG:
+        _import_optional_dependency("rioxarray")
+        default_kwargs = {"engine": "rasterio"}
+    elif obj.media_type == "application/vnd+zarr":
+        _import_optional_dependency("zarr")
+        default_kwargs = {"engine": "zarr"}
+    else:
+        default_kwargs = {}
 
-    ds = xarray.open_dataset(obj.href, **default_kwargs, **open_kwargs, **kwargs)
+    ds = xarray.open_dataset(obj.href, **{**default_kwargs, **open_kwargs, **kwargs})
     return ds