Remove pagination from the repair step #30989

sharidas · 2018-04-03T10:58:33Z

The pagination is not required as the
rows are deleted during the operation.

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

Description

The pagination should be removed from this repair step. Because if the pagination is there, then the changes will not be taken after the first removal in the loop. The second iteration might omit changes which have broken links.

Related Issue

#30653 (comment)

Motivation and Context

If the rows are deleted on the fly, then its better to avoid pagination and use the pagelimit .

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
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.

PVince81 · 2018-04-03T12:18:08Z

Please add a unit test that proves that it works. The test should fail without this commit.

You could add an attribute to set pagination to smaller values if it makes it easier to test.

PVince81 · 2018-04-03T16:20:07Z

add unit tests

codecov · 2018-04-03T17:31:21Z

Codecov Report

Merging #30989 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #30989      +/-   ##
============================================
+ Coverage      62.3%   62.45%   +0.14%     
- Complexity    18208    18229      +21     
============================================
  Files          1142     1145       +3     
  Lines         68229    68275      +46     
  Branches       1234     1234              
============================================
+ Hits          42511    42641     +130     
+ Misses        25357    25273      -84     
  Partials        361      361

Flag	Coverage Δ	Complexity Δ
#javascript	`52% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`63.64% <100%> (+0.16%)`	`18229 <1> (+21)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Repair/RepairOrphanedSubshare.php	`86.66% <100%> (-7.28%)`	`3 <1> (-3)`
core/Migrations/Version20170221114437.php	`0% <0%> (ø)`	`2% <0%> (ø)`	⬇️
lib/private/User/Sync/SeenUsersIterator.php	`100% <0%> (ø)`	`4% <0%> (?)`
lib/private/User/Sync/AllUsersIterator.php	`100% <0%> (ø)`	`4% <0%> (?)`
lib/private/User/Sync/UsersIterator.php	`100% <0%> (ø)`	`5% <0%> (?)`
lib/private/User/SyncService.php	`74.83% <0%> (+1.71%)`	`56% <0%> (-1%)`	⬇️
lib/private/User/AccountMapper.php	`77.77% <0%> (+4.3%)`	`31% <0%> (+6%)`	⬆️
core/Command/User/SyncBackend.php	`55.17% <0%> (+55.17%)`	`42% <0%> (+6%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bedfb50...14edb80. Read the comment docs.

sharidas · 2018-04-03T17:33:53Z

Added unit tests:

One of the test proves that if pagination is there then the code would fail to remove all orphan shares (i.e, without this change).
Another test to prove that with this change all orphan shares could be removed.

Will see how the tests progress in the CI.

PVince81 · 2018-04-04T09:59:17Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

+	 * This test will have more orphan shares and will check if the repair
+	 * test is working properly.
+	 */
+	public function testOrphanSharesFailsWithPagination() {


the test is only testing pagination itself, not a failure

sharidas · 2018-04-04T18:09:44Z

lib/private/Repair/RepairOrphanedSubshare.php

 		$this->missingParents->where($this->missingParents->expr()->isNotNull('parent'));
-		$this->missingParents->andWhere($this->missingParents->expr()->notIn('parent', $this->missingParents->createFunction($qb->getSQL())));
+		$this->missingParents->andWhere($this->missingParents->createFunction('NOT EXISTS (' . $qb->getSQL() . ')'));


Updated query builder from -> #30989 (comment).
Also resumed $this->deleteOrphanReshares ( which was omitted in the previous version of the PR).

PVince81 · 2018-04-05T05:54:09Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

+			//Check if the row is there before deleting
+			$result = $qb->select('id')
+				->from('share')
+				->where($qb->expr()->eq('id', $qb->createNamedParameter($randomRow + $pareReshareCountRest)))


this is very unreliable and will likely not work on Oracle: please store the ids of all inserted entries from the above query and then delete a few selected ones.

you cannot assume that the share ids will start at 1 here as not all DBs reset their autoincrement to zero

please implement this in a proper way.

also see owncloud-archive/documentation#3805 (comment) when retrieving inserted ids

PVince81 · 2018-04-05T05:55:38Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

+							'stime' => $qb->expr()->literal($time),
+						])
+						->execute();
+					if (($parentReshareCount - $pareReshareCountRest) >= 3000) {


PVince81 · 2018-04-05T05:57:23Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

+			$randomRow++;
+		}
+
+		//Now run the repair step and prove that this test fails


no, we don't prove that a test fail. a test must always pass.

what I said about failing tests is based on Test Driven Development (TDD) where you first write a test that reproduces an issue. the test will fail at first and then you implement the code to make it pass.

it doesn't mean that the test exists to prove that something fails

PVince81 · 2018-04-05T06:00:12Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

+		$this->assertTrue($testFail);
+	}
+
+	public function mockRepairOrphanShareMethodWithPagination() {


this looks wrong as you reimplemented the same code as the repair step

a much better approach is to keep track of what entries you "destroyed" when creating your test set and then writing a query that checks that exactly these entries were cleant up

for example if your test code destroyed the parents of the share ids 5, 15, 18 then in your assertion code after running the repair step, you'll query whether the sub-shares with parents 5, 15 and 18 are gone.

this is better than using count. Maybe we should move away from just counting as it's not accurate enough.

sharidas · 2018-04-05T12:34:41Z

We need to consider following PR's:

fix repair orphaned shares sql and loop condition #31014

The backport PR should be considered last: #31004

The pagination is not required as the rows are deleted during the operation. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2018-04-05T13:52:42Z

Added commit: #31014 to this PR.

PVince81 · 2018-04-06T09:49:54Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

@@ -286,4 +286,128 @@ public function testLargeSharesWithOrphans() {
 		} while(count($results) > 0);
 		$this->assertEquals($totalParents - 20, $result);
 	}
+
+	/**
+	 * A new approach


the approach is not new, this is "the" approach

if you want to write "new approach" the text needs to be in the PR comment

please replace the comment with whatever the test is testing

what does "no more pagination" means ? no more compared to what ?

PVince81 · 2018-04-06T09:50:19Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

+		}
+
+		//Lets check rance of 1 to 9
+		foreach (range(1, 9) as $adminIndex) {


this looks like the same loop code like line 391, maybe you can put these together

The intention here was to see the first 9 indexes are there or not. A proof that first 9 parents are not deleted according to the test.

Removed the duplicated code and updated with a loop so that it look better. So if one has to check if the index is there or not in this large scenario, appending the array $checkRandomAvailableEntries is enough.

PVince81 · 2018-04-06T09:51:18Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

+	/**
+	 * A new approach
+	 */
+	public function testOrphanSharesNoMorePagination() {


the test is not testing a code change, it's testing a behavior: what behavior are you testing ?

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

sharidas · 2018-04-06T11:32:18Z

@PVince81 Ready for review.

PVince81

👍

PVince81 · 2018-04-06T12:01:14Z

@sharidas please backport as part of #30653 (comment), all in a single PR

sharidas · 2018-04-06T12:18:58Z

Updated the backport PR : #31004 to a single PR. It includes the changes we have made so far mentioned here... #30653 (comment)

lock · 2019-07-31T11:02:14Z

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.

sharidas added the 3 - To Review label Apr 3, 2018

sharidas added this to the development milestone Apr 3, 2018

sharidas self-assigned this Apr 3, 2018

sharidas requested a review from PVince81 April 3, 2018 10:58

PVince81 mentioned this pull request Apr 3, 2018

Repair step to fix orphan reshares #30653

Merged

9 tasks

PVince81 added 2 - Developing backport-request and removed 3 - To Review backport-request labels Apr 3, 2018

sharidas force-pushed the remove-pagination-repair-orphansubshare branch from f8e668b to 12b0d54 Compare April 3, 2018 17:30

sharidas added 3 - To Review 2 - Developing and removed 2 - Developing 3 - To Review labels Apr 3, 2018

sharidas force-pushed the remove-pagination-repair-orphansubshare branch 9 times, most recently from c8f303f to 25a6225 Compare April 4, 2018 09:27

PVince81 reviewed Apr 4, 2018

View reviewed changes

sharidas force-pushed the remove-pagination-repair-orphansubshare branch 2 times, most recently from 71d3fe2 to 63ab8a4 Compare April 4, 2018 10:38

sharidas force-pushed the remove-pagination-repair-orphansubshare branch from 958c7aa to 11b82ee Compare April 4, 2018 16:00

sharidas commented Apr 4, 2018

View reviewed changes

PVince81 reviewed Apr 5, 2018

View reviewed changes

butonic mentioned this pull request Apr 5, 2018

fix repair orphaned shares sql and loop condition #31014

Closed

sharidas force-pushed the remove-pagination-repair-orphansubshare branch 3 times, most recently from 6edd6ec to 5ee7920 Compare April 5, 2018 12:08

Remove pagination from the repair step

82fa0e4

The pagination is not required as the rows are deleted during the operation. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the remove-pagination-repair-orphansubshare branch from 5ee7920 to 8fb942f Compare April 5, 2018 13:34

PVince81 reviewed Apr 6, 2018

View reviewed changes

fix repair orphaned shares sql and loop condition

14edb80

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

sharidas force-pushed the remove-pagination-repair-orphansubshare branch from 8fb942f to 14edb80 Compare April 6, 2018 10:51

PVince81 approved these changes Apr 6, 2018

View reviewed changes

PVince81 merged commit a996a23 into master Apr 6, 2018

PVince81 deleted the remove-pagination-repair-orphansubshare branch April 6, 2018 12:00

PVince81 added the backport-request label Apr 6, 2018

PVince81 removed the backport-request label Apr 10, 2018

ownclouders mentioned this pull request Jul 5, 2018

Extend maintenance:repair to allow runtime repairs. #31995

Closed

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pagination from the repair step #30989

Remove pagination from the repair step #30989

sharidas commented Apr 3, 2018

PVince81 commented Apr 3, 2018

PVince81 commented Apr 3, 2018

codecov bot commented Apr 3, 2018 •

edited

Loading

sharidas commented Apr 3, 2018

PVince81 Apr 4, 2018

sharidas Apr 4, 2018 •

edited

Loading

PVince81 Apr 5, 2018

PVince81 Apr 5, 2018

PVince81 Apr 5, 2018

PVince81 Apr 5, 2018

PVince81 Apr 5, 2018

sharidas commented Apr 5, 2018 •

edited

Loading

sharidas commented Apr 5, 2018

PVince81 Apr 6, 2018

PVince81 Apr 6, 2018

sharidas Apr 6, 2018

sharidas Apr 6, 2018

PVince81 Apr 6, 2018 •

edited

Loading

sharidas commented Apr 6, 2018

PVince81 left a comment

PVince81 commented Apr 6, 2018

sharidas commented Apr 6, 2018

lock bot commented Jul 31, 2019

Remove pagination from the repair step #30989

Remove pagination from the repair step #30989

Conversation

sharidas commented Apr 3, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

PVince81 commented Apr 3, 2018

PVince81 commented Apr 3, 2018

codecov bot commented Apr 3, 2018 • edited Loading

Codecov Report

sharidas commented Apr 3, 2018

Choose a reason for hiding this comment

sharidas Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas commented Apr 5, 2018 • edited Loading

sharidas commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

sharidas commented Apr 6, 2018

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Apr 6, 2018

sharidas commented Apr 6, 2018

lock bot commented Jul 31, 2019

codecov bot commented Apr 3, 2018 •

edited

Loading

sharidas Apr 4, 2018 •

edited

Loading

sharidas commented Apr 5, 2018 •

edited

Loading

PVince81 Apr 6, 2018 •

edited

Loading