fix/group no error

[d-v-b]

This PR adds tests for the error reported in #3406, and makes a change to the mode handling in zarr.api.asynchronous.group that resolves the error reported there, but causes tests elsewhere in the test suite to fail.

I'm posting this incomplete PR so that anyone who enjoys debugging store permissions problems can pick it up.

[dstansby]

I can take a quick look now - will post back in an hour or so either way if I find any fixes or not 😄

[dstansby]

As a starter for 10, de-duplicating group() and create_group() so one calls the other fixes a bunch of the test failures (and passes the new test):

diff --git a/src/zarr/api/asynchronous.py b/src/zarr/api/asynchronous.py
index 7ff73cc0..0cde9103 100644
--- a/src/zarr/api/asynchronous.py
+++ b/src/zarr/api/asynchronous.py
@@ -673,13 +673,6 @@ async def group(
 
     zarr_format = _handle_zarr_version_or_format(zarr_version=zarr_version, zarr_format=zarr_format)
 
-    mode: AccessModeLiteral
-    if overwrite:
-        mode = "w"
-    else:
-        mode = "w-"
-    store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options)
-
     if chunk_store is not None:
         warnings.warn("chunk_store is not yet implemented", ZarrRuntimeWarning, stacklevel=2)
     if cache_attrs is not None:
@@ -689,20 +682,7 @@ async def group(
     if meta_array is not None:
         warnings.warn("meta_array is not yet implemented", ZarrRuntimeWarning, stacklevel=2)
 
-    if attributes is None:
-        attributes = {}
-
-    try:
-        return await AsyncGroup.open(store=store_path, zarr_format=zarr_format)
-    except (KeyError, FileNotFoundError):
-        _zarr_format = zarr_format or _default_zarr_format()
-        return await AsyncGroup.from_store(
-            store=store_path,
-            zarr_format=_zarr_format,
-            overwrite=overwrite,
-            attributes=attributes,
-        )
-
+    return await create_group(store=store, path=path, overwrite=overwrite, zarr_format=zarr_format, attributes=attributes, storage_options=storage_options)
 
 async def create_group(
     *,
@@ -742,9 +722,7 @@ async def create_group(
     if zarr_format is None:
         zarr_format = _default_zarr_format()
 
-    mode: Literal["a"] = "a"
-
-    store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options)
+    store_path = await make_store_path(store, path=path, mode="a", storage_options=storage_options)
 
     return await AsyncGroup.from_store(
         store=store_path,

(I will PR my findings to your branch when I'm done, but thought I'd post along the way)

[dstansby]

It looks like d-v-b#147 does the trick. I'm not entirely sure why, but it works and deduplicates some code so 🤷

[d-v-b]

i'd like to take this opportunity to flag the fact that, collectively, permissions for stores have consumed a lot of developer energy. I would LOVE to find something more literate than arcane string codes like w- for expressing our intent in this API.

[dstansby]

The change also caught this, which was trying to create a group on top of another group - I changed it to open the existing roup instead.

[dstansby]

Related to this PR, it seems like group() should be deprecated (and then removed) in favour of open_group?

[d-v-b]

Related to this PR, it seems like group() should be deprecated (and then removed) in favour of open_group?

i'm glad you suggested this! it should absolutely be deprecated.

[dstansby]

Dammit I missed some failing tests. I think they can be fixed easily enough by replacing some calls to group() (which promises to create a group) with open_group()

[dstansby]

(sorry I realise my comments are scattergun) If group() is actually meant to try opening a group first (it's not documented like that, but I guess it's existing behaviour we shouldn't break...?), maybe the try/except statement needs putting back, but the new create_group call should go in the except block?

[d-v-b]

we don't need this any more now that #3431 is in