Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potentially unexpected behavior on FsspecStore list method #2777

Closed
moradology opened this issue Jan 29, 2025 · 2 comments · Fixed by #2778
Closed

Potentially unexpected behavior on FsspecStore list method #2777

moradology opened this issue Jan 29, 2025 · 2 comments · Fixed by #2778
Labels
bug Potential issues with the zarr-python library

Comments

@moradology
Copy link
Contributor

Zarr version

3.0.1

Numcodecs version

0.15.0

Python Version

3.12

Operating System

Mac

Installation

pip

Description

Looks as though this line will have some unexpected results (wiping out all separators) if the empty string is passed as the path during store initialization. The empty string will otherwise work as a way of specifying the current working directory, so this may be undesirable. A couple of resolutions make sense here:

  1. guard against empty string path arguments in init
  2. modify this line to avoid wholesale replace of self.path + "/" with "":
    for onefile in (a.replace(self.path + "/", "") for a in allfiles):

Steps to reproduce

Simplest path to reproduction is just to simulate values. These come from a grib in the kerchunk tests:

allfiles = ['.zattrs', '.zgroup', 'refc/.zattrs', 'refc/.zgroup', 'refc/instant.zattrs', 'refc/instant.zgroup', 'refc/instant/atmosphere/.zattrs', 'refc/instant/atmosphere/.zgroup', 'refc/instant/atmosphere/atmosphere/.zarray']
path = ""

# following the logic here https://github.com/zarr-developers/zarr-python/blob/e602aa1d19f26bb06669994231e524c55bcecbeb/src/zarr/storage/_fsspec.py#L344:
for onefile in (a.replace(path + "/", "") for a in allfiles):
    print(onefile)

Additional output

No response

@moradology moradology added the bug Potential issues with the zarr-python library label Jan 29, 2025
@d-v-b
Copy link
Contributor

d-v-b commented Jan 29, 2025

yikes. given the number of bugs in stores relating to improper string -> path conversion, I wonder if we need something a bit more robust to guard against this kind of thing.

for example, in this case I think a.replace(path + "/", "") is an attempt to make a relative to path. Besides the obvious solution of using removeprefix instead of replace, we could have a stand-alone make_relative_path(a,b) function that's well tested.

@moradology moradology changed the title Potentially unexpected behavior on fsspecstore list method Potentially unexpected behavior on FsspecStore list method Jan 30, 2025
@moradology
Copy link
Contributor Author

I'll throw together the simpler of the two solutions; if there's an appetite for it, I'll also (or in lieu of this PR) cut a new issue for a reusable make_relative_path function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Potential issues with the zarr-python library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants