Feature: Notification API for Filesystem events #277
Conversation
|
Warning Rate limit exceeded@infeo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new event handling mechanism throughout the cryptographic filesystem module. It exports the event package and adds several event types through new record classes and a sealed interface. A new property for specifying an event listener (as a Consumer) is added to the filesystem properties and incorporated via its builder. The API now allows injection of an event consumer into key components such as the conflict resolver, chunk loader, and file header holder, which use it to report events like conflict resolution failures, decryption failures, and successful conflict resolutions. Additionally, the test suite has been updated to verify that these events are correctly triggered during various error and conflict scenarios. Assessment against linked issues
Suggested labels
Suggested reviewers
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: 3
🧹 Nitpick comments (12)
src/main/java/org/cryptomator/cryptofs/event/DecryptionFailedEvent.java (1)
13-13: Consider using a more descriptive parameter name.The parameter name 'e' could be more descriptive. Consider renaming it to 'exception' or 'authenticationException' to improve code readability.
-public record DecryptionFailedEvent(Path ciphertextPath, AuthenticationFailedException e) implements FilesystemEvent { +public record DecryptionFailedEvent(Path ciphertextPath, AuthenticationFailedException exception) implements FilesystemEvent {src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java (1)
12-12: Consider using a more specific exception type.Using a generic
Exceptiontype for thereasonparameter might be too broad. Consider using a more specific exception type or creating a custom exception type that better represents the specific failures that can occur during conflict resolution.src/main/java/org/cryptomator/cryptofs/event/FilesystemEvent.java (1)
23-23: Consider using more formal language in API documentation.The Star Wars reference in the API note might be too informal for API documentation. Consider rephrasing it to maintain a professional tone.
-@apiNote Events might have occured a long time ago in a galaxy far, far away... therefore, any feedback method is non-blocking and might fail due to changes in the filesystem. +@apiNote Events might have occurred in the past, therefore any feedback method is non-blocking and might fail due to changes in the filesystem.src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java (1)
84-86: Consider extracting error handling to a utility method.The error handling pattern might be reused in other components. Consider extracting it to a utility method.
+private void handleDecryptionFailure(Exception e) { + if (e instanceof AuthenticationFailedException afe) { + eventConsumer.accept(new DecryptionFailedEvent(path.get(), afe)); + } +} try { FileHeader existingHeader = cryptor.fileHeaderCryptor().decryptHeader(existingHeaderBuf); encryptedHeader.set(existingHeaderBuf.flip().asReadOnlyBuffer()); header.set(existingHeader); isPersisted.set(true); return existingHeader; } catch (IllegalArgumentException | CryptoException e) { - if (e instanceof AuthenticationFailedException afe) { - eventConsumer.accept(new DecryptionFailedEvent(path.get(), afe)); - } + handleDecryptionFailure(e); throw new IOException("Unable to decrypt header of file " + path.get(), e); }src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java (1)
91-99: Enhance test coverage for decryption failure events.While the test correctly verifies the basic failure scenario, consider enhancing it to:
- Add a descriptive error message to the assertion.
- Verify the properties of the DecryptionFailedEvent.
- Assertions.assertThrows(IOException.class, () -> inTest.loadExisting(channel)); + var ex = Assertions.assertThrows(IOException.class, + () -> inTest.loadExisting(channel), + "Expected loadExisting to throw IOException on decryption failure" + ); var isDecryptionFailedEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof DecryptionFailedEvent; - verify(eventConsumer).accept(ArgumentMatchers.argThat(isDecryptionFailedEvent)); + var eventCaptor = ArgumentCaptor.forClass(DecryptionFailedEvent.class); + verify(eventConsumer).accept(ArgumentMatchers.argThat(isDecryptionFailedEvent)); + verify(eventConsumer).accept(eventCaptor.capture()); + var event = eventCaptor.getValue(); + Assertions.assertEquals(path, event.getPath()); + Assertions.assertTrue(event.getCause() instanceof AuthenticationFailedException);src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java (1)
135-150: Enhance test coverage for chunk decryption failure events.While the test correctly verifies the basic failure scenario, consider enhancing it to verify the properties of the DecryptionFailedEvent.
- Assertions.assertThrows(AuthenticationFailedException.class, () -> inTest.load(chunkIndex)); + var ex = Assertions.assertThrows(AuthenticationFailedException.class, + () -> inTest.load(chunkIndex), + "Expected load to throw AuthenticationFailedException on chunk decryption failure" + ); + Assertions.assertEquals("FAIL", ex.getMessage()); var isDecryptionFailedEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof DecryptionFailedEvent; - verify(eventConsumer).accept(ArgumentMatchers.argThat(isDecryptionFailedEvent)); + var eventCaptor = ArgumentCaptor.forClass(DecryptionFailedEvent.class); + verify(eventConsumer).accept(ArgumentMatchers.argThat(isDecryptionFailedEvent)); + verify(eventConsumer).accept(eventCaptor.capture()); + var event = eventCaptor.getValue(); + Assertions.assertEquals(filePath.get(), event.getPath()); + Assertions.assertTrue(event.getCause() instanceof AuthenticationFailedException); + Assertions.assertEquals("FAIL", event.getCause().getMessage());src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java (2)
88-89: Reduce duplication in event verification code.Consider extracting the event verification into a helper method and enhancing it to verify event properties.
+ private void verifyConflictResolvedEvent(String originalName, Path originalPath, String newName, Path newPath) { + var eventCaptor = ArgumentCaptor.forClass(ConflictResolvedEvent.class); + verify(eventConsumer).accept(eventCaptor.capture()); + var event = eventCaptor.getValue(); + Assertions.assertEquals(cleartextPath.resolve(originalName), event.getOriginalCleartextPath()); + Assertions.assertEquals(originalPath, event.getOriginalCiphertextPath()); + Assertions.assertEquals(cleartextPath.resolve(newName), event.getNewCleartextPath()); + Assertions.assertEquals(newPath, event.getNewCiphertextPath()); + }Then use it in both test methods:
- var isConflictResolvedEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof ConflictResolvedEvent; - verify(eventConsumer).accept(ArgumentMatchers.argThat(isConflictResolvedEvent)); + verifyConflictResolvedEvent("bar.txt", resolved.ciphertextPath, "bar (1).txt", resolved.ciphertextPath);Also applies to: 109-110
167-185: Enhance test coverage for conflict resolution failure events.While the test correctly verifies the basic failure scenario, consider enhancing it to verify the properties of the ConflictResolutionFailedEvent.
Assertions.assertTrue(Files.exists(p1)); Assertions.assertTrue(Files.exists(p2)); - var isConflictResolutionFailedEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof ConflictResolutionFailedEvent; - verify(eventConsumer).accept(ArgumentMatchers.argThat(isConflictResolutionFailedEvent)); + var eventCaptor = ArgumentCaptor.forClass(ConflictResolutionFailedEvent.class); + verify(eventConsumer).accept(eventCaptor.capture()); + var event = eventCaptor.getValue(); + Assertions.assertEquals(cleartextPath.resolve("bar.txt"), event.getCleartextPath()); + Assertions.assertEquals(p1, event.getCiphertextPath()); + Assertions.assertTrue(event.getCause() instanceof IOException);src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java (3)
44-52: Add null checks for new constructor parameters.Consider adding null checks for the new parameters to fail fast if null values are provided.
public C9rConflictResolver(Cryptor cryptor, @Named("dirId") String dirId, VaultConfig vaultConfig, Consumer<FilesystemEvent> eventConsumer, @Named("cleartextPath") Path cleartextPath) { + Preconditions.checkNotNull(eventConsumer, "eventConsumer must not be null"); + Preconditions.checkNotNull(cleartextPath, "cleartextPath must not be null"); this.cryptor = cryptor; this.dirId = dirId.getBytes(StandardCharsets.US_ASCII); this.maxC9rFileNameLength = vaultConfig.getShorteningThreshold(); this.cleartextPath = cleartextPath; this.maxCleartextFileNameLength = (maxC9rFileNameLength - 4) / 4 * 3 - 16; this.eventConsumer = eventConsumer; }
72-73: Improve event handling readability.Consider extracting the event creation to a separate method and adding a comment explaining its purpose.
- eventConsumer.accept(new ConflictResolutionFailedEvent(cleartextPath.resolve(node.cleartextName), node.ciphertextPath.resolve(node.fullCiphertextFileName), e)); - LOG.error("Failed to resolve conflict for {}", node.ciphertextPath, e); + notifyConflictResolutionFailed(node, e); + LOG.error("Failed to resolve conflict for {}", node.ciphertextPath, e); return Stream.empty(); + } + + /** + * Notifies listeners that a conflict resolution attempt has failed. + * This occurs when an IOException is thrown during the resolution process. + * + * @param node The node for which conflict resolution failed + * @param cause The exception that caused the failure + */ + private void notifyConflictResolutionFailed(Node node, IOException cause) { + var cleartextFilePath = cleartextPath.resolve(node.cleartextName); + var ciphertextFilePath = node.ciphertextPath.resolve(node.fullCiphertextFileName); + eventConsumer.accept(new ConflictResolutionFailedEvent(cleartextFilePath, ciphertextFilePath, cause));
124-124: Improve event handling readability.Consider extracting the event creation to a separate method and adding a comment explaining its purpose.
- eventConsumer.accept(new ConflictResolvedEvent(cleartextPath.resolve(cleartext), canonicalPath, cleartextPath.resolve(alternativeCleartext), alternativePath)); + notifyConflictResolved(cleartext, canonicalPath, alternativeCleartext, alternativePath); return node; + } + + /** + * Notifies listeners that a conflict has been successfully resolved by renaming the conflicting file. + * + * @param originalCleartext The original cleartext name + * @param originalPath The original ciphertext path + * @param newCleartext The new cleartext name after resolution + * @param newPath The new ciphertext path after resolution + */ + private void notifyConflictResolved(String originalCleartext, Path originalPath, String newCleartext, Path newPath) { + var originalCleartextPath = cleartextPath.resolve(originalCleartext); + var newCleartextPath = cleartextPath.resolve(newCleartext); + eventConsumer.accept(new ConflictResolvedEvent(originalCleartextPath, originalPath, newCleartextPath, newPath));src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java (1)
222-223: Consider improving type safety for event consumer validation.The current implementation uses
Consumer.classfor type checking, which allows any Consumer type. Consider using a more specific type check to ensure onlyConsumer<FilesystemEvent>is accepted.- checkedSet(Consumer.class, PROPERTY_NOTIFY_METHOD, properties, this::withFilesystemEventConsumer); + @SuppressWarnings("unchecked") // safe cast as we verify it's a Consumer + Consumer<FilesystemEvent> consumer = (Consumer<FilesystemEvent>) properties.get(PROPERTY_NOTIFY_METHOD); + if (consumer != null) { + withFilesystemEventConsumer(consumer); + }Also applies to: 235-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/module-info.java(1 hunks)src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java(3 hunks)src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java(6 hunks)src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java(5 hunks)src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/DecryptionFailedEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/FilesystemEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java(2 hunks)src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java(3 hunks)src/test/java/org/cryptomator/cryptofs/CryptoFileSystemPropertiesTest.java(5 hunks)src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java(5 hunks)src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java(5 hunks)src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse
🔇 Additional comments (12)
src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java (1)
13-13: LGTM!The implementation is well-structured with clear documentation and consistent parameter naming.
src/main/java/org/cryptomator/cryptofs/event/FilesystemEvent.java (1)
25-25: LGTM!The sealed interface is well-designed and properly documented with helpful usage examples.
src/main/java/module-info.java (1)
25-25: LGTM! Export declaration aligns with PR objectives.The export of
org.cryptomator.cryptofs.eventpackage is necessary for the new notification API.src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java (1)
55-57: LGTM! Error handling with event notification is well implemented.The error handling properly notifies about decryption failures while maintaining the original exception flow.
src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java (2)
3-6: LGTM!The imports and field declarations are appropriate for the new event handling functionality.
Also applies to: 15-16, 25-25, 44-44
52-52: LGTM!The setup method correctly initializes the test environment with the new event consumer.
src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java (1)
4-6: LGTM!The imports and field declarations are appropriate for the new event handling functionality.
Also applies to: 16-17, 23-25, 55-57
src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java (2)
4-7: LGTM!The imports and field declarations are appropriate for the new event handling functionality.
Also applies to: 15-16, 23-23, 33-34
44-45: LGTM!The setup method correctly initializes the test environment with the new event consumer and cleartext path.
src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java (1)
9-11: LGTM!The imports and field declarations are appropriate for the new event handling functionality.
Also applies to: 24-24, 40-42
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java (2)
84-92: LGTM! Well-documented constants with proper default behavior.The implementation follows best practices:
- Constants are properly named and documented
- Default no-op consumer prevents null pointer exceptions
- Version tracking is maintained with @SInCE tags
126-127: LGTM! Property entry correctly integrated.The notification consumer property is properly integrated into the immutable properties set, maintaining consistency with other properties.
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java
Outdated
Show resolved
Hide resolved
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemPropertiesTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/event/FilesystemEvent.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java (1)
5-17: Consider adding an example to the documentation.While the documentation clearly explains the concepts, adding a concrete example would make it even more helpful. For instance:
/* * Example: * If two files exist: * - encrypted/path/to/file.txt (canonical) * - encrypted/path/to/file (1).txt (conflicting) * After resolution: * - encrypted/path/to/file (1).txt is renamed to encrypted/path/to/resolved-file.txt */src/test/java/org/cryptomator/cryptofs/CryptoFileSystemModuleTest.java (1)
21-33: Enhance test coverage for the event consumer.While the current test verifies exception handling, consider adding tests for:
- Normal operation (no exceptions)
- Different event types
- Null events
- Multiple consecutive events
@Test void testEventConsumerIsDecorated() { var p = Mockito.mock(Path.class); var event = new ConflictResolutionFailedEvent(p, p, new RuntimeException()); var eventConsumer = (Consumer<FilesystemEvent>) mock(Consumer.class); doThrow(new RuntimeException("fail")).when(eventConsumer).accept(event); var props = mock(CryptoFileSystemProperties.class); when(props.fsEventConsumner()).thenReturn(eventConsumer); var decoratedConsumer = inTest.provideFilesystemEventConsumer(props); Assertions.assertDoesNotThrow(() -> decoratedConsumer.accept(event)); verify(eventConsumer).accept(event); } + +@Test +void testEventConsumerNormalOperation() { + var p = Mockito.mock(Path.class); + var event = new ConflictResolvedEvent(p, p); + var eventConsumer = (Consumer<FilesystemEvent>) mock(Consumer.class); + var props = mock(CryptoFileSystemProperties.class); + when(props.fsEventConsumner()).thenReturn(eventConsumer); + + var decoratedConsumer = inTest.provideFilesystemEventConsumer(props); + decoratedConsumer.accept(event); + verify(eventConsumer).accept(event); +} + +@Test +void testEventConsumerWithNullEvent() { + var eventConsumer = (Consumer<FilesystemEvent>) mock(Consumer.class); + var props = mock(CryptoFileSystemProperties.class); + when(props.fsEventConsumner()).thenReturn(eventConsumer); + + var decoratedConsumer = inTest.provideFilesystemEventConsumer(props); + Assertions.assertThrows(NullPointerException.class, () -> decoratedConsumer.accept(null)); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java(3 hunks)src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java(7 hunks)src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java(1 hunks)src/test/java/org/cryptomator/cryptofs/CryptoFileSystemModuleTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (4)
src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java (2)
1-4: LGTM!The package structure and imports are clean and minimal.
18-20: Consider the parameter naming and ordering.
Regarding the previous suggestion to simplify parameter names: The current verbose names actually provide better semantic clarity by distinguishing between canonical, conflicting, and resolved paths. This is preferable to simpler names like
oldCiphertextPathwhich might not capture these important distinctions.Consider reordering the parameters to group related paths together:
-public record ConflictResolvedEvent(Path canonicalCleartextPath, Path conflictingCiphertextPath, Path resolvedCleartextPath, Path resolvedCiphertextPath) +public record ConflictResolvedEvent( + // Group cleartext paths + Path canonicalCleartextPath, + Path resolvedCleartextPath, + // Group ciphertext paths + Path conflictingCiphertextPath, + Path resolvedCiphertextPath +)src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java (2)
354-367: Fix version inconsistency in @SInCE tag.The
@sinceversion tag (2.8.0) is inconsistent with the version in thePROPERTY_NOTIFY_METHODconstant (2.9.0)./** * Sets the consumer for filesystem events * * @param eventConsumer the consumer to receive filesystem events * @return this - * @since 2.8.0 + * @since 2.9.0 */ public Builder withFilesystemEventConsumer(Consumer<FilesystemEvent> eventConsumer) {
89-91: Update property name to maintain consistency.Based on past review comments, the property name should be updated to maintain naming coherence.
- public static final String PROPERTY_NOTIFY_METHOD = "notificationConsumer"; - - static final Consumer<FilesystemEvent> DEFAULT_NOTIFY_METHOD = (FilesystemEvent e) -> {}; + public static final String PROPERTY_EVENT_CONSUMER = "fsEventConsumer"; + + static final Consumer<FilesystemEvent> DEFAULT_EVENT_CONSUMER = (FilesystemEvent e) -> {};
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java
Outdated
Show resolved
Hide resolved
# Conflicts: # src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java # src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemModuleTest.java (1)
17-20: Consider adding test coverage for other module methods.The test class currently only tests the event consumer decoration. Consider adding tests for other methods in the
CryptoFileSystemModuleclass.src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java (2)
33-34: Consider using a more descriptive mock path.The mock path
/clear/text/path/could be more specific to the test context, e.g.,/clear/text/conflictresolver/test/.
108-109: Consider extracting the event matcher to a reusable field.The
isConflictResolvedEventmatcher is duplicated. Consider extracting it to a class field to improve maintainability.public class C9rConflictResolverTest { + private static final ArgumentMatcher<FilesystemEvent> IS_CONFLICT_RESOLVED_EVENT = ev -> ev instanceof ConflictResolvedEvent; // ... existing fields ... @Test public void testResolveConflictingFileByAddingNumericSuffix(@TempDir Path dir) throws IOException { // ... existing test code ... - var isConflictResolvedEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof ConflictResolvedEvent; - verify(eventConsumer).accept(ArgumentMatchers.argThat(isConflictResolvedEvent)); + verify(eventConsumer).accept(ArgumentMatchers.argThat(IS_CONFLICT_RESOLVED_EVENT)); }Also applies to: 129-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java(3 hunks)src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java(7 hunks)src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java(5 hunks)src/test/java/org/cryptomator/cryptofs/CryptoFileSystemModuleTest.java(1 hunks)src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/cryptomator/cryptofs/CryptoFileSystemModule.java
- src/main/java/org/cryptomator/cryptofs/CryptoFileSystemProperties.java
- src/main/java/org/cryptomator/cryptofs/dir/C9rConflictResolver.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (5)
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemModuleTest.java (2)
1-16: LGTM!The imports are appropriate for the test class requirements.
21-33: LGTM! Robust test for event consumer decoration.The test effectively verifies that:
- Exceptions from the event consumer are caught and don't propagate
- Events are still delivered to the consumer despite exceptions
This ensures the filesystem remains stable even when event consumers misbehave.
src/test/java/org/cryptomator/cryptofs/dir/C9rConflictResolverTest.java (3)
3-7: LGTM!The imports are appropriate for the new event handling functionality.
Also applies to: 15-17, 23-23, 26-27
44-45: LGTM!The setup is properly configured with the new event consumer and cleartext path parameters.
203-221: LGTM! Comprehensive test for conflict resolution failure.The test effectively verifies that:
- The process doesn't throw when resolution fails
- No nodes are returned
- Original files remain intact
- Appropriate event is published
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java (1)
92-110: LGTM! Comprehensive test coverage for failure scenarios.The test cases properly verify the event handling behavior for both
AuthenticationFailedExceptionandIllegalArgumentExceptionscenarios.Consider reducing code duplication by extracting the common ArgumentMatcher:
@Nested @DisplayName("existing header") public class ExistingHeader { + private final ArgumentMatcher<FilesystemEvent> isDecryptionFailedEvent = + ev -> ev instanceof DecryptionFailedEvent; + @Test @DisplayName("load failure due to authenticationFailedException") public void testLoadExistingFailureWithAuthFailed() { Mockito.doThrow(AuthenticationFailedException.class).when(fileHeaderCryptor).decryptHeader(Mockito.any()); Assertions.assertThrows(IOException.class, () -> inTest.loadExisting(channel)); - var isDecryptionFailedEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof DecryptionFailedEvent; verify(eventConsumer).accept(ArgumentMatchers.argThat(isDecryptionFailedEvent)); } @Test @DisplayName("load failure due to IllegalArgumentException") public void testLoadExistingFailureWithIllegalArgument() { Mockito.doThrow(IllegalArgumentException.class).when(fileHeaderCryptor).decryptHeader(Mockito.any()); Assertions.assertThrows(IOException.class, () -> inTest.loadExisting(channel)); - var isDecryptionFailedEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof DecryptionFailedEvent; verify(eventConsumer, never()).accept(ArgumentMatchers.argThat(isDecryptionFailedEvent)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemPropertiesTest.java(6 hunks)src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/cryptomator/cryptofs/CryptoFileSystemPropertiesTest.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (2)
src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java (2)
3-4: LGTM! Well-structured import changes and field setup.The new imports and event consumer mock field are properly organized to support the event handling functionality.
Also applies to: 15-16, 25-25, 28-28, 45-45
53-53: LGTM! Constructor updated correctly.The
FileHeaderHolderconstructor is properly updated to include the event consumer parameter.
…er api Co-authored-by: Sebastian Stenzel <[email protected]>
This PR adds a notification API to the filesytem and closes #89.
The notification API is simple: At filesystem creation the caller hands over a Consumer object via the CryptoFilesystemProperties map. Everytime, the filesystem creates an event, it is consumed by this object.
For the start there are 3 different events:
Supersedes #276 by reducing the diff.
Remark: The DecryptionFailed event will be enhanced with the cleartext path in a different PR