chore: align main with 0.18.x #155
Conversation
WalkthroughThis pull request upgrades the storage library to PHP 8.1, refactoring the S3 device architecture from bucket-based to host-based initialization. A new AWS device class is introduced with AWS region constants and endpoint mapping. The base Device class gains telemetry scaffolding via constructor and setTelemetry method. A new NotFoundException is added for improved error handling. All storage provider implementations (Backblaze, DigitalOcean Spaces, Linode, Wasabi) are refactored to compute and pass host values to parent constructors. A Telemetry wrapper class delegates device operations while recording execution metrics. Type hints are improved with proper nullable parameter annotations (?string, ?int). The README documentation is condensed and API examples are updated to reflect the new adapter names and methods. Tests are updated with the new AWSTest class and NotFoundException assertions. Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Key changes requiring attentionArchitecture changes:
New public entities:
Device implementations:
Type system improvements:
Error handling:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Storage/S3Base.php (1)
102-105:⚠️ Potential issue | 🟠 MajorMissing abstract declaration for
getAdapterType().Line 104 calls
$this->getAdapterType(), but there is no abstract declaration in this class. The abstract declaration was removed but the method is still called. While all five current S3Base subclasses (AWSTest, WasabiTest, LinodeTest, DOSpacesTest, BackblazeTest) implement this method, the missing abstract declaration breaks the interface contract. A new subclass could omit the implementation without a compile-time error, leading to a runtime failure instead of a clear design error.Restore the abstract method declaration after Line 22:
abstract protected function getAdapterDescription(): string; + /** + * `@return` string + */ + abstract protected function getAdapterType(): string; +src/Storage/Device/S3.php (1)
889-899:⚠️ Potential issue | 🔴 CriticalBug: response body accumulates across retry attempts.
CURLOPT_WRITEFUNCTION(Line 857) appends to$response->body, but the retry loop never resets it. After a failed 5xx attempt whose body is (e.g.) an XML error message, a successful retry will concatenate the new response onto the old, corrupting XML parsing and all downstream consumers.Reset body (and headers, for safety) at the top of each retry iteration:
🐛 Proposed fix
$attempt = 0; while ($attempt < self::$retryAttempts && $response->code >= 500) { usleep(self::$retryDelay * 1000); $attempt++; + $response->body = ''; + $response->headers = []; $result = \curl_exec($curl); $response->code = \curl_getinfo($curl, CURLINFO_HTTP_CODE); }
🤖 Fix all issues with AI agents
In `@Dockerfile`:
- Line 18: The Dockerfile uses an EOL PHP base image ("FROM php:8.1-cli-alpine
as compile"); update that FROM to a supported minor (e.g., php:8.2-cli-alpine or
php:8.3-cli-alpine) across the Dockerfile, then run/build tests and adjust any
extension, ini, or dependency installation commands that assume PHP 8.1 API
changes (verify scripts that reference phpize, docker build args, or extension
names), and update CI/build matrix and any documentation that pins PHP 8.1 to
reflect the new supported PHP version.
In `@README.md`:
- Line 50: Update the README's PHP requirement sentence to match the project's
current minimum (change "PHP 7.4 or later" to "PHP 8.1 or later") so it aligns
with the Dockerfile and composer.json; locate the README line containing "Utopia
Framework requires PHP 7.4 or later" and replace the version number and any
related recommendation text to reference PHP 8.1 as the minimum while keeping
the recommendation to use the latest PHP version.
- Line 3: The build badge in README currently references the wrong repository in
the image URL (it uses utopia-php/ab); update the Markdown badge line so both
the image source and link point to utopia-php/storage (i.e., replace occurrences
of "utopia-php/ab" with "utopia-php/storage" in the badge markup), ensuring the
branch query (branch=master) remains correct or is adjusted to the repository's
default branch if needed.
- Around line 25-27: The example PHP use statements (e.g., use
Utopia\Storage\Device\Local, use Utopia\Storage\Device\S3, use
Utopia\Storage\Device\DOSpaces) are missing trailing semicolons and will not
parse; update each use statement in the README example to end with a semicolon
(for the symbols use Utopia\Storage\Device\Local, use Utopia\Storage\Device\S3,
use Utopia\Storage\Device\DOSpaces) so the snippet is valid PHP.
In `@src/Storage/Compression/Algorithms/LZ4.php`:
- Around line 17-20: The constructor for class LZ4 currently assigns
$this->level directly and bypasses validation; update __construct(int $level =
0) to delegate to the existing setLevel(int $level) method (or perform the same
validation logic) so invalid levels (e.g., 99) are rejected consistently;
replace the direct assignment $this->level = $level with a call to
$this->setLevel($level) to enforce the invariant.
In `@src/Storage/Compression/Algorithms/Zstd.php`:
- Around line 17-20: The Zstd constructor currently assigns $this->level
directly and bypasses validation in setLevel, allowing invalid levels to reach
\zstd_compress; change the constructor in class Zstd to call the existing
setLevel(int $level) method (or otherwise validate using the same 1–22 bounds)
instead of direct assignment so all instances enforce the same level validation
before any compression call.
In `@src/Storage/Compression/Compression.php`:
- Around line 28-38: The abstract methods compress and decompress lack explicit
string return types; update the abstract declarations (abstract public function
compress(string $data) and abstract public function decompress(string $data)) to
declare a return type of string (add ": string") so the abstract class enforces
the same string return contract as its concrete implementations and matches
getName(): string.
In `@src/Storage/Device/AWS.php`:
- Line 50: The SA_EAST_1 constant currently maps to the wrong region string;
update the value of the SA_EAST_1 constant (constant name: SA_EAST_1) to the
correct AWS region identifier 'sa-east-1' so it no longer points to
'eu-north-1'; ensure any usages of AWS::SA_EAST_1 continue to resolve to the
proper South America (São Paulo) region string.
- Line 54: The CN_NORTH_4 constant in class/namespace AWS (symbol CN_NORTH_4)
represents a non-existent AWS China region; either remove the CN_NORTH_4
constant from src/Storage/Device/AWS.php or, if you intentionally added it for
future-proofing, add a clear comment above the const declaration stating that it
is a placeholder for potential future AWS China regions and not an
official/active region (include date/author or TODO), so reviewers understand
the intent.
In `@src/Storage/Device/S3.php`:
- Around line 285-289: The catch block that calls getInfo($path) throws a new
NotFoundException but discards the caught \Throwable $e; update the catch to
pass $e as the previous exception when constructing the NotFoundException so the
original S3 error context is preserved (i.e., throw new NotFoundException('File
not found', 0, $e) or equivalent) referencing the getInfo method and the caught
$e.
- Around line 170-173: The getPath method in the S3 class accepts a $prefix
parameter but ignores it; either incorporate it into the returned path or
remove/document it. Update S3::getPath(string $filename, ?string $prefix = null)
to include the $prefix between the root and filename when provided
(sanitize/trim leading/trailing slashes and use S3-style '/' separators rather
than DIRECTORY_SEPARATOR), e.g. build root + '/' + prefix + '/' + filename;
ensure alignment with the abstract contract's expectations (or remove the
parameter from the signature in both the abstract contract and implementations
if prefix support is not intended).
In `@tests/Storage/Compression/Algorithms/LZ4Test.php`:
- Line 3: The test file's namespace declaration is missing the Storage segment;
update the namespace in LZ4Test.php from "Utopia\Tests\Compression\Algorithms"
to "Utopia\Tests\Storage\Compression\Algorithms" so it matches the directory
structure and other compression tests (e.g., ZstdTest, XZTest) and resolves
PSR-4 autoloading errors.
🧹 Nitpick comments (17)
composer.json (1)
29-29: Version constraint forutopia-php/systemis inconsistently broad.The constraint
"0.*.*"allows any0.x.yrelease, whereas other dependencies in this file use tighter scoping (e.g.,"0.2.*","0.1.*"). Under semantic versioning,0.xreleases carry no backward-compatibility guarantees between minor versions, so a jump from0.9.xto0.10.xcould introduce breaking changes. Clarify whether the broader constraint is intentional to support multiple minor versions, or align with the pattern used for other0.x.*dependencies.src/Storage/Compression/Algorithms/GZIP.php (1)
28-42:gzencodeandgzdecodereturnstring|false— afalsereturn will throw aTypeErrorat runtime.Both functions return
falseon failure. Since the return type is declared as: string, PHP will raise aTypeErrorrather than surfacing a descriptive error. The same pattern exists in sibling algorithms (XZ, Snappy), so this is consistent, but worth noting.src/Storage/Compression/Algorithms/LZ4.php (1)
64-78:lz4_compress/lz4_uncompresscan returnfalseon failure.Same pattern as the other algorithms —
falsewill trigger aTypeErrordue to the: stringreturn type. Consistent with siblings but worth noting.src/Storage/Device/Backblaze.php (1)
37-38: Simplify the host string concatenation.The concatenation is overly fragmented. Consider simplifying for readability, consistent with DOSpaces and Linode patterns.
Suggested simplification
- $host = $bucket.'.'.'s3'.'.'.$region.'.backblazeb2.com'; + $host = $bucket.'.s3.'.$region.'.backblazeb2.com';src/Storage/Device/Wasabi.php (1)
44-45: Simplify the host string concatenation.Same fragmentation issue as Backblaze. This is hard to read and error-prone.
Suggested simplification
- $host = $bucket.'.'.'s3'.'.'.$region.'.'.'wasabisys'.'.'.'com'; + $host = $bucket.'.s3.'.$region.'.wasabisys.com';tests/Storage/Compression/Algorithms/GZIPTest.php (1)
10-13: Minor style inconsistency with other test classes.This uses
protected $object = nullwhileXZTestusesprotected XZ $object(typed property without default). Consider aligning the declaration style across test suites for consistency.src/Storage/Compression/Algorithms/Brotli.php (1)
80-80: Stray comment artifact on assignment.
$this->level = $level; // $level;has a leftover comment that looks like a copy-paste artifact.Cleanup
- $this->level = $level; // $level; + $this->level = $level;src/Storage/Device/Local.php (1)
273-280: Consider aligning exception types for file-not-found scenarios.The
read()method now correctly usesNotFoundExceptionfor file-not-found cases. Thetransfer()method (line 214) anddelete()method (line 252) should use the sameNotFoundExceptionfor consistency, sinceNotFoundExceptionis already imported and available.src/Storage/Compression/Algorithms/XZ.php (1)
23-37: Add error handling for compression/decompression failures.
\xzencode()and\xzdecode()returnfalseon failure, which violates the declaredstringreturn type and would cause aTypeErrorat runtime. This gap is consistent across all compression algorithm implementations in the codebase (GZIP, Snappy, Brotli, LZ4, Zstd), and the test suite covers only happy paths.Proposed defensive handling
public function compress(string $data): string { - return \xzencode($data); + $result = \xzencode($data); + if ($result === false) { + throw new \RuntimeException('Failed to compress data with xz'); + } + return $result; } public function decompress(string $data): string { - return \xzdecode($data); + $result = \xzdecode($data); + if ($result === false) { + throw new \RuntimeException('Failed to decompress data with xz'); + } + return $result; }tests/Storage/Compression/Algorithms/BrotliTest.php (2)
36-48: SwappedassertEqualsargument order — expected should be first.PHPUnit's
assertEqualssignature isassertEquals($expected, $actual). Throughout this test file, the arguments are reversed (e.g.,assertEquals($demoSize, 21)instead ofassertEquals(21, $demoSize)). This won't break the test, but failure messages will display "expected" and "actual" values backwards, making debugging confusing. Note thatZstdTestalready uses the correct order.♻️ Example fix for testCompressDecompressWithText
- $this->assertEquals($demoSize, 21); - $this->assertEquals($dataSize, 25); + $this->assertEquals(21, $demoSize); + $this->assertEquals(25, $dataSize); - $this->assertEquals($this->object->decompress($data), $demo); + $this->assertEquals($demo, $this->object->decompress($data));The same pattern applies to all other
assertEqualscalls in this file (lines 59-60, 67, 80, 82, 87, 99, 101, 106).
71-88: Missing assertion that decompressed image data matches original.
testCompressDecompressWithJPGImagedecompresses and checks the size but never asserts that the decompressed content equals the original$demo. The large-text test (line 68) does this correctly. The same gap exists intestCompressDecompressWithPNGImage(lines 103-107).🧪 Proposed fix
$this->assertEquals($dataSize, 599639); + $this->assertEquals($data, $demo); }And similarly for the PNG test.
tests/Storage/Compression/Algorithms/ZstdTest.php (2)
40-57: Missing content-equality assertion after decompression.
testCompressDecompressWithLargeTextverifies the decompressed size but never asserts that the decompressed content equals the original$demo. The same gap exists in the JPG (line 72-75) and PNG (line 91-94) tests. Compare withBrotliTestline 68, which correctly asserts$data === $demo.🧪 Proposed fix for large-text test
$this->assertEquals($dataSize, 386795); + $this->assertEquals($data, $demo); }Apply the same pattern to the JPG and PNG tests.
8-24: MissingtestErrorsWhenSettingLevelfor Zstd.
BrotliTestincludes atestErrorsWhenSettingLeveltest. TheZstdclass also validates levels (1–22) and throwsInvalidArgumentException, but this test suite doesn't cover that path. Consider adding a similar test for completeness.tests/Storage/Compression/Algorithms/SnappyTest.php (1)
44-80: Missing content-equality assertion and large-text test.Both image tests (JPG at line 57-60, PNG at line 76-79) verify the decompressed size but never assert that decompressed content matches the original. Additionally, unlike the other compression test suites (Brotli, Zstd, LZ4), there is no
testCompressDecompressWithLargeTexttest method here.🧪 Proposed fix for JPG test
$this->assertEquals(599639, $dataSize); + $this->assertEquals($demo, $data); }Apply similarly to the PNG test.
tests/Storage/Compression/Algorithms/LZ4Test.php (1)
40-76: Missing content-equality assertion after decompression in image tests.Same gap as the other compression test files — decompressed size is checked but content is not compared against the original.
src/Storage/Device/Telemetry.php (2)
10-13: Constructor property promotion ordering is correct but subtle.The promoted property
private Device $underlyingis assigned beforeparent::__construct($telemetry)executes (PHP 8.1+ behavior). SinceDevice::__constructcalls$this->setTelemetry(), which is overridden here to access$this->underlying(Line 18), the ordering matters. This works correctly, but a brief inline comment would help future readers understand the dependency.
21-35: Add test coverage for the&...$argsby-reference propagation inmeasure().The variadic by-reference pattern (
&...$argswith dynamic dispatch via...) is valid and correctly preserves reference semantics in PHP 8.1+, soarray &$metadatamodifications inside underlying device methods will propagate back to callers as intended. However, this pattern is untested in the codebase and relatively advanced, making it fragile against future maintenance. Consider adding explicit tests that verifyupload()anduploadData()correctly modify$metadata['uploadId']and$metadata['parts']when called through the Telemetry wrapper, ensuring the reference propagation behavior is captured and maintained.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Storage/Device/S3.php (2)
904-914:⚠️ Potential issue | 🔴 CriticalBug: response body accumulates across retries.
The
CURLOPT_WRITEFUNCTIONcallback (line 872) appends to$response->body, but the retry loop never resets$response->body(or$response->headers) between attempts. On a 5xx retry, the new response data is concatenated to the previous failed response, corrupting the body and likely causing XML parse failures or wrong data on eventual success.Proposed fix
$attempt = 0; while ($attempt < self::$retryAttempts && $response->code >= 500) { usleep(self::$retryDelay * 1000); $attempt++; + $response->body = ''; + $response->headers = []; $result = \curl_exec($curl); $response->code = \curl_getinfo($curl, CURLINFO_HTTP_CODE); }
866-870:⚠️ Potential issue | 🟡 MinorUse strict comparison for null check.
Line 868 uses
!=(loose comparison) to check$this->curlHttpVersionagainstnull. This would also treat0as null-like. Use!==for correctness.Proposed fix
- if ($this->curlHttpVersion != null) { + if ($this->curlHttpVersion !== null) {
🤖 Fix all issues with AI agents
In `@composer.json`:
- Line 24: The dependency constraint "utopia-php/system": "0.*.*" in
composer.json is too loose and may pull incompatible pre-1.0 releases; change it
to a tighter minor series you have tested (for example "0.9.*" or the specific
minor used by the 0.18.x branch) so it matches the pattern used by other Utopia
deps (e.g., "0.2.*"); update the composer.json entry for utopia-php/system
accordingly and run composer update/install to verify the lockfile and CI pass.
🧹 Nitpick comments (5)
composer.json (1)
29-31: Consider upgrading dev tooling for PHP 8.1 compatibility.
vimeo/psalmat4.0.1is quite dated and may not fully support PHP 8.1 features (e.g., enums, intersection types, fibers). Psalm 5.x+ has proper PHP 8.1 support. Not blocking, but worth tracking for a follow-up.README.md (2)
3-3: Travis CI badge is likely non-functional.Travis CI has sunset
travis-ci.organd largely deprecated their free tier. The badge image URL uses.orgwhile the link uses.com. Consider switching to GitHub Actions badges or another active CI service if applicable.
7-7: Minor grammar polish.The sentence is a bit run-on with repeated sentence starters ("It supports…", "We already support…", "This library is aiming…"). Consider condensing.
Suggested rewrite
-Utopia Storage library is simple and lite library for managing application storage. It supports multiple storage adapters. We already support AWS S3 storage, Digitalocean Spaces storage, Backblaze B2 Cloud storage, Linode Object storage and Wasabi Cloud storage. This library is aiming to be as simple and easy to learn and use. This library is maintained by the [Appwrite team](https://appwrite.io). +Utopia Storage is a lightweight library for managing application storage across multiple adapters, including AWS S3, DigitalOcean Spaces, Backblaze B2, Linode Object Storage, and Wasabi. Maintained by the [Appwrite team](https://appwrite.io).src/Storage/Device/S3.php (2)
955-974:parseAndThrowS3Error— silent catch of XML parse failures could hide issues.When
simplexml_load_stringfails (e.g., malformed XML that starts with<?xml), the\Throwablecatch on line 968 silently discards the parse error and falls through to throw a genericExceptionwith the raw body. This is acceptable behavior, but consider logging or including the parse failure context for debuggability.
926-929: Complex XML detection condition — consider extracting for readability.The condition on line 926 checks content-type, XML prolog, and SVG exclusion in a single expression. It works but is hard to scan. A small helper or descriptive boolean variable would improve clarity.
Example
- if ($decode && ((isset($response->headers['content-type']) && $response->headers['content-type'] == 'application/xml') || (str_starts_with($response->body, '<?xml') && ($response->headers['content-type'] ?? '') !== 'image/svg+xml'))) { + $contentType = $response->headers['content-type'] ?? ''; + $isXmlResponse = $contentType === 'application/xml' + || (str_starts_with($response->body, '<?xml') && $contentType !== 'image/svg+xml'); + + if ($decode && $isXmlResponse) {
ChiragAgg5k
force-pushed
the
chore/reset-main-to-0-18-x
branch
from
8131407 to
e883679
Compare
2 participants
Summary by CodeRabbit
Release Notes
New Features
uploadDatamethod for direct data uploads.Chores
Bug Fixes