chore(application-system): using aws-sdkv3 initial implementation #16281
Conversation
WalkthroughThe changes in this pull request focus on enhancing AWS service integration within the application. Key updates include refactoring methods in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (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
|
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 (9)
|
|
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. |
…/island.is into chore/aws-v3-s3-implementation
…-is/island.is into chore/aws-v3-s3-implementation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (16)
libs/nest/aws/src/lib/aws.module.ts (1)
7-13: LGTM: Updated providers with S3ClientThe addition of S3Client as a provider is correct and necessary for dependency injection. Using
useValuewithnew S3Client()creates a singleton instance, which is generally a good practice.Consider using
useFactoryinstead ofuseValuefor more flexibility in configuration, especially if you need to pass options to the S3Client constructor in the future. For example:{ provide: S3Client, useFactory: () => new S3Client(/* potential config options */), }This approach would allow for easier configuration changes and environment-specific setups in the future.
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.module.ts (2)
26-26: LGTM: AwsModule correctly added to imports. Consider sorting imports alphabetically.The addition of
AwsModuleto the imports array is consistent with the new import statement and aligns with the PR objective of implementing AWS SDK v3.For better readability and consistency, consider sorting the imports alphabetically. Here's a suggested change:
imports: [ + ApplicationApiCoreModule, + AwsModule, NationalRegistryClientModule, SharedTemplateAPIModule.register(config), SmsModule, VMSTModule, - ApplicationApiCoreModule, - AwsModule, ],
Line range hint
1-38: Confirm adherence to coding guidelines for libs//* files**The module adheres well to the coding guidelines:
- It's located in the
libs/directory, promoting reusability across different NextJS apps.- It uses TypeScript effectively with proper import/export statements and type definitions.
- The specific import of
AwsModulesupports effective tree-shaking.To further improve type safety and exportability:
Consider explicitly exporting the
BaseTemplateAPIModuleConfigtype used in theregistermethod:export { BaseTemplateAPIModuleConfig } from '../../../types'This would enhance the module's type safety and make it easier for consumers to use the correct configuration type.
apps/application-system/api/src/app/modules/application/template-api.service.ts (1)
45-47: LGTM! Consider adding type annotation for improved clarity.The changes to the
uploadFilemethod call align well with AWS SDK v3's object-based parameter approach. This update improves code readability and maintainability while maintaining the same functionality.Consider adding a type annotation to the object parameter for improved clarity:
await this.awsService.uploadFile( buffer, { bucket: uploadBucket, key: s3key } as const, uploadParameters, )This change would make the parameter structure more explicit and help prevent potential errors in the future.
libs/application/api/files/src/lib/file.service.ts (3)
57-59: Approve changes with a suggestion for type safetyThe updates to
fileExistsanduploadFilemethod calls align well with the transition to AWS SDK v3's object-based parameters. This change enhances code consistency and readability.To further improve type safety and maintainability, consider defining an interface for the bucket and key object:
interface S3Location { bucket: string; key: string; }Then use this interface in method calls:
if ((await this.awsService.fileExists(location as S3Location)) === false) { // ... await this.awsService.uploadFile( content, location as S3Location, // ... ) }This approach will make the code more robust and easier to maintain across the codebase.
Also applies to: 61-69
88-91: Approve change with suggestion for consistencyThe update to the
uploadFilemethod call in theuploadSignedFilemethod is consistent with the transition to object-based parameters. This change enhances code consistency and aligns with the AWS SDK v3 style.For improved consistency, consider using a single object for all parameters:
await this.awsService.uploadFile({ content: Buffer.from(file, 'binary'), location: { bucket, key: s3FileName }, options: { ContentEncoding: 'binary', ContentType: 'application/pdf', }, });This approach would make all AWS service method calls consistent throughout the file.
156-159: Approve change with suggestion for type safetyThe introduction of the
getFileContentmethod with object-based parameters is consistent with the AWS SDK v3 style and enhances code flexibility.To improve type safety and code clarity, consider defining an enum for the encoding type:
enum S3FileEncoding { BINARY = 'binary', UTF8 = 'utf8', // Add other encoding types as needed } // Usage const fileContent = await this.awsService.getFileContent( { bucket, key: s3FileName }, S3FileEncoding.BINARY );This approach will make the code more robust and self-documenting.
libs/application/api/files/src/lib/file.service.spec.ts (1)
Line range hint
1-377: Overall assessment: Code changes improve quality and consistency.The updates in this file consistently implement the transition to AWS SDK v3, improving code clarity and maintaining a uniform approach across the test suite. The changes adhere to the coding guidelines for the
libsdirectory, enhancing reusability and modularity. The object-based parameter structure for AWS service methods improves readability and reduces the likelihood of errors.To further improve the code:
- Consider adding type annotations for the object parameters to enhance type safety.
- Update the documentation comments if any exist for these methods to reflect the new parameter structure.
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.spec.ts (1)
Line range hint
1-524: Adherence to coding guidelines forlibs/**/*The file generally adheres to the coding guidelines:
- Reusability: As part of a library, this test suite contributes to the reusability of the ParentalLeaveService across different NextJS apps.
- TypeScript usage: The file effectively uses TypeScript for defining types and interfaces, enhancing code quality and maintainability.
However, to further improve adherence to guidelines:
- Consider extracting common test setup logic into shared helper functions to enhance reusability within the test suite.
- Ensure that any new AWS SDK v3 related code (when implemented) follows effective tree-shaking and bundling practices.
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (3)
686-689: Improved AWS S3 integration and error handlingThe changes to the
getPresignedUrlmethod call are good. Using an object parameter enhances code readability and reduces the likelihood of errors. The error handling has also been improved.Consider adding more specific error types for different failure scenarios. For example:
} catch (error) { this.logger.error(`Failed to export list ${listId}`, { error }) if (error instanceof NotFoundException) { throw error; } throw new InternalServerErrorException('Failed to export list'); }This approach provides more granular error responses to the client.
754-754: Consistent use of object parameters for AWS S3 operationsThe update to use an object parameter for bucket and key in the
uploadFileToS3method is consistent with the changes made in theexportListmethod. This improves code readability and maintainability.Consider enhancing the error handling to provide more context:
} catch (error) { this.logger.error(`Failed to upload file to S3`, { error, filename, bucket: environment.exportsBucketName }) throw new InternalServerErrorException('Error uploading file to S3'); }This will provide more detailed logging and a more specific error response.
Line range hint
1-754: Overall assessment of AWS SDK v3 integrationThe changes in this file successfully implement the transition to AWS SDK v3 for S3 operations. The modifications are focused on the
exportListanduploadFileToS3methods, updating them to use the new object-based parameter structure for S3 operations.Key points:
- The changes are consistent and well-implemented.
- Error handling has been maintained and slightly improved.
- The updates don't introduce any breaking changes to the service's public API.
- The modifications align with the PR objective of implementing AWS SDK v3.
As you continue to update other parts of the application to use AWS SDK v3, ensure consistent error handling and logging practices across all AWS service integrations. Consider creating a centralized AWS error handling utility to standardize error responses and logging for all AWS operations.
libs/nest/aws/src/lib/aws.service.ts (1)
85-85: Address TODO: Select default expiration for presigned URLsThere's a TODO comment indicating the need to select a default expiration time for presigned URLs. Defining a standard expiration ensures consistency and security across the application.
Would you like assistance in determining an appropriate default expiration time and implementing it?
libs/nest/aws/src/lib/aws.service.spec.ts (3)
110-110: Fix typo in test descriptionThere's a typo in the test description on line 110: "doesnt" should be "doesn't".
Apply this diff to correct the typo:
- 'should return false if the object doesnt exist in s3', + "should return false if the object doesn't exist in s3",
150-162: Add test cases for edge URLs ingetBucketKeymethodThe test for
getBucketKeycould include additional edge case URLs, such as URLs with query parameters or different S3 endpoint formats, to ensure robust parsing.Consider adding more test cases:
const edgeCaseUri = 'https://abc.s3.example.com/def?versionId=123' const resultFromEdgeCase = awsService.getBucketKey(edgeCaseUri) expect(resultFromEdgeCase.bucket).toEqual(bucketKeyPair.bucket) expect(resultFromEdgeCase.key).toEqual(bucketKeyPair.key)
145-145: Log the error message directly instead of creating a new ErrorWhen logging the error, it's not necessary to create a new
Errorobject with the message. Instead, log the message directly or pass the actual error object if available.Apply this diff to simplify the logging:
- Error('Unexpected http response when deleting object from S3'), + 'Unexpected HTTP response when deleting object from S3',Or if there's an error object:
- Error('Unexpected http response when deleting object from S3'), + actualError,This approach provides clearer logging without unnecessary object creation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
- 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.spec.ts (8 hunks)
- libs/application/api/files/src/lib/file.service.ts (6 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/parental-leave/parental-leave.service.spec.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (3 hunks)
- libs/nest/aws/src/lib/aws.module.ts (1 hunks)
- libs/nest/aws/src/lib/aws.service.spec.ts (1 hunks)
- libs/nest/aws/src/lib/aws.service.ts (1 hunks)
- package.json (2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
apps/application-system/api/src/app/modules/application/template-api.service.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
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."
libs/application/api/files/src/lib/file.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/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/parental-leave/parental-leave.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/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."
libs/nest/aws/src/lib/aws.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.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/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 (23)
libs/nest/aws/src/lib/aws.module.ts (2)
3-3: LGTM: Import of S3Client from AWS SDK v3The import of
S3Clientfrom@aws-sdk/client-s3is correct and aligns with the PR objective of implementing AWS SDK v3.
14-14: LGTM: Exported AwsService and S3ClientThe updated exports array now includes both
AwsServiceandS3Client, which increases the module's flexibility and reusability. This change aligns well with the coding guideline for reusability of components across different NextJS apps.libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.module.ts (2)
14-14: LGTM: Import statement is correctly formatted and follows best practices.The import of
AwsModulefrom '@island.is/nest/aws' is well-structured and follows TypeScript conventions. It imports a specific module, which is beneficial for effective tree-shaking.
Line range hint
1-38: Summary: AWS integration successfully implementedThe changes in this file successfully integrate the AWS module into the ParentalLeaveModule. This aligns with the PR objective of implementing AWS SDK v3. The modifications are minimal and maintain the existing structure of the module.
Key points:
AwsModuleis correctly imported and added to the module's imports.- The changes adhere to the coding guidelines for
libs/**/*files.- The module remains reusable and continues to use TypeScript effectively.
These changes lay the groundwork for using AWS services within the parental leave application system. Ensure that the actual usage of AWS services is implemented in the relevant service files.
libs/application/api/files/src/lib/file.service.ts (4)
72-72: Approve change to getPresignedUrlThe update to the
getPresignedUrlmethod call is consistent with the transition to object-based parameters. This change enhances code consistency and aligns with the AWS SDK v3 style.
138-138: Approve change to getPresignedUrlThe update to the
getPresignedUrlmethod call is consistent with the transition to object-based parameters. This change enhances code consistency and aligns with the AWS SDK v3 style.
260-260: Approve change to deleteObjectThe update to the
deleteObjectmethod call is consistent with the transition to object-based parameters. This change enhances code consistency and aligns with the AWS SDK v3 style.
Line range hint
1-300: Summary of FileService changesThe changes made to the
FileServiceclass consistently update AWS S3 operations to use object-based parameters, aligning with AWS SDK v3 standards. This transition enhances code readability, flexibility, and maintainability.Key improvements:
- Consistent use of object parameters for bucket and key across all AWS service method calls.
- Enhanced reusability of the code, making it more adaptable to different AWS configurations.
Suggestions for further improvement:
- Introduce interfaces or types for commonly used parameter objects (e.g.,
S3Location).- Consider using enums for string literals to improve type safety.
- Standardize the parameter structure across all AWS service method calls for even greater consistency.
Overall, these changes represent a significant step towards modernizing the codebase and improving its maintainability.
libs/application/api/files/src/lib/file.service.spec.ts (7)
8-8: LGTM: Import statement updated correctly.The import statement has been updated to include both
AwsModuleandAwsService. This change aligns with the transition to AWS SDK v3 and adheres to the coding guideline for reusability of components across different NextJS apps.
Line range hint
118-128: LGTM: Test module setup updated correctly.The test module setup has been updated to use
AwsModulein the imports array instead of providingAwsServicedirectly. This change enhances modularity and reusability, aligning with the coding guidelines for thelibsdirectory.
178-189: LGTM: Mock calls updated to use object-based parameters.The
uploadFileandgetPresignedUrlmock calls have been updated to use an object-based parameter structure. This change improves code clarity and consistency, aligning with the transition to AWS SDK v3.
201-203: LGTM: getFileContent mock call updated consistently.The
getFileContentmock call has been updated to use an object-based parameter structure, consistent with the other AWS service method updates in this file. This change enhances code clarity and maintains consistency across the test suite.
274-277: LGTM: getPresignedUrl mock call updated consistently.The
getPresignedUrlmock call has been updated to use an object-based parameter structure, maintaining consistency with the other AWS service method updates in this file. This change enhances code clarity and ensures a uniform approach across the test suite.
227-228: LGTM: getFileContent mock implementation updated correctly.The mock implementation for
getFileContenthas been updated, consistent with the method name change observed earlier in the file.To ensure the implementation is consistent across the codebase, please run the following script:
#!/bin/bash # Description: Verify the implementation of 'getFileContent' across the codebase # Test: Search for the implementation of 'getFileContent'. Expect: Consistent implementation. rg --type typescript -A 5 'getFileContent\s*\('
134-135: LGTM: Mock updated to reflect new method name.The mock has been correctly updated from
getFiletogetFileContent, reflecting the changes in the AWS SDK v3 implementation.To ensure consistency across the codebase, please run the following script to verify the method name change:
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.spec.ts (1)
45-45: LGTM: AwsService added to test providersThe addition of AwsService to the test module providers aligns with the PR objective of implementing AWS SDK v3. This change prepares the testing environment for potential AWS-related tests in the future.
However, to ensure comprehensive test coverage:
- Consider adding specific tests for AWS SDK v3 functionality if applicable to this service.
- Verify that existing tests still pass with this new provider.
To ensure no regressions were introduced, please run the following command:
Also applies to: 243-246
package.json (1)
72-72: LGTM: AWS SDK v3 dependencies added successfully.The addition of AWS SDK v3 dependencies for S3 operations aligns well with the PR objective. The consistent versioning (3.662.0) across these packages is good for compatibility.
Could you please verify if the AWS SDK v2 dependency ("aws-sdk": "2.814.0") is still needed? If it's part of a gradual migration strategy, consider updating the project documentation to reflect the use of both SDK versions to avoid confusion for other developers.
Also applies to: 75-76, 148-148
libs/nest/aws/src/lib/aws.service.ts (1)
1-12: Positive impact on bundling by using AWS SDK v3's modular importsBy updating to AWS SDK v3 and utilizing modular imports, the codebase benefits from improved tree-shaking and reduced bundle sizes. This aligns with effective bundling practices outlined in the coding guidelines.
libs/nest/aws/src/lib/aws.service.spec.ts (2)
21-163: Comprehensive test coverage and adherence to coding guidelinesThe test suite provides comprehensive coverage of the
AwsServicefunctionalities, adhering to the coding guidelines for TypeScript usage and code reusability.Great job on implementing thorough tests that ensure the reliability of the AWS SDK v3 integration.
41-42: Ensure mocks are properly resetIn the
beforeEachblock, consider usings3Mock.resetHistory()ands3Mock.resetBehavior()for a more thorough reset of the mock state.Run the following script to confirm the available reset methods and their usage:
Ensure that you're using the most appropriate methods to reset the mock between tests.
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (3)
2-2: LGTM!The import statement for
AwsServiceis correctly added.
115-115: Constructor injection of AwsService is correctly implementedThe
AwsServiceis properly injected into theParentalLeaveServiceconstructor.
386-392: Ensure correct usage of awsService.getFileContentVerify that the
getFileContentmethod is being called with the correct parameters and that'base64'is the appropriate argument according to theAwsServicemethod signature.
...tion/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts
Show resolved
Hide resolved
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/shared.service.ts (1)
290-292: LGTM! Consider adding a type for the upload options.The change to use an object for specifying the bucket and key in the
uploadFilemethod call is a good improvement. It aligns with the goal of implementing AWS SDK v3 and makes the code more structured and readable.To further enhance type safety and code clarity, consider defining an interface for the upload options:
interface UploadOptions { bucket: string; key: string; }Then use it in the method call:
const uploadOptions: UploadOptions = { bucket: uploadBucket, key: s3key }; const url = await this.awsService.uploadFile(buffer, uploadOptions, uploadParameters);This will make the code more self-documenting and help prevent potential errors in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/system-e2e/src/tests/islandis/application-system/acceptance/parental-leave.spec.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/shared/shared.service.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/system-e2e/src/tests/islandis/application-system/acceptance/parental-leave.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/template-api-modules/src/lib/modules/shared/shared.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."
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)
28-41: LGTM: ImprovedgetFileand newgetFileContentmethodThe
getFilemethod has been updated to use a more flexibleBucketKeyPairOrFilenameparameter, and a newgetFileContentmethod has been added with encoding support. These changes improve the API's usability and flexibility.Consider adding a comment explaining the purpose of the
getFileContentmethod and when to use it versusgetFile.
44-76: LGTM: ImproveduploadFilemethod with better error handlingThe
uploadFilemethod has been updated to use the newUploadclass from AWS SDK v3, which allows for better handling of large files and provides progress tracking capabilities. The error handling and logging have also been improved.Consider adding a comment explaining the benefits of using the
Uploadclass, such as its ability to handle large files and track progress.
79-94: LGTM: UpdatedgetPresignedUrlmethod with flexible expirationThe
getPresignedUrlmethod has been updated to use the newgetSignedUrlfunction from AWS SDK v3 and now includes anexpirationOverrideparameter for flexibility.Consider reducing the default expiration time from 120 minutes to a shorter duration (e.g., 15 minutes) to enhance security. Long-lived presigned URLs can pose a security risk if intercepted.
97-117: LGTM: UpdatedfileExistsmethod with improved error handlingThe
fileExistsmethod has been updated to use theHeadObjectCommandfrom AWS SDK v3 and includes improved error handling.Consider differentiating between "file not found" errors and other types of errors. Currently, the method returns
falsefor all error cases, which might mask unexpected issues. You could throw an error for unexpected cases while returningfalseonly when the file is confirmed not to exist.
143-173: LGTM: Useful utility methodsgetBucketKeyandgetFileResponseThe new
getBucketKeyandgetFileResponsemethods improve code reusability and error handling across the service. ThegetBucketKeymethod correctly handles both object and string inputs, enhancing flexibility.Consider making
getBucketKeya private method since it's an internal utility and doesn't need to be exposed in the public API.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
- libs/nest/aws/src/lib/aws.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (3)
libs/nest/aws/src/lib/aws.service.ts (3)
1-19: LGTM: Updated imports and new interfacesThe import statements have been correctly updated to use AWS SDK v3, and the new
BucketKeyPairinterface andEncodingStringtype are well-defined. These changes align with the transition to the new SDK version and improve type safety in the service.
23-26: LGTM: Updated constructor for AWS SDK v3The constructor has been correctly updated to inject
S3Clientinstead ofS3, which is necessary for the transition to AWS SDK v3. The injection is properly done using the@Injectdecorator.
119-141: LGTM: Well-implementeddeleteObjectmethodThe new
deleteObjectmethod is well-implemented using theDeleteObjectCommandfrom AWS SDK v3. It includes proper error handling, logging, and correctly checks for both 200 and 204 status codes as successful responses.
HjorturJ
added
automerge
…/island.is into chore/aws-v3-s3-implementation
...
https://app.asana.com/0/1207402031871459/1208590659495997/f
What
Implement s3 aws sdk v3 without breaking anything
Why
because aws sdk v2 will be depricated in 2025
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation