chore(PM-2539): added timeout for prisma service #128
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| constructor() { | ||
| super({ | ||
| transactionOptions: { | ||
| timeout: process.env.REVIEW_SERVICE_PRISMA_TIMEOUT |
There was a problem hiding this comment.
[❗❗ correctness]
Consider validating the environment variable REVIEW_SERVICE_PRISMA_TIMEOUT before parsing it. If it contains non-numeric values, parseInt will return NaN, which could lead to unexpected behavior. You might want to add a check to ensure it's a valid number.
|
|
||
| super({ | ||
| transactionOptions: { | ||
| timeout: process.env.REVIEW_SERVICE_PRISMA_TIMEOUT |
There was a problem hiding this comment.
[correctness]
Consider validating the environment variable REVIEW_SERVICE_PRISMA_TIMEOUT to ensure it is a valid integer before parsing. This can prevent potential runtime errors if the environment variable is set to a non-numeric value.
| constructor() { | ||
| super({ | ||
| transactionOptions: { | ||
| timeout: process.env.REVIEW_SERVICE_PRISMA_TIMEOUT |
There was a problem hiding this comment.
[correctness]
Using parseInt without specifying the radix can lead to unexpected results if the environment variable contains a non-decimal number. Although the radix is specified here, ensure that the environment variable is always a valid integer to avoid potential issues.
| constructor() { | ||
| super({ | ||
| transactionOptions: { | ||
| timeout: process.env.REVIEW_SERVICE_PRISMA_TIMEOUT |
There was a problem hiding this comment.
[security]
Consider validating the environment variable REVIEW_SERVICE_PRISMA_TIMEOUT to ensure it is a positive integer. This will prevent potential misconfigurations from causing unexpected behavior.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
@hentrymartin can we refactor the PR to extract the parsing and add validation login to a utility method please? We repeat the same checks more than 2 times so it's more manageble and flexible for updates.
|
|
||
| constructor() { | ||
| super({ | ||
| ...Utils.getPrismaTimeout(), |
There was a problem hiding this comment.
[❗❗ correctness]
The change replaces direct environment variable parsing with a utility function Utils.getPrismaTimeout(). Ensure that this utility function handles errors or edge cases, such as missing environment variables or invalid values, to prevent potential runtime issues.
|
|
||
| constructor() { | ||
| super({ | ||
| ...Utils.getPrismaTimeout(), |
There was a problem hiding this comment.
[correctness]
The Utils.getPrismaTimeout() function should be reviewed to ensure it handles environment variables and defaults correctly, similar to the previous implementation. Ensure it provides a clear error or fallback if the environment variable is not set or is invalid.
| import { LoggerService } from './logger.service'; | ||
| import { PrismaErrorService } from './prisma-error.service'; | ||
| import { getStore } from 'src/shared/request/requestStore'; | ||
| import { Utils } from './utils.service'; |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that Utils.getPrismaTimeout() is correctly handling environment variables and defaults. If getPrismaTimeout() is not properly defined, it could lead to unexpected behavior or errors.
|
|
||
| constructor() { | ||
| super({ | ||
| ...Utils.getPrismaTimeout(), |
There was a problem hiding this comment.
[❗❗ correctness]
The Utils.getPrismaTimeout() function is used here, but it's unclear if it handles the case where process.env.REVIEW_SERVICE_PRISMA_TIMEOUT is not set. Ensure that this function provides a default timeout value to prevent potential runtime errors.
@kkartunov I've extracted it to a util method now! |
There was a problem hiding this comment.
@hentrymartin looks good. Please fix the lint error before merging this one.
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-2539