Restore file from trashbin to a different place overwriting a file

[phil-davis]

Steps to reproduce

  Scenario Outline: restoring a file to an already existing path overrides the file
    Given using <dav-path> DAV path
    And user "user0" has been created with default attributes and skeleton files
    And user "user0" has uploaded file with content "file to delete" to "/textfile0.txt"
    And user "user0" has uploaded file with content "PARENT textfile0 content" to "/PARENT/textfile0.txt"
    And user "user0" has deleted file "/textfile0.txt"
    When user "user0" restores the file with original path "/textfile0.txt" to "/PARENT/textfile0.txt" using the trashbin API
    Then the HTTP status code should be "204"
    # Sometimes "/PARENT/textfile0.txt" is found in the trashbin. Should it? Or not?
    # That seems to be what happens when the restore-overwrite happens properly,
    # The original /PARENT/textfile0.txt seems to be "deleted" and so goes to the trashbin
    #And as "user0" the file with original path "/PARENT/textfile0.txt" should not exist in trash
    And as "user0" file "/PARENT/textfile0.txt" should exist
    # sometimes the restore from trashbin does overwrite the existing file, but sometimes it does not. That is also surprising.
    # the current observed behavior is that if the original /PARENT/textfile0.txt ended up in the trashbin,
    # then the new /PARENT/textfile0.txt has the "file to delete" content.
    # otherwise /PARENT/textfile0.txt has its old content
    And the content of file "/PARENT/textfile0.txt" for user "user0" if the file is also in the trashbin should be "file to delete" otherwise "PARENT textfile0 content"
    #And the content of file "/PARENT/textfile0.txt" for user "user0" should be "file to delete"
    Examples:
      | dav-path |
      | old      |
      | new      |

Expected behaviour

Maybe:

1. the restored file should just overwrite the existing file, and the existing file is gone (or is saved as an old version if files_versions is enabled), or;
2. The restored file has (2) on the end of its name, and the existing file stays, or;
3. ???

Actual behaviour

Sometimes the restore from trashbin overwrites the existing file. When that happens, the existing file is found in the trashbin. I guess that the code-path deletes the existing file (triggering an event that causes it to be saved into the trashbin). And the restored file has the correctly-restored content.

But also sometimes the existing file is not found in the trashbin, and has not been overwritten (it still has the old content). But the API call to restore from trashbin still returns 204 success.

Server configuration

Local runs of the above scenario in a development environment on top of current master
And in drone CI it happens also.

[phil-davis]

One guess is that there is some timing of events/callbacks happening when the server is processing a restore from trashbin request, and that restore involves deleting (or overwriting) the target file, and so save-to-trashbin callbacks also get triggered...

@micbar @DeepDiver1975 I tried to make sure that this is not an artifact of the test runner.

The Webdav Trashbin API will end up in 10.3, so maybe this issue should be looked into, as there might be some underlying behaviour found that can be adjusted/fixed.

[phil-davis]

@micbar I added this to the current sprint backlog. Please decide if it is something that should be looked into for 10.3

[VicDeo]

As far as I remember, the restore from trash was designed to work exactly like this.
In case files_versions is disabled the user will loose existing file with the same name.

To change the behavior here one just needs to delete the code that keeps user data safe.

[PVince81]

is this using the new API or old API ?

did OC behave the same using the old API / old OC version ?

[VicDeo]

it was already proposed as an enhancement a while ago and declined.
See #3131 (comment)

[PVince81]

Ok, so something to clarify on the requirements side @pmaier1

We could decide to keep the behavior as is for now and document it.
There's also the question how often it happens that an admin disabled files_versions.

[phil-davis]

Note: from run to run of the above scenario I get inconsistent results. I made the test scenario in PR #35948 be "flexible" to allow the 2 observed behaviours. Whatever the desired behaviour, at least it should be consistent. If someone can find how this scenario can always return HTTP 204 but sometimes the file content is restored, and sometimes not, then that would be very helpful.

(events for trashbin and/or versions going off in not-predictable orders?)

[PVince81]

when I hear "sometimes" I usually hear "race conditions".
but don't see how those could happen in a linear test scenario

[phil-davis]

when I hear "sometimes" I usually hear "race conditions".
but don't see how those could happen in a linear test scenario

same here - but I get close to 50% split of each result???

[PVince81]

@phil-davis this reminds me now about another time related component: the file name used when trashing a file contains the current timestamp. If tests run faster there could be timestamp overwrites.

[phil-davis]

I assigned myself to investigate test timing.

[phil-davis]

As far as I can tell, there is not a test timing issue.

https://drone.owncloud.com/owncloud/core/30747/4/13
I put extra sleep(5) before the checks in Then steps and still the same result - about half the time the updated content gets restored, and half the time not. So it does not seem to be a test timing issue.

See #38867 (comment)

[phil-davis]

PR #38869 cleans up the "good" scenario so that it only checks for the really-required behavior. That will help for testing the OCIS implementation. And it still gets 50% failures in oC10 core, so it can be used as a test for fixing the problem in oC10.

[phil-davis]

PR #38867 demonstrates that the issue is still happening.