Properly expire ext storage versions by PVince81 · Pull Request #26601 · owncloud/core

PVince81 · 2016-11-10T14:49:30Z

Description

System-wide external storages have no real owner so the current user is
used as owner. However when running cron.php there is no current user,
so no expiry can be done.

This fix adds an user argument to the expire() function to tell for
which user to expire files. This information is anyway always available
now through the expire command job.

Related Issue

Fixes #24161

Motivation and Context

How Has This Been Tested?

See ticket for details

TEST: versions on ext storage are expired properly
TEST: versions on regular storage are expired properly
TEST: versions expired properly when file was edited from share recipient

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.

Backports

stable9.1
stable9

Please review @VicDeo @jvillafanez @DeepDiver1975

System-wide external storages have no real owner so the current user is used as owner. However when running cron.php there is no current user, so no expiry can be done. This fix adds an user argument to the expire() function to tell for which user to expire files. This information is anyway always available now through the expire command job.

mention-bot · 2016-11-10T14:49:32Z

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @scrutinizer-auto-fixer and @bartv2 to be potential reviewers.

jvillafanez · 2016-11-14T11:26:27Z


 		\OC_Util::setupFS($this->user);
-		Storage::expire($this->fileName);
+		Storage::expire($this->fileName, $this->user);


I find weird that you have to setup the FS for a user and then provide the same user to expire the versions. One of those shouldn't be needed from my point of view.
With the new code it's possible to setup the FS for one sure and expire versions for other... not from the command, but from other parts of the code.

I prefer this explicit way. If we don't it means we need to find out who is the user that was currently set up and extract the name from the mount point paths... super-ugly.

Or would you prefer if we moved the setupFS into the expire function instead ?

As long as we make clear that the "expire" function will unset the previous FS and set the new one, I think it's fine. It's acceptable from my point of view.

I don't know if it's being used outside of the CLI though.

Ok, now I moved setupFS into the expire function.

I also removed tearDownFS because recently we added automatic tear down between background jobs (which is the other usage outside of CLI)

PVince81 · 2016-11-21T11:40:55Z

@jvillafanez second review ?

jvillafanez · 2016-11-21T13:34:13Z

+				throw new \OC\User\NoUserException('Backends provided no user object for ' . $uid);
+			}
+
+			\OC_Util::setupFS($uid);


Shouldn't we tearDown the FS first? What happens if there is another FS already set up?

If the method won't re-set the previous FS after finishing the operation (which might be fine), then we should document this properly. Other users might consider that after this function is called, the FS would be the same, which isn't true right now.

In general calling setupFS with the same user will do nothing. And calling it with another user will just add that user's FS to the existing mount points, making the new mount points available as well. This is done for sharing for example.

I don't think we should tear down here at all, especially considering that this isn't used anywhere else and is private API anyway.

This whole setupFS/tearDown FS needs to be redesigned from the ground up...

Ok, but let's write that in the docs in order to make clear what is the expected behaviour for the function.

PVince81 · 2016-11-21T17:32:33Z

@jvillafanez done e71b59f

jvillafanez · 2016-11-22T07:46:59Z

👍

* Properly expire ext storage versions System-wide external storages have no real owner so the current user is used as owner. However when running cron.php there is no current user, so no expiry can be done. This fix adds an user argument to the expire() function to tell for which user to expire files. This information is anyway always available now through the expire command job. * Move version expire setupFS into the expire function * Add comment about not tearing down in version Storage::expire()

PVince81 · 2016-11-22T09:27:44Z

stable9.1: #26681

* Properly expire ext storage versions System-wide external storages have no real owner so the current user is used as owner. However when running cron.php there is no current user, so no expiry can be done. This fix adds an user argument to the expire() function to tell for which user to expire files. This information is anyway always available now through the expire command job. * Move version expire setupFS into the expire function * Add comment about not tearing down in version Storage::expire()

PVince81 · 2016-11-22T09:33:58Z

stable9: #26682

…sions [stable9] Properly expire ext storage versions (#26601)

…ersions [stable9.1] Properly expire ext storage versions (#26601)

lock · 2019-08-04T08:02:36Z

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 added 3 - To Review app:files_versions labels Nov 10, 2016

PVince81 added this to the 9.2 milestone Nov 10, 2016

PVince81 mentioned this pull request Nov 10, 2016

Versions on external storage never expire #24161

Closed

jvillafanez reviewed Nov 14, 2016

View reviewed changes

Move version expire setupFS into the expire function

3917ff8

jvillafanez reviewed Nov 21, 2016

View reviewed changes

Add comment about not tearing down in version Storage::expire()

e71b59f

PVince81 added the backport-request label Nov 22, 2016

PVince81 merged commit d8443b8 into master Nov 22, 2016

PVince81 deleted the ext-storage-expireversions branch November 22, 2016 09:22

PVince81 self-assigned this Nov 22, 2016

PVince81 mentioned this pull request Nov 22, 2016

[stable9.1] Properly expire ext storage versions (#26601) #26681

Merged

PVince81 mentioned this pull request Nov 22, 2016

[stable9] Properly expire ext storage versions (#26601) #26682

Merged

PVince81 removed the backport-request label Nov 22, 2016

DeepDiver1975 added a commit that referenced this pull request Nov 22, 2016

Merge pull request #26682 from owncloud/stable9-ext-storage-expirever…

2924094

…sions [stable9] Properly expire ext storage versions (#26601)

DeepDiver1975 added a commit that referenced this pull request Nov 22, 2016

Merge pull request #26681 from owncloud/stable9.1-ext-storage-expirev…

dfc86f1

…ersions [stable9.1] Properly expire ext storage versions (#26601)

LukasReschke mentioned this pull request Dec 22, 2016

Properly expire ext storage versions (#26601) nextcloud/server#2824

Merged

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

Uh oh!

Conversation

PVince81 commented Nov 10, 2016

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Backports

Uh oh!

mention-bot commented Nov 10, 2016

Uh oh!

jvillafanez Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

jvillafanez Nov 15, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 18, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 21, 2016

Uh oh!

jvillafanez Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

jvillafanez Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 21, 2016

Uh oh!

jvillafanez commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

PVince81 commented Nov 22, 2016

Uh oh!

lock Bot commented Aug 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants