Pluggy.ai bank sync #530
Pluggy.ai bank sync #530
Conversation
|
@coderabbitai review |
WalkthroughThe pull request introduces a new module for integrating the PluggyAI service into the application. The changes include creating a new Express.js application ( Suggested labels
✨ Finishing Touches
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 (
|
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
|
@coderabbitai full review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/app-pluggyai/app-pluggyai.js (4)
7-10: Consider more descriptive naming or a dedicated router for pluggyAI.
Using a simpleappwithexport { app as handlers }works, but a separate router or more descriptive naming can improve clarity and maintainability when scaling.
12-25: Review the use of POST method for '/status' endpoint.
Endpoints that only retrieve status are often exposed as GET for semantic clarity. Consider switching to GET unless you have a specific reason for POST.
66-88: Use safer pagination approach to avoid potential large response accumulation.
This while loop fetches all transactions, which might be extremely large for certain users. Consider a more controlled or streaming approach, or limiting the maximum pages fetched to prevent memory exhaustion.
221-223: Expand or renamegetDatefor time-sensitive use cases.
Currently,getDatestrips time toYYYY-MM-DD. If time zones or partial dates become relevant, consider returning a full ISO string or storing time detail in a separate field.src/app-pluggyai/pluggyai-service.js (4)
7-19: Consider concurrency and caching strategy forpluggyApiKey.
pluggyApiKeyis stored as a global variable; concurrent requests could result in race conditions if the token expires mid-request. Consider storing the token on a per-request basis or leveraging a caching mechanism with proper locking.
25-79: Handle JSON parsing and success checks robustly when generating API key.
While you handle403and401statuses, other status codes or network anomalies could still cause unexpected results. Consider uniform status code handling and error messaging to simplify debugging.
97-105: Move token persistence behind a stable interface.
Directly callingsecretsService.setfrom within the token logic can create coupling between this layer and the secrets layer. Consider an abstraction that can handle all token-related operations consistently.
185-228: Examine large date range queries ingetTransactionsByAccountId.
Constructing a giant query combining multiple dates and pages may lead to potential timeouts or performance bottlenecks. Offer a robust approach, e.g., date-chunking or cursors, if users often retrieve large date ranges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/530.mdis excluded by!**/*.md
📒 Files selected for processing (4)
src/app-pluggyai/app-pluggyai.js(1 hunks)src/app-pluggyai/pluggyai-service.js(1 hunks)src/app.js(2 hunks)src/services/secrets-service.js(1 hunks)
🔇 Additional comments (5)
src/app-pluggyai/app-pluggyai.js (2)
185-189: Double-check sign logic for amounts with credit accounts.
For'BANK'vs.'CREDIT'accounts, the code reuses the'BANK'condition for the sign. Validate that you’ve accurately handled all relevant account types if there are more possibilities (e.g.,'INVESTMENT').
27-59: Validate existence of item IDs before processing.
The code splits and maps the item IDs, but there is no check to ensure the secret is defined. A missing or emptypluggyai_itemIdssecret could cause runtime errors.+ if (!itemIds || itemIds.length === 0) { + return res.status(400).send({ + status: 'error', + message: 'No itemIds found in configuration.' + }); + }✅ Verification successful
Validate existence and validity of item IDs before processing
The validation is necessary as other routes in the codebase explicitly check for null/forbidden secrets. Update the code to match existing error handling patterns:+ const itemIdsSecret = secretsService.get(SecretName.pluggyai_itemIds); + if (!itemIdsSecret || itemIdsSecret === 'Forbidden') { + return res.status(400).send({ + status: 'error', + reason: 'missing-item-ids-configuration' + }); + } - const itemIds = secretsService + const itemIds = itemIdsSecret .split(',') .map((item) => item.trim()); + if (itemIds.length === 0) { + return res.status(400).send({ + status: 'error', + reason: 'empty-item-ids-configuration' + }); + }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find secretsService implementation ast-grep --pattern 'class $_ { $$$ get($name) { $$$ } $$$ }' # Look for similar patterns of secret usage rg "secretsService\.get\(" -A 2 -B 2 # Check error handling patterns in routes rg "res\.status\(.*\)\.send\(" -A 2Length of output: 11963
src/services/secrets-service.js (1)
14-17: Ensure secret usage follows security best practices.
Adding these secrets expands the surface area of sensitive data. Make sure you are encrypting secrets at rest, limiting read/write access, and rotating credentials as needed.src/app.js (2)
16-16: LGTM! Import follows established patterns.The import statement follows the consistent pattern used for other bank sync modules, maintaining code organization standards.
53-53: LGTM! Route registration is secure and consistent.The
/pluggyairoute registration:
- Follows the established pattern used by other bank sync routes
- Inherits security measures:
- Rate limiting (500 requests/minute)
- CORS protection
- Request size limits
- Proxy trust configuration
Let's verify the security implementation in the Pluggy.ai handlers:
| if (transactionDate < startDate) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure request body date fields are reliably parsed before comparison.
Currently, you compare transactionDate < startDate directly, but startDate might be a string from req.body. Confirm it’s converted to a Date object to avoid unexpected behaviors.
- if (transactionDate < startDate) {
+ const parsedStart = new Date(startDate);
+ if (transactionDate < parsedStart) {
continue;
}📝 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 (transactionDate < startDate) { | |
| continue; | |
| } | |
| const parsedStart = new Date(startDate); | |
| if (transactionDate < parsedStart) { | |
| continue; | |
| } | |
|
Did you see that there is an official node sdk for pluggy? https://www.npmjs.com/package/pluggy-sdk |
I didn't. I found the API so simple that I implemented myself. Do you prefer that I use the official sdk? |
Another opinion would be good but IMO using the official SDK gives us a level of security with sanitisation etc. |
Pluggy.ai bank sync for Brazilian Banks
Needs this actualbudget/actual#4049