newtype pattern to protect against sloppy path normalization

[d-v-b]

#3922 revealed that we don't normalize the path parameter when creating an instance of FSSpecStore. So Store(path="foo/") and Store(path="foo") end up with different .path attributes. One way to guard against this type of bug would be to introduce a new type that models "strings that have undergone path normalization", define functions that returns instances of this type (the path normalization function) and annotate the path attribute with this type. That should reduce the rate of these "trailing path separator or not" bugs.

[chuckwondo]

@d-v-b, what do you imagine the normalized format to be? With or without trailing path separator? I'd imagine without, but don't want to assume.

This sounds like a breaking change, but perhaps you can elaborate a bit on what you have in mind.

[d-v-b]

see https://github.com/zarrs/zarrs/blob/a9060ba788df916858b87997bbb247fcd65e9e39/zarrs_storage/src/store_key.rs#L53-L63 for an example

[chuckwondo]

Gotcha. Would you want to mimic the Rust approach (i.e., change the path parameter's type from str to StoreKey, or perhaps StorePath), or simply do the normalization internally, before setting the path attribute to the new type?

More specifically, like this?

class FsspecStore(Store):
    def __init__(
        self,
        ...,
        path: StorePath = StorePath("/")  # Although this default would be invalid, no?
        ...,
    ): ...

or like this?

class FsspecStore(Store):
    def __init__(
        self,
        ...,
        path: str = "/",  # Although this default would be invalid, no?
        ...,
    ) -> None:
        ...
        self.path = StorePath(path)
        ...

In both cases, instantiation of a StorePath would raise ValueError if the specified path is invalid, but would "normalize" a valid value.

In the former case, we would introduce a breaking change.

[chuckwondo]

@d-v-b, nudge. I'm happy to pick up this issue. Just let me know your thoughts regarding my previous comment.

[d-v-b]

there's a complication here with the .path attribute of the fsspec store -- that string is interpreted differently by different fsspec backends. so I don't think there's much we can do to shore up the type safety there. But for internal paths (stuff we feed to Store.get, then I think a NewType would be super useful, and I think the risk of changing the type is very low. So +1 from me for that effort.

[chuckwondo]

@d-v-b, from out discussion last week, I believe we agreed that NewType wouldn't buy us anything in this scenario, but I can't recall if we devised an alternative, or decided to put this on the back burner until some other issues were resolved. Do you recall?

[d-v-b]

I think the alternative was to use a newtype internally to brand strings that represent valid node names according to the zarr v3 spec, which we would then use for things like the path attribute of an array or group

[d-v-b]

see these properties:

zarr-python/src/zarr/core/group.py

Lines 833 to 852 in 1cda981

	@property
	def path(self) -> str:
	"""Storage path."""
	return self.store_path.path
	@property
	def name(self) -> str:
	"""Group name following h5py convention."""
	if self.path:
	# follow h5py convention: add leading slash
	name = self.path
	if name[0] != "/":
	name = f"/{name}"
	return name
	return "/"
	@property
	def basename(self) -> str:
	"""Final component of name."""
	return self.name.split("/")[-1]

Although annotated as str, the values cannot be any string. they can only be a string that complies with the rules in the zarr v3 spec, namely the rules for node paths and the rules for node names

we can use the type system to express "string that is a valid zarr path" and also "string that is a valid node name" via NewType:

from typing import NewType

ZarrNodeName = NewType('ZarrNodeName', str)
"""Docstring that explains what this type means"""

ZarrNodePath = NewType('ZarrNodePath', str)
"""Docstring that explains what this type means"""

def normalize_name(name: str) -> ZarrNodeName: ...
def normalize_path(path: str) -> ZarrNodePath: ...

we would then change the signature of Array.path, Group.path, etc to return ZarrNodePath, and we would also use these types internally. As long as we control the source of ZarrNodePath (the string normalization routine), all consumers of ZarrNodePath know that the input has been validated. This allows us to concentrate string validation in exactly one place.

This pattern is used in other language under the name "opaque type" or "branded types". Here's a scala example, and a typescript example. The python example from the standard library is also useful.