fix: [#554] Storage can't judge nesting folder correctly #39
Conversation
WalkthroughThe pull request introduces modifications to the GitHub Actions workflows and the OSS (Object Storage Service) implementation. The changes remove the Codecov workflow, update the test workflow with Aliyun-related environment variables, and enhance the OSS functionality in Changes
Assessment against linked issues
The changes appear to partially address the issue of nested folder handling by introducing a Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
oss.go (1)
361-371: Incremental folder creation logic is clear and avoids repeated calls.
Your loop carefully builds up each subpath. However, ifpathends with a trailing slash, consider verifying that it doesn't cause redundant calls toMakeDirectory.for i := 1; i < len(folders); i++ { folder := strings.Join(folders[:i], "/") + if folder == "" { + continue + } if err := r.MakeDirectory(folder); err != nil { return err } }oss_test.go (1)
383-393: Minor overshadowing of the variable named 'url'.
It may be slightly confusing to reuseurlfrom the environment variable. Consider renaming one of them to avoid overshadowing.- url := url + "/Url/1.txt" + newURL := url + "/Url/1.txt" assert.Equal(t, newURL, driver.Url("Url/1.txt")) resp, err := http.Get(newURL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/codecov.yml(0 hunks).github/workflows/test.yml(1 hunks)oss.go(5 hunks)oss_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/codecov.yml
🔇 Additional comments (26)
oss.go (4)
170-174: Skipping empty directory entries is a good safeguard.
Your check ensures that only valid non-empty directory names are captured. This helps avoid issues when the prefix path is re-inserted as an empty string.
197-200: Proper approach to ignore empty file entries.
This condition prevents the accidental addition of empty filenames. It neatly handles the case where a directory gets returned instead of a file.
280-285: Creating top-level folder ensures path integrity.
By callingmakeDirectorieshere, you reduce failures caused by missing intermediate directories. Error handling is also handled properly.
305-310: Consistent folder creation before file upload in PutFileAs.
Callingr.makeDirectories(str.Of(filePath).Finish("/").String())is consistent with the logic applied inPut(). This avoids nested folder creation errors.oss_test.go (20)
48-74: Comprehensive test for nested structures in AllDirectories.
Your assertions cover multiple nested levels. This thoroughly validates that subfolders can be listed.
80-100: Good coverage of multi-level scenario in AllFiles.
Testing both top-level and nested files ensures the listing logic is correct.
106-112: Copy operation confirmed for file existence.
The test verifies that the original file still exists after copying, which is correct behavior for a copy.
118-122: Delete operation tests normal removal and existence checks.
Looks good, verifying the file is missing after deletion.
128-132: DeleteDirectory test is straightforward and conclusive.
Confirms that all contents are removed.
138-160: Directories list test with multiple subdirectories.
The approach of verifying partial path inputs is robust.
166-186: Files listing test ensures partial path usage.
You cover plain, dot-prefixed, and slash-prefixed paths, guaranteeing consistent results.
192-200: Get operation test ensures correct file contents.
Retrieving the file as a string and verifying size is a strong validation.
206-214: GetBytes test provides binary retrieval check.
Covers critical functionality for byte-level operations.
220-228: LastModified test ensures timestamp accuracy.
You validate the truncated timestamp in UTC, which is sufficient for functional checks.
234-240: MakeDirectory test covers single and nested directory creation.
Cleanup after creation is well done.
246-261: MimeType validations for text and image.
Testing both textual and binary files ensures robust MIME type detection.
267-273: Move operation tested thoroughly.
The test ensures the old path is missing and the new path is present, which matches expected behavior.
279-285: Put operation test for deep nested structure.
Successfully confirms that each nested directory is created implicitly.
292-295: PutFile_Image covers a typical use case.
Verifies the file is uploaded properly in a folder.
302-309: PutFile_Text ensures text content is correct after upload.
Reading back the file contents to confirm is a good approach.
316-332: PutFileAs_Text test checks custom file naming.
You confirm both a name without extension and with extension. The test looks thorough.
339-349: PutFileAs_Image test validates custom name with .png extension.
Retains correct extension, ensuring the naming logic is correct.
355-360: Size test ensures file length retrieval.
Matches the expected length of the string "Goravel".
Line range hint
366-377: TemporaryUrl test confirms correct ephemeral link behavior.
Fetching the content from the short-lived URL is a strong integration check..github/workflows/test.yml (2)
7-12: Environment variables properly set from GitHub secrets.
This eases running integration tests against Aliyun. No concerns here.
14-24: The ubuntu job is well defined and minimal.
Actions that install dependencies and then run tests are correct. This effectively removes external coverage steps if Codecov was previously used, but meets the PR's objective.
📑 Description
Closes goravel/goravel#554
Summary by CodeRabbit
Chores
Bug Fixes
✅ Checks