Use Conflict Suffixes from Ciphertext Files #275
Conversation
fallback to numerical conflict suffixes only if name is already taken
WalkthroughThe pull request introduces updates to the Maven project configuration and several Java classes. In the Maven Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/java/org/cryptomator/cryptofs/dir/Base64UrlRegexTest.java (1)
127-133: Consider using parameterized inputs for benchmarking.The benchmark could be more comprehensive by parameterizing the test inputs.
@Benchmark public void testPattern(Blackhole bh) { - for (String input : TEST_INPUTS) { + @Param({"short", "medium", "long"}) + private String inputType; + + private String[] inputs; + + @Setup(Level.Trial) + public void setupInputs() { + switch(inputType) { + case "short": inputs = new String[]{"aaaaBBBBccccDDDDeeeeFFFF"}; break; + case "medium": inputs = new String[]{"aaaaBBBBccccDDDDeeeeFFFFggggHH=="}; break; + case "long": inputs = new String[]{"-3h6-FSsYhMCJHSAV9cjJ89F7R73b0zIB4vvO01b7uWq28fWioRagWpMv-w0MA-2ORjbShuv"}; break; + } + } + + @Benchmark + public void testPattern(Blackhole bh) { + for (String input : inputs) { Matcher matcher = pattern.matcher(input); bh.consume(matcher.matches()); } }src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java (2)
90-109: Consider extracting magic numbers into constants.The logic for handling conflict suffixes and length restrictions is well-implemented and documented. However, consider extracting magic numbers like
5(minimum conflict suffix length) into named constants for better maintainability.+ private static final int MIN_CONFLICT_SUFFIX_LENGTH = 5; + private static final double CONFLICT_SUFFIX_MAX_RATIO = 0.5; // 50% of available space ... - final int conflictSuffixLen = Math.max(5, conflictSuffix.length()); + final int conflictSuffixLen = Math.max(MIN_CONFLICT_SUFFIX_LENGTH, conflictSuffix.length()); - final String conflictSuffix = originalConflictSuffix.substring(0, Math.min(originalConflictSuffix.length(), netCleartext / 2)); + final String conflictSuffix = originalConflictSuffix.substring(0, Math.min(originalConflictSuffix.length(), (int)(netCleartext * CONFLICT_SUFFIX_MAX_RATIO)));
106-108: Consider extracting magic numbers and improving division logicA few suggestions for improvement:
- The magic number '5' for minimum suffix length should be a named constant
- The integer division for space allocation could be made more explicit
+ private static final int MIN_CONFLICT_SUFFIX_LENGTH = 5; + private static final double CONFLICT_SUFFIX_SPACE_RATIO = 0.5; // 50% of available space - final String conflictSuffix = originalConflictSuffix.substring(0, Math.min(originalConflictSuffix.length(), netCleartext / 2)); - final int conflictSuffixLen = Math.max(5, conflictSuffix.length()); + final int maxConflictSuffixLength = (int)(netCleartext * CONFLICT_SUFFIX_SPACE_RATIO); + final String conflictSuffix = originalConflictSuffix.substring(0, Math.min(originalConflictSuffix.length(), maxConflictSuffixLength)); + final int conflictSuffixLen = Math.max(MIN_CONFLICT_SUFFIX_LENGTH, conflictSuffix.length());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(3 hunks)src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java(2 hunks)src/main/java/org/cryptomator/cryptofs/dir/C9rDecryptor.java(2 hunks)src/test/java/org/cryptomator/cryptofs/dir/Base64UrlRegexTest.java(1 hunks)src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java(3 hunks)src/test/java/org/cryptomator/cryptofs/dir/C9rDecryptorTest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse
🔇 Additional comments (19)
src/main/java/org/cryptomator/cryptofs/dir/C9rDecryptor.java (2)
23-23: LGTM! The regex pattern is now more robust.The updated pattern correctly handles Base64URL-encoded strings with proper padding rules:
- Requires exactly 20 base64url chars at the start
- Allows groups of 4 base64url chars in the middle
- Handles proper base64url padding at the end
39-39: LGTM! Simplified stream return logic.Using
Optional.stream()is a cleaner way to convert an Optional to a Stream.src/test/java/org/cryptomator/cryptofs/dir/C9rDecryptorTest.java (2)
37-37: LGTM! Added test case for valid Base64URL pattern.The new test case validates Base64URL strings with proper padding and special characters.
48-50: LGTM! Added test cases for invalid Base64URL patterns.The new test cases validate that the regex correctly rejects:
- Strings with excessive padding
- Strings with padding in wrong positions
src/test/java/org/cryptomator/cryptofs/dir/Base64UrlRegexTest.java (2)
35-45: LGTM! Comprehensive test data for Base64URL patterns.The test data covers various scenarios:
- Standard Base64URL strings
- Strings with padding
- Strings with special characters
- Long Base64URL strings
101-107: LGTM! Well-configured JMH benchmark.The benchmark is properly configured with:
- Multiple forks for statistical significance
- Appropriate warmup and measurement iterations
- Average time measurement mode
- Microsecond output unit
src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java (3)
34-34: LGTM! Updated shorteningThreshold for longer cleartext names.The increased threshold (84) allows for longer cleartext names (max 44 chars) in the tests.
80-94: LGTM! Added test for numeric suffix conflict resolution.The new test verifies that:
- When a descriptive conflict suffix exists
- And the resolved name is occupied
- The resolver falls back to numeric suffixes
102-114: LGTM! Updated test for length-limited conflict resolution.The test now uses more realistic filenames with:
- Longer descriptive conflict suffixes
- Longer base filenames
- Proper length limitation verification
src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java (7)
64-64: LGTM! Improved logging performance.The change to use SLF4J placeholder format instead of string concatenation is a good practice that avoids unnecessary string concatenation when logging is disabled.
85-89: LGTM! Improved method signature.The simplified method signature that uses the
Nodeobject directly improves code readability and reduces parameter count. The documentation has been updated accordingly.
110-123: LGTM! Robust alternative name generation.The implementation handles alternative name generation well, including:
- Length validation with assertions
- Informative logging
- Counter-based fallback for name conflicts
64-64: LGTM! Improved logging formatUpdated to use SLF4J placeholder syntax which is more efficient as it avoids unnecessary string concatenation.
78-78: LGTM! Improved method signatureGood refactoring to use the Node object directly instead of passing individual parameters. This encapsulates the data better and simplifies the interface.
Also applies to: 86-89
Line range hint
121-127: LGTM! Robust post-resolution handlingGood use of assertions and atomic file operations to ensure data integrity during the rename operation.
100-101: Verify assumption about conflict suffix positionThe code assumes the sync client always appends the conflict suffix after the ciphertext but before the
.c9rextension. This assumption should be verified against all sync clients that might interact with the system.✅ Verification successful
Conflict Suffix Position Assumption Verified
The repository’s tests and naming conventions confirm that the sync conflict string is indeed expected to be placed immediately after the
extractedCiphertextand right before the file suffix (e.g., “.c9r”). The tests (e.g., inC9rConflictResolverTest.javaand migration tests) consistently rely on this naming pattern. While it’s important to remain vigilant for future changes from sync clients, the current evidence supports the assumption made in the code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases or documentation about sync client conflict patterns rg -l "conflict.*pattern|sync.*conflict" --type mdLength of output: 52
Script:
#!/bin/bash echo "Searching in repository for sync conflict patterns in Markdown files:" rg -n "sync.*conflict" --type md echo "Searching in repository for conflict patterns in Java files:" rg -n "conflict" --type java echo "Searching occurrences of 'extractedCiphertext' to understand conflict suffix usage:" rg -n "extractedCiphertext" --type java echo "Displaying snippet of C9rConflictResolver.java (lines 80-130):" sed -n '80,130p' src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.javaLength of output: 20344
pom.xml (3)
30-30: LGTM! Proper version management.The JMH version is correctly defined as a property, following Maven best practices for version management.
127-138: LGTM! Correct JMH dependency configuration.The JMH dependencies are properly configured with:
- Correct scopes (
testfor core,providedfor annotation processor)- Version referencing the property
- Clear comment explaining the annotation processor scope
179-183: LGTM! Proper annotation processor configuration.The JMH annotation processor is correctly configured in the Maven Compiler Plugin, using the version property.
src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java
Outdated
Show resolved
Hide resolved
This change attempts to solve cryptomator/cryptomator#3707.
The first commit is just a slight improvement for the regex matcher in order to make it faster and more robust.
The second commit alters the
renameConflictingFile()method, which is now rather big, but I feel like it is the right amount of readability for this project. This is the new logic:.c9rextensionlengthLimitedBasname + lengthLimitedConflictSuffix + fileExtensionOn the test side, I altered/added the test scenarios:
testResolveConflictingFileByChoosingNewName, happy pathtestResolveConflictingFileByAddingNumericSuffix, preferred resolved file name already takenCreated by Alice on 2024-01-31
).c9r
.txt
.txt
testResolveConflictingFileByChoosingNewLengthLimitedName, limiting both basename as well as conflict suffix