chore: refactor persist API to use same pattern as other services #2658
Conversation
|
Warning Rate limit exceeded@TBonnin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 1 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce new API endpoints for managing records, including functionalities for posting, updating, and deleting records in a specified environment. Validation mechanisms have been implemented for incoming requests using the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant PersistRecords
Client->>Server: POST /environment/:environmentId/records
Server->>PersistRecords: persistRecords(records)
PersistRecords-->>Server: Success/Failure
Server-->>Client: 201 Created / 500 Error
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 (
|
| return recordsService.update({ records, connectionId: nangoConnectionId, model }); | ||
| }; | ||
| break; | ||
| } |
There was a problem hiding this comment.
this function is exactly the same except this switch logic which is setting the persistFunction based on the persistType instead of passing both the type and the function as arguments
| message: 'Array must contain at least 1 element(s)', | ||
| path: ['records'] | ||
| } | ||
| ] |
There was a problem hiding this comment.
the formatting of the errors changed but since the API is not exposed our customers it is not a breaking change
![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, codebase verification and nitpick comments (1)
packages/persist/lib/routes/environment/environmentId/postLog.ts (1)
50-72: Handler function is well-implemented with good error handling.The function handles different scenarios effectively, including message truncation and error responses. Consider adding more detailed logging for debugging and monitoring purposes.
Add more detailed logging to enhance debugging and monitoring capabilities.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (10)
- packages/persist/lib/records.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/deleteRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/postRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/putRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/validate.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/postLog.ts (1 hunks)
- packages/persist/lib/routes/getHealth.ts (1 hunks)
- packages/persist/lib/server.integration.test.ts (8 hunks)
- packages/persist/lib/server.ts (2 hunks)
- packages/persist/package.json (1 hunks)
Additional comments not posted (15)
packages/persist/lib/routes/getHealth.ts (1)
1-18: Well-implemented health check endpoint.The structure and implementation of the health check endpoint are clear and follow best practices for API development with TypeScript.
The code changes are approved.
packages/persist/package.json (1)
28-28: Verify the impact of removingzod-express.The removal of
zod-expressfrom dependencies suggests a change in how validation is handled. Ensure that all existing validation logic has been appropriately refactored to usezoddirectly.Run the following script to verify the impact on validation logic:
Verification successful
Removal of
zod-expressdoes not impact the codebase.The search across JavaScript, TypeScript, and JSON files found no occurrences of
zod-express, indicating that it is not actively used. Its removal should not affect the current validation logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing `zod-express` on validation logic. # Test: Search for the usage of `zod-express`. Expect: No occurrences. rg --type json -A 5 $'zod-express'Length of output: 33
Script:
#!/bin/bash # Description: Search for the usage of `zod-express` in JavaScript and TypeScript files. # Test: Search for the usage of `zod-express`. Expect: No occurrences. rg 'zod-express' --type js --type ts -A 5Length of output: 41
packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/validate.ts (1)
1-28: Robust validation logic implemented.The use of
zodfor defining detailed validation schemas is commendable. The implementation is thorough and aligns with best practices for input validation.The code changes are approved.
packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/putRecords.ts (1)
6-24: Verify the use ofneverfor theSuccesstype.The
Successtype is set tonever, which is unusual for an API endpoint. Please confirm if this is intentional to indicate that the endpoint does not return a success body, only status codes.packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/postRecords.ts (1)
6-24: Verify the use ofneverfor theSuccesstype.The
Successtype is set tonever, which is unusual for an API endpoint. Please confirm if this is intentional to indicate that the endpoint does not return a success body, only status codes.packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/deleteRecords.ts (1)
6-24: Verify the use ofneverfor theSuccesstype.The
Successtype is set tonever, which is unusual for an API endpoint. Please confirm if this is intentional to indicate that the endpoint does not return a success body, only status codes.packages/persist/lib/server.ts (3)
3-10: Imports and setup are correctly configured.The imports are appropriately organized, and the setup of handlers and paths aligns with the refactoring goals.
24-25: Middleware setup is appropriate.The use of
express.json()with specific limits for different routes is a good practice to handle large JSON payloads effectively.
27-31: Routing setup is well-organized and modular.The use of
createRoutefor handling routes is a good practice. It enhances modularity and readability. Ensure that this new routing setup integrates well with existing systems.Run the following script to verify the integration of the new routing setup:
Verification successful
Routing setup is successfully integrated across the codebase.
The
createRoutefunction is consistently used across multiple server modules, confirming a well-organized and modular routing setup. This aligns with the review comment's approval. No issues were found with the integration.
createRouteis defined inpackages/utils/lib/express/route.ts.- Used in
packages/persist/lib/server.ts,packages/orchestrator/lib/server.ts, andpackages/jobs/lib/server.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the new routing setup with existing systems. # Test: Search for the usage of `createRoute`. Expect: Proper integration with existing systems. rg --type python -A 5 $'createRoute'Length of output: 66
Script:
#!/bin/bash # Description: Verify the integration of the new routing setup with existing systems. # Test: Search for the usage of `createRoute` in TypeScript files. Expect: Proper integration with existing systems. rg --type typescript -A 5 $'createRoute'Length of output: 74
Script:
#!/bin/bash # Description: Verify the integration of the new routing setup with existing systems. # Test: Search for the usage of `createRoute` in .ts files. Expect: Proper integration with existing systems. rg --glob '*.ts' -A 5 $'createRoute'Length of output: 5176
packages/persist/lib/routes/environment/environmentId/postLog.ts (1)
30-48: Validation setup is robust and well-defined.The use of
zodfor validating request parameters and body ensures that only well-formed requests are processed. This setup is crucial for maintaining API integrity.packages/persist/lib/server.integration.test.ts (5)
101-112: Refactored error handling approved.The changes to the error handling logic align with the PR's objectives to standardize response formats across services. The test case correctly asserts the new error structure.
130-130: Consistent use ofactivityLogId.The inclusion of
activityLogIdin the request payload across all test cases ensures consistent logging context, which is crucial for traceability and debugging.
157-157: Consistent use ofactivityLogId.The inclusion of
activityLogIdin the request payload across all test cases ensures consistent logging context, which is crucial for traceability and debugging.
183-183: Consistent use ofactivityLogId.The inclusion of
activityLogIdin the request payload across all test cases ensures consistent logging context, which is crucial for traceability and debugging.
Line range hint
212-244: Refactored error handling approved.The changes to the error handling logic align with the PR's objectives to standardize response formats across services. The test case correctly asserts the new error structure.
| const handler = async (req: EndpointRequest<PutRecords>, res: EndpointResponse<PutRecords>) => { | ||
| const { | ||
| params: { environmentId, nangoConnectionId, syncId, syncJobId }, | ||
| body: { model, records, providerConfigKey, connectionId, activityLogId } | ||
| } = req; | ||
| const result = await persistRecords({ | ||
| persistType: 'update', | ||
| environmentId, | ||
| connectionId, | ||
| providerConfigKey, | ||
| nangoConnectionId, | ||
| syncId, | ||
| syncJobId, | ||
| model, | ||
| records, | ||
| activityLogId | ||
| }); | ||
| if (result.isOk()) { | ||
| res.status(201).send(); | ||
| } else { | ||
| res.status(500).json({ error: { code: 'put_records_failed', message: `Failed to save records: ${result.error.message}` } }); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Enhance error handling in the API handler.
The error handling could be improved by logging the error or providing more detailed diagnostics to aid in troubleshooting. Consider adding logging before sending the error response.
+ import { logger } from '@nangohq/utils';
if (result.isOk()) {
res.status(201).send();
} else {
+ logger.error(`Failed to save records: ${result.error.message}`);
res.status(500).json({ error: { code: 'put_records_failed', message: `Failed to save records: ${result.error.message}` } });
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handler = async (req: EndpointRequest<PutRecords>, res: EndpointResponse<PutRecords>) => { | |
| const { | |
| params: { environmentId, nangoConnectionId, syncId, syncJobId }, | |
| body: { model, records, providerConfigKey, connectionId, activityLogId } | |
| } = req; | |
| const result = await persistRecords({ | |
| persistType: 'update', | |
| environmentId, | |
| connectionId, | |
| providerConfigKey, | |
| nangoConnectionId, | |
| syncId, | |
| syncJobId, | |
| model, | |
| records, | |
| activityLogId | |
| }); | |
| if (result.isOk()) { | |
| res.status(201).send(); | |
| } else { | |
| res.status(500).json({ error: { code: 'put_records_failed', message: `Failed to save records: ${result.error.message}` } }); | |
| } | |
| return; | |
| const handler = async (req: EndpointRequest<PutRecords>, res: EndpointResponse<PutRecords>) => { | |
| const { | |
| params: { environmentId, nangoConnectionId, syncId, syncJobId }, | |
| body: { model, records, providerConfigKey, connectionId, activityLogId } | |
| } = req; | |
| const result = await persistRecords({ | |
| persistType: 'update', | |
| environmentId, | |
| connectionId, | |
| providerConfigKey, | |
| nangoConnectionId, | |
| syncId, | |
| syncJobId, | |
| model, | |
| records, | |
| activityLogId | |
| }); | |
| if (result.isOk()) { | |
| res.status(201).send(); | |
| } else { | |
| logger.error(`Failed to save records: ${result.error.message}`); | |
| res.status(500).json({ error: { code: 'put_records_failed', message: `Failed to save records: ${result.error.message}` } }); | |
| } | |
| return; |
| const handler = async (req: EndpointRequest<PostRecords>, res: EndpointResponse<PostRecords>) => { | ||
| const { | ||
| params: { environmentId, nangoConnectionId, syncId, syncJobId }, | ||
| body: { model, records, providerConfigKey, connectionId, activityLogId } | ||
| } = req; | ||
| const result = await persistRecords({ | ||
| persistType: 'save', | ||
| environmentId, | ||
| connectionId, | ||
| providerConfigKey, | ||
| nangoConnectionId, | ||
| syncId, | ||
| syncJobId, | ||
| model, | ||
| records, | ||
| activityLogId | ||
| }); | ||
| if (result.isOk()) { | ||
| res.status(201).send(); | ||
| } else { | ||
| res.status(500).json({ error: { code: 'post_records_failed', message: `Failed to save records: ${result.error.message}` } }); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Enhance error handling in the API handler.
The error handling could be improved by logging the error or providing more detailed diagnostics to aid in troubleshooting. Consider adding logging before sending the error response.
+ import { logger } from '@nangohq/utils';
if (result.isOk()) {
res.status(201).send();
} else {
+ logger.error(`Failed to save records: ${result.error.message}`);
res.status(500).json({ error: { code: 'post_records_failed', message: `Failed to save records: ${result.error.message}` } });
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handler = async (req: EndpointRequest<PostRecords>, res: EndpointResponse<PostRecords>) => { | |
| const { | |
| params: { environmentId, nangoConnectionId, syncId, syncJobId }, | |
| body: { model, records, providerConfigKey, connectionId, activityLogId } | |
| } = req; | |
| const result = await persistRecords({ | |
| persistType: 'save', | |
| environmentId, | |
| connectionId, | |
| providerConfigKey, | |
| nangoConnectionId, | |
| syncId, | |
| syncJobId, | |
| model, | |
| records, | |
| activityLogId | |
| }); | |
| if (result.isOk()) { | |
| res.status(201).send(); | |
| } else { | |
| res.status(500).json({ error: { code: 'post_records_failed', message: `Failed to save records: ${result.error.message}` } }); | |
| } | |
| return; | |
| import { logger } from '@nangohq/utils'; | |
| const handler = async (req: EndpointRequest<PostRecords>, res: EndpointResponse<PostRecords>) => { | |
| const { | |
| params: { environmentId, nangoConnectionId, syncId, syncJobId }, | |
| body: { model, records, providerConfigKey, connectionId, activityLogId } | |
| } = req; | |
| const result = await persistRecords({ | |
| persistType: 'save', | |
| environmentId, | |
| connectionId, | |
| providerConfigKey, | |
| nangoConnectionId, | |
| syncId, | |
| syncJobId, | |
| model, | |
| records, | |
| activityLogId | |
| }); | |
| if (result.isOk()) { | |
| res.status(201).send(); | |
| } else { | |
| logger.error(`Failed to save records: ${result.error.message}`); | |
| res.status(500).json({ error: { code: 'post_records_failed', message: `Failed to save records: ${result.error.message}` } }); | |
| } | |
| return; |
| const handler = async (req: EndpointRequest<DeleteRecords>, res: EndpointResponse<DeleteRecords>) => { | ||
| const { | ||
| params: { environmentId, nangoConnectionId, syncId, syncJobId }, | ||
| body: { model, records, providerConfigKey, connectionId, activityLogId } | ||
| } = req; | ||
| const result = await persistRecords({ | ||
| persistType: 'delete', | ||
| environmentId, | ||
| connectionId, | ||
| providerConfigKey, | ||
| nangoConnectionId, | ||
| syncId, | ||
| syncJobId, | ||
| model, | ||
| records, | ||
| activityLogId | ||
| }); | ||
| if (result.isOk()) { | ||
| res.status(201).send(); | ||
| } else { | ||
| res.status(500).json({ error: { code: 'delete_records_failed', message: `Failed to save records: ${result.error.message}` } }); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Enhance error handling in the API handler.
The error handling could be improved by logging the error or providing more detailed diagnostics to aid in troubleshooting. Consider adding logging before sending the error response.
+ import { logger } from '@nangohq/utils';
if (result.isOk()) {
res.status(201).send();
} else {
+ logger.error(`Failed to save records: ${result.error.message}`);
res.status 500).json({ error: { code: 'delete_records_failed', message: `Failed to save records: ${result.error.message}` } });
}Committable suggestion was skipped due to low confidence.
| providerConfigKey, | ||
| nangoConnectionId, | ||
| syncId, | ||
| syncJobId, | ||
| model, | ||
| activityLogId, | ||
| 'records.count': records.length, | ||
| 'records.sizeInBytes': recordsSizeInBytes | ||
| } | ||
| }); | ||
|
|
||
| let persistFunction: (records: FormattedRecord[]) => Promise<Result<UpsertSummary>>; | ||
| let softDelete: boolean; | ||
| switch (persistType) { | ||
| case 'save': | ||
| softDelete = false; | ||
| persistFunction = async (records: FormattedRecord[]) => recordsService.upsert({ records, connectionId: nangoConnectionId, model, softDelete }); | ||
| break; | ||
| case 'delete': | ||
| softDelete = true; | ||
| persistFunction = async (records: FormattedRecord[]) => recordsService.upsert({ records, connectionId: nangoConnectionId, model, softDelete }); | ||
| break; | ||
| case 'update': | ||
| softDelete = false; | ||
| persistFunction = async (records: FormattedRecord[]) => { | ||
| return recordsService.update({ records, connectionId: nangoConnectionId, model }); | ||
| }; | ||
| break; | ||
| } | ||
|
|
||
| const formatting = recordsFormatter.formatRecords({ | ||
| data: records as UnencryptedRecordData[], | ||
| connectionId: nangoConnectionId, | ||
| model, | ||
| syncId, | ||
| syncJobId, | ||
| softDelete | ||
| }); | ||
| const logCtx = logContextGetter.getStateLess({ id: String(activityLogId) }); | ||
| if (formatting.isErr()) { | ||
| await logCtx.error('There was an issue with the batch', { error: formatting.error, persistType }); | ||
| const err = new Error(`Failed to ${persistType} records ${activityLogId}`); | ||
|
|
||
| span.setTag('error', err).finish(); | ||
| return Err(err); | ||
| } | ||
|
|
||
| const syncConfig = await getSyncConfigByJobId(syncJobId); | ||
| if (syncConfig && !syncConfig?.models.includes(model)) { | ||
| const err = new Error(`The model '${model}' is not included in the declared sync models: ${syncConfig.models}.`); | ||
| await logCtx.error(`The model '${model}' is not included in the declared sync models`); | ||
|
|
||
| span.setTag('error', err).finish(); | ||
| return Err(err); | ||
| } | ||
|
|
||
| const persistResult = await persistFunction(formatting.value); | ||
| if (persistResult.isOk()) { | ||
| const summary = persistResult.value; | ||
| const updatedResults = { | ||
| [model]: { | ||
| added: summary.addedKeys.length, | ||
| updated: summary.updatedKeys.length, | ||
| deleted: summary.deletedKeys?.length || 0 | ||
| } | ||
| }; | ||
| for (const nonUniqueKey of summary.nonUniqueKeys) { | ||
| await logCtx.error(`Found duplicate key '${nonUniqueKey}' for model ${model}. The record was ignored.`); | ||
| } | ||
|
|
||
| await logCtx.info('Batched successfully', { persistType, updatedResults }); | ||
|
|
||
| await updateSyncJobResult(syncJobId, updatedResults, model); | ||
|
|
||
| metrics.increment(metrics.Types.PERSIST_RECORDS_COUNT, records.length); | ||
| metrics.increment(metrics.Types.PERSIST_RECORDS_SIZE_IN_BYTES, recordsSizeInBytes); | ||
|
|
||
| span.finish(); | ||
| return Ok(void 0); | ||
| } else { | ||
| const content = `There was an issue with the batch ${persistType}. ${stringifyError(persistResult.error)}`; | ||
|
|
||
| await logCtx.error('There was an issue with the batch', { error: persistResult.error, persistType }); | ||
|
|
||
| errorManager.report(content, { | ||
| environmentId: environmentId, | ||
| source: ErrorSourceEnum.CUSTOMER, | ||
| operation: LogActionEnum.SYNC, | ||
| metadata: { | ||
| connectionId: connectionId, | ||
| providerConfigKey: providerConfigKey, | ||
| syncId: syncId, | ||
| nangoConnectionId: nangoConnectionId, | ||
| syncJobId: syncJobId | ||
| } | ||
| }); | ||
| span.setTag('error', persistResult.error).finish(); | ||
| return Err(persistResult.error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Function persistRecords is comprehensive but complex.
The function handles various record operations effectively, integrating well with tracing, logging, and metrics. Consider refactoring into smaller, more manageable functions to improve maintainability and readability.
Refactor the persistRecords function into smaller functions to enhance maintainability and readability.
| export const routeHandler: RouteHandler<Health> = { | ||
| path, | ||
| method, | ||
| validate: (_req, _res, next) => next(), // No extra validation needed |
There was a problem hiding this comment.
imo the opposite of validation is not 'no validation' but ensure it's empty
There was a problem hiding this comment.
the emphasis is on the extra but I understand it might be understood differently. will remove the comment that doesn't bring much info anyway
| activityLogId | ||
| }); | ||
| if (result.isOk()) { | ||
| res.status(201).send(); |
There was a problem hiding this comment.
201 is incorrect imo, 204 if you want
(for consistency I'm always leaning toward 200 with content-type json but as you want)
There was a problem hiding this comment.
thanks. I always get 201 and 204 confused, I don't know why
| if (result.isOk()) { | ||
| res.status(201).send(); | ||
| } else { | ||
| res.status(500).json({ error: { code: 'delete_records_failed', message: `Failed to save records: ${result.error.message}` } }); |
There was a problem hiding this comment.
| res.status(500).json({ error: { code: 'delete_records_failed', message: `Failed to save records: ${result.error.message}` } }); | |
| res.status(500).json({ error: { code: 'delete_records_failed', message: `Failed to delete records: ${result.error.message}` } }); |
There was a problem hiding this comment.
nb you could also pass the full error in errors: [result.error]
There was a problem hiding this comment.
not sure I understand what you mean here
There was a problem hiding this comment.
nvm I got it. I will leave it like this for now but I am gonna focus on errors soon so I might reassess then
| parseParams: (data) => | ||
| z | ||
| .object({ | ||
| environmentId: z.string().transform(Number).pipe(z.number().int().positive()) as unknown as z.ZodNumber |
There was a problem hiding this comment.
this envid is not parsed the same as in validate.js that would be great to unify this
TBonnin
force-pushed
the
tbonnin/refactor-persist-api
branch
2 times, most recently
from
baede05
to
9ee461c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (10)
- packages/persist/lib/records.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/deleteRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/postRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/putRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/validate.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/postLog.ts (1 hunks)
- packages/persist/lib/routes/getHealth.ts (1 hunks)
- packages/persist/lib/server.integration.test.ts (9 hunks)
- packages/persist/lib/server.ts (2 hunks)
- packages/persist/package.json (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- packages/persist/lib/records.ts
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/deleteRecords.ts
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/postRecords.ts
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/putRecords.ts
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/validate.ts
- packages/persist/lib/routes/getHealth.ts
- packages/persist/lib/server.integration.test.ts
- packages/persist/lib/server.ts
Additional comments not posted (6)
packages/persist/package.json (1)
28-28: Verify the impact of removing thezod-expressdependency on the codebase.The removal of the
zod-expressdependency suggests a shift away from usingzod-expressfor validation in the Express.js application. Ensure that no dependent features are adversely affected by this change.Run the following script to verify the impact:
Verification successful
Removal of
zod-expressdependency successfully handled.The
zod-expressdependency has been completely removed, andzodis being used directly for validation across the codebase. There are no adverse impacts from this change.
- No remaining imports of
zod-expressfound.zodis widely used for validation in various controllers and utility files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `zod-express` dependency on the codebase. # Test 1: Search for any remaining `zod-express` imports. Expect: No occurrences. rg --type typescript $'from \'zod-express\'' # Test 2: Search for Express.js request validation using `zod`. Expect: Occurrences of `zod` validation without `zod-express`. rg --type typescript -A 5 $'express\.(get|post|put|delete|patch)\(.*\)' | rg $'zod'Length of output: 201
Script:
#!/bin/bash # Description: Verify the impact of removing the `zod-express` dependency on the codebase. # Test 1: Search for any remaining `zod-express` imports. Expect: No occurrences. rg --type ts $'from \'zod-express\'' # Test 2: Search for Express.js request validation using `zod`. Expect: Occurrences of `zod` validation without `zod-express`. rg --type ts -A 5 $'express\.(get|post|put|delete|patch)\(.*\)' | rg $'zod'Length of output: 115
Script:
#!/bin/bash # Description: Search for any usage of `zod` in the codebase to understand its utilization. # Search for any imports of `zod`. rg --type ts $'from \'zod\'' # Search for any usage of `zod` in the codebase. rg --type ts $'zod'Length of output: 34313
packages/persist/lib/routes/environment/environmentId/postLog.ts (5)
1-80: The comment from the previous review is still valid and applicable to the current code. The parsing ofenvironmentIdin this file is different from the parsing invalidate.js.
1-6: LGTM!The code changes are approved.
7-29: LGTM!The code changes are approved.
30-48: LGTM!The code changes are approved.
50-80: LGTM!The code changes are approved.
TBonnin
force-pushed
the
tbonnin/refactor-persist-api
branch
from
9ee461c
to
736e70f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/deleteRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/postRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/putRecords.ts (1 hunks)
- packages/persist/lib/routes/environment/environmentId/postLog.ts (1 hunks)
- packages/persist/lib/routes/getHealth.ts (1 hunks)
- packages/persist/lib/server.integration.test.ts (9 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/deleteRecords.ts
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/postRecords.ts
- packages/persist/lib/routes/environment/environmentId/connection/connectionId/sync/syncId/job/jobId/putRecords.ts
- packages/persist/lib/routes/getHealth.ts
- packages/persist/lib/server.integration.test.ts
Additional comments not posted (4)
packages/persist/lib/routes/environment/environmentId/postLog.ts (4)
1-6: LGTM!The code segment is correctly importing the required dependencies.
7-48: LGTM!The code segment is correctly defining the types and validation for the API endpoint.
50-72: LGTM!The code segment is correctly implementing the logic for the API endpoint.
74-80: LGTM!The code segment is correctly exporting the
routeandrouteHandlerobjects for the API endpoint.
TBonnin
force-pushed
the
tbonnin/refactor-persist-api
branch
from
736e70f
to
524f426
Compare
## Describe your changes Following up on #2658 Please review this one first Adding support for passing full log message to persistAPI. We are still sending legacy simplified logs for now. ## Issue ticket number and link https://linear.app/nango/issue/NAN-1525/improve-actions-error-logs ## Checklist before requesting a review (skip if just adding/editing APIs & templates) - [ ] I added tests, otherwise the reason is: - [ ] I added observability, otherwise the reason is: - [ ] I added analytics, otherwise the reason is: <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced logging functionality with support for a new log structure, allowing for more detailed log entries. - Introduced a new interface for improved request body handling in the logging system. - **Bug Fixes** - Updated error messages for deletion and update operations to provide clearer feedback to users. - **Documentation** - Improved clarity in error handling responses to better reflect the nature of failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Describe your changes
I am about to modify the persist API to support full message log instead of just a string like it is today.
While this refactoring is not absolutely required I thought it would be a good idea to do it now and have persist align with how other service APIs are implemented.
I am trying to keep PR small so this first one is just refactoring without changing the behavior.
Checklist before requesting a review (skip if just adding/editing APIs & templates)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores