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

Added support for setting file IDs explicitly. #70

Closed
wants to merge 2 commits into from

Conversation

jpmccu
Copy link

@jpmccu jpmccu commented Jul 26, 2021

Use with care, especially with MongoDB. This is probably going to be a controversial PR, but I wanted to put it out there. It should be 100% backwards compatible.

@jpmccu
Copy link
Author

jpmccu commented Jul 29, 2021

Fixes #69.

@amol-
Copy link
Owner

amol- commented Sep 21, 2021

Most of depot design is centered around the concept that users should never care about the IDs. They are an opaque autogenerated thing. You should usually access the files through their SQLAlchemy or Ming interface, so the ID is only helpful for DEPOT itself to retrieve the right file when the column of models is accessed. If possible I'm for keeping ids out of the hands of users.

If the goal is to restore a backup, then the backup should restore the same exact IDs and thus I don't get the need for passing the Ids to the file saving function. Are you trying to make&restore the backup through depot itself? Because that wasn't what depot was built for and it takes for granted that your backup tools depend on the storage backend

@jpmccu
Copy link
Author

jpmccu commented Sep 21, 2021 via email

self.fs.delete(file_id)
assert not self.fs.exists(file_id)

self.fs.create(FILE_CONTENT, fileid=file_id)
Copy link
Owner

Choose a reason for hiding this comment

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

I think you want a assert self.fs.exists(file_id) affert this line. Otherwise we wouldn't be testing much.

Copy link
Author

Choose a reason for hiding this comment

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

Added.

@gerhardkeuck
Copy link

Will setting a custom ID not conflict with the logic in _check_file_id?

I came onto this MR as I want to set a path prefix for uploaded files.

@jpmccu
Copy link
Author

jpmccu commented May 17, 2022

Will setting a custom ID not conflict with the logic in _check_file_id?

No, since everything uses UUIDs and validates them as such. If you're inserting a UUID, you'll be fine.

@jpmccu jpmccu requested a review from amol- May 17, 2022 21:38
@amol-
Copy link
Owner

amol- commented May 3, 2023

I'm considering a different approach to this one. Instead of allowing the users to pass ID explicitly, which is something I want to avoid as the ID should be totally opaque and an implementation detail.

The idea is that FileStorage.replace can be used for the purpose of copying a file from one storage to the other preserving the same ID. I'll have to write tests to confirm it behaves as expected, but that would allow backups to happen.

@amol- amol- closed this May 3, 2023
@jpmccu
Copy link
Author

jpmccu commented May 4, 2023

Works for me, looking forward to the PR.

@amol-
Copy link
Owner

amol- commented Jun 8, 2023

Works for me, looking forward to the PR.

@jpmccu See https://github.com/amol-/depot/blob/master/docs/userguide.rst#performing-backups-between-storages for docs, the PR is merged.

@jpmccu jpmccu deleted the import_export branch June 8, 2023 16:49
@jpmccu jpmccu restored the import_export branch June 13, 2023 12:21
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.

None yet

3 participants