amol- / depot

Toolkit for storing files and attachments in web applications
MIT License
161 stars 41 forks source link

Added support for setting file IDs explicitly. #70

Closed jpmccu closed 1 year ago

jpmccu commented 3 years ago

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 commented 3 years ago

Fixes #69.

amol- commented 3 years ago

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 commented 3 years ago

I'm using a depot as the backup itself. I guess it's intended to be as much import/export as backup and restore, so I wanted to be able to pull the data out from whatever backend is being used without having to deal with the specifics, nor to have to rewrite the database as it goes in or out.

Thanks, Jamie

On Tue, Sep 21, 2021 at 11:22 AM Alessandro Molina @.***> wrote:

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

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/amol-/depot/pull/70#issuecomment-924095098, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAETCEOS67I2LXMUMRCVTQLUDCPMHANCNFSM5BA7SDCA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Jamie McCusker (she/they)

Director, Data Operations Tetherless World Constellation Rensselaer Polytechnic Institute @. @.> http://tw.rpi.edu

gerhardkeuck commented 2 years ago

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 commented 2 years ago

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.

amol- commented 1 year ago

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.

jpmccu commented 1 year ago

Works for me, looking forward to the PR.

amol- commented 1 year ago

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.