Make DosFileAttributes immutable #305
Conversation
references #42
WalkthroughThe CryptoDosFileAttributes class was refactored to cache DOS attribute values (isReadOnly, isHidden, isArchive, isSystem) as boolean fields initialized in the constructor; isReadOnly is computed as the logical OR of the filesystem readonly flag and the delegate’s readonly value. The getter methods now return these cached booleans. Tests in CryptoDosFileAttributesTest switched @BeforeAll to @beforeeach, removed @testinstance(Lifecycle.PER_CLASS), moved instantiation of the class-under-test into each test method, renamed several tests in the ReadWriteFileSystem nested class, and added explicit Mockito verifications of delegate invocations (including atMostOnce/times checks). Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 📜 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (5)
src/main/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributes.java (3)
34-37: Clarify readonly precedence in JavadocConsider a short Javadoc note that
isReadOnlyreflects the logical OR of FS-level readonly and delegate’s readonly, i.e., FS-level readonly dominates even if the delegate reports writable. This avoids ambiguity for future readers.
27-33: Optional: Defensive null checks for constructor parametersIf not already enforced by callers, add
Objects.requireNonNullchecks fordelegateandfileSystemPropertiesto avoid NPEs during construction. Low-cost, improves robustness.import java.util.Optional; +import java.util.Objects; public CryptoDosFileAttributes(DosFileAttributes delegate, // CiphertextFileType ciphertextFileType, // Path ciphertextPath, // Cryptor cryptor, // Optional<OpenCryptoFile> openCryptoFile, // CryptoFileSystemProperties fileSystemProperties) { - super(delegate, ciphertextFileType, ciphertextPath, cryptor, openCryptoFile); + super(Objects.requireNonNull(delegate), ciphertextFileType, ciphertextPath, cryptor, openCryptoFile); + Objects.requireNonNull(fileSystemProperties);
27-33: Confirm BasicFileAttributes immutability path via superclassThis class no longer uses the DOS delegate after construction, which is good. Please verify that
CryptoBasicFileAttributesalso snapshots/caches basic attributes (size, times, etc.). If it still holds/consults the same mutableDosFileAttributesinstance at access time, Basic attributes might remain mutable. If that’s already handled, we’re good.src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java (2)
37-37: Prefer uppercase long literalUse
0Linstead of0lto avoid confusion with the digit1.- when(delegate.size()).thenReturn(0l); + when(delegate.size()).thenReturn(0L);
64-66: Use assertEquals/assertTrue for booleans instead of assertSame
assertSamerelies on Boolean object identity;assertEquals(orassertTrue/assertFalse) is clearer and idiomatic for primitives.- Assertions.assertSame(value, inTest.isArchive()); + Assertions.assertEquals(value, inTest.isArchive());- Assertions.assertSame(value, inTest.isHidden()); + Assertions.assertEquals(value, inTest.isHidden());- Assertions.assertSame(value, inTest.isReadOnly()); + Assertions.assertEquals(value, inTest.isReadOnly());- Assertions.assertSame(value, inTest.isSystem()); + Assertions.assertEquals(value, inTest.isSystem());Also applies to: 76-78, 88-90, 100-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributes.java(1 hunks)src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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:20-26
Timestamp: 2024-10-21T15:54:48.161Z
Learning: In the Java test file `ClearToCipherDirCacheTest.java`, the `beforeEach` method includes a call to `dirLoader.load()` which can throw an `IOException`, so the `throws IOException` declaration is necessary.
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.
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.
📚 Learning: 2024-10-21T15:54:48.161Z
Learnt from: infeo
PR: cryptomator/cryptofs#253
File: src/test/java/org/cryptomator/cryptofs/ClearToCipherDirCacheTest.java:20-26
Timestamp: 2024-10-21T15:54:48.161Z
Learning: In the Java test file `ClearToCipherDirCacheTest.java`, the `beforeEach` method includes a call to `dirLoader.load()` which can throw an `IOException`, so the `throws IOException` declaration is necessary.
Applied to files:
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
📚 Learning: 2024-10-21T15:52:40.091Z
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.
Applied to files:
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
📚 Learning: 2024-10-21T15:56:44.806Z
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.
Applied to files:
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyse
🔇 Additional comments (2)
src/main/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributes.java (1)
22-38: Immutability of DOS flags achieved; constructor-cached fields LGTMCaching
isReadOnly,isHidden,isArchive, andisSystemin final fields during construction fulfills the immutability goal and prevents post-construction changes from a mutable delegate. TheisReadOnlyOR with FS-level readonly is also correct.src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java (1)
59-66: Good isolation and verification of cached behaviorInstantiating the SUT after stubbing and verifying only 1 delegate call across construction and getter usage accurately tests the immutability/caching behavior. Nicely done.
Also applies to: 71-78, 83-90, 95-102
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java (2)
83-90: Read-only attribute on RW FS: correct verification, minor display name nittimes(1) is appropriate here since delegate.isReadOnly() must be consulted when FS is RW. Nit: the @ParameterizedTest name says delegate.readOnly(); consider correcting to delegate.isReadOnly() for clarity.
- @ParameterizedTest(name = "is {0} if delegate.readOnly() is {0}") + @ParameterizedTest(name = "is {0} if delegate.isReadOnly() is {0}")
118-125: Optional: unify local vs. field usage of inTestHere you use a local
var inTest, while the RW tests use a class fieldinTest. For consistency, consider using local variables in all tests and removing the field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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:20-26
Timestamp: 2024-10-21T15:54:48.161Z
Learning: In the Java test file `ClearToCipherDirCacheTest.java`, the `beforeEach` method includes a call to `dirLoader.load()` which can throw an `IOException`, so the `throws IOException` declaration is necessary.
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.
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.
📚 Learning: 2024-10-21T15:54:48.161Z
Learnt from: infeo
PR: cryptomator/cryptofs#253
File: src/test/java/org/cryptomator/cryptofs/ClearToCipherDirCacheTest.java:20-26
Timestamp: 2024-10-21T15:54:48.161Z
Learning: In the Java test file `ClearToCipherDirCacheTest.java`, the `beforeEach` method includes a call to `dirLoader.load()` which can throw an `IOException`, so the `throws IOException` declaration is necessary.
Applied to files:
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
📚 Learning: 2024-10-21T15:52:40.091Z
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.
Applied to files:
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
📚 Learning: 2024-10-21T15:56:44.806Z
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.
Applied to files:
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
📚 Learning: 2024-10-22T08:36:41.464Z
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.
Applied to files:
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
🪛 GitHub Actions: Build
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
[error] 122-124: Compilation error: cannot find symbol 'Mockito' in class ReadOnlyFileSystem. Variable 'Mockito' is undefined at lines 122 and 124.
🪛 GitHub Actions: CodeQL
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
[error] 122-124: Compilation error: cannot find symbol 'Mockito' in class ReadOnlyFileSystem at lines 122 and 124.
🔇 Additional comments (8)
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java (8)
10-10: Good change: switch to per-test lifecycleUsing @beforeeach improves test isolation and aligns with the new immutability semantics.
21-23: Consistent Mockito usage via static importsStatic imports for verify/times/when improve readability and consistency.
35-37: Per-test setup looks correctStubbing size() and cryptor-related values up front is fine for constructor-time caching.
51-54: RW FS setup is clearExplicitly setting readonly() to false keeps the tests deterministic.
59-66: Archive attribute: correct immutability verificationVerifying one delegate call at construction and none on accessor is appropriate.
71-78: Hidden attribute: correct immutability verificationSame pattern correctly asserts caching behavior.
95-102: System attribute: correct immutability verificationPattern matches intended behavior. Looks good.
110-113: RO FS setup is correctreadonly() = true is the scenario that may short-circuit delegate access.
src/test/java/org/cryptomator/cryptofs/attr/CryptoDosFileAttributesTest.java
Outdated
Show resolved
Hide resolved
references #42
The PosixFIleAttributes are already immutable. For Windows, we depend on the JDK implemenation of the DosFileAttributes. This PR removes the dependency by setting the CryptoDosFileAttributes in the constructor and discards the DosFileAttributes delegate afterwards.