fix restore behavior of the trashbin API

[C0rby]

• The precondition were not checked before doing a trashbin restore in the ownCloud dav API. Without the checks the API would behave differently compared to the oC10 API.
• The restore response was missing HTTP headers like ETag

[C0rby]

The CI had some issues but I can't restart the build.

[C0rby]

@phil-davis, I think the tests in core for this are currently wrong IMO.
https://github.com/owncloud/core/blob/510bc012ae25da69730f2de6ed94ad725a6be49a/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L96-L119

In particular https://github.com/owncloud/core/blob/510bc012ae25da69730f2de6ed94ad725a6be49a/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L107.

Both backends, oC10 and with this PR oCIS too, delete the conflicting file, which then lands in the trashbin. I.e. effectively the two files change places and in the end there should still be a file with the upload-path in the trashbin.

What do you think?

[phil-davis]

What do you think?

When this scenario was being run on oC10 it had 2 different behaviors and I could not sort out a test that would give a consistent result. Read issue owncloud/core#35974 - sometimes the restore from trashbin overwrites the existing file, and the existing file is found in the trashbin. That seems a reasonable behavior (but one could argue that the existing file should end up in the version history).

But sometimes the existing file is not overwritten. The trashbin restore API request returns a "good" HTTP status, but actually the original file is still there, and actually the API request "did nothing".

I will try enabling this again on oC10 and see if the variable results still happen. Then we need to decide what the system is supposed to do in this case.

[phil-davis]

core PR owncloud/core#38869 has cleaned-up the scenario.
We will get #1801 to bump the core commit id so that the testing is using that adjusted scenario. Then it should be easier to get that scenario passing.

[phil-davis]

@C0rby #1801 has been merged - now you get conflicts for free :)

[C0rby]

Oh nice. I'll resolve that tomorrow. 👍

[phil-davis]

This LGTM. Well done getting a number of scenarios out of expected-failures.

[phil-davis]

I guess this will handle the case where the restore wants to overwrite an existing resource, but the user does not have delete access to the resource.

And it might be more tricky if the user has change access to the resource, but not delete access. client.Delete will probably refuse, but actually the end-effect that we want is to just overwrite. IMO this is also a hassle in oC10 - the back-end code has a similar flow where it deletes the resource first, then adds the restored resource. That kind of edge-case is something for a future PR...

[refs]

So, essentially restoring from the trash bin branches if:

1. a resource in the restore destination exists and overwrite=T
2. a resource in the restore destination exists and overwrite=F
3. the restore destination is available

case 1 will delete the reference on dst and call RestoreRecycleItem
case 2 will fail because the precondition does not pass: destination exists and overwrite is false (precondition = destination location is free)
case 3 will succeed as it is the opposite to case 2

looks good to me!

[labkode]

Just to add more into the fire, what about this scenario?

I deleted /a/b/c/e.txt
I removed /a

When restoring e.txt as a user I'd expect the hierarchy recreated and maybe asked about the possibility to choose where the restore will happen? original location and hierarchy recreated or another place?

[phil-davis]

https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature currently has scenarios that use the API to restore e.txt to a place specified in the API call (the caller has to say where to restore it, and IMO the target folder has to already exist)

I don't see scenarios for when the restore does not specify the target, and the original folder has been deleted. I should add scenarios for that, so that we at least know how oC10 behaves and can decide what to do! Issue owncloud/core#38882 - more test scenarios will come...

But this PR is definitely a great step forward - lots of existing scenarios pass.