-
Couldn't load subscription status.
- Fork 35
Fix: Remove CryptoPaths from API #319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request modifies three event record classes in the cryptofs event system, converting Path-typed fields to String-typed fields. Specifically: BrokenFileNodeEvent's cleartextPath field changes from Path to String; ConflictResolutionFailedEvent's canonicalCleartextPath changes from Path to String; and ConflictResolvedEvent's canonicalCleartextPath and resolvedCleartextPath change from Path to String. All affected constructors are updated to convert Path parameters to Strings via toString(). Additionally, some records include getTimestamp() override methods. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Rationale: Three files are affected with a repetitive pattern of Path-to-String field conversions and constructor adjustments. While the changes are structurally straightforward (type conversions via toString()), they constitute public API modifications to record signatures that warrant careful verification of downstream consumer impact and serialization behavior. The homogeneous nature of the conversions reduces complexity compared to highly heterogeneous changes, but the scope of API modifications across multiple event records justifies moderate review effort. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java (1)
10-10: Document string path format (absolute/relative; separator normalization)Please clarify whether the stored string is absolute or relative to the vault root and which separator is guaranteed (e.g., forward slash). Consider normalizing before toString() if needed to ensure stable semantics across platforms.
src/main/java/org/cryptomator/cryptofs/event/BrokenFileNodeEvent.java (1)
10-10: Javadoc: specify path string canonicalizationPlease note whether cleartextPath is normalized (e.g., no “.”/“..”), and the expected separator. This helps downstream consumers avoid platform-conditional parsing.
src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java (1)
15-18: Clarify path string semanticsPlease document whether these strings are absolute vs. vault-relative and the separator guarantees. If they are vault-relative, say so explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/cryptomator/cryptofs/event/BrokenFileNodeEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/ConflictResolutionFailedEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/ConflictResolvedEvent.java(1 hunks)
⏰ 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
This only effects classes in the event package.
Some events contained CryptoPaths, which cannot be accessed anymore if the actual filesystem is closed. This can lead to unforseen errors in consumer libraries, which expect the path objects can still be "used".
This is a breaking change.