fix: deletePath not working properly in Local#134
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughChange to Local storage's deletePath: when recursing into subdirectories the code now computes child relative paths by taking a substring starting at strlen($this->getRoot().DIRECTORY_SEPARATOR) rather than the previous substr_replace approach. Adds a test that verifies deletion of multi-level nested directories and files. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Local as Local::deletePath
participant FS as Filesystem
Caller->>Local: deletePath(path)
Local->>FS: list directory entries
loop for each entry
alt entry is file
Local->>FS: delete(file, recursive=true)
else entry is directory
Note right of Local #DDEBF7: New: relative = substring(absolutePath, strlen(root + DIRECTORY_SEPARATOR))
Local->>Local: deletePath(relative)
end
end
Local-->>Caller: return success
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue in the Local storage device's deletePath method where it wasn't properly handling nested directory structures, which could lead to "Directory not empty" errors when attempting to delete paths containing subdirectories.
- Fixed the path calculation logic for recursive directory deletion
- Added comprehensive test coverage for nested directory deletion scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Storage/Device/Local.php | Fixed the relative path calculation in deletePath method to properly handle recursive directory deletion |
| tests/Storage/Device/LocalTest.php | Added test case for nested directory structure deletion to verify the fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/Local.php (2)
361-367: Guard against realpath() returning false to avoid TypeErrorIf realpath() returns false, file_exists($path) will trigger a TypeError in PHP 8+. Check the result before calling filesystem functions.
- $path = realpath($this->getRoot().DIRECTORY_SEPARATOR.$path); - - if (! file_exists($path) || ! is_dir($path)) { + $absolute = $this->getRoot().DIRECTORY_SEPARATOR.$path; + $path = \realpath($absolute); + if ($path === false || ! \is_dir($path)) { return false; }
338-353: delete() returns false when deleting directories recursivelyWhen deleting a directory with $recursive = true, the function doesn’t return the rmdir result and falls through to return false. This is a correctness bug and can mislead callers.
public function delete(string $path, bool $recursive = false): bool { if (\is_dir($path) && $recursive) { $files = $this->getFiles($path); foreach ($files as $file) { $this->delete($file, true); } - \rmdir($path); + return \rmdir($path); } elseif (\is_file($path) || \is_link($path)) { return \unlink($path); } return false; }
🧹 Nitpick comments (4)
src/Storage/Device/Local.php (1)
369-376: Make relative-path derivation robust; anchor removal to the root prefixUsing str_replace on the full path can silently fail on case differences or if $this->root isn’t normalized, and it’s not anchored to the beginning of the string. Prefer a prefix check on normalized absolute paths, then substr. Also add a safe fallback to delete absolute dirs directly to avoid leaving undeleted children.
Recommended change:
- if (is_dir($file)) { - $relativePath = \str_replace($this->getRoot().DIRECTORY_SEPARATOR, '', $file); - $this->deletePath($relativePath); - } else { - $this->delete($file, true); - } + if (\is_dir($file)) { + $root = \rtrim((string) \realpath($this->getRoot()), DIRECTORY_SEPARATOR); + $fileAbs = (string) \realpath($file); + if ($fileAbs !== false && \str_starts_with($fileAbs, $root.DIRECTORY_SEPARATOR)) { + $relativePath = \substr($fileAbs, \strlen($root) + 1); + $this->deletePath($relativePath); + } else { + // Fallback to absolute delete to ensure children are removed + $this->delete($file, true); + } + } else { + $this->delete($file, true); + }Alternative (simpler and avoids relative-path math entirely): always call delete($file, true) for both files and directories in this loop, then rmdir($path). It reduces complexity and eliminates path-prefix issues.
tests/Storage/Device/LocalTest.php (3)
384-387: Create nested directories under the storage root, not the CWDcreateDirectory($nestedBucket) uses a relative path from the process working directory, not the Local root. Anchor directory creation to the device root to avoid leaking test artifacts outside the storage area.
- $this->assertTrue($this->object->createDirectory($nestedBucket)); - $this->assertTrue($this->object->createDirectory($nestedBucket.DIRECTORY_SEPARATOR.'sub1')); - $this->assertTrue($this->object->createDirectory($nestedBucket.DIRECTORY_SEPARATOR.'sub1'.DIRECTORY_SEPARATOR.'sub2')); + $this->assertTrue($this->object->createDirectory($this->object->getPath($nestedBucket))); + $this->assertTrue($this->object->createDirectory($this->object->getPath($nestedBucket.DIRECTORY_SEPARATOR.'sub1'))); + $this->assertTrue($this->object->createDirectory($this->object->getPath($nestedBucket.DIRECTORY_SEPARATOR.'sub1'.DIRECTORY_SEPARATOR.'sub2')));
389-396: Build file paths via getPath() instead of str_ireplace on absolute pathsRelying on string replacement of the root is brittle and OS-dependent. Compose the desired logical path and let getPath() resolve it within the storage root.
- $rootFile = $this->object->getPath('file1.txt'); - $rootFile = str_ireplace($this->object->getRoot(), $this->object->getRoot().DIRECTORY_SEPARATOR.$nestedBucket, $rootFile); + $rootFile = $this->object->getPath($nestedBucket.DIRECTORY_SEPARATOR.'file1.txt'); @@ - $nestedFile = $this->object->getPath('file2.txt'); - $nestedFile = str_ireplace($this->object->getRoot(), $this->object->getRoot().DIRECTORY_SEPARATOR.$nestedBucket.DIRECTORY_SEPARATOR.'sub1'.DIRECTORY_SEPARATOR.'sub2', $nestedFile); + $nestedFile = $this->object->getPath($nestedBucket.DIRECTORY_SEPARATOR.'sub1'.DIRECTORY_SEPARATOR.'sub2'.DIRECTORY_SEPARATOR.'file2.txt');
380-408: Reduce duplication with testNestedDeletePath or split into its own testThis block overlaps with testNestedDeletePath below. Consider consolidating into a single focused test for nested deletePath behavior to keep the suite lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Storage/Device/Local.php(1 hunks)tests/Storage/Device/LocalTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Storage/Device/LocalTest.php (1)
src/Storage/Device/Local.php (6)
createDirectory(441-450)getPath(63-66)getRoot(53-56)write(288-297)exists(389-392)deletePath(361-381)
src/Storage/Device/Local.php (2)
src/Storage/Device/S3.php (2)
getRoot(220-223)deletePath(587-616)src/Storage/Device.php (2)
getRoot(74-74)deletePath(201-201)
🪛 GitHub Actions: Linter
tests/Storage/Device/LocalTest.php
[error] 1-1: Lint error: no_whitespace_in_blank_line detected. File: tests/Storage/Device/LocalTest.php. Command './vendor/bin/pint --test' exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Unit
fixes:
Warning: rmdir(/tmp/executor-test-build-a5847102/src): Directory not empty in /usr/local/vendor/utopia-php/storage/src/Storage/Device/Local.php on line 380Summary by CodeRabbit
Bug Fixes
Tests