Feature: Refactor filesystem checker #268
Conversation
WalkthroughThe changes involve a significant refactoring of the
These changes simplify the class structure by eliminating the need to instantiate 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: 1
🔭 Outside diff range comments (2)
src/main/java/org/cryptomator/cryptofs/common/FileSystemCapabilityChecker.java (2)
Line range hint
62-69: Avoid usingassertfor critical checks; use explicit exception handling instead.Using
assert ds != null;for checking critical conditions is discouraged because assertions may be disabled at runtime. Consider usingObjects.requireNonNull(ds);or throwing an appropriate exception to ensure the condition is always enforced.Apply this diff to replace the
assertstatement:- assert ds != null; + Objects.requireNonNull(ds, "DirectoryStream should not be null");
Line range hint
141-154: Replaceassertstatements with proper exception handling.The use of
assertindetermineSupportedCiphertextFileNameLengthmight not prevent invalid parameters when assertions are disabled. Replaceassertwith explicit checks and throw exceptions to ensure robustness.Apply this diff to replace
assertstatements:- assert lowerBoundIncl < upperBoundExcl; + Preconditions.checkArgument(lowerBoundIncl < upperBoundExcl, "lowerBoundIncl must be less than upperBoundExcl"); - assert mid < upperBoundExcl; + Preconditions.checkState(mid < upperBoundExcl, "mid must be less than upperBoundExcl"); - assert lowerBoundIncl < mid; + Preconditions.checkState(lowerBoundIncl < mid, "lowerBoundIncl must be less than mid");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/org/cryptomator/cryptofs/CryptoFileSystems.java(2 hunks)src/main/java/org/cryptomator/cryptofs/common/FileSystemCapabilityChecker.java(11 hunks)src/main/java/org/cryptomator/cryptofs/migration/MigrationModule.java(0 hunks)src/main/java/org/cryptomator/cryptofs/migration/Migrators.java(3 hunks)src/main/java/org/cryptomator/cryptofs/migration/v7/Version7Migrator.java(1 hunks)src/test/java/org/cryptomator/cryptofs/CryptoFileSystemsTest.java(1 hunks)src/test/java/org/cryptomator/cryptofs/common/FileSystemCapabilityCheckerTest.java(6 hunks)src/test/java/org/cryptomator/cryptofs/migration/MigratorsTest.java(11 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/org/cryptomator/cryptofs/migration/MigrationModule.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse
🔇 Additional comments (13)
src/test/java/org/cryptomator/cryptofs/common/FileSystemCapabilityCheckerTest.java (4)
Line range hint
27-43: Well-structured test setup!The mock configuration is comprehensive and follows good testing practices, providing a solid foundation for testing filesystem capabilities.
87-87: Good improvement using Integer.parseInt!The change from
Integer.valueOf()toInteger.parseInt()is more appropriate here since we need a primitive int. This is a small but good optimization sinceparseInt()directly returns a primitive whilevalueOf()would create an Integer object.Also applies to: 114-114
135-140: Excellent use of static mocking and parameterized testing!The test demonstrates several good practices:
- Proper use of try-with-resources for static mock cleanup
- Clear mock configuration with chained when/thenReturn calls
- Effective use of parameterized testing for multiple scenarios
58-65: Successfully refactored to use static method call!The test case has been properly updated to use the static
determineSupportedCiphertextFileNameLengthmethod while maintaining the original test logic.Let's verify that all instance-based calls have been converted to static:
✅ Verification successful
Refactoring to static utility class verified
The codebase scan confirms:
- All method calls are properly using the static form
- No instances of the class are being created
- The class is properly designed as a utility class with a private constructor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instance-based calls to FileSystemCapabilityChecker rg "new FileSystemCapabilityChecker\(\)" --type javaLength of output: 54
Script:
#!/bin/bash # Search for any references to FileSystemCapabilityChecker class rg "FileSystemCapabilityChecker" --type java -A 2 -B 2 # Also check if there are any remaining instance method calls by looking for "this" keyword in the class ast-grep --pattern 'class FileSystemCapabilityChecker { $$$ $this.$_($$$) $$$ }'Length of output: 13046
src/main/java/org/cryptomator/cryptofs/common/FileSystemCapabilityChecker.java (2)
38-39: Conversion to a utility class with a private constructor is appropriate.Making
FileSystemCapabilityCheckerafinalclass with aprivateconstructor ensures that it cannot be instantiated, which is suitable for a utility class containing only static methods.
113-114:⚠️ Potential issueUse explicit validation instead of
assertin public methods.The assertion
assert fillerName.length() > 0;may be bypassed if assertions are disabled. UsePreconditions.checkState()or throw an exception to enforce this condition.Apply this diff to enforce the condition:
- assert fillerName.length() > 0; + Preconditions.checkState(fillerName.length() > 0, "Filler name must not be empty");Likely invalid or redundant comment.
src/main/java/org/cryptomator/cryptofs/CryptoFileSystems.java (2)
40-42: Constructor refactoring aligns with utility class changes.Removing
FileSystemCapabilityCheckerfrom the constructor simplifies the class and aligns with the conversion ofFileSystemCapabilityCheckerto a static utility class.
59-61: Ensure filesystem capability checks are performed appropriately.With the removal of
adjustForCapabilities, verify that any necessary filesystem capability checks are handled elsewhere, and that operations fail gracefully if the filesystem lacks required capabilities.src/main/java/org/cryptomator/cryptofs/migration/Migrators.java (2)
51-53: Simplifying constructor by removing unnecessary dependency.Eliminating
FileSystemCapabilityCheckerfrom the constructor reduces complexity and reflects the shift to static utility methods.
96-99: Static method call is consistent with utility class design.Calling
FileSystemCapabilityChecker.assertAllCapabilities(pathToVault);statically is appropriate after refactoring the class into a utility with static methods.src/main/java/org/cryptomator/cryptofs/migration/v7/Version7Migrator.java (1)
69-69: LGTM! Static method call aligns with the refactoring objective.The change from instance method to static method call maintains the same functionality while simplifying the code.
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemsTest.java (1)
67-67: LGTM! Constructor updated to reflect static utility pattern.The test setup correctly reflects the removal of FileSystemCapabilityChecker from the constructor parameters.
src/test/java/org/cryptomator/cryptofs/migration/MigratorsTest.java (1)
Line range hint
53-219: LGTM! Consistent updates to test setup.All Migrators constructor calls have been consistently updated to remove the FileSystemCapabilityChecker parameter, aligning with the static utility pattern while maintaining test coverage.
src/main/java/org/cryptomator/cryptofs/common/FileSystemCapabilityChecker.java
Show resolved
Hide resolved
This PR does two, related things:
FileSystemCapabilityCheckerclass to be static onlyMotivation
This PR applies the principle behind the short story when creating the cryptographic filesystem. The implicit change from read/write to read-only, if the storage location is read-only, is removed.
Additionaly, the FileSystemChecker is converted to a static only class.