fix(application-system): using aws-sdkv3 instead of v2 and refactoring #15842
Conversation
WalkthroughThe pull request introduces substantial changes across multiple components of the application, primarily focusing on the integration and refactoring of AWS S3-related services. Key modifications include the adoption of a new Changes
Sequence Diagram(s)sequenceDiagram
participant A as Application
participant S3 as AwsService
participant AS3 as AttachmentS3Service
A->>AS3: Request to add attachment
AS3->>S3: Upload file to S3
S3-->>AS3: Confirmation of upload
AS3-->>A: Attachment added confirmation
A->>AS3: Request to get attachment
AS3->>S3: Retrieve file from S3
S3-->>AS3: File data
AS3-->>A: Return attachment data
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (4)
Files not reviewed due to no reviewable changes (1)
Files skipped from review as they are similar to previous changes (1)
Additional context usedPath-based instructions (2)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
HjorturJ
changed the title
Datadog ReportAll test runs ✅ 101 Total Test Services: 0 Failed, 99 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
|
Affected services are: air-discount-scheme-api,air-discount-scheme-backend,api,application-system-api,download-service,financial-aid-api,financial-aid-backend,financial-aid-open-api,github-actions-cache,icelandic-names-registry-backend,judicial-system-api,judicial-system-digital-mailbox-api,judicial-system-backend,judicial-system-message-handler,judicial-system-robot-api,judicial-system-scheduler,judicial-system-xrd-api,license-api,reference-backend,regulations-admin-backend,services-auth-admin-api,services-auth-ids-api,services-auth-delegation-api,services-auth-personal-representative,services-auth-personal-representative-public,services-auth-public-api,services-contentful-entry-tagger,services-documents,services-endorsements-api,services-form-system,services-search-indexer,services-sessions,services-university-gateway,services-user-notification,services-user-profile,services-xroad-collector,skilavottord-ws,air-discount-scheme-web,auth-admin-web,consultation-portal,contentful-apps,financial-aid-web-osk,financial-aid-web-veita,judicial-system-web,skilavottord-web,web,application-system-form,island-ui-storybook,portals-admin,service-portal,system-e2e,external-contracts-tests, Deployed services: application-system-api,application-system-form,service-portal,service-portal-api,portals-admin,consultation-portal,api,web,xroad-collector,skilavottord-web,skilavottord-ws,services-documents,island-ui-storybook,download-service,icelandic-names-registry-backend,endorsement-system-api,air-discount-scheme-web,air-discount-scheme-backend,air-discount-scheme-api,regulations-admin-backend,external-contracts-tests,application-system-api-worker,license-api,services-sessions,services-sessions-worker,services-sessions-cleanup,services-university-gateway,services-university-gateway-worker. |
HjorturJ
added
deploy-feature
…and-is/island.is into fix/attachment-and-s3--consolidation
HjorturJ
changed the title
HjorturJ
changed the title
HjorturJ
changed the title
…and-is/island.is into fix/attachment-and-s3--consolidation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
libs/nest/aws/src/lib/aws.service.ts (5)
33-39: LGTM, but consider adding a default value for theencodingparameter.The
getFileContentmethod is a useful addition to the class as it provides a convenient way to get the content of a file as a string.The method handles the case where the file is not found by returning
undefinedinstead of throwing an error, which is a good practice.Consider adding a default value for the
encodingparameter. For example:public async getFileContent( BucketKeyPairOrFilename: BucketKeyPair | string, encoding: string = 'utf-8', ): Promise<string | undefined> { // ... }
42-75: LGTM, but consider adding a timeout to the upload operation.The changes to the
uploadFilemethod are approved.The use of the
Uploadclass is a good practice as it provides a higher-level abstraction for uploading files to S3.The method logs errors using the injected
Loggerinstance, which is a good practice.Consider adding a timeout to the upload operation to prevent the method from hanging indefinitely. For example:
const parallelUploads3 = new Upload({ client: this.s3Client, params: uploadParams, tags: [], // Add tags to the upload operation queueSize: 4, // Increase the queue size to improve performance partSize: 5 * 1024 * 1024, // Increase the part size to improve performance leavePartsOnError: false, // Delete parts on error to avoid incurring additional costs }); await parallelUploads3.done({ timeout: 60 * 1000 }); // Add a timeout of 60 seconds
78-93: LGTM, but consider adding a check for theexpirationOverrideparameter.The changes to the
getPresignedUrlmethod are approved.The use of the
getSignedUrlfunction is a good practice as it provides a more secure way to generate presigned URLs.The method accepts an optional
expirationOverrideparameter to override the default expiration time of 120 minutes, which is a good practice.Consider adding a check to ensure that the
expirationOverrideparameter is a positive number. For example:const expiration = expirationOverride && expirationOverride > 0 ? expirationOverride : oneMinute * 120;
96-123: LGTM, but consider adding a timeout to theHeadObjectCommand.The
fileExistsmethod is a useful addition to the class as it provides a convenient way to check if a file exists in S3.The method handles the case where the file does not exist by returning
falseinstead of throwing an error, which is a good practice.The method logs errors using the injected
Loggerinstance, which is a good practice.Consider adding a timeout to the
HeadObjectCommandto prevent the method from hanging indefinitely. For example:const command = new HeadObjectCommand({ Bucket: bucket, Key: key, }); const results = await this.s3Client.send(command, { timeout: 5000 }); // Add a timeout of 5 seconds
125-145: LGTM, but consider adding a check to ensure that the file exists before attempting to delete it.The
deleteObjectmethod is a useful addition to the class as it provides a convenient way to delete a file from S3.The method logs errors using the injected
Loggerinstance, which is a good practice.Consider adding a check to ensure that the file exists before attempting to delete it. For example:
public async deleteObject(BucketKeyPairOrFilename: BucketKeyPair | string) { try { const { bucket, key } = this.getBucketKey(BucketKeyPairOrFilename); const exists = await this.fileExists(BucketKeyPairOrFilename); if (!exists) { this.logger.warn(`File ${key} does not exist in bucket ${bucket}`); return; } const result = await this.s3Client.send( new DeleteObjectCommand({ Bucket: bucket, Key: key, }), ); // ... } catch (error) { // ... } }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- apps/application-system/api/src/app/modules/application/template-api.service.ts (1 hunks)
- apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (2 hunks)
- libs/application/api/files/src/lib/file.service.ts (6 hunks)
- libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/shared/shared.module.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (7 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/estate/estate.service.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.utils.ts (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (5 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.ts (3 hunks)
- libs/nest/aws/src/lib/aws.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- apps/application-system/api/src/app/modules/application/template-api.service.ts
- apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
- libs/application/api/files/src/lib/file.service.ts
- libs/application/template-api-modules/src/lib/modules/shared/shared.module.ts
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts
- libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.service.ts
- libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.ts
Additional context used
Path-based instructions (5)
libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/nest/aws/src/lib/aws.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/estate/estate.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.utils.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (18)
libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (6)
22-28: Excellent use of dependency injection!Injecting the dependencies (
AwsService,ConfigService, andBaseTemplateApiApplicationService) in the constructor is a great improvement. It promotes better separation of concerns, facilitates testing, and makes the code more maintainable.
31-31: Good use of a more specific type!Updating the method signature to use
ApplicationWithAttachmentsinstead ofApplicationis a good change. It improves type safety and makes the code more expressive by indicating that the application object includes attachment-related properties.
71-80: Great improvements in type safety and abstraction!The changes in the
getAttachmentContentAsBase64method are excellent:
- Using
ApplicationWithAttachmentsimproves type safety.- Using
awsService.getFileabstracts the S3 client usage, making the code more maintainable and less coupled to the specific AWS SDK version.
82-94: Excellent addition of thegetAttachmentContentAsBlobmethod!Adding this method enhances the service's capabilities by providing functionality to retrieve attachment content as a Blob. It follows a similar pattern to the
getAttachmentContentAsBase64method, usingawsService.getFilefor a more maintainable and less coupled implementation.
96-113: Great addition of thegetAttachmentUrlmethod!Adding this method enhances the service's capabilities by providing functionality to generate signed URLs for attachments. The error handling for invalid expiration values and missing bucket configurations improves the robustness of the method.
115-131: Excellent addition of theaddAttachmentmethod!Adding this method enhances the service's capabilities by providing functionality to add attachments to the application. Using the
applicationService.saveAttachmentToApplicatonmethod abstracts the saving logic, making the code more maintainable and less coupled to the specific implementation.libs/application/template-api-modules/src/lib/modules/templates/estate/estate.service.ts (2)
35-35: LGTM!The import statement for
AwsServiceis correct and necessary.
44-44: Refactoring approved!The refactoring of the
EstateTemplateServiceto use the injectedAwsServicefor retrieving file content is a positive change that enhances modularity, testability, and code clarity. The error handling remains intact.Key improvements:
- Decoupling S3 operations by injecting
AwsServiceinto the constructor.- Streamlining file content retrieval by using the
getFileContentmethod ofAwsService.- Maintaining error handling.
Great job on this refactoring! It aligns well with best practices for dependency injection and separation of concerns.
Also applies to: 273-277
libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.utils.ts (2)
16-16: LGTM!The import statement is correctly importing the
AwsServicefrom the specified path.
37-37: LGTM!The changes in the
insuranceToXMLfunction look good:
- The
bucketServiceparameter has been correctly replaced withawsServiceof typeAwsServicein the function signature.- The
getFileContentmethod is now correctly called on theawsServiceinstance to retrieve file content from S3.- The error handling ensures that an error is thrown if the file content retrieval fails, improving the robustness of the code.
Also applies to: 96-102, 106-106
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (8)
118-118: LGTM!The addition of the
awsServicedependency in the constructor promotes dependency injection and modularity. It's a good practice to use a dedicated service for AWS operations.
Line range hint
407-584:
Line range hint
586-633:
Line range hint
635-1104:
Line range hint
1106-1247:
Line range hint
1249-1277:
Line range hint
1279-1296:
389-395: LGTM, but verify thegetFileContentimplementation.The change to use
this.awsService.getFileContent()aligns with the shift towards using theAwsServicefor AWS operations. It's a good practice to centralize AWS-related functionality in a dedicated service.However, ensure that the
getFileContentmethod ofAwsServiceis implemented correctly to handle the retrieval of file content from S3. Verify that it returns the file content in the expected format (base64 in this case) and handles errors appropriately.Run the following script to verify the
getFileContentimplementation:Verification successful
The
getFileContentmethod is correctly implemented in theAwsServiceclass.The method is located in
libs/nest/aws/src/lib/aws.service.tsand handles S3 operations to retrieve file content, returning it in the specified encoding format. This aligns with the intended functionality and centralizes AWS-related operations within theAwsService.
- File:
libs/nest/aws/src/lib/aws.service.ts- Method:
getFileContentScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `AwsService.getFileContent()`. # Test: Search for the method implementation. Expect: The method should handle S3 operations and return the file content in base64 format. ast-grep --lang typescript --pattern $'class AwsService { $$$ getFileContent($_,$_) { $$$ } $$$ }'Length of output: 111
Script:
#!/bin/bash # Locate files containing the AwsService class fd -e ts 'AwsService' --exec rg 'class AwsService' -l # Search for the getFileContent method within the located files fd -e ts 'AwsService' --exec rg 'getFileContent' -A 5Length of output: 105
Script:
#!/bin/bash # Search for the AwsService class definition across the entire codebase rg 'class AwsService' -t ts -A 5 # Search for the getFileContent method across the entire codebase rg 'getFileContent' -t ts -A 5Length of output: 10672
.../template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.service.ts
Outdated
Show resolved
Hide resolved
...e-api-modules/src/lib/modules/templates/health-insurance-declaration/attachments/provider.ts
Show resolved
Hide resolved
...s/templates/data-protection-complaint/attachments/providers/applicationAttachmentProvider.ts
Outdated
Show resolved
Hide resolved
...ates/complaints-to-althingi-ombudsman/attachments/providers/applicationAttachmentProvider.ts
Outdated
Show resolved
Hide resolved
...les/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts
Outdated
Show resolved
Hide resolved
libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts
Outdated
Show resolved
Hide resolved
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
libs/nest/aws/src/lib/aws.service.ts (2)
77-92: LGTM with a minor suggestion!The
getPresignedUrlmethod is correctly implemented and follows the best practices for generating presigned URLs using the AWS SDK v3. The method extracts thebucketandkeyusing thegetBucketKeymethod, constructs aGetObjectCommand, and uses thegetSignedUrlfunction from@aws-sdk/s3-request-presignerto generate the presigned URL.Consider moving the default expiration time (
oneMinute * 120) to a constant or configuration value to improve readability and maintainability.
123-142: LGTM with a minor suggestion!The
deleteObjectmethod is correctly implemented and provides a straightforward way to delete an object from the S3 bucket. The method extracts thebucketandkeyusing thegetBucketKeymethod, constructs aDeleteObjectCommand, and sends the command using thes3Client. The error handling ensures that unexpected HTTP status codes are caught and logged appropriately.Consider throwing an error instead of just logging it when an unexpected HTTP status code is encountered during object deletion. This would allow the caller to handle the error more effectively.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/data-protection-complaint/attachments/providers/applicationAttachmentProvider.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.service.ts (2 hunks)
- libs/nest/aws/src/lib/aws.service.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- libs/application/template-api-modules/src/lib/modules/templates/data-protection-complaint/attachments/providers/applicationAttachmentProvider.ts
Additional context used
Path-based instructions (3)
libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/nest/aws/src/lib/aws.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (17)
libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.service.ts (3)
19-19: LGTM!The import statement is necessary and correctly added.
67-67: LGTM!The change to use
awsServiceinstead ofbucketServicein theinsuranceToXMLfunction call is consistent with the updated constructor and the transition to usingAwsService. The change looks good.
26-26: Verify that all usages ofbucketServicehave been replaced withawsService.The change in the constructor to inject
AwsServiceinstead ofBucketServiceis consistent with the updated import statement. This change suggests a transition to utilizing AWS services for storage, which may enhance functionality or performance.Please ensure that all usages of
bucketServicehave been replaced withawsServicein the rest of the file to maintain consistency and avoid any potential issues.Run the following script to verify the usage of
bucketServiceandawsService:Verification successful
All usages of
bucketServicehave been replaced withawsService.The verification confirms that
bucketServiceis no longer used in the file, andawsServiceis correctly utilized. This ensures consistency with the constructor change and suggests a successful transition to using AWS services.
- No occurrences of
bucketServicefound.awsServiceis used as expected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `bucketService` is not used and `awsService` is used instead. # Test 1: Search for the usage of `bucketService`. Expect: No occurrences. rg --type typescript $'bucketService' libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.service.ts # Test 2: Search for the usage of `awsService`. Expect: At least one occurrence. rg --type typescript $'awsService' libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.service.tsLength of output: 358
Script:
#!/bin/bash # Description: Verify that `bucketService` is not used and `awsService` is used instead. # Test 1: Search for the usage of `bucketService`. Expect: No occurrences. rg 'bucketService' libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.service.ts # Test 2: Search for the usage of `awsService`. Expect: At least one occurrence. rg 'awsService' libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.service.tsLength of output: 311
libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (6)
22-28: LGTM!The use of dependency injection in the constructor is a good practice. It promotes better separation of concerns and facilitates more flexible testing and configuration.
31-31: LGTM!The change to use
ApplicationWithAttachmentsinstead ofApplicationin the method signature is appropriate as it likely includes additional attachment-related properties.
69-78: LGTM!The changes to the
getAttachmentContentAsBase64method are an improvement:
- The use of
ApplicationWithAttachmentsis consistent with other method signature changes.- The use of
AmazonS3URIandawsService.getFileprovides a more robust way to retrieve the file content.
80-92: LGTM!The addition of the
getAttachmentContentAsBlobmethod is a useful enhancement:
- It provides the functionality to retrieve the attachment content as a Blob object.
- The implementation is correct and follows a similar pattern to the
getAttachmentContentAsBase64method.
94-111: LGTM!The addition of the
getAttachmentUrlmethod is a valuable enhancement:
- It provides the functionality to generate signed URLs for accessing attachments.
- The error handling for invalid expiration values and missing bucket configurations is appropriate.
- The use of
awsService.getPresignedUrlis the correct way to generate signed URLs.
113-129: LGTM!The addition of the
addAttachmentmethod is a helpful enhancement:
- It provides a convenient way to add attachments to the application.
- The use of
applicationService.saveAttachmentToApplicatonis appropriate for saving the attachment.libs/nest/aws/src/lib/aws.service.ts (8)
1-12: LGTM!The imports from the AWS SDK v3 and other dependencies are correctly added and necessary for the refactored code.
14-17: LGTM!The
BucketKeyPairinterface is correctly defined and will be useful for the refactored code.
21-24: LGTM!The constructor is correctly defined and the dependencies (
S3Clientand logger) are properly injected.
26-30: LGTM!The
getFilemethod is correctly implemented and handles the input parameter (BucketKeyPairOrFilename) flexibly by accepting either aBucketKeyPairobject or a string. The method extracts thebucketandkeyusing thegetBucketKeymethod and calls thegetFileResponsemethod to fetch the file.
33-39: LGTM!The
getFileContentmethod is correctly implemented and provides a convenient way to fetch the file content as a string. The method extracts thebucketandkeyusing thegetBucketKeymethod, calls thegetFileResponsemethod to fetch the file, and transforms the response body to a string using thetransformToStringmethod with the optionalencodingparameter.
42-74: LGTM!The
uploadFilemethod is correctly implemented and leverages theUploadclass from@aws-sdk/lib-storagefor efficient parallel uploads. The method extracts thebucketandkeyusing thegetBucketKeymethod, constructs the upload parameters, and performs the upload. The error handling and logging are also properly implemented, ensuring that any errors during the upload process are caught and logged appropriately.
95-121: LGTM!The
fileExistsmethod is correctly implemented and provides a reliable way to check if a file exists in the S3 bucket. The method extracts thebucketandkeyusing thegetBucketKeymethod, constructs aHeadObjectCommand, and sends the command using thes3Client. The error handling covers different scenarios (404 and 403 status codes) and ensures proper logging of unexpected errors.
144-168: LGTM!The private methods
getFileResponseandgetBucketKeyare correctly implemented and provide reusable functionality for the other methods in theAwsServiceclass.The
getFileResponsemethod encapsulates the logic for fetching a file from S3 by accepting abucketandkey, constructing aGetObjectCommand, sending the command using thes3Client, and returning the response orundefinedif an error occurs.The
getBucketKeymethod handles the parsing of the input parameter (BucketKeyPairOrFilename) to extract thebucketandkey. It either directly returns theBucketKeyPairobject or parses the string using theAmazonS3URIfunction.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (1)
58-64: Consider renaminggetFilenameFromApplicationto a more appropriate name.The changes are correctly implemented. However, as mentioned in the past review comment, the naming of
getFilenameFromApplicationis a bit confusing as it returns a URL. Consider renaming it to a more appropriate name, such asgetAttachmentUrlFromApplication, to improve code readability.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (3 hunks)
- libs/nest/aws/src/lib/aws.service.ts (1 hunks)
Additional context used
Path-based instructions (2)
libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/nest/aws/src/lib/aws.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (17)
libs/application/template-api-modules/src/lib/modules/shared/services/attachment-s3.service.ts (7)
22-28: LGTM!The updated constructor using dependency injection for
AwsService,ConfigService, andBaseTemplateApiApplicationServiceis a good architectural improvement. It promotes better separation of concerns and facilitates more flexible testing and configuration.
31-31: LGTM!The updated method signature using
ApplicationWithAttachmentsinstead ofApplicationis a good change. It indicates a more specific type that likely includes additional attachment-related properties.
54-54: LGTM!The updated method signature using
ApplicationWithAttachmentsinstead ofApplicationis a good change. It indicates a more specific type that likely includes additional attachment-related properties.
70-79: LGTM!The
getAttachmentContentAsBase64method is correctly implemented. It uses theAmazonS3URIlibrary to parse the S3 URI and retrieve the bucket and key, and then uses theawsService.getFilemethod to retrieve the file content and transform it to a base64 string.
81-93: LGTM!The
getAttachmentContentAsBlobmethod is correctly implemented. It uses theAmazonS3URIlibrary to parse the S3 URI and retrieve the bucket and key, and then uses theawsService.getFilemethod to retrieve the file content and transform it to a byte array. Finally, it creates a new Blob object using the byte array and the content type of the file.
95-112: LGTM!The
getAttachmentUrlmethod is correctly implemented. It checks if the expiration value is positive and rejects the promise if it's not. It also retrieves the S3 bucket name from the configuration service and rejects the promise if it's not found. Finally, it uses theawsService.getPresignedUrlmethod to generate a signed URL for the specified bucket and key. The error handling for invalid expiration values and missing bucket configurations is a good addition.
114-130: LGTM!The
addAttachmentmethod is correctly implemented. It uses theapplicationService.saveAttachmentToApplicatonmethod to save the attachment and accepts optional upload parameters for specifying the content type, disposition, and encoding.libs/nest/aws/src/lib/aws.service.ts (10)
1-12: LGTM!The imports are correctly updated to use the AWS SDK v3 and other necessary modules.
14-17: LGTM!The
BucketKeyPairinterface is correctly defined and improves the type safety of the code.
21-24: LGTM!The constructor correctly uses dependency injection to inject an instance of
S3Clientand a logger, which improves the testability and maintainability of the code.
26-30: LGTM!The
getFilemethod is correctly defined and uses thegetBucketKeymethod to extract thebucketandkeyfrom the parameter, which improves the flexibility of the method. The method also uses thegetFileResponsemethod to fetch the file from S3, which centralizes the logic for fetching files.
33-39: LGTM!The
getFileContentmethod is correctly defined and uses thegetBucketKeymethod to extract thebucketandkeyfrom the parameter, which improves the flexibility of the method. The method also uses thegetFileResponsemethod to fetch the file from S3, which centralizes the logic for fetching files. The method then transforms the response body to a string using thetransformToStringmethod, which is a useful utility method.
42-74: LGTM!The
uploadFilemethod is correctly defined and uses thegetBucketKeymethod to extract thebucketandkeyfrom the parameter, which improves the flexibility of the method. The method also uses theUploadclass from the AWS SDK v3 to upload the file to S3, which is a more efficient and flexible way to upload files compared to the AWS SDK v2. The method also catches any errors that occur during the upload and logs them using the logger, which improves the error handling of the method.
77-92: LGTM!The
getPresignedUrlmethod is correctly defined and uses thegetBucketKeymethod to extract thebucketandkeyfrom the parameter, which improves the flexibility of the method. The method also uses thegetSignedUrlfunction from the AWS SDK v3 to generate a presigned URL for the file, which is a more efficient and flexible way to generate presigned URLs compared to the AWS SDK v2. The method also allows the expiration time of the presigned URL to be overridden using theexpirationOverrideparameter, which is a useful feature.
95-124: LGTM!The
fileExistsmethod is correctly defined and uses thegetBucketKeymethod to extract thebucketandkeyfrom the parameter, which improves the flexibility of the method. The method also uses theHeadObjectCommandclass from the AWS SDK v3 to check if the file exists, which is a more efficient and flexible way to check if a file exists compared to the AWS SDK v2. The method also handles any errors that occur during the check based on thehttpStatusCodeof the error, which improves the error handling of the method.
126-145: LGTM!The
deleteObjectmethod is correctly defined and uses thegetBucketKeymethod to extract thebucketandkeyfrom the parameter, which improves the flexibility of the method. The method also uses theDeleteObjectCommandclass from the AWS SDK v3 to delete the file, which is a more efficient and flexible way to delete files compared to the AWS SDK v2. The method also checks thehttpStatusCodeof the response to determine if the file was deleted successfully, which improves the error handling of the method. The method also catches any errors that occur during the deletion and logs them using the logger, which further improves the error handling of the method.
147-162: LGTM!The
getFileResponsemethod is correctly defined and uses the `Get
…ment-and-s3--consolidation
…and-is/island.is into fix/attachment-and-s3--consolidation
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
libs/nest/aws/src/lib/aws.service.ts (1)
79-94: LGTM with a suggestion!The changes to the
getPresignedUrlmethod look good. However, consider removing the TODO comment and deciding on a default expiration time for the presigned URLs.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- libs/application/api/files/src/lib/file.service.spec.ts (8 hunks)
- libs/application/template-api-modules/src/lib/modules/shared/shared.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.module.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/estate/estate.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.module.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.module.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.module.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.module.ts (2 hunks)
- libs/nest/aws/src/lib/aws.service.spec.ts (1 hunks)
- libs/nest/aws/src/lib/aws.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- libs/application/template-api-modules/src/lib/modules/shared/shared.module.ts
- libs/application/template-api-modules/src/lib/modules/templates/health-insurance/health-insurance.module.ts
Additional context used
Path-based instructions (9)
libs/application/template-api-modules/src/lib/modules/templates/estate/estate.module.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.module.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.module.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.module.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.module.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.module.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/nest/aws/src/lib/aws.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/nest/aws/src/lib/aws.service.spec.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/api/files/src/lib/file.service.spec.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
Additional comments not posted (41)
libs/application/template-api-modules/src/lib/modules/templates/estate/estate.module.ts (2)
9-9: LGTM!The
AwsModuleimport is correctly written and suggests that AWS services will be integrated into this module.
17-17: Verify the adherence to the additional instructions.The addition of
AwsModuleto the imports array is syntactically correct and enhances the module's functionality by integrating AWS services. However, please ensure that the following points from the additional instructions are addressed:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices.
libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.module.ts (1)
6-6: LGTM!The
AwsModuleis correctly added to the imports array of thePSignSubmissionModule. This change enhances the module's functionality by integrating AWS services, which is consistent with the PR objectives and the AI-generated summary.The change also adheres to the additional instructions for files matching the
libs/**/*pattern:
- The module is reusable across different NextJS apps.
- TypeScript is used for defining the module configuration.
- The module is effectively tree-shaken and bundled.
Also applies to: 15-15
libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.module.ts (2)
9-9: LGTM!The
AwsModuleimport is correctly added, following the project's import style. This change suggests that AWS services will be integrated into theOperatingLicenseModule.
18-18: Verify the usage of AWS services in theOperatingLicenseService.Adding the
AwsModuleto theimportsarray is the correct way to integrate it into the module's dependency injection system. This change will make AWS services available for injection in theOperatingLicenseServiceand other providers of this module, facilitating cloud-related operations or features in the context of operating licenses.Please ensure that the injected AWS services are being used correctly in the
OperatingLicenseServiceand other relevant parts of the codebase.Run the following script to verify the usage of AWS services:
Verification successful
AWS services are correctly used in the
OperatingLicenseService.The
AwsServiceis properly imported and injected into theOperatingLicenseService, confirming that AWS services are being utilized as intended. This pattern is consistent across other services in the codebase, indicating correct integration of theAwsModule.
OperatingLicenseServiceimports and usesAwsService.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of AWS services in the `OperatingLicenseService` and other relevant parts of the codebase. # Test 1: Search for the usage of AWS services in the `OperatingLicenseService`. Expect: At least one occurrence. rg --type typescript -A 5 $'AwsService|S3Client' libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.service.ts # Test 2: Search for the usage of AWS services in other relevant parts of the codebase. Expect: At least one occurrence. rg --type typescript -A 5 $'AwsService|S3Client' $(fd --type file --extension ts libs/application/template-api-modules/src/lib/modules/templates/operating-license)Length of output: 1024
Script:
#!/bin/bash # Description: Verify the usage of AWS services in the `OperatingLicenseService` and other relevant parts of the codebase. # Test 1: Search for the usage of AWS services in the `OperatingLicenseService`. Expect: At least one occurrence. rg --type ts -A 5 $'AwsService|S3Client' libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.service.ts # Test 2: Search for the usage of AWS services in other relevant parts of the codebase. Expect: At least one occurrence. fd --type file --extension ts libs/application/template-api-modules/src/lib/modules/templates/operating-license | xargs rg --type ts -A 5 $'AwsService|S3Client'Length of output: 41061
libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.module.ts (2)
12-12: LGTM!The import statement for
AwsModuleis correct. It is likely needed for utilizing AWS services or functionalities within theSocialInsuranceAdministrationModule.
23-23: Looks good!Adding
AwsModuleto the imports array of theSocialInsuranceAdministrationModuleis the correct way to make it available for dependency injection. This change aligns with the PR objective of refactoring the usage of S3 clients by consolidating them into a single service class.libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.module.ts (2)
14-14: LGTM!The import statement for
AwsModuleis correctly added.
26-26: Verify the proper configuration and usage ofAwsModule.The addition of
AwsModuleto theimportsarray is correct and will provide theParentalLeaveModulewith access to AWS-related functionalities. However, please ensure that theAwsModuleis properly configured and utilized within theParentalLeaveModuleto leverage the AWS capabilities effectively.Run the following script to verify the usage of
AwsModule:libs/application/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.module.ts (2)
9-9: LGTM!The import statement for
AwsModuleis correct and follows the expected syntax.
28-28: Verify the usage ofAwsModuleand the removal ofS3ServiceandS3provider.The addition of
AwsModuleto theimportsarray is correct and aligns with the objective of refactoring the usage of S3 clients. However, please ensure that:
- The
AwsModuleprovides the necessary S3 service that was previously provided by theS3ServiceandS3provider.- The removal of
S3ServiceandS3provider from theprovidersarray does not introduce any issues or break any existing functionality.Run the following script to verify the usage of
AwsModuleand the removal ofS3ServiceandS3provider:libs/nest/aws/src/lib/aws.service.ts (9)
23-26: LGTM!The constructor changes improve dependency injection and logging practices.
28-32: LGTM!The new
getFilemethod provides a convenient way to fetch files from S3 and follows good practices by centralizing the logic in a separate method.
35-41: LGTM!The new
getFileContentmethod is a useful addition that provides a convenient way to fetch file content as a string.
44-76: LGTM!The changes to the
uploadFilemethod, including the use of theUploadclass for parallel uploads and the improved error handling and logging, are great improvements.
97-126: LGTM!The new
fileExistsmethod is a useful addition, and the robust error handling for different HTTP status codes is a good practice.
128-147: LGTM!The new
deleteObjectmethod is a necessary addition, and the error handling and logging are good practices.
149-156: LGTM!Extracting the
getBucketKeymethod is a good practice to avoid code duplication, as it is used in multiple methods.
158-172: LGTM!Extracting the logic for fetching files into a separate
getFileResponsemethod is a good practice, and the error handling and logging are beneficial for diagnostics.
1-12: LGTM!Updating the imports to use the new AWS SDK v3 modules is necessary for the transition.
libs/nest/aws/src/lib/aws.service.spec.ts (8)
45-59: LGTM!The test case is well-written and covers the positive scenario of retrieving a file from an S3 bucket. It mocks the necessary dependencies, calls the
getFilemethod ofAwsService, and asserts that the returned result matches the expected value.
61-67: LGTM!The test case is well-written and covers the negative scenario of handling an error when retrieving a file from an S3 bucket. It mocks the
GetObjectCommandto reject with an error, calls thegetFilemethod ofAwsService, and asserts that the returned result isundefined.
69-82: LGTM!The test case is well-written and covers various encoding scenarios for retrieving file content from an S3 bucket. It uses
it.eachto test multiple encoding values, mocks theGetObjectCommandto return a specific result, calls thegetFileContentmethod ofAwsService, and asserts that the returned result matches the expected value based on the provided encoding.
84-93: LGTM!The test case is well-written and covers the scenario of retrieving a presigned URL for an S3 object. It mocks the
getSignedUrlfunction to return a specific result, calls thegetPresignedUrlmethod ofAwsService, and asserts that the returned result matches the expected value.
95-103: LGTM!The test case is well-written and covers the scenario of checking if an object exists in an S3 bucket. It mocks the
HeadObjectCommandto resolve with a specific metadata, calls thefileExistsmethod ofAwsService, and asserts that the returned result istrue.
105-113: LGTM!The test case is well-written and covers various scenarios where the object doesn't exist in an S3 bucket. It uses
it.eachto test multiple HTTP status codes, mocks theHeadObjectCommandto resolve with specific metadata based on the status code, calls thefileExistsmethod ofAwsService, and asserts that the returned result isfalse.
115-135: LGTM!The test cases are well-written and cover various scenarios related to deleting an object from an S3 bucket. They use
it.eachto test multiple HTTP status codes, mock theDeleteObjectCommandto resolve with specific metadata based on the status code, call thedeleteObjectmethod ofAwsService, and assert the expected behavior.The first test case asserts that the method resolves without an error for successful status codes, while the second test case asserts that an error is logged for unexpected status codes.
137-149: LGTM!The test case is well-written and covers the scenario of extracting bucket and key information from different input formats. It calls the
getBucketKeymethod ofAwsServicewith a bucket-key pair and a file URI, and asserts that the returned bucket and key values match the expected values.libs/application/api/files/src/lib/file.service.spec.ts (13)
8-8: LGTM!The import statement for
AwsModuleandAwsServiceis correct and follows the proper syntax.
16-16: LGTM!The import statement for
GetObjectCommandOutputis correct and follows the proper syntax.
17-17: LGTM!The import statement for
Readableis correct and follows the proper syntax.
120-120: LGTM!The addition of
AwsModuleto the list of imports in the testing module is correct and necessary for using theAwsServicein the tests.
130-130: LGTM!The addition of
FileServiceto the list of providers in the testing module is correct and necessary for testing theFileService.
136-137: LGTM!The mocking of the
getFileContentmethod ofAwsServiceis correct and necessary for testing theFileServicewithout making actual calls to AWS. The mock implementation returns a string value, which is likely the expected type of the file content.
180-181: LGTM!The change in the method signature of
uploadFileto use an object containingbucketandkeyproperties is consistent with the alterations mentioned in the AI-generated summary. The object destructuring syntax is correct and improves the readability of the code.
189-189: LGTM!The change in the method signature of
getPresignedUrlto use an object containingbucketandkeyproperties is consistent with the alterations mentioned in the AI-generated summary. The object destructuring syntax is correct and improves the readability of the code.
201-203: LGTM!The change in the method signature of
getFileContentto use an object containingbucketandkeyproperties is consistent with the alterations mentioned in the AI-generated summary. The object destructuring syntax is correct and improves the readability of the code. The addition of the'binary'argument is likely necessary for the correct handling of the file content.
227-228: LGTM!The mocking of the
getFileContentmethod ofAwsServiceto return an empty string is likely done to test the error handling scenario when the file content is missing. The mock implementation is correct and necessary for testing the error handling in theFileService.
238-240: LGTM!The change in the method signature of
getFileContentto use an object containingbucketandkeyproperties is consistent with the alterations mentioned in the AI-generated summary. The object destructuring syntax is correct and improves the readability of the code. The addition of the'binary'argument is likely necessary for the correct handling of the file content.
274-274: LGTM!The change in the method signature of
getPresignedUrlto use an object containingbucketandkeyproperties is consistent with the alterations mentioned in the AI-generated summary. The object destructuring syntax is correct and improves the readability of the code.
Line range hint
1-346: Overall assessment: The changes in the file are well-structured and improve the code quality.The changes in the
file.service.spec.tsfile are primarily focused on updating the usage of theAwsServicemethods to align with the new method signatures. The changes are consistent with the alterations mentioned in the AI-generated summary and improve the readability of the code by using object destructuring syntax.The addition of the
AwsModuleto the list of imports in the testing module is necessary for using theAwsServicein the tests. The mocking of theAwsServicemethods is done correctly and is necessary for testing theFileServicewithout making actual calls to AWS.Overall, the changes in the file are well-structured, follow best practices, and improve the code quality.
...
Attach a link to issue if relevant
What
Refactor usages of s3 clients into a single service class within the template-api-modules backend.
Why
To consolidate s3 useage to make it easier to pinpoint bugs in the future and hopefully make it easier to make new stuff with predefined functions for common use cases of s3 buckets.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
package.jsonto enhance AWS functionality.