Providing --path option to transfer-ownership by sharidas · Pull Request #27343 · owncloud/core

sharidas · 2017-03-09T10:14:23Z

This will help user to selectively move the folders
specified using --path option, instead of moving
entire folder under files directory.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

The change has been made for occ command's transfer-ownership. An additional --path argument has been added to support. This will help user to pass particular folder/file to move to destination user/target. So I have introduced an optional argument --path. I have deliberately made it optional, as its user choice to move entire folder under files or selectively.

Related Issue

This issue which this patch addresses is https://github.com/owncloud/platform/issues/91

Motivation and Context

This helps the user to selectively transfer the ownership of the files/folders instead of moving entire folder under files.

How Has This Been Tested?

Below is the way I have tested:

The folder files/1/1a/1aa/ has 1aaa, 1aab and 1aac folders
root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud# sudo -u www-data ./occ files:transfer-ownership --path "test1/files/1/1a/1aa" test1 test2
Analysing files of test1 ...
0 [>---------------------------]
Collecting all share information for files and folder of test1 ...
0 [>---------------------------]
Transferring files to test2/files/transferred from test1 on 20170309_101206 ...
Restoring shares ...
0 [>---------------------------]
root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud#

after the execution I was able to see in test2 user:

root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud# ls -lt data/test2/files/transferred\ from\ test1\ on\ 20170309_101206/
total 12
drwxr-xr-x 2 www-data www-data 4096 Mar 9 15:42 1aac
drwxr-xr-x 2 www-data www-data 4096 Mar 9 15:42 1aab
drwxr-xr-x 2 www-data www-data 4096 Mar 9 15:42 1aaa
root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud#

and in test1 user:

root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud# ls data/test1/files/1/1a/
1ab
root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud#

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
[x ] New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-03-09T10:14:24Z

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

PVince81 · 2017-03-09T10:23:14Z

apps/files/lib/Command/TransferOwnership.php

s/copy/transfer/ (it's a move, not a copy)

Replaced copy with transfer in the new update.

PVince81 · 2017-03-09T10:23:55Z

apps/files/lib/Command/TransferOwnership.php

missing double quotes on string ?

Fixed the double quotes on string in the new update.

sharidas · 2017-03-10T13:45:48Z

The updated version of the patch is ready for review. @PVince81 Can you please review this change.

PVince81 · 2017-03-10T14:19:09Z

BUG: specifying a non-existing path on the CLI must fail instead of attempting transfer
BUG: some problem with transferring shares, steps:

Create three users "user1", "user2" and "user3"
From "user3" share a folder "foruser1" with "user1"
Login as "user1"
Create a folder "transferthis"
Move "foruser1" to "transferthis"
Create a folder "transferthis/foruser2/x"
Share folder "transferthis/foruser2" with "user2" AND "user3"
Create a folder "transferthis/normal/y"
Log out

± % occ files:transfer-ownership user1 user2 --path=transferthis
Analysing files of user1 ...
    0 [>---------------------------]
Collecting all share information for files and folder of user1 ...
    2 [============================]
Transferring files to user2/files/transferred from user1 on 20170310_021750 ...
Restoring shares ...
 1/2 [==============>-------------]  50%Share with id 3 points at deleted file, skipping
 2/2 [============================] 100%

Login as "user1" and "user2" to check.

Expected result: folder "transferthis" is no more in "user1"'s account and is present in "user2's" account.
Actual result: nothing was transferred.

Please have a look.

PVince81 · 2017-03-10T14:21:30Z

you might need to debug into $view->rename to find out what happens and whether it fails somewhere there

sharidas · 2017-03-10T14:24:22Z

@PVince81 Thanks for the feedback. I will have a look at it.

PVince81 · 2017-03-10T15:00:02Z

Best would be to write an integration test right away because setting this up for testing is time consuming as you need to start from scratch after a transfer.

Running the test when debugging would help save this time.

PVince81 · 2017-03-13T16:11:59Z

@sharidas it seems you have found a bug deep in the FS code.

In the old code, the move operation would do: rename('test1/files', 'test2/files/transferred ...') which moves "files" to the target and renames it to "transferred ...". So far so good, this is legitimate.

But now with the new logic the rename is: rename('test1/files/a', 'test2/files/transferred .../a').
However the folder "transferred ..." doesn't exist yet.

Still, it seems the View happily moves the folder to this non-existing target folder instead of failing... resulting in the inconsistency you saw. The filecache will contain "a" but with a parent -1 because the parent did not exist / have an id.

@sharidas for you the fix is to call $view->mkdir() for the target folder first.

The View but will need to be fixed separately, I'll make a ticket for it.

PVince81 · 2017-03-13T16:15:57Z

Raised View issue here: #27376

sharidas · 2017-03-13T16:47:54Z

@PVince81 I have updated the PR by adding $view->mkdir at https://github.com/owncloud/core/pull/27343/files#diff-0674cc8bda871e32d63705af16fa8144R258. I have verified that share folders and other folders which are created by user1 do get transferred.

SergioBertolinSG · 2017-03-14T11:42:55Z

tests/integration/features/transfer-ownership.feature

This sentence is not clear.
Something like this seems better:

When transfering ownership of path "test" from "user0" to "user1"

Also I recommend to put the starting slash before any paths as it is the decided choice to name paths in scenarios.

sharidas · 2017-03-14T14:01:52Z

@SergioBertolinSG I have updated the integration tests. Kindly review the same.

SergioBertolinSG · 2017-03-14T12:19:47Z

tests/integration/features/bootstrap/CommandLineContext.php

The occ command is expecting a --path=$PATH. So this is failing because the lack of '='.

Also the occ command example is wrong because it is not adding the '='.

Usage: files:transfer-ownership [options] [--] <source-user> <destination-user> Arguments: source-user owner of files which shall be moved destination-user user who will be the new owner of the files Options: --path=PATH selectively provide the path to transfer. For example --path "alice/files/Music"

SergioBertolinSG · 2017-03-14T15:29:35Z

tests/integration/features/transfer-ownership.feature

Missing Then

SergioBertolinSG · 2017-03-14T15:33:37Z

Tests passed locally.

But this explanation in the help of the command is wrong:

Options:
--path=PATH selectively provide the path to transfer. For example --path "alice/files/Music"

Which this changes It has to be:
--path PATH

Or change it to use '=' with the command.

sharidas · 2017-03-14T18:01:14Z

@PVince81 @SergioBertolinSG I have made the changes. Kindly review them.

PVince81 · 2017-03-14T18:28:52Z

Seems to have trouble with PHP 5.6 ?

± % sudo -u wwwrun ./occ
PHP Fatal error:  Class 'OCA\Files\Command\InputOption' not found in /srv/www/htdocs/owncloud/apps/files/lib/Command/TransferOwnership.php on line 96
PHP Stack trace:
PHP   1. {main}() /srv/www/htdocs/owncloud/occ:0
PHP   2. require_once() /srv/www/htdocs/owncloud/occ:11
PHP   3. OC\Console\Application->loadCommands() /srv/www/htdocs/owncloud/console.php:98
PHP   4. OC\Console\Application->loadCommandsFromInfoXml() /srv/www/htdocs/owncloud/lib/private/Console/Application.php:108
PHP   5. OC\ServerContainer->query() /srv/www/htdocs/owncloud/lib/private/Console/Application.php:166
PHP   6. OC\AppFramework\Utility\SimpleContainer->query() /srv/www/htdocs/owncloud/lib/private/ServerContainer.php:80
PHP   7. OC\AppFramework\Utility\SimpleContainer->resolve() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:113
PHP   8. OC\AppFramework\Utility\SimpleContainer->buildClass() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:92
PHP   9. ReflectionClass->newInstanceArgs() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:75
PHP  10. OCA\Files\Command\TransferOwnership->__construct() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:75
PHP  11. Symfony\Component\Console\Command\Command->__construct() /srv/www/htdocs/owncloud/apps/files/lib/Command/TransferOwnership.php:76
PHP  12. OCA\Files\Command\TransferOwnership->configure() /srv/www/htdocs/owncloud/lib/composer/symfony/console/Command/Command.php:63

I did run 'make cleanandmake` to make sure it wasn't a deps issue.

PVince81 · 2017-03-14T18:35:28Z

apps/files/lib/Command/TransferOwnership.php

the example is wrong, it should not contain the "$userId/files/" piece

PVince81 · 2017-03-14T18:36:46Z

apps/files/lib/Command/TransferOwnership.php

is this safe ? what if there are two directories within the same path, like "somedir/test/somedir" ?

Can you add a PHP comment to explain why you are using strpos here ?

Would also be good to add an explicit condition like >= 0

PVince81 · 2017-03-14T18:38:34Z

apps/files/lib/Command/TransferOwnership.php

Argh, no. Don't do anything with the direct FS. There are some storages that do not store anything in the "data" folder but store in object store.

Please always use the $view methods for checking path existence. $view->is_dir($this->inputPath).

In general in OC never ever access the FS directly, always use a $view or the files public API.

sharidas · 2017-03-15T07:54:46Z

@PVince81 Kindly review my updated PR. I have removed ugly changes which directly access FS. Used $view->is_dir in the updated PR. I have also modified the example 'selectively provide the path to transfer. For example --path "folder_name" or --path="folder_name'.
The reason for failure in 5.6 was not having "use Symfony\Component\Console\Input\InputOption;". I hav e fixed that too.

PVince81 · 2017-03-15T08:45:19Z

apps/files/lib/Command/TransferOwnership.php

--path "folder_name" or --path="folder_name" ? it's twice the same

PVince81

Almost there!

A few more required changes, especially the one about init mount points.

PVince81 · 2017-03-15T08:46:27Z

apps/files/lib/Command/TransferOwnership.php

add a space after ":" for better readability

PVince81 · 2017-03-15T08:47:16Z

apps/files/lib/Command/TransferOwnership.php

I suggest you use ltrim($this->inputPath, '/') to avoid doubling slashes in case the user input a leading slash in the path

PVince81 · 2017-03-15T08:49:45Z

apps/files/lib/Command/TransferOwnership.php

Before you can use the view you need to first call initMountPoints which is done a bit later.
I suggest you move this validation after the initMountPoints call below, or at least the one from the source user.

The reason for this is that sometimes the storage is not directly on FS but in an object store. Only if the object store is mounted for that user will you be able to access it with the View.

PVince81 · 2017-03-15T08:50:51Z

apps/files/lib/Command/TransferOwnership.php

ah yes, when comparing paths you really need to make sure there are no double slashes.

this makes the ltrim($path, '/') call I suggested above really necessary.

sharidas · 2017-03-15T09:23:04Z

@PVince81 I have moved the validation part for the path under the initMount. Have provided ltrim for the path ( i.e, $this->inputPath ) and during the path comparison I have used ltrim both in the left hand side and right hand side of the comparison. I have added leading space for $unknownDir ( around the ). I have removed --path="folder_name" in the help. All these are made in the updated commit.

PVince81 · 2017-03-15T15:23:38Z

Integration tests fail, see https://jenkins.owncloud.org/job/owncloud-core/job/core/job/PR-27343/15/display/redirect

SergioBertolinSG · 2017-03-15T16:41:38Z

This behaviour should not be this way:

/occ files:transfer-ownership --help

--path[=PATH] selectively provide the path to transfer. For example --path "folder_name"

it looks like path value is optional, that's misleading.

It should look like --path=PATH

Also when running the command without --help it should show the same [--path=PATH] In this case:

./occ files:transfer-ownership       

                                                                    
  [Symfony\Component\Console\Exception\RuntimeException]            
  Not enough arguments (missing: "source-user, destination-user").  
                                                                    

files:transfer-ownership [--path [PATH]] [--] <source-user> <destination-user>

PVince81 · 2017-03-16T16:11:33Z

apps/files/lib/Command/TransferOwnership.php

ltrim($this->inputPath,'/') to remove leading slashes

PVince81 · 2017-03-16T16:13:03Z

apps/files/lib/Command/TransferOwnership.php

you don't need to ltrim the input path if you do it right at the start

PVince81 · 2017-03-16T16:13:44Z

apps/files/lib/Command/TransferOwnership.php

the reason I'm asking you to ltrim the path is that if someone sets --path=/somedir, this concatenation here would result in "$userId/files//somedir" (double slash)

This will help user to selectively move the folders specified using --path option, instead of moving entire folder under files directory. Signed-off-by: Sujith H <sharidasan@owncloud.com>

Update the integration test for transfer-ownership as the new option --path is introduced in the command. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81 · 2017-03-16T19:02:06Z

👍 if tests still pass

lock · 2019-08-03T18:02:11Z

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.

PVince81 reviewed Mar 9, 2017

View reviewed changes

sharidas force-pushed the occ-transfer-ownership-with-path branch from d5eb4ce to d0b7dfb Compare March 9, 2017 10:37

sharidas added the 3 - To Review label Mar 9, 2017

sharidas force-pushed the occ-transfer-ownership-with-path branch 2 times, most recently from 8531de1 to 3440a1f Compare March 10, 2017 13:44

sharidas force-pushed the occ-transfer-ownership-with-path branch 3 times, most recently from cde6634 to 8a62eb7 Compare March 13, 2017 16:01

sharidas force-pushed the occ-transfer-ownership-with-path branch 2 times, most recently from 04f7dd1 to f388285 Compare March 13, 2017 16:45

This was referenced Mar 14, 2017

Update the integration test for transfer-ownership #27382

Closed

Update the integration test for transfer-ownership #27383

Closed

sharidas force-pushed the occ-transfer-ownership-with-path branch 2 times, most recently from 9c2b4d3 to 339e546 Compare March 14, 2017 11:33

SergioBertolinSG suggested changes Mar 14, 2017

View reviewed changes

sharidas force-pushed the occ-transfer-ownership-with-path branch from 339e546 to ed8523a Compare March 14, 2017 13:57

SergioBertolinSG reviewed Mar 14, 2017

View reviewed changes

sharidas force-pushed the occ-transfer-ownership-with-path branch 2 times, most recently from ed044bd to 2c691dd Compare March 14, 2017 18:00

PVince81 reviewed Mar 14, 2017

View reviewed changes

apps/files/lib/Command/TransferOwnership.php Outdated

Copy link

Contributor

PVince81 Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the example is wrong, it should not contain the "$userId/files/" piece

PVince81 reviewed Mar 14, 2017

View reviewed changes

sharidas force-pushed the occ-transfer-ownership-with-path branch from 2c691dd to 0886c50 Compare March 15, 2017 07:51

PVince81 reviewed Mar 15, 2017

View reviewed changes

apps/files/lib/Command/TransferOwnership.php Outdated

Copy link

Contributor

PVince81 Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--path "folder_name" or --path="folder_name" ? it's twice the same

PVince81 reviewed Mar 15, 2017

View reviewed changes

sharidas force-pushed the occ-transfer-ownership-with-path branch from 0886c50 to a42b2af Compare March 15, 2017 09:20

sharidas force-pushed the occ-transfer-ownership-with-path branch 2 times, most recently from e630a80 to 6008bce Compare March 16, 2017 09:15

PVince81 reviewed Mar 16, 2017

View reviewed changes

apps/files/lib/Command/TransferOwnership.php Outdated

Copy link

Contributor

PVince81 Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to ltrim the input path if you do it right at the start

PVince81 reviewed Mar 16, 2017

View reviewed changes

sharidas added 2 commits March 16, 2017 23:17

Providing --path option to transfer-ownership

3751963

This will help user to selectively move the folders specified using --path option, instead of moving entire folder under files directory. Signed-off-by: Sujith H <sharidasan@owncloud.com>

Update the integration test for transfer-ownership

ed2be64

Update the integration test for transfer-ownership as the new option --path is introduced in the command. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the occ-transfer-ownership-with-path branch from 6008bce to ed2be64 Compare March 16, 2017 17:51

PVince81 added this to the 10.0 milestone Mar 16, 2017

PVince81 added 4 - To release and removed 3 - To Review labels Mar 16, 2017

PVince81 merged commit 90a094a into owncloud:master Mar 17, 2017

MorrisJobke mentioned this pull request Mar 21, 2017

Providing --path option to transfer-ownership nextcloud/server#3965

Merged

PVince81 mentioned this pull request Sep 7, 2017

files:transfer-ownership command could be extended to allow per file and directory transfer #24579

Closed

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

Conversation

sharidas commented Mar 9, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

mention-bot commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Mar 10, 2017

Uh oh!

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

Uh oh!

Uh oh!

PVince81 commented Mar 10, 2017

Uh oh!

sharidas commented Mar 10, 2017

Uh oh!

PVince81 commented Mar 10, 2017

Uh oh!

PVince81 commented Mar 13, 2017

Uh oh!

PVince81 commented Mar 13, 2017

Uh oh!

sharidas commented Mar 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergioBertolinSG commented Mar 14, 2017

Uh oh!

sharidas commented Mar 14, 2017

Uh oh!

PVince81 commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Mar 15, 2017

Uh oh!

PVince81 commented Mar 15, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

PVince81 commented Mar 10, 2017 •

edited

Loading

SergioBertolinSG commented Mar 15, 2017 •

edited

Loading

PVince81 Mar 16, 2017 •

edited

Loading