Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(api): Replace for loop with array indexing while decrypting secrets during bulk fetch #265 #266

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

anudeeps352
Copy link
Contributor

@anudeeps352 anudeeps352 commented Jun 9, 2024

User description

…bulk fetch

Description

Replace for loop with array indexing while decrypting secrets during bulk fetch

Fixes #265

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

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement


Description

  • Replaced the for loop with array indexing to optimize the decryption of secrets during bulk fetch.
  • Simplified the decryption logic by assuming only the first version of each secret needs to be decrypted.

Changes walkthrough 📝

Relevant files
Enhancement
secret.service.ts
Optimize secret decryption by replacing loop with array indexing

apps/api/src/secret/service/secret.service.ts

  • Replaced for loop with array indexing for decrypting secrets.
  • Simplified decryption logic by assuming only the first version needs
    decryption.
  • +5/-10   

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

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are localized to a specific function and involve simplifying the decryption logic by replacing a loop with direct indexing. The logic is straightforward, and the impact is limited to one part of the code.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The new code assumes that only the first version of each secret needs to be decrypted. If there are scenarios where other versions need decryption, this change could lead to incorrect behavior.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a check to ensure that the versions array is not empty before accessing its first element

    Consider handling the scenario where secret.versions might be an empty array. This can
    prevent runtime errors when attempting to access secret.versions[0]. You can add a check
    to ensure the array is not empty before proceeding with decryption.

    apps/api/src/secret/service/secret.service.ts [508]

    -const version = secret.versions[0]
    +if (secret.versions.length > 0) {
    +  const version = secret.versions[0]
    +  // Optionally decrypt secret value if decryptValue is true
    +  if (decryptValue) {
    +    const decryptedValue = await decrypt(project.privateKey, version.value)
    +    version.value = decryptedValue
    +  }
    +}
     
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a potential runtime error by ensuring that the secret.versions array is not empty before accessing its first element. This is crucial for preventing crashes and maintaining the robustness of the code.

    10
    Performance
    Modify the decryption to be handled asynchronously for all versions, improving performance

    To ensure that the decryption process does not block other operations, consider handling
    the decryption asynchronously for each secret version, possibly using Promise.all if
    reverting to processing all versions.

    apps/api/src/secret/service/secret.service.ts [511-512]

    -const decryptedValue = await decrypt(project.privateKey, version.value)
    -version.value = decryptedValue
    +const decryptedValues = await Promise.all(secrets.map(secret => decrypt(project.privateKey, secret.versions[0].value)));
    +secrets.forEach((secret, index) => {
    +  secret.versions[0].value = decryptedValues[index];
    +});
     
    Suggestion importance[1-10]: 7

    Why: This suggestion can improve performance by handling decryption asynchronously. However, it introduces a more complex change and assumes that reverting to processing all versions is desired, which may not align with the current PR's intent.

    7
    Maintainability
    Use a more descriptive variable name for the decrypted value

    Consider using a more descriptive variable name for decryptedValue to indicate what the
    value represents, such as decryptedSecretValue.

    apps/api/src/secret/service/secret.service.ts [511-512]

    -const decryptedValue = await decrypt(project.privateKey, version.value)
    +const decryptedSecretValue = await decrypt(project.privateKey, version.value)
    +version.value = decryptedSecretValue
     
    Suggestion importance[1-10]: 6

    Why: Using a more descriptive variable name can enhance code readability and maintainability. However, it is a minor improvement and does not affect the functionality of the code.

    6
    Rename the variable to reflect that it handles only the first version of the secret

    Since the loop now only processes the first version of each secret, consider renaming the
    variable secret to firstSecretVersion or similar to reflect that it's specifically
    handling the first version. This enhances code readability and maintainability.

    apps/api/src/secret/service/secret.service.ts [508]

    -const version = secret.versions[0]
    +const firstSecretVersion = secret.versions[0]
     
    Suggestion importance[1-10]: 5

    Why: While renaming the variable can improve code readability, it is not crucial for the functionality or performance of the code. It is a minor improvement focused on maintainability.

    5

    @anudeeps352 anudeeps352 changed the title API: Replace for loop with array indexing while decrypting secrets during bulk fetch #265 refactor(api): Replace for loop with array indexing while decrypting secrets during bulk fetch #265 Jun 9, 2024
    Copy link

    sonarcloud bot commented Jun 10, 2024

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    @rajdip-b rajdip-b merged commit 62a1731 into keyshade-xyz:develop Jun 10, 2024
    5 checks passed
    rajdip-b pushed a commit that referenced this pull request Jun 12, 2024
    ## [2.0.0](v1.4.0...v2.0.0) (2024-06-12)
    
    ### ⚠ BREAKING CHANGES
    
    * **api:** Refactor environment, [secure] and variable functionality
    
    ### 🚀 Features
    
    * **platform:** Workspace integrate ([#241](#241)) ([6107e7d](6107e7d))
    
    ### 📚 Documentation
    
    * Fix broken links in README.md ([9266788](9266788))
    * Modified environment-variable.md ([#256](#256)) ([4974756](4974756))
    
    ### 🔧 Miscellaneous Chores
    
    * Added docker build and run commands to` package.json` ([#258](#258)) ([af61791](af61791))
    * **api:** Fix inconsistencies in zod schema ([#240](#240)) ([f3a3632](f3a3632))
    * **ci:** Update deploy web ([e80d47d](e80d47d))
    * **docker:** Grant correct permissions to docker image ([#251](#251)) ([49546aa](49546aa))
    * Update GitHub Action plugin versions  ([#263](#263)) ([020bbf6](020bbf6))
    * Update package versions for release ([93785be](93785be))
    
    ### 🔨 Code Refactoring
    
    * **api:** Refactor environment, [secure] and variable functionality ([#270](#270)) ([55a6d37](55a6d37))
    * **api:** Replace for loop with array indexing while decrypting [secure]s during bulk fetch [#265](#265) ([#266](#266)) ([62a1731](62a1731))
    * **api:** Update return type while fetching [secure]s and variables ([#264](#264)) ([fd36abd](fd36abd))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 2.0.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    rajdip-b pushed a commit that referenced this pull request Jun 27, 2024
    ## [2.0.0](v1.4.0...v2.0.0) (2024-06-12)
    
    ### ⚠ BREAKING CHANGES
    
    * **api:** Refactor environment, [secure] and variable functionality
    
    ### 🚀 Features
    
    * **platform:** Workspace integrate ([#241](#241)) ([6107e7d](6107e7d))
    
    ### 📚 Documentation
    
    * Fix broken links in README.md ([9266788](9266788))
    * Modified environment-variable.md ([#256](#256)) ([4974756](4974756))
    
    ### 🔧 Miscellaneous Chores
    
    * Added docker build and run commands to` package.json` ([#258](#258)) ([af61791](af61791))
    * **api:** Fix inconsistencies in zod schema ([#240](#240)) ([f3a3632](f3a3632))
    * **ci:** Update deploy web ([e80d47d](e80d47d))
    * **docker:** Grant correct permissions to docker image ([#251](#251)) ([49546aa](49546aa))
    * Update GitHub Action plugin versions  ([#263](#263)) ([020bbf6](020bbf6))
    * Update package versions for release ([93785be](93785be))
    
    ### 🔨 Code Refactoring
    
    * **api:** Refactor environment, [secure] and variable functionality ([#270](#270)) ([55a6d37](55a6d37))
    * **api:** Replace for loop with array indexing while decrypting [secure]s during bulk fetch [#265](#265) ([#266](#266)) ([62a1731](62a1731))
    * **api:** Update return type while fetching [secure]s and variables ([#264](#264)) ([fd36abd](fd36abd))
    yogesh1801 pushed a commit to yogesh1801/keyshade that referenced this pull request Jun 29, 2024
    ## [2.0.0](keyshade-xyz/keyshade@v1.4.0...v2.0.0) (2024-06-12)
    
    ### ⚠ BREAKING CHANGES
    
    * **api:** Refactor environment, [secure] and variable functionality
    
    ### 🚀 Features
    
    * **platform:** Workspace integrate ([keyshade-xyz#241](keyshade-xyz#241)) ([6107e7d](keyshade-xyz@6107e7d))
    
    ### 📚 Documentation
    
    * Fix broken links in README.md ([9266788](keyshade-xyz@9266788))
    * Modified environment-variable.md ([keyshade-xyz#256](keyshade-xyz#256)) ([4974756](keyshade-xyz@4974756))
    
    ### 🔧 Miscellaneous Chores
    
    * Added docker build and run commands to` package.json` ([keyshade-xyz#258](keyshade-xyz#258)) ([af61791](keyshade-xyz@af61791))
    * **api:** Fix inconsistencies in zod schema ([keyshade-xyz#240](keyshade-xyz#240)) ([f3a3632](keyshade-xyz@f3a3632))
    * **ci:** Update deploy web ([e80d47d](keyshade-xyz@e80d47d))
    * **docker:** Grant correct permissions to docker image ([keyshade-xyz#251](keyshade-xyz#251)) ([49546aa](keyshade-xyz@49546aa))
    * Update GitHub Action plugin versions  ([keyshade-xyz#263](keyshade-xyz#263)) ([020bbf6](keyshade-xyz@020bbf6))
    * Update package versions for release ([93785be](keyshade-xyz@93785be))
    
    ### 🔨 Code Refactoring
    
    * **api:** Refactor environment, [secure] and variable functionality ([keyshade-xyz#270](keyshade-xyz#270)) ([55a6d37](keyshade-xyz@55a6d37))
    * **api:** Replace for loop with array indexing while decrypting [secure]s during bulk fetch [keyshade-xyz#265](keyshade-xyz#265) ([keyshade-xyz#266](keyshade-xyz#266)) ([62a1731](keyshade-xyz@62a1731))
    * **api:** Update return type while fetching [secure]s and variables ([keyshade-xyz#264](keyshade-xyz#264)) ([fd36abd](keyshade-xyz@fd36abd))
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    API: Replace for loop with array indexing while decrypting secrets during bulk fetch
    2 participants