Skip to content

fix: include the store location in ObjectRef.full_path - #1648

Open
TheeCoderAhmed wants to merge 1 commit into
datajoint:masterfrom
TheeCoderAhmed:fix/objectref-full-path-location
Open

TheeCoderAhmed wants to merge 1 commit into
datajoint:masterfrom
TheeCoderAhmed:fix/objectref-full-path-location

Conversation

@TheeCoderAhmed

Copy link
Copy Markdown

Fixes #1646.

For s3, gcs and azure stores, ObjectRef.full_path built <protocol>://<bucket>/<path> by hand and left out the store's location. The backend stores objects at <bucket>/<location>/<path> (StorageBackend._full_path), so on a store with a location the returned URI points where the object isn't: ref.fs.exists(ref.full_path) is False even though ref.read() works.

The cloud branches now return self._backend.get_url(self.path). That builds the URL from _full_path, the same path the backend writes to and fsmap already uses. The file branch and the fallback for other protocols are unchanged, so full_path still returns a plain filesystem path for local stores.

Added tests/unit/test_objectref.py. It covers s3 with and without a location, gcs and azure, using StorageBackend specs that don't touch the network. The three cases with a location fail on master (for example s3://bkt/_schema/... instead of s3://bkt/proj/outbox/_schema/...) and pass with the change. tests/unit/test_storage_urls.py and tests/unit/test_storage_adapter.py still pass, and ruff check / ruff format --check are clean on the changed files.

For s3, gcs and azure stores, full_path built <protocol>://<bucket>/<path> and left out the store's location, so it pointed to a path where the object is not when location is set. Use StorageBackend.get_url(), which builds the URL from the same _full_path the backend writes to.

Fixes datajoint#1646

@dimitri-yatsenko dimitri-yatsenko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Correct. StorageBackend.get_url goes through _full_path, which is where objects are written, so fs.exists(ref.full_path) now agrees with the backend.
  2. A test with an s3 endpoint plus location, and a trailing-slash location ("proj/outbox/"), would pin the double-slash case if _full_path doesn't normalize it.
  3. The file branch still builds its path by hand. It could also use get_url/_full_path for consistency, as a follow-up.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ObjectRef.full_path leaves out the store's location for s3, gcs and azure stores

2 participants