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: Transfer ownership with S3 as primary #51020

Open
wants to merge 1 commit into
base: stable29
Choose a base branch
from

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Feb 25, 2025

Issue

When using S3 as primary storage, transferring ownership with the --move option fail with the following error:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '8-45b963397aa40d4a0063e0d85e4fe7a1' for key 'fs_storage_path_hash'

The --move option moves the entire home folder from one account to another.
The error means that the move failed because the destination folder already exist in oc_filecache.

Might have been introduced by: #26149

Investigation

  1. With S3 as primary storage, folders only exists as entries in oc_filecache.
  2. With S3 as primary storage, moveFromStorage(...) only moves the cache entry, as nothing needs to be moved on disk. This cache move does not delete potentially pre-existing destination folder.
  3. With Local storage, moveFromStorage(...) calls rename(...) which delete pre-existing folder.

S3's moveFromStorage

public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath, ?ICacheEntry $sourceCacheEntry = null): bool {
$sourceCache = $sourceStorage->getCache();
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class) && $sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) {
$this->getCache()->moveFromCache($sourceCache, $sourceInternalPath, $targetInternalPath);
// Do not import any data when source and target bucket are identical.
return true;
}

Local's rename, called by moveFromStorage

if ($this->file_exists($target)) {
if ($this->is_dir($target)) {
$this->rmdir($target);
} elseif ($this->is_file($target)) {
$this->unlink($target);
}
}

Solution

Delete pre-existing folder in moveFromStorage(...)

Does it make sense to have that here? It feels off.

Todo

  • Target master

@artonge artonge added bug 3. to review Waiting for reviews feature: filesystem php Pull requests that update Php code labels Feb 25, 2025
@artonge artonge added this to the Nextcloud 32 milestone Feb 25, 2025
@artonge artonge self-assigned this Feb 25, 2025
When using S3 as primary storage, transferring ownership with the `--move` option fail with the following error:

`SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '8-45b963397aa40d4a0063e0d85e4fe7a1' for key 'fs_storage_path_hash'`

The `--move` option moves the entire home folder from one account to another.
The error means that the move failed because the destination folder already exist in `oc_filecache`.

- With S3 as primary storage, folders only exists as entries in `oc_filecache`.
- With S3 as primary storage, `moveFromStorage(...)` only moves the cache entry, as nothing needs to be moved on disk. This cache move does not delete potentially pre-existing destination folder.
- With Local storage, `moveFromStorage(...)` calls `rename(...)` which delete pre-existing folder.

- `transfer(...)`: https://github.com/nextcloud/server/blob/687a4d9ac7fcdbd935f81a0def567a1092306f7a/apps/files/lib/Service/OwnershipTransferService.php#L112
- `oneTimeUserSetup(...)`: https://github.com/nextcloud/server/blob/687a4d9ac7fcdbd935f81a0def567a1092306f7a/lib/private/Files/SetupManager.php#L261-L262
- `mkdir(...)`: https://github.com/nextcloud/server/blob/687a4d9ac7fcdbd935f81a0def567a1092306f7a/lib/private/Files/ObjectStore/ObjectStoreStorage.php#L91-L135
- `moveFromStorage(...)`: https://github.com/nextcloud/server/blob/687a4d9ac7fcdbd935f81a0def567a1092306f7a/lib/private/Files/ObjectStore/ObjectStoreStorage.php#L635-L636

Delete pre-existing folder in `moveFromStorage(...)`

Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge force-pushed the artonge/fix/transfer_ownership branch from 8858cbe to 50696c5 Compare February 25, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug feature: filesystem php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant