upgrade to sharing 2.0, remove hierarchical re-shares by schiessle · Pull Request #21660 · owncloud/core

schiessle · 2016-01-12T16:03:36Z

Update script to remove all hierarchical re-shares and create a flat list instead.

I will add unit test over the next days... Feel free to have a look if you see stuff which could be optimized/improved.

cc @rullzer @DeepDiver1975

mention-bot · 2016-01-12T16:03:38Z

By analyzing the blame information on this pull request, we identified @rullzer, @icewind1991 and @MTRichards to be potential reviewers

rullzer · 2016-01-12T20:37:54Z

Great start!

DeepDiver1975 · 2016-01-13T11:30:35Z

use in() ??? - just an idea - no clue if that could help with query execution ...

I'm also not sure if there is a performance difference... still it makes probably sense to use the in operator here

schiessle · 2016-01-13T12:06:07Z

While writing tests I wonder if we also want to move the password for link shares from the "share_with" column to a new column "password"? I always hated that we abuse the share_with column for the password. I think now it would be the right time to change it... What do you think @DeepDiver1975 @rullzer ?

rullzer · 2016-01-13T12:08:16Z

@schiesbn We can only do that once we have moved all the internal handling to that column as well... so basically after all is done we can move it.. but now there are still to many places that use the old code path...

rullzer · 2016-01-13T12:08:24Z

@schiesbn but i agree it has to go :P

schiessle · 2016-01-13T12:09:12Z

We can only do that once we have moved all the internal handling to that column as well... so basically after all is done we can move it.. but now there are still to many places that use the old code path...

OK, so let's do this in a second step.

DeepDiver1975 · 2016-01-13T12:09:52Z

ci error:

PHP Fatal error:  Uncaught TypeError: Argument 1 passed to OCA\Files_Sharing\Migration::__construct() must implement interface OCP\IDBConnection, none given, called in /ssd/jenkins/workspace/server-master-linux-php7-ci/database/sqlite/label/SLAVE/apps/files_sharing/tests/migrationtest.php on line 63 and defined in /ssd/jenkins/workspace/server-master-linux-php7-ci/database/sqlite/label/SLAVE/apps/files_sharing/lib/migration.php:37
11:03:46 Stack trace:
11:03:46 #0 /ssd/jenkins/workspace/server-master-linux-php7-ci/database/sqlite/label/SLAVE/apps/files_sharing/tests/migrationtest.php(63): OCA\Files_Sharing\Migration->__construct()
11:03:46 #1 [internal function]: MigrationTest->testAddAccept()
11:03:46 #2 phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php(865): ReflectionMethod->invokeArgs(Object(MigrationTest), Array)
11:03:46 #3 phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php(743): PHPUnit_Framework_TestCase->runTest()
11:03:46 #4 phar:///usr/local/bin/phpunit/php-invoker/Invoker.php(93): PHPUnit_Framework_TestCase->runBare()
11:03:46 #5 phar:///usr/local/bin/phpu in /ssd/jenkins/workspace/server-master-linux-php7-ci/database/sqlite/label/SLAVE/apps/files_sharing/lib/migration.php on line 37

schiessle · 2016-01-13T12:33:46Z

@DeepDiver1975 this are still the tests for the old migration class... Ignore it for now. Updated unit tests will come soon 😄

MorrisJobke · 2016-01-13T13:38:52Z

What happens on commit failure? 🙈 Is it still consistent and can be called a second time?

yes, if the commit fails nothing was changed and you can call it a second time.

This was exactly the reason why I used a transaction here (beside some potential performance improvements), I want to make sure that either we update all shares or none so that it can be safely re-run in case of a error.

DeepDiver1975 · 2016-01-13T15:04:00Z

pgsql issue:

MigrationTest::testRemoveReShares
Doctrine\DBAL\Exception\SyntaxErrorException: An exception occurred while executing 'UPDATE "oc_share" SET "parent" = ?, "uid_owner" = ?, "uid_initiator" = ?, "parent" = ?, "uid_owner" = ?, "uid_initiator" = ? WHERE "id" = ?' with params [null, "owner2", "user1", null, "owner2", "user2", 346]:

SQLSTATE[42601]: Syntax error: 7 ERROR:  multiple assignments to same column "parent"

