Feature: Add trash support

[Gykes]

This adds the ability for users to define a path for "Trash" If this path is set then Stash will move files to this location on delete rather than completely removing the file from the file system. If no path is set then the usual function of permanently deleting still applies.

Currently, I am unable to test the actual moving of files so I am requesting that someone please pull this down and try the function.

Fixes: #3288
UI:

[Gykes]

@feederbox826 , @DogmaDragon

Would one, or both, of you be willing to pull this down and test? I dont currently have any files on my dev system to move this around with. Sorry if I nag y'all too much for testing but I prefer to test with the regular userbase so it's easier for WP to merge comfortably lol.

[WithoutPants]

I think the delete operation should fail if it fails to move the file to trash. If the user has set the trash directory, they would expect files to be moved there instead of being deleted. Deleting the file when it failed to move goes against this expectation and could potentially result in data loss.

I'm also more inclined to move the file into the trash location during the renameForDelete part in Deleter. This provides an immediate failure point if the file fails to be moved to the new location, and doesn't require any further operations during the Commit call.

[Gykes]

@WithoutPants I anticipated you would want me to remove that so luckily I had most of the work done before I clock out for the night.

I removed the fallback. Now, the file should immediately get moved to Trash via renameForDelete. If it fails the tracked path is restored here trashedPaths["/some/path/here.mp4"]. If it is canceled for any reason, like an invalid location, then rollback kicks in to restore the file.

[feederbox826]

Would you like a machine to test on?

[Gykes]

Would you like a machine to test on?

I have a machine just I moved all of my Dev work to my Mac laptop. Currently I don't have any files on it to test with.

I'll go ahead and set something up on my main server so I can test.

[feederbox826]

I might have... Broken it... A lot 👉👈

After trying to fail the trash, I can't get it to persist the deleteTrashPath, setting it in config.yml removes it it when its' started and the GQL goes through, but the response doesn't contain deleteTrashPath. The UI reflects it but it's not persisted or actually used

[Gykes]

I might have... Broken it... A lot 👉👈

After trying to fail the trash, I can't get it to persist the deleteTrashPath, setting it in config.yml removes it it when its' started and the GQL goes through, but the response doesn't contain deleteTrashPath. The UI reflects it but it's not persisted or actually used

Okay, will look at it tomorrow after work.

Thanks for the test 🤙

[Gykes]

So, I think this is an issue with the Mutation and query configuration resolvers. I forgot to add them there so it will never persists....... dur
Will try and get a new commit up here in the next little while.

[Gykes]

So, these latest updates fix the issues that @feederbox826 was having.

• The trash path now persists.
• The response contains the DeleteTrashPath

Other Tests:

• Tested an "invalid path" and received an error stating that it was an invalid path to prevent user confusion.
• Tested a RO folder and received an error stating that it was a RO folder to prevent user confusion.

I also fixed the UI in the delete dialouge box. If the trash path is enabled then the warning now reflects the files will be moved to trash rather than permanently deleted.

Was able to test this by using /Users/gykes/.Trash as the trash path for mac and it worked accordingly.

I would love if others could test this on other operating systems and give feedback. I tried to break it but I can only do so much with my system.

[feederbox826]

My tests, for posterity

assert fail

• RW lib to RO trash
  • invalid cross-device link
• RO lib to RW trash
  • invalid cross-device link
• RO lib to RO trash
• RW lib to -x trash

assert success

• cross-device move
• subdirectory move
• path > 256ch
• nonexistent trash
  • created automatically
• windows $RECYCLE_BIN
  • access denied
• appends date-time if existing file

[feederbox826]

Partially validated, waiting on docker build to fully validate RO librarary and windows $RECYCLE_BIN

EDIT: having issues with my internal build

[feederbox826]

Fails in every case I've given it where I don't expect it to succeed, validator works great. Only complaint would be that since it uses SafeMove, it fails with error "invalid cross-device link" when instead it should be "access denied" in the first error.

Might be worth having a custom message intead of the generic one

scenesDestroy: input: scenesDestroy marking file "/library/062212-055.mp4" for deletion: failed to move to trash: copying file during SaveMove failed with: 'open /trash/062212-055.mp4: permission denied'; renaming file failed previously with: 'rename /library/062212-055.mp4 /trash/062212-055.mp4: permission denied'

Also $RECYCLE.BIN fails but that's for the best, IIRC it needs custom format

[WithoutPants]

We need to be very careful about this and cover all our bases, because a mistake in this can lead to data loss. Case in point: I just deleted an image using this and it did not use the trash feature 😬

Having had a quick look around, it looks like only the scene mutators are using the trash feature. This needs to be added to the image, gallery and file mutation resolvers, along with the clean task. Basically anywhere the file.NewDeleter function is called.

[WithoutPants]

Only complaint would be that since it uses SafeMove, it fails with error "invalid cross-device link" when instead it should be "access denied" in the first error.

Might be worth having a custom message intead of the generic one

scenesDestroy: input: scenesDestroy marking file "/library/062212-055.mp4" for deletion: failed to move to trash: copying file during SaveMove failed with: 'open /trash/062212-055.mp4: permission denied'; renaming file failed previously with: 'rename /library/062212-055.mp4 /trash/062212-055.mp4: permission denied'

I think that, in fsutil.SafeMove, if the rename operation fails with os.ErrPermission we should probably just fail then and there.

[Gykes]

We need to be very careful about this and cover all our bases, because a mistake in this can lead to data loss. Case in point: I just deleted an image using this and it did not use the trash feature 😬

Having had a quick look around, it looks like only the scene mutators are using the trash feature. This needs to be added to the image, gallery and file mutation resolvers, along with the clean task. Basically anywhere the file.NewDeleter function is called.

Thats actually a very good point and I didn't even think to extend it to images or anything like that.

Good catch, will update tomorrow to include images and will look at groups as well (I think that calls scene mutations but will verify)

[Gykes]

We need to be very careful about this and cover all our bases, because a mistake in this can lead to data loss. Case in point: I just deleted an image using this and it did not use the trash feature 😬

Having had a quick look around, it looks like only the scene mutators are using the trash feature. This needs to be added to the image, gallery and file mutation resolvers, along with the clean task. Basically anywhere the file.NewDeleter function is called.

Added in the support for this. Added for all scene, image,gallery, file, and clean task operations. Currently untested but will test when I get home unless our lord and tester FB wants to do it.