Feat: Delete versioned S3 objects #54
Conversation
|
Warning Rate Limit Exceeded@nao1215 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 4 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe project implemented enhancements to handle S3 object version listing within its infrastructure. It involved adding a new dependency, updating domain models and services, and extending functionality in the external interface and interactor to support object version management. This update allows for more granular control over S3 objects, including version listing and deletion based on specific versions, thereby improving data management and operational capabilities. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (8)
- app/di/wire.go (1 hunks)
- app/di/wire_gen.go (1 hunks)
- app/domain/model/s3.go (1 hunks)
- app/domain/service/s3.go (1 hunks)
- app/external/mock/s3.go (1 hunks)
- app/external/s3.go (1 hunks)
- app/interactor/s3.go (3 hunks)
- app/interactor/s3_test.go (4 hunks)
Additional comments: 11
app/di/wire.go (1)
- 51-51: Change approved: Correctly added
external.S3ObjectVersionsListerSetto the dependency injection setup inNewS3App.app/external/mock/s3.go (1)
- 97-103: Change approved: Correctly added mock type
S3ObjectVersionsListerand its methodListS3ObjectVersionsfor unit testing.app/domain/service/s3.go (1)
- 179-194: Change approved: Correctly added interfaces and structures for
S3ObjectVersionsListerdomain service.app/di/wire_gen.go (1)
- 36-37: Change approved: Correctly instantiated
s3ObjectVersionsListerand passed it tointeractor.NewS3ObjectsDeleter.app/interactor/s3.go (3)
- 144-144: Change approved: Correctly added
service.S3ObjectVersionsListerfield toS3ObjectsDeleterstruct.- 158-166: Change approved: Correctly updated
NewS3ObjectsDeleterfunction signature to includeservice.S3ObjectVersionsLister.- 183-217: Change approved: Correctly implemented logic in
DeleteS3Objectsmethod for listing object versions and deleting objects based on those versions.app/domain/model/s3.go (1)
- 406-406: The
Joinmethod implementation inS3Keystruct is correct and follows best practices for string manipulation in Go.app/external/s3.go (1)
- 417-471: The implementation of
S3ObjectVersionsListerand its methodListS3ObjectVersionscorrectly adheres to the interface contract for listing S3 object versions. The method efficiently handles pagination with AWS SDK'sListObjectVersionscall, ensuring all versions and delete markers are captured. The use ofMaxS3Keysconstant for limiting the number of keys per request is a good practice for managing performance and avoiding excessive data retrieval in a single call.app/interactor/s3_test.go (2)
- 457-457: This test case does not include the
s3ObjectVersionListerparameter. If this is intentional (e.g., testing behavior when the version lister is not available), it's acceptable. Otherwise, consider adding it to maintain consistency across test cases.- 482-482: The absence of
s3ObjectVersionListerin this test case appears intentional, focusing on testing with minimal setup. No action needed unless the test's scope is intended to cover versioned object deletion logic.
app/interactor/s3_test.go
Outdated
| s3ObjectVersionLister := mock.S3ObjectVersionsLister(func(ctx context.Context, input *service.S3ObjectVersionsListerInput) (*service.S3ObjectVersionsListerOutput, error) { | ||
| return &service.S3ObjectVersionsListerOutput{ | ||
| Objects: model.S3ObjectIdentifiers{}, | ||
| }, nil | ||
| }) | ||
|
|
||
| s3ObjectsDeleter := NewS3ObjectsDeleter(s3ObjectsDeleterMock, s3BucketLocationGetter, s3ObjectVersionLister) |
There was a problem hiding this comment.
Ensure the s3ObjectVersionLister mock is correctly set up to return a meaningful response for testing the DeleteS3Objects functionality, even if it's an empty list in this case. Consider adding test cases or modifying the mock to return different sets of object versions to more thoroughly test the handling of versioned objects.
app/interactor/s3_test.go
Outdated
| s3ObjectVersionLister := mock.S3ObjectVersionsLister(func(ctx context.Context, input *service.S3ObjectVersionsListerInput) (*service.S3ObjectVersionsListerOutput, error) { | ||
| return &service.S3ObjectVersionsListerOutput{ | ||
| Objects: model.S3ObjectIdentifiers{}, | ||
| }, nil | ||
| }) | ||
|
|
||
| s3ObjectsDeleter := NewS3ObjectsDeleter(s3ObjectsDeleterMock, s3BucketLocationGetter, s3ObjectVersionLister) |
There was a problem hiding this comment.
Similar to the previous comment, verify that the s3ObjectVersionLister mock setup is appropriate for the test scenario. If testing error handling or specific logic related to object versions, adjust the mock's return values accordingly.
HOTTEST report
Reported by hottest |
Code Metrics Report
Details | | main (a105246) | #54 (cec711b) | +/- |
|---------------------|----------------|---------------|-------|
+ | Coverage | 24.6% | 24.8% | +0.2% |
| Files | 42 | 42 | 0 |
| Lines | 1619 | 1649 | +30 |
+ | Covered | 398 | 409 | +11 |
+ | Test Execution Time | 12s | 6s | -6s |Code coverage of files in pull request scope (69.1% → 66.5%)
Reported by octocov |
Summary by CodeRabbit