Merge pull request #48235 from nextcloud/readd-object-store-phpunit

test: re-add object store primary storage phpunit tests

.github/workflows/phpunit-object-store-primary.yml

@@ -0,0 +1,121 @@
+# SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+# SPDX-License-Identifier: MIT
+name: PHPUnit primary object store
+on:
+  pull_request:
+  schedule:
+    - cron: "15 2 * * *"
+
+concurrency:
+  group: phpunit-object-store-primary-${{ github.head_ref || github.run_id }}
+  cancel-in-progress: true
+
+jobs:
+  changes:
+    runs-on: ubuntu-latest-low
+
+    outputs:
+      src: ${{ steps.changes.outputs.src}}
+
+    steps:
+      - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
+        id: changes
+        continue-on-error: true
+        with:
+          filters: |
+            src:
+              - '.github/workflows/**'
+              - '3rdparty/**'
+              - '**/appinfo/**'
+              - '**/lib/**'
+              - '**/templates/**'
+              - '**/tests/**'
+              - 'vendor/**'
+              - 'vendor-bin/**'
+              - '.php-cs-fixer.dist.php'
+              - 'composer.json'
+              - 'composer.lock'
+              - '**.php'
+
+  object-store-primary-tests-minio:
+    runs-on: ubuntu-latest
+    needs: changes
+
+    if: ${{ github.repository_owner != 'nextcloud-gmbh' && needs.changes.outputs.src != 'false' }}
+
+    strategy:
+      # do not stop on another job's failure
+      fail-fast: false
+      matrix:
+        php-versions: ['8.1']
+        key: ['s3', 's3-multibucket']
+
+    name: php${{ matrix.php-versions }}-${{ matrix.key }}-minio
+
+    services:
+      cache:
+        image: ghcr.io/nextcloud/continuous-integration-redis:latest
+        ports:
+          - 6379:6379/tcp
+        options: --health-cmd="redis-cli ping" --health-interval=10s --health-timeout=5s --health-retries=3
+
+      minio:
+        image: bitnami/minio
+        env:
+          MINIO_ROOT_USER: nextcloud
+          MINIO_ROOT_PASSWORD: bWluaW8tc2VjcmV0LWtleS1uZXh0Y2xvdWQ=
+          MINIO_DEFAULT_BUCKETS: nextcloud
+        ports:
+          - "9000:9000"
+
+    steps:
+      - name: Checkout server
+        uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11
+        with:
+          submodules: true
+
+      - name: Set up php ${{ matrix.php-versions }}
+        uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d #v2.25.2
+        with:
+          php-version: ${{ matrix.php-versions }}
+          extensions: mbstring, fileinfo, intl, sqlite, pdo_sqlite, zip, gd
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+
+      - name: Set up Nextcloud
+        env:
+          OBJECT_STORE: ${{ matrix.key }}
+          OBJECT_STORE_KEY: nextcloud
+          OBJECT_STORE_SECRET: bWluaW8tc2VjcmV0LWtleS1uZXh0Y2xvdWQ=
+        run: |
+          composer install
+          cp tests/redis.config.php config/
+          cp tests/preseed-config.php config/config.php
+          ./occ maintenance:install --verbose --database=sqlite --database-name=nextcloud --database-host=127.0.0.1 --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass password
+          php -f tests/enable_all.php | grep -i -C9999 error && echo "Error during app setup" && exit 1 || exit 0
+
+      - name: Wait for S3
+        run: |
+          sleep 10
+          curl -f -m 1 --retry-connrefused --retry 10 --retry-delay 10 http://localhost:9000/minio/health/ready
+
+      - name: PHPUnit
+        run: composer run test:db
+
+      - name: S3 logs
+        if: always()
+        run: |
+          cat data/nextcloud.log
+          docker ps -a
+          docker ps -aq | while read container ; do IMAGE=$(docker inspect --format='{{.Config.Image}}' $container); echo $IMAGE; docker logs $container; echo "\n\n" ; done
+
+
+  object-store-primary-summary:
+    runs-on: ubuntu-latest-low
+    needs: [changes,object-store-primary-tests-minio]
+
+    if: always()
+
+    steps:
+      - name: Summary status
+        run: if ${{ needs.changes.outputs.src != 'false' && needs.object-store-primary-tests-minio.result != 'success' }}; then exit 1; fi

apps/files_sharing/tests/SharedStorageTest.php

