Feature: Invalidate dir cache correctly #253
Conversation
…he base invalidated path
WalkthroughThe changes in this pull request introduce several new classes and methods related to the management of cleartext and ciphertext directory mappings within the Cryptomator application. Key additions include the Changes
Assessment against linked issues
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: 17
🧹 Outside diff range and nitpick comments (11)
src/main/java/org/cryptomator/cryptofs/CipherNodeNameParameters.java (1)
5-6: Consider clarifying the comment about Dagger.The comment "own file due to dagger" is not very informative for developers who might not be familiar with the project's dependency injection setup. Consider expanding this comment to explain why this record needs to be in its own file due to Dagger constraints.
Suggestion for a more informative comment:
// This record is in its own file to facilitate Dagger's code generation and dependency injection. // Keeping it separate ensures proper scoping and easier management of dependencies.src/main/java/org/cryptomator/cryptofs/CipherDir.java (1)
6-7: LGTM: Record definition is well-structured. Consider clarifying the comment.The
CipherDirrecord is well-defined with appropriate field names and types. However, the comment "own file due to dagger" might benefit from additional context.Consider expanding the comment to provide more context about why this needs to be in its own file due to Dagger. This will help future maintainers understand the reasoning behind this design decision.
src/main/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilter.java (1)
31-31: LGTM! Consider enhancing error logging.The change from
pathtocontentDirPath()aligns with the PR objectives and should improve consistency in directory path handling. This modification is part of the larger effort to address the cache invalidation issues mentioned in #252.Consider enhancing the error logging on line 33 to include the new
contentDirPath()information:- LOG.warn("Broken directory file: " + dirFile, e); + LOG.warn("Broken directory file: {}. Content dir path: {}", dirFile, dirPath, e);This would provide more context for debugging potential issues related to the new path resolution method.
src/test/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackupTest.java (1)
47-47: LGTM: ArgumentMatcher updated to use CipherDir.The change from
CryptoPathMapper.CiphertextDirectorytoCipherDiraligns with the PR objectives of using more efficient records. The matcher logic has been correctly updated to use the new methodsdirId()andcontentDirPath().Consider adding a comment explaining the purpose of this matcher for improved readability:
+// Matcher to verify that the CipherDir passed to backupManually has the correct dirId and contentDirPath ArgumentMatcher<CipherDir> cipherDirMatcher = obj -> obj.dirId().equals(dirId) && obj.contentDirPath().isAbsolute() && obj.contentDirPath().equals(expectedPath);src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java (2)
37-39: Method body updates improve encapsulationThe changes in the
executemethod body, specifically the use ofcontentDirPath()anddirId()methods instead of direct field access, improve encapsulation and are consistent with the newCipherDirtype.Consider extracting the long line 37-38 into multiple lines for better readability:
- try (var channel = Files.newByteChannel(ciphertextDirectory.contentDirPath().resolve(Constants.DIR_BACKUP_FILE_NAME), StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE); // - var encryptingChannel = wrapEncryptionAround(channel, cryptor)) { + try (var channel = Files.newByteChannel( + ciphertextDirectory.contentDirPath().resolve(Constants.DIR_BACKUP_FILE_NAME), + StandardOpenOption.CREATE_NEW, + StandardOpenOption.WRITE + ); + var encryptingChannel = wrapEncryptionAround(channel, cryptor)) {
Line range hint
1-59: Summary: Consistent refactoring improves encapsulation and API designThe changes in this file are part of a larger refactoring effort to improve encapsulation and API design. The consistent replacement of
CryptoPathMapper.CiphertextDirectorywithCipherDir, and the use of method calls instead of direct field access fordirIdandpath, align well with the PR objectives.These modifications enhance the code's maintainability and potentially its performance, especially in relation to the cache invalidation issue mentioned in the PR objectives. The new
CipherDirtype likely provides better control over directory operations, which could contribute to more accurate cache management.To ensure the effectiveness of these changes:
- Verify that all other parts of the codebase that interact with this class have been updated accordingly.
- Consider adding unit tests to validate the behavior of the
CipherDirclass, particularly itscontentDirPath()anddirId()methods.- Update any relevant documentation or JavaDocs for the
CipherDirclass to explain its purpose and usage.src/test/java/org/cryptomator/cryptofs/health/dirid/MissingContentDirTest.java (2)
64-64: LGTM: ArgumentMatcher updated to use CipherDir.The change correctly updates the
ArgumentMatcherto work with the newCipherDirtype, maintaining the same logical checks as before.Consider if an exact path match using
equals()instead ofendsWith()would be more appropriate forcontentDirPath(). This depends on whether partial path matches are intentional or if only exact matches are valid.
Line range hint
1-85: Summary: Test updated to use new CipherDir type.The changes in this file successfully update the
MissingContentDirTestto work with the newCipherDirtype, which appears to be a replacement for the previousCryptoPathMapper.CiphertextDirectory. The modifications maintain the same logical checks while adapting to the new structure.These changes align well with the PR objectives of refactoring to facilitate proper testing and utilizing more efficient records instead of classes. The update to use
CipherDirsuggests an improvement in the overall design of the ciphertext directory representation.As the codebase transitions to using
CipherDir, ensure that all related tests and production code are consistently updated to use this new type. This will help maintain code consistency and prevent potential issues arising from mixed usage of old and new types.src/test/java/org/cryptomator/cryptofs/attr/AttributeProviderTest.java (1)
Line range hint
1-224: Summary of changes and potential impact.The changes in this file are minimal and focused, primarily involving the replacement of
CiphertextDirectorywithCipherDir. This modification appears to be part of a larger refactoring effort. The test structure and coverage should remain unchanged, assumingCipherDirprovides equivalent functionality toCiphertextDirectory.To ensure the integrity of these tests:
- Verify that all tests in this class pass after the changes.
- Consider adding specific tests for any new or changed functionality in
CipherDirif necessary.- Review other test classes that might be affected by this refactoring to ensure consistent usage of
CipherDir.src/test/java/org/cryptomator/cryptofs/ClearToCipherDirCacheTest.java (1)
31-32: Redundant stubbing ofdirLoader.load()intestPuttingNewEntryTriggersLoaderThe
dirLoader.load()method is already stubbed in thebeforeEachmethod. Redefining it in this test is unnecessary and could lead to confusion if the stubs diverge in the future.Apply this diff to remove the redundant stubbing:
- var cipherDir = Mockito.mock(CipherDir.class); - Mockito.when(dirLoader.load()).thenReturn(cipherDir);src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java (1)
331-331: Use constants for hard-coded valuesThe hard-coded strings
"aaaaaa"and directory paths like"d/22/2222"are used repeatedly across the tests. Defining these values as constants can improve readability and make future updates easier.Also applies to: 352-352, 380-380, 414-414, 447-447, 486-486, 529-529
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
- src/main/java/org/cryptomator/cryptofs/CipherDir.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/CipherNodeNameParameters.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/ClearToCipherDirCache.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java (6 hunks)
- src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProvider.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java (5 hunks)
- src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/attr/AbstractCryptoFileAttributeView.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/attr/AttributeProvider.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilter.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactory.java (2 hunks)
- src/main/java/org/cryptomator/cryptofs/health/dirid/MissingContentDir.java (2 hunks)
- src/main/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackup.java (2 hunks)
- src/main/java/org/cryptomator/cryptofs/health/dirid/OrphanContentDir.java (5 hunks)
- src/test/java/org/cryptomator/cryptofs/ClearToCipherDirCacheTest.java (1 hunks)
- src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (14 hunks)
- src/test/java/org/cryptomator/cryptofs/CryptoPathMapperTest.java (5 hunks)
- src/test/java/org/cryptomator/cryptofs/DirectoryIdBackupTest.java (2 hunks)
- src/test/java/org/cryptomator/cryptofs/attr/AttributeProviderTest.java (2 hunks)
- src/test/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilterTest.java (3 hunks)
- src/test/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactoryTest.java (3 hunks)
- src/test/java/org/cryptomator/cryptofs/health/dirid/MissingContentDirTest.java (2 hunks)
- src/test/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackupTest.java (2 hunks)
- src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java (11 hunks)
🧰 Additional context used
🔇 Additional comments (78)
src/main/java/org/cryptomator/cryptofs/CipherNodeNameParameters.java (1)
6-11: LGTM! Well-structured record with null-safety.The
CipherNodeNameParametersrecord is well-implemented:
- It correctly uses the Java 14+ record syntax.
- The custom constructor ensures non-null values for both fields using
Objects.requireNonNull().- The code follows Java naming conventions.
This implementation provides a concise and null-safe way to represent cipher node name parameters.
src/main/java/org/cryptomator/cryptofs/CipherDir.java (3)
1-5: LGTM: Package declaration and imports are appropriate.The package declaration aligns with the Cryptomator project structure, and the imports are relevant for the record implementation.
9-12: LGTM: Constructor implementation ensures data integrity.The custom constructor effectively uses
Objects.requireNonNull()to guarantee that bothdirIdandcontentDirPathare non-null. This is a good practice for maintaining data integrity and preventing null pointer exceptions.
1-14: LGTM: Well-implementedCipherDirrecord.This new file introduces a well-structured
CipherDirrecord that appears to be part of the solution for the cache invalidation issue mentioned in the PR objectives. The implementation is concise, follows Java best practices for records, and ensures null-safety for its fields.A few points to highlight:
- The record encapsulates
dirIdandcontentDirPath, which are likely key components in the directory mapping system.- The use of a custom constructor with null checks adds an extra layer of data integrity.
- The code is clean and easy to understand, which will aid in maintainability.
Overall, this addition seems to be a solid foundation for improving the directory mapping and cache management in the Cryptomator project.
src/main/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackup.java (3)
3-3: LGTM: Import statement added correctly.The import for
org.cryptomator.cryptofs.CipherDiris necessary for its usage in thefixmethod and is correctly placed with other imports from the same package.
Line range hint
1-42: Summary: Approve changes, recommend broader consistency check.The modifications in this file, namely the addition of the
CipherDirimport and its usage in thefixmethod, align well with the PR objectives of refactoring to use more efficient records. These changes appear to be part of a larger effort to replaceCryptoPathMapper.CiphertextDirectorywithCipherDir.While the changes in this file look good, I recommend:
- Ensuring that all occurrences of
CryptoPathMapper.CiphertextDirectoryacross the codebase have been replaced withCipherDir.- Verifying that all methods expecting
CryptoPathMapper.CiphertextDirectoryhave been updated to work withCipherDir.- Updating any relevant documentation or comments that might reference the old
CiphertextDirectoryclass.To assist with this broader check, please run the following script:
#!/bin/bash # Description: Check for any remaining usages of CryptoPathMapper.CiphertextDirectory # Test: Search for any remaining usages of CryptoPathMapper.CiphertextDirectory rg --type java "CryptoPathMapper\.CiphertextDirectory" # Test: List all files that use CipherDir to ensure it's used consistently rg --type java --files-with-matches "CipherDir"Review the results to ensure all necessary changes have been made consistently across the codebase.
34-34: Approve change, but verify compatibility withDirectoryIdBackup.backupManually.The modification to use
CipherDirinstead ofCryptoPathMapper.CiphertextDirectoryaligns with the PR objectives of using more efficient records. However, we should ensure that theDirectoryIdBackup.backupManuallymethod is compatible with theCipherDirrecord.Please run the following script to verify the compatibility:
If the first test passes and the second test returns no results, it suggests that the change is compatible and consistent across the codebase.
src/main/java/org/cryptomator/cryptofs/health/dirid/MissingContentDir.java (3)
3-3: LGTM: New import statement is correctly placed and necessary.The addition of
import org.cryptomator.cryptofs.CipherDir;is appropriate and required for the subsequent code changes.
Line range hint
1-63: Overall changes look good, but clarification needed on cache invalidation.The changes in this file are minimal and focused, aligning with the PR objectives of refactoring to use more efficient records. The replacement of
CryptoPathMapper.CiphertextDirectorywithCipherDiris likely part of a larger refactoring effort to improve code efficiency and maintainability.However, it's not immediately clear how these changes relate to the cache invalidation issue mentioned in the PR objectives. Could you please clarify how this refactoring contributes to solving the problem of invalid entries in the clear-to-cipher directory mapping cache?
To better understand the context of these changes, let's check for other occurrences of
CipherDirin the codebase:#!/bin/bash # Description: Search for other occurrences of CipherDir in the codebase # Test: Search for CipherDir usage rg --type java 'CipherDir'
55-55: LGTM: Updated to useCipherDirinstead ofCryptoPathMapper.CiphertextDirectory.This change aligns with the PR objectives of using more efficient records. The functionality appears to be maintained.
To ensure consistency across the codebase, please verify the signature of the
DirectoryIdBackup.backupManuallymethod:src/test/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackupTest.java (2)
3-3: LGTM: New import statement added.The import for
CipherDiris correctly added and is necessary for the changes in thetestFixmethod.
Line range hint
1-51: Overall assessment: Changes improve test efficiency and maintain correctness.The modifications in this file successfully update the
MissingDirIdBackupTestto use the newCipherDirrecord type, aligning with the PR objectives of using more efficient data structures. The test logic remains intact, ensuring that theMissingDirIdBackup.fix()method correctly interacts withDirectoryIdBackup.backupManually().These changes contribute to the overall goal of improving the cache invalidation process in the
CryptoPathMapperclass, as mentioned in the PR objectives.src/test/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilterTest.java (4)
3-3: LGTM: Import statement added correctly.The import for
CipherDiris necessary for the changes made in the test methods and is correctly placed with other imports.
49-49: LGTM: Consistent update to useCipherDir.This change is consistent with the previous modification and aligns with the PR objectives. The test logic remains intact, continuing to verify the behavior of the
brokenDirectoryFilterwhen processing nodes with missing target directories.
Line range hint
1-56: Overall assessment: Changes look good and align with PR objectives.The modifications in this file consistently replace
CryptoPathMapper.CiphertextDirectorywithCipherDir, which aligns with the PR objective of using more efficient records instead of classes. The test logic remains intact, ensuring that the behavior of theBrokenDirectoryFilteris still properly verified. These changes contribute to the overall improvement of the codebase.
35-35: LGTM: Updated to useCipherDirinstead ofCryptoPathMapper.CiphertextDirectory.This change aligns with the PR objectives of using more efficient records. The test logic remains intact, still verifying the behavior of the
brokenDirectoryFilterwhen processing nodes.To ensure consistency across the codebase, let's verify that
CryptoPathMapper.CiphertextDirectoryis no longer used:src/test/java/org/cryptomator/cryptofs/DirectoryIdBackupTest.java (3)
25-25: Approved: Type change aligns with PR objectivesThe change from
CryptoPathMapper.CiphertextDirectorytoCipherDiraligns well with the PR objectives of refactoring to use more efficient records instead of classes. This modification should improve code efficiency without altering the test's behavior.
34-34: Approved: Consistent initialization with new typeThe initialization of
cipherDirObjectusingCipherDiris consistent with the type change on line 25. The constructor parameters remain unchanged, indicating that theCipherDirrecord maintains the same structure as the previousCiphertextDirectoryclass. This change preserves the test's integrity while aligning with the refactoring objectives.
Line range hint
1-70: Overall assessment: Changes maintain test integrity while improving code efficiencyThe modifications in this file successfully transition from using
CryptoPathMapper.CiphertextDirectorytoCipherDir, which aligns with the PR's objective of refactoring to use more efficient records. The changes are consistent throughout the file and do not alter the behavior of the test methods. This refactoring should contribute to improved code efficiency without compromising the effectiveness of the tests.src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java (3)
29-31: Documentation updates accurately reflect code changesThe changes in the class documentation correctly align with the modifications in the code. The switch from
CryptoPathMapper.CiphertextDirectorytoCipherDirand the use ofcontentDirPath()method indicate an improvement in the API design, potentially offering better encapsulation or clarity.
47-47: Static method updates are consistentThe changes in the
backupManuallymethod, both in the documentation and method signature, are consistent with the previous modifications in the class. The use ofCipherDirinstead ofCryptoPathMapper.CiphertextDirectorymaintains consistency throughout the class.Also applies to: 50-50
36-36: Method signature update is consistentThe change in the
executemethod signature fromCryptoPathMapper.CiphertextDirectorytoCipherDiris consistent with the documentation updates and reflects a refactoring to use a more specific type.To ensure this change doesn't introduce any issues, please verify that all callers of this method have been updated accordingly. Run the following script to check for any remaining usages of
CryptoPathMapper.CiphertextDirectory:src/main/java/org/cryptomator/cryptofs/attr/AttributeProvider.java (1)
56-56: Approved: Enhanced directory path retrieval, but verification needed.The change from
pathtocontentDirPath()appears to align with the PR objectives of improving cache invalidation and path mapping. This modification likely provides a more specific path for directory contents, which could be crucial for proper cache management.However, to ensure the change's correctness and impact:
Could you provide more context on the
contentDirPath()method? Specifically:
- How does it differ from the previous
pathproperty?- Are there any scenarios where this change might alter existing behavior?
Have all relevant tests been updated to account for this change?
To verify the impact and usage of this new method, please run the following script:
This will help ensure consistent usage across the codebase and identify any areas that might need updates.
✅ Verification successful
Verification Successful: All instances now use
contentDirPath()and tests are updated accordingly.The search confirmed that there are no remaining usages of
.pathin thegetCiphertextDircontext. All relevant components and tests have been updated to usecontentDirPath(), ensuring consistency and alignment with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of 'path' vs 'contentDirPath()' in ciphertext directory context # Search for other occurrences of '.path' in similar context echo "Searching for '.path' usages:" rg --type java "getCiphertextDir\(.*\)\.path" # Search for other occurrences of 'contentDirPath()' method echo "Searching for 'contentDirPath()' usages:" rg --type java "\.contentDirPath\(\)" # Search for test cases related to directory path retrieval echo "Searching for relevant test cases:" rg --type java "test.*Directory.*Path"Length of output: 6216
src/main/java/org/cryptomator/cryptofs/attr/AbstractCryptoFileAttributeView.java (1)
63-63: LGTM: Improved clarity in directory path retrievalThe change from
pathtocontentDirPath()enhances the clarity of the code by explicitly indicating that we're dealing with the content directory path for ciphertext directories. This aligns well with the PR objectives of improving directory mapping and cache invalidation.To ensure consistency across the codebase, let's verify the usage of
contentDirPath():This will help us confirm that the change has been applied consistently throughout the project.
✅ Verification successful
Verification Successful: Consistent Usage of
contentDirPath()All instances of
getCiphertextDir()in the codebase consistently use thecontentDirPath()method, ensuring clarity and reducing potential errors related to directory path retrieval.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of contentDirPath() method # Expected result: All occurrences should use contentDirPath() for directory content paths # Search for usages of getCiphertextDir and contentDirPath rg --type java -e "getCiphertextDir.*\.path" -e "getCiphertextDir.*\.contentDirPath"Length of output: 1945
src/test/java/org/cryptomator/cryptofs/health/dirid/MissingContentDirTest.java (1)
3-3: LGTM: New import statement added correctly.The import for
CipherDiris necessary for the changes in thetestFixmethod and is placed appropriately.src/test/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactoryTest.java (3)
3-3: LGTM: Import statement added forCipherDir.The addition of this import statement is consistent with the refactoring mentioned in the PR objectives, replacing
CiphertextDirectorywith the more efficientCipherDirrecord.
77-78: LGTM:CiphertextDirectoryinstances replaced withCipherDir.These changes are in line with the PR objectives of refactoring to use more efficient records. The test logic remains unchanged, with only the return type of
getCiphertextDirbeing updated in both cases.
Line range hint
1-124: Overall assessment: Changes are well-implemented and consistent.The refactoring from
CiphertextDirectorytoCipherDirhas been applied consistently throughout the test file. These changes align with the PR objectives of using more efficient records and do not alter the underlying test logic. The modifications improve code efficiency while maintaining the integrity of the tests.src/test/java/org/cryptomator/cryptofs/attr/AttributeProviderTest.java (1)
11-11: LGTM: Import statement for CipherDir.The import statement for
CipherDiris correctly placed and necessary for the changes in the test class.src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProvider.java (3)
158-158: Ensure vault initialization functionality is maintainedWhile the change from
CryptoPathMapper.CiphertextDirectorytoCipherDiris a localized refactoring, it's crucial to ensure that the overall functionality of theinitializemethod remains intact.Please ensure that:
- The
CipherDirclass provides the same functionality as the previousCiphertextDirectoryclass.- The vault initialization process is thoroughly tested with the new
CipherDirclass to verify that no regressions have been introduced.Consider adding or updating unit tests for the
initializemethod to cover this change.
158-158: Clarify relationship with cache invalidation improvementsWhile this change is part of the refactoring effort, it's not immediately clear how it relates to the cache invalidation improvements mentioned in the PR objectives.
Could you please clarify:
- How does the introduction of
CipherDircontribute to the cache invalidation improvements?- Are there any properties or methods in the
CipherDirrecord that facilitate better cache management?- Where are the main cache invalidation improvements implemented in the codebase?
This information will help reviewers better understand the overall impact of the changes and ensure that all objectives of the PR are met.
158-158: LGTM: Refactoring to useCipherDirThe change from
CryptoPathMapper.CiphertextDirectorytoCipherDiraligns with the PR objectives of refactoring to use more efficient records instead of classes. This should improve performance and reduce memory usage.To ensure consistency across the codebase:
- Verify that the
CipherDirrecord is properly implemented and tested.- Check for any other occurrences of
CryptoPathMapper.CiphertextDirectorythat might need to be updated.You can use the following script to search for other occurrences:
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java (6)
318-318: LGTM. Good improvements in code structure and consistency.The changes in the
createDirectorymethod are consistent with the previous modification, usingcontentDirPath()instead ofpath. The introduction of theciphertextDirvariable improves code readability and potentially reduces repeated method calls. The use ofciphertextDir.dirId()suggests improved encapsulation of directory-related functionality.Also applies to: 325-329
434-434: LGTM. Consistent with previous changes.The modification in the
deleteDirectorymethod follows the same pattern as the previous changes, replacingpathwithcontentDirPath(). This change maintains consistency across the class in how ciphertext directory paths are accessed.
507-507: LGTM. Maintains consistency with previous changes.The modifications in the
copyDirectorymethod continue the pattern of replacingpathwithcontentDirPath(). This change ensures consistency across the class in how ciphertext directory paths are accessed and manipulated.Also applies to: 517-518
624-624: LGTM. Consistent change, but verify overall impact.The modification in the
moveDirectorymethod follows the same pattern as the previous changes, replacingpathwithcontentDirPath(). This change maintains consistency across the class in how ciphertext directory paths are accessed.To ensure all necessary changes have been made and to verify the overall impact, please run the following script:
#!/bin/bash # Check for any remaining instances of 'getCiphertextDir(...)' followed by '.path' rg --type java "getCiphertextDir\([^)]+\)\.path" -A 3 -B 3 # Check all usages of 'contentDirPath()' to ensure consistent usage rg --type java "contentDirPath\(\)" -A 3 -B 3 # Check for any potential missed updates in methods dealing with directories rg --type java "(?i)(create|copy|move|delete).*directory" -A 10 -B 10
Line range hint
1-696: LGTM. Consistent refactoring, but ensure comprehensive testing.The changes throughout this file consistently replace
pathwithcontentDirPath()when dealing with ciphertext directories. This refactoring appears to be part of a larger effort to improve the handling of ciphertext directory paths, potentially providing more specific access to directory contents.While the changes are consistent and likely beneficial, they have the potential to significantly impact the behavior of the CryptoFileSystem. Therefore, it's crucial to ensure that these modifications are thoroughly tested, particularly focusing on:
- Directory creation, deletion, and manipulation operations
- Path resolution and traversal
- Interactions between cleartext and ciphertext paths
- Edge cases involving root directories and nested directory structures
To assist in verifying the changes, please run the following script:
#!/bin/bash # List all test files related to CryptoFileSystem fd -e java test.*CryptoFileSystem # Check for existing tests that might need updating rg --type java "(?i)test.*(directory|path|ciphertext)" -A 10 -B 10 # Look for any TODOs or FIXMEs that might be related to this refactoring rg --type java "TODO|FIXME" -A 3 -B 3Consider updating existing tests or adding new ones to cover the modified behavior, especially focusing on directory path handling and interactions between cleartext and ciphertext paths.
144-144: LGTM, but verify impact on directory path handling.The change from
getCiphertextDir(p).pathtogetCiphertextDir(p).contentDirPath()looks good and likely provides a more specific path to the directory's content. However, it's important to ensure this change doesn't unexpectedly alter the behavior of thegetCiphertextPathmethod.To verify the impact of this change, please run the following script:
✅ Verification successful
LGTM. Change consistently applied across codebase.
The modification from
getCiphertextDir(p).pathtogetCiphertextDir(p).contentDirPath()has been consistently implemented throughout the codebase. This change appears to be part of a broader refactoring effort to improve directory path handling specificity. The widespread use ofcontentDirPath()in various components and test classes suggests that it's now an integral part of the system's directory management.While the change seems correct and well-integrated, it may be worthwhile to review the test coverage specifically for this modification to ensure all edge cases are properly handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any other usages of 'path' that might need to be updated rg --type java "getCiphertextDir\([^)]+\)\.path" -A 3 -B 3 # Check for existing usages of 'contentDirPath()' rg --type java "contentDirPath\(\)" -A 3 -B 3Length of output: 249758
Script:
#!/bin/bash # Check for any remaining usages of 'path' that might need to be updated rg --type java "getCiphertextDir\([^)]+\)\.path" -A 3 -B 3 # Check the implementation of contentDirPath() rg --type java "contentDirPath\(\s*\)\s*\{" -A 10 -B 3 # Look for tests related to contentDirPath rg --type java "test.*contentDirPath" -A 10 -B 3Length of output: 168
Script:
#!/bin/bash # Check if we can find Java files rg --type-list | grep java # Search for any Java files rg --type java "" -l | head -n 5 # Look for any usage of getCiphertextDir rg --type java "getCiphertextDir" # Search for contentDirPath without specific formatting rg --type java "contentDirPath" # Check for any test files rg --type java "class.*Test"Length of output: 28967
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (14)
205-205: Update mock to use new CipherDir classThe change from
CiphertextDirectorytoCipherDirhas been correctly implemented in this mock setup. This update ensures that the test is aligned with the new implementation.
581-581: Consistent use of CipherDir in mock setupThe mock setup for
getCiphertextDirhas been updated to use the newCipherDirclass. This change maintains consistency with the updated implementation.
697-698: Multiple CipherDir instances correctly used in test setupThe test setup has been updated to use multiple
CipherDirinstances for different paths. This change accurately reflects the new implementation and ensures that the tests cover various scenarios with the newCipherDirclass.
1158-1158: CipherDir usage in createDirectory testThe
createDirectorytest has been updated to use the newCipherDirclass. This change ensures that the test accurately reflects the current implementation.
1171-1171: Consistent CipherDir usage in createDirectory testAnother instance of
CipherDirusage in thecreateDirectorytest, maintaining consistency with the updated implementation.
1193-1194: Multiple CipherDir instances in createDirectory testThe test setup uses multiple
CipherDirinstances for different paths, accurately reflecting the new implementation and ensuring comprehensive test coverage.
1223-1224: CipherDir usage in error handling testThe error handling test for
createDirectoryhas been updated to useCipherDirinstances. This change ensures that error scenarios are properly tested with the new implementation.
1260-1265: CipherDir object creation and usage in backup testThe test for directory ID backup has been updated to use a
CipherDirobject. This change ensures that the backup functionality is properly tested with the new implementation.
1327-1327: CipherDir usage in isHidden testThe
isHiddentest has been updated to use the newCipherDirclass. This change maintains consistency with the updated implementation in the context of checking hidden file attributes.
1375-1375: CipherDir usage in checkAccess testThe
checkAccesstest has been updated to use the newCipherDirclass. This change ensures that access checks are properly tested with the new implementation.
1557-1557: CipherDir usage in setAttribute testThe
setAttributetest for regular directories has been updated to use the newCipherDirclass. This change ensures that attribute setting is properly tested with the new implementation.
1572-1572: CipherDir usage in setAttribute test for non-existing root directoryThe
setAttributetest for non-existing root directories has been updated to use the newCipherDirclass. This change ensures that edge cases are properly tested with the new implementation.
1590-1590: CipherDir usage in setAttribute test for filesThe
setAttributetest for files has been updated to use the newCipherDirclass. This change ensures that attribute setting for files is properly tested with the new implementation.
Line range hint
1-1639: Overall assessment of CipherDir implementation in testsThe changes in this file consistently replace the usage of
CiphertextDirectorywith the newCipherDirclass throughout the test suite. This update ensures that all relevant test cases are aligned with the new implementation. The modifications cover various scenarios, including directory creation, attribute setting, access checks, and error handling. These changes maintain the comprehensiveness and validity of the test suite, ensuring that the newCipherDirimplementation is thoroughly tested across different use cases.src/main/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactory.java (4)
3-3: Import statement updated toCipherDirThe import has been correctly updated to
CipherDir, ensuring that the class references align with the new implementation.
39-39: VariableciphertextDirnow usesCipherDirThe variable
ciphertextDiris now appropriately declared as aCipherDir, reflecting the updated class design.
40-40: UtilizingcontentDirPath()methodThe call to
Files.newDirectoryStreamnow usesciphertextDir.contentDirPath(), which is correct ifCipherDirprovides this method to access the directory path.
41-41: Accessing directory ID withdirId()methodChanged from
ciphertextDir.dirIdtociphertextDir.dirId(), properly invoking the method to retrieve the directory ID.src/test/java/org/cryptomator/cryptofs/ClearToCipherDirCacheTest.java (1)
110-114: Unexpected loader trigger fornewClearPathinentryUntouchedOnPrefixFailureIn this test,
cache.get(newClearPath, dirLoader);triggers the loader, but sinceclearPathdoes not start witholdPrefixPath, and no remapping should occur, it's unclear whynewClearPathis involved.Please verify whether
newClearPathshould be part of this test. If not, consider removing it to keep the test focused.src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java (9)
44-45: Cache Initialization Updated to Reflect New Caching StrategyThe declaration of
ciphertextNamesandclearToCipherDirCachealigns with the new caching mechanism, enhancing the management of directory mappings.
47-47: Use of CipherDir for Root DirectoryThe
rootDirectorynow utilizes theCipherDirclass, which is consistent with the refactored directory representation.
57-57: Proper Initialization in ConstructorThe constructor correctly initializes
clearToCipherDirCacheandrootDirectory, ensuring that the new cache and directory structures are set up at instantiation.
66-66: Documentation Updated to Include IOExceptionThe method
assertNonExistingnow accurately documents that it may throw anIOException, improving clarity for users of the API.
116-118: Updated Method Calls with CipherDir IntegrationThe use of
CipherDiringetCiphertextFilePathenhances consistency with the new directory structure. The method correctly retrieves the parent directory's content path and directory ID.
132-133: Updated getCiphertextFileName Method to Use CipherNodeNameParametersThe method now properly generates the ciphertext file name using the
CipherNodeNameParameters, which improves encapsulation and clarity of parameters.
153-162:⚠️ Potential issueVerify Exception Handling in Lambda Expression
The lambda
cipherDirLoaderIfAbsentmay throw anIOExceptionwhen callingresolveDirectory(dirFile). Ensure that the functional interfaceCipherDirLoaderallows throwing checked exceptions to prevent compilation errors.Run the following script to confirm that
CipherDirLoaderis defined to throwIOException:#!/bin/bash # Description: Check if 'CipherDirLoader' interface allows throwing IOException. # Search for the 'CipherDirLoader' interface definition. rg --type java 'interface CipherDirLoader' -A 5
171-174:⚠️ Potential issueEnsure dirHash Has Sufficient Length to Prevent Index Out of Bounds
When using
dirHash.substring(0, 2)anddirHash.substring(2), there is a risk ofStringIndexOutOfBoundsExceptionifdirHashis shorter than expected. Verify thathashDirectoryIdalways returns a string of adequate length.Run the following script to test the length of
dirHashoutputs:#!/bin/bash # Description: Verify that 'hashDirectoryId' outputs strings of sufficient length. # Replace 'SampleDirId' with a test directory ID. echo "SampleDirId" | java -cp path/to/cryptolib CryptorClassName hashDirectoryId # Expected result: The output string length is greater than or equal to 2.Ensure that all possible inputs to
hashDirectoryIdproduce a hash string of at least two characters.
Line range hint
122-130: Ensure Proper Implementation of equals() and hashCode() in CipherNodeNameParametersSince
CipherNodeNameParametersis used as a key in theciphertextNamescache, it's crucial that it correctly overridesequals()andhashCode()methods to maintain cache integrity and prevent unintended behavior.Run the following script to verify the implementation:
src/main/java/org/cryptomator/cryptofs/health/dirid/OrphanContentDir.java (4)
4-4: Necessary import ofCipherDiraddedThe import statement for
CipherDiris appropriate and required for the updated code.
Line range hint
158-174: MethodprepareStepParentnow returnsCipherDirfor enhanced type safetyChanging the return type of
prepareStepParentfromPathtoCipherDirimproves type safety and clarity in the codebase.
173-173: Correct instantiation ofCipherDirThe
CipherDirobjectstepParentCipherDiris properly instantiated withstepParentUUIDandstepParentDir, reflecting the new return type.
219-234: Updated method signature foradoptOrphanedResourcealigns with new typesThe parameter
stepParentDirinadoptOrphanedResourceis now of typeCipherDir, ensuring consistency with the rest of the code.src/test/java/org/cryptomator/cryptofs/CryptoPathMapperTest.java (7)
78-79: Use ofcontentDirPath()ensures correct path retrievalThe update from
.pathto.contentDirPath()intestPathEncryptionForRootaligns with the updated API, ensuring that the correct content directory path is retrieved.
102-103: Consistent application ofcontentDirPath()in path encryption testsUpdating to
.contentDirPath()intestPathEncryptionForFoomaintains consistency with the API changes and ensures accurate path mappings in the test.
136-137: Correct use ofcontentDirPath()in nested path encryptionThe change in
testPathEncryptionForFooBarto use.contentDirPath()correctly reflects the updated method, verifying that nested path encryption operates as expected.
386-396: Effective test for cache invalidation on node removalThe
testRemovedEntryMissmethod correctly verifies that invalidating a node causes a cache miss on subsequent retrievals, ensuring the cache invalidation logic functions properly.
398-412: Validation of child cache invalidation upon parent removalThe
testRemovedEntryChildMissmethod appropriately checks that invalidating a parent node also invalidates all its child entries in the cache, which is crucial for cache consistency.
416-427: Accurate testing of cache behavior during node movementThe
testMoveEntryOldMissNewHitmethod effectively confirms that moving a node results in a cache miss for the old path and a cache hit for the new path, validating the correctness of cache updates during move operations.
429-443: Ensuring child cache entries are invalidated when parent node is movedThe
testMoveEntryOldChildMissmethod successfully tests that moving a parent node invalidates the cache entries of its children at the old path, ensuring that the cache reflects the new state of the filesystem.src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java (1)
486-486: Ensure consistent use of test doublesIn line 486,
CipherDiris instantiated with hard-coded values. Verify that mock objects or test doubles are consistently used where appropriate to isolate the unit tests and avoid dependencies on actual implementations.
| Path dirPath = mock(Path.class, "dirAbc"); | ||
| when(dirPath.getFileSystem()).thenReturn(fileSystem); | ||
| when(cryptoPathMapper.getCiphertextDir(path)).thenReturn(new CiphertextDirectory(dirId, dirPath)); | ||
| when(cryptoPathMapper.getCiphertextDir(path)).thenReturn(new CipherDir(dirId, dirPath)); |
There was a problem hiding this comment.
💡 Codebase verification
Remaining CiphertextDirectory Instances Detected
Several files still reference CiphertextDirectory, indicating incomplete refactoring. Please update the following to CipherDir:
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.javasrc/main/java/org/cryptomator/cryptofs/dir/CiphertextDirectoryDeleter.javasrc/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.javasrc/test/java/org/cryptomator/cryptofs/DeleteNonEmptyCiphertextDirectoryIntegrationTest.java- Additional test files under
src/test/java/org/cryptomator/cryptofs/
🔗 Analysis chain
LGTM: CiphertextDirectory replaced with CipherDir.
This change aligns with the PR objectives of refactoring to use more efficient records. The test logic remains intact, with only the return type of getCiphertextDir being updated.
To ensure consistency across the codebase, let's verify that all occurrences of CiphertextDirectory have been replaced with CipherDir:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of CiphertextDirectory
# Test 1: Search for CiphertextDirectory in Java files
echo "Searching for CiphertextDirectory in Java files:"
rg --type java "CiphertextDirectory"
# Test 2: Verify CipherDir usage
echo "Verifying CipherDir usage:"
rg --type java "CipherDir"
Length of output: 17471
src/test/java/org/cryptomator/cryptofs/attr/AttributeProviderTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/ClearToCipherDirCache.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/health/dirid/OrphanContentDir.java
Outdated
Show resolved
Hide resolved
src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java
Outdated
Show resolved
Hide resolved
overheadhunter
left a comment
overheadhunter
left a comment
There was a problem hiding this comment.
Looks good, just some wishes regarding names.
| import java.util.Objects; | ||
|
|
||
| //own file due to dagger | ||
| public record CipherDir(String dirId, Path contentDirPath) { |
There was a problem hiding this comment.
- make package-private
- rename
contentDirPathtoc9rDirPathfor consistency?
There was a problem hiding this comment.
i'll revert it to path for a small diff and consistency.
c9rDirPath is, from my point of view, the most misleading, because the directory with the c9r suffix contains the dir.c9r file with the dir-id. For a cleartext directory /foo/bar there are always two directories on the cipher side:
- the "linking" directory with the encrypted dir name, the c9r suffix and the dir-id as content
- the "content" directory with a bas64-encoded hash as name and with all encrypted files and other "linking" directories
There was a problem hiding this comment.
Also, this class cannot be package private. It is used in different packages, e.g. DirectoryStreamFactory or MissingContentDir.
src/main/java/org/cryptomator/cryptofs/CipherNodeNameParameters.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/ClearToCipherDirCache.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (15)
src/main/java/org/cryptomator/cryptofs/DirIdAndName.java (2)
5-11: Enhance documentation for better maintainability.
The current documentation could be improved in several ways:
- Expand on why this needs to be in its own file for Dagger (dependency injection)
- Document the parameter constraints (non-null requirements)
- Add examples of typical usage
Consider updating the documentation like this:
-//own file due to dagger
+// This class is in its own file to support Dagger's dependency injection requirements
+// and to maintain a clean separation of concerns in the codebase.
/**
* Helper object to store the dir id of a directory along with its cleartext name (aka, the last element in the cleartext path)
- * @param dirId
- * @param clearNodeName
+ * @param dirId The unique identifier of the directory (must be non-null)
+ * @param clearNodeName The unencrypted name of the directory as it appears in the virtual filesystem (must be non-null)
+ *
+ * @see CiphertextDirectory
+ * @see CiphertextDirCache
*/1-19: Document the record's role in the caching architecture.
This record is part of a larger caching mechanism refactoring. Consider adding class-level documentation that explains:
- Its relationship with
CiphertextDirectoryandCiphertextDirCache - Its role in the cache invalidation process
- Performance characteristics and usage patterns
This will help maintainers understand the design decisions and ensure proper usage of the cache invalidation system.
src/main/java/org/cryptomator/cryptofs/CiphertextDirectory.java (1)
6-13: Enhance documentation clarity and completeness.
The documentation could be improved in the following ways:
- Expand the comment about Dagger to explain why this needs to be in its own file
- Add more context about this record's role in the caching system
- Consider adding
@sincetag to track when this was introduced - Add
@seetags to reference related classes likeCryptoPathMapperandCiphertextDirCache
Example enhancement:
-//own file due to dagger
+// Separated into its own file to support Dagger's dependency injection requirements
+// and to maintain clean component boundaries
/**
* Represents a ciphertext directory without it's mount point in the virtual filesystem.
+ * This record is used by the caching system to maintain mappings between cleartext and
+ * ciphertext paths, improving performance by reducing I/O operations.
*
* @param dirId The (ciphertext) dir id (not encrypted, just a uuid)
* @param path The path to content directory (which contains the actual encrypted files and links to subdirectories)
+ * @since 1.x.x
+ * @see org.cryptomator.cryptofs.CryptoPathMapper
+ * @see org.cryptomator.cryptofs.CiphertextDirCache
*/src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java (1)
47-47: Enhance parameter documentation for clarity.
While the documentation is technically correct, it could be more informative by explaining what a dirId is and why it needs to be backed up.
Consider expanding the documentation like this:
-@param ciphertextDirectory A {@link CiphertextDirectory} for which the dirId should be back up'd.
+@param ciphertextDirectory A {@link CiphertextDirectory} containing the directory identifier that needs to be backed up for recovery purposes in case of filesystem corruption or metadata loss.src/main/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactory.java (2)
Line range hint 35-44: Consider documenting the thread-safety guarantees
Given that this class is part of the directory mapping system that's being refactored for better cache invalidation, it would be valuable to:
- Document the thread-safety guarantees of the new factory-based approach
- Add a comment explaining how this class interacts with the new caching mechanism
- Consider adding unit tests specifically for concurrent access patterns
This documentation would help maintain the correctness of the cache invalidation system as the codebase evolves.
Synchronization should be maintained for thread safety
The synchronization is still necessary as the class manages shared mutable state:
- A non-thread-safe
HashMapstoring stream mappings - A
volatile boolean closedflag that's checked and modified - Concurrent access to these fields during
newDirectoryStream()andclose()operations needs synchronization to maintain thread safety and prevent race conditions
The thread-safe factory mentioned in the TODO only ensures thread-safe creation of components but doesn't protect the shared state within DirectoryStreamFactory itself. Removing synchronization could lead to:
- Race conditions when adding/removing streams from the map
- Inconsistencies between the closed flag and stream operations
- Potential resource leaks if streams aren't properly tracked
🔗 Analysis chain
Line range hint 35-35: Verify thread safety before removing synchronization
While the TODO suggests that synchronization might be unnecessary due to the thread-safe factory, we should carefully verify this assumption. The newDirectoryStream method manages shared state through the streams map and closed flag.
Let's verify the thread safety of the factory:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the factory implementation and its thread safety guarantees
ast-grep --pattern 'class $_ implements DirectoryStreamComponent.Factory {
$$$
}'
# Look for any concurrent access patterns
rg -A 5 'synchronized|volatile|concurrent|atomic' --type java
Length of output: 1548
Script:
#!/bin/bash
# Let's search for the DirectoryStreamFactory class implementation to understand its thread safety
rg -A 20 "class DirectoryStreamFactory" --type java
# Look for any concurrent collections or synchronization in the factory
rg -A 10 "private.*Map|private.*Set|private.*List" --type java src/main/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactory.java
# Search for any concurrent access patterns in the factory and related classes
rg -A 5 "synchronized|volatile|concurrent|atomic" --type java src/main/java/org/cryptomator/cryptofs/dir/
Length of output: 8560
src/test/java/org/cryptomator/cryptofs/CiphertextDirCacheTest.java (3)
19-26: Consider adding more descriptive mock setup.
While the setup is functional, consider making it more maintainable by:
- Using meaningful names in mock setup (e.g.,
mockClearPath,mockDirLoader). - Adding comments to explain the expected behavior of mocks.
@BeforeEach
public void beforeEach() throws IOException {
cache = new CiphertextDirCache();
- clearPath = Mockito.mock(CryptoPath.class);
- dirLoader = Mockito.mock(CiphertextDirCache.CipherDirLoader.class);
- var cipherDir = Mockito.mock(CiphertextDirectory.class);
+ // Mock the cleartext path that will be used as cache key
+ clearPath = Mockito.mock(CryptoPath.class, "mockClearPath");
+ // Mock the loader that provides ciphertext directories
+ dirLoader = Mockito.mock(CiphertextDirCache.CipherDirLoader.class, "mockDirLoader");
+ // Mock a default ciphertext directory for the loader to return
+ var cipherDir = Mockito.mock(CiphertextDirectory.class, "mockCipherDir");
Mockito.when(dirLoader.load()).thenReturn(cipherDir);
}28-47: Enhance test readability and coverage.
Consider these improvements:
- Use more descriptive test method names following the given/when/then pattern.
- Add assertions to verify the cache state more thoroughly.
@Test
-public void testPuttingNewEntryTriggersLoader() throws IOException {
+public void givenEmptyCache_whenGettingNewEntry_thenLoaderIsTriggeredOnce() throws IOException {
var cipherDir = Mockito.mock(CiphertextDirectory.class);
Mockito.when(dirLoader.load()).thenReturn(cipherDir);
var result = cache.get(clearPath, dirLoader);
Assertions.assertEquals(cipherDir, result);
Mockito.verify(dirLoader).load();
+ Mockito.verifyNoMoreInteractions(dirLoader);
}
@Test
-public void testPuttingKnownEntryDoesNotTriggerLoader() throws IOException {
+public void givenCachedEntry_whenGettingSameEntry_thenLoaderIsNotTriggered() throws IOException {78-117: Improve documentation of complex mock setup.
The remap tests are thorough, but the mock setup could be more self-documenting:
@BeforeEach
public void beforeEach() throws IOException {
newClearPath = Mockito.mock(CryptoPath.class);
oldPrefixPath = Mockito.mock(CryptoPath.class);
newPrefixPath = Mockito.mock(CryptoPath.class);
+ // Setup path resolution behavior for remapping
+ // When old prefix is removed from a path, return the relative portion
Mockito.when(oldPrefixPath.relativize(Mockito.any(Path.class))).thenReturn(oldPrefixPath);
+ // When new prefix is added to a path, return the complete new path
Mockito.when(newPrefixPath.resolve(Mockito.any(Path.class))).thenReturn(newClearPath);
}Also, consider adding more descriptive comments in the test methods to explain the expected behavior:
cache.get(clearPath, dirLoader); //triggers loader
cache.recomputeAllKeysWithPrefix(oldPrefixPath, newPrefixPath);
-cache.get(clearPath, dirLoader); //does trigger
-cache.get(newClearPath, dirLoader); //does not trigger
+cache.get(clearPath, dirLoader); //triggers loader again as old path is invalidated
+cache.get(newClearPath, dirLoader); //doesn't trigger as it was remapped from old pathsrc/test/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactoryTest.java (1)
Line range hint 64-64: Update CiphertextDirectory usage to CipherDir.
These instances of CiphertextDirectory need to be updated to use CipherDir to maintain consistency with the refactoring changes identified in previous reviews.
Apply this change to all instances:
-new CiphertextDirectory(dirId, dirPath)
+new CipherDir(dirId, dirPath)Also applies to: 89-90
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java (1)
Line range hint 1-700: Consider adding documentation for the path() method.
While updating the access pattern from property to method is good, it would be helpful to document why this change was made and what guarantees the path() method provides (e.g., immutability, caching behavior, etc.).
src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java (1)
329-330: LGTM: Consistent refactoring to use the new CiphertextDirectory record.
The changes correctly replace the nested class with the new record class across all test methods. The usage of path() method is consistent and appropriate.
Consider extracting the repeated CiphertextDirectory setup into a helper method to reduce duplication:
private CiphertextDirectory createStepParentDir() {
CiphertextDirectory dir = new CiphertextDirectory("aaaaaa", dataDir.resolve("22/2222"));
Files.createDirectories(dir.path());
return dir;
}This would help maintain DRY principles and make the tests more maintainable.
Also applies to: 350-351, 378-379, 412-412, 445-445, 484-484, 527-528
src/main/java/org/cryptomator/cryptofs/CiphertextDirCache.java (2)
16-17: Review Cache Configuration Parameters
The cache is configured with a maximumSize of 5000 entries and an expireAfterWrite duration of 20 seconds.
Consider reviewing these parameters to ensure they align with the expected usage patterns and performance requirements. Adjusting the cache size or expiration policy may optimize memory usage and system performance.
11-13: Improve Class Documentation for Clarity
The class-level Javadoc could be enhanced to provide a clearer description of the CiphertextDirCache's purpose and usage.
Consider rephrasing the documentation:
/**
* Caches the mapping between cleartext directory paths and their corresponding ciphertext directories in the content directory.
*/src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java (1)
146-148: Typo in JavaDoc Comment
In the JavaDoc comment for @param cleartextDst, the word "childs" should be corrected to "children" for proper grammar.
Apply this diff to fix the typo:
* @param cleartextDst the destination cleartext path. The path itself and all
- * childs will be adjusted to start with cleartextDst.
+ * children will be adjusted to start with cleartextDst.📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
- src/main/java/org/cryptomator/cryptofs/CiphertextDirCache.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/CiphertextDirectory.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java (6 hunks)
- src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProvider.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java (5 hunks)
- src/main/java/org/cryptomator/cryptofs/DirIdAndName.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/attr/AbstractCryptoFileAttributeView.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/attr/AttributeProvider.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilter.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactory.java (2 hunks)
- src/main/java/org/cryptomator/cryptofs/health/dirid/MissingContentDir.java (2 hunks)
- src/main/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackup.java (2 hunks)
- src/main/java/org/cryptomator/cryptofs/health/dirid/OrphanContentDir.java (5 hunks)
- src/test/java/org/cryptomator/cryptofs/CiphertextDirCacheTest.java (1 hunks)
- src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (2 hunks)
- src/test/java/org/cryptomator/cryptofs/CryptoPathMapperTest.java (5 hunks)
- src/test/java/org/cryptomator/cryptofs/DirectoryIdBackupTest.java (4 hunks)
- src/test/java/org/cryptomator/cryptofs/attr/AttributeProviderTest.java (1 hunks)
- src/test/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilterTest.java (3 hunks)
- src/test/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactoryTest.java (1 hunks)
- src/test/java/org/cryptomator/cryptofs/health/dirid/MissingContentDirTest.java (2 hunks)
- src/test/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackupTest.java (2 hunks)
- src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java (11 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/cryptomator/cryptofs/attr/AttributeProviderTest.java
🧰 Additional context used
📓 Learnings (2)
src/main/java/org/cryptomator/cryptofs/CiphertextDirCache.java (1)
Learnt from: infeo
PR: cryptomator/cryptofs#253
File: src/main/java/org/cryptomator/cryptofs/ClearToCipherDirCache.java:59-68
Timestamp: 2024-10-22T08:36:41.464Z
Learning: In the `ClearToCipherDirCache` class, recursive writes to the cache are not allowed because the `CipherDirLoader` might alter the cache by recursive computation. Therefore, computation is performed outside of the atomic context, as suggested in the Caffeine wiki.
src/test/java/org/cryptomator/cryptofs/CiphertextDirCacheTest.java (2)
Learnt from: infeo
PR: cryptomator/cryptofs#253
File: src/test/java/org/cryptomator/cryptofs/ClearToCipherDirCacheTest.java:55-63
Timestamp: 2024-10-21T15:52:40.091Z
Learning: In the Java test file `ClearToCipherDirCacheTest.java`, within the `entryRemovedOnPrefixSuccess` test method, the cache miss is confirmed by verifying that the loader is triggered again, so adding an assertion to check the cache state is unnecessary.
Learnt from: infeo
PR: cryptomator/cryptofs#253
File: src/test/java/org/cryptomator/cryptofs/ClearToCipherDirCacheTest.java:115-115
Timestamp: 2024-10-21T15:56:44.806Z
Learning: In the Java test file `ClearToCipherDirCacheTest.java`, within the `entryUntouchedOnPrefixFailure` test method, the `dirLoader.load()` method is expected to be called twice, corresponding to invocations at lines 110 and 113.
🔇 Additional comments (51)
src/main/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilter.java (2)
31-31: LGTM! Clean transition to record accessor.
The change from .path to .path() aligns with the PR's objective to utilize records instead of classes, following Java best practices for immutable data structures.
31-31: Verify consistent usage of path() accessor across the codebase.
Let's ensure this refactoring has been consistently applied across all usages of CiphertextDirectory.
src/main/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackup.java (2)
3-3: LGTM! Good dependency reduction.
The removal of the CryptoPathMapper dependency in favor of directly importing CiphertextDirectory helps reduce coupling and aligns with the PR's objective of using more efficient record types.
32-32: LGTM! Clean refactoring to use the new CiphertextDirectory record.
The change simplifies the code by directly using the standalone CiphertextDirectory record while maintaining the same functionality. This refactoring improves testability and code organization.
src/main/java/org/cryptomator/cryptofs/health/dirid/MissingContentDir.java (2)
3-3: LGTM: Clean import refactoring
The direct import of CiphertextDirectory aligns with the PR's goal of streamlining the codebase and removing nested class dependencies.
54-54: LGTM: Verify consistent CiphertextDirectory usage
The direct instantiation of CiphertextDirectory is correct and maintains the same functionality while following the new pattern.
Let's verify that this refactoring pattern is consistently applied across the codebase:
✅ Verification successful
Let me check the actual usage patterns in these files to verify consistency.
Based on the comprehensive code analysis, I can now provide the final response:
Verified: CiphertextDirectory usage is consistent
The direct instantiation of CiphertextDirectory in MissingContentDir.java follows the established pattern. All usages across the codebase consistently use direct instantiation of CiphertextDirectory as a record class, and there are no remaining references to CryptoPathMapper.CiphertextDirectory. Key evidence:
CiphertextDirectoryis properly defined as a record in its own file- All instantiations use the direct constructor pattern
new CiphertextDirectory(dirId, path) - The class is used consistently across main and test code
- No deprecated or legacy patterns found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of CiphertextDirectory across the codebase
# Expected: All instances should use the direct class reference, not through CryptoPathMapper
# Check for any remaining references to CryptoPathMapper.CiphertextDirectory
rg -l "CryptoPathMapper\.CiphertextDirectory"
# Check for direct usage of CiphertextDirectory
rg -l "new CiphertextDirectory\("
Length of output: 864
Script:
#!/bin/bash
# Get the actual context of CiphertextDirectory usage
rg "CiphertextDirectory" -B2 -A2
# Specifically check CryptoPathMapper.java since it's the original source
cat src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java
Length of output: 61478
src/test/java/org/cryptomator/cryptofs/health/dirid/MissingDirIdBackupTest.java (2)
3-3: LGTM! Import change aligns with refactoring goals.
The direct import of CiphertextDirectory instead of accessing it through CryptoPathMapper reduces coupling and aligns with the PR's objective of utilizing more efficient records.
46-46: LGTM! Consider adding edge cases to the test suite.
The updated matcher correctly uses the record's methods and maintains the test's verification logic. However, since this test is crucial for vault integrity, consider adding test cases for:
- Paths with special characters
- Very long directory IDs
- Invalid directory IDs
Let's verify if these edge cases are covered elsewhere:
src/test/java/org/cryptomator/cryptofs/dir/BrokenDirectoryFilterTest.java (2)
3-3: LGTM: Import aligns with the new standalone CiphertextDirectory record.
The import change correctly reflects the architectural change from an inner class to a standalone record.
35-35: LGTM: Consistent usage of the new CiphertextDirectory record.
The mock setup has been correctly updated to use the new standalone CiphertextDirectory record in both test methods. The behavior and test coverage remain unchanged.
Let's verify that all other test classes have been updated consistently:
Also applies to: 49-49
✅ Verification successful
All test classes consistently use the standalone CiphertextDirectory record
The verification shows that:
- No references to the old
CryptoPathMapper.CiphertextDirectorywere found - All test classes consistently use the new standalone
CiphertextDirectoryrecord with the correct constructor signature (dirId, path)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to CryptoPathMapper.CiphertextDirectory
# Expected: No matches, as all references should use the new standalone CiphertextDirectory
# Search for any remaining old-style references
rg -l "CryptoPathMapper\.CiphertextDirectory"
# Search for test files using CiphertextDirectory to verify consistent usage
rg "new CiphertextDirectory\(" "src/test/"
Length of output: 6119
src/test/java/org/cryptomator/cryptofs/DirectoryIdBackupTest.java (4)
25-25: LGTM! Improved type and variable naming.
The change from nested CryptoPathMapper.CiphertextDirectory to standalone CiphertextDirectory aligns with good architectural practices by reducing coupling. The new variable name is also more descriptive.
34-34: LGTM! Initialization properly updated.
The initialization correctly uses the new CiphertextDirectory constructor while maintaining the same test setup logic.
47-47: LGTM! Test methods properly updated.
The test methods have been consistently updated to use the renamed variable while maintaining their original test coverage and assertions.
Also applies to: 61-61
Line range hint 1-67: Verify test coverage for new caching behavior.
While these tests cover the directory ID backup functionality well, we should ensure there are comprehensive tests for the new cache invalidation behavior mentioned in the PR objectives, especially given the complexity change from O(log n) to O(n).
Let's check for related test files:
✅ Verification successful
Let's examine the content of these test files to understand the cache invalidation test coverage.
Let's search for any new cache-related tests that might have been added as part of this change, and also look for the implementation to understand what kind of tests we should expect.
Cache complexity changes are well tested and documented
The codebase shows comprehensive test coverage for the new caching behavior:
ClearToCipherDirCacheTest.javacontains dedicated unit tests for the new cache implementation- The O(n) complexity is explicitly documented in the code comments with worst-case scenarios:
removeAllKeysWithPrefix: O(n*(x+y)) for recursive directory deletionrecomputeAllKeysWithPrefix: O(nx + ny + n*z) for moving deeply nested directories
- Test cases cover both cache invalidation and remapping scenarios with nested directory structures
- Performance implications are clearly documented with examples like "moving the root of a deeply nested dir, where all child dirs are cached"
The original review comment's concern about test coverage has been addressed by the recent commits which added comprehensive tests for the new caching behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cache-related test files
echo "Searching for cache-related test files..."
fd -e java -e kt test.*Cache.*Test
# Search for test methods related to cache invalidation
echo -e "\nSearching for cache invalidation test methods..."
rg -l "test.*[Cc]ache.*[Ii]nvalidat|test.*[Ii]nvalidat.*[Cc]ache" -g "*Test.{java,kt}"
Length of output: 471
Script:
#!/bin/bash
# Check cache invalidation test methods in OpenCryptoFileTest
echo "=== Cache invalidation tests in OpenCryptoFileTest ==="
rg "test.*[Cc]ache.*[Ii]nvalidat|test.*[Ii]nvalidat.*[Cc]ache" -A 10 src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java
echo -e "\n=== Cache invalidation tests in CleartextFileChannelTest ==="
rg "test.*[Cc]ache.*[Ii]nvalidat|test.*[Ii]nvalidat.*[Cc]ache" -A 10 src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java
# Look for cache complexity related tests
echo -e "\n=== Tests mentioning cache complexity or performance ==="
rg "test.*[Cc]ache.*([Cc]omplex|[Pp]erform)" -A 10 -g "*Test.java"
Length of output: 1586
Script:
#!/bin/bash
# Find cache-related implementation files
echo "=== Cache implementation files ==="
fd -e java -e kt Cache
# Look for recent changes in cache-related files
echo -e "\n=== Cache-related changes in recent commits ==="
git log -p --grep="cache\|Cache" -i --max-count=3
# Search for O(n) complexity mentions
echo -e "\n=== O(n) complexity mentions ==="
rg "O\(n\)" -A 5 -B 5
Length of output: 29954
src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java (4)
29-31: LGTM! Documentation updates are clear and accurate.
The Javadoc changes properly reflect the refactored CiphertextDirectory reference and maintain clear documentation of the backup file naming convention.
36-39: Verify the atomic file creation security.
The use of StandardOpenOption.CREATE_NEW helps prevent race conditions by failing if the file already exists. However, ensure that the parent directory's permissions are properly set to prevent unauthorized access during the small window between existence check and file creation.
Consider adding filesystem permission checks before attempting to create the backup file to ensure the operation will be secure.
50-50: LGTM! Method signature update is appropriate.
The static helper method is well-designed, properly documented, and maintains good separation of concerns by delegating to the instance method.
36-50: Verify integration with cache invalidation system.
Since this PR aims to fix cache invalidation issues, we should verify that directory ID backups are properly handled when directories are moved or deleted.
✅ Verification successful
Based on the search results, I can see that the DirectoryIdBackup class is used to backup directory IDs during filesystem operations, and there is extensive integration with the cache invalidation system. The key findings are:
-
Directory ID backups are properly handled during:
- Directory deletion (via
CryptoFileSystemImpl.deleteDirectory()) - Directory moves (via
CryptoFileSystemImpl.moveDirectory()) - Directory creation (with error handling that cleans up the backup file)
- Directory deletion (via
-
The cache invalidation system is properly integrated:
cryptoPathMapper.invalidatePathMapping()is called after directory operationsdirIdProvider.delete()anddirIdProvider.move()are called to handle cache updates- The backup file is cleaned up if directory creation fails
-
The error handling in DirectoryIdBackup.execute() uses CREATE_NEW to prevent overwriting existing backups, which is the correct approach for maintaining filesystem integrity.
Therefore, I can conclude that the DirectoryIdBackup class is properly integrated with the cache invalidation system and has appropriate error handling.
✓ Directory ID backup system is properly integrated with cache invalidation
The implementation ensures directory ID backups are maintained consistently with the cache state through proper invalidation and cleanup during all directory operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cache invalidation calls around directory operations
rg -A 5 -B 5 "invalidate|remove|delete|move" --type java
# Look for any error handling related to backup file operations
ast-grep --pattern 'try {
$$$
DirectoryIdBackup
$$$
} catch ($$$) {
$$$
}'
Length of output: 278083
src/main/java/org/cryptomator/cryptofs/attr/AttributeProvider.java (1)
56-56: LGTM! Good transition to record accessor.
The change from property access to method call aligns with Java records best practices and maintains better encapsulation.
Let's verify that similar changes have been made consistently across the codebase:
✅ Verification successful
All path() method calls are consistent across the codebase
The verification shows that all instances of getCiphertextDir() in the codebase consistently use the record accessor method path(). No instances of direct property access were found, confirming that the migration to record accessors has been completed thoroughly.
Found in:
- CryptoFileSystemImpl.java (7 occurrences)
- AttributeProvider.java (1 occurrence)
- AbstractCryptoFileAttributeView.java (1 occurrence)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct property access to 'path'
# that might need to be updated to use the method call 'path()'
# Search for remaining direct property access patterns
echo "Checking for potential inconsistent path property access:"
rg -p "getCiphertextDir\([^)]+\)\.path[^(]"
# Search for the new method call pattern to confirm consistency
echo "Checking current path() method call usage:"
rg -p "getCiphertextDir\([^)]+\)\.path\(\)"
Length of output: 1275
src/main/java/org/cryptomator/cryptofs/attr/AbstractCryptoFileAttributeView.java (1)
63-63: LGTM! Consistent with record accessor pattern.
The change from field access .path to method call .path() aligns with Java records best practices and improves encapsulation.
Let's verify the consistent usage of this pattern across the codebase:
✅ Verification successful
All usages of getCiphertextDir().path() are consistent across the codebase
The verification shows that all instances of accessing the path from getCiphertextDir() use the correct record accessor pattern .path(). No instances of direct field access were found. The pattern is consistently used across:
CryptoFileSystemImpl.javaAbstractCryptoFileAttributeView.javaAttributeProvider.java
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of CiphertextDirectory.path() across the codebase
# Expect: All accesses should use .path() method call, not direct field access
# Search for any remaining direct field access patterns
rg "getCiphertextDir\([^)]+\)\.path[^()]"
# Search for the correct method call pattern for comparison
rg "getCiphertextDir\([^)]+\)\.path\(\)"
Length of output: 1403
src/main/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactory.java (2)
3-3: LGTM: Import change aligns with architectural improvements
The direct import of CiphertextDirectory from the base package aligns with the PR's objective of using records for better efficiency.
40-41: Verify consistent usage of method calls across codebase
The change from field access to method calls (.path() and .dirId()) aligns with the record-based approach. However, we should ensure this pattern is consistently applied throughout the codebase.
Let's verify the consistent usage:
#!/bin/bash
# Search for any remaining field access patterns that should be method calls
rg -A 2 'ciphertextDir\.(path|dirId)[^(]'
# Verify the new method call pattern is used consistently
rg -A 2 'ciphertextDir\.(path|dirId)\(\)'src/test/java/org/cryptomator/cryptofs/health/dirid/MissingContentDirTest.java (2)
3-3: LGTM! Import change aligns with architectural improvements.
The change from nested CryptoPathMapper.CiphertextDirectory to standalone CiphertextDirectory aligns with the PR's objective of utilizing more efficient records instead of classes.
63-63: Verify consistent usage of CiphertextDirectory across tests.
The ArgumentMatcher update correctly uses the new record-style accessor methods. Let's verify this pattern is consistently applied across the test suite.
✅ Verification successful
CiphertextDirectory usage is consistent across test suite
Based on the comprehensive search results, all test cases consistently use the record-style accessor methods dirId() and path() when working with CiphertextDirectory. The ArgumentMatcher implementations in MissingContentDirTest and MissingDirIdBackupTest confirm this pattern. No instances of deprecated or inconsistent access patterns were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of the old CryptoPathMapper.CiphertextDirectory pattern
# or inconsistent property access patterns in tests
# Test 1: Check for any remaining nested CiphertextDirectory references
echo "Checking for old nested CiphertextDirectory references..."
rg "CryptoPathMapper\.CiphertextDirectory" "src/test"
# Test 2: Look for potential inconsistent property access patterns
echo "Checking for inconsistent property access patterns..."
rg -A 2 "CiphertextDirectory.*\{.*\}" "src/test"
Length of output: 344
Script:
#!/bin/bash
# Let's try a different approach to find CiphertextDirectory usage patterns
# Search for CiphertextDirectory usage in test files
echo "Searching for CiphertextDirectory usage patterns..."
rg "CiphertextDirectory" "src/test" -B 1 -A 1
# Search specifically for dirId() method calls
echo "Searching for dirId() method calls..."
rg "\.dirId\(\)" "src/test" -B 1 -A 1
# Search for path() method calls in context
echo "Searching for path() method calls..."
rg "\.path\(\)" "src/test" -B 1 -A 1
# Look for ArgumentMatcher usage with CiphertextDirectory
echo "Searching for ArgumentMatcher with CiphertextDirectory..."
rg "ArgumentMatcher.*CiphertextDirectory" "src/test" -B 1 -A 1
Length of output: 31721
src/test/java/org/cryptomator/cryptofs/CiphertextDirCacheTest.java (2)
1-17: LGTM! Well-structured test class setup.
The imports and class structure follow testing best practices, with appropriate use of JUnit 5 and Mockito frameworks.
49-75: LGTM! Comprehensive removal test cases.
The tests effectively verify the cache invalidation behavior, which is a key objective of this PR. The verification of loader calls is sufficient to confirm cache state, as per previous feedback.
Let's verify that all cache invalidation scenarios are covered:
✅ Verification successful
Based on the results from our verification scripts, I can now provide the final response:
Cache invalidation tests are complete and well-structured
The cache invalidation implementation is properly tested with:
- Successful prefix removal case (triggers cache reload)
- Failed prefix match case (preserves cache entry)
- Integration with
CryptoPathMapperfor path-based invalidation
The implementation in CiphertextDirCache uses a simple and effective approach with removeIf() on the cache keys, which is correctly verified by these test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other cache invalidation scenarios in the codebase
rg -l "removeAllKeysWithPrefix|invalidateCache|clearCache" --type java
Length of output: 263
Script:
#!/bin/bash
# Let's check the implementation and usage of cache invalidation methods
rg "removeAllKeysWithPrefix|invalidateCache|clearCache" --type java -A 5
Length of output: 2428
Script:
#!/bin/bash
# Let's check if there are any other test classes that might be testing cache-related functionality
ast-grep --pattern 'class $_ {
$$$
@Test
$$$
cache.$_($$$)
$$$
}'
Length of output: 77
src/test/java/org/cryptomator/cryptofs/dir/DirectoryStreamFactoryTest.java (1)
3-3:
Import statement updated but class name needs revision.
The import has been correctly moved to use the standalone CiphertextDirectory class. However, based on previous review comments, this class should be renamed to CipherDir for consistency with the ongoing refactoring.
src/main/java/org/cryptomator/cryptofs/health/dirid/OrphanContentDir.java (2)
Line range hint 157-172: LGTM! Good improvements in error handling.
The changes improve robustness by properly handling backup file creation failures and using the new CiphertextDirectory record type.
218-232:
Consider improving error handling and thread safety.
The method performs multiple file operations that could fail or be affected by concurrent access:
- The
Files.moveoperations could fail if the target already exists - The file channel operations could fail due to I/O errors
- Concurrent access to the same files could cause race conditions
Let's check if there are any concurrent accesses to these paths:
Consider wrapping the operations in a try-catch block and implementing proper synchronization:
void adoptOrphanedResource(Path oldCipherPath, String newClearName, boolean isShortened, CiphertextDirectory stepParentDir, FileNameCryptor cryptor, MessageDigest sha1) throws IOException {
var newCipherName = encrypt(cryptor, newClearName, stepParentDir.dirId());
if (isShortened) {
var deflatedName = BaseEncoding.base64Url().encode(sha1.digest(newCipherName.getBytes(StandardCharsets.UTF_8))) + Constants.DEFLATED_FILE_SUFFIX;
Path targetPath = stepParentDir.path().resolve(deflatedName);
- Files.move(oldCipherPath, targetPath);
+ try {
+ Files.move(oldCipherPath, targetPath, StandardCopyOption.ATOMIC_MOVE);
+ } catch (FileAlreadyExistsException e) {
+ LOG.warn("Target file already exists: {}", targetPath, e);
+ throw e;
+ }
//adjust name.c9s
- try (var fc = Files.newByteChannel(targetPath.resolve(Constants.INFLATED_FILE_NAME), StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) {
- fc.write(ByteBuffer.wrap(newCipherName.getBytes(StandardCharsets.UTF_8)));
+ Path inflatedNameFile = targetPath.resolve(Constants.INFLATED_FILE_NAME);
+ try {
+ synchronized (this) {
+ try (var fc = Files.newByteChannel(inflatedNameFile,
+ StandardOpenOption.WRITE,
+ StandardOpenOption.CREATE,
+ StandardOpenOption.TRUNCATE_EXISTING)) {
+ fc.write(ByteBuffer.wrap(newCipherName.getBytes(StandardCharsets.UTF_8)));
+ }
+ }
+ } catch (IOException e) {
+ LOG.error("Failed to write inflated filename: {}", inflatedNameFile, e);
+ throw e;
}
} else {
Path targetPath = stepParentDir.path().resolve(newCipherName);
- Files.move(oldCipherPath, targetPath);
+ try {
+ Files.move(oldCipherPath, targetPath, StandardCopyOption.ATOMIC_MOVE);
+ } catch (FileAlreadyExistsException e) {
+ LOG.warn("Target file already exists: {}", targetPath, e);
+ throw e;
+ }
}
}✅ Verification successful
Based on the search results, I can see that:
- The
Files.moveoperations in the codebase generally handle atomic moves and error cases, especially inCryptoFileSystemImplandC9rConflictResolver. - The
Files.newByteChanneloperations are typically wrapped in try-with-resources blocks with proper error handling. - The codebase already implements synchronization and error handling patterns in similar scenarios.
Consider using atomic move operations and proper error handling.
The method should use StandardCopyOption.ATOMIC_MOVE for the move operations and handle potential exceptions:
void adoptOrphanedResource(Path oldCipherPath, String newClearName, boolean isShortened, CiphertextDirectory stepParentDir, FileNameCryptor cryptor, MessageDigest sha1) throws IOException {
var newCipherName = encrypt(cryptor, newClearName, stepParentDir.dirId());
if (isShortened) {
var deflatedName = BaseEncoding.base64Url().encode(sha1.digest(newCipherName.getBytes(StandardCharsets.UTF_8))) + Constants.DEFLATED_FILE_SUFFIX;
Path targetPath = stepParentDir.path().resolve(deflatedName);
Files.move(oldCipherPath, targetPath, StandardCopyOption.ATOMIC_MOVE);
//adjust name.c9s
Path inflatedNameFile = targetPath.resolve(Constants.INFLATED_FILE_NAME);
try (var fc = Files.newByteChannel(inflatedNameFile,
StandardOpenOption.WRITE,
StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING)) {
fc.write(ByteBuffer.wrap(newCipherName.getBytes(StandardCharsets.UTF_8)));
}
} else {
Path targetPath = stepParentDir.path().resolve(newCipherName);
Files.move(oldCipherPath, targetPath, StandardCopyOption.ATOMIC_MOVE);
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for concurrent access patterns to the same paths
rg -A 5 "Files\.(move|newByteChannel)"
Length of output: 21534
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProvider.java (1)
158-158: LGTM: Constructor call updated to use standalone CiphertextDirectory.
The change correctly adapts to the refactored CiphertextDirectory class structure, maintaining the same functionality while aligning with the broader improvements to the caching system.
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java (5)
144-144: LGTM: Method call updated correctly.
The change from accessing the path as a property to invoking it as a method aligns with the refactoring to use the new CiphertextDirectory record.
434-434: LGTM: Method call updated correctly.
The change from accessing the path as a property to invoking it as a method is consistent with the refactoring.
624-624: LGTM: Method call updated correctly.
The change from accessing the path as a property to invoking it as a method is consistent with the refactoring.
507-507: 🛠️ Refactor suggestion
Verify directory stream resource management.
The code properly uses try-with-resources for the DirectoryStream, but there's a potential resource leak if an exception occurs between the two directory operations.
Consider combining the directory operations or ensuring both are properly cleaned up in case of failures:
-Path ciphertextSourceDir = cryptoPathMapper.getCiphertextDir(cleartextSource).path();
-Path ciphertextTargetDir = cryptoPathMapper.getCiphertextDir(cleartextTarget).path();
+var sourceDir = cryptoPathMapper.getCiphertextDir(cleartextSource);
+var targetDir = cryptoPathMapper.getCiphertextDir(cleartextTarget);
+try {
+ copyAttributes(sourceDir.path(), targetDir.path());
+} catch (IOException e) {
+ // Handle cleanup if needed
+ throw e;
+}Also applies to: 517-518
318-318: Verify atomic directory creation.
The directory creation process involves multiple steps that need to be atomic to prevent race conditions:
- Creating the raw path
- Writing the directory ID
- Creating the actual directory
- Persisting the long file name
While the code handles cleanup on failure, there might be a small window where concurrent operations could lead to inconsistencies.
Consider wrapping the directory creation steps in a synchronized block or using file system locks to ensure atomicity.
Also applies to: 325-325, 329-329, 333-333
src/test/java/org/cryptomator/cryptofs/health/dirid/OrphanDirTest.java (1)
4-4: LGTM: Import added for the new CiphertextDirectory class.
The import is correctly placed and aligns with the refactoring from nested class to standalone class.
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (2)
1260-1264: LGTM! Variable name improvement
The variable rename from cipherDirObject to ciphertextDirectoryObject improves code clarity by using the full term "ciphertext" consistently with the rest of the codebase.
1280-1280: LGTM! Added test coverage for directory ID backup
Added verification that dirIdBackup.execute() is called with the correct ciphertextDirectoryObject parameter, which aligns with the PR objective of properly handling directory cache invalidation.
src/main/java/org/cryptomator/cryptofs/CiphertextDirCache.java (2)
40-51:
Address Thread Safety in Recomputing Cache Keys
Manipulating the cache using ciphertextDirectories.asMap().entrySet().removeIf(...) can introduce thread safety issues due to concurrent access.
To ensure thread-safe recomputation of cache keys, consider the following approach:
var entriesToRecompute = ciphertextDirectories.synchronous().asMap().entrySet().stream()
.filter(e -> e.getKey().startsWith(oldPrefix))
.collect(Collectors.toList());
entriesToRecompute.forEach(e -> {
var remappedPath = newPrefix.resolve(oldPrefix.relativize(e.getKey()));
ciphertextDirectories.synchronous().asMap().remove(e.getKey());
ciphertextDirectories.put(remappedPath, e.getValue());
});This code safely collects the entries to be recomputed and performs removal and insertion operations in a thread-safe manner.
⛔ Skipped due to learnings
Learnt from: infeo
PR: cryptomator/cryptofs#253
File: src/main/java/org/cryptomator/cryptofs/ClearToCipherDirCache.java:26-28
Timestamp: 2024-10-22T08:34:19.526Z
Learning: In the Java project using Caffeine's `AsyncCache`, the `asMap()` method returns a thread-safe map, so modifying it directly is thread-safe. Therefore, in `ClearToCipherDirCache.java`, using `ciphertextDirectories.asMap().keySet().removeIf(...)` is acceptable and thread-safe.
29-31:
Ensure Thread-Safe Removal of Cache Entries
The use of ciphertextDirectories.asMap().keySet().removeIf(p -> p.startsWith(basePrefix)); may not be thread-safe, as concurrent modifications to the cache could lead to inconsistent states.
Consider using the cache's built-in thread-safe methods for removal. You can replace the current implementation with:
ciphertextDirectories.synchronous().asMap().keySet().removeIf(p -> p.startsWith(basePrefix));Alternatively, iterate over the keys and remove them safely:
ciphertextDirectories.synchronous().invalidateAll(
ciphertextDirectories.synchronous().asMap().keySet().stream()
.filter(p -> p.startsWith(basePrefix))
.collect(Collectors.toSet())
);⛔ Skipped due to learnings
Learnt from: infeo
PR: cryptomator/cryptofs#253
File: src/main/java/org/cryptomator/cryptofs/ClearToCipherDirCache.java:26-28
Timestamp: 2024-10-22T08:34:19.526Z
Learning: In the Java project using Caffeine's `AsyncCache`, the `asMap()` method returns a thread-safe map, so modifying it directly is thread-safe. Therefore, in `ClearToCipherDirCache.java`, using `ciphertextDirectories.asMap().keySet().removeIf(...)` is acceptable and thread-safe.
Learnt from: infeo
PR: cryptomator/cryptofs#253
File: src/main/java/org/cryptomator/cryptofs/ClearToCipherDirCache.java:59-68
Timestamp: 2024-10-22T08:36:41.464Z
Learning: In the `ClearToCipherDirCache` class, recursive writes to the cache are not allowed because the `CipherDirLoader` might alter the cache by recursive computation. Therefore, computation is performed outside of the atomic context, as suggested in the Caffeine wiki.
src/test/java/org/cryptomator/cryptofs/CryptoPathMapperTest.java (6)
78-78: Update assertions to match new method return types
The addition of .path() indicates that getCiphertextDir() now returns an object with a path() method. The assertion correctly compares the expected Path object with the actual path.
102-102: Consistent use of .path() with getCiphertextDir()
The modification ensures that the retrieved ciphertext directory's path is obtained correctly for the test case.
136-136: Correctly accessing the path for deeper directory levels
Adding .path() ensures that the test retrieves the actual Path object for the /foo/bar directory, aligning with the method's updated return type.
337-338: Well-structured nested test class for cache invalidation
Introducing the InvalidateOrMovePathMapping nested class groups the related test cases logically, enhancing readability and maintainability of the test suite.
425-427: Validate cache behavior after moving path mappings
In testMoveEntryOldMissNewHit(), verifying that getCiphertextFilePath(fooPath) is called (indicating a cache miss) and that getCiphertextFilePath(kikPath) is not called (indicating a cache hit) correctly tests the desired cache behavior.
441-443: Ensure cache invalidation for child paths after moving a node
The test testMoveEntryOldChildMiss() correctly verifies that after moving a node, child paths of the old location result in cache misses. This is important to ensure that stale cache entries are not used.
src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java (5)
45-45: Proper Declaration of 'ciphertextDirCache'
The ciphertextDirCache field is correctly declared and will facilitate efficient caching of directory paths.
57-57: Correct Initialization of 'ciphertextDirCache'
The ciphertextDirCache is properly initialized in the constructor, ensuring the cache is ready for use.
66-66: Improved Documentation with '@throws IOException'
Adding the @throws IOException tag enhances the method documentation by clearly specifying potential I/O exceptions.
118-118: Accurate Retrieval of Ciphertext File Path
The call to getCiphertextFilePath(parent.path(), parent.dirId(), cleartextName) correctly retrieves the ciphertext file path using the parent directory's path and ID.
158-161:
Undefined Variable 'dirFile' in Lambda Expression
The variable dirFile is used in the lambda expression but is not defined within its scope, leading to a potential compilation error.
Apply this diff to define dirFile within the lambda:
CiphertextDirCache.CipherDirLoader cipherDirLoaderIfAbsent = () -> {
+ Path dirFile = getCiphertextFilePath(cleartextPath).getDirFilePath();
return resolveDirectory(dirFile);
};Likely invalid or redundant comment.
| public record CiphertextDirectory(String dirId, Path path) { | ||
|
|
||
| public CiphertextDirectory(String dirId, Path path) { | ||
| this.dirId = Objects.requireNonNull(dirId); | ||
| this.path = Objects.requireNonNull(path); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding format validation for dirId.
While null checks are good, consider adding validation for the dirId format since it's mentioned to be a UUID in the documentation.
Here's a suggested implementation:
public record CiphertextDirectory(String dirId, Path path) {
+ private static final Pattern UUID_PATTERN =
+ Pattern.compile("^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$");
public CiphertextDirectory(String dirId, Path path) {
this.dirId = Objects.requireNonNull(dirId);
+ if (!UUID_PATTERN.matcher(dirId).matches()) {
+ throw new IllegalArgumentException("dirId must be a valid UUID");
+ }
this.path = Objects.requireNonNull(path);
}
}Don't forget to add the import:
import java.util.regex.Pattern;
Closes #252.
This PR fixes a problem of cache invalidation in the CryptoPathMapper.
The fix itself is trivial: Instead of only removing/moving the requested entry, we iterate over the whole cache and remove/move all entries, where the cleartextPath starts with the requested cleartext path. Unfortunately, this drastically increases the runtime of the operation from O(log n) to O(n) (assuming finding a single entry needs O(log n) time), where n is the size of the cache.
For proper testing and using more efficient records instead of class, some refactorings were applied.