-
Notifications
You must be signed in to change notification settings - Fork 6
[v6 PROD RELEASE] - dev -> master #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Expose phaseId for defaultReviewer
expose fixedAmount
…rms in a PUT / PATCH
…type phases to ensure they can't be manually opened without at least 1 resource assigned to the review type role associated with the phase type.
…ure no appeals are pending
fix(PM-2539): Added timeout to prisma client
| jobs: | ||
| trivy-scan: | ||
| name: Use Trivy | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
Consider using ubuntu-latest instead of a specific version like ubuntu-24.04 to ensure the workflow uses the most up-to-date and supported version of the runner. This can help avoid issues with deprecated or unsupported versions in the future.
| output: "trivy-results.sarif" | ||
| severity: "CRITICAL,HIGH,UNKNOWN" | ||
| scanners: vuln,secret,misconfig,license | ||
| github-pat: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[security]
Ensure that the GITHUB_TOKEN secret has the necessary permissions for the actions being performed. While the default permissions should suffice, it's important to verify this to avoid unexpected permission issues.
| // Database schemas for direct counts (shared DB) | ||
| RESOURCES_DB_SCHEMA: process.env.RESOURCES_DB_SCHEMA || "resources", | ||
| REVIEW_DB_SCHEMA: process.env.REVIEW_DB_SCHEMA || "reviews", | ||
| CHALLENGE_SERVICE_PRISMA_TIMEOUT: process.env.CHALLENGE_SERVICE_PRISMA_TIMEOUT ? parseInt(process.env.CHALLENGE_SERVICE_PRISMA_TIMEOUT, 10) : 10000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Consider using Number() instead of parseInt() for parsing CHALLENGE_SERVICE_PRISMA_TIMEOUT. Number() will convert the entire string to a number, while parseInt() stops at the first non-numeric character, which might lead to unexpected results if the environment variable contains non-numeric characters.
| @@ -0,0 +1,29 @@ | |||
| -- CreateIndex | |||
| CREATE INDEX "Challenge_status_startDate_idx" ON "Challenge"("status", "startDate"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Consider the selectivity of the status and startDate columns when creating this index. If status has low cardinality, this index might not be as effective as expected.
| CREATE INDEX "Challenge_status_startDate_idx" ON "Challenge"("status", "startDate"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "Challenge_trackId_typeId_status_idx" ON "Challenge"("trackId", "typeId", "status"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Ensure that the order of columns in this index (trackId, typeId, status) aligns with the most common query patterns. The order can significantly impact the index's effectiveness.
| CREATE INDEX "Challenge_legacyId_idx" ON "Challenge"("legacyId"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "Challenge_projectId_status_idx" ON "Challenge"("projectId", "status"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Verify that the projectId and status columns are frequently queried together. Otherwise, this index might not provide the expected performance benefits.
| CREATE INDEX "Challenge_projectId_status_idx" ON "Challenge"("projectId", "status"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "ChallengePhase_challengeId_isOpen_idx" ON "ChallengePhase"("challengeId", "isOpen"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Consider the impact of indexing the isOpen column if it has low cardinality. This might not be beneficial if the column has few distinct values.
| CREATE INDEX "ChallengeReviewer_challengeId_phaseId_idx" ON "ChallengeReviewer"("challengeId", "phaseId"); | ||
|
|
||
| -- CreateIndex | ||
| CREATE INDEX "ChallengeWinner_challengeId_type_placement_idx" ON "ChallengeWinner"("challengeId", "type", "placement"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Check if the placement column is used in range queries or sorting operations. If not, its inclusion in the index might not be necessary.
| transactionOptions: { | ||
| maxWait: Number(process.env.PRISMA_TRANSACTION_MAX_WAIT_MS || 10000), // wait up to 10s to start | ||
| timeout: Number(process.env.PRISMA_TRANSACTION_TIMEOUT_MS || 10000), // allow up to 30s per transaction | ||
| timeout: config.CHALLENGE_SERVICE_PRISMA_TIMEOUT, // allow up to 30s per transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The change from an environment variable to a config value for timeout means that the timeout is now controlled by the config module rather than environment variables. Ensure that config.CHALLENGE_SERVICE_PRISMA_TIMEOUT is correctly set and documented, as this could impact transaction behavior if not properly configured.
| transactionOptions: { | ||
| maxWait: Number(process.env.PRISMA_TRANSACTION_MAX_WAIT_MS || 10000), | ||
| timeout: Number(process.env.PRISMA_TRANSACTION_TIMEOUT_MS || 10000), | ||
| timeout: config.CHALLENGE_SERVICE_PRISMA_TIMEOUT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
The timeout value is now being set from config.CHALLENGE_SERVICE_PRISMA_TIMEOUT. Ensure that this configuration value is validated and defaults are handled appropriately elsewhere in the codebase. This change assumes that config.CHALLENGE_SERVICE_PRISMA_TIMEOUT is always set and valid, which could lead to runtime errors if not properly managed.
| * @param {Object} res the response | ||
| */ | ||
| async function searchDefaultChallengeReviewers(req, res) { | ||
| const result = await service.searchDefaultChallengeReviewers(req.query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Consider adding error handling for the service.searchDefaultChallengeReviewers call to manage potential exceptions and provide meaningful error responses.
| * @param {Object} res the response | ||
| */ | ||
| async function createDefaultChallengeReviewer(req, res) { | ||
| const result = await service.createDefaultChallengeReviewer(req.authUser, req.body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Consider adding error handling for the service.createDefaultChallengeReviewer call to manage potential exceptions and provide meaningful error responses.
| * @param {Object} res the response | ||
| */ | ||
| async function getDefaultChallengeReviewer(req, res) { | ||
| const result = await service.getDefaultChallengeReviewer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Consider adding error handling for the service.getDefaultChallengeReviewer call to manage potential exceptions and provide meaningful error responses.
| * @param {Object} res the response | ||
| */ | ||
| async function fullyUpdateDefaultChallengeReviewer(req, res) { | ||
| const result = await service.fullyUpdateDefaultChallengeReviewer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Consider adding error handling for the service.fullyUpdateDefaultChallengeReviewer call to manage potential exceptions and provide meaningful error responses.
| * @param {Object} res the response | ||
| */ | ||
| async function partiallyUpdateDefaultChallengeReviewer(req, res) { | ||
| const result = await service.partiallyUpdateDefaultChallengeReviewer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Consider adding error handling for the service.partiallyUpdateDefaultChallengeReviewer call to manage potential exceptions and provide meaningful error responses.
| * @param {Object} res the response | ||
| */ | ||
| async function deleteDefaultChallengeReviewer(req, res) { | ||
| const result = await service.deleteDefaultChallengeReviewer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Consider adding error handling for the service.deleteDefaultChallengeReviewer call to manage potential exceptions and provide meaningful error responses.
| controller: "DefaultChallengeReviewerController", | ||
| method: "searchDefaultChallengeReviewers", | ||
| auth: "jwt", | ||
| access: [constants.UserRoles.Admin], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
Consider adding more roles to the access list for the /default-challenge-reviewers endpoint if other roles besides Admin should have access. This will enhance flexibility and maintainability if access requirements change in the future.
| controller: "DefaultChallengeReviewerController", | ||
| method: "getDefaultChallengeReviewer", | ||
| auth: "jwt", | ||
| access: [constants.UserRoles.Admin], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[security]
The access list for the /default-challenge-reviewers/:defaultChallengeReviewerId endpoint is limited to Admin. Ensure this aligns with the intended access control policy, as it might restrict legitimate users from accessing this resource.
| let rows; | ||
| try { | ||
| rows = await reviewPrisma.$queryRaw( | ||
| Prisma.sql` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The SQL query uses LEFT JOIN on appealResponseTable but then checks ar."id" IS NULL. This pattern is correct for finding appeals without responses, but ensure that this behavior is intentional and aligns with business logic.
|
|
||
| async function ensureRequiredResourcesBeforeOpeningPhase(challengeId, phaseName) { | ||
| const normalizedPhaseName = _.toLower(_.trim(phaseName || "")); | ||
| const requiredRoleName = PHASE_RESOURCE_ROLE_REQUIREMENTS[normalizedPhaseName]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The PHASE_RESOURCE_ROLE_REQUIREMENTS object is accessed using a normalized phase name. Ensure that all possible phase names are covered in this object to avoid potential undefined values.
| ); | ||
| } | ||
| if ( | ||
| String(closingPhaseName || "").toLowerCase() === "appeals response" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The check for closingPhaseName being 'appeals response' is case-sensitive. Consider normalizing closingPhaseName to avoid potential issues with case variations.
| const perPage = criteria.perPage || 50; | ||
|
|
||
| const [total, rows] = await Promise.all([ | ||
| prisma.defaultChallengeReviewer.count({ where: searchFilter }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Consider adding error handling for the prisma.defaultChallengeReviewer.count and prisma.defaultChallengeReviewer.findMany calls to handle potential database errors gracefully.
| payload.createdBy = userId; | ||
| payload.updatedBy = userId; | ||
|
|
||
| const duplicate = await prisma.defaultChallengeReviewer.findFirst({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The findFirst method is used here to check for duplicates. Ensure that the combination of fields used in the where clause is sufficient to uniquely identify a duplicate. Otherwise, consider using a more specific query or adding a unique constraint at the database level.
| * @returns {Promise<Object>} updated record | ||
| */ | ||
| async function fullyUpdateDefaultChallengeReviewer(authUser, id, data) { | ||
| await getDefaultChallengeReviewer(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Consider handling the case where getDefaultChallengeReviewer(id) might throw a NotFoundError. This could be done by catching the error and returning a more user-friendly message or status code.
| * @returns {Promise<Object>} deleted record | ||
| */ | ||
| async function deleteDefaultChallengeReviewer(id) { | ||
| const existing = await prisma.defaultChallengeReviewer.findUnique({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The deleteDefaultChallengeReviewer function does not handle potential errors from the prisma.defaultChallengeReviewer.delete call. Consider adding error handling to manage database errors gracefully.
| "reviewItemCommentId" varchar(14) NOT NULL | ||
| ) | ||
| `) | ||
| await reviewClient.$executeRawUnsafe(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ security]
Using $executeRawUnsafe can be risky as it may expose the application to SQL injection vulnerabilities. Consider using parameterized queries or $executeRaw with proper parameter binding to mitigate this risk.
|
|
||
| after(async () => { | ||
| if (reviewClient) { | ||
| await reviewClient.$executeRawUnsafe(`TRUNCATE TABLE "${reviewSchema}"."appealResponse"`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ security]
Using $executeRawUnsafe for truncating tables can be risky if the table names are derived from user input or untrusted sources. Ensure that reviewSchema is sanitized and validated to prevent SQL injection.
| } | ||
| }) | ||
|
|
||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[security]
Using $executeRaw with Prisma.sql for inserting data is safer than $executeRawUnsafe, but ensure that all variables used in the query are properly sanitized and validated to prevent SQL injection.
| "Appeals Response phase can't be closed because there are still appeals that haven't been responded to" | ||
| ) | ||
| } finally { | ||
| await reviewClient.$executeRaw( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
Ensure that the cleanup operations in the finally block are robust and handle any potential errors that might occur during the deletion of records. Consider wrapping each delete operation in a try-catch block to prevent partial cleanup in case of an error.
| { id: 'reviewer-role-id', name: 'Reviewer' } | ||
| ]) | ||
|
|
||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[design]
The test case modifies global helper functions (getChallengeResources and getResourceRoles) which could lead to side effects in other tests. Consider using a mocking library to temporarily override these functions within the test scope.
| { id: 'reviewer-role-id', name: 'Reviewer' } | ||
| ]) | ||
|
|
||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[design]
The test case modifies global helper functions (getChallengeResources and getResourceRoles) which could lead to side effects in other tests. Consider using a mocking library to temporarily override these functions within the test scope.
|
|
||
| it('preserves existing terms when update payload omits the terms field', async () => { | ||
| const challengeData = _.cloneDeep(testChallengeData) | ||
| challengeData.name = `${challengeData.name} Terms ${Date.now()}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Using Date.now() for generating a unique name is not reliable for uniqueness due to potential race conditions. Consider using a UUID or a more robust unique identifier.
| updatedBy: 'unit-test' | ||
| } | ||
| ] | ||
| await prisma.challengeTerm.createMany({ data: termRecords }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Ensure that prisma.challengeTerm.createMany handles potential errors, such as database constraints or connectivity issues, to prevent unhandled promise rejections.
| ) | ||
| sortedTerms.should.deep.equal(expectedTerms) | ||
| } finally { | ||
| await prisma.challenge.delete({ where: { id: challengeWithTerms.id } }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
Consider using a transaction to ensure that both the creation of terms and the deletion of the challenge are atomic. This will prevent partial updates if an error occurs.
| return knownFields; | ||
| }; | ||
|
|
||
| const validateIncrementalFieldConfiguration = (config) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[design]
The function validateIncrementalFieldConfiguration directly logs warnings and errors to the console. Consider refactoring to allow the caller to handle logging, which would improve testability and flexibility in handling different logging mechanisms.
| * Works in tandem with INCREMENTAL_SINCE_DATE to scope the incremental window. | ||
| */ | ||
| INCREMENTAL_FIELDS: parseListEnv(process.env.INCREMENTAL_FIELDS), | ||
| INCREMENTAL_DATE_FIELDS: parseListEnv(process.env.INCREMENTAL_DATE_FIELDS) || ['updatedAt', 'updated'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The default value for INCREMENTAL_DATE_FIELDS is set to ['updatedAt', 'updated'] if the environment variable is not provided. Ensure that these default fields are appropriate for all use cases, as hardcoding defaults can lead to unexpected behavior if the schema changes.
| }, | ||
| }; | ||
|
|
||
| config.incrementalFieldValidation = validateIncrementalFieldConfiguration(config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
Assigning the result of validateIncrementalFieldConfiguration to config.incrementalFieldValidation might lead to unexpected behavior if the configuration is modified after this assignment. Consider making this validation a function that can be called to get the latest validation results.
| } else { | ||
| const now = new Date(); | ||
| if (sinceDateObj > now) { | ||
| this.manager.logger.error(`${this.modelName} incremental run date ${sinceDateObj.toISOString()} is in the future; this may yield zero records.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The check for sinceDateObj > now could lead to unexpected behavior if the system clock is incorrect. Consider logging a warning if the system time is significantly different from a reliable time source.
| if (sinceDateObj > now) { | ||
| this.manager.logger.error(`${this.modelName} incremental run date ${sinceDateObj.toISOString()} is in the future; this may yield zero records.`); | ||
| } | ||
| const oneYearMs = 365 * 24 * 60 * 60 * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 correctness]
The calculation for oneYearMs assumes a year has 365 days, which is not accurate for leap years. Consider using a more precise method to calculate the difference in years.
| } | ||
| } | ||
|
|
||
| async _executeUpsertWithStats(prisma, upsertData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The _executeUpsertWithStats method includes a try-catch block for findUnique but not for upsert. Consider adding error handling for the upsert operation to ensure any issues are logged and managed appropriately.
| const start = Date.now(); | ||
| let existing = null; | ||
| if (this.collectUpsertStats) { | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 readability]
The collectUpsertStats flag is checked multiple times within _executeUpsertWithStats. Consider consolidating these checks to improve readability and reduce redundancy.
| } catch (error) { | ||
| const resolution = await this.handleUpsertError(error, upsertData); | ||
|
|
||
| if (resolution?.retryData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
In performUpsert, the retry logic does not handle the case where resolution.retryData is malformed or causes another type of error. Consider adding validation or additional error handling for retryData.
| const config = require('../config'); | ||
| const isDebugLog = (config.LOG_VERBOSITY || '').toLowerCase() === 'debug'; | ||
| const summaryLimit = Number.isFinite(config.SUMMARY_LOG_LIMIT) | ||
| ? Math.max(0, config.SUMMARY_LOG_LIMIT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
Using Math.max(0, config.SUMMARY_LOG_LIMIT) ensures that summaryLimit is non-negative, but it might be more intuitive to handle non-numeric values explicitly before this check. Consider validating config.SUMMARY_LOG_LIMIT separately to ensure it's a number before applying Math.max.
| }; | ||
|
|
||
| const parseBehavior = (behavior) => { | ||
| const normalized = (behavior || '').toLowerCase().trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
The parseBehavior function uses a default strategy of 'skip' for unrecognized behavior strings. Consider logging a warning when an unrecognized behavior is encountered to aid in debugging and configuration validation.
| outcome.usedField = usedField; | ||
| const recordDateValue = record[usedField]; | ||
| const parsedDate = new Date(recordDateValue); | ||
| if (Number.isNaN(parsedDate.getTime())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The new Date(recordDateValue) conversion does not handle invalid date formats robustly. Consider using a library like date-fns or moment for more reliable date parsing and validation, especially if date formats can vary.
| const nowTimestamp = Date.now(); | ||
| let suspiciousReason = null; | ||
|
|
||
| if (parsedDate.getTime() > nowTimestamp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 design]
The check for future dates (parsedDate.getTime() > nowTimestamp) assumes that any future date is suspicious. If future dates are valid in some contexts, consider making this behavior configurable.
| } | ||
| } | ||
|
|
||
| if (parsedDate.getFullYear() < 2000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 design]
The check for 'ancient' dates (parsedDate.getFullYear() < 2000) is hardcoded. Consider making the threshold year configurable to accommodate different use cases or datasets.
| // For Elasticsearch format (line-delimited JSON) | ||
| const results = []; | ||
| const fileStream = fs.createReadStream(filePath, { | ||
| encoding: 'utf8', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 performance]
The highWaterMark option for fs.createReadStream is set to 1MB. Ensure this buffer size is appropriate for the expected file sizes and system memory constraints to avoid potential performance issues.
|
|
||
| if (suspiciousReason) { | ||
| if (invalidDateBehavior.warn) { | ||
| console.warn(`${fileName}: record ${recordIdentifier} has ${suspiciousReason.replace('|', ' & ')} (${parsedDate.toISOString()}); strategy=${invalidDateBehavior.strategy}`); |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
The best way to fix this problem is to ensure that all occurrences of the pipe character (|) in the suspiciousReason string are replaced with " & ". This can be achieved by using a regular expression with the global flag (/g), specifically .replace(/\|/g, ' & '), so that every pipe in the string is replaced—not just the first. Only line 178 needs to be changed, and no additional imports or method definitions are required.
-
Copy modified line R178
| @@ -175,7 +175,7 @@ | ||
|
|
||
| if (suspiciousReason) { | ||
| if (invalidDateBehavior.warn) { | ||
| console.warn(`${fileName}: record ${recordIdentifier} has ${suspiciousReason.replace('|', ' & ')} (${parsedDate.toISOString()}); strategy=${invalidDateBehavior.strategy}`); | ||
| console.warn(`${fileName}: record ${recordIdentifier} has ${suspiciousReason.replace(/\|/g, ' & ')} (${parsedDate.toISOString()}); strategy=${invalidDateBehavior.strategy}`); | ||
| } | ||
| if (!invalidDateBehavior.include) { | ||
| invalidDateSkippedCount += 1; |
Additional indices for performance issues noted
| @@ -0,0 +1,13 @@ | |||
| -- Add indexes to support faster `/v6/my-reviews` queries. | |||
|
|
|||
| CREATE EXTENSION IF NOT EXISTS pg_trgm; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ correctness]
Ensure that the pg_trgm extension is supported and enabled in all environments where this migration will be applied. This extension is not enabled by default in all PostgreSQL installations and could lead to deployment issues.
| ON "Challenge" ("status", "typeId", "trackId", "createdAt" DESC); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS "challenge_phase_challenge_open_end_idx" | ||
| ON "ChallengePhase" ("challengeId", "isOpen", "scheduledEndDate", "actualEndDate"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Consider the order of columns in the challenge_phase_challenge_open_end_idx index. If queries often filter by isOpen first, it might be beneficial to place isOpen as the first column in the index for better performance.
| @@index([endDate]) | ||
| @@index([status, startDate]) | ||
| @@index([trackId, typeId, status]) | ||
| @@index([status, typeId, trackId, createdAt(sort: Desc)], map: "challenge_status_type_track_created_at_idx") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
The index challenge_status_type_track_created_at_idx includes createdAt with a descending sort order. Ensure that this aligns with the query patterns you expect to optimize, as descending order can impact index usage and performance.
prisma/schema.prisma
Outdated
| @@index([status, startDate]) | ||
| @@index([trackId, typeId, status]) | ||
| @@index([status, typeId, trackId, createdAt(sort: Desc)], map: "challenge_status_type_track_created_at_idx") | ||
| @@index([name], type: Gin, ops: [gin_trgm_ops], map: "challenge_name_trgm_idx") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
The use of a Gin index with gin_trgm_ops on the name field is appropriate for text search. However, consider the potential impact on write performance due to the overhead of maintaining this index, especially if the name field is frequently updated.
| @@index([challengeId]) | ||
| @@index([challengeId, isOpen]) | ||
| @@index([challengeId, name]) | ||
| @@index([challengeId, isOpen, scheduledEndDate, actualEndDate], map: "challenge_phase_challenge_open_end_idx") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
The index challenge_phase_challenge_open_end_idx includes both scheduledEndDate and actualEndDate. Ensure that queries requiring both fields are common enough to justify the composite index, as maintaining it could affect performance.
Fix up extension creation
| generator client { | ||
| provider = "prisma-client-js" | ||
| previewFeatures = ["fullTextSearchPostgres"] | ||
| previewFeatures = ["fullTextSearchPostgres", "postgresqlExtensions"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
Adding postgresqlExtensions to previewFeatures could introduce instability if the feature is not yet fully supported or stable. Ensure that this feature is necessary and that any potential issues are documented and tested.
| @@index([status, startDate]) | ||
| @@index([trackId, typeId, status]) | ||
| @@index([status, typeId, trackId, createdAt(sort: Desc)], map: "challenge_status_type_track_created_at_idx") | ||
| @@index([name(ops: raw("pg_catalog.gin_trgm_ops"))], type: Gin, map: "challenge_name_trgm_idx") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
The use of raw("pg_catalog.gin_trgm_ops") in the index definition is technically correct, but it introduces raw SQL into the Prisma schema. Ensure that this is well-documented and understood by the team, as it may affect portability and maintainability of the schema.
No description provided.