Merge pull request #242 from open-runtimes/fix-s3-bucket-prefix

Fix S3 bucket prefix omission for custom endpoints in StorageFactory

Author: ChiragAgg5k

README.md

@@ -92,7 +92,15 @@ OPR_EXECUTOR_RETRY_ATTEMPTS=5
 OPR_EXECUTOR_RETRY_DELAY_MS=500
 ```
 
-> `OPR_EXECUTOR_CONNECTION_STORAGE` takes a DSN string that represents a connection to your storage device. If you would like to use your local filesystem, you can use `file://localhost`. If using S3 or any other provider for storage, use a DSN of the following format `s3://access_key:access_secret@host:port/bucket_name?region=us-east-1`
+> `OPR_EXECUTOR_CONNECTION_STORAGE` takes a DSN string that represents a connection to your storage device. A host is always required in the DSN, even when it is not used to reach the storage backend. For example:
+>
+> | Storage | DSN |
+> |---------|-----|
+> | Local filesystem | `file://localhost` |
+> | AWS S3 | `s3://access_key:access_secret@bucket_name.s3.us-east-1.amazonaws.com?region=us-east-1` |
+> | S3-compatible (MinIO, Garage, etc.) | `s3://access_key:access_secret@localhost/bucket_name?region=us-east-1&url=http%3A%2F%2Fminio%3A9000` |
+>
+> When a host is provided, the executor connects to it directly using the generic S3 device. For AWS S3, use the bucket's virtual-hosted-style endpoint as the host. For S3-compatible providers, pass the URL-encoded endpoint via the `url` parameter and use any placeholder host.
 
 > For backwards compatibility, executor also supports `OPR_EXECUTOR_STORAGE_*` variables as replacement for `OPR_EXECUTOR_CONNECTION_STORAGE`, as seen in [Appwrite repository](https://github.com/appwrite/appwrite/blob/1.3.8/.env#L26-L46).
 

src/Executor/StorageFactory.php

@@ -54,13 +54,17 @@ public static function getDevice(string $root, ?string $connection = ''): Device
 
         switch ($deviceType) {
             case Storage::DEVICE_S3:
+                $bucketRoot = ($bucket === '' || $bucket === '0')
+                    ? $root
+                    : \rtrim($bucket . '/' . \ltrim($root, '/'), '/');
+
                 if ($url !== '' && $url !== '0') {
-                    return new S3($root, $accessKey, $accessSecret, $url, $dsnRegion, $acl);
+                    return new S3($bucketRoot, $accessKey, $accessSecret, $url, $dsnRegion, $acl);
                 }
 
                 if ($host !== '' && $host !== '0') {
                     $host = $insecure ? 'http://' . $host : $host;
-                    return new S3(root: $root, accessKey: $accessKey, secretKey: $accessSecret, host: $host, region: $dsnRegion, acl: $acl);
+                    return new S3(root: $bucketRoot, accessKey: $accessKey, secretKey: $accessSecret, host: $host, region: $dsnRegion, acl: $acl);
                 }
 
                 return new AWS(root: $root, accessKey: $accessKey, secretKey: $accessSecret, bucket: $bucket, region: $dsnRegion, acl: $acl);

tests/unit/Executor/StorageFactoryTest.php

@@ -0,0 +1,82 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Tests\Unit\Executor;
+
+use OpenRuntimes\Executor\StorageFactory;
+use PHPUnit\Framework\TestCase;
+use Utopia\Storage\Device\Local;
+use Utopia\Storage\Device\S3;
+
+final class StorageFactoryTest extends TestCase
+{
+    public function testEmptyConnectionReturnsLocalDevice(): void
+    {
+        $device = StorageFactory::getDevice('/storage/builds/app-test', '');
+
+        $this->assertInstanceOf(Local::class, $device);
+        $this->assertSame('/storage/builds/app-test', $device->getRoot());
+    }
+
+    public function testNullConnectionReturnsLocalDevice(): void
+    {
+        $device = StorageFactory::getDevice('/storage/builds/app-test', null);
+
+        $this->assertInstanceOf(Local::class, $device);
+    }
+
+    public function testInvalidDsnFallsBackToLocalDevice(): void
+    {
+        // Missing host is invalid for a DSN
+        $device = StorageFactory::getDevice('/storage/builds/app-test', 's3://accessKey:secret@/mybucket?region=garage');
+
+        $this->assertInstanceOf(Local::class, $device);
+    }
+
+    public function testS3WithUrlPrependsBucketToRoot(): void
+    {
+        $dsn = 's3://accessKey:secret@localhost/mybucket?region=garage&url=' . \urlencode('http://127.0.0.1:3900');
+        $device = StorageFactory::getDevice('/storage/builds/app-test', $dsn);
+
+        $this->assertSame(S3::class, $device::class);
+        $this->assertSame('mybucket/storage/builds/app-test', $device->getRoot());
+        $this->assertSame('mybucket/storage/builds/app-test/artifact.tar.gz', $device->getPath('artifact.tar.gz'));
+    }
+
+    public function testS3WithUrlAndRootSlashUsesBucketAsRoot(): void
+    {
+        $dsn = 's3://accessKey:secret@localhost/mybucket?region=garage&url=' . \urlencode('http://127.0.0.1:3900');
+        $device = StorageFactory::getDevice('/', $dsn);
+
+        $this->assertSame(S3::class, $device::class);
+        $this->assertSame('mybucket', $device->getRoot());
+    }
+
+    public function testS3WithUrlWithoutBucketKeepsRoot(): void
+    {
+        $dsn = 's3://accessKey:secret@localhost?region=garage&url=' . \urlencode('http://127.0.0.1:3900');
+        $device = StorageFactory::getDevice('/storage/builds/app-test', $dsn);
+
+        $this->assertSame(S3::class, $device::class);
+        $this->assertSame('/storage/builds/app-test', $device->getRoot());
+    }
+
+    public function testS3WithHostPrependsBucketToRoot(): void
+    {
+        $dsn = 's3://accessKey:secret@minio/mybucket?region=us-east-1&insecure=true';
+        $device = StorageFactory::getDevice('/storage/builds/app-test', $dsn);
+
+        $this->assertSame(S3::class, $device::class);
+        $this->assertSame('mybucket/storage/builds/app-test', $device->getRoot());
+    }
+
+    public function testS3WithHostWithoutBucketKeepsRoot(): void
+    {
+        $dsn = 's3://accessKey:secret@mybucket.s3.us-east-1.amazonaws.com?region=us-east-1';
+        $device = StorageFactory::getDevice('/storage/builds/app-test', $dsn);
+
+        $this->assertSame(S3::class, $device::class);
+        $this->assertSame('/storage/builds/app-test', $device->getRoot());
+    }
+}