Map some individual API errors to CloudProviderError.unauthorized #37
Conversation
WalkthroughThe pull request enhances error handling mechanisms in the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Learnings (1)
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: 0
🧹 Outside diff range and nitpick comments (5)
Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (1)
535-536: Enhanced error handling for unauthorized accessThe changes in this file improve the error handling for unauthorized access in both the
executeQueryandexecuteFetchermethods of theGoogleDriveCloudProviderclass. These modifications align well with the PR objectives of standardizing error handling across different cloud providers.Key improvements:
- In
executeQuery, an additional error domain"org.openid.appauth.oauth_token"is now checked for unauthorized access.- In
executeFetcher, unauthorized access errors (status code 401) are now explicitly mapped toCloudProviderError.unauthorized.These changes will help in consistently identifying authentication issues and potentially trigger reauthentication processes when needed.
Consider creating a separate method for error mapping to reduce code duplication and improve maintainability. This could be especially useful if more cloud providers are added in the future.
Also applies to: 573-574
Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift (1)
582-583: Approved with a suggestion for refinement.The change aligns well with the PR objective to map specific API authentication errors to
CloudProviderError.unauthorized. Including both 400 and 401 status codes broadens the range of errors treated as unauthorized, which can improve error handling consistency across different cloud providers.However, treating all 400 status codes as unauthorized might be too broad, as 400 typically indicates a bad request, which isn't always an authentication issue. Consider refining the condition for 400 status codes to only include specific authentication-related errors if the Box API provides more detailed error information.
Consider modifying the condition to be more specific for 400 status codes:
- case let error as BoxAPIError where error.responseInfo.statusCode == 400 || error.responseInfo.statusCode == 401: + case let error as BoxAPIError where error.responseInfo.statusCode == 401 || (error.responseInfo.statusCode == 400 && error.message?.contains("authentication") == true):This suggestion assumes that the
BoxAPIErrorhas amessageproperty that might contain more specific error information. Adjust the condition based on the actual structure of theBoxAPIErrorand the specific authentication-related errors that Box returns with a 400 status code.Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (3)
197-201: Improved error handling for authentication errorsThe changes in the
fetchItemMetadatamethod enhance error handling by specifically checking for authentication errors and mapping them toCloudProviderError.unauthorized. This aligns well with the PR objectives of standardizing authentication error handling.Consider extracting this authentication error check into a separate method for reusability, as it might be needed in other network error handling scenarios throughout the class.
private func handleNetworkError(_ error: Error) -> Error { guard let networkError = error as? DBRequestError else { return error } if networkError.isAuthError() { return CloudProviderError.unauthorized } return convertRequestErrorToDropboxError(networkError) }This method can then be used in all places where network errors are handled.
Line range hint
661-681: Consider updatingconvertRequestErrorToDropboxErrorfor consistencyWhile this method wasn't changed, it's relevant to the PR objectives. To maintain consistency with the new error handling approach:
- Consider mapping
DropboxError.authErrordirectly toCloudProviderError.unauthorizedhere.- Update other parts of the code that use this method to handle the
unauthorizederror appropriately.This change would centralize the authentication error mapping and ensure consistent behavior across the class.
func convertRequestErrorToDropboxError(_ error: DBRequestError) -> Error { if error.isAuthError() { return CloudProviderError.unauthorized } // ... (rest of the method remains the same) }
Line range hint
1-681: Consider a comprehensive review of error handlingThe changes made in this PR improve error handling, particularly for authentication errors, which aligns well with the stated objectives. To further enhance the consistency and robustness of the
DropboxCloudProviderclass:
- Review all methods that interact with the Dropbox API to ensure they handle authentication errors consistently.
- Consider applying the new error handling pattern (checking for
isAuthError()and returningCloudProviderError.unauthorized) in other relevant methods.- Update the class documentation to reflect the new error handling behavior, especially regarding authentication errors.
This comprehensive review will ensure that the entire class benefits from the improved error handling introduced in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift (1 hunks)
- Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (3 hunks)
- Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (1 hunks)
🧰 Additional context used
📓 Learnings (1)
Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift (4)
Learnt from: iammajid PR: cryptomator/cloud-access-swift#34 File: Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift:320-320 Timestamp: 2024-06-21T13:51:55.259Z Learning: iammajid prefers to keep the function lengths and parameter counts as they are in the `BoxCloudProvider` for now.Learnt from: iammajid PR: cryptomator/cloud-access-swift#34 File: Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift:320-320 Timestamp: 2024-10-08T16:37:09.740Z Learning: iammajid prefers to keep the function lengths and parameter counts as they are in the `BoxCloudProvider` for now.Learnt from: iammajid PR: cryptomator/cloud-access-swift#34 File: Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift:320-320 Timestamp: 2024-10-08T16:37:08.970Z Learning: iammajid prefers to keep the function lengths and parameter counts as they are in the `BoxCloudProvider` for now.Learnt from: iammajid PR: cryptomator/cloud-access-swift#34 File: Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift:320-320 Timestamp: 2024-10-08T16:37:09.440Z Learning: iammajid prefers to keep the function lengths and parameter counts as they are in the `BoxCloudProvider` for now.
🔇 Additional comments (4)
Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (2)
535-536: Improved error handling for unauthorized accessThe error handling has been expanded to include an additional error domain
"org.openid.appauth.oauth_token". This change enhances the robustness of the error handling by accommodating potential new error sources related to OAuth token issues.To ensure this change doesn't introduce any regressions, let's verify the usage of
CloudProviderError.unauthorizedacross the codebase:#!/bin/bash # Search for usage of CloudProviderError.unauthorized rg "CloudProviderError\.unauthorized" --type swift
Line range hint
573-574: Consistent error handling for unauthorized access in executeFetcherThe error handling in
executeFetcherhas been updated to check for unauthorized access errors (status code 401) and map them toCloudProviderError.unauthorized. This change ensures consistent error handling betweenexecuteQueryandexecuteFetchermethods.Let's verify the usage of
kGTMSessionFetcherStatusDomainto ensure it's used consistently:✅ Verification successful
It seems that
kGTMSessionFetcherStatusDomainwas not found in the initial search and instead references toCloudProviderError.unauthorizedwere returned. To ensure comprehensive verification, let's perform the following searches:
Consistent usage of
kGTMSessionFetcherStatusDomainconfirmedThe usage of
kGTMSessionFetcherStatusDomaininexecuteFetcheris consistent across the codebase. All instances are used to handle specific error codes, ensuring uniform error handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of kGTMSessionFetcherStatusDomain rg "kGTMSessionFetcherStatusDomain" --type swiftLength of output: 7545
Script:
#!/bin/bash # Search for the definition of kGTMSessionFetcherStatusDomain rg "let\s+kGTMSessionFetcherStatusDomain" --type swift # Search for any occurrences or usages of kGTMSessionFetcherStatusDomain rg "kGTMSessionFetcherStatusDomain" --type swiftLength of output: 722
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (2)
27-28: Good addition for handling authentication errorsThe explicit handling of the
authErrorcase in theshouldRetryForErrorclosure is a positive change. It prevents unnecessary retries on authentication errors, which aligns with best practices for error handling in cloud services.
574-578: Improved Promise chain inensureParentFolderExistsThe refactoring of the
ensureParentFolderExistsmethod improves code readability and follows idiomatic Promise-based patterns. The simplified chain with direct error throwing is more concise and easier to understand.
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift
Outdated
Show resolved
Hide resolved
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift
Outdated
Show resolved
Hide resolved
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift
Outdated
Show resolved
Hide resolved
Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (1)
536-536: Approve change with minor improvement suggestion.The addition of
OIDOAuthTokenErrorDomainerror handling aligns well with the PR objective of mapping specific API authentication errors toCloudProviderError.unauthorized. This change enhances the application's ability to handle reauthentication processes consistently across various cloud providers.To improve code readability and address the static analysis hint, consider the following minor adjustments:
- Remove unnecessary parentheses as suggested by SwiftLint.
- Use a named constant for the magic number
-10to improve code clarity.Here's a suggested refactor:
let OIDOAuthTokenErrorCode = -10 if error.domain == kGTLRErrorObjectDomain && (error.code == 401 || error.code == 403) || error.domain == OIDOAuthTokenErrorDomain && error.code == OIDOAuthTokenErrorCode { reject(CloudProviderError.unauthorized) }This refactor maintains the logic while improving readability and addressing the static analysis hint.
🧰 Tools
🪛 SwiftLint
[Warning] 536-536:
if,for,guard,switch,while, andcatchstatements shouldn't unnecessarily wrap their conditionals or arguments in parentheses(control_statement)
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (1)
195-195: Remove trailing whitespace at line 195There's a trailing whitespace at the end of line 195. Removing it will adhere to coding standards and improve code cleanliness.
Apply this diff to remove the trailing whitespace:
- reject(self.convertRequestError(networkError)) + reject(self.convertRequestError(networkError))🧰 Tools
🪛 SwiftLint
[Warning] 195-195: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift (1 hunks)
- Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (10 hunks)
- Sources/CryptomatorCloudAccess/Dropbox/DropboxError.swift (0 hunks)
- Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (2 hunks)
💤 Files with no reviewable changes (1)
- Sources/CryptomatorCloudAccess/Dropbox/DropboxError.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift
🧰 Additional context used
🪛 SwiftLint
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift
[Warning] 195-195: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift
[Warning] 536-536:
if,for,guard,switch,while, andcatchstatements shouldn't unnecessarily wrap their conditionals or arguments in parentheses(control_statement)
🔇 Additional comments (1)
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (1)
194-195: Consistent use ofconvertRequestErrorimproves error handlingThe updated error handling using
self.convertRequestError(networkError)across multiple methods enhances consistency and maintainability. Good job on applying a uniform approach to error conversion.Also applies to: 244-244, 287-287, 334-334, 398-398, 404-404, 440-440, 476-476, 509-509, 548-548
🧰 Tools
🪛 SwiftLint
[Warning] 195-195: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift
Outdated
Show resolved
Hide resolved
Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (2)
645-665: NewconvertRequestErrormethod enhances error handlingThe addition of the
convertRequestErrormethod is a key implementation of the PR objective. It effectively maps various Dropbox-specific errors to either CloudProviderError or DropboxError types, enhancing error handling consistency across the provider.One suggestion for improvement:
Consider adding a default case to handle any unexpected error types that might be introduced in future Dropbox API updates. This would make the error handling more robust.func convertRequestError(_ error: DBRequestError) -> Error { if error.isHttpError() { return DropboxError.httpError } else if error.isBadInputError() { return DropboxError.badInputError } else if error.isAuthError() { return CloudProviderError.unauthorized } else if error.isAccessError() { return DropboxError.accessError } else if error.isPathRootError() { return DropboxError.pathRootError } else if error.isRateLimitError() { let rateLimitError = error.asRateLimitError() return DropboxError.rateLimitError(retryAfter: rateLimitError.backoff.intValue) } else if error.isInternalServerError() { return DropboxError.internalServerError } else if error.isClientError() { return DropboxError.clientError } else { - return DropboxError.unexpectedError + return DropboxError.unexpectedError(description: error.description) } }This change would provide more context for unexpected errors, aiding in debugging and future error handling improvements.
195-195: Remove trailing whitespaceThere is trailing whitespace on this line. While it doesn't affect functionality, removing it improves code cleanliness.
- reject(self.convertRequestError(networkError)) + reject(self.convertRequestError(networkError))🧰 Tools
🪛 SwiftLint
[Warning] 195-195: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift (1 hunks)
- Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (10 hunks)
- Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: tobihagemann PR: cryptomator/cloud-access-swift#37 File: Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift:645-664 Timestamp: 2024-10-14T15:07:43.088Z Learning: In `DropboxCloudProvider.swift`, `DBRequestError` is not an `Error` object; therefore, within the `convertRequestError(_:)` method, we can only map `authError` to `CloudProviderError.unauthorized`. Other cases return `DropboxError` instances.Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (1)
Learnt from: tobihagemann PR: cryptomator/cloud-access-swift#37 File: Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift:645-664 Timestamp: 2024-10-14T15:07:43.088Z Learning: In `DropboxCloudProvider.swift`, `DBRequestError` is not an `Error` object; therefore, within the `convertRequestError(_:)` method, we can only map `authError` to `CloudProviderError.unauthorized`. Other cases return `DropboxError` instances.
🪛 SwiftLint
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift
[Warning] 195-195: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift
[Warning] 536-536:
if,for,guard,switch,while, andcatchstatements shouldn't unnecessarily wrap their conditionals or arguments in parentheses(control_statement)
🔇 Additional comments (7)
Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (1)
536-536: Improved error handling for unauthorized accessThis change enhances the error handling for unauthorized access by covering additional cases:
- It now includes both 401 and 403 error codes from the
kGTLRErrorObjectDomain.- It adds support for OAuth token errors (
OIDOAuthTokenErrorDomain) with theinvalidGranterror code.These improvements align well with the PR objective of mapping specific API authentication errors to
CloudProviderError.unauthorized, which will help in consistent handling of reauthentication processes across various cloud providers.Regarding the static analysis hint about unnecessary parentheses: In this case, the parentheses actually improve the readability of the complex condition. They clearly group the different error scenarios, making the code easier to understand and maintain. I recommend keeping the parentheses for clarity.
🧰 Tools
🪛 SwiftLint
[Warning] 536-536:
if,for,guard,switch,while, andcatchstatements shouldn't unnecessarily wrap their conditionals or arguments in parentheses(control_statement)
Sources/CryptomatorCloudAccess/Dropbox/DropboxCloudProvider.swift (6)
195-195: Improved error handling infetchItemMetadataThe modification to use
self.convertRequestError(networkError)instead of directly rejecting withnetworkErrorenhances error handling consistency. This change aligns well with the PR objective of standardizing error types across different cloud providers.🧰 Tools
🪛 SwiftLint
[Warning] 195-195: Lines should not have trailing whitespace
(trailing_whitespace)
244-244: Consistent error handling infetchItemListThe change to use
self.convertRequestError(networkError)in this method maintains consistency with the error handling approach used infetchItemMetadata. This modification supports the overall objective of standardizing error types across the cloud provider implementation.
287-287: Enhanced error handling infetchItemListContinueThe modification to use
self.convertRequestError(networkError)maintains consistency with other methods while preserving the specific handling forCloudProviderError.pageTokenInvalid. This approach effectively balances the need for standardized error handling with retaining important specific error cases.
398-398: Standardized error handling inhandleBatchUploadMissingResultThe modifications to use
convertRequestErrorfor bothrequestErrorandfinishBatchRequestErroralign with the overall approach to standardize error handling across the class. These changes contribute to the PR objective of mapping specific API errors to standardized error types, enhancing consistency and maintainability.Also applies to: 404-404
440-440: Consistent error handling across multiple methodsThe changes in
uploadSmallFile,createFolder,deleteItem, andmoveItemmethods to useself.convertRequestError(networkError)instead of directly rejecting withnetworkErrordemonstrate a consistent approach to error handling. This standardization across multiple methods enhances code consistency and aligns with the PR objective of mapping specific API errors to standardized error types.Also applies to: 476-476, 509-509, 548-548
Line range hint
1-665: Summary of changes in DropboxCloudProviderThe modifications in this file successfully implement standardized error handling across the
DropboxCloudProviderclass. The newconvertRequestErrormethod effectively maps Dropbox-specific errors to standardizedCloudProviderErrororDropboxErrortypes. These changes enhance consistency, improve maintainability, and align well with the PR objective of standardizing error handling across different cloud providers.The consistent application of
self.convertRequestError(networkError)across various methods (fetchItemMetadata,fetchItemList,fetchItemListContinue,uploadSmallFile,createFolder,deleteItem, andmoveItem) demonstrates a thorough implementation of this standardization.These changes will likely improve the application's ability to handle reauthentication processes consistently across various cloud providers, as stated in the PR objectives.
There was a problem hiding this comment.
Oh, just noticed that the CI build doesn't run. Some fixes are still needed: https://github.com/cryptomator/cloud-access-swift/actions/runs/11344129567/job/31548212002
This update maps specific API authentication errors to CloudProviderError.unauthorized. By standardizing these errors, the app can handle reauthentication more consistently across different cloud providers. This change addresses the requirements in cryptomator/ios#18.