Refactor/logic vfs #678
Refactor/logic vfs #678
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe overall changes introduce a structured way to handle virtual file systems within the project. New functionalities include creating composite and filtered file systems, as well as specific file systems for WebAssembly environments. The modifications enhance the ability to manage and interact with these diverse file systems seamlessly, incorporating path validation, mounting logic, and improved testing utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Composite as Composite FS
participant Wasm as Wasm FS
App->>Composite: Initialize Composite FS
Composite->>Composite: Mount Wasm FS
App->>Wasm: Create Wasm FS instance
Wasm-->>Composite: Wasm FS instance mounted
App->>Composite: Open file
Composite->>Composite: Validate and resolve path
Composite->>Wasm: Open file in Wasm FS
Wasm-->>Composite: File content
Composite-->>App: Return file content
App->>Composite: List mounts
Composite-->>App: Return mount points
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
ccamel
force-pushed
the
refactor/logic-vfs
branch
from
b806eae to
22e4f6b
Compare
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## main #678 +/- ##
==========================================
+ Coverage 51.98% 54.35% +2.36%
==========================================
Files 75 74 -1
Lines 2824 2896 +72
==========================================
+ Hits 1468 1574 +106
+ Misses 1260 1226 -34
Partials 96 96
|
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- app/app.go (2 hunks)
- x/logic/fs/composite/fs.go (1 hunks)
- x/logic/fs/filtered/fs.go (1 hunks)
- x/logic/fs/filtered/fs_test.go (4 hunks)
- x/logic/fs/provider.go (1 hunks)
- x/logic/fs/wasm/file.go (1 hunks)
- x/logic/fs/wasm/fs.go (1 hunks)
- x/logic/fs/wasm/fs_test.go (4 hunks)
- x/logic/keeper/features_test.go (2 hunks)
- x/logic/keeper/interpreter.go (2 hunks)
- x/logic/keeper/keeper.go (3 hunks)
Additional context used
GitHub Check: codecov/patch
x/logic/fs/wasm/file.go
[warning] 37-38: x/logic/fs/wasm/file.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 45-46: x/logic/fs/wasm/file.go#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 49-50: x/logic/fs/wasm/file.go#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 53-54: x/logic/fs/wasm/file.go#L53-L54
Added lines #L53 - L54 were not covered by tests
[warning] 57-58: x/logic/fs/wasm/file.go#L57-L58
Added lines #L57 - L58 were not covered by testsx/logic/fs/filtered/fs.go
[warning] 34-36: x/logic/fs/filtered/fs.go#L34-L36
Added lines #L34 - L36 were not covered by tests
[warning] 39-40: x/logic/fs/filtered/fs.go#L39-L40
Added lines #L39 - L40 were not covered by tests
[warning] 43-43: x/logic/fs/filtered/fs.go#L43
Added line #L43 was not covered by testsx/logic/fs/wasm/fs.go
[warning] 54-55: x/logic/fs/wasm/fs.go#L54-L55
Added lines #L54 - L55 were not covered by tests
[warning] 67-70: x/logic/fs/wasm/fs.go#L67-L70
Added lines #L67 - L70 were not covered by tests
[warning] 102-103: x/logic/fs/wasm/fs.go#L102-L103
Added lines #L102 - L103 were not covered by tests
Additional comments not posted (25)
x/logic/fs/provider.go (1)
1-9: The definition ofProvideras a type alias for a function returning afs.FSobject is clear and well-implemented.x/logic/fs/wasm/file.go (1)
9-12: The structure definitions forfileandfileInfoare well-defined and appropriate for the intended functionality.Also applies to: 14-18
x/logic/keeper/keeper.go (1)
Line range hint
12-26: The integration of thefs.Providerin theKeeperstruct and its constructor function is correctly implemented. This change supports the PR's goal of enhancing the filesystem's extensibility.Also applies to: 37-37
Verification successful
The integration of
fs.Providerinto theKeeperstruct is evident, and its usage spans several files within thekeepermodule. To ensure correctness, we need to verify its initialization and usage across the module.Let's proceed by checking the constructor of the
Keeperstruct to confirm the correct initialization offsProvider, and then we will verify its usage in methods and tests.
The
fsProvideris correctly integrated into theKeeperstruct and initialized in theNewKeeperconstructor. Its usage is evident in both the implementation and test files, indicating consistent application across the module.
x/logic/keeper/keeper.go: Initialization offsProviderin theNewKeeperconstructor and its inclusion in theKeeperstruct.x/logic/keeper/interpreter.go: Usage offsProviderin theinterpreter.WithFSmethod.- Various test files (
msg_server_test.go,grpc_query_params_test.go,grpc_query_ask_test.go): Mocking and usage offsProvider.The integration appears to be thorough and aligns with the intended functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of fs.Provider in Keeper and its usage across the module. # Test: Search for usage of `fsProvider` in the keeper module. Expect: Correct usage and initialization. rg --type go $'fsProvider' 'x/logic/keeper'Length of output: 756
Script:
#!/bin/bash # Description: Verify the initialization of fsProvider in the Keeper constructor and its usage in methods. # Test: Search for the Keeper constructor to check fsProvider initialization. rg --type go 'func NewKeeper' 'x/logic/keeper' # Test: Search for methods in the keeper module that use fsProvider. rg --type go 'fsProvider' 'x/logic/keeper'Length of output: 844
x/logic/fs/filtered/fs.go (1)
10-14: The implementation of thevfsstruct and theNewFSfunction is well-executed. The use of whitelists and blacklists to filter file access is a robust feature for security.Also applies to: 21-25
x/logic/fs/composite/fs.go (7)
32-34: Constructor methodNewFScorrectly initializes avfsinstance.
36-47: TheOpenmethod correctly handles path validation and resolution errors.
49-76: TheReadFilemethod appropriately handles bothReadFileFSand regularFStypes, ensuring compatibility.
78-80: TheMountmethod effectively assigns a filesystem to a mount point.
82-87: MethodListMountsuses efficient library functions to return sorted mount points.
89-99: ThevalidatePathmethod correctly validates URLs and ensures they contain a scheme.
101-108: Theresolvemethod accurately retrieves filesystems based on URI schemes, handling errors appropriately.x/logic/fs/filtered/fs_test.go (2)
79-81: TheDoAndReturnmethod correctly simulates theOpenoperation using a mocked filesystem.
86-90: TheNewFSmethod correctly initializes a filtered filesystem with URL-based whitelists and blacklists.x/logic/fs/wasm/fs.go (5)
38-42: TheNewFSmethod initializes the WASM filesystem with appropriate context and keeper.
44-52: MethodOpencorrectly retrieves data and creates a virtual file.
58-96: ThereadFilemethod effectively handles complex path parsing and data retrieval from a WASM contract.Tools
GitHub Check: codecov/patch
[warning] 67-70: x/logic/fs/wasm/fs.go#L67-L70
Added lines #L67 - L70 were not covered by tests
98-153: TheparsePathmethod thoroughly validates and decomposes the URI into components, handling various errors.Tools
GitHub Check: codecov/patch
[warning] 102-103: x/logic/fs/wasm/fs.go#L102-L103
Added lines #L102 - L103 were not covered by tests
54-56: Ensure thereadFilemethod is covered by tests as it is critical for reading files.Tools
GitHub Check: codecov/patch
[warning] 54-55: x/logic/fs/wasm/fs.go#L54-L55
Added lines #L54 - L55 were not covered by testsx/logic/keeper/interpreter.go (1)
142-142: TheWithFSmethod correctly sets up the filesystem with whitelisted and blacklisted URLs.x/logic/keeper/features_test.go (2)
35-36: The new imports for thecompositeandwasmpackages reflect the changes in the virtual file system as described in the PR summary.
300-303: The implementation of theProviderfunction now constructs a composite file system and mounts a WASM file system, which aligns with the PR's objectives to enhance modularity and extensibility in the VFS handling.x/logic/fs/wasm/fs_test.go (2)
Line range hint
31-121: This test case setup for theTestWasmHandlerfunction is comprehensive, covering various scenarios including different data, URIs, and expected results. The error messages are well-formed and provide clear indications of the expected failures, which improves debugging and maintainability.
140-143: The test for theOpenmethod in thewasmfile system handler is well-structured. It effectively tests both the error and success paths, ensuring that the file system behavior aligns with the expected outcomes based on the URI provided.app/app.go (2)
142-143: Import statements for new VFS modules are correctly added.
1165-1169: TheprovideFSfunction correctly constructs and configures the virtual file system as described in the PR. Ensure that thewasm2.NewFSmethod properly handles the context andWasmKeeperas these are critical for its operation.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
x/logic/fs/composite/fs_test.go (1)
18-23: DefinefileSpecstruct more clearly.Consider adding a comment above the
fileSpecstruct to explain its purpose and usage within the test suite. This can improve code readability and maintainability.Makefile (1)
Line range hint
259-259: Address potential security issue with hardcoded API key.There is a potential security risk due to a hardcoded API key. Consider removing or encrypting this key, or fetching it from a secure environment variable.
- MNEMONIC_VALIDATOR="island position immense mom cross enemy grab little deputy tray hungry detect state helmet tomorrow trap expect admit inhale present vault reveal scene atom" + MNEMONIC_VALIDATOR=$(getenv "MNEMONIC_VALIDATOR")
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- Makefile (1 hunks)
- x/logic/fs/composite/fs.go (1 hunks)
- x/logic/fs/composite/fs_test.go (1 hunks)
- x/logic/fs/filtered/fs.go (1 hunks)
- x/logic/fs/filtered/fs_test.go (4 hunks)
- x/logic/fs/wasm/fs.go (1 hunks)
- x/logic/fs/wasm/fs_test.go (4 hunks)
- x/logic/testutil/fs_mocks.go (2 hunks)
- x/logic/testutil/read_file_fs_mocks.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/logic/fs/filtered/fs.go
- x/logic/fs/filtered/fs_test.go
- x/logic/fs/wasm/fs_test.go
Additional context used
Learnings (1)
x/logic/fs/wasm/fs.go (1)
User: ccamel PR: axone-protocol/axoned#678 File: x/logic/fs/wasm/file.go:37-38 Timestamp: 2024-06-17T19:59:54.055Z Learning: The test coverage for methods `Name`, `Mode`, `ModTime`, `IsDir`, and `Sys` in the `wasm` package of the `logic` module has been significantly improved by the user ccamel.
GitHub Check: codecov/patch
x/logic/fs/composite/fs.go
[warning] 78-78: x/logic/fs/composite/fs.go#L78
Added line #L78 was not covered by tests
[warning] 123-123: x/logic/fs/composite/fs.go#L123
Added line #L123 was not covered by testsx/logic/fs/wasm/fs.go
[warning] 67-70: x/logic/fs/wasm/fs.go#L67-L70
Added lines #L67 - L70 were not covered by tests
Gitleaks
Makefile
259-259: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (13)
x/logic/testutil/fs_mocks.go (2)
38-40: The implementation of theOpenmethod in the mock looks correct, adhering to the expected behavior of returning a file and an error.
47-49: The recorder method forOpencorrectly sets up the expected call within the mock framework.x/logic/testutil/read_file_fs_mocks.go (4)
38-43: TheOpenmethod's mock implementation is correctly set up to simulate the interface's behavior, returning a file and an error based on the input.
47-49: The recorder method forOpenis properly configured to record the expected method call in the mock framework.
52-58: TheReadFilemethod in the mock is implemented correctly to return a byte array and an error, reflecting the behavior expected from the interface.
61-64: The recorder method forReadFilecorrectly sets up the expected call within the mock framework.x/logic/fs/composite/fs.go (3)
33-35: TheNewFSfunction correctly initializes a new instance ofvfswith an empty map for mounted file systems, adhering to the expected behavior of the composite file system.
37-48: TheOpenmethod properly handles path validation and resolves the file system based on the URI, returning the file object. The error handling is robust, usingfs.PathErrorto provide detailed error information.
84-92: TheMountandListMountsmethods are correctly implemented to manage the mounting of file systems and listing them in sorted order. The use ofsort.Stringsensures that the mounts are returned in a predictable order.x/logic/fs/wasm/fs.go (3)
38-42: TheNewFSconstructor is correctly set up to create a new instance of the WASM file system with the provided context and keeper.
44-52: TheOpenmethod effectively reads data from a WASM contract and wraps it into a virtual file. The integration with the SDK context is well-handled.
54-56: TheReadFilemethod simplifies file reading by delegating to thereadFilehelper function, maintaining consistency in how files are read.Makefile (1)
380-381: Ensure mock generation commands are appropriately scoped.The addition of mock generation commands for
FSandReadFileFSinterfaces is appropriate and aligns with the testing strategy. Ensure that these mocks are utilized effectively in unit tests.
|
@coderabbitai pause |
Actions performedReviews paused. |
ccamel
force-pushed
the
refactor/logic-vfs
branch
from
4aba324 to
e4f9d92
Compare
ccamel
force-pushed
the
refactor/logic-vfs
branch
from
e4f9d92 to
2c41f3c
Compare
By this PR, I’ve taken the liberty of refactoring a bit the Virtual File System of the
logicmodule to make it more coherent and consistent, and to enhance its extensibility to other types of virtual file systems (for instance, a memory-based VFS could facilitate certain operations with Prolog streams - subject to evaluation).So we now have the following vfs:
I'd like to say that although it might be tempting to rely on existing VFS implementations (such as C2FO/vfs,
mandelsoft/vfs or spf13/afero), this refactoring builds on what already exists and offers the simplest possible implementation, focused specifically on the needs of the
logicmodule.Note: the PR introduces no breaking changes; it only refines some error messages, and the tests have been updated accordingly.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores