PM-2614 - Module updates -> dev #32
Conversation
|
@jmgasper this is deployed to dev env. It is good time to have the updates in as QA is testing the API for the scopes. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Check if the route is not found or HTTP method is not supported | ||
| app.use("*", (req, res) => { | ||
| app.use((req, res) => { |
There was a problem hiding this comment.
[❗❗ correctness]
Removing the "*" wildcard from app.use means that this middleware will now handle all requests, not just those that don't match any defined route. This could lead to unexpected behavior if there are other middlewares or routes defined after this point that should be executed. Consider whether this change is intentional and if it aligns with the desired routing logic.
| 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]
The change from _.values(ChallengeTrackEnum) to ..._.values(ChallengeTrackEnum) is technically correct and improves the code by spreading the array values directly into the valid method. However, ensure that ChallengeTrackEnum is always an object with enumerable properties, as spreading will fail if it's not.
| 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]
The use of the spread operator here assumes that ChallengeTrackEnum is an object with enumerable properties. If ChallengeTrackEnum can be null or undefined, this will throw an error. Consider adding a check or default value to prevent runtime errors.
| 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 that ChallengeTrackEnum is always defined and is an object with enumerable properties. The spread operator will throw an error if ChallengeTrackEnum is null or undefined.
| 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 assumes ChallengeTrackEnum is always an object with enumerable properties. If there's a possibility of it being null or undefined, consider adding a safeguard to prevent potential runtime errors.
|
|
||
| // 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 any object without a validate method is a plain object schema. This could lead to incorrect assumptions if a non-Joi object with a validate method is passed. Consider explicitly checking for Joi schema types instead.
| // lazy initialization of S3 instance | ||
| // lazy initialization of S3 instance (kept for backward compatibility) | ||
| function getS3() { | ||
| if (!s3) { |
There was a problem hiding this comment.
[design]
Replacing AWS.S3() with s3Client directly in getS3() changes the behavior from creating a new S3 instance to reusing the same client instance. Ensure that this change is intentional and that the client is safe to reuse across multiple calls, as this could affect thread safety or configuration changes.
|
|
||
| // 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 assumed to be an async iterable. Ensure that this assumption holds true for all possible responses, as any deviation could lead to runtime errors.
| secretAccessKey: config.AMAZON.AWS_SECRET_ACCESS_KEY | ||
| }, | ||
| forcePathStyle: true, | ||
| tls: false |
There was a problem hiding this comment.
[❗❗ security]
The tls: false option is set, which disables TLS. Ensure this is intentional and that the endpoint is trusted, as it could expose sensitive data over the network.
| sslEnabled: false, | ||
| s3ForcePathStyle: true | ||
| }) | ||
| initBucket() |
There was a problem hiding this comment.
[correctness]
The initBucket() function returns a promise, but any errors it throws will not be caught here. Consider adding a .catch() block to handle potential errors during initialization.
| 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.
[performance]
Using Readable.from([attachmentContent]) is technically correct, but ensure that attachmentContent is always a valid buffer or string. If attachmentContent can be large, consider using streams directly to avoid memory issues.
Branch
module-updatesis deployed to dev env.https://topcoder.atlassian.net/browse/PM-2614