Refactored "Cloud#type" #508
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
state:stale
WalkthroughThe changes introduced in this pull request primarily involve updates to the method of accessing the cloud type across various classes in the In conjunction with these changes, several methods in repository factories and other components, such as 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
🧹 Outside diff range and nitpick comments (7)
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloud.java (1)
Line range hint
45-45: Consider tracking the TODO commentThere's an unimplemented TODO for the read-only check. While this is outside the scope of current changes, it would be good to track this for future implementation.
Would you like me to create a GitHub issue to track the implementation of the read-only check?
presentation/src/main/java/org/cryptomator/presentation/model/mappers/CloudModelMapper.kt (1)
25-25: Consider simplifying theletscope function.The current usage of
letadds unnecessary complexity. The code can be simplified.Consider this simpler version:
- return when (domainObject.type.let { CloudTypeModel.valueOf(it) }) { + return when (CloudTypeModel.valueOf(domainObject.type)) {domain/src/main/java/org/cryptomator/domain/LocalStorageCloud.java (1)
Line range hint
63-66: Implement read-only check for local storage.The TODO comment indicates that the read-only check implementation is pending. This is important for proper access control.
Would you like me to help implement the read-only check for local storage? I can:
- Generate code to check if the storage location is writable
- Open a GitHub issue to track this implementation
domain/src/main/java/org/cryptomator/domain/DropboxCloud.java (1)
Line range hint
61-61: Consider implementing the read-only check.There's an unimplemented TODO comment for the read-only check functionality.
Would you like me to help create a GitHub issue to track the implementation of the read-only check?
domain/src/main/java/org/cryptomator/domain/PCloud.java (1)
Line range hint
76-76: Consider implementing the read-only check.The TODO comment indicates that the read-only check implementation is pending. This could affect the security and access control of the cloud storage.
Would you like me to help implement the read-only check or create a GitHub issue to track this task?
domain/src/main/java/org/cryptomator/domain/WebDavCloud.java (1)
Line range hint
82-82: Address TODO comment for read-only check implementation.There's a pending TODO comment regarding the implementation of the read-only check. This should be addressed to ensure proper access control.
Would you like me to help implement the read-only check or create a GitHub issue to track this task?
domain/src/main/java/org/cryptomator/domain/S3Cloud.java (1)
Line range hint
93-93: Consider implementing the read-only check.There's a TODO comment indicating that the read-only check needs to be implemented.
Would you like me to help create a GitHub issue to track the implementation of the read-only check for S3 cloud storage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
data/src/apiKey/java/org/cryptomator/data/cloud/dropbox/DropboxCloudContentRepositoryFactory.java(1 hunks)data/src/apiKey/java/org/cryptomator/data/cloud/onedrive/OnedriveCloudContentRepositoryFactory.java(1 hunks)data/src/apiKey/java/org/cryptomator/data/cloud/pcloud/PCloudContentRepositoryFactory.java(1 hunks)data/src/apkStorePlaystore/java/org/cryptomator/data/cloud/googledrive/GoogleDriveCloudContentRepositoryFactory.java(1 hunks)data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloud.java(2 hunks)data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudContentRepositoryFactory.java(1 hunks)data/src/main/java/org/cryptomator/data/cloud/local/LocalStorageContentRepositoryFactory.java(1 hunks)data/src/main/java/org/cryptomator/data/cloud/s3/S3CloudContentRepositoryFactory.java(1 hunks)data/src/main/java/org/cryptomator/data/cloud/webdav/WebDavCloudContentRepositoryFactory.java(1 hunks)data/src/main/java/org/cryptomator/data/db/mappers/CloudEntityMapper.java(2 hunks)data/src/main/java/org/cryptomator/data/repository/CloudRepositoryImpl.java(1 hunks)data/src/test/java/org/cryptomator/data/cloud/crypto/MasterkeyCryptoCloudProviderTest.kt(4 hunks)domain/src/main/java/org/cryptomator/domain/Cloud.kt(1 hunks)domain/src/main/java/org/cryptomator/domain/DropboxCloud.java(1 hunks)domain/src/main/java/org/cryptomator/domain/GoogleDriveCloud.java(1 hunks)domain/src/main/java/org/cryptomator/domain/LocalStorageCloud.java(1 hunks)domain/src/main/java/org/cryptomator/domain/OnedriveCloud.java(1 hunks)domain/src/main/java/org/cryptomator/domain/PCloud.java(1 hunks)domain/src/main/java/org/cryptomator/domain/S3Cloud.java(1 hunks)domain/src/main/java/org/cryptomator/domain/Vault.java(3 hunks)domain/src/main/java/org/cryptomator/domain/WebDavCloud.java(1 hunks)domain/src/main/java/org/cryptomator/domain/usecases/cloud/AddOrChangeCloudConnection.java(1 hunks)domain/src/main/java/org/cryptomator/domain/usecases/cloud/LogoutCloud.java(1 hunks)presentation/src/main/java/org/cryptomator/presentation/model/CloudFolderModel.kt(1 hunks)presentation/src/main/java/org/cryptomator/presentation/model/mappers/CloudModelMapper.kt(1 hunks)presentation/src/main/java/org/cryptomator/presentation/presenter/AuthenticateCloudPresenter.kt(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- domain/src/main/java/org/cryptomator/domain/usecases/cloud/LogoutCloud.java
🔇 Additional comments (34)
domain/src/main/java/org/cryptomator/domain/Cloud.kt (2)
8-8: LGTM! Excellent Kotlin idiom improvement.
Converting type() to a non-nullable property makes the code more idiomatic and safer by:
- Following Kotlin's property-first approach
- Eliminating potential null checks
- Providing a cleaner API surface
8-8: Verify implementation updates across the codebase.
Since this is a breaking change, let's verify that all implementations have been properly updated.
presentation/src/main/java/org/cryptomator/presentation/model/CloudFolderModel.kt (1)
Line range hint 20-24: LGTM! Improved Kotlin idiomaticity
The change from type() to property access syntax makes the code more idiomatic Kotlin while maintaining the same functionality. This aligns well with Kotlin's conventions for property access.
data/src/apiKey/java/org/cryptomator/data/cloud/dropbox/DropboxCloudContentRepositoryFactory.java (1)
27-27: LGTM! Change aligns with the PR objective.
The update from type() to getType() correctly implements the intended refactoring of making the Cloud type a non-nullable property.
Let's verify the consistency of this change across all repository factories:
✅ Verification successful
The results show that no files are using the old type() method (first rg command returned empty), while all repository factories consistently use getType() (second rg command returned all factory files). Let me verify the actual implementation in these files to ensure they all follow the same pattern:
Change is consistently implemented across all repository factories
The verification confirms that:
- No files are using the old
type()method anymore - All repository factories consistently use
getType()with the same pattern - Each factory correctly compares with its corresponding cloud type enum value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all repository factories use getType() consistently
# Expected: All cloud type checks should use getType()
# Search for any remaining usage of type() method
rg -l "\.type\(\s*\)\s*=="
# Search for consistent usage of getType()
rg -l "\.getType\(\s*\)\s*=="
Length of output: 834
Script:
#!/bin/bash
# Get the actual implementation of getType() checks in factory files
rg "\.getType\(\s*\)\s*==" -A 1
Length of output: 1928
data/src/apiKey/java/org/cryptomator/data/cloud/pcloud/PCloudContentRepositoryFactory.java (1)
27-27: LGTM! Consistent with the refactoring objective.
The change from type() to getType() aligns with the PR's goal of making the cloud type a non-nullable property.
Let's verify the consistency of this change across other repository factories:
✅ Verification successful
All repository factories consistently use getType()
After examining all repository factory implementations, I can confirm that every factory consistently uses cloud.getType() in their supports() method:
- LocalStorageContentRepositoryFactory:
cloud.getType() == LOCAL - S3CloudContentRepositoryFactory:
cloud.getType() == S3 - WebDavCloudContentRepositoryFactory:
cloud.getType() == WEBDAV - CryptoCloudContentRepositoryFactory:
cloud.getType() == CRYPTO - GoogleDriveCloudContentRepositoryFactory:
cloud.getType() == CloudType.GOOGLE_DRIVE - OnedriveCloudContentRepositoryFactory:
cloud.getType() == ONEDRIVE - PCloudContentRepositoryFactory:
cloud.getType() == PCLOUD - DropboxCloudContentRepositoryFactory:
cloud.getType() == DROPBOX
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all repository factories use getType() consistently
# Expected: All cloud type checks should use getType()
# Search for type() or getType() usage in all repository factories
rg -t java "cloud\.(get)?type\(\)" --glob "*RepositoryFactory.java"
Length of output: 69
Script:
#!/bin/bash
# Let's try a different approach to find repository factory implementations
# First, find all repository factory files
fd "RepositoryFactory.java"
# Then examine their content for type comparisons
echo "=== Content of factory files ==="
fd "RepositoryFactory.java" -x cat {}
Length of output: 13003
data/src/apiKey/java/org/cryptomator/data/cloud/onedrive/OnedriveCloudContentRepositoryFactory.java (1)
27-27: LGTM! Consistent with the refactoring objective.
The change from type() to getType() correctly implements the PR's objective of making the cloud type a non-nullable property while maintaining Java conventions.
Let's verify consistent implementation across other cloud repository factories:
✅ Verification successful
Let me gather more specific information about the implementation across cloud repository factories.
Consistent implementation verified across all cloud repository factories
All cloud repository factory implementations consistently use getType() in their supports method, following the same pattern:
- GoogleDriveCloudContentRepositoryFactory
- S3CloudContentRepositoryFactory
- WebDavCloudContentRepositoryFactory
- PCloudContentRepositoryFactory
- LocalStorageContentRepositoryFactory
- DropboxCloudContentRepositoryFactory
- CryptoCloudContentRepositoryFactory
- OnedriveCloudContentRepositoryFactory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of getType() across all cloud repository factories
# Expected: All cloud repository factories should use getType() in their supports method
# Search for all cloud repository factory implementations
ast-grep --pattern 'class $_ implements CloudContentRepositoryFactory {
$$$
public boolean supports(Cloud cloud) {
$$$
}
$$$
}'
# Verify consistent usage of getType()
rg -l "cloud\.type\(\)" || echo "No occurrences of old type() method found"
Length of output: 260
Script:
#!/bin/bash
# Search for all implementations of supports method using getType()
rg -U "supports.*Cloud.*\{[\s\S]*?getType\(\).*\}" --type java
# Also search for any remaining potential inconsistencies
rg -U "supports.*Cloud.*\{[\s\S]*?\}" --type java
Length of output: 3084
data/src/apkStorePlaystore/java/org/cryptomator/data/cloud/googledrive/GoogleDriveCloudContentRepositoryFactory.java (1)
28-28: LGTM! Verify consistency across other repository factories.
The change from type() to getType() aligns with the PR objective and maintains Java idioms while supporting better Kotlin interop.
Let's verify that this change has been consistently applied across all cloud repository factories:
✅ Verification successful
All repository factories consistently use getType() method
The verification confirms that:
- No occurrences of
cloud.type()were found in the codebase - All cloud repository factories consistently use
cloud.getType()in their type checks:- GoogleDriveCloudContentRepositoryFactory
- OnedriveCloudContentRepositoryFactory
- WebDavCloudContentRepositoryFactory
- S3CloudContentRepositoryFactory
- PCloudContentRepositoryFactory
- DropboxCloudContentRepositoryFactory
- LocalStorageContentRepositoryFactory
- CryptoCloudContentRepositoryFactory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of getType() across all repository factories
# Expected: All cloud repository factories should use getType() instead of type()
# Search for any remaining usage of type() method
echo "Checking for any remaining type() calls:"
rg "cloud\.type\(\)" "data/src"
# Verify getType() usage in all repository factories
echo -e "\nVerifying getType() usage in repository factories:"
rg "cloud\.getType\(\)" "data/src" --type java
Length of output: 1482
data/src/main/java/org/cryptomator/data/cloud/s3/S3CloudContentRepositoryFactory.java (1)
30-30: LGTM! Verify consistent usage across other factory classes.
The change from type() to getType() aligns with the PR objective and maintains the same behavior while making the code more idiomatic.
Let's verify the consistent usage across other factory classes:
✅ Verification successful
Let me verify the actual implementation in these files to ensure they all consistently use getType().
Let me try a different approach to verify the implementations.
✓ Change verified - All factory classes consistently use getType()
The verification confirms that all cloud factory classes (WebDav, LocalStorage, S3, and Crypto) consistently use the new getType() method in their supports method implementation. The changes are uniform across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all cloud factory classes use getType() consistently
# Expected: All factory classes should use cloud.getType()
# Search for any remaining usage of type() method
rg -l "cloud\.type\(\)" "data/src/main/java/org/cryptomator/data/cloud"
# Verify consistent usage of getType()
rg -l "cloud\.getType\(\)" "data/src/main/java/org/cryptomator/data/cloud"
Length of output: 514
Script:
#!/bin/bash
# Show the actual implementation of the supports method in all factory classes
rg -U "supports.*cloud\.getType\(\)" data/src/main/java/org/cryptomator/data/cloud --type java -A 1 -B 1
Length of output: 106
Script:
#!/bin/bash
# Show the actual implementation of the supports method in all factory classes
rg "supports.*\{" -A 2 data/src/main/java/org/cryptomator/data/cloud --type java
Length of output: 1506
domain/src/main/java/org/cryptomator/domain/usecases/cloud/AddOrChangeCloudConnection.java (1)
36-36: LGTM! Change aligns with the non-nullable property refactoring.
The update from cloud.type() to cloud.getType() correctly implements the PR's objective of making the Cloud type a non-nullable property.
Let's verify the consistent application of this change across the codebase:
✅ Verification successful
All occurrences of cloud.type() have been updated to cloud.getType()
The verification confirms that:
- No instances of the old
cloud.type()pattern remain in the codebase - The new
cloud.getType()is consistently used across all cloud-related implementations including:- Core domain logic (Vault, AddOrChangeCloudConnection, LogoutCloud)
- Repository implementations
- All cloud provider factories (WebDav, S3, OneDrive, PCloud, Google Drive, Dropbox, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all occurrences of cloud.type() have been updated to cloud.getType()
# and no old usage patterns remain
# Check for any remaining cloud.type() calls
echo "Checking for remaining cloud.type() calls:"
rg "cloud\.type\(\)"
# Check for consistent usage of getType()
echo "Verifying getType() usage:"
rg "cloud\.getType\(\)"
Length of output: 2033
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloud.java (2)
6-6: LGTM: Import added for nullability annotation
The addition of JetBrains @NotNull annotation import aligns with the PR objective of making the type property non-nullable for better Kotlin interoperability.
21-25: LGTM: Method signature changes align with PR objectives
The changes correctly implement the non-nullable property pattern:
- @NotNull annotation ensures compile-time null safety in Kotlin
- Renamed to follow Java bean convention
- Implementation guarantees non-null return value
Let's verify consistent implementation across other Cloud implementations:
✅ Verification successful
Let me gather more information to verify the Cloud implementations properly.
Let me search for the Cloud interface definition since it wasn't found in the previous results.
Let me try one more search to find the Cloud interface definition.
Based on the comprehensive search results, I can now generate the final response. The evidence shows that all Cloud implementations consistently use @NotNull with getType() and follow the Java bean naming convention.
Verified: Method signature changes are consistently implemented
All Cloud implementations (DropboxCloud, GoogleDriveCloud, LocalStorageCloud, OnedriveCloud, PCloud, S3Cloud, WebDavCloud, and CryptoCloud) consistently:
- Use
@NotNullannotation ongetType() - Follow the Java bean naming convention with
getType() - Return non-null CloudType enum values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of getType() across all Cloud implementations
# Expected: All Cloud implementations should use getType() with @NotNull
# Check for any remaining type() methods
echo "Checking for any remaining type() methods in Cloud implementations:"
rg "type\(\)\s+\{" --type java
# Check for consistent @NotNull usage with getType()
echo -e "\nVerifying @NotNull usage with getType():"
ast-grep --pattern 'class $_ implements Cloud {
$$$
@NotNull
$$$
getType() {
$$$
}
$$$
}'
Length of output: 826
Script:
#!/bin/bash
# Let's try a different approach to find Cloud implementations and their getType methods
# First, find all Cloud implementations
echo "Finding Cloud implementations:"
rg "class.*implements.*Cloud" --type java -A 5
# Then specifically look for getType() methods
echo -e "\nSearching for getType() methods:"
rg "getType\(\)" --type java -A 2
# Also check the Cloud interface definition
echo -e "\nChecking Cloud interface definition:"
fd "Cloud.java" --type f --exec cat {}
Length of output: 47933
Script:
#!/bin/bash
# Find and show the Cloud interface definition
echo "Searching for Cloud interface:"
rg "interface.*Cloud.*\{" --type java -A 10
Length of output: 7867
Script:
#!/bin/bash
# Search for Cloud interface in domain package
echo "Searching for Cloud interface in domain package:"
fd "Cloud.java" --type f --full-path "domain/src/main/java/org/cryptomator/domain" --exec cat {}
Length of output: 21071
data/src/main/java/org/cryptomator/data/cloud/webdav/WebDavCloudContentRepositoryFactory.java (1)
29-29: LGTM! The change aligns with the non-nullable property refactoring.
The update from type() to getType() correctly implements the PR's objective of making the Cloud type property non-nullable, improving type safety.
Let's verify the consistency of this change across other factory classes:
✅ Verification successful
The verification results show that:
- No factory classes are using the old
cloud.type()method (first grep returned empty) - All factory classes consistently use
cloud.getType()(second grep shows all factory implementations)
Change verified: All cloud factory classes consistently use getType()
The migration from type() to getType() has been completed consistently across all cloud factory implementations, including:
- WebDav
- S3
- Crypto
- LocalStorage
- GoogleDrive
- Dropbox
- PCloud
- Onedrive
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all cloud factory classes use getType() consistently
# Expected: All cloud factories should use cloud.getType() instead of cloud.type()
# Search for any remaining cloud.type() calls in factory classes
rg -l "cloud\.type\(\)" --type java | grep "Factory"
# Verify the consistent usage of cloud.getType() in factory classes
rg -l "cloud\.getType\(\)" --type java | grep "Factory"
Length of output: 882
data/src/main/java/org/cryptomator/data/cloud/local/LocalStorageContentRepositoryFactory.java (1)
34-34: LGTM! Change aligns with the non-nullable property refactoring.
The modification from type() to getType() correctly implements the PR's objective of transitioning to a non-nullable property access pattern.
Let's verify consistent implementation across other cloud-related files:
✅ Verification successful
All cloud type checks consistently use getType() across the codebase
The verification confirms that:
- No remaining instances of
type()method calls exist - All cloud-related files consistently use
getType()for type checks:LocalStorageContentRepositoryFactory.javaWebDavCloudContentRepositoryFactory.javaCryptoCloudContentRepositoryFactory.javaS3CloudContentRepositoryFactory.javaCloudRepositoryImpl.javaCloudEntityMapper.java
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of getType() across cloud-related files
# Expected: All cloud type checks should use getType()
# Search for any remaining usage of type() method
echo "Checking for any remaining type() method calls:"
rg "\.type\(\)" "data/src/main/java/org/cryptomator/data/"
# Search for consistent usage of getType()
echo "Verifying getType() usage in cloud-related files:"
rg "\.getType\(\)" "data/src/main/java/org/cryptomator/data/"
Length of output: 1488
presentation/src/main/java/org/cryptomator/presentation/model/mappers/CloudModelMapper.kt (1)
25-25: LGTM! Property access aligns with Kotlin idioms.
The change from method call to property access aligns with the PR objective and follows Kotlin's idiomatic approach.
domain/src/main/java/org/cryptomator/domain/LocalStorageCloud.java (1)
35-39: LGTM! Method signature change aligns with PR objectives.
The changes correctly implement the non-nullable property pattern by:
- Adding
@NotNullannotation to explicitly declare non-null contract - Renaming
type()togetType()for better Java/Kotlin interop - Maintaining the constant return value of
CloudType.LOCAL
Let's verify the interface changes are consistent across all cloud implementations:
domain/src/main/java/org/cryptomator/domain/DropboxCloud.java (2)
Line range hint 67-71: LGTM! Appropriate @NotNull annotation.
The @NotNull annotation on toString() is correct as it returns a constant string literal. This improves API clarity and Kotlin interoperability.
41-43: LGTM! Verify interface compatibility.
The changes align well with the PR objectives by making the type property non-null and following JavaBean conventions, which improves Kotlin interoperability.
Let's verify the interface contract and usage:
✅ Verification successful
Based on the comprehensive results, I can now provide a final response:
The changes to getType() are consistent across the codebase
The verification shows that:
- The Cloud interface in
domain/src/main/java/org/cryptomator/domain/Cloud.ktcorrectly defines the non-null propertyval type: CloudType - All cloud implementations (DropboxCloud, GoogleDriveCloud, S3Cloud, etc.) consistently:
- Override the method with
@NotNullannotation - Return their respective
CloudTypeenum constant
- Override the method with
- All usages of
getType()in the codebase are compatible with non-null return type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Cloud interface declaration and usage patterns
# Check if the Cloud interface has been updated consistently
ast-grep --pattern 'interface Cloud {
$$$
CloudType $_($$_) {
$$$
}
$$$
}'
# Check for any remaining direct type() calls that might break
rg -l '\btype\(\)' --type java
Length of output: 418
Script:
#!/bin/bash
# Let's check the Cloud interface definition and other cloud implementations
rg "interface Cloud" -A 5
# Check the actual usage of type() or getType() in cloud-related files
rg "\.type\(\)|\.getType\(\)" --type java
# Check for other cloud implementations
fd ".*Cloud.java" --type f --exec cat {}
Length of output: 32587
domain/src/main/java/org/cryptomator/domain/GoogleDriveCloud.java (1)
33-35: LGTM! Changes align well with the PR objectives.
The changes effectively:
- Make nullability explicit through
@NotNullannotation, improving Kotlin interoperability - Follow Java bean naming conventions with the
getType()method name - Maintain the existing correct implementation that always returns a non-null
CloudType.GOOGLE_DRIVE
domain/src/main/java/org/cryptomator/domain/OnedriveCloud.java (1)
41-43: LGTM! Clean and idiomatic implementation.
The changes effectively achieve the PR's objective by:
- Making the contract explicitly non-null with
@NotNull - Following Java bean naming conventions for better Kotlin interop
- Maintaining type-safety through enum return type
domain/src/main/java/org/cryptomator/domain/PCloud.java (1)
48-50: LGTM! Verify interface consistency.
The implementation correctly follows the Java bean naming convention and properly enforces non-null contract with @NotNull annotation.
Let's verify that the Cloud interface and all other implementations are consistent with this change:
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudContentRepositoryFactory.java (1)
38-38: LGTM! The change aligns with making the code more Kotlin-friendly.
The update from type() to getType() is consistent with the PR objective of making the cloud type a non-nullable property in Kotlin.
Let's verify that this change has been consistently applied across the codebase:
✅ Verification successful
All cloud type access consistently uses getType() in production code
The verification shows that all cloud-related code in production paths consistently uses getType(). The remaining type() calls found are in the generator module, which is a build-time code generation tool working with Java reflection APIs (e.g., field.type()) and not related to cloud type access.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of getType() vs type() across the codebase
# Expected: All cloud type access should use getType()
# Check for any remaining usage of type()
echo "Checking for remaining type() calls:"
rg "\.type\(\)" --type java
# Check for consistent usage of getType()
echo "Verifying getType() usage:"
rg "\.getType\(\)" --type java
Length of output: 4309
domain/src/main/java/org/cryptomator/domain/WebDavCloud.java (1)
48-50: LGTM! Clean and consistent change.
The change from type() to getType() with @NotNull annotation is well-implemented and aligns with both Java bean conventions and Kotlin interop requirements. The implementation remains correct by returning a constant value.
domain/src/main/java/org/cryptomator/domain/S3Cloud.java (2)
69-71: LGTM! Implementation guarantees non-null return value.
The @NotNull annotation is appropriate here as the method returns a constant enum value CloudType.S3.
69-71: Let me search for more specific information about the Cloud interface and CloudType.
#!/bin/bash
# Search for Cloud interface and CloudType declarations
ast-grep --pattern 'interface Cloud {
$$$
}'
# Search for CloudType definition
ast-grep --pattern 'enum CloudType {
$$$
}'
# Search for getType method specifically
rg -t java "^\s*(public\s+)?(\w+\s+)?getType\(\)"data/src/main/java/org/cryptomator/data/repository/CloudRepositoryImpl.java (1)
50-50: LGTM! The change aligns with making Cloud#type non-nullable.
The update from type() to getType() correctly implements the access to the new non-nullable Kotlin property. Since the property is non-nullable, no additional null checks are needed in this Java code.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
All type() occurrences in Java code are unrelated to Cloud class
The verification shows that:
- All
type()calls found are in generator code and refer to Java reflection API (e.g.,field.type(),getType()) - All Cloud-related code consistently uses
getType()method across the codebase:- Repository implementations (WebDav, S3, Crypto, etc.)
- Mappers (CloudEntityMapper)
- Domain classes (Vault, AddOrChangeCloudConnection)
- Cloud factories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all Java code consistently uses getType() instead of type()
# and that there are no remaining usages of the old type() method.
# Search for any remaining calls to type()
echo "Checking for any remaining type() calls:"
rg "\.type\(\)" --type java
# Search for all getType() calls to confirm consistent usage
echo "Verifying getType() usage:"
rg "\.getType\(\)" --type java
Length of output: 4317
data/src/main/java/org/cryptomator/data/db/mappers/CloudEntityMapper.java (2)
129-129: LGTM!
The change to use getType() in the default case maintains consistency with the property access pattern.
92-93: LGTM! Verify enum coverage.
The changes from type() to getType() align with making Cloud#type a non-nullable property. However, let's verify that the switch statement handles all possible CloudType values.
domain/src/main/java/org/cryptomator/domain/Vault.java (4)
162-162: LGTM! Safe access to non-nullable type property
The change from type() to getType() is properly guarded by the null check on cloud, ensuring safe access to the now non-nullable type property.
Line range hint 223-225: Consider removing redundant null check for cloudType
Since Cloud#getType() is now non-nullable and cloudType is always set from it (in withCloud and withNamePathAndCloudFrom), the null check in validate() might be redundant. However, this depends on whether it's possible to create a Builder without calling these methods.
Let's verify all paths that can set cloudType:
#!/bin/bash
# Search for all assignments to cloudType in the Builder class
ast-grep --pattern 'class Builder {
$$$
this.cloudType = $value;
$$$
}'171-171: LGTM! Proper validation of cloud type consistency
The validation ensures that the cloud type matches the cloud when both are set, maintaining data consistency.
Let's verify that this validation is consistently applied across the codebase:
✅ Verification successful
Validation is correctly implemented and consistently used
The validation in withCloudType is part of a well-designed builder pattern where:
- The
withCloudmethod automatically sets thecloudTypewhen a cloud is provided - The
withCloudTypemethod ensures consistency by validating against any previously set cloud - The
validate()method ensurescloudTypeis set before building - Other parts of the codebase (
CloudRepositoryImpl,CloudConnectionListPresenter, etc.) show consistent cloud type validation patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar validation patterns in other builder classes
ast-grep --pattern 'if ($cloud != null && $cloud.getType() != $cloudType) {
throw new IllegalStateException($message);
}'
Length of output: 125
Script:
#!/bin/bash
# Let's try a broader search for cloud type validation patterns
rg -U "cloud.*getType.*cloudType" -A 2
# Also search for IllegalStateException related to cloud type
rg "throw.*IllegalStateException.*cloud.*type"
# Let's also check the Vault class implementation to understand the context better
fd "Vault.java" --exec cat {}
Length of output: 12709
182-182: Verify null safety of cloud access
The getType() call assumes that cloud is non-null, but this depends on whether vaultFolder.getCloud() can return null. Please verify this assumption.
Let's check if CloudFolder#getCloud() is annotated as non-null:
✅ Verification successful
Based on the search results, I can now generate the final response. The code shows that:
CloudNodeinterface (parent ofCloudFolder) definescloudas a nullable property:val cloud: Cloud?CloudFolderinterface extendsCloudNodeand inherits this nullable property- The
Vaultclass has null checks when setting the cloud field:if (cloud != null)
No null check needed here
The code is safe as written. The cloudType assignment happens inside the withNamePathAndCloudFrom method right after setting the cloud field from vaultFolder.getCloud(). Since we're using the same cloud instance that was just assigned, and the code shows that null checks are properly handled elsewhere in the codebase when needed, there's no risk of NPE here.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CloudFolder class definition and getCloud method
ast-grep --pattern 'class CloudFolder {
$$$
$return_type getCloud() {
$$$
}
$$$
}'
Length of output: 125
Script:
#!/bin/bash
# Let's try searching for CloudFolder class and getCloud method with ripgrep
rg "class CloudFolder" -A 20
Length of output: 8281
Script:
#!/bin/bash
# Search for getCloud method implementation
rg "getCloud\(\)" -A 3
Length of output: 6278
Script:
#!/bin/bash
# Search for CloudFolder usage and initialization
rg "new CloudFolder" -A 3
Length of output: 2632
Script:
#!/bin/bash
# Search for any null checks on getCloud
rg "getCloud\(\)" -B 2 -A 2
Length of output: 7706
Script:
#!/bin/bash
# Let's search for CloudNode interface/class to understand the contract
rg "interface CloudNode|class CloudNode" -A 10
Length of output: 16796
Script:
#!/bin/bash
# Search for CloudFolder interface/class definition
rg "interface CloudFolder|class CloudFolder" -A 10
Length of output: 5513
Script:
#!/bin/bash
# Search for any null checks on cloud field or getCloud() method
rg "if.*cloud\s*[!=]=\s*null|if.*getCloud\(\)\s*[!=]=\s*null" -B 2 -A 2
Length of output: 2065
presentation/src/main/java/org/cryptomator/presentation/presenter/AuthenticateCloudPresenter.kt (2)
339-339: LGTM!
The change from cloud.type() to cloud.type property access is correct and aligns with the PR objective.
Line range hint 117-339: Verify consistent usage of cloud.type across the codebase
Let's verify that all occurrences of cloud type access have been updated from method calls to property access.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the cloud type access changes:
Property access for cloud.type is consistently implemented across the codebase
The search results confirm that:
- In the domain layer,
Cloudinterface definestypeas a property (not a method) withCloudTypereturn type - All cloud implementations (DropboxCloud, OnedriveCloud, etc.) correctly implement
typeas a property - The reviewed code in
AuthenticateCloudPresenter.ktcorrectly uses property access syntax withcloud.type - No instances of method-style access (
cloud.type()) were found in the codebase for cloud type access
The changes in the review are part of a consistent implementation of cloud type as a property rather than a method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining cloud.type() method calls that need to be updated
# Search for remaining .type() method calls
echo "Searching for remaining .type() method calls..."
rg -g '*.{kt,java}' '\.type\(\)'
# Search for updated .type property access for comparison
echo "Searching for updated .type property access..."
rg -g '*.{kt,java}' '(?<!\.|\(|->)type(?!\()'
Length of output: 1746
Script:
#!/bin/bash
# Let's search for cloud type access patterns more specifically
echo "Searching for cloud.type() method calls..."
rg -g '*.{kt,java}' 'cloud\.type\(\)'
echo -e "\nSearching for cloud.type property access..."
rg -g '*.{kt,java}' 'cloud\.type\s+[^(]'
echo -e "\nSearching for CloudType usage context..."
rg -g '*.{kt,java}' 'CloudType' -C 2
Length of output: 117478
data/src/test/java/org/cryptomator/data/cloud/crypto/MasterkeyCryptoCloudProviderTest.kt (1)
159-159: LGTM! Consistent property access pattern applied.
The changes from cloud.type() to cloud.type property access align with Kotlin's idiomatic approach to property access. The modification has been consistently applied across all test methods.
Let's verify that all occurrences of type() method have been replaced with the property access:
Also applies to: 186-186, 220-220, 337-337
✅ Verification successful
Let me gather more specific information about the cloud type usage in the test file and related cloud implementations:
Let me check the actual Cloud implementation to verify the property vs method change:
Property access change is correctly implemented and verified
The verification confirms that:
- The
typeproperty is correctly defined in the Cloud interface (domain/src/main/java/org/cryptomator/domain/Cloud.kt) asval type: CloudType - All usages in the test file
MasterkeyCryptoCloudProviderTest.ktconsistently use the property access syntax - No remaining method-style
type()calls exist in the codebase related to this Cloud type property
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the type() method call
# Test: Search for remaining .type() calls that might have been missed
rg -l '\.type\(\)'
# Test: Verify the new .type property access is used consistently
rg -l '\.type\s+[^(]'
Length of output: 1352
Script:
#!/bin/bash
# Check the specific test file for all type usages
rg -A 2 '\.type' data/src/test/java/org/cryptomator/data/cloud/crypto/MasterkeyCryptoCloudProviderTest.kt
# Check the cloud interface/class definition
ast-grep --pattern 'interface Cloud {
$$$
}'
# Check for any other test files that might be using cloud.type
rg -l 'cloud\.type' 'data/src/test'
Length of output: 838
Script:
#!/bin/bash
# Find and check the Cloud interface/class implementation
rg -A 5 'class.*Cloud.*\{|interface.*Cloud.*\{' data/src/main/java/
# Check for any type property or method definitions
ast-grep --pattern 'val type'
ast-grep --pattern 'fun type()'
Length of output: 20660
| return when (cloud.type) { | ||
| CloudType.DROPBOX -> DropboxCloud.aCopyOf(cloud as DropboxCloud).withUsername(username).build() | ||
| CloudType.ONEDRIVE -> OnedriveCloud.aCopyOf(cloud as OnedriveCloud).withUsername(username).build() | ||
| else -> throw IllegalStateException("Cloud " + cloud.type() + " is not supported") | ||
| else -> throw IllegalStateException("Cloud " + cloud.type + " is not supported") |
There was a problem hiding this comment.
Add missing cases to the when expression
The when expression only handles DROPBOX and ONEDRIVE cases, but other cloud types like GOOGLE_DRIVE, WEBDAV, S3, PCLOUD, and LOCAL could also be passed to this method. This could lead to runtime exceptions.
Consider adding all possible cases or documenting why only these types are handled:
return when (cloud.type) {
CloudType.DROPBOX -> DropboxCloud.aCopyOf(cloud as DropboxCloud).withUsername(username).build()
CloudType.ONEDRIVE -> OnedriveCloud.aCopyOf(cloud as OnedriveCloud).withUsername(username).build()
+ CloudType.GOOGLE_DRIVE -> GoogleDriveCloud.aCopyOf(cloud as GoogleDriveCloud).withUsername(username).build()
+ CloudType.WEBDAV -> WebDavCloud.aCopyOf(cloud as WebDavCloud).withUsername(username).build()
+ CloudType.S3 -> S3Cloud.aCopyOf(cloud as S3Cloud).withUsername(username).build()
+ CloudType.PCLOUD -> PCloud.aCopyOf(cloud as PCloud).withUsername(username).build()
+ CloudType.LOCAL -> LocalStorage.aCopyOf(cloud as LocalStorage).withUsername(username).build()
else -> throw IllegalStateException("Cloud " + cloud.type + " is not supported")
}Committable suggestion skipped: line range outside the PR's diff.
github-actions
bot
removed
the
state:stale
This PR turns
org.cryptomator.domain.Cloud#type()from a nullable function into a non-nullable property. This allows for a simpler usage in Kotlin code and makes the code more idiomatic overall.