exporter: external api publisher #636
Conversation
![flatfile-nullify[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
Nullify Code Vulnerabilities1 findings found in this pull request
You can find a list of all findings here |
bangarang
changed the title
carlbrugger
force-pushed
the
feat/ExternalAPIPublisher
branch
from
dc31ba5 to
49f395e
Compare
| const response = await fetch(apiEndpoint, { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Bearer ${authToken}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(data), | ||
| }) |
There was a problem hiding this comment.
Nullify Code Language: TypeScript 🟡 HIGH Severity CWE-918
Rules lgpl javascript ssrf rule node ssrf
This application allows user-controlled URLs to be passed directly to HTTP client libraries. This can result in Server-Side Request Forgery (SSRF). SSRF refers to an attack where the attacker can abuse functionality on the server to force it to make requests to other internal systems within your infrastructure that are not directly exposed to the internet. This allows the attacker to access internal resources they do not have direct access to.
Some risks of SSRF are:
- Access and manipulation of internal databases, APIs, or administrative panels - Ability to scan internal network architecture and services - Can be used to pivot attacks into the internal network - Circumvent network segregation and firewall rules
To avoid this, try using hardcoded HTTP request calls or a whitelisting object to check whether the user input is trying to access allowed resources or not.
Here is an example:var whitelist = [ "https://example.com", "https://example.com/sample" ] app.get('/ssrf/node-ssrf/axios/safe/3', function (req, res) { if(whitelist.includes(req.query.url)){ axios.get(url, {}) .then(function (response) { console.log(response); }) .catch(function (response) { console.log(response); }) } });For more information on SSRF see OWASP: https://cheatsheetseries.owasp.org/cheatsheets/Server_Side_Request_Forgery_Prevention_Cheat_Sheet.html
⚡ Here's how you might fix this potential vulnerability
In the initial code, the 'apiEndpoint' parameter is passed directly to the 'fetch' function without any validation, which could lead to Server Side Request Forgery (SSRF) if an attacker can control the value of 'apiEndpoint'. The modified code mitigates this vulnerability by validating the 'apiEndpoint' parameter using the 'URL' class before passing it to the 'fetch' function. It checks the protocol and hostname of the URL against a whitelist of allowed values, and throws an error if the URL is not allowed.
autoFixesExperimental
Add URL validation before making the fetch request
| const response = await fetch(apiEndpoint, { | |
| method: 'POST', | |
| headers: { | |
| Authorization: `Bearer ${authToken}`, | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify(data), | |
| }) | |
| const url = new URL(apiEndpoint); | |
| if (url.protocol !== 'https:' || !['api.example.com', 'api2.example.com'].includes(url.hostname)) { | |
| throw new Error('Invalid API endpoint'); | |
| } | |
| const response = await fetch(apiEndpoint, { |
poweredByNullify
Reply with /nullify to interact with me like another developer
(you will need to refresh the page for updates)
WalkthroughThis pull request introduces the Changes
🪧 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: 4
🧹 Outside diff range and nitpick comments (4)
export/external-api/jest.config.js (1)
14-14: Consider removingforceExit: true.While
forceExit: trueensures Jest exits after tests complete, it might mask issues with asynchronous operations or resources not being properly cleaned up. Consider removing this option and addressing any hanging processes directly in your test teardown.export/external-api/README.MD (1)
22-43: Adjust heading levels for consistency.The Parameters section provides clear and concise descriptions for all required parameters. However, the heading levels should be adjusted for consistency with markdown best practices.
Apply this change to fix the heading levels:
-## Parameters +## Parameters -#### `job` - `string` - (required) +### `job` - `string` - (required) The type of job to create. -#### `apiEndpoint` - `string` - (required) +### `apiEndpoint` - `string` - (required) The URL of the external API endpoint to send data to. -#### `authToken` - `string` - (required) +### `authToken` - `string` - (required) The authentication token for the external API. -#### `dataMapping` - `object` - (required) +### `dataMapping` - `object` - (required) A mapping of Flatfile field names to external API field names. -#### `batchSize` - `number` - (required) +### `batchSize` - `number` - (required) The number of records to process in each batch. -#### `maxRetries` - `number` - (required) +### `maxRetries` - `number` - (required) The maximum number of retry attempts for failed API calls. -#### `retryDelay` - `number` - (required) +### `retryDelay` - `number` - (required) The delay (in milliseconds) between retry attempts.🧰 Tools
🪛 Markdownlint
24-24: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
export/external-api/src/external.api.plugin.ts (2)
84-88: Provide more informative error messagesWhen an error occurs during export, the thrown error message is generic:
'An error occurred during export.'Consider including the original error message to aid in debugging.Update the throw statement to include the original error message:
console.error('Error during export:', (error as Error).message) - throw new Error('An error occurred during export.') + throw new Error(`An error occurred during export: ${(error as Error).message}`)
70-70: Use a logging library instead ofconsole.errorUsing
console.errorfor logging errors may not align with the application's logging practices. Consider using a dedicated logging library or the application's standard logging mechanism to handle error logging.Replace
console.errorcalls with the appropriate logger methods.Also applies to: 85-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
export/external-api/package.jsonis excluded by!**/*.jsonpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (6)
- export/external-api/README.MD (1 hunks)
- export/external-api/jest.config.js (1 hunks)
- export/external-api/rollup.config.mjs (1 hunks)
- export/external-api/src/external.api.plugin.ts (1 hunks)
- export/external-api/src/external.api.utils.ts (1 hunks)
- export/external-api/src/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- export/external-api/rollup.config.mjs
- export/external-api/src/index.ts
🧰 Additional context used
🪛 Markdownlint
export/external-api/README.MD
24-24: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (8)
export/external-api/jest.config.js (2)
1-16: LGTM! Comprehensive Jest configuration.The Jest configuration is well-structured and includes essential settings for a Node.js environment with TypeScript support. It covers important aspects such as environment setup, TypeScript transformation, and test timeouts.
15-15: Caution:passWithNoTests: truemight hide issues.Setting
passWithNoTests: trueallows the test suite to pass even if no tests are found. This could potentially hide issues where tests are accidentally skipped or not detected. Consider setting this tofalseto ensure that tests are always run as expected.To ensure that tests are present and being detected, you can run the following command:
✅ Verification successful
Tests are present and being detected as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of test files in the project # Test: Search for test files. Expect: At least one test file should be found. fd -e test.ts -e test.js -e spec.ts -e spec.jsLength of output: 2015
export/external-api/README.MD (3)
1-9: LGTM: Infocard section is informative and well-structured.The infocard provides a clear and concise overview of the plugin's purpose and functionality. The inclusion of the event type "job:ready" is particularly helpful for integration purposes.
11-20: LGTM: Features section is comprehensive and well-presented.The list of features provides a clear overview of the plugin's capabilities, covering crucial aspects such as data mapping, batch processing, error handling, and user notifications. This section effectively communicates the value proposition of the plugin to potential users.
1-75: Overall, excellent documentation with minor improvements needed.The README provides comprehensive and well-structured documentation for the
@flatfile/plugin-export-external-apiplugin. It effectively communicates the plugin's purpose, features, configuration, and usage. To further enhance the documentation:
- Address the heading level inconsistencies in the Parameters section.
- Ensure consistency in the plugin/package name across all sections.
- Update the import statement in the Example Usage section to match the correct package name and export.
These minor adjustments will improve the overall accuracy and consistency of the documentation.
🧰 Tools
🪛 Markdownlint
24-24: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
export/external-api/src/external.api.utils.ts (3)
5-16: LGTM!The
processRecordfunction accurately processes records based on the provided data mapping.
23-30: Potential SSRF vulnerability in 'exportToExternalAPI' function.The issue raised in the previous review comment regarding SSRF risks is still applicable.
37-52: LGTM!The
retryOperationfunction effectively implements retry logic with a configurable delay.
export/external-api/README.MD
Outdated
| ## Installation | ||
|
|
||
| To install the plugin, use npm: | ||
|
|
||
| ```bash | ||
| npm install @flatfile/plugin-external-api-export | ||
| ``` |
There was a problem hiding this comment.
Update the npm package name in the installation command.
The installation instructions are clear, but there's a discrepancy between the plugin name mentioned earlier and the npm package name in the installation command.
Update the npm package name to match the plugin name:
-npm install @flatfile/plugin-external-api-export
+npm install @flatfile/plugin-export-external-api📝 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.
| ## Installation | |
| To install the plugin, use npm: | |
| ```bash | |
| npm install @flatfile/plugin-external-api-export | |
| ``` | |
| ## Installation | |
| To install the plugin, use npm: | |
| ```bash | |
| npm install @flatfile/plugin-export-external-api | |
| ``` |
export/external-api/README.MD
Outdated
| ## Example Usage | ||
|
|
||
| ```javascript | ||
| import externalApiExportPlugin from '@flatfile/plugin-external-api-export'; | ||
|
|
||
| export default function (listener) { | ||
| listener.use( | ||
| externalApiExportPlugin({ | ||
| apiEndpoint: 'https://api.example.com/import', | ||
| authToken: 'your-api-token', | ||
| dataMapping: { | ||
| 'First Name': 'firstName', | ||
| 'Last Name': 'lastName', | ||
| 'Email': 'emailAddress' | ||
| }, | ||
| batchSize: 100, | ||
| maxRetries: 3, | ||
| retryDelay: 1000 | ||
| }) | ||
| ); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Update import statement and function name in the example usage.
The example usage provides a clear demonstration of how to configure and use the plugin. However, there are discrepancies in the import statement that need to be addressed.
Apply the following changes to correct the import statement and function name:
-import externalApiExportPlugin from '@flatfile/plugin-external-api-export';
+import { externalApiExportPlugin } from '@flatfile/plugin-export-external-api';Also, ensure that the function name externalApiExportPlugin is correct and matches the actual export from the plugin.
📝 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.
| ## Example Usage | |
| ```javascript | |
| import externalApiExportPlugin from '@flatfile/plugin-external-api-export'; | |
| export default function (listener) { | |
| listener.use( | |
| externalApiExportPlugin({ | |
| apiEndpoint: 'https://api.example.com/import', | |
| authToken: 'your-api-token', | |
| dataMapping: { | |
| 'First Name': 'firstName', | |
| 'Last Name': 'lastName', | |
| 'Email': 'emailAddress' | |
| }, | |
| batchSize: 100, | |
| maxRetries: 3, | |
| retryDelay: 1000 | |
| }) | |
| ); | |
| } | |
| ``` | |
| ## Example Usage | |
| ```javascript | |
| import { externalApiExportPlugin } from '@flatfile/plugin-export-external-api'; | |
| export default function (listener) { | |
| listener.use( | |
| externalApiExportPlugin({ | |
| apiEndpoint: 'https://api.example.com/import', | |
| authToken: 'your-api-token', | |
| dataMapping: { | |
| 'First Name': 'firstName', | |
| 'Last Name': 'lastName', | |
| 'Email': 'emailAddress' | |
| }, | |
| batchSize: 100, | |
| maxRetries: 3, | |
| retryDelay: 1000 | |
| }) | |
| ); | |
| } | |
| ``` |
| await retryOperation( | ||
| () => | ||
| exportToExternalAPI( | ||
| processedBatch, | ||
| config.apiEndpoint, | ||
| config.authToken | ||
| ), | ||
| config.maxRetries, | ||
| config.retryDelay | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement exponential backoff for retries
The current retry mechanism uses a constant delay defined by config.retryDelay. Consider implementing an exponential backoff strategy to improve resilience against transient failures when exporting to the external API.
| while (pageNumber <= batchCount) { | ||
| const records = await Simplified.getAllRecords(sheetId, { | ||
| pageSize: config.batchSize, | ||
| pageNumber, | ||
| }) | ||
|
|
||
| const processedBatch = records.map((record) => | ||
| processRecord(record, config.dataMapping) | ||
| ) | ||
|
|
||
| try { | ||
| await retryOperation( | ||
| () => | ||
| exportToExternalAPI( | ||
| processedBatch, | ||
| config.apiEndpoint, | ||
| config.authToken | ||
| ), | ||
| config.maxRetries, | ||
| config.retryDelay | ||
| ) | ||
| totalExported += processedBatch.length | ||
| successfulBatches++ | ||
| } catch (error) { | ||
| console.error('Failed to export batch after retries:', error) | ||
| failedRecords += processedBatch.length | ||
| } | ||
|
|
||
| const progress = (successfulBatches / batchCount) * 100 | ||
| await tick( | ||
| progress, | ||
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix missing pageNumber increment to prevent infinite loop
The pageNumber variable is not incremented within the while loop, which will cause an infinite loop because pageNumber will always be less than or equal to batchCount. Increment pageNumber at the end of each loop iteration to ensure the loop progresses correctly.
Apply this diff to fix the issue:
while (pageNumber <= batchCount) {
const records = await Simplified.getAllRecords(sheetId, {
pageSize: config.batchSize,
pageNumber,
})
// ... processing code ...
await tick(
progress,
`Exported ${totalExported} records. Failed to export ${failedRecords} records.`
)
+ pageNumber++
}📝 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.
| while (pageNumber <= batchCount) { | |
| const records = await Simplified.getAllRecords(sheetId, { | |
| pageSize: config.batchSize, | |
| pageNumber, | |
| }) | |
| const processedBatch = records.map((record) => | |
| processRecord(record, config.dataMapping) | |
| ) | |
| try { | |
| await retryOperation( | |
| () => | |
| exportToExternalAPI( | |
| processedBatch, | |
| config.apiEndpoint, | |
| config.authToken | |
| ), | |
| config.maxRetries, | |
| config.retryDelay | |
| ) | |
| totalExported += processedBatch.length | |
| successfulBatches++ | |
| } catch (error) { | |
| console.error('Failed to export batch after retries:', error) | |
| failedRecords += processedBatch.length | |
| } | |
| const progress = (successfulBatches / batchCount) * 100 | |
| await tick( | |
| progress, | |
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | |
| ) | |
| } | |
| while (pageNumber <= batchCount) { | |
| const records = await Simplified.getAllRecords(sheetId, { | |
| pageSize: config.batchSize, | |
| pageNumber, | |
| }) | |
| const processedBatch = records.map((record) => | |
| processRecord(record, config.dataMapping) | |
| ) | |
| try { | |
| await retryOperation( | |
| () => | |
| exportToExternalAPI( | |
| processedBatch, | |
| config.apiEndpoint, | |
| config.authToken | |
| ), | |
| config.maxRetries, | |
| config.retryDelay | |
| ) | |
| totalExported += processedBatch.length | |
| successfulBatches++ | |
| } catch (error) { | |
| console.error('Failed to export batch after retries:', error) | |
| failedRecords += processedBatch.length | |
| } | |
| const progress = (successfulBatches / batchCount) * 100 | |
| await tick( | |
| progress, | |
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | |
| ) | |
| pageNumber++ | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
export/external-api/README.MD (2)
21-40: Adjust heading levels for parametersThe parameters section is well-documented, but the heading levels are inconsistent with the document structure. To improve readability and adhere to markdown best practices, consider changing the parameter headings from h4 (####) to h3 (###).
Apply this change to all parameter headings. For example:
-#### `job` - `string` - (required) +### `job` - `string` - (required)🧰 Tools
🪛 Markdownlint
23-23: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
1-66: Great job on the comprehensive READMEThe README for @flatfile/plugin-export-external-api is well-structured and provides all necessary information for users to understand, install, and use the plugin. It effectively covers the plugin's purpose, features, parameters, installation process, and usage example. Previous issues have been addressed, resulting in a high-quality documentation file.
One minor suggestion for improvement would be to consider adding a section on error handling or troubleshooting, which could be helpful for users implementing the plugin.
Consider adding a brief section on error handling or troubleshooting tips to further enhance the README's usefulness.
🧰 Tools
🪛 Markdownlint
23-23: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .changeset/short-papayas-share.md (1 hunks)
- export/external-api/README.MD (1 hunks)
- export/external-api/src/external.api.plugin.ts (1 hunks)
- export/external-api/src/external.api.utils.ts (1 hunks)
- export/external-api/src/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/short-papayas-share.md
🚧 Files skipped from review as they are similar to previous changes (2)
- export/external-api/src/external.api.utils.ts
- export/external-api/src/index.ts
🧰 Additional context used
🪛 Markdownlint
export/external-api/README.MD
23-23: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (6)
export/external-api/README.MD (3)
1-20: LGTM: Clear and informative introductionThe plugin introduction and features section provide a comprehensive overview of the @flatfile/plugin-export-external-api. The description effectively communicates the plugin's purpose and key functionalities.
41-49: LGTM: Installation instructions are correctThe installation section provides clear instructions for installing the plugin. The npm package name has been correctly updated to
@flatfile/plugin-export-external-api, addressing the previous review comment.
50-66: LGTM: Example usage is correct and comprehensiveThe example usage section provides a clear and accurate demonstration of how to use the plugin. The import statement and function name have been corrected, addressing the previous review comment. All required parameters are included in the example, making it a helpful reference for users.
export/external-api/src/external.api.plugin.ts (3)
1-5: Imports look good and are relevant to the plugin's functionality.The necessary dependencies from @flatfile libraries and local utility file are correctly imported.
7-14: PluginConfig interface is well-defined and comprehensive.The interface includes all necessary configuration options with proper type definitions.
16-84: Overall structure and implementation of the plugin function is well-designedThe
exportToExternalAPIPluginfunction is well-structured and implements the necessary logic for batch processing, error handling, and progress reporting. It effectively utilizes the jobHandler and other utility functions to manage the export process.Some notable strengths:
- Clear separation of concerns with the use of jobHandler.
- Proper error handling and logging.
- Effective use of batch processing to handle large datasets.
- Progress reporting using the tick function.
Consider addressing the previously mentioned issues (pageNumber increment and retry logic) to further improve the robustness of the plugin.
| await retryOperation( | ||
| () => | ||
| exportToExternalAPI( | ||
| records, | ||
| config.apiEndpoint, | ||
| config.authToken | ||
| ), | ||
| config.maxRetries, | ||
| config.retryDelay | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing exponential backoff for retries
The current retry mechanism uses a constant delay defined by config.retryDelay. Consider implementing an exponential backoff strategy to improve resilience against transient failures when exporting to the external API.
Here's a suggested implementation of exponential backoff:
const retryWithExponentialBackoff = async (
operation: () => Promise<any>,
maxRetries: number,
initialDelay: number
): Promise<any> => {
let retries = 0;
while (retries < maxRetries) {
try {
return await operation();
} catch (error) {
if (retries === maxRetries - 1) throw error;
const delay = initialDelay * Math.pow(2, retries);
await new Promise(resolve => setTimeout(resolve, delay));
retries++;
}
}
};
// Usage in the plugin
await retryWithExponentialBackoff(
() => exportToExternalAPI(records, config.apiEndpoint, config.authToken),
config.maxRetries,
config.retryDelay
);| let pageNumber = 1 | ||
| while (pageNumber <= batchCount) { | ||
| const records = await Simplified.getAllRecords(sheetId, { | ||
| pageSize: config.batchSize, | ||
| pageNumber, | ||
| }) | ||
|
|
||
| try { | ||
| await retryOperation( | ||
| () => | ||
| exportToExternalAPI( | ||
| records, | ||
| config.apiEndpoint, | ||
| config.authToken | ||
| ), | ||
| config.maxRetries, | ||
| config.retryDelay | ||
| ) | ||
| totalExported += records.length | ||
| successfulBatches++ | ||
| } catch (error) { | ||
| console.error('Failed to export batch after retries:', error) | ||
| failedRecords += records.length | ||
| } | ||
|
|
||
| const progress = (successfulBatches / batchCount) * 100 | ||
| await tick( | ||
| progress, | ||
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix missing pageNumber increment to prevent infinite loop
The pageNumber variable is not incremented within the while loop, which will cause an infinite loop because pageNumber will always be less than or equal to batchCount. Increment pageNumber at the end of each loop iteration to ensure the loop progresses correctly.
Apply this diff to fix the issue:
while (pageNumber <= batchCount) {
const records = await Simplified.getAllRecords(sheetId, {
pageSize: config.batchSize,
pageNumber,
})
// ... processing code ...
await tick(
progress,
`Exported ${totalExported} records. Failed to export ${failedRecords} records.`
)
+ pageNumber++
}📝 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 pageNumber = 1 | |
| while (pageNumber <= batchCount) { | |
| const records = await Simplified.getAllRecords(sheetId, { | |
| pageSize: config.batchSize, | |
| pageNumber, | |
| }) | |
| try { | |
| await retryOperation( | |
| () => | |
| exportToExternalAPI( | |
| records, | |
| config.apiEndpoint, | |
| config.authToken | |
| ), | |
| config.maxRetries, | |
| config.retryDelay | |
| ) | |
| totalExported += records.length | |
| successfulBatches++ | |
| } catch (error) { | |
| console.error('Failed to export batch after retries:', error) | |
| failedRecords += records.length | |
| } | |
| const progress = (successfulBatches / batchCount) * 100 | |
| await tick( | |
| progress, | |
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | |
| ) | |
| } | |
| let pageNumber = 1 | |
| while (pageNumber <= batchCount) { | |
| const records = await Simplified.getAllRecords(sheetId, { | |
| pageSize: config.batchSize, | |
| pageNumber, | |
| }) | |
| try { | |
| await retryOperation( | |
| () => | |
| exportToExternalAPI( | |
| records, | |
| config.apiEndpoint, | |
| config.authToken | |
| ), | |
| config.maxRetries, | |
| config.retryDelay | |
| ) | |
| totalExported += records.length | |
| successfulBatches++ | |
| } catch (error) { | |
| console.error('Failed to export batch after retries:', error) | |
| failedRecords += records.length | |
| } | |
| const progress = (successfulBatches / batchCount) * 100 | |
| await tick( | |
| progress, | |
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | |
| ) | |
| pageNumber++ | |
| } |
| export default function (listener) { | ||
| listener.use( | ||
| exportToExternalAPIPlugin({ | ||
| apiEndpoint: 'https://api.example.com/import', |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
export/external-api/src/external.api.plugin.ts (2)
7-14: Add validation for configuration fieldsThe
PluginConfiginterface should include runtime validation to ensure:
batchSizeis a positive integermaxRetriesis a non-negative integerretryDelayis a positive numberapiEndpointis a valid URLConsider adding a validation function:
function validateConfig(config: PluginConfig): void { if (config.batchSize <= 0) throw new Error('batchSize must be positive'); if (config.maxRetries < 0) throw new Error('maxRetries must be non-negative'); if (config.retryDelay <= 0) throw new Error('retryDelay must be positive'); try { new URL(config.apiEndpoint); } catch { throw new Error('apiEndpoint must be a valid URL'); } }
40-43: Optimize memory usage for large datasetsThe
recordsarray holds all records in memory until the batch is processed. For large batches, this could lead to memory pressure.Consider implementing a streaming approach or adding a memory limit check:
const memoryLimit = 100 * 1024 * 1024; // 100MB const records = await Simplified.getAllRecords(sheetId, { pageSize: Math.min(config.batchSize, Math.floor(memoryLimit / averageRecordSize)), pageNumber, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (2)
- export/external-api/src/external.api.plugin.ts (1 hunks)
- flatfilers/sandbox/src/index.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
flatfilers/sandbox/src/index.ts (1)
2-2: Verify plugin availability and versionLet's ensure the plugin is properly published and available in the package registry.
export/external-api/src/external.api.plugin.ts (1)
16-27: LGTM! Well-structured plugin setup.The plugin initialization follows best practices with proper type safety and event handling setup.
| @@ -1,14 +1,15 @@ | |||
| import type { FlatfileListener } from '@flatfile/listener' | |||
| import { pivotTablePlugin } from '@flatfile/plugin-export-pivot-table' | |||
| import { exportToExternalAPIPlugin } from '@flatfile/plugin-export-external-api' | |||
There was a problem hiding this comment.
Security and configuration concerns in plugin setup
- The authentication token should not be hardcoded in the source code. Consider using environment variables or a secure configuration management system.
- The API endpoint is set to a local development URL (
http://localhost:5678), which won't work in production. - Configuration values like
batchSize,maxRetries, andretryDelayshould be externalized for easier maintenance.
Consider refactoring to:
import { exportToExternalAPIPlugin } from '@flatfile/plugin-export-external-api'
+import { config } from './config' // Create a separate config file
listener.use(
exportToExternalAPIPlugin({
job: 'export-external-api',
- apiEndpoint: 'http://localhost:5678/api/import',
- authToken: 'your-api-token',
- batchSize: 100,
- maxRetries: 3,
- retryDelay: 1000,
+ apiEndpoint: config.apiEndpoint,
+ authToken: config.authToken,
+ batchSize: config.batchSize,
+ maxRetries: config.maxRetries,
+ retryDelay: config.retryDelay,
})
)Also applies to: 6-12
| actions: [ | ||
| { | ||
| operation: 'export-external-api', | ||
| label: 'Export to External API', | ||
| description: | ||
| 'This custom action code exports the records in the Sales sheet to an external API.', | ||
| primary: false, | ||
| mode: 'foreground', | ||
| type: 'string', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Review and enhance action configuration
- The
type: 'string'seems incorrect for an export action. This property typically defines the return type of the action. - Consider adding a confirmation dialog for this operation since it's exporting data to an external system.
- The description could be more informative about error handling and retry behavior.
Consider applying these improvements:
actions: [
{
operation: 'export-external-api',
label: 'Export to External API',
description:
- 'This custom action code exports the records in the Sales sheet to an external API.',
+ 'Exports records from the Sales sheet to an external API. Failed requests will be retried up to 3 times. Data is sent in batches of 100 records.',
primary: false,
mode: 'foreground',
- type: 'string',
+ confirm: {
+ title: 'Confirm Export',
+ description: 'Are you sure you want to export all records to the external API?'
+ }
},
],📝 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.
| actions: [ | |
| { | |
| operation: 'export-external-api', | |
| label: 'Export to External API', | |
| description: | |
| 'This custom action code exports the records in the Sales sheet to an external API.', | |
| primary: false, | |
| mode: 'foreground', | |
| type: 'string', | |
| }, | |
| ], | |
| actions: [ | |
| { | |
| operation: 'export-external-api', | |
| label: 'Export to External API', | |
| description: | |
| 'Exports records from the Sales sheet to an external API. Failed requests will be retried up to 3 times. Data is sent in batches of 100 records.', | |
| primary: false, | |
| mode: 'foreground', | |
| confirm: { | |
| title: 'Confirm Export', | |
| description: 'Are you sure you want to export all records to the external API?' | |
| } | |
| }, | |
| ], |
| const progress = (successfulBatches / batchCount) * 100 | ||
| await tick( | ||
| progress, | ||
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | ||
| ) |
There was a problem hiding this comment.
Adjust progress calculation to account for failed batches
The progress calculation only considers successful batches, which could be misleading if some batches fail.
Consider updating the progress calculation:
- const progress = (successfulBatches / batchCount) * 100
+ const progress = ((pageNumber - 1) / batchCount) * 100
await tick(
progress,
`Exported ${totalExported} records. Failed to export ${failedRecords} records.`
)📝 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.
| const progress = (successfulBatches / batchCount) * 100 | |
| await tick( | |
| progress, | |
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | |
| ) | |
| const progress = ((pageNumber - 1) / batchCount) * 100 | |
| await tick( | |
| progress, | |
| `Exported ${totalExported} records. Failed to export ${failedRecords} records.` | |
| ) |
| } catch (error) { | ||
| console.error('Error during export:', (error as Error).message) | ||
|
|
||
| throw new Error('An error occurred during export.') | ||
| } |
There was a problem hiding this comment.
Improve error handling and propagation
The generic error message hides potentially useful information from the user.
Consider propagating the error details:
- console.error('Error during export:', (error as Error).message)
- throw new Error('An error occurred during export.')
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error'
+ console.error('Error during export:', errorMessage)
+ throw new Error(`Export failed: ${errorMessage}`)📝 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) { | |
| console.error('Error during export:', (error as Error).message) | |
| throw new Error('An error occurred during export.') | |
| } | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : 'Unknown error' | |
| console.error('Error during export:', errorMessage) | |
| throw new Error(`Export failed: ${errorMessage}`) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- export/external-api/README.MD (1 hunks)
- export/external-api/src/external.api.plugin.ts (1 hunks)
- flatfilers/sandbox/src/index.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- export/external-api/src/external.api.plugin.ts
- flatfilers/sandbox/src/index.ts
🧰 Additional context used
🪛 Markdownlint
export/external-api/README.MD
23-23: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
| exportToExternalAPIPlugin({ | ||
| apiEndpoint: 'https://api.example.com/import', | ||
| secretName: 'YOUR_SECRET_NAME', | ||
| batchSize: 100, | ||
| maxRetries: 3, | ||
| retryDelay: 1000 | ||
| }) |
There was a problem hiding this comment.
Add missing required job parameter in example configuration.
The example configuration is missing the required job parameter that was documented in the parameters section.
Add the missing parameter to the configuration:
exportToExternalAPIPlugin({
+ job: 'export-data',
apiEndpoint: 'https://api.example.com/import',
secretName: 'YOUR_SECRET_NAME',
batchSize: 100,
maxRetries: 3,
retryDelay: 1000
})📝 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.
| exportToExternalAPIPlugin({ | |
| apiEndpoint: 'https://api.example.com/import', | |
| secretName: 'YOUR_SECRET_NAME', | |
| batchSize: 100, | |
| maxRetries: 3, | |
| retryDelay: 1000 | |
| }) | |
| exportToExternalAPIPlugin({ | |
| job: 'export-data', | |
| apiEndpoint: 'https://api.example.com/import', | |
| secretName: 'YOUR_SECRET_NAME', | |
| batchSize: 100, | |
| maxRetries: 3, | |
| retryDelay: 1000 | |
| }) |
| #### `job` - `string` - (required) | ||
| The type of job to create. | ||
|
|
||
| #### `apiEndpoint` - `string` - (required) | ||
| The URL of the external API endpoint to send data to. | ||
|
|
||
| #### `secretName` - `string` - (required) | ||
| The name of the Flatfile Secret that contains the authentication token for the external API. | ||
|
|
||
| #### `batchSize` - `number` - (required) | ||
| The number of records to process in each batch. | ||
|
|
||
| #### `maxRetries` - `number` - (required) | ||
| The maximum number of retry attempts for failed API calls. | ||
|
|
||
| #### `retryDelay` - `number` - (required) | ||
| The delay (in milliseconds) between retry attempts. |
There was a problem hiding this comment.
Fix heading hierarchy in parameters section.
The parameter headings should be h3 (###) instead of h4 (####) to maintain proper heading hierarchy.
Apply this change to all parameter headings:
-#### `job` - `string` - (required)
+### `job` - `string` - (required)
-#### `apiEndpoint` - `string` - (required)
+### `apiEndpoint` - `string` - (required)
-#### `secretName` - `string` - (required)
+### `secretName` - `string` - (required)
-#### `batchSize` - `number` - (required)
+### `batchSize` - `number` - (required)
-#### `maxRetries` - `number` - (required)
+### `maxRetries` - `number` - (required)
-#### `retryDelay` - `number` - (required)
+### `retryDelay` - `number` - (required)📝 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.
| #### `job` - `string` - (required) | |
| The type of job to create. | |
| #### `apiEndpoint` - `string` - (required) | |
| The URL of the external API endpoint to send data to. | |
| #### `secretName` - `string` - (required) | |
| The name of the Flatfile Secret that contains the authentication token for the external API. | |
| #### `batchSize` - `number` - (required) | |
| The number of records to process in each batch. | |
| #### `maxRetries` - `number` - (required) | |
| The maximum number of retry attempts for failed API calls. | |
| #### `retryDelay` - `number` - (required) | |
| The delay (in milliseconds) between retry attempts. | |
| ### `job` - `string` - (required) | |
| The type of job to create. | |
| ### `apiEndpoint` - `string` - (required) | |
| The URL of the external API endpoint to send data to. | |
| ### `secretName` - `string` - (required) | |
| The name of the Flatfile Secret that contains the authentication token for the external API. | |
| ### `batchSize` - `number` - (required) | |
| The number of records to process in each batch. | |
| ### `maxRetries` - `number` - (required) | |
| The maximum number of retry attempts for failed API calls. | |
| ### `retryDelay` - `number` - (required) | |
| The delay (in milliseconds) between retry attempts. |
🧰 Tools
🪛 Markdownlint
23-23: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
Please explain how to summarize this PR for the Changelog:
Tell code reviewer how and what to test: