[Tomaz-Vieira]

Summary

Parsing relative file URLs like file:://something.zarr would ultimately deduce a path of "", even causing the weirdest way I've ever deleted my own project directory:

import zarr

g = zarr.open_group("file://bla.zarr") # notice relative path
assert str(g.store_path) == "file://." # bla.zarr disappeared
g.store.close()

g2 = zarr.open_group(str(g.store_path), mode="w") # deletes current directory then raises

For reviewers

The behavior from this PR is definitely more correct than before, and it prevents me from nuking my current directory. I guess one could argue that relative file:// URLs should not be allowed, as I had to ignore the parsing from urlparse, but if that is the case, then at least I don't think StorePath.__str__ should be losing the "hostname" from the URL.

Author attestation

• I am a human, these are my changes, and I have reviewed and understood every change and can explain why each is correct.

TODO

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/user-guide/*.md
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes) (+0.04%)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.71%. Comparing base (ec2c359) to head (4092d9b).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4533   +/-   ##
=======================================
  Coverage   94.71%   94.71%           
=======================================
  Files          94       94           
  Lines       13663    13664    +1     
=======================================
+ Hits        12941    12942    +1     
  Misses        722      722           

Files with missing lines	Coverage Δ
src/zarr/storage/_utils.py	96.47% <100.00%> (+0.04%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[d-v-b]

you probably didnt lint in the same environment ci runs in, so those type ignores have to come back :/

[d-v-b]

dont do string manipulation. we just parsed the url. used the value we got from parsing it

[Tomaz-Vieira]

I don't like it either, but re-joining pieces of the parsed URL is destructive:

>>> urlparse("file://a/b")
ParseResult(scheme='file', netloc='a', path='/b', params='', query='', fragment='')
>>> urlparse("file://a/b?")
ParseResult(scheme='file', netloc='a', path='/b', params='', query='', fragment='')

[d-v-b]

fwiw "file://a/b" is not a valid file uri. maybe thats part of the issue here

[d-v-b]

unless your hostname is "a"

[d-v-b]

i did a bit of reading and it seems like fsspec was naughty and treats file://a/b as a relative path starting with a, so we are probably on the hook for continuing to support that, even though its clearly wrong.

try this:

parsed = urlparse(url)

if parsed.scheme == "file":
    # RFC 8089: the host is "localhost" or empty. Anything else is a user
    # writing a relative path, so fold it back into the path.
    host = "" if parsed.netloc == "localhost" else parsed.netloc
    path = host + parsed.path
    if path == "":
        raise ValueError(f"The URL {url!r} has no path. ...")
    name = None
elif parsed.scheme:
    name = parsed.netloc or None
    path = parsed.path.lstrip("/")
else:
    name = None
    path = parsed.path

then, in LocalStore, make the root absolute. We need to check if anyone is affected downstream of that change.