Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete orphaned shares in a background job #14676

Merged
merged 3 commits into from
Apr 8, 2015
Merged

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Mar 3, 2015

Partial fix for #12323
Fixes #14664

Even though this solves the issue in a "brutal" manner by simply deleting ALL orphaned shares it feels hacky.
I could add code to find all child elements but that would take more time.

@DeepDiver1975 @icewind1991

$foldersShared = \OCP\Share::getItemsShared('file');
$this->assertCount(1, $foldersShared);

// TODO: run commands
Copy link
Contributor

Choose a reason for hiding this comment

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

#14644 provides $this->runCommands() for that

Copy link
Contributor

Choose a reason for hiding this comment

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

#14644 is now merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, I need to get my hands dirty again here 😄

@PVince81
Copy link
Contributor Author

Fixes #6985

@PVince81
Copy link
Contributor Author

@MorrisJobke check this out, you asked about the SQL query to delete orphaned shares

@MorrisJobke MorrisJobke self-assigned this Mar 27, 2015
@PVince81
Copy link
Contributor Author

  • make it work with SQLite
  • make a background job instead of a command
  • add unit test

@PVince81 PVince81 force-pushed the deleteorphanedshares branch from 7d9b937 to f1074fe Compare March 27, 2015 18:15
@PVince81 PVince81 changed the title [WIP] Delete orphaned shares in a background job Delete orphaned shares in a background job Mar 28, 2015
@PVince81 PVince81 force-pushed the deleteorphanedshares branch from f1074fe to 8e25d90 Compare March 28, 2015 17:31
@PVince81
Copy link
Contributor Author

Added unit test and special case for SQLite.

@MorrisJobke
Copy link
Contributor

Test\BackgroundJob\DeleteOrphanedSharesJobTest::testClearShares
Orphaned shares deleted
Failed asserting that actual size 1 matches expected size 0.

/var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/apps/files_sharing/tests/deleteorphanedsharesjobtest.php:99

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 7, 2015

After discussing with @MorrisJobke we decided to go the sub-query approach which works for all DBs (note that the current approach also fails on PostgreSQL).

I'll make the change.

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 7, 2015

  • make sure we don't delete non-file shares, need to check item_type

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 7, 2015

Retested with a shared address book. I can confirm that only the orphaned folder share was deleted.

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 7, 2015

@MorrisJobke MorrisJobke force-pushed the deleteorphanedshares branch from 8b02313 to 7fb7774 Compare April 7, 2015 16:08
@MorrisJobke
Copy link
Contributor

Squashed and jenkins PR update pushed :)

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 7, 2015

Individual test pass locally but not on Jenkins, possibly a side-effect of other tests:

12:24:51 1) Test\BackgroundJob\DeleteOrphanedSharesJobTest::testClearShares
12:24:51 Orphaned shares deleted
12:24:51 Failed asserting that actual size 1 matches expected size 0.
12:24:51 
12:24:51 /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/apps/files_sharing/tests/deleteorphanedsharesjobtest.php:122

I'll see what I can do.

@PVince81 PVince81 force-pushed the deleteorphanedshares branch from 7fb7774 to 17b1413 Compare April 8, 2015 08:48
@PVince81
Copy link
Contributor Author

PVince81 commented Apr 8, 2015

Rebased onto master

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 8, 2015

Jenkins PR: #15457

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 8, 2015

Still the same:

05:24:53 1) Test\BackgroundJob\DeleteOrphanedSharesJobTest::testClearShares
05:24:53 Orphaned shares deleted
05:24:53 Failed asserting that actual size 1 matches expected size 0.

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 8, 2015

The test doesn't work any more because somehow the trashbin appears and moves the files to trash instead of a full deletion. For some reason it didn't many commits ago.

I'll try and find a solution.

@nickvergessen
Copy link
Contributor

@PVince81 related to storage wrapping?

@scrutinizer-notifier
Copy link

A new inspection was created.

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 8, 2015

Jenkins PR: #15466

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 8, 2015

@nickvergessen yes, but the wrapping is normal here (not a bug).
But for the sake of unit testing, I disabled the trashbin (like we do in other places)

@ghost
Copy link

ghost commented Apr 8, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11313/
🚀 Test PASSed.🚀
chuck

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 8, 2015

Tests passed, please review @MorrisJobke @DeepDiver1975 @LukasReschke @nickvergessen

@MorrisJobke
Copy link
Contributor

Tested and works 👍

@MorrisJobke
Copy link
Contributor

maybe @rullzer or @Xenopathic want to review this? ;)

@MorrisJobke
Copy link
Contributor

@LukasReschke @nickvergessen @DeepDiver1975 Reviewers needed :)

@karlitschek
Copy link
Contributor

👍

MorrisJobke added a commit that referenced this pull request Apr 8, 2015
Delete orphaned shares in a background job
@MorrisJobke MorrisJobke merged commit 56f1ffe into master Apr 8, 2015
@MorrisJobke MorrisJobke deleted the deleteorphanedshares branch April 8, 2015 22:18
@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stray share entries in DB after deleting parent folder
7 participants