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

fix: add moveFromStorage for objectstorage to fix transfer ownership on s3 #32781

Closed
wants to merge 1 commit into from

Conversation

unteem
Copy link

@unteem unteem commented Jun 9, 2022

This PR fixes #25693 and #31870

When transferring ownership on s3 primary storage it was doing a copy and delete leading to shares being linked to non-existing files and failing to transfer

Implementing moveFromStorage in lib/private/Files/ObjectStore/ObjectStoreStorage.php fixes it.

It doesn't fix the isSameStorage (

if ($this->isSameStorage($sourceStorage)) {
) that should return true as mentioned in #31870 which anyway leads to another issue. If this condition passes it then goes to rename which only works for renaming a folder/files in the same user storage.

@@ -611,4 +611,17 @@ private function copyFile(ICacheEntry $sourceEntry, string $to) {
throw $e;
}
}

public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
while ($sourceStorage->instanceOfStorage(Jail::class)) {
Copy link
Author

Choose a reason for hiding this comment

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

I have no idea if this is needed nor what it does.
It was in moveFromStorage function in Common.php and Local.php so I left it here

@szaimen szaimen requested review from icewind1991, a team, PVince81 and blizzz and removed request for a team June 9, 2022 13:31
@szaimen szaimen added bug 3. to review Waiting for reviews labels Jun 9, 2022
@szaimen szaimen added this to the Nextcloud 25 milestone Jun 9, 2022
@@ -611,4 +611,17 @@
throw $e;
}
}

public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
while ($sourceStorage->instanceOfStorage(Jail::class)) {

Check failure

Code scanning / Psalm

UndefinedClass

Class, interface or enum named OC\Files\ObjectStore\Jail does not exist
@@ -611,4 +611,17 @@
throw $e;
}
}

public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
while ($sourceStorage->instanceOfStorage(Jail::class)) {

Check failure

Code scanning / Psalm

InvalidArgument

Argument 1 of OCP\Files\Storage\IStorage::instanceOfStorage expects class-string<OCP\Files\Storage\IStorage>, OC\Files\ObjectStore\Jail::class provided
/**
* @var Jail $sourceStorage
*/
$sourceInternalPath = $sourceStorage->getUnjailedPath($sourceInternalPath);

Check failure

Code scanning / Psalm

UndefinedDocblockClass

Docblock-defined class, interface or enum named OC\Files\ObjectStore\Jail does not exist
/**
* @var Jail $sourceStorage
*/
$sourceInternalPath = $sourceStorage->getUnjailedPath($sourceInternalPath);

Check failure

Code scanning / Psalm

UndefinedDocblockClass

Docblock-defined class, interface or enum named OC\Files\ObjectStore\Jail does not exist
* @var Jail $sourceStorage
*/
$sourceInternalPath = $sourceStorage->getUnjailedPath($sourceInternalPath);
$sourceStorage = $sourceStorage->getUnjailedStorage();

Check failure

Code scanning / Psalm

UndefinedDocblockClass

Docblock-defined class, interface or enum named OC\Files\ObjectStore\Jail does not exist
@PVince81 PVince81 requested a review from artonge June 10, 2022 08:17
@PVince81
Copy link
Member

@icewind1991 please validate this approach

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

This needs to check that the source storage is also an object store storage and uses the same underlying object store. If a file is moved from an external storage to the primary object store just moving the cache item isn't enough.

This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz removed this from the Nextcloud 26 milestone Mar 9, 2023
@blizzz blizzz added this to the Nextcloud 27 milestone Mar 9, 2023
This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 15, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Mar 15, 2024
@skjnldsv
Copy link
Member

As there is no feedback since a while I will close this ticket.
If you will decide to work on this feature again and if it hasn't been fixed or implemented already, feel free to re-open and solve the various conflicts.

Thanks for the interest in Nextcloud and the effort put into this! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfer Ownership: "Share with id XX points at deleted file, skipping" and shares lost during transfer
6 participants