Skip to content
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

Allow the injection of background URLSession identifier #33

Merged
merged 11 commits into from
Apr 16, 2024

Conversation

phil1995
Copy link
Collaborator

@phil1995 phil1995 commented Apr 6, 2024

This PR adds the ability to inject the unique identifier used by the background URLSession for each CloudProvider.
This is needed since issues appeared in the FileProviderExtension where we tried to use the same credential, which we usually used to make the identifier unique, for multiple vaults. Therefore, it was possible that more than one background URLSession gets created with the same identifier which then results in a failing URLSession.

Previously we could workaround this issue by just cache each CloudProvider but it seems like iOS creates now a own process for each domain.

Summary by CodeRabbit

  • New Features
    • Introduced enhanced asynchronous handling capabilities for promises, improving error management and operational efficiency.
  • Tests
    • Updated testing framework to support asynchronous test functions, ensuring robust error checking and streamlined test execution.
  • Documentation
    • Enhanced code documentation and project structure to include new asynchronous functionalities.

Copy link

coderabbitai bot commented Apr 6, 2024

Walkthrough

The recent updates across various test files in the CryptomatorCloudAccess project involve transitioning to asynchronous programming using async/await syntax. These changes enhance the test suite's readability and maintainability by simplifying the handling of asynchronous operations and error assertions.

Changes

File Path Change Summary
.../Promise+Async.swift Added extension to Promise for asynchronous operations with error throwing.
.../LocalFileSystemTests.swift, .../CloudProvider+CreateIntermediateFolderTests.swift, .../API/CloudProvider+ConvenienceTests.swift, .../Crypto/DirectoryIdCacheTests.swift, .../Crypto/VaultFormat6/VaultFormat6CloudProviderMockTests.swift, .../Crypto/VaultFormat6/VaultFormat6ProviderDecoratorTests.swift, .../Crypto/VaultFormat6/VaultFormat6ShortenedNameCacheTests.swift, .../Crypto/VaultFormat6/VaultFormat6ShorteningProviderDecoratorTests.swift, .../Crypto/VaultFormat7/VaultFormat7CloudProviderMockTests.swift, .../Crypto/VaultFormat7/VaultFormat7ProviderDecoratorTests.swift, .../Crypto/VaultFormat7/VaultFormat7ShortenedNameCacheTests.swift, .../Crypto/VaultFormat7/VaultFormat7ShorteningProviderDecoratorTests.swift, .../Crypto/VaultFormat8/VaultFormat8ProviderDecoratorTests.swift, .../WebDAV/WebDAVAuthenticatorTests.swift Updated test files to use async/await for asynchronous operations, improving test logic and readability.
.../XCTAssertThrowsError+Async.swift Introduced function for async error assertions in tests.
CryptomatorCloudAccess.xcodeproj/project.pbxproj Included new Swift files in the project for async handling and testing.

🐇✨
In the world of tests and code,
Async whispers, a new mode.
Tests now dance with async grace,
Errors caught in async's embrace.
Cheers to code that async gleams,
In the realm of digital dreams. 🌟
🐇✨


