Closed unamdev0 closed 2 weeks ago
β±οΈ Estimated effort to review: 3 π΅π΅π΅βͺβͺ |
π§ͺ PR contains tests |
π No security concerns identified |
β‘ Key issues to review Performance Concern The new implementation fetches all environments, secrets, and variables for each project, which could be inefficient for projects with many items. Consider using aggregation queries or optimizing the database calls. Error Handling The new code doesn't include error handling for database queries. Consider adding try-catch blocks or error handling mechanisms to manage potential failures gracefully. |
Category | Suggestion | Score |
Performance |
Optimize database queries by fetching all required counts in a single query___ **Consider using a single database query to fetch all required counts instead ofmultiple separate queries for each environment.** [apps/api/src/project/service/project.service.ts [818-863]](https://github.com/keyshade-xyz/keyshade/pull/434/files#diff-d85f483eff15c28a4b3e5f7a54c9b159c71003f8a95ef7a4be00eb80ec72b2faR818-R863) ```diff -const allEnvs = await this.prisma.environment.findMany({ - where: { projectId: project.id } +const projectCounts = await this.prisma.environment.findMany({ + where: { projectId: project.id }, + select: { + id: true, + slug: true, + _count: { + select: { + secrets: { + where: { projectId: project.id } + }, + variables: { + where: { projectId: project.id } + } + } + } + } }) -const envPromises = allEnvs.map(async (env) => { +for (const env of projectCounts) { const hasRequiredPermission = await this.authorityCheckerService.checkAuthorityOverEnvironment({ userId: user.id, entity: { slug: env.slug }, authorities: [Authority.READ_ENVIRONMENT], prisma: this.prisma }) if (hasRequiredPermission) { totalEnvironmentsOfProject += 1 + totalSecretsOfProject += env._count.secrets + totalVariablesOfProject += env._count.variables + } +} - const [secretCount, variableCount] = await Promise.all([ - this.prisma.secret.count({ - where: { - projectId: project.id, - versions: { some: { environmentId: env.id } } - } - }), - this.prisma.variable.count({ - where: { - projectId: project.id, - versions: { some: { environmentId: env.id } } - } - }) - ]) - - totalSecretsOfProject += secretCount - totalVariablesOfProject += variableCount - } -}) - -await Promise.all(envPromises) - ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: This suggestion offers a substantial performance improvement by reducing the number of database queries, which can lead to faster execution and reduced load on the database. | 9 |
β Optimize performance by parallelizing the fetching of environment, secret, and variable counts___ **Consider using Promise.all() to parallelize the fetching of environment, secret, andvariable counts for better performance.** [apps/api/src/project/service/project.service.ts [822-863]](https://github.com/keyshade-xyz/keyshade/pull/434/files#diff-d85f483eff15c28a4b3e5f7a54c9b159c71003f8a95ef7a4be00eb80ec72b2faR822-R863) ```diff const envPromises = allEnvs.map(async (env) => { const hasRequiredPermission = await this.authorityCheckerService.checkAuthorityOverEnvironment({ userId: user.id, entity: { slug: env.slug }, authorities: [Authority.READ_ENVIRONMENT], prisma: this.prisma }) if (hasRequiredPermission) { totalEnvironmentsOfProject += 1 - - const [secretCount, variableCount] = await Promise.all([ + return Promise.all([ this.prisma.secret.count({ where: { projectId: project.id, versions: { some: { environmentId: env.id } } } }), this.prisma.variable.count({ where: { projectId: project.id, versions: { some: { environmentId: env.id } } } }) ]) - - totalSecretsOfProject += secretCount - totalVariablesOfProject += variableCount } + return [0, 0] }) -await Promise.all(envPromises) +const counts = await Promise.all(envPromises) +totalSecretsOfProject = counts.reduce((sum, [secretCount]) => sum + secretCount, 0) +totalVariablesOfProject = counts.reduce((sum, [, variableCount]) => sum + variableCount, 0) ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 8Why: The suggestion correctly identifies an opportunity to optimize performance by parallelizing asynchronous operations, which can significantly reduce execution time in scenarios with multiple environments. | 8 | |
Enhancement |
β Enhance test assertions to verify both count and content of fetched project data___ **Consider adding assertions to verify the actual content of the fetched items, notjust their counts.** [apps/api/src/project/project.e2e.spec.ts [604-610]](https://github.com/keyshade-xyz/keyshade/pull/434/files#diff-f1b22baab0b4173d825570c9030ef014582bc75bb67ef2e544f0dd879ff941a2R604-R610) ```diff expect(response.statusCode).toBe(200) expect(response.json().items.length).toEqual(1) -//One environment is added by default -expect(response.json().items[0].totalEnvironmentsOfProject).toEqual(2) -expect(response.json().items[0].totalVariablesOfProject).toEqual(2) -expect(response.json().items[0].totalSecretsOfProject).toEqual(2) +const project = response.json().items[0] +expect(project.totalEnvironmentsOfProject).toEqual(2) +expect(project.totalVariablesOfProject).toEqual(2) +expect(project.totalSecretsOfProject).toEqual(2) +// Verify project details +expect(project.name).toEqual('Project4') +expect(project.description).toEqual('Project for testing if all environments,secrets and keys are being fetched or not') + +// Verify that sensitive data is not included +expect(project).not.toHaveProperty('privateKey') +expect(project).not.toHaveProperty('publicKey') + ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 7Why: This suggestion enhances the robustness of the test by verifying the content of the fetched data, ensuring that the test checks more than just the counts, which improves test coverage. | 7 |
Best practice |
β Improve test case naming for better clarity and understanding___ **Consider using a more descriptive name for the test case to clearly indicate what isbeing tested.** [apps/api/src/project/project.e2e.spec.ts [530]](https://github.com/keyshade-xyz/keyshade/pull/434/files#diff-f1b22baab0b4173d825570c9030ef014582bc75bb67ef2e544f0dd879ff941a2R530-R530) ```diff -it('should be able fetch all environments,variables,secrets of all projects of a workspace', async () => { +it('should fetch correct counts of environments, variables, and secrets for projects in a workspace', async () => { ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 5Why: The suggestion improves code readability and maintainability by making the test case name more descriptive, though it does not address a critical issue. | 5 |
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 87.93%. Comparing base (
ce50743
) to head (d32f207
). Report is 150 commits behind head on develop.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hold up a min, I'm making some changes.
I have updated the method by which projects are fetched. Doesn't directly affect your changes. Please make the required changes now.
User description
Description
Sending total Count of env,variable and secrets when fetching project data for a workspace
Fixes #308
Screenshots of response
Further improvement
PR Type
Tests, Enhancement
Description
Changes walkthrough π
project.e2e.spec.ts
Add tests for fetching environments, variables, and secrets in
projects
apps/api/src/project/project.e2e.spec.ts
fetching.
and secrets for projects in a workspace.
project.service.ts
Enhance project fetching with total counts of environments, variables,
and secrets
apps/api/src/project/service/project.service.ts
variables, and secrets.