0.18.x #149
0.18.x #149
Conversation
Feat: s3 retry
S3 retry backported to
Retry if error code is >= 500
Feat: ZIP file extension
feat(s3): add generic S3 device
feat: implement telemetry
fix(telemetry): setTelemetry on underlying device
… uses by-value instead
fix(telemetry): func_get_args doesn't forward arguments by reference, uses by-value instead
fix: exists method to check for dirs as well
fix: Allow hyphen and underscore in filename
Add explicit nullable type hints to parameters with null defaults: - getPath(): Change string $prefix = null to ?string $prefix = null - read(): Change int $length = null to ?int $length = null This resolves deprecation warnings in PHP 8.1+ where implicitly marking parameters as nullable is deprecated.
Fix nullable parameter deprecation warnings
Bump telemetry
WalkthroughThis PR upgrades PHP to 8.1 (Docker and composer), adds ext-curl and ext-simplexml requirements, and updates composer plugin config. It introduces telemetry: Device gains telemetry wiring and a Histogram; a Telemetry device wrapper records timings. S3 is refactored to use host/fqdn-based requests, adds retry configuration, improved error parsing, NotFoundException usage, and multipart upload changes. A new AWS device class is added. Several providers (Backblaze, DOSpaces, Linode, Wasabi, Local) shift host initialization into constructors; Local adds idempotent chunk handling and throws NotFoundException on read. Validators and tests updated accordingly. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Nitpick comments (9)
src/Storage/Device/Wasabi.php (1)
44-45: Simplify the host string concatenation.The host construction is functionally correct but uses excessive concatenation operators. Consider simplifying for better readability.
🔎 Proposed simplification
- $host = $bucket.'.'.'s3'.'.'.$region.'.'.'wasabisys'.'.'.'com'; + $host = $bucket.'.s3.'.$region.'.wasabisys.com'; parent::__construct($root, $accessKey, $secretKey, $host, $region, $acl);src/Storage/Device/Linode.php (1)
32-33: Simplify the host string concatenation.The host construction uses unnecessary concatenation. Consider simplifying for consistency and readability.
🔎 Proposed simplification
- $host = $bucket.'.'.$region.'.'.'linodeobjects.com'; + $host = $bucket.'.'.$region.'.linodeobjects.com'; parent::__construct($root, $accessKey, $secretKey, $host, $region, $acl);src/Storage/Device/Backblaze.php (1)
37-38: Consider simplifying string concatenation.The host construction works correctly but has unnecessary concatenation with separate dot strings. This can be simplified for readability.
🔎 Proposed simplification
- $host = $bucket.'.'.'s3'.'.'.$region.'.backblazeb2.com'; + $host = $bucket.'.s3.'.$region.'.backblazeb2.com';tests/Storage/Device/LocalTest.php (2)
188-189: Comment references AWS S3 in a Local storage test.This comment about AWS S3's 5MB chunk requirement is copied from S3 tests but may be misleading here since this tests
Localstorage which doesn't have such requirements. Consider updating the comment or using a different chunk size rationale.
196-210: Redundant variable assignments.The variable
$opis assigned at line 196, then reassigned identically inside the loop at line 199. This pattern is repeated in the second loop. Consider removing the redundant assignments inside the loops.🔎 Proposed cleanup
$op = __DIR__.'/chunkx.part'; while ($start < $totalSize) { $contents = fread($handle, $chunkSize); - $op = __DIR__.'/chunkx.part'; $cc = fopen($op, 'wb');src/Storage/Device/AWS.php (1)
64-65: Docblock says "S3 Constructor" but this is the AWS class.Minor documentation inconsistency - the docblock refers to "S3 Constructor" but this is the
AWSclass constructor.🔎 Proposed fix
/** - * S3 Constructor + * AWS Constructor *src/Storage/Device/S3.php (2)
195-198: Missing return type annotation.
setRetryAttemptslacks an explicit: voidreturn type, whilesetRetryDelayhas one. Add the return type for consistency.🔎 Proposed fix
- public static function setRetryAttempts(int $attempts) + public static function setRetryAttempts(int $attempts): void { self::$retryAttempts = $attempts; }
920-927: Consider renamingattemptstoretriesfor clarity.The
attemptsattribute tracks the number of retries (0 = no retries, 1 = one retry). Renaming toretrieswould make this semantically clearer in telemetry dashboards.src/Storage/Device/Telemetry.php (1)
57-60: Use explicit nullable type syntax.The parameter types should use explicit nullable syntax (
?string,?int) to match the base class signatures and follow PHP 8.1+ best practices.🔎 Proposed fix
- public function getPath(string $filename, string $prefix = null): string + public function getPath(string $filename, ?string $prefix = null): string { return $this->measure(__FUNCTION__, $filename, $prefix); }- public function read(string $path, int $offset = 0, int $length = null): string + public function read(string $path, int $offset = 0, ?int $length = null): string { return $this->measure(__FUNCTION__, $path, $offset, $length); }Also applies to: 77-80
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/workflows/tests.ymlDockerfilecomposer.jsonsrc/Storage/Device.phpsrc/Storage/Device/AWS.phpsrc/Storage/Device/Backblaze.phpsrc/Storage/Device/DOSpaces.phpsrc/Storage/Device/Linode.phpsrc/Storage/Device/Local.phpsrc/Storage/Device/S3.phpsrc/Storage/Device/Telemetry.phpsrc/Storage/Device/Wasabi.phpsrc/Storage/Exception/NotFoundException.phpsrc/Storage/Storage.phpsrc/Storage/Validator/FileExt.phpsrc/Storage/Validator/FileName.phptests/Storage/Device/AWSTest.phptests/Storage/Device/LocalTest.phptests/Storage/S3Base.phptests/Storage/Validator/FileExtTest.phptests/Storage/Validator/FileNameTest.php
🧰 Additional context used
🧬 Code graph analysis (13)
src/Storage/Exception/NotFoundException.php (1)
src/Storage/Storage.php (1)
Storage(7-126)
tests/Storage/S3Base.php (1)
src/Storage/Exception/NotFoundException.php (1)
NotFoundException(7-9)
src/Storage/Device/Linode.php (4)
src/Storage/Device/S3.php (1)
__construct(113-131)src/Storage/Device/Backblaze.php (1)
__construct(35-39)src/Storage/Device/DOSpaces.php (1)
__construct(34-38)src/Storage/Device/Wasabi.php (1)
__construct(42-46)
src/Storage/Device/Wasabi.php (2)
src/Storage/Device/S3.php (1)
__construct(113-131)src/Storage/Device/AWS.php (1)
__construct(74-81)
src/Storage/Device/AWS.php (2)
src/Storage/Storage.php (1)
Storage(7-126)src/Storage/Device/S3.php (4)
S3(10-986)getName(136-139)getType(144-147)getDescription(152-155)
src/Storage/Device/DOSpaces.php (1)
src/Storage/Device/S3.php (1)
__construct(113-131)
src/Storage/Device/Local.php (1)
src/Storage/Exception/NotFoundException.php (1)
NotFoundException(7-9)
tests/Storage/Device/LocalTest.php (2)
src/Storage/Device/AWS.php (1)
AWS(7-106)src/Storage/Exception/NotFoundException.php (1)
NotFoundException(7-9)
tests/Storage/Validator/FileNameTest.php (1)
src/Storage/Validator/FileName.php (1)
isValid(23-38)
src/Storage/Device/Telemetry.php (1)
src/Storage/Device.php (24)
Device(10-339)__construct(24-27)setTelemetry(77-84)getType(66-66)getName(57-57)getRoot(93-93)getPath(104-104)upload(121-121)uploadData(139-139)abort(148-148)read(158-158)transfer(169-169)write(178-178)delete(211-211)deletePath(220-220)exists(228-228)getFileSize(238-238)getFileMimeType(248-248)getFileHash(258-258)createDirectory(268-268)getDirectorySize(280-280)getPartitionFreeSpace(289-289)getPartitionTotalSpace(298-298)getFiles(308-308)
src/Storage/Device.php (1)
src/Storage/Device/Telemetry.php (5)
Telemetry(8-146)__construct(10-13)setTelemetry(15-19)getPath(57-60)read(77-80)
src/Storage/Device/Backblaze.php (2)
src/Storage/Device/S3.php (1)
__construct(113-131)src/Storage/Device/AWS.php (1)
__construct(74-81)
src/Storage/Device/S3.php (1)
src/Storage/Exception/NotFoundException.php (1)
NotFoundException(7-9)
🔇 Additional comments (33)
.github/workflows/tests.yml (1)
61-61: LGTM!The rename from
S3TesttoAWSTestaligns with the introduction of the new AWS device class. The S3 environment variables are appropriately retained since AWS uses S3-compatible storage APIs. TheAWSTest.phptest file exists and is properly referenced in the matrix.Dockerfile (3)
116-116: Clarify the purpose of the CMD directive.The addition of
CMD [ "tail", "-f", "/dev/null" ]keeps the container running indefinitely without performing any work. This pattern is typically used for development, testing, or CI/CD scenarios where the container needs to remain active for interactive debugging or testing.If this image is intended for production use where the storage library should be invoked as part of another service, consider whether this CMD is appropriate or if it should be removed to allow the consuming application to define its own entrypoint.
107-111: Extension API version correctly updated for PHP 8.1.The extension paths have been correctly updated from API version 20200930 (PHP 8.0) to 20210902 (PHP 8.1). This change is consistent across all five extensions.
18-18: PHP 8.1 compatibility is already in place.The
composer.jsonalready specifies"php": ">=8.1", confirming the project targets PHP 8.1 as minimum. The extension API version (20210902) is correct for PHP 8.1. A scan of the source code found no deprecated patterns (such asstrptime,strftime,Serializableinterface, or passing null to non-nullable parameters) that would conflict with PHP 8.1. All custom compression extensions compile correctly for the target PHP version.composer.json (3)
24-24: Verify that the new extension requirements are utilized in the codebase.The addition of
ext-curlandext-simplexmlaligns with the S3/AWS device refactoring mentioned in the PR summary. However, please verify these extensions are actually used in the implementation.#!/bin/bash # Description: Verify usage of curl and simplexml extensions in the codebase. # Search for curl function calls echo "=== Checking for curl usage ===" rg -n --type=php '\b(curl_init|curl_exec|curl_setopt|curl_close|curl_multi_)\w*\s*\(' -C2 # Search for simplexml function calls echo "=== Checking for simplexml usage ===" rg -n --type=php '\b(simplexml_load_string|simplexml_load_file|SimpleXMLElement)\b' -C2Also applies to: 26-26
31-31: Telemetry dependency properly integrated and actively utilized.The
utopia-php/telemetry: 0.2.*addition is correctly implemented through a Decorator pattern wrapper class (src/Storage/Device/Telemetry.php) that instruments all storage operations. The base Device class accepts the Adapter with a no-op default, and creates a histogram to measure operation latency with appropriate bucket boundaries. All major storage operations (upload, read, write, delete, etc.) are instrumented to record execution time and operation metadata.
39-44: This configuration follows Composer security best practices.Disabling
php-http/discoveryandtbachert/spiplugin discovery is the correct approach. Best practice is to use a deny-by-default posture: explicitly allow only Composer plugins you trust and actually need. These plugins execute code duringcomposer install/update, so disabling them reduces supply-chain risk. If the project requires automatic HTTP client discovery, it should be handled via explicit pinning incomposer.jsonextra configuration rather than auto-discovery.src/Storage/Storage.php (1)
16-16: LGTM!The new
DEVICE_AWS_S3constant follows the established naming pattern and provides a distinct device type for AWS-specific S3 storage.src/Storage/Validator/FileExt.php (1)
19-19: LGTM!The
TYPE_ZIPconstant follows the existing pattern and enables ZIP file validation support.tests/Storage/Validator/FileExtTest.php (1)
17-40: LGTM!The test coverage correctly validates the new ZIP extension support:
file.zipis properly recognized as validfile.7zipis correctly rejected (different extension)tests/Storage/Validator/FileNameTest.php (1)
32-34: LGTM!The new test cases properly validate the expanded character support in file names (dashes, underscores, and combinations with dots), aligning with the validator implementation.
src/Storage/Exception/NotFoundException.php (1)
1-9: LGTM!The new
NotFoundExceptionprovides specific exception handling for not-found scenarios in storage operations, improving error granularity over generic exceptions.src/Storage/Device/DOSpaces.php (1)
36-37: LGTM!The refactored host construction aligns with the parent S3 constructor's signature and follows the same pattern as other storage devices in this PR, providing cleaner initialization.
src/Storage/Validator/FileName.php (1)
18-35: LGTM!The updated validation logic correctly expands the allowed character set while properly rejecting filenames that contain only special characters or other invalid characters.
tests/Storage/Device/LocalTest.php (2)
79-83: LGTM!Good addition of test coverage for the
NotFoundExceptionwhen attempting to read a non-existent file.
323-323: LGTM!The transfer tests are correctly updated to use the new
AWSdevice class.Also applies to: 342-342
tests/Storage/S3Base.php (3)
137-141: LGTM!Good test coverage for
NotFoundExceptionon reading non-existent files from S3-compatible storage.
304-371: Test validates multipart upload retry semantics.The test correctly validates that re-uploading chunks from the beginning doesn't cause issues. However, note the same redundant
$opassignments as inLocalTest.php(lines 324/327 and 346/349).
420-430: LGTM!Good addition of test coverage for transfer operations on non-existent source files.
src/Storage/Device/AWS.php (1)
76-81: LGTM!The host construction correctly handles Chinese AWS regions with the
.amazonaws.cndomain suffix.tests/Storage/Device/AWSTest.php (1)
5-36: LGTM!The test class is correctly updated to use the new
AWSdevice class with appropriate adapter name and description.src/Storage/Device/Local.php (3)
22-26: LGTM!The
parent::__construct()call ensures proper initialization of the baseDeviceclass, including telemetry setup.
100-119: Idempotent chunk upload implementation looks correct.The logic correctly prevents duplicate log entries when a chunk is re-uploaded by checking if the chunk file already exists. This ensures multipart uploads can be safely retried.
275-277: LGTM!Appropriately using
NotFoundExceptionfor missing file reads, providing more precise error handling.src/Storage/Device.php (2)
22-27: LGTM!The telemetry integration is well-structured with a sensible default of
NoTelemetry(no-op adapter), ensuring backward compatibility.
77-84: Good histogram configuration for storage operation metrics.The bucket boundaries (5ms to 10s) are appropriate for storage operations, covering both fast local operations and slower remote/S3 operations.
src/Storage/Device/S3.php (4)
51-53: Static retry config affects all S3 instances globally.These static properties are shared across all S3-derived instances (S3, AWS, Backblaze, DOSpaces, etc.). Consider whether per-instance configuration would be more appropriate for multi-tenant scenarios.
113-131: LGTM!The host-based construction logic correctly handles both protocol-prefixed and bare hostnames, appropriately setting
fqdnand thehostheader.
260-271: LGTM!The re-upload detection using
array_key_existscorrectly prevents double-counting chunks when retrying failed part uploads.
940-959: LGTM!The error parsing correctly identifies
NoSuchKeyerrors and throws the appropriateNotFoundException, with proper re-throw handling and graceful fallback for XML parsing failures.src/Storage/Device/Telemetry.php (3)
10-19: LGTM!The telemetry propagation to both the wrapper and underlying device ensures consistent instrumentation.
21-35: LGTM!The
measurehelper correctly uses by-reference variadic parameters to support methods likeupload()that modify their$metadataargument. Thefinallyblock ensures telemetry is recorded regardless of success or failure.
132-140: LGTM!Parameterless methods correctly delegate through
measure()with an empty argument list.
| * @return string | ||
| */ | ||
| abstract public function getPath(string $filename, string $prefix = null): string; | ||
| abstract public function getPath(string $filename, ?string $prefix = null): string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the abstract method signatures from Device.php
rg -n "abstract public function" src/Storage/Device.php -A 1
# Get the implementation signatures from Telemetry.php
rg -n "public function" src/Storage/Device/Telemetry.php -A 1Repository: utopia-php/storage
Length of output: 3996
Align type declarations with abstract method signatures in Telemetry wrapper class.
The abstract method signatures declare nullable types (?string $prefix = null at line 104 and ?int $length = null at line 158), but the Telemetry wrapper class (src/Storage/Device/Telemetry.php lines 57 and 77) declares these parameters without nullable type hints (string $prefix = null and int $length = null). Ensure the wrapper implementation matches the abstract class declarations for consistent type safety.
🤖 Prompt for AI Agents
In src/Storage/Device.php around lines 104 and 158 the abstract methods declare
nullable parameters (?string $prefix = null and ?int $length = null); update the
Telemetry wrapper at src/Storage/Device/Telemetry.php (around lines 57 and 77)
to match those signatures by changing the parameter type hints to nullable
(?string $prefix = null and ?int $length = null), keep the default null, and
adjust any internal usage or docblocks accordingly to respect the nullable
types.
|
|
||
| const EU_NORTH_1 = 'eu-north-1'; | ||
|
|
||
| const SA_EAST_1 = 'eu-north-1'; |
There was a problem hiding this comment.
Incorrect region value for SA_EAST_1.
The constant SA_EAST_1 is assigned 'eu-north-1' but should be 'sa-east-1'. This appears to be a copy-paste error from line 48.
🔎 Proposed fix
- const SA_EAST_1 = 'eu-north-1';
+ const SA_EAST_1 = 'sa-east-1';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SA_EAST_1 = 'eu-north-1'; | |
| const SA_EAST_1 = 'sa-east-1'; |
🤖 Prompt for AI Agents
In src/Storage/Device/AWS.php around line 50, the constant SA_EAST_1 is
incorrectly set to 'eu-north-1'; change its value to the correct AWS region
identifier 'sa-east-1' to fix the copy-paste error and ensure region constants
map to the proper AWS regions.
| $attempt = 0; | ||
| while ($attempt < self::$retryAttempts && $response->code >= 500) { | ||
| usleep(self::$retryDelay * 1000); | ||
| $attempt++; | ||
| $result = \curl_exec($curl); | ||
| $response->code = \curl_getinfo($curl, CURLINFO_HTTP_CODE); | ||
| } |
There was a problem hiding this comment.
Response body accumulates across retries.
The retry loop re-executes curl_exec, but $response->body is not reset between attempts. Since CURLOPT_WRITEFUNCTION appends to $response->body, retries will concatenate all response bodies, corrupting the final result.
🔎 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);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $attempt = 0; | |
| while ($attempt < self::$retryAttempts && $response->code >= 500) { | |
| usleep(self::$retryDelay * 1000); | |
| $attempt++; | |
| $result = \curl_exec($curl); | |
| $response->code = \curl_getinfo($curl, CURLINFO_HTTP_CODE); | |
| } | |
| $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); | |
| } |
🤖 Prompt for AI Agents
In src/Storage/Device/S3.php around lines 893 to 899, the retry loop re-calls
curl_exec but never clears the previous response data so CURLOPT_WRITEFUNCTION
keeps appending bodies across attempts; before each retry execution reset
$response->body (and any related header/length state used by the write callback)
to an empty value so each attempt starts with a fresh buffer, then re-run
curl_exec and update the HTTP code as shown.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.