@@ -451,6 +451,7 @@ class SharedStorageTest extends TestCase {
 
 		$sourceStorage = new \OC\Files\Storage\Temporary([]);
 		$sourceStorage->file_put_contents('foo.txt', 'asd');
+		$sourceStorage->getScanner()->scan('');
 
 		$sharedStorage->moveFromStorage($sourceStorage, 'foo.txt', 'bar.txt');
 		$this->assertTrue($sharedStorage->file_exists('bar.txt'));

apps/files_trashbin/tests/StorageTest.php

@@ -8,7 +8,6 @@ namespace OCA\Files_Trashbin\Tests;
 
 use OC\Files\Filesystem;
 use OC\Files\Storage\Common;
-use OC\Files\Storage\Local;
 use OC\Files\Storage\Temporary;
 use OCA\Files_Trashbin\AppInfo\Application;
 use OCA\Files_Trashbin\Events\MoveToTrashEvent;
@@ -673,9 +672,6 @@ class StorageTest extends \Test\TestCase {
 	}
 
 	public function testMoveFromStoragePreserveFileId(): void {
-		if (!$this->userView->getMount('')->getStorage()->instanceOfStorage(Local::class)) {
-			$this->markTestSkipped('Skipping on non-local users storage');
-		}
 		$this->userView->file_put_contents('test.txt', 'foo');
 		$fileId = $this->userView->getFileInfo('test.txt')->getId();
 

lib/private/Files/ObjectStore/ObjectStoreStorage.php

@@ -38,6 +38,7 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil
 
 	private bool $handleCopiesAsOwned;
 	protected bool $validateWrites = true;
+	private bool $preserveCacheItemsOnDelete = false;
 
 	/**
 	 * @param array $params
@@ -171,7 +172,9 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil
 			}
 		}
 
-		$this->getCache()->remove($entry->getPath());
+		if (!$this->preserveCacheItemsOnDelete) {
+			$this->getCache()->remove($entry->getPath());
+		}
 
 		return true;
 	}
@@ -206,7 +209,9 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil
 			}
 			//removing from cache is ok as it does not exist in the objectstore anyway
 		}
-		$this->getCache()->remove($entry->getPath());
+		if (!$this->preserveCacheItemsOnDelete) {
+			$this->getCache()->remove($entry->getPath());
+		}
 		return true;
 	}
 
@@ -596,31 +601,68 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil
 		if (!$sourceCacheEntry) {
 			$sourceCacheEntry = $sourceCache->get($sourceInternalPath);
 		}
-		if ($sourceCacheEntry->getMimeType() === FileInfo::MIMETYPE_FOLDER) {
-			$this->mkdir($targetInternalPath);
-			foreach ($sourceCache->getFolderContentsById($sourceCacheEntry->getId()) as $child) {
-				$this->moveFromStorage($sourceStorage, $child->getPath(), $targetInternalPath . '/' . $child->getName(), $child);
-			}
+
+		$this->copyObjects($sourceStorage, $sourceCache, $sourceCacheEntry);
+		if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
+			/** @var ObjectStoreStorage $sourceStorage */
+			$sourceStorage->setPreserveCacheOnDelete(true);
+		}
+		if ($sourceCacheEntry->getMimeType() === ICacheEntry::DIRECTORY_MIMETYPE) {
 			$sourceStorage->rmdir($sourceInternalPath);
 		} else {
-			$sourceStream = $sourceStorage->fopen($sourceInternalPath, 'r');
-			if (!$sourceStream) {
-				return false;
-			}
-			// move the cache entry before the contents so that we have the correct fileid/urn for the target
-			$this->getCache()->moveFromCache($sourceCache, $sourceInternalPath, $targetInternalPath);
-			try {
-				$this->writeStream($targetInternalPath, $sourceStream, $sourceCacheEntry->getSize());
-			} catch (\Exception $e) {
-				// restore the cache entry
-				$sourceCache->moveFromCache($this->getCache(), $targetInternalPath, $sourceInternalPath);
-				throw $e;
-			}
 			$sourceStorage->unlink($sourceInternalPath);
 		}
+		if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
+			/** @var ObjectStoreStorage $sourceStorage */
+			$sourceStorage->setPreserveCacheOnDelete(false);
+		}
+		$this->getCache()->moveFromCache($sourceCache, $sourceInternalPath, $targetInternalPath);
+
 		return true;
 	}
 
