feat(filesystem): add filesystem modules for go-fries #915
Conversation
WalkthroughThis pull request introduces a set of new filesystem components. A common interface for file operations (Filesystem and Copyable) is defined along with multiple implementations including noop, local, and S3-based filesystems. The changes add repository functionality that wraps filesystem operations and introduce a path prefixing utility with customizable options. Each module includes corresponding tests, and new Go module files and version updates are provided to support the structured organization of these features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Repository
participant Filesystem
Client->>Repository: Invoke operation (e.g., Get/Put)
Repository->>Filesystem: Delegate operation call
Filesystem-->>Repository: Return result or error
Repository-->>Client: Return final response
Poem
✨ Finishing Touches
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:
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: 5
🔭 Outside diff range comments (1)
filesystem/s3/filesystem_test.go (1)
1-2:⚠️ Potential issueMissing tests in S3 filesystem test file
This test file is empty except for the package declaration. As a test file, it should contain test functions to validate the S3 filesystem implementation, similar to tests that would be present in
filesystem/local/filesystem_test.go.Additionally, there's a pipeline failure mentioning an unchecked error return value from
os.Mkdirat line 15, but the file only has 2 lines.Please implement proper tests for the S3 filesystem implementation, including tests for operations like reading, writing, checking existence, copying, etc. Make sure to handle all error return values properly.
🧹 Nitpick comments (15)
filesystem/s3/filesystem.go (2)
53-69: Check error wrapping for theReadmethod.The method simply returns the inner error from
s3.GetObjectandio.ReadAll. Consider wrapping or annotating these errors with contextual information (“failed reading from path: …”) to improve debuggability.
171-189: Consider removing or renaming unused parameters.Static analysis indicates that
ctxis unused in the following methods:
Name(lines 171-174)Basename(lines 176-179)Dirname(lines 181-184)Extension(lines 186-189)Either refactor so
ctxis needed or rename it to_.-func (s *Filesystem) Name(ctx context.Context, path string) string { +func (s *Filesystem) Name(_ context.Context, path string) string { // ... }🧰 Tools
🪛 golangci-lint (1.64.8)
[warning] 171-171: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 176-176: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 181-181: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
filesystem/local/filesystem.go (3)
18-18: File formatting issue.Static analysis tools (e.g., gofumpt) note that the file is not properly formatted. Consider running a formatting tool or including it in your CI checks.
🧰 Tools
🪛 golangci-lint (1.64.8)
18-18: File is not properly formatted
(gofumpt)
34-34: Address magic number usage.Using
0o644directly could trigger lint warnings. Extract it into a named constant or clarifying comment to improve readability and maintainability.- return os.WriteFile(s.prefixer.Prefix(path), value, 0o644) + const filePermissions = 0o644 + return os.WriteFile(s.prefixer.Prefix(path), value, filePermissions)🧰 Tools
🪛 golangci-lint (1.64.8)
34-34: Magic number: 0o644, in detected
(mnd)
61-63: Symlink method is unimplemented & parameters are unused.This panics and does not use
oldPathornewPath. Consider returning an error or removing the unused parameters if you cannot implement it at this time.Would you like me to provide a partial Symlink implementation or open an issue for future work?
🧰 Tools
🪛 golangci-lint (1.64.8)
[warning] 61-61: unused-parameter: parameter 'oldPath' seems to be unused, consider removing or renaming it as _
(revive)
filesystem/local/filesystem_test.go (2)
63-67: Uncomment or remove commented test codeThere are commented out test cases for symlinks. These should either be uncommented if the functionality is ready to be tested or removed if the functionality is not implemented.
13-78: Consider handling MkdirAll instead of MkdirThe current test creates a single directory, but if
./testfile/doesn't exist,os.Mkdirwill fail. Consider usingos.MkdirAllto create parent directories if they don't exist.-os.Mkdir("./testfile/basic", os.ModePerm) +if err := os.MkdirAll("./testfile/basic", os.ModePerm); err != nil { + t.Fatalf("Failed to create test directory: %v", err) +}🧰 Tools
🪛 golangci-lint (1.64.8)
15-15: Error return value of
os.Mkdiris not checked(errcheck)
🪛 GitHub Actions: Go Lint
[error] 15-15: Error return value of
os.Mkdiris not checked (errcheck)filesystem/paths.go (2)
27-29: Potential duplicate separator for identical prefix and separator.
Ifprefix == "/", you end up withprefixer.prefix = "//", which might be unexpected. Consider ensuring a single slash in that scenario:if prefixer.prefix != "" && prefixer.prefix != prefixer.separator { prefixer.prefix += prefixer.separator }
34-36: Windows paths caution.
Prefixstrips both\and/, but the default separator is hard-coded as"/". If cross-platform usage is intended, consider clarifying or extending platform handling.Do you need a platform-agnostic approach for prefix handling? If so, I'd be happy to open an issue or propose a cross-platform solution.
filesystem/repository.go (3)
5-35: Repository interface extends multiple interfaces.
CombiningFilesystemandCopyablemay yield a large interface. Ensure this design aligns with the single responsibility principle, and confirm that downstream consumers truly need all embedded functions.
88-94: Likewise concurrency concerns for append.
Similar toPrepend, concurrent calls toAppendmay override each other’s changes.
96-106: Good fallback strategy inCopy.
If the underlying filesystem does not supportCopyable, reading and writing content is a sensible fallback. However, for very large files, consider streaming to manage memory usage.filesystem/filesystem.go (3)
9-9: Consider aligning error message prefix with package nameThe error message uses a "[storage]" prefix while the package is named "filesystem". This could cause confusion for users of this package.
-var ErrNotSupported = errors.New("[storage] not supported") +var ErrNotSupported = errors.New("[filesystem] not supported")
11-77: Consider refining the Filesystem interface designThe interface is comprehensive but has a few design considerations:
The path utility methods (Path, Name, Basename, Dirname, Extension) don't appear to require a context parameter or filesystem-specific implementations as they operate on path strings.
Some method comments could be more detailed to clarify their behavior, particularly for Link, Symlink, AllFiles, and AllDirectories.
Consider:
- Moving path utility methods to a separate utility package or interface
- Removing context parameters from path methods that don't need them
- Enhancing method comments for clarity
For example:
// AllFiles lists all the files in the given path.(including subdirectories) +// AllFiles recursively traverses the directory structure starting at the given path +// and returns a list of all files found, including those in subdirectories. +// Paths returned are relative to the given path. AllFiles(ctx context.Context, path string) ([]string, error)
79-81: Consider integrating Copy into main Filesystem interfaceThe Copyable interface contains only one method and is implemented by NoopFilesystem alongside the Filesystem interface. Consider whether this method should be part of the main Filesystem interface for consistency.
If some filesystem implementations cannot support copying, they could return ErrNotSupported rather than introducing a separate interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
filesystem/go.sumis excluded by!**/*.sumfilesystem/local/go.sumis excluded by!**/*.sumfilesystem/s3/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
filesystem/filesystem.go(1 hunks)filesystem/filesystem_test.go(1 hunks)filesystem/go.mod(1 hunks)filesystem/local/filesystem.go(1 hunks)filesystem/local/filesystem_test.go(1 hunks)filesystem/local/go.mod(1 hunks)filesystem/local/testfile/.gitignore(1 hunks)filesystem/paths.go(1 hunks)filesystem/paths_test.go(1 hunks)filesystem/repository.go(1 hunks)filesystem/repository_test.go(1 hunks)filesystem/s3/filesystem.go(1 hunks)filesystem/s3/filesystem_test.go(1 hunks)filesystem/s3/go.mod(1 hunks)versions.yaml(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
filesystem/repository.go (1)
filesystem/filesystem.go (2)
Filesystem(11-77)Copyable(79-81)
filesystem/s3/filesystem.go (3)
filesystem/paths.go (2)
PathPrefixer(5-8)NewPathPrefixer(18-32)filesystem/filesystem.go (4)
Filesystem(11-77)Copyable(79-81)_(86-86)_(87-87)filesystem/local/filesystem.go (19)
Filesystem(13-16)s(29-31)s(33-35)s(37-39)s(41-51)s(53-55)s(57-59)s(61-64)s(66-79)s(81-101)s(103-117)s(119-137)s(139-141)s(143-145)s(147-157)s(159-169)s(171-178)_(19-19)ptr(211-213)
🪛 GitHub Actions: Go Lint
filesystem/s3/filesystem_test.go
[error] 15-15: Error return value of os.Mkdir is not checked (errcheck)
filesystem/local/filesystem_test.go
[error] 15-15: Error return value of os.Mkdir is not checked (errcheck)
filesystem/filesystem_test.go
[error] 15-15: Error return value of os.Mkdir is not checked (errcheck)
🪛 golangci-lint (1.64.8)
filesystem/local/filesystem_test.go
15-15: Error return value of os.Mkdir is not checked
(errcheck)
filesystem/s3/filesystem.go
[warning] 171-171: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 176-176: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 181-181: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
filesystem/local/filesystem.go
[warning] 61-61: unused-parameter: parameter 'oldPath' seems to be unused, consider removing or renaming it as _
(revive)
18-18: File is not properly formatted
(gofumpt)
34-34: Magic number: 0o644, in detected
(mnd)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (21)
filesystem/local/testfile/.gitignore (1)
1-2: No issues with the.gitignore.This is a standard approach to ignore all files except the
.gitignoreitself.filesystem/s3/filesystem.go (1)
82-103: Consistent error handling for non-existent objects.You're handling “not found” errors in multiple ways here (
NotFoundvs.responseError.ErrorCode()). This is valid, but ensure that all possible AWS error messages are included. Also confirm thatExistsworks reliably for edge cases (empty keys, special characters, etc.).versions.yaml (1)
49-53: LGTM! New filesystem modules properly addedThe filesystem modules have been correctly added to the versions.yaml file, following the same pattern as other module groupings in this file.
filesystem/repository_test.go (2)
17-20: Verify test assertion logic forExists()The test expects
repo.Exists(ctx, "noop")to returntrueeven after callingrepo.Destroy(ctx, "noop"). This seems contradictory unless the underlyingNoopFilesystemimplementation is designed to always returntrueforExists()regardless of previous operations.Make sure this is the intended behavior of
NoopFilesystemand not a test logic error.
10-30:❓ Verification inconclusive
Clarify expected behavior of NoopFilesystem in test
There's a logical inconsistency in the test expectations:
- The test uses a
NoopFilesystem(presumably a no-operation implementation)- The test expects
Exists()to returntrueandMissing()to returnfalseThese expectations imply that files always exist in the
NoopFilesystem, which seems inconsistent with a true no-op behavior where operations don't actually do anything.Consider:
- Documenting the expected behavior of
NoopFilesystem(e.g., "all files are considered to exist")- Adding tests with mock implementations that return errors to ensure the repository handles errors correctly
🏁 Script executed:
#!/bin/bash # Check the implementation of NoopFilesystem to understand its behavior rg -A 5 "type NoopFilesystem" rg -A 3 "func .* NoopFilesystem.* Exists" rg -A 3 "func .* NoopFilesystem.* Missing"Length of output: 386
Clarify and validate the NoopFilesystem behavior in repository tests
The test in
filesystem/repository_test.goexpects that operations on aNoopFilesystemsucceed without error—and thatExistsreturnstruewhileMissingreturnsfalse. This implies that files are considered to always exist, which seems at odds with a typical no-op behavior. However, the grep for implementations of theExistsandMissingmethods onNoopFilesystemproduced no output, so it's unclear if this behavior is explicitly defined or simply assumed.Suggestions:
- Document Intent: Clarify in the
NoopFilesystemimplementation (e.g., infilesystem/filesystem.go) that it is intended to simulate a filesystem where files always exist (or clearly detail its intended semantics).- Expand Tests: Consider adding tests with mock implementations that can return errors, ensuring that the repository correctly handles error conditions.
- Manual Verification: Please manually verify the implementations of
ExistsandMissingforNoopFilesystemin the codebase to confirm their behavior.filesystem/go.mod (2)
5-5:❌ Incorrect review comment
Verify testify version compatibility
The specified testify version
v1.10.0appears to be a future version. As of early 2025, the latest stable testify versions would typically be around v1.8.x or v1.9.x.Please verify if this version is correct or if it should be adjusted to match a currently available version.
🌐 Web query:
What is the latest stable version of github.com/stretchr/testify?💡 Result:
The latest stable version of
github.com/stretchr/testifyis v1.10.0, as detailed in its release notes[9]. This version includes functional changes like new assertions (NotElementsMatch,NotImplements), mock improvements (ordered calls), and documentation updates. It also introduces deprecations such asCompareTypeand updates dependencies likegithub.com/stretchr/objxto v0.5.2[9][15].Key updates in v1.10.0:
- Added
NotElementsMatchfor comparing unordered slices- Introduced ordered mock call expectations via
InOrder[9]- Deprecated
CompareTypein favor of type-specific assertions[9]- Added support for comparing
uintptrvalues[9]The project maintains compatibility with recent Go versions (1.21 and 1.22 as of March 2025)[6][15], and users experiencing installation errors typically resolve them by updating to at least Go 1.13[2]. The recommended update command remains:
go get -u github.com/stretchr/testifyCitations:
- 1: https://github.com/stretchr/objx/releases
- 2: error during install strechr/testify, undefined: errors.Is stretchr/testify#1091
- 3: https://github.com/urfave/cli/actions/runs/13219174749/workflow
- 4: https://pkg.go.dev/github.com/stretchr/testify/suite
- 5: https://github.com/stretchr/testify/actions/workflows/main.yml
- 6: Fix Go the versions used by our workflow tests stretchr/testify#1564
- 7: https://github.com/urfave/cli/actions/runs/13219174749
- 8: https://github.com/stretchr/testify/issues
- 9: https://github.com/stretchr/testify/releases
- 10: https://pkg.go.dev/github.com/stretchr/testify/http
- 11: https://pkg.go.dev/github.com/stretchr/testify/assert
- 12: Github releases page does not contain newer versions stretchr/testify#1295
- 13: https://vektra.github.io/mockery/latest/
- 14: https://github.com/stretchr/testify
- 15: https://github.com/stretchr/testify/actions/runs/8068183369
- 16: https://gitlab.com/gitlab-org/gitlab-runner/-/tree/10-1-stable/vendor/github.com/stretchr/testify
Testify version is up-to-date:
The version specified infilesystem/go.mod(v1.10.0) is indeed the latest stable version ofgithub.com/stretchr/testifyas confirmed by the release notes and community feedback. No changes are needed.Likely an incorrect or invalid review comment.
3-3:❓ Verification inconclusive
Verify Go version compatibility
The specified Go version
1.23.0appears to be a future version. As of March 2025, the latest stable Go releases would typically be around 1.21 or 1.22.Please verify if this is intentional or if it should be adjusted to match the current stable Go version used by the project.
🌐 Web query:
What is the latest stable Go version as of March 2025?💡 Result:
The latest stable Go version as of March 2025 is Go 1.24.1, released on March 4, 2025[1][5][7]. This minor update includes security fixes for the
net/httppackage and bug fixes across the compiler, runtime, and other core components[7].Key details about the current release:
- Release date: February 11, 2025 (1.24.0) → March 4, 2025 (1.24.1)[1][7]
- Support status: Actively maintained until two subsequent major releases (typically ~1 year)[1]
- Key requirements:
- Linux kernel 3.2+[9]
- macOS 12 Monterey or later (last release supporting Big Sur)[9]
The Go team maintains a predictable six-month release cycle, with each major version supported until two newer releases appear[7][10]. Current supported versions include 1.24.x and 1.23.x, while older versions like 1.22.x reached end-of-life in February 2025[1].
Citations:
- 1: https://endoflife.date/go
- 2: http://xahlee.info/golang/golang_version_dates.html
- 3: https://developer.android.com/about/versions/16/release-notes
- 4: https://blog.jetbrains.com/go/2025/01/16/the-goland-2025-1-early-access-program-has-started/
- 5: https://en.wikipedia.org/wiki/Go_(programming_language)
- 6: https://canonical-juju.readthedocs-hosted.com/en/latest/user/reference/juju/juju-roadmap-and-releases/
- 7: https://go.dev/doc/devel/release
- 8: https://www.bytesizego.com/blog/go-language-history
- 9: https://tip.golang.org/doc/go1.24
- 10: https://antonz.org/which-go/
- 11: https://go.dev/dl/
- 12: https://tip.golang.org/doc/go1.23
- 13: https://snapcraft.io/go
- 14: https://snapcraft.io/install/go/ubuntu
Action: Verify Go and Testify Versions in filesystem/go.mod
- The Go version is currently set to
1.23.0. As of March 2025, the latest stable release is Go 1.24.1. Note that both 1.23.x and 1.24.x are actively supported, so please confirm whether targeting Go 1.23.0 is intentional or if an update to 1.24.1 is preferred.- The dependency on
github.com/stretchr/testify v1.10.0appears to be in line with current stable releases, so no action is needed there.filesystem/filesystem_test.go (1)
1-53: LGTM! Well-structured test for the NoopFilesystem implementation.This test thoroughly validates the behavior of NoopFilesystem, covering all essential file operations like reading, writing, deleting, checking existence, renaming, linking, and directory operations. The assertions correctly verify that operations succeed and return expected values.
🧰 Tools
🪛 GitHub Actions: Go Lint
[error] 15-15: Error return value of
os.Mkdiris not checked (errcheck)filesystem/local/go.mod (3)
5-5: Replacement directive is correctly configuredThe replacement directive is properly set up to point to the parent directory, which is essential for local development.
7-10: Dependencies are correctly specifiedThe direct dependencies are properly defined with appropriate versions.
3-3:✅ Verification successful
Verify Go version compatibility
The specified Go version 1.23.0 appears to be a future version. This might cause compatibility issues as most environments will be running older versions.
🌐 Web query:
What is the latest stable version of Go as of March 2025?💡 Result:
The latest stable version of Go as of March 2025 is Go 1.24.1, released on March 4, 2025[1][3][4]. This minor update includes security fixes to the
net/httppackage and bug fixes across various components like cgo, the compiler, and runtime libraries[7][13].Key details about Go's release model:
- Each major release (e.g., 1.24) is supported until two newer versions exist
- The current supported releases are 1.24 and 1.23[1]
- Version 1.24 initially appeared on February 11, 2025[7], with its first patch (1.24.1) addressing critical security issues[15]
This release maintains compatibility with the Go 1 promise while introducing performance improvements like a new map implementation based on Swiss Tables and reduced WebAssembly memory usage[13][15]. Developers should always use the latest patch version (1.24.1) for security updates and stability enhancements[1].
Citations:
- 1: https://endoflife.date/go
- 2: http://xahlee.info/golang/golang_version_dates.html
- 3: https://golang.current-version.com
- 4: https://en.wikipedia.org/wiki/Go_(programming_language)
- 5: https://www.bytesizego.com/blog/go-language-history
- 6: https://blog.jetbrains.com/go/2025/01/16/the-goland-2025-1-early-access-program-has-started/
- 7: https://go.dev/doc/devel/release
- 8: https://antonz.org/which-go/
- 9: https://www.gitclear.com/open_repos/golang/go/releases
- 10: https://ssojet.com/blog/unlocking-the-future-of-golang-trends-predictions-and-business-impact-in-2025/
- 11: https://tip.golang.org/doc/go1.23
- 12: https://cloud.google.com/appengine/docs/standard/go/release-notes
- 13: https://tip.golang.org/doc/go1.24
- 14: https://www.jetbrains.com/go/download/other.html
- 15: https://www.yellowduck.be/posts/go-1-24-is-released-the-go-programming-language
- 16: https://go.dev/dl/
- 17: https://dev.to/empiree/why-you-should-learn-go-in-2025-308l/comments
Go Version Validated: No Compatibility Issue
After verifying the releases as of March 2025, it's clear that Go 1.23.0 is a supported release (with both 1.23 and 1.24 currently maintained). Since environments are expected to support Go 1.23, there is no incompatibility arising from using this version. Only update if you need the latest patch-level security or performance enhancements.
filesystem/local/filesystem_test.go (1)
80-200: Well-structured path manipulation testsThe path manipulation tests are well-organized with table-driven tests covering a variety of input paths and expected outputs.
filesystem/paths_test.go (1)
9-87: Well-structured tests for PathPrefixerThe PathPrefixer tests are comprehensive and well-organized using table-driven tests. They effectively validate the behavior with different prefixes, separators, and edge cases like empty values and paths with leading slashes.
filesystem/paths.go (2)
5-8: Struct fields look appropriate.
Theprefixandseparatorfields are properly scoped to store essential components for the path prefix logic.
12-16: Good use of functional options.
Using a composable approach withPathPrefixerOptionsets the stage for future customizations without overloading the constructor.filesystem/s3/go.mod (1)
3-3:✅ Verification successful
Validate Go version support.
You specifygo 1.23.0. Ensure your CI and deployment environments support this version, as it may be more recent than the stable release used in many production setups.
🌐 Web query:
Is Go 1.23.0 officially released and stable?💡 Result:
Yes, Go 1.23.0 was officially released on August 13, 2024, and is considered stable[1][4][7]. The release includes significant language and toolchain improvements while maintaining Go's backward compatibility promise. Key indicators of stability include:
- Official release channels providing binary/source distributions via go.dev/dl[1][7]
- Multiple security patches in subsequent minor versions (1.23.1 through 1.23.7) addressing issues in core packages[2][4]
- Linux distribution support with Ubuntu maintaining updated packages through March 2025[2]
- Production-ready features like range-over-func iterators and timer improvements that underwent extensive testing[3][10][13]
The release introduced several notable features while preserving compatibility:
- New iterator pattern using
rangeover function types[7][10]- Garbage collection improvements for timers/tickers[11][13]
- Enhanced PGO (Profile-Guided Optimization) build performance[11]
- Three new standard library packages:
iter,structs, andunique[7][10]While minor versions like 1.23.1 (released September 5, 2024) addressed security vulnerabilities[4][16], the core 1.23.0 release demonstrated sufficient stability for production use, as evidenced by its adoption in major projects like Gitea's 1.23.x branch[6][12] and toolchain updates[9].
Citations:
- 1: https://groups.google.com/g/golang-announce/c/RQjbRNOcV74
- 2: https://launchpad.net/ubuntu/+source/golang-1.23/+changelog
- 3: https://mariocarrion.com/2024/09/02/what-is-new-in-go-1-23.html
- 4: https://go.dev/doc/devel/release
- 5: https://tracker.debian.org/pkg/golang-1.23
- 6: https://blog.gitea.com/release-of-1.23.0/
- 7: https://go.dev/blog/go1.23
- 8: https://hachyderm.io/@golang/112955790911529315
- 9: https://github.com/golang/tools/releases
- 10: https://tip.golang.org/doc/go1.23
- 11: https://appliedgo.net/go123/
- 12: https://blog.gitea.com/release-of-1.23.0
- 13: https://antonz.org/go-1-23/
- 14: https://tip.golang.org/doc/go1.24
- 15: https://github.com/golang/go/milestone/212
- 16: Update Go main branch to 1.23.1 release branches to 1.22.7 etcd-io/etcd#18548
Validated Go 1.23.0 Stability in CI/Deployment Environments
Thego.modnow specifies Go 1.23.0, which, as confirmed by the official release notes (released on August 13, 2024), is both officially supported and stable. Please ensure that your CI and deployment pipelines are updated to run this version, as some environments may still default to older Go releases.filesystem/repository.go (4)
47-49: Simple delegation for Get.
Getsimply callsRead. This is consistent and maintains clarity for the repository API.
51-57:SetandPutduplication.
Both methods delegate toWrite. This duplication is likely intentional to provide distinct semantics.
76-78: Rename operation is straightforward.
Movedelegates toFilesystem.Rename, which keeps it consistent with the underlying interface.
80-86: Potential concurrency issues when prepending.
Prependreads and then writes. Multiple concurrent calls could interleave reads/writes unexpectedly. Consider documenting concurrency constraints or using some locking strategy.filesystem/filesystem.go (1)
1-113: Well-structured foundation for filesystem operationsOverall, this provides a solid foundation for filesystem operations with a comprehensive interface and a no-op implementation. The separation of concerns is clear, and the methods cover the essential operations needed for file manipulation.
| func (s *Filesystem) Link(context.Context, string, string) error { | ||
| return filesystem.ErrNotSupported | ||
| } | ||
|
|
||
| func (s *Filesystem) Symlink(context.Context, string, string) error { | ||
| return filesystem.ErrNotSupported | ||
| } |
There was a problem hiding this comment.
Provide non-panicking implementations or user-friendly errors for unimplemented methods.
Panicking in production library code can be disruptive. For the following methods, consider returning a structured error such as filesystem.ErrNotSupported or implementing partial logic if feasible:
Link,SymlinkFiles,AllFilesDirectories,AllDirectoriesIsFile,IsDirCopy
Also applies to: 207-210, 212-215, 217-220, 222-225, 227-230, 232-235, 237-240
| func (s *Filesystem) AllFiles(ctx context.Context, path string) ([]string, error) { | ||
| f, err := os.ReadDir(s.prefixer.Prefix(path)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var files []string //molint:prealloc | ||
| for _, file := range f { | ||
| if !file.IsDir() { | ||
| files = append(files, file.Name()) | ||
| } else { | ||
| subFiles, err := s.AllFiles(ctx, file.Name()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| files = append(files, subFiles...) | ||
| } | ||
| } | ||
|
|
||
| return files, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential bug in recursive file path.
AllFiles calls s.AllFiles(ctx, file.Name()) instead of recursively including the parent path (e.g., filepath.Join(path, file.Name())). This might silently skip deeper nested paths.
- subFiles, err := s.AllFiles(ctx, file.Name())
+ newPath := filepath.Join(path, file.Name())
+ subFiles, err := s.AllFiles(ctx, newPath)📝 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.
| func (s *Filesystem) AllFiles(ctx context.Context, path string) ([]string, error) { | |
| f, err := os.ReadDir(s.prefixer.Prefix(path)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var files []string //molint:prealloc | |
| for _, file := range f { | |
| if !file.IsDir() { | |
| files = append(files, file.Name()) | |
| } else { | |
| subFiles, err := s.AllFiles(ctx, file.Name()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| files = append(files, subFiles...) | |
| } | |
| } | |
| return files, nil | |
| } | |
| func (s *Filesystem) AllFiles(ctx context.Context, path string) ([]string, error) { | |
| f, err := os.ReadDir(s.prefixer.Prefix(path)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var files []string //molint:prealloc | |
| for _, file := range f { | |
| if !file.IsDir() { | |
| files = append(files, file.Name()) | |
| } else { | |
| newPath := filepath.Join(path, file.Name()) | |
| subFiles, err := s.AllFiles(ctx, newPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| files = append(files, subFiles...) | |
| } | |
| } | |
| return files, nil | |
| } |
| type Filesystem interface { | ||
| // Read the value at the given path. | ||
| Read(ctx context.Context, path string) ([]byte, error) | ||
|
|
||
| // Write the value at the given path. | ||
| Write(ctx context.Context, path string, value []byte) error | ||
|
|
||
| // Delete the value at the given path. | ||
| Delete(ctx context.Context, path string) error | ||
|
|
||
| // Exists checks if the path exists. | ||
| Exists(ctx context.Context, path string) (bool, error) | ||
|
|
||
| // Rename renames the value from the old path to the new path. | ||
| Rename(ctx context.Context, oldPath, newPath string) error | ||
|
|
||
| // Link creates a hard link from the old path to the new path. | ||
| Link(ctx context.Context, oldPath, newPath string) error | ||
|
|
||
| // Symlink creates a symbolic link from the old path to the new path. | ||
| Symlink(ctx context.Context, oldPath, newPath string) error | ||
|
|
||
| // Files lists the files in the given path. | ||
| Files(ctx context.Context, path string) ([]string, error) | ||
|
|
||
| // AllFiles lists all the files in the given path.(including subdirectories) | ||
| AllFiles(ctx context.Context, path string) ([]string, error) | ||
|
|
||
| // Directories lists the directories in the given path. | ||
| Directories(ctx context.Context, path string) ([]string, error) | ||
|
|
||
| // AllDirectories lists all the directories in the given path.(including subdirectories) | ||
| AllDirectories(ctx context.Context, path string) ([]string, error) | ||
|
|
||
| // MakeDirectory creates a directory at the given path. | ||
| MakeDirectory(ctx context.Context, path string) error | ||
|
|
||
| // DeleteDirectory deletes the directory at the given path. | ||
| DeleteDirectory(ctx context.Context, path string) error | ||
|
|
||
| // IsFile checks if the path is a file. | ||
| IsFile(ctx context.Context, path string) (bool, error) | ||
|
|
||
| // IsDir checks if the path is a directory. | ||
| IsDir(ctx context.Context, path string) (bool, error) | ||
|
|
||
| // Size returns the size of the file in bytes. | ||
| Size(ctx context.Context, path string) (int64, error) | ||
|
|
||
| // LastModified returns the last modified time of the file. | ||
| LastModified(ctx context.Context, path string) (*time.Time, error) | ||
|
|
||
| // Path returns the full path for the given path. | ||
| Path(ctx context.Context, path string) string | ||
|
|
||
| // Name returns the name of the file, without the extension. | ||
| Name(ctx context.Context, path string) string | ||
|
|
||
| // Basename returns the base name of the file, with the extension. | ||
| Basename(ctx context.Context, path string) string | ||
|
|
||
| // Dirname returns the directory name of the file. | ||
| Dirname(ctx context.Context, path string) string | ||
|
|
||
| // Extension returns the extension of the file. | ||
| Extension(ctx context.Context, path string) string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling signatures in interface methods
Several interface methods should include error return values for failed operations, but some path-related methods (like Path, Name, Basename, Dirname, Extension) don't return errors even though they could fail in real implementations.
Consider adding error return values to these methods for consistency and to allow implementations to properly handle errors.
| func (NoopFilesystem) Exists(context.Context, string) (bool, error) { return true, nil } | ||
| func (NoopFilesystem) Rename(context.Context, string, string) error { return nil } | ||
| func (NoopFilesystem) Link(context.Context, string, string) error { return nil } | ||
| func (NoopFilesystem) Symlink(context.Context, string, string) error { return nil } | ||
| func (NoopFilesystem) Files(context.Context, string) ([]string, error) { return nil, nil } | ||
| func (NoopFilesystem) AllFiles(context.Context, string) ([]string, error) { return nil, nil } | ||
| func (NoopFilesystem) Directories(context.Context, string) ([]string, error) { return nil, nil } | ||
| func (NoopFilesystem) AllDirectories(context.Context, string) ([]string, error) { return nil, nil } | ||
| func (NoopFilesystem) MakeDirectory(context.Context, string) error { return nil } | ||
| func (NoopFilesystem) DeleteDirectory(context.Context, string) error { return nil } | ||
| func (NoopFilesystem) IsFile(context.Context, string) (bool, error) { return false, nil } | ||
| func (NoopFilesystem) IsDir(context.Context, string) (bool, error) { return false, nil } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure logical consistency in NoopFilesystem boolean methods
The implementation has a logical inconsistency: Exists() returns true (line 93) while IsFile() and IsDir() both return false (lines 103-104). In most filesystems, if a path exists, it should be either a file or directory.
For a no-op implementation, consider making these logically consistent:
-func (NoopFilesystem) Exists(context.Context, string) (bool, error) { return true, nil }
+func (NoopFilesystem) Exists(context.Context, string) (bool, error) { return false, nil }Or alternatively:
-func (NoopFilesystem) IsFile(context.Context, string) (bool, error) { return false, nil }
+func (NoopFilesystem) IsFile(context.Context, string) (bool, error) { return true, nil }📝 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.
| func (NoopFilesystem) Exists(context.Context, string) (bool, error) { return true, nil } | |
| func (NoopFilesystem) Rename(context.Context, string, string) error { return nil } | |
| func (NoopFilesystem) Link(context.Context, string, string) error { return nil } | |
| func (NoopFilesystem) Symlink(context.Context, string, string) error { return nil } | |
| func (NoopFilesystem) Files(context.Context, string) ([]string, error) { return nil, nil } | |
| func (NoopFilesystem) AllFiles(context.Context, string) ([]string, error) { return nil, nil } | |
| func (NoopFilesystem) Directories(context.Context, string) ([]string, error) { return nil, nil } | |
| func (NoopFilesystem) AllDirectories(context.Context, string) ([]string, error) { return nil, nil } | |
| func (NoopFilesystem) MakeDirectory(context.Context, string) error { return nil } | |
| func (NoopFilesystem) DeleteDirectory(context.Context, string) error { return nil } | |
| func (NoopFilesystem) IsFile(context.Context, string) (bool, error) { return false, nil } | |
| func (NoopFilesystem) IsDir(context.Context, string) (bool, error) { return false, nil } | |
| func (NoopFilesystem) Exists(context.Context, string) (bool, error) { return false, nil } | |
| func (NoopFilesystem) Rename(context.Context, string, string) error { return nil } | |
| func (NoopFilesystem) Link(context.Context, string, string) error { return nil } | |
| func (NoopFilesystem) Symlink(context.Context, string, string) error { return nil } | |
| func (NoopFilesystem) Files(context.Context, string) ([]string, error) { return nil, nil } | |
| func (NoopFilesystem) AllFiles(context.Context, string) ([]string, error) { return nil, nil } | |
| func (NoopFilesystem) Directories(context.Context, string) ([]string, error) { return nil, nil } | |
| func (NoopFilesystem) AllDirectories(context.Context, string) ([]string, error) { return nil, nil } | |
| func (NoopFilesystem) MakeDirectory(context.Context, string) error { return nil } | |
| func (NoopFilesystem) DeleteDirectory(context.Context, string) error { return nil } | |
| func (NoopFilesystem) IsFile(context.Context, string) (bool, error) { return false, nil } | |
| func (NoopFilesystem) IsDir(context.Context, string) (bool, error) { return false, nil } |
…ves and updating test initialization
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
filesystem/s3/filesystem.go (3)
53-69: Consider using a streaming approach for large files.
Reading the entire object into memory may be inefficient for large files. If your use case involves large uploads or downloads, implementing a streaming approach might be more performant and memory-friendly.
71-80: Consider streaming writes for large files.
The current implementation buffers the entire payload in memory. For very large values, you may want to accept anio.Readerfor streaming data to S3.
105-116: Note that renames via copy + delete are not atomic.
UsingCopyObjectfollowed byDeleteObjectis a common approach for rename, but it’s not atomic. Concurrent processes might briefly see both copies or neither if a problem occurs midway. Ensure that your application tolerates this non-atomic behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
filesystem/s3/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
.golangci.yml(0 hunks)filesystem/local/filesystem.go(1 hunks)filesystem/local/filesystem_test.go(1 hunks)filesystem/s3/filesystem.go(1 hunks)filesystem/s3/go.mod(1 hunks)
💤 Files with no reviewable changes (1)
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- filesystem/local/filesystem_test.go
- filesystem/s3/go.mod
- filesystem/local/filesystem.go
🧰 Additional context used
🧬 Code Definitions (1)
filesystem/s3/filesystem.go (3)
filesystem/paths.go (2)
PathPrefixer(5-8)NewPathPrefixer(18-32)filesystem/local/filesystem.go (18)
Filesystem(13-16)s(27-29)s(31-33)s(35-37)s(39-49)s(51-53)s(55-57)s(59-63)s(65-78)s(80-100)s(102-116)s(118-136)s(138-140)s(142-144)s(146-156)s(158-168)s(170-177)_(18-18)filesystem/filesystem.go (4)
Filesystem(11-77)Copyable(79-81)_(86-86)_(87-87)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint
- GitHub Check: go test (1.23.x, ubuntu-latest, mysql:latest)
- GitHub Check: go test (1.23.x, ubuntu-latest, mysql:5.7)
- GitHub Check: go test (1.24.x, ubuntu-latest, mysql:5.7)
- GitHub Check: go test (1.24.x, ubuntu-latest, mysql:latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (26)
filesystem/s3/filesystem.go (26)
1-2: No issues found.
3-15: No issues found.
17-23: No issues found.
25-28: No issues found.
30-37: No issues found.
38-51: No issues found.
82-103: No issues found.
118-125: Folder simulation with trailing slash is valid for S3.
Creating a zero-length object ending with a slash is a recognized strategy to emulate directories in S3.
135-149: No issues found.
151-165: No issues found.
167-169: No issues found.
171-174: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
176-179: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
181-184: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
186-189: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
191-197: No issues found.
199-201: No issues found.
203-205: No issues found.
207-210: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
212-215: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
217-220: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
222-225: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
227-230: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
232-235: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
237-240: Panic in unimplemented method.
This duplicates a past review comment advising to return a non-panicking alternative or structured error instead of panicking in production code.
242-244: No issues found.
| func (s *Filesystem) DeleteDirectory(ctx context.Context, path string) error { | ||
| _, err := s.s3.DeleteObject(ctx, &s3.DeleteObjectInput{ | ||
| Bucket: ptr(s.bucket), | ||
| Key: ptr(s.prefixer.Prefix(path) + "/"), | ||
| }) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deleting only the “directory” marker object might leave child objects in place.
This approach removes the synthetic folder object but does not recursively remove objects under the prefix, potentially leading to orphaned files. If the desired behavior is to remove all contained objects, consider listing and deleting matching keys.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.x #915 +/- ##
=======================================
Coverage 86.90% 86.90%
=======================================
Files 13 13
Lines 771 771
=======================================
Hits 670 670
Misses 95 95
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Tests
Chores
llllinter from the configuration.