feat: Add integration with: Dropbox #695
Conversation
|
Qovery can create a Preview Environment for this PR.
This comment has been generated from Qovery AI 🤖.
|
|
|
|
amuwal
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe changes in this pull request introduce integration with Dropbox as a file storage option. This includes the addition of environment variables for Dropbox API credentials, modifications to database schemas to accommodate Dropbox connectors, and the implementation of services and mappers for managing files, folders, groups, and users within Dropbox. The updates extend the existing functionality of the application, allowing it to interact with Dropbox's API for file management and synchronization. Changes
Assessment against linked issues
Possibly related PRs
Tip OpenAI O1 model for chat
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: 12
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (24)
- .env.example (1 hunks)
- packages/api/scripts/init.sql (1 hunks)
- packages/api/scripts/seed.sql (1 hunks)
- packages/api/src/@core/utils/types/original/original.file-storage.ts (5 hunks)
- packages/api/src/filestorage/file/file.module.ts (2 hunks)
- packages/api/src/filestorage/file/services/dropbox/index.ts (1 hunks)
- packages/api/src/filestorage/file/services/dropbox/mappers.ts (1 hunks)
- packages/api/src/filestorage/file/services/dropbox/types.ts (1 hunks)
- packages/api/src/filestorage/folder/folder.module.ts (2 hunks)
- packages/api/src/filestorage/folder/services/dropbox/index.ts (1 hunks)
- packages/api/src/filestorage/folder/services/dropbox/mappers.ts (1 hunks)
- packages/api/src/filestorage/folder/services/dropbox/types.ts (1 hunks)
- packages/api/src/filestorage/group/group.module.ts (2 hunks)
- packages/api/src/filestorage/group/services/box/mappers.ts (1 hunks)
- packages/api/src/filestorage/group/services/dropbox/index.ts (1 hunks)
- packages/api/src/filestorage/group/services/dropbox/mappers.ts (1 hunks)
- packages/api/src/filestorage/group/services/dropbox/types.ts (1 hunks)
- packages/api/src/filestorage/user/services/dropbox/index.ts (1 hunks)
- packages/api/src/filestorage/user/services/dropbox/mappers.ts (1 hunks)
- packages/api/src/filestorage/user/services/dropbox/types.ts (1 hunks)
- packages/api/src/filestorage/user/user.module.ts (2 hunks)
- packages/shared/src/connectors/enum.ts (1 hunks)
- packages/shared/src/connectors/index.ts (1 hunks)
- packages/shared/src/connectors/metadata.ts (1 hunks)
Additional context used
Biome
packages/api/src/filestorage/group/services/dropbox/index.ts
[error] 59-59: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/user/services/dropbox/index.ts
[error] 60-60: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/user/services/dropbox/mappers.ts
[error] 66-66: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
packages/api/src/filestorage/file/services/dropbox/index.ts
[error] 23-23: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 110-110: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
Additional comments not posted (61)
packages/shared/src/connectors/index.ts (1)
7-7: LGTM!The addition of 'dropbox' to the
FILESTORAGE_PROVIDERSarray is consistent with the PR objective of integrating Dropbox into the file storage vertical. This change expands the supported file storage providers and enhances the application's file management capabilities.The change is localized and does not introduce any syntax errors or type mismatches.
packages/shared/src/connectors/enum.ts (1)
30-30: LGTM!The addition of the
DROPBOXentry to theFilestorageConnectorsenum is a valid change that aligns with the PR objective of integrating Dropbox into the file storage vertical. The enum key and value follow the existing naming convention, and the change is unlikely to have any negative impact on the codebase.packages/api/src/filestorage/file/file.module.ts (2)
1-2: LGTM!The imports for
DropboxFileMapperandDropboxServiceare correctly specified and align with the PR objective of integrating Dropbox into the file storage vertical.
33-34: LGTM!The addition of
DropboxServiceandDropboxFileMapperto the list of providers in theFileModuleis correct and aligns with the PR objective of integrating Dropbox functionality into the file storage vertical.packages/api/src/filestorage/user/user.module.ts (2)
1-2: LGTM!The import statements for
DropboxUserMapperandDropboxServiceare correctly written and necessary for using these services in the module.
32-33: Verify the usage of the added Dropbox services.Adding
DropboxServiceandDropboxUserMapperto the module's providers array is necessary for integrating Dropbox functionality. This change makes these services available for dependency injection in the module.Please ensure that the added services are being used correctly in the relevant parts of the codebase. You can use the following script to search for their usage:
Verification successful
DropboxService and DropboxUserMapper are correctly integrated and used
The verification process confirms that both
DropboxServiceandDropboxUserMapperare properly defined and used in the codebase:
DropboxServiceis implemented for user, folder, group, and file services.DropboxUserMapperis defined in the user service.- Both services are imported in
user.module.ts, which aligns with their addition to the providers array.DropboxServiceis also imported in other relevant modules (group, folder, and file).This integration is consistent with the intended Dropbox functionality across different parts of the application.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `DropboxService` and `DropboxUserMapper` in the codebase. # Test 1: Search for the usage of `DropboxService`. Expect: At least one occurrence. rg --type typescript -A 5 $'DropboxService' # Test 2: Search for the usage of `DropboxUserMapper`. Expect: At least one occurrence. rg --type typescript -A 5 $'DropboxUserMapper'Length of output: 158
Script:
#!/bin/bash # Description: Verify the usage of `DropboxService` and `DropboxUserMapper` in the codebase. # Test 1: Search for the declaration and usage of `DropboxService` echo "Searching for DropboxService:" rg -p --glob '*.ts' 'class DropboxService' -A 5 rg -p --glob '*.ts' '@Injectable\(\).*\nclass DropboxService' -A 5 rg -p --glob '*.ts' 'private .*dropboxService:.*DropboxService' # Test 2: Search for the declaration and usage of `DropboxUserMapper` echo "Searching for DropboxUserMapper:" rg -p --glob '*.ts' 'class DropboxUserMapper' -A 5 rg -p --glob '*.ts' '@Injectable\(\).*\nclass DropboxUserMapper' -A 5 rg -p --glob '*.ts' 'private .*dropboxUserMapper:.*DropboxUserMapper' # Test 3: Search for imports of these services echo "Searching for imports:" rg -p --glob '*.ts' 'import.*DropboxService' rg -p --glob '*.ts' 'import.*DropboxUserMapper'Length of output: 3128
packages/api/src/filestorage/group/group.module.ts (2)
1-2: LGTM!The imports for
DropboxGroupMapperandDropboxServiceare necessary to enable Dropbox integration in thegroup.module.tsfile. They follow the existing pattern of importing mappers and services for other cloud storage providers and are correctly named and formatted.
33-34: Looks good!The addition of
DropboxServiceandDropboxGroupMapperto the list of providers in the@Moduledecorator is necessary to make them available for dependency injection within the module. The changes are consistent with the existing providers for other cloud storage services and are correctly formatted.packages/api/src/filestorage/folder/folder.module.ts (2)
1-2: LGTM!The addition of
DropboxFolderMapperandDropboxServiceimports is consistent with the PR objective of integrating Dropbox functionality into the file storage vertical. The import statements are correctly specified and follow the existing import style in the file.
33-34: Looks good!The
DropboxServiceandDropboxFolderMapperproviders have been correctly added to the@Moduledecorator, following the existing style and placement in the file. This addition aligns with the PR objective of integrating Dropbox functionality and is consistent with the corresponding imports added earlier in the file.packages/api/scripts/seed.sql (1)
4-7: Verify if thefs_dropboxcolumn has been created in theconnector_setstable schema.The changes to seed data for the new
fs_dropboxcolumn look good. However, please ensure that the corresponding changes have been made to create this column in theconnector_setstable schema.Run the following script to verify the schema change:
Verification successful
Verification successful:
fs_dropboxcolumn added toconnector_setstableThe
fs_dropboxcolumn has been successfully added to theconnector_setstable schema in thepackages/api/scripts/init.sqlfile. TheCREATE TABLEstatement now includesfs_dropbox boolean NULL,as the last column before the primary key constraint.Additionally, the
packages/api/scripts/seed.sqlfile has been updated to include data for the newfs_dropboxcolumn in theINSERT INTO connector_setsstatement.Both the schema definition and the seed data are consistent with the addition of the
fs_dropboxcolumn, confirming that the necessary changes have been properly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `fs_dropbox` column has been created in the `connector_sets` table schema. # Test: Search for the schema file that creates the `connector_sets` table. # Expect: A file containing the `CREATE TABLE` statement for `connector_sets`. fd --type file --extension sql --exec rg --multiline --multiline-dotall $'CREATE TABLE connector_sets'Length of output: 131
Script:
#!/bin/bash # Description: Search for the `CREATE TABLE connector_sets` statement with surrounding context # Test: Look for the `CREATE TABLE connector_sets` statement and display 20 lines before and after the match rg --multiline --multiline-dotall --before-context 20 --after-context 20 'CREATE TABLE connector_sets' -g '*.sql'Length of output: 2566
Script:
#!/bin/bash # Description: Search for the complete `CREATE TABLE connector_sets` statement # Test: Look for the entire `CREATE TABLE connector_sets` statement including opening and closing parentheses rg --multiline --multiline-dotall 'CREATE TABLE connector_sets\s*\([^)]+\);' -g '*.sql'Length of output: 89
Script:
#!/bin/bash # Description: Search for "connector_sets" in SQL files with extended context # Test: Look for lines containing "connector_sets" in SQL files with 30 lines of context before and after rg -C 30 "connector_sets" -g "*.sql"Length of output: 9826
packages/api/src/filestorage/folder/services/dropbox/types.ts (3)
4-36: LGTM!The
DropboxFolderOutputinterface is well-defined and captures the essential properties of a Dropbox folder. The comments provide clear explanations for each property, making the interface easy to understand and use.
41-56: LGTM!The
SharingInfointerface is well-defined and captures the essential sharing-related properties of a folder. The comments provide clear explanations for each property, making the interface easy to understand and use.
61-73: LGTM!The
DropboxFolderInputinterface is well-defined and captures the essential properties required to create a new folder in Dropbox. The comments provide clear explanations and examples for each property, making the interface easy to understand and use.packages/api/src/filestorage/group/services/dropbox/types.ts (3)
1-34: LGTM!The
DropboxGroupOutputinterface is well-defined and follows TypeScript best practices. The properties have appropriate types and JSDoc comments, making the interface clear and easy to understand.
36-43: LGTM!The
GroupManagementTypetype alias is appropriately defined as a union type of string literals, capturing the possible values for group management type in Dropbox.
45-73: LGTM!The
DropboxGroupInputinterface is well-defined and follows TypeScript best practices. The properties have appropriate types and JSDoc comments, making the interface clear and easy to understand. The optional properties provide flexibility when creating or updating a group in Dropbox.packages/api/src/filestorage/group/services/box/mappers.ts (1)
67-67: LGTM!Initializing the
usersproperty to an empty array ([]) instead ofnullis a good practice. It avoids potential null pointer exceptions and makes the code more robust. It also aligns with the TypeScript best practices of using an empty array to represent the absence of items in a collection.packages/api/src/filestorage/group/services/dropbox/index.ts (2)
13-25: LGTM!The
DropboxServiceclass structure and setup look good. The necessary dependencies are injected, logging is set up, and the service is registered correctly.
27-57: LGTM!The
syncmethod implementation looks good. It correctly retrieves the Dropbox connection details, makes an authenticated request to the Dropbox API to list the groups, and returns the response data with a success message and status code.packages/api/src/filestorage/user/services/dropbox/index.ts (2)
14-25: LGTM!The
DropboxServiceclass structure and constructor look good:
- Implements the
IUserServiceinterface.- Injects relevant dependencies:
PrismaService,LoggerService,EncryptionService,ServiceRegistry.- Sets the logger context.
- Registers the service with the
ServiceRegistry.
27-58: Excellent work on the Dropbox user sync implementation!The
syncmethod aligns perfectly with the PR objective of integrating with Dropbox to retrieve users:
- Finds the correct Dropbox connection using the provided
linkedUserId.- Makes an HTTP POST request to the Dropbox API to list team members.
- Decrypts the access token using the
EncryptionServicebefore adding it to the request headers, ensuring security.- Returns the retrieved Dropbox users with a success message and status code.
packages/api/src/filestorage/group/services/dropbox/mappers.ts (2)
13-20: LGTM!The constructor correctly injects the required dependencies and registers the service with the
MappersRegistry.
32-53: LGTM!The
unifymethod correctly handles both single and array inputs and maps them to the unified format using themapSingleGroupToUnifiedmethod.packages/api/src/filestorage/user/services/dropbox/mappers.ts (4)
13-20: LGTM!The constructor correctly injects the required dependencies and registers the service with the
MappersRegistry.
22-30: Verify the empty implementation.The
desunifymethod has an empty implementation. Is this intentional or a work in progress? If it's a work in progress, consider adding a TODO comment to track the pending implementation.
32-53: LGTM!The
unifymethod correctly handles both single and array ofDropboxUserOutputobjects and maps them toUnifiedFilestorageUserOutputobjects using themapSingleUserToUnifiedmethod.
55-81: LGTM!The
mapSingleUserToUnifiedmethod correctly maps a singleDropboxUserOutputobject to aUnifiedFilestorageUserOutputobject. It handles the custom field mappings and returns a new object with the mapped properties.Tools
Biome
[error] 66-66: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
packages/api/src/filestorage/file/services/dropbox/mappers.ts (3)
17-28: LGTM!The constructor is correctly injecting the required dependencies and registering the service with the
MappersRegistry.
30-43: LGTM!The
desunifymethod is correctly transforming the unified input to the Dropbox-specific format.
45-96: LGTM!The
unifyandmapSingleFileToUnifiedmethods are correctly transforming the Dropbox-specific output to the unified format. Theunifymethod is also correctly handling both single and array inputs by calling themapSingleFileToUnifiedmethod.packages/api/src/filestorage/folder/services/dropbox/mappers.ts (2)
16-109: LGTM!The
DropboxFolderMapperclass is well-structured and follows the Single Responsibility Principle (SRP) by focusing on mapping between Dropbox and unified folder types. The class uses dependency injection to inject the required dependencies and correctly maps between the unified and Dropbox folder types. The usage of optionalcustomFieldMappingsto map custom fields is a nice touch.
16-109: Verify the usage of theDropboxFolderMapperclass in the codebase.Please ensure that the
DropboxFolderMapperclass is being used correctly in the codebase to map between Dropbox and unified folder types.Run the following script to verify the class usage:
Verification successful
DropboxFolderMapper is correctly integrated into the application's dependency injection system.
The DropboxFolderMapper class is properly imported and provided in the FolderModule, making it available for dependency injection throughout the application. This follows the expected pattern for NestJS modules and is consistent with how other file storage providers are handled in the same module.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `DropboxFolderMapper` class in the codebase. # Test: Search for the class usage. Expect: Occurrences of the class being injected and used. rg --type typescript -A 5 $'DropboxFolderMapper'Length of output: 82
Script:
#!/bin/bash # Description: Verify the usage of the `DropboxFolderMapper` class in the codebase. # Find the class definition echo "Class definition:" grep -n "export class DropboxFolderMapper" $(fd -e ts) # Search for usage of DropboxFolderMapper echo -e "\nClass usage:" fd -e ts -x grep -n "DropboxFolderMapper" # Look for potential dependency injection configurations echo -e "\nPotential dependency injection:" fd -e ts -x grep -n "DropboxFolderMapper" {} | grep -i "inject\|provide\|module"Length of output: 88242
Script:
#!/bin/bash # Search for module files importing DropboxFolderMapper echo "Module files importing DropboxFolderMapper:" fd -e ts -x grep -l "import.*DropboxFolderMapper" {} | xargs grep -n "Module({" # Look for service or controller files using DropboxFolderMapper echo -e "\nService or controller files using DropboxFolderMapper:" fd -e ts -x grep -l "DropboxFolderMapper" {} | xargs grep -n "class.*\(Service\|Controller\)" # Examine the content of the file where DropboxFolderMapper is imported echo -e "\nContent of file importing DropboxFolderMapper:" fd -e ts -x grep -l "import.*DropboxFolderMapper" {} | xargs catLength of output: 2030
packages/api/src/@core/utils/types/original/original.file-storage.ts (8)
68-68: LGTM!The
OriginalFileInputtype definition is correctly updated to include the newDropboxFileInputtype.
71-71: LGTM!The
OriginalFolderInputtype definition is correctly updated to include the newDropboxFolderInputtype.
83-83: LGTM!The
OriginalGroupInputtype definition is correctly updated to include the newDropboxGroupInputtype.
86-86: LGTM!The
OriginalUserInputtype definition is correctly updated to include the newDropboxUserInputtype.
100-100: LGTM!The
OriginalFileOutputtype definition is correctly updated to include the newDropboxFileOutputtype.
103-103: LGTM!The
OriginalFolderOutputtype definition is correctly updated to include the newDropboxFolderOutputtype.
115-115: LGTM!The
OriginalGroupOutputtype definition is correctly updated to include the newDropboxGroupOutputtype.
118-118: LGTM!The
OriginalUserOutputtype definition is correctly updated to include the newDropboxUserOutputtype.packages/api/src/filestorage/file/services/dropbox/index.ts (1)
16-26: LGTM!The constructor is properly injecting the required dependencies, setting the logger context, and registering the service with the
ServiceRegistry. The implementation looks good.Tools
Biome
[error] 23-23: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
.env.example (1)
115-117: LGTM!The addition of the Dropbox environment variables is consistent with the PR objectives and follows the existing conventions in the file. The placeholders for the variable values are appropriately left empty in the example file.
packages/api/src/filestorage/folder/services/dropbox/index.ts (4)
20-31: LGTM!The constructor correctly initializes the
DropboxServiceclass by:
- Injecting the required dependencies
- Setting the appropriate logger context
- Registering the service in the
ServiceRegistry
33-67: TheaddFolderfunction implementation looks good.The function correctly:
- Retrieves the Dropbox connection details from the database using the provided
linkedUserId.- Constructs the API request with the necessary headers and request body.
- Returns the API response with a success message and status code.
69-102: ThegetAllFoldersfunction implementation looks good.The function correctly:
- Handles pagination by using a while loop and the
has_moreandcursorproperties from the API response.- Filters the API response to only include folder entries.
104-128: Thesyncfunction implementation looks good.The function correctly:
- Retrieves the Dropbox connection details from the database using the provided
linkedUserId.- Calls the
getAllFoldersfunction to synchronize the folders.- Returns the synchronized folders with a success message and status code.
packages/api/src/filestorage/user/services/dropbox/types.ts (4)
4-141: LGTM!The
DropboxUserProfileinterface is well-defined and covers a comprehensive set of properties for representing a Dropbox team member's profile information. The property names are clear, and the comments provide helpful descriptions. The use of optional properties and string literal types is appropriate. The ISO 8601 timestamp format for date properties is a good choice for interoperability.
146-161: LGTM!The
DropboxUserRoleinterface is simple and well-defined. The property names are clear, and the comments provide sufficient descriptions. Marking therole_id,name, anddescriptionproperties as required is appropriate since a role should always have these attributes.
166-176: LGTM!The
DropboxUserOutputinterface is a simple and clear representation of a Dropbox team member entry returned from the API. It appropriately includes theDropboxUserProfileandDropboxUserRoleinterfaces. Marking theprofileproperty as required and therolesproperty as optional is logical based on the expected data structure.
181-228: LGTM!The
DropboxUserInputinterface effectively captures the input fields required for adding a new Dropbox team member. Marking themember_emailproperty as required is crucial, while keeping other properties optional allows flexibility in specifying additional details. Thesend_welcome_emailproperty defaulting totrueis a sensible choice. The comments provide clear explanations and constraints for each property.packages/api/src/filestorage/file/services/dropbox/types.ts (7)
4-87: LGTM!The
DropboxFileOutputinterface is well-defined with clear and concise documentation for each property. The property names are descriptive, and the types used are appropriate. Good job marking some properties as optional to handle cases where the API might not always return them.
92-117: LGTM!The
SharingInfointerface looks good. The property names and types are appropriate for representing sharing information of a file.
122-127: LGTM!The
ExportInfointerface is correctly defined with an appropriate property name and type for representing the export format of a file.
132-142: LGTM!The
PropertyGroupinterface looks good. It correctly defines the structure of a property group with a template ID and a list of property fields.
147-157: LGTM!The
PropertyFieldinterface is correctly defined with appropriate property names and types for representing a property field.
161-176: LGTM!The
FileLockInfointerface looks good. It correctly defines the structure of file lock information with appropriate property names and types.
181-232: LGTM!The
DropboxFileInputinterface is well-defined with clear and concise documentation for each property. The property names are descriptive, and the types used are appropriate, including union types for properties with multiple possible values. Good job marking some properties as optional to handle cases where they might not always be provided in the request body.packages/api/scripts/init.sql (1)
555-555: LGTM!The new column
fs_dropboxis appropriately added to theconnector_setstable to track Dropbox file storage integration. The column definition is syntactically correct, and thebooleantype is suitable for representing the presence or absence of the integration. AllowingNULLvalues provides flexibility.packages/shared/src/connectors/metadata.ts (2)
2735-2735: LGTM! The addition of thescopesproperty is necessary and well-defined.The specified scopes cover a comprehensive set of permissions required for the Dropbox integration, including reading and writing files and metadata, accessing team data, and member information. This change enhances the connector's capability by clearly defining the permissions needed for its operations.
2738-2738: Verify the compatibility of the updated API endpoint and thoroughly test the connector before enabling it in production.The changes to the
apiUrlandactiveproperties are necessary for utilizing a more stable and feature-rich version of the Dropbox API and enabling the connector for use. However, consider the following:
- Verify that the updated versioned endpoint (
https://api.dropboxapi.com/2) is compatible with the existing implementation and doesn't introduce any breaking changes.- Ensure that the Dropbox connector has been thoroughly tested and validated, considering the updated API endpoint and enabled status, before deploying it to a production environment.
To verify the compatibility of the updated API endpoint, you can run the following script:
This script searches for occurrences of the Dropbox API URL in TypeScript files and includes five lines of context around each match. Review the API calls to ensure they are compatible with the updated versioned endpoint.
Additionally, consider running comprehensive integration tests to validate the functionality of the Dropbox connector with the updated configuration before enabling it in production.
Also applies to: 2743-2743
Verification successful
These scripts will help us gather more comprehensive information about the usage of the Dropbox API in the codebase, allowing us to better assess the compatibility of the updated endpoint.
The Dropbox API URL update appears to be an isolated change with low compatibility risk.
Based on the comprehensive search of the codebase, we can confirm that:
- The update to the Dropbox API URL (from v1 to v2) is isolated to a single location in
packages/shared/src/connectors/metadata.ts.- No other occurrences of the Dropbox API URL were found in the codebase, suggesting that the API is not directly used elsewhere.
Given these findings, the compatibility risk appears to be low. However, to ensure a smooth transition to the v2 API, please:
- Thoroughly test the Dropbox connector functionality to verify compatibility with the updated API endpoint.
- Pay special attention to any features that interact with the Dropbox API, as the impact may not be immediately apparent in the code.
The change is approved, but remember that comprehensive testing is crucial before enabling the connector in a production environment.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the compatibility of the updated Dropbox API endpoint. # Test: Search for Dropbox API usage in the codebase. # Expect: Review the API calls to ensure compatibility with the updated endpoint. rg --type typescript -A 5 $'https://api.dropboxapi.com'Length of output: 89
Script:
#!/bin/bash # Description: Verify the compatibility of the updated Dropbox API endpoint. # Test 1: Search for Dropbox API usage in the codebase without specifying file type. echo "Searching for new Dropbox API URL (v2):" rg -n -A 5 'https://api.dropboxapi.com/2' echo -e "\nSearching for old Dropbox API URL:" rg -n -A 5 'https://api.dropboxapi.com/1' # Test 2: Find TypeScript files and search within them for Dropbox API usage. echo -e "\nSearching for Dropbox API usage in TypeScript files:" fd -e ts -e tsx | xargs rg -n -A 5 'https://api.dropboxapi.com' # Test 3: Use ast-grep to find function calls or string literals containing the Dropbox API URL. echo -e "\nSearching for Dropbox API usage using AST:" ast-grep --lang typescript --pattern '$_($$$, "https://api.dropboxapi.com", $$$)' ast-grep --lang typescript --pattern '"https://api.dropboxapi.com"'Length of output: 1971
| } catch (error) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Remove the redundant catch clause.
The static analysis hint correctly points out that the catch clause only rethrows the original error, making it redundant.
Apply this diff to remove the redundant catch clause:
- } catch (error) {
- throw error;
- }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| throw error; | |
| } | |
| } |
Tools
Biome
[error] 59-59: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
| } catch (error) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Remove the redundant catch block.
The catch block that only rethrows the original error is redundant and can be confusing.
Apply this diff to remove it:
- } catch (error) {
- throw error;
- }
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| throw error; | |
| } | |
| } |
Tools
Biome
[error] 60-60: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
| async desunify( | ||
| source: UnifiedFilestorageGroupInput, | ||
| customFieldMappings?: { | ||
| slug: string; | ||
| remote_id: string; | ||
| }[], | ||
| ): Promise<DropboxGroupInput> { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Implement the desunify method.
The desunify method is not implemented. It should map the UnifiedFilestorageGroupInput to a DropboxGroupInput.
Do you want me to generate the implementation for the desunify method or open a GitHub issue to track this task?
| private async mapSingleGroupToUnified( | ||
| group: DropboxGroupOutput, | ||
| connectionId: string, | ||
| customFieldMappings?: { | ||
| slug: string; | ||
| remote_id: string; | ||
| }[], | ||
| ): Promise<UnifiedFilestorageGroupOutput> { | ||
| const field_mappings: { [key: string]: any } = {}; | ||
| if (customFieldMappings) { | ||
| for (const mapping of customFieldMappings) { | ||
| field_mappings[mapping.slug] = group[mapping.remote_id]; | ||
| } | ||
| } | ||
| return { | ||
| remote_id: group.group_id, | ||
| remote_data: group, | ||
| name: group.group_name, | ||
| users: [], | ||
| field_mappings, | ||
| remote_was_deleted: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
LGTM with potential improvements!
The mapSingleGroupToUnified method correctly maps the DropboxGroupOutput to a UnifiedFilestorageGroupOutput object.
Consider the following potential improvements:
- Populate the
usersfield with the actual users of the group if available. - Update the
remote_was_deletedfield based on the actual status of the group if available.
| const field_mappings: { [key: string]: any } = {}; | ||
| if (customFieldMappings) { | ||
| for (const mapping of customFieldMappings) { | ||
| if (user.profile.hasOwnProperty(mapping.slug)) { |
There was a problem hiding this comment.
Use Object.hasOwn() instead of hasOwnProperty.
The static analysis tool has flagged the use of hasOwnProperty method as a potential issue. It's recommended to use Object.hasOwn() instead.
Apply this diff to fix the issue:
-if (user.profile.hasOwnProperty(mapping.slug)) {
+if (Object.hasOwn(user.profile, mapping.slug)) {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (user.profile.hasOwnProperty(mapping.slug)) { | |
| if (Object.hasOwn(user.profile, mapping.slug)) { |
Tools
Biome
[error] 66-66: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
| async sync(data: SyncParam): Promise<ApiResponse<DropboxFileOutput[]>> { | ||
| try { | ||
| const { linkedUserId, id_folder } = data; | ||
| if (!id_folder) return; | ||
|
|
||
| const connection = await this.prisma.connections.findFirst({ | ||
| where: { | ||
| id_linked_user: linkedUserId, | ||
| provider_slug: 'dropbox', | ||
| vertical: 'filestorage', | ||
| }, | ||
| }); | ||
|
|
||
| const folder = await this.prisma.fs_folders.findUnique({ | ||
| where: { | ||
| id_fs_folder: id_folder as string, | ||
| }, | ||
| }); | ||
|
|
||
| const remote_data = await this.prisma.remote_data.findFirst({ | ||
| where: { | ||
| ressource_owner_id: folder.id_fs_folder, | ||
| }, | ||
| }); | ||
|
|
||
| const folder_remote_data = JSON.parse(remote_data.data); | ||
|
|
||
| const files = await this.getAllFilesInFolder( | ||
| folder_remote_data.path_display, | ||
| connection, | ||
| ); | ||
|
|
||
| this.logger.log(`Synced dropbox files !`); | ||
|
|
||
| return { | ||
| data: files, | ||
| message: 'Dropbox files retrieved', | ||
| statusCode: 200, | ||
| }; | ||
| } catch (error) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove the redundant catch clause.
The method implementation looks good. It correctly synchronizes the files by retrieving the necessary data from the database and calling the getAllFilesInFolder method. Returning an ApiResponse with the retrieved files is appropriate.
However, as flagged by static analysis, the catch clause that only rethrows the original error is redundant and can be confusing. It is recommended to remove it.
Apply this diff to remove the redundant catch clause:
- } catch (error) {
- throw error;
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async sync(data: SyncParam): Promise<ApiResponse<DropboxFileOutput[]>> { | |
| try { | |
| const { linkedUserId, id_folder } = data; | |
| if (!id_folder) return; | |
| const connection = await this.prisma.connections.findFirst({ | |
| where: { | |
| id_linked_user: linkedUserId, | |
| provider_slug: 'dropbox', | |
| vertical: 'filestorage', | |
| }, | |
| }); | |
| const folder = await this.prisma.fs_folders.findUnique({ | |
| where: { | |
| id_fs_folder: id_folder as string, | |
| }, | |
| }); | |
| const remote_data = await this.prisma.remote_data.findFirst({ | |
| where: { | |
| ressource_owner_id: folder.id_fs_folder, | |
| }, | |
| }); | |
| const folder_remote_data = JSON.parse(remote_data.data); | |
| const files = await this.getAllFilesInFolder( | |
| folder_remote_data.path_display, | |
| connection, | |
| ); | |
| this.logger.log(`Synced dropbox files !`); | |
| return { | |
| data: files, | |
| message: 'Dropbox files retrieved', | |
| statusCode: 200, | |
| }; | |
| } catch (error) { | |
| throw error; | |
| } | |
| } | |
| async sync(data: SyncParam): Promise<ApiResponse<DropboxFileOutput[]>> { | |
| try { | |
| const { linkedUserId, id_folder } = data; | |
| if (!id_folder) return; | |
| const connection = await this.prisma.connections.findFirst({ | |
| where: { | |
| id_linked_user: linkedUserId, | |
| provider_slug: 'dropbox', | |
| vertical: 'filestorage', | |
| }, | |
| }); | |
| const folder = await this.prisma.fs_folders.findUnique({ | |
| where: { | |
| id_fs_folder: id_folder as string, | |
| }, | |
| }); | |
| const remote_data = await this.prisma.remote_data.findFirst({ | |
| where: { | |
| ressource_owner_id: folder.id_fs_folder, | |
| }, | |
| }); | |
| const folder_remote_data = JSON.parse(remote_data.data); | |
| const files = await this.getAllFilesInFolder( | |
| folder_remote_data.path_display, | |
| connection, | |
| ); | |
| this.logger.log(`Synced dropbox files !`); | |
| return { | |
| data: files, | |
| message: 'Dropbox files retrieved', | |
| statusCode: 200, | |
| }; | |
| } | |
| } |
Tools
Biome
[error] 110-110: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
| async getAllFilesInFolder( | ||
| folderPath: string, | ||
| connection: any, | ||
| ): Promise<DropboxFileOutput[]> { | ||
| // ref: https://www.dropbox.com/developers/documentation/http/documentation#files-list_folder | ||
| const files: DropboxFileOutput[] = []; | ||
| let cursor: string | null = null; | ||
| let hasMore = true; | ||
|
|
||
| while (hasMore) { | ||
| const url = cursor | ||
| ? `${connection.account_url}/files/list_folder/continue` | ||
| : `${connection.account_url}/files/list_folder`; | ||
|
|
||
| const data = cursor ? { cursor } : { path: folderPath, recursive: false }; | ||
|
|
||
| try { | ||
| const response = await axios.post(url, data, { | ||
| headers: { | ||
| Authorization: `Bearer ${this.cryptoService.decrypt( | ||
| connection.access_token, | ||
| )}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| const { entries, has_more, cursor: newCursor } = response.data; | ||
|
|
||
| // Collect all file entries | ||
| files.push(...entries.filter((entry: any) => entry['.tag'] === 'file')); | ||
|
|
||
| hasMore = has_more; | ||
| cursor = newCursor; | ||
| } catch (error) { | ||
| console.error('Error listing files in folder:', error); | ||
| throw new Error('Failed to list all files in the folder.'); | ||
| } | ||
| } | ||
|
|
||
| return files; | ||
| } |
There was a problem hiding this comment.
Enhance error handling.
The method implementation looks good overall. It correctly retrieves all files in a specified Dropbox folder, handles pagination, and filters the entries to only include files.
However, the error handling could be improved:
- Provide more specific error messages based on the type of error encountered.
- Log the error using the injected
LoggerServicefor easier debugging.
Apply this diff to enhance the error handling:
} catch (error) {
- console.error('Error listing files in folder:', error);
+ this.logger.error('Error listing files in folder:', error);
- throw new Error('Failed to list all files in the folder.');
+ throw new Error(`Failed to list all files in the folder: ${error.message}`);
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getAllFilesInFolder( | |
| folderPath: string, | |
| connection: any, | |
| ): Promise<DropboxFileOutput[]> { | |
| // ref: https://www.dropbox.com/developers/documentation/http/documentation#files-list_folder | |
| const files: DropboxFileOutput[] = []; | |
| let cursor: string | null = null; | |
| let hasMore = true; | |
| while (hasMore) { | |
| const url = cursor | |
| ? `${connection.account_url}/files/list_folder/continue` | |
| : `${connection.account_url}/files/list_folder`; | |
| const data = cursor ? { cursor } : { path: folderPath, recursive: false }; | |
| try { | |
| const response = await axios.post(url, data, { | |
| headers: { | |
| Authorization: `Bearer ${this.cryptoService.decrypt( | |
| connection.access_token, | |
| )}`, | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| const { entries, has_more, cursor: newCursor } = response.data; | |
| // Collect all file entries | |
| files.push(...entries.filter((entry: any) => entry['.tag'] === 'file')); | |
| hasMore = has_more; | |
| cursor = newCursor; | |
| } catch (error) { | |
| console.error('Error listing files in folder:', error); | |
| throw new Error('Failed to list all files in the folder.'); | |
| } | |
| } | |
| return files; | |
| } | |
| async getAllFilesInFolder( | |
| folderPath: string, | |
| connection: any, | |
| ): Promise<DropboxFileOutput[]> { | |
| // ref: https://www.dropbox.com/developers/documentation/http/documentation#files-list_folder | |
| const files: DropboxFileOutput[] = []; | |
| let cursor: string | null = null; | |
| let hasMore = true; | |
| while (hasMore) { | |
| const url = cursor | |
| ? `${connection.account_url}/files/list_folder/continue` | |
| : `${connection.account_url}/files/list_folder`; | |
| const data = cursor ? { cursor } : { path: folderPath, recursive: false }; | |
| try { | |
| const response = await axios.post(url, data, { | |
| headers: { | |
| Authorization: `Bearer ${this.cryptoService.decrypt( | |
| connection.access_token, | |
| )}`, | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| const { entries, has_more, cursor: newCursor } = response.data; | |
| // Collect all file entries | |
| files.push(...entries.filter((entry: any) => entry['.tag'] === 'file')); | |
| hasMore = has_more; | |
| cursor = newCursor; | |
| } catch (error) { | |
| this.logger.error('Error listing files in folder:', error); | |
| throw new Error(`Failed to list all files in the folder: ${error.message}`); | |
| } | |
| } | |
| return files; | |
| } |
| try { | ||
| const connection = await this.prisma.connections.findFirst({ | ||
| where: { | ||
| id_linked_user: linkedUserId, | ||
| provider_slug: 'dropbox', | ||
| vertical: 'filestorage', | ||
| }, | ||
| }); | ||
| // ref: https://www.dropbox.com/developers/documentation/http/documentation#files-create_folder | ||
| const resp = await axios.post( | ||
| `${connection.account_url}/files/create_folder_v2`, | ||
| JSON.stringify(folderData), | ||
| { | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${this.cryptoService.decrypt( | ||
| connection.access_token, | ||
| )}`, | ||
| }, | ||
| }, | ||
| ); | ||
| return { | ||
| data: resp?.data, | ||
| message: 'Dropbox folder created', | ||
| statusCode: 201, | ||
| }; | ||
| } catch (error) { | ||
| console.log(error.response); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Improve error handling using the handle3rdPartyServiceError utility function.
Consider replacing the generic try-catch block with the handle3rdPartyServiceError utility function for more robust error handling. This utility function provides a standardized way to handle errors from third-party services.
try {
// ...
} catch (error) {
handle3rdPartyServiceError(
error,
ActionType.CREATE,
FileStorageObject.folder,
'dropbox',
);
}| while (hasMore) { | ||
| const url = cursor | ||
| ? `${connection.account_url}/files/list_folder/continue` | ||
| : `${connection.account_url}/files/list_folder`; | ||
| const data = cursor ? { cursor } : { path: '', recursive: true }; | ||
|
|
||
| const response = await axios.post(url, data, { | ||
| headers: { | ||
| Authorization: `Bearer ${this.cryptoService.decrypt( | ||
| connection.access_token, | ||
| )}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| const { entries, has_more, cursor: newCursor } = response.data; | ||
|
|
||
| // Collect all folder entries | ||
| folders.push( | ||
| ...entries.filter((entry: any) => entry['.tag'] === 'folder'), | ||
| ); | ||
|
|
||
| hasMore = has_more; | ||
| cursor = newCursor; | ||
| } |
There was a problem hiding this comment.
Consider refactoring the while loop to use for await...of syntax.
The current implementation using a while loop is correct but can be refactored to use for await...of syntax for better readability and conciseness.
async function* listFolders(connection: any): AsyncGenerator<any> {
let cursor: string | null = null;
while (true) {
const url = cursor
? `${connection.account_url}/files/list_folder/continue`
: `${connection.account_url}/files/list_folder`;
const data = cursor ? { cursor } : { path: '', recursive: true };
const response = await axios.post(url, data, {
headers: {
Authorization: `Bearer ${this.cryptoService.decrypt(
connection.access_token,
)}`,
'Content-Type': 'application/json',
},
});
const { entries, has_more, cursor: newCursor } = response.data;
yield entries;
if (!has_more) {
break;
}
cursor = newCursor;
}
}
async getAllFolders(connection: any): Promise<DropboxFolderOutput[]> {
const folders: DropboxFolderOutput[] = [];
for await (const entries of this.listFolders(connection)) {
folders.push(...entries.filter((entry: any) => entry['.tag'] === 'folder'));
}
return folders;
}| try { | ||
| const { linkedUserId } = data; | ||
|
|
||
| const connection = await this.prisma.connections.findFirst({ | ||
| where: { | ||
| id_linked_user: linkedUserId, | ||
| provider_slug: 'dropbox', | ||
| vertical: 'filestorage', | ||
| }, | ||
| }); | ||
|
|
||
| const results = await this.getAllFolders(connection); | ||
| this.logger.log(`Synced dropbox folders !`); | ||
|
|
||
| return { | ||
| data: results, | ||
| message: 'Dropbox folders retrieved', | ||
| statusCode: 200, | ||
| }; | ||
| } catch (error) { | ||
| console.log(error.response); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Improve error handling using the handle3rdPartyServiceError utility function.
Consider replacing the generic try-catch block with the handle3rdPartyServiceError utility function for more robust error handling. This utility function provides a standardized way to handle errors from third-party services.
try {
// ...
} catch (error) {
handle3rdPartyServiceError(
error,
ActionType.SYNC,
FileStorageObject.folder,
'dropbox',
);
}
naelob
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 9198067 | Triggered | Generic Password | c178763 | .env.example | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (11)
- .env.example (1 hunks)
- packages/api/scripts/init.sql (1 hunks)
- packages/api/scripts/seed.sql (1 hunks)
- packages/api/src/@core/utils/types/original/original.file-storage.ts (5 hunks)
- packages/api/src/filestorage/file/file.module.ts (2 hunks)
- packages/api/src/filestorage/folder/folder.module.ts (2 hunks)
- packages/api/src/filestorage/group/group.module.ts (2 hunks)
- packages/api/src/filestorage/user/user.module.ts (2 hunks)
- packages/shared/src/connectors/enum.ts (1 hunks)
- packages/shared/src/connectors/index.ts (1 hunks)
- packages/shared/src/connectors/metadata.ts (1 hunks)
Additional context used
Biome
packages/shared/src/connectors/enum.ts
[error] 32-32: expected
,but instead foundSHAREPOINTRemove SHAREPOINT
(parse)
packages/api/src/@core/utils/types/original/original.file-storage.ts
[error] 125-126: Expected a statement but instead found '| GoogleDriveFileInput
| DropboxFileInput'.Expected a statement here.
(parse)
[error] 133-134: Expected a statement but instead found '| GoogleDriveFolderInput
| DropboxFolderInput'.Expected a statement here.
(parse)
[error] 182-183: Expected a statement but instead found '| GoogleDriveFileOutput
| DropboxFileOutput'.Expected a statement here.
(parse)
[error] 190-191: Expected a statement but instead found '| GoogleDriveFolderOutput
| DropboxFolderOutput'.Expected a statement here.
(parse)
Additional comments not posted (34)
packages/shared/src/connectors/index.ts (1)
7-7: LGTM! Verify if the Google Drive integration is also intended.The addition of 'dropbox' to the
FILESTORAGE_PROVIDERSarray is consistent with the PR objective to integrate Dropbox into the file storage vertical.However, I noticed that 'googledrive' is also added. Is the Google Drive integration intended as part of this PR? If so, please ensure that the corresponding implementation for Google Drive is complete and the PR objectives are updated accordingly.
packages/shared/src/connectors/enum.ts (1)
29-30: LGTM!The addition of the
DROPBOXentry to theFilestorageConnectorsenum is a valid change that expands the supported file storage services. This aligns with the PR objective of integrating Dropbox into the file storage vertical.packages/api/src/filestorage/user/user.module.ts (2)
1-2: LGTM!The imports for
DropboxUserMapperandDropboxServiceare correctly added at the top of the file. These imports are necessary to use the Dropbox functionality in the module.
36-37: LGTM!The
DropboxServiceandDropboxUserMapperare correctly added to the module's providers array. This is necessary to use the Dropbox functionality in the module and is consistent with the imports added at the top of the file.packages/api/src/filestorage/group/group.module.ts (2)
1-2: LGTM!The imports for
DropboxGroupMapperandDropboxServiceare correctly specified and follow the existing import style in the file. The addition of these components aligns with the PR objective of integrating with Dropbox.
37-38: LGTM!The
DropboxServiceandDropboxGroupMapperare correctly added to the list of providers in the@Moduledecorator. This is necessary for the module to use the Dropbox-related functionality and aligns with the PR objective of integrating with Dropbox.packages/api/scripts/seed.sql (2)
5-8: The changes align with the PR objective and expand the integration scope.The addition of
fs_dropbox,fs_sharepoint, andfs_onedrivecolumns to theconnector_setstable is consistent with the PR objective of integrating Dropbox into the file storage vertical. It's great to see that the integration scope has been expanded to include SharePoint and OneDrive as well.The SQL syntax and the data being inserted look good.
6-8: Verify if enabling the new integrations by default for all connector sets is intended.The values for the new
fs_dropbox,fs_sharepoint, andfs_onedrivecolumns are set toTRUEfor all existing rows in theconnector_setstable. This suggests that the new file storage integrations will be enabled by default for all connector sets.Please confirm if this is the intended behavior. If not, consider updating the values based on the specific requirements for each connector set.
packages/api/src/filestorage/file/file.module.ts (2)
1-2: LGTM!The imports for
DropboxFileMapperandDropboxServiceare correctly added and follow the existing import style. This is a good addition as part of the Dropbox integration.
42-43: Looks good!
DropboxServiceandDropboxFileMapperare correctly added to the list of providers. This is a necessary step for the Dropbox integration.packages/api/src/filestorage/folder/folder.module.ts (2)
1-2: LGTM!The imports for
DropboxFolderMapperandDropboxServiceare correctly added to support the Dropbox integration. The import paths are consistent with the existing pattern.
42-44: Looks good!The
DropboxServiceandDropboxFolderMapperare correctly added to the list of providers in the module definition. This ensures that they can be injected as dependencies where needed..env.example (1)
115-117: LGTM!The addition of the Dropbox environment variables for file storage integration is consistent with the existing pattern and naming convention. The variables are correctly placed under the "File Storage" section.
Please ensure that the actual client ID and secret are securely provided by the user when deploying the application.
packages/api/src/@core/utils/types/original/original.file-storage.ts (17)
1-2: LGTM!The import statement for
DropboxGroupInputandDropboxGroupOutputtypes is correct and necessary for integrating Dropbox group types into the file storage API.
3-4: LGTM!The import statement for
DropboxUserInputandDropboxUserOutputtypes is correct and necessary for integrating Dropbox user types into the file storage API.
5-6: LGTM!The import statement for
DropboxFileInputandDropboxFileOutputtypes is correct and necessary for integrating Dropbox file types into the file storage API.
7-8: LGTM!The import statement for
DropboxFolderInputandDropboxFolderOutputtypes is correct and necessary for integrating Dropbox folder types into the file storage API.
119-126: LGTM!The change to include
GoogleDriveFileInputandDropboxFileInputtypes in theOriginalFileInputtype aligns with the PR objective of integrating Dropbox into the file storage API.Tools
Biome
[error] 125-126: Expected a statement but instead found '| GoogleDriveFileInput
| DropboxFileInput'.Expected a statement here.
(parse)
125-126: Skipping the static analysis hint.The parsing error flagged by the static analysis tool is a false positive because the code segment is a valid type definition.
Tools
Biome
[error] 125-126: Expected a statement but instead found '| GoogleDriveFileInput
| DropboxFileInput'.Expected a statement here.
(parse)
133-134: LGTM!The change to include
GoogleDriveFolderInputandDropboxFolderInputtypes in theOriginalFolderInputtype aligns with the PR objective of integrating Dropbox into the file storage API.Tools
Biome
[error] 133-134: Expected a statement but instead found '| GoogleDriveFolderInput
| DropboxFolderInput'.Expected a statement here.
(parse)
133-134: Skipping the static analysis hint.The parsing error flagged by the static analysis tool is a false positive because the code segment is a valid type definition.
Tools
Biome
[error] 133-134: Expected a statement but instead found '| GoogleDriveFolderInput
| DropboxFolderInput'.Expected a statement here.
(parse)
149-155: LGTM!The change to include
SharepointGroupInputandDropboxGroupInputtypes in theOriginalGroupInputtype aligns with the PR objective of integrating Dropbox into the file storage API.
161-162: LGTM!The change to include
SharepointUserInputandDropboxUserInputtypes in theOriginalUserInputtype aligns with the PR objective of integrating Dropbox into the file storage API.
176-183: LGTM!The change to include
GoogleDriveFileOutputandDropboxFileOutputtypes in theOriginalFileOutputtype aligns with the PR objective of integrating Dropbox into the file storage API.Tools
Biome
[error] 182-183: Expected a statement but instead found '| GoogleDriveFileOutput
| DropboxFileOutput'.Expected a statement here.
(parse)
182-183: Skipping the static analysis hint.The parsing error flagged by the static analysis tool is a false positive because the code segment is a valid type definition.
Tools
Biome
[error] 182-183: Expected a statement but instead found '| GoogleDriveFileOutput
| DropboxFileOutput'.Expected a statement here.
(parse)
190-191: LGTM!The change to include
GoogleDriveFolderOutputandDropboxFolderOutputtypes in theOriginalFolderOutputtype aligns with the PR objective of integrating Dropbox into the file storage API.Tools
Biome
[error] 190-191: Expected a statement but instead found '| GoogleDriveFolderOutput
| DropboxFolderOutput'.Expected a statement here.
(parse)
190-191: Skipping the static analysis hint.The parsing error flagged by the static analysis tool is a false positive because the code segment is a valid type definition.
Tools
Biome
[error] 190-191: Expected a statement but instead found '| GoogleDriveFolderOutput
| DropboxFolderOutput'.Expected a statement here.
(parse)
206-212: LGTM!The change to include
SharepointGroupOutputandDropboxGroupOutputtypes in theOriginalGroupOutputtype aligns with the PR objective of integrating Dropbox into the file storage API.
218-219: LGTM!The change to include
SharepointUserOutputandDropboxUserOutputtypes in theOriginalUserOutputtype aligns with the PR objective of integrating Dropbox into the file storage API.
Line range hint
1-219: Overall assessment: The changes in this file are a crucial part of the Dropbox integration effort.The changes introduce Dropbox-related types for files, folders, groups, and users, which lay the foundation for handling Dropbox-specific data structures in the file storage API. The changes are well-implemented and align perfectly with the PR objective of integrating Dropbox into the file storage vertical.
Great work on this file! The Dropbox integration is shaping up nicely.
Tools
Biome
[error] 125-126: Expected a statement but instead found '| GoogleDriveFileInput
| DropboxFileInput'.Expected a statement here.
(parse)
[error] 133-134: Expected a statement but instead found '| GoogleDriveFolderInput
| DropboxFolderInput'.Expected a statement here.
(parse)
packages/api/scripts/init.sql (1)
555-558: The new columns for Dropbox, SharePoint, and OneDrive look good!The additions of
fs_dropbox,fs_sharepoint, andfs_onedrivecolumns to theconnector_setstable follow the existing schema design and naming conventions. Thebooleandata type is appropriate for storing the presence of these connectors.To verify if the new columns are being used correctly, run this script:
Verification successful
To get a more comprehensive view of how these new columns are being used across the codebase, let's search for references in non-SQL files. This will help us understand if the application logic has been updated to work with these new columns.
Let's first verify that our search command is working correctly and then try a broader search to catch any potential variations in how these new columns might be referenced.
The new columns align with existing Dropbox, SharePoint, and OneDrive integrations
The codebase shows extensive integration with Dropbox, SharePoint, and OneDrive across various components, including API services, data mappers, and connection services. While the new columns (
fs_dropbox,fs_sharepoint,fs_onedrive) are not directly referenced in non-SQL files, their addition to the database schema aligns with the existing support for these file storage services.Consider the following:
- Ensure that these new columns are utilized in the application logic if they're intended for a new feature or enhancement.
- If the columns are for internal tracking or future expansion, document their purpose for other developers.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new fs_dropbox, fs_sharepoint, fs_onedrive columns. # Test: Search for references to the new column names. Expect: SQL statements using these columns. rg --type sql $'fs_(dropbox|sharepoint|onedrive)'Length of output: 675
Script:
#!/bin/bash # Description: Search for references to the new columns in non-SQL files # Exclude SQL files and search for the new column names rg --type-not sql 'fs_(dropbox|sharepoint|onedrive)'Length of output: 54
Script:
#!/bin/bash # Description: Verify search functionality and perform a broader search # Test the search functionality with a known term (e.g., 'connector') echo "Testing search functionality:" rg --type-not sql 'connector' # Perform a broader search for potential references to the new columns echo -e "\nBroader search for new column references:" rg --type-not sql '(dropbox|sharepoint|onedrive)'Length of output: 50068
packages/shared/src/connectors/metadata.ts (3)
2735-2735: Looks good!The added
scopesproperty for the Dropbox connector configuration is appropriate and aligns with the PR objective. The included permissions cover the necessary access for file metadata, content, team data, and member information.
2738-2738: Good catch!Updating the
apiUrlto use a specific versioned endpoint (https://api.dropboxapi.com/2) is a positive change. It ensures the integration leverages a stable and feature-rich version of the Dropbox API.
2743-2743: Connector activated!Updating the
activeproperty totruefor the Dropbox connector is a crucial step in enabling the integration. This change aligns perfectly with the PR objective of integrating Dropbox functionality into the file storage vertical.
| ONEDRIVE = 'onedrive' | ||
| SHAREPOINT = 'sharepoint', |
There was a problem hiding this comment.
Add a missing comma after ONEDRIVE.
The static analysis tool has correctly flagged a missing comma after the ONEDRIVE entry on line 31. Each enum entry should be separated by a comma.
Apply this diff to fix the missing comma:
- ONEDRIVE = 'onedrive'
+ ONEDRIVE = 'onedrive',
SHAREPOINT = 'sharepoint',Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ONEDRIVE = 'onedrive' | |
| SHAREPOINT = 'sharepoint', | |
| ONEDRIVE = 'onedrive', | |
| SHAREPOINT = 'sharepoint', |
Tools
Biome
[error] 32-32: expected
,but instead foundSHAREPOINTRemove SHAREPOINT
(parse)
naelob
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
Add dropbox integration to filestorage vertical #536
Objects
closes #536
Summary by CodeRabbit
Release Notes
New Features
Enhancements
These updates enhance user flexibility and choice in managing files across different platforms.