Merged
Conversation
also fix formatting
keep old API and only extend it
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR propagates cleartext CryptoPath through open-file lifecycle and move operations. OpenCryptoFiles gains overloads for getOrCreate, readCiphertextFile, writeCiphertextFile, and prepareMove to accept a CryptoPath; TwoPhaseMove and commit now carry cleartextDst. OpenCryptoFile and OpenCryptoFileModule store a current cleartext-path AtomicReference. CryptoFileSystemImpl and Symlinks call sites were updated. CleartextFileChannel now checks InUseManager before writes, emits FileIsInUse events via an injected Consumer, and constructors across related classes were extended to accept the new dependencies and path references. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java (1)
73-107: Consider extracting shared logic to reduce duplication.The
writeCiphertextFileandreadCiphertextFilemethod pairs are nearly identical — the only difference is whether they callgetOrCreate(CryptoPath, Path)orgetOrCreate(Path). The non-CryptoPath versions could delegate to the CryptoPath versions withnull:♻️ Example for writeCiphertextFile
public void writeCiphertextFile(Path ciphertextPath, EffectiveOpenOptions openOptions, ByteBuffer contents) throws IOException { - try (OpenCryptoFile f = getOrCreate(ciphertextPath); FileChannel ch = f.newFileChannel(openOptions)) { - ch.write(contents); - } + writeCiphertextFile(null, ciphertextPath, openOptions, contents); }This is optional since the current approach is clear and the duplication is minimal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java` around lines 73 - 107, The two pairs of methods writeCiphertextFile and readCiphertextFile duplicate logic; refactor the Path-only overloads to delegate to the CryptoPath versions by calling getOrCreate with a null CryptoPath (i.e. change writeCiphertextFile(Path, EffectiveOpenOptions, ByteBuffer) to call writeCiphertextFile((CryptoPath) null, ciphertextPath, openOptions, contents) and similarly have readCiphertextFile(Path, EffectiveOpenOptions, int) call readCiphertextFile((CryptoPath) null, ciphertextPath, openOptions, maxBufferSize)), preserving exception signatures and behavior of getOrCreate(CryptoPath, Path), newFileChannel and buffer size checks.src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java (2)
44-87: Multiple testing constructors add maintenance burden.There are now 4 constructors (1 production + 3 testing). The two "testing compatibility" constructors (lines 53-59 and 81-87) exist solely to avoid updating older test call sites. Consider cleaning these up once the PR is stable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java` around lines 44 - 87, The class OpenCryptoFile currently has four constructors including two "testing compatibility" overloads that duplicate logic and increase maintenance; remove the two older compatibility constructors (the ones whose signatures omit `@CurrentOpenFileCleartextPath` and simply call the full constructor with new AtomicReference<>(null)) and keep only the `@Inject` production constructor and a single package-private testing constructor that accepts `@CurrentOpenFileCleartextPath` and UseToken; then update test call sites to call that single testing constructor (or add a small test-only factory method on OpenCryptoFileComponent) so tests supply the currentCleartextPath explicitly instead of relying on the deprecated overloads.
223-228: Minor TOCTOU betweencurrentFilePath.get()and thecurrentCleartextPathupdate.
updateCurrentCleartextPathreadscurrentFilePath.get()inside thegetAndUpdatelambda, whileupdateCurrentFilePathcan concurrently set it tonull. In the worst case, the cleartext path gets updated to a non-null value right after the ciphertext path was nulled. Given that both methods are called from controlled contexts (commit/delete flows), this is unlikely to cause real issues, but it's worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java` around lines 223 - 228, There’s a TOCTOU between currentFilePath.get() and the currentCleartextPath update: make the check-and-update atomic by protecting both updateCurrentCleartextPath and updateCurrentFilePath with the same lock; introduce a shared mutex (e.g., a private final Object filePathLock) and wrap the bodies of updateCurrentCleartextPath and updateCurrentFilePath in synchronized(filePathLock) so the null-check of currentFilePath and the update of currentCleartextPath happen without concurrent mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java`:
- Around line 174-183: The code currently skips emitting FileIsInUseEvent when
currentCleartextPath.get() returns null, causing the FileAlreadyInUseException
to be thrown without any event context; change the logic in writeLocked to
always emit a FileIsInUseEvent before throwing by using
currentCleartextPath.get() if non-null or falling back to the ciphertext path
from currentFilePath.get(), e.g. resolve UseInfo via
inUseManager.getUseInfo(path) and call eventConsumer.accept(new
FileIsInUseEvent(fallbackPath, path, useInfo.owner(), useInfo.lastUpdated(), ()
-> inUseManager.ignoreInUse(path))) so an event is always delivered prior to
throwing FileAlreadyInUseException; keep existing uses of inUseManager, UseInfo,
eventConsumer, FileIsInUseEvent, and ignoreInUse.
---
Nitpick comments:
In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java`:
- Around line 44-87: The class OpenCryptoFile currently has four constructors
including two "testing compatibility" overloads that duplicate logic and
increase maintenance; remove the two older compatibility constructors (the ones
whose signatures omit `@CurrentOpenFileCleartextPath` and simply call the full
constructor with new AtomicReference<>(null)) and keep only the `@Inject`
production constructor and a single package-private testing constructor that
accepts `@CurrentOpenFileCleartextPath` and UseToken; then update test call sites
to call that single testing constructor (or add a small test-only factory method
on OpenCryptoFileComponent) so tests supply the currentCleartextPath explicitly
instead of relying on the deprecated overloads.
- Around line 223-228: There’s a TOCTOU between currentFilePath.get() and the
currentCleartextPath update: make the check-and-update atomic by protecting both
updateCurrentCleartextPath and updateCurrentFilePath with the same lock;
introduce a shared mutex (e.g., a private final Object filePathLock) and wrap
the bodies of updateCurrentCleartextPath and updateCurrentFilePath in
synchronized(filePathLock) so the null-check of currentFilePath and the update
of currentCleartextPath happen without concurrent mutation.
In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java`:
- Around line 73-107: The two pairs of methods writeCiphertextFile and
readCiphertextFile duplicate logic; refactor the Path-only overloads to delegate
to the CryptoPath versions by calling getOrCreate with a null CryptoPath (i.e.
change writeCiphertextFile(Path, EffectiveOpenOptions, ByteBuffer) to call
writeCiphertextFile((CryptoPath) null, ciphertextPath, openOptions, contents)
and similarly have readCiphertextFile(Path, EffectiveOpenOptions, int) call
readCiphertextFile((CryptoPath) null, ciphertextPath, openOptions,
maxBufferSize)), preserving exception signatures and behavior of
getOrCreate(CryptoPath, Path), newFileChannel and buffer size checks.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.javasrc/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.javasrc/main/java/org/cryptomator/cryptofs/fh/CurrentOpenFileCleartextPath.javasrc/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.javasrc/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileModule.javasrc/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.javasrc/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.javasrc/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFilesTest.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java (1)
177-183:⚠️ Potential issue | 🟡 MinorEmit an in-use event even when cleartext path is unavailable.
At Line 177-183, the code throws
FileAlreadyInUseExceptionwithout emittingFileIsInUseEventwhencurrentCleartextPathisnull. That can leave listeners/UI without ownership/context metadata for the rejection. Consider emitting the event with a fallback identifier before throwing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java` around lines 177 - 183, When a file is detected in use inside CleartextFileChannel, always emit a FileIsInUseEvent before throwing FileAlreadyInUseException: if cleartextPath (currentCleartextPath) is non-null keep the existing event payload, otherwise emit the event using a safe fallback identifier (e.g. the ciphertext path or path.toString()) so listeners still receive owner/lastUpdated and the ignore callback (inUseManager.ignoreInUse(path)); after emitting the event via eventConsumer.accept(...) proceed to throw FileAlreadyInUseException(path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java`:
- Around line 177-183: When a file is detected in use inside
CleartextFileChannel, always emit a FileIsInUseEvent before throwing
FileAlreadyInUseException: if cleartextPath (currentCleartextPath) is non-null
keep the existing event payload, otherwise emit the event using a safe fallback
identifier (e.g. the ciphertext path or path.toString()) so listeners still
receive owner/lastUpdated and the ignore callback
(inUseManager.ignoreInUse(path)); after emitting the event via
eventConsumer.accept(...) proceed to throw FileAlreadyInUseException(path).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.javasrc/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java
remove OpenCryptoFiles methods without cleartext path parameter
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java (1)
115-119: Make nullcleartextDstbehavior explicit inTwoPhaseMove.
prepareMove(src, dst)now flowsnullintocommit()viaupdateCurrentCleartextPath(cleartextDst). Please codify whether null means “clear mapping” or “leave unchanged” to avoid accidental misuse.Optional hardening if null should mean “leave unchanged”
public void commit() { if (rolledBack) { throw new IllegalStateException(); } if (openCryptoFile != null) { openCryptoFile.updateCurrentFilePath(dst); - openCryptoFile.updateCurrentCleartextPath(cleartextDst); + if (cleartextDst != null) { + openCryptoFile.updateCurrentCleartextPath(cleartextDst); + } } openCryptoFiles.remove(src, openCryptoFile); committed = true; }Also applies to: 135-147, 162-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java` around lines 115 - 119, Make null handling for cleartextDst explicit: change TwoPhaseMove to accept Optional<CryptoPath> (or add an explicit enum/flag) and update its constructor and commit()/updateCurrentCleartextPath(Optional<CryptoPath>) to handle Optional.empty() vs Optional.of(value) deterministically (either "clear mapping" or "leave unchanged"); then modify prepareMove(Path src, Path dst) and other prepareMove overloads that currently pass null to pass Optional.empty() or Optional.of(cleartextDst) and update callers (including the other prepareMove usages noted) so behavior is unambiguous and no raw nulls flow into commit().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java`:
- Around line 115-119: Make null handling for cleartextDst explicit: change
TwoPhaseMove to accept Optional<CryptoPath> (or add an explicit enum/flag) and
update its constructor and
commit()/updateCurrentCleartextPath(Optional<CryptoPath>) to handle
Optional.empty() vs Optional.of(value) deterministically (either "clear mapping"
or "leave unchanged"); then modify prepareMove(Path src, Path dst) and other
prepareMove overloads that currently pass null to pass Optional.empty() or
Optional.of(cleartextDst) and update callers (including the other prepareMove
usages noted) so behavior is unambiguous and no raw nulls flow into commit().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/cryptomator/cryptofs/Symlinks.javasrc/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.javasrc/test/java/org/cryptomator/cryptofs/SymlinksTest.javasrc/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFilesTest.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java (1)
206-211: Consider adding a brief doc comment to clarify intent.The conditional logic is correct but subtly different from
updateCurrentFilePath. The method silently ignores updates whencurrentFilePathis null (file deleted) or when the input is null. A short Javadoc would help maintainers understand the design:📝 Suggested documentation
+ /** + * Updates the cleartext path if the file is not deleted (i.e., currentFilePath is not null). + * Null input is ignored. + * + * `@param` cleartextPath new cleartext path, or null to skip update + */ public void updateCurrentCleartextPath(CryptoPath cleartextPath) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java` around lines 206 - 211, The updateCurrentCleartextPath method in OpenCryptoFile silently ignores null inputs and any update when currentFilePath is null; add a brief Javadoc above the updateCurrentCleartextPath(CryptoPath cleartextPath) method explaining its intent and behavior: that null cleartextPath is ignored, and updates are only applied if currentFilePath is non-null (to avoid resurrecting paths for deleted files), matching the subtle difference to updateCurrentFilePath and making maintainers aware of this design decision.src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java (1)
263-273: Consider adding unit tests for the new cleartext path methods.The test
testUpdateCurrentPathWithNullverifiesuseToken.close()but doesn't verify thatcurrentCleartextPathis also set tonull(per line 201 in the production code). Additionally, there are no unit tests for the newupdateCurrentCleartextPathmethod and its specific behaviors:
updateCurrentCleartextPath(null)should be a no-opupdateCurrentCleartextPath(path)when file is deleted (currentFilePathis null) should not updateupdateCurrentCleartextPath(path)when file exists should update📝 Suggested test additions
`@Test` `@DisplayName`("Updating current cleartext path with null does nothing") public void testUpdateCleartextPathWithNull() { var currentPath = mock(Path.class, "current Path"); var currentPathWrapper = new AtomicReference<>(currentPath); var cleartextPath = mock(CryptoPath.class, "cleartext Path"); var cleartextPathWrapper = new AtomicReference<>(cleartextPath); OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, cryptor, headerHolder, chunkIO, currentPathWrapper, fileSize, cleartextPathWrapper, lastModified, openCryptoFileComponent, inUseManager, useToken); openCryptoFile.updateCurrentCleartextPath(null); Assertions.assertSame(cleartextPath, openCryptoFile.getCurrentCleartextPath()); } `@Test` `@DisplayName`("Updating current cleartext path when file is deleted does not update") public void testUpdateCleartextPathWhenDeleted() { var currentPathWrapper = new AtomicReference<Path>(null); // deleted var cleartextPath = mock(CryptoPath.class, "original cleartext"); var cleartextPathWrapper = new AtomicReference<>(cleartextPath); var newCleartextPath = mock(CryptoPath.class, "new cleartext"); OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, cryptor, headerHolder, chunkIO, currentPathWrapper, fileSize, cleartextPathWrapper, lastModified, openCryptoFileComponent, inUseManager, useToken); openCryptoFile.updateCurrentCleartextPath(newCleartextPath); Assertions.assertSame(cleartextPath, openCryptoFile.getCurrentCleartextPath()); } `@Test` `@DisplayName`("Updating current file path with null also nulls cleartext path") public void testUpdateCurrentPathWithNullAlsoNullsCleartext() { var currentPath = mock(Path.class, "current Path"); var currentPathWrapper = new AtomicReference<>(currentPath); var cleartextPath = mock(CryptoPath.class, "cleartext Path"); var cleartextPathWrapper = new AtomicReference<>(cleartextPath); OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, cryptor, headerHolder, chunkIO, currentPathWrapper, fileSize, cleartextPathWrapper, lastModified, openCryptoFileComponent, inUseManager, useToken); doNothing().when(useToken).close(); openCryptoFile.updateCurrentFilePath(null); Assertions.assertNull(openCryptoFile.getCurrentCleartextPath()); verify(useToken).close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java` around lines 263 - 273, Add unit assertions and new tests to cover the cleartext-path behavior: in the existing testUpdateCurrentPathWithNull verify that OpenCryptoFile.getCurrentCleartextPath() is null after calling updateCurrentFilePath(null); add three tests for updateCurrentCleartextPath: (1) testUpdateCleartextPathWithNull — construct OpenCryptoFile with non-null currentCleartextPathWrapper, call updateCurrentCleartextPath(null) and assert getCurrentCleartextPath() is unchanged; (2) testUpdateCleartextPathWhenDeleted — construct OpenCryptoFile with currentPathWrapper set to null and a non-null cleartextPathWrapper, call updateCurrentCleartextPath(newPath) and assert getCurrentCleartextPath() remains the original; (3) testUpdateCurrentPathWithNullAlsoNullsCleartext — similar to your suggested test that calls updateCurrentFilePath(null) and asserts getCurrentCleartextPath() is null and useToken.close() was invoked. Ensure each test constructs OpenCryptoFile with the appropriate AtomicReference wrappers and uses getCurrentCleartextPath(), updateCurrentFilePath(), updateCurrentCleartextPath(), and verify(useToken).close() as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java`:
- Around line 206-211: The updateCurrentCleartextPath method in OpenCryptoFile
silently ignores null inputs and any update when currentFilePath is null; add a
brief Javadoc above the updateCurrentCleartextPath(CryptoPath cleartextPath)
method explaining its intent and behavior: that null cleartextPath is ignored,
and updates are only applied if currentFilePath is non-null (to avoid
resurrecting paths for deleted files), matching the subtle difference to
updateCurrentFilePath and making maintainers aware of this design decision.
In `@src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java`:
- Around line 263-273: Add unit assertions and new tests to cover the
cleartext-path behavior: in the existing testUpdateCurrentPathWithNull verify
that OpenCryptoFile.getCurrentCleartextPath() is null after calling
updateCurrentFilePath(null); add three tests for updateCurrentCleartextPath: (1)
testUpdateCleartextPathWithNull — construct OpenCryptoFile with non-null
currentCleartextPathWrapper, call updateCurrentCleartextPath(null) and assert
getCurrentCleartextPath() is unchanged; (2) testUpdateCleartextPathWhenDeleted —
construct OpenCryptoFile with currentPathWrapper set to null and a non-null
cleartextPathWrapper, call updateCurrentCleartextPath(newPath) and assert
getCurrentCleartextPath() remains the original; (3)
testUpdateCurrentPathWithNullAlsoNullsCleartext — similar to your suggested test
that calls updateCurrentFilePath(null) and asserts getCurrentCleartextPath() is
null and useToken.close() was invoked. Ensure each test constructs
OpenCryptoFile with the appropriate AtomicReference wrappers and uses
getCurrentCleartextPath(), updateCurrentFilePath(),
updateCurrentCleartextPath(), and verify(useToken).close() as needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.javasrc/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a follow-up on #312.
In the former PR, opening a file channel was blocked when a file is marked in use. This is extended by preventing (not already cached) writes on a file if the file is suddenly (e.g. sync activity) marked as in use.
Implementation note: