immich
immich copied to clipboard
Allow non-read-only external libraries that support asset deletion
Related: https://github.com/immich-app/immich/discussions/5449 https://github.com/immich-app/immich/discussions/8438 Sponsored by: @benmccann
This is an attempt at switching from hardcoded "external means cannot delete" to an explicit isReadOnly flag for libraries. (Not fully polished change yet, hence draft)
The per-asset isExternal and isReadOnly flags being stored in the database is making this difficult… I guess just using them and updating them while the library's read-onliness changes would be significantly easier than removing them and making everywhere query the library flags, but I'm looking for feedback re: the right way to do this.
I think the most natural change of this feature is to let the user know they can mount the library with ro or without that flag for docker mount; then, we can treat external library assets as normal upload assets to avoid introducing an additional boolean flag in the system. What do you think?
Hm. That would mean checking mount info (only on boot? on rescan too?) and updating the flags based on that. Seems nice in terms of not adding any admin UI. I'm slightly worried about potential surprising behavior (e.g. adding a writable mount, possibly for multiple libraries inside of it, and adding libraries in the admin interface, not expecting them to be automatically writable) but on the other hand "it's a purely outside configured sysadmin level decision" does feel nice in a way.
Not sure we'd actually avoid the flag on the library, even if it would get set automatically based on the mount: it would definitely make sense to show in the admin interface whether it is writable, and have that fully consistent with what we've set for the assets…
@etnoy I am pretty sure isReadOnly flag isn't used anymore after we move to your implementation of external library, correct? It was for the previous import mechanism
Hm. That would mean checking mount info (only on boot? on rescan too?) and updating the flags based on that. Seems nice in terms of not adding any admin UI. I'm slightly worried about potential surprising behavior (e.g. adding a writable mount, possibly for multiple libraries inside of it, and adding libraries in the admin interface, not expecting them to be automatically writable) but on the other hand "it's a purely outside configured sysadmin level decision" does feel nice in a way.
Not sure we'd actually avoid the flag on the library, even if it would get set automatically based on the mount: it would definitely make sense to show in the admin interface whether it is writable, and have that fully consistent with what we've set for the assets…
I am thinking more of, if the user know that they use the ro flag in docker mount, the deletion call will error out expectedly, otherwise, the operation will happen as if it the assets are from the upload library. So we don't necessary need to do anything special here in term of checking the state of read/write of the mount
@etnoy I am pretty sure
isReadOnlyflag isn't used anymore after we move to your implementation of external library, correct? It was for the previous import mechanism
That is my understanding, too.
I've argued for removing the field but I think @jrasm91 knew of something that still used it
I would recommend keeping the per asset flags. Otherwise it will probably be a nightmare trying to load in the property everywhere it needs to be used. If a library is marked as read only, it seems easy enough to do a bulk asset update
Hey, thanks for working on this and sorry for the confusion/back and forth around this topic. Internally we have had a lot of discussions about the direction we want to take external libraries. Long story short, we decided to go ahead with the implementation laid out in #9235.