Incremental sync for Attio provider #701
Conversation
|
Qovery can create a Preview Environment for this PR.
This comment has been generated from Qovery AI 🤖.
|
|
WalkthroughWalkthroughThe pull request introduces a new database table for tracking synchronization data related to vertical objects and enhances the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (4)
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 (
|
mit-27
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (9)
- packages/api/scripts/init.sql (1 hunks)
- packages/api/src/crm/company/services/attio/index.ts (2 hunks)
- packages/api/src/crm/company/services/attio/types.ts (1 hunks)
- packages/api/src/crm/contact/services/attio/index.ts (2 hunks)
- packages/api/src/crm/contact/services/attio/types.ts (1 hunks)
- packages/api/src/crm/deal/services/attio/index.ts (2 hunks)
- packages/api/src/crm/deal/services/attio/types.ts (1 hunks)
- packages/api/src/crm/task/services/attio/index.ts (2 hunks)
- packages/api/src/crm/task/services/attio/types.ts (1 hunks)
Additional context used
Biome
packages/api/src/crm/task/services/attio/index.ts
[error] 105-105: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 104-104: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
packages/api/src/crm/contact/services/attio/index.ts
[error] 88-88: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 87-87: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
packages/api/src/crm/deal/services/attio/index.ts
[error] 88-88: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 87-87: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
packages/api/src/crm/company/services/attio/index.ts
[error] 89-89: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 88-88: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
Additional comments not posted (18)
packages/api/src/crm/task/services/attio/types.ts (1)
49-51: LGTM!The new
paginationTypeprovides a structured and type-safe way to represent pagination parameters. The type name and structure are clear and appropriate for the intended purpose.To ensure the new type is being used correctly and consistently, please run the following script:
Verification successful
Consistent usage across modules, but consider centralizing the type definition
The
paginationTypeis being used consistently across different Attio service modules (deal, task, company, contact). The new implementation in the task module aligns well with existing usage in other modules. However, there's an opportunity for improvement:
- The type is currently defined multiple times in separate files, which could lead to maintenance issues.
- Consider centralizing the
paginationTypedefinition in a common types file for the Attio service to reduce duplication and improve maintainability.Example locations where
paginationTypeis defined and used:
- Defined in: packages/api/src/crm/task/services/attio/types.ts
- Used in: packages/api/src/crm/task/services/attio/index.ts
The usage is correct and consistent, but centralizing the type definition would be beneficial for long-term maintenance.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `paginationType` in the codebase. # Test: Search for the type usage. Expect: Consistent usage across the codebase. rg --type typescript -A 5 $'paginationType'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage of the `paginationType` in the codebase. # Find TypeScript files and search for paginationType fd -e ts -e tsx | xargs grep -n "paginationType" -A 5 # Search for export of paginationType fd -e ts -e tsx | xargs grep -n "export.*paginationType" # Search for import of paginationType fd -e ts -e tsx | xargs grep -n "import.*paginationType"Length of output: 6420
packages/api/src/crm/deal/services/attio/types.ts (1)
107-109: LGTM!The new
paginationTypedefinition is a clear and concise way to represent pagination-related data. Theoffsetproperty of typenumberis appropriate for handling paginated results.This addition expands the type system without affecting existing functionality, allowing for more efficient data retrieval or management in parts of the application that require pagination.
packages/api/src/crm/company/services/attio/types.ts (1)
131-133: LGTM!The introduction of the
paginationTypetype alias is a good addition for handling pagination-related parameters. It provides a clear and structured way to represent theoffsetvalue, which is commonly used in pagination scenarios.By encapsulating the pagination parameter in a separate type, it promotes code consistency and makes it explicit what the expected input or output format is when dealing with paginated data.
The type alias follows the naming convention of using a
Typesuffix, which aligns with the TypeScript best practices.packages/api/src/crm/contact/services/attio/types.ts (1)
131-133: LGTM!The new
paginationTypeis a simple and clear type definition for handling pagination. Theoffsetproperty of typenumberis appropriate for this use case. Exporting the type is consistent with the other types in the file.packages/api/src/crm/task/services/attio/index.ts (4)
11-12: LGTM!The imports are correctly added and follow the existing import style. They are likely used in the updated
syncmethod.
95-194: Robust synchronization mechanism for tasks!The updated
syncmethod introduces a more robust mechanism for synchronizing tasks from the Attio API, supporting both initial and incremental sync processes. The key improvements include:
- Handling pagination to manage larger datasets efficiently by fetching tasks in batches of 500.
- Checking for existing pagination tracking information in the database to determine whether to perform an initial or incremental sync.
- Storing and retrieving pagination tracking information in the database to maintain the sync state across operations.
- Implementing error handling and logging mechanisms to capture and report any issues during the sync process.
The code follows a logical flow, adheres to the existing coding style and practices, and effectively addresses the synchronization requirements.
Tools
Biome
[error] 105-105: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 104-104: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
197-199: LGTM!The return statement is consistent with the existing code and follows the expected format. The success message and status code are appropriate for the operation.
95-199: Overall assessment: Significant improvements to the task synchronization process!The changes introduced in this pull request bring substantial enhancements to the task synchronization process for the Attio API. The key highlights include:
- A robust mechanism for handling both initial and incremental sync processes, ensuring efficient and reliable synchronization of tasks.
- Effective pagination handling to manage larger datasets, optimizing data retrieval and reducing unnecessary API calls.
- Integration with the database for storing and retrieving pagination tracking information, maintaining the sync state across operations and providing a seamless sync experience.
- Implementation of error handling and logging mechanisms, improving visibility into the sync process and facilitating issue identification and resolution.
The code follows a logical flow, adheres to the existing coding style and practices, and effectively addresses the synchronization requirements. The suggested improvements based on static analysis hints further enhance the code quality and maintainability.
Overall, these changes have a positive impact on the codebase, significantly improving the efficiency, reliability, and maintainability of the task synchronization process for the Attio API.
Tools
Biome
[error] 105-105: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 104-104: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
packages/api/src/crm/contact/services/attio/index.ts (4)
78-85: LGTM!The code correctly retrieves pagination tracking information from the database to determine the synchronization strategy.
131-172: Incremental sync implementation is solid!The code correctly performs an incremental synchronization by starting from the last known offset stored in the pagination data. It follows the same looping mechanism as the initial sync to fetch new records efficiently. Error handling is in place to log any issues during the sync process.
174-201: Pagination tracking record management is implemented correctly!The code properly updates or creates pagination tracking records in the database after the sync operation. It accurately stores the current
offsetfor future incremental syncs, ensuring that the synchronization state is maintained across runs.
204-206: Response handling is correct!The code properly returns the retrieved contact records (
respData) along with a success message and status code, which is the expected behavior for thesyncmethod.packages/api/src/crm/deal/services/attio/index.ts (3)
11-11: LGTM!The imports are necessary for the new functionality introduced in the
syncmethod.
14-14: LGTM!The import is necessary for generating unique IDs for the
vertical_objects_sync_track_datatable.
77-201: Excellent work on the sync method implementation!The code is well-structured, follows a logical flow, and efficiently handles both initial and incremental synchronization of deal data from the Attio API. The use of pagination tracking data ensures that the service can resume synchronization from the last known offset, reducing unnecessary API calls and improving performance.
The error handling and logging mechanisms are also implemented appropriately, which will help in identifying and resolving issues if they occur.
Overall, the changes made to the
syncmethod are a significant improvement and enhance the data synchronization capabilities of theAttioService.Tools
Biome
[error] 88-88: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 87-87: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
packages/api/src/crm/company/services/attio/index.ts (2)
12-12: LGTM!The imported types are necessary for the changes made in the
syncmethod to handle pagination and track synchronization data.
78-202: Great work on implementing pagination and maintaining sync state!The updated
syncmethod effectively handles large datasets by implementing pagination and maintaining state across sync operations. The logic for initial and incremental sync is well-structured and follows a similar pattern, making it easier to understand and maintain.The error handling using
try-catchblocks ensures that any issues encountered during the sync process are logged and handled gracefully. Additionally, updating or creating the pagination tracking data in the database ensures that the offset is correctly recorded for future sync operations.Overall, the changes enhance the functionality and reliability of the
AttioServicewhen syncing company records.Tools
Biome
[error] 89-89: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 88-88: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
packages/api/scripts/init.sql (1)
2903-2915: The newvertical_objects_sync_track_datatable looks good!The table structure appears well-suited to track sync state for vertical objects across providers. The fields capture the key dimensions and the JSON
datafield allows flexibility for provider-specific state.Suggestions for future enhancements:
- Consider defining enums or lookup tables for the
objectandpagination_typefields to standardize the allowed values, but not critical for the initial implementation.- Add indexes on foreign keys and commonly queried fields if needed to optimize query performance.
Overall, the table definition looks good to merge as-is. Nice work!
| let respData: AttioTaskOutput[] = []; | ||
| let initialOffset: number = 0; |
There was a problem hiding this comment.
Minor improvements suggested by static analysis.
Consider the following suggestions to improve code readability and adhere to best practices:
- Remove the type annotation for
respDataas it is trivially inferred from its initialization. - Use
constinstead ofletforrespDatasince it is never reassigned.
Apply this diff to implement the suggestions:
-let respData: AttioTaskOutput[] = [];
+const respData = [];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.
| let respData: AttioTaskOutput[] = []; | |
| let initialOffset: number = 0; | |
| const respData = []; | |
| let initialOffset: number = 0; |
Tools
Biome
[error] 105-105: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 104-104: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| let respData: AttioContactOutput[] = []; | ||
| let initialOffset: number = 0; | ||
|
|
||
| if (!paginationTrackInfo) { | ||
| // Intial sync | ||
| try { | ||
| while (true) { | ||
| const resp = await axios.post( | ||
| `${connection.account_url}/v2/objects/people/records/query`, | ||
| { | ||
| "sorts": [ | ||
| { | ||
| "attribute": "created_at", | ||
| "direction": "asc" | ||
| } | ||
| ], | ||
| "offset": initialOffset, | ||
| "limit": 500 | ||
| }, | ||
| { | ||
| headers: { | ||
| accept: 'application/json', | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${this.cryptoService.decrypt( | ||
| connection.access_token, | ||
| )}`, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
|
|
||
| respData.push(...resp.data.data); | ||
| initialOffset = initialOffset + resp.data.data.length; | ||
|
|
||
| if (resp.data.data.length < 500) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| catch (error) { | ||
| this.logger.log(`Error in initial sync ${error.message} and last offset is ${initialOffset}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
Initial sync implementation looks good!
The code correctly performs an initial synchronization by repeatedly querying the Attio API with pagination until all records are retrieved. Error handling is in place to log any issues during the sync process.
Consider using const instead of let for respData as it is never reassigned:
-let respData: AttioContactOutput[] = [];
+const respData: AttioContactOutput[] = [];This change adheres to the best practice of using const for variables that are not reassigned.
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.
| let respData: AttioContactOutput[] = []; | |
| let initialOffset: number = 0; | |
| if (!paginationTrackInfo) { | |
| // Intial sync | |
| try { | |
| while (true) { | |
| const resp = await axios.post( | |
| `${connection.account_url}/v2/objects/people/records/query`, | |
| { | |
| "sorts": [ | |
| { | |
| "attribute": "created_at", | |
| "direction": "asc" | |
| } | |
| ], | |
| "offset": initialOffset, | |
| "limit": 500 | |
| }, | |
| { | |
| headers: { | |
| accept: 'application/json', | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${this.cryptoService.decrypt( | |
| connection.access_token, | |
| )}`, | |
| }, | |
| }, | |
| ); | |
| respData.push(...resp.data.data); | |
| initialOffset = initialOffset + resp.data.data.length; | |
| if (resp.data.data.length < 500) { | |
| break; | |
| } | |
| } | |
| } | |
| catch (error) { | |
| this.logger.log(`Error in initial sync ${error.message} and last offset is ${initialOffset}`); | |
| } | |
| const respData: AttioContactOutput[] = []; | |
| let initialOffset: number = 0; | |
| if (!paginationTrackInfo) { | |
| // Intial sync | |
| try { | |
| while (true) { | |
| const resp = await axios.post( | |
| `${connection.account_url}/v2/objects/people/records/query`, | |
| { | |
| "sorts": [ | |
| { | |
| "attribute": "created_at", | |
| "direction": "asc" | |
| } | |
| ], | |
| "offset": initialOffset, | |
| "limit": 500 | |
| }, | |
| { | |
| headers: { | |
| accept: 'application/json', | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${this.cryptoService.decrypt( | |
| connection.access_token, | |
| )}`, | |
| }, | |
| }, | |
| ); | |
| respData.push(...resp.data.data); | |
| initialOffset = initialOffset + resp.data.data.length; | |
| if (resp.data.data.length < 500) { | |
| break; | |
| } | |
| } | |
| } | |
| catch (error) { | |
| this.logger.log(`Error in initial sync ${error.message} and last offset is ${initialOffset}`); | |
| } |
Tools
Biome
[error] 88-88: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 87-87: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| ); | ||
| }); | ||
|
|
||
| let respData: AttioDealOutput[] = []; |
There was a problem hiding this comment.
Consider removing the type annotation and using const.
The type annotation AttioDealOutput[] is not necessary as it can be inferred from the initialization. Additionally, since the respData variable is never reassigned, it's a good practice to use const instead of let.
Apply this diff to make the suggested changes:
-let respData: AttioDealOutput[] = [];
+const respData = [];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.
| let respData: AttioDealOutput[] = []; | |
| const respData = []; |
Tools
Biome
[error] 87-87: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| }); | ||
|
|
||
| let respData: AttioDealOutput[] = []; | ||
| let initialOffset: number = 0; |
There was a problem hiding this comment.
Consider removing the type annotation.
The type annotation number is not necessary as it can be inferred from the initialization.
Apply this diff to make the suggested change:
-let initialOffset: number = 0;
+let initialOffset = 0;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.
| let initialOffset: number = 0; | |
| let initialOffset = 0; |
Tools
Biome
[error] 88-88: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
| ); | ||
| }); | ||
|
|
||
| let respData: AttioCompanyOutput[] = []; |
There was a problem hiding this comment.
Use const instead of let.
The variable respData is only assigned once during its initialization and is not reassigned throughout the code. Using const instead of let would ensure that respData cannot be accidentally reassigned, improving code quality and readability.
Apply this diff to use const:
-let respData = [];
+const respData = [];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.
| let respData: AttioCompanyOutput[] = []; | |
| const respData: AttioCompanyOutput[] = []; |
Tools
Biome
[error] 88-88: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
Consider removing the type annotation.
The type annotation AttioCompanyOutput[] is not necessary as TypeScript can infer the type of respData based on its initialization to an empty array []. Removing the type annotation would not affect the functionality of the code and would improve readability.
Apply this diff to remove the type annotation:
-let respData: AttioCompanyOutput[] = [];
+let respData = [];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.
| let respData: AttioCompanyOutput[] = []; | |
| let respData = []; |
Tools
Biome
[error] 88-88: This let declares a variable that is only assigned once.
'respData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| }); | ||
|
|
||
| let respData: AttioCompanyOutput[] = []; | ||
| let initialOffset: number = 0; |
There was a problem hiding this comment.
Consider removing the type annotation.
The type annotation number is not necessary as TypeScript can infer the type of initialOffset based on its initialization to 0. Removing the type annotation would not affect the functionality of the code and would improve readability.
Apply this diff to remove the type annotation:
-let initialOffset: number = 0;
+let initialOffset = 0;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.
| let initialOffset: number = 0; | |
| let initialOffset = 0; |
Tools
Biome
[error] 89-89: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
mit-27
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation