Fix fileid loss on MOVE #27508

SergioBertolinSG · 2017-03-27T10:30:26Z

This PR will require to remove getFileIdForPath function in Comments after #27446 is merged. Or remove it from there after this PR is merged.

mention-bot · 2017-03-27T10:30:27Z

@SergioBertolinSG, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @davitol and @DeepDiver1975 to be potential reviewers.

PVince81 · 2017-03-27T16:48:30Z

This test doesn't match the case from #26992 but seems to have found yet another bug related to MOVE on the new endpoint.

I'll extend this PR to also test #26992

PVince81 · 2017-03-27T16:52:05Z

The first test is overkill. No need to upload.

Just use the new DAV endpoint and move an existing file into a subdir, its fileid changes...

PVince81 · 2017-03-27T16:58:18Z

Uh oh, it looks like the new Sabre requires destination nodes to implement IMoveTarget: https://github.com/fruux/sabre-dav/blob/3.2.2/lib/DAV/Tree.php#L155

Since we didn't implement this, it falls back to do a copy + delete. A copy will create a new file...

Now let's fine out how old this issue is.

PVince81 · 2017-03-27T17:01:21Z

The IMoveTarget interface already existed since a while, even in stable9.

The reason we didn't have this issue is because on the old endpoint we overrode the move method in our own objecttree.

So the fix here is to either override the ObjectTree for the new DAV endpoint again or implement IMoveTarget. Quickfix is the ObjectTree approach with a proved complex move algorithm.
If we decided to go the IMoveTarget way we need to reimplement that complex algorithm from a different perspective.

@DeepDiver1975 thoughts ?

PVince81 · 2017-03-27T17:06:00Z

Seems we can't easily reuse our ObjectTree. In the past the root was simply the files.

But now the files stuff is embedded inside a sub-sub-collection.

SergioBertolinSG · 2017-03-27T17:22:09Z

👍 to the tests changes, I misunderstood the issue.

PVince81 · 2017-03-29T05:25:29Z

@DeepDiver1975 are you investigating this ?

PVince81 · 2017-03-29T09:07:01Z

I've ported (read: copy-pasted) the code from ObjectTree::move into the Sabre Directory node which now implements IMoveTarget: 51b1b1c

This is still experimental. Apparently tests pass and fileid is kept.
Needs more extensive testing.

PVince81 · 2017-03-29T09:14:36Z

To avoid duplicate code, I suggest to remove ObjectTree::move() completely as this will then trigger the fallback logic that calls Directory::moveInto(). So the logic for old and new endpoints would be exactly the same code.
what about "copy" permissions ?

PVince81 · 2017-03-29T09:50:47Z

Ok, so my last change makes tests fail. Maybe tests need adjusting since the logic is happening somewhere else.

I'll move the tests then.

PVince81 · 2017-03-29T10:16:54Z

Fixed unit tests by moving them from ObjectTreeTest to DirectoryTest and adjusting.

SergioBertolinSG · 2017-03-29T10:19:58Z

@PVince81 About my last added tests,

These were failing before your commits:

Scenario: Renaming a folder including a backslash encoded should return an error using old endpoint
Scenario: Renaming a folder beginning with a backslash encoded should return an error using new endpoint
Scenario: Renaming a folder including a backslash encoded should return an error using new endpoint

PVince81 · 2017-03-29T10:20:56Z

rerun perms smashbox tests against new endpoint

PVince81 · 2017-03-29T10:58:02Z

smashbox tests for test_sharePermissions ran fine on master and on this branch, with owncloud/pyocclient#191.

I suspect that there are permission checks further down the road, so if they're not done on DAV level, they'll be done on View level. The only difference might be the returned exceptions.

PVince81 · 2017-03-29T13:42:10Z

14:37:59     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA@2/tests/integration/features/checksums.feature:143
14:37:59     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA@2/tests/integration/features/webdav-related.feature:374
14:37:59     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA@2/tests/integration/features/webdav-related.feature:386
14:37:59     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA@2/tests/integration/features/webdav-related.feature:398
14:37:59     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA@2/tests/integration/features/webdav-related.feature:478
14:37:59     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA@2/tests/integration/features/webdav-related.feature:504
14:37:59     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA@2/tests/integration/features/webdav-related.feature:518
14:37:59     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA@2/tests/integration/features/webdav-related.feature:525

PVince81 · 2017-03-29T15:05:31Z

Fixed to accept FutureFile (IFile) to be moved into a Directory instance.
FutureFile isn't an instance of ownCloud's Node class but still a valid file that can be streamed with copy+delete.

This should fix the webdav integration tests at least.

PVince81 · 2017-03-29T19:53:41Z

Less failures now.

19:30:49 --- Failed scenarios:
19:30:49 
19:30:49     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA/tests/integration/features/webdav-related.feature:478
19:30:49     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA/tests/integration/features/webdav-related.feature:504
19:30:49     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA/tests/integration/features/webdav-related.feature:518
19:30:49     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA/tests/integration/features/webdav-related.feature:525

PVince81 · 2017-03-30T08:19:51Z

New problem: now the MOVE from Sabre that moves the ".file" to the final file is triggering the default Sabre logic which first deletes the target file when it exists: https://github.com/fruux/sabre-dav/blob/3.2.2/lib/DAV/CorePlugin.php#L637

On the old endpoint this did not happen for chunk upload because we never actually did a Webdav MOVE but always used PUT. And the last PUT was internally doing the assembly.

Need to think of a better solution than having to rewrite/override httpMove

PVince81 · 2017-03-30T08:21:47Z

ref https://github.com/fruux/sabre-dav/issues/948

PVince81 · 2017-03-30T09:33:00Z

Added Sabre dodging for FutureFile to prevent deleting the target node.
Fixed integration test about backslashes to really be about backslashes.

I expect all tests to pass now.

PVince81 · 2017-03-30T11:58:46Z

One last bit hopefully:

13:53:04 --- Failed scenarios:
13:53:04 
13:53:04     /var/lib/jenkins/workspace/owncloud-core_core_PR-27508-MESR5CP2N2QXQOWAZONE6PLG33IGBKP2PGDCIAYEVIKNFF7CK7LA/tests/integration/features/webdav-related.feature:525

This removes the duplicated code

Sabre usually deletes the target node on MOVE before proceeding with the actual move operation. This fix prevents this to happen in case the source node is a FutureFile.

PVince81 · 2017-03-30T12:01:02Z

Ok, apparently I missed fixing a test.

NOW all tests shall pass

PVince81 · 2017-03-30T12:01:33Z

@jvillafanez @DeepDiver1975 @butonic @guruz please review (yes @guruz the former Sabre master 😉)

jvillafanez · 2017-04-06T12:39:27Z

apps/dav/lib/Connector/Sabre/Directory.php

+	public function moveInto($targetName, $fullSourcePath, INode $sourceNode) {
+		if (!$sourceNode instanceof Node) {
+			// it's a file of another kind, like FutureFile
+			if ($sourceNode instanceof IFile) {


Are directories included?

nah, forget it. They should go through this function.

jvillafanez · 2017-04-06T12:48:29Z

apps/dav/lib/Connector/Sabre/FilesPlugin.php

+		}
+
+		// do a move manually, skipping Sabre's default "delete" for existing nodes
+        $this->tree->move($path, $destination);


indentation

who stole my tab?

probably copied this from the Sabre code which uses spaces and not tabs :-S

jvillafanez · 2017-04-06T13:11:38Z

Some questions:

Sabre/Directory uses a moveInto function. Is there a move function (maybe inherited) that could be being used? How the directories will be moved?
I think there is a Sabe/File file... Is move a possible action there (I guess files can be moved)? What method would be called?
There is $this->tree->move($path, $destination); call in the beforeMoveFutureFile... if the Sabre/ObjectTree move method has been removed, how it will be handled?

I'm not used to this part of the code, so I'm a bit lost here.

PVince81 · 2017-04-06T13:17:30Z

Thanks for the review.

Sabre/Directory uses a moveInto function. Is there a move function (maybe inherited) that could be being used? How the directories will be moved?

Directories are moved also through this function.
Normally if you don't implement moveInto from the IMoveTarget interface, it's the Sabre Tree that does a move by actually doing a copy+delete: https://github.com/fruux/sabre-dav/blob/master/lib/DAV/Tree.php#L158 (see a few lines down).

I think there is a Sabe/File file... Is move a possible action there (I guess files can be moved)? What method would be called?

There is no move function on Sabre nodes at all, neither files (IFile) nor directories (ICollection). This is all handled by the Tree Sabre class. In the old endpoint we override Tree::move but the new endpoint cannot use overriding any more because files are only a subtree of the whole DAV tree. For the old endpoint the file tree was the full tree.

There is $this->tree->move($path, $destination); call in the beforeMoveFutureFile... if the Sabre/ObjectTree move method has been removed, how it will be handled?

The base class takes care of it with a copy+delete: https://github.com/fruux/sabre-dav/blob/master/lib/DAV/Tree.php#L158

For the future file this is the right way because internally it will do a stream copy of the virtual IFutureFile (which is not a real file so cannot really be moved)

jvillafanez · 2017-04-06T13:23:45Z

Ok, it make sense now.

Taking into account the moveInto function has been copied from another place, I'm not going to complain about that (now).

I haven't tested, but 👍 for the code.

PVince81 · 2017-04-06T13:30:13Z

Thanks. I'll fix then indent and insta-merge.

Testing already done mostly through the new integration tests. I also did manual testing which worked fine.

PVince81 · 2017-04-07T12:06:51Z

We might want to backport this to stable9.1 (and stable9?) where the new endpoint already existed but isn't used yet

PVince81 · 2018-01-31T17:13:33Z

Regression: #30325

lock · 2019-08-01T08:02:30Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

SergioBertolinSG added the 3 - To Review label Mar 27, 2017

SergioBertolinSG requested a review from PVince81 March 27, 2017 10:30

SergioBertolinSG mentioned this pull request Mar 27, 2017

Add integration tests for this case https://github.com/owncloud/core/issues/26992 owncloud/QA#368

Closed

SergioBertolinSG added this to the 10.0 milestone Mar 27, 2017

SergioBertolinSG mentioned this pull request Mar 27, 2017

ChunkingNG: Uploading existing file re-creates fileID #26992

Closed

SergioBertolinSG added the dev:integration-tests label Mar 27, 2017

PVince81 mentioned this pull request Mar 28, 2017

Rename hooks not fired when moving a file to a directory on new webdav endpoint #20982

Closed

PVince81 changed the title ~~Added test about checking file id after a move~~ Fix fileid loss on MOVE Mar 29, 2017

Vincent Petry and others added 7 commits March 30, 2017 14:00

Ported ObjectTree::move to IMoveTarget in new DAV endpoint

2f1c528

Remove ObjectTree::move and let is use the IMoveTarget approach instead

e29dd79

This removes the duplicated code

Added test cases from core 16825

7608ab8

Moved unit tests from ObjectTree::move to Directory

3e2c1be

Accept moving FutureFile into a Directory

3b934b3

Fix FutureFile MOVE to keep destination node

9bd0aa8

Sabre usually deletes the target node on MOVE before proceeding with the actual move operation. This fix prevents this to happen in case the source node is a FutureFile.

Fix forbidden backslash DAV integration tests

435404b

PVince81 force-pushed the inttest-checking-fileid-new-chunking branch from 1b168bd to 435404b Compare March 30, 2017 12:00

PVince81 added the sev1-critical label Apr 3, 2017

guruz requested review from butonic and jvillafanez April 4, 2017 13:05

SamuAlfageme mentioned this pull request Apr 6, 2017

Moving files causes re-download and lost of shares in client 2.3.1 owncloud/client#5670

Closed

jvillafanez reviewed Apr 6, 2017

View reviewed changes

Replace copy-pasted spaces to tabs

a1e6123

PVince81 merged commit 78685c2 into master Apr 6, 2017

PVince81 deleted the inttest-checking-fileid-new-chunking branch April 6, 2017 13:30

PVince81 added the backport-request label Apr 7, 2017

kahlertl mentioned this pull request Apr 10, 2017

Moving files causes re-download and lost of shares with new chunking algorithmn nextcloud/server#4284

Closed

PVince81 removed the backport-request label Aug 15, 2017

PVince81 mentioned this pull request Jan 31, 2018

Rename option is visible, even though user only has share permissions #30325

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

Fix fileid loss on MOVE #27508

Fix fileid loss on MOVE #27508

Uh oh!

Conversation

SergioBertolinSG commented Mar 27, 2017

Uh oh!

mention-bot commented Mar 27, 2017

Uh oh!

PVince81 commented Mar 27, 2017

Uh oh!

PVince81 commented Mar 27, 2017

Uh oh!

PVince81 commented Mar 27, 2017

Uh oh!

PVince81 commented Mar 27, 2017

Uh oh!

PVince81 commented Mar 27, 2017

Uh oh!

SergioBertolinSG commented Mar 27, 2017

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

SergioBertolinSG commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 29, 2017

Uh oh!

PVince81 commented Mar 30, 2017

Uh oh!

PVince81 commented Mar 30, 2017

Uh oh!

PVince81 commented Mar 30, 2017

Uh oh!

PVince81 commented Mar 30, 2017

Uh oh!

PVince81 commented Mar 30, 2017

Uh oh!

PVince81 commented Mar 30, 2017

Uh oh!

jvillafanez Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

jvillafanez Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

jvillafanez Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

jvillafanez commented Apr 6, 2017

Uh oh!

PVince81 commented Apr 6, 2017

Uh oh!

jvillafanez commented Apr 6, 2017

Uh oh!

PVince81 commented Apr 6, 2017

Uh oh!

PVince81 commented Apr 7, 2017

Uh oh!

PVince81 commented Mar 29, 2017 •

edited

Loading