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

feat(api): add minio-client provider #237

Merged
merged 12 commits into from
May 24, 2024

Conversation

HarshPatel5940
Copy link
Contributor

@HarshPatel5940 HarshPatel5940 commented May 23, 2024

User description

Description

This pull request adds the minio-client provider to the project. The minio-client allows for interaction with a MinIO server, including uploading, downloading, and deleting files. This feature will enhance the functionality of the project by providing support for MinIO storage.

Fixes #120

Mentions

Mention and tag the people

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, Documentation, Configuration changes


Description

  • Added MinioProvider to interact with MinIO server, including functions for uploading, downloading, and deleting files.
  • Added MinIO related environment variables to the schema and documented them.
  • Registered MinioProvider in the provider module.
  • Added minio package to dependencies.
  • Added MinIO service configuration to Docker Compose and test Docker Compose files.
  • Added example MinIO environment variables to .env.example.

Changes walkthrough 📝

Relevant files
Enhancement
minio.provider.ts
Add `MinioProvider` for MinIO server interactions.             

apps/api/src/provider/minio.provider.ts

  • Added MinioProvider to interact with MinIO server.
  • Implemented functions for uploading, downloading, and deleting files.
  • Included environment variable checks and bucket creation logic.
  • +74/-0   
    provider.module.ts
    Register `MinioProvider` in provider module.                         

    apps/api/src/provider/provider.module.ts

    • Registered MinioProvider in the provider module.
    +6/-1     
    Configuration changes
    env.schema.ts
    Add MinIO environment variables to schema.                             

    apps/api/src/common/env/env.schema.ts

    • Added MinIO related environment variables to the schema.
    +7/-0     
    docker-compose.yml
    Add MinIO service to Docker Compose.                                         

    docker-compose.yml

    • Added MinIO service configuration.
    +13/-0   
    .env.example
    Add example MinIO environment variables.                                 

    .env.example

    • Added example MinIO environment variables.
    +8/-2     
    docker-compose-test.yml
    Add MinIO service to test Docker Compose.                               

    docker-compose-test.yml

    • Added MinIO service configuration for testing.
    +13/-0   
    Documentation
    environment-variables.md
    Document MinIO environment variables.                                       

    docs/contributing-to-keyshade/environment-variables.md

    • Documented new environment variables for MinIO configuration.
    +9/-1     
    Dependencies
    package.json
    Add `minio` package to dependencies.                                         

    apps/api/package.json

    • Added minio package to dependencies.
    +4/-2     

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

    apps/api/src/provider/minio.provider.ts Show resolved Hide resolved
    docker-compose.yml Outdated Show resolved Hide resolved
    docker-compose.yml Outdated Show resolved Hide resolved
    docker-compose.yml Show resolved Hide resolved
    Copy link

    codecov bot commented May 23, 2024

    Codecov Report

    Attention: Patch coverage is 34.21053% with 25 lines in your changes are missing coverage. Please review.

    Project coverage is 90.73%. Comparing base (ce50743) to head (2f7b569).
    Report is 2 commits behind head on develop.

    Current head 2f7b569 differs from pull request most recent head 204eab9

    Please upload reports for the commit 204eab9 to get more accurate results.

    Files Patch % Lines
    apps/api/src/provider/minio.provider.ts 30.55% 25 Missing ⚠️
    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #237      +/-   ##
    ===========================================
    - Coverage    91.71%   90.73%   -0.98%     
    ===========================================
      Files          111      112       +1     
      Lines         2510     2547      +37     
      Branches       469      478       +9     
    ===========================================
    + Hits          2302     2311       +9     
    - Misses         208      236      +28     
    Flag Coverage Δ
    api-e2e-tests 90.73% <34.21%> (-0.98%) ⬇️

    Flags with carried forward coverage won't be shown. Click here to find out more.

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and configurations including environment variables, Docker Compose settings, and TypeScript code. The complexity is moderate due to the integration of a new service (MinIO) and requires understanding of both the infrastructure and application code.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The isServiceLoaded flag is set inside the createBucketIfNotExists function which might not complete before other methods like uploadFile are called. This can lead to race conditions where file operations are attempted before the bucket is ready.

    Error Handling: The use of InternalServerErrorException for Minio client not loaded might not be appropriate for all cases, such as when a file operation is called. A more specific exception or error handling strategy could be beneficial.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure createBucketIfNotExists is awaited before checking isServiceLoaded

    The isServiceLoaded flag is set inside the createBucketIfNotExists function but checked
    immediately after its call without waiting for its completion. This might lead to
    incorrect logging about the Minio provider's load status. To fix this, you should await
    the createBucketIfNotExists call before checking the isServiceLoaded flag.

    apps/api/src/provider/minio.provider.ts [34-36]

    -createBucketIfNotExists()
    +await createBucketIfNotExists()
     if (isServiceLoaded) {
       logger.log('Minio Provider Loaded Successfully.')
     }
     
    Suggestion importance[1-10]: 10

    Why: This suggestion fixes a logical error that could lead to incorrect logging. Ensuring that the createBucketIfNotExists function is awaited before checking the isServiceLoaded flag is crucial for accurate status reporting.

    10
    Possible issue
    Add error handling for bucket creation in the Minio client

    Consider handling the case where minioClient.makeBucket might throw an error due to
    network issues or permissions. This can be done by wrapping the makeBucket call in a
    try-catch block to handle exceptions gracefully.

    apps/api/src/provider/minio.provider.ts [45]

    -await minioClient.makeBucket(bucketName, 'ap-south-1')
    +try {
    +  await minioClient.makeBucket(bucketName, 'ap-south-1')
    +  logger.log(`Bucket ${bucketName} created.`)
    +} catch (error) {
    +  logger.error(`Failed to create bucket ${bucketName}: ${error}`)
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime issue where the makeBucket operation might fail due to network issues or permissions. Adding error handling improves the robustness of the code and ensures that failures are logged appropriately.

    9
    Provide a default value for MINIO_PORT to avoid runtime errors

    The environment variable MINIO_PORT is treated as optional in the environment validation
    schema but is used without null checks when creating the Minio.Client. This could lead to
    runtime errors if MINIO_PORT is not provided. Consider providing a default value or making
    the port mandatory in the environment schema.

    apps/api/src/provider/minio.provider.ts [27]

    -port: Number(process.env.MINIO_PORT),
    +port: Number(process.env.MINIO_PORT || '9000'),
     
    Suggestion importance[1-10]: 8

    Why: Providing a default value for MINIO_PORT helps prevent potential runtime errors if the environment variable is not set. This improves the reliability of the code by ensuring that a valid port number is always used.

    8
    Enhancement
    Add error handling for the file upload operation in Minio

    The uploadFile function does not handle exceptions that might occur during the putObject
    operation. It's good practice to wrap such operations in a try-catch block to handle
    potential errors and provide a better error message to the client.

    apps/api/src/provider/minio.provider.ts [56]

    -await minioClient.putObject(bucketName, fileName, file.buffer, file.size)
    +try {
    +  await minioClient.putObject(bucketName, fileName, file.buffer, file.size)
    +} catch (error) {
    +  logger.error(`Error uploading file to Minio: ${error}`)
    +  throw new InternalServerErrorException('Error uploading file')
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling to the uploadFile function enhances the robustness of the code by ensuring that any issues during the file upload process are caught and logged, and a meaningful error message is returned to the client.

    9

    apps/api/src/provider/minio.provider.ts Outdated Show resolved Hide resolved
    apps/api/src/provider/minio.provider.ts Outdated Show resolved Hide resolved
    apps/api/src/provider/minio.provider.ts Outdated Show resolved Hide resolved
    apps/api/src/provider/minio.provider.ts Outdated Show resolved Hide resolved
    apps/api/src/provider/minio.provider.ts Outdated Show resolved Hide resolved
    apps/api/src/provider/minio.provider.ts Show resolved Hide resolved
    @HarshPatel5940
    Copy link
    Contributor Author

    @rajdip-b i have added error handling and implemented other comments has you said. One thing i was not able to figure out is the error types. i mean catch (error: <type>) so we can might handle it in a better way

    @HarshPatel5940 HarshPatel5940 requested a review from rajdip-b May 23, 2024 15:55
    @rajdip-b
    Copy link
    Member

    @rajdip-b i have added error handling and implemented other comments has you said. One thing i was not able to figure out is the error types. i mean catch (error: <type>) so we can might handle it in a better way

    I don't think you need to catch the types. The type will be very much expected to be caused by bucket errors, so it is fine.

    apps/api/src/provider/minio.provider.ts Outdated Show resolved Hide resolved
    @HarshPatel5940
    Copy link
    Contributor Author

    @rajdip-b i have added error handling and implemented other comments has you said. One thing i was not able to figure out is the error types. i mean catch (error: <type>) so we can might handle it in a better way

    I don't think you need to catch the types. The type will be very much expected to be caused by bucket errors, so it is fine.

    Oh ok, noted.

    @HarshPatel5940 HarshPatel5940 requested a review from rajdip-b May 23, 2024 17:15
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I see you are returning errors. Throw them instead. Otherwise the code will be a lot more difficult!

    @HarshPatel5940
    Copy link
    Contributor Author

    I see you are returning errors. Throw them instead. Otherwise the code will be a lot more difficult!

    are you talking about line 24?

    instead of return -> throw ? and is the exit in the place you needed?

    Copy link

    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

    @HarshPatel5940 HarshPatel5940 requested a review from rajdip-b May 24, 2024 12:39
    @HarshPatel5940
    Copy link
    Contributor Author

    Fixed it with 204eab9

    @rajdip-b rajdip-b merged commit cd71c5a into keyshade-xyz:develop May 24, 2024
    7 checks passed
    rajdip-b pushed a commit that referenced this pull request May 24, 2024
    ## [1.4.0](v1.3.0...v1.4.0) (2024-05-24)
    
    ### 🚀 Features
    
    * add example for health and email auth ([b834d25](b834d25))
    * **api:** Add `minio-client` provider ([#237](#237)) ([cd71c5a](cd71c5a))
    * **api:** Add feature to fork projects ([#239](#239)) ([3bab653](3bab653))
    * **api:** Added feedback form module ([#210](#210)) ([ae1efd8](ae1efd8))
    * **api:** Added Project Level Access  ([#221](#221)) ([564f5ed](564f5ed))
    * **api:** Added support for changing email of users ([#233](#233)) ([5ea9a10](5ea9a10))
    * implemented auth, ui for most, and fixed cors ([#217](#217)) ([feace86](feace86))
    * **platfrom:** add delete method in api client ([#225](#225)) ([55cf09f](55cf09f))
    * **postman:** add example for get_self and update_self ([e015acf](e015acf))
    * **web:** Add and link privacy and tnc page ([#226](#226)) ([ec81eb9](ec81eb9))
    
    ### 🐛 Bug Fixes
    
    * **web:** docker next config not found ([#228](#228)) ([afe3160](afe3160))
    
    ### 📚 Documentation
    
    * Added docs regarding postman, and refactored architecture diagrams ([f1c9777](f1c9777))
    * Fix typo in organization-of-code.md ([#234](#234)) ([11244a2](11244a2))
    
    ### 🔧 Miscellaneous Chores
    
    * **api:** Get feedback forward email from process.env ([#236](#236)) ([204c9d1](204c9d1))
    * **postman:** Initialized postman ([bb76384](bb76384))
    * **release:** Update changelog config ([af91283](af91283))
    * Remove swagger docs ([#220](#220)) ([7640299](7640299))
    
    ### 🔨 Code Refactoring
    
    * **api:** Replaced OTP code from alphanumeric to numeric ([#230](#230)) ([f16162a](f16162a))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 1.4.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    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.

    Create S3 client provider
    2 participants