+	/**
+	 * Copy the object(s) of a file or folder into this storage, without touching the cache
+	 */
+	private function copyObjects(IStorage $sourceStorage, ICache $sourceCache, ICacheEntry $sourceCacheEntry) {
+		$copiedFiles = [];
+		try {
+			foreach ($this->getAllChildObjects($sourceCache, $sourceCacheEntry) as $file) {
+				$sourceStream = $sourceStorage->fopen($file->getPath(), 'r');
+				if (!$sourceStream) {
+					throw new \Exception("Failed to open source file {$file->getPath()} ({$file->getId()})");
+				}
+				$this->objectStore->writeObject($this->getURN($file->getId()), $sourceStream, $file->getMimeType());
+				if (is_resource($sourceStream)) {
+					fclose($sourceStream);
+				}
+				$copiedFiles[] = $file->getId();
+			}
+		} catch (\Exception $e) {
+			foreach ($copiedFiles as $fileId) {
+				try {
+					$this->objectStore->deleteObject($this->getURN($fileId));
+				} catch (\Exception $e) {
+					// ignore
+				}
+			}
+			throw $e;
+		}
+	}
+
+	/**
+	 * @return \Iterator<ICacheEntry>
+	 */
+	private function getAllChildObjects(ICache $cache, ICacheEntry $entry): \Iterator {
+		if ($entry->getMimeType() === FileInfo::MIMETYPE_FOLDER) {
+			foreach ($cache->getFolderContentsById($entry->getId()) as $child) {
+				yield from $this->getAllChildObjects($cache, $child);
+			}
+		} else {
+			yield $entry;
+		}
+	}
+
 	public function copy($source, $target): bool {
 		$source = $this->normalizePath($source);
 		$target = $this->normalizePath($target);
@@ -754,4 +796,8 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil
 		$urn = $this->getURN($cacheEntry->getId());
 		$this->objectStore->abortMultipartUpload($urn, $writeToken);
 	}
+
+	public function setPreserveCacheOnDelete(bool $preserve) {
+		$this->preserveCacheItemsOnDelete = $preserve;
+	}
 }

lib/private/Files/Storage/Common.php

@@ -15,6 +15,7 @@ use OC\Files\Cache\Updater;
 use OC\Files\Cache\Watcher;
 use OC\Files\FilenameValidator;
 use OC\Files\Filesystem;
+use OC\Files\ObjectStore\ObjectStoreStorage;
 use OC\Files\Storage\Wrapper\Jail;
 use OC\Files\Storage\Wrapper\Wrapper;
 use OCP\Files\Cache\ICache;
@@ -586,10 +587,21 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage,
 
 		$result = $this->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, true);
 		if ($result) {
-			if ($sourceStorage->is_dir($sourceInternalPath)) {
-				$result = $sourceStorage->rmdir($sourceInternalPath);
-			} else {
-				$result = $sourceStorage->unlink($sourceInternalPath);
+			if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
+				/** @var ObjectStoreStorage $sourceStorage */
+				$sourceStorage->setPreserveCacheOnDelete(true);
+			}
+			try {
+				if ($sourceStorage->is_dir($sourceInternalPath)) {
+					$result = $sourceStorage->rmdir($sourceInternalPath);
+				} else {
+					$result = $sourceStorage->unlink($sourceInternalPath);
+				}
+			} finally {
+				if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
+					/** @var ObjectStoreStorage $sourceStorage */
+					$sourceStorage->setPreserveCacheOnDelete(false);
+				}
 			}
 		}
 		return $result;

tests/preseed-config.php

@@ -38,6 +38,21 @@ if (getenv('OBJECT_STORE') === 's3') {
 			'use_path_style' => true
 		]
 	];
+} elseif (getenv('OBJECT_STORE') === 's3-multibucket') {
+	$CONFIG['objectstore_multibucket'] = [
+		'class' => 'OC\\Files\\ObjectStore\\S3',
+		'arguments' => [
+			'bucket' => 'nextcloud',
+			'autocreate' => true,
+			'key' => getenv('OBJECT_STORE_KEY') ?: 'nextcloud',
+			'secret' => getenv('OBJECT_STORE_SECRET') ?: 'nextcloud',
+			'hostname' => getenv('OBJECT_STORE_HOST') ?: 'localhost',
+			'port' => 9000,
+			'use_ssl' => false,
+			// required for some non amazon s3 implementations
+			'use_path_style' => true
+		]
+	];
 } elseif (getenv('OBJECT_STORE') === 'azure') {
 	$CONFIG['objectstore'] = [
 		'class' => 'OC\\Files\\ObjectStore\\Azure',