/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractPostgreSQLDriver.php:66
/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:116
/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:996
/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/lib/private/db/connection.php:203
/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php:208
/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/lib/private/db/querybuilder/querybuilder.php:124
/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/apps/files_sharing/lib/migration.php:156
/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/apps/files_sharing/lib/migration.php:57
/ssd/jenkins/workspace/core-ci-linux@6/database/pgsql/label/SLAVE/apps/files_sharing/tests/migrationtest.php:215

DeepDiver1975 · 2016-01-13T15:17:24Z

oracel issue:

MigrationTest::testRemoveReShares
Doctrine\DBAL\Driver\OCI8\OCI8Exception: ORA-02289: sequence does not exist

/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Exception.php:33
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php:214
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php:116
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php:157
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:1087
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/lib/private/db/connection.php:226
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/lib/private/db/adapteroci8.php:36
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/lib/private/db/connection.php:221
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/apps/files_sharing/tests/migrationtest.php:144
/ssd/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/apps/files_sharing/tests/migrationtest.php:214

schiessle · 2016-01-14T13:32:48Z

I fixed the pgsql error, but I have no idea oci doesn't like $parent = $this->connection->lastInsertId($this->table); https://github.com/owncloud/core/pull/21660/files#diff-9617a0875321cd12aaccd88a631e5dadR144 @DeepDiver1975 any idea?

rullzer · 2016-01-14T13:36:57Z

aaah you forgot the backticks!

schiessle · 2016-01-14T13:45:36Z

now it should work... let's cross the fingers 😉

schiessle · 2016-01-14T17:58:19Z

perfect... all tests passed. Failing tests are unrelated (cURL error 7: Failed to connect to localhost port 8083: Connection refused)

rullzer · 2016-01-16T13:06:59Z

Nothing you can fix here. But it is kinda weird this needs to be included here. Breaks the abstraction

rullzer · 2016-01-25T19:01:34Z

Ok updated this PR.

Queries are now done in chunks of 1000 shares
Cache caches max 10k shares (~20MB).
Also migrate non reshares to new format (owner/initiator).

Everything still passes.

rullzer · 2016-01-25T19:08:20Z

I tested locally and for me migrating approx 100k shares takes 2 minutes and 40 seconds. This is including ~500K assertions. Wich seems very reasonable.

rullzer · 2016-01-25T19:08:50Z

@schiesbn @nickvergessen @DeepDiver1975 @PVince81 @LukasReschke please review.

DeepDiver1975 · 2016-01-25T21:09:51Z

This requires heavy upgrade testing by @owncloud/qa

DeepDiver1975 · 2016-01-25T21:13:17Z

owncloud/QA#92

rullzer · 2016-01-26T09:38:51Z

@DeepDiver1975 yes it does!

owncloud/QA#92

Thanks

rullzer · 2016-01-28T16:02:32Z

Now that the webUI uses the OCS Share API as well. It is time to get this in! So everybody just uses new shares!

CC: @DeepDiver1975 @PVince81 @MorrisJobke @nickvergessen

PVince81 · 2016-01-28T16:42:08Z

Had a quick test, set up some reshares on ~~master~~ stable8.2 and also some reshares from inside subfolders as recipient. Ran the upgrade. The shares were flatten properly, looks nice!

PVince81 · 2016-01-28T16:59:19Z

Okay, I tested again this time starting on stable8.2, because master isn't suitable for testing because it already creates the shares in the correct format, not legacy.
Worked with link shares, reshares, different files_targets, double shares (shared to same zser through user + group), fed shares, reshared fed shares, reshared subdirs.

PVince81 · 2016-01-28T17:00:11Z

Code looks good too 👍

PVince81 · 2016-01-28T17:00:28Z

Needs a seconds reviewer @nickvergessen @MorrisJobke @icewind1991

MorrisJobke · 2016-01-29T13:15:59Z

Tested and works 👍

upgrade to sharing 2.0, remove hierarchical re-shares

schiessle added 2 - Developing feature:sharing labels Jan 12, 2016

schiessle added this to the 9.0-current milestone Jan 12, 2016

DeepDiver1975 added the in progress label Jan 12, 2016

rullzer mentioned this pull request Jan 12, 2016

Sharing 2.0 #19331

Closed

35 of 43 tasks

DeepDiver1975 reviewed Jan 13, 2016
View reviewed changes

MorrisJobke reviewed Jan 13, 2016
View reviewed changes

schiessle force-pushed the sharing2_update_reshares branch 3 times, most recently from f7b732e to 2b4ac5a Compare January 13, 2016 14:11

schiessle added 3 - To Review and removed 2 - Developing labels Jan 13, 2016

schiessle force-pushed the sharing2_update_reshares branch from 2b4ac5a to 071e443 Compare January 13, 2016 16:54

rullzer reviewed Jan 14, 2016
View reviewed changes

Comment thread apps/files_sharing/tests/migrationtest.php Outdated

rullzer Jan 14, 2016

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

aaah you forgot the backticks!

schiessle force-pushed the sharing2_update_reshares branch 2 times, most recently from 0a40d5d to 3666b32 Compare January 14, 2016 13:44

schiessle force-pushed the sharing2_update_reshares branch from 3666b32 to 4db4346 Compare January 14, 2016 15:14

rullzer reviewed Jan 16, 2016
View reviewed changes

rullzer force-pushed the sharing2_update_reshares branch 3 times, most recently from 05e4d89 to 3c671fa Compare January 25, 2016 18:42

schiessle and others added 3 commits January 28, 2016 19:24

upgrade to sharing 2.0, remove hierarchical re-shares

dc8d435

Update reshares in batches as not to run out of memory

b85770d

Update the initiator of the share as well in migration

6151a05

rullzer force-pushed the sharing2_update_reshares branch from 3c671fa to 6151a05 Compare January 28, 2016 18:27

MorrisJobke added 4 - To release and removed 3 - To Review labels Jan 29, 2016

DeepDiver1975 added a commit that referenced this pull request Jan 29, 2016

Merge pull request #21660 from owncloud/sharing2_update_reshares

8ad45da

upgrade to sharing 2.0, remove hierarchical re-shares

DeepDiver1975 merged commit 8ad45da into master Jan 29, 2016

DeepDiver1975 deleted the sharing2_update_reshares branch January 29, 2016 13:43

DeepDiver1975 removed the in progress label Jan 29, 2016

This was referenced Jan 29, 2016

[Share 2.0] Allow recipient to be passed in to getShareById #22005

Merged

[Share 2.0] Allow moving of shares #22013

Merged

paulijar mentioned this pull request Jul 12, 2017

Applications cannot detect "unshare from self" #28337

Closed

lock Bot locked as resolved and limited conversation to collaborators Aug 8, 2019

Conversation

schiessle commented Jan 12, 2016

Uh oh!

mention-bot commented Jan 12, 2016

Uh oh!

rullzer commented Jan 12, 2016

Uh oh!

DeepDiver1975 Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

schiessle Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

schiessle commented Jan 13, 2016

Uh oh!

rullzer commented Jan 13, 2016

Uh oh!

rullzer commented Jan 13, 2016

Uh oh!

schiessle commented Jan 13, 2016

Uh oh!

DeepDiver1975 commented Jan 13, 2016

Uh oh!

schiessle commented Jan 13, 2016

Uh oh!

MorrisJobke Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

schiessle Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Jan 13, 2016

Uh oh!

DeepDiver1975 commented Jan 13, 2016

Uh oh!

schiessle commented Jan 14, 2016

Uh oh!

rullzer Jan 14, 2016

Choose a reason for hiding this comment

Uh oh!

schiessle commented Jan 14, 2016

Uh oh!

schiessle commented Jan 14, 2016

Uh oh!

rullzer Jan 16, 2016

Choose a reason for hiding this comment

Uh oh!

rullzer commented Jan 25, 2016

Uh oh!

rullzer commented Jan 25, 2016

Uh oh!

rullzer commented Jan 25, 2016

Uh oh!

DeepDiver1975 commented Jan 25, 2016

Uh oh!

DeepDiver1975 commented Jan 25, 2016

Uh oh!

rullzer commented Jan 26, 2016

Uh oh!

rullzer commented Jan 28, 2016

Uh oh!

PVince81 commented Jan 28, 2016

Uh oh!

PVince81 commented Jan 28, 2016

Uh oh!

PVince81 commented Jan 28, 2016

Uh oh!

PVince81 commented Jan 28, 2016

Uh oh!

MorrisJobke commented Jan 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants