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

Shares not removed on file deletion #20908

Closed
rullzer opened this issue Dec 2, 2015 · 11 comments
Closed

Shares not removed on file deletion #20908

rullzer opened this issue Dec 2, 2015 · 11 comments

Comments

@rullzer
Copy link
Contributor

rullzer commented Dec 2, 2015

I came across this when debugging the smashbox tests for the new share creation code. However it is also happening on master.

Steps:

  1. user1 shares a file foo with user2
  2. All is well and user2 has access. There is a nice share entry in the db
  3. user1 deletes file foo
  4. DB entry is still present.

Now of course we would expect to have the share also removed since the file is explicitly deleted. This can lead to a lot of pollution in the sharing table.

I'm not sure if it will lead to problems since the file id no longer exists. But I can't be certain.

@MorrisJobke another one for the health app
@cmonteroluque FYI

CC: @schiesbn @PVince81

@rullzer rullzer added this to the 9.0-current milestone Dec 2, 2015
@rullzer
Copy link
Contributor Author

rullzer commented Dec 2, 2015

It feels like a regression. But no bisect tonight for me... it is on the list for tomorrow.

@MorrisJobke
Copy link
Contributor

@MorrisJobke another one for the health app

Existing behavior (until 8.2):

@MorrisJobke another one for the health app

Not really because it is a repairable state and then we should auto repair it.

@rullzer
Copy link
Contributor Author

rullzer commented Dec 3, 2015

Ah mmm. starting to remember now.

Do you remember what was the reason we did not install a hook to listen for the delete events?

@rullzer
Copy link
Contributor Author

rullzer commented Dec 3, 2015

Because the repair step is fine... but it should not happen any more.

@PVince81
Copy link
Contributor

PVince81 commented Dec 3, 2015

@rullzer the reason for not using hooks: the delete hook only fires for the deleted folder, not its children. We didn't want to have recursive logic to find the file id of all children and delete the matching parents as it would be too expensive and would delay the delete operation for too long for the user/client.

@rullzer
Copy link
Contributor Author

rullzer commented Dec 3, 2015

Ai... I get it... but we need some way to solve this if we are going to allow 3rdparty shareproviders.

@rullzer
Copy link
Contributor Author

rullzer commented Jan 15, 2016

So this bit me again today..

We really should think how to fix this..

@rullzer
Copy link
Contributor Author

rullzer commented Feb 16, 2016

Ok so this can haunt us now that we have lazy shares... let me fix the OCS endpoint to not crash on this...

rullzer added a commit that referenced this issue Feb 17, 2016
Since we have lazy shares it can happen that a share is actually
invalid. See #20908

This add checks for the get methods to handle the NotFound exception.
@nickvergessen
Copy link
Contributor

Also think about accidently deletion of a folder...
Currently you can quickly restore it from the trashbin, and all the thousand shares are still there.

@nickvergessen
Copy link
Contributor

Closing as per #22434 and after talking to @rullzer who agreed, that this is all we can do atm

@lock
Copy link

lock bot commented Aug 6, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants