Skip to content

Conversation

@guzman-raphael
Copy link
Collaborator

@guzman-raphael guzman-raphael commented Nov 8, 2019

Fix #692

Currently, on fetch the root of the location from s3 storage is stripped but not on insert. This change is to align both so that insert and fetch both strip the root for s3 storage. This makes it compatible with all S3 providers e.g. Minio does not allow objects on root of bucket but AWS hosted objects are allowed.

file storage, however, will still allow insert on root.

@dimitri-yatsenko dimitri-yatsenko merged commit 031f7da into datajoint:master Nov 10, 2019
@ixcat
Copy link

ixcat commented Nov 13, 2019

I didn't catch this at the time -

Wasn't the previous resolution to simply 'not manipulate' location - e.g. if user provides leading slash, use leading slash, if not, dont? This was the previous policy in the resolution to #648

Previous PR against old code structure (#657) was implemented in an 'always-strip' policy, but was closed in favor of the "don't manipulate" policy which came in in #659

I'm OK either way as to the resulting behavior, but this seems like a reversion to one mode of behavior which was previously explicitly overturned ..

@guzman-raphael
Copy link
Collaborator Author

@ixcat The main issue this PR aimed to solve was that although DataJoint 0.12.1 permitted users to store with external location set as /lab, records were unfetchable as the logic did not match. This is b/c inserts would place records in S3 as bucket/schema//lab/object but attempted to fetch as bucket/schema/lab/object.

Therefore, the logic for the insert only was modified to match. Additionally, this serves to solve the concerns in #648 for s3 (preserving compatibility between AWS S3 and Minio) while still letting file stores store as they please on the root. Also, this satisfies Dimitri's concerns from #648 "If double '/' appear anywhere, they should be eliminated".

Now this will require modification of S3 objects if they were already inserted with 0.12.1 under this use case, however, I would argue this was necessary in any case since the underlying bug did not allow these records to be fetched.

@ixcat
Copy link

ixcat commented Nov 13, 2019

Thanks @guzman-raphael - this clears it up / makes sense 👍

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.

DJ filepath inconsistent handling of read/write of location with leading slash on S3 protocol

3 participants