Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| us.user_id, | ||
| COUNT(*) AS skill_count | ||
| COUNT(*) AS skill_count, | ||
| ARRAY_AGG(DISTINCT us.skill_id::uuid) AS skill_ids |
There was a problem hiding this comment.
[performance]
Using ARRAY_AGG(DISTINCT us.skill_id::uuid) can potentially lead to performance issues if the dataset is large, as it requires sorting to remove duplicates. Consider evaluating the performance impact and whether this aggregation is necessary for the query's purpose.
| AND m."photoURL" <> '' | ||
| AND m."homeCountryCode" IS NOT NULL | ||
| AND ($1::text IS NULL OR COALESCE(m."homeCountryCode", m."competitionCountryCode") = $1) | ||
| AND ($3::uuid[] IS NULL OR ms.skill_ids && $3::uuid[]) |
There was a problem hiding this comment.
[performance]
The condition ms.skill_ids && $3::uuid[] uses the array overlap operator, which can be inefficient for large arrays. Ensure that the skill_ids array is indexed appropriately to optimize this operation.
| us.user_id, | ||
| COUNT(*) AS skill_count | ||
| COUNT(*) AS skill_count, | ||
| ARRAY_AGG(DISTINCT us.skill_id::uuid) AS skill_ids |
There was a problem hiding this comment.
[performance]
Using ARRAY_AGG(DISTINCT us.skill_id::uuid) can potentially lead to performance issues if the dataset is large, as it requires deduplication and aggregation. Consider evaluating the performance impact and if necessary, optimize by ensuring indexes on skill_id or using alternative methods for deduplication.
| AND m."photoURL" <> '' | ||
| AND m."homeCountryCode" IS NOT NULL | ||
| AND ($1::text IS NULL OR COALESCE(m."homeCountryCode", m."competitionCountryCode") = $1) | ||
| AND ($5::uuid[] IS NULL OR ms.skill_ids && $5::uuid[]) |
There was a problem hiding this comment.
[correctness]
The condition ms.skill_ids && $5::uuid[] checks for any overlap between the arrays, which is correct for filtering by skills. However, ensure that $5 is always provided as an array, even if empty, to avoid unexpected behavior. Consider adding validation or default handling for this parameter.
| return undefined; | ||
| } | ||
| const values = Array.isArray(value) ? value : [value]; | ||
| return values.map((v) => String(v)).filter((v) => v.trim().length > 0); |
There was a problem hiding this comment.
[💡 readability]
The transformation logic for skillId could be improved by explicitly checking for empty strings after trimming. Currently, it filters out empty strings, but it might be clearer to explicitly handle cases where v.trim() results in an empty string. Consider using a more explicit condition to improve readability.
| : skillId !== undefined && skillId !== null | ||
| ? [skillId] | ||
| : []; | ||
| const skillIds = rawSkillIds.filter((id) => id && id.trim().length > 0); |
There was a problem hiding this comment.
[correctness]
The filtering of skillIds using id && id.trim().length > 0 assumes that id is a string. If skillId can be a non-string type (e.g., number), this could lead to unexpected behavior. Consider ensuring skillId is always a string before this operation or adjusting the logic to handle different types appropriately.
| : 50; | ||
| const offset = (safePage - 1) * safePerPage; | ||
|
|
||
| const hasSkillIds = Array.isArray(skillIds) && skillIds.length > 0; |
There was a problem hiding this comment.
[💡 maintainability]
The variable hasSkillIds is used only once to determine the value of skillIdsParam. Consider inlining this variable to simplify the code.
| const offset = (safePage - 1) * safePerPage; | ||
|
|
||
| const hasSkillIds = Array.isArray(skillIds) && skillIds.length > 0; | ||
| const skillIdsParam = hasSkillIds ? skillIds : null; |
There was a problem hiding this comment.
[❗❗ security]
The skillIdsParam is set to null if skillIds is not an array or is empty. Ensure that the SQL query handles null values for this parameter correctly to avoid potential SQL injection or logic errors.
2 participants
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-4203