Allow retransmission requests for singular segments #22
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files. The Changes
Possibly related PRs
Suggested reviewers
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 (
|
esterlus
changed the title
![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
🧹 Outside diff range and nitpick comments (11)
src/db.ts (4)
1-1: Consider specifying the sqlite3 package version.While the import statement is correct, it's generally a good practice to specify the exact version of the package you're using in your
package.jsonfile. This ensures consistency across different environments and prevents potential issues from unexpected updates.
5-14: LGTM: Well-implemented database setup function.The
setupfunction correctly creates a new SQLite database connection and handles errors appropriately. It provides a clean Promise-based interface for database initialization.Consider adding a method to initialize the database schema (create tables, etc.) if needed for your application. This could be called after the database connection is established.
16-25: LGTM: Well-implemented database closure function.The
closefunction correctly closes the SQLite database connection and handles errors appropriately. It provides a clean Promise-based interface for database closure.Consider adding a method to perform any necessary cleanup operations (like flushing pending writes) before closing the connection, if needed for your application.
1-25: Overall: Solid foundation for database management.This file provides a clean and well-structured interface for SQLite database connection management. The use of Promises for asynchronous operations and consistent error handling are commendable.
For future enhancements, consider:
- Implementing connection pooling for better performance in multi-user scenarios.
- Adding transaction management utilities.
- Including methods for common database operations (e.g., query, insert, update).
- Implementing a logging mechanism for database operations.
These additions would make the database module more robust and easier to use throughout the application.
src/request-store.spec.ts (1)
Line range hint
1-48: Overall LGTM: Successful transition to DB-centric approach in tests.The changes in this file consistently implement the switch from a
RequestStoreinstance to aDBinstance across all tests. This refactoring maintains the original test logic while potentially providing more flexibility and control over database operations. The direct use of thedbobject for queries is a positive change that simplifies the code.A few points to consider:
- Ensure that these changes are part of a broader refactoring effort across the codebase for consistency.
- Consider adding a comment explaining the empty string parameter in
DB.setup('')if it's intended for in-memory database usage.- If not already done, update the documentation to reflect this new DB-centric approach in testing.
src/request-store.ts (4)
Line range hint
8-28: Ensure Promise Resolves After All Database Operations CompleteIn the
setupfunction, theres()callback is called inside the seconddb.runoperation's callback. However, if the firstdb.runfails afterres()is called, the promise would have already been resolved, potentially leading to unhandled errors.Consider nesting the second
db.runinside the first one's callback to ensure that the promise only resolves after both database operations have completed successfully.Apply the following changes to properly sequence the database operations:
export function setup(db: DB): Promise<void> { return new Promise((res, rej) => { db.serialize(() => { db.run( 'CREATE TABLE IF NOT EXISTS request_store (uuid TEXT PRIMARY KEY, counter INTEGER)', (err) => { if (err) { return rej(`Error creating table request_store: ${err}`); } + // Proceed to create index only if table creation succeeded + db.run( + 'CREATE INDEX IF NOT EXISTS request_store_counter_index ON request_store (counter)', + (err) => { + if (err) { + return rej(`Error creating index request_store_counter_index: ${err}`); + } + return res(); + }, + ); }, ); - - db.run( - 'CREATE INDEX IF NOT EXISTS request_store_counter_index ON request_store (counter)', - (err) => { - if (err) { - return rej(`Error creating index request_store_counter_index: ${err}`); - } - return res(); - }, - ); }); }); }
Line range hint
33-54: Use Error Codes for Robust Duplicate DetectionIn the
addIfAbsentfunction, the current approach checks for duplicate entries by examining the error message string. This method is fragile and can fail if the error message changes or varies by locale.Instead, check the SQLite error code to reliably detect constraint violations.
Apply the following changes to use error codes:
export function addIfAbsent(db: DB, id: string, counter: number): Promise<AddRes> { return new Promise((res, rej) => { db.run( 'INSERT INTO request_store (uuid, counter) VALUES ($id, $counter);', { $id: id, $counter: counter, }, (err) => { if (err) { - const errStr = String(err).toLowerCase(); - const cts = ['sqlite_constraint', 'unique', 'request_store.uuid']; - if (cts.every((c) => errStr.includes(c))) { + if (err.code === 'SQLITE_CONSTRAINT' || err.code === 'SQLITE_CONSTRAINT_PRIMARYKEY') { return res(AddRes.Duplicate); } return rej(`Error inserting into request_store: ${err}`); } return res(AddRes.Success); }, ); }); }
Line range hint
8-59: Refactor to Use Async/Await for Better ReadabilityThe current implementation wraps each database operation in a new
Promise, resulting in nested and verbose code. Refactoring the code to useutil.promisifyor an asynchronous wrapper library would allow the use ofasync/await, improving readability and maintainability.For example, using
util.promisify:import { promisify } from 'util'; const runAsync = promisify(db.run.bind(db)); export async function setup(db: DB): Promise<void> { await runAsync('CREATE TABLE IF NOT EXISTS request_store (uuid TEXT PRIMARY KEY, counter INTEGER);'); await runAsync('CREATE INDEX IF NOT EXISTS request_store_counter_index ON request_store (counter);'); }Consider applying this pattern to other functions like
addIfAbsentandremoveExpired.
Line range hint
56-68: Handle Potential Errors inremoveExpiredFunctionWhile the
removeExpiredfunction correctly handles errors during the deletion operation, consider logging the number of rows affected to monitor the cleanup process. This can aid in debugging and ensure that old entries are being removed as expected.For example, modify the function to log the changes:
export function removeExpired(db: DB, olderThan: number): Promise<void> { return new Promise((res, rej) => { - db.run( + db.run( 'DELETE FROM request_store where counter < $counter;', { $counter: olderThan }, + function (err) { if (err) { return rej(`Error deleting from request_store: ${err}`); } + console.log(`Removed ${this.changes} expired requests.`); return res(); }, - ); + ); }); }src/response-segment-store.ts (1)
51-67: Handle database errors consistentlyIn the
putfunction, consider handling database errors consistently and providing meaningful error messages or retry mechanisms if necessary. This ensures that any issues during data insertion are properly logged and managed.src/index.ts (1)
203-203: Typo in comment: Correct 'somehwere' to 'somewhere'The comment contains a typo. 'somehwere' should be corrected to 'somewhere' for clarity.
Apply this diff to fix the typo:
- // schedule next run somehwere between 1h and 1h and 10m + // schedule next run somewhere between 1h and 1h and 10m
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
- .gitignore (1 hunks)
- package.json (1 hunks)
- src/db.ts (1 hunks)
- src/index.ts (17 hunks)
- src/request-store.spec.ts (1 hunks)
- src/request-store.ts (3 hunks)
- src/response-segment-store.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🔇 Additional comments (12)
src/db.ts (1)
3-3: LGTM: Clear and useful type alias.The exported type alias
DBforsqlite3.Databaseenhances code readability and provides a convenient way to refer to the database type throughout the application.package.json (1)
35-35: Approve dependency update and request verification.The update of
@hoprnet/uhttp-libfrom^3.3.0to^3.7.2looks good. This minor version update likely introduces new features or bug fixes that could be beneficial to the project.However, to ensure this change aligns with the PR objectives:
- Could you confirm if this update is intentional and related to the PR's goal of allowing "retransmission requests for singular segments"?
- Have you reviewed the changelog for
@hoprnet/uhttp-libto understand the changes introduced in versions 3.4.0 through 3.7.2?- Have you tested the application with this new version to ensure compatibility and desired functionality?
To help verify the impact of this change, you can run the following script:
This script will help identify any potential breaking changes or areas that might need adjustment due to the library update.
src/request-store.spec.ts (7)
2-2: LGTM: DB module import added.The addition of the DB module import is consistent with the changes in the test suite, reflecting the transition to using a DB instance for database operations.
4-4: LGTM: DB variable declaration added.The addition of the
dbvariable with the correct type annotation is appropriate for the new approach using a DB instance throughout the test suite.
13-13: LGTM: Cleanup process updated.The change to use
DB.close(db)in theafterEachhook is consistent with the new DB-centric approach and ensures proper cleanup after each test.
16-16: LGTM: Test updated to use DB instance.The change to use
reqStore.addIfAbsent(db, ...)is consistent with the new DB-centric approach while maintaining the original test logic.
21-22: LGTM: Test updated to use DB instance.The changes to use
reqStore.addIfAbsent(db, ...)in both calls are consistent with the new DB-centric approach while maintaining the original test logic for detecting duplicates.
29-31: LGTM: Test updated to use DB instance with improved database access.The changes to use the
dbobject in allreqStoremethod calls and the direct use ofdb.all(...)for the database query are consistent with the new DB-centric approach. This update maintains the original test logic while providing a more straightforward database access method.Also applies to: 34-34, 37-37
9-10: LGTM with a question: DB setup changes look good.The changes to use
DB.setup('')andreqStore.setup(db)are consistent with the new DB-centric approach. However, could you clarify why an empty string is passed toDB.setup? Is this intended for in-memory database usage, or should it be a file path for persistent storage?✅ Verification successful
DB.setup changes verified and consistent.
No additional instances of
DB.setup('')were found in the codebase, confirming that its usage insrc/request-store.spec.tsis intentional and isolated to the test setup.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if DB.setup is called with an empty string elsewhere rg --type typescript "DB\.setup\(''\)" -C 3Length of output: 86
Script:
#!/bin/bash # Check if DB.setup is called with an empty string in TypeScript files rg "DB\.setup\(''\)" -g "*.ts" -g "*.tsx" -C 3Length of output: 422
src/index.ts (3)
88-97: Proper setup and error handling for database initializationThe code correctly initializes the database and handles errors appropriately. The
RequestStoreandResponseSegmentStoreare properly set up with the database instance.
169-181: Implementation ofremoveExpiredSegmentResponsesis correctThe function successfully removes expired response segments from the database and schedules the next execution. Error handling and logging are handled appropriately.
182-193: Implementation ofremoveExpiredRequestsis correctThe function correctly removes expired requests from the database and schedules the next run. It includes proper error handling and logging.
| export function all(db: DB, requestId: string, segmentNrs: number[]): Promise<Segment.Segment[]> { | ||
| // manually handle IN operator behaviour | ||
| const placeholders = segmentNrs.map(() => '?').join(','); | ||
| const params: Record<number, string | number> = { 1: requestId }; | ||
| for (let i = 0; i < segmentNrs.length; i++) { | ||
| // sql indexes start at 1 - which was already used for requestId | ||
| params[i + 2] = segmentNrs[i]; | ||
| } | ||
| const select = [ | ||
| 'SELECT nr, body, total_count FROM response_segment_store', | ||
| `WHERE request_id = ? and nr in (${placeholders})`, | ||
| ].join(' '); | ||
| return new Promise((res, rej) => { | ||
| db.all(select, params, function (err: any, rows: any) { | ||
| if (err) { | ||
| return rej(`Error selecting segment from response_segment_store: ${err}`); | ||
| } | ||
| if (!rows) { | ||
| return rej('Unable to find any segments matching missing segments request'); | ||
| } | ||
| const segments = rows.map((rawRow: any) => { | ||
| const row = rawRow as { nr: number; total_count: number; body: string }; | ||
| return { | ||
| requestId, | ||
| nr: row.nr, | ||
| totalCount: row.total_count, | ||
| body: row.body, | ||
| }; | ||
| }); | ||
| return res(segments); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix parameter binding in the all function to prevent potential issues
The all function constructs an SQL query using '?' placeholders but passes the parameters as an object with numeric keys. When using '?' placeholders, the parameters should be provided as an array in the correct order, not as an object. This mismatch can lead to unexpected behavior or SQL injection vulnerabilities.
Current code:
const params: Record<number, string | number> = { 1: requestId };
for (let i = 0; i < segmentNrs.length; i++) {
// SQL indexes start at 1 - which was already used for requestId
params[i + 2] = segmentNrs[i];
}Suggested fix:
Construct the params as an array to ensure proper parameter binding:
const params = [requestId, ...segmentNrs];Ensure that the parameters array matches the order of the placeholders in your SQL query.
| if (!rows) { | ||
| return rej('Unable to find any segments matching missing segments request'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary check for undefined rows
In the callback of db.all, the rows parameter will always be an array, even if no results are found. The check if (!rows) is unnecessary and can be removed to simplify the code.
Current code:
if (!rows) {
return rej('Unable to find any segments matching missing segments request');
}Suggested fix:
Remove the check for rows being undefined:
// This check is unnecessary as `rows` will always be an arrayHandle the case where rows is an empty array if needed.
| db.all(select, params, function (err: any, rows: any) { | ||
| if (err) { | ||
| return rej(`Error selecting segment from response_segment_store: ${err}`); | ||
| } | ||
| if (!rows) { | ||
| return rej('Unable to find any segments matching missing segments request'); | ||
| } | ||
| const segments = rows.map((rawRow: any) => { | ||
| const row = rawRow as { nr: number; total_count: number; body: string }; | ||
| return { | ||
| requestId, | ||
| nr: row.nr, | ||
| totalCount: row.total_count, | ||
| body: row.body, | ||
| }; | ||
| }); | ||
| return res(segments); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance TypeScript typings for better type safety
The use of any in TypeScript defeats the purpose of strong typing. Improving typings can help catch errors at compile time and improve code readability.
Current code:
db.all(select, params, function (err: any, rows: any) {
// ...
const segments = rows.map((rawRow: any) => {
const row = rawRow as { nr: number; total_count: number; body: string };
// ...
});
// ...
});Suggested fix:
- Specify the types for
errandrows. - Define an interface or type for the rows returned from the database.
- Use proper typing in the callback function.
Example:
interface SegmentRow {
nr: number;
total_count: number;
body: Buffer;
}
db.all(select, params, function (err: Error | null, rows: SegmentRow[]) {
if (err) {
return rej(new Error(`Error selecting segment from response_segment_store: ${err}`));
}
const segments = rows.map((row) => ({
requestId,
nr: row.nr,
totalCount: row.total_count,
body: row.body,
}));
return res(segments);
});| ].join(' '), | ||
| (err) => { | ||
| if (err) { | ||
| return rej(`Error creating table response_segment_store: ${err}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Error objects when rejecting Promises
Rejecting Promises with plain strings can lead to loss of stack trace and makes error handling less effective. It's better to reject with Error objects to preserve the stack trace and provide more meaningful error information.
Current code examples:
return rej(`Error creating table response_segment_store: ${err}`);Suggested fix:
Wrap error messages in Error objects:
return rej(new Error(`Error creating table response_segment_store: ${err}`));Apply this change to all instances where the Promise is rejected:
- Line 24
- Line 33
- Line 42
- Line 61
- Line 84
- Line 110
Also applies to: 33-33, 42-42, 61-61, 84-84, 110-110
| export function setup(db: DB) { | ||
| return new Promise((res, rej) => { | ||
| // requestId - uuid | ||
| // segment nr - integer, indexed | ||
| // segment body - blob, content | ||
| // inserted_at - integer, timestamp, indexed | ||
| db.serialize(() => { | ||
| db.run( | ||
| [ | ||
| 'CREATE TABLE IF NOT EXISTS response_segment_store', | ||
| '(request_id TEXT NOT NULL,', | ||
| 'nr INTEGER NOT NULL,', | ||
| 'total_count INTEGER NOT NULL,', | ||
| 'body BLOB,', | ||
| 'inserted_at INTEGER,', | ||
| 'PRIMARY KEY (request_id, nr))', | ||
| ].join(' '), | ||
| (err) => { | ||
| if (err) { | ||
| return rej(`Error creating table response_segment_store: ${err}`); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| db.run( | ||
| 'CREATE INDEX IF NOT EXISTS response_store_inserted_at_index ON response_segment_store (inserted_at)', | ||
| (err) => { | ||
| if (err) { | ||
| return rej(`Error creating index response_store_inserted_at_index: ${err}`); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| db.run( | ||
| 'CREATE INDEX IF NOT EXISTS response_store_nr_index ON response_segment_store (nr)', | ||
| (err) => { | ||
| if (err) { | ||
| return rej(`Error creating index response_store_nr_index: ${err}`); | ||
| } | ||
| return res({ db }); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify asynchronous code using async/await
The setup function uses nested callbacks and manual Promise construction, which can make the code harder to read and maintain.
Suggested fix:
Refactor the setup function to use async/await and promisify the db.run and db.serialize methods for cleaner asynchronous code.
Example:
import { promisify } from 'util';
export async function setup(db: DB): Promise<void> {
const runAsync = promisify(db.run.bind(db));
await runAsync(`
CREATE TABLE IF NOT EXISTS response_segment_store (
request_id TEXT NOT NULL,
nr INTEGER NOT NULL,
total_count INTEGER NOT NULL,
body BLOB,
inserted_at INTEGER,
PRIMARY KEY (request_id, nr)
);
`);
await runAsync(`
CREATE INDEX IF NOT EXISTS response_store_inserted_at_index
ON response_segment_store (inserted_at);
`);
await runAsync(`
CREATE INDEX IF NOT EXISTS response_store_nr_index
ON response_segment_store (nr);
`);
}Note: You'll need to ensure that the database methods support promisification or use a library that provides Promise-based methods.
| log.error('error sending %s: %o', Segment.prettyPrint(seg), err); | ||
| // remove relay if it fails | ||
| state.relays = state.relays.filter((r) => r !== relay); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding retry logic before removing a relay from the list
Currently, any error encountered while sending a segment results in the relay being immediately removed from state.relays. This might be too aggressive, as transient network issues could cause relays to be removed unnecessarily. Consider implementing a retry mechanism or tracking error counts to remove a relay only after multiple consecutive failures.
| // resg;originPeerId;hops;requestId;segmentNrs | ||
| const [, recipient, hopsStr, requestId, rawSegNrs] = msg.body.split(';'); | ||
| const hops = parseInt(hopsStr, 10); | ||
| const conn = { ...ops, hops }; | ||
| const segNrs = rawSegNrs.split(',').map((s) => parseInt(s)); | ||
|
|
There was a problem hiding this comment.
Add input validation when parsing message in onRetransferSegmentsReq
The code assumes that msg.body.split(';') will always return an array with at least five elements. If the message is malformed or doesn't conform to the expected format, accessing array elements without checking could result in undefined values and potential runtime errors. It's advisable to add input validation to ensure that all expected fields are present and valid before using them.
Apply this diff to add input validation:
function onRetransferSegmentsReq(state: State, ops: Ops, msg: Msg) {
log.info('received retransfer segments req:', msg.body);
- // resg;originPeerId;hops;requestId;segmentNrs
- const [, recipient, hopsStr, requestId, rawSegNrs] = msg.body.split(';');
+ // Expected format: resg;originPeerId;hops;requestId;segmentNrs
+ const parts = msg.body.split(';');
+ if (parts.length < 5) {
+ log.error('Malformed retransfer segments request message');
+ return;
+ }
+ const [, recipient, hopsStr, requestId, rawSegNrs] = parts;
const hops = parseInt(hopsStr, 10);
const conn = { ...ops, hops };
const segNrs = rawSegNrs.split(',').map((s) => parseInt(s));📝 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.
| // resg;originPeerId;hops;requestId;segmentNrs | |
| const [, recipient, hopsStr, requestId, rawSegNrs] = msg.body.split(';'); | |
| const hops = parseInt(hopsStr, 10); | |
| const conn = { ...ops, hops }; | |
| const segNrs = rawSegNrs.split(',').map((s) => parseInt(s)); | |
| // Expected format: resg;originPeerId;hops;requestId;segmentNrs | |
| const parts = msg.body.split(';'); | |
| if (parts.length < 5) { | |
| log.error('Malformed retransfer segments request message'); | |
| return; | |
| } | |
| const [, recipient, hopsStr, requestId, rawSegNrs] = parts; | |
| const hops = parseInt(hopsStr, 10); | |
| const conn = { ...ops, hops }; | |
| const segNrs = rawSegNrs.split(',').map((s) => parseInt(s)); |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (8)
src/db.ts (3)
5-14: Approve setup function with suggestions for improvement.The
setupfunction is well-implemented, using a Promise to handle the asynchronous nature of database creation. However, consider the following improvements:
- Use more specific error types instead of string messages.
- Add database initialization logic (e.g., creating necessary tables) within this function.
Here's a suggested improvement for error handling:
import { DatabaseError } from './errors'; // Create this file with custom error types export function setup(dbFile: string): Promise<DB> { return new Promise((res, rej) => { const db: sqlite3.Database = new sqlite3.Database(dbFile, (err) => { if (err) { return rej(new DatabaseError(`Error creating db: ${err.message}`, err)); } // Add initialization logic here (e.g., creating tables) return res(db); }); }); }
16-25: Approve close function with suggestions for improvement.The
closefunction is well-implemented, using a Promise to handle the asynchronous nature of closing the database connection. However, consider the following improvements:
- Use more specific error types instead of string messages, similar to the suggestion for the
setupfunction.- Consider adding a timeout mechanism to ensure the database closes even if the operation hangs.
Here's a suggested improvement:
import { DatabaseError } from './errors'; // Create this file with custom error types export function close(db: DB): Promise<void> { return new Promise((res, rej) => { const timeout = setTimeout(() => { rej(new DatabaseError('Database close operation timed out')); }, 5000); // 5 seconds timeout db.close((err) => { clearTimeout(timeout); if (err) { return rej(new DatabaseError(`Error closing db: ${err.message}`, err)); } return res(); }); }); }
1-25: Overall feedback: Good foundation, room for improvement.The
db.tsfile provides a solid foundation for managing SQLite database connections. However, consider the following improvements:
- Implement more robust error handling with custom error types.
- Add database initialization logic (e.g., creating tables) in the
setupfunction.- Include JSDoc comments for better documentation of functions and types.
- Consider adding a function to run migrations or check the database schema version.
To improve the overall architecture, consider creating a
DatabaseManagerclass that encapsulates these functions and manages the database lifecycle. This could provide a more cohesive API for database operations throughout your application.src/request-store.ts (4)
1-1: Approved: Streamlined database interfaceThe removal of the
RequestStoretype and the direct import of theDBtype from './db' indicates a shift towards more direct database handling. This change aligns with the overall goal of streamlining the database interface, which should lead to cleaner and more maintainable code.
Line range hint
33-55: Approved: Updated addIfAbsent function with direct DB handlingThe
addIfAbsentfunction has been successfully updated to work with the new direct DB handling approach. The core logic and error handling remain intact, which is good for maintaining existing functionality.Consider enhancing the error handling by using a more robust method to check for the specific SQLite constraint error. Here's a suggestion:
- const errStr = String(err).toLowerCase(); - const cts = ['sqlite_constraint', 'unique', 'request_store.uuid']; - if (cts.every((c) => errStr.includes(c))) { + if (err.code === 'SQLITE_CONSTRAINT' && err.message.includes('UNIQUE constraint failed: request_store.uuid')) { return res(AddRes.Duplicate); }This approach relies on specific error codes and messages, which are less likely to change and provide more accurate error identification.
Line range hint
1-68: Overall assessment: Successful refactoring to direct DB handlingThe changes in this file successfully implement the shift towards direct database handling, removing the
RequestStoreabstraction. This refactoring aligns with the PR objectives and should lead to cleaner, more maintainable code. The core functionality has been preserved while simplifying the interface.Key points:
- Removed
RequestStoretype and importedDBtype directly.- Updated function signatures to work with
DBinstances directly.- Maintained existing logic and error handling in core functions.
- Removed the
closefunction, centralizing database lifecycle management.These changes contribute to a more streamlined and consistent approach to database operations across the codebase.
As this change affects the database handling approach, ensure that all other parts of the codebase that interact with this module are updated accordingly. Consider creating a database connection management module if not already present, to handle the lifecycle of the DB instance consistently across the application.
Issues Found: Remaining References to the Removed
closeFunctionThe
removeExpiredfunction has been successfully updated, but there are still references to theclosefunction in the following files:
src/db.tssrc/request-store.spec.tssrc/index.tsThese remaining references need to be addressed to ensure consistency and avoid potential runtime errors.
🔗 Analysis chain
Line range hint
56-68: Approved: Updated removeExpired function and verified close function removalThe
removeExpiredfunction has been successfully updated to work with the new direct DB handling approach. The core logic and error handling remain intact, which is good for maintaining existing functionality.The
closefunction has been removed as mentioned in the summary. To ensure this doesn't cause issues elsewhere in the codebase, please run the following script to check for any remaining references to theclosefunction:If any results are found, they may need to be updated to reflect the new database handling approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the removed close function # Test: Search for close function usage rg --type typescript -e "close\(" -e "\.close\(" -A 5 -B 5Length of output: 95
Script:
#!/bin/bash # Description: Check for any remaining references to the removed close function # Test: Search for close function usage in TypeScript files rg -g "*.ts" -g "*.tsx" -e "close\(" -e "\.close\(" -A 5 -B 5Length of output: 1661
src/response-segment-store.ts (1)
103-115: Consistent SQL Statement FormattingFor consistency and readability, ensure that SQL keywords are capitalized throughout your queries.
Consider updating the DELETE statement:
- 'DELETE FROM response_segment_store where inserted_at < $olderThan;', + 'DELETE FROM response_segment_store WHERE inserted_at < $olderThan',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
- .gitignore (1 hunks)
- package.json (1 hunks)
- src/db.ts (1 hunks)
- src/index.ts (17 hunks)
- src/request-store.ts (3 hunks)
- src/response-segment-store.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
.gitignore (1)
31-33: Excellent additions to .gitignore!These new entries enhance the project's security and maintainability:
gha-creds-*.json: This prevents accidental commits of GitHub Actions credential files, which is crucial for maintaining security.*.sqlite3: Ignoring SQLite database files is a good practice, as these are typically used for local development or testing and should not be tracked in version control.Both additions align well with best practices for version control management.
src/db.ts (1)
1-3: LGTM: Import and type alias are well-defined.The import statement for sqlite3 and the type alias for DB are correctly implemented. This provides a clean interface for working with the SQLite database throughout the rest of the codebase.
package.json (1)
35-35: Dependency update approved, but verify changelog and test thoroughly.The update of
@hoprnet/uhttp-libfrom^3.3.0to^3.7.2is a good practice to keep dependencies up-to-date. This minor version bump should be backwards compatible according to semantic versioning.To ensure a smooth transition:
- Please review the changelog of
@hoprnet/uhttp-libfor any important updates or potential breaking changes between versions 3.3.0 and 3.7.2.- Thoroughly test the application with this new version to ensure full compatibility.
Run the following script to check if there are any breaking changes mentioned in the changelog:
If this script returns any results, please review them carefully and make any necessary adjustments to your code.
src/request-store.ts (1)
Line range hint
8-30: Approved: Updated setup function with direct DB handlingThe
setupfunction has been refactored to accept aDBinstance directly, aligning with the new architecture. The removal of database creation logic and the change in return type are consistent with this approach.To ensure that this change doesn't introduce issues, please verify that the DB instance is properly initialized before being passed to this function. Run the following script to check for proper DB initialization:
✅ Verification successful
Verified: DB instance is properly initialized before being passed to the
setupfunctionThe
DBinstances are correctly initialized usingDB.setupbefore being passed to thesetupfunctions, ensuring proper database handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify DB initialization before passing to setup function # Test: Search for DB initialization and usage with setup function rg --type typescript -e "new DB\(" -e "setup\(" -A 10 -B 10Length of output: 96
Script:
#!/bin/bash # Description: Verify DB initialization before passing to setup function # Test: Search for DB initialization and usage with setup function rg --type ts -e "new DB\(" -e "setup\(" -A 10 -B 10Length of output: 5141
| (err) => { | ||
| if (err) { | ||
| return rej(`Error inserting segment into response_segment_store: ${err}`); | ||
| } | ||
| return res(); |
There was a problem hiding this comment.
Pass Error Objects Directly in Rejections
In the put function, you're stringifying the error object in the rejection. Passing the error object directly preserves stack traces and error information, which is valuable for debugging.
Modify the rejection to pass the error object:
- return rej(`Error inserting segment into response_segment_store: ${err}`);
+ return rej(err);📝 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.
| (err) => { | |
| if (err) { | |
| return rej(`Error inserting segment into response_segment_store: ${err}`); | |
| } | |
| return res(); | |
| (err) => { | |
| if (err) { | |
| return rej(err); | |
| } | |
| return res(); |
| export function setup(db: DB) { | ||
| return new Promise((res, rej) => { | ||
| // requestId - uuid | ||
| // segment nr - integer, indexed | ||
| // segment body - blob, content | ||
| // inserted_at - integer, timestamp, indexed | ||
| db.serialize(() => { | ||
| db.run( | ||
| [ | ||
| 'CREATE TABLE IF NOT EXISTS response_segment_store', | ||
| '(request_id TEXT NOT NULL,', | ||
| 'nr INTEGER NOT NULL,', | ||
| 'total_count INTEGER NOT NULL,', | ||
| 'body BLOB,', | ||
| 'inserted_at INTEGER,', | ||
| 'PRIMARY KEY (request_id, nr))', | ||
| ].join(' '), | ||
| (err) => { | ||
| if (err) { | ||
| return rej(`Error creating table response_segment_store: ${err}`); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| db.run( | ||
| 'CREATE INDEX IF NOT EXISTS response_store_inserted_at_index ON response_segment_store (inserted_at)', | ||
| (err) => { | ||
| if (err) { | ||
| return rej(`Error creating index response_store_inserted_at_index: ${err}`); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| db.run( | ||
| 'CREATE INDEX IF NOT EXISTS response_store_nr_index ON response_segment_store (nr)', | ||
| (err) => { | ||
| if (err) { | ||
| return rej(`Error creating index response_store_nr_index: ${err}`); | ||
| } | ||
| return res({ db }); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Promise Usage with Async/Await in setup Function
The setup function uses nested callbacks and manual Promise construction, which can be simplified using async/await for better readability and maintainability.
Consider refactoring the setup function as follows:
-export function setup(db: DB) {
- return new Promise((res, rej) => {
- db.serialize(() => {
- db.run(
- 'CREATE TABLE IF NOT EXISTS response_segment_store ...',
- (err) => {
- if (err) {
- return rej(`Error creating table response_segment_store: ${err}`);
- }
- }
- );
- // Additional db.run statements...
- return res({ db });
- });
- });
+export async function setup(db: DB): Promise<void> {
+ try {
+ await db.run('CREATE TABLE IF NOT EXISTS response_segment_store (request_id TEXT NOT NULL, nr INTEGER NOT NULL, total_count INTEGER NOT NULL, body BLOB, inserted_at INTEGER, PRIMARY KEY (request_id, nr))');
+ await db.run('CREATE INDEX IF NOT EXISTS response_store_inserted_at_index ON response_segment_store (inserted_at)');
+ await db.run('CREATE INDEX IF NOT EXISTS response_store_nr_index ON response_segment_store (nr)');
+ } catch (err) {
+ throw new Error(`Error setting up response_segment_store: ${err}`);
+ }
}📝 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.
| export function setup(db: DB) { | |
| return new Promise((res, rej) => { | |
| // requestId - uuid | |
| // segment nr - integer, indexed | |
| // segment body - blob, content | |
| // inserted_at - integer, timestamp, indexed | |
| db.serialize(() => { | |
| db.run( | |
| [ | |
| 'CREATE TABLE IF NOT EXISTS response_segment_store', | |
| '(request_id TEXT NOT NULL,', | |
| 'nr INTEGER NOT NULL,', | |
| 'total_count INTEGER NOT NULL,', | |
| 'body BLOB,', | |
| 'inserted_at INTEGER,', | |
| 'PRIMARY KEY (request_id, nr))', | |
| ].join(' '), | |
| (err) => { | |
| if (err) { | |
| return rej(`Error creating table response_segment_store: ${err}`); | |
| } | |
| }, | |
| ); | |
| db.run( | |
| 'CREATE INDEX IF NOT EXISTS response_store_inserted_at_index ON response_segment_store (inserted_at)', | |
| (err) => { | |
| if (err) { | |
| return rej(`Error creating index response_store_inserted_at_index: ${err}`); | |
| } | |
| }, | |
| ); | |
| db.run( | |
| 'CREATE INDEX IF NOT EXISTS response_store_nr_index ON response_segment_store (nr)', | |
| (err) => { | |
| if (err) { | |
| return rej(`Error creating index response_store_nr_index: ${err}`); | |
| } | |
| return res({ db }); | |
| }, | |
| ); | |
| }); | |
| }); | |
| } | |
| export async function setup(db: DB): Promise<void> { | |
| try { | |
| await db.run('CREATE TABLE IF NOT EXISTS response_segment_store (request_id TEXT NOT NULL, nr INTEGER NOT NULL, total_count INTEGER NOT NULL, body BLOB, inserted_at INTEGER, PRIMARY KEY (request_id, nr))'); | |
| await db.run('CREATE INDEX IF NOT EXISTS response_store_inserted_at_index ON response_segment_store (inserted_at)'); | |
| await db.run('CREATE INDEX IF NOT EXISTS response_store_nr_index ON response_segment_store (nr)'); | |
| } catch (err) { | |
| throw new Error(`Error setting up response_segment_store: ${err}`); | |
| } | |
| } |
| db.all(select, params, function (err: any, rows: any) { | ||
| if (err) { | ||
| return rej(`Error selecting segment from response_segment_store: ${err}`); | ||
| } | ||
| if (!rows) { | ||
| return rej('Unable to find any segments matching missing segments request'); | ||
| } | ||
| const segments = rows.map((rawRow: any) => { | ||
| const row = rawRow as { nr: number; total_count: number; body: string }; | ||
| return { | ||
| requestId, | ||
| nr: row.nr, | ||
| totalCount: row.total_count, | ||
| body: row.body, | ||
| }; | ||
| }); | ||
| return res(segments); | ||
| }); |
There was a problem hiding this comment.
Handle Case When No Rows Are Returned
In the callback for db.all, if no rows are found, rows will be an empty array, not undefined. The current check if (!rows) may not work as expected.
Update the condition to check if the array is empty:
- if (!rows) {
+ if (rows.length === 0) {
return rej('Unable to find any segments matching missing segments request');
}📝 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.
| db.all(select, params, function (err: any, rows: any) { | |
| if (err) { | |
| return rej(`Error selecting segment from response_segment_store: ${err}`); | |
| } | |
| if (!rows) { | |
| return rej('Unable to find any segments matching missing segments request'); | |
| } | |
| const segments = rows.map((rawRow: any) => { | |
| const row = rawRow as { nr: number; total_count: number; body: string }; | |
| return { | |
| requestId, | |
| nr: row.nr, | |
| totalCount: row.total_count, | |
| body: row.body, | |
| }; | |
| }); | |
| return res(segments); | |
| }); | |
| db.all(select, params, function (err: any, rows: any) { | |
| if (err) { | |
| return rej(`Error selecting segment from response_segment_store: ${err}`); | |
| } | |
| if (rows.length === 0) { | |
| return rej('Unable to find any segments matching missing segments request'); | |
| } | |
| const segments = rows.map((rawRow: any) => { | |
| const row = rawRow as { nr: number; total_count: number; body: string }; | |
| return { | |
| requestId, | |
| nr: row.nr, | |
| totalCount: row.total_count, | |
| body: row.body, | |
| }; | |
| }); | |
| return res(segments); | |
| }); |
| db.run( | ||
| 'DELETE FROM response_segment_store where inserted_at < $olderThan;', | ||
| { $olderThan: olderThan }, | ||
| (err) => { | ||
| if (err) { | ||
| return rej(`Error deleting from response_segment_store: ${err}`); | ||
| } | ||
| return res(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Using Manual Promise Construction
In the removeExpired function, you can simplify the code by avoiding manual Promise construction and using util.promisify or migrating to async/await.
If using util.promisify:
+import { promisify } from 'util';
+
export function removeExpired(db: DB, olderThan: number): Promise<void> {
- return new Promise((res, rej) => {
- db.run(
- 'DELETE FROM response_segment_store WHERE inserted_at < $olderThan',
- { $olderThan: olderThan },
- (err) => {
- if (err) {
- return rej(`Error deleting from response_segment_store: ${err}`);
- }
- return res();
- },
- );
- });
+ const runAsync = promisify(db.run.bind(db));
+ return runAsync(
+ 'DELETE FROM response_segment_store WHERE inserted_at < $olderThan',
+ { $olderThan: olderThan }
+ );
}Alternatively, using async/await:
-export function removeExpired(db: DB, olderThan: number): Promise<void> {
- return new Promise((res, rej) => {
- db.run(
- 'DELETE FROM response_segment_store WHERE inserted_at < $olderThan',
- { $olderThan: olderThan },
- (err) => {
- if (err) {
- return rej(`Error deleting from response_segment_store: ${err}`);
- }
- return res();
- },
- );
- });
+export async function removeExpired(db: DB, olderThan: number): Promise<void> {
+ try {
+ await db.run(
+ 'DELETE FROM response_segment_store WHERE inserted_at < $olderThan',
+ { $olderThan: olderThan }
+ );
+ } catch (err) {
+ throw new Error(`Error deleting from response_segment_store: ${err}`);
+ }
}Committable suggestion was skipped due to low confidence.
| export function all(db: DB, requestId: string, segmentNrs: number[]): Promise<Segment.Segment[]> { | ||
| // manually handle IN operator behaviour | ||
| const placeholders = segmentNrs.map(() => '?').join(','); | ||
| const params: Record<number, string | number> = { 1: requestId }; | ||
| for (let i = 0; i < segmentNrs.length; i++) { | ||
| // sql indexes start at 1 - which was already used for requestId | ||
| params[i + 2] = segmentNrs[i]; | ||
| } | ||
| const select = [ | ||
| 'SELECT nr, body, total_count FROM response_segment_store', | ||
| `WHERE request_id = ? and nr in (${placeholders})`, | ||
| ].join(' '); | ||
| return new Promise((res, rej) => { | ||
| db.all(select, params, function (err: any, rows: any) { | ||
| if (err) { | ||
| return rej(`Error selecting segment from response_segment_store: ${err}`); | ||
| } | ||
| if (!rows) { | ||
| return rej('Unable to find any segments matching missing segments request'); | ||
| } | ||
| const segments = rows.map((rawRow: any) => { | ||
| const row = rawRow as { nr: number; total_count: number; body: string }; | ||
| return { | ||
| requestId, | ||
| nr: row.nr, | ||
| totalCount: row.total_count, | ||
| body: row.body, | ||
| }; | ||
| }); | ||
| return res(segments); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Correct Parameter Binding in all Function to Prevent SQL Injection
The current parameter binding uses an object with numerical keys, which may not work as intended with SQLite's parameter binding, potentially leading to SQL injection vulnerabilities.
Refactor the parameter binding to use an array:
- const placeholders = segmentNrs.map(() => '?').join(',');
- const params: Record<number, string | number> = { 1: requestId };
- for (let i = 0; i < segmentNrs.length; i++) {
- params[i + 2] = segmentNrs[i];
- }
+ const placeholders = segmentNrs.map(() => '?').join(', ');
+ const params = [requestId, ...segmentNrs];Also, ensure that the SQL query and method call correctly utilize the parameters:
db.all(select, params, function (err: any, rows: any) {
// ...
});📝 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.
| export function all(db: DB, requestId: string, segmentNrs: number[]): Promise<Segment.Segment[]> { | |
| // manually handle IN operator behaviour | |
| const placeholders = segmentNrs.map(() => '?').join(','); | |
| const params: Record<number, string | number> = { 1: requestId }; | |
| for (let i = 0; i < segmentNrs.length; i++) { | |
| // sql indexes start at 1 - which was already used for requestId | |
| params[i + 2] = segmentNrs[i]; | |
| } | |
| const select = [ | |
| 'SELECT nr, body, total_count FROM response_segment_store', | |
| `WHERE request_id = ? and nr in (${placeholders})`, | |
| ].join(' '); | |
| return new Promise((res, rej) => { | |
| db.all(select, params, function (err: any, rows: any) { | |
| if (err) { | |
| return rej(`Error selecting segment from response_segment_store: ${err}`); | |
| } | |
| if (!rows) { | |
| return rej('Unable to find any segments matching missing segments request'); | |
| } | |
| const segments = rows.map((rawRow: any) => { | |
| const row = rawRow as { nr: number; total_count: number; body: string }; | |
| return { | |
| requestId, | |
| nr: row.nr, | |
| totalCount: row.total_count, | |
| body: row.body, | |
| }; | |
| }); | |
| return res(segments); | |
| }); | |
| }); | |
| export function all(db: DB, requestId: string, segmentNrs: number[]): Promise<Segment.Segment[]> { | |
| const placeholders = segmentNrs.map(() => '?').join(', '); | |
| const params = [requestId, ...segmentNrs]; | |
| const select = [ | |
| 'SELECT nr, body, total_count FROM response_segment_store', | |
| `WHERE request_id = ? and nr in (${placeholders})`, | |
| ].join(' '); | |
| return new Promise((res, rej) => { | |
| db.all(select, params, function (err: any, rows: any) { | |
| if (err) { | |
| return rej(`Error selecting segment from response_segment_store: ${err}`); | |
| } | |
| if (!rows) { | |
| return rej('Unable to find any segments matching missing segments request'); | |
| } | |
| const segments = rows.map((rawRow: any) => { | |
| const row = rawRow as { nr: number; total_count: number; body: string }; | |
| return { | |
| requestId, | |
| nr: row.nr, | |
| totalCount: row.total_count, | |
| body: row.body, | |
| }; | |
| }); | |
| return res(segments); | |
| }); | |
| }); | |
| } |
| log.error('error sending %s: %o', Segment.prettyPrint(seg), err); | ||
| // remove relay if it fails | ||
| state.relays = state.relays.filter((r) => r !== relay); | ||
| }); |
There was a problem hiding this comment.
Handle Potential Undefined 'relay' When Filtering Relays
In the error handling of sendResponse, if sending a message fails, the code attempts to remove the relay from state.relays. However, since relay may be undefined, this could inadvertently modify state.relays incorrectly.
It's advisable to check if relay is defined before attempting to remove it:
.catch((err: Error) => {
log.error('error sending %s: %o', Segment.prettyPrint(seg), err);
// remove relay if it fails
+ if (relay) {
state.relays = state.relays.filter((r) => r !== relay);
+ }
});📝 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.
| log.error('error sending %s: %o', Segment.prettyPrint(seg), err); | |
| // remove relay if it fails | |
| state.relays = state.relays.filter((r) => r !== relay); | |
| }); | |
| log.error('error sending %s: %o', Segment.prettyPrint(seg), err); | |
| // remove relay if it fails | |
| if (relay) { | |
| state.relays = state.relays.filter((r) => r !== relay); | |
| } | |
| }); |
| const [, recipient, hopsStr, requestId, rawSegNrs] = msg.body.split(';'); | ||
| const hops = parseInt(hopsStr, 10); | ||
| const conn = { ...ops, hops }; | ||
| const segNrs = rawSegNrs.split(',').map((s) => parseInt(s)); | ||
|
|
There was a problem hiding this comment.
Validate Message Structure and Inputs in onRetransferSegmentsReq
The function onRetransferSegmentsReq splits msg.body without validating its structure. If msg.body doesn't have the expected format, accessing array elements may result in undefined, leading to runtime errors. Similarly, parsing hopsStr and segment numbers without validation may result in NaN or unexpected behavior.
Consider adding validation to ensure that msg.body splits into the expected number of parts and that parsed values are valid:
function onRetransferSegmentsReq(state: State, ops: Ops, msg: Msg) {
log.info('received retransfer segments req:', msg.body);
// resg;originPeerId;hops;requestId;segmentNrs
const parts = msg.body.split(';');
+ if (parts.length !== 5) {
+ log.error('Invalid resg message format: %s', msg.body);
+ return;
+ }
const [, recipient, hopsStr, requestId, rawSegNrs] = parts;
const hops = parseInt(hopsStr, 10);
+ if (isNaN(hops)) {
+ log.error('Invalid hops value in resg message: %s', hopsStr);
+ return;
+ }
const conn = { ...ops, hops };
const segNrs = rawSegNrs.split(',').map((s) => parseInt(s));
+ if (segNrs.some(isNaN)) {
+ log.error('Invalid segment numbers in resg message: %s', rawSegNrs);
+ return;
+ }
// ...
}Committable suggestion was skipped due to low confidence.
| const db = await DB.setup(ops.dbFile).catch((err) => { | ||
| log.error('error setting up database: %o', err); | ||
| }); | ||
| if (!requestStore) { | ||
| throw new Error('No request store'); | ||
| if (!db) { | ||
| throw new Error('No database'); |
There was a problem hiding this comment.
Improve Error Handling When Setting Up the Database
Currently, if DB.setup(ops.dbFile) fails, the error is logged but not re-thrown, and the code proceeds to check if db is undefined to throw a generic error. This can mask the original error and make debugging more difficult. It's better to re-throw the original error or include its message when throwing the new error to provide clearer error information.
Here's a suggested fix:
async function setup(ops: Ops): Promise<State> {
const db = await DB.setup(ops.dbFile).catch((err) => {
log.error('error setting up database: %o', err);
+ throw err;
});
- if (!db) {
- throw new Error('No database');
- }
await RequestStore.setup(db);
await ResponseSegmentStore.setup(db);
log.verbose('set up DB at', ops.dbFile);
// ...
}Committable suggestion was skipped due to low confidence.
| @@ -644,5 +724,6 @@ | |||
| accessToken: process.env.UHTTP_EA_HOPRD_ACCESS_TOKEN, | |||
| discoveryPlatform, | |||
| dbFile: process.env.UHTTP_EA_DATABASE_FILE, | |||
| pinnedFetch: globalThis.fetch.bind(globalThis), | |||
There was a problem hiding this comment.
Ensure Compatibility of 'globalThis.fetch' Across Node.js Versions
The use of globalThis.fetch may not be supported in Node.js versions prior to 18. If this code is intended to run in environments with older Node.js versions, globalThis.fetch will be undefined, leading to runtime errors.
Consider adding a polyfill or using a library like node-fetch to ensure fetch is available:
+import fetch from 'node-fetch';
start({
privateKey: process.env.UHTTP_EA_PRIVATE_KEY,
publicKey: process.env.UHTTP_EA_PUBLIC_KEY,
// ...
- pinnedFetch: globalThis.fetch.bind(globalThis),
+ pinnedFetch: fetch,
});Alternatively, ensure that the runtime environment is Node.js v18 or later where globalThis.fetch is available.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores
.gitignoreto exclude sensitive files and SQLite database files.