[PROD RELEASE] - Updates & fixes #42
Conversation
…ing or encoding Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Potential fix for code scanning alert no. 72: Incomplete string escaping or encoding
PM-2614 - Module updates -> dev
drop yarn -> develop
PS-441 Fix for processing phases in challenge update
…6 into PM-2206-2nd-fix
Master -> develop sync
Automate migrations
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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}`); |
There was a problem hiding this comment.
[correctness]
The change from suspiciousReason.replace('|', ' & ') to suspiciousReason.replace(/\|/g, ' & ') is correct as it ensures all occurrences of the pipe character are replaced. However, consider verifying that suspiciousReason is always a string to avoid potential runtime errors if it is unexpectedly not a string.
|
|
||
| CMD ["node","/challenge-api/app.js"] | ||
| # Copy entrypoint script and make it executable | ||
| COPY docker/entrypoint.sh /entrypoint.sh |
There was a problem hiding this comment.
[security]
Consider verifying the integrity of entrypoint.sh after copying it into the image. This can help ensure that the script has not been tampered with and is safe to execute.
|
|
||
| # Use entrypoint to run migrations at startup (not build time) | ||
| # Prisma uses PostgreSQL advisory locks to prevent concurrent migrations | ||
| ENTRYPOINT ["/entrypoint.sh"] |
There was a problem hiding this comment.
[correctness]
Using an entrypoint script to run migrations at startup can be risky if the script fails or if the database is not ready. Consider adding error handling or retry logic within entrypoint.sh to handle such cases gracefully.
| # Prisma uses PostgreSQL advisory locks to prevent concurrent migrations | ||
| # Only one instance will run migrations, others will wait | ||
| echo "Running database migrations..." | ||
| npx prisma migrate deploy |
There was a problem hiding this comment.
[maintainability]
Consider adding a retry mechanism or handling specific errors for npx prisma migrate deploy to improve robustness in case of transient issues with the database connection.
|
|
||
| # Start the application | ||
| echo "Starting application server..." | ||
| exec node /challenge-api/app.js |
There was a problem hiding this comment.
[correctness]
Using exec to start the application server is good for replacing the shell process with the node process, but ensure that /challenge-api/app.js handles termination signals properly to allow graceful shutdowns.
|
|
||
| // Convert the readable stream to buffer | ||
| const chunks = []; | ||
| for await (const chunk of response.Body) { |
There was a problem hiding this comment.
[❗❗ correctness]
The response.Body is expected to be a readable stream, but the code assumes it can be directly iterated over with for await. Ensure that response.Body is indeed a stream and compatible with this iteration method, as this could lead to runtime errors if the assumption is incorrect.
| async function deleteFromS3(bucket, key) { | ||
| return getS3().deleteObject({ Bucket: bucket, Key: key }).promise(); | ||
| const command = new DeleteObjectCommand({ Bucket: bucket, Key: key }); | ||
| return s3Client.send(command); |
There was a problem hiding this comment.
[correctness]
Ensure that the s3Client.send(command) method handles errors appropriately, as it may throw exceptions if the operation fails. Consider adding error handling to manage potential failures gracefully.
|
|
||
| // Convert plain object schema to Joi schema if needed | ||
| let schema = method.schema; | ||
| if (schema && !schema.validate && typeof schema === 'object') { |
There was a problem hiding this comment.
[correctness]
The check !schema.validate assumes that the schema is a plain object if it lacks a validate method. This could lead to incorrect assumptions if the schema is an instance of another class or object that doesn't have validate. Consider explicitly checking for plain object types using _.isPlainObject(schema) from lodash for better reliability.
| abbreviation: Joi.string(), | ||
| legacyId: Joi.number().integer().positive(), | ||
| track: Joi.string().valid(_.values(ChallengeTrackEnum)), | ||
| track: Joi.string().valid(..._.values(ChallengeTrackEnum)), |
There was a problem hiding this comment.
[correctness]
Using the spread operator (...) with _.values(ChallengeTrackEnum) is correct, but ensure that ChallengeTrackEnum is an object with enumerable properties. If ChallengeTrackEnum is not an object or has non-enumerable properties, this change might not work as expected.
| abbreviation: Joi.string().required(), | ||
| legacyId: Joi.number().integer().positive(), | ||
| track: Joi.string().valid(_.values(ChallengeTrackEnum)), | ||
| track: Joi.string().valid(..._.values(ChallengeTrackEnum)), |
There was a problem hiding this comment.
[correctness]
Ensure ChallengeTrackEnum is an object with enumerable properties when using the spread operator (...). If it is not, this change might not function correctly.
| abbreviation: Joi.string().required(), | ||
| legacyId: Joi.number().integer().positive(), | ||
| track: Joi.string().valid(_.values(ChallengeTrackEnum)), | ||
| track: Joi.string().valid(..._.values(ChallengeTrackEnum)), |
There was a problem hiding this comment.
[correctness]
Verify that ChallengeTrackEnum is an object with enumerable properties when using the spread operator (...). If it isn't, this change may not behave as intended.
| abbreviation: Joi.string(), | ||
| legacyId: Joi.number().integer().positive(), | ||
| track: Joi.string().valid(_.values(ChallengeTrackEnum)), | ||
| track: Joi.string().valid(..._.values(ChallengeTrackEnum)), |
There was a problem hiding this comment.
[correctness]
Check that ChallengeTrackEnum is an object with enumerable properties when using the spread operator (...). If it is not, this change could lead to unexpected behavior.
| secretAccessKey: config.AMAZON.AWS_SECRET_ACCESS_KEY | ||
| }, | ||
| forcePathStyle: true, | ||
| tls: false |
There was a problem hiding this comment.
[❗❗ security]
Disabling TLS (tls: false) may expose data to security risks. Ensure this is intentional and appropriate for the testing environment.
|
|
||
| try { | ||
| await s3.headBucket({ Bucket: config.AMAZON.ATTACHMENT_S3_BUCKET }).promise() | ||
| await s3.send(new HeadBucketCommand({ Bucket: config.AMAZON.ATTACHMENT_S3_BUCKET })) |
There was a problem hiding this comment.
[maintainability]
Consider logging the error in the catch block to aid in debugging and understanding why the bucket creation was necessary.
| awsMock.mock('S3', 'getObject', (params, callback) => { | ||
| callback(null, { Body: Buffer.from(attachmentContent) }); | ||
| // mock S3 GetObject command | ||
| s3Mock.on(GetObjectCommand).resolves({ |
There was a problem hiding this comment.
[correctness]
Using Readable.from([attachmentContent]) to create a stream from a buffer is technically correct, but ensure that attachmentContent is always a buffer and not accidentally a string or other type. Consider adding validation for attachmentContent to prevent runtime errors.
…es request (PM-2206)
Migration history fix
| LIMIT 1; | ||
|
|
||
| IF idx_schema IS NOT NULL THEN | ||
| EXECUTE format('DROP INDEX %I.%I', idx_schema, 'challenge_phase_order_idx'); |
There was a problem hiding this comment.
[maintainability]
Consider adding a check to ensure that the index exists before attempting to drop it. Although the current logic handles the case where idx_schema is NULL, explicitly checking for the existence of the index might prevent unexpected errors if the index is dropped or renamed outside of this script.
| RETURN; | ||
| END IF; | ||
|
|
||
| EXECUTE format( |
There was a problem hiding this comment.
[correctness]
The ALTER TABLE operation assumes that the column isAIReviewer exists. Consider adding a check to ensure the column exists before attempting to alter it. This will prevent runtime errors if the column is missing or renamed.
| generator client { | ||
| provider = "prisma-client-js" | ||
| previewFeatures = ["fullTextSearchPostgres", "postgresqlExtensions"] | ||
| previewFeatures = ["fullTextSearchPostgres", "postgresqlExtensions", "views"] |
There was a problem hiding this comment.
[maintainability]
Adding views to previewFeatures may introduce instability as preview features are not fully stable. Ensure that this feature is necessary and tested thoroughly in your environment.
| @@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") | ||
| @@index([legacyId]) |
There was a problem hiding this comment.
[❗❗ performance]
The removal of the @@index([name(ops: raw("pg_catalog.gin_trgm_ops"))], type: Gin, map: "challenge_name_trgm_idx") index could impact performance if text search on the name field was previously optimized by this index. Verify that this change does not degrade query performance.
| ////////////////////////////////////////// | ||
|
|
||
| model MemberChallengeAccess { | ||
| view MemberChallengeAccess { |
There was a problem hiding this comment.
[❗❗ correctness]
Changing MemberChallengeAccess from a model to a view alters its behavior significantly. Ensure that all dependencies on this model are updated to handle it as a view, and verify that the view definition aligns with the expected data structure.
| incrementalCoefficient Float? | ||
| opportunityType ReviewOpportunityTypeEnum? | ||
| isAIReviewer Boolean | ||
| isAIReviewer Boolean? |
There was a problem hiding this comment.
[❗❗ correctness]
Changing isAIReviewer from Boolean to Boolean? introduces a potential for null values. Ensure that the application logic correctly handles null values for this field.
Fix errors seen in build
| paths: | ||
| - node_modules | ||
|
|
||
| finger_print_add: &finger_print_add |
There was a problem hiding this comment.
[performance]
The removal of the restore_cache_settings_for_build and save_cache_settings anchors, along with their usage in the builddeploy_steps, could lead to increased build times if the node_modules directory is not cached elsewhere. Consider verifying if caching is handled in another part of the pipeline or if this removal is intentional and acceptable.
Allow for Topgear Submission type phase when calculating start / end …
| const { ensureAcessibilityToModifiedGroups } = require("./group-helper"); | ||
| const { ChallengeStatusEnum } = require("@prisma/client"); | ||
|
|
||
| const SUBMISSION_PHASE_PRIORITY = ["Topcoder Submission", "Submission"]; |
There was a problem hiding this comment.
[💡 performance]
Consider using a Set for SUBMISSION_PHASE_PRIORITY if the list of phase names is expected to grow or if you need to perform frequent lookups. This could improve performance slightly by reducing the time complexity of lookups from O(n) to O(1).
|
|
||
| const registrationPhase = _.find(phases, (p) => p.name === "Registration"); | ||
| const submissionPhase = | ||
| _.find(phases, (p) => p.name === SUBMISSION_PHASE_PRIORITY[0]) || |
There was a problem hiding this comment.
[correctness]
The logic for finding the submissionPhase uses a priority list but does not handle the case where neither 'Topcoder Submission' nor 'Submission' phases are found. Consider adding a fallback or handling this case explicitly to avoid potential issues if both phases are missing.
Allow for registration phase reopening if submission or TG submission phases are open
| }); | ||
|
|
||
| if (dependentOpenPhases.length === 0) { | ||
| const normalizedPhaseName = normalizePhaseName(phaseName); |
There was a problem hiding this comment.
[performance]
The normalizePhaseName function is called twice for each phase in openPhases within the some method. Consider normalizing the phase name once before the some method to improve performance.
| const hasSubmissionVariantOpen = openPhases.some((phase) => | ||
| SUBMISSION_PHASE_NAME_SET.has(normalizePhaseName(phase?.name)) | ||
| ); | ||
| const allowRegistrationReopenWithoutExplicitDependency = |
There was a problem hiding this comment.
[maintainability]
The logic to allow reopening the registration phase without explicit dependency when a submission variant is open is embedded within the condition. Consider extracting this logic into a well-named function to improve readability and maintainability.
| } | ||
| }) | ||
|
|
||
| try { |
There was a problem hiding this comment.
[correctness]
The try block is used to execute partiallyUpdateChallengePhase, but the finally block contains database updates that should be in a catch block to handle potential errors from the try block. Consider adding a catch block to handle errors explicitly.
|
|
||
| }) | ||
|
|
||
| it('partially update challenge phase - cannot reopen when open phase is not a successor or submission variant', async () => { |
There was a problem hiding this comment.
[correctness]
The test case description mentions 'cannot reopen when open phase is not a successor or submission variant', but the logic in the test does not verify the 'submission variant' condition explicitly. Ensure the test case covers all conditions mentioned in the description.
Allow reopening of registration phase and submission start / end date fix
Pm 2206 2nd fix
| -- Improve search responsiveness for group-constrained queries | ||
| CREATE INDEX IF NOT EXISTS "challenge_groups_gin_idx" | ||
| ON "challenges"."Challenge" | ||
| USING GIN ("groups"); |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the groups column is of a suitable data type for a GIN index, such as jsonb or array. Using GIN on unsupported types may lead to unexpected behavior or performance issues.
| @@index([updatedAt]) | ||
| @@index([typeId]) | ||
| @@index([trackId]) | ||
| @@index([groups], type: Gin, map: "challenge_groups_gin_idx") |
There was a problem hiding this comment.
[performance]
The use of a GIN index on the groups array field is appropriate for efficient querying of array data types in PostgreSQL. Ensure that the queries taking advantage of this index are optimized to benefit from the GIN index, as not all operations on array fields will automatically leverage it.
https://topcoder.atlassian.net/browse/PM-2614