search

keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
196 stars 96 forks source link

feat(api): added getRevisionsOfVariable method #287

Closed yogesh1801 closed 3 months ago

yogesh1801 commented 3 months ago

User description

Description

added getRevisionsOfVariable method

Fixes #271

Dependencies

Mention any dependencies/packages used

Future Improvements

Mention any improvements to be done in future related to any file/feature

Mentions

Mention and tag the people

Screenshots of relevant screens

Add screenshots of relevant screens

Developer's checklist

If changes are made in the code:

Documentation Update

PR Type

Enhancement

Description

Changes walkthrough 📝

Relevant files
Enhancement
variable.controller.ts
Add endpoint to fetch revisions of a variable by environment
apps/api/src/variable/controller/variable.controller.ts
  • Added a new endpoint getRevisionsOfVariable to fetch variable
    revisions.
  • Endpoint requires variableId and environmentId as parameters.
  • Applied RequiredApiKeyAuthorities decorator with
    Authority.READ_VARIABLE.
    		•  +14/-0   
    variable.service.ts
    Implement service method to get variable revisions by environment
    apps/api/src/variable/service/variable.service.ts
  • Added getRevisionsOfVariable method to fetch variable revisions.
  • Method checks user authority over the variable.
  • Filters variable versions by the provided environment ID.
    		•  +22/-0   

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-free[bot] commented 3 months ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The method getRevisionsOfVariable in variable.service.ts assumes that variable.versions is always populated. If versions is not populated or if the variable does not exist, this could lead to runtime errors. Consider adding error handling for these cases.
    Data Integrity:
    The filtering logic for revisions is done in the application layer rather than the database layer. This could be inefficient if there are many versions. Consider implementing the filter directly in the database query to improve performance and reduce memory usage.
    codiumai-pr-agent-free[bot] commented 3 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Optimize data fetching by querying the database directly ___ **To enhance the performance of the getRevisionsOfVariable method, consider using a database
    query to directly fetch the filtered revisions based on the environmentId, instead of
    fetching all versions and filtering them in the service layer.** [apps/api/src/variable/service/variable.service.ts [553-555]](https://github.com/keyshade-xyz/keyshade/pull/287/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR553-R555) ```diff -const revisions = variable.versions.filter( - (version) => version.environmentId === environmentId -) +const revisions = await this.prisma.version.findMany({ + where: { + variableId: variableId, + environmentId: environmentId + } +}); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: This suggestion significantly improves performance by directly querying the database for the filtered revisions, rather than fetching all versions and filtering them in the service layer. It is a major enhancement in terms of efficiency.
    		10
    Security
    Validate API parameters to enhance security and reliability ___ **To ensure the API's security and proper usage, validate the variableId and environmentId
    parameters in the getRevisionsOfVariable method to prevent invalid or malicious inputs.** [apps/api/src/variable/controller/variable.controller.ts [92-93]](https://github.com/keyshade-xyz/keyshade/pull/287/files#diff-7639f037978995ad2ebde29feddffad312b5f7f22625024df30c59d3c0b7927aR92-R93) ```diff -@Param('variableId') variableId: string, -@Param('environmentId') environmentId: string +@Param('variableId', ParseUUIDPipe) variableId: string, +@Param('environmentId', ParseUUIDPipe) environmentId: string ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: Validating the `variableId` and `environmentId` parameters is essential for preventing invalid or malicious inputs, thereby enhancing the security and reliability of the API.
    		10
    Enhancement
    Add error handling to improve API robustness ___ **Consider adding error handling in the getRevisionsOfVariable method to manage cases where
    the variable or environment IDs do not exist or the user does not have the required
    permissions. This will improve the robustness of the API by providing meaningful error
    responses to the clients.** [apps/api/src/variable/controller/variable.controller.ts [95-99]](https://github.com/keyshade-xyz/keyshade/pull/287/files#diff-7639f037978995ad2ebde29feddffad312b5f7f22625024df30c59d3c0b7927aR95-R99) ```diff -return await this.variableService.getRevisionsOfVariable( - user, - variableId, - environmentId -) +try { + return await this.variableService.getRevisionsOfVariable( + user, + variableId, + environmentId + ); +} catch (error) { + throw new HttpException('Error fetching revisions', HttpStatus.BAD_REQUEST); +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: Adding error handling is crucial for providing meaningful error responses and improving the robustness of the API. This suggestion addresses potential issues where the variable or environment IDs do not exist or the user lacks the required permissions.
    		9
    Maintainability
    Add logging to monitor method execution and data handling ___ **Implement logging within the getRevisionsOfVariable method to track the flow and data,
    which can be crucial for debugging issues and monitoring the system's behavior.** [apps/api/src/variable/service/variable.service.ts [544-550]](https://github.com/keyshade-xyz/keyshade/pull/287/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR544-R550) ```diff +console.log(`Checking authority for user ${user.id} on variable ${variableId}`); const variable = await this.authorityCheckerService.checkAuthorityOverVariable({ userId: user.id, entity: { id: variableId }, authority: Authority.READ_VARIABLE, prisma: this.prisma - }) + }); +console.log(`Authority check complete. Variable fetched: ${variable}`); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Implementing logging is beneficial for debugging and monitoring, but it is not as critical as error handling or performance optimization. It enhances maintainability and traceability of the code.
    		7