Recent Review Details

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f310d25 and edb2c5f.
Files selected for processing (14)
  • Tests/CryptomatorCloudAccessIntegrationTests/Extensions/CloudProvider+CreateIntermediateFolderTests.swift (2 hunks)
  • Tests/CryptomatorCloudAccessTests/API/CloudProvider+ConvenienceTests.swift (1 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/DirectoryIdCacheTests.swift (2 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat6/VaultFormat6CloudProviderMockTests.swift (2 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat6/VaultFormat6ProviderDecoratorTests.swift (3 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat6/VaultFormat6ShortenedNameCacheTests.swift (1 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat6/VaultFormat6ShorteningProviderDecoratorTests.swift (1 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat7/VaultFormat7CloudProviderMockTests.swift (1 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat7/VaultFormat7ProviderDecoratorTests.swift (3 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat7/VaultFormat7ShortenedNameCacheTests.swift (1 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat7/VaultFormat7ShorteningProviderDecoratorTests.swift (1 hunks)
  • Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat8/VaultFormat8ProviderDecoratorTests.swift (1 hunks)
  • Tests/CryptomatorCloudAccessTests/WebDAV/WebDAVAuthenticatorTests.swift (3 hunks)
  • Tests/CryptomatorCloudAccessTests/WebDAV/WebDAVProviderTests.swift (29 hunks)
Files not summarized due to errors (1)
  • Tests/CryptomatorCloudAccessTests/WebDAV/WebDAVProviderTests.swift: Error: Message exceeds token limit
Additional comments not posted (57)
Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat8/VaultFormat8ProviderDecoratorTests.swift (1)

39-47: The conversion of testCreateFolder to use async/await syntax enhances readability and aligns with modern Swift concurrency practices. The assertions are well-placed to verify the behavior of the createFolder method under test. Ensure that the CloudProviderMock and VaultFormat8ProviderDecorator are correctly handling asynchronous operations as expected.

Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat6/VaultFormat6ShortenedNameCacheTests.swift (1)

51-58: The update to async/await in testGetOriginalPath simplifies the handling of asynchronous operations, making the test more straightforward and easier to maintain. The use of a closure to simulate fetching the original path and the subsequent assertions ensure that the cache's behavior is correctly verified.

Tests/CryptomatorCloudAccessTests/WebDAV/WebDAVAuthenticatorTests.swift (1)

45-54: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [30-51]

The conversion of testVerifyClient to async/await syntax is a positive change, enhancing the clarity and maintainability of the test. The detailed setup of mock responses and the comprehensive assertions ensure that the WebDAV client's authentication process is thoroughly tested.

Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat6/VaultFormat6CloudProviderMockTests.swift (2)

29-38: The update to async/await in testVaultRootContainsFiles simplifies the flow of the test and improves readability. The assertions are well-placed to verify the correct behavior of the mock cloud provider when fetching a list of items.


41-48: The testDir1FileContainsDirId method's conversion to async/await syntax aligns with modern Swift concurrency practices. The test effectively checks the metadata retrieval and file download processes, ensuring that the mock provider behaves as expected.

Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat7/VaultFormat7CloudProviderMockTests.swift (2)

29-38: The conversion of testVaultRootContainsFiles to async/await syntax is well-executed, enhancing the test's clarity and maintainability. The assertions accurately verify the behavior of the mock provider when fetching a list of items.


41-48: The update to async/await in testDir1FileContainsDirId improves the readability and flow of the test. The detailed assertions ensure that the file metadata retrieval and download processes are correctly tested.

Tests/CryptomatorCloudAccessTests/Crypto/DirectoryIdCacheTests.swift (1)

80-88: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [58-84]

The update to async/await in testRecursiveGet significantly simplifies the test logic, making it more readable and easier to maintain. The detailed setup of the cache miss handler and the comprehensive assertions ensure that the cache's recursive retrieval functionality is thoroughly tested.

Tests/CryptomatorCloudAccessIntegrationTests/Extensions/CloudProvider+CreateIntermediateFolderTests.swift (3)

73-77: The testDoesNotCreateAnyFolderForRootPath method correctly verifies that no folders are created when the root path is specified, which aligns with expected behavior. The use of async/await syntax simplifies the test structure.


81-89: The testCreateFolderWithIntermediates method effectively tests the creation of nested folders using async/await syntax. The assertions confirm that the correct folders are created, ensuring the functionality works as intended.


92-100: The testCreateFolderWithIntermediatesIfAFolderAlreadyExists method handles the scenario where a parent folder already exists. The test confirms that only the necessary folders are created, which is crucial for avoiding redundant operations.

Tests/CryptomatorCloudAccessTests/Crypto/VaultFormat7/VaultFormat7ShortenedNameCacheTests.swift (2)

67-74: The update to async/await in testGetOriginalPath1 simplifies the handling of asynchronous operations, making the test more straightforward and easier to maintain. The use of a closure to simulate fetching the original path and the subsequent assertions ensure that the cache's behavior is correctly verified.


77-84: The testGetOriginalPath2 method's conversion to async/await syntax aligns with modern Swift concurrency practices. The test effectively checks the retrieval of the original path for nested directories, ensuring that the cache behaves as expected.

Tests/CryptomatorCloudAccessTests/API/CloudProvider+ConvenienceTests.swift (10)

18-27: LGTM! The conversion to async/await syntax is correctly implemented.


30-33: LGTM! The async/await syntax is correctly used for testing folder creation when the folder already exists.


36-39: LGTM! Proper use of async/await syntax for testing folder creation when the folder does not exist.


42-46: LGTM! Correct implementation of async/await syntax for testing error handling in folder creation.


50-53: LGTM! Proper use of async/await syntax for testing folder deletion when the folder does not exist.


56-59: LGTM! Correct use of async/await syntax for testing folder deletion when the folder exists.


62-66: LGTM! Correct implementation of async/await syntax for testing error handling in folder deletion.


70-74: LGTM! Proper use of async/await syntax for testing item existence check when the item exists.


77-81: LGTM! Proper use of async/await syntax for testing item existence check when the item does not exist.


84-88: LGTM! Correct implementation of async/await syntax for testing error handling in item existence check.

Tests/CryptomatorCloudAccessTests/WebDAV/WebDAVProviderTests.swift (34)

48-64: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [39-58]

The test testFetchItemMetadata is well-structured and covers the basic functionality of fetching item metadata. The use of mock responses and the validation of the metadata properties are correctly implemented. However, consider adding more assertions to cover edge cases or unexpected data formats.


69-97: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [61-75]

The test testFetchItemMetadataWithNotFoundError correctly handles the scenario where the item metadata is not found. The use of a 404 status code in the mock response and the validation of the thrown error are appropriate. Good job on ensuring that the request handler is empty after the test, which helps maintain clean test states.


79-90: The test testFetchItemMetadataWithUnauthorizedError effectively tests the unauthorized access scenario. The setup with a custom mock client that simulates authentication challenges is a good approach. This test ensures that the provider correctly handles HTTP 401 errors.


103-119: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [94-113]

The test testFetchItemMetadataWithMissingResourcetype seems to be designed to handle cases where the resource type is missing from the metadata. However, the test does not explicitly check for the absence of the resource type in the response data. Consider adding an assertion to verify that the resource type is indeed missing or handled correctly.


123-140: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [116-134]

The test testFetchItemList is comprehensive and checks various aspects of the item list fetched. It validates the count and contents of the list, ensuring that specific items are present. This is crucial for verifying the integrity of the list operations.


143-156: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [137-149]

The test testFetchItemListWithNotFoundError handles the scenario where the item list is not found correctly. Using a 404 status code for the mock response and checking the error type is a good practice. This test ensures robust error handling in the list fetching functionality.


162-189: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [153-168]

In testFetchItemListWithTypeMismatchError, the scenario where a non-folder path is requested for a list operation is tested. The setup and assertions are correctly implemented to ensure that the type mismatch error is handled properly.


172-182: The test testFetchItemListWithUnauthorizedError is crucial for ensuring that unauthorized errors are handled correctly during list operations. The setup with a custom mock client for authentication challenges is consistent with other unauthorized tests, which is good for maintainability.


205-220: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [186-214]

The test testDownloadFile covers the download functionality comprehensively. It checks both the retrieval of file data and the correctness of the data saved to the local file system. The assertions on the request counts and data equality are essential for verifying the download logic.


235-249: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [217-242]

The test testDownloadFileWithNotFoundError handles the file not found error scenario effectively. It ensures that the provider reacts correctly to a 404 status code during a download operation. The assertions on the error type and request handling are well-placed.


266-280: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [246-273]

In testDownloadFileWithAlreadyExistsError, the scenario where a file already exists at the destination is tested. This is important for ensuring that the provider does not overwrite existing files without explicit permission. The test setup and assertions are appropriate.


287-301: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [277-294]

The test testDownloadFileWithTypeMismatchError checks the error handling when attempting to download a directory as if it were a file. This is a critical test for ensuring robustness in file operations. The setup and assertions are correctly aimed at this scenario.


304-318: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [298-311]

testDownloadFileWithUnauthorizedError effectively tests the response to unauthorized access during a file download. The use of a custom mock client for simulating authentication challenges is consistent and ensures that the provider's error handling is robust.


343-360: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [315-354]

The test testUploadFile checks the upload functionality by simulating a file upload and verifying the metadata of the uploaded file. The detailed checks on the request handling and the metadata properties ensure that the upload process is correctly implemented.


386-413: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [357-397]

testUploadFileWithReplaceExisting tests the upload functionality with the replace existing option enabled. This is crucial for scenarios where files need to be updated. The test ensures that the provider can handle file replacements correctly.


400-406: The test testUploadFileWithNotFoundError handles the scenario where the file to be uploaded does not exist. This test is important for ensuring that the provider reports errors correctly when source files are missing.


421-435: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [410-428]

In testUploadFileWithAlreadyExistsError, the handling of errors when an existing file is found where a new file is being uploaded without replacement is tested. This scenario is important for data integrity, and the test setup and assertions are well-crafted.


447-458: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [432-451]

testUploadFileWithTypeMismatchError tests error handling when there is a type mismatch during a file upload, such as trying to upload a directory as a file. This test is essential for ensuring that the provider does not perform incorrect file operations.


466-480: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [455-473]

The test testUploadFileWithReplaceExistingAndAlreadyExistsError is similar to testUploadFileWithAlreadyExistsError but with the replace existing flag set. It's good to see both scenarios tested separately to ensure that the flag's behavior is implemented correctly.


494-508: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [477-501]

testUploadFileWithParentFolderDoesNotExistError tests the scenario where the parent directory of the destination path does not exist. This is crucial for ensuring that the provider handles directory structure issues correctly before attempting file operations.


522-536: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [505-529]

The test testUploadFileWithParentFolderDoesNotExistErrorWhenReceiving404Error is a specific case of the previous test where a 404 error is used to simulate the non-existence of the parent folder. It's good to see different error codes being tested for the same scenario.


540-554: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [533-547]

testUploadFileWithUnauthorizedError checks the provider's response to unauthorized attempts to upload files. The consistency in testing unauthorized scenarios across different operations (download, upload, create, etc.) is good for ensuring comprehensive coverage.


558-568: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [551-562]

The test testCreateFolder verifies the folder creation functionality. The use of the MKCOL request and the assertion that it was called correctly are appropriate for this test. It's straightforward and effectively tests the basic create operation.


572-585: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [565-578]

testCreateFolderWithAlreadyExistsError handles the error scenario where the folder to be created already exists. The use of a 405 status code in the mock response is appropriate for this scenario, and the test ensures that the provider handles it correctly.


589-617: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [582-595]

The test testCreateFolderWithParentFolderDoesNotExistError is important for ensuring that the provider checks for the existence of parent directories before attempting to create a new folder. The use of a 409 status code is appropriate for this scenario.


599-610: testCreateFolderWithUnauthorizedError tests the provider's handling of unauthorized attempts to create folders. The setup with a custom mock client for authentication challenges is consistent with other tests, ensuring that the provider's error handling is robust across different operations.


621-632: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [614-626]

The test testDeleteFile checks the delete functionality by simulating a file deletion and verifying that the DELETE request was made correctly. This is a straightforward test that effectively verifies the basic delete operation.


637-665: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [629-643]

testDeleteFileWithNotFoundError handles the error scenario where the file to be deleted does not exist. This test is important for ensuring that the provider reports errors correctly and does not perform delete operations on non-existent files.


647-658: The test testDeleteFileWithUnauthorizedError checks the provider's response to unauthorized attempts to delete files. The consistency in testing unauthorized scenarios is good for ensuring that the provider's error handling is robust.


670-681: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [662-675]

The test testMoveFile verifies the file move functionality by simulating a file move and checking that the MOVE request was made correctly. This test effectively tests the basic move operation and ensures that the provider handles it correctly.


686-699: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [678-692]

testMoveFileWithNotFoundError handles the error scenario where the file to be moved does not exist. This test is crucial for ensuring that the provider does not attempt to move non-existent files and handles errors appropriately.


705-718: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [696-711]

The test testMoveFileWithAlreadyExistsError tests the error handling when the destination file already exists during a move operation. This scenario is important for data integrity, and the test setup and assertions are well-crafted.


723-748: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [715-729]

testMoveFileWithParentFolderDoesNotExistError tests the scenario where the parent directory of the destination path does not exist during a move operation. This is crucial for ensuring that the provider handles directory structure issues correctly before attempting file operations.


733-744: The test testMoveFileWithUnauthorizedError checks the provider's response to unauthorized attempts to move files. The consistency in testing unauthorized scenarios across different operations is good for ensuring comprehensive coverage.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Status

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 0fd3321 and c00d647.
Files selected for processing (11)
  • Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (3 hunks)
  • Sources/CryptomatorCloudAccess/OneDrive/OneDriveCloudProvider.swift (1 hunks)
  • Sources/CryptomatorCloudAccess/PCloud/PCloudCredential.swift (1 hunks)
  • Sources/CryptomatorCloudAccess/WebDAV/WebDAVClient.swift (1 hunks)
  • Sources/CryptomatorCloudAccess/WebDAV/WebDAVSession.swift (1 hunks)
  • Tests/CryptomatorCloudAccessIntegrationTests/CryptoDecorator/VaultFormat6/VaultFormat6GoogleDriveIntegrationTests.swift (3 hunks)
  • Tests/CryptomatorCloudAccessIntegrationTests/CryptoDecorator/VaultFormat6/VaultFormat6OneDriveIntegrationTests.swift (2 hunks)
  • Tests/CryptomatorCloudAccessIntegrationTests/CryptoDecorator/VaultFormat7/VaultFormat7GoogleDriveIntegrationTests.swift (3 hunks)
  • Tests/CryptomatorCloudAccessIntegrationTests/CryptoDecorator/VaultFormat7/VaultFormat7OneDriveIntegrationTests.swift (2 hunks)
  • Tests/CryptomatorCloudAccessIntegrationTests/Google Drive/GoogleDriveCloudProviderIntegrationTests.swift (2 hunks)
  • Tests/CryptomatorCloudAccessIntegrationTests/OneDrive/OneDriveCloudProviderIntegrationTests.swift (2 hunks)
Additional comments not posted (17)
Tests/CryptomatorCloudAccessIntegrationTests/Google Drive/GoogleDriveCloudProviderIntegrationTests.swift (2)

28-28: Ensure that the removal of the useBackgroundSession parameter and the direct use of GoogleDriveCloudProvider(credential: credential) does not introduce any unintended side effects, especially in terms of session management and identifier uniqueness.


35-35: Confirm that the updated initialization of GoogleDriveCloudProvider without the useBackgroundSession parameter is consistent across all test setups and aligns with the new session management strategy.

Tests/CryptomatorCloudAccessIntegrationTests/OneDrive/OneDriveCloudProviderIntegrationTests.swift (2)

28-28: Verify that the removal of the useBackgroundSession parameter from OneDriveCloudProvider initialization is thoroughly tested to ensure that session management behaves as expected.


35-35: Ensure that the updated initialization of OneDriveCloudProvider without the useBackgroundSession parameter is correctly applied in all relevant test cases and aligns with the intended session management improvements.

Tests/CryptomatorCloudAccessIntegrationTests/CryptoDecorator/VaultFormat6/VaultFormat6OneDriveIntegrationTests.swift (1)

25-25: Confirm that the updated OneDriveCloudProvider initialization without the useBackgroundSession parameter does not impact the functionality of the vault format 6 integration tests with OneDrive.

Tests/CryptomatorCloudAccessIntegrationTests/CryptoDecorator/VaultFormat7/VaultFormat7OneDriveIntegrationTests.swift (1)

25-25: Ensure that the removal of the useBackgroundSession parameter in the OneDriveCloudProvider initialization is compatible with the vault format 7 integration tests and does not introduce any regressions.

Tests/CryptomatorCloudAccessIntegrationTests/CryptoDecorator/VaultFormat6/VaultFormat6GoogleDriveIntegrationTests.swift (1)

24-24: Verify that the removal of the useBackgroundSession parameter from GoogleDriveCloudProvider initialization is properly tested and does not affect the functionality of vault format 6 integration tests with Google Drive.

Tests/CryptomatorCloudAccessIntegrationTests/CryptoDecorator/VaultFormat7/VaultFormat7GoogleDriveIntegrationTests.swift (1)

24-24: Ensure that the updated GoogleDriveCloudProvider initialization without the useBackgroundSession parameter is thoroughly evaluated to confirm its compatibility with vault format 7 integration tests.

Sources/CryptomatorCloudAccess/PCloud/PCloudCredential.swift (1)

40-50: The addition of the sessionIdentifier parameter to the createBackgroundClient function is a critical change for ensuring unique session management in PCloud clients. Confirm that this change is consistently applied wherever createBackgroundClient is used and that the unique session identifiers are correctly generated and managed.

Sources/CryptomatorCloudAccess/WebDAV/WebDAVClient.swift (1)

35-37: The introduction of the sessionIdentifier parameter to the withBackgroundSession function in WebDAVClient is crucial for unique session management. Ensure that this change is properly integrated and tested across all uses of WebDAVClient to guarantee that session identifiers are uniquely managed.

Sources/CryptomatorCloudAccess/WebDAV/WebDAVSession.swift (1)

189-190: Ensure the sessionIdentifier passed to createBackgroundSession is managed to be unique across different instances to prevent session conflicts.

Sources/CryptomatorCloudAccess/OneDrive/OneDriveCloudProvider.swift (2)

22-22: Ensure the urlSessionConfiguration passed to the init method is properly configured for different use cases, especially for background sessions.


38-38: Ensure the identifier passed to withBackgroundSession is managed to be unique across different instances to prevent session conflicts.

Sources/CryptomatorCloudAccess/GoogleDrive/GoogleDriveCloudProvider.swift (4)

17-18: LGTM! Using an enum for constants is a clean and effective way to manage static values within the class.


28-35: Consider adding documentation or comments for the urlSessionConfiguration parameter to clarify its purpose and guide its correct usage.


38-43: LGTM! The convenience initializer is a good addition for backward compatibility and ease of use.


46-54: Great addition with the withBackgroundSession method. Ensure the implications of using sharedContainerIdentifier, especially in the context of app extensions and shared data, are clearly documented for developers.

@tobihagemann tobihagemann force-pushed the feature/unique-identifier-bg-session branch from 0bcee8b to 4912c34 Compare April 12, 2024 14:29
@tobihagemann tobihagemann force-pushed the feature/unique-identifier-bg-session branch from 4912c34 to 812ad30 Compare April 12, 2024 14:30
@tobihagemann
Copy link
Member

Looking good, just did some minor refactorings, but really no big deal, could've also just left everything as-is.

I was just wondering: Can you quickly elaborate, why there were no changes in Dropbox and S3? Is Dropbox foreground-only? But what about S3? There is also a useBackgroundSession, but I'm not sure how it works.

Copy link
Member

@tobihagemann tobihagemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't quite answered the question for Dropbox and S3 yet, but I wanted to work on an update this week, because more and more OneDrive users are reporting this issue. 😄

@tobihagemann tobihagemann merged commit 243d39e into develop Apr 16, 2024
3 checks passed
@tobihagemann tobihagemann deleted the feature/unique-identifier-bg-session branch April 16, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants