fix: deployment download bugs and range clamping in Appwrite source#152
fix: deployment download bugs and range clamping in Appwrite source#152
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRemoved the normalization that lowercased computed resource identifiers in src/Migration/Transfer.php so computedResources now preserves original casing. Updated public constant string values in src/Migration/Resource.php from kebab-case to camelCase (site-deployment → siteDeployment, site-variable → siteVariable, environment-variable → environmentVariable). In src/Migration/Sources/Appwrite.php: replaced Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Migration/Resource.php`:
- Around line 55-61: The constants in Resource (TYPE_SITE_DEPLOYMENT,
TYPE_SITE_VARIABLE, TYPE_HASH, TYPE_ENVIRONMENT_VARIABLE) use camelCase string
values but must be snake_case; change their assigned values to
'site_deployment', 'site_variable', 'hash' (keep as-is if already correct), and
'environment_variable' respectively, and update any consuming code/tests to use
the new snake_case string values if they reference the literal strings instead
of the constants.
- Around line 55-61: The public constants TYPE_SITE_DEPLOYMENT,
TYPE_SITE_VARIABLE, TYPE_HASH, and TYPE_ENVIRONMENT_VARIABLE in Resource.php
were changed from kebab-case to camelCase which will break external consumers
and the strtolower() call in Transfer.php (around line 235) will produce
unexpected values; restore the original kebab-case literal values
('site-deployment', 'site-variable', 'hash', 'environment-variable') in
Resource.php OR alternatively update the normalization in Transfer.php to
explicitly convert camelCase to kebab-case (e.g., insert a camelCase->kebab-case
transformation before strtolower) so external serialized outputs remain
unchanged.
premtsd-code
force-pushed
the
fix-kebab-case-site
branch
from
79cab8e to
c8260df
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Migration/Resource.php (1)
55-61:⚠️ Potential issue | 🔴 CriticalConstants still use camelCase — PR objective requires snake_case.
The three changed values are still camelCase (
'siteDeployment','siteVariable','environmentVariable'), contradicting the PR's own stated objective:"Updated Resource.php type constants:
site-deployment→site_deployment,site-variable→site_variable,environment-variable→environment_variable."The
strtolower()concern from the earlier review is now moot since Transfer.php removed that normalization, but the intended literal values in this file are still wrong.🐛 Proposed fix: align values with the PR's stated snake_case objective
- public const TYPE_SITE_DEPLOYMENT = 'siteDeployment'; + public const TYPE_SITE_DEPLOYMENT = 'site_deployment'; - public const TYPE_SITE_VARIABLE = 'siteVariable'; + public const TYPE_SITE_VARIABLE = 'site_variable'; - public const TYPE_ENVIRONMENT_VARIABLE = 'environmentVariable'; + public const TYPE_ENVIRONMENT_VARIABLE = 'environment_variable';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Migration/Resource.php` around lines 55 - 61, The type constants in Resource.php still use camelCase values; update the constant values for TYPE_SITE_DEPLOYMENT, TYPE_SITE_VARIABLE, and TYPE_ENVIRONMENT_VARIABLE to use snake_case literals ('site_deployment', 'site_variable', 'environment_variable') so they match the PR objective and other code expecting those exact strings; leave the constant names unchanged and only change the right-hand string literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Migration/Resource.php`:
- Around line 55-61: The type constants in Resource.php still use camelCase
values; update the constant values for TYPE_SITE_DEPLOYMENT, TYPE_SITE_VARIABLE,
and TYPE_ENVIRONMENT_VARIABLE to use snake_case literals ('site_deployment',
'site_variable', 'environment_variable') so they match the PR objective and
other code expecting those exact strings; leave the constant names unchanged and
only change the right-hand string literals.
…eploymentData - Use lowercase 'content-length' header key (Target::call stores headers lowercase via strtolower) - Use strlen instead of mb_strlen for binary file data (mb_strlen misinterprets gzip bytes as UTF-8) - Cap $end to $fileSize - 1 before first chunked Range request (server rejects $end >= $size with 416)
This reverts commit 987d11c.
This reverts commit c8260df.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Migration/Sources/Appwrite.php (1)
1927-1927: Same header-casing concern as inexportDeploymentData.This has the same dependency on
$this->call()returning lowercase header keys. See the earlier comment on line 1708.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Migration/Sources/Appwrite.php` at line 1927, The check for 'content-length' assumes $responseHeaders keys are lowercase (same issue as in exportDeploymentData); update the handling in the code path that uses $responseHeaders (the conditional around array_key_exists('content-length', $responseHeaders)) to perform a case-insensitive header lookup or normalize header keys (e.g., map keys to lowercase before checking) so the check works regardless of $this->call()'s header casing; ensure you apply the same normalization approach used/fixed in exportDeploymentData to keep behavior consistent.
🧹 Nitpick comments (2)
src/Migration/Sources/Appwrite.php (2)
1740-1744: Good addition of initial end-offset clamping, but note inconsistency with loop clamping.The new guard at lines 1742-1744 correctly uses
$fileSize - 1, however the existing loop guard at line 1779 uses$end > $fileSize(without- 1). This means if$endequals exactly$fileSizeafter incrementing, it won't be clamped, producing an out-of-range byte request (bytes=start-fileSizewhere valid indices are0tofileSize-1).Consider aligning the loop guard for consistency:
♻️ Align loop clamping with initial clamping
- if ($end > $fileSize) { + if ($end > $fileSize - 1) { $end = $fileSize - 1; }Also applies to: 1779-1781
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Migration/Sources/Appwrite.php` around lines 1740 - 1744, The loop that re-clamps $end in Appwrite.php is inconsistent with the initial clamp: change the loop guard that currently compares `$end > $fileSize` to use `$end > $fileSize - 1` so `$end` is never set to `fileSize` (out of range); update the loop logic that increments and clamps `$end` (refer to the `$end` variable and the loop block around the existing guard) to match the initial clamping behavior and ensure all byte-range requests use indices 0..$fileSize-1.
1958-1962: Same loop clamping inconsistency asexportDeploymentData.The initial guard correctly uses
$fileSize - 1but the loop at line 1993 uses$end > $fileSize. Apply the same fix here for consistency:♻️ Align loop clamping
- if ($end > $fileSize) { + if ($end > $fileSize - 1) { $end = $fileSize - 1; }Also applies to: 1993-1995
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Migration/Sources/Appwrite.php` around lines 1958 - 1962, The loop clamping is inconsistent: after reading $fileSize from $responseHeaders the initial guard sets $end = $fileSize - 1 but the later loop compares using $end > $fileSize; locate the loop that uses the $end variable in this class (same area as exportDeploymentData) and change the condition to compare against $fileSize - 1 (i.e., use $end > $fileSize - 1) so both the initial guard and the loop use the same off-by-one logic for $fileSize and $end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Migration/Sources/Appwrite.php`:
- Line 1927: The check for 'content-length' assumes $responseHeaders keys are
lowercase (same issue as in exportDeploymentData); update the handling in the
code path that uses $responseHeaders (the conditional around
array_key_exists('content-length', $responseHeaders)) to perform a
case-insensitive header lookup or normalize header keys (e.g., map keys to
lowercase before checking) so the check works regardless of $this->call()'s
header casing; ensure you apply the same normalization approach used/fixed in
exportDeploymentData to keep behavior consistent.
---
Nitpick comments:
In `@src/Migration/Sources/Appwrite.php`:
- Around line 1740-1744: The loop that re-clamps $end in Appwrite.php is
inconsistent with the initial clamp: change the loop guard that currently
compares `$end > $fileSize` to use `$end > $fileSize - 1` so `$end` is never set
to `fileSize` (out of range); update the loop logic that increments and clamps
`$end` (refer to the `$end` variable and the loop block around the existing
guard) to match the initial clamping behavior and ensure all byte-range requests
use indices 0..$fileSize-1.
- Around line 1958-1962: The loop clamping is inconsistent: after reading
$fileSize from $responseHeaders the initial guard sets $end = $fileSize - 1 but
the later loop compares using $end > $fileSize; locate the loop that uses the
$end variable in this class (same area as exportDeploymentData) and change the
condition to compare against $fileSize - 1 (i.e., use $end > $fileSize - 1) so
both the initial guard and the loop use the same off-by-one logic for $fileSize
and $end.
2 participants
Summary
content-lengthheader key —Target::call()lowercases all response headers viastrtolower(), soContent-Lengthnever matchedstrleninstead ofmb_strlenfor binary deployment data —mb_strlenmisinterprets gzip bytes as UTF-8 characters, returning incorrect size$end >= $fileSizefor range clamping — server rejects Range requests where$end >= $size, the previous$end > $fileSizemissed the edge case where$end == $fileSizeSummary by CodeRabbit
Bug Fixes
